Skip to content

[HRMP] Check messages order before enqueing them#9326

Merged
serban300 merged 12 commits intoparitytech:masterfrom
serban300:hrmp-ordering-check
Jul 28, 2025
Merged

[HRMP] Check messages order before enqueing them#9326
serban300 merged 12 commits intoparitytech:masterfrom
serban300:hrmp-ordering-check

Conversation

@serban300
Copy link
Copy Markdown
Contributor

@serban300 serban300 commented Jul 25, 2025

Related to #8860

This PR adds a check in order to ensure that the collator has respected the proper order when sending the HRMP messages to the runtime.

@serban300 serban300 self-assigned this Jul 25, 2025
@serban300 serban300 added T9-cumulus This PR/Issue is related to cumulus. A4-backport-unstable2507 Pull request must be backported to the unstable2507 release branch labels Jul 25, 2025
@serban300
Copy link
Copy Markdown
Contributor Author

/cmd prdoc --audience runtime_dev --bump patch

];
messages.check_messages_order();

// Unsorted messages shouldn't be accepted
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you verify sorted and unsorted messages within one test named check_messages_order_works. It might be just my perception but when I read check_messages_order_works, I assume that it verifies happy path; though, this test verifies both: happy and unhappy path.

My small suggestion here:

  • either move this unsorted assertion to the very top to show at the very beginning what is NOT accepted,
  • or split the test into two, or even better use table-driven tests to clearly show what's the input and what's the expected state

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Move the unsorted assertions to the top and added some comments. Not sure in table driven testing is worth the effort for these very simple use cases.

Copy link
Copy Markdown
Contributor Author

@serban300 serban300 Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to move the logic and removed these tests. Defined other ones. And split them in multiple test cases for success vs failure.

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Jul 27, 2025

A test would also be nice. I know that we now only can have some panicking test, but better than nothing.

@serban300
Copy link
Copy Markdown
Contributor Author

Yes, agree, trying to add one

@serban300
Copy link
Copy Markdown
Contributor Author

Added some tests

@serban300 serban300 added this pull request to the merge queue Jul 28, 2025
Merged via the queue into paritytech:master with commit bd78cc5 Jul 28, 2025
254 of 259 checks passed
@serban300 serban300 deleted the hrmp-ordering-check branch July 28, 2025 15:48
paritytech-release-backport-bot bot pushed a commit that referenced this pull request Jul 28, 2025
Related to #8860

This PR adds a check in order to ensure that the collator has respected
the proper order when sending the HRMP messages to the runtime.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Andrii <ndk@parity.io>
(cherry picked from commit bd78cc5)
@paritytech-release-backport-bot
Copy link
Copy Markdown

Successfully created backport PR for unstable2507:

serban300 added a commit that referenced this pull request Jul 29, 2025
Backport #9326 into `unstable2507` from serban300.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

Co-authored-by: Serban Iorga <serban@parity.io>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Andrii <ndk@parity.io>
@acatangiu acatangiu added the A4-backport-stable2506 Pull request must be backported to the stable2506 release branch label Jul 29, 2025
@paritytech-release-backport-bot
Copy link
Copy Markdown

Created backport PR for stable2506:

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin backport-9326-to-stable2506
git worktree add --checkout .worktree/backport-9326-to-stable2506 backport-9326-to-stable2506
cd .worktree/backport-9326-to-stable2506
git reset --hard HEAD^
git cherry-pick -x bd78cc52b6271e2463b32de80410ab26602fc616
git push --force-with-lease

@paritytech-release-backport-bot
Copy link
Copy Markdown

Backport failed for unstable2507, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin unstable2507
git worktree add -d .worktree/backport-9326-to-unstable2507 origin/unstable2507
cd .worktree/backport-9326-to-unstable2507
git switch --create backport-9326-to-unstable2507
git cherry-pick -x bd78cc52b6271e2463b32de80410ab26602fc616

@serban300 serban300 removed the A4-backport-stable2506 Pull request must be backported to the stable2506 release branch label Jul 29, 2025
@redzsina redzsina moved this from Backlog to Waiting for fix in Security Audit (PRs) - SRLabs Aug 28, 2025
@redzsina redzsina moved this from Waiting for fix to Scheduled in Security Audit (PRs) - SRLabs Sep 2, 2025
@redzsina redzsina moved this from Scheduled to Waiting for fix in Security Audit (PRs) - SRLabs Sep 2, 2025
@redzsina redzsina moved this from Waiting for fix to Audited in Security Audit (PRs) - SRLabs Oct 16, 2025
alvicsam pushed a commit that referenced this pull request Oct 17, 2025
Related to #8860

This PR adds a check in order to ensure that the collator has respected
the proper order when sending the HRMP messages to the runtime.

---------

Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Andrii <ndk@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A4-backport-unstable2507 Pull request must be backported to the unstable2507 release branch T9-cumulus This PR/Issue is related to cumulus.

Projects

Status: Audited
Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants