`polkadot-parachain-bin`: small cosmetics and improvements #4666

serban300 · 2024-06-01T14:25:24Z

Related to: #5

A couple of cosmetics and improvements related to polkadot-parachain-bin:

Adding some convenience traits in order to avoid declaring long duplicate bounds
Specifically check if the runtime exposes AuraApi when executing start_lookahead_aura_consensus()
Some fixes for the RelayChainCli. Details in the commits description

kianenigma · 2024-06-03T09:01:39Z

cumulus/polkadot-parachain/src/service.rs

@@ -251,6 +235,11 @@ where
 let client = params.client.clone();
 let backend = params.backend.clone();

+ let info = backend.blockchain().info();
+ if !client.runtime_api().has_basic_apis(info.genesis_hash) {


I am not sure if this works atm, some of these system chains were initially run as Shell and might not have the aura api in the initial blocks afaik.

Alternatively, the changes in https://github.com/paritytech/polkadot-sdk/tree/kiz-omni-node is one way to keep this crate mostly unchanged, and only enable the omni-node when an unknonw chain-spec is given.

We could subsume that branch here.

Specifically, see how I have replaced enum Runtime::Default with Runtime::Omni, and all new codes are constrained to this code path.

Tbh, solely because of these legacy decisions in system chains and then requiring special code such as WaitForAuraConsensus, I am in favour of not touching polkadot-parachain, and starting a fresh new omni-node crate, keep polkadot-parachain unchanged, and drop-in replace it once done. @bkchr iirc you had some reservations about this, wdyt?

Good point. You're right. It wasn't working when syncing system parachains. Removed the has_basic_apis() check and also the check for AuraUnincludedSegmentApi.

Maybe we can add has_basic_apis() back somewhere after upgrading from shell. I don't know. But probably it would also require some refactoring in order to make it fit. We can revisit it in the future. For the moment this PR remains mainly refactoring with the only functional change being the added check for AuraApi inside start_lookahead_aura_consensus().

I am not sure if this works atm, some of these system chains were initially run as Shell and might not have the aura api in the initial blocks afaik.

But that only applies to asset hub runtimes, which is why they use the separate start_asset_hub_lookahead_node() entry point. So, might it still be okay to keep that check in the general start_generic_aura_lookahead_node()?

The problem is that we would have to perform this check somewhere after new_partial() was called. And I don't think there is a generic way to do this now just for start_asset_hub_lookahead_node(). I think it would be better to do it in a future PR together with some more refactoring.

- Parse the `RelayChainCli` before starting the runner. This is helpful in case of executing `polkadot-parachain ... -- --help` or if the relay chain command is malformed - Set `no_binary_name(true)` - Use the same `base_path` as the parachain This will lead to using paths like for example: - /tmp/substratexaqpXv/chains/bridge-hub-kusama - /tmp/substratexaqpXv/chains/kusama instead of: - /tmp/substratexaqpXv/chains/bridge-hub-kusama - /tmp/substratexaqpXv/polkadot/chains/kusama

For consistency. For example `start_node()` uses `FullNetworkConfiguration::<_, _, Net>::new(&parachain_config.network)`

bkchr · 2024-06-12T08:40:08Z

cumulus/polkadot-parachain/common/Cargo.toml

@@ -0,0 +1,25 @@
+[package]


This entire crate is not really needed. If you want to have these aliases, declare them in the polkadot parachain binary.

My intention with this crate is to slowly start moving generic logic into it. It would be the equivalent of the cumulus-service crate in Kian's omni-node PR: #3597 . I started with defining these aliases here and then I would like to slowly start moving parts of the start_node logic, while also refactoring them. So that then we could reuse this logic for polkadot-parachain, the parachain template, the solochain template, and also for the parachain teams to be able to build custom parachains with it. WDYT ?

They are not required. I already assumed that you wanted to do this. For now we should focus on the omni node and node some non existing builder. So, either remove or move to the binary directly.

They are not required. I already assumed that you wanted to do this. For now we should focus on the omni node and node some non existing builder. So, either remove or move to the binary directly.

@bkchr I don't agree your opinion here, please hear me out:

In order to get out of the technical debt of the node side, we inevitably need cumulus-service or omni-node-common or similar to exist.

A lot of the code between the 3 templates + polkadot-parachain is shared, but in reality is is copy-pasted because we don't have a shared crate. And reusable compoents (like, "build an aura import queue for me please") are the long hanging fruit way to make improvements to this.

I personally think it makes for a good learning objective for @serban300 to start thinking about such refactors. The time he would spend doing this refactor is well worth it + he can demonstrate that he can already simplify the code of the templates as well with the same traits and helpers that he has added to this PR.

I think your intention in asking to revert these refactors is speeding up the release of the omni-node itself, but I don't think this will speed it up by much. @serban300 and I already have another branch ready with release-able changes, which will come right after this PR. The main reason I am holding back on the above branch is to do more testing with more teams etc, not coding or engineering effort.

I won't insist on revering the changes in this PR per-se, but I hope we have a consensus to encourage such refactors in the coming PRs related to omni-node, as it serves our long term goals.

I would like to merge this PR in order to then continue the work related to renaming polkadot-parachain on top of it. But let's continue this discussion and use the conclusion in the following PRs

cumulus/polkadot-parachain/src/cli.rs

cumulus/polkadot-parachain/src/service.rs

s0me0ne-unkn0wn

Nice cleanup, thank you! I'd still run a manual test to make sure the asset hub is functional and can sync from scratch, just in case.

And move logic to polakdot-parachain

serban300 · 2024-06-13T06:47:08Z

Nice cleanup, thank you! I'd still run a manual test to make sure the asset hub is functional and can sync from scratch, just in case.

Thank you ! Yes, syncing polkadot-asset-hub from scratch. Seems ok so far, but letting it fully sync.

serban300 · 2024-06-14T06:27:52Z

Nice cleanup, thank you! I'd still run a manual test to make sure the asset hub is functional and can sync from scratch, just in case.

Thank you ! Yes, syncing polkadot-asset-hub from scratch. Seems ok so far, but letting it fully sync.

Synced polkadot-asset-hub and also polkadot-bridge-hub partially and everything worked correctly

Related to: #5 A couple of cosmetics and improvements related to `polkadot-parachain-bin`: - Adding some convenience traits in order to avoid declaring long duplicate bounds - Specifically check if the runtime exposes `AuraApi` when executing `start_lookahead_aura_consensus()` - Some fixes for the `RelayChainCli`. Details in the commits description

…h#4666) Related to: paritytech#5 A couple of cosmetics and improvements related to `polkadot-parachain-bin`: - Adding some convenience traits in order to avoid declaring long duplicate bounds - Specifically check if the runtime exposes `AuraApi` when executing `start_lookahead_aura_consensus()` - Some fixes for the `RelayChainCli`. Details in the commits description

serban300 self-assigned this Jun 1, 2024

serban300 added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. labels Jun 1, 2024

serban300 force-pushed the polkadot-parachain-service-2 branch from 8531cce to 8905ab5 Compare June 3, 2024 08:10

kianenigma reviewed Jun 3, 2024

View reviewed changes

serban300 marked this pull request as ready for review June 3, 2024 11:01

serban300 changed the title ~~[Draft] Add extra startup checks to polkadot-parachain-bin~~ Add extra startup checks to polkadot-parachain-bin Jun 3, 2024

serban300 marked this pull request as draft June 4, 2024 07:07

serban300 changed the title ~~Add extra startup checks to polkadot-parachain-bin~~ [Draft] Add extra startup checks to polkadot-parachain-bin Jun 4, 2024

serban300 changed the title ~~[Draft] Add extra startup checks to polkadot-parachain-bin~~ [WIP] Add extra startup checks to polkadot-parachain-bin Jun 4, 2024

serban300 force-pushed the polkadot-parachain-service-2 branch from 8c16afb to 1e5aa34 Compare June 10, 2024 07:43

serban300 added 4 commits June 11, 2024 10:36

Remove unused code

2dd7b91

Add polkadot-parachain-common crate

c30df86

Add convenience traits

493c1f1

Add Aura API check

c1d213c

serban300 force-pushed the polkadot-parachain-service-2 branch from 71b551d to 9c6ea89 Compare June 11, 2024 08:10

serban300 added 2 commits June 11, 2024 13:35

Use the network backend from the parachain config

6bfdebd

For consistency. For example `start_node()` uses `FullNetworkConfiguration::<_, _, Net>::new(&parachain_config.network)`

serban300 force-pushed the polkadot-parachain-service-2 branch from 9c6ea89 to 6bfdebd Compare June 11, 2024 10:45

serban300 changed the title ~~[WIP] Add extra startup checks to polkadot-parachain-bin~~ [WIP] polkadot-parachain-bin: small cosmetics and improvements Jun 12, 2024

serban300 changed the title ~~[WIP] polkadot-parachain-bin: small cosmetics and improvements~~ polkadot-parachain-bin: small cosmetics and improvements Jun 12, 2024

serban300 marked this pull request as ready for review June 12, 2024 07:18

Fix umbrella crate

f4e415d

bkchr reviewed Jun 12, 2024

View reviewed changes

s0me0ne-unkn0wn approved these changes Jun 12, 2024

View reviewed changes

serban300 added 2 commits June 12, 2024 15:49

Revert base_path change

597067f

Remove polkadot-parachains-common crate

b517d76

And move logic to polakdot-parachain

kianenigma approved these changes Jun 13, 2024

View reviewed changes

serban300 added this pull request to the merge queue Jun 14, 2024

Merged via the queue into paritytech:master with commit 7f7f5fa Jun 14, 2024
158 of 160 checks passed

serban300 deleted the polkadot-parachain-service-2 branch June 14, 2024 06:57

This was referenced Aug 21, 2024

Update polkadot-sdk from v1.11.0 to stable2407 moondance-labs/tanssi#659

Open

Update polkadot-sdk from v1.11.0 to stable2407 moonbeam-foundation/moonbeam#2912

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`polkadot-parachain-bin`: small cosmetics and improvements #4666

`polkadot-parachain-bin`: small cosmetics and improvements #4666

serban300 commented Jun 1, 2024 •

edited

Loading

kianenigma Jun 3, 2024

kianenigma Jun 3, 2024

kianenigma Jun 3, 2024

serban300 Jun 3, 2024 •

edited

Loading

s0me0ne-unkn0wn Jun 12, 2024

serban300 Jun 12, 2024

bkchr Jun 12, 2024

serban300 Jun 12, 2024

bkchr Jun 12, 2024

serban300 Jun 12, 2024

kianenigma Jun 13, 2024

serban300 Jun 14, 2024

s0me0ne-unkn0wn left a comment

serban300 commented Jun 13, 2024

serban300 commented Jun 14, 2024

polkadot-parachain-bin: small cosmetics and improvements #4666

polkadot-parachain-bin: small cosmetics and improvements #4666

Conversation

serban300 commented Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s0me0ne-unkn0wn left a comment

Choose a reason for hiding this comment

serban300 commented Jun 13, 2024

serban300 commented Jun 14, 2024

`polkadot-parachain-bin`: small cosmetics and improvements #4666

`polkadot-parachain-bin`: small cosmetics and improvements #4666

serban300 commented Jun 1, 2024 •

edited

Loading

serban300 Jun 3, 2024 •

edited

Loading