From 092b809b8dd6deb3f4cd933efd6b09c394c8cad8 Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Thu, 18 Jan 2024 10:20:18 +0100 Subject: [PATCH] [`polkadot-staging`] Separate `send_message()` validations (#2795) * Fix xcm-bridge-hub build * Move send_message definition to MessagesBridge impl * Make send_message infallible * Split `MessagesBridge` into 2 phases Split `MessagesBridge` into 2 phases: - message validation - message sending --- modules/messages/src/benchmarking.rs | 4 +- modules/messages/src/lib.rs | 185 +++++++++++++----------- modules/messages/src/mock.rs | 8 +- modules/messages/src/outbound_lane.rs | 58 ++++---- modules/xcm-bridge-hub/src/exporter.rs | 71 +++++---- modules/xcm-bridge-hub/src/mock.rs | 22 +-- primitives/messages/src/source_chain.rs | 26 ++-- primitives/runtime/src/lib.rs | 2 +- 8 files changed, 180 insertions(+), 196 deletions(-) diff --git a/modules/messages/src/benchmarking.rs b/modules/messages/src/benchmarking.rs index 8c4e6fbf00ca..4f13c4409672 100644 --- a/modules/messages/src/benchmarking.rs +++ b/modules/messages/src/benchmarking.rs @@ -31,7 +31,7 @@ use codec::Decode; use frame_benchmarking::{account, benchmarks_instance_pallet}; use frame_support::weights::Weight; use frame_system::RawOrigin; -use sp_runtime::traits::TrailingZeroInput; +use sp_runtime::{traits::TrailingZeroInput, BoundedVec}; use sp_std::{ops::RangeInclusive, prelude::*}; const SEED: u32 = 0; @@ -443,7 +443,7 @@ benchmarks_instance_pallet! { fn send_regular_message, I: 'static>() { let mut outbound_lane = outbound_lane::(T::bench_lane_id()); - outbound_lane.send_message(vec![]).expect("We craft valid messages"); + outbound_lane.send_message(BoundedVec::try_from(vec![]).expect("We craft valid messages")); } fn receive_messages, I: 'static>(nonce: MessageNonce) { diff --git a/modules/messages/src/lib.rs b/modules/messages/src/lib.rs index 401ec185485b..a86cb326cf04 100644 --- a/modules/messages/src/lib.rs +++ b/modules/messages/src/lib.rs @@ -534,6 +534,7 @@ pub mod pallet { } #[pallet::error] + #[derive(PartialEq, Eq)] pub enum Error { /// Pallet is not in Normal operating mode. NotOperatingNormally, @@ -543,8 +544,6 @@ pub mod pallet { MessageDispatchInactive, /// Message has been treated as invalid by chain verifier. MessageRejectedByChainVerifier(VerificationError), - /// Message has been treated as invalid by lane verifier. - MessageRejectedByLaneVerifier(VerificationError), /// Message has been treated as invalid by the pallet logic. MessageRejectedByPallet(VerificationError), /// Submitter has failed to pay fee for delivering and dispatching messages. @@ -690,68 +689,72 @@ pub mod pallet { } } +/// Structure, containing a validated message payload and all the info required +/// to send it on the bridge. +#[derive(Debug, PartialEq, Eq)] +pub struct SendMessageArgs, I: 'static> { + lane_id: LaneId, + payload: StoredMessagePayload, +} + impl bp_messages::source_chain::MessagesBridge for Pallet where T: Config, I: 'static, { - type Error = sp_runtime::DispatchErrorWithPostInfo; + type Error = Error; + type SendMessageArgs = SendMessageArgs; - fn send_message( + fn validate_message( lane: LaneId, - message: T::OutboundPayload, - ) -> Result { - crate::send_message::(lane, message) + message: &T::OutboundPayload, + ) -> Result, Self::Error> { + ensure_normal_operating_mode::()?; + + // let's check if outbound lane is active + ensure!(T::ActiveOutboundLanes::get().contains(&lane), Error::::InactiveOutboundLane); + + // let's first check if message can be delivered to target chain + T::TargetHeaderChain::verify_message(message).map_err(|err| { + log::trace!( + target: LOG_TARGET, + "Message to lane {:?} is rejected by target chain: {:?}", + lane, + err, + ); + + Error::::MessageRejectedByChainVerifier(err) + })?; + + Ok(SendMessageArgs { + lane_id: lane, + payload: StoredMessagePayload::::try_from(message.encode()).map_err(|_| { + Error::::MessageRejectedByPallet(VerificationError::MessageTooLarge) + })?, + }) } -} -/// Function that actually sends message. -fn send_message, I: 'static>( - lane_id: LaneId, - payload: T::OutboundPayload, -) -> sp_std::result::Result< - SendMessageArtifacts, - sp_runtime::DispatchErrorWithPostInfo, -> { - ensure_normal_operating_mode::()?; - - // let's check if outbound lane is active - ensure!(T::ActiveOutboundLanes::get().contains(&lane_id), Error::::InactiveOutboundLane,); - - // let's first check if message can be delivered to target chain - T::TargetHeaderChain::verify_message(&payload).map_err(|err| { + fn send_message(args: SendMessageArgs) -> SendMessageArtifacts { + // save message in outbound storage and emit event + let mut lane = outbound_lane::(args.lane_id); + let message_len = args.payload.len(); + let nonce = lane.send_message(args.payload); + + // return number of messages in the queue to let sender know about its state + let enqueued_messages = lane.data().queued_messages().saturating_len(); + log::trace!( target: LOG_TARGET, - "Message to lane {:?} is rejected by target chain: {:?}", - lane_id, - err, + "Accepted message {} to lane {:?}. Message size: {:?}", + nonce, + args.lane_id, + message_len, ); - Error::::MessageRejectedByChainVerifier(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 - .send_message(encoded_payload) - .map_err(Error::::MessageRejectedByPallet)?; - - // return number of messages in the queue to let sender know about its state - let enqueued_messages = lane.data().queued_messages().saturating_len(); - - log::trace!( - target: LOG_TARGET, - "Accepted message {} to lane {:?}. Message size: {:?}", - nonce, - lane_id, - encoded_payload_len, - ); - - Pallet::::deposit_event(Event::MessageAccepted { lane_id, nonce }); + Pallet::::deposit_event(Event::MessageAccepted { lane_id: args.lane_id, nonce }); - Ok(SendMessageArtifacts { nonce, enqueued_messages }) + SendMessageArtifacts { nonce, enqueued_messages } + } } /// Ensure that the pallet is in normal operational mode. @@ -852,6 +855,8 @@ struct RuntimeOutboundLaneStorage { } impl, I: 'static> OutboundLaneStorage for RuntimeOutboundLaneStorage { + type StoredMessagePayload = StoredMessagePayload; + fn id(&self) -> LaneId { self.lane_id } @@ -865,22 +870,15 @@ impl, I: 'static> OutboundLaneStorage for RuntimeOutboundLaneStorag } #[cfg(test)] - fn message(&self, nonce: &MessageNonce) -> Option { + fn message(&self, nonce: &MessageNonce) -> Option { OutboundMessages::::get(MessageKey { lane_id: self.lane_id, nonce: *nonce }) - .map(Into::into) } - fn save_message( - &mut self, - nonce: MessageNonce, - message_payload: MessagePayload, - ) -> Result<(), VerificationError> { + fn save_message(&mut self, nonce: MessageNonce, message_payload: Self::StoredMessagePayload) { OutboundMessages::::insert( MessageKey { lane_id: self.lane_id, nonce }, - StoredMessagePayload::::try_from(message_payload) - .map_err(|_| VerificationError::MessageTooLarge)?, + message_payload, ); - Ok(()) } fn remove_message(&mut self, nonce: &MessageNonce) { @@ -927,7 +925,10 @@ mod tests { }, outbound_lane::ReceivalConfirmationError, }; - use bp_messages::{BridgeMessagesCall, UnrewardedRelayer, UnrewardedRelayersState}; + use bp_messages::{ + source_chain::MessagesBridge, BridgeMessagesCall, UnrewardedRelayer, + UnrewardedRelayersState, + }; use bp_test_utils::generate_owned_bridge_module_tests; use frame_support::{ assert_noop, assert_ok, @@ -944,14 +945,15 @@ mod tests { System::::reset_events(); } - fn send_regular_message() { + fn send_regular_message(lane_id: LaneId) { get_ready_for_events(); - let outbound_lane = outbound_lane::(TEST_LANE_ID); + let outbound_lane = outbound_lane::(lane_id); let message_nonce = outbound_lane.data().latest_generated_nonce + 1; let prev_enqueud_messages = outbound_lane.data().queued_messages().saturating_len(); - let artifacts = send_message::(TEST_LANE_ID, REGULAR_PAYLOAD) - .expect("send_message has failed"); + let valid_message = Pallet::::validate_message(lane_id, ®ULAR_PAYLOAD) + .expect("validate_message has failed"); + let artifacts = Pallet::::send_message(valid_message); assert_eq!(artifacts.enqueued_messages, prev_enqueud_messages + 1); // check event with assigned nonce @@ -960,7 +962,7 @@ mod tests { vec![EventRecord { phase: Phase::Initialization, event: TestEvent::Messages(Event::MessageAccepted { - lane_id: TEST_LANE_ID, + lane_id, nonce: message_nonce }), topics: vec![], @@ -1011,14 +1013,14 @@ mod tests { fn pallet_rejects_transactions_if_halted() { run_test(|| { // send message first to be able to check that delivery_proof fails later - send_regular_message(); + send_regular_message(TEST_LANE_ID); PalletOperatingMode::::put(MessagesOperatingMode::Basic( BasicOperatingMode::Halted, )); assert_noop!( - send_message::(TEST_LANE_ID, REGULAR_PAYLOAD,), + Pallet::::validate_message(TEST_LANE_ID, ®ULAR_PAYLOAD), Error::::NotOperatingNormally, ); @@ -1061,14 +1063,14 @@ mod tests { fn pallet_rejects_new_messages_in_rejecting_outbound_messages_operating_mode() { run_test(|| { // send message first to be able to check that delivery_proof fails later - send_regular_message(); + send_regular_message(TEST_LANE_ID); PalletOperatingMode::::put( MessagesOperatingMode::RejectingOutboundMessages, ); assert_noop!( - send_message::(TEST_LANE_ID, REGULAR_PAYLOAD,), + Pallet::::validate_message(TEST_LANE_ID, ®ULAR_PAYLOAD), Error::::NotOperatingNormally, ); @@ -1104,7 +1106,7 @@ mod tests { #[test] fn send_message_works() { run_test(|| { - send_regular_message(); + send_regular_message(TEST_LANE_ID); }); } @@ -1118,7 +1120,7 @@ mod tests { .extra .extend_from_slice(&[0u8; MAX_OUTBOUND_PAYLOAD_SIZE as usize]); assert_noop!( - send_message::(TEST_LANE_ID, message_payload.clone(),), + Pallet::::validate_message(TEST_LANE_ID, &message_payload.clone(),), Error::::MessageRejectedByPallet( VerificationError::MessageTooLarge ), @@ -1129,7 +1131,11 @@ mod tests { message_payload.extra.pop(); } assert_eq!(message_payload.encoded_size() as u32, MAX_OUTBOUND_PAYLOAD_SIZE); - assert_ok!(send_message::(TEST_LANE_ID, message_payload,),); + + let valid_message = + Pallet::::validate_message(TEST_LANE_ID, &message_payload) + .expect("validate_message has failed"); + Pallet::::send_message(valid_message); }) } @@ -1138,7 +1144,10 @@ mod tests { run_test(|| { // messages with this payload are rejected by target chain verifier assert_noop!( - send_message::(TEST_LANE_ID, PAYLOAD_REJECTED_BY_TARGET_CHAIN,), + Pallet::::validate_message( + TEST_LANE_ID, + &PAYLOAD_REJECTED_BY_TARGET_CHAIN, + ), Error::::MessageRejectedByChainVerifier(VerificationError::Other( mock::TEST_ERROR )), @@ -1298,7 +1307,7 @@ mod tests { #[test] fn receive_messages_delivery_proof_works() { run_test(|| { - send_regular_message(); + send_regular_message(TEST_LANE_ID); receive_messages_delivery_proof(); assert_eq!( @@ -1311,8 +1320,8 @@ mod tests { #[test] fn receive_messages_delivery_proof_rewards_relayers() { run_test(|| { - assert_ok!(send_message::(TEST_LANE_ID, REGULAR_PAYLOAD,)); - assert_ok!(send_message::(TEST_LANE_ID, REGULAR_PAYLOAD,)); + send_regular_message(TEST_LANE_ID); + send_regular_message(TEST_LANE_ID); // this reports delivery of message 1 => reward is paid to TEST_RELAYER_A let single_message_delivery_proof = TestMessagesDeliveryProof(Ok(( @@ -1698,9 +1707,9 @@ mod tests { #[test] fn messages_delivered_callbacks_are_called() { run_test(|| { - send_regular_message(); - send_regular_message(); - send_regular_message(); + send_regular_message(TEST_LANE_ID); + send_regular_message(TEST_LANE_ID); + send_regular_message(TEST_LANE_ID); // messages 1+2 are confirmed in 1 tx, message 3 in a separate tx // dispatch of message 2 has failed @@ -1759,7 +1768,7 @@ mod tests { ) { run_test(|| { // send message first to be able to check that delivery_proof fails later - send_regular_message(); + send_regular_message(TEST_LANE_ID); // 1) InboundLaneData declares that the `last_confirmed_nonce` is 1; // 2) InboundLaneData has no entries => `InboundLaneData::last_delivered_nonce()` @@ -1826,10 +1835,10 @@ mod tests { #[test] fn on_idle_callback_respects_remaining_weight() { run_test(|| { - send_regular_message(); - send_regular_message(); - send_regular_message(); - send_regular_message(); + send_regular_message(TEST_LANE_ID); + send_regular_message(TEST_LANE_ID); + send_regular_message(TEST_LANE_ID); + send_regular_message(TEST_LANE_ID); assert_ok!(Pallet::::receive_messages_delivery_proof( RuntimeOrigin::signed(1), @@ -1908,10 +1917,10 @@ mod tests { fn on_idle_callback_is_rotating_lanes_to_prune() { run_test(|| { // send + receive confirmation for lane 1 - send_regular_message(); + send_regular_message(TEST_LANE_ID); receive_messages_delivery_proof(); // send + receive confirmation for lane 2 - assert_ok!(send_message::(TEST_LANE_ID_2, REGULAR_PAYLOAD,)); + send_regular_message(TEST_LANE_ID_2); assert_ok!(Pallet::::receive_messages_delivery_proof( RuntimeOrigin::signed(1), TestMessagesDeliveryProof(Ok(( @@ -1987,7 +1996,7 @@ mod tests { fn outbound_message_from_unconfigured_lane_is_rejected() { run_test(|| { assert_noop!( - send_message::(TEST_LANE_ID_3, REGULAR_PAYLOAD,), + Pallet::::validate_message(TEST_LANE_ID_3, ®ULAR_PAYLOAD,), Error::::InactiveOutboundLane, ); }); diff --git a/modules/messages/src/mock.rs b/modules/messages/src/mock.rs index 303e641ea2d3..af9212053985 100644 --- a/modules/messages/src/mock.rs +++ b/modules/messages/src/mock.rs @@ -17,7 +17,7 @@ // From construct_runtime macro #![allow(clippy::from_over_into)] -use crate::Config; +use crate::{Config, StoredMessagePayload}; use bp_messages::{ calc_relayers_rewards, @@ -26,7 +26,7 @@ use bp_messages::{ DeliveryPayments, DispatchMessage, DispatchMessageData, MessageDispatch, ProvedLaneMessages, ProvedMessages, SourceHeaderChain, }, - DeliveredMessages, InboundLaneData, LaneId, Message, MessageKey, MessageNonce, MessagePayload, + DeliveredMessages, InboundLaneData, LaneId, Message, MessageKey, MessageNonce, UnrewardedRelayer, UnrewardedRelayersState, VerificationError, }; use bp_runtime::{messages::MessageDispatchResult, Size}; @@ -402,8 +402,8 @@ pub fn message(nonce: MessageNonce, payload: TestPayload) -> Message { } /// Return valid outbound message data, constructed from given payload. -pub fn outbound_message_data(payload: TestPayload) -> MessagePayload { - payload.encode() +pub fn outbound_message_data(payload: TestPayload) -> StoredMessagePayload { + StoredMessagePayload::::try_from(payload.encode()).expect("payload too large") } /// Return valid inbound (dispatch) message data, constructed from given payload. diff --git a/modules/messages/src/outbound_lane.rs b/modules/messages/src/outbound_lane.rs index f92e9ccfd95c..431c2cfb7eef 100644 --- a/modules/messages/src/outbound_lane.rs +++ b/modules/messages/src/outbound_lane.rs @@ -18,10 +18,7 @@ use crate::{Config, LOG_TARGET}; -use bp_messages::{ - DeliveredMessages, LaneId, MessageNonce, MessagePayload, OutboundLaneData, UnrewardedRelayer, - VerificationError, -}; +use bp_messages::{DeliveredMessages, LaneId, MessageNonce, OutboundLaneData, UnrewardedRelayer}; use codec::{Decode, Encode}; use frame_support::{ weights::{RuntimeDbWeight, Weight}, @@ -34,6 +31,8 @@ use sp_std::collections::vec_deque::VecDeque; /// Outbound lane storage. pub trait OutboundLaneStorage { + type StoredMessagePayload; + /// Lane id. fn id(&self) -> LaneId; /// Get lane data from the storage. @@ -42,13 +41,9 @@ pub trait OutboundLaneStorage { fn set_data(&mut self, data: OutboundLaneData); /// Returns saved outbound message payload. #[cfg(test)] - fn message(&self, nonce: &MessageNonce) -> Option; + fn message(&self, nonce: &MessageNonce) -> Option; /// Save outbound message in the storage. - fn save_message( - &mut self, - nonce: MessageNonce, - message_payload: MessagePayload, - ) -> Result<(), VerificationError>; + fn save_message(&mut self, nonce: MessageNonce, message_payload: Self::StoredMessagePayload); /// Remove outbound message from the storage. fn remove_message(&mut self, nonce: &MessageNonce); } @@ -91,18 +86,15 @@ impl OutboundLane { /// Send message over lane. /// /// Returns new message nonce. - pub fn send_message( - &mut self, - message_payload: MessagePayload, - ) -> Result { + pub fn send_message(&mut self, message_payload: S::StoredMessagePayload) -> MessageNonce { let mut data = self.storage.data(); let nonce = data.latest_generated_nonce + 1; data.latest_generated_nonce = nonce; - self.storage.save_message(nonce, message_payload)?; + self.storage.save_message(nonce, message_payload); self.storage.set_data(data); - Ok(nonce) + nonce } /// Confirm messages delivery. @@ -218,7 +210,7 @@ mod tests { }, outbound_lane, }; - use frame_support::{assert_ok, weights::constants::RocksDbWeight}; + use frame_support::weights::constants::RocksDbWeight; use sp_std::ops::RangeInclusive; fn unrewarded_relayers( @@ -239,9 +231,9 @@ mod tests { ) -> Result, ReceivalConfirmationError> { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 0); let result = lane.confirm_delivery(3, latest_received_nonce, relayers); @@ -256,7 +248,7 @@ mod tests { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); assert_eq!(lane.storage.data().latest_generated_nonce, 0); - assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), Ok(1)); + assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), 1); assert!(lane.storage.message(&1).is_some()); assert_eq!(lane.storage.data().latest_generated_nonce, 1); }); @@ -266,9 +258,9 @@ mod tests { fn confirm_delivery_works() { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); - assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), Ok(1)); - assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), Ok(2)); - assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), Ok(3)); + assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), 1); + assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), 2); + assert_eq!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD)), 3); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 0); assert_eq!( @@ -284,9 +276,9 @@ mod tests { fn confirm_delivery_rejects_nonce_lesser_than_latest_received() { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); assert_eq!(lane.storage.data().latest_generated_nonce, 3); assert_eq!(lane.storage.data().latest_received_nonce, 0); assert_eq!( @@ -368,9 +360,9 @@ mod tests { ); assert_eq!(lane.storage.data().oldest_unpruned_nonce, 1); // when nothing is confirmed, nothing is pruned - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); assert!(lane.storage.message(&1).is_some()); assert!(lane.storage.message(&2).is_some()); assert!(lane.storage.message(&3).is_some()); @@ -412,9 +404,9 @@ mod tests { fn confirm_delivery_detects_when_more_than_expected_messages_are_confirmed() { run_test(|| { let mut lane = outbound_lane::(TEST_LANE_ID); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); - assert_ok!(lane.send_message(outbound_message_data(REGULAR_PAYLOAD))); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); + lane.send_message(outbound_message_data(REGULAR_PAYLOAD)); assert_eq!( lane.confirm_delivery(0, 3, &unrewarded_relayers(1..=3)), Err(ReceivalConfirmationError::TryingToConfirmMoreMessagesThanExpected), diff --git a/modules/xcm-bridge-hub/src/exporter.rs b/modules/xcm-bridge-hub/src/exporter.rs index 5318b222c545..4e899dfa0113 100644 --- a/modules/xcm-bridge-hub/src/exporter.rs +++ b/modules/xcm-bridge-hub/src/exporter.rs @@ -42,12 +42,13 @@ type MessagesPallet = BridgeMessagesPallet>::BridgeMess impl, I: 'static> ExportXcm for Pallet where - T: BridgeMessagesConfig< - >::BridgeMessagesPalletInstance, - OutboundPayload = XcmAsPlainPayload, - >, + T: BridgeMessagesConfig, { - type Ticket = (SenderAndLane, XcmAsPlainPayload, XcmHash); + type Ticket = ( + SenderAndLane, + as MessagesBridge>::SendMessageArgs, + XcmHash, + ); fn validate( network: NetworkId, @@ -74,42 +75,38 @@ where message, )?; - Ok(((sender_and_lane, blob, id), price)) - } - - fn deliver( - (sender_and_lane, blob, id): (SenderAndLane, XcmAsPlainPayload, XcmHash), - ) -> Result { - let lane_id = sender_and_lane.lane; - let send_result = MessagesPallet::::send_message(lane_id, blob); - - match send_result { - Ok(artifacts) => { - log::info!( - target: LOG_TARGET, - "XCM message {:?} has been enqueued at bridge {:?} with nonce {}", - id, - lane_id, - artifacts.nonce, - ); - - // notify XCM queue manager about updated lane state - LocalXcmQueueManager::::on_bridge_message_enqueued( - &sender_and_lane, - artifacts.enqueued_messages, - ); - }, - Err(error) => { + let bridge_message = MessagesPallet::::validate_message(sender_and_lane.lane, &blob) + .map_err(|e| { log::debug!( target: LOG_TARGET, - "XCM message {:?} has been dropped because of bridge error {:?} on bridge {:?}", + "XCM message {:?} cannot be exported because of bridge error {:?} on bridge {:?}", id, - error, - lane_id, + e, + sender_and_lane.lane, ); - return Err(SendError::Transport("BridgeSendError")) - }, - } + SendError::Transport("BridgeValidateError") + })?; + + Ok(((sender_and_lane, bridge_message, id), price)) + } + + fn deliver((sender_and_lane, bridge_message, id): Self::Ticket) -> Result { + let lane_id = sender_and_lane.lane; + let artifacts = MessagesPallet::::send_message(bridge_message); + + log::info!( + target: LOG_TARGET, + "XCM message {:?} has been enqueued at bridge {:?} with nonce {}", + id, + lane_id, + artifacts.nonce, + ); + + // notify XCM queue manager about updated lane state + LocalXcmQueueManager::::on_bridge_message_enqueued( + &sender_and_lane, + artifacts.enqueued_messages, + ); Ok(id) } diff --git a/modules/xcm-bridge-hub/src/mock.rs b/modules/xcm-bridge-hub/src/mock.rs index 8edd4b1f7aa9..97fc7741e405 100644 --- a/modules/xcm-bridge-hub/src/mock.rs +++ b/modules/xcm-bridge-hub/src/mock.rs @@ -19,11 +19,10 @@ use crate as pallet_xcm_bridge_hub; use bp_messages::{ - source_chain::LaneMessageVerifier, target_chain::{DispatchMessage, MessageDispatch}, - LaneId, OutboundLaneData, VerificationError, + LaneId, }; -use bp_runtime::{messages::MessageDispatchResult, Chain, UnderlyingChainProvider}; +use bp_runtime::{messages::MessageDispatchResult, Chain, ChainId, UnderlyingChainProvider}; use bridge_runtime_common::{ messages::{ source::TargetHeaderChainAdapter, target::SourceHeaderChainAdapter, @@ -78,20 +77,6 @@ impl pallet_balances::Config for TestRuntime { type AccountStore = System; } -/// 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: &Vec, - ) -> Result<(), VerificationError> { - Ok(()) - } -} - parameter_types! { pub const ActiveOutboundLanes: &'static [LaneId] = &[TEST_LANE_ID]; } @@ -110,7 +95,6 @@ impl pallet_bridge_messages::Config for TestRuntime { type InboundRelayer = (); type DeliveryPayments = (); type TargetHeaderChain = TargetHeaderChainAdapter; - type LaneMessageVerifier = TestLaneMessageVerifier; type DeliveryConfirmationPayments = (); type OnMessagesDelivered = (); type SourceHeaderChain = SourceHeaderChainAdapter; @@ -220,6 +204,7 @@ impl XcmBlobHauler for TestXcmBlobHauler { pub struct ThisChain; impl Chain for ThisChain { + const ID: ChainId = *b"tuch"; type BlockNumber = u64; type Hash = H256; type Hasher = BlakeTwo256; @@ -243,6 +228,7 @@ pub type BridgedHeaderHash = H256; pub type BridgedChainHeader = SubstrateHeader; impl Chain for BridgedChain { + const ID: ChainId = *b"tuch"; type BlockNumber = u64; type Hash = BridgedHeaderHash; type Hasher = BlakeTwo256; diff --git a/primitives/messages/src/source_chain.rs b/primitives/messages/src/source_chain.rs index 56bcf0bb4bc0..f4aefd973558 100644 --- a/primitives/messages/src/source_chain.rs +++ b/primitives/messages/src/source_chain.rs @@ -125,22 +125,22 @@ pub trait MessagesBridge { /// Error type. type Error: Debug; - /// Send message over the bridge. + /// Intermediary structure returned by `validate_message()`. /// - /// Returns unique message nonce or error if send has failed. - fn send_message(lane: LaneId, message: Payload) -> Result; -} - -/// Bridge that does nothing when message is being sent. -#[derive(Eq, RuntimeDebug, PartialEq)] -pub struct NoopMessagesBridge; + /// It can than be passed to `send_message()` in order to actually send the message + /// on the bridge. + type SendMessageArgs; -impl MessagesBridge for NoopMessagesBridge { - type Error = &'static str; + /// Check if the message can be sent over the bridge. + fn validate_message( + lane: LaneId, + message: &Payload, + ) -> Result; - fn send_message(_lane: LaneId, _message: Payload) -> Result { - Ok(SendMessageArtifacts { nonce: 0, enqueued_messages: 0 }) - } + /// Send message over the bridge. + /// + /// Returns unique message nonce or error if send has failed. + fn send_message(message: Self::SendMessageArgs) -> SendMessageArtifacts; } /// Structure that may be used in place of `TargetHeaderChain` and diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index e3bdc53c5154..850318923dc7 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -313,7 +313,7 @@ pub trait StorageDoubleMapKeyProvider { } /// Error generated by the `OwnedBridgeModule` trait. -#[derive(Encode, Decode, TypeInfo, PalletError)] +#[derive(Encode, Decode, PartialEq, Eq, TypeInfo, PalletError)] pub enum OwnedBridgeModuleError { /// All pallet operations are halted. Halted,