polkadot-parachain: Add omni-node variant with u64 block number #5269
Conversation
serban300
added
R0-silent
| cmd: &BlockCmd, | ||
| ) -> SyncCmdResult; | ||
|
|
||
| #[cfg(any(feature = "runtime-benchmarks"))] |
There was a problem hiding this comment.
I've talked about this before, so possibly it is duplicate:
we anticipate omni nodes to not have any benchmarking functionality, so I do encourage you to explore the state of this (e.g. how far we are from omni-bencher being self-sufficient) this and possibly NOT add code that we are going to remove later.
Probably what I am suggesting today is not feasible, because we use the benchmark subcommand in many places in our CI.
I am okay with you making this compromise now, but reminder that we need plan + tracking issue to remove benchmarking subcommand from here. The first step couple be keeping the subcommand but marking it as deprecated.
There was a problem hiding this comment.
Upon further thought, I questioned my own opinion.
Keeping benchmark, try-runtime or build-spec subcommand is fine, as long as they don't require a hard dependency to the runtime. In the past they did rely on the runtime being linked as a dependency.
Especially now that we have the omni-node library in place (#5288) and teams can abstract away from needing to define these commands in e.g. struct Commands, in fact it is beneficial to keep these subcommand in the omni-node, so that teams don't need multiple binaries to do their day-to-day work.
Imagine how nicer it is to have, instead of:
polkadot-parachainframe-omni-bencherchain-spec-builder
have one called polkadot-omni-node?
It seems to me that we mainly separated these binaries because we had leaky abstraction in the node side (e.g. sturct Command that had to be updated by each and every team to support a new subcommand).
TLDR; in light of #5288 which is a step in the direction of the original vision of #5, I don't see why things like omni-bencher and chain-spec-builder cannot be part of the omni-node again.
@michalkucharczyk @ggwpez @bkchr wdyt?
There was a problem hiding this comment.
have one called polkadot-omni-node?
Yeah could work. Generally I think doing this to get rid of the included chain specs etc could be a good idea. So, that we have the new binary as you proposed and can remove stuff.
There was a problem hiding this comment.
I don't see why things like omni-bencher and chain-spec-builder cannot be part of the omni-node again.
Yes the omni-node can definitely re-expose the benchmark commands with a flag. The advantage of the omni-bencher is faster compile time and smaller binary for CI. Since building a complete node just for benchmarking is a bit of a waste.
There was a problem hiding this comment.
I am okay with you making this compromise now, but reminder that we need plan + tracking issue to remove benchmarking subcommand from here. The first step couple be keeping the subcommand but marking it as deprecated.
Yes, agree. This is tracked here: #4966 . From what I udnerstand we still need #4405 to be merged, in order to remove the benchmarks from the node.
Especially now that we have the omni-node library in place (#5288) and teams can abstract away from needing to define these commands in e.g. struct Commands, in fact it is beneficial to keep these subcommand in the omni-node, so that teams don't need multiple binaries to do their day-to-day work.
Keeping it also sounds good.
There was a problem hiding this comment.
I personally think the best way to expose this is neither CLI nor chain-spec, and instead the pattern introduced in #5288, so I would wait for that to be merged first.
CLI and/or chain-spec and/or env variable and all indeed work, but the most meta and un-opinionated way is 5288.
serban300
removed
the
R0-silent
Done. @kianenigma can you please take a look ? |
cumulus/polkadot-parachain/polkadot-parachain-lib/src/fake_runtime_api/mod.rs
Outdated
Show resolved
Hide resolved
cumulus/polkadot-parachain/polkadot-parachain-lib/src/fake_runtime_api/mod.rs
Outdated
Show resolved
Hide resolved
| pub use parachains_common::{AccountId, Balance, Hash, Nonce}; | ||
|
|
||
| type Header<BlockNumber> = generic::Header<BlockNumber, BlakeTwo256>; | ||
| pub type CustomBlock<BlockNumber> = generic::Block<Header<BlockNumber>, UncheckedExtrinsic>; |
There was a problem hiding this comment.
| pub type CustomBlock<BlockNumber> = generic::Block<Header<BlockNumber>, UncheckedExtrinsic>; | |
| pub type BlockOf<BlockNumber> = generic::Block<Header<BlockNumber>, UncheckedExtrinsic>; |
nit, but usually we called such aliases in substrate like that.
There was a problem hiding this comment.
I removed CustomBlock. Now it's just called Block.
I saw this kind of naming in substrate but I think it was for things like: HeaderOf<Block> which is an alias for Block::Header. I wouldn't use BlockOf here since we're not accessing a property Block of something. We are referencing a Block that uses a certain number. But I don't know. I don't have strong preferences.
cumulus/polkadot-parachain/polkadot-parachain-lib/src/common/types.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code looks broadly good, I will approve after another quick pass.
Would be great if the second review is from a node-sdk expert.
Beyond "the code compiles", what steps do you perform to make sure the scenario we have in mind in every step of the way is correct?
we should work towards a number of network setups where we can quickly test relevant scenarios. Even if they are not tested in CI (for now), they are great help for us to ensure correctness of what we are doing.
For example, as with this PR, I should expect to quickly run a zombienet network with a U64 block number, and see it work in action.
This setup would then also help us verify some questions in the code. For example, at some point I had the guess that the types passed into FakeRuntime in impl_runtime_apis really don't matter AT ALL. As in, I think I got a node working with totally fake/dumb types. Having a simple test setup will help us test this kind of hypothesis, without risk breaking anything.
For this PR, personally I tested, asset hub polkadot, asset hub rococo and peregrine
I think that in the CI there are zombienet tests that spawn most of the runtimes suported by polkadot-parachain. The bridges zombienet tests spawn for example, asset hub rococo, asset hub westend, bridge hub rococo, bridge hub westend But yes, there is no test for a parachain with u64 block number. I agree it would be good to have a test setup. But also, I think we should do it as part of a separate PR/issue |
This hypothesis is completely wrong. |
Also think could be a separate PR, would just be a matter of introducing a new cumulus test runtime like the variants that we have, e.g. here. |
I see, so the But it makes sense, even in wasm we need some reference to decode the types I suppose. |
Related to #4787 The main changes in this PR are the following: - making the NodeSpec logic generic on the Block type - adding an omni-node variant with u64 block number Apart from this, the PR also moves some of the logic in `service.rs` to the `common` subfolder The omni-node variant with u64 block number is not used yet. We have to either expose the option in the CLI or to read the block number from the chain spec somehow. Will do it in a future PR.
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
c4ced11
* master: (39 commits) short-term fix for para inherent weight overestimation (#5082) CI: Add backporting bot (#4795) Fix benchmark failures when using `insecure_zero_ed` flag (#5354) Command bot GHA v2 - /cmd <cmd> (#5457) Remove pallet::getter usage from treasury (#4962) Bump blake2b_simd from 1.0.1 to 1.0.2 (#5404) Bump rustversion from 1.0.14 to 1.0.17 (#5405) Bridge zombienet tests: remove old command (#5434) polkadot-parachain: Add omni-node variant with u64 block number (#5269) Refactor verbose test (#5506) Use umbrella crate for minimal template (#5155) IBP Coretime Polkadot bootnodes (#5499) rpc server: listen to `ipv6 socket` if available and `--experimental-rpc-endpoint` CLI option (#4792) Update approval-voting-regression-bench (#5504) change try-runtime rpc domains (#5443) polkadot-parachain-bin: Remove contracts parachain (#5471) Add feature to allow Aura collator to use full PoV size (#5393) Adding stkd bootnodes (#5470) Make `PendingConfigs` storage item public (#5467) frame-omni-bencher maintenance (#5466) ...
Related to #4787
The main changes in this PR are the following:
Apart from this, the PR also moves some of the logic in
service.rsto thecommonsubfolderThe omni-node variant with u64 block number is not used yet. We have to either expose the option in the CLI or to read the block number from the chain spec somehow. Will do it in a future PR.