XCMP and DMP improvements #8860

serban300 · 2025-06-16T07:02:20Z

Related to #489

This PR changes the parachain receiving logic for XCMP and DMP by adding some offchain processing before forwarding the messages to the parachain set_validation_data inherent. This enables us to relax the advancement rule.

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

Perform some offchain processing for the XCMP and DMP messages before forwarding them to the parachain `set_validation_data` inherent.

bkchr

Generally looking good. Though, left comments on a lot of things :)

polkadot/runtime/parachains/src/hrmp.rs

cumulus/primitives/parachain-inherent/src/lib.rs

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

cumulus/primitives/parachain-inherent/src/lib.rs

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

cumulus/primitives/parachain-inherent/src/lib.rs

…djustments

bkchr

Generally looking good. There are still a lot of missing docs for public methods.

bkchr · 2025-06-25T08:05:59Z

cumulus/primitives/parachain-inherent/src/lib.rs

@@ -64,7 +66,7 @@ pub mod v0 {
 		PartialEq,
 		TypeInfo,
 	)]
-	pub struct ParachainInherentData {
+	pub struct RawParachainInherentData {


I don't like the name 🙈

Also while thinking about this, all the new stuff you introduced here does not need to be in this crate. All that can just live in the parachain-system crate. The ParachainInherentData (RawParachainInherentData right now) is the only thing that needs to stay here, because it is the thing that is exchanged between the node and the runtime.

Ok. Changed the names back:
RawParachainInherentData -> ParachainInherentData
ParachainInherentData -> BasicParachainInherentData

Also moved all the new structures to parachain-system except HashedMessage because it simplifies some interactions with MessageQueueChain.

polkadot/core-primitives/src/lib.rs

bkchr · 2025-06-25T10:17:17Z

polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs

-			match new.hrmp_inbound.valid_watermarks.binary_search(&hrmp_watermark.watermark()) {
-				Ok(pos) => {
-					// Exact match, so this is OK in all cases.
-					let _ = new.hrmp_inbound.valid_watermarks.drain(..pos + 1);


The only change we need here is that we drop the + 1 or not?

Aka we don't need your new code? :D

I think we also need an adjustment for the case:

Err(pos) => match hrmp_watermark { HrmpWatermarkUpdate::Head(_) => { // Updates to Head are always OK. let _ = new.hrmp_inbound.valid_watermarks.drain(..pos); }, }

Here we should also retain the current watermark:

// The current watermark will remain valid until updated. if new.hrmp_inbound.valid_watermarks.first() != Some(&hrmp_watermark) { new.hrmp_inbound.valid_watermarks.insert(0, hrmp_watermark) }

With my changes I tried to avoid having multiple branches and to do this in a more generic way. I can change it back if you prefer.

Here we should also retain the current watermark:

Sounds reasonable, but it would just mean that we need to add the insert to this branch? (I would keep the original code)

Also it would be nice to test these corner cases.

Ok, adjusted the original code.

There are the constraints_disallowed_trunk_watermark and constraints_always_allow_head_watermark that cover these cases.

The only change we need here is that we drop the + 1 or not?

It looks the same to me.

Why do we need to insert anything if we don't find the Head watermark? valid_watermarks should contain these watermarks coming from the relay chain runtime (for real hrmp messages). The Head watermark is always valid, regardless of whether we have any queued messages at that block. And if at one candidate we reach the Head (current relay chain number) without finding it in the valid_watermarks, this means we don't have any more messages to process. Therefore, the next candidate has no good reason to not advance the hrmp watermark to its new relay parent (if it has some messages, the relay chain will contain this valid hrmp watermark). Am I missing something?

You're right, good point ! I missed this. Removed the additional insert.

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

cumulus/primitives/parachain-inherent/src/lib.rs

bkchr · 2025-06-25T13:16:44Z

cumulus/primitives/parachain-inherent/src/lib.rs

+			if sent_at < last_processed_msg.sent_at {
+				last_processed_msg_idx = Some(idx);
+				break;
+			}


That should be impossible to hit?

Yes, in theory. But I think it's better to check.

polkadot/core-primitives/src/lib.rs

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

ggwpez · 2025-06-25T20:29:36Z

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

+			mqc_heads.entry(*sender).or_default().extend_with_hashed_msg(msg);
+
+			if msg.sent_at == last_processed_msg.sent_at {
+				last_processed_msg.reverse_idx += 1;


We are not going in reverse but that does not matter, right? The meaning of reverse_idx is more like unprocessed_count.

We are not going in reverse here when counting, but it's actually a reverse index inside the group of messages with the provided sent_at. I added some examples to the InboundMessageId:

/// For the collection /// `msgs = [{sent_at: 0}, {sent_at: 1}, {sent_at: 1}, {sent_at: 1}, {sent_at: 1}, {sent_at: 3}]` /// /// `InboundMessageId {sent_at: 1, reverse_idx: 0}` points to `msgs[4]` /// `InboundMessageId {sent_at: 1, reverse_idx: 3}` points to `msgs[1]` /// `InboundMessageId {sent_at: 1, reverse_idx: 4}` points to `msgs[0]`

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

ggwpez · 2025-06-25T20:44:59Z

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

+	/// A malicious collator could provide an invalid collection. This function checks that the
+	/// current collection respects the advancement rule. Specifically, it checks that the current
+	/// collection contains as many full messages as possible.
+	pub fn check_advancement_rule(&self, collection_name: &str, max_size: usize) {


Suggested change

pub fn check_advancement_rule(&self, collection_name: &str, max_size: usize) {

pub fn check_all_messages_included(&self, collection_name: &str, max_size: usize) {

? Since its not really advancement rule, more like censorship resistance.

Yes, you're right, it's not really the advancement rule. Renamed it to check_enough_messages_included.

Is panick-ing in this case better than just accepting this block?
Is the reasoning that the collator will not end up getting a block reward?

The most they can do is delay the message processing anyway

Yes, I think we need to have this for censorship resistance.

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

ggwpez · 2025-06-25T20:47:47Z

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

+			for msg in &self.full_messages {
+				size = size.saturating_add(msg.data().len());
+			}
+			let min_size = ((max_size as f64) * 0.75) as usize;


Please add a sanity check to this pallet that we do not shoot ourselves in the foot here.
There is no guarantee here that a parachain wont have a max message size configured of 500KiB or something stupid... actually not sure if we can even check this since its in the host config.
Maybe we can read the abridged host config? Not sure how complicated that is

let's add a comment for the reasoning behind this 0.75 coefficient

The abridged host config doesn't contain the max downward message size. This value is only present on the relay chain from what I understand.

Also a sanity check would only check this on code changes right ? But I think the config can be changed dynamically on chain ? So it wouldn't cover all possible problems.

I think the safest thing would be to check only if there is at least 1 full message in the collection. Or to have min_size = 50% of max size. @ggwpez @bkchr WDYT ?

As I said here: #8860 (comment), I'm wondering if it is worth panicking for this?

Yes, I think we should have at least a basic check here for censorship resistance. If we want to be super safe we can just check that self.full_messages.len() > 1. For the moment I'll change the coefficient to 0.5.

add a comment

Done

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

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

polkadot/runtime/parachains/src/hrmp.rs

alindima

Overall LGTM, nice work!

I assume it was tested end-to-end with zombienet?

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

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

alindima · 2025-06-26T07:24:15Z

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

+			for msg in &self.full_messages {
+				size = size.saturating_add(msg.data().len());
+			}
+			let min_size = ((max_size as f64) * 0.75) as usize;


let's add a comment for the reasoning behind this 0.75 coefficient

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

alindima · 2025-06-26T08:37:26Z

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

+	/// A malicious collator could provide an invalid collection. This function checks that the
+	/// current collection respects the advancement rule. Specifically, it checks that the current
+	/// collection contains as many full messages as possible.
+	pub fn check_advancement_rule(&self, collection_name: &str, max_size: usize) {


Is panick-ing in this case better than just accepting this block?
Is the reasoning that the collator will not end up getting a block reward?

The most they can do is delay the message processing anyway

alindima · 2025-06-26T09:38:34Z

polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs

-			match new.hrmp_inbound.valid_watermarks.binary_search(&hrmp_watermark.watermark()) {
-				Ok(pos) => {
-					// Exact match, so this is OK in all cases.
-					let _ = new.hrmp_inbound.valid_watermarks.drain(..pos + 1);


The only change we need here is that we drop the + 1 or not?

It looks the same to me.

Why do we need to insert anything if we don't find the Head watermark? valid_watermarks should contain these watermarks coming from the relay chain runtime (for real hrmp messages). The Head watermark is always valid, regardless of whether we have any queued messages at that block. And if at one candidate we reach the Head (current relay chain number) without finding it in the valid_watermarks, this means we don't have any more messages to process. Therefore, the next candidate has no good reason to not advance the hrmp watermark to its new relay parent (if it has some messages, the relay chain will contain this valid hrmp watermark). Am I missing something?

alindima · 2025-06-26T09:52:31Z

polkadot/runtime/parachains/src/hrmp.rs

+			.collect();
+
+		// The current watermark will remain valid until updated.
+		if let Some(last_watermark) = HrmpWatermarks::<T>::get(&recipient) {


same as in the inclusion_emulator, do we really need this?
This is only really used in the inclusion emulator

Yes, it's needed. If the hrmp watermark remains the same for a couple of blocks, check_modifications will always fail.

I tried without this and either the parachain was stallig or the parachain block time was increasing for a while. Don't remember exactly, but anyway, it's needed.

…djustments

serban300

Overall LGTM, nice work!

I assume it was tested end-to-end with zombienet?

@alindima Thanks for the review ! Yes, it was tested end-to-end with zombienet.

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

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

serban300 · 2025-06-26T12:52:01Z

polkadot/runtime/parachains/src/hrmp.rs

+			.collect();
+
+		// The current watermark will remain valid until updated.
+		if let Some(last_watermark) = HrmpWatermarks::<T>::get(&recipient) {


Yes, it's needed. If the hrmp watermark remains the same for a couple of blocks, check_modifications will always fail.

I tried without this and either the parachain was stallig or the parachain block time was increasing for a while. Don't remember exactly, but anyway, it's needed.

serban300 · 2025-06-26T12:54:40Z

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

+	/// A malicious collator could provide an invalid collection. This function checks that the
+	/// current collection respects the advancement rule. Specifically, it checks that the current
+	/// collection contains as many full messages as possible.
+	pub fn check_advancement_rule(&self, collection_name: &str, max_size: usize) {


Yes, I think we need to have this for censorship resistance.

serban300 · 2025-06-26T12:57:54Z

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

+			for msg in &self.full_messages {
+				size = size.saturating_add(msg.data().len());
+			}
+			let min_size = ((max_size as f64) * 0.75) as usize;


Yes, I think we should have at least a basic check here for censorship resistance. If we want to be super safe we can just check that self.full_messages.len() > 1. For the moment I'll change the coefficient to 0.5.

add a comment

Done

serban300 · 2025-06-26T13:14:42Z

polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs

-			match new.hrmp_inbound.valid_watermarks.binary_search(&hrmp_watermark.watermark()) {
-				Ok(pos) => {
-					// Exact match, so this is OK in all cases.
-					let _ = new.hrmp_inbound.valid_watermarks.drain(..pos + 1);


You're right, good point ! I missed this. Removed the additional insert.

serban300 self-assigned this Jun 16, 2025

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

serban300 force-pushed the message-passing-adjustments branch 5 times, most recently from b65eafa to ae8fcc5 Compare June 16, 2025 11:23

serban300 requested review from bkchr, ggwpez, seadanda and acatangiu June 16, 2025 13:53

ggwpez reviewed Jun 16, 2025

View reviewed changes

cumulus/pallets/parachain-system/src/lib.rs Outdated Show resolved Hide resolved

serban300 changed the title ~~[WIP] XCMP and DMP improvements~~ XCMP and DMP improvements Jun 16, 2025

ggwpez reviewed Jun 16, 2025

View reviewed changes

cumulus/pallets/parachain-system/src/lib.rs Show resolved Hide resolved

serban300 force-pushed the message-passing-adjustments branch from ae8fcc5 to c1811ae Compare June 16, 2025 16:15

serban300 added 3 commits June 20, 2025 12:14

XCMP and DMP improvements

fef3e08

Perform some offchain processing for the XCMP and DMP messages before forwarding them to the parachain `set_validation_data` inherent.

prdoc

a071ec0

Docs

f797f1c

serban300 force-pushed the message-passing-adjustments branch from 3ee5165 to f797f1c Compare June 20, 2025 09:14

bkchr reviewed Jun 20, 2025

View reviewed changes

serban300 added 5 commits June 24, 2025 16:02

CR comments

9c77a98

Merge remote-tracking branch 'upstream/master' into message-passing-a…

de04f73

…djustments

Fix CI

e65f933

fix prdoc

e9e98d6

Merge remote-tracking branch 'upstream/master' into message-passing-a…

8308fca

…djustments

bkchr reviewed Jun 25, 2025

View reviewed changes

acatangiu requested a review from alindima June 25, 2025 15:06

serban300 added 2 commits June 25, 2025 20:51

CR comments

5f614e2

fix

6ecd1be

Fix unit test

ef34a10

ggwpez reviewed Jun 25, 2025

View reviewed changes

cumulus/pallets/parachain-system/src/lib.rs Outdated Show resolved Hide resolved

ggwpez reviewed Jun 25, 2025

View reviewed changes

cumulus/pallets/parachain-system/src/lib.rs Show resolved Hide resolved

ggwpez reviewed Jun 25, 2025

View reviewed changes

cumulus/pallets/parachain-system/src/parachain_inherent.rs Outdated Show resolved Hide resolved

ggwpez reviewed Jun 25, 2025

View reviewed changes

cumulus/pallets/parachain-system/src/parachain_inherent.rs Outdated Show resolved Hide resolved

ggwpez reviewed Jun 25, 2025

View reviewed changes

cumulus/pallets/parachain-system/src/parachain_inherent.rs Outdated Show resolved Hide resolved

ggwpez reviewed Jun 25, 2025

View reviewed changes

cumulus/pallets/parachain-system/src/unincluded_segment.rs Outdated Show resolved Hide resolved

ggwpez reviewed Jun 25, 2025

View reviewed changes

cumulus/pallets/parachain-system/src/unincluded_segment.rs Show resolved Hide resolved

ggwpez reviewed Jun 25, 2025

View reviewed changes

polkadot/runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved

CR comments

5dc6667

alindima reviewed Jun 26, 2025

View reviewed changes

serban300 added 2 commits June 26, 2025 16:16

CR comments

2335cc8

Merge remote-tracking branch 'upstream/master' into message-passing-a…

f7839f7

…djustments

serban300 commented Jun 26, 2025

View reviewed changes

	pub fn check_advancement_rule(&self, collection_name: &str, max_size: usize) {
	pub fn check_all_messages_included(&self, collection_name: &str, max_size: usize) {

XCMP and DMP improvements #8860

Are you sure you want to change the base?

XCMP and DMP improvements #8860

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr left a comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggwpez Jun 25, 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!

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!

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!

serban300 commented Jun 16, 2025 •

edited

Loading

ggwpez Jun 25, 2025 •

edited

Loading