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.

Sign up for GitHub

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

Add discv5 #72

Merged
merged 24 commits into from
Jan 2, 2025
Merged

Add discv5 #72

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
248466c
update discv5 to 0.9.0
diegomrsantos Dec 13, 2024
f8ca4b4
add discovery module
diegomrsantos Dec 13, 2024
c016651
add Discovery struct and implement NetworkBehaviour
diegomrsantos Dec 13, 2024
daff657
add build_enr function
diegomrsantos Dec 13, 2024
e1b09d6
add discovery to AnchorBehaviour
diegomrsantos Dec 13, 2024
467b48a
cargo fmt
diegomrsantos Dec 13, 2024
a12a8e9
expose Enr
diegomrsantos Dec 16, 2024
f71df58
add disable_discovery
diegomrsantos Dec 16, 2024
1f3d955
hardcode holesky bootnode
diegomrsantos Dec 16, 2024
b5cdbca
add discovery
diegomrsantos Dec 16, 2024
2ca4167
cargo fmt
diegomrsantos Dec 16, 2024
cc5004e
cargo clippy
diegomrsantos Dec 17, 2024
e532014
remove hardcoded bootnode enr
diegomrsantos Dec 17, 2024
3080f3d
dial discovered peers
diegomrsantos Dec 17, 2024
c091041
remove event
diegomrsantos Dec 17, 2024
2cd0e43
uncomment logs
diegomrsantos Dec 17, 2024
afcfe99
simplify error handling
diegomrsantos Dec 17, 2024
14fc072
TODO handle local enr
diegomrsantos Dec 18, 2024
ae8d4a9
add ssv predicate
diegomrsantos Dec 19, 2024
19cdba9
Merge branch 'unstable' into add-discv5
diegomrsantos Dec 20, 2024
92c0468
Update anchor/network/src/network.rs
diegomrsantos Dec 23, 2024
a323635
Update anchor/network/src/network.rs
diegomrsantos Dec 23, 2024
a8b5f52
cargo fmt
diegomrsantos Dec 23, 2024
24fa59a
fix formatting
diegomrsantos Dec 23, 2024
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
33 changes: 1 addition & 32 deletions Cargo.lock
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ derive_more = { version = "1.0.0", features = ["full"] }
async-channel = "1.9"
axum = "0.7.7"
clap = { version = "4.5.15", features = ["derive", "wrap_help"] }
discv5 = "0.8.0"
discv5 = "0.9.0"
dirs = "5.0.1"
either = "1.13.0"
futures = "0.3.30"
Expand Down
8 changes: 8 additions & 0 deletions anchor/client/src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,14 @@ pub struct Anchor {
help_heading = FLAG_HEADER
)]
help: Option<bool>,
#[clap(
long,
global = true,
value_delimiter = ',',
help = "One or more comma-delimited base64-encoded ENR's to bootstrap the p2p network",
display_order = 0
Copy link
Member

Choose a reason for hiding this comment

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

We can declare the default boot nodes somewhere (like anchor/network/src/config.rs) with:

const DEFAULT_BOOTNODES: [&'static str; 3] = ["ENR1", "ENR2", "ENR3"];

and then import it and use it here:

Suggested change
display_order = 0
display_order = 0
default_values_t = DEFAULT_BOOTNODES.into_iter().map(Into::into)

Copy link
Author

Choose a reason for hiding this comment

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

It seems LH defines them in yaml files https://github.com/sigp/lighthouse/blob/stable/common/eth2_network_config/built_in_network_configs/holesky/boot_enr.yaml

Copy link
Member

Choose a reason for hiding this comment

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

yeah can also be, but then we should get them by default, if you prefer it can be done in a subsequent PR

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's probably better in another PR.

)]
pub boot_nodes_enr: Vec<String>,
}

