Add messages count parameter to delivery transaction (paritytech#581)

* add messages count parameter to delivery transaction

* fix benchmarks compilation

bridges/bin/millau/runtime/src/lib.rs

@@ -313,16 +313,13 @@ parameter_types! {
 		bp_millau::MAX_UNREWARDED_RELAYER_ENTRIES_AT_INBOUND_LANE;
 	pub const MaxUnconfirmedMessagesAtInboundLane: bp_message_lane::MessageNonce =
 		bp_millau::MAX_UNCONFIRMED_MESSAGES_AT_INBOUND_LANE;
-	pub const MaxMessagesInDeliveryTransaction: bp_message_lane::MessageNonce =
-		bp_millau::MAX_MESSAGES_IN_DELIVERY_TRANSACTION;
 }
 
 impl pallet_message_lane::Config for Runtime {
 	type Event = Event;
 	type MaxMessagesToPruneAtOnce = MaxMessagesToPruneAtOnce;
 	type MaxUnrewardedRelayerEntriesAtInboundLane = MaxUnrewardedRelayerEntriesAtInboundLane;
 	type MaxUnconfirmedMessagesAtInboundLane = MaxUnconfirmedMessagesAtInboundLane;
-	type MaxMessagesInDeliveryTransaction = MaxMessagesInDeliveryTransaction;
 
 	type OutboundPayload = crate::rialto_messages::ToRialtoMessagePayload;
 	type OutboundMessageFee = Balance;

bridges/bin/millau/runtime/src/rialto_messages.rs

@@ -192,8 +192,8 @@ impl SourceHeaderChain<bp_rialto::Balance> for Rialto {
 
 	fn verify_messages_proof(
 		proof: Self::MessagesProof,
-		max_messages: MessageNonce,
+		messages_count: MessageNonce,
 	) -> Result<ProvedMessages<Message<bp_rialto::Balance>>, Self::Error> {
-		messages::target::verify_messages_proof::<WithRialtoMessageBridge, Runtime>(proof, max_messages)
+		messages::target::verify_messages_proof::<WithRialtoMessageBridge, Runtime>(proof, messages_count)
 	}
 }

bridges/bin/rialto/runtime/src/lib.rs

@@ -420,8 +420,6 @@ parameter_types! {
 		bp_millau::MAX_UNREWARDED_RELAYER_ENTRIES_AT_INBOUND_LANE;
 	pub const MaxUnconfirmedMessagesAtInboundLane: bp_message_lane::MessageNonce =
 		bp_rialto::MAX_UNCONFIRMED_MESSAGES_AT_INBOUND_LANE;
-	pub const MaxMessagesInDeliveryTransaction: bp_message_lane::MessageNonce =
-		bp_rialto::MAX_MESSAGES_IN_DELIVERY_TRANSACTION;
 }
 
 pub(crate) type WithMillauMessageLaneInstance = pallet_message_lane::DefaultInstance;
@@ -430,7 +428,6 @@ impl pallet_message_lane::Config for Runtime {
 	type MaxMessagesToPruneAtOnce = MaxMessagesToPruneAtOnce;
 	type MaxUnrewardedRelayerEntriesAtInboundLane = MaxUnrewardedRelayerEntriesAtInboundLane;
 	type MaxUnconfirmedMessagesAtInboundLane = MaxUnconfirmedMessagesAtInboundLane;
-	type MaxMessagesInDeliveryTransaction = MaxMessagesInDeliveryTransaction;
 
 	type OutboundPayload = crate::millau_messages::ToMillauMessagePayload;
 	type OutboundMessageFee = Balance;

bridges/bin/rialto/runtime/src/millau_messages.rs

@@ -192,8 +192,8 @@ impl SourceHeaderChain<bp_millau::Balance> for Millau {
 
 	fn verify_messages_proof(
 		proof: Self::MessagesProof,
-		max_messages: MessageNonce,
+		messages_count: MessageNonce,
 	) -> Result<ProvedMessages<Message<bp_millau::Balance>>, Self::Error> {
-		messages::target::verify_messages_proof::<WithMillauMessageBridge, Runtime>(proof, max_messages)
+		messages::target::verify_messages_proof::<WithMillauMessageBridge, Runtime>(proof, messages_count)
 	}
 }

bridges/bin/runtime-common/src/messages.rs

@@ -399,7 +399,7 @@ pub mod target {
 	/// Verify proof of Bridged -> This chain messages.
 	pub fn verify_messages_proof<B: MessageBridge, ThisRuntime>(
 		proof: FromBridgedChainMessagesProof<B>,
-		max_messages: MessageNonce,
+		messages_count: MessageNonce,
 	) -> Result<ProvedMessages<Message<BalanceOf<BridgedChain<B>>>>, &'static str>
 	where
 		ThisRuntime: pallet_substrate_bridge::Config,
@@ -409,7 +409,7 @@ pub mod target {
 	{
 		verify_messages_proof_with_parser::<B, _, _>(
 			proof,
-			max_messages,
+			messages_count,
 			|bridged_header_hash, bridged_storage_proof| {
 				pallet_substrate_bridge::Module::<ThisRuntime>::parse_finalized_storage_proof(
 					bridged_header_hash.into(),
@@ -429,7 +429,7 @@ pub mod target {
 	#[derive(Debug, PartialEq)]
 	pub(crate) enum MessageProofError {
 		Empty,
-		TooManyMessages,
+		MessagesCountMismatch,
 		MissingRequiredMessage,
 		FailedToDecodeMessage,
 		FailedToDecodeOutboundLaneState,
@@ -440,7 +440,7 @@ pub mod target {
 		fn from(err: MessageProofError) -> &'static str {
 			match err {
 				MessageProofError::Empty => "Messages proof is empty",
-				MessageProofError::TooManyMessages => "Too many messages in the proof",
+				MessageProofError::MessagesCountMismatch => "Declared messages count doesn't match actual value",
 				MessageProofError::MissingRequiredMessage => "Message is missing from the proof",
 				MessageProofError::FailedToDecodeMessage => "Failed to decode message from the proof",
 				MessageProofError::FailedToDecodeOutboundLaneState => {
@@ -488,7 +488,7 @@ pub mod target {
 	/// Verify proof of Bridged -> This chain messages using given message proof parser.
 	pub(crate) fn verify_messages_proof_with_parser<B: MessageBridge, BuildParser, Parser>(
 		proof: FromBridgedChainMessagesProof<B>,
-		max_messages: MessageNonce,
+		messages_count: MessageNonce,
 		build_parser: BuildParser,
 	) -> Result<ProvedMessages<Message<BalanceOf<BridgedChain<B>>>>, MessageProofError>
 	where
@@ -500,8 +500,8 @@ pub mod target {
 		// receiving proofs where end < begin is ok (if proof includes outbound lane state)
 		// => hence unwrap_or(0)
 		let messages_in_the_proof = end.checked_sub(begin).and_then(|diff| diff.checked_add(1)).unwrap_or(0);
-		if messages_in_the_proof > max_messages {
-			return Err(MessageProofError::TooManyMessages);
+		if messages_in_the_proof != messages_count {
+			return Err(MessageProofError::MessagesCountMismatch);
 		}
 
 		let parser = build_parser(bridged_header_hash, bridged_storage_proof)?;
@@ -1009,14 +1009,26 @@ mod tests {
 	}
 
 	#[test]
-	fn messages_proof_is_rejected_if_there_are_too_many_messages() {
+	fn messages_proof_is_rejected_if_declared_less_than_actual_number_of_messages() {
 		assert_eq!(
 			target::verify_messages_proof_with_parser::<OnThisChainBridge, _, TestMessageProofParser>(
-				(Default::default(), StorageProof::new(vec![]), Default::default(), 1, 11),
-				10,
+				(Default::default(), StorageProof::new(vec![]), Default::default(), 1, 10),
+				5,
 				|_, _| unreachable!(),
 			),
-			Err(target::MessageProofError::TooManyMessages),
+			Err(target::MessageProofError::MessagesCountMismatch),
+		);
+	}
+
+	#[test]
+	fn messages_proof_is_rejected_if_declared_more_than_actual_number_of_messages() {
+		assert_eq!(
+			target::verify_messages_proof_with_parser::<OnThisChainBridge, _, TestMessageProofParser>(
+				(Default::default(), StorageProof::new(vec![]), Default::default(), 1, 10),
+				15,
+				|_, _| unreachable!(),
+			),
+			Err(target::MessageProofError::MessagesCountMismatch),
 		);
 	}
 
@@ -1069,7 +1081,7 @@ mod tests {
 		assert_eq!(
 			target::verify_messages_proof_with_parser::<OnThisChainBridge, _, _>(
 				(Default::default(), StorageProof::new(vec![]), Default::default(), 1, 0),
-				10,
+				0,
 				|_, _| Ok(TestMessageProofParser {
 					failing: true,
 					messages: no_messages_range(),
@@ -1089,7 +1101,7 @@ mod tests {
 		assert_eq!(
 			target::verify_messages_proof_with_parser::<OnThisChainBridge, _, _>(
 				(Default::default(), StorageProof::new(vec![]), Default::default(), 1, 0),
-				10,
+				0,
 				|_, _| Ok(TestMessageProofParser {
 					failing: false,
 					messages: no_messages_range(),
@@ -1105,7 +1117,7 @@ mod tests {
 		assert_eq!(
 			target::verify_messages_proof_with_parser::<OnThisChainBridge, _, _>(
 				(Default::default(), StorageProof::new(vec![]), Default::default(), 1, 0),
-				10,
+				0,
 				|_, _| Ok(TestMessageProofParser {
 					failing: false,
 					messages: no_messages_range(),
@@ -1137,7 +1149,7 @@ mod tests {
 		assert_eq!(
 			target::verify_messages_proof_with_parser::<OnThisChainBridge, _, _>(
 				(Default::default(), StorageProof::new(vec![]), Default::default(), 1, 1),
-				10,
+				1,
 				|_, _| Ok(TestMessageProofParser {
 					failing: false,
 					messages: 1..=1,

bridges/modules/message-lane/src/benchmarking.rs

@@ -25,7 +25,7 @@ use frame_benchmarking::{account, benchmarks_instance};
 use frame_support::{traits::Get, weights::Weight};
 use frame_system::RawOrigin;
 use num_traits::Zero;
-use sp_std::{convert::TryInto, ops::RangeInclusive, prelude::*};
+use sp_std::{ops::RangeInclusive, prelude::*};
 
 /// Message crafted with this size factor should be the largest possible message.
 pub const WORST_MESSAGE_SIZE_FACTOR: u32 = 1000;
@@ -127,7 +127,7 @@ benchmarks_instance! {
 			message_nonces: 1..=1,
 			outbound_lane_data: None,
 		});
-	}: receive_messages_proof(RawOrigin::Signed(relayer_id_on_target), relayer_id_on_source, proof, dispatch_weight)
+	}: receive_messages_proof(RawOrigin::Signed(relayer_id_on_target), relayer_id_on_source, proof, 1, dispatch_weight)
 	verify {
 		assert_eq!(
 			crate::Module::<T, I>::inbound_latest_received_nonce(bench_lane_id()),
@@ -154,7 +154,7 @@ benchmarks_instance! {
 			message_nonces: 1..=2,
 			outbound_lane_data: None,
 		});
-	}: receive_messages_proof(RawOrigin::Signed(relayer_id_on_target), relayer_id_on_source, proof, dispatch_weight)
+	}: receive_messages_proof(RawOrigin::Signed(relayer_id_on_target), relayer_id_on_source, proof, 2, dispatch_weight)
 	verify {
 		assert_eq!(
 			crate::Module::<T, I>::inbound_latest_received_nonce(bench_lane_id()),
@@ -188,7 +188,7 @@ benchmarks_instance! {
 				latest_generated_nonce: 21,
 			}),
 		});
-	}: receive_messages_proof(RawOrigin::Signed(relayer_id_on_target), relayer_id_on_source, proof, dispatch_weight)
+	}: receive_messages_proof(RawOrigin::Signed(relayer_id_on_target), relayer_id_on_source, proof, 1, dispatch_weight)
 	verify {
 		assert_eq!(
 			crate::Module::<T, I>::inbound_latest_received_nonce(bench_lane_id()),
@@ -214,19 +214,24 @@ benchmarks_instance! {
 	// `weight(receive_two_messages_proof) - weight(receive_single_message_proof)`. So it may be used
 	// to verify that the other approximation is correct.
 	receive_multiple_messages_proof {
-		let i in 1..T::MaxMessagesInDeliveryTransaction::get()
-			.try_into()
-			.expect("Value of MaxMessagesInDeliveryTransaction is too large");
+		let i in 1..128;
 
 		let relayer_id_on_source = T::bridged_relayer_id();
 		let relayer_id_on_target = account("relayer", 0, SEED);
+		let messages_count = i as _;
 
 		let (proof, dispatch_weight) = T::prepare_message_proof(MessageProofParams {
 			lane: bench_lane_id(),
 			message_nonces: 1..=i as _,
 			outbound_lane_data: None,
 		});
-	}: receive_messages_proof(RawOrigin::Signed(relayer_id_on_target), relayer_id_on_source, proof, dispatch_weight)
+	}: receive_messages_proof(
+		RawOrigin::Signed(relayer_id_on_target),
+		relayer_id_on_source,
+		proof,
+		messages_count,
+		dispatch_weight
+	)
 	verify {
 		assert_eq!(
 			crate::Module::<T, I>::inbound_latest_received_nonce(bench_lane_id()),
@@ -244,12 +249,11 @@ benchmarks_instance! {
 	// `weight(receive_single_message_proof_with_outbound_lane_state) - weight(receive_single_message_proof)`.
 	// So it may be used to verify that the other approximation is correct.
 	receive_multiple_messages_proof_with_outbound_lane_state {
-		let i in 1..T::MaxMessagesInDeliveryTransaction::get()
-			.try_into()
-			.expect("Value of MaxMessagesInDeliveryTransaction is too large");
+		let i in 1..128;
 
 		let relayer_id_on_source = T::bridged_relayer_id();
 		let relayer_id_on_target = account("relayer", 0, SEED);
+		let messages_count = i as _;
 
 		// mark messages 1..=20 as delivered
 		receive_messages::<T, I>(20);
@@ -263,7 +267,13 @@ benchmarks_instance! {
 				latest_generated_nonce: 21,
 			}),
 		});
-	}: receive_messages_proof(RawOrigin::Signed(relayer_id_on_target), relayer_id_on_source, proof, dispatch_weight)
+	}: receive_messages_proof(
+		RawOrigin::Signed(relayer_id_on_target),
+		relayer_id_on_source,
+		proof,
+		messages_count,
+		dispatch_weight
+	)
 	verify {
 		assert_eq!(
 			crate::Module::<T, I>::inbound_latest_received_nonce(bench_lane_id()),

bridges/modules/message-lane/src/lib.rs

@@ -93,11 +93,6 @@ pub trait Config<I = DefaultInstance>: frame_system::Config {
 	/// This constant limits difference between last message from last entry of the
 	/// `InboundLaneData::relayers` and first message at the first entry.
 	type MaxUnconfirmedMessagesAtInboundLane: Get<MessageNonce>;
-	/// Maximal number of messages in single delivery transaction. This directly affects the base
-	/// weight of the delivery transaction.
-	///
-	/// All transactions that deliver more messages than this number, are rejected.
-	type MaxMessagesInDeliveryTransaction: Get<MessageNonce>;
 
 	/// Payload type of outbound messages. This payload is dispatched on the bridged chain.
 	type OutboundPayload: Parameter;
@@ -322,17 +317,16 @@ decl_module! {
 		}
 
 		/// Receive messages proof from bridged chain.
-		#[weight = DELIVERY_OVERHEAD_WEIGHT
-			.saturating_add(
-				T::MaxMessagesInDeliveryTransaction::get()
-					.saturating_mul(SINGLE_MESSAGE_DELIVERY_WEIGHT)
-			)
+		#[weight = messages_count
+			.saturating_mul(SINGLE_MESSAGE_DELIVERY_WEIGHT)
+			.saturating_add(DELIVERY_OVERHEAD_WEIGHT)
 			.saturating_add(*dispatch_weight)
 		]
 		pub fn receive_messages_proof(
 			origin,
 			relayer_id: T::InboundRelayer,
 			proof: MessagesProofOf<T, I>,
+			messages_count: MessageNonce,
 			dispatch_weight: Weight,
 		) -> DispatchResult {
 			ensure_operational::<T, I>()?;
@@ -343,7 +337,7 @@ decl_module! {
 				T::SourceHeaderChain,
 				T::InboundMessageFee,
 				T::InboundPayload,
-			>(proof, T::MaxMessagesInDeliveryTransaction::get())
+			>(proof, messages_count)
 				.map_err(|err| {
 					frame_support::debug::trace!(
 						"Rejecting invalid messages proof: {:?}",
@@ -674,9 +668,9 @@ impl<T: Config<I>, I: Instance> OutboundLaneStorage for RuntimeOutboundLaneStora
 /// Verify messages proof and return proved messages with decoded payload.
 fn verify_and_decode_messages_proof<Chain: SourceHeaderChain<Fee>, Fee, DispatchPayload: Decode>(
 	proof: Chain::MessagesProof,
-	max_messages: MessageNonce,
+	messages_count: MessageNonce,
 ) -> Result<ProvedMessages<DispatchMessage<DispatchPayload, Fee>>, Chain::Error> {
-	Chain::verify_messages_proof(proof, max_messages).map(|messages_by_lane| {
+	Chain::verify_messages_proof(proof, messages_count).map(|messages_by_lane| {
 		messages_by_lane
 			.into_iter()
 			.map(|(lane, lane_data)| {
@@ -846,6 +840,7 @@ mod tests {
 					Origin::signed(1),
 					TEST_RELAYER_A,
 					Ok(vec![message(2, REGULAR_PAYLOAD)]).into(),
+					1,
 					REGULAR_PAYLOAD.1,
 				),
 				Error::<TestRuntime, DefaultInstance>::Halted,
@@ -924,6 +919,7 @@ mod tests {
 				Origin::signed(1),
 				TEST_RELAYER_A,
 				Ok(vec![message(1, REGULAR_PAYLOAD)]).into(),
+				1,
 				REGULAR_PAYLOAD.1,
 			));
 
@@ -964,6 +960,7 @@ mod tests {
 				Origin::signed(1),
 				TEST_RELAYER_A,
 				message_proof,
+				1,
 				REGULAR_PAYLOAD.1,
 			));
 
@@ -995,6 +992,7 @@ mod tests {
 					Origin::signed(1),
 					TEST_RELAYER_A,
 					Ok(vec![message(1, REGULAR_PAYLOAD)]).into(),
+					1,
 					REGULAR_PAYLOAD.1 - 1,
 				),
 				Error::<TestRuntime, DefaultInstance>::InvalidMessagesDispatchWeight,
@@ -1010,6 +1008,7 @@ mod tests {
 					Origin::signed(1),
 					TEST_RELAYER_A,
 					Err(()).into(),
+					1,
 					0,
 				),
 				Error::<TestRuntime, DefaultInstance>::InvalidMessagesProof,
@@ -1112,6 +1111,7 @@ mod tests {
 				Origin::signed(1),
 				TEST_RELAYER_A,
 				Ok(vec![invalid_message]).into(),
+				1,
 				0, // weight may be zero in this case (all messages are improperly encoded)
 			),);
 
@@ -1134,6 +1134,7 @@ mod tests {
 					message(3, REGULAR_PAYLOAD),
 				])
 				.into(),
+				3,
 				REGULAR_PAYLOAD.1 + REGULAR_PAYLOAD.1,
 			),);