From 51db99ea79c8c813426bc54327fe745cf60786a2 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 8 Mar 2021 17:13:25 -0500 Subject: [PATCH] Move Storage Parser from Bridge Pallet (#793) * Move storage proof checker to runtime primtives * Add method for parsing storage proofs * Use finality-verifier pallet in runtime-common * Get bridge pallet compiling again * Use storage prover from bp-runtime in a few more places * Don't leak `std` items from proof helper into `no-std` builds * Fix benchmarking compilation * Remove unused import in fuzzer --- bridges/bin/runtime-common/Cargo.toml | 6 +- bridges/bin/runtime-common/src/messages.rs | 15 ++-- .../src/messages_benchmarking.rs | 8 +-- bridges/modules/finality-verifier/Cargo.toml | 3 + bridges/modules/finality-verifier/src/lib.rs | 68 ++++++++++++++++++- bridges/modules/substrate/src/lib.rs | 11 ++- bridges/primitives/runtime/Cargo.toml | 9 +++ bridges/primitives/runtime/src/lib.rs | 5 ++ .../runtime}/src/storage_proof.rs | 63 ++++++++--------- 9 files changed, 128 insertions(+), 60 deletions(-) rename bridges/{modules/substrate => primitives/runtime}/src/storage_proof.rs (70%) diff --git a/bridges/bin/runtime-common/Cargo.toml b/bridges/bin/runtime-common/Cargo.toml index 1a6a5db5370e..a0221cbb7cb9 100644 --- a/bridges/bin/runtime-common/Cargo.toml +++ b/bridges/bin/runtime-common/Cargo.toml @@ -19,7 +19,7 @@ bp-message-lane = { path = "../../primitives/message-lane", default-features = f bp-runtime = { path = "../../primitives/runtime", 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-substrate-bridge = { path = "../../modules/substrate", default-features = false } +pallet-finality-verifier = { path = "../../modules/finality-verifier", default-features = false } # Substrate dependencies @@ -40,8 +40,8 @@ std = [ "frame-support/std", "hash-db/std", "pallet-bridge-call-dispatch/std", + "pallet-finality-verifier/std", "pallet-message-lane/std", - "pallet-substrate-bridge/std", "sp-core/std", "sp-runtime/std", "sp-state-machine/std", @@ -50,7 +50,7 @@ std = [ ] runtime-benchmarks = [ "ed25519-dalek/u64_backend", + "pallet-finality-verifier/runtime-benchmarks", "pallet-message-lane/runtime-benchmarks", - "pallet-substrate-bridge/runtime-benchmarks", "sp-state-machine", ] diff --git a/bridges/bin/runtime-common/src/messages.rs b/bridges/bin/runtime-common/src/messages.rs index 755562db1f72..cc30a33671a1 100644 --- a/bridges/bin/runtime-common/src/messages.rs +++ b/bridges/bin/runtime-common/src/messages.rs @@ -26,11 +26,10 @@ use bp_message_lane::{ target_chain::{DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages}, InboundLaneData, LaneId, Message, MessageData, MessageKey, MessageNonce, OutboundLaneData, }; -use bp_runtime::{InstanceId, Size}; +use bp_runtime::{InstanceId, Size, StorageProofChecker}; use codec::{Decode, Encode}; use frame_support::{traits::Instance, weights::Weight, RuntimeDebug}; use hash_db::Hasher; -use pallet_substrate_bridge::StorageProofChecker; use sp_runtime::traits::{AtLeast32BitUnsigned, CheckedAdd, CheckedDiv, CheckedMul}; use sp_std::{cmp::PartialOrd, convert::TryFrom, fmt::Debug, marker::PhantomData, ops::RangeInclusive, vec::Vec}; use sp_trie::StorageProof; @@ -352,17 +351,17 @@ pub mod source { proof: FromBridgedChainMessagesDeliveryProof>>, ) -> Result, &'static str> where - ThisRuntime: pallet_substrate_bridge::Config, + ThisRuntime: pallet_finality_verifier::Config, ThisRuntime: pallet_message_lane::Config>>, HashOf>: - Into::BridgedChain>>, + Into::BridgedChain>>, { let FromBridgedChainMessagesDeliveryProof { bridged_header_hash, storage_proof, lane, } = proof; - pallet_substrate_bridge::Module::::parse_finalized_storage_proof( + pallet_finality_verifier::Module::::parse_finalized_storage_proof( bridged_header_hash.into(), StorageProof::new(storage_proof), |storage| { @@ -506,16 +505,16 @@ pub mod target { messages_count: u32, ) -> Result>>>, &'static str> where - ThisRuntime: pallet_substrate_bridge::Config, + ThisRuntime: pallet_finality_verifier::Config, ThisRuntime: pallet_message_lane::Config>>, HashOf>: - Into::BridgedChain>>, + Into::BridgedChain>>, { verify_messages_proof_with_parser::( proof, messages_count, |bridged_header_hash, bridged_storage_proof| { - pallet_substrate_bridge::Module::::parse_finalized_storage_proof( + pallet_finality_verifier::Module::::parse_finalized_storage_proof( bridged_header_hash.into(), StorageProof::new(bridged_storage_proof), |storage_adapter| storage_adapter, diff --git a/bridges/bin/runtime-common/src/messages_benchmarking.rs b/bridges/bin/runtime-common/src/messages_benchmarking.rs index 4aa2abbd6b47..73bdd5755f06 100644 --- a/bridges/bin/runtime-common/src/messages_benchmarking.rs +++ b/bridges/bin/runtime-common/src/messages_benchmarking.rs @@ -73,7 +73,7 @@ pub fn prepare_message_proof( where B: MessageBridge, H: Hasher, - R: pallet_substrate_bridge::Config, + R: pallet_finality_verifier::Config, ::Hash: Into>>, MM: Fn(MessageKey) -> Vec, ML: Fn(LaneId) -> Vec, @@ -129,7 +129,7 @@ where // prepare Bridged chain header and insert it into the Substrate pallet let bridged_header = make_bridged_header(root); let bridged_header_hash = bridged_header.hash(); - pallet_substrate_bridge::initialize_for_benchmarks::(bridged_header); + pallet_finality_verifier::initialize_for_benchmarks::(bridged_header); ( FromBridgedChainMessagesProof { @@ -154,7 +154,7 @@ pub fn prepare_message_delivery_proof( where B: MessageBridge, H: Hasher, - R: pallet_substrate_bridge::Config, + R: pallet_finality_verifier::Config, ::Hash: Into>>, ML: Fn(LaneId) -> Vec, MH: Fn(H::Out) -> ::Header, @@ -181,7 +181,7 @@ where // prepare Bridged chain header and insert it into the Substrate pallet let bridged_header = make_bridged_header(root); let bridged_header_hash = bridged_header.hash(); - pallet_substrate_bridge::initialize_for_benchmarks::(bridged_header); + pallet_finality_verifier::initialize_for_benchmarks::(bridged_header); FromBridgedChainMessagesDeliveryProof { bridged_header_hash: bridged_header_hash.into(), diff --git a/bridges/modules/finality-verifier/Cargo.toml b/bridges/modules/finality-verifier/Cargo.toml index 539ef03a425d..fe670aeb68fc 100644 --- a/bridges/modules/finality-verifier/Cargo.toml +++ b/bridges/modules/finality-verifier/Cargo.toml @@ -25,6 +25,7 @@ frame-system = { git = "https://github.com/paritytech/substrate.git", branch = " sp-finality-grandpa = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } sp-runtime = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } sp-std = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } +sp-trie = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } [dev-dependencies] bp-test-utils = {path = "../../primitives/test-utils" } @@ -46,4 +47,6 @@ std = [ "sp-finality-grandpa/std", "sp-runtime/std", "sp-std/std", + "sp-trie/std", ] +runtime-benchmarks = [] diff --git a/bridges/modules/finality-verifier/src/lib.rs b/bridges/modules/finality-verifier/src/lib.rs index 5eba7f158873..41a6cacc15d5 100644 --- a/bridges/modules/finality-verifier/src/lib.rs +++ b/bridges/modules/finality-verifier/src/lib.rs @@ -56,7 +56,7 @@ pub type BridgedBlockNumber = BlockNumberOf<::BridgedChain>; /// Block hash of the bridged chain. pub type BridgedBlockHash = HashOf<::BridgedChain>; /// Hasher of the bridged chain. -pub type _BridgedBlockHasher = HasherOf<::BridgedChain>; +pub type BridgedBlockHasher = HasherOf<::BridgedChain>; /// Header of the bridged chain. pub type BridgedHeader = HeaderOf<::BridgedChain>; @@ -335,6 +335,8 @@ pub mod pallet { TooManyRequests, /// The header being imported is older than the best finalized header known to the pallet. OldHeader, + /// The header is unknown to the pallet. + UnknownHeader, /// The scheduled authority set change found in the header is unsupported by the pallet. /// /// This is the case for non-standard (e.g forced) authority set changes. @@ -343,6 +345,8 @@ pub mod pallet { AlreadyInitialized, /// All pallet operations are halted. Halted, + /// The storage proof doesn't contains storage root. So it is invalid for given header. + StorageRootMismatch, } /// Import the given header to the pallet's storage. @@ -387,7 +391,7 @@ pub mod pallet { /// Since this writes to storage with no real checks this should only be used in functions that /// were called by a trusted origin. - fn initialize_bridge(init_params: super::InitializationData>) { + pub(crate) fn initialize_bridge(init_params: super::InitializationData>) { let super::InitializationData { header, authority_list, @@ -447,6 +451,21 @@ impl Pallet { pub fn is_known_header(hash: BridgedBlockHash) -> bool { >::contains_key(hash) } + + /// Verify that the passed storage proof is valid, given it is crafted using + /// known finalized header. If the proof is valid, then the `parse` callback + /// is called and the function returns its result. + pub fn parse_finalized_storage_proof( + hash: BridgedBlockHash, + storage_proof: sp_trie::StorageProof, + parse: impl FnOnce(bp_runtime::StorageProofChecker>) -> R, + ) -> Result { + let header = >::get(hash).ok_or(Error::::UnknownHeader)?; + let storage_proof_checker = bp_runtime::StorageProofChecker::new(*header.state_root(), storage_proof) + .map_err(|_| Error::::StorageRootMismatch)?; + + Ok(parse(storage_proof_checker)) + } } /// Data required for initializing the bridge pallet. @@ -499,6 +518,17 @@ pub(crate) fn find_forced_change( header.digest().convert_first(|l| l.try_to(id).and_then(filter_log)) } +/// (Re)initialize bridge with given header for using it in external benchmarks. +#[cfg(feature = "runtime-benchmarks")] +pub fn initialize_for_benchmarks(header: BridgedHeader) { + initialize_bridge::(InitializationData { + header, + authority_list: Vec::new(), // we don't verify any proofs in external benchmarks + set_id: 0, + is_halted: false, + }); +} + #[cfg(test)] mod tests { use super::*; @@ -864,6 +894,40 @@ mod tests { ); }) } + + #[test] + fn parse_finalized_storage_proof_rejects_proof_on_unknown_header() { + run_test(|| { + assert_noop!( + Module::::parse_finalized_storage_proof( + Default::default(), + sp_trie::StorageProof::new(vec![]), + |_| (), + ), + Error::::UnknownHeader, + ); + }); + } + + #[test] + fn parse_finalized_storage_accepts_valid_proof() { + run_test(|| { + let (state_root, storage_proof) = bp_runtime::craft_valid_storage_proof(); + + let mut header = test_header(2); + header.set_state_root(state_root); + + let hash = header.hash(); + >::put(hash); + >::insert(hash, header); + + assert_ok!( + Module::::parse_finalized_storage_proof(hash, storage_proof, |_| (),), + (), + ); + }); + } + #[test] fn rate_limiter_disallows_imports_once_limit_is_hit_in_single_block() { run_test(|| { diff --git a/bridges/modules/substrate/src/lib.rs b/bridges/modules/substrate/src/lib.rs index abadbb9dd0aa..86925d52cf2c 100644 --- a/bridges/modules/substrate/src/lib.rs +++ b/bridges/modules/substrate/src/lib.rs @@ -46,10 +46,7 @@ use sp_trie::StorageProof; // Re-export since the node uses these when configuring genesis pub use storage::{InitializationData, ScheduledChange}; -pub use storage_proof::StorageProofChecker; - mod storage; -mod storage_proof; mod verifier; #[cfg(test)] @@ -355,7 +352,7 @@ impl Module { pub fn parse_finalized_storage_proof( finalized_header_hash: BridgedBlockHash, storage_proof: StorageProof, - parse: impl FnOnce(StorageProofChecker>) -> R, + parse: impl FnOnce(bp_runtime::StorageProofChecker>) -> R, ) -> Result { let storage = PalletStorage::::new(); let header = storage @@ -365,8 +362,8 @@ impl Module { return Err(Error::::UnfinalizedHeader.into()); } - let storage_proof_checker = - StorageProofChecker::new(*header.state_root(), storage_proof).map_err(Error::::from)?; + let storage_proof_checker = bp_runtime::StorageProofChecker::new(*header.state_root(), storage_proof) + .map_err(|_| Error::::StorageRootMismatch)?; Ok(parse(storage_proof_checker)) } } @@ -898,7 +895,7 @@ mod tests { fn parse_finalized_storage_accepts_valid_proof() { run_test(|| { let mut storage = PalletStorage::::new(); - let (state_root, storage_proof) = storage_proof::tests::craft_valid_storage_proof(); + let (state_root, storage_proof) = bp_runtime::craft_valid_storage_proof(); let mut header = unfinalized_header(1); header.is_finalized = true; header.header.set_state_root(state_root); diff --git a/bridges/primitives/runtime/Cargo.toml b/bridges/primitives/runtime/Cargo.toml index 50305013cebf..37d769c92802 100644 --- a/bridges/primitives/runtime/Cargo.toml +++ b/bridges/primitives/runtime/Cargo.toml @@ -8,6 +8,7 @@ license = "GPL-3.0-or-later WITH Classpath-exception-2.0" [dependencies] codec = { package = "parity-scale-codec", version = "2.0.0", default-features = false } +hash-db = { version = "0.15.2", default-features = false } num-traits = { version = "0.2", default-features = false } # Substrate Dependencies @@ -17,15 +18,23 @@ sp-core = { git = "https://github.com/paritytech/substrate.git", branch = "maste sp-io = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } sp-runtime = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } sp-std = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } +sp-state-machine = { git = "https://github.com/paritytech/substrate.git", branch = "master", default-features = false } +sp-trie = { git = "https://github.com/paritytech/substrate.git", branch = "master" , default-features = false } + +[dev-dependencies] +sp-core = { git = "https://github.com/paritytech/substrate.git", branch = "master" } [features] default = ["std"] std = [ "codec/std", "frame-support/std", + "hash-db/std", "num-traits/std", "sp-core/std", "sp-io/std", "sp-runtime/std", "sp-std/std", + "sp-state-machine/std", + "sp-trie/std", ] diff --git a/bridges/primitives/runtime/src/lib.rs b/bridges/primitives/runtime/src/lib.rs index 6612e26c37dc..3eb2c5377178 100644 --- a/bridges/primitives/runtime/src/lib.rs +++ b/bridges/primitives/runtime/src/lib.rs @@ -24,8 +24,13 @@ use sp_io::hashing::blake2_256; use sp_std::convert::TryFrom; pub use chain::{BlockNumberOf, Chain, HashOf, HasherOf, HeaderOf}; +pub use storage_proof::StorageProofChecker; + +#[cfg(feature = "std")] +pub use storage_proof::craft_valid_storage_proof; mod chain; +mod storage_proof; /// Use this when something must be shared among all instances. pub const NO_INSTANCE_ID: InstanceId = [0, 0, 0, 0]; diff --git a/bridges/modules/substrate/src/storage_proof.rs b/bridges/primitives/runtime/src/storage_proof.rs similarity index 70% rename from bridges/modules/substrate/src/storage_proof.rs rename to bridges/primitives/runtime/src/storage_proof.rs index 4b908dde15e9..4c8c2047d2fd 100644 --- a/bridges/modules/substrate/src/storage_proof.rs +++ b/bridges/primitives/runtime/src/storage_proof.rs @@ -14,12 +14,10 @@ // You should have received a copy of the GNU General Public License // along with Parity Bridges Common. If not, see . -// TODO: remove on actual use -#![allow(dead_code)] - //! Logic for checking Substrate storage proofs. use hash_db::{HashDB, Hasher, EMPTY_PREFIX}; +use sp_core::H256; use sp_runtime::RuntimeDebug; use sp_std::vec::Vec; use sp_trie::{read_trie_value, Layout, MemoryDB, StorageProof}; @@ -65,49 +63,42 @@ pub enum Error { StorageValueUnavailable, } -impl From for crate::Error { - fn from(error: Error) -> Self { - match error { - Error::StorageRootMismatch => crate::Error::StorageRootMismatch, - Error::StorageValueUnavailable => crate::Error::StorageValueUnavailable, - } - } +/// Return valid storage proof and state root. +/// +/// NOTE: This should only be used for **testing**. +#[cfg(feature = "std")] +pub fn craft_valid_storage_proof() -> (H256, StorageProof) { + use sp_state_machine::{backend::Backend, prove_read, InMemoryBackend}; + + // construct storage proof + let backend = >::from(vec![ + (None, vec![(b"key1".to_vec(), Some(b"value1".to_vec()))]), + (None, vec![(b"key2".to_vec(), Some(b"value2".to_vec()))]), + (None, vec![(b"key3".to_vec(), Some(b"value3".to_vec()))]), + // Value is too big to fit in a branch node + (None, vec![(b"key11".to_vec(), Some(vec![0u8; 32]))]), + ]); + let root = backend.storage_root(std::iter::empty()).0; + let proof = StorageProof::new( + prove_read(backend, &[&b"key1"[..], &b"key2"[..], &b"key22"[..]]) + .unwrap() + .iter_nodes() + .collect(), + ); + + (root, proof) } #[cfg(test)] pub mod tests { use super::*; - use sp_core::{Blake2Hasher, H256}; - use sp_state_machine::{backend::Backend, prove_read, InMemoryBackend}; - - /// Return valid storage proof and state root. - pub fn craft_valid_storage_proof() -> (H256, StorageProof) { - // construct storage proof - let backend = >::from(vec![ - (None, vec![(b"key1".to_vec(), Some(b"value1".to_vec()))]), - (None, vec![(b"key2".to_vec(), Some(b"value2".to_vec()))]), - (None, vec![(b"key3".to_vec(), Some(b"value3".to_vec()))]), - // Value is too big to fit in a branch node - (None, vec![(b"key11".to_vec(), Some(vec![0u8; 32]))]), - ]); - let root = backend.storage_root(std::iter::empty()).0; - let proof = StorageProof::new( - prove_read(backend, &[&b"key1"[..], &b"key2"[..], &b"key22"[..]]) - .unwrap() - .iter_nodes() - .collect(), - ); - - (root, proof) - } - #[test] fn storage_proof_check() { let (root, proof) = craft_valid_storage_proof(); // check proof in runtime - let checker = >::new(root, proof.clone()).unwrap(); + let checker = >::new(root, proof.clone()).unwrap(); assert_eq!(checker.read_value(b"key1"), Ok(Some(b"value1".to_vec()))); assert_eq!(checker.read_value(b"key2"), Ok(Some(b"value2".to_vec()))); assert_eq!(checker.read_value(b"key11111"), Err(Error::StorageValueUnavailable)); @@ -115,7 +106,7 @@ pub mod tests { // checking proof against invalid commitment fails assert_eq!( - >::new(H256::random(), proof).err(), + >::new(H256::random(), proof).err(), Some(Error::StorageRootMismatch) ); }