`polkadot-parachain` simplifications and deduplications by serban300 · Pull Request #4916 · paritytech/polkadot-sdk

serban300 · 2024-07-01T06:08:10Z

polkadot-parachain simplifications and deduplications

Details in the commit messages. Just copy-pasting the last commit description since it introduces the biggest changes:

    Implement a more structured way to define a node spec
    
    - use traits instead of bounds for `rpc_ext_builder()`,
      `build_import_queue()`, `start_consensus()`
    - add a `NodeSpec` trait for defining the specifications of a node
    - deduplicate the code related to building a node's components /
      starting a node

The other changes are much smaller, most of them trivial and are isolated in separate commits.

cumulus/polkadot-parachain/src/common/command.rs

kianenigma

Looks broadly good, would be great to get this in and on top of it build support for manual-seal.

- use traits instead of bounds for `rpc_ext_builder()`, `build_import_queue()`, `start_consensus()` - add a `NodeSpec` trait for defining the specifications of a node - deduplicate the code related to building a node's components / starting a node

skunert

In general the new structure seems reasonable to me.

One use-case I was thinking about during the review is temporary or experimental features. One example is a --experimental-use-slot-based command line flag that will enable a new experimental collator.

Previously I was just adding this flag and based on it choosing the closure to start consensus, here. With the model proposed here stuff like this gets a bit harder, I would need to implement StartConsensus and a new NodeSpec that uses it.

One reasonable addition would be to have some struct like extra_args (name is shitty 😬 ) that gets added to the parameters of the associated types.

skunert · 2024-07-03T13:21:12Z

cumulus/polkadot-parachain/src/rpc.rs

 /// A type representing all RPC extensions.
 pub type RpcExtension = jsonrpsee::RpcModule<()>;

+pub trait BuildRpcExtensions<Client, Backend, Pool> {


IMO we can move the impls of this trait to this module too. The impls of this trait mostly do nothing and just call the methods of this module. And instantiate the FullDeps struct which I think we can get rid of.

Yes, looks better. And service.rs is pretty big anyway, so it helps to move some of the logic. Done.

cumulus/polkadot-parachain/src/command.rs

bkchr

Generally the idea of getting rid off the macros is a good idea. However, I think we can simplify stuff even more.

cumulus/polkadot-parachain/src/service.rs

…n-refactoring

serban300 · 2024-07-04T09:23:12Z

One use-case I was thinking about during the review is temporary or experimental features. One example is a --experimental-use-slot-based command line flag that will enable a new experimental collator.

Previously I was just adding this flag and based on it choosing the closure to start consensus, here. With the model proposed here stuff like this gets a bit harder, I would need to implement StartConsensus and a new NodeSpec that uses it.

One reasonable addition would be to have some struct like extra_args (name is shitty 😬 ) that gets added to the parameters of the associated types.

Yes, sounds good. The extra_args struct should be the right solution in this case. But I wouldn't add it now since it would just be an unused empty struct. I would add it together with the --experimental-use-slot-based logic. If you merge #4097 first, I can then add it here when I fix the conflicts. Or if this PR gets merged first, please can you add it in #4097 ?

serban300 · 2024-07-04T09:25:30Z

@kianenigma @skunert @bkchr thanks for the reviews ! I addressed all your comments. Can you please take another look when you have time ?

kianenigma · 2024-07-04T11:37:04Z

cumulus/polkadot-parachain/src/rpc.rs

+		_backend: Arc<ParachainBackend>,
+		_pool: Arc<sc_transaction_pool::FullPool<Block, ParachainClient<RuntimeApi>>>,
+	) -> sc_service::error::Result<RpcExtension> {
+		Ok(RpcExtension::new(()))


Is this useful in any code path? I can't imagine it being used, although I have not fully looked at the PR.

Yes, it's used for the shell runtime. Although I don't know. Maybe we could use the BuildParachainRpcExtensions there as well ?

Nah I think the Shell doesn't have all the runtime apis that are needed. So it makes sense to have this for now. Later on, I hope we can get rid of shell in general :)

btw about RPCs and runtime apis, Bryan and I recently wrote: https://paritytech.github.io/polkadot-sdk/master/polkadot_sdk_docs/reference_docs/custom_runtime_api_rpc/index.html

cumulus/polkadot-parachain/src/rpc.rs

cumulus/polkadot-parachain/src/service.rs

kianenigma · 2024-07-04T11:44:59Z

cumulus/polkadot-parachain/src/service.rs

+		cmd.run(partial.client)
+	}
+
+	#[cfg(any(feature = "runtime-benchmarks"))]


I have raised this in another PR as well, high level my understanding is that we should be able to already remove all traces of benchmarking from normal nodes, not to mention the omni-node. @ggwpez any comments from you, re. how far we are from this?

@ggwpez , can you take a look on the question above please ?

I would like to merge this PR since it's pretty verbose and I would like to avoid merge conflicts. I will address this point in a future PR.

kianenigma

Looks significantly better! Some comments left, but all are thoughts for the future.

skunert · 2024-07-04T15:46:35Z

One use-case I was thinking about during the review is temporary or experimental features. One example is a --experimental-use-slot-based command line flag that will enable a new experimental collator.

Previously I was just adding this flag and based on it choosing the closure to start consensus, here. With the model proposed here stuff like this gets a bit harder, I would need to implement StartConsensus and a new NodeSpec that uses it.

One reasonable addition would be to have some struct like extra_args (name is shitty 😬 ) that gets added to the parameters of the associated types.

Yes, sounds good. The extra_args struct should be the right solution in this case. But I wouldn't add it now since it would just be an unused empty struct. I would add it together with the --experimental-use-slot-based logic. If you merge #4097 first, I can then add it here when I fix the conflicts. Or if this PR gets merged first, please can you add it in #4097 ?

Lets try to merge #4097 first and then integrate here.

…n-refactoring

serban300 · 2024-07-08T06:55:15Z

Lets try to merge #4097 first and then integrate here.

@skunert Done. I integrated #4097 into this PR. PTAL !

For the experimental_use_slot_based_flag I added a new_aura_node_spec constructor that receives the flag and then it just plugs-in the required StartConsensus into the AuraNode. Please check this commit. It seemed simpler than providing an extra_args param to all the associated traits.

bkchr · 2024-07-08T07:25:58Z

For the experimental_use_slot_based_flag I added a new_aura_node_spec constructor that receives the flag and then it just plugs-in the required StartConsensus into the AuraNode

Please make it a struct from the beginning. In the future that will be much easier to forward there stuff if we have a struct.

Also, please use if for checking booleans :D

skunert

I agree that the extra args should be a struct.

Once that is addressed, good to merge from my side, nice!

serban300 · 2024-07-08T16:08:14Z

Please make it a struct from the beginning. In the future that will be much easier to forward there stuff if we have a struct.

I agree that the extra args should be a struct.

Done

Also, please use if for checking booleans :D

Done

bkchr

One last nitpick. Then we are good to go.

cumulus/polkadot-parachain/src/service.rs

cumulus/polkadot-parachain/src/cli.rs

bkchr · 2024-07-08T19:44:37Z

cumulus/polkadot-parachain/src/command.rs

+		Runtime::Penpal(_) =>
+			new_aura_node_spec::<AuraRuntimeApi, AuraId>(extra_args.experimental_use_slot_based),
+		Runtime::Shell | Runtime::Seedling => Box::new(ShellNode),
+		Runtime::Glutton => Box::new(BasicLookaheadNode),


Suggested change

Runtime::Glutton => Box::new(BasicLookaheadNode),

cumulus/polkadot-parachain/src/command.rs

serban300 · 2024-07-09T07:11:00Z

I see that the following tests are failing:

gitlab-zombienet-cumulus-0007-full_node_warp_sync
gitlab-zombienet-substrate-0002-validators-warp-sync
gitlab-zombienet-substrate-0003-block-building-warp-sync

But doesn't seem related to this PR. They are also failing in all latest PRs (https://github.com/paritytech/polkadot-sdk/pulls) with the same errors:

2024-07-08T22:49:44.045Z zombie::network-node current value: undefined for metric substrate_beefy_best_block, keep trying...

2024-07-08T23:11:03.075Z zombie::network-node current value: undefined for metric block height, keep trying...

bkchr

Ty @serban300!

bkchr · 2024-07-09T07:51:03Z

I see that the following tests are failing:
* gitlab-zombienet-cumulus-0007-full_node_warp_sync

* gitlab-zombienet-substrate-0002-validators-warp-sync

* gitlab-zombienet-substrate-0003-block-building-warp-sync
But doesn't seem related to this PR. They are also failing in all latest PRs (https://github.com/paritytech/polkadot-sdk/pulls) with the same errors:
2024-07-08T22:49:44.045Z zombie::network-node current value: undefined for metric substrate_beefy_best_block, keep trying...
2024-07-08T23:11:03.075Z zombie::network-node current value: undefined for metric block height, keep trying...

The BEEFY errors are not related to your changes here. Generally these tests are quite flaky..

) `polkadot-parachain` simplifications and deduplications Details in the commit messages. Just copy-pasting the last commit description since it introduces the biggest changes: ``` Implement a more structured way to define a node spec - use traits instead of bounds for `rpc_ext_builder()`, `build_import_queue()`, `start_consensus()` - add a `NodeSpec` trait for defining the specifications of a node - deduplicate the code related to building a node's components / starting a node ``` The other changes are much smaller, most of them trivial and are isolated in separate commits.

* master: (120 commits) network/tx: Ban peers with tx that fail to decode (#5002) Try State Hook for Bounties (#4563) [statement-distribution] Add metrics for distributed statements in V2 (#4554) added sync command (#4818) Bridges V2 refactoring backport and `pallet_bridge_messages` simplifications (#4935) xcm-executor: Improve logging (#4996) Remove usage of `sp-std` on templates (#5001) fixed cmd bot commenting not working (#5000) Explain usage of `<T: Config>` in FRAME storage + Update parachain pallet template (#4941) Expose metadata-hash feature from polkadot crate (#4886) Add `MAX_INSTRUCTIONS_TO_DECODE` to XCMv2 (#4978) add notices to the implementer's guide docs that changed for elastic scaling (#4983) `polkadot-parachain` simplifications and deduplications (#4916) Update Templates README docs (#4980) allow clear_origin in safe xcm builder (#4777) litep2p/peerstore: Fix bump last updated time (#4971) Make `tracing::log` work in the runtime (#4863) sp-core: Improve docs generated by `generate_feature_enabled_macro` (#4968) [Backport] Version bumps and prdocs reordering from 1.14.0 (#4955) Assets: can_decrease/increase for destroying asset is not successful (#3286) ...

) `polkadot-parachain` simplifications and deduplications Details in the commit messages. Just copy-pasting the last commit description since it introduces the biggest changes: ``` Implement a more structured way to define a node spec - use traits instead of bounds for `rpc_ext_builder()`, `build_import_queue()`, `start_consensus()` - add a `NodeSpec` trait for defining the specifications of a node - deduplicate the code related to building a node's components / starting a node ``` The other changes are much smaller, most of them trivial and are isolated in separate commits.

serban300 added R0-no-crate-publish-required The change does not require any crates to be re-published. T0-node This PR/Issue is related to the topic “node”. labels Jul 1, 2024

serban300 self-assigned this Jul 1, 2024

serban300 added 4 commits July 1, 2024 14:18

Simplifications and fixes

436c4a8

Simplify start_node_impl() return type

283d474

Deduplicate create_runner() logic

270060b

Deduplicate construct_partials!() and construct_async_run!()

0121d3c

serban300 changed the title ~~[WIP] polkadot-parachain simplifications and deduplications~~ polkadot-parachain simplifications and deduplications Jul 1, 2024

serban300 marked this pull request as ready for review July 1, 2024 11:34

kianenigma reviewed Jul 1, 2024

View reviewed changes

cumulus/polkadot-parachain/src/common/command.rs Outdated Show resolved Hide resolved

kianenigma approved these changes Jul 1, 2024

View reviewed changes

acatangiu requested a review from a team July 2, 2024 11:41

serban300 requested a review from kianenigma July 2, 2024 14:33

serban300 mentioned this pull request Jul 3, 2024

polkadot-parachain: Unify asset-hub authoring codepath with general AURA #4932

Merged

serban300 requested a review from skunert July 3, 2024 09:59

skunert reviewed Jul 3, 2024

View reviewed changes

Remove unneeded Net generic for DynNodeSpec

9aa7818

bkchr reviewed Jul 3, 2024

View reviewed changes

cumulus/polkadot-parachain/src/service.rs Outdated Show resolved Hide resolved

cumulus/polkadot-parachain/src/service.rs Outdated Show resolved Hide resolved

cumulus/polkadot-parachain/src/service.rs Outdated Show resolved Hide resolved

serban300 added 2 commits July 4, 2024 10:26

Merge remote-tracking branch 'upstream/master' into polkadot-parachai…

5ef5648

…n-refactoring

Address code review comments

df41f67