Make HRMP advancement rule more restrictive by serban300 · Pull Request #9086 · paritytech/polkadot-sdk

serban300 · 2025-07-03T10:11:22Z

Related to: #9021
Followup for #8860

Looked some more into this and from what I understand the HRMP max message size can't be changed dynamically. In order to change it we would need to close the channel and than open it again which would lead to clearing all the pending messages. So it's safe to use the current hrmp max message size in the advancement rule check.

serban300 · 2025-07-03T10:50:26Z

/cmd prdoc --bump patch

cumulus/pallets/parachain-system/src/lib.rs

bkchr · 2025-09-15T08:40:50Z

cumulus/pallets/parachain-system/src/parachain_inherent.rs

+					min_size
+				);
+			},
+			None => {


If this is None, it means there are no hashed messages and we left the function already.

I don't know if I understand. This should mean that no size constraints were provided in which case we should apply the most basic constraint. Which is the case now for DMP. There might be hashed messages.

The if in line 203. We can never end up here, because the if should resolve to true if the hashed messages are empty, which also means that the message_info is not set.

Maybe the entire function should only be called when we have message_size_info.

Done, but for DMP we need the logic in the None branch. So I split this in 2 functions.

cumulus/pallets/parachain-system/src/parachain_inherent.rs

bkchr · 2025-09-15T09:05:57Z

cumulus/pallets/parachain-system/src/parachain_inherent.rs

+					size = size.saturating_add(msg.data().len());
+				}
+
+				// Let's take a margin of safety.


I can remove it, but I think it's safer to have it. It can save us from a parachain stall in very ugly corner cases while it doesn't impact the min size very much normally. I don't know.

For example if there have been some miscalculations in other places and we end up with a message that is off by a couple of bytes for example. Off by 1 errors are very common in my experience (there was one in this PR as well :D ).

bkchr · 2025-09-15T09:14:07Z

cumulus/pallets/parachain-system/src/lib.rs

+		let first_hashed_msg_channel = hashed_messages
+			.first()
+			.map(|(sender, _msg)| *sender)
+			.map(|sender| Self::get_ingress_channel_or_panic(ingress_channels, sender));


We should take the maximum from all the max_message_size. Otherwise you may take 100bytes while some other channel has a max size of 1MIB.

Yes, but I think only the max size of the first hashed message should be important. For example:

Let's say that we are connected to parachains:

1000 -> max_message_size = 1

2000 -> max_message_size = 50

And:

XCMP full messages total size limit = 100

full_messages: [{len: 50, sender: 2000}, {len: 1, sender: 1000}] => total length = 51

If we have:
hashed_messages: [{sender: 2000}, ...]
It's ok, because in the worst case scenario we had a maximal message from parachain 2000 and this would have exceeded the max full messages limit: 51 + 50 = 101 > 100

If we have:
hashed_messages: [{sender: 1000}, ...]
We should panic because the first message could have been added as a full message: 51 + 1 = 52 < 100 . Regardless of the fact that sender 2000 has a max size of 50.

Am I missing something ?

I see where my confusion is coming from. I assumed we took messages from the hrmp messages until we hit the maximum. But this is not correct, as we directly stop when one message doesn't fit anymore. Maybe another improvement 🙈

Yes, would be nice. We can consider doing this in the future. But I think it would require changing the MQC head logic ? which wouldn't be trivial

We would need to keep the order per parachain. That is fine. But we should be able to skip parachain A and continue with B etc.

cumulus/pallets/parachain-system/src/parachain_inherent.rs

cumulus/pallets/parachain-system/src/lib.rs

bkchr · 2025-11-04T09:52:04Z

cumulus/pallets/parachain-system/src/parachain_inherent.rs

