Store both block number and hash in best finalized storage value (#1475)

* store both block number and hash in BestFinalized

* also fix relay code

* spelling

bridges/modules/grandpa/src/benchmarking.rs

@@ -115,7 +115,7 @@ benchmarks_instance_pallet! {
 		let header: BridgedHeader<T, I> = bp_test_utils::test_header(header_number::<T, I, _>());
 		let expected_hash = header.hash();
 
-		assert_eq!(<BestFinalized<T, I>>::get(), expected_hash);
+		assert_eq!(<BestFinalized<T, I>>::get().unwrap().1, expected_hash);
 		assert!(<ImportedHeaders<T, I>>::contains_key(expected_hash));
 	}
 }

bridges/modules/grandpa/src/extension.rs

@@ -66,12 +66,9 @@ macro_rules! declare_bridge_reject_obsolete_grandpa_header {
 
 							let bundled_block_number = *finality_target.number();
 
-							let best_finalized_hash = $crate::BestFinalized::<$runtime, $instance>::get();
+							let best_finalized = $crate::BestFinalized::<$runtime, $instance>::get();
-							let best_finalized_number = match $crate::ImportedHeaders::<
+							let best_finalized_number = match best_finalized {
-								$runtime,
+								Some((best_finalized_number, _)) => best_finalized_number,
-								$instance,
-							>::get(best_finalized_hash) {
-								Some(best_finalized_header) => *best_finalized_header.number(),
 								None => return sp_runtime::transaction_validity::InvalidTransaction::Call.into(),
 							};
 
@@ -113,7 +110,7 @@ macro_rules! declare_bridge_reject_obsolete_grandpa_header {
 mod tests {
 	use crate::{
 		mock::{run_test, test_header, Call, TestNumber, TestRuntime},
-		BestFinalized, ImportedHeaders,
+		BestFinalized,
 	};
 	use bp_test_utils::make_default_justification;
 	use frame_support::weights::{DispatchClass, DispatchInfo, Pays};
@@ -140,8 +137,7 @@ mod tests {
 
 	fn sync_to_header_10() {
 		let header10_hash = sp_core::H256::default();
-		BestFinalized::<TestRuntime, ()>::put(header10_hash);
+		BestFinalized::<TestRuntime, ()>::put((10, header10_hash));
-		ImportedHeaders::<TestRuntime, ()>::insert(header10_hash, test_header(10));
 	}
 
 	#[test]

bridges/modules/grandpa/src/lib.rs

@@ -143,7 +143,10 @@ pub mod pallet {
 			let (hash, number) = (finality_target.hash(), finality_target.number());
 			log::trace!(target: "runtime::bridge-grandpa", "Going to try and finalize header {:?}", finality_target);
 
-			let best_finalized = match <ImportedHeaders<T, I>>::get(<BestFinalized<T, I>>::get()) {
+			let best_finalized = BestFinalized::<T, I>::get();
+			let best_finalized =
+				best_finalized.and_then(|(_, hash)| ImportedHeaders::<T, I>::get(hash));
+			let best_finalized = match best_finalized {
 				Some(best_finalized) => best_finalized,
 				None => {
 					log::error!(
@@ -273,7 +276,7 @@ pub mod pallet {
 	/// Hash of the best finalized header.
 	#[pallet::storage]
 	pub type BestFinalized<T: Config<I>, I: 'static = ()> =
-		StorageValue<_, BridgedBlockHash<T, I>, ValueQuery>;
+		StorageValue<_, (BridgedBlockNumber<T, I>, BridgedBlockHash<T, I>), OptionQuery>;
 
 	/// A ring buffer of imported hashes. Ordered by the insertion time.
 	#[pallet::storage]
@@ -458,7 +461,7 @@ pub mod pallet {
 	) {
 		let index = <ImportedHashesPointer<T, I>>::get();
 		let pruning = <ImportedHashes<T, I>>::try_get(index);
-		<BestFinalized<T, I>>::put(hash);
+		<BestFinalized<T, I>>::put((*header.number(), hash));
 		<ImportedHeaders<T, I>>::insert(hash, header);
 		<ImportedHashes<T, I>>::insert(index, hash);
 
@@ -538,7 +541,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// Returns a dummy header if there is no best header. This can only happen
 	/// if the pallet has not been initialized yet.
 	pub fn best_finalized() -> Option<BridgedHeader<T, I>> {
-		let hash = <BestFinalized<T, I>>::get();
+		let (_, hash) = <BestFinalized<T, I>>::get()?;
 		<ImportedHeaders<T, I>>::get(hash)
 	}
 
@@ -706,16 +709,13 @@ mod tests {
 	#[test]
 	fn init_storage_entries_are_correctly_initialized() {
 		run_test(|| {
-			assert_eq!(
+			assert_eq!(BestFinalized::<TestRuntime>::get(), None,);
-				BestFinalized::<TestRuntime>::get(),
-				BridgedBlockHash::<TestRuntime, ()>::default()
-			);
 			assert_eq!(Pallet::<TestRuntime>::best_finalized(), None);
 
 			let init_data = init_with_origin(Origin::root()).unwrap();
 
 			assert!(<ImportedHeaders<TestRuntime>>::contains_key(init_data.header.hash()));
-			assert_eq!(BestFinalized::<TestRuntime>::get(), init_data.header.hash());
+			assert_eq!(BestFinalized::<TestRuntime>::get().unwrap().1, init_data.header.hash());
 			assert_eq!(
 				CurrentAuthoritySet::<TestRuntime>::get().authorities,
 				init_data.authority_list
@@ -826,7 +826,7 @@ mod tests {
 			);
 
 			let header = test_header(1);
-			assert_eq!(<BestFinalized<TestRuntime>>::get(), header.hash());
+			assert_eq!(<BestFinalized<TestRuntime>>::get().unwrap().1, header.hash());
 			assert!(<ImportedHeaders<TestRuntime>>::contains_key(header.hash()));
 		})
 	}
@@ -943,7 +943,7 @@ mod tests {
 			);
 
 			// Make sure that our header is the best finalized
-			assert_eq!(<BestFinalized<TestRuntime>>::get(), header.hash());
+			assert_eq!(<BestFinalized<TestRuntime>>::get().unwrap().1, header.hash());
 			assert!(<ImportedHeaders<TestRuntime>>::contains_key(header.hash()));
 
 			// Make sure that the authority set actually changed upon importing our header
@@ -1027,7 +1027,7 @@ mod tests {
 			header.set_state_root(state_root);
 
 			let hash = header.hash();
-			<BestFinalized<TestRuntime>>::put(hash);
+			<BestFinalized<TestRuntime>>::put((2, hash));
 			<ImportedHeaders<TestRuntime>>::insert(hash, header);
 
 			assert_ok!(
@@ -1153,7 +1153,7 @@ mod tests {
 
 		assert_eq!(
 			BestFinalized::<TestRuntime>::storage_value_final_key().to_vec(),
-			bp_header_chain::storage_keys::best_finalized_hash_key("Grandpa").0,
+			bp_header_chain::storage_keys::best_finalized_key("Grandpa").0,
 		);
 	}
 }

bridges/primitives/header-chain/src/storage_keys.rs

@@ -34,8 +34,8 @@ pub fn is_halted_key(pallet_prefix: &str) -> StorageKey {
 	)
 }
 
-/// Storage key of the best finalized header hash value in the runtime storage.
+/// Storage key of the best finalized header number and hash value in the runtime storage.
-pub fn best_finalized_hash_key(pallet_prefix: &str) -> StorageKey {
+pub fn best_finalized_key(pallet_prefix: &str) -> StorageKey {
 	StorageKey(
 		bp_runtime::storage_value_final_key(
 			pallet_prefix.as_bytes(),
@@ -64,10 +64,10 @@ mod tests {
 	}
 
 	#[test]
-	fn best_finalized_hash_key_computed_properly() {
+	fn best_finalized_key_computed_properly() {
 		// If this test fails, then something has been changed in module storage that is breaking
 		// compatibility with previous pallet.
-		let storage_key = best_finalized_hash_key("BridgeGrandpa").0;
+		let storage_key = best_finalized_key("BridgeGrandpa").0;
 		assert_eq!(
 			storage_key,
 			hex!("0b06f475eddb98cf933a12262e0388dea4ebafdd473c549fdb24c5c991c5591c").to_vec(),

bridges/relays/lib-substrate-relay/src/finality/engine.rs

@@ -118,7 +118,7 @@ impl<C: ChainWithGrandpa> Engine<C> for Grandpa<C> {
 	}
 
 	fn is_initialized_key() -> StorageKey {
-		bp_header_chain::storage_keys::best_finalized_hash_key(C::WITH_CHAIN_GRANDPA_PALLET_NAME)
+		bp_header_chain::storage_keys::best_finalized_key(C::WITH_CHAIN_GRANDPA_PALLET_NAME)
 	}
 
 	async fn finality_proofs(client: Client<C>) -> Result<Subscription<Bytes>, SubstrateError> {