feat(swarm): introduce ranked external addr registry #4689

mxinden · 2023-10-19T18:51:16Z

Description

Ranked external address registry based on the previous https://github.com/libp2p/rust-libp2p/blob/v0.51/swarm/src/registry.rs pluggeable as a NetworkBehaviour. Allows confirming external addresses in a probabilistic manner.

Notes & open questions

See context and discussion in #4688.

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

Ranked external address registry based on the previous https://github.com/libp2p/rust-libp2p/blob/v0.51/swarm/src/registry.rs pluggeable as a `NetworkBehaviour`. Allows confirming external addresses in a probabilistic manner.

mxinden · 2023-10-19T18:52:11Z

swarm/src/registry.rs

+impl NetworkBehaviour for Addresses {
+    type ConnectionHandler = crate::dummy::ConnectionHandler;
+
+    type ToSwarm = Void;
+
+    fn handle_established_inbound_connection(
+        &mut self,
+        _connection_id: crate::ConnectionId,
+        peer: libp2p_identity::PeerId,
+        local_addr: &Multiaddr,
+        remote_addr: &Multiaddr,
+    ) -> Result<crate::THandler<Self>, crate::ConnectionDenied> {
+        Ok(crate::dummy::ConnectionHandler)
+    }
+
+    fn handle_established_outbound_connection(
+        &mut self,
+        _connection_id: crate::ConnectionId,
+        peer: libp2p_identity::PeerId,
+        addr: &Multiaddr,
+        role_override: libp2p_core::Endpoint,
+    ) -> Result<crate::THandler<Self>, crate::ConnectionDenied> {
+        Ok(crate::dummy::ConnectionHandler)
+    }
+
+    fn on_swarm_event(&mut self, event: crate::FromSwarm<Self::ConnectionHandler>) {
+        match event {
+            crate::FromSwarm::ConnectionEstablished(_) => todo!(),
+            crate::FromSwarm::ConnectionClosed(_) => todo!(),
+            crate::FromSwarm::AddressChange(_) => todo!(),
+            crate::FromSwarm::DialFailure(_) => todo!(),
+            crate::FromSwarm::ListenFailure(_) => todo!(),
+            crate::FromSwarm::NewListener(_) => todo!(),
+            crate::FromSwarm::NewListenAddr(_) => todo!(),
+            crate::FromSwarm::ExpiredListenAddr(_) => todo!(),
+            crate::FromSwarm::ListenerError(_) => todo!(),
+            crate::FromSwarm::ListenerClosed(_) => todo!(),
+            crate::FromSwarm::NewExternalAddrCandidate(NewExternalAddrCandidate { addr }) => {
+                match self.add(addr.clone(), AddressScore::Finite(1)) {
+                    AddAddressResult::Inserted { expired } => {
+                        todo!("emit ToSwarm::NewExternalAddr(addr) and ToSwarm::ExternalAddrExpired(expired)")
+                    }
+                    AddAddressResult::Updated { expired } => {
+                        todo!("emit ToSwarm::ExternalAddrExpired(expired)")
+                    }
+                };
+            }
+            crate::FromSwarm::ExternalAddrConfirmed(ExternalAddrConfirmed { addr }) => {
+                self.add(addr.clone(), AddressScore::Infinite);
+            }
+            crate::FromSwarm::ExternalAddrExpired(ExternalAddrExpired { addr }) => {
+                self.remove(addr);
+            }
+        }
+    }
+
+    fn on_connection_handler_event(
+        &mut self,
+        _peer_id: libp2p_identity::PeerId,
+        _connection_id: crate::ConnectionId,
+        event: crate::THandlerOutEvent<Self>,
+    ) {
+        void::unreachable(event)
+    }
+
+    fn poll(
+        &mut self,
+        _cx: &mut std::task::Context<'_>,
+        _params: &mut impl crate::PollParameters,
+    ) -> std::task::Poll<crate::ToSwarm<Self::ToSwarm, crate::THandlerInEvent<Self>>> {
+        Poll::Pending
+    }
+}


Note the only addition to the v0.51 version, namely the NetworkBehaviour implementation that would allow one to simply plug this into ones NetworkBehaviour tree.

thomaseizinger

I think it makes sense to add something like this although I'd like to see the following changes:

Move it to a dedicated crate under misc/.
Don't call it registry but something that communicates the probabilistic nature of these external addresses.
Introduce a threshold of 2 for when we promote a candidate to an external address. That should effectively never promote ephemeral ports as external addresses.

