get rid of LaneMessageVerifier (paritytech#2168) (paritytech#2764)

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<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(
@@ -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> {

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<OnThisChainBridge>;
-	type LaneMessageVerifier = FromThisChainMessageVerifier<OnThisChainBridge>;
 	type DeliveryConfirmationPayments = pallet_bridge_relayers::DeliveryConfirmationPaymentsAdapter<
 		TestRuntime,
 		(),

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?
 

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<Self::OutboundPayload, Self::AccountId>;
-		/// Message payload verifier.
-		type LaneMessageVerifier: LaneMessageVerifier<Self::OutboundPayload>;
 		/// Delivery confirmation payments.
 		type DeliveryConfirmationPayments: DeliveryConfirmationPayments<Self::AccountId>;
 		/// Delivery confirmation callback.
@@ -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
@@ -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(|| {

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<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;
@@ -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(),

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<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> {
@@ -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;
 
@@ -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;