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 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. @@ -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 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::::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"); + } + } + }); +} 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