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

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

Merged
merged 9 commits into from
Jun 14, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ members = [
"cumulus/parachains/runtimes/testing/penpal",
"cumulus/parachains/runtimes/testing/rococo-parachain",
"cumulus/polkadot-parachain",
"cumulus/polkadot-parachain/common",
"cumulus/primitives/aura",
"cumulus/primitives/core",
"cumulus/primitives/parachain-inherent",
Expand Down
3 changes: 2 additions & 1 deletion cumulus/polkadot-parachain/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ path = "src/main.rs"
async-trait = "0.1.79"
clap = { version = "4.5.3", features = ["derive"] }
codec = { package = "parity-scale-codec", version = "3.6.12" }
color-print = "0.3.4"
futures = "0.3.28"
hex-literal = "0.4.1"
log = { workspace = true, default-features = true }
Expand Down Expand Up @@ -111,7 +112,7 @@ cumulus-client-service = { path = "../client/service" }
cumulus-primitives-aura = { path = "../primitives/aura" }
cumulus-primitives-core = { path = "../primitives/core" }
cumulus-relay-chain-interface = { path = "../client/relay-chain-interface" }
color-print = "0.3.4"
polkadot-parachain-common = { path = "common" }

[build-dependencies]
substrate-build-script-utils = { path = "../../substrate/utils/build-script-utils" }
Expand Down
25 changes: 25 additions & 0 deletions cumulus/polkadot-parachain/common/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
[package]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

name = "polkadot-parachain-common"
version = "0.1.0"
authors.workspace = true
edition.workspace = true
description = "Primitives for building a polkadot parachain node"
license = "Apache-2.0"

[lints]
workspace = true

[dependencies]
codec = { package = "parity-scale-codec", version = "3.6.12" }

# Substrate
sp-api = { path = "../../../substrate/primitives/api" }
sp-block-builder = { path = "../../../substrate/primitives/block-builder" }
sp-consensus-aura = { path = "../../../substrate/primitives/consensus/aura" }
sp-runtime = { path = "../../../substrate/primitives/runtime", default-features = false }
sp-session = { path = "../../../substrate/primitives/session" }
sp-transaction-pool = { path = "../../../substrate/primitives/transaction-pool" }

# Cumulus
cumulus-primitives-aura = { path = "../../primitives/aura" }
cumulus-primitives-core = { path = "../../primitives/core" }
68 changes: 68 additions & 0 deletions cumulus/polkadot-parachain/common/src/aura.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Copyright (C) Parity Technologies (UK) Ltd.
// This file is part of Cumulus.

// Cumulus is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Cumulus is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Cumulus. If not, see <http://www.gnu.org/licenses/>.

//! Aura-related primitives for cumulus parachain collators.

use codec::Codec;
use cumulus_primitives_aura::AuraUnincludedSegmentApi;
use cumulus_primitives_core::BlockT;
use sp_consensus_aura::AuraApi;
use sp_runtime::app_crypto::{AppCrypto, AppPair, AppSignature, Pair};

/// Convenience trait for defining the basic bounds of an `AuraId`.
pub trait AuraIdT: AppCrypto<Pair = Self::BoundedPair> + Codec + Send {
/// Extra bounds for the `Pair`.
type BoundedPair: AppPair + AppCrypto<Signature = Self::BoundedSignature>;

/// Extra bounds for the `Signature`.
type BoundedSignature: AppSignature
+ TryFrom<Vec<u8>>
+ std::hash::Hash
+ sp_runtime::traits::Member
+ Codec;
}

impl<T> AuraIdT for T
where
T: AppCrypto + Codec + Send + Sync,
<<T as AppCrypto>::Pair as AppCrypto>::Signature:
TryFrom<Vec<u8>> + std::hash::Hash + sp_runtime::traits::Member + Codec,
{
type BoundedPair = <T as AppCrypto>::Pair;
type BoundedSignature = <<T as AppCrypto>::Pair as AppCrypto>::Signature;
}

/// Convenience trait for defining the basic bounds of a parachain runtime that supports
/// the Aura consensus.
pub trait AuraRuntimeApi<Block: BlockT, AuraId: AuraIdT>:
sp_api::ApiExt<Block>
+ AuraApi<Block, <AuraId::BoundedPair as Pair>::Public>
+ AuraUnincludedSegmentApi<Block>
+ Sized
{
/// Check if the runtime has the Aura API.
fn has_aura_api(&self, at: Block::Hash) -> bool {
self.has_api::<dyn AuraApi<Block, <AuraId::BoundedPair as Pair>::Public>>(at)
.unwrap_or(false)
}
}

impl<T, Block: BlockT, AuraId: AuraIdT> AuraRuntimeApi<Block, AuraId> for T where
T: sp_api::ApiExt<Block>
+ AuraApi<Block, <AuraId::BoundedPair as Pair>::Public>
+ AuraUnincludedSegmentApi<Block>
{
}
67 changes: 67 additions & 0 deletions cumulus/polkadot-parachain/common/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright (C) Parity Technologies (UK) Ltd.
// This file is part of Cumulus.

// Cumulus is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Cumulus is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Cumulus. If not, see <http://www.gnu.org/licenses/>.

//! Cumulus parachain collator primitives.

#![warn(missing_docs)]

pub mod aura;

use cumulus_primitives_core::CollectCollationInfo;
use sp_api::{ApiExt, CallApiAt, ConstructRuntimeApi, Metadata};
use sp_block_builder::BlockBuilder;
use sp_runtime::traits::Block as BlockT;
use sp_session::SessionKeys;
use sp_transaction_pool::runtime_api::TaggedTransactionQueue;

/// Convenience trait that defines the basic bounds for the `RuntimeApi` of a parachain node.
pub trait NodeRuntimeApi<Block: BlockT>:
ApiExt<Block>
+ Metadata<Block>
+ SessionKeys<Block>
+ BlockBuilder<Block>
+ TaggedTransactionQueue<Block>
+ CollectCollationInfo<Block>
+ Sized
{
}

impl<T, Block: BlockT> NodeRuntimeApi<Block> for T where
T: ApiExt<Block>
+ Metadata<Block>
+ SessionKeys<Block>
+ BlockBuilder<Block>
+ TaggedTransactionQueue<Block>
+ CollectCollationInfo<Block>
{
}

/// Convenience trait that defines the basic bounds for the `ConstructRuntimeApi` of a parachain
/// node.
pub trait ConstructNodeRuntimeApi<Block: BlockT, C: CallApiAt<Block>>:
ConstructRuntimeApi<Block, C, RuntimeApi = Self::BoundedRuntimeApi> + Send + Sync + 'static
{
/// Basic bounds for the `RuntimeApi` of a parachain node.
type BoundedRuntimeApi: NodeRuntimeApi<Block>;
}

impl<T, Block: BlockT, C: CallApiAt<Block>> ConstructNodeRuntimeApi<Block, C> for T
where
T: ConstructRuntimeApi<Block, C> + Send + Sync + 'static,
T::RuntimeApi: NodeRuntimeApi<Block>,
{
type BoundedRuntimeApi = T::RuntimeApi;
}
14 changes: 8 additions & 6 deletions cumulus/polkadot-parachain/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// You should have received a copy of the GNU General Public License
// along with Cumulus. If not, see <http://www.gnu.org/licenses/>.

use clap::{CommandFactory, FromArgMatches};
use std::path::PathBuf;

/// Sub-commands supported by the collator.
Expand Down Expand Up @@ -108,18 +109,19 @@ pub struct RelayChainCli {
}

impl RelayChainCli {
/// Parse the relay chain CLI parameters using the para chain `Configuration`.
/// Parse the relay chain CLI parameters using the parachain `Configuration`.
pub fn new<'a>(
para_config: &sc_service::Configuration,
relay_chain_args: impl Iterator<Item = &'a String>,
) -> Self {
let polkadot_cmd = polkadot_cli::RunCmd::command().no_binary_name(true);
let matches = polkadot_cmd.get_matches_from(relay_chain_args);
let base = FromArgMatches::from_arg_matches(&matches).unwrap_or_else(|e| e.exit());

let extension = crate::chain_spec::Extensions::try_get(&*para_config.chain_spec);
let chain_id = extension.map(|e| e.relay_chain.clone());

let base_path = para_config.base_path.path().join("polkadot");
Self {
base_path: Some(base_path),
chain_id,
base: clap::Parser::parse_from(relay_chain_args),
}
Self { base, chain_id, base_path: Some(base_path) }
}
}
14 changes: 3 additions & 11 deletions cumulus/polkadot-parachain/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -530,13 +530,9 @@ pub fn run() -> Result<()> {
}),
Some(Subcommand::PurgeChain(cmd)) => {
let runner = cli.create_runner(cmd)?;
let polkadot_cli = RelayChainCli::new(runner.config(), cli.relaychain_args.iter());

runner.sync_run(|config| {
let polkadot_cli = RelayChainCli::new(
&config,
[RelayChainCli::executable_name()].iter().chain(cli.relaychain_args.iter()),
);

let polkadot_config = SubstrateCli::create_configuration(
&polkadot_cli,
&polkadot_cli,
Expand Down Expand Up @@ -603,6 +599,7 @@ pub fn run() -> Result<()> {
Some(Subcommand::Key(cmd)) => Ok(cmd.run(&cli)?),
None => {
let runner = cli.create_runner(&cli.run.normalize())?;
let polkadot_cli = RelayChainCli::new(runner.config(), cli.relaychain_args.iter());
let collator_options = cli.run.collator_options();

runner.run_node_until_exit(|config| async move {
Expand Down Expand Up @@ -648,11 +645,6 @@ pub fn run() -> Result<()> {
.map(|e| e.para_id)
.ok_or("Could not find parachain extension in chain-spec.")?;

let polkadot_cli = RelayChainCli::new(
&config,
[RelayChainCli::executable_name()].iter().chain(cli.relaychain_args.iter()),
);

let id = ParaId::from(para_id);

let parachain_account =
Expand All @@ -667,7 +659,7 @@ pub fn run() -> Result<()> {
info!("Parachain Account: {}", parachain_account);
info!("Is collating: {}", if config.role.is_authority() { "yes" } else { "no" });

match polkadot_config.network.network_backend {
match config.network.network_backend {
sc_network::config::NetworkBackendType::Libp2p =>
start_node::<sc_network::NetworkWorker<_, _>>(
config,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,6 @@ sp_api::impl_runtime_apis! {
}
}

impl sp_offchain::OffchainWorkerApi<Block> for Runtime {
fn offchain_worker(_: &<Block as BlockT>::Header) {
unimplemented!()
}
}

impl sp_session::SessionKeys<Block> for Runtime {
fn generate_session_keys(_: Option<Vec<u8>>) -> Vec<u8> {
unimplemented!()
Expand Down
6 changes: 0 additions & 6 deletions cumulus/polkadot-parachain/src/fake_runtime_api/aura.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,6 @@ sp_api::impl_runtime_apis! {
}
}

impl sp_offchain::OffchainWorkerApi<Block> for Runtime {
fn offchain_worker(_: &<Block as BlockT>::Header) {
unimplemented!()
}
}

impl sp_session::SessionKeys<Block> for Runtime {
fn generate_session_keys(_: Option<Vec<u8>>) -> Vec<u8> {
unimplemented!()
Expand Down
Loading
Loading