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

Improve parachains node --dev flag experience #6537

Open
2 tasks done
iulianbarbu opened this issue Nov 19, 2024 · 15 comments · May be fixed by #6646
Open
2 tasks done

Improve parachains node --dev flag experience #6537

iulianbarbu opened this issue Nov 19, 2024 · 15 comments · May be fixed by #6646
Assignees
Labels
I5-enhancement An additional feature request.

Comments

@iulianbarbu
Copy link
Contributor

iulianbarbu commented Nov 19, 2024

Is there an existing issue?

  • I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

  • This is not a support question.

Motivation

There are a few issues with the current flow of how --dev flag works with parachain development:

  1. Starting omni node in development mode with --dev isn't supported because --dev and --chain can not be both provided at the same time to the node.
  2. Starting parachain-template-node with --dev requires polkadot-sdk to be built with a dependency on polkadot-cli?/rococo-native, since the dev chain spec of the polkadot-template-node uses rococo-local as relay chain.
  3. My understanding (based on a quick experiment) is also that polkadot-omni-node and polkadot-template-node would require a working relay chain network (1 validator at least? to be clarified), which if not created before the parachain node startup will result in block authoring/finalization issues.

Request

Unify & simplify the experience of how --dev flag works for OmniNode and regular parachain development - for what regular means we can refer to parachain-template-node and using it with --dev flag.

parachain-template-node might phase out with time, and only OmniNode usage might be relevant, but I think fixing/simplifying the --dev outcome for the parachain-template-node would be useful to ensure that all substrate/cumulus/omni-node/templates built-in behaviors converge when using --dev flag.

Solution

  1. Enable both --dev & --chain flags to work together for OmniNode, keeping in mind the experience shouldn't be very different from what we settle on as parachain-template-node --dev behavior and side effects, after we clarify how to approach point 3.
  2. Make polkadot-sdk (umbrella crate) std feature depend on polkadot-cli?/rococo-native, to enable the --dev flag usage with parachain-template-node.
  3. Not very sure about this one yet, but an opinion I've seen here is to defer to using a manual seal when passing --dev. Manual seal requires setting a block time though, and we could hardcode it to 3000ms (as I've seen in practice used in a few places). Open to other suggestions though. Other opinions I've seen were to depend on zombienet to handle --dev flag accordingly (as in detecting that parachain node has such a flag and setup a relaychain network accordingly), or make a similar thing within the node itself, or mock relay chain network dependencies (which needs more scoping).

I will follow up with a more concise plan here once we'll confirm the problem statement looks right, and the solutions are sufficiently scoped for a decision.

Are you willing to help with this request?

Yes!

@iulianbarbu iulianbarbu added the I5-enhancement An additional feature request. label Nov 19, 2024
@iulianbarbu iulianbarbu self-assigned this Nov 19, 2024
@bkchr
Copy link
Member

bkchr commented Nov 19, 2024

2. Make polkadot-sdk (umbrella crate) std feature depend on polkadot-cli?/rococo-native, to enable the --dev flag usage with parachain-template-node.

This should not be required when you use manual-seal, as you will not require the relay chain.

3. Manual seal requires setting a block time though, and we could hardcode it to 3000ms (as I've seen in practice used in a few places)

Not sure for what you need a block time, because manual seal only does something when you ask it to do something. At some point we had an issue that manual seal actually build blocks automatically when there isn't a tx send for some time, but we never implemented it. If you need some block time, you could try to fetch it from Aura_slot_duration.

@DrW3RK
Copy link
Contributor

DrW3RK commented Nov 20, 2024

For point 3:

Ideally, there should be a flag to see the parachain in action, like a solochain, and test the runtime features that have been developed. A parachain runtime developer could use this and just focus on the runtime development and testing, without running any validator in the background.

@xlc
Copy link
Contributor

xlc commented Nov 20, 2024

runtime devs can just use chopsticks

@DrW3RK
Copy link
Contributor

DrW3RK commented Nov 21, 2024

@xlc Fair point. It would be nice to integrate Chopsticks into the node repo and be invoked with a flag.

@iulianbarbu
Copy link
Contributor Author

This should not be required when you use manual-seal, as you will not require the relay chain.

I am fine with scratching 2 in the context of enabling manual seal on parachain-template-node side when used with --dev.

Not sure for what you need a block time, because manual seal only does something when you ask it to do something.

Yes, for OmniNode and minimal-template-node we tell it to do something with a timer which triggers the block sealing continuously at certain intervals.

If you need some block time, you could try to fetch it from Aura_slot_duration

I will check it out, we should be able to assume that OmniNode works with runtimes that use AURA only (for the time being) and parachain-template-runtime already uses it. If AURA isn't configured, we could emit a warning and default to a value (but we can also standardize determining block times based on common usecases other than AURA, and the default choice - or configurable block time, which I wouldn't worry for right now - would be for niche use cases). WDYT @bkchr ?

