From fb7c191234d5acd2e2f588fceaad26bbf6947aba Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 10 Feb 2021 05:14:58 -0500 Subject: [PATCH] Stop Importing Full Header Chain (#707) * Make AncestryProof type more flexible * Only import single finalized header instead of a chain * Fix unchecked header import tests * Add option for limiting ancestry proof size * Update finality verifier Config in runtimes * Update some documentation * Fix Clippy warning * Allow AncestryChecker to return proof size Stops us from abusing the `Size` trait * Remove Size impl for Vec * Remove size contraints for ancestry proofs With different proof types its unclear how to "size" should be interpreted, so we remove this requirement all together to avoid confusion. --- bridges/bin/millau/runtime/src/lib.rs | 8 +- bridges/bin/rialto/runtime/src/lib.rs | 8 +- bridges/modules/finality-verifier/Cargo.toml | 2 - bridges/modules/finality-verifier/src/lib.rs | 89 +++++-------------- bridges/modules/finality-verifier/src/mock.rs | 6 +- bridges/modules/substrate/src/lib.rs | 55 ++---------- bridges/primitives/header-chain/src/lib.rs | 16 +--- 7 files changed, 38 insertions(+), 146 deletions(-) diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index 3bf9309dfb..65cd08970f 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -300,17 +300,11 @@ impl pallet_substrate_bridge::Config for Runtime { type BridgedChain = bp_rialto::Rialto; } -parameter_types! { - // We'll use the length of a session on the bridged chain as our bound since GRANDPA is - // guaranteed to produce a justification every session. - pub const MaxHeadersInSingleProof: bp_rialto::BlockNumber = bp_rialto::SESSION_LENGTH; -} - impl pallet_finality_verifier::Config for Runtime { type BridgedChain = bp_rialto::Rialto; type HeaderChain = pallet_substrate_bridge::Module; + type AncestryProof = Vec; type AncestryChecker = bp_header_chain::LinearAncestryChecker; - type MaxHeadersInSingleProof = MaxHeadersInSingleProof; } impl pallet_shift_session_manager::Config for Runtime {} diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index f7717b1c76..3ebb487fe9 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -407,17 +407,11 @@ impl pallet_substrate_bridge::Config for Runtime { type BridgedChain = bp_millau::Millau; } -parameter_types! { - // We'll use the length of a session on the bridged chain as our bound since GRANDPA is - // guaranteed to produce a justification every session. - pub const MaxHeadersInSingleProof: bp_millau::BlockNumber = bp_millau::SESSION_LENGTH; -} - impl pallet_finality_verifier::Config for Runtime { type BridgedChain = bp_millau::Millau; type HeaderChain = pallet_substrate_bridge::Module; + type AncestryProof = Vec; type AncestryChecker = bp_header_chain::LinearAncestryChecker; - type MaxHeadersInSingleProof = MaxHeadersInSingleProof; } impl pallet_shift_session_manager::Config for Runtime {} diff --git a/bridges/modules/finality-verifier/Cargo.toml b/bridges/modules/finality-verifier/Cargo.toml index c1e09c7cf9..b98f995b61 100644 --- a/bridges/modules/finality-verifier/Cargo.toml +++ b/bridges/modules/finality-verifier/Cargo.toml @@ -11,7 +11,6 @@ license = "GPL-3.0-or-later WITH Classpath-exception-2.0" codec = { package = "parity-scale-codec", version = "1.3.1", default-features = false } finality-grandpa = { version = "0.12.3", default-features = false } serde = { version = "1.0", optional = true } -num-traits = { version = "0.2", default-features = false } # Bridge Dependencies @@ -40,7 +39,6 @@ std = [ "finality-grandpa/std", "frame-support/std", "frame-system/std", - "num-traits/std", "serde", "sp-runtime/std", "sp-std/std", diff --git a/bridges/modules/finality-verifier/src/lib.rs b/bridges/modules/finality-verifier/src/lib.rs index 4e0a3583a2..cfcb610e7f 100644 --- a/bridges/modules/finality-verifier/src/lib.rs +++ b/bridges/modules/finality-verifier/src/lib.rs @@ -35,9 +35,8 @@ use bp_header_chain::{justification::verify_justification, AncestryChecker, HeaderChain}; use bp_runtime::{Chain, HeaderOf}; use finality_grandpa::voter_set::VoterSet; -use frame_support::{dispatch::DispatchError, ensure, traits::Get}; +use frame_support::{dispatch::DispatchError, ensure}; use frame_system::ensure_signed; -use num_traits::AsPrimitive; use sp_runtime::traits::Header as HeaderT; use sp_std::vec::Vec; @@ -64,17 +63,15 @@ pub mod pallet { /// The pallet which we will use as our underlying storage mechanism. type HeaderChain: HeaderChain<::Header, DispatchError>; + /// The type of ancestry proof used by the pallet. + /// + /// Will be used by the ancestry checker to verify that the header being finalized is + /// related to the best finalized header in storage. + type AncestryProof: Parameter; + /// The type through which we will verify that a given header is related to the last /// finalized header in our storage pallet. - type AncestryChecker: AncestryChecker< - ::Header, - Vec<::Header>, - >; - - /// The maximum length of headers we can have in a single ancestry proof. This prevents - /// unbounded iteration when verifying proofs. - #[pallet::constant] - type MaxHeadersInSingleProof: Get<::BlockNumber>; + type AncestryChecker: AncestryChecker<::Header, Self::AncestryProof>; } #[pallet::pallet] @@ -87,45 +84,33 @@ pub mod pallet { impl Pallet { /// Verify a target header is finalized according to the given finality proof. /// - /// Will use the underlying storage pallet to fetch information about the current + /// It will use the underlying storage pallet to fetch information about the current /// authorities and best finalized header in order to verify that the header is finalized. /// - /// If successful in verification, it will write the header as well as its ancestors (from - /// the given `ancestry_proof`) to the underlying storage pallet. - /// - /// Note that the expected format for `ancestry_proof` is a continguous list of finalized - /// headers containing (current_best_finalized_header, finality_target] + /// If successful in verification, it will write the target header to the underlying storage + /// pallet. #[pallet::weight(0)] pub fn submit_finality_proof( origin: OriginFor, finality_target: BridgedHeader, justification: Vec, - ancestry_proof: Vec>, + ancestry_proof: T::AncestryProof, ) -> DispatchResultWithPostInfo { let _ = ensure_signed(origin)?; frame_support::debug::trace!("Going to try and finalize header {:?}", finality_target); - frame_support::debug::trace!("Got ancestry proof of length {}", ancestry_proof.len()); - - ensure!( - ancestry_proof.len() <= T::MaxHeadersInSingleProof::get().as_(), - >::OversizedAncestryProof - ); let authority_set = T::HeaderChain::authority_set(); let voter_set = VoterSet::new(authority_set.authorities).ok_or(>::InvalidAuthoritySet)?; let set_id = authority_set.set_id; - verify_justification::>( - (finality_target.hash(), *finality_target.number()), - set_id, - voter_set, - &justification, - ) - .map_err(|e| { - frame_support::debug::error!("Received invalid justification for {:?}: {:?}", finality_target, e); - >::InvalidJustification - })?; + let (hash, number) = (finality_target.hash(), *finality_target.number()); + verify_justification::>((hash, number), set_id, voter_set, &justification).map_err( + |e| { + frame_support::debug::error!("Received invalid justification for {:?}: {:?}", finality_target, e); + >::InvalidJustification + }, + )?; let best_finalized = T::HeaderChain::best_finalized(); frame_support::debug::trace!("Checking ancestry against best finalized header: {:?}", &best_finalized); @@ -135,17 +120,8 @@ pub mod pallet { >::InvalidAncestryProof ); - // Note that this won't work if we ever change the `ancestry_proof` format to be - // sparse since this expects a contiguous set of finalized headers. - let _ = T::HeaderChain::append_finalized_chain(ancestry_proof).map_err(|_| { - frame_support::debug::error!("Failed to append finalized header chain."); - >::FailedToWriteHeader - })?; - - frame_support::debug::info!( - "Succesfully imported finalized header chain for target header {:?}!", - finality_target - ); + T::HeaderChain::append_header(finality_target); + frame_support::debug::info!("Succesfully imported finalized header with hash {:?}!", hash); Ok(().into()) } @@ -162,8 +138,6 @@ pub mod pallet { InvalidAuthoritySet, /// Failed to write a header to the underlying header chain. FailedToWriteHeader, - /// The given ancestry proof is too large to be verified in a single transaction. - OversizedAncestryProof, } } @@ -283,27 +257,6 @@ mod tests { }) } - #[test] - fn disallows_ancestry_proofs_which_are_too_large() { - run_test(|| { - initialize_substrate_bridge(); - - let header = test_header(1); - let justification = [1u8; 32].encode(); - - let mut ancestry_proof = vec![]; - let max_len = ::MaxHeadersInSingleProof::get(); - for i in 1..=max_len + 1 { - ancestry_proof.push(test_header(i as u64)); - } - - assert_err!( - Module::::submit_finality_proof(Origin::signed(1), header, justification, ancestry_proof,), - >::OversizedAncestryProof - ); - }) - } - #[test] fn disallows_invalid_authority_set() { run_test(|| { diff --git a/bridges/modules/finality-verifier/src/mock.rs b/bridges/modules/finality-verifier/src/mock.rs index daacc45841..1ef1249545 100644 --- a/bridges/modules/finality-verifier/src/mock.rs +++ b/bridges/modules/finality-verifier/src/mock.rs @@ -71,14 +71,14 @@ impl pallet_substrate_bridge::Config for TestRuntime { } parameter_types! { - pub const MaxHeadersInSingleProof: u8 = 5; + pub const MaxElementsInSingleProof: Option = Some(5); } impl crate::pallet::Config for TestRuntime { type BridgedChain = TestBridgedChain; type HeaderChain = pallet_substrate_bridge::Module; - type AncestryChecker = Checker<::Header, Vec<::Header>>; - type MaxHeadersInSingleProof = MaxHeadersInSingleProof; + type AncestryProof = Vec<::Header>; + type AncestryChecker = Checker<::Header, Self::AncestryProof>; } #[derive(Debug)] diff --git a/bridges/modules/substrate/src/lib.rs b/bridges/modules/substrate/src/lib.rs index 2a3ad1b018..c14db8596f 100644 --- a/bridges/modules/substrate/src/lib.rs +++ b/bridges/modules/substrate/src/lib.rs @@ -375,25 +375,8 @@ impl bp_header_chain::HeaderChain, sp_runtime::Dispa PalletStorage::::new().current_authority_set() } - fn append_finalized_chain( - headers: impl IntoIterator>, - ) -> Result<(), sp_runtime::DispatchError> { - let mut storage = PalletStorage::::new(); - - let mut header_iter = headers.into_iter().peekable(); - let first_header = header_iter.peek().ok_or(Error::::NotDescendant)?; - - // Quick ancestry check to make sure we're not writing complete nonsense to storage - ensure!( - >::get() == *first_header.parent_hash(), - Error::::NotDescendant, - ); - - for header in header_iter { - import_header_unchecked::<_, T>(&mut storage, header); - } - - Ok(()) + fn append_header(header: BridgedHeader) { + import_header_unchecked::<_, T>(&mut PalletStorage::::new(), header); } } @@ -923,36 +906,15 @@ mod tests { init_with_origin(Origin::root()).unwrap(); let storage = PalletStorage::::new(); - let child = test_header(2); - let header = test_header(3); + let header = test_header(2); + Module::::append_header(header.clone()); - let header_chain = vec![child.clone(), header.clone()]; - assert_ok!(Module::::append_finalized_chain(header_chain)); - - assert!(storage.header_by_hash(child.hash()).unwrap().is_finalized); 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 prevents_unchecked_header_import_if_headers_are_unrelated() { - run_test(|| { - init_with_origin(Origin::root()).unwrap(); - - // Pallet is expecting test_header(2) as the child - let not_a_child = test_header(3); - let header_chain = vec![not_a_child]; - - assert_noop!( - Module::::append_finalized_chain(header_chain), - Error::::NotDescendant, - ); - }) - } - #[test] fn importing_unchecked_headers_enacts_new_authority_set() { run_test(|| { @@ -968,7 +930,7 @@ mod tests { header.digest = fork_tests::change_log(0); // Let's import our test header - assert_ok!(Module::::append_finalized_chain(vec![header.clone()])); + Module::::append_header(header.clone()); // Make sure that our header is the best finalized assert_eq!(storage.best_finalized_header().header, header); @@ -998,8 +960,8 @@ mod tests { let header = test_header(3); // Let's import our test headers - let header_chain = vec![schedules_change, header.clone()]; - assert_ok!(Module::::append_finalized_chain(header_chain)); + Module::::append_header(schedules_change); + Module::::append_header(header.clone()); // Make sure that our header is the best finalized assert_eq!(storage.best_finalized_header().header, header); @@ -1039,8 +1001,7 @@ mod tests { // We are expecting an authority set change at height 2, so this header should enact // that upon being imported. - let header_chain = vec![test_header(2)]; - assert_ok!(Module::::append_finalized_chain(header_chain)); + Module::::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 a9623b13dc..1663717646 100644 --- a/bridges/primitives/header-chain/src/lib.rs +++ b/bridges/primitives/header-chain/src/lib.rs @@ -69,7 +69,7 @@ pub trait InclusionProofVerifier { fn verify_transaction_inclusion_proof(proof: &Self::TransactionInclusionProof) -> Option; } -/// A base trait for pallets which want to keep track of a full set of headers from a bridged chain. +/// A trait for pallets which want to keep track of finalized headers from a bridged chain. pub trait HeaderChain { /// Get the best finalized header known to the header chain. fn best_finalized() -> H; @@ -77,14 +77,8 @@ pub trait HeaderChain { /// Get the best authority set known to the header chain. fn authority_set() -> AuthoritySet; - /// Write a finalized chain of headers to the underlying pallet storage. - /// - /// It is assumed that each header in this chain been finalized, and that the given headers are - /// in order (e.g vec![header_1, header_2, ..., header_n]). - /// - /// This function should fail if the first header is not a child of the current best finalized - /// header known to the underlying pallet storage. - fn append_finalized_chain(headers: impl IntoIterator) -> Result<(), E>; + /// Write a header finalized by GRANDPA to the underlying pallet storage. + fn append_header(header: H); } impl HeaderChain for () { @@ -96,9 +90,7 @@ impl HeaderChain for () { AuthoritySet::default() } - fn append_finalized_chain(_headers: impl IntoIterator) -> Result<(), E> { - Ok(()) - } + fn append_header(_header: H) {} } /// A trait for checking if a given child header is a direct descendant of an ancestor.