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.