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: add forc-node command for easily bootstrapping a node #6473

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

kayagokalp
Copy link
Member

@kayagokalp kayagokalp commented Aug 28, 2024

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:

  1. Ignition
  2. Testnet
  3. Local

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/

> ./target/debug/forc-node ignition
✔ Do you have a keypair in hand? · yes
✔ Peer Id: · 16Uiu2HAm6U2QNh35FAoob8LNS9zA6tz4BSrsyCmJxgpEzmYRNgrk
✔ Secret: · ********
? Ethereum RPC (Sepolia) Endpoint: › https://eth-mainnet.g.alchemy.com/v2/....
2024-11-28T06:06:36.369956Z  INFO fuel_core_bin::cli::run: 315: `[Importer, P2P, Producer, TxPool, GraphQL]` metrics are enabled
2024-11-28T06:06:36.370965Z  INFO fuel_core_bin::cli::run: 345: Block production disabled
2024-11-28T06:06:36.370973Z  WARN fuel_core_bin::cli::run: 405: The coinbase recipient `ContractId` is not set!
2024-11-28T06:06:36.370981Z  INFO fuel_core_bin::cli::run: 608: Fuel Core version v0.40.0
...

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/

> ./target/debug/forc-node testnet
✔ Do you have a keypair in hand? · yes
✔ Peer Id: · 16Uiu2HAm6U2QNh35FAoob8LNS9zA6tz4BSrsyCmJxgpEzmYRNgrk
✔ Secret: · ********
? Ethereum RPC (Sepolia) Endpoint: › https://eth-sepolia.g.alchemy.com/v2/....
2024-11-28T06:06:36.369956Z  INFO fuel_core_bin::cli::run: 315: `[Importer, P2P, Producer, TxPool, GraphQL]` metrics are enabled
2024-11-28T06:06:36.370965Z  INFO fuel_core_bin::cli::run: 345: Block production disabled
2024-11-28T06:06:36.370973Z  WARN fuel_core_bin::cli::run: 405: The coinbase recipient `ContractId` is not set!
2024-11-28T06:06:36.370981Z  INFO fuel_core_bin::cli::run: 608: Fuel Core version v0.40.0
...

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.

> ./target/debug/forc-node local
2024-11-28T06:04:44.654891Z  INFO fuel_core_bin::cli::run: 315: `[Importer, P2P, Producer, TxPool, GraphQL]` metrics are enabled
2024-11-28T06:04:44.655762Z  INFO fuel_core_bin::cli::run::relayer: 56: Relayer service disabled
2024-11-28T06:04:44.655769Z  INFO fuel_core_bin::cli::run::p2p: 259: P2P service disabled
2024-11-28T06:04:44.655797Z  INFO fuel_core_bin::cli::run: 343: Block production mode: Instant
...

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:

> ./target/debug/forc-node --dry-run local
fuel-core run --debug --snapshot /Users/kayagokalp/.forc/chainspecs/local --db-type in-memory --poa-instant true

TODO

  • [] Add docs to sway book plugins section
  • [] Add CI check to compare cargo fuel-core version with min supported

@kayagokalp kayagokalp force-pushed the kayagokalp/forc-node-local branch from bd90b81 to 4cab4db Compare November 28, 2024 05:47
Copy link
Member

@mchristopher mchristopher left a 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.

/// 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";
Copy link
Member

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.

Copy link
Member Author

@kayagokalp kayagokalp Dec 2, 2024

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

forc-plugins/forc-node/src/consts.rs Outdated Show resolved Hide resolved
@kayagokalp kayagokalp force-pushed the kayagokalp/forc-node-local branch from 4cab4db to 2db09cc Compare December 2, 2024 23:16
@kayagokalp kayagokalp self-assigned this Dec 2, 2024
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`");
Copy link
Member

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.

@kayagokalp kayagokalp added enhancement New feature or request forc labels Dec 4, 2024
@kayagokalp kayagokalp force-pushed the kayagokalp/forc-node-local branch from 5873fab to fadc4ee Compare December 12, 2024 07:01
@kayagokalp kayagokalp marked this pull request as ready for review December 12, 2024 07:06
@kayagokalp kayagokalp requested review from a team as code owners December 12, 2024 07:06
@kayagokalp kayagokalp force-pushed the kayagokalp/forc-node-local branch from 5e2f71c to 91b8e10 Compare December 12, 2024 07:06
use std::path::PathBuf;

#[derive(Parser, Debug, Clone)]
pub struct LocalCmd {
Copy link
Contributor

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

Copy link
Contributor

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)

Copy link
Member Author

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?

Copy link
Contributor

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!

@zees-dev
Copy link
Contributor

zees-dev commented Dec 12, 2024

opinion: I've seen dry-run mostly used for making transactions or some state-changing/persisting action; I personally think that the default command run when starting up the node should be printed in the --help output instead.
Alternatively, if we still want to continue using dry-run for this; then it should adapt with the CLI flags; for example when running:

cargo run -p forc-node -- --dry-run local --port 8090

It prints the following (port is missing):

fuel-core run --debug --snapshot /Users/z/.forc/chainspecs/local --db-type in-memory --poa-instant true

Comment on lines +24 to +29
let mut handle = match op::run(command).await {
Ok(handler) => handler,
Err(err) => {
forc_result_bail!(err);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
let mut handle = match op::run(command).await {
Ok(handler) => handler,
Err(err) => {
forc_result_bail!(err);
}
};
let mut handle = op::run(command).await?;

@zees-dev
Copy link
Contributor

Is it possible to make the forc-node a bit more extensible; i.e. user can provide additional CLI args - which would simply be appended onto the underlying fuel-core run command.
I.e. something like forc-node local -- <additional CLI flags> - where everything after the -- would simply be appended to the underlying fuel-core run command.

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).

Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +59 to +65
} 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))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
} 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))

Copy link
Contributor

@zees-dev zees-dev left a 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! 🚀

Copy link
Member

@sdankel sdankel left a 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! 🏆

forc-plugins/forc-node/src/cmd.rs Outdated Show resolved Hide resolved
forc-plugins/forc-node/src/cmd.rs Outdated Show resolved Hide resolved
use std::{net::IpAddr, path::PathBuf};

#[derive(Parser, Debug, Clone)]
pub struct IgnitionCmd {
Copy link
Member

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

forc-plugins/forc-node/src/consts.rs Show resolved Hide resolved

fn build_api_endpoint(&self, folder_name: &str) -> String {
format!(
"{}/repos/FuelLabs/{}/contents/{}",
Copy link
Member

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?

forc-plugins/forc-node/src/run_opts.rs Show resolved Hide resolved
if update {
fetcher.download_config(&ChainConfig::Local).await?;
} else {
bail!(
Copy link
Member

Choose a reason for hiding this comment

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

For some reason these errors aren't being formatted properly.
image


#[test]
fn test_basic_command() {
let mut command = Command::new("fuel-core");
Copy link
Member

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?

Copy link
Member Author

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!(
Copy link
Member

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.

Copy link
Member Author

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.

kayagokalp and others added 2 commits December 17, 2024 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request forc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants