Remove without_storage_info for messages pallet (#1487)

* draft: remove without_storage_info for messages pallet

* some cleanup

bridges/modules/messages/src/inbound_lane.rs

@@ -16,13 +16,17 @@
 
 //! Everything about incoming messages receival.
 
+use crate::Config;
+
 use bp_messages::{
 	target_chain::{DispatchMessage, DispatchMessageData, MessageDispatch},
 	DeliveredMessages, InboundLaneData, LaneId, MessageKey, MessageNonce, OutboundLaneData,
 	UnrewardedRelayer,
 };
 use bp_runtime::messages::MessageDispatchResult;
-use frame_support::RuntimeDebug;
+use codec::{Decode, Encode, EncodeLike, MaxEncodedLen};
+use frame_support::{traits::Get, RuntimeDebug};
+use scale_info::{Type, TypeInfo};
 use sp_std::prelude::PartialEq;
 
 /// Inbound lane storage.
@@ -44,6 +48,76 @@ pub trait InboundLaneStorage {
 	fn set_data(&mut self, data: InboundLaneData<Self::Relayer>);
 }
 
+/// Inbound lane data wrapper that implements `MaxEncodedLen`.
+///
+/// We have already had `MaxEncodedLen`-like functionality before, but its usage has
+/// been localized and we haven't been passing bounds (maximal count of unrewarded relayer entries,
+/// maximal count of unconfirmed messages) everywhere. This wrapper allows us to avoid passing
+/// these generic bounds all over the code.
+///
+/// The encoding of this type matches encoding of the corresponding `MessageData`.
+#[derive(Encode, Decode, Clone, RuntimeDebug, PartialEq, Eq)]
+pub struct StoredInboundLaneData<T: Config<I>, I: 'static>(pub InboundLaneData<T::InboundRelayer>);
+
+impl<T: Config<I>, I: 'static> sp_std::ops::Deref for StoredInboundLaneData<T, I> {
+	type Target = InboundLaneData<T::InboundRelayer>;
+
+	fn deref(&self) -> &Self::Target {
+		&self.0
+	}
+}
+
+impl<T: Config<I>, I: 'static> sp_std::ops::DerefMut for StoredInboundLaneData<T, I> {
+	fn deref_mut(&mut self) -> &mut Self::Target {
+		&mut self.0
+	}
+}
+
+impl<T: Config<I>, I: 'static> Default for StoredInboundLaneData<T, I> {
+	fn default() -> Self {
+		StoredInboundLaneData(Default::default())
+	}
+}
+
+impl<T: Config<I>, I: 'static> From<InboundLaneData<T::InboundRelayer>>
+	for StoredInboundLaneData<T, I>
+{
+	fn from(data: InboundLaneData<T::InboundRelayer>) -> Self {
+		StoredInboundLaneData(data)
+	}
+}
+
+impl<T: Config<I>, I: 'static> From<StoredInboundLaneData<T, I>>
+	for InboundLaneData<T::InboundRelayer>
+{
+	fn from(data: StoredInboundLaneData<T, I>) -> Self {
+		data.0
+	}
+}
+
+impl<T: Config<I>, I: 'static> EncodeLike<StoredInboundLaneData<T, I>>
+	for InboundLaneData<T::InboundRelayer>
+{
+}
+
+impl<T: Config<I>, I: 'static> TypeInfo for StoredInboundLaneData<T, I> {
+	type Identity = Self;
+
+	fn type_info() -> Type {
+		InboundLaneData::<T::InboundRelayer>::type_info()
+	}
+}
+
+impl<T: Config<I>, I: 'static> MaxEncodedLen for StoredInboundLaneData<T, I> {
+	fn max_encoded_len() -> usize {
+		InboundLaneData::<T::InboundRelayer>::encoded_size_hint(
+			T::MaxUnrewardedRelayerEntriesAtInboundLane::get() as usize,
+			T::MaxUnconfirmedMessagesAtInboundLane::get() as usize,
+		)
+		.unwrap_or(usize::MAX)
+	}
+}
+
 /// Result of single message receival.
 #[derive(RuntimeDebug, PartialEq, Eq)]
 pub enum ReceivalResult {
@@ -333,7 +407,7 @@ mod tests {
 		run_test(|| {
 			let mut lane = inbound_lane::<TestRuntime, _>(TEST_LANE_ID);
 			let max_nonce =
-				<TestRuntime as crate::Config>::MaxUnrewardedRelayerEntriesAtInboundLane::get();
+				<TestRuntime as Config>::MaxUnrewardedRelayerEntriesAtInboundLane::get();
 			for current_nonce in 1..max_nonce + 1 {
 				assert_eq!(
 					lane.receive_message::<TestMessageDispatch, _>(
@@ -372,8 +446,7 @@ mod tests {
 	fn fails_to_receive_messages_above_unconfirmed_messages_limit_per_lane() {
 		run_test(|| {
 			let mut lane = inbound_lane::<TestRuntime, _>(TEST_LANE_ID);
-			let max_nonce =
-				<TestRuntime as crate::Config>::MaxUnconfirmedMessagesAtInboundLane::get();
+			let max_nonce = <TestRuntime as Config>::MaxUnconfirmedMessagesAtInboundLane::get();
 			for current_nonce in 1..=max_nonce {
 				assert_eq!(
 					lane.receive_message::<TestMessageDispatch, _>(

bridges/modules/messages/src/lib.rs

@@ -37,6 +37,8 @@
 // Generated by `decl_event!`
 #![allow(clippy::unused_unit)]
 
+pub use inbound_lane::StoredInboundLaneData;
+pub use outbound_lane::StoredMessageData;
 pub use weights::WeightInfo;
 pub use weights_ext::{
 	ensure_able_to_receive_confirmation, ensure_able_to_receive_message,
@@ -62,9 +64,9 @@ use bp_messages::{
 	UnrewardedRelayersState,
 };
 use bp_runtime::{BasicOperatingMode, ChainId, OwnedBridgeModule, Size};
-use codec::{Decode, Encode};
+use codec::{Decode, Encode, MaxEncodedLen};
 use frame_support::{
-	fail,
+	ensure, fail,
 	traits::Get,
 	weights::{Pays, PostDispatchInfo},
 };
@@ -145,6 +147,9 @@ pub mod pallet {
 		/// these messages are from different lanes.
 		type MaxUnconfirmedMessagesAtInboundLane: Get<MessageNonce>;
 
+		/// Maximal size of the outbound payload.
+		#[pallet::constant]
+		type MaximalOutboundPayloadSize: Get<u32>;
 		/// Payload type of outbound messages. This payload is dispatched on the bridged chain.
 		type OutboundPayload: Parameter + Size;
 		/// Message fee type of outbound messages. This fee is paid on this chain.
@@ -154,7 +159,8 @@ pub mod pallet {
 			+ Parameter
 			+ SaturatingAdd
 			+ Zero
-			+ Copy;
+			+ Copy
+			+ MaxEncodedLen;
 
 		/// Payload type of inbound messages. This payload is dispatched on this chain.
 		type InboundPayload: Decode;
@@ -162,7 +168,7 @@ pub mod pallet {
 		type InboundMessageFee: Decode + Zero;
 		/// Identifier of relayer that deliver messages to this chain. Relayer reward is paid on the
 		/// bridged chain.
-		type InboundRelayer: Parameter;
+		type InboundRelayer: Parameter + MaxEncodedLen;
 
 		/// A type which can be turned into an AccountId from a 256-bit hash.
 		///
@@ -216,7 +222,6 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
-	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(PhantomData<(T, I)>);
 
 	impl<T: Config<I>, I: 'static> OwnedBridgeModule<T> for Pallet<T, I> {
@@ -658,6 +663,8 @@ pub mod pallet {
 	pub enum Error<T, I = ()> {
 		/// Pallet is not in Normal operating mode.
 		NotOperatingNormally,
+		/// The message is too large to be sent over the bridge.
+		MessageIsTooLarge,
 		/// Message has been treated as invalid by chain verifier.
 		MessageRejectedByChainVerifier,
 		/// Message has been treated as invalid by lane verifier.
@@ -707,7 +714,7 @@ pub mod pallet {
 	/// Map of lane id => inbound lane data.
 	#[pallet::storage]
 	pub type InboundLanes<T: Config<I>, I: 'static = ()> =
-		StorageMap<_, Blake2_128Concat, LaneId, InboundLaneData<T::InboundRelayer>, ValueQuery>;
+		StorageMap<_, Blake2_128Concat, LaneId, StoredInboundLaneData<T, I>, ValueQuery>;
 
 	/// Map of lane id => outbound lane data.
 	#[pallet::storage]
@@ -717,7 +724,7 @@ pub mod pallet {
 	/// All queued outbound messages.
 	#[pallet::storage]
 	pub type OutboundMessages<T: Config<I>, I: 'static = ()> =
-		StorageMap<_, Blake2_128Concat, MessageKey, MessageData<T::OutboundMessageFee>>;
+		StorageMap<_, Blake2_128Concat, MessageKey, StoredMessageData<T, I>>;
 
 	#[pallet::genesis_config]
 	pub struct GenesisConfig<T: Config<I>, I: 'static = ()> {
@@ -756,7 +763,7 @@ pub mod pallet {
 			lane: LaneId,
 			nonce: MessageNonce,
 		) -> Option<MessageData<T::OutboundMessageFee>> {
-			OutboundMessages::<T, I>::get(MessageKey { lane_id: lane, nonce })
+			OutboundMessages::<T, I>::get(MessageKey { lane_id: lane, nonce }).map(Into::into)
 		}
 
 		/// Prepare data, related to given inbound message.
@@ -822,6 +829,12 @@ fn send_message<T: Config<I>, I: 'static>(
 > {
 	ensure_normal_operating_mode::<T, I>()?;
 
+	// the most lightweigh check is the message size check
+	ensure!(
+		payload.size() < T::MaximalOutboundPayloadSize::get(),
+		Error::<T, I>::MessageIsTooLarge,
+	);
+
 	// initially, actual (post-dispatch) weight is equal to pre-dispatch weight
 	let mut actual_weight = T::WeightInfo::send_message_weight(&payload, T::DbWeight::get());
 
@@ -955,7 +968,8 @@ where
 		// this loop is bound by `T::MaxUnconfirmedMessagesAtInboundLane` on the bridged chain
 		let mut relayer_reward = relayers_rewards.entry(entry.relayer).or_default();
 		for nonce in nonce_begin..nonce_end + 1 {
-			let message_data = OutboundMessages::<T, I>::get(MessageKey { lane_id, nonce })
+			let key = MessageKey { lane_id, nonce };
+			let message_data = OutboundMessages::<T, I>::get(key)
 				.expect("message was just confirmed; we never prune unconfirmed messages; qed");
 			relayer_reward.reward = relayer_reward.reward.saturating_add(&message_data.fee);
 			relayer_reward.messages += 1;
@@ -1027,7 +1041,8 @@ impl<T: Config<I>, I: 'static> InboundLaneStorage for RuntimeInboundLaneStorage<
 		match self.cached_data.clone().into_inner() {
 			Some(data) => data,
 			None => {
-				let data = InboundLanes::<T, I>::get(&self.lane_id);
+				let data: InboundLaneData<T::InboundRelayer> =
+					InboundLanes::<T, I>::get(&self.lane_id).into();
 				*self.cached_data.try_borrow_mut().expect(
 					"we're in the single-threaded environment;\
 						we have no recursive borrows; qed",
@@ -1042,7 +1057,7 @@ impl<T: Config<I>, I: 'static> InboundLaneStorage for RuntimeInboundLaneStorage<
 			"we're in the single-threaded environment;\
 				we have no recursive borrows; qed",
 		) = Some(data.clone());
-		InboundLanes::<T, I>::insert(&self.lane_id, data)
+		InboundLanes::<T, I>::insert(&self.lane_id, StoredInboundLaneData::<T, I>(data))
 	}
 }
 
@@ -1070,6 +1085,7 @@ impl<T: Config<I>, I: 'static> OutboundLaneStorage for RuntimeOutboundLaneStorag
 	#[cfg(test)]
 	fn message(&self, nonce: &MessageNonce) -> Option<MessageData<T::OutboundMessageFee>> {
 		OutboundMessages::<T, I>::get(MessageKey { lane_id: self.lane_id, nonce: *nonce })
+			.map(Into::into)
 	}
 
 	fn save_message(
@@ -1116,8 +1132,9 @@ mod tests {
 		message, message_payload, run_test, unrewarded_relayer, Event as TestEvent, Origin,
 		TestMessageDeliveryAndDispatchPayment, TestMessagesDeliveryProof, TestMessagesParameter,
 		TestMessagesProof, TestOnDeliveryConfirmed1, TestOnDeliveryConfirmed2,
-		TestOnMessageAccepted, TestRuntime, TokenConversionRate, PAYLOAD_REJECTED_BY_TARGET_CHAIN,
-		REGULAR_PAYLOAD, TEST_LANE_ID, TEST_RELAYER_A, TEST_RELAYER_B,
+		TestOnMessageAccepted, TestRuntime, TokenConversionRate, MAX_OUTBOUND_PAYLOAD_SIZE,
+		PAYLOAD_REJECTED_BY_TARGET_CHAIN, REGULAR_PAYLOAD, TEST_LANE_ID, TEST_RELAYER_A,
+		TEST_RELAYER_B,
 	};
 	use bp_messages::{UnrewardedRelayer, UnrewardedRelayersState};
 	use bp_test_utils::generate_owned_bridge_module_tests;
@@ -1137,7 +1154,7 @@ mod tests {
 	fn inbound_unrewarded_relayers_state(
 		lane: bp_messages::LaneId,
 	) -> bp_messages::UnrewardedRelayersState {
-		let inbound_lane_data = InboundLanes::<TestRuntime, ()>::get(&lane);
+		let inbound_lane_data = InboundLanes::<TestRuntime, ()>::get(&lane).0;
 		let last_delivered_nonce = inbound_lane_data.last_delivered_nonce();
 		let relayers = inbound_lane_data.relayers;
 		bp_messages::UnrewardedRelayersState {
@@ -1443,6 +1460,27 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn send_message_rejects_too_large_message() {
+		run_test(|| {
+			let mut message_payload = message_payload(1, 0);
+			// the payload isn't simply extra, so it'll definitely overflow
+			// `MAX_OUTBOUND_PAYLOAD_SIZE` if we add `MAX_OUTBOUND_PAYLOAD_SIZE` bytes to extra
+			message_payload
+				.extra
+				.extend_from_slice(&[0u8; MAX_OUTBOUND_PAYLOAD_SIZE as usize]);
+			assert_noop!(
+				Pallet::<TestRuntime>::send_message(
+					Origin::signed(1),
+					TEST_LANE_ID,
+					message_payload,
+					0,
+				),
+				Error::<TestRuntime, ()>::MessageIsTooLarge,
+			);
+		})
+	}
+
 	#[test]
 	fn chain_verifier_rejects_invalid_message_in_send_message() {
 		run_test(|| {
@@ -1502,7 +1540,7 @@ mod tests {
 				REGULAR_PAYLOAD.declared_weight,
 			));
 
-			assert_eq!(InboundLanes::<TestRuntime>::get(TEST_LANE_ID).last_delivered_nonce(), 1);
+			assert_eq!(InboundLanes::<TestRuntime>::get(TEST_LANE_ID).0.last_delivered_nonce(), 1);
 		});
 	}
 
@@ -1547,7 +1585,7 @@ mod tests {
 			));
 
 			assert_eq!(
-				InboundLanes::<TestRuntime>::get(TEST_LANE_ID),
+				InboundLanes::<TestRuntime>::get(TEST_LANE_ID).0,
 				InboundLaneData {
 					last_confirmed_nonce: 9,
 					relayers: vec![
@@ -2146,8 +2184,8 @@ mod tests {
 		run_test(|| {
 			let mut small_payload = message_payload(0, 100);
 			let mut large_payload = message_payload(1, 100);
-			small_payload.extra = vec![1; 100];
-			large_payload.extra = vec![2; 16_384];
+			small_payload.extra = vec![1; MAX_OUTBOUND_PAYLOAD_SIZE as usize / 10];
+			large_payload.extra = vec![2; MAX_OUTBOUND_PAYLOAD_SIZE as usize / 5];
 
 			assert_ok!(Pallet::<TestRuntime>::send_message(
 				Origin::signed(1),

bridges/modules/messages/src/mock.rs

@@ -174,6 +174,7 @@ impl Config for TestRuntime {
 	type MaxUnrewardedRelayerEntriesAtInboundLane = MaxUnrewardedRelayerEntriesAtInboundLane;
 	type MaxUnconfirmedMessagesAtInboundLane = MaxUnconfirmedMessagesAtInboundLane;
 
+	type MaximalOutboundPayloadSize = frame_support::traits::ConstU32<MAX_OUTBOUND_PAYLOAD_SIZE>;
 	type OutboundPayload = TestPayload;
 	type OutboundMessageFee = TestMessageFee;
 
@@ -205,11 +206,14 @@ impl SenderOrigin<AccountId> for Origin {
 }
 
 impl Size for TestPayload {
-	fn size_hint(&self) -> u32 {
+	fn size(&self) -> u32 {
 		16 + self.extra.len() as u32
 	}
 }
 
+/// Maximal outbound payload size.
+pub const MAX_OUTBOUND_PAYLOAD_SIZE: u32 = 4096;
+
 /// Account that has balance to use in tests.
 pub const ENDOWED_ACCOUNT: AccountId = 0xDEAD;
 
@@ -244,7 +248,7 @@ pub struct TestMessagesProof {
 }
 
 impl Size for TestMessagesProof {
-	fn size_hint(&self) -> u32 {
+	fn size(&self) -> u32 {
 		0
 	}
 }
@@ -271,7 +275,7 @@ impl From<Result<Vec<Message<TestMessageFee>>, ()>> for TestMessagesProof {
 pub struct TestMessagesDeliveryProof(pub Result<(LaneId, InboundLaneData<TestRelayer>), ()>);
 
 impl Size for TestMessagesDeliveryProof {
-	fn size_hint(&self) -> u32 {
+	fn size(&self) -> u32 {
 		0
 	}
 }

bridges/modules/messages/src/outbound_lane.rs

@@ -16,12 +16,16 @@
 
 //! Everything about outgoing messages sending.
 
+use crate::Config;
+
 use bitvec::prelude::*;
 use bp_messages::{
 	DeliveredMessages, DispatchResultsBitVec, LaneId, MessageData, MessageNonce, OutboundLaneData,
 	UnrewardedRelayer,
 };
-use frame_support::RuntimeDebug;
+use codec::{Decode, Encode, EncodeLike, MaxEncodedLen};
+use frame_support::{traits::Get, RuntimeDebug};
+use scale_info::{Type, TypeInfo};
 use sp_std::collections::vec_deque::VecDeque;
 
 /// Outbound lane storage.
@@ -44,6 +48,66 @@ pub trait OutboundLaneStorage {
 	fn remove_message(&mut self, nonce: &MessageNonce);
 }
 
+/// Outbound message data wrapper that implements `MaxEncodedLen`.
+///
+/// We have already had `MaxEncodedLen`-like functionality before, but its usage has
+/// been localized and we haven't been passing it everywhere. This wrapper allows us
+/// to avoid passing these generic bounds all over the code.
+///
+/// The encoding of this type matches encoding of the corresponding `MessageData`.
+#[derive(Encode, Decode, Clone, RuntimeDebug, PartialEq, Eq)]
+pub struct StoredMessageData<T: Config<I>, I: 'static>(pub MessageData<T::OutboundMessageFee>);
+
+impl<T: Config<I>, I: 'static> sp_std::ops::Deref for StoredMessageData<T, I> {
+	type Target = MessageData<T::OutboundMessageFee>;
+
+	fn deref(&self) -> &Self::Target {
+		&self.0
+	}
+}
+
+impl<T: Config<I>, I: 'static> sp_std::ops::DerefMut for StoredMessageData<T, I> {
+	fn deref_mut(&mut self) -> &mut Self::Target {
+		&mut self.0
+	}
+}
+
+impl<T: Config<I>, I: 'static> From<MessageData<T::OutboundMessageFee>>
+	for StoredMessageData<T, I>
+{
+	fn from(data: MessageData<T::OutboundMessageFee>) -> Self {
+		StoredMessageData(data)
+	}
+}
+
+impl<T: Config<I>, I: 'static> From<StoredMessageData<T, I>>
+	for MessageData<T::OutboundMessageFee>
+{
+	fn from(data: StoredMessageData<T, I>) -> Self {
+		data.0
+	}
+}
+
+impl<T: Config<I>, I: 'static> TypeInfo for StoredMessageData<T, I> {
+	type Identity = Self;
+
+	fn type_info() -> Type {
+		MessageData::<T::OutboundMessageFee>::type_info()
+	}
+}
+
+impl<T: Config<I>, I: 'static> EncodeLike<StoredMessageData<T, I>>
+	for MessageData<T::OutboundMessageFee>
+{
+}
+
+impl<T: Config<I>, I: 'static> MaxEncodedLen for StoredMessageData<T, I> {
+	fn max_encoded_len() -> usize {
+		T::OutboundMessageFee::max_encoded_len()
+			.saturating_add(T::MaximalOutboundPayloadSize::get() as usize)
+	}
+}
+
 /// Result of messages receival confirmation.
 #[derive(RuntimeDebug, PartialEq, Eq)]
 pub enum ReceivalConfirmationResult {

bridges/modules/messages/src/weights_ext.rs

@@ -199,7 +199,7 @@ pub trait WeightInfoExt: WeightInfo {
 	/// Weight of message send extrinsic.
 	fn send_message_weight(message: &impl Size, db_weight: RuntimeDbWeight) -> Weight {
 		let transaction_overhead = Self::send_message_overhead();
-		let message_size_overhead = Self::send_message_size_overhead(message.size_hint());
+		let message_size_overhead = Self::send_message_size_overhead(message.size());
 		let call_back_overhead = Self::single_message_callback_overhead(db_weight);
 
 		transaction_overhead
@@ -225,7 +225,7 @@ pub trait WeightInfoExt: WeightInfo {
 		let expected_proof_size = EXPECTED_DEFAULT_MESSAGE_LENGTH
 			.saturating_mul(messages_count.saturating_sub(1))
 			.saturating_add(Self::expected_extra_storage_proof_size());
-		let actual_proof_size = proof.size_hint();
+		let actual_proof_size = proof.size();
 		let proof_size_overhead = Self::storage_proof_size_overhead(
 			actual_proof_size.saturating_sub(expected_proof_size),
 		);
@@ -253,7 +253,7 @@ pub trait WeightInfoExt: WeightInfo {
 
 		// proof size overhead weight
 		let expected_proof_size = Self::expected_extra_storage_proof_size();
-		let actual_proof_size = proof.size_hint();
+		let actual_proof_size = proof.size();
 		let proof_size_overhead = Self::storage_proof_size_overhead(
 			actual_proof_size.saturating_sub(expected_proof_size),
 		);