[HRMP] Dont partially modify pages (paritytech#4710)

Changes:
- The XCMP queue does not partially modify pages anymore by using
`try_mutate` instead of `mutate`.
- The XCMP queue max page size is now the min between the value that the
relay reports and the local limit.

Thanks to whom pointed this out to me via DM.

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

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