upgrade to Libp2p 0.52 by divagant-martian · Pull Request #4087 · sigp/lighthouse

divagant-martian · 2023-03-14T23:22:07Z

Issue Addressed

Which issue # does this PR address?

Proposed Changes

Please list or describe the changes introduced by this PR.

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

divagant-martian · 2023-03-16T14:55:25Z

put on halt until libp2p/rust-libp2p#3623 is somewhat addressed. Right now the suggested solution is to change the way we encode and store on disk keys, which makes no sense to me.

Possible solution addressing this: libp2p/rust-libp2p#3626 waiting for a release that contains this

thomaseizinger

Some thoughts, take as you will :)

beacon_node/lighthouse_network/src/config.rs

beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs

beacon_node/lighthouse_network/src/service/behaviour.rs

thomaseizinger · 2023-03-16T16:17:37Z

beacon_node/lighthouse_network/src/service/mod.rs

                )
                .notify_handler_buffer_size(std::num::NonZeroUsize::new(7).expect("Not zero"))
-                .connection_event_buffer_size(64)
+                .per_connection_event_buffer_size(4)


This is less than the default, FYI.

we allow one connection per peer and have a target peer count of 80. So I think this is still more than what the previous value allowed. I'm changing it to 4 (I'm aware the default is 7) for now until we can have an internal discussion about this

thomaseizinger · 2023-03-20T12:47:53Z

Possible solution addressing this: libp2p/rust-libp2p#3626 waiting for a release that contains this

This is now released: https://crates.io/crates/libp2p-identity/0.1.1

cargo update -p libp2p-identity should do the trick.

divagant-martian · 2023-03-20T17:38:25Z

Possible solution addressing this: libp2p/rust-libp2p#3626 waiting for a release that contains this

This is now released: https://crates.io/crates/libp2p-identity/0.1.1

cargo update -p libp2p-identity should do the trick.

Thanks @thomaseizinger this helped in some places. On another note, I added a new issue since I'm encountering a lot of unnecessary clones while trying to deal with references in libp2p/rust-libp2p#3649

thomaseizinger · 2023-03-21T14:38:37Z

beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs

+    fn on_connection_handler_event(
+        &mut self,
+        _peer_id: PeerId,
+        _connection_id: ConnectionId,
+        _event: libp2p::swarm::THandlerOutEvent<Self>,
+    ) {
+        todo!()
+    }


I would recommend:

Suggested change

fn on_connection_handler_event(

&mut self,

_peer_id: PeerId,

_connection_id: ConnectionId,

_event: libp2p::swarm::THandlerOutEvent<Self>,

) {

todo!()

}

fn on_connection_handler_event(

&mut self,

_peer_id: PeerId,

_connection_id: ConnectionId,

event: libp2p::swarm::THandlerOutEvent<Self>,

) {

void::unreachable(event)

}

thomaseizinger · 2023-03-21T14:39:44Z

beacon_node/lighthouse_network/src/service/utils.rs

    yamux_config.set_window_update_mode(libp2p::yamux::WindowUpdateMode::on_read());
+    let multiplexer = core::upgrade::SelectUpgrade::new(yamux_config, mplex_config);
+    let (transport, bandwidth) = transport
+        .upgrade(core::upgrade::Version::V1)


In general (not just due to this upgrade): Consider using Version::V1Lazy for 0-RTT protocol negotiation.

thanks, this one is new to me, I'll be reading about this

This reverts commit 58bd2f7.

This reverts commit ab8dad6, reversing changes made to d1a9635.

thomaseizinger

Made some comments on things I noticed, hope they are helpful :)

thomaseizinger · 2023-06-26T19:05:33Z

beacon_node/lighthouse_network/src/discovery/enr_ext.rs

-            }
-            Keypair::Ecdsa(_) => Err("Ecdsa keypairs not supported"),
+    fn from_libp2p(key: Keypair) -> Result<CombinedKey, &'static str> {
+        // TODO: more clones that should not be happening.


As you probably noticed, libp2p/rust-libp2p#3649 is now resolved. Let me know if you'd like a patch release for that.

thomaseizinger · 2023-06-26T19:06:43Z

beacon_node/lighthouse_network/src/discovery/mod.rs

+        _local_addr: &Multiaddr,
+        _remote_addr: &Multiaddr,
+    ) -> Result<(), libp2p::swarm::ConnectionDenied> {
+        todo!()


There is no need to override this function if you are not doing any connection management of pending connections.

thomaseizinger · 2023-06-26T19:07:45Z

beacon_node/lighthouse_network/src/discovery/mod.rs

@@ -925,31 +926,94 @@ impl<TSpec: EthSpec> Discovery<TSpec> {
 impl<TSpec: EthSpec> NetworkBehaviour for Discovery<TSpec> {
    // Discovery is not a real NetworkBehaviour...


FWIW, we see NetworkBehaviours as plugins and mDNS for example is also a NetworkBehaviour so implementing discovery as one is totally fine.

thomaseizinger · 2023-06-26T19:08:28Z

beacon_node/lighthouse_network/src/discovery/mod.rs

+    fn handle_established_inbound_connection(
+        &mut self,
+        _connection_id: ConnectionId,
+        peer: PeerId,
+        local_addr: &Multiaddr,
+        remote_addr: &Multiaddr,
+    ) -> Result<libp2p::swarm::THandler<Self>, libp2p::swarm::ConnectionDenied> {
+        todo!()
+    }
+
+    #[allow(unused)]
+    fn handle_established_outbound_connection(
+        &mut self,
+        _connection_id: ConnectionId,
+        peer: PeerId,
+        addr: &Multiaddr,
+        role_override: libp2p::core::Endpoint,
+    ) -> Result<libp2p::swarm::THandler<Self>, libp2p::swarm::ConnectionDenied> {
+        todo!()
+    }


These will need to return a handler otherwise you'll panic on the first established connection.

thomaseizinger · 2023-06-26T19:09:09Z

beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs

+        _local_addr: &libp2p::Multiaddr,
+        _remote_addr: &libp2p::Multiaddr,
+    ) -> Result<(), libp2p::swarm::ConnectionDenied> {
+        // TODO... is it guaranteed that the ip the peer is connecting from is in the _remote_addr?


It is taken from the socket but we can't do anything if the remote spoofs their IP.

thomaseizinger · 2023-06-26T19:10:25Z

beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs

+        //             // What happens if another behaviour prevents our dial attempt? do we get any
+        //             // notification to get the peer out of dialing state? dial failed?


Yes, you will get a FromSwarm::DialFailure with a DialError::Denied.

thomaseizinger · 2023-06-26T19:12:07Z

beacon_node/lighthouse_network/src/rpc/mod.rs

+    #[allow(unused)]
+    fn handle_pending_outbound_connection(
+        &mut self,
+        _connection_id: ConnectionId,
+        maybe_peer: Option<PeerId>,
+        _addresses: &[libp2p::Multiaddr],
+        _effective_role: libp2p::core::Endpoint,
+    ) -> Result<Vec<libp2p::Multiaddr>, libp2p::swarm::ConnectionDenied> {
+        todo!()
+    }


These have a sensible default impl which I'd use instead of overwriting it.

divagant-martian · 2023-10-23T02:53:55Z

closed in favor of #4431

divagant-martian added blocked Networking labels Mar 16, 2023

thomaseizinger reviewed Mar 16, 2023

View reviewed changes

thomaseizinger reviewed Mar 21, 2023

View reviewed changes

divagant-martian added 22 commits April 13, 2023 12:26

update libp2p to v0.51.1

7dd0ea0

Revert "Ensure protoc is installed for release CI (sigp#3621)"

f3693c2

This reverts commit 58bd2f7.

Revert "Merge pull request sigp#3546 from divagant-martian/cross-update"

b700771

This reverts commit ab8dad6, reversing changes made to d1a9635.

remove protobuf installation ops and instructions

28e02c4

update path for ConnectionId

672807b

update deprecated type aliases

2cbd05a

use new identity top level crate

77681d2

fix bandwidth logging code

d273f1b

update most of the handler required changes

d203526

update behaviour function names in the rpc

0e87da9

weird change needed for the NetworkBehaviour macro to work

d213b6a

update prometheus client

07b711b

fix updates to the rpc handler

ec8fc7d

partially update the peer manager's network behaviour code

26d24ff

fix generics over NetworkBehaviourAction

d004913

fix rpc network behaviour

51445b5

fix connection event buffer limits

a596d54

fix discovery and rpc network behaviour impls

a5c2e72

use utf encoding in prometheus to align with open metrics format

05ea5e6

remove unnecesary code

d425cb5

update libp2p-identity to latest release

b76e3b6

fix warnings where the fix does not bring an unnecesary clone

823828a

divagant-martian added 25 commits June 23, 2023 12:18

follow suggested code style

007e6db

Merge remote-tracking branch 'origin/libp2p-0.51.1' into libp2p-0.51.1

9e47c60

follow suggested code style

c97c5ff

incrementally addressing deprecations

e01e17c

use gossipsub types from the root

9949dfa

remove ProtocolName

fcbae56

no deprecations checkpoint

0ce49bd

remove handling of error that was dead code in libp2p

06b9da1

remove deprecated ban reports

0f9f012

placeholders everywhere

3a09b5d

address change to StreamUpgradeError

ce2c34b

address change to StreamUpgradeError

0102ee7

Merge remote-tracking branch 'origin/libp2p-0.51.1' into libp2p-0.51.1

4ab0ef4

change to Stream

d537025

so many renamings omg

1a2fe18

rename Custom variant to NotifyBehaviour

0e509e5

use new event field

9c4b429

add new events reporting addresses

c95c5c5

checkpoint

165f2d2

change identity path

73947b5

multiaddr can no longer produce invalid peer ids

4fa7762

checkpoint

1dd0420

v0.52. Finally

eeb9232

cargo update

e62db42

Merge branch 'unstable' into libp2p-0.51.1

44e4e90

divagant-martian changed the title ~~upgrade to Libp2p 0.51.3~~ upgrade to Libp2p 0.52 Jun 24, 2023

thomaseizinger reviewed Jun 26, 2023

View reviewed changes

mxinden mentioned this pull request Aug 10, 2023

chore: rm rust-libp2p v0.49 and v0.50, update v0.52 libp2p/test-plans#253

Merged

divagant-martian closed this Oct 23, 2023

		@@ -925,31 +926,94 @@ impl<TSpec: EthSpec> Discovery<TSpec> {
		impl<TSpec: EthSpec> NetworkBehaviour for Discovery<TSpec> {
		// Discovery is not a real NetworkBehaviour...

		// // What happens if another behaviour prevents our dial attempt? do we get any
		// // notification to get the peer out of dialing state? dial failed?

Conversation

divagant-martian commented Mar 14, 2023

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

divagant-martian commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

divagant-martian commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

thomaseizinger 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!

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!

divagant-martian commented Oct 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

divagant-martian commented Mar 16, 2023 •

edited

Loading

thomaseizinger commented Mar 20, 2023 •

edited

Loading

divagant-martian commented Mar 20, 2023 •

edited

Loading