Move Storage Parser from Bridge Pallet (paritytech#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

@@ -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",
 ]

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<HashOf<BridgedChain<B>>>,
 	) -> Result<ParsedMessagesDeliveryProofFromBridgedChain<B>, &'static str>
 	where
-		ThisRuntime: pallet_substrate_bridge::Config,
+		ThisRuntime: pallet_finality_verifier::Config,
 		ThisRuntime: pallet_message_lane::Config<MessageLaneInstanceOf<BridgedChain<B>>>,
 		HashOf<BridgedChain<B>>:
-			Into<bp_runtime::HashOf<<ThisRuntime as pallet_substrate_bridge::Config>::BridgedChain>>,
+			Into<bp_runtime::HashOf<<ThisRuntime as pallet_finality_verifier::Config>::BridgedChain>>,
 	{
 		let FromBridgedChainMessagesDeliveryProof {
 			bridged_header_hash,
 			storage_proof,
 			lane,
 		} = proof;
-		pallet_substrate_bridge::Module::<ThisRuntime>::parse_finalized_storage_proof(
+		pallet_finality_verifier::Module::<ThisRuntime>::parse_finalized_storage_proof(
 			bridged_header_hash.into(),
 			StorageProof::new(storage_proof),
 			|storage| {
@@ -506,16 +505,16 @@ pub mod target {
 		messages_count: u32,
 	) -> Result<ProvedMessages<Message<BalanceOf<BridgedChain<B>>>>, &'static str>
 	where
-		ThisRuntime: pallet_substrate_bridge::Config,
+		ThisRuntime: pallet_finality_verifier::Config,
 		ThisRuntime: pallet_message_lane::Config<MessageLaneInstanceOf<BridgedChain<B>>>,
 		HashOf<BridgedChain<B>>:
-			Into<bp_runtime::HashOf<<ThisRuntime as pallet_substrate_bridge::Config>::BridgedChain>>,
+			Into<bp_runtime::HashOf<<ThisRuntime as pallet_finality_verifier::Config>::BridgedChain>>,
 	{
 		verify_messages_proof_with_parser::<B, _, _>(
 			proof,
 			messages_count,
 			|bridged_header_hash, bridged_storage_proof| {
-				pallet_substrate_bridge::Module::<ThisRuntime>::parse_finalized_storage_proof(
+				pallet_finality_verifier::Module::<ThisRuntime>::parse_finalized_storage_proof(
 					bridged_header_hash.into(),
 					StorageProof::new(bridged_storage_proof),
 					|storage_adapter| storage_adapter,

bridges/bin/runtime-common/src/messages_benchmarking.rs

@@ -73,7 +73,7 @@ pub fn prepare_message_proof<B, H, R, MM, ML, MH>(
 where
 	B: MessageBridge,
 	H: Hasher,
-	R: pallet_substrate_bridge::Config,
+	R: pallet_finality_verifier::Config,
 	<R::BridgedChain as bp_runtime::Chain>::Hash: Into<HashOf<BridgedChain<B>>>,
 	MM: Fn(MessageKey) -> Vec<u8>,
 	ML: Fn(LaneId) -> Vec<u8>,
@@ -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::<R>(bridged_header);
+	pallet_finality_verifier::initialize_for_benchmarks::<R>(bridged_header);
 
 	(
 		FromBridgedChainMessagesProof {
@@ -154,7 +154,7 @@ pub fn prepare_message_delivery_proof<B, H, R, ML, MH>(
 where
 	B: MessageBridge,
 	H: Hasher,
-	R: pallet_substrate_bridge::Config,
+	R: pallet_finality_verifier::Config,
 	<R::BridgedChain as bp_runtime::Chain>::Hash: Into<HashOf<BridgedChain<B>>>,
 	ML: Fn(LaneId) -> Vec<u8>,
 	MH: Fn(H::Out) -> <R::BridgedChain as bp_runtime::Chain>::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::<R>(bridged_header);
+	pallet_finality_verifier::initialize_for_benchmarks::<R>(bridged_header);
 
 	FromBridgedChainMessagesDeliveryProof {
 		bridged_header_hash: bridged_header_hash.into(),

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 = []

bridges/modules/finality-verifier/src/lib.rs

@@ -56,7 +56,7 @@ pub type BridgedBlockNumber<T> = BlockNumberOf<<T as Config>::BridgedChain>;
 /// Block hash of the bridged chain.
 pub type BridgedBlockHash<T> = HashOf<<T as Config>::BridgedChain>;
 /// Hasher of the bridged chain.
-pub type _BridgedBlockHasher<T> = HasherOf<<T as Config>::BridgedChain>;
+pub type BridgedBlockHasher<T> = HasherOf<<T as Config>::BridgedChain>;
 /// Header of the bridged chain.
 pub type BridgedHeader<T> = HeaderOf<<T as Config>::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<T: Config>(init_params: super::InitializationData<BridgedHeader<T>>) {
+	pub(crate) fn initialize_bridge<T: Config>(init_params: super::InitializationData<BridgedHeader<T>>) {
 		let super::InitializationData {
 			header,
 			authority_list,
@@ -447,6 +451,21 @@ impl<T: Config> Pallet<T> {
 	pub fn is_known_header(hash: BridgedBlockHash<T>) -> bool {
 		<ImportedHeaders<T>>::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<R>(
+		hash: BridgedBlockHash<T>,
+		storage_proof: sp_trie::StorageProof,
+		parse: impl FnOnce(bp_runtime::StorageProofChecker<BridgedBlockHasher<T>>) -> R,
+	) -> Result<R, sp_runtime::DispatchError> {
+		let header = <ImportedHeaders<T>>::get(hash).ok_or(Error::<T>::UnknownHeader)?;
+		let storage_proof_checker = bp_runtime::StorageProofChecker::new(*header.state_root(), storage_proof)
+			.map_err(|_| Error::<T>::StorageRootMismatch)?;
+
+		Ok(parse(storage_proof_checker))
+	}
 }
 
 /// Data required for initializing the bridge pallet.
@@ -499,6 +518,17 @@ pub(crate) fn find_forced_change<H: HeaderT>(
 	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<T: Config>(header: BridgedHeader<T>) {
+	initialize_bridge::<T>(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::<TestRuntime>::parse_finalized_storage_proof(
+					Default::default(),
+					sp_trie::StorageProof::new(vec![]),
+					|_| (),
+				),
+				Error::<TestRuntime>::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();
+			<BestFinalized<TestRuntime>>::put(hash);
+			<ImportedHeaders<TestRuntime>>::insert(hash, header);
+
+			assert_ok!(
+				Module::<TestRuntime>::parse_finalized_storage_proof(hash, storage_proof, |_| (),),
+				(),
+			);
+		});
+	}
+
 	#[test]
 	fn rate_limiter_disallows_imports_once_limit_is_hit_in_single_block() {
 		run_test(|| {

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<T: Config> Module<T> {
 	pub fn parse_finalized_storage_proof<R>(
 		finalized_header_hash: BridgedBlockHash<T>,
 		storage_proof: StorageProof,
-		parse: impl FnOnce(StorageProofChecker<BridgedBlockHasher<T>>) -> R,
+		parse: impl FnOnce(bp_runtime::StorageProofChecker<BridgedBlockHasher<T>>) -> R,
 	) -> Result<R, sp_runtime::DispatchError> {
 		let storage = PalletStorage::<T>::new();
 		let header = storage
@@ -365,8 +362,8 @@ impl<T: Config> Module<T> {
 			return Err(Error::<T>::UnfinalizedHeader.into());
 		}
 
-		let storage_proof_checker =
-			StorageProofChecker::new(*header.state_root(), storage_proof).map_err(Error::<T>::from)?;
+		let storage_proof_checker = bp_runtime::StorageProofChecker::new(*header.state_root(), storage_proof)
+			.map_err(|_| Error::<T>::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::<TestRuntime>::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);

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",
 ]

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];