[HRMP] Dont partially modify pages

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

@@ -491,7 +491,7 @@ impl<T: Config> Pallet<T> {
 		let channel_info =
 			T::ChannelInfo::get_channel_info(recipient).ok_or(MessageSendError::NoChannel)?;
 		// Max message size refers to aggregates, or pages. Not to individual fragments.
-		let max_message_size = channel_info.max_message_size as usize;
+		let max_message_size = channel_info.max_message_size.min(T::MaxPageSize::get()) as usize;
 		let format_size = format.encoded_size();
 		// We check the encoded fragment length plus the format size against the max message size
 		// because the format is concatenated if a new page is needed.
@@ -522,7 +522,7 @@ impl<T: Config> Pallet<T> {
 		// We return the size of the last page inside of the option, to not calculate it again.
 		let appended_to_last_page = have_active
 			.then(|| {
-				<OutboundXcmpMessages<T>>::mutate(
+				<OutboundXcmpMessages<T>>::try_mutate(
 					recipient,
 					channel_details.last_index - 1,
 					|page| {
@@ -532,17 +532,18 @@ impl<T: Config> Pallet<T> {
 						) != Ok(format)
 						{
 							defensive!("Bad format in outbound queue; dropping message");
-							return None
+							return Err(())
 						}
 						if page.len() + encoded_fragment.len() > max_message_size {
-							return None
+							return Err(())
 						}
 						for frag in encoded_fragment.iter() {
-							page.try_push(*frag).ok()?;
+							page.try_push(*frag)?;
 						}
-						Some(page.len())
+						Ok(page.len())
 					},
 				)
+				.ok()
 			})
 			.flatten();
 

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

@@ -28,6 +28,7 @@ use frame_support::{
 use mock::{new_test_ext, ParachainSystem, RuntimeOrigin as Origin, Test, XcmpQueue};
 use sp_runtime::traits::{BadOrigin, Zero};
 use std::iter::{once, repeat};
+use xcm_builder::InspectMessageQueues;
 
 #[test]
 fn empty_concatenated_works() {
@@ -854,7 +855,6 @@ fn verify_fee_factor_increase_and_decrease() {
 #[test]
 fn get_messages_works() {
 	new_test_ext().execute_with(|| {
-		use xcm_builder::InspectMessageQueues;
 		let sibling_para_id = ParaId::from(2001);
 		ParachainSystem::open_outbound_hrmp_channel_for_benchmarks_or_tests(sibling_para_id);
 		let destination: Location = (Parent, Parachain(sibling_para_id.into())).into();
@@ -890,3 +890,32 @@ fn get_messages_works() {
 		);
 	});
 }
+
+/// We try to send a fragment that will not fit into the currently active page. This should
+/// therefore not modify the current page but instead create a new one.
+#[test]
+fn page_not_modified_when_fragment_does_not_fit() {
+	new_test_ext().execute_with(|| {
+		let sibling = ParaId::from(2001);
+		ParachainSystem::open_outbound_hrmp_channel_for_benchmarks_or_tests(sibling);
+
+		let destination: Location = (Parent, Parachain(sibling.into())).into();
+		let message = Xcm(vec![ClearOrigin; 600]);
+
+		loop {
+			let old_page_zero = OutboundXcmpMessages::<Test>::get(sibling, 0);
+			assert_ok!(send_xcm::<XcmpQueue>(destination.clone(), message.clone()));
+
+			// If a new page was created by this send_xcm call, then page_zero was not also
+			// modified:
+			let num_pages = OutboundXcmpMessages::<Test>::iter_prefix(sibling).count();
+			if num_pages == 2 {
+				let new_page_zero = OutboundXcmpMessages::<Test>::get(sibling, 0);
+				assert_eq!(old_page_zero, new_page_zero);
+				break
+			} else if num_pages > 2 {
+				panic!("Too many pages created");
+			}
+		}
+	});
+}

prdoc/pr_4710.prdoc

@@ -0,0 +1,11 @@
+title: "Dont partially modify HRMP pages"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      The xcmp-queue pallet now does not partially modify a page anymore when the next message does
+      not fully fit into it but instead cleanly creates a new one.
+
+crates:
+  - name: cumulus-pallet-xcmp-queue
+    bump: patch