From 8c6dfc41c5d2d204503e1b92ba57e430ffd66b1c Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Wed, 3 Jan 2024 12:13:32 +0300 Subject: [PATCH] get rid of LaneMessageVerifier (#2168) (#2764) --- bridges/bin/runtime-common/src/messages.rs | 41 +------------------ bridges/bin/runtime-common/src/mock.rs | 3 +- bridges/modules/messages/README.md | 26 +++--------- bridges/modules/messages/src/lib.rs | 34 +-------------- bridges/modules/messages/src/mock.rs | 28 +------------ .../primitives/messages/src/source_chain.rs | 32 +-------------- 6 files changed, 15 insertions(+), 149 deletions(-) diff --git a/bridges/bin/runtime-common/src/messages.rs b/bridges/bin/runtime-common/src/messages.rs index ac66adae6614..4aca53f3b983 100644 --- a/bridges/bin/runtime-common/src/messages.rs +++ b/bridges/bin/runtime-common/src/messages.rs @@ -24,7 +24,7 @@ pub use bp_runtime::{RangeInclusiveExt, UnderlyingChainOf, UnderlyingChainProvid use bp_header_chain::HeaderChain; use bp_messages::{ - source_chain::{LaneMessageVerifier, TargetHeaderChain}, + source_chain::TargetHeaderChain, target_chain::{ProvedLaneMessages, ProvedMessages, SourceHeaderChain}, InboundLaneData, LaneId, Message, MessageKey, MessageNonce, MessagePayload, OutboundLaneData, VerificationError, @@ -120,42 +120,6 @@ pub mod source { pub type ParsedMessagesDeliveryProofFromBridgedChain = (LaneId, InboundLaneData>>); - /// Message verifier that is doing all basic checks. - /// - /// This verifier assumes following: - /// - /// - all message lanes are equivalent, so all checks are the same; - /// - /// Following checks are made: - /// - /// - message is rejected if its lane is currently blocked; - /// - message is rejected if there are too many pending (undelivered) messages at the outbound - /// lane; - /// - check that the sender has rights to dispatch the call on target chain using provided - /// dispatch origin; - /// - check that the sender has paid enough funds for both message delivery and dispatch. - #[derive(RuntimeDebug)] - pub struct FromThisChainMessageVerifier(PhantomData); - - impl LaneMessageVerifier for FromThisChainMessageVerifier - where - B: MessageBridge, - { - fn verify_message( - _lane: &LaneId, - _lane_outbound_data: &OutboundLaneData, - _payload: &FromThisChainMessagePayload, - ) -> 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 - // sent and we can't report it back to sender (unless error report mechanism is - // embedded into message and its dispatcher). - - Ok(()) - } - } - /// Return maximal message size of This -> Bridged chain message. pub fn maximal_message_size() -> u32 { super::target::maximal_incoming_message_size( @@ -185,8 +149,7 @@ pub mod source { /// Do basic Bridged-chain specific verification of This -> Bridged chain message. /// /// Ok result from this function means that the delivery transaction with this message - /// may be 'mined' by the target chain. But the lane may have its own checks (e.g. fee - /// check) that would reject message (see `FromThisChainMessageVerifier`). + /// may be 'mined' by the target chain. pub fn verify_chain_message( payload: &FromThisChainMessagePayload, ) -> Result<(), VerificationError> { diff --git a/bridges/bin/runtime-common/src/mock.rs b/bridges/bin/runtime-common/src/mock.rs index bd47d37fc07d..b783ce987ced 100644 --- a/bridges/bin/runtime-common/src/mock.rs +++ b/bridges/bin/runtime-common/src/mock.rs @@ -21,7 +21,7 @@ use crate::messages::{ source::{ FromThisChainMaximalOutboundPayloadSize, FromThisChainMessagePayload, - FromThisChainMessageVerifier, TargetHeaderChainAdapter, + TargetHeaderChainAdapter, }, target::{FromBridgedChainMessagePayload, SourceHeaderChainAdapter}, BridgedChainWithMessages, HashOf, MessageBridge, ThisChainWithMessages, @@ -213,7 +213,6 @@ impl pallet_bridge_messages::Config for TestRuntime { type DeliveryPayments = (); type TargetHeaderChain = TargetHeaderChainAdapter; - type LaneMessageVerifier = FromThisChainMessageVerifier; type DeliveryConfirmationPayments = pallet_bridge_relayers::DeliveryConfirmationPaymentsAdapter< TestRuntime, (), diff --git a/bridges/modules/messages/README.md b/bridges/modules/messages/README.md index 457d5f5facfa..fe62305748cd 100644 --- a/bridges/modules/messages/README.md +++ b/bridges/modules/messages/README.md @@ -116,26 +116,12 @@ maximal possible transaction size of the chain and so on. And when the relayer s implementation must be able to parse and verify the proof of messages delivery. Normally, you would reuse the same (configurable) type on all chains that are sending messages to the same bridged chain. -The `pallet_bridge_messages::Config::LaneMessageVerifier` defines a single callback to verify outbound messages. The -simplest callback may just accept all messages. But in this case you'll need to answer many questions first. Who will -pay for the delivery and confirmation transaction? Are we sure that someone will ever deliver this message to the -bridged chain? Are we sure that we don't bloat our runtime storage by accepting this message? What if the message is -improperly encoded or has some fields set to invalid values? Answering all those (and similar) questions would lead to -correct implementation. - -There's another thing to consider when implementing type for use in -`pallet_bridge_messages::Config::LaneMessageVerifier`. It is whether we treat all message lanes identically, or they'll -have different sets of verification rules? For example, you may reserve lane#1 for messages coming from some -'wrapped-token' pallet - then you may verify in your implementation that the origin is associated with this pallet. -Lane#2 may be reserved for 'system' messages and you may charge zero fee for such messages. You may have some rate -limiting for messages sent over the lane#3. Or you may just verify the same rules set for all outbound messages - it is -all up to the `pallet_bridge_messages::Config::LaneMessageVerifier` implementation. - -The last type is the `pallet_bridge_messages::Config::DeliveryConfirmationPayments`. When confirmation transaction is -received, we call the `pay_reward()` method, passing the range of delivered messages. You may use the -[`pallet-bridge-relayers`](../relayers/) pallet and its -[`DeliveryConfirmationPaymentsAdapter`](../relayers/src/payment_adapter.rs) adapter as a possible implementation. It -allows you to pay fixed reward for relaying the message and some of its portion for confirming delivery. +The last type is the `pallet_bridge_messages::Config::DeliveryConfirmationPayments`. When confirmation +transaction is received, we call the `pay_reward()` method, passing the range of delivered messages. +You may use the [`pallet-bridge-relayers`](../relayers/) pallet and its +[`DeliveryConfirmationPaymentsAdapter`](../relayers/src/payment_adapter.rs) adapter as a possible +implementation. It allows you to pay fixed reward for relaying the message and some of its portion +for confirming delivery. ### I have a Messages Module in my Runtime, but I Want to Reject all Outbound Messages. What shall I do? diff --git a/bridges/modules/messages/src/lib.rs b/bridges/modules/messages/src/lib.rs index b87c64d16075..eeae95b482c8 100644 --- a/bridges/modules/messages/src/lib.rs +++ b/bridges/modules/messages/src/lib.rs @@ -53,8 +53,7 @@ use crate::{ use bp_messages::{ source_chain::{ - DeliveryConfirmationPayments, LaneMessageVerifier, OnMessagesDelivered, - SendMessageArtifacts, TargetHeaderChain, + DeliveryConfirmationPayments, OnMessagesDelivered, SendMessageArtifacts, TargetHeaderChain, }, target_chain::{ DeliveryPayments, DispatchMessage, MessageDispatch, ProvedLaneMessages, ProvedMessages, @@ -155,8 +154,6 @@ pub mod pallet { /// Target header chain. type TargetHeaderChain: TargetHeaderChain; - /// Message payload verifier. - type LaneMessageVerifier: LaneMessageVerifier; /// Delivery confirmation payments. type DeliveryConfirmationPayments: DeliveryConfirmationPayments; /// Delivery confirmation callback. @@ -723,20 +720,8 @@ fn send_message, I: 'static>( Error::::MessageRejectedByChainVerifier(err) })?; - // now let's enforce any additional lane rules - let mut lane = outbound_lane::(lane_id); - T::LaneMessageVerifier::verify_message(&lane_id, &lane.data(), &payload).map_err(|err| { - log::trace!( - target: LOG_TARGET, - "Message to lane {:?} is rejected by lane verifier: {:?}", - lane_id, - err, - ); - - Error::::MessageRejectedByLaneVerifier(err) - })?; - // finally, save message in outbound storage and emit event + let mut lane = outbound_lane::(lane_id); let encoded_payload = payload.encode(); let encoded_payload_len = encoded_payload.len(); let nonce = lane @@ -1151,21 +1136,6 @@ mod tests { }); } - #[test] - fn lane_verifier_rejects_invalid_message_in_send_message() { - run_test(|| { - // messages with zero fee are rejected by lane verifier - let mut message = REGULAR_PAYLOAD; - message.reject_by_lane_verifier = true; - assert_noop!( - send_message::(TEST_LANE_ID, message,), - Error::::MessageRejectedByLaneVerifier(VerificationError::Other( - mock::TEST_ERROR - )), - ); - }); - } - #[test] fn receive_messages_proof_works() { run_test(|| { diff --git a/bridges/modules/messages/src/mock.rs b/bridges/modules/messages/src/mock.rs index 648acad772d7..303e641ea2d3 100644 --- a/bridges/modules/messages/src/mock.rs +++ b/bridges/modules/messages/src/mock.rs @@ -21,15 +21,13 @@ use crate::Config; use bp_messages::{ calc_relayers_rewards, - source_chain::{ - DeliveryConfirmationPayments, LaneMessageVerifier, OnMessagesDelivered, TargetHeaderChain, - }, + source_chain::{DeliveryConfirmationPayments, OnMessagesDelivered, TargetHeaderChain}, target_chain::{ DeliveryPayments, DispatchMessage, DispatchMessageData, MessageDispatch, ProvedLaneMessages, ProvedMessages, SourceHeaderChain, }, DeliveredMessages, InboundLaneData, LaneId, Message, MessageKey, MessageNonce, MessagePayload, - OutboundLaneData, UnrewardedRelayer, UnrewardedRelayersState, VerificationError, + UnrewardedRelayer, UnrewardedRelayersState, VerificationError, }; use bp_runtime::{messages::MessageDispatchResult, Size}; use codec::{Decode, Encode}; @@ -50,8 +48,6 @@ pub type Balance = u64; pub struct TestPayload { /// Field that may be used to identify messages. pub id: u64, - /// Reject this message by lane verifier? - pub reject_by_lane_verifier: bool, /// Dispatch weight that is declared by the message sender. pub declared_weight: Weight, /// Message dispatch result. @@ -120,7 +116,6 @@ impl Config for TestRuntime { type DeliveryPayments = TestDeliveryPayments; type TargetHeaderChain = TestTargetHeaderChain; - type LaneMessageVerifier = TestLaneMessageVerifier; type DeliveryConfirmationPayments = TestDeliveryConfirmationPayments; type OnMessagesDelivered = TestOnMessagesDelivered; @@ -268,24 +263,6 @@ impl TargetHeaderChain for TestTargetHeaderChain { } } -/// Lane message verifier that is used in tests. -#[derive(Debug, Default)] -pub struct TestLaneMessageVerifier; - -impl LaneMessageVerifier for TestLaneMessageVerifier { - fn verify_message( - _lane: &LaneId, - _lane_outbound_data: &OutboundLaneData, - payload: &TestPayload, - ) -> Result<(), VerificationError> { - if !payload.reject_by_lane_verifier { - Ok(()) - } else { - Err(VerificationError::Other(TEST_ERROR)) - } - } -} - /// Reward payments at the target chain during delivery transaction. #[derive(Debug, Default)] pub struct TestDeliveryPayments; @@ -438,7 +415,6 @@ pub fn inbound_message_data(payload: TestPayload) -> DispatchMessageData TestPayload { TestPayload { id, - reject_by_lane_verifier: false, declared_weight: Weight::from_parts(declared_weight, 0), dispatch_result: dispatch_result(0), extra: Vec::new(), diff --git a/bridges/primitives/messages/src/source_chain.rs b/bridges/primitives/messages/src/source_chain.rs index 73092c3cce0a..56bcf0bb4bc0 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, VerificationError}; +use crate::{InboundLaneData, LaneId, MessageNonce, VerificationError}; use crate::UnrewardedRelayer; use bp_runtime::Size; @@ -64,24 +64,6 @@ pub trait TargetHeaderChain { ) -> Result<(LaneId, InboundLaneData), VerificationError>; } -/// Lane message verifier. -/// -/// Runtime developer may implement any additional validation logic over message-lane mechanism. -/// E.g. if lanes should have some security (e.g. you can only accept Lane1 messages from -/// Submitter1, Lane2 messages for those who has submitted first message to this lane, disable -/// Lane3 until some block, ...), then it may be built using this verifier. -/// -/// Any fee requirements should also be enforced here. -pub trait LaneMessageVerifier { - /// Verify message payload and return Ok(()) if message is valid and allowed to be sent over the - /// lane. - fn verify_message( - lane: &LaneId, - outbound_data: &OutboundLaneData, - payload: &Payload, - ) -> Result<(), VerificationError>; -} - /// Manages payments that are happening at the source chain during delivery confirmation /// transaction. pub trait DeliveryConfirmationPayments { @@ -161,7 +143,7 @@ impl MessagesBridge for NoopMessagesBridge { } } -/// Structure that may be used in place of `TargetHeaderChain`, `LaneMessageVerifier` and +/// Structure that may be used in place of `TargetHeaderChain` and /// `MessageDeliveryAndDispatchPayment` on chains, where outbound messages are forbidden. pub struct ForbidOutboundMessages; @@ -183,16 +165,6 @@ impl TargetHeaderChain for ForbidOutboun } } -impl LaneMessageVerifier for ForbidOutboundMessages { - fn verify_message( - _lane: &LaneId, - _outbound_data: &OutboundLaneData, - _payload: &Payload, - ) -> Result<(), VerificationError> { - Err(VerificationError::Other(ALL_OUTBOUND_MESSAGES_REJECTED)) - } -} - impl DeliveryConfirmationPayments for ForbidOutboundMessages { type Error = &'static str;