From 86834e2fd6a378550c308e312456ec1c6e8d3b2f Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 6 Oct 2020 15:53:25 -0400 Subject: [PATCH] Implement Substrate Pallet Runtime APIs (#389) * Implement public helpers for querying header info * Update `best_header` when importing headers * Add BestHeader to GenesisConfig * Define extra types for Millau primitives * Start implementing runtime APIs in Millau runtime * Add helper for getting headers which require a justification * Add runtime API for getting headers requiring a justification * Reword `expect()` proof for valid authority sets * Fix typo * Clean up Hasher comment * Add the Call Dispatch Pallet back to the Millau runtime * Use types from Rialto in bridge pallet config * Use the Rialto runtime APIS in the Millau runtime * Include Millau bridge instance in Rialto runtime * Add missing doc comment * Use one storage function for setting and clearing `RequiresJustification` * Remove TODO comments --- bridges/bin/millau/runtime/Cargo.toml | 4 + bridges/bin/millau/runtime/src/lib.rs | 40 +++++++++- bridges/bin/rialto/runtime/Cargo.toml | 4 +- bridges/bin/rialto/runtime/src/lib.rs | 54 +++++++++---- bridges/modules/substrate/src/lib.rs | 97 +++++++++++++++++++++++ bridges/modules/substrate/src/verifier.rs | 32 ++++++-- bridges/primitives/millau/Cargo.toml | 2 + bridges/primitives/millau/src/lib.rs | 17 +++- bridges/primitives/rialto/Cargo.toml | 2 + bridges/primitives/rialto/src/lib.rs | 17 +++- 10 files changed, 238 insertions(+), 31 deletions(-) diff --git a/bridges/bin/millau/runtime/Cargo.toml b/bridges/bin/millau/runtime/Cargo.toml index 039444b1e55b..e961ccf8c8d7 100644 --- a/bridges/bin/millau/runtime/Cargo.toml +++ b/bridges/bin/millau/runtime/Cargo.toml @@ -15,9 +15,11 @@ serde = { version = "1.0.115", optional = true, features = ["derive"] } bp-message-lane = { path = "../../../primitives/message-lane", default-features = false } bp-millau = { path = "../../../primitives/millau", default-features = false } +bp-rialto = { path = "../../../primitives/rialto", default-features = false } pallet-bridge-call-dispatch = { path = "../../../modules/call-dispatch", default-features = false } pallet-message-lane = { path = "../../../modules/message-lane", default-features = false } pallet-shift-session-manager = { path = "../../../modules/shift-session-manager", default-features = false } +pallet-substrate-bridge = { path = "../../../modules/substrate", default-features = false } # Substrate Dependencies @@ -53,6 +55,7 @@ default = ["std"] std = [ "bp-message-lane/std", "bp-millau/std", + "bp-rialto/std", "codec/std", "frame-executive/std", "frame-support/std", @@ -66,6 +69,7 @@ std = [ "pallet-randomness-collective-flip/std", "pallet-shift-session-manager/std", "pallet-session/std", + "pallet-substrate-bridge/std", "pallet-sudo/std", "pallet-timestamp/std", "pallet-transaction-payment/std", diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index 1adab7e6a2ec..656b0c292b48 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -54,6 +54,7 @@ pub use frame_support::{ }; pub use pallet_balances::Call as BalancesCall; +pub use pallet_substrate_bridge::Call as BridgeSubstrateCall; pub use pallet_timestamp::Call as TimestampCall; #[cfg(any(feature = "std", test))] @@ -216,7 +217,6 @@ impl frame_system::Trait for Runtime { impl pallet_aura::Trait for Runtime { type AuthorityId = AuraId; } - impl pallet_bridge_call_dispatch::Trait for Runtime { type Event = Event; type MessageId = (bp_message_lane::LaneId, bp_message_lane::MessageNonce); @@ -308,6 +308,13 @@ impl pallet_session::Trait for Runtime { type WeightInfo = (); } +impl pallet_substrate_bridge::Trait for Runtime { + type BridgedHeader = bp_rialto::Header; + type BridgedBlockNumber = bp_rialto::BlockNumber; + type BridgedBlockHash = bp_rialto::Hash; + type BridgedBlockHasher = bp_rialto::Hasher; +} + impl pallet_shift_session_manager::Trait for Runtime {} construct_runtime!( @@ -316,6 +323,7 @@ construct_runtime!( NodeBlock = opaque::Block, UncheckedExtrinsic = UncheckedExtrinsic { + BridgeRialto: pallet_substrate_bridge::{Module, Call, Storage}, BridgeCallDispatch: pallet_bridge_call_dispatch::{Module, Event}, System: frame_system::{Module, Call, Config, Storage, Event}, RandomnessCollectiveFlip: pallet_randomness_collective_flip::{Module, Call, Storage}, @@ -479,4 +487,34 @@ impl_runtime_apis! { None } } + + impl bp_rialto::RialtoHeaderApi for Runtime { + fn best_block() -> (bp_rialto::BlockNumber, bp_rialto::Hash) { + let header = BridgeRialto::best_header(); + (header.number, header.hash()) + } + + fn finalized_block() -> (bp_rialto::BlockNumber, bp_rialto::Hash) { + let header = BridgeRialto::best_finalized(); + (header.number, header.hash()) + } + + fn incomplete_headers() -> Vec<(bp_rialto::BlockNumber, bp_rialto::Hash)> { + // Since the pallet doesn't accept multiple scheduled changes right now + // we can only have one header requiring a justification at any time. + if let Some(header) = BridgeRialto::requires_justification() { + vec![(header.number, header.hash())] + } else { + vec![] + } + } + + fn is_known_block(hash: bp_rialto::Hash) -> bool { + BridgeRialto::is_known_header(hash) + } + + fn is_finalized_block(hash: bp_rialto::Hash) -> bool { + BridgeRialto::is_finalized_header(hash) + } + } } diff --git a/bridges/bin/rialto/runtime/Cargo.toml b/bridges/bin/rialto/runtime/Cargo.toml index 41dd0e9d672e..60de3e70b448 100644 --- a/bridges/bin/rialto/runtime/Cargo.toml +++ b/bridges/bin/rialto/runtime/Cargo.toml @@ -24,6 +24,7 @@ bp-rialto = { path = "../../../primitives/rialto", default-features = false } pallet-bridge-eth-poa = { path = "../../../modules/ethereum", default-features = false } pallet-bridge-call-dispatch = { path = "../../../modules/call-dispatch", default-features = false } pallet-bridge-currency-exchange = { path = "../../../modules/currency-exchange", default-features = false } +pallet-substrate-bridge = { path = "../../../modules/substrate", default-features = false } pallet-message-lane = { path = "../../../modules/message-lane", default-features = false } pallet-shift-session-manager = { path = "../../../modules/shift-session-manager", default-features = false } @@ -68,8 +69,8 @@ std = [ "bp-eth-poa/std", "bp-header-chain/std", "bp-message-lane/std", - "bp-millau/std", "bp-rialto/std", + "bp-millau/std", "codec/std", "frame-benchmarking/std", "frame-executive/std", @@ -85,6 +86,7 @@ std = [ "pallet-message-lane/std", "pallet-randomness-collective-flip/std", "pallet-shift-session-manager/std", + "pallet-substrate-bridge/std", "pallet-sudo/std", "pallet-timestamp/std", "pallet-transaction-payment/std", diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index 2d0c6fbb0c66..5ee7ff92460f 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -414,6 +414,13 @@ impl pallet_session::Trait for Runtime { type WeightInfo = (); } +impl pallet_substrate_bridge::Trait for Runtime { + type BridgedHeader = bp_millau::Header; + type BridgedBlockNumber = bp_millau::BlockNumber; + type BridgedBlockHash = bp_millau::Hash; + type BridgedBlockHasher = bp_millau::Hasher; +} + impl pallet_shift_session_manager::Trait for Runtime {} construct_runtime!( @@ -426,6 +433,7 @@ construct_runtime!( BridgeKovan: pallet_bridge_eth_poa::::{Module, Call, Config, Storage, ValidateUnsigned}, BridgeRialtoCurrencyExchange: pallet_bridge_currency_exchange::::{Module, Call}, BridgeKovanCurrencyExchange: pallet_bridge_currency_exchange::::{Module, Call}, + BridgeMillau: pallet_substrate_bridge::{Module, Call, Storage}, BridgeCallDispatch: pallet_bridge_call_dispatch::{Module, Event}, System: frame_system::{Module, Call, Config, Storage, Event}, RandomnessCollectiveFlip: pallet_randomness_collective_flip::{Module, Call, Storage}, @@ -562,33 +570,45 @@ impl_runtime_apis! { } } - impl bp_currency_exchange::RialtoCurrencyExchangeApi for Runtime { - fn filter_transaction_proof(proof: exchange::EthereumTransactionInclusionProof) -> bool { - BridgeRialtoCurrencyExchange::filter_transaction_proof(&proof) - } - } - - impl bp_currency_exchange::KovanCurrencyExchangeApi for Runtime { - fn filter_transaction_proof(proof: exchange::EthereumTransactionInclusionProof) -> bool { - BridgeKovanCurrencyExchange::filter_transaction_proof(&proof) - } - } - impl bp_millau::MillauHeaderApi for Runtime { fn best_block() -> (bp_millau::BlockNumber, bp_millau::Hash) { - unimplemented!("https://github.com/paritytech/parity-bridges-common/issues/368") + let header = BridgeMillau::best_header(); + (header.number, header.hash()) } fn finalized_block() -> (bp_millau::BlockNumber, bp_millau::Hash) { - unimplemented!("https://github.com/paritytech/parity-bridges-common/issues/368") + let header = BridgeMillau::best_finalized(); + (header.number, header.hash()) } fn incomplete_headers() -> Vec<(bp_millau::BlockNumber, bp_millau::Hash)> { - unimplemented!("https://github.com/paritytech/parity-bridges-common/issues/368") + // Since the pallet doesn't accept multiple scheduled changes right now + // we can only have one header requiring a justification at any time. + if let Some(header) = BridgeMillau::requires_justification() { + vec![(header.number, header.hash())] + } else { + vec![] + } } - fn is_known_block(_hash: bp_millau::Hash) -> bool { - unimplemented!("https://github.com/paritytech/parity-bridges-common/issues/368") + fn is_known_block(hash: bp_millau::Hash) -> bool { + BridgeMillau::is_known_header(hash) + } + + fn is_finalized_block(hash: bp_millau::Hash) -> bool { + BridgeMillau::is_finalized_header(hash) + } + } + + impl bp_currency_exchange::RialtoCurrencyExchangeApi for Runtime { + fn filter_transaction_proof(proof: exchange::EthereumTransactionInclusionProof) -> bool { + BridgeRialtoCurrencyExchange::filter_transaction_proof(&proof) + } + } + + impl bp_currency_exchange::KovanCurrencyExchangeApi for Runtime { + fn filter_transaction_proof(proof: exchange::EthereumTransactionInclusionProof) -> bool { + BridgeKovanCurrencyExchange::filter_transaction_proof(&proof) } } diff --git a/bridges/modules/substrate/src/lib.rs b/bridges/modules/substrate/src/lib.rs index dfc3cb181156..4b55bd41d75a 100644 --- a/bridges/modules/substrate/src/lib.rs +++ b/bridges/modules/substrate/src/lib.rs @@ -109,8 +109,15 @@ pub trait Trait: frame_system::Trait { decl_storage! { trait Store for Module as SubstrateBridge { + /// Hash of the header at the highest known height. + BestHeader: T::BridgedBlockHash; /// Hash of the best finalized header. BestFinalized: T::BridgedBlockHash; + /// A header which enacts an authority set change and therefore + /// requires a Grandpa justification. + // Since we won't always have an authority set change scheduled we + // won't always have a header which needs a justification. + RequiresJustification: Option; /// Headers which have been imported into the pallet. ImportedHeaders: map hasher(identity) T::BridgedBlockHash => Option>; /// The current Grandpa Authority set. @@ -136,6 +143,7 @@ decl_storage! { .clone() .expect("An initial header is needed"); + >::put(initial_header.hash()); >::put(initial_header.hash()); >::insert( initial_header.hash(), @@ -223,6 +231,58 @@ decl_module! { } } +impl Module { + /// Get the highest header that the pallet knows of. + // In a future where we support forks this could be a Vec of headers + // since we may have multiple headers at the same height. + pub fn best_header() -> T::BridgedHeader { + PalletStorage::::new().best_header().header + } + + /// Get the best finalized header the pallet knows of. + /// + /// Since this has been finalized correctly a user of the bridge + /// pallet should be confident that any transactions that were + /// included in this or any previous header will not be reverted. + pub fn best_finalized() -> T::BridgedHeader { + PalletStorage::::new().best_finalized_header().header + } + + /// Check if a particular header is known to the bridge pallet. + pub fn is_known_header(hash: T::BridgedBlockHash) -> bool { + PalletStorage::::new().header_exists(hash) + } + + /// Check if a particular header is finalized. + /// + /// Will return false if the header is not known to the pallet. + // One thing worth noting here is that this approach won't work well + // once we track forks since there could be an older header on a + // different fork which isn't an ancestor of our best finalized header. + pub fn is_finalized_header(hash: T::BridgedBlockHash) -> bool { + let storage = PalletStorage::::new(); + if let Some(header) = storage.header_by_hash(hash) { + header.number() <= storage.best_finalized_header().number() + } else { + false + } + } + + /// Return the latest header which enacts an authority set change + /// and still needs a finality proof. + /// + /// Will return None if there are no headers which are missing finality proofs. + pub fn requires_justification() -> Option { + let storage = PalletStorage::::new(); + let hash = storage.unfinalized_header()?; + let imported_header = storage.header_by_hash(hash).expect( + "We write a header to storage before marking it as unfinalized, therefore + this must always exist if we got an unfinalized header hash.", + ); + Some(imported_header.header) + } +} + /// Expected interface for interacting with bridge pallet storage. // TODO: This should be split into its own less-Substrate-dependent crate pub trait BridgeStorage { @@ -232,6 +292,12 @@ pub trait BridgeStorage { /// Write a header to storage. fn write_header(&mut self, header: &ImportedHeader); + /// Get the header at the highest known height. + fn best_header(&self) -> ImportedHeader; + + /// Update the header at the highest height. + fn update_best_header(&mut self, hash: ::Hash); + /// Get the best finalized header the pallet knows of. fn best_finalized_header(&self) -> ImportedHeader; @@ -241,6 +307,15 @@ pub trait BridgeStorage { /// Check if a particular header is known to the pallet. fn header_exists(&self, hash: ::Hash) -> bool; + /// Return a header which requires a justification. A header will require + /// a justification when it enacts an new authority set. + fn unfinalized_header(&self) -> Option<::Hash>; + + /// Mark a header as eventually requiring a justification. + /// + /// If None is passed the storage item is cleared. + fn update_unfinalized_header(&mut self, hash: Option<::Hash>); + /// Get a specific header by its hash. /// /// Returns None if it is not known to the pallet. @@ -284,6 +359,16 @@ impl BridgeStorage for PalletStorage { >::insert(hash, header); } + fn best_header(&self) -> ImportedHeader { + let hash = >::get(); + self.header_by_hash(hash) + .expect("A header must have been written at genesis, therefore this must always exist") + } + + fn update_best_header(&mut self, hash: T::BridgedBlockHash) { + >::put(hash) + } + fn best_finalized_header(&self) -> ImportedHeader { let hash = >::get(); self.header_by_hash(hash) @@ -302,6 +387,18 @@ impl BridgeStorage for PalletStorage { >::get(hash) } + fn unfinalized_header(&self) -> Option { + >::get() + } + + fn update_unfinalized_header(&mut self, hash: Option<::Hash>) { + if let Some(hash) = hash { + >::put(hash); + } else { + >::kill(); + } + } + fn current_authority_set(&self) -> AuthoritySet { CurrentAuthoritySet::get() } diff --git a/bridges/modules/substrate/src/verifier.rs b/bridges/modules/substrate/src/verifier.rs index 3bf99df5a21f..edde9d220d71 100644 --- a/bridges/modules/substrate/src/verifier.rs +++ b/bridges/modules/substrate/src/verifier.rs @@ -106,13 +106,14 @@ where /// Will perform some basic checks to make sure that this header doesn't break any assumptions /// such as being on a different finalized fork. pub fn import_header(&mut self, header: H) -> Result<(), ImportError> { + let hash = header.hash(); let best_finalized = self.storage.best_finalized_header(); if header.number() <= best_finalized.number() { return Err(ImportError::OldHeader); } - if self.storage.header_exists(header.hash()) { + if self.storage.header_exists(hash) { return Err(ImportError::HeaderAlreadyExists); } @@ -176,13 +177,20 @@ where } }; - let is_finalized = false; self.storage.write_header(&ImportedHeader { header, requires_justification, - is_finalized, + is_finalized: false, }); + if requires_justification { + self.storage.update_unfinalized_header(Some(hash)); + } + + // Since we're not dealing with forks at the moment we know that + // the header we just got will be the one at the best height + self.storage.update_best_header(hash); + Ok(()) } @@ -205,8 +213,8 @@ where let current_authority_set = self.storage.current_authority_set(); let voter_set = VoterSet::new(current_authority_set.authorities).expect( - "This only fails if we have an invalid list of authorities. Since we - got this from storage it should always be valid, otherwise we have a bug.", + "We verified the correctness of the authority list during header import, + before writing them to storage. This must always be valid.", ); verify_justification::( (hash, *header.number()), @@ -255,6 +263,9 @@ where let _ = self.storage.enact_authority_set().expect( "Headers must only be marked as `requires_justification` if there's a scheduled change in storage.", ); + + // Clear the storage entry since we got a justification + self.storage.update_unfinalized_header(None); } for header in finalized_headers.iter_mut() { @@ -442,9 +453,11 @@ mod tests { }; assert_ok!(verifier.import_header(header.clone())); - let stored_header = storage.header_by_hash(header.hash()); - assert!(stored_header.is_some()); - assert_eq!(stored_header.unwrap().is_finalized, false); + let stored_header = storage + .header_by_hash(header.hash()) + .expect("Should have been imported successfully"); + assert_eq!(stored_header.is_finalized, false); + assert_eq!(stored_header, storage.best_header()); }) } @@ -648,11 +661,14 @@ mod tests { }; assert_ok!(verifier.import_header(header.clone())); + assert_eq!(storage.unfinalized_header(), Some(header.hash())); + assert_ok!(verifier.import_finality_proof(header.hash(), justification.into())); assert_eq!(storage.best_finalized_header().header, header); // Make sure that we have updated the set now that we've finalized our header assert_eq!(storage.current_authority_set(), change.authority_set); + assert_eq!(storage.unfinalized_header(), None); }) } diff --git a/bridges/primitives/millau/Cargo.toml b/bridges/primitives/millau/Cargo.toml index 831a33b0403e..2125fa8205b3 100644 --- a/bridges/primitives/millau/Cargo.toml +++ b/bridges/primitives/millau/Cargo.toml @@ -11,6 +11,7 @@ license = "GPL-3.0-or-later WITH Classpath-exception-2.0" # Substrate Based Dependencies sp-api = { version = "2.0", default-features = false } sp-core = { version = "2.0", default-features = false } +sp-runtime = { version = "2.0", default-features = false } sp-std = { version = "2.0", default-features = false } [features] @@ -18,5 +19,6 @@ default = ["std"] std = [ "sp-api/std", "sp-core/std", + "sp-runtime/std", "sp-std/std", ] diff --git a/bridges/primitives/millau/src/lib.rs b/bridges/primitives/millau/src/lib.rs index 214497d1e858..dc233caab4ce 100644 --- a/bridges/primitives/millau/src/lib.rs +++ b/bridges/primitives/millau/src/lib.rs @@ -20,13 +20,21 @@ // Runtime-generated DecodeLimit::decode_all_With_depth_limit #![allow(clippy::unnecessary_mut_passed)] +use sp_core::Hasher as HasherT; +use sp_runtime::traits::BlakeTwo256; use sp_std::prelude::*; /// Block number type used in Millau. pub type BlockNumber = u32; /// Hash type used in Millau. -pub type Hash = sp_core::H256; +pub type Hash = ::Out; + +/// The type of an object that can produce hashes on Millau. +pub type Hasher = BlakeTwo256; + +/// The header type used by Millau. +pub type Header = sp_runtime::generic::Header; sp_api::decl_runtime_apis! { /// API for querying information about Millau headers from the Bridge Pallet instance. @@ -42,8 +50,13 @@ sp_api::decl_runtime_apis! { /// Returns number and hash of the best finalized block known to the bridge module. fn finalized_block() -> (BlockNumber, Hash); /// Returns numbers and hashes of headers that require finality proofs. + /// + /// An empty response means that there are no headers which currently require a + /// finality proof. fn incomplete_headers() -> Vec<(BlockNumber, Hash)>; - /// Returns true if header is known to the runtime. + /// Returns true if the header is known to the runtime. fn is_known_block(hash: Hash) -> bool; + /// Returns true if the header is considered finalized by the runtime. + fn is_finalized_block(hash: Hash) -> bool; } } diff --git a/bridges/primitives/rialto/Cargo.toml b/bridges/primitives/rialto/Cargo.toml index e8a04907b8c3..80616f3863d3 100644 --- a/bridges/primitives/rialto/Cargo.toml +++ b/bridges/primitives/rialto/Cargo.toml @@ -11,6 +11,7 @@ license = "GPL-3.0-or-later WITH Classpath-exception-2.0" # Substrate Based Dependencies sp-api = { version = "2.0.0", default-features = false } sp-core = { version = "2.0.0", default-features = false } +sp-runtime = { version = "2.0.0", default-features = false } sp-std = { version = "2.0.0", default-features = false } [features] @@ -18,5 +19,6 @@ default = ["std"] std = [ "sp-api/std", "sp-core/std", + "sp-runtime/std", "sp-std/std", ] diff --git a/bridges/primitives/rialto/src/lib.rs b/bridges/primitives/rialto/src/lib.rs index f23e09c51149..0a3fddbc0b74 100644 --- a/bridges/primitives/rialto/src/lib.rs +++ b/bridges/primitives/rialto/src/lib.rs @@ -20,13 +20,21 @@ // Runtime-generated DecodeLimit::decode_all_With_depth_limit #![allow(clippy::unnecessary_mut_passed)] +use sp_core::Hasher as HasherT; +use sp_runtime::traits::BlakeTwo256; use sp_std::prelude::*; /// Block number type used in Rialto. pub type BlockNumber = u32; /// Hash type used in Rialto. -pub type Hash = sp_core::H256; +pub type Hash = ::Out; + +/// The type of an object that can produce hashes on Rialto. +pub type Hasher = BlakeTwo256; + +/// The header type used by Rialto. +pub type Header = sp_runtime::generic::Header; sp_api::decl_runtime_apis! { /// API for querying information about Rialto headers from the Bridge Pallet instance. @@ -42,8 +50,13 @@ sp_api::decl_runtime_apis! { /// Returns number and hash of the best finalized block known to the bridge module. fn finalized_block() -> (BlockNumber, Hash); /// Returns numbers and hashes of headers that require finality proofs. + /// + /// An empty response means that there are no headers which currently require a + /// finality proof. fn incomplete_headers() -> Vec<(BlockNumber, Hash)>; - /// Returns true if header is known to the runtime. + /// Returns true if the header is known to the runtime. fn is_known_block(hash: Hash) -> bool; + /// Returns true if the header is considered finalized by the runtime. + fn is_finalized_block(hash: Hash) -> bool; } }