@bkchr
Copy link
Member

bkchr commented Nov 21, 2024

runtime devs can just use chopsticks

I'm telling this to people as well ;)

If AURA isn't configured, we could emit a warning and default to a value (but we can also standardize determining block times based on common usecases other than AURA, and the default choice - or configurable block time, which I wouldn't worry for right now - would be for niche use cases). WDYT @bkchr ?

Sounds good to me

@iulianbarbu
Copy link
Contributor Author

@xlc @DrW3RK I am not super familiar with chopsticks. My understanding is that it needs a live chain and it will fork its current state and work with it locally. In the case of non-live chain, I think we should start a local chain/node based on the runtime we want to test, and then point chopsticks to that (and then, to start the local node, we use --dev, which behind the scenes uses manual seal). LMK if such setup makes sense to you at least for the start. If it makes sense, I can focus on this DX for the scope of this issue.

It would be nice to integrate Chopsticks into the node repo and be invoked with a flag.

Integrating chopsticks more into the node, by instructing it via a flag to start in --dev mode and also start chopsticks with the correct configuration that points to it can be another follow up. I would focus in this issue on fixing --dev to some extent for both OmniNode and parachain-template-node (and possibly how it is perceived in substrate/cumulus), and optionally add chopsticks docs for usage with a local node started with --dev in the parachain-template. WDYT?

@iulianbarbu iulianbarbu changed the title Improve parachains --dev flag experience Improve parachains node --dev flag experience Nov 21, 2024
@xlc
Copy link
Contributor

xlc commented Nov 21, 2024

you can run a dev chain from chain spec using chopsticks. we may need to improve the docs but there isnt much reason to use omni node when developing runtime

@iulianbarbu
Copy link
Contributor Author

you can run a dev chain from chain spec using chopsticks. we may need to improve the docs but there isnt much reason to use omni node when developing runtime

I see. In this case I am not super sure why --dev is needed. It might be for some non-runtime development related reason. @bkchr any idea?

@iulianbarbu
Copy link
Contributor Author

iulianbarbu commented Nov 22, 2024

you can run a dev chain from chain spec using chopsticks. we may need to improve the docs but there isnt much reason to use omni node when developing runtime

I see. In this case I am not super sure why --dev is needed. It might be for some non-runtime development related reason. @bkchr any idea?

Leaving here a brain dump with my perspective over --dev, which I am hoping it will also help with alignment:

Regular OmniNode usage with --dev

  1. If people use the shipped polkadot-omni-node binary as is they are probably more interested in runtime development, and for this case chopsticks makes a better debugging companion. We should direct people to chopsticks in this case.

  2. polkadot-omni-node has the --dev-block-time flag which makes the node start with manual seal. This overlaps with our intention of making --dev start the node with manual seal. To not have two flags doing the same thing I am thinking about deprecating --dev-block-time and rely on --dev to achieve the same thing but slightly better (since we'd use AURA config to determine the block-time that is used to set the intervals when block sealing happens, and if not present, set a default for it, and possibly in the future if needed, make this default configurable by passing it in the chain-spec file).

  3. Then there is polkadot-omni-node-lib case, where people develop certain nodes when polkadot-omni-node isn't enough, so in this case --dev makes more sense, and if used, it should act similarly to how polkadot-omni-node acts, but it could be extended by the node devs as they need.

Other --dev usage (e.g. parachain-template-node)

--dev is part of SharedParams struct in substrate/client/cli/config::CliConfiguration, and is used across multiple places. We can understand its scope by following & understanding its usage in these multiple locations. I think we can document this, and even go for a node/runtime development workflows kind of doc, at least in the templates.

@bkchr
Copy link
Member

bkchr commented Nov 24, 2024

I see. In this case I am not super sure why --dev is needed. It might be for some non-runtime development related reason. @bkchr any idea?

Chopsticks is just not that old ;) --dev exists much longer and was mainly used when wanting to run a node for local development.

