`polkadot-parachain`: add manual seal support #5586

serban300 · 2024-09-04T11:35:50Z

Resolves #5026

This PR adds support for starting a dev node with manual seal consensus. This can be done by using the --manual-seal argument and it will work only with dev chains (chain id ends with -dev) . For example:

polkadot-parachain --chain asset-hub-rococo-dev --manual-seal --tmp

kianenigma · 2024-09-04T16:00:38Z

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

@@ -215,7 +212,24 @@ pub fn run<CliConfig: crate::cli::CliConfig>(cmd_config: RunConfig) -> Result<()
 				RelayChainCli::<CliConfig>::new(runner.config(), cli.relay_chain_args.iter());
 			let collator_options = cli.run.collator_options();

+			let is_dev_chain = runner.config().chain_spec.id().ends_with("-dev");


I am a bit unsure as to why this limitation should exist. So this will only work if the chain-spec's id ends with -dev. Why should this limitation exist?

I will investigate and get back with the details, but the general problem is that I'm getting errors at block production when trying to use a non-dev runtime (for example: asset-hub-polkadot).

The problem is that for live chains we use predefined json configs (https://github.com/paritytech/polkadot-sdk/tree/master/cumulus/polkadot-parachain/chain-specs) and I think that most of them are outdated. I tried to update the code for the asset hub rococo and asset hub westend configs (chain-spec-builder update-code) and after that polkadot-parachain --chain asset-hub-rococo --dev-block-time 5000 worked for example. But polkadot-parachain --chain asset-hub-rococo stopped working, because of errors like Bootnode with peer id `12D3KooWG4YUe7AfSxVwyLQBRRMU99krssmGAUghqUFoVY1iPkQs` is on a different chain (our genesis: 0xf52a…f62e theirs: 0x67f9…98c9).

Anyway, if the chain spec is generated correctly, manual seal should work. So I removed this if and I can open a different issue for updating the predefined chain specs. Or we can leave them as they are.

kianenigma · 2024-09-04T16:02:06Z

cumulus/polkadot-parachain/polkadot-parachain-lib/src/cli.rs

+	///
+	/// This works only with dev chains (e.g. asset-hub-rococo-dev).
+	#[arg(long)]
+	pub manual_seal: bool,


The raw manual seal also requires you to submit some kind of an RPC to author a block. What I have done in the past, I think this is a good improvement to it, is to run an RPC client internally that pings the manual seal server and produces blocks every x ms.

This is then how I re-formulated this CLI argument as --dev-block-time 100, which produces a block every 100ms.

Sounds good. Done.

skunert

Did not look at the manual_seal.rs code itself yet, will make one more pass.

cumulus/polkadot-parachain/polkadot-parachain-lib/src/cli.rs

cumulus/polkadot-parachain/polkadot-parachain-lib/src/common/mod.rs

skunert · 2024-09-10T19:41:02Z

cumulus/polkadot-parachain/polkadot-parachain-lib/src/common/mod.rs


 pub trait NodeBlock:
 	BlockT<Extrinsic = OpaqueExtrinsic, Header = Self::BoundedHeader, Hash = DbHash>
 	+ for<'de> serde::Deserialize<'de>
+	+ UnwindSafe


I see that these bounds where just moved, but why exactly do we need them in the first place?

We need it because of this:

polkadot-sdk/substrate/primitives/api/proc-macro/src/impl_runtime_apis.rs

Line 635 in 221565d

// All this `UnwindSafe` magic below here is required for this rust bug:

skunert · 2024-09-10T19:44:37Z

cumulus/polkadot-parachain/polkadot-parachain-lib/src/nodes/shell.rs

@@ -0,0 +1,152 @@
+// Copyright (C) Parity Technologies (UK) Ltd.


Do we still need the shell node? I think we can just remove it. In the past it was used to transition from this placeholder shell runtime to system-chains. I think no one does it anymore.

Personally I don't know. I would be happy to remove it, but I'm not familiar with it and I don't know if it's still used by anyone. @kianenigma WDYT ?

skunert · 2024-09-10T19:47:13Z

cumulus/polkadot-parachain/polkadot-parachain-lib/src/nodes/mod.rs

+use manual_seal::ManualSealNode;
+use sc_service::{Configuration, TaskManager};
+
+pub trait DynNodeSpecExt: DynNodeSpec {


This trait should be more clearly named, it is specifically tailored to enable this manual seal functionality. So maybe DevNodeExt or something? Just a suggestion, maybe someone has another idea.

I also thought of something like DevNodeExt, but I think it can lead to misunderstandings. For example I would be tempted to understand that it's only something related to dev nodes. But it also contains the logic related to production nodes. Because of this, I would prefer to leave it as DynNodeSpecExt. But if you have strong preferences I can change it.

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

skunert · 2024-09-10T20:23:14Z

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

+						.ok_or("Could not find parachain extension in chain-spec.")?,
+				);
+
+				if let Some(dev_block_time) = cli.dev_block_time {


Did not check this in-depth, but could we not mingle the dev_block_time into extra args and make the new_node_spec above have something like Consensus::Manual. I am wondering if we can get rid of the "special handling" of start_manual_seal and just treat it as a consensus variant.

I don't think we can treat the manual seal just like another consensus variant. Because the process of starting the node is a bit different (the relay_chain_interface is not built), and the arguments passed to start_consensus are also different. Mixing this in the NodeSpec feels a bit dirty.

That's why I just added a new type of node with a new start_manual_seal_node() fn that differs from start_node(). We could probably make it more generic and name it SoloChainNode instead of ManualSealNode, if it makes any difference, but I would keep it separate.

…n-manual-seal

serban300 added 2 commits September 4, 2024 14:09

Split NodeSpec

c03cb1f

Split service.rs

c9b3469

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 Sep 4, 2024

serban300 self-assigned this Sep 4, 2024

serban300 force-pushed the polkadot-parachain-manual-seal branch from d283731 to bc89536 Compare September 4, 2024 11:37

Add manual seal node

0c8a994

serban300 force-pushed the polkadot-parachain-manual-seal branch from bc89536 to 0c8a994 Compare September 4, 2024 11:50

paritytech deleted a comment from paritytech-cicd-pr Sep 4, 2024

serban300 requested review from kianenigma and skunert September 4, 2024 12:36

kianenigma reviewed Sep 4, 2024

View reviewed changes

serban300 added 2 commits September 5, 2024 12:24

manual_seal -> dev_block_time

097aa4b

Remove check

d94e75e

skunert reviewed Sep 10, 2024

View reviewed changes

serban300 added 3 commits September 20, 2024 09:56

Address code review comments

755af4a

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

7979491

…n-manual-seal

Fix merge change

f7bfff8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`polkadot-parachain`: add manual seal support #5586

`polkadot-parachain`: add manual seal support #5586

serban300 commented Sep 4, 2024

kianenigma Sep 4, 2024

serban300 Sep 5, 2024

serban300 Sep 5, 2024 •

edited

Loading

kianenigma Sep 4, 2024

serban300 Sep 5, 2024

skunert left a comment

skunert Sep 10, 2024

serban300 Sep 20, 2024

skunert Sep 10, 2024

serban300 Sep 20, 2024

skunert Sep 10, 2024

serban300 Sep 20, 2024

skunert Sep 10, 2024

serban300 Sep 20, 2024

		@@ -0,0 +1,152 @@
		// Copyright (C) Parity Technologies (UK) Ltd.

polkadot-parachain: add manual seal support #5586

Are you sure you want to change the base?

polkadot-parachain: add manual seal support #5586

Conversation

serban300 commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skunert left a 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

Choose a reason for hiding this comment

`polkadot-parachain`: add manual seal support #5586

`polkadot-parachain`: add manual seal support #5586

serban300 Sep 5, 2024 •

edited

Loading