From 59cd8deb2c14d7e286082c4bc77ecb93c539b705 Mon Sep 17 00:00:00 2001 From: Just van Stam Date: Tue, 29 Aug 2023 13:38:46 +0200 Subject: [PATCH 01/12] fix max message size --- cumulus/pallets/xcmp-queue/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cumulus/pallets/xcmp-queue/src/lib.rs b/cumulus/pallets/xcmp-queue/src/lib.rs index 960af9b5b772..8e035d821016 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -507,13 +507,13 @@ impl Pallet { fragment: Fragment, ) -> Result { let data = 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 max_message_size = T::ChannelInfo::get_channel_max(recipient).ok_or(MessageSendError::NoChannel)?; - if data.len() > max_message_size { + let format_size = format.encode().len(); + if data.len() + format_size > max_message_size { return Err(MessageSendError::TooBig) } From 7c91fee133a00cdbf45dd72fb04c1fe66dd9091e Mon Sep 17 00:00:00 2001 From: Just van Stam Date: Tue, 29 Aug 2023 13:42:17 +0200 Subject: [PATCH 02/12] fmt --- cumulus/pallets/xcmp-queue/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/cumulus/pallets/xcmp-queue/src/lib.rs b/cumulus/pallets/xcmp-queue/src/lib.rs index 8e035d821016..947271ce98b9 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -506,6 +506,7 @@ impl Pallet { format: XcmpMessageFormat, fragment: Fragment, ) -> Result { + let data = 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. From c1ddfad7c0fa443ffb2e67b7933458fa2e5b0571 Mon Sep 17 00:00:00 2001 From: Just van Stam Date: Tue, 29 Aug 2023 13:42:53 +0200 Subject: [PATCH 03/12] fix fmt --- 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 947271ce98b9..fcc40acb5c37 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -506,8 +506,8 @@ impl Pallet { format: XcmpMessageFormat, fragment: Fragment, ) -> Result { - let data = 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. From 602c9f870ac89915750f954c2812714638ed4c3e Mon Sep 17 00:00:00 2001 From: Just van Stam Date: Tue, 29 Aug 2023 13:43:40 +0200 Subject: [PATCH 04/12] final fmt fix --- 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 fcc40acb5c37..a74705458e39 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -507,7 +507,7 @@ impl Pallet { fragment: Fragment, ) -> Result { let data = 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. From c82f2af294d57566f090e78b4c54ed970a6ee962 Mon Sep 17 00:00:00 2001 From: Just van Stam Date: Tue, 29 Aug 2023 14:19:42 +0200 Subject: [PATCH 05/12] change to encoded_size --- 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 a74705458e39..7f8ce7dae235 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -513,7 +513,7 @@ impl Pallet { let max_message_size = T::ChannelInfo::get_channel_max(recipient).ok_or(MessageSendError::NoChannel)?; - let format_size = format.encode().len(); + let format_size = format.encoded_size(); if data.len() + format_size > max_message_size { return Err(MessageSendError::TooBig) } From 5417d0fd425605498142d26701e6746be209f06b Mon Sep 17 00:00:00 2001 From: Francisco Aguirre Date: Mon, 4 Dec 2023 18:32:41 +0100 Subject: [PATCH 06/12] Add test for exceeding in XCMP --- cumulus/pallets/xcmp-queue/src/lib.rs | 7 ++-- cumulus/pallets/xcmp-queue/src/tests.rs | 47 +++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/cumulus/pallets/xcmp-queue/src/lib.rs b/cumulus/pallets/xcmp-queue/src/lib.rs index 6560b695f598..4203cda10dfb 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -479,10 +479,13 @@ 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 format_size = format.encoded_size(); - // Max message size refers to aggregates, or pages. Not to individual fragments. - if encoded_fragment.len() + format_size > max_message_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..937cf40343b0 100644 --- a/cumulus/pallets/xcmp-queue/src/tests.rs +++ b/cumulus/pallets/xcmp-queue/src/tests.rs @@ -731,6 +731,53 @@ 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.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; From 5568bee4c7bb73df01a181291b6f6423fe0b9592 Mon Sep 17 00:00:00 2001 From: Francisco Aguirre Date: Mon, 4 Dec 2023 18:57:05 +0100 Subject: [PATCH 07/12] Add PRDoc --- prdoc/pr_1250.prdoc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 prdoc/pr_1250.prdoc diff --git a/prdoc/pr_1250.prdoc b/prdoc/pr_1250.prdoc new file mode 100644 index 000000000000..84a30a8ef372 --- /dev/null +++ b/prdoc/pr_1250.prdoc @@ -0,0 +1,17 @@ +title: Fix XCMP max message size check + +doc: + - audience: Core Dev + description: | + Improved XCMP max message check logic. + +migrations: + db: [] + + runtime: [] + +crates: + - name: "cumulus-pallet-xcmp-queue" + semver: patch + +host_functions: [] From 2c62b39bd0dc8bda884c91db2944afe9b26fa629 Mon Sep 17 00:00:00 2001 From: Francisco Aguirre Date: Mon, 4 Dec 2023 18:59:36 +0100 Subject: [PATCH 08/12] Fix issue with XCM builder pattern --- cumulus/pallets/xcmp-queue/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cumulus/pallets/xcmp-queue/src/tests.rs b/cumulus/pallets/xcmp-queue/src/tests.rs index 937cf40343b0..b02a42a3475f 100644 --- a/cumulus/pallets/xcmp-queue/src/tests.rs +++ b/cumulus/pallets/xcmp-queue/src/tests.rs @@ -755,7 +755,7 @@ fn xcmp_queue_send_too_big_xcm_fails() { // Message is crafted to exceed `max_message_size` let mut message = Xcm::builder_unsafe(); for _ in 0..97 { - message.clear_origin(); + message = message.clear_origin(); } let message = message.build(); let encoded_message_size = message.encode().len(); From 0eafc94710efffcf72861b632c1141ee3dc15a47 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Mon, 4 Dec 2023 18:12:34 +0000 Subject: [PATCH 09/12] ".git/.scripts/commands/fmt/fmt.sh" --- cumulus/pallets/xcmp-queue/src/lib.rs | 5 ++++- cumulus/pallets/xcmp-queue/src/tests.rs | 13 +++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/cumulus/pallets/xcmp-queue/src/lib.rs b/cumulus/pallets/xcmp-queue/src/lib.rs index c2b42bd9f77f..71cd21d45f77 100644 --- a/cumulus/pallets/xcmp-queue/src/lib.rs +++ b/cumulus/pallets/xcmp-queue/src/lib.rs @@ -464,7 +464,10 @@ impl Pallet { 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)?; + 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 b02a42a3475f..50c2a057d421 100644 --- a/cumulus/pallets/xcmp-queue/src/tests.rs +++ b/cumulus/pallets/xcmp-queue/src/tests.rs @@ -749,7 +749,7 @@ fn xcmp_queue_send_too_big_xcm_fails() { msg_count: 0, total_size: 0, mqc_head: None, - } + }, ); // Message is crafted to exceed `max_message_size` @@ -765,13 +765,10 @@ fn xcmp_queue_send_too_big_xcm_fails() { // 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")), - ); + // 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()); From 5aed8afbe547339d431f506d035ed290159c7376 Mon Sep 17 00:00:00 2001 From: Francisco Aguirre Date: Mon, 4 Dec 2023 19:34:09 +0100 Subject: [PATCH 10/12] Change PRDoc audience --- prdoc/pr_1250.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_1250.prdoc b/prdoc/pr_1250.prdoc index 84a30a8ef372..0fb2072ebc23 100644 --- a/prdoc/pr_1250.prdoc +++ b/prdoc/pr_1250.prdoc @@ -1,7 +1,7 @@ title: Fix XCMP max message size check doc: - - audience: Core Dev + - audience: Parachain Builder description: | Improved XCMP max message check logic. From 168e7790bc6f04c06e76709e2968d1b0910eca40 Mon Sep 17 00:00:00 2001 From: Francisco Aguirre Date: Mon, 4 Dec 2023 19:35:50 +0100 Subject: [PATCH 11/12] Change PRDoc audience --- prdoc/pr_1250.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_1250.prdoc b/prdoc/pr_1250.prdoc index 0fb2072ebc23..981d0b323b2e 100644 --- a/prdoc/pr_1250.prdoc +++ b/prdoc/pr_1250.prdoc @@ -1,7 +1,7 @@ title: Fix XCMP max message size check doc: - - audience: Parachain Builder + - audience: Parachain Dev description: | Improved XCMP max message check logic. From 313a93b08d2c0feedc8b8fdd6172f41ec001f137 Mon Sep 17 00:00:00 2001 From: Francisco Aguirre Date: Mon, 4 Dec 2023 19:37:45 +0100 Subject: [PATCH 12/12] Delete PRDoc --- prdoc/pr_1250.prdoc | 17 ----------------- 1 file changed, 17 deletions(-) delete mode 100644 prdoc/pr_1250.prdoc diff --git a/prdoc/pr_1250.prdoc b/prdoc/pr_1250.prdoc deleted file mode 100644 index 981d0b323b2e..000000000000 --- a/prdoc/pr_1250.prdoc +++ /dev/null @@ -1,17 +0,0 @@ -title: Fix XCMP max message size check - -doc: - - audience: Parachain Dev - description: | - Improved XCMP max message check logic. - -migrations: - db: [] - - runtime: [] - -crates: - - name: "cumulus-pallet-xcmp-queue" - semver: patch - -host_functions: []