+					min_size
+				);
+			},
+			None => {


The if in line 203. We can never end up here, because the if should resolve to true if the hashed messages are empty, which also means that the message_info is not set.

Maybe the entire function should only be called when we have message_size_info.

bkchr · 2025-11-04T13:18:45Z

cumulus/pallets/parachain-system/src/lib.rs

+		let first_hashed_msg_channel = hashed_messages
+			.first()
+			.map(|(sender, _msg)| *sender)
+			.map(|sender| Self::get_ingress_channel_or_panic(ingress_channels, sender));


I see where my confusion is coming from. I assumed we took messages from the hrmp messages until we hit the maximum. But this is not correct, as we directly stop when one message doesn't fit anymore. Maybe another improvement 🙈

bkchr · 2025-11-04T13:19:38Z

cumulus/pallets/parachain-system/src/parachain_inherent.rs

+				assert!(
+					full_messages_size > min_full_messages_size,


Just doing full_message_size + first_hashed_msg_max_size > max_full_messages_size should be easier? Then we don't need to do leave any extra room or whatever?

We would still have no margin of safety in the worst case scenario. Anyway, normally it's more correct like this. Done.

…rule

ggwpez · 2025-11-06T12:46:16Z

cumulus/pallets/parachain-system/src/lib.rs

+		ingress_channels
+			.get(channel_idx)
+			.map(|(_, channel)| channel)
+			.expect("Channel index was just calculated and is correct; qed")


You can probably use some and, map or then functions on the option to directly check both in the if above.
Two possible pancis for the same thing is a bit odd.

ggwpez

Okay so you make the HRMP rule greedy now? So that it forces the collator to include all possible messages and not just one? good

cumulus/pallets/parachain-system/src/parachain_inherent.rs

ggwpez · 2025-11-06T13:12:57Z

cumulus/pallets/parachain-system/src/parachain_inherent.rs

+
+		// Here we just check that there is at least 1 full message.
+		assert!(
+			self.full_messages.len() >= 1,


So the case that already the first message is too big can never happen, right?

Very good point !

With the current Polkadot and Kusama configs it won't happen. But with a misconfiguration it probably could. Anyway this check is only used for DMP and the problem there is that we don't know the max message size in order to do a proper check. This will be fixed when we migrate DMP to check_enough_messages_included_advanced as well. But in order to do this we need to also forward the max dmp message size to the parachain in the metadata. It would be great if we could do that in a separate PR and merge this one as it is. WDYT ?

Yea we can do it separate

So the case that already the first message is too big can never happen, right?

If this would happen, we could never move forward. I see this not as an issue. Yes, could be a misconfiguration, but then you can not receive messages anymore.

cumulus/pallets/parachain-system/src/lib.rs

serban300 self-assigned this Jul 3, 2025

serban300 added T9-cumulus This PR/Issue is related to cumulus. T18-zombienet_tests Trigger zombienet CI tests. labels Jul 3, 2025

serban300 force-pushed the hrmp-advancement-rule branch from 077f83f to 69609db Compare July 3, 2025 10:34

serban300 force-pushed the hrmp-advancement-rule branch 2 times, most recently from 5a51d66 to eeb9161 Compare July 3, 2025 11:08

serban300 requested review from bkchr and ggwpez July 3, 2025 11:59

serban300 force-pushed the hrmp-advancement-rule branch 3 times, most recently from 77f14a0 to 0ca86b6 Compare September 12, 2025 11:08

serban300 changed the title ~~Improve HRMP advancement rule~~ Make HRMP advancement rule more restrictive Sep 12, 2025

Improve check_enough_messages_included()

408a035

serban300 force-pushed the hrmp-advancement-rule branch from 0ca86b6 to 408a035 Compare September 12, 2025 11:12

bkchr reviewed Sep 15, 2025

View reviewed changes

CR comments

e66c81d

bkchr reviewed Nov 4, 2025

View reviewed changes

serban300 added 2 commits November 4, 2025 16:51

CR comments 2

fa4a7cc

Merge remote-tracking branch 'upstream/master' into hrmp-advancement-…

ba143f4

…rule

bkchr approved these changes Nov 5, 2025

View reviewed changes

ggwpez reviewed Nov 6, 2025

View reviewed changes

CR comments 3

48476b3

ggwpez approved these changes Nov 6, 2025

View reviewed changes

redzsina added this to Security Audit (PRs) - SRLabs Nov 17, 2025

github-project-automation bot moved this to Backlog in Security Audit (PRs) - SRLabs Nov 17, 2025

redzsina moved this from Backlog to Scheduled in Security Audit (PRs) - SRLabs Nov 17, 2025

redzsina moved this from Scheduled to In progress in Security Audit (PRs) - SRLabs Nov 24, 2025

rachsrl moved this from In progress to Audited in Security Audit (PRs) - SRLabs Dec 2, 2025

Merge branch 'master' into hrmp-advancement-rule

f082fc1

serban300 added this pull request to the merge queue Jan 20, 2026

Merged via the queue into paritytech:master with commit e30ff0d Jan 20, 2026
218 of 220 checks passed

serban300 deleted the hrmp-advancement-rule branch January 20, 2026 14:06

serban300 mentioned this pull request Feb 9, 2026

Fix check_enough_messages_included_advanced() #11024

Closed

pgherveou added this to [preview] release tracker Mar 31, 2026

github-project-automation bot moved this from Todo to Done in [preview] release tracker Mar 31, 2026

github-project-automation bot moved this to Todo in [preview] release tracker Mar 31, 2026

Conversation

serban300 commented Jul 3, 2025

Uh oh!

serban300 commented Jul 3, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serban300 Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

serban300 Nov 4, 2025 •

edited

Loading