Use No-Op Ancestry Checker (paritytech#755)

* Use no-op ancestry checker

* Check that current header height is greater than last finalized

* Ensure that incoming headers are strictly greater than last finalized

* Ensure that header numbers always increase in tests

bridges/bin/millau/runtime/src/lib.rs

@@ -313,8 +313,8 @@ parameter_types! {
 impl pallet_finality_verifier::Config for Runtime {
 	type BridgedChain = bp_rialto::Rialto;
 	type HeaderChain = pallet_substrate_bridge::Module<Runtime>;
-	type AncestryProof = Vec<bp_rialto::Header>;
-	type AncestryChecker = bp_header_chain::LinearAncestryChecker;
+	type AncestryProof = ();
+	type AncestryChecker = ();
 	type MaxRequests = MaxRequests;
 }
 

bridges/bin/rialto/runtime/src/lib.rs

@@ -420,8 +420,8 @@ parameter_types! {
 impl pallet_finality_verifier::Config for Runtime {
 	type BridgedChain = bp_millau::Millau;
 	type HeaderChain = pallet_substrate_bridge::Module<Runtime>;
-	type AncestryProof = Vec<bp_millau::Header>;
-	type AncestryChecker = bp_header_chain::LinearAncestryChecker;
+	type AncestryProof = ();
+	type AncestryChecker = ();
 	type MaxRequests = MaxRequests;
 }
 

bridges/modules/finality-verifier/src/lib.rs

@@ -142,7 +142,7 @@ pub mod pallet {
 				<Error<T>>::InvalidAncestryProof
 			);
 
-			T::HeaderChain::append_header(finality_target);
+			let _ = T::HeaderChain::append_header(finality_target)?;
 			frame_support::debug::info!("Succesfully imported finalized header with hash {:?}!", hash);
 
 			<RequestCount<T>>::mutate(|count| *count += 1);
@@ -203,9 +203,9 @@ mod tests {
 		));
 	}
 
-	fn submit_finality_proof() -> frame_support::dispatch::DispatchResultWithPostInfo {
-		let child = test_header(1);
-		let header = test_header(2);
+	fn submit_finality_proof(child: u8, header: u8) -> frame_support::dispatch::DispatchResultWithPostInfo {
+		let child = test_header(child.into());
+		let header = test_header(header.into());
 
 		let set_id = 1;
 		let grandpa_round = 1;
@@ -228,7 +228,7 @@ mod tests {
 		run_test(|| {
 			initialize_substrate_bridge();
 
-			assert_ok!(submit_finality_proof());
+			assert_ok!(submit_finality_proof(1, 2));
 
 			let header = test_header(2);
 			assert_eq!(
@@ -337,9 +337,9 @@ mod tests {
 	fn disallows_imports_once_limit_is_hit_in_single_block() {
 		run_test(|| {
 			initialize_substrate_bridge();
-			assert_ok!(submit_finality_proof());
-			assert_ok!(submit_finality_proof());
-			assert_err!(submit_finality_proof(), <Error<TestRuntime>>::TooManyRequests);
+			assert_ok!(submit_finality_proof(1, 2));
+			assert_ok!(submit_finality_proof(3, 4));
+			assert_err!(submit_finality_proof(5, 6), <Error<TestRuntime>>::TooManyRequests);
 		})
 	}
 
@@ -379,40 +379,40 @@ mod tests {
 	fn allows_request_after_new_block_has_started() {
 		run_test(|| {
 			initialize_substrate_bridge();
-			assert_ok!(submit_finality_proof());
-			assert_ok!(submit_finality_proof());
+			assert_ok!(submit_finality_proof(1, 2));
+			assert_ok!(submit_finality_proof(3, 4));
 
 			next_block();
-			assert_ok!(submit_finality_proof());
+			assert_ok!(submit_finality_proof(5, 6));
 		})
 	}
 
 	#[test]
 	fn disallows_imports_once_limit_is_hit_across_different_blocks() {
 		run_test(|| {
 			initialize_substrate_bridge();
-			assert_ok!(submit_finality_proof());
-			assert_ok!(submit_finality_proof());
+			assert_ok!(submit_finality_proof(1, 2));
+			assert_ok!(submit_finality_proof(3, 4));
 
 			next_block();
-			assert_ok!(submit_finality_proof());
-			assert_err!(submit_finality_proof(), <Error<TestRuntime>>::TooManyRequests);
+			assert_ok!(submit_finality_proof(5, 6));
+			assert_err!(submit_finality_proof(7, 8), <Error<TestRuntime>>::TooManyRequests);
 		})
 	}
 
 	#[test]
 	fn allows_max_requests_after_long_time_with_no_activity() {
 		run_test(|| {
 			initialize_substrate_bridge();
-			assert_ok!(submit_finality_proof());
-			assert_ok!(submit_finality_proof());
+			assert_ok!(submit_finality_proof(1, 2));
+			assert_ok!(submit_finality_proof(3, 4));
 
 			next_block();
 			next_block();
 
 			next_block();
-			assert_ok!(submit_finality_proof());
-			assert_ok!(submit_finality_proof());
+			assert_ok!(submit_finality_proof(5, 6));
+			assert_ok!(submit_finality_proof(7, 8));
 		})
 	}
 }

bridges/modules/substrate/src/lib.rs

@@ -155,6 +155,11 @@ decl_error! {
 		AlreadyInitialized,
 		/// The given header is not a descendant of a particular header.
 		NotDescendant,
+		/// The header being imported is on a fork which is incompatible with the current chain.
+		///
+		/// This can happen if we try and import a finalized header at a lower height than our
+		/// current `best_finalized` header.
+		ConflictingFork,
 	}
 }
 
@@ -375,8 +380,14 @@ impl<T: Config> bp_header_chain::HeaderChain<BridgedHeader<T>, sp_runtime::Dispa
 		PalletStorage::<T>::new().current_authority_set()
 	}
 
-	fn append_header(header: BridgedHeader<T>) {
+	fn append_header(header: BridgedHeader<T>) -> Result<(), sp_runtime::DispatchError> {
+		// We do a quick check here to ensure that our header chain is making progress and isn't
+		// "travelling back in time" (which would be indicative of something bad, e.g a hard-fork).
+		let best_finalized = PalletStorage::<T>::new().best_finalized_header().header;
+		ensure!(best_finalized.number() < header.number(), <Error<T>>::ConflictingFork);
 		import_header_unchecked::<_, T>(&mut PalletStorage::<T>::new(), header);
+
+		Ok(())
 	}
 }
 
@@ -714,7 +725,7 @@ mod tests {
 	use crate::mock::{run_test, test_header, unfinalized_header, Origin, TestHeader, TestRuntime};
 	use bp_header_chain::HeaderChain;
 	use bp_test_utils::{alice, authority_list, bob};
-	use frame_support::{assert_noop, assert_ok};
+	use frame_support::{assert_err, assert_noop, assert_ok};
 	use sp_runtime::DispatchError;
 
 	fn init_with_origin(origin: Origin) -> Result<InitializationData<TestHeader>, DispatchError> {
@@ -907,14 +918,33 @@ mod tests {
 			let storage = PalletStorage::<TestRuntime>::new();
 
 			let header = test_header(2);
-			Module::<TestRuntime>::append_header(header.clone());
+			assert_ok!(Module::<TestRuntime>::append_header(header.clone()));
 
 			assert!(storage.header_by_hash(header.hash()).unwrap().is_finalized);
 			assert_eq!(storage.best_finalized_header().header, header);
 			assert_eq!(storage.best_headers()[0].hash, header.hash());
 		})
 	}
 
+	#[test]
+	fn importing_unchecked_header_ensures_that_chain_is_extended() {
+		run_test(|| {
+			init_with_origin(Origin::root()).unwrap();
+
+			let header = test_header(3);
+			assert_ok!(Module::<TestRuntime>::append_header(header));
+
+			let header = test_header(2);
+			assert_err!(
+				Module::<TestRuntime>::append_header(header),
+				Error::<TestRuntime>::ConflictingFork,
+			);
+
+			let header = test_header(4);
+			assert_ok!(Module::<TestRuntime>::append_header(header));
+		})
+	}
+
 	#[test]
 	fn importing_unchecked_headers_enacts_new_authority_set() {
 		run_test(|| {
@@ -930,7 +960,7 @@ mod tests {
 			header.digest = fork_tests::change_log(0);
 
 			// Let's import our test header
-			Module::<TestRuntime>::append_header(header.clone());
+			assert_ok!(Module::<TestRuntime>::append_header(header.clone()));
 
 			// Make sure that our header is the best finalized
 			assert_eq!(storage.best_finalized_header().header, header);
@@ -960,8 +990,8 @@ mod tests {
 			let header = test_header(3);
 
 			// Let's import our test headers
-			Module::<TestRuntime>::append_header(schedules_change);
-			Module::<TestRuntime>::append_header(header.clone());
+			assert_ok!(Module::<TestRuntime>::append_header(schedules_change));
+			assert_ok!(Module::<TestRuntime>::append_header(header.clone()));
 
 			// Make sure that our header is the best finalized
 			assert_eq!(storage.best_finalized_header().header, header);
@@ -1001,7 +1031,7 @@ mod tests {
 
 			// We are expecting an authority set change at height 2, so this header should enact
 			// that upon being imported.
-			Module::<TestRuntime>::append_header(test_header(2));
+			assert_ok!(Module::<TestRuntime>::append_header(test_header(2)));
 
 			// Make sure that the authority set actually changed upon importing our header
 			assert_eq!(

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

@@ -78,7 +78,7 @@ pub trait HeaderChain<H, E> {
 	fn authority_set() -> AuthoritySet;
 
 	/// Write a header finalized by GRANDPA to the underlying pallet storage.
-	fn append_header(header: H);
+	fn append_header(header: H) -> Result<(), E>;
 }
 
 impl<H: Default, E> HeaderChain<H, E> for () {
@@ -90,7 +90,9 @@ impl<H: Default, E> HeaderChain<H, E> for () {
 		AuthoritySet::default()
 	}
 
-	fn append_header(_header: H) {}
+	fn append_header(_header: H) -> Result<(), E> {
+		Ok(())
+	}
 }
 
 /// A trait for checking if a given child header is a direct descendant of an ancestor.