From a9738adedb74e2ef068d433f2edb5690ebfc0088 Mon Sep 17 00:00:00 2001 From: Just van Stam Date: Mon, 4 Dec 2023 23:02:16 +0100 Subject: [PATCH] Fix XCMP max message size check (#1250) Improved max message size logic and added test for it --------- Co-authored-by: Francisco Aguirre Co-authored-by: command-bot <> --- cumulus/pallets/xcmp-queue/src/lib.rs | 14 ++++++-- cumulus/pallets/xcmp-queue/src/tests.rs | 44 +++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/cumulus/pallets/xcmp-queue/src/lib.rs b/cumulus/pallets/xcmp-queue/src/lib.rs index d3443163d086..71cd21d45f77 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -454,11 +454,21 @@ impl Pallet { ) -> Result { let encoded_fragment = fragment.encode(); + // Optimization note: `max_message_size` could potentially be stored in + // `OutboundXcmpMessages` once known; that way it's only accessed when a new page is needed. + let channel_info = T::ChannelInfo::get_channel_info(recipient).ok_or(MessageSendError::NoChannel)?; - let max_message_size = channel_info.max_message_size as usize; // Max message size refers to aggregates, or pages. Not to individual fragments. - if encoded_fragment.len() > max_message_size { + let max_message_size = channel_info.max_message_size as usize; + let format_size = format.encoded_size(); + // We check the encoded fragment length plus the format size agains the max message size + // because the format is concatenated if a new page is needed. + let size_to_check = encoded_fragment + .len() + .checked_add(format_size) + .ok_or(MessageSendError::TooBig)?; + if size_to_check > max_message_size { return Err(MessageSendError::TooBig) } diff --git a/cumulus/pallets/xcmp-queue/src/tests.rs b/cumulus/pallets/xcmp-queue/src/tests.rs index f88dedc1ece4..50c2a057d421 100644 --- a/cumulus/pallets/xcmp-queue/src/tests.rs +++ b/cumulus/pallets/xcmp-queue/src/tests.rs @@ -731,6 +731,50 @@ fn xcmp_queue_send_xcm_works() { }) } +#[test] +fn xcmp_queue_send_too_big_xcm_fails() { + new_test_ext().execute_with(|| { + let sibling_para_id = ParaId::from(12345); + let dest = (Parent, X1(Parachain(sibling_para_id.into()))).into(); + + let max_message_size = 100_u32; + + // open HRMP channel to the sibling_para_id with a set `max_message_size` + ParachainSystem::open_custom_outbound_hrmp_channel_for_benchmarks_or_tests( + sibling_para_id, + cumulus_primitives_core::AbridgedHrmpChannel { + max_message_size, + max_capacity: 10, + max_total_size: 10_000_000_u32, + msg_count: 0, + total_size: 0, + mqc_head: None, + }, + ); + + // Message is crafted to exceed `max_message_size` + let mut message = Xcm::builder_unsafe(); + for _ in 0..97 { + message = message.clear_origin(); + } + let message = message.build(); + let encoded_message_size = message.encode().len(); + let versioned_size = 1; // VersionedXcm enum is added by `send_xcm` and it add one additional byte + assert_eq!(encoded_message_size, max_message_size as usize - versioned_size); + + // check empty outbound queue + assert!(XcmpQueue::take_outbound_messages(usize::MAX).is_empty()); + + // Message is too big because after adding the VersionedXcm enum, it would reach + // `max_message_size` Then, adding the format, which is the worst case scenario in which a + // new page is needed, would get it over the limit + assert_eq!(send_xcm::(dest, message), Err(SendError::Transport("TooBig")),); + + // outbound queue is still empty + assert!(XcmpQueue::take_outbound_messages(usize::MAX).is_empty()); + }); +} + #[test] fn verify_fee_factor_increase_and_decrease() { use cumulus_primitives_core::AbridgedHrmpChannel;