Add discv5 by diegomrsantos · Pull Request #72 · sigp/anchor

diegomrsantos · 2024-12-13T10:26:35Z

Issue Addressed

Add discv5 and partially implement #35.

Proposed Changes

Updated the discv5 dependency from version 0.8.0 to 0.9.0.
Added a new CLI argument boot_nodes_enr for specifying base64-encoded ENRs to bootstrap the p2p network.
Enhanced the configuration handling to include the new boot_nodes_enr argument and parse it correctly.
Introduced the Discovery module to handle peer discovery using the discv5 protocol.
Modified the network behavior to integrate the Discovery module, enabling peer discovery and connection management.
Added a configuration option disable_discovery to control the activation of the discovery protocol.

Additional Info

This is mostly copying the discovery code from LH for now while we figure out the best way to modularise and reuse this code.
This is the first step toward a full discovery implementation. More features must be added, like saving ENRs to the database.

diegomrsantos · 2024-12-17T11:38:52Z

anchor/network/src/discovery.rs

+            EventStream::InActive
+        };
+
+        if !network_config.boot_nodes_multiaddr.is_empty() {


Bootnodes MA might not be necessary. See https://github.com/ssvlabs/ssv-spec/blob/main/p2p/SPEC.md#discovery

Or maybe we should accept a UDP MA for discv5

diegomrsantos · 2024-12-17T14:11:22Z

I removed the hardcoded bootnode ENR. It's possible to use the holesky bootnode running: run --package anchor --bin anchor -- --boot-nodes-enr=enr:-Li4QFIQzamdvTxGJhvcXG_DFmCeyggSffDnllY5DiU47pd_K_1MRnSaJimWtfKJ-MD46jUX9TwgW5Jqe0t4pH41RYWGAYuFnlyth2F0dG5ldHOIAAAAAAAAAACEZXRoMpD1pf1CAAAAAP__________gmlkgnY0gmlwhCLdu_SJc2VjcDI1NmsxoQN4v-N9zFYwEqzGPBBX37q24QPFvAVUtokIo1fblIsmTIN0Y3CCE4uDdWRwgg-j

jxs

Thanks for starting this Diego! Left some comments, mainly I think we should not merge commented instructions without proper explaining on the why

jxs · 2024-12-17T11:50:48Z

anchor/network/src/network.rs

                            }
                        },
                        // TODO handle other swarm events
+                        SwarmEvent::NewListenAddr { .. } => {},


this isn't required for now right?

Good catch, removed.

jxs · 2024-12-17T14:07:27Z

anchor/network/src/network.rs

+        // Build and start the discovery sub-behaviour
+        let mut discovery = Discovery::new(local_keypair.clone(), network_config)
+            .await
+            .unwrap();


nit, either:

Suggested change

.unwrap();

.expect("Discovery should be able to start");

or ? to bubble up the error and treat it in the function caller

we'll need to refactor the function and return a Result instead.

Could we create an issue and address it in a new PR?

yeah ofc :)

jxs · 2024-12-17T15:06:44Z

anchor/client/src/cli.rs

+        global = true,
+        value_delimiter = ',',
+        help = "One or more comma-delimited base64-encoded ENR's to bootstrap the p2p network",
+        display_order = 0


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)

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

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

Yes, it's probably better in another PR.

jxs · 2024-12-17T16:41:04Z

anchor/network/src/discovery.rs

+        }
+
+        // Start the discv5 service and obtain an event stream
+        let event_stream = if !network_config.disable_discovery {


will we want to disable discovery? cc @Age @jking-aus

hmm not sure. It is mainly used in testing, where you can set the bootnodes and only connect to that node. It is kinda handy. Maybe worth keeping

jxs · 2024-12-17T16:41:59Z

anchor/network/src/discovery.rs

+        // TODO info!(log, "ENR Initialised"; "enr" => local_enr.to_base64(), "seq" => local_enr.seq(), "id"=> %local_enr.node_id(),
+        //       "ip4" => ?local_enr.ip4(), "udp4"=> ?local_enr.udp4(), "tcp4" => ?local_enr.tcp4(), "tcp6" => ?local_enr.tcp6(), "udp6" => ?local_enr.udp6(),
+        //       "quic4" => ?local_enr.quic4(), "quic6" => ?local_enr.quic6()
+        // );


this can be removed right?

We probably need a local_enr, but I don't understand how it works yet.

Added a TODO

jxs · 2024-12-17T16:49:28Z

anchor/network/src/discovery.rs

+            // TODO if bootnode_enr.node_id() == local_node_id {
+            //     // If we are a boot node, ignore adding it to the routing table
+            //     continue;
+            // }


is anchor planned to also be run as bootnode? If so this can be uncommented right?

Waiting for input here as I don't know the answer

I think so yep -- would be good to be able to provide anchor based bootnodes so not reliant on the go client for entrypoints

anchor/network/src/discovery.rs

jxs · 2024-12-17T16:55:20Z

anchor/network/src/discovery.rs

+    }
+}
+
+impl NetworkBehaviour for Discovery {


this can probably be moved into upstream discv5 feature gated by libp2p (like the enr crate does) as its shared by both lighthouse and anchor wdyt @AgeManning

Yeah, thats a good idea

oh except the types might be annoying. The NetworkBehaviour and libp2p traits, will come from the upstream libp2p dependency and then this won't work if Anchor sets its own libp2p version.

i.e We will have to always be doing releases for discv5 to keep up to libp2p, which could get annoying

the NetworkBehaviour traits (which they all depend) are contained in libp2p-swarm which updates roughly once a year, the last update to 0.45 was due to the Transport trait update. There isn't an update like that in sight wrt that I think we are ok

jxs · 2024-12-17T17:01:03Z

anchor/network/src/discovery.rs

+    // Add QUIC fields to the ENR.
+    // Since QUIC is used as an alternative transport for the libp2p protocols,
+    // the related fields should only be added when both QUIC and libp2p are enabled
+    if !config.disable_quic_support {
+        // If we are listening on ipv4, add the quic ipv4 port.
+        if let Some(quic4_port) = config.enr_quic4_port.or_else(|| {
+            config
+                .listen_addresses
+                .v4()
+                .and_then(|v4_addr| v4_addr.quic_port.try_into().ok())
+        }) {
+            builder.add_value(QUIC_ENR_KEY, &quic4_port.get());
+        }
+
+        // If we are listening on ipv6, add the quic ipv6 port.
+        if let Some(quic6_port) = config.enr_quic6_port.or_else(|| {
+            config
+                .listen_addresses
+                .v6()
+                .and_then(|v6_addr| v6_addr.quic_port.try_into().ok())
+        }) {
+            builder.add_value(QUIC6_ENR_KEY, &quic6_port.get());
+        }
+    }


the spec only mentions TCP as a transport, and the go ssv client doesn't seem to support it, do we plan to support it nonetheless?

We could support QUIC for communication between rust nodes.

Yeah, lets support it, and hopefully they follow suit

ok nice! Then we should probably create a PR upstream to manifest that

jking-aus

one small change then let's merge

anchor/network/src/network.rs

Co-authored-by: jking-aus <72330194+jking-aus@users.noreply.github.com>

diegomrsantos force-pushed the add-discv5 branch from cc3a2a2 to 5bbfb57 Compare December 16, 2024 23:07

diegomrsantos added 11 commits December 17, 2024 09:32

update discv5 to 0.9.0

248466c

add discovery module

f8ca4b4

add Discovery struct and implement NetworkBehaviour

c016651

add build_enr function

daff657

add discovery to AnchorBehaviour

e1b09d6

cargo fmt

467b48a

expose Enr

a12a8e9

add disable_discovery

f71df58

hardcode holesky bootnode

1f3d955

add discovery

b5cdbca

cargo fmt

2ca4167

diegomrsantos force-pushed the add-discv5 branch from 7eda5b5 to 2ca4167 Compare December 17, 2024 08:33

cargo clippy

cc5004e

diegomrsantos commented Dec 17, 2024

View reviewed changes

diegomrsantos marked this pull request as ready for review December 17, 2024 12:12

remove hardcoded bootnode enr

e532014

diegomrsantos force-pushed the add-discv5 branch from 4ea508d to e532014 Compare December 17, 2024 14:15

dial discovered peers

3080f3d

jxs reviewed Dec 17, 2024

View reviewed changes

diegomrsantos added 3 commits December 17, 2024 18:29

remove event

c091041

uncomment logs

2cd0e43

simplify error handling

afcfe99

diegomrsantos requested a review from jking-aus December 17, 2024 18:43

diegomrsantos and others added 3 commits December 18, 2024 12:54

TODO handle local enr

14fc072

add ssv predicate

ae8d4a9

Merge branch 'unstable' into add-discv5

19cdba9

jking-aus requested changes Dec 23, 2024

View reviewed changes

anchor/network/src/network.rs Outdated Show resolved Hide resolved

anchor/network/src/network.rs Outdated Show resolved Hide resolved

Update anchor/network/src/network.rs

92c0468

Co-authored-by: jking-aus <72330194+jking-aus@users.noreply.github.com>

diegomrsantos and others added 2 commits December 23, 2024 12:56

Update anchor/network/src/network.rs

a323635

Co-authored-by: jking-aus <72330194+jking-aus@users.noreply.github.com>

cargo fmt

a8b5f52

diegomrsantos requested a review from jking-aus December 23, 2024 12:09

fix formatting

24fa59a

jking-aus approved these changes Jan 2, 2025

View reviewed changes

jking-aus merged commit e15bdb4 into sigp:unstable Jan 2, 2025
9 checks passed

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

Comments

Conversation

diegomrsantos commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegomrsantos Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegomrsantos commented Dec 17, 2024

Uh oh!

jxs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegomrsantos Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegomrsantos Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jking-aus left a comment

diegomrsantos commented Dec 13, 2024 •

edited

Loading

diegomrsantos Dec 17, 2024 •

edited

Loading

diegomrsantos Dec 17, 2024 •

edited

Loading

diegomrsantos Dec 17, 2024 •

edited

Loading