Fix XCMP max message size check (#1250)

Improved max message size logic and added test for it

---------

Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: command-bot <>

cumulus/pallets/xcmp-queue/src/lib.rs

@@ -454,11 +454,21 @@ impl<T: Config> Pallet<T> {
 	) -> Result<u32, MessageSendError> {
 		let encoded_fragment = fragment.encode();
 
+		// Optimization note: `max_message_size` could potentially be stored in
+		// `OutboundXcmpMessages` once known; that way it's only accessed when a new page is needed.
+
 		let channel_info =
 			T::ChannelInfo::get_channel_info(recipient).ok_or(MessageSendError::NoChannel)?;
-		let max_message_size = channel_info.max_message_size as usize;
 		// Max message size refers to aggregates, or pages. Not to individual fragments.
-		if encoded_fragment.len() > max_message_size {
+		let max_message_size = channel_info.max_message_size as usize;
+		let format_size = format.encoded_size();
+		// We check the encoded fragment length plus the format size agains the max message size
+		// because the format is concatenated if a new page is needed.
+		let size_to_check = encoded_fragment
+			.len()
+			.checked_add(format_size)
+			.ok_or(MessageSendError::TooBig)?;
+		if size_to_check > max_message_size {
 			return Err(MessageSendError::TooBig)
 		}
 

cumulus/pallets/xcmp-queue/src/tests.rs

@@ -731,6 +731,50 @@ fn xcmp_queue_send_xcm_works() {
 	})
 }
 
+#[test]
+fn xcmp_queue_send_too_big_xcm_fails() {
+	new_test_ext().execute_with(|| {
+		let sibling_para_id = ParaId::from(12345);
+		let dest = (Parent, X1(Parachain(sibling_para_id.into()))).into();
+
+		let max_message_size = 100_u32;
+
+		// open HRMP channel to the sibling_para_id with a set `max_message_size`
+		ParachainSystem::open_custom_outbound_hrmp_channel_for_benchmarks_or_tests(
+			sibling_para_id,
+			cumulus_primitives_core::AbridgedHrmpChannel {
+				max_message_size,
+				max_capacity: 10,
+				max_total_size: 10_000_000_u32,
+				msg_count: 0,
+				total_size: 0,
+				mqc_head: None,
+			},
+		);
+
+		// Message is crafted to exceed `max_message_size`
+		let mut message = Xcm::builder_unsafe();
+		for _ in 0..97 {
+			message = message.clear_origin();
+		}
+		let message = message.build();
+		let encoded_message_size = message.encode().len();
+		let versioned_size = 1; // VersionedXcm enum is added by `send_xcm` and it add one additional byte
+		assert_eq!(encoded_message_size, max_message_size as usize - versioned_size);
+
+		// check empty outbound queue
+		assert!(XcmpQueue::take_outbound_messages(usize::MAX).is_empty());
+
+		// Message is too big because after adding the VersionedXcm enum, it would reach
+		// `max_message_size` Then, adding the format, which is the worst case scenario in which a
+		// new page is needed, would get it over the limit
+		assert_eq!(send_xcm::<XcmpQueue>(dest, message), Err(SendError::Transport("TooBig")),);
+
+		// outbound queue is still empty
+		assert!(XcmpQueue::take_outbound_messages(usize::MAX).is_empty());
+	});
+}
+
 #[test]
 fn verify_fee_factor_increase_and_decrease() {
 	use cumulus_primitives_core::AbridgedHrmpChannel;