From 408a035006cc2ba5945041ae00522fb46484319c Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Fri, 27 Jun 2025 14:53:47 +0300 Subject: [PATCH 1/4] Improve check_enough_messages_included() --- Cargo.lock | 76 +++++++-------- cumulus/pallets/parachain-system/src/lib.rs | 52 ++++++---- .../src/parachain_inherent.rs | 95 ++++++++++++++----- cumulus/pallets/parachain-system/src/tests.rs | 5 +- prdoc/pr_9086.prdoc | 8 ++ 5 files changed, 156 insertions(+), 80 deletions(-) create mode 100644 prdoc/pr_9086.prdoc diff --git a/Cargo.lock b/Cargo.lock index 687935c21fd6d..40fcfcc68f8e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -173,9 +173,9 @@ dependencies = [ [[package]] name = "alloy-eips" -version = "1.0.28" +version = "1.0.30" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9d9ebd288f87aa496179b83471f5b00bcd01def4048714234d3ddbf97a6b9dd2" +checksum = "2a15b4b0f6bab47aae017d52bb5a739bda381553c09fb9918b7172721ef5f5de" dependencies = [ "alloy-eip2124", "alloy-eip2930", @@ -256,9 +256,9 @@ dependencies = [ [[package]] name = "alloy-serde" -version = "1.0.28" +version = "1.0.30" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "743e0fe099fe90a07215ae08413b271b8fac12fca6880c85326ce61341566e6f" +checksum = "f1b3b1078b8775077525bc9fe9f6577e815ceaecd6c412a4f3b4d8aa2836e8f6" dependencies = [ "alloy-primitives", "serde", @@ -412,9 +412,9 @@ dependencies = [ [[package]] name = "anyhow" -version = "1.0.98" +version = "1.0.99" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e16d2d3311acee920a9eb8d33b8cbc1787ce4a264e85f964c2404b969bdcd487" +checksum = "b0674a1ddeecb70197781e945de4b3b8ffb61fa939a5597bcf48503737663100" [[package]] name = "approx" @@ -441,9 +441,9 @@ dependencies = [ [[package]] name = "arbitrary" -version = "1.4.1" +version = "1.4.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dde20b3d026af13f561bdd0f15edf01fc734f0dafcedbaf42bba506a9517f223" +checksum = "c3d036a3c4ab069c7b410a2ce876bd74808d2d0888a82667669f8e783a898bf1" dependencies = [ "derive_arbitrary", ] @@ -3051,9 +3051,9 @@ checksum = "a2698f953def977c68f935bb0dfa959375ad4638570e969e2f1e9f433cbf1af6" [[package]] name = "cc" -version = "1.2.35" +version = "1.2.37" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "590f9024a68a8c40351881787f1934dc11afd69090f5edb6831464694d836ea3" +checksum = "65193589c6404eb80b450d618eaf9a2cafaaafd57ecce47370519ef674a7bd44" dependencies = [ "find-msvc-tools", "jobserver", @@ -5511,9 +5511,9 @@ dependencies = [ [[package]] name = "derive_arbitrary" -version = "1.4.1" +version = "1.4.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "30542c1ad912e0e3d22a1935c290e12e8a29d704a420177a31faad4a601a0800" +checksum = "1e567bd82dcff979e4b03460c307b3cdc9e96fde3d73bed1496d2bc75d9dd62a" dependencies = [ "proc-macro2 1.0.95", "quote 1.0.40", @@ -6439,9 +6439,9 @@ dependencies = [ [[package]] name = "find-msvc-tools" -version = "0.1.0" +version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e178e4fba8a2726903f6ba98a6d221e76f9c12c650d5dc0e6afdc50677b49650" +checksum = "7fd99930f64d146689264c637b5af2f0233a933bef0d8570e2526bf9e083192d" [[package]] name = "findshlibs" @@ -7529,9 +7529,9 @@ dependencies = [ [[package]] name = "gmp-mpfr-sys" -version = "1.6.7" +version = "1.6.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a636fb6a653382a379ee1e5593dacdc628667994167024c143214cafd40c1a86" +checksum = "60f8970a75c006bb2f8ae79c6768a116dd215fa8346a87aed99bf9d82ca43394" dependencies = [ "libc", "windows-sys 0.60.2", @@ -9815,9 +9815,9 @@ checksum = "78b3ae25bc7c8c38cec158d1f2757ee79e9b3740fbc7ccf0e59e4b08d793fa89" [[package]] name = "linux-raw-sys" -version = "0.9.4" +version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cd945864f07fe9f5371a27ad7b52a172b4b499999f1d97574c9fa68373937e12" +checksum = "df1d3c3b53da64cf5760482273a98e575c651a67eec7f77df96b5b642de8f039" [[package]] name = "lioness" @@ -17805,7 +17805,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8a56d757972c98b346a9b766e3f02746cde6dd1cd1d1d563472929fdd74bec4d" dependencies = [ "anyhow", - "itertools 0.14.0", + "itertools 0.13.0", "proc-macro2 1.0.95", "quote 1.0.40", "syn 2.0.98", @@ -19196,15 +19196,15 @@ dependencies = [ [[package]] name = "rustix" -version = "1.0.8" +version = "1.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "11181fbabf243db407ef8df94a6ce0b2f9a733bd8be4ad02b4eda9602296cac8" +checksum = "cd15f8a2c5551a84d56efdc1cd049089e409ac19a3072d5037a17fd70719ff3e" dependencies = [ "bitflags 2.9.4", "errno", "libc", - "linux-raw-sys 0.9.4", - "windows-sys 0.60.2", + "linux-raw-sys 0.11.0", + "windows-sys 0.59.0", ] [[package]] @@ -20095,7 +20095,7 @@ dependencies = [ "parity-scale-codec", "parking_lot 0.12.3", "paste", - "rustix 1.0.8", + "rustix 1.1.2", "sc-allocator", "sc-executor-common", "sc-runtime-test", @@ -21818,7 +21818,7 @@ dependencies = [ "slab", "smallvec", "soketto", - "twox-hash 2.1.1", + "twox-hash 2.1.2", "wasmi 0.40.0", "x25519-dalek", "zeroize", @@ -25211,9 +25211,9 @@ dependencies = [ [[package]] name = "target-lexicon" -version = "0.13.2" +version = "0.13.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e502f78cdbb8ba4718f566c418c52bc729126ffd16baee5baa718cf25dd5a69a" +checksum = "df7f62577c25e07834649fc3b39fafdc597c0a3527dc1c60129201ccfcbaa50c" [[package]] name = "target-triple" @@ -25280,9 +25280,9 @@ checksum = "3369f5ac52d5eb6ab48c6b4ffdc8efbcad6b89c765749064ba298f2c68a16a76" [[package]] name = "test-log" -version = "0.2.18" +version = "0.2.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e33b98a582ea0be1168eba097538ee8dd4bbe0f2b01b22ac92ea30054e5be7b" +checksum = "3dffced63c2b5c7be278154d76b479f9f9920ed34e7574201407f0b14e2bbb93" dependencies = [ "env_logger 0.11.3", "test-log-macros", @@ -25291,9 +25291,9 @@ dependencies = [ [[package]] name = "test-log-macros" -version = "0.2.18" +version = "0.2.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "451b374529930d7601b1eef8d32bc79ae870b6079b069401709c2a8bf9e75f36" +checksum = "5999e24eaa32083191ba4e425deb75cdf25efefabe5aaccb7446dd0d4122a3f5" dependencies = [ "proc-macro2 1.0.95", "quote 1.0.40", @@ -26123,9 +26123,9 @@ dependencies = [ [[package]] name = "twox-hash" -version = "2.1.1" +version = "2.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8b907da542cbced5261bd3256de1b3a1bf340a3d37f93425a07362a1d687de56" +checksum = "9ea3136b675547379c4bd395ca6b938e5ad3c3d20fad76e7fe85f9e0d011419c" [[package]] name = "typenum" @@ -26829,7 +26829,7 @@ dependencies = [ "postcard", "pulley-interpreter", "rayon", - "rustix 1.0.8", + "rustix 1.1.2", "serde", "serde_derive", "smallvec", @@ -26894,7 +26894,7 @@ dependencies = [ "directories-next", "log", "postcard", - "rustix 1.0.8", + "rustix 1.1.2", "serde", "serde_derive", "sha2 0.10.9", @@ -26940,7 +26940,7 @@ dependencies = [ "cc", "cfg-if", "libc", - "rustix 1.0.8", + "rustix 1.1.2", "wasmtime-internal-asm-macros", "wasmtime-internal-versioned-export-macros", "windows-sys 0.59.0", @@ -28400,9 +28400,9 @@ dependencies = [ [[package]] name = "zstd-sys" -version = "2.0.15+zstd.1.5.7" +version = "2.0.16+zstd.1.5.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eb81183ddd97d0c74cedf1d50d85c8d08c1b8b68ee863bdee9e706eedba1a237" +checksum = "91e19ebc2adc8f83e43039e79776e3fda8ca919132d68a1fed6a5faca2683748" dependencies = [ "cc", "pkg-config", diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index 589c97499acc9..2f0b0ef78dcc8 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -1146,7 +1146,7 @@ impl Pallet { expected_dmq_mqc_head: relay_chain::Hash, downward_messages: AbridgedInboundDownwardMessages, ) -> Weight { - downward_messages.check_enough_messages_included("DMQ"); + downward_messages.check_enough_messages_included("DMQ", None); let mut dmq_head = >::get(); @@ -1192,6 +1192,26 @@ impl Pallet { weight_used } + fn get_ingress_channel_or_panic( + ingress_channels: &[(ParaId, cumulus_primitives_core::AbridgedHrmpChannel)], + sender: ParaId, + ) -> &cumulus_primitives_core::AbridgedHrmpChannel { + let maybe_channel_idx = + ingress_channels.binary_search_by_key(&sender, |&(channel_sender, _)| channel_sender); + let channel = match maybe_channel_idx { + Ok(channel_idx) => ingress_channels.get(channel_idx).map(|(_, channel)| channel), + Err(_) => None, + }; + + channel.unwrap_or_else(|| { + panic!( + "One of the messages submitted by the collator was sent from a sender ({}) \ + that doesn't have a channel opened to this parachain", + >::into(sender) + ) + }) + } + fn check_hrmp_mcq_heads( ingress_channels: &[(ParaId, cumulus_primitives_core::AbridgedHrmpChannel)], mqc_heads: &mut BTreeMap, @@ -1225,17 +1245,8 @@ impl Pallet { } *maybe_prev_msg_metadata = Some(msg_metadata); - // Check that the message is sent from an existing channel. The channel exists - // if its MQC head is present in `vfp.hrmp_mqc_heads`. - let sender = msg_metadata.1; - let maybe_channel_idx = - ingress_channels.binary_search_by_key(&sender, |&(channel_sender, _)| channel_sender); - assert!( - maybe_channel_idx.is_ok(), - "One of the messages submitted by the collator was sent from a sender ({}) \ - that doesn't have a channel opened to this parachain", - >::into(sender) - ); + // Check that the message is sent from an existing channel. + let _ = Self::get_ingress_channel_or_panic(ingress_channels, msg_metadata.1); } /// Process all inbound horizontal messages relayed by the collator. @@ -1253,11 +1264,20 @@ impl Pallet { horizontal_messages: AbridgedInboundHrmpMessages, relay_parent_number: relay_chain::BlockNumber, ) -> Weight { - // First, check the HRMP advancement rule. - horizontal_messages.check_enough_messages_included("HRMP"); - - let (messages, hashed_messages) = horizontal_messages.messages(); let mut mqc_heads = >::get(); + let (messages, hashed_messages) = horizontal_messages.messages(); + + // First, check the HRMP advancement rule. + let first_hashed_msg_channel = hashed_messages + .first() + .map(|(sender, _msg)| *sender) + .map(|sender| Self::get_ingress_channel_or_panic(ingress_channels, sender)); + horizontal_messages.check_enough_messages_included( + "HRMP", + first_hashed_msg_channel.map(|channel| { + (Self::messages_collection_size_limit(), channel.max_message_size as usize) + }), + ); if messages.is_empty() { Self::check_hrmp_mcq_heads(ingress_channels, &mut mqc_heads); diff --git a/cumulus/pallets/parachain-system/src/parachain_inherent.rs b/cumulus/pallets/parachain-system/src/parachain_inherent.rs index 13d1619357fba..d843602b410db 100644 --- a/cumulus/pallets/parachain-system/src/parachain_inherent.rs +++ b/cumulus/pallets/parachain-system/src/parachain_inherent.rs @@ -187,25 +187,56 @@ impl AbridgedInboundMessagesCollection { /// The `AbridgedInboundMessagesCollection` is provided to the runtime by a collator. /// A malicious collator can provide a collection that contains no full messages or fewer /// full messages than possible, leading to censorship. - pub fn check_enough_messages_included(&self, collection_name: &str) { + /// + /// # Parameters + /// - `collection_name`: The name of the collection (e.g. HRMP). + /// - `size_constraints`: An optional tuple consisting of: + /// 1. The max size of the full messages collection + /// 2. The max size of the first hashed message + pub fn check_enough_messages_included( + &self, + collection_name: &str, + size_constraints: Option<(usize, usize)>, + ) { if self.hashed_messages.is_empty() { return; } - // Ideally, we should check that the collection contains as many full messages as possible - // without exceeding the max expected size. The worst case scenario is that were the first - // message that had to be hashed is a max size message. So in this case, the min expected - // size would be `max_expected_size - max_msg_size`. However, there are multiple issues: - // 1. The max message size config can change while we still have to process messages with - // the old max message size. - // 2. We can't access the max downward message size from the parachain runtime. - // - // So the safest approach is to check that there is at least 1 full message. - assert!( - self.full_messages.len() >= 1, - "[{}] Advancement rule violation: mandatory messages missing", - collection_name, - ); + // We should check that the collection contains as many full messages as possible + // without exceeding the max expected size. + match size_constraints { + Some((max_size, first_hashed_msg_max_size)) => { + let mut size = 0usize; + for msg in &self.full_messages { + size = size.saturating_add(msg.data().len()); + } + + // Let's take a margin of safety. + let max_size = max_size * 3 / 4; + // The worst case scenario is that were the first message that had to be hashed + // is a max size message. So in this case, the min expected size would be + // `max_expected_size - first_hashed_msg_max_size`. + let min_size = max_size.saturating_sub(first_hashed_msg_max_size); + + assert!( + size >= min_size, + "[{}] Advancement rule violation: full messages size smaller than expected: \ + {} < {}", + collection_name, + size, + min_size + ); + }, + None => { + // If no `size_constraints` are provided, let's just check that there is at least + // 1 full message. + assert!( + self.full_messages.len() >= 1, + "[{}] Advancement rule violation: full messages missing", + collection_name, + ); + }, + } } } @@ -515,24 +546,40 @@ mod tests { #[test] fn check_enough_messages_included_works() { - let mut messages = AbridgedInboundHrmpMessages { + let empty_messages = + AbridgedInboundHrmpMessages { full_messages: vec![], hashed_messages: vec![] }; + empty_messages.check_enough_messages_included("Test", None); + + let full_messages = AbridgedInboundHrmpMessages { full_messages: vec![( 1000.into(), - InboundHrmpMessage { sent_at: 0, data: vec![1; 100] }, + InboundHrmpMessage { sent_at: 0, data: vec![1; 50] }, )], + hashed_messages: vec![], + }; + full_messages.check_enough_messages_included("Test", Some((100, 24))); + + let hashed_messages = AbridgedInboundHrmpMessages { + full_messages: vec![], hashed_messages: vec![( 2000.into(), HashedMessage { sent_at: 1, msg_hash: Default::default() }, )], }; - - messages.check_enough_messages_included("Test"); - - messages.full_messages = vec![]; - let result = std::panic::catch_unwind(|| messages.check_enough_messages_included("Test")); + let result = std::panic::catch_unwind(|| { + hashed_messages.check_enough_messages_included("Test", None) + }); assert!(result.is_err()); - messages.hashed_messages = vec![]; - messages.check_enough_messages_included("Test"); + let mixed_messages = AbridgedInboundHrmpMessages { + full_messages: full_messages.full_messages.clone(), + hashed_messages: hashed_messages.hashed_messages.clone(), + }; + let result = std::panic::catch_unwind(|| { + mixed_messages.check_enough_messages_included("Test", Some((100, 24))) + }); + assert!(result.is_err()); + mixed_messages.check_enough_messages_included("Test", Some((100, 25))); + mixed_messages.check_enough_messages_included("Test", None); } } diff --git a/cumulus/pallets/parachain-system/src/tests.rs b/cumulus/pallets/parachain-system/src/tests.rs index 0b1ad28b4e82a..0fd0a0d6b63aa 100755 --- a/cumulus/pallets/parachain-system/src/tests.rs +++ b/cumulus/pallets/parachain-system/src/tests.rs @@ -410,8 +410,9 @@ fn inherent_messages_are_compressed() { sproof.dmq_mqc_head = Some(dmp_mqc.head()); for (sender, msg) in &hrmp_msgs_clone { - let mqc_head = - sproof.upsert_inbound_channel(*sender).mqc_head.get_or_insert_default(); + let channel = sproof.upsert_inbound_channel(*sender); + channel.max_message_size = 100 * 1024; + let mqc_head = channel.mqc_head.get_or_insert_default(); let mut mqc = MessageQueueChain::new(*mqc_head); mqc.extend_hrmp(msg); *mqc_head = mqc.head(); diff --git a/prdoc/pr_9086.prdoc b/prdoc/pr_9086.prdoc new file mode 100644 index 0000000000000..b06f41efa56b0 --- /dev/null +++ b/prdoc/pr_9086.prdoc @@ -0,0 +1,8 @@ +title: Make HRMP advancement rule more restrictive +doc: +- audience: Runtime Dev + description: |- + This PR improves `check_enough_messages_included()` and makes the advancement rule more restrictive for HRMP. +crates: +- name: cumulus-pallet-parachain-system + bump: major From e66c81d2424cded8e09ae6658dc49d08696ec1c8 Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Mon, 15 Sep 2025 17:15:07 +0300 Subject: [PATCH 2/4] CR comments --- cumulus/pallets/parachain-system/src/lib.rs | 20 +++--- .../src/parachain_inherent.rs | 62 +++++++++++++------ 2 files changed, 54 insertions(+), 28 deletions(-) diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index 2f0b0ef78dcc8..8275c8d774835 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -110,6 +110,7 @@ pub use cumulus_pallet_parachain_system_proc_macro::register_validate_block; pub use relay_state_snapshot::{MessagingStateSnapshot, RelayChainStateProof}; pub use unincluded_segment::{Ancestor, UsedBandwidth}; +use crate::parachain_inherent::AbridgedInboundMessagesSizeInfo; pub use pallet::*; const LOG_TARGET: &str = "parachain-system"; @@ -1198,18 +1199,18 @@ impl Pallet { ) -> &cumulus_primitives_core::AbridgedHrmpChannel { let maybe_channel_idx = ingress_channels.binary_search_by_key(&sender, |&(channel_sender, _)| channel_sender); - let channel = match maybe_channel_idx { - Ok(channel_idx) => ingress_channels.get(channel_idx).map(|(_, channel)| channel), - Err(_) => None, - }; - - channel.unwrap_or_else(|| { + let Ok(channel_idx) = maybe_channel_idx else { panic!( "One of the messages submitted by the collator was sent from a sender ({}) \ that doesn't have a channel opened to this parachain", >::into(sender) ) - }) + }; + + ingress_channels + .get(channel_idx) + .map(|(_, channel)| channel) + .expect("Channel index was just calculated and is correct; qed") } fn check_hrmp_mcq_heads( @@ -1274,8 +1275,9 @@ impl Pallet { .map(|sender| Self::get_ingress_channel_or_panic(ingress_channels, sender)); horizontal_messages.check_enough_messages_included( "HRMP", - first_hashed_msg_channel.map(|channel| { - (Self::messages_collection_size_limit(), channel.max_message_size as usize) + first_hashed_msg_channel.map(|channel| AbridgedInboundMessagesSizeInfo { + max_full_messages_size: Self::messages_collection_size_limit(), + first_hashed_msg_max_size: channel.max_message_size as usize, }), ); diff --git a/cumulus/pallets/parachain-system/src/parachain_inherent.rs b/cumulus/pallets/parachain-system/src/parachain_inherent.rs index d843602b410db..f11008d56842f 100644 --- a/cumulus/pallets/parachain-system/src/parachain_inherent.rs +++ b/cumulus/pallets/parachain-system/src/parachain_inherent.rs @@ -157,6 +157,14 @@ impl InboundMessagesCollection { } } +/// A struct containing some info about the expected size of the abridged inbound messages. +pub struct AbridgedInboundMessagesSizeInfo { + /// The max size of the full messages collection + pub max_full_messages_size: usize, + /// The max size of the first hashed message + pub first_hashed_msg_max_size: usize, +} + /// A compressed collection of inbound messages. /// /// The first messages in the collection (up to a limit) contain the full message data. @@ -187,16 +195,10 @@ impl AbridgedInboundMessagesCollection { /// The `AbridgedInboundMessagesCollection` is provided to the runtime by a collator. /// A malicious collator can provide a collection that contains no full messages or fewer /// full messages than possible, leading to censorship. - /// - /// # Parameters - /// - `collection_name`: The name of the collection (e.g. HRMP). - /// - `size_constraints`: An optional tuple consisting of: - /// 1. The max size of the full messages collection - /// 2. The max size of the first hashed message pub fn check_enough_messages_included( &self, collection_name: &str, - size_constraints: Option<(usize, usize)>, + maybe_size_info: Option, ) { if self.hashed_messages.is_empty() { return; @@ -204,27 +206,31 @@ impl AbridgedInboundMessagesCollection { // We should check that the collection contains as many full messages as possible // without exceeding the max expected size. - match size_constraints { - Some((max_size, first_hashed_msg_max_size)) => { - let mut size = 0usize; + match maybe_size_info { + Some(AbridgedInboundMessagesSizeInfo { + mut max_full_messages_size, + first_hashed_msg_max_size, + }) => { + let mut full_messages_size = 0usize; for msg in &self.full_messages { - size = size.saturating_add(msg.data().len()); + full_messages_size = full_messages_size.saturating_add(msg.data().len()); } // Let's take a margin of safety. - let max_size = max_size * 3 / 4; + max_full_messages_size = max_full_messages_size * 3 / 4; // The worst case scenario is that were the first message that had to be hashed // is a max size message. So in this case, the min expected size would be // `max_expected_size - first_hashed_msg_max_size`. - let min_size = max_size.saturating_sub(first_hashed_msg_max_size); + let min_full_messages_size = + max_full_messages_size.saturating_sub(first_hashed_msg_max_size); assert!( - size >= min_size, + full_messages_size > min_full_messages_size, "[{}] Advancement rule violation: full messages size smaller than expected: \ {} < {}", collection_name, - size, - min_size + full_messages_size, + min_full_messages_size ); }, None => { @@ -557,7 +563,13 @@ mod tests { )], hashed_messages: vec![], }; - full_messages.check_enough_messages_included("Test", Some((100, 24))); + full_messages.check_enough_messages_included( + "Test", + Some(AbridgedInboundMessagesSizeInfo { + max_full_messages_size: 100, + first_hashed_msg_max_size: 25, + }), + ); let hashed_messages = AbridgedInboundHrmpMessages { full_messages: vec![], @@ -576,10 +588,22 @@ mod tests { hashed_messages: hashed_messages.hashed_messages.clone(), }; let result = std::panic::catch_unwind(|| { - mixed_messages.check_enough_messages_included("Test", Some((100, 24))) + mixed_messages.check_enough_messages_included( + "Test", + Some(AbridgedInboundMessagesSizeInfo { + max_full_messages_size: 100, + first_hashed_msg_max_size: 25, + }), + ) }); assert!(result.is_err()); - mixed_messages.check_enough_messages_included("Test", Some((100, 25))); + mixed_messages.check_enough_messages_included( + "Test", + Some(AbridgedInboundMessagesSizeInfo { + max_full_messages_size: 100, + first_hashed_msg_max_size: 26, + }), + ); mixed_messages.check_enough_messages_included("Test", None); } } From fa4a7ccd333ef90274a7b3b7c7061142081936de Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Tue, 4 Nov 2025 16:09:23 +0200 Subject: [PATCH 3/4] CR comments 2 --- cumulus/pallets/parachain-system/src/lib.rs | 27 ++-- .../src/parachain_inherent.rs | 143 ++++++++---------- 2 files changed, 81 insertions(+), 89 deletions(-) diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index 8275c8d774835..933e9932cd01e 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -1147,7 +1147,7 @@ impl Pallet { expected_dmq_mqc_head: relay_chain::Hash, downward_messages: AbridgedInboundDownwardMessages, ) -> Weight { - downward_messages.check_enough_messages_included("DMQ", None); + downward_messages.check_enough_messages_included_basic("DMQ"); let mut dmq_head = >::get(); @@ -1247,7 +1247,7 @@ impl Pallet { *maybe_prev_msg_metadata = Some(msg_metadata); // Check that the message is sent from an existing channel. - let _ = Self::get_ingress_channel_or_panic(ingress_channels, msg_metadata.1); + Self::get_ingress_channel_or_panic(ingress_channels, msg_metadata.1); } /// Process all inbound horizontal messages relayed by the collator. @@ -1269,17 +1269,18 @@ impl Pallet { let (messages, hashed_messages) = horizontal_messages.messages(); // First, check the HRMP advancement rule. - let first_hashed_msg_channel = hashed_messages - .first() - .map(|(sender, _msg)| *sender) - .map(|sender| Self::get_ingress_channel_or_panic(ingress_channels, sender)); - horizontal_messages.check_enough_messages_included( - "HRMP", - first_hashed_msg_channel.map(|channel| AbridgedInboundMessagesSizeInfo { - max_full_messages_size: Self::messages_collection_size_limit(), - first_hashed_msg_max_size: channel.max_message_size as usize, - }), - ); + let maybe_first_hashed_msg_sender = hashed_messages.first().map(|(sender, _msg)| *sender); + if let Some(first_hashed_msg_sender) = maybe_first_hashed_msg_sender { + let channel = + Self::get_ingress_channel_or_panic(ingress_channels, first_hashed_msg_sender); + horizontal_messages.check_enough_messages_included_advanced( + "HRMP", + AbridgedInboundMessagesSizeInfo { + max_full_messages_size: Self::messages_collection_size_limit(), + first_hashed_msg_max_size: channel.max_message_size as usize, + }, + ); + } if messages.is_empty() { Self::check_hrmp_mcq_heads(ingress_channels, &mut mqc_heads); diff --git a/cumulus/pallets/parachain-system/src/parachain_inherent.rs b/cumulus/pallets/parachain-system/src/parachain_inherent.rs index f11008d56842f..895c6d198de9d 100644 --- a/cumulus/pallets/parachain-system/src/parachain_inherent.rs +++ b/cumulus/pallets/parachain-system/src/parachain_inherent.rs @@ -190,59 +190,52 @@ impl AbridgedInboundMessagesCollection { (&self.full_messages, &self.hashed_messages) } - /// Check that the current collection contains as many full messages as possible. + /// Check that the current collection contains at least 1 full message if needed. + pub fn check_enough_messages_included_basic(&self, collection_name: &str) { + if self.hashed_messages.is_empty() { + return; + } + + // Here we just check that there is at least 1 full message. + assert!( + self.full_messages.len() >= 1, + "[{}] Advancement rule violation: full messages missing", + collection_name, + ); + } + + /// Check that the current collection contains as many full messages as possible, taking into + /// consideration the collection constraints. /// /// The `AbridgedInboundMessagesCollection` is provided to the runtime by a collator. /// A malicious collator can provide a collection that contains no full messages or fewer /// full messages than possible, leading to censorship. - pub fn check_enough_messages_included( + pub fn check_enough_messages_included_advanced( &self, collection_name: &str, - maybe_size_info: Option, + maybe_size_info: AbridgedInboundMessagesSizeInfo, ) { - if self.hashed_messages.is_empty() { - return; - } - // We should check that the collection contains as many full messages as possible // without exceeding the max expected size. - match maybe_size_info { - Some(AbridgedInboundMessagesSizeInfo { - mut max_full_messages_size, - first_hashed_msg_max_size, - }) => { - let mut full_messages_size = 0usize; - for msg in &self.full_messages { - full_messages_size = full_messages_size.saturating_add(msg.data().len()); - } - - // Let's take a margin of safety. - max_full_messages_size = max_full_messages_size * 3 / 4; - // The worst case scenario is that were the first message that had to be hashed - // is a max size message. So in this case, the min expected size would be - // `max_expected_size - first_hashed_msg_max_size`. - let min_full_messages_size = - max_full_messages_size.saturating_sub(first_hashed_msg_max_size); - - assert!( - full_messages_size > min_full_messages_size, - "[{}] Advancement rule violation: full messages size smaller than expected: \ - {} < {}", - collection_name, - full_messages_size, - min_full_messages_size - ); - }, - None => { - // If no `size_constraints` are provided, let's just check that there is at least - // 1 full message. - assert!( - self.full_messages.len() >= 1, - "[{}] Advancement rule violation: full messages missing", - collection_name, - ); - }, + let AbridgedInboundMessagesSizeInfo { max_full_messages_size, first_hashed_msg_max_size } = + maybe_size_info; + + let mut full_messages_size = 0usize; + for msg in &self.full_messages { + full_messages_size = full_messages_size.saturating_add(msg.data().len()); } + + // The worst case scenario is that were the first message that had to be hashed + // is a max size message. + assert!( + full_messages_size.saturating_add(first_hashed_msg_max_size) > max_full_messages_size, + "[{}] Advancement rule violation: full messages size smaller than expected. \ + full msgs size: {}, first hashed msg max size: {}, max full msgs size: {}", + collection_name, + full_messages_size, + first_hashed_msg_max_size, + max_full_messages_size + ); } } @@ -551,59 +544,57 @@ mod tests { } #[test] - fn check_enough_messages_included_works() { - let empty_messages = - AbridgedInboundHrmpMessages { full_messages: vec![], hashed_messages: vec![] }; - empty_messages.check_enough_messages_included("Test", None); - - let full_messages = AbridgedInboundHrmpMessages { + fn check_enough_messages_included_basic_works() { + let mut messages = AbridgedInboundHrmpMessages { full_messages: vec![( 1000.into(), - InboundHrmpMessage { sent_at: 0, data: vec![1; 50] }, + InboundHrmpMessage { sent_at: 0, data: vec![1; 100] }, )], - hashed_messages: vec![], - }; - full_messages.check_enough_messages_included( - "Test", - Some(AbridgedInboundMessagesSizeInfo { - max_full_messages_size: 100, - first_hashed_msg_max_size: 25, - }), - ); - - let hashed_messages = AbridgedInboundHrmpMessages { - full_messages: vec![], hashed_messages: vec![( 2000.into(), HashedMessage { sent_at: 1, msg_hash: Default::default() }, )], }; - let result = std::panic::catch_unwind(|| { - hashed_messages.check_enough_messages_included("Test", None) - }); + + messages.check_enough_messages_included_basic("Test"); + + messages.full_messages = vec![]; + let result = + std::panic::catch_unwind(|| messages.check_enough_messages_included_basic("Test")); assert!(result.is_err()); + messages.hashed_messages = vec![]; + messages.check_enough_messages_included_basic("Test"); + } + + #[test] + fn check_enough_messages_included_advanced_works() { let mixed_messages = AbridgedInboundHrmpMessages { - full_messages: full_messages.full_messages.clone(), - hashed_messages: hashed_messages.hashed_messages.clone(), + full_messages: vec![( + 1000.into(), + InboundHrmpMessage { sent_at: 0, data: vec![1; 50] }, + )], + hashed_messages: vec![( + 2000.into(), + HashedMessage { sent_at: 1, msg_hash: Default::default() }, + )], }; let result = std::panic::catch_unwind(|| { - mixed_messages.check_enough_messages_included( + mixed_messages.check_enough_messages_included_advanced( "Test", - Some(AbridgedInboundMessagesSizeInfo { + AbridgedInboundMessagesSizeInfo { max_full_messages_size: 100, - first_hashed_msg_max_size: 25, - }), + first_hashed_msg_max_size: 50, + }, ) }); assert!(result.is_err()); - mixed_messages.check_enough_messages_included( + mixed_messages.check_enough_messages_included_advanced( "Test", - Some(AbridgedInboundMessagesSizeInfo { + AbridgedInboundMessagesSizeInfo { max_full_messages_size: 100, - first_hashed_msg_max_size: 26, - }), + first_hashed_msg_max_size: 51, + }, ); - mixed_messages.check_enough_messages_included("Test", None); } } From 48476b3fca2b385e13b5fecc2783d02c64c1511a Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Thu, 6 Nov 2025 15:50:51 +0200 Subject: [PATCH 4/4] CR comments 3 --- cumulus/pallets/parachain-system/src/lib.rs | 17 ++++++++--------- .../parachain-system/src/parachain_inherent.rs | 4 ++-- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index 40ea93c0680bd..5424a7a57d796 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -1193,20 +1193,19 @@ impl Pallet { ingress_channels: &[(ParaId, cumulus_primitives_core::AbridgedHrmpChannel)], sender: ParaId, ) -> &cumulus_primitives_core::AbridgedHrmpChannel { - let maybe_channel_idx = - ingress_channels.binary_search_by_key(&sender, |&(channel_sender, _)| channel_sender); - let Ok(channel_idx) = maybe_channel_idx else { + let maybe_channel_idx = ingress_channels + .binary_search_by_key(&sender, |&(channel_sender, _)| channel_sender) + .ok(); + let maybe_channel = maybe_channel_idx + .and_then(|channel_idx| ingress_channels.get(channel_idx)) + .map(|(_, channel)| channel); + maybe_channel.unwrap_or_else(|| { panic!( "One of the messages submitted by the collator was sent from a sender ({}) \ that doesn't have a channel opened to this parachain", >::into(sender) ) - }; - - ingress_channels - .get(channel_idx) - .map(|(_, channel)| channel) - .expect("Channel index was just calculated and is correct; qed") + }) } fn check_hrmp_mcq_heads( diff --git a/cumulus/pallets/parachain-system/src/parachain_inherent.rs b/cumulus/pallets/parachain-system/src/parachain_inherent.rs index aa5fddb9414af..72d691413bbcd 100644 --- a/cumulus/pallets/parachain-system/src/parachain_inherent.rs +++ b/cumulus/pallets/parachain-system/src/parachain_inherent.rs @@ -213,12 +213,12 @@ impl AbridgedInboundMessagesCollection { pub fn check_enough_messages_included_advanced( &self, collection_name: &str, - maybe_size_info: AbridgedInboundMessagesSizeInfo, + size_info: AbridgedInboundMessagesSizeInfo, ) { // We should check that the collection contains as many full messages as possible // without exceeding the max expected size. let AbridgedInboundMessagesSizeInfo { max_full_messages_size, first_hashed_msg_max_size } = - maybe_size_info; + size_info; let mut full_messages_size = 0usize; for msg in &self.full_messages {