Skip to content

Comments

check XCM size in VMP routing#8409

Merged
kianenigma merged 19 commits intomasterfrom
kiz-vmp-message-size-check
May 14, 2025
Merged

check XCM size in VMP routing#8409
kianenigma merged 19 commits intomasterfrom
kiz-vmp-message-size-check

Conversation

@kianenigma
Copy link
Contributor

@kianenigma kianenigma commented May 2, 2025

Contxt: within AHM, AH and RC need to communicate large-ish payloads of data, such as 1000 validator key and corresponding reward points per session. While the current limits on Polkadot and Kusama allow for such payloads, I am putting the code in place such that we SendXcm::validate any message before sending, and based on that, if MessageSizeExceeded is returned, we recursively split the messages. This code itself will come in a sparate PR.

So far, it turned out that in the code bath of a UMP (para -> RC), validate actually didn't check the message size. This PR adds this. I am not super familiar yet with this part of the codebase to know where to add unit tests for this, but I can tell that in my ZN experiments, this fixes the issue.

I was expecting a similar hack to also be needed for DMP (RC -> para), but it seems like this is not needed and in this code-path (based on who is the router: ChildParachainRouter in my case).

@kianenigma kianenigma added the T6-XCM This PR/Issue is related to XCM. label May 2, 2025
@franciscoaguirre
Copy link
Contributor

franciscoaguirre commented May 2, 2025

The approach looks good to me. Why didn't you have to add it to the ChildParachainRouter? Does it already validate the message size?

There are unit tests for the ParentAsUmp router in the file where it's defined: cumulus/primitives/utility/src/lib.rs. I would add a unit test for validating a message that's too big there.

@acatangiu
Copy link
Contributor

acatangiu commented May 2, 2025

cc @serban300

bkontur
bkontur reviewed May 5, 2025
View reviewed changes
@kianenigma kianenigma mentioned this pull request May 5, 2025
Merged
9 tasks
@kianenigma
Copy link
Contributor Author

kianenigma commented May 5, 2025

\cmd prdoc --bump minor

@kianenigma kianenigma marked this pull request as ready for review May 5, 2025 09:42
@kianenigma
Copy link
Contributor Author

kianenigma commented May 5, 2025

\cmd prdoc --bump minor

@kianenigma
Copy link
Contributor Author

kianenigma commented May 5, 2025

/cmd prdoc --bump minor

bkontur
bkontur reviewed May 5, 2025
View reviewed changes
@kianenigma
Copy link
Contributor Author

kianenigma commented May 6, 2025 •
edited
Loading

I looked a bit into why the benches are failing. It is because HostConfigurations is not set, and at the moment we fail if it doesn't exist. It seems to me the right behavior.

I have 2 options, and would like advice on which to take, based on the "standards" and conventions of XCM related code that I don't know very well, but would like to respect:

  1. #[cfg(feature = runtime-bencharks)] let max_size = HostConfigurations.unwrap_or(usize::max) directly in UpwardMessageSender.
  2. Add an impl of EnsureDelivery for parachains_system::Pallet, which sets up HostConfigurations (and potentially any other configuration that might be needed in the future)

Latter is clearly more elegant to me, but a bit more work

@kianenigma
Copy link
Contributor Author

kianenigma commented May 7, 2025

I pushed approach 1 in 5506193 for now.

@kianenigma kianenigma mentioned this pull request May 7, 2025
Closed
5 tasks
@bkontur
Copy link
Contributor

bkontur commented May 7, 2025

I pushed approach 1 in 5506193 for now.

nice quick hack :), something like this for approach 2 should also work: https://github.com/paritytech/polkadot-sdk/compare/kiz-vmp-message-size-check...bko-on-kiz-vmp-message-size-check?expand=1

