From 33849f155adbf8a9c56d2a2e3858a103bb89c2e8 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 5 Jun 2024 15:07:22 +0200 Subject: [PATCH 1/4] [HRMP] Dont write partial fragments to page Signed-off-by: Oliver Tale-Yazdi --- cumulus/pallets/xcmp-queue/src/lib.rs | 11 +++++---- cumulus/pallets/xcmp-queue/src/tests.rs | 33 ++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/cumulus/pallets/xcmp-queue/src/lib.rs b/cumulus/pallets/xcmp-queue/src/lib.rs index 5633f05f13bb..254c6fdcfe13 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -522,7 +522,7 @@ impl Pallet { // 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(|| { - >::mutate( + >::try_mutate( recipient, channel_details.last_index - 1, |page| { @@ -532,17 +532,18 @@ impl Pallet { ) != 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 cdf41e27f0b2..0f1404f76288 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,34 @@ 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 other_sibling = ParaId::from(2002); + let message = Xcm(vec![ClearOrigin; 600]); + let s = message.encoded_size(); + + loop { + let old_page_zero = OutboundXcmpMessages::::get(sibling, 0); + assert_ok!(send_xcm::(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::::iter_prefix(sibling).count(); + if num_pages == 2 { + let new_page_zero = OutboundXcmpMessages::::get(sibling, 0); + assert_eq!(old_page_zero, new_page_zero); + break + } else if num_pages > 2 { + panic!("Too many pages created"); + } + } + }); +} From 1a4eefc89586363be39a13747184429849538f2f Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 5 Jun 2024 15:12:25 +0200 Subject: [PATCH 2/4] Use min for size limit Signed-off-by: Oliver Tale-Yazdi --- cumulus/pallets/xcmp-queue/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cumulus/pallets/xcmp-queue/src/lib.rs b/cumulus/pallets/xcmp-queue/src/lib.rs index 254c6fdcfe13..45126a9425d4 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -491,7 +491,7 @@ impl Pallet { 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. From 6504a9874f41d4036d8376d79593d69d6bba91e4 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 5 Jun 2024 15:14:01 +0200 Subject: [PATCH 3/4] Unused vars Signed-off-by: Oliver Tale-Yazdi --- cumulus/pallets/xcmp-queue/src/tests.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/cumulus/pallets/xcmp-queue/src/tests.rs b/cumulus/pallets/xcmp-queue/src/tests.rs index 0f1404f76288..5b02baf2310a 100644 --- a/cumulus/pallets/xcmp-queue/src/tests.rs +++ b/cumulus/pallets/xcmp-queue/src/tests.rs @@ -900,9 +900,7 @@ fn page_not_modified_when_fragment_does_not_fit() { ParachainSystem::open_outbound_hrmp_channel_for_benchmarks_or_tests(sibling); let destination: Location = (Parent, Parachain(sibling.into())).into(); - let other_sibling = ParaId::from(2002); let message = Xcm(vec![ClearOrigin; 600]); - let s = message.encoded_size(); loop { let old_page_zero = OutboundXcmpMessages::::get(sibling, 0); From c660a28fbdc54f862fa661571a2c3554adb65ce2 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 5 Jun 2024 15:18:40 +0200 Subject: [PATCH 4/4] Add prdoc Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_4710.prdoc | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 prdoc/pr_4710.prdoc diff --git a/prdoc/pr_4710.prdoc b/prdoc/pr_4710.prdoc new file mode 100644 index 000000000000..d7d31d817208 --- /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