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.

Sign up for GitHub

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

Jump to bottom

omni-node: --dev sets manual seal and allows --chain to be set #6646

Merged
merged 25 commits into from
Dec 10, 2024
Merged

omni-node: --dev sets manual seal and allows --chain to be set #6646

merged 25 commits into from
Dec 10, 2024

Conversation

iulianbarbu
Copy link
Contributor

@iulianbarbu iulianbarbu commented Nov 26, 2024 •
edited
Loading

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: omni-node: add metadata checks for runtime/parachain compatibility #6450 before starting to infer AURA config slot duration via the same way.
  • update the docs to mention --dev now.
  • mention about chopsticks in the context of runtime development

@iulianbarbu iulianbarbu added T9-cumulus This PR/Issue is related to cumulus. T17-Templates This PR/Issue is related to templates labels Nov 26, 2024
@iulianbarbu iulianbarbu self-assigned this Nov 26, 2024
@iulianbarbu
comments fixes
e321830 
Signed-off-by: Iulian Barbu <[email protected]>
@iulianbarbu
fix comments
922285c 
Signed-off-by: Iulian Barbu <[email protected]>
@iulianbarbu iulianbarbu marked this pull request as draft November 26, 2024 10:48
@iulianbarbu iulianbarbu changed the title omni-node: --dev starts manual seal and allows --chain to be set omni-node: --dev sets manual seal and allows --chain to be set Nov 26, 2024
@iulianbarbu iulianbarbu marked this pull request as ready for review November 27, 2024 00:37
@iulianbarbu iulianbarbu requested review from skunert and bkchr November 27, 2024 00:39
@iulianbarbu iulianbarbu mentioned this pull request Nov 27, 2024
Closed
2 tasks
@iulianbarbu
add choptsticks guide
f913cec 
Signed-off-by: Iulian Barbu <[email protected]>
@iulianbarbu iulianbarbu requested review from michalkucharczyk and a team November 27, 2024 13:43
skunert
skunert reviewed Nov 27, 2024
View reviewed changes
templates/parachain/README.md Outdated Show resolved Hide resolved
cumulus/polkadot-omni-node/lib/src/cli.rs Outdated Show resolved Hide resolved
michalkucharczyk
michalkucharczyk reviewed Nov 28, 2024
View reviewed changes
cumulus/polkadot-omni-node/lib/src/cli.rs Outdated
@@ -128,7 +128,8 @@ pub struct Cli<Config: CliConfig> {
///
/// This is a dev option, and it won't result in starting or connecting to a parachain network.
/// The resulting node will work on its own, running the wasm blob and artificially producing
/// a block each `dev_block_time` ms, as if it was part of a parachain.
/// a block each `dev_block_time` ms, as if it was part of a parachain. Defaults to 3000ms if
/// not set and `--dev` is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be more explicit here about enabling manual sealing and fact that --dev enables this.

/// Start a dev node that produces a block each `dev_block_time` ms.
///
/// This is a dev option. It enables a manual sealing, meaning blocks are produced manually 
/// rather than being part of an actual network consensus process. Using the option won't 
/// result in starting or connecting to a parachain network. The resulting node will work on
/// its own, running the wasm blob and artificially producing a block each `dev_block_time` ms, 
/// as if it was part of a parachain. 
///
/// The `--dev` flag sets the `dev_block_time` to a default value of 3000ms unless explicitly provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used your phrasing here: d6faae7

michalkucharczyk
michalkucharczyk reviewed Nov 28, 2024
View reviewed changes
substrate/client/cli/src/params/shared_params.rs Outdated
#[arg(long, conflicts_with_all = &["chain"])]
/// This flag sets `--chain=dev`, `--force-authoring`, `--rpc-cors=all`, `--alice`, and `--tmp`
/// flags, unless explicitly overridden. It also disables local peer discovery (see `--no-mdns`
/// and `--discover-local`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention manual seal here? (I know it does not enable it for every node, but I think it would be good to have this information in cli).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note here: d6faae7.

@iulianbarbu
polish public docs
d6faae7 
Signed-off-by: Iulian Barbu <[email protected]>
@iulianbarbu iulianbarbu requested review from a team as code owners November 29, 2024 17:14
michalkucharczyk
michalkucharczyk reviewed Dec 9, 2024
View reviewed changes
templates/parachain/runtime/src/configs/mod.rs
@@ -134,7 +133,7 @@ impl pallet_timestamp::Config for Runtime {
/// A timestamp: milliseconds since the unix epoch.
type Moment = u64;
type OnTimestampSet = Aura;
type MinimumPeriod = ConstU64<MINIMUM_PERIOD>;
type MinimumPeriod = ConstU64<0>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouf of curiosity - why this was set to 0?

Copy link
Contributor Author

@iulianbarbu iulianbarbu Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

At first, it seemed I couldn't run the parachain runtime based chain spec with chopsticks because of an error which IIRC stated that: "Timestamp slot must match CurrentSlot. This likely means that the configured block time in the node and/or rest of the runtime is not compatible with Aura's SlotDuration". After reading the code for a bit I thought it might have to do with the MinimumPeriod, which I changed to SLOT_DURATION / 2, resulting in the newly generated chain spec to work with chopsticks. I don't remember if at that time though I validated that changing the minimum period back to 0 and rebuilding everything made chopsticks fail again. Then rebuilding the runtime at a later day made things work without the MinimumPeriod change. I also tested this right now (removed the previous target folder) and things work as expected without the minimum period change. It might have been some inconsistent local state with my artifacts that caused the issue, but this begs for ensuring that the commands we recommend in the READMEs work in CI. Opened this: #6837.

@iulianbarbu iulianbarbu added this pull request to the merge queue Dec 10, 2024
Merged via the queue into paritytech:master with commit 48c28d4 Dec 10, 2024
200 of 203 checks passed
@iulianbarbu iulianbarbu deleted the ib-dev-flag-with-manual-seal branch December 10, 2024 23:21
@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 paritytech-cmd-bot-polkadot-sdk bot mentioned this pull request Dec 13, 2024
Closed
@paritytech-cmd-bot-polkadot-sdk
Copy link

Created backport PR for stable2407:

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

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

@paritytech-cmd-bot-polkadot-sdk paritytech-cmd-bot-polkadot-sdk bot mentioned this pull request Dec 13, 2024
Closed
@paritytech-cmd-bot-polkadot-sdk
Copy link

Created backport PR for stable2409:

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

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

github-actions bot pushed a commit that referenced this pull request Dec 13, 2024
@iulianbarbu @github-actions
omni-node: --dev sets manual seal and allows --chain to be set (#6646)
b12ca0f 
# 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)
@paritytech-cmd-bot-polkadot-sdk paritytech-cmd-bot-polkadot-sdk bot mentioned this pull request Dec 13, 2024
Merged
@paritytech-cmd-bot-polkadot-sdk
Copy link

Successfully created backport PR for stable2412:

EgorPopelyaev pushed a commit that referenced this pull request Dec 13, 2024
@paritytech-cmd-bot-polkadot-sdk @iulianbarbu
[stable2412] Backport #6646 (#6870)
ba9b9e5 
Backport #6646 into `stable2412` from iulianbarbu.

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: Iulian Barbu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skunert skunert skunert approved these changes

@michalkucharczyk michalkucharczyk michalkucharczyk approved these changes

@bkchr bkchr Awaiting requested review from bkchr

Assignees

@iulianbarbu iulianbarbu

Labels
A4-needs-backport Pull request must be backported to all maintained releases. T9-cumulus This PR/Issue is related to cumulus. T17-Templates This PR/Issue is related to templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve parachains node --dev flag experience
3 participants
@iulianbarbu @michalkucharczyk @skunert