diff --git a/bridges/modules/finality-verifier/src/lib.rs b/bridges/modules/finality-verifier/src/lib.rs index 170b03462e..924a1c0054 100644 --- a/bridges/modules/finality-verifier/src/lib.rs +++ b/bridges/modules/finality-verifier/src/lib.rs @@ -35,7 +35,7 @@ use bp_header_chain::{justification::verify_justification, AncestryChecker, HeaderChain}; use bp_runtime::{Chain, HeaderOf}; use finality_grandpa::voter_set::VoterSet; -use frame_support::{ensure, traits::Get}; +use frame_support::{dispatch::DispatchError, ensure, traits::Get}; use frame_system::ensure_signed; use sp_runtime::traits::Header as HeaderT; @@ -57,7 +57,7 @@ pub mod pallet { type BridgedChain: Chain; /// The pallet which we will use as our underlying storage mechanism. - type HeaderChain: HeaderChain<::Header>; + type HeaderChain: HeaderChain<::Header, DispatchError>; /// The type through which we will verify that a given header is related to the last /// finalized header in our storage pallet. @@ -80,13 +80,16 @@ pub mod pallet { #[pallet::call] impl Pallet { - /// Verify a header is finalized according to the given finality proof. + /// Verify a target header is finalized according to the given finality proof. /// /// 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 headers to the underlying storage - /// pallet as well as import the valid finality proof. + /// 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] #[pallet::weight(0)] pub fn submit_finality_proof( origin: OriginFor, @@ -119,22 +122,10 @@ pub mod pallet { >::InvalidAncestryProof ); - // If for whatever reason we are unable to fully import headers and the corresponding - // finality proof we want to avoid writing to the base pallet storage - use frame_support::storage::{with_transaction, TransactionOutcome}; - with_transaction(|| { - for header in ancestry_proof { - if T::HeaderChain::import_header(header).is_err() { - return TransactionOutcome::Rollback(Err(>::FailedToWriteHeader)); - } - } - - if T::HeaderChain::import_finality_proof(finality_target, justification).is_err() { - return TransactionOutcome::Rollback(Err(>::FailedToWriteFinalityProof)); - } - - TransactionOutcome::Commit(Ok(())) - })?; + // 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(|_| >::FailedToWriteHeader)?; Ok(().into()) } @@ -151,8 +142,6 @@ pub mod pallet { InvalidAuthoritySet, /// Failed to write a header to the underlying header chain. FailedToWriteHeader, - /// Failed to write finality proof to the underlying header chain. - FailedToWriteFinalityProof, /// The given ancestry proof is too large to be verified in a single transaction. OversizedAncestryProof, } diff --git a/bridges/modules/substrate/src/fork_tests.rs b/bridges/modules/substrate/src/fork_tests.rs index 810678852c..445ffd8ce5 100644 --- a/bridges/modules/substrate/src/fork_tests.rs +++ b/bridges/modules/substrate/src/fork_tests.rs @@ -503,7 +503,7 @@ where storage.update_current_authority_set(authority_set); } -fn change_log(delay: u64) -> Digest { +pub(crate) fn change_log(delay: u64) -> Digest { let consensus_log = ConsensusLog::::ScheduledChange(sp_finality_grandpa::ScheduledChange { next_authorities: vec![(alice(), 1), (bob(), 1)], delay, diff --git a/bridges/modules/substrate/src/lib.rs b/bridges/modules/substrate/src/lib.rs index 2ecee8628e..2a3ad1b018 100644 --- a/bridges/modules/substrate/src/lib.rs +++ b/bridges/modules/substrate/src/lib.rs @@ -83,6 +83,8 @@ pub trait Config: frame_system::Config { decl_storage! { trait Store for Module as SubstrateBridge { + /// Hash of the header used to bootstrap the pallet. + InitialHash: BridgedBlockHash; /// The number of the highest block(s) we know of. BestHeight: BridgedBlockNumber; /// Hash of the header at the highest known height. @@ -151,6 +153,8 @@ decl_error! { Halted, /// The pallet has already been initialized. AlreadyInitialized, + /// The given header is not a descendant of a particular header. + NotDescendant, } } @@ -362,7 +366,7 @@ impl Module { } } -impl bp_header_chain::HeaderChain> for Module { +impl bp_header_chain::HeaderChain, sp_runtime::DispatchError> for Module { fn best_finalized() -> BridgedHeader { PalletStorage::::new().best_finalized_header().header } @@ -371,27 +375,92 @@ impl bp_header_chain::HeaderChain> for Module { PalletStorage::::new().current_authority_set() } - fn import_header(header: BridgedHeader) -> Result<(), ()> { - let mut verifier = verifier::Verifier { - storage: PalletStorage::::new(), - }; + fn append_finalized_chain( + headers: impl IntoIterator>, + ) -> Result<(), sp_runtime::DispatchError> { + let mut storage = PalletStorage::::new(); - let _ = verifier.import_header(header.hash(), header).map_err(|_| ())?; + 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 import_finality_proof(header: BridgedHeader, finality_proof: Vec) -> Result<(), ()> { - let mut verifier = verifier::Verifier { - storage: PalletStorage::::new(), - }; +/// Import a finalized header without checking if this is true. +/// +/// This function assumes that all the given header has already been proven to be valid and +/// finalized. Using this assumption it will write them to storage with minimal checks. That +/// means it's of great importance that this function *not* called with any headers whose +/// finality has not been checked, otherwise you risk bricking your bridge. +/// +/// One thing this function does do for you is GRANDPA authority set handoffs. However, since it +/// does not do verification on the incoming header it will assume that the authority set change +/// signals in the digest are well formed. +fn import_header_unchecked(storage: &mut S, header: BridgedHeader) +where + S: BridgeStorage
>, + T: Config, +{ + // Since we want to use the existing storage infrastructure we need to indicate the fork + // that we're on. We will assume that since we are using the unchecked import there are no + // forks, and can indicate that by using the first imported header's "fork". + let dummy_fork_hash = >::get(); - let _ = verifier - .import_finality_proof(header.hash(), finality_proof.into()) - .map_err(|_| ())?; + // If we have a pending change in storage let's check if the current header enacts it. + let enact_change = if let Some(pending_change) = storage.scheduled_set_change(dummy_fork_hash) { + pending_change.height == *header.number() + } else { + // We don't have a scheduled change in storage at the moment. Let's check if the current + // header signals an authority set change. + if let Some(change) = verifier::find_scheduled_change(&header) { + let next_set = AuthoritySet { + authorities: change.next_authorities, + set_id: storage.current_authority_set().set_id + 1, + }; - Ok(()) + let height = *header.number() + change.delay; + let scheduled_change = ScheduledChange { + authority_set: next_set, + height, + }; + + storage.schedule_next_set_change(dummy_fork_hash, scheduled_change); + + // If the delay is 0 this header will enact the change it signaled + height == *header.number() + } else { + false + } + }; + + if enact_change { + const ENACT_SET_PROOF: &str = "We only set `enact_change` as `true` if we are sure that there is a scheduled + authority set change in storage. Therefore, it must exist."; + + // If we are unable to enact an authority set it means our storage entry for scheduled + // changes is missing. Best to crash since this is likely a bug. + let _ = storage.enact_authority_set(dummy_fork_hash).expect(ENACT_SET_PROOF); } + + storage.update_best_finalized(header.hash()); + + storage.write_header(&ImportedHeader { + header, + requires_justification: false, + is_finalized: true, + signal_hash: None, + }); } /// Ensure that the origin is either root, or `ModuleOwner`. @@ -448,6 +517,7 @@ fn initialize_bridge(init_params: InitializationData >::insert(initial_hash, change); }; + >::put(initial_hash); >::put(header.number()); >::put(vec![initial_hash]); >::put(initial_hash); @@ -659,7 +729,8 @@ impl BridgeStorage for PalletStorage { mod tests { use super::*; use crate::mock::{run_test, test_header, unfinalized_header, Origin, TestHeader, TestRuntime}; - use bp_test_utils::authority_list; + use bp_header_chain::HeaderChain; + use bp_test_utils::{alice, authority_list, bob}; use frame_support::{assert_noop, assert_ok}; use sp_runtime::DispatchError; @@ -845,4 +916,137 @@ mod tests { ); }); } + + #[test] + fn importing_unchecked_headers_works() { + run_test(|| { + init_with_origin(Origin::root()).unwrap(); + let storage = PalletStorage::::new(); + + let child = test_header(2); + let header = test_header(3); + + 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(|| { + init_with_origin(Origin::root()).unwrap(); + let storage = PalletStorage::::new(); + + let next_set_id = 2; + let next_authorities = vec![(alice(), 1), (bob(), 1)]; + + // Need to update the header digest to indicate that our header signals an authority set + // change. The change will be enacted when we import our header. + let mut header = test_header(2); + header.digest = fork_tests::change_log(0); + + // Let's import our test header + assert_ok!(Module::::append_finalized_chain(vec![header.clone()])); + + // Make sure that our header is the best finalized + assert_eq!(storage.best_finalized_header().header, header); + assert_eq!(storage.best_headers()[0].hash, header.hash()); + + // Make sure that the authority set actually changed upon importing our header + assert_eq!( + storage.current_authority_set(), + AuthoritySet::new(next_authorities, next_set_id), + ); + }) + } + + #[test] + fn importing_unchecked_headers_enacts_new_authority_set_from_old_header() { + run_test(|| { + init_with_origin(Origin::root()).unwrap(); + let storage = PalletStorage::::new(); + + let next_set_id = 2; + let next_authorities = vec![(alice(), 1), (bob(), 1)]; + + // Need to update the header digest to indicate that our header signals an authority set + // change. However, the change doesn't happen until the next block. + let mut schedules_change = test_header(2); + schedules_change.digest = fork_tests::change_log(1); + 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)); + + // Make sure that our header is the best finalized + assert_eq!(storage.best_finalized_header().header, header); + assert_eq!(storage.best_headers()[0].hash, header.hash()); + + // Make sure that the authority set actually changed upon importing our header + assert_eq!( + storage.current_authority_set(), + AuthoritySet::new(next_authorities, next_set_id), + ); + }) + } + + #[test] + fn importing_unchecked_header_can_enact_set_change_scheduled_at_genesis() { + run_test(|| { + let storage = PalletStorage::::new(); + + let next_authorities = vec![(alice(), 1)]; + let next_set_id = 2; + let next_authority_set = AuthoritySet::new(next_authorities.clone(), next_set_id); + + let first_scheduled_change = ScheduledChange { + authority_set: next_authority_set, + height: 2, + }; + + let init_data = InitializationData { + header: test_header(1), + authority_list: authority_list(), + set_id: 1, + scheduled_change: Some(first_scheduled_change), + is_halted: false, + }; + + assert_ok!(Module::::initialize(Origin::root(), init_data)); + + // 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)); + + // Make sure that the authority set actually changed upon importing our header + assert_eq!( + storage.current_authority_set(), + AuthoritySet::new(next_authorities, next_set_id), + ); + }) + } } diff --git a/bridges/modules/substrate/src/verifier.rs b/bridges/modules/substrate/src/verifier.rs index b22d69f78d..0c3bd1b5dd 100644 --- a/bridges/modules/substrate/src/verifier.rs +++ b/bridges/modules/substrate/src/verifier.rs @@ -242,9 +242,13 @@ where &proof.0, ) .map_err(|_| FinalizationError::InvalidJustification)?; - frame_support::debug::trace!(target: "sub-bridge", "Received valid justification for {:?}", header); + frame_support::debug::trace!("Received valid justification for {:?}", header); - frame_support::debug::trace!(target: "sub-bridge", "Checking ancestry for headers between {:?} and {:?}", last_finalized, header); + frame_support::debug::trace!( + "Checking ancestry for headers between {:?} and {:?}", + last_finalized, + header + ); let mut finalized_headers = if let Some(ancestors) = headers_between(&self.storage, last_finalized, header.clone()) { // Since we only try and finalize headers with a height strictly greater @@ -335,7 +339,7 @@ where Some(ancestors) } -fn find_scheduled_change(header: &H) -> Option> { +pub(crate) fn find_scheduled_change(header: &H) -> Option> { let id = OpaqueDigestItemId::Consensus(&GRANDPA_ENGINE_ID); let filter_log = |log: ConsensusLog| match log { diff --git a/bridges/primitives/header-chain/src/lib.rs b/bridges/primitives/header-chain/src/lib.rs index 699e79cae5..b65325b850 100644 --- a/bridges/primitives/header-chain/src/lib.rs +++ b/bridges/primitives/header-chain/src/lib.rs @@ -28,7 +28,6 @@ use core::fmt::Debug; use serde::{Deserialize, Serialize}; use sp_finality_grandpa::{AuthorityList, SetId}; use sp_runtime::RuntimeDebug; -use sp_std::prelude::Vec; pub mod justification; @@ -69,25 +68,24 @@ pub trait InclusionProofVerifier { } /// A base trait for pallets which want to keep track of a full set of headers from a bridged chain. -pub trait HeaderChain { +pub trait HeaderChain { /// Get the best finalized header known to the header chain. fn best_finalized() -> H; /// Get the best authority set known to the header chain. fn authority_set() -> AuthoritySet; - /// Write the given header to the underlying pallet storage. - #[allow(clippy::result_unit_err)] - fn import_header(header: H) -> Result<(), ()>; - - /// Submit a valid finality proof for the given header to the underlying pallet storage. + /// Write a finalized chain of headers to the underlying pallet storage. /// - /// This will finalize the given header and enact any authority set changes if required. - #[allow(clippy::result_unit_err)] - fn import_finality_proof(header: H, finality_proof: Vec) -> Result<(), ()>; + /// 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>; } -impl HeaderChain for () { +impl HeaderChain for () { fn best_finalized() -> H { H::default() } @@ -96,20 +94,14 @@ impl HeaderChain for () { AuthoritySet::default() } - #[allow(clippy::result_unit_err)] - fn import_header(_header: H) -> Result<(), ()> { - Ok(()) - } - - #[allow(clippy::result_unit_err)] - fn import_finality_proof(_header: H, _finality_proof: Vec) -> Result<(), ()> { + fn append_finalized_chain(_headers: impl IntoIterator) -> Result<(), E> { Ok(()) } } -/// A trait for checking if a given child header is a direct decendant of an ancestor. +/// A trait for checking if a given child header is a direct descendant of an ancestor. pub trait AncestryChecker { - /// Is the child header a decendant of the ancestor header? + /// Is the child header a descendant of the ancestor header? fn are_ancestors(ancestor: &H, child: &H, proof: &P) -> bool; }