bkontur
bkontur reviewed May 7, 2025
View reviewed changes
@bkontur @github-actions
Bko on kiz vmp message size check (#8458)
dfe5fe5 
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@paritytech-workflow-stopper
Copy link

paritytech-workflow-stopper bot commented May 13, 2025

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/14991655906
Failed job name: test-linux-stable-no-try-runtime

@kianenigma kianenigma enabled auto-merge May 14, 2025 20:09
@kianenigma
Copy link
Contributor Author

kianenigma commented May 14, 2025

/cmd prdoc --bump minor

@kianenigma
Copy link
Contributor Author

kianenigma commented May 14, 2025

\cmd prdoc --bump minor

@github-actions
Copy link
Contributor

github-actions bot commented May 14, 2025

Command "prdoc --bump minor" has failed ❌! See logs here

@kianenigma kianenigma added this pull request to the merge queue May 14, 2025
@kianenigma
Copy link
Contributor Author

kianenigma commented May 14, 2025

/cmd prdoc --bump minor --force

Merged via the queue into master with commit da8c374 May 14, 2025
246 of 249 checks passed
@kianenigma kianenigma deleted the kiz-vmp-message-size-check branch May 14, 2025 21:29
github-merge-queue bot pushed a commit that referenced this pull request May 30, 2025
@kianenigma @acatangiu
@github-actions @sigurpol
[AHM] Staking async fixes for XCM and election planning (#8422)
7bb570d 
This PR brings a few small fixes related to the XCM messages of
staking-async, among other small fixes:


* [x] Allows `xcm::validate` to check the message size, and we actually
now act upon it in the `staking-async-rc/parachain-runtime`s. The code
is a bit duplicate now, and there is a TOOD about how to better refactor
it later.
* [x] Part of this work is backported separately as
#8409
* [x] It brings a default `EraElectionPlannerOf` which should be the
right tool to use to ensure elections always happen in time, with an
educated guess based on `ElectionProvider::duration` rather than a
random number.
* [x] It adds a few unit tests about the above
* [x] It silences some logs that were needlessly `INFO`, and makes the
printing of some types a bit more CLI friendly.
* [x] Fix the issue with duplicate voters in solution type: see
#8585
* [x] Move `PagedRawSolution` to be unbounded, and therefore decode-able
without PoV
* [x] Undo a rename from `ClaimedRewards` to `ErasClaimedRewards`
  * [ ] Needs fixing in westend

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Paolo La Camera <paolo@parity.io>
pgherveou pushed a commit that referenced this pull request Jun 11, 2025
@kianenigma @acatangiu
@github-actions @sigurpol @pgherveou
[AHM] Staking async fixes for XCM and election planning (#8422)
80ff238 
This PR brings a few small fixes related to the XCM messages of
staking-async, among other small fixes:


* [x] Allows `xcm::validate` to check the message size, and we actually
now act upon it in the `staking-async-rc/parachain-runtime`s. The code
is a bit duplicate now, and there is a TOOD about how to better refactor
it later.
* [x] Part of this work is backported separately as
#8409
* [x] It brings a default `EraElectionPlannerOf` which should be the
right tool to use to ensure elections always happen in time, with an
educated guess based on `ElectionProvider::duration` rather than a
random number.
* [x] It adds a few unit tests about the above
* [x] It silences some logs that were needlessly `INFO`, and makes the
printing of some types a bit more CLI friendly.
* [x] Fix the issue with duplicate voters in solution type: see
#8585
* [x] Move `PagedRawSolution` to be unbounded, and therefore decode-able
without PoV
* [x] Undo a rename from `ClaimedRewards` to `ErasClaimedRewards`
  * [ ] Needs fixing in westend

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Paolo La Camera <paolo@parity.io>
@acatangiu acatangiu mentioned this pull request Jul 16, 2025
Merged
21 tasks
fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this pull request Aug 7, 2025
@acatangiu @actions-user
@claravanstaden @bkontur @bkchr @brenzi @kianenigma @franciscoaguirre @yrong @joepetrowski @Overkillus
Upgrade to Polkadot-SDK stable2506 (#817)
d6c5bd3 
This brings in `stable2506` Polkadot SDK, and integrates many new
features.

Integrated breaking changes to be verified by the original authors:

- [x] ~paritytech/polkadot-sdk#8127 @kianenigma
@Ank4n~
     This will come in with AHM, and not before.
- [x] paritytech/polkadot-sdk#7597 @gui1117 
- [x] paritytech/polkadot-sdk#8254 @bkchr 
- [x] paritytech/polkadot-sdk#7592 @bkontur 
- [x] paritytech/polkadot-sdk#8382
@UtkarshBhardwaj007
- [x] paritytech/polkadot-sdk#8021 @serban300 
- [x] paritytech/polkadot-sdk#8344 @serban300 
- [x] paritytech/polkadot-sdk#8262 @athei 
- [x] paritytech/polkadot-sdk#8584 @athei 
- [x] paritytech/polkadot-sdk#8299 @skunert
- [x] paritytech/polkadot-sdk#8652 @pgherveou 
- [x] paritytech/polkadot-sdk#8554 @pgherveou 
- [x] paritytech/polkadot-sdk#8281 @mrshiposha 
- [x] paritytech/polkadot-sdk#7730
@franciscoaguirre
- [x] paritytech/polkadot-sdk#8599 @yrong
@claravanstaden
- [x] paritytech/polkadot-sdk#8531 @bkontur 
- [x] paritytech/polkadot-sdk#8409 @kianenigma 
- [x] paritytech/polkadot-sdk#9137
@franciscoaguirre
- [x] paritytech/polkadot-sdk#7944 @bkontur 
- [x] paritytech/polkadot-sdk#8179 @bkontur 
- [x] paritytech/polkadot-sdk#8037 @yrong

---------

Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: claravanstaden <claravanstaden64@gmail.com>
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Alain Brenzikofer <alain@integritee.network>
Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: ron <yrong1997@gmail.com>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Co-authored-by: Overkillus <maciej.zyszkiewicz@parity.io>
@kianenigma kianenigma mentioned this pull request Sep 9, 2025
Merged
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
@kianenigma @github-actions
@bkontur @alvicsam
check XCM size in VMP routing (#8409)
dbca2b3 
Contxt: within AHM, AH and RC need to communicate large-ish payloads of
data, such as 1000 validator key and corresponding reward points per
session. While the current limits on Polkadot and Kusama allow for such
payloads, I am putting the code in place such that we
`SendXcm::validate` any message before sending, and based on that, if
`MessageSizeExceeded` is returned, we recursively split the messages.
This code itself will come in a sparate PR.

So far, it turned out that in the code bath of a UMP (para -> RC),
`validate` actually didn't check the message size. This PR adds this. I
am not super familiar yet with this part of the codebase to know where
to add unit tests for this, but I can tell that in my ZN experiments,
this fixes the issue.

I was expecting a similar hack to also be needed for DMP (RC -> para),
but it seems like this is not needed and in this code-path (based on who
is the router: `ChildParachainRouter` in my case).

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
@kianenigma @acatangiu
@github-actions @sigurpol @alvicsam
[AHM] Staking async fixes for XCM and election planning (#8422)
0f47934 
This PR brings a few small fixes related to the XCM messages of
staking-async, among other small fixes:


* [x] Allows `xcm::validate` to check the message size, and we actually
now act upon it in the `staking-async-rc/parachain-runtime`s. The code
is a bit duplicate now, and there is a TOOD about how to better refactor
it later.
* [x] Part of this work is backported separately as
#8409
* [x] It brings a default `EraElectionPlannerOf` which should be the
right tool to use to ensure elections always happen in time, with an
educated guess based on `ElectionProvider::duration` rather than a
random number.
* [x] It adds a few unit tests about the above
* [x] It silences some logs that were needlessly `INFO`, and makes the
printing of some types a bit more CLI friendly.
* [x] Fix the issue with duplicate voters in solution type: see
#8585
* [x] Move `PagedRawSolution` to be unbounded, and therefore decode-able
without PoV
* [x] Undo a rename from `ClaimedRewards` to `ErasClaimedRewards`
  * [ ] Needs fixing in westend

---------

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Paolo La Camera <paolo@parity.io>
@github-actions github-actions bot mentioned this pull request Oct 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkontur bkontur bkontur approved these changes

@franciscoaguirre franciscoaguirre franciscoaguirre approved these changes

@acatangiu acatangiu acatangiu approved these changes

Assignees

No one assigned

Labels

T6-XCM This PR/Issue is related to XCM.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kianenigma @franciscoaguirre @acatangiu @bkontur