From 6187e00bf423f8192882f0a368b99ccae1d390ff Mon Sep 17 00:00:00 2001
From: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Date: Fri, 21 Jun 2024 16:30:29 +0200
Subject: [PATCH] [HRMP] Dont partially modify pages (#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 <oliver.tale-yazdi@parity.io>
---
 cumulus/pallets/xcmp-queue/src/lib.rs   | 13 ++++++-----
 cumulus/pallets/xcmp-queue/src/tests.rs | 31 ++++++++++++++++++++++++-
 prdoc/pr_4710.prdoc                     | 11 +++++++++
 3 files changed, 48 insertions(+), 7 deletions(-)
 create mode 100644 prdoc/pr_4710.prdoc

diff --git a/cumulus/pallets/xcmp-queue/src/lib.rs b/cumulus/pallets/xcmp-queue/src/lib.rs
index 5633f05f13bb8..45126a9425d4c 100644
--- a/cumulus/pallets/xcmp-queue/src/lib.rs
+++ b/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();
 
diff --git a/cumulus/pallets/xcmp-queue/src/tests.rs b/cumulus/pallets/xcmp-queue/src/tests.rs
index cdf41e27f0b27..5b02baf2310a3 100644
--- a/cumulus/pallets/xcmp-queue/src/tests.rs
+++ b/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");
+			}
+		}
+	});
+}
diff --git a/prdoc/pr_4710.prdoc b/prdoc/pr_4710.prdoc
new file mode 100644
index 0000000000000..d7d31d817208a
--- /dev/null
+++ b/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