pub fn get_color_style() -> Styles {
Expand Down
19 changes: 19 additions & 0 deletions anchor/client/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,25 @@ pub fn from_cli(cli_args: &Anchor) -> Result<Config, String> {
*/
config.network.listen_addresses = parse_listening_addresses(cli_args)?;

for addr in cli_args.boot_nodes_enr.clone() {
match addr.parse() {
Ok(enr) => config.network.boot_nodes_enr.push(enr),
Err(_) => {
// parsing as ENR failed, try as Multiaddr
// let multi: Multiaddr = addr
// .parse()
// .map_err(|_| format!("Not valid as ENR nor Multiaddr: {}", addr))?;
// if !multi.iter().any(|proto| matches!(proto, Protocol::Udp(_))) {
// slog::error!(log, "Missing UDP in Multiaddr {}", multi.to_string());
// }
// if !multi.iter().any(|proto| matches!(proto, Protocol::P2p(_))) {
// slog::error!(log, "Missing P2P in Multiaddr {}", multi.to_string());
// }
// multiaddrs.push(multi);
}
}
}
Comment on lines +134 to +151
Copy link
Member

Choose a reason for hiding this comment

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

we are currently iterating the bootnode list twice, first here and then when adding to discv5, we can parse them when adding to discv5 and avoid double iteration

Copy link
Author

@diegomrsantos diegomrsantos Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is adding the ENRs from the cli into the network config. LH does the same https://github.com/sigp/lighthouse/blob/stable/beacon_node/src/config.rs#L1159

Copy link
Member

Choose a reason for hiding this comment

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

LH doing something shouldn't be used as the only source of truth to follow for anchor.
In this case specifically lighthouse is not using clap derive yet (it's an ongoing process, see 1, 2).
Anchor is using clap derive from the beginning so we don't need to iterate the cli arguments as a string separated by commas, we already have a Vec<String>.
What I meant in the previous comment was that we can still have config.network.boot_nodes_enr be Vec<String>, and just call

config.network.boot_nodes_enr.append(cli_args.boot_nodes_enr)

and then on https://github.com/sigp/anchor/pull/72/files#diff-377ff7ebcbab6b7243aedbcf1c9139b355dd5294398ca8b0b218c32ccf2f1387R121 we parse them, so that we we only iterate the enr bootnodes list once

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for clarifying, I understand now. In this case, the vector will only contain a few values. Would this optimization have a significant impact on performance?

Copy link
Member

Choose a reason for hiding this comment

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

no worries Diego. probably it won't have a big impact on the performance but it isn't only about performance, it also makes the code more elegant at no cost

Copy link
Author

Choose a reason for hiding this comment

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

There's also the validation perspective. It's probably a good idea to parse and check if the ENR is valid here and add only valid ones to the network config.


config.beacon_nodes_tls_certs = cli_args.beacon_nodes_tls_certs.clone();
config.execution_nodes_tls_certs = cli_args.execution_nodes_tls_certs.clone();

Expand Down
3 changes: 3 additions & 0 deletions anchor/network/src/behaviour.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::discovery::Discovery;
use libp2p::swarm::NetworkBehaviour;
use libp2p::{gossipsub, identify, ping};

Expand All @@ -9,4 +10,6 @@ pub struct AnchorBehaviour {
pub ping: ping::Behaviour,
/// The routing pub-sub mechanism for Anchor.
pub gossipsub: gossipsub::Behaviour,
/// Discv5 Discovery protocol.
pub discovery: Discovery,
}
4 changes: 4 additions & 0 deletions anchor/network/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ pub struct Config {
/// Disables peer scoring altogether.
pub disable_peer_scoring: bool,

/// Disables the discovery protocol from starting.
pub disable_discovery: bool,

/// Disables quic support.
pub disable_quic_support: bool,

Expand Down Expand Up @@ -94,6 +97,7 @@ impl Default for Config {
boot_nodes_enr: vec![],
boot_nodes_multiaddr: vec![],
disable_peer_scoring: false,
disable_discovery: false,
disable_quic_support: false,
topics: vec![],
}
Expand Down
Loading
Loading