Skip to content

Commit

Permalink
get rid of LaneMessageVerifier (paritytech#2168) (paritytech#2764)
Browse files Browse the repository at this point in the history
  • Loading branch information
svyatonik authored and serban300 committed Apr 8, 2024
1 parent 5daeb42 commit e59e7c1
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 149 deletions.
41 changes: 2 additions & 39 deletions bridges/bin/runtime-common/src/messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -120,42 +120,6 @@ pub mod source {
pub type ParsedMessagesDeliveryProofFromBridgedChain<B> =
(LaneId, InboundLaneData<AccountIdOf<ThisChain<B>>>);

/// 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<B>(PhantomData<B>);

impl<B> LaneMessageVerifier<FromThisChainMessagePayload> for FromThisChainMessageVerifier<B>
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<B: MessageBridge>() -> u32 {
super::target::maximal_incoming_message_size(
Expand Down Expand Up @@ -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<B: MessageBridge>(
payload: &FromThisChainMessagePayload,
) -> Result<(), VerificationError> {
Expand Down
3 changes: 1 addition & 2 deletions bridges/bin/runtime-common/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
use crate::messages::{
source::{
FromThisChainMaximalOutboundPayloadSize, FromThisChainMessagePayload,
FromThisChainMessageVerifier, TargetHeaderChainAdapter,
TargetHeaderChainAdapter,
},
target::{FromBridgedChainMessagePayload, SourceHeaderChainAdapter},
BridgedChainWithMessages, HashOf, MessageBridge, ThisChainWithMessages,
Expand Down Expand Up @@ -213,7 +213,6 @@ impl pallet_bridge_messages::Config for TestRuntime {
type DeliveryPayments = ();

type TargetHeaderChain = TargetHeaderChainAdapter<OnThisChainBridge>;
type LaneMessageVerifier = FromThisChainMessageVerifier<OnThisChainBridge>;
type DeliveryConfirmationPayments = pallet_bridge_relayers::DeliveryConfirmationPaymentsAdapter<
TestRuntime,
(),
Expand Down
26 changes: 6 additions & 20 deletions bridges/modules/messages/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Expand Down
34 changes: 2 additions & 32 deletions bridges/modules/messages/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -155,8 +154,6 @@ pub mod pallet {

/// Target header chain.
type TargetHeaderChain: TargetHeaderChain<Self::OutboundPayload, Self::AccountId>;
/// Message payload verifier.
type LaneMessageVerifier: LaneMessageVerifier<Self::OutboundPayload>;
/// Delivery confirmation payments.
type DeliveryConfirmationPayments: DeliveryConfirmationPayments<Self::AccountId>;
/// Delivery confirmation callback.
Expand Down Expand Up @@ -723,20 +720,8 @@ fn send_message<T: Config<I>, I: 'static>(
Error::<T, I>::MessageRejectedByChainVerifier(err)
})?;

// now let's enforce any additional lane rules
let mut lane = outbound_lane::<T, I>(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::<T, I>::MessageRejectedByLaneVerifier(err)
})?;

// finally, save message in outbound storage and emit event
let mut lane = outbound_lane::<T, I>(lane_id);
let encoded_payload = payload.encode();
let encoded_payload_len = encoded_payload.len();
let nonce = lane
Expand Down Expand Up @@ -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::<TestRuntime, ()>(TEST_LANE_ID, message,),
Error::<TestRuntime, ()>::MessageRejectedByLaneVerifier(VerificationError::Other(
mock::TEST_ERROR
)),
);
});
}

#[test]
fn receive_messages_proof_works() {
run_test(|| {
Expand Down
28 changes: 2 additions & 26 deletions bridges/modules/messages/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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.
Expand Down Expand Up @@ -120,7 +116,6 @@ impl Config for TestRuntime {
type DeliveryPayments = TestDeliveryPayments;

type TargetHeaderChain = TestTargetHeaderChain;
type LaneMessageVerifier = TestLaneMessageVerifier;
type DeliveryConfirmationPayments = TestDeliveryConfirmationPayments;
type OnMessagesDelivered = TestOnMessagesDelivered;

Expand Down Expand Up @@ -268,24 +263,6 @@ impl TargetHeaderChain<TestPayload, TestRelayer> for TestTargetHeaderChain {
}
}

/// Lane message verifier that is used in tests.
#[derive(Debug, Default)]
pub struct TestLaneMessageVerifier;

impl LaneMessageVerifier<TestPayload> 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;
Expand Down Expand Up @@ -438,7 +415,6 @@ pub fn inbound_message_data(payload: TestPayload) -> DispatchMessageData<TestPay
pub const fn message_payload(id: u64, declared_weight: u64) -> TestPayload {
TestPayload {
id,
reject_by_lane_verifier: false,
declared_weight: Weight::from_parts(declared_weight, 0),
dispatch_result: dispatch_result(0),
extra: Vec::new(),
Expand Down
32 changes: 2 additions & 30 deletions bridges/primitives/messages/src/source_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -64,24 +64,6 @@ pub trait TargetHeaderChain<Payload, AccountId> {
) -> Result<(LaneId, InboundLaneData<AccountId>), 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<Payload> {
/// 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<AccountId> {
Expand Down Expand Up @@ -161,7 +143,7 @@ impl<Payload> MessagesBridge<Payload> 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;

Expand All @@ -183,16 +165,6 @@ impl<Payload, AccountId> TargetHeaderChain<Payload, AccountId> for ForbidOutboun
}
}

impl<Payload> LaneMessageVerifier<Payload> for ForbidOutboundMessages {
fn verify_message(
_lane: &LaneId,
_outbound_data: &OutboundLaneData,
_payload: &Payload,
) -> Result<(), VerificationError> {
Err(VerificationError::Other(ALL_OUTBOUND_MESSAGES_REJECTED))
}
}

impl<AccountId> DeliveryConfirmationPayments<AccountId> for ForbidOutboundMessages {
type Error = &'static str;

Expand Down

0 comments on commit e59e7c1

Please sign in to comment.