-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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: add forc-node command for easily bootstrapping a node #6473
base: master
Are you sure you want to change the base?
Conversation
bd90b81
to
4cab4db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comments.
Also, can we remove the wasm bytecodes & benchmark JSON files? Those are not needed at runtime (it will pull updates from the live network if needed), and I don't like committing or shipping binary blobs in this repo if we can avoid it.
forc-plugins/forc-node/src/consts.rs
Outdated
/// Minimum fuel-core version supported. | ||
pub const MIN_FUEL_CORE_VERSION: &str = "0.40.0"; | ||
|
||
pub const TESTNET_RESERVED_NODE: &str = "/dns4/p2p-testnet.fuel.network/tcp/30333/p2p/16Uiu2HAmDxoChB7AheKNvCVpD4PHJwuDGn8rifMBEHmEynGHvHrf"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use the same setup for testnet that we do for mainnet. Primarily - testnet should use bootstrap nodes the same as mainnet does.
It should also use /dnsaddr/testnet.fuel.network.
instead of a specific node like above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me! Fixing it now, we might want to update the this docs as well as I was following this one: https://docs.fuel.network/guides/running-a-node/running-a-testnet-node/ Will open a PR for that as well
4cab4db
to
2db09cc
Compare
let secret = ask_user_discreetly("Secret:")?; | ||
Ok(KeyPair { peer_id, secret }) | ||
} else { | ||
bail!("Please create a keypair with `fuel-core-keygen new --key-type peering`"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we generate a keypair instead of requiring the user to manually do this? For the vast majority of users, just taking care of automatically setting up the keypair would be the desired behavior.
5873fab
to
fadc4ee
Compare
5e2f71c
to
91b8e10
Compare
use std::path::PathBuf; | ||
|
||
#[derive(Parser, Debug, Clone)] | ||
pub struct LocalCmd { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also have DB path for local node; it's good to be able to persist local node setup across restarts/runs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably implies that in-memory
is the default option (you may need to use an enum here so we can override to custom path)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point! I think we should enable local node with persisting db but that shouldn't be the default. Maybe we can do it with a flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever is easiest/maintainable, don't really mind; thought an enum made sense since you'd either have 1 or the other option.
Another option is to use Either
type (if we're certain there'd only ever be these 2 options); but an additional flag is fine too!
opinion: I've seen cargo run -p forc-node -- --dry-run local --port 8090 It prints the following (port is missing):
|
let mut handle = match op::run(command).await { | ||
Ok(handler) => handler, | ||
Err(err) => { | ||
forc_result_bail!(err); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
let mut handle = match op::run(command).await { | |
Ok(handler) => handler, | |
Err(err) => { | |
forc_result_bail!(err); | |
} | |
}; | |
let mut handle = op::run(command).await?; |
Is it possible to make the This way, if a user wants to run the node with some additional/custom CLI args, they could simply append those args (without us having to make a PR for it for the time being). |
} else { | ||
// Spawn the process with proper error handling | ||
let handle = fuel_core_command | ||
.spawn() | ||
.with_context(|| "Failed to spawn fuel-core process:".to_string())?; | ||
Ok(Some(handle)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
} else { | |
// Spawn the process with proper error handling | |
let handle = fuel_core_command | |
.spawn() | |
.with_context(|| "Failed to spawn fuel-core process:".to_string())?; | |
Ok(Some(handle)) | |
} | |
return Ok(None); | |
} | |
// Spawn the process with proper error handling | |
let handle = fuel_core_command | |
.spawn() | |
.with_context(|| "Failed to spawn fuel-core process:".to_string())?; | |
Ok(Some(handle)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work; have a few suggestions above^; otherwise looking good! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this! 🏆
use std::{net::IpAddr, path::PathBuf}; | ||
|
||
#[derive(Parser, Debug, Clone)] | ||
pub struct IgnitionCmd { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it looks like the options are the same for TestnetCmd
, it's just the defaults that differ. We could have a common struct for the options that are the same and use flatten
|
||
fn build_api_endpoint(&self, folder_name: &str) -> String { | ||
format!( | ||
"{}/repos/FuelLabs/{}/contents/{}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're always fetching from master, would this mean forc-node might be using a newer version of the chain config than what's actually deployed?
if update { | ||
fetcher.download_config(&ChainConfig::Local).await?; | ||
} else { | ||
bail!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
#[test] | ||
fn test_basic_command() { | ||
let mut command = Command::new("fuel-core"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing where fuel-core gets installed for these tests. In CI, do they always run with the latest version of fuel-core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These do not need fuel-core itself as these tests only tests the pretty printer. Adding integration tests are tricky and only possible for local node case (as others basically needs to sync with rest of the network > 24h to run a single test 😄) I'll try to come-up with the integration tests for happy paths for local shortly
fuel_core_command.args(params.as_slice()); | ||
|
||
if dry_run { | ||
println_green(&format!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to print the command even when it's not a dry run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm that is an interesting idea! I think we should do it. Adding it.
Co-authored-by: Sophie Dankel <[email protected]>
Co-authored-by: Sophie Dankel <[email protected]>
Description
WIP at the moment, I have couple of code duplications to remove but we can start playing with this.
Forc Node
Forc node is a new plugin for easily bootstrapping fuel-core instances with sensible defaults. We currently support:
Ignition
This configuration automatically syncs with ignition network, basically run
forc node ignition
and the created node instance will start syncing.The defaults are taken from: https://docs.fuel.network/guides/running-a-node/running-a-mainnet-node/
Testnet
This is for connecting to the latest testnet. Similar to mainnet only thing required is to run
forc node testnet
to start a node created for syncing with testnet.The defaults are taken from: https://docs.fuel.network/guides/running-a-node/running-a-testnet-node/
Local
This is a in-memory instance mostly suited for local developments with instant block production and debug mode enabled in the fuel-core level.
Dry run
It is possible that users of this new plugin might not want to run the node without seeing the parameters send to it. So forc-node has a dry run mode that can be enabled
forc-node --dry-run <ignition|testnet|local>
which will instead of running the node print the command that would achieve the same result:TODO