Subtle DCUtR and Identify interactions

[dhuseby]

I'm writing this as a public service announcement. I don't think there is a bug here otherwise I would have filed an issue, but I do want to call this out so others can learn from my digging.

I've been trying to get the hole-punch test working in the new unified-testing framework. I wrote a rust-v0.56 relay and a rust-v0.56 peer that acts as both dialer and listener. The test framework uses Docker to set up three networks and five containers and a separate network with a redis server in it for OOB coordination and testing purposes. The relay is in one network, a peer instance in dialer mode is another and a peer instance in listener mode is in the last. Then there are two linux containers set up with NAT/firewall and two network interfaces, one interface on the relay's network and one interface on the dialer/listener networks. The code for these is here.

I just couldn't get the thing to work. I kept getting a DCUtR failure in the dialer (InboundError(Protocol(NoAddress))) and another in the listener (OutboundError(Io(Kind(UnexpectedEof)))).

After doing some deep root cause analysis, I discovered the listener's DCUtR CONNECT message contains zero addresses because DCUtR never receives the listener's external address. The cause is a subtle interaction between swarm.add_external_address() and identify's event ordering:

1. Identify event ordering: When identify completes, it queues events in this order: GenerateEvent(Received) first, then NewExternalAddrCandidate last.
2. My code poisoned the confirmed set: In both listener and dialer modes the Identify::Received handler called swarm.add_external_address(observed_addr). This inserts the address into the swarm's confirmed_external_addr set and emits FromSwarm::ExternalAddrConfirmed.
3. Identify's candidate is suppressed: When identify's ToSwarm::NewExternalAddrCandidate arrives next, the swarm checks if !self.confirmed_external_addr.contains(&addr) -- since we already confirmed it, both the FromSwarm::NewExternalAddrCandidate broadcast to behaviours AND the SwarmEvent::NewExternalAddrCandidate emission are skipped.
4. DCUtR never gets the address: DCUtR only handles FromSwarm::NewExternalAddrCandidate in on_swarm_event (ignores ExternalAddrConfirmed). Since the broadcast was suppressed, DCUtR's address_candidates remains empty which leads to the empty CONNECT and NoAddresses error.

What is the correct way to do DCUtR?

The short answer is never call swarm.add_external_address(address.clone()) in the swarm::SwarmEvent::NewExternalAddrCandidate { address } arm. You should also never call swarm.add_external_address(observed_addr.clone()) in the swarm::SwarmEvent::Behaviour(BehaviourEvent::Identify(identify::Event::Received { info: identify::Info { observed_addr, ..}, .. })) arm. I think you should just never call swarm.add_external_address. Because without letting the FromSwarm event to propagate, there's no way for DCUtR to get the external address it requires to succeed.

So maybe there is a bug-ish thing here?

[dhuseby]

I just submitted libp2p/unified-testing#48 with correct relay and peer code. I kept it as minimal and straightforward as possible. The dcutr example in this repo works because it does nothing other than to log the behaviour events that, if handled incorrectly, can mess up the dcutr state.

[dariusc93]

Hey @dhuseby. Looking at the relay code, I dont see anywhere where it is adding the address as an external address as it is done in the relay example, pretty much letting the local node what address is reachable.

EDIT: Just saw part of the post about warm.add_external_address(address.clone()) so you could ignore what I said above slightly. I usually use SwarmEvent::NewListenAddr to determine what address to add, or in the case of UPnP, I would grab the address from there (via ToSwarm::ExternalAddrConfirmed or Event::NewExternalAddr), though at that point it would have been added. If the address you are listening on is already publicly reachable, then you would need to add it manually.

EDIT2: To add another comment, if you are listening on the address directly or using 0.0.0.0 (or the ipv6 equivalent), with one of the addresses being public and reachable, you could check the multiaddr ip to determine if its not a private or local ip. Eg in rust-ipfs I used to do the following.

[dhuseby]

@dariusc93 this is the networking setup that the hole-punch test uses:

• The listener and dialer LAN's both get a default route to the WAN network via the IP address of their respective NAT's.
• From the listener perspective, the relay is publicly accessible but the dialer is not.
• From the dialer perspective, the relay is publicly accessible but the listener is not.
• Both the listener and dialer listen on 0.0.0.0 and use the SwarmEvent::NewListenAddr to determine their non-localhost address

The point of the original post was to point out that calling swarm.add_external_address() from event handlers that get the observed address reported is incorrect because it prevents the dcutr state machine from receiving the FromSwarm::NewExternalAddrCandidate event. It's a subtle foot-gun I ran into.