chore: fix connectedness

[2color]

This PR builds on #974 and fixes a bug whereby FindPeer won't return results for peers behind NAT which only have p2p-circuit multiaddrs.

This fixes the regression starting from v0.34 of go-libp2p:

Peers connected to the host via relayed or any other limited connection now report their connectivity state as Limited.
NOTE: This changes the behavior of the Connected Connectedness state. Previously it included all limited connections and now it doesn't. To keep existing behavior in your code you can replace checks connectedness == network.Connected with connectedness != network.NotConnected

[guillaumemichel]

Thanks @2color! Is there a way to test if Limited connectivity (in kad-dht) must be considered as Connected from Kubo?

We may want to reverse the fix on TestRTEvictionOnFailedQuery after libp2p/go-libp2p#2916 is resolved.

[2color]

Not sure I understood the question @guillaumemichel

But I presume that in some instances, we want to make this distinction in the context of Kubo.

[sukunrt]

If a host.Connect is fine with a Limited connection then we should use the dial option network.WithAllowLimitedConn. This works currently because of a go-libp2p bug. We should fix it right now so that it doesn't break silently in the future.

ctx = network.WithAllowLimitedConn(ctx, "dht-find-peer")
h.Connect(ctx, ...)

libp2p/go-libp2p#2714

[sukunrt]

NIT:

dht.host.Network().Connectedness(p) != network.NotConnected 

is simpler and reads better, I think.

[2color]

I was apprehensive about the double negation making it less readable and wanted to ensure it's future proof.

[sukunrt]

We can handle this in a different PR. Preferably before a release.

[guillaumemichel]

Not sure I understood the question @guillaumemichel

Sorry, I meant:

Is there a way to verify that Kubo is fine with Limited connections, or if it requires peers to be actually Connected? Your comment mentions that you want to make sure whether we need this change everywhere.

In other words does Kubo need

 if connectedness == network.Connected || connectedness == network.Limited
(or connectedness != network.NotConnected)

or

 if connectedness == network.Connected

[2color]

Is there a way to verify that Kubo is fine with Limited connections, or if it requires peers to be actually Connected? Your #976 (comment) mentions that you want to make sure whether we need this change everywhere.

I'll let @lidel or @aschmahmann chime in regarding this, as I'm not familiar enough with the Kubo/Boxo codebase.

[aschmahmann]

Is there a way to verify that Kubo is fine with Limited connections

Hope I'm answering the question here, if not ping and I'll try again 😅.

In practice the libp2p protocols that kubo relies on (with the exception of the ping protocol I suppose) do not want to work over limited connections (e.g. bitswap, kad, p2p-forwarding, trustless-gateway over libp2p, etc.). However, those protocols should force the transport to upgrade when we call NewStream, right?

The main area (aside from ping) where I could see limited connections useful at the application layer (as opposed to internally to the libp2p transport layer for DCUtR, etc.) is related to Kademlia's FindPeer

• It's sort of awkward that FindPeer actually does the connection to the peer (which happens both because of the protocol not differentiating FIND_NODE from FIND_PEER, and because of the lack of signed peer records encouraging us to make sure we have the "real addresses") rather than just returning addresses
• However, at an API layer we don't really need a full connection here (although work might be needed to see if this behavior was relied on)
• Commands like ipfs routing findpeer and ipfs id <peerID> seem like they should be fine even with a limited transport whereas others like ipfs swarm connect would want a non-limited transport
  • However, it's hard to know what implicit behaviors people were relying on even if not guaranteed by the interfaces, APIs, commands, etc.

[sukunrt]

I have a couple of questions

1. Should dht.FindPeer return peers with only p2p-circuit addresses?
2. Should someguy delegated routing https://delegated-ipfs.dev/routing/v1/peers/12D3KooWRBy97UB99e3J6hiPesre1MZeuNQvfan4gBziswrRJsNK use a dht.FindPeer query to find the peer's info?

[Stebalien]

Should dht.FindPeer return peers with only p2p-circuit addresses?

It absolutely must or it breaks peer routing for NATed peers.

NOTE:

1. Peers with only circuit addresses shouldn't be added to the routing table.
2. In general, we should be careful to distinguish between "p2p-circuit" and "transient".

[2color]

In general, we should be careful to distinguish between "p2p-circuit" and "transient".

Can you explain what you mean?

In what situations are the two not synonymous?

[Stebalien]

In what situations are the two not synonymous?

P2p circuit addresses can be used for non-transient connectivity as well. We don't run any non-transient relays, but users can always run/use their own.

Truly transient connections will have Limited set to true in the connection/stream stat, and the connectivity will be "transient".

[sukunrt]

Peers with only circuit addresses shouldn't be added to the routing table.

This is what was bothering me. If these peers are not in the routing table, how do we actually find them?
This is handled in this block of code where the server returns addresses for a target peer even if it's not in the routing table: https://github.com/libp2p/go-libp2p-kad-dht/blob/master/handlers.go#L268-L285

[Stebalien]

This is handled in this block of code where the server returns addresses for a target peer even if it's not in the routing table: https://github.com/libp2p/go-libp2p-kad-dht/blob/master/handlers.go#L268-L285

Yep! It's all a giant hack. Hacks all the way down.

[Stebalien]

(ideally we'd use actual peer records and keep them separate from the record table)

[lidel]

@MarcoPolo @Stebalien fysa this did not end up in v0.26.0, see #980

[MarcoPolo]

Oh this PR didn’t target master. Thanks