From 4dbdf0c22a9da7a043b55446e8aa1fa772037cf1 Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Tue, 9 May 2023 08:21:02 +0300 Subject: [PATCH] Propagate message verification errors (#2114) * Propagate message verification errors * Replace parse_finalized_storage_proof() with storage_proof_checker() * small fixes * fix comment --- bridges/bin/runtime-common/src/messages.rs | 218 ++++++++---------- bridges/modules/grandpa/src/lib.rs | 10 +- bridges/modules/messages/src/lib.rs | 20 +- bridges/modules/messages/src/mock.rs | 27 +-- bridges/modules/parachains/src/lib.rs | 207 ++++++++--------- bridges/primitives/header-chain/src/lib.rs | 12 +- bridges/primitives/messages/Cargo.toml | 2 + bridges/primitives/messages/src/lib.rs | 39 +++- .../primitives/messages/src/source_chain.rs | 30 +-- .../primitives/messages/src/target_chain.rs | 16 +- 10 files changed, 277 insertions(+), 304 deletions(-) diff --git a/bridges/bin/runtime-common/src/messages.rs b/bridges/bin/runtime-common/src/messages.rs index 6f6b19595775d..97a460716c904 100644 --- a/bridges/bin/runtime-common/src/messages.rs +++ b/bridges/bin/runtime-common/src/messages.rs @@ -22,18 +22,19 @@ pub use bp_runtime::{RangeInclusiveExt, UnderlyingChainOf, UnderlyingChainProvider}; -use bp_header_chain::{HeaderChain, HeaderChainError}; +use bp_header_chain::HeaderChain; use bp_messages::{ source_chain::{LaneMessageVerifier, TargetHeaderChain}, target_chain::{ProvedLaneMessages, ProvedMessages, SourceHeaderChain}, InboundLaneData, LaneId, Message, MessageKey, MessageNonce, MessagePayload, OutboundLaneData, + VerificationError, }; -use bp_runtime::{Chain, RawStorageProof, Size, StorageProofChecker, StorageProofError}; +use bp_runtime::{Chain, RawStorageProof, Size, StorageProofChecker}; use codec::{Decode, Encode}; use frame_support::{traits::Get, weights::Weight, RuntimeDebug}; use hash_db::Hasher; use scale_info::TypeInfo; -use sp_std::{convert::TryFrom, fmt::Debug, marker::PhantomData, vec::Vec}; +use sp_std::{convert::TryFrom, marker::PhantomData, vec::Vec}; /// Bidirectional message bridge. pub trait MessageBridge { @@ -74,29 +75,6 @@ pub type BalanceOf = bp_runtime::BalanceOf>; /// Type of origin that is used on the chain. pub type OriginOf = ::RuntimeOrigin; -/// Error that happens during message verification. -#[derive(Debug, PartialEq, Eq)] -pub enum Error { - /// The message proof is empty. - EmptyMessageProof, - /// Error returned by the bridged header chain. - HeaderChain(HeaderChainError), - /// Error returned while reading/decoding inbound lane data from the storage proof. - InboundLaneStorage(StorageProofError), - /// The declared message weight is incorrect. - InvalidMessageWeight, - /// Declared messages count doesn't match actual value. - MessagesCountMismatch, - /// Error returned while reading/decoding message data from the storage proof. - MessageStorage(StorageProofError), - /// The message is too large. - MessageTooLarge, - /// Error returned while reading/decoding outbound lane data from the storage proof. - OutboundLaneStorage(StorageProofError), - /// Storage proof related error. - StorageProof(StorageProofError), -} - /// Sub-module that is declaring types required for processing This -> Bridged chain messages. pub mod source { use super::*; @@ -169,14 +147,12 @@ pub mod source { + Into>>, OriginOf>>>, AccountIdOf>: PartialEq + Clone, { - type Error = &'static str; - fn verify_message( _submitter: &OriginOf>, _lane: &LaneId, _lane_outbound_data: &OutboundLaneData, _payload: &FromThisChainMessagePayload, - ) -> Result<(), Self::Error> { + ) -> Result<(), VerificationError> { // IMPORTANT: any error that is returned here is fatal for the bridge, because // this code is executed at the bridge hub and message sender actually lives // at some sibling parachain. So we are failing **after** the message has been @@ -200,16 +176,15 @@ pub mod source { impl TargetHeaderChain>> for TargetHeaderChainAdapter { - type Error = Error; type MessagesDeliveryProof = FromBridgedChainMessagesDeliveryProof>>; - fn verify_message(payload: &FromThisChainMessagePayload) -> Result<(), Self::Error> { + fn verify_message(payload: &FromThisChainMessagePayload) -> Result<(), VerificationError> { verify_chain_message::(payload) } fn verify_messages_delivery_proof( proof: Self::MessagesDeliveryProof, - ) -> Result<(LaneId, InboundLaneData>>), Self::Error> { + ) -> Result<(LaneId, InboundLaneData>>), VerificationError> { verify_messages_delivery_proof::(proof) } } @@ -221,7 +196,7 @@ pub mod source { /// check) that would reject message (see `FromThisChainMessageVerifier`). pub fn verify_chain_message( payload: &FromThisChainMessagePayload, - ) -> Result<(), Error> { + ) -> Result<(), VerificationError> { // IMPORTANT: any error that is returned here is fatal for the bridge, because // this code is executed at the bridge hub and message sender actually lives // at some sibling parachain. So we are failing **after** the message has been @@ -243,7 +218,7 @@ pub mod source { // transaction also contains signatures and signed extensions. Because of this, we reserve // 1/3 of the the maximal extrinsic weight for this data. if payload.len() > maximal_message_size::() as usize { - return Err(Error::MessageTooLarge) + return Err(VerificationError::MessageTooLarge) } Ok(()) @@ -255,31 +230,26 @@ pub mod source { /// parachains, please use the `verify_messages_delivery_proof_from_parachain`. pub fn verify_messages_delivery_proof( proof: FromBridgedChainMessagesDeliveryProof>>, - ) -> Result, Error> { + ) -> Result, VerificationError> { let FromBridgedChainMessagesDeliveryProof { bridged_header_hash, storage_proof, lane } = proof; - B::BridgedHeaderChain::parse_finalized_storage_proof( - bridged_header_hash, - storage_proof, - |mut storage| { - // Messages delivery proof is just proof of single storage key read => any error - // is fatal. - let storage_inbound_lane_data_key = - bp_messages::storage_keys::inbound_lane_data_key( - B::BRIDGED_MESSAGES_PALLET_NAME, - &lane, - ); - let inbound_lane_data = storage - .read_and_decode_mandatory_value(storage_inbound_lane_data_key.0.as_ref()) - .map_err(Error::InboundLaneStorage)?; - - // check that the storage proof doesn't have any untouched trie nodes - storage.ensure_no_unused_nodes().map_err(Error::StorageProof)?; - - Ok((lane, inbound_lane_data)) - }, - ) - .map_err(Error::HeaderChain)? + let mut storage = + B::BridgedHeaderChain::storage_proof_checker(bridged_header_hash, storage_proof) + .map_err(VerificationError::HeaderChain)?; + // Messages delivery proof is just proof of single storage key read => any error + // is fatal. + let storage_inbound_lane_data_key = bp_messages::storage_keys::inbound_lane_data_key( + B::BRIDGED_MESSAGES_PALLET_NAME, + &lane, + ); + let inbound_lane_data = storage + .read_and_decode_mandatory_value(storage_inbound_lane_data_key.0.as_ref()) + .map_err(VerificationError::InboundLaneStorage)?; + + // check that the storage proof doesn't have any untouched trie nodes + storage.ensure_no_unused_nodes().map_err(VerificationError::StorageProof)?; + + Ok((lane, inbound_lane_data)) } } @@ -335,13 +305,12 @@ pub mod target { pub struct SourceHeaderChainAdapter(PhantomData); impl SourceHeaderChain for SourceHeaderChainAdapter { - type Error = Error; type MessagesProof = FromBridgedChainMessagesProof>>; fn verify_messages_proof( proof: Self::MessagesProof, messages_count: u32, - ) -> Result, Self::Error> { + ) -> Result, VerificationError> { verify_messages_proof::(proof, messages_count) } } @@ -357,7 +326,7 @@ pub mod target { pub fn verify_messages_proof( proof: FromBridgedChainMessagesProof>>, messages_count: u32, - ) -> Result, Error> { + ) -> Result, VerificationError> { let FromBridgedChainMessagesProof { bridged_header_hash, storage_proof, @@ -365,57 +334,52 @@ pub mod target { nonces_start, nonces_end, } = proof; + let storage = + B::BridgedHeaderChain::storage_proof_checker(bridged_header_hash, storage_proof) + .map_err(VerificationError::HeaderChain)?; + let mut parser = StorageProofCheckerAdapter::<_, B> { storage, _dummy: Default::default() }; let nonces_range = nonces_start..=nonces_end; - B::BridgedHeaderChain::parse_finalized_storage_proof( - bridged_header_hash, - storage_proof, - |storage| { - let mut parser = - StorageProofCheckerAdapter::<_, B> { storage, _dummy: Default::default() }; - - // receiving proofs where end < begin is ok (if proof includes outbound lane state) - let messages_in_the_proof = nonces_range.checked_len().unwrap_or(0); - if messages_in_the_proof != MessageNonce::from(messages_count) { - return Err(Error::MessagesCountMismatch) - } - - // Read messages first. All messages that are claimed to be in the proof must - // be in the proof. So any error in `read_value`, or even missing value is fatal. - // - // Mind that we allow proofs with no messages if outbound lane state is proved. - let mut messages = Vec::with_capacity(messages_in_the_proof as _); - for nonce in nonces_range { - let message_key = MessageKey { lane_id: lane, nonce }; - let message_payload = parser.read_and_decode_message_payload(&message_key)?; - messages.push(Message { key: message_key, payload: message_payload }); - } - - // Now let's check if proof contains outbound lane state proof. It is optional, so - // we simply ignore `read_value` errors and missing value. - let proved_lane_messages = ProvedLaneMessages { - lane_state: parser.read_and_decode_outbound_lane_data(&lane)?, - messages, - }; - - // Now we may actually check if the proof is empty or not. - if proved_lane_messages.lane_state.is_none() && - proved_lane_messages.messages.is_empty() - { - return Err(Error::EmptyMessageProof) - } - - // check that the storage proof doesn't have any untouched trie nodes - parser.storage.ensure_no_unused_nodes().map_err(Error::StorageProof)?; - - // We only support single lane messages in this generated_schema - let mut proved_messages = ProvedMessages::new(); - proved_messages.insert(lane, proved_lane_messages); - - Ok(proved_messages) - }, - ) - .map_err(Error::HeaderChain)? + // receiving proofs where end < begin is ok (if proof includes outbound lane state) + let messages_in_the_proof = nonces_range.checked_len().unwrap_or(0); + if messages_in_the_proof != MessageNonce::from(messages_count) { + return Err(VerificationError::MessagesCountMismatch) + } + + // Read messages first. All messages that are claimed to be in the proof must + // be in the proof. So any error in `read_value`, or even missing value is fatal. + // + // Mind that we allow proofs with no messages if outbound lane state is proved. + let mut messages = Vec::with_capacity(messages_in_the_proof as _); + for nonce in nonces_range { + let message_key = MessageKey { lane_id: lane, nonce }; + let message_payload = parser.read_and_decode_message_payload(&message_key)?; + messages.push(Message { key: message_key, payload: message_payload }); + } + + // Now let's check if proof contains outbound lane state proof. It is optional, so + // we simply ignore `read_value` errors and missing value. + let proved_lane_messages = ProvedLaneMessages { + lane_state: parser.read_and_decode_outbound_lane_data(&lane)?, + messages, + }; + + // Now we may actually check if the proof is empty or not. + if proved_lane_messages.lane_state.is_none() && proved_lane_messages.messages.is_empty() { + return Err(VerificationError::EmptyMessageProof) + } + + // check that the storage proof doesn't have any untouched trie nodes + parser + .storage + .ensure_no_unused_nodes() + .map_err(VerificationError::StorageProof)?; + + // We only support single lane messages in this generated_schema + let mut proved_messages = ProvedMessages::new(); + proved_messages.insert(lane, proved_lane_messages); + + Ok(proved_messages) } struct StorageProofCheckerAdapter { @@ -427,7 +391,7 @@ pub mod target { fn read_and_decode_outbound_lane_data( &mut self, lane_id: &LaneId, - ) -> Result, Error> { + ) -> Result, VerificationError> { let storage_outbound_lane_data_key = bp_messages::storage_keys::outbound_lane_data_key( B::BRIDGED_MESSAGES_PALLET_NAME, lane_id, @@ -435,13 +399,13 @@ pub mod target { self.storage .read_and_decode_opt_value(storage_outbound_lane_data_key.0.as_ref()) - .map_err(Error::OutboundLaneStorage) + .map_err(VerificationError::OutboundLaneStorage) } fn read_and_decode_message_payload( &mut self, message_key: &MessageKey, - ) -> Result { + ) -> Result { let storage_message_key = bp_messages::storage_keys::message_key( B::BRIDGED_MESSAGES_PALLET_NAME, &message_key.lane_id, @@ -449,7 +413,7 @@ pub mod target { ); self.storage .read_and_decode_mandatory_value(storage_message_key.0.as_ref()) - .map_err(Error::MessageStorage) + .map_err(VerificationError::MessageStorage) } } } @@ -470,8 +434,8 @@ mod tests { }, mock::*, }; - use bp_header_chain::StoredHeaderDataBuilder; - use bp_runtime::HeaderId; + use bp_header_chain::{HeaderChainError, StoredHeaderDataBuilder}; + use bp_runtime::{HeaderId, StorageProofError}; use codec::Encode; use sp_core::H256; use sp_runtime::traits::Header as _; @@ -559,7 +523,7 @@ mod tests { using_messages_proof(10, None, encode_all_messages, encode_lane_data, |proof| { target::verify_messages_proof::(proof, 5) }), - Err(Error::MessagesCountMismatch), + Err(VerificationError::MessagesCountMismatch), ); } @@ -569,7 +533,7 @@ mod tests { using_messages_proof(10, None, encode_all_messages, encode_lane_data, |proof| { target::verify_messages_proof::(proof, 15) }), - Err(Error::MessagesCountMismatch), + Err(VerificationError::MessagesCountMismatch), ); } @@ -582,7 +546,7 @@ mod tests { pallet_bridge_grandpa::ImportedHeaders::::remove(bridged_header_hash); target::verify_messages_proof::(proof, 10) }), - Err(Error::HeaderChain(HeaderChainError::UnknownHeader)), + Err(VerificationError::HeaderChain(HeaderChainError::UnknownHeader)), ); } @@ -605,7 +569,7 @@ mod tests { ); target::verify_messages_proof::(proof, 10) }), - Err(Error::HeaderChain(HeaderChainError::StorageProof( + Err(VerificationError::HeaderChain(HeaderChainError::StorageProof( StorageProofError::StorageRootMismatch ))), ); @@ -620,7 +584,7 @@ mod tests { proof.storage_proof.push(node); target::verify_messages_proof::(proof, 10) },), - Err(Error::HeaderChain(HeaderChainError::StorageProof( + Err(VerificationError::HeaderChain(HeaderChainError::StorageProof( StorageProofError::DuplicateNodesInProof ))), ); @@ -633,7 +597,7 @@ mod tests { proof.storage_proof.push(vec![42]); target::verify_messages_proof::(proof, 10) },), - Err(Error::StorageProof(StorageProofError::UnusedNodesInTheProof)), + Err(VerificationError::StorageProof(StorageProofError::UnusedNodesInTheProof)), ); } @@ -647,7 +611,7 @@ mod tests { encode_lane_data, |proof| target::verify_messages_proof::(proof, 10) ), - Err(Error::MessageStorage(StorageProofError::StorageValueEmpty)), + Err(VerificationError::MessageStorage(StorageProofError::StorageValueEmpty)), ); } @@ -667,7 +631,7 @@ mod tests { encode_lane_data, |proof| target::verify_messages_proof::(proof, 10), ), - Err(Error::MessageStorage(StorageProofError::StorageValueDecodeFailed(_))), + Err(VerificationError::MessageStorage(StorageProofError::StorageValueDecodeFailed(_))), ); } @@ -689,7 +653,9 @@ mod tests { }, |proof| target::verify_messages_proof::(proof, 10), ), - Err(Error::OutboundLaneStorage(StorageProofError::StorageValueDecodeFailed(_))), + Err(VerificationError::OutboundLaneStorage( + StorageProofError::StorageValueDecodeFailed(_) + )), ); } @@ -699,7 +665,7 @@ mod tests { using_messages_proof(0, None, encode_all_messages, encode_lane_data, |proof| { target::verify_messages_proof::(proof, 0) },), - Err(Error::EmptyMessageProof), + Err(VerificationError::EmptyMessageProof), ); } @@ -773,7 +739,7 @@ mod tests { proof.nonces_end = u64::MAX; target::verify_messages_proof::(proof, u32::MAX) },), - Err(Error::MessagesCountMismatch), + Err(VerificationError::MessagesCountMismatch), ); } } diff --git a/bridges/modules/grandpa/src/lib.rs b/bridges/modules/grandpa/src/lib.rs index 329e4c2113677..50286512db896 100644 --- a/bridges/modules/grandpa/src/lib.rs +++ b/bridges/modules/grandpa/src/lib.rs @@ -1212,11 +1212,8 @@ mod tests { fn parse_finalized_storage_proof_rejects_proof_on_unknown_header() { run_test(|| { assert_noop!( - Pallet::::parse_finalized_storage_proof( - Default::default(), - vec![], - |_| (), - ), + Pallet::::storage_proof_checker(Default::default(), vec![],) + .map(|_| ()), bp_header_chain::HeaderChainError::UnknownHeader, ); }); @@ -1235,8 +1232,7 @@ mod tests { >::insert(hash, header.build()); assert_ok!( - Pallet::::parse_finalized_storage_proof(hash, storage_proof, |_| (),), - (), + Pallet::::storage_proof_checker(hash, storage_proof).map(|_| ()) ); }); } diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index 045015b775125..6e27c7f2f7aeb 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/bridges/modules/messages/src/lib.rs @@ -61,7 +61,7 @@ use bp_messages::{ }, total_unrewarded_messages, DeliveredMessages, InboundLaneData, InboundMessageDetails, LaneId, MessageKey, MessageNonce, MessagePayload, MessagesOperatingMode, OutboundLaneData, - OutboundMessageDetails, UnrewardedRelayersState, + OutboundMessageDetails, UnrewardedRelayersState, VerificationError, }; use bp_runtime::{BasicOperatingMode, ChainId, OwnedBridgeModule, PreComputedSize, Size}; use codec::{Decode, Encode, MaxEncodedLen}; @@ -557,9 +557,9 @@ pub mod pallet { /// The message is too large to be sent over the bridge. MessageIsTooLarge, /// Message has been treated as invalid by chain verifier. - MessageRejectedByChainVerifier, + MessageRejectedByChainVerifier(VerificationError), /// Message has been treated as invalid by lane verifier. - MessageRejectedByLaneVerifier, + MessageRejectedByLaneVerifier(VerificationError), /// Submitter has failed to pay fee for delivering and dispatching messages. FailedToWithdrawMessageFee, /// The transaction brings too many messages. @@ -732,7 +732,7 @@ fn send_message, I: 'static>( err, ); - Error::::MessageRejectedByChainVerifier + Error::::MessageRejectedByChainVerifier(err) })?; // now let's enforce any additional lane rules @@ -746,7 +746,7 @@ fn send_message, I: 'static>( err, ); - Error::::MessageRejectedByLaneVerifier + Error::::MessageRejectedByLaneVerifier(err) }, )?; @@ -918,7 +918,7 @@ impl, I: 'static> OutboundLaneStorage for RuntimeOutboundLaneStorag fn verify_and_decode_messages_proof( proof: Chain::MessagesProof, messages_count: u32, -) -> Result>, Chain::Error> { +) -> Result>, VerificationError> { // `receive_messages_proof` weight formula and `MaxUnconfirmedMessagesAtInboundLane` check // guarantees that the `message_count` is sane and Vec may be allocated. // (tx with too many messages will either be rejected from the pool, or will fail earlier) @@ -1177,7 +1177,9 @@ mod tests { TEST_LANE_ID, PAYLOAD_REJECTED_BY_TARGET_CHAIN, ), - Error::::MessageRejectedByChainVerifier, + Error::::MessageRejectedByChainVerifier(VerificationError::Other( + mock::TEST_ERROR + )), ); }); } @@ -1190,7 +1192,9 @@ mod tests { message.reject_by_lane_verifier = true; assert_noop!( send_message::(RuntimeOrigin::signed(1), TEST_LANE_ID, message,), - Error::::MessageRejectedByLaneVerifier, + Error::::MessageRejectedByLaneVerifier(VerificationError::Other( + mock::TEST_ERROR + )), ); }); } diff --git a/bridges/modules/messages/src/mock.rs b/bridges/modules/messages/src/mock.rs index 2e45d5b601f0b..f0516fbc23f96 100644 --- a/bridges/modules/messages/src/mock.rs +++ b/bridges/modules/messages/src/mock.rs @@ -27,7 +27,7 @@ use bp_messages::{ ProvedLaneMessages, ProvedMessages, SourceHeaderChain, }, DeliveredMessages, InboundLaneData, LaneId, Message, MessageKey, MessageNonce, MessagePayload, - OutboundLaneData, UnrewardedRelayer, UnrewardedRelayersState, + OutboundLaneData, UnrewardedRelayer, UnrewardedRelayersState, VerificationError, }; use bp_runtime::{messages::MessageDispatchResult, Size}; use codec::{Decode, Encode}; @@ -295,13 +295,11 @@ impl Size for TestMessagesDeliveryProof { pub struct TestTargetHeaderChain; impl TargetHeaderChain for TestTargetHeaderChain { - type Error = &'static str; - type MessagesDeliveryProof = TestMessagesDeliveryProof; - fn verify_message(payload: &TestPayload) -> Result<(), Self::Error> { + fn verify_message(payload: &TestPayload) -> Result<(), VerificationError> { if *payload == PAYLOAD_REJECTED_BY_TARGET_CHAIN { - Err(TEST_ERROR) + Err(VerificationError::Other(TEST_ERROR)) } else { Ok(()) } @@ -309,8 +307,8 @@ impl TargetHeaderChain for TestTargetHeaderChain { fn verify_messages_delivery_proof( proof: Self::MessagesDeliveryProof, - ) -> Result<(LaneId, InboundLaneData), Self::Error> { - proof.0.map_err(|_| TEST_ERROR) + ) -> Result<(LaneId, InboundLaneData), VerificationError> { + proof.0.map_err(|_| VerificationError::Other(TEST_ERROR)) } } @@ -319,18 +317,16 @@ impl TargetHeaderChain for TestTargetHeaderChain { pub struct TestLaneMessageVerifier; impl LaneMessageVerifier for TestLaneMessageVerifier { - type Error = &'static str; - fn verify_message( _submitter: &RuntimeOrigin, _lane: &LaneId, _lane_outbound_data: &OutboundLaneData, payload: &TestPayload, - ) -> Result<(), Self::Error> { + ) -> Result<(), VerificationError> { if !payload.reject_by_lane_verifier { Ok(()) } else { - Err(TEST_ERROR) + Err(VerificationError::Other(TEST_ERROR)) } } } @@ -400,15 +396,16 @@ impl DeliveryConfirmationPayments for TestDeliveryConfirmationPayment pub struct TestSourceHeaderChain; impl SourceHeaderChain for TestSourceHeaderChain { - type Error = &'static str; - type MessagesProof = TestMessagesProof; fn verify_messages_proof( proof: Self::MessagesProof, _messages_count: u32, - ) -> Result, Self::Error> { - proof.result.map(|proof| proof.into_iter().collect()).map_err(|_| TEST_ERROR) + ) -> Result, VerificationError> { + proof + .result + .map(|proof| proof.into_iter().collect()) + .map_err(|_| VerificationError::Other(TEST_ERROR)) } } diff --git a/bridges/modules/parachains/src/lib.rs b/bridges/modules/parachains/src/lib.rs index 6c89b09513cd6..7670a3bacfe8d 100644 --- a/bridges/modules/parachains/src/lib.rs +++ b/bridges/modules/parachains/src/lib.rs @@ -91,6 +91,8 @@ pub mod pallet { BoundedStorageValue<>::MaxParaHeadDataSize, ParaStoredHeaderData>; /// Weight info of the given parachains pallet. pub type WeightInfoOf = >::WeightInfo; + type GrandpaPalletOf = + pallet_bridge_grandpa::Pallet>::BridgesGrandpaPalletInstance>; #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] @@ -125,14 +127,8 @@ pub mod pallet { UnknownRelayChainBlock, /// The number of stored relay block is different from what the relayer has provided. InvalidRelayChainBlockNumber, - /// Error generated by a method defined in `bp-header-chain`. - HeaderChain(HeaderChainError), - /// Given parachain head is unknown. - UnknownParaHead, - /// The storage proof doesn't contains storage root. So it is invalid for given header. - StorageRootMismatch, - /// Failed to extract state root from given parachain head. - FailedToExtractStateRoot, + /// Parachain heads storage proof is invalid. + HeaderChainStorageProof(HeaderChainError), /// Error generated by the `OwnedBridgeModule` trait. BridgeModule(bp_runtime::OwnedBridgeModuleError), } @@ -337,112 +333,113 @@ pub mod pallet { parachains.len() as _, ); - pallet_bridge_grandpa::Pallet::::parse_finalized_storage_proof( + let mut storage = GrandpaPalletOf::::storage_proof_checker( relay_block_hash, parachain_heads_proof.0, - move |mut storage| { - for (parachain, parachain_head_hash) in parachains { - let parachain_head = match Pallet::::read_parachain_head(&mut storage, parachain) { - Ok(Some(parachain_head)) => parachain_head, - Ok(None) => { - log::trace!( - target: LOG_TARGET, - "The head of parachain {:?} is None. {}", - parachain, - if ParasInfo::::contains_key(parachain) { - "Looks like it is not yet registered at the source relay chain" - } else { - "Looks like it has been deregistered from the source relay chain" - }, - ); - Self::deposit_event(Event::MissingParachainHead { parachain }); - continue; - }, - Err(e) => { - log::trace!( - target: LOG_TARGET, - "The read of head of parachain {:?} has failed: {:?}", - parachain, - e, - ); - Self::deposit_event(Event::MissingParachainHead { parachain }); - continue; + ) + .map_err(Error::::HeaderChainStorageProof)?; + + for (parachain, parachain_head_hash) in parachains { + let parachain_head = match Self::read_parachain_head(&mut storage, parachain) { + Ok(Some(parachain_head)) => parachain_head, + Ok(None) => { + log::trace!( + target: LOG_TARGET, + "The head of parachain {:?} is None. {}", + parachain, + if ParasInfo::::contains_key(parachain) { + "Looks like it is not yet registered at the source relay chain" + } else { + "Looks like it has been deregistered from the source relay chain" }, - }; + ); + Self::deposit_event(Event::MissingParachainHead { parachain }); + continue + }, + Err(e) => { + log::trace!( + target: LOG_TARGET, + "The read of head of parachain {:?} has failed: {:?}", + parachain, + e, + ); + Self::deposit_event(Event::MissingParachainHead { parachain }); + continue + }, + }; + + // if relayer has specified invalid parachain head hash, ignore the head + // (this isn't strictly necessary, but better safe than sorry) + let actual_parachain_head_hash = parachain_head.hash(); + if parachain_head_hash != actual_parachain_head_hash { + log::trace!( + target: LOG_TARGET, + "The submitter has specified invalid parachain {:?} head hash: \ + {:?} vs {:?}", + parachain, + parachain_head_hash, + actual_parachain_head_hash, + ); + Self::deposit_event(Event::IncorrectParachainHeadHash { + parachain, + parachain_head_hash, + actual_parachain_head_hash, + }); + continue + } - // if relayer has specified invalid parachain head hash, ignore the head - // (this isn't strictly necessary, but better safe than sorry) - let actual_parachain_head_hash = parachain_head.hash(); - if parachain_head_hash != actual_parachain_head_hash { + // convert from parachain head into stored parachain head data + let parachain_head_data = + match T::ParaStoredHeaderDataBuilder::try_build(parachain, ¶chain_head) { + Some(parachain_head_data) => parachain_head_data, + None => { log::trace!( - target: LOG_TARGET, - "The submitter has specified invalid parachain {:?} head hash: {:?} vs {:?}", - parachain, - parachain_head_hash, - actual_parachain_head_hash, - ); - Self::deposit_event(Event::IncorrectParachainHeadHash { - parachain, - parachain_head_hash, - actual_parachain_head_hash, - }); - continue; - } - - // convert from parachain head into stored parachain head data - let parachain_head_data = match T::ParaStoredHeaderDataBuilder::try_build( - parachain, - ¶chain_head, - ) { - Some(parachain_head_data) => parachain_head_data, - None => { - log::trace!( target: LOG_TARGET, "The head of parachain {:?} has been provided, but it is not tracked by the pallet", parachain, ); - Self::deposit_event(Event::UntrackedParachainRejected { parachain }); - continue; - }, - }; - - let update_result: Result<_, ()> = ParasInfo::::try_mutate(parachain, |stored_best_head| { - let artifacts = Pallet::::update_parachain_head( - parachain, - stored_best_head.take(), - relay_block_number, - parachain_head_data, - parachain_head_hash, - )?; - *stored_best_head = Some(artifacts.best_head); - Ok(artifacts.prune_happened) - }); - - // we're refunding weight if update has not happened and if pruning has not happened - let is_update_happened = matches!(update_result, Ok(_)); - if !is_update_happened { - actual_weight = actual_weight - .saturating_sub(WeightInfoOf::::parachain_head_storage_write_weight(T::DbWeight::get())); - } - let is_prune_happened = matches!(update_result, Ok(true)); - if !is_prune_happened { - actual_weight = actual_weight - .saturating_sub(WeightInfoOf::::parachain_head_pruning_weight(T::DbWeight::get())); - } - } + Self::deposit_event(Event::UntrackedParachainRejected { parachain }); + continue + }, + }; + + let update_result: Result<_, ()> = + ParasInfo::::try_mutate(parachain, |stored_best_head| { + let artifacts = Pallet::::update_parachain_head( + parachain, + stored_best_head.take(), + relay_block_number, + parachain_head_data, + parachain_head_hash, + )?; + *stored_best_head = Some(artifacts.best_head); + Ok(artifacts.prune_happened) + }); + + // we're refunding weight if update has not happened and if pruning has not happened + let is_update_happened = matches!(update_result, Ok(_)); + if !is_update_happened { + actual_weight = actual_weight.saturating_sub( + WeightInfoOf::::parachain_head_storage_write_weight( + T::DbWeight::get(), + ), + ); + } + let is_prune_happened = matches!(update_result, Ok(true)); + if !is_prune_happened { + actual_weight = actual_weight.saturating_sub( + WeightInfoOf::::parachain_head_pruning_weight(T::DbWeight::get()), + ); + } + } - // even though we may have accepted some parachain heads, we can't allow relayers to submit - // proof with unused trie nodes - // => treat this as an error - // - // (we can throw error here, because now all our calls are transactional) - storage.ensure_no_unused_nodes() - }, - ) - .and_then(|r| r.map_err(HeaderChainError::StorageProof)) - .map_err(|e| { - log::trace!(target: LOG_TARGET, "Parachain heads storage proof is invalid: {:?}", e); - Error::::HeaderChain(e) + // even though we may have accepted some parachain heads, we can't allow relayers to + // submit proof with unused trie nodes + // => treat this as an error + // + // (we can throw error here, because now all our calls are transactional) + storage.ensure_no_unused_nodes().map_err(|e| { + Error::::HeaderChainStorageProof(HeaderChainError::StorageProof(e)) })?; Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee: Pays::Yes }) @@ -1438,7 +1435,7 @@ pub(crate) mod tests { // try to import head#5 of parachain#1 at relay chain block #0 assert_noop!( import_parachain_1_head(0, Default::default(), parachains, proof), - Error::::HeaderChain(HeaderChainError::StorageProof( + Error::::HeaderChainStorageProof(HeaderChainError::StorageProof( StorageProofError::StorageRootMismatch )) ); diff --git a/bridges/primitives/header-chain/src/lib.rs b/bridges/primitives/header-chain/src/lib.rs index 5e2bbad242e5b..5b27845472896 100644 --- a/bridges/primitives/header-chain/src/lib.rs +++ b/bridges/primitives/header-chain/src/lib.rs @@ -73,18 +73,14 @@ impl StoredHeaderDataBuilder for H { pub trait HeaderChain { /// Returns state (storage) root of given finalized header. fn finalized_header_state_root(header_hash: HashOf) -> Option>; - /// Parse storage proof using finalized header. - fn parse_finalized_storage_proof( + /// Get storage proof checker using finalized header. + fn storage_proof_checker( header_hash: HashOf, storage_proof: RawStorageProof, - parse: impl FnOnce(StorageProofChecker>) -> R, - ) -> Result { + ) -> Result>, HeaderChainError> { let state_root = Self::finalized_header_state_root(header_hash) .ok_or(HeaderChainError::UnknownHeader)?; - let storage_proof_checker = bp_runtime::StorageProofChecker::new(state_root, storage_proof) - .map_err(HeaderChainError::StorageProof)?; - - Ok(parse(storage_proof_checker)) + StorageProofChecker::new(state_root, storage_proof).map_err(HeaderChainError::StorageProof) } } diff --git a/bridges/primitives/messages/Cargo.toml b/bridges/primitives/messages/Cargo.toml index 32a89f6cf78f0..cb35b4ae65b2d 100644 --- a/bridges/primitives/messages/Cargo.toml +++ b/bridges/primitives/messages/Cargo.toml @@ -14,6 +14,7 @@ serde = { version = "1.0", optional = true, features = ["derive"] } # Bridge dependencies bp-runtime = { path = "../runtime", default-features = false } +bp-header-chain = { path = "../header-chain", default-features = false } # Substrate Dependencies @@ -29,6 +30,7 @@ hex-literal = "0.4" default = ["std"] std = [ "bp-runtime/std", + "bp-header-chain/std", "codec/std", "frame-support/std", "scale-info/std", diff --git a/bridges/primitives/messages/src/lib.rs b/bridges/primitives/messages/src/lib.rs index e485aa2f801c3..10aee6ce97b7b 100644 --- a/bridges/primitives/messages/src/lib.rs +++ b/bridges/primitives/messages/src/lib.rs @@ -20,9 +20,15 @@ // RuntimeApi generated functions #![allow(clippy::too_many_arguments)] -use bp_runtime::{BasicOperatingMode, OperatingMode, RangeInclusiveExt}; +use bp_header_chain::HeaderChainError; +use bp_runtime::{ + messages::MessageDispatchResult, BasicOperatingMode, OperatingMode, RangeInclusiveExt, + StorageProofError, +}; use codec::{Decode, Encode, MaxEncodedLen}; -use frame_support::RuntimeDebug; +use frame_support::{PalletError, RuntimeDebug}; +// Weight is reexported to avoid additional frame-support dependencies in related crates. +pub use frame_support::weights::Weight; use scale_info::TypeInfo; use source_chain::RelayersRewards; use sp_core::TypeId; @@ -32,10 +38,6 @@ pub mod source_chain; pub mod storage_keys; pub mod target_chain; -use bp_runtime::messages::MessageDispatchResult; -// Weight is reexported to avoid additional frame-support dependencies in related crates. -pub use frame_support::weights::Weight; - /// Messages pallet operating mode. #[derive(Encode, Decode, Clone, Copy, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)] #[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))] @@ -414,6 +416,31 @@ pub enum BridgeMessagesCall { }, } +/// Error that happens during message verification. +#[derive(Encode, Decode, RuntimeDebug, PartialEq, Eq, PalletError, TypeInfo)] +pub enum VerificationError { + /// The message proof is empty. + EmptyMessageProof, + /// Error returned by the bridged header chain. + HeaderChain(HeaderChainError), + /// Error returned while reading/decoding inbound lane data from the storage proof. + InboundLaneStorage(StorageProofError), + /// The declared message weight is incorrect. + InvalidMessageWeight, + /// Declared messages count doesn't match actual value. + MessagesCountMismatch, + /// Error returned while reading/decoding message data from the storage proof. + MessageStorage(StorageProofError), + /// The message is too large. + MessageTooLarge, + /// Error returned while reading/decoding outbound lane data from the storage proof. + OutboundLaneStorage(StorageProofError), + /// Storage proof related error. + StorageProof(StorageProofError), + /// Custom error + Other(#[codec(skip)] &'static str), +} + #[cfg(test)] mod tests { use super::*; diff --git a/bridges/primitives/messages/src/source_chain.rs b/bridges/primitives/messages/src/source_chain.rs index 394a934171fea..f3c50b84a09ad 100644 --- a/bridges/primitives/messages/src/source_chain.rs +++ b/bridges/primitives/messages/src/source_chain.rs @@ -16,7 +16,7 @@ //! Primitives of messages module, that are used on the source chain. -use crate::{InboundLaneData, LaneId, MessageNonce, OutboundLaneData}; +use crate::{InboundLaneData, LaneId, MessageNonce, OutboundLaneData, VerificationError}; use crate::UnrewardedRelayer; use bp_runtime::Size; @@ -40,9 +40,6 @@ pub type RelayersRewards = BTreeMap; /// source chain to the target chain. The `AccountId` type here means the account /// type used by the source chain. pub trait TargetHeaderChain { - /// Error type. - type Error: Debug; - /// Proof that messages have been received by target chain. type MessagesDeliveryProof: Parameter + Size; @@ -58,12 +55,12 @@ pub trait TargetHeaderChain { /// 1MB. BTC nodes aren't accepting transactions that are larger than 1MB, so relayer /// will be unable to craft valid transaction => this (and all subsequent) messages will /// never be delivered. - fn verify_message(payload: &Payload) -> Result<(), Self::Error>; + fn verify_message(payload: &Payload) -> Result<(), VerificationError>; /// Verify messages delivery proof and return lane && nonce of the latest received message. fn verify_messages_delivery_proof( proof: Self::MessagesDeliveryProof, - ) -> Result<(LaneId, InboundLaneData), Self::Error>; + ) -> Result<(LaneId, InboundLaneData), VerificationError>; } /// Lane message verifier. @@ -75,9 +72,6 @@ pub trait TargetHeaderChain { /// /// Any fee requirements should also be enforced here. pub trait LaneMessageVerifier { - /// Error type. - type Error: Debug + Into<&'static str>; - /// Verify message payload and return Ok(()) if message is valid and allowed to be sent over the /// lane. fn verify_message( @@ -85,7 +79,7 @@ pub trait LaneMessageVerifier { lane: &LaneId, outbound_data: &OutboundLaneData, payload: &Payload, - ) -> Result<(), Self::Error>; + ) -> Result<(), VerificationError>; } /// Manages payments that are happening at the source chain during delivery confirmation @@ -169,31 +163,27 @@ const ALL_OUTBOUND_MESSAGES_REJECTED: &str = "This chain is configured to reject all outbound messages"; impl TargetHeaderChain for ForbidOutboundMessages { - type Error = &'static str; - type MessagesDeliveryProof = (); - fn verify_message(_payload: &Payload) -> Result<(), Self::Error> { - Err(ALL_OUTBOUND_MESSAGES_REJECTED) + fn verify_message(_payload: &Payload) -> Result<(), VerificationError> { + Err(VerificationError::Other(ALL_OUTBOUND_MESSAGES_REJECTED)) } fn verify_messages_delivery_proof( _proof: Self::MessagesDeliveryProof, - ) -> Result<(LaneId, InboundLaneData), Self::Error> { - Err(ALL_OUTBOUND_MESSAGES_REJECTED) + ) -> Result<(LaneId, InboundLaneData), VerificationError> { + Err(VerificationError::Other(ALL_OUTBOUND_MESSAGES_REJECTED)) } } impl LaneMessageVerifier for ForbidOutboundMessages { - type Error = &'static str; - fn verify_message( _submitter: &SenderOrigin, _lane: &LaneId, _outbound_data: &OutboundLaneData, _payload: &Payload, - ) -> Result<(), Self::Error> { - Err(ALL_OUTBOUND_MESSAGES_REJECTED) + ) -> Result<(), VerificationError> { + Err(VerificationError::Other(ALL_OUTBOUND_MESSAGES_REJECTED)) } } diff --git a/bridges/primitives/messages/src/target_chain.rs b/bridges/primitives/messages/src/target_chain.rs index 3c2e8cf0cb074..385bc4ac373d6 100644 --- a/bridges/primitives/messages/src/target_chain.rs +++ b/bridges/primitives/messages/src/target_chain.rs @@ -16,7 +16,9 @@ //! Primitives of messages module, that are used on the target chain. -use crate::{LaneId, Message, MessageKey, MessageNonce, MessagePayload, OutboundLaneData}; +use crate::{ + LaneId, Message, MessageKey, MessageNonce, MessagePayload, OutboundLaneData, VerificationError, +}; use bp_runtime::{messages::MessageDispatchResult, Size}; use codec::{Decode, Encode, Error as CodecError}; @@ -58,9 +60,6 @@ pub struct DispatchMessage { /// can't change. Wrong implementation may lead to invalid lane states (i.e. lane /// that's stuck) and/or processing messages without paying fees. pub trait SourceHeaderChain { - /// Error type. - type Error: Debug; - /// Proof that messages are sent from source chain. This may also include proof /// of corresponding outbound lane states. type MessagesProof: Parameter + Size; @@ -79,7 +78,7 @@ pub trait SourceHeaderChain { fn verify_messages_proof( proof: Self::MessagesProof, messages_count: u32, - ) -> Result, Self::Error>; + ) -> Result, VerificationError>; } /// Called when inbound message is received. @@ -164,21 +163,20 @@ pub struct ForbidInboundMessages( PhantomData<(MessagesProof, DispatchPayload)>, ); -/// Error message that is used in `ForbidOutboundMessages` implementation. +/// Error message that is used in `ForbidInboundMessages` implementation. const ALL_INBOUND_MESSAGES_REJECTED: &str = "This chain is configured to reject all inbound messages"; impl SourceHeaderChain for ForbidInboundMessages { - type Error = &'static str; type MessagesProof = MessagesProof; fn verify_messages_proof( _proof: Self::MessagesProof, _messages_count: u32, - ) -> Result, Self::Error> { - Err(ALL_INBOUND_MESSAGES_REJECTED) + ) -> Result, VerificationError> { + Err(VerificationError::Other(ALL_INBOUND_MESSAGES_REJECTED)) } }