Skip to content

Commit

Permalink
Separate send_message() validations (#2797)
Browse files Browse the repository at this point in the history 
* Separate `send_message()` validations
  • Loading branch information
@serban300
serban300 authored Jan 18, 2024
1 parent 126f9e5 commit 446e978
Show file tree
Hide file tree
Showing 10 changed files with 174 additions and 175 deletions.
4 changes: 2 additions & 2 deletions modules/messages/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use codec::Decode;
use frame_benchmarking::{account, v2::*};
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;
Expand Down Expand Up @@ -121,7 +121,7 @@ fn send_regular_message<T: Config<I>, I: 'static>() {
);

let mut outbound_lane = active_outbound_lane::<T, I>(T::bench_lane_id()).unwrap();
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<T: Config<I>, I: 'static>(nonce: MessageNonce) {
Expand Down
18 changes: 7 additions & 11 deletions modules/messages/src/lanes_manager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use crate::{

use bp_messages::{
target_chain::MessageDispatch, ChainWithMessages, InboundLaneData, LaneId, LaneState,
MessageKey, MessageNonce, MessagePayload, OutboundLaneData, VerificationError,
MessageKey, MessageNonce, OutboundLaneData,
};
use bp_runtime::AccountIdOf;
use codec::{Decode, Encode, MaxEncodedLen};
Expand Down Expand Up @@ -230,6 +230,7 @@ impl<T: Config<I>, I: 'static> InboundLaneStorage for RuntimeInboundLaneStorage<
}

/// Runtime outbound lane storage.
#[derive(Debug, PartialEq, Eq)]
pub struct RuntimeOutboundLaneStorage<T, I = ()> {
pub(crate) lane_id: LaneId,
pub(crate) cached_data: OutboundLaneData,
Expand All @@ -250,6 +251,8 @@ impl<T: Config<I>, I: 'static> RuntimeOutboundLaneStorage<T, I> {
}

impl<T: Config<I>, I: 'static> OutboundLaneStorage for RuntimeOutboundLaneStorage<T, I> {
type StoredMessagePayload = StoredMessagePayload<T, I>;

fn id(&self) -> LaneId {
self.lane_id
}
Expand All @@ -264,22 +267,15 @@ impl<T: Config<I>, I: 'static> OutboundLaneStorage for RuntimeOutboundLaneStorag
}

#[cfg(test)]
fn message(&self, nonce: &MessageNonce) -> Option<MessagePayload> {
fn message(&self, nonce: &MessageNonce) -> Option<Self::StoredMessagePayload> {
OutboundMessages::<T, I>::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::<T, I>::insert(
MessageKey { lane_id: self.lane_id, nonce },
StoredMessagePayload::<T, I>::try_from(message_payload)
.map_err(|_| VerificationError::MessageTooLarge)?,
message_payload,
);
Ok(())
}

fn remove_message(&mut self, nonce: &MessageNonce) {
Expand Down
113 changes: 59 additions & 54 deletions modules/messages/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ pub mod pallet {
}

#[pallet::error]
#[derive(PartialEq, Eq)]
pub enum Error<T, I = ()> {
/// Pallet is not in Normal operating mode.
NotOperatingNormally,
Expand Down Expand Up @@ -590,67 +591,71 @@ 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<T: Config<I>, I: 'static> {
lane_id: LaneId,
lane: OutboundLane<RuntimeOutboundLaneStorage<T, I>>,
payload: StoredMessagePayload<T, I>,
}

impl<T, I> bp_messages::source_chain::MessagesBridge<T::OutboundPayload> for Pallet<T, I>
where
T: Config<I>,
I: 'static,
{
type Error = sp_runtime::DispatchErrorWithPostInfo<PostDispatchInfo>;

fn send_message(
lane: LaneId,
message: T::OutboundPayload,
) -> Result<SendMessageArtifacts, Self::Error> {
crate::send_message::<T, I>(lane, message)
type Error = Error<T, I>;
type SendMessageArgs = SendMessageArgs<T, I>;

fn validate_message(
lane_id: LaneId,
message: &T::OutboundPayload,
) -> Result<SendMessageArgs<T, I>, Self::Error> {
// 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).

// apart from maximal message size check (see below), we should also check the message
// dispatch weight here. But we assume that the bridged chain will just push the message
// to some queue (XCMP, UMP, DMP), so the weight is constant and fits the block.

// we can't accept any messages if the pallet is halted
ensure_normal_operating_mode::<T, I>()?;

let lane = active_outbound_lane::<T, I>(lane_id)?;

Ok(SendMessageArgs {
lane_id,
lane,
payload: StoredMessagePayload::<T, I>::try_from(message.encode()).map_err(|_| {
Error::<T, I>::MessageRejectedByPallet(VerificationError::MessageTooLarge)
})?,
})
}
}

/// Function that actually sends message.
fn send_message<T: Config<I>, I: 'static>(
lane_id: LaneId,
payload: T::OutboundPayload,
) -> sp_std::result::Result<
SendMessageArtifacts,
sp_runtime::DispatchErrorWithPostInfo<PostDispatchInfo>,
> {
// 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).

// apart from maximal message size check (see below), we should also check the message
// dispatch weight here. But we assume that the bridged chain will just push the message
// to some queue (XCMP, UMP, DMP), so the weight is constant and fits the block.

// we can't accept any messages if the pallet is halted
ensure_normal_operating_mode::<T, I>()?;

// finally, save message in outbound storage and emit event
let mut lane = active_outbound_lane::<T, I>(lane_id)?;
let encoded_payload = payload.encode();
let encoded_payload_len = encoded_payload.len();

// the message size is checked by the `send_message` method, so we don't need to repeat
// it here
let nonce = lane
.send_message(encoded_payload)
.map_err(Error::<T, I>::MessageRejectedByPallet)?;

// return number of messages in the queue to let sender know about its state
let enqueued_messages = lane.queued_messages().saturating_len();

log::trace!(
target: LOG_TARGET,
"Accepted message {} to lane {:?}. Message size: {:?}",
nonce,
lane_id,
encoded_payload_len,
);

Pallet::<T, I>::deposit_event(Event::MessageAccepted { lane_id, nonce });

Ok(SendMessageArtifacts { nonce, enqueued_messages })
fn send_message(mut args: SendMessageArgs<T, I>) -> SendMessageArtifacts {
// save message in outbound storage and emit event
let message_len = args.payload.len();
let nonce = args.lane.send_message(args.payload);

// return number of messages in the queue to let sender know about its state
let enqueued_messages = args.lane.queued_messages().saturating_len();

log::trace!(
target: LOG_TARGET,
"Accepted message {} to lane {:?}. Message size: {:?}",
nonce,
args.lane_id,
message_len,
);

Pallet::<T, I>::deposit_event(Event::MessageAccepted { lane_id: args.lane_id, nonce });

SendMessageArtifacts { nonce, enqueued_messages }
}
}

/// Ensure that the pallet is in normal operational mode.
Expand Down
57 changes: 26 additions & 31 deletions modules/messages/src/outbound_lane.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@
use crate::{Config, LOG_TARGET};

use bp_messages::{
ChainWithMessages, LaneId, LaneState, MessageNonce, MessagePayload, OutboundLaneData,
UnrewardedRelayer, VerificationError,
ChainWithMessages, LaneId, LaneState, MessageNonce, OutboundLaneData, UnrewardedRelayer,
};
use bp_runtime::RangeInclusiveExt;
use codec::{Decode, Encode};
Expand All @@ -31,6 +30,9 @@ use sp_std::{collections::vec_deque::VecDeque, marker::PhantomData, ops::RangeIn

/// Outbound lane storage.
pub trait OutboundLaneStorage {
/// The form under which the message payload is kept in storage.
type StoredMessagePayload;

/// Lane id.
fn id(&self) -> LaneId;
/// Get lane data from the storage.
Expand All @@ -39,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<MessagePayload>;
fn message(&self, nonce: &MessageNonce) -> Option<Self::StoredMessagePayload>;
/// 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);
/// Purge lane data from the storage.
Expand Down Expand Up @@ -81,6 +79,7 @@ pub enum ReceivalConfirmationError {
}

/// Outbound messages lane.
#[derive(Debug, PartialEq, Eq)]
pub struct OutboundLane<S> {
storage: S,
}
Expand Down Expand Up @@ -118,18 +117,15 @@ impl<S: OutboundLaneStorage> OutboundLane<S> {
/// Send message over lane.
///
/// Returns new message nonce.
pub fn send_message(
&mut self,
message_payload: MessagePayload,
) -> Result<MessageNonce, VerificationError> {
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.
Expand Down Expand Up @@ -227,7 +223,6 @@ fn ensure_unrewarded_relayers_are_correct<RelayerId>(
mod tests {
use super::*;
use crate::{active_outbound_lane, tests::mock::*};
use frame_support::assert_ok;
use sp_std::ops::RangeInclusive;

fn unrewarded_relayers(
Expand All @@ -244,9 +239,9 @@ mod tests {
) -> Result<Option<RangeInclusive<MessageNonce>>, ReceivalConfirmationError> {
run_test(|| {
let mut lane = active_outbound_lane::<TestRuntime, _>(test_lane_id()).unwrap();
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);
Expand All @@ -261,7 +256,7 @@ mod tests {
run_test(|| {
let mut lane = active_outbound_lane::<TestRuntime, _>(test_lane_id()).unwrap();
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);
});
Expand All @@ -271,9 +266,9 @@ mod tests {
fn confirm_delivery_works() {
run_test(|| {
let mut lane = active_outbound_lane::<TestRuntime, _>(test_lane_id()).unwrap();
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!(lane.storage.data().oldest_unpruned_nonce, 1);
Expand All @@ -288,9 +283,9 @@ mod tests {
fn confirm_partial_delivery_works() {
run_test(|| {
let mut lane = active_outbound_lane::<TestRuntime, _>(test_lane_id()).unwrap();
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!(lane.storage.data().oldest_unpruned_nonce, 1);
Expand All @@ -311,9 +306,9 @@ mod tests {
fn confirm_delivery_rejects_nonce_lesser_than_latest_received() {
run_test(|| {
let mut lane = active_outbound_lane::<TestRuntime, _>(test_lane_id()).unwrap();
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!(lane.storage.data().oldest_unpruned_nonce, 1);
Expand Down Expand Up @@ -388,9 +383,9 @@ mod tests {
fn confirm_delivery_detects_when_more_than_expected_messages_are_confirmed() {
run_test(|| {
let mut lane = active_outbound_lane::<TestRuntime, _>(test_lane_id()).unwrap();
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),
Expand Down
8 changes: 4 additions & 4 deletions modules/messages/src/tests/mock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use crate::{
encode_all_messages, encode_lane_data, prepare_message_delivery_storage_proof,
prepare_messages_storage_proof,
},
Config,
Config, StoredMessagePayload,
};

use bp_header_chain::{ChainWithGrandpa, StoredHeaderData};
Expand All @@ -34,7 +34,7 @@ use bp_messages::{
MessageDispatch,
},
ChainWithMessages, DeliveredMessages, InboundLaneData, LaneId, LaneState, Message, MessageKey,
MessageNonce, MessagePayload, OutboundLaneData, RelayerRewardAtSource, UnrewardedRelayer,
MessageNonce, OutboundLaneData, RelayerRewardAtSource, UnrewardedRelayer,
UnrewardedRelayersState,
};
use bp_runtime::{
Expand Down Expand Up @@ -408,8 +408,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<TestRuntime, ()> {
StoredMessagePayload::<TestRuntime, ()>::try_from(payload.encode()).expect("payload too large")
}

/// Return valid inbound (dispatch) message data, constructed from given payload.
Expand Down
Loading

0 comments on commit 446e978

Please sign in to comment.