2. polkadot-omni-node has the --dev-block-time flag which makes the node start with manual seal. This overlaps with our intention of making --dev start the node with manual seal. To not have two flags doing the same thing I am thinking about deprecating --dev-block-time and rely on --dev to achieve the same thing but slightly better (since we'd use AURA config to determine the block-time that is used to set the intervals when block sealing happens, and if not present, set a default for it, and possibly in the future if needed, make this default configurable by passing it in the chain-spec file).

If we have that already, make --dev enable manual seal and call it a day. All the discussions here are costing more time than doing this change. In the docs I would then mention chopsticks.

@kianenigma
Copy link
Contributor

High level I want to ack that the future of local development is chopsticks, ergo why I have noted it here as something to fully integrate into our templates. Yet, given the history of something like --dev, it would be good to have both options for some time. Also, I am not sure about feature-parity yet.

we can in the meantime print an info log in appropriate times to "suggest" people to using chopsticks instead as well.

@iulianbarbu
Copy link
Contributor Author

@DrW3RK , can you tell where this --dev flag is more relevant?

I opened #6646 which adds support on OmniNode side for --dev, and thought that since we're aiming to phase out parachain-template-node and replace it with OmniNode, it is wasted effort to add manual seal there and make it support --dev with it. Happy to reconsider this though if there is a stronger opinion for it.

Separately, I am trying to suggest clearer chopsticks usage in the templates docs for runtime development, but so far I am finding it a bit difficult to point to some clear docs that make use of chopsticks in a setup where it would start from a chain spec.

@DrW3RK
Copy link
Contributor

DrW3RK commented Nov 27, 2024

Hey @iulianbarbu, I defer this to others in this discussion who are more knowledgeable about the context of the node flags. I'm okay with just using Chopsticks for runtime development and not using the —-dev flag on the parachain node.

Separately, I am trying to suggest clearer chopsticks usage in the templates docs for runtime development, but so far I am finding it a bit difficult to point to some clear docs that make use of chopsticks in a setup where it would start from a chain spec.

Thanks for pointing this out. To test runtime feature changes on a parachain that is live on a relay chain, the --wasm-override flag shown here should help https://github.com/ajuna-network/bajun-parachain?tab=readme-ov-file#test-runtime-upgrades I do not know if Chopsticks instance can be started with chainspec as input. It will require these config files https://github.com/AcalaNetwork/chopsticks/tree/master/configs

@iulianbarbu
Copy link
Contributor Author

iulianbarbu commented Nov 27, 2024

Hey @iulianbarbu, I defer this to others in this discussion who are more knowledgeable about the context of the node flags. I'm okay with just using Chopsticks for runtime development and not using the —-dev flag on the parachain node.

Thanks for confirming the current proposal sounds good! We can follow up with more parachain-template-node --dev support later if it is requested more.

Will double down a bit on my decision by mentioning what I considered (please anyone correct me if I am wrong):

  • people should use/migrate to OmniNode if they are just starting developing or they use the parachain-template-node as is.
  • existing teams that don't fall under the previous case will probably implement their own node and decide on the development approach they find fit, but either way chopsticks is still there for them, maybe --dev flag is there too, but they'll have to wrap around the existing logic that depends on --dev flag (from substrate/cumulus), and add their own knobs too.

Thanks for pointing this out. To test runtime feature changes on a parachain that is live on a relay chain, the --wasm-override flag shown here should help https://github.com/ajuna-network/bajun-parachain?tab=readme-ov-file#test-runtime-upgrades I do not know if Chopsticks instance can be started with chainspec as input. It will require these config files https://github.com/AcalaNetwork/chopsticks/tree/master/configs

Thanks for the materials! I found out it is also possible to directly use a chain spec. You can check out the comments started from this. For parachain-template-runtime to start with chopsticks we also need to set the pallet_timestamp config's MinimumPeriod to half of the configured slot_duration of the AURA pallet. Once that's done, and using the --allow-unresolved-imports=true as @xlc mentioned, the runtime should start fine. Updated the parachain-template README.md with more details here: f913cec.

I am getting an output like below (we're getting an RPC endpoint that can be used to drive the state forward):

[12:43:30.921] INFO (block-builder): custom building #1
    app: "chopsticks"
    number: 1
    extrinsics: []
    umpCount: 0
[12:43:31.194] INFO (block-builder): custom new head #1
    app: "chopsticks"
    number: 1
    hash: "0xc5cbee54c812d0845b1bd983d259b8c319de0650827ab651236ab6155e91f5f9"
    extrinsics: []
    pendingExtrinsics: []
    ump: {}
[12:43:31.246] INFO: custom RPC listening on http://[::]:8000 and ws://[::]:8000
    app: "chopsticks"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants