From 658e4e9b5c9ca6020155bd13ae7c1d342a312050 Mon Sep 17 00:00:00 2001
From: Hernando Castano <HCastano@users.noreply.github.com>
Date: Fri, 26 Feb 2021 12:57:06 -0500
Subject: [PATCH] Use No-Op Ancestry Checker (#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 | 4 +-
bridges/bin/rialto/runtime/src/lib.rs | 4 +-
bridges/modules/finality-verifier/src/lib.rs | 38 ++++++++---------
bridges/modules/substrate/src/lib.rs | 44 ++++++++++++++++----
bridges/primitives/header-chain/src/lib.rs | 6 ++-
5 files changed, 64 insertions(+), 32 deletions(-)
diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs
index bee7881f26d..89440184002 100644
--- a/bridges/bin/millau/runtime/src/lib.rs
+++ b/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;
}
diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs
index af1d7c6c9fa..6bb73ef037a 100644
--- a/bridges/bin/rialto/runtime/src/lib.rs
+++ b/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;
}
diff --git a/bridges/modules/finality-verifier/src/lib.rs b/bridges/modules/finality-verifier/src/lib.rs
index 4e05c5754d9..8f3505252ec 100644
--- a/bridges/modules/finality-verifier/src/lib.rs
+++ b/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,11 +379,11 @@ 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));
})
}
@@ -391,12 +391,12 @@ mod tests {
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);
})
}
@@ -404,15 +404,15 @@ mod tests {
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));
})
}
}
diff --git a/bridges/modules/substrate/src/lib.rs b/bridges/modules/substrate/src/lib.rs
index c14db8596f8..1ae23fdf91b 100644
--- a/bridges/modules/substrate/src/lib.rs
+++ b/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,7 +918,7 @@ 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);
@@ -915,6 +926,25 @@ mod tests {
})
}
+ #[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!(
diff --git a/bridges/primitives/header-chain/src/lib.rs b/bridges/primitives/header-chain/src/lib.rs
index 16637176460..a2d8574b6dc 100644
--- a/bridges/primitives/header-chain/src/lib.rs
+++ b/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.
--
GitLab