thomaseizinger · 2023-10-19T22:45:53Z

Move it to a dedicated crate under misc/.

Alternatively, we could also re-add such functionality back into Swarm but that would go against the current design philosophy of moving this kind of functionality into plugins.

mxinden · 2023-10-20T09:54:46Z

Introduce a threshold of 2 for when we promote a candidate to an external address. That should effectively never promote ephemeral ports as external addresses.

I had the same thought. Problem though: libp2p-identify reports an observed address on each identify exchange as a new candidate. Two identify exchanges on the same connection could thus lead to an address being confirmed, even though the connection might be on an ephemeral port.

thomaseizinger · 2023-10-20T10:13:02Z

Introduce a threshold of 2 for when we promote a candidate to an external address. That should effectively never promote ephemeral ports as external addresses.

I had the same thought. Problem though: libp2p-identify reports an observed address on each identify exchange as a new candidate. Two identify exchanges on the same connection could thus lead to an address being confirmed, even though the connection might be on an ephemeral port.

We can fix that right? We can remember, which ones we already reported and don't report them again. That might actually be more correct because the API says NewExternalAddrCandidate.

thomaseizinger · 2023-10-20T10:16:30Z

Introduce a threshold of 2 for when we promote a candidate to an external address. That should effectively never promote ephemeral ports as external addresses.

I had the same thought. Problem though: libp2p-identify reports an observed address on each identify exchange as a new candidate. Two identify exchanges on the same connection could thus lead to an address being confirmed, even though the connection might be on an ephemeral port.

We can fix that right? We can remember, which ones we already reported and don't report them again. That might actually be more correct because the API says NewExternalAddrCandidate.

In theory, a connection could roam and change its address. If we want to ignore that for now, we could also go with a "Only report once" behaviour.

thomaseizinger · 2023-10-20T10:23:50Z

Another idea:

We make libp2p-identify smarter: Instead of naively reporting each observed address, we only emit a candidate once at least X (default 2?) nodes reported us the same address. We might have to make it configurable for tests but this could be a good fix until we have the new PortUse information available, at which point we can probably just rely on that.

thomaseizinger · 2023-10-25T02:17:21Z

Introduce a threshold of 2 for when we promote a candidate to an external address. That should effectively never promote ephemeral ports as external addresses.

I had the same thought. Problem though: libp2p-identify reports an observed address on each identify exchange as a new candidate. Two identify exchanges on the same connection could thus lead to an address being confirmed, even though the connection might be on an ephemeral port.

We can fix that right? We can remember, which ones we already reported and don't report them again. That might actually be more correct because the API says NewExternalAddrCandidate.

I implemented this here: #4721

thomaseizinger · 2023-11-14T02:28:12Z

Closing in favor of #4721.

feat(swarm): introduce ranked external addr registry

9cb79dc

Ranked external address registry based on the previous https://github.com/libp2p/rust-libp2p/blob/v0.51/swarm/src/registry.rs pluggeable as a `NetworkBehaviour`. Allows confirming external addresses in a probabilistic manner.

mxinden commented Oct 19, 2023

View reviewed changes

thomaseizinger reviewed Oct 19, 2023

View reviewed changes

mxinden mentioned this pull request Oct 20, 2023

Upgrade libp2p to 0.52.4 paritytech/polkadot-sdk#1631

Merged

7 tasks

thomaseizinger closed this Nov 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(swarm): introduce ranked external addr registry #4689

feat(swarm): introduce ranked external addr registry #4689

mxinden commented Oct 19, 2023

mxinden Oct 19, 2023

thomaseizinger left a comment

thomaseizinger commented Oct 19, 2023

mxinden commented Oct 20, 2023

thomaseizinger commented Oct 20, 2023

thomaseizinger commented Oct 20, 2023

thomaseizinger commented Oct 20, 2023

thomaseizinger commented Oct 25, 2023

thomaseizinger commented Nov 14, 2023

feat(swarm): introduce ranked external addr registry #4689

feat(swarm): introduce ranked external addr registry #4689

Conversation

mxinden commented Oct 19, 2023

Description

Notes & open questions

Change checklist

mxinden Oct 19, 2023

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Oct 19, 2023

mxinden commented Oct 20, 2023

thomaseizinger commented Oct 20, 2023

thomaseizinger commented Oct 20, 2023

thomaseizinger commented Oct 20, 2023

thomaseizinger commented Oct 25, 2023

thomaseizinger commented Nov 14, 2023