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

feat: Move bootstrap node into collator #727

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

aidan46
Copy link
Contributor

@aidan46 aidan46 commented Feb 4, 2025

Description

Move the P2P bootstrap node from the polka-storage-provider-server into the collator. All the examples have been updated to reflect this change.
The zombienet setup has been updated to include the p2p key and the p2p listen address.

@aidan46 aidan46 self-assigned this Feb 4, 2025
@aidan46 aidan46 added the enhancement New feature or request label Feb 4, 2025
@aidan46 aidan46 added this to the Phase 3 milestone Feb 4, 2025
@aidan46 aidan46 linked an issue Feb 4, 2025 that may be closed by this pull request
@aidan46 aidan46 added the ready for review Review is needed label Feb 4, 2025
@aidan46 aidan46 added ready for review Review is needed and removed ready for review Review is needed labels Feb 4, 2025
@aidan46 aidan46 added ready for review Review is needed and removed ready for review Review is needed labels Feb 4, 2025
#[clap(flatten)]
pub base: cumulus_client_cli::RunCmd,

/// P2P ED25519 private key
Copy link
Contributor

Choose a reason for hiding this comment

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

P2P is too generic, I'd say something like:

Suggested change
/// P2P ED25519 private key
/// Key identifying nodes in p2p network of Storage Providers and Collators.
/// It must be ED25519 private key, supplied as a short format XXXX or XXX or XXX.

(please fill the description of how it should look like)

pub p2p_listen_address: Option<Multiaddr>,
}

impl std::ops::Deref for RunCmd {
Copy link
Contributor

Choose a reason for hiding this comment

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

Genuinely curious, why is it needed and how does this work?

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 took this from moonbeam. It is to prevent calling cli.run.base on collator_options and normalize which are used is the codebase.


/// Parses a ED25519 private key into a Keypair.
/// Takes in a private key or the path to a PEM file, depending on the @ prefix.
pub(crate) fn keypair_value_parser(src: &str) -> Result<Keypair, String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We use similar functions across the codebase, should we put somewhere and reuse it?

@@ -225,6 +225,17 @@ pub fn run() -> Result<()> {
let collator_options = cli.run.collator_options();

runner.run_node_until_exit(|config| async move {
let bootstrap_config = if config.role.is_authority() {
let p2p_key = cli.run.p2p_key.ok_or("Could not find p2p key")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let p2p_key = cli.run.p2p_key.ok_or("Could not find p2p key")?;
let p2p_key = cli.run.p2p_key.ok_or("This node is configured as authority, so it will be used as Bootstrap node for Storage Provider & Collator network, but the key is missing. Set --p2p--key argument of the node.")?;

Let's try to make error messages which are shown to the user as concise and sufficient enough so they don't need to go to the source code to figure this one out.

Comment on lines +340 to +345
if parachain_config.role.is_authority() {
let bootstrap_config = bootstrap_config.ok_or("Could not find bootstrap config")?;
task_manager
.spawn_handle()
.spawn("p2p", None, run_bootstrap_node(bootstrap_config));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if parachain_config.role.is_authority() {
let bootstrap_config = bootstrap_config.ok_or("Could not find bootstrap config")?;
task_manager
.spawn_handle()
.spawn("p2p", None, run_bootstrap_node(bootstrap_config));
}
if let Ok(cfg) = bootstrap_config {
task_manager
.spawn_handle()
.spawn("p2p", None, run_bootstrap_node(bootstrap_config));
}

We should be able assume that if bootstrap config is set, then the node is authority and is supposed to be the authority.

Comment on lines +31 to +36
"--detailed-log-output",
"--p2p-key=@zombienet/private.pem",
"--p2p-listen-address=/ip4/127.0.0.1/tcp/62649",
"--pool-type=fork-aware",
"-lparachain=debug,xcm=trace,runtime=trace,txpool=debug,basic-authorship=debug",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

It does also need changes to maat/src/lib.rs , we pass those args manually as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • kube-testnet as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure whether it's a good idea to save any private keys on github.

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 agree with that. I don't know how to generate the key and then pass it into the zombienet configuration. Do you have any ideas?

Copy link
Contributor

@th7nder th7nder Feb 4, 2025

Choose a reason for hiding this comment

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

We could specify the path in the hard path zombienet and add generation of the .pem key as part of the Justfile 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready for review Review is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Move P2P bootstrap into collator
2 participants