Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

omni-node: add metadata checks for runtime/parachain compatibility #6450

Merged
merged 71 commits into from
Dec 12, 2024

Conversation

iulianbarbu
Copy link
Contributor

@iulianbarbu iulianbarbu commented Nov 12, 2024

Description

Get runtime's metadata, parse it and verify pallets list for a pallet named ParachainSystem (for now), and block number to be the same for both node and runtime. Ideally we'll add other pallets checks too, at least a small set of pallets we think right away as mandatory for parachain compatibility.
Closes: #5565

Integration

Runtime devs must be made aware that to be fully compatible with Omni Node, certain naming conventions should be respected when defining pallets (e.g we verify parachain-system pallet existence by searching for a pallet with name ParachainSystem in runtime's metadata). Not finding such a pallet will not influence the functionality yet, but by doing these checks we could provide useful feedback for runtimes that are clearly not implementing what's required for full parachain compatibility with Omni Node.

Review Notes

  • parachain system check
  • check frame_system's metadata to ensure the block number in there is the same as the one in the node side
  • add tests for the previous checking logic
  • update omni node polkadot-sdk docs to make these conventions visible.
  • add more pallets checks?

@iulianbarbu iulianbarbu self-assigned this Nov 12, 2024
@iulianbarbu iulianbarbu changed the title omni-node: check if parachain system pallet exists omni-node: add metadata checks for runtime/parachain compatibility Nov 12, 2024
Signed-off-by: Iulian Barbu <[email protected]>
@iulianbarbu iulianbarbu added the T9-cumulus This PR/Issue is related to cumulus. label Nov 15, 2024
@iulianbarbu iulianbarbu marked this pull request as ready for review November 18, 2024 16:30
iulianbarbu and others added 8 commits November 18, 2024 18:40
Signed-off-by: Iulian Barbu <[email protected]>
Signed-off-by: Iulian Barbu <[email protected]>
Signed-off-by: Iulian Barbu <[email protected]>
Signed-off-by: Iulian Barbu <[email protected]>
Signed-off-by: Iulian Barbu <[email protected]>
Signed-off-by: Iulian Barbu <[email protected]>
@iulianbarbu
Copy link
Contributor Author

iulianbarbu commented Dec 11, 2024

The PR is pending on fixing the Check publish build and Check semver jobs: paritytech/parity-publish#45 & paritytech/parity-publish#43 (although only check publish is required though). I did run parity-publish according to each failing job with each fix respectively and made the required changes as suggested (for check-semver, while for check-publish we just need to ensure commands run with success).

github-merge-queue bot pushed a commit that referenced this pull request Dec 12, 2024
# Description

Upgrades parity-publish with fixes for `Error: no dep` and panic
triggered when running `plan` and there is a new unpublished member
crate introduced in the workspace.

## Integration

N/A

## Review Notes

Context:
#6450 (comment)

Signed-off-by: Iulian Barbu <[email protected]>
@iulianbarbu iulianbarbu disabled auto-merge December 12, 2024 13:15
@iulianbarbu iulianbarbu added this pull request to the merge queue Dec 12, 2024
Merged via the queue into paritytech:master with commit 7cc5cdd Dec 12, 2024
198 of 199 checks passed
@iulianbarbu iulianbarbu deleted the ib-omni-node-ps-check branch December 12, 2024 13:54
github-actions bot pushed a commit that referenced this pull request Dec 13, 2024
# Description

This PR changes a few things:
* `--dev` flag will not conflict with `--chain` anymore, but if
`--chain` is not given will set `--chain=dev`.
* `--dev-block-time` is optional and it defaults to 3000ms if not set
after setting `--dev`.
* to start OmniNode with manual seal it is enough to pass just `--dev`.
* `--dev-block-time` can still be used to start a node with manual seal,
but it will not set it up as `--dev` does (it will not set a bunch of
flags which are enabled by default when `--dev` is set: e.g. `--tmp`,
`--alice` and `--force-authoring`.

Closes: #6537

## Integration

Relevant for node/runtime developers that use OmniNode lib, including
`polkadot-omni-node` binary, although the recommended way for runtime
development is to use `chopsticks`.

## Review Notes

* Decided to focus only on OmniNode & templates docs in relation to it,
and leave the `parachain-template-node` as is (meaning `--dev` isn't
usable and testing a runtime with the `parachain-template-node` still
needs a relay chain here). I am doing this because I think we want
either way to phase out `parachain-template-node` and adding manual seal
support for it is wasted effort. We might add support though if the
demand is for `parachain-template-node`.
* Decided to not infer the block time based on AURA config yet because
there is still the option of setting a specific block time by using
`--dev-block-time`. Also, would want first to align & merge on runtime
metadata checks we added in Omni Node here:
#6450 before starting to
infer AURA config slot duration via the same way.

- [x] update the docs to mention `--dev` now.
- [x] mention about chopsticks in the context of runtime development

---------

Signed-off-by: Iulian Barbu <[email protected]>
Co-authored-by: Michal Kucharczyk <[email protected]>
(cherry picked from commit 48c28d4)
@iulianbarbu iulianbarbu added the A4-needs-backport Pull request must be backported to all maintained releases. label Dec 13, 2024
@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2407:

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

git fetch origin backport-6450-to-stable2407
git worktree add --checkout .worktree/backport-6450-to-stable2407 backport-6450-to-stable2407
cd .worktree/backport-6450-to-stable2407
git reset --hard HEAD^
git cherry-pick -x 7cc5cdd0d98ae3466dc33b339197c169cf241fc0
git push --force-with-lease

@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2409:

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

git fetch origin backport-6450-to-stable2409
git worktree add --checkout .worktree/backport-6450-to-stable2409 backport-6450-to-stable2409
cd .worktree/backport-6450-to-stable2409
git reset --hard HEAD^
git cherry-pick -x 7cc5cdd0d98ae3466dc33b339197c169cf241fc0
git push --force-with-lease

@paritytech-cmd-bot-polkadot-sdk

Created backport PR for stable2412:

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

git fetch origin backport-6450-to-stable2412
git worktree add --checkout .worktree/backport-6450-to-stable2412 backport-6450-to-stable2412
cd .worktree/backport-6450-to-stable2412
git reset --hard HEAD^
git cherry-pick -x 7cc5cdd0d98ae3466dc33b339197c169cf241fc0
git push --force-with-lease

iulianbarbu added a commit that referenced this pull request Dec 13, 2024
…6450)

Get runtime's metadata, parse it and verify pallets list for a pallet
named `ParachainSystem` (for now), and block number to be the same for
both node and runtime. Ideally we'll add other pallets checks too, at
least a small set of pallets we think right away as mandatory for
parachain compatibility.
Closes: #5565

Runtime devs must be made aware that to be fully compatible with Omni
Node, certain naming conventions should be respected when defining
pallets (e.g we verify parachain-system pallet existence by searching
for a pallet with `name` `ParachainSystem` in runtime's metadata). Not
finding such a pallet will not influence the functionality yet, but by
doing these checks we could provide useful feedback for runtimes that
are clearly not implementing what's required for full parachain
compatibility with Omni Node.

- [x] parachain system check
- [x] check frame_system's metadata to ensure the block number in there
is the same as the one in the node side
- [x] add tests for the previous checking logic
- [x] update omni node polkadot-sdk docs to make these conventions
visible.
- [ ] add more pallets checks?

---------

Signed-off-by: Iulian Barbu <[email protected]>
Co-authored-by: Alexandru Vasile <[email protected]>
Co-authored-by: Michal Kucharczyk <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Dec 16, 2024
# Description

Redundant dep that made its way in #6450 . 😅. It can be
brought up when using `cargo udeps`. Added a github action that runs
`cargo udeps` on the repo too.

## Integration

N/A

## Review Notes

N/A

---------

Signed-off-by: Iulian Barbu <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Dec 17, 2024
#6450 introduced metadata checks. Supported are metadata v14 and higher.

However, of course old chain-specs have a genesis code blob that might
be on older version. This needs to be tolerated. We should just skip the
checks in that case.

Fixes #6921

---------

Co-authored-by: command-bot <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A4-needs-backport Pull request must be backported to all maintained releases. T9-cumulus This PR/Issue is related to cumulus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

polkadot-omni-node: Metadata checks
4 participants