feat(p2p): support inbound-only peers without underlay addresses#5326
feat(p2p): support inbound-only peers without underlay addresses#5326gacevicljubisa wants to merge 7 commits intomasterfrom
Conversation
janos
left a comment
There was a problem hiding this comment.
Very nice, maybe just to add some tests for behaviour changes.
|
Also, do not add peer without underlays to the addressbook in hive here https://github.com/ethersphere/bee/blob/master/pkg/hive/hive.go#L314. |
They are skipped here Lines 288 to 291 in 706fe4c |
pkg/p2p/libp2p/libp2p_test.go
Outdated
| // If emptyUnderlays is set, use a custom host factory that creates a host with no listen addresses | ||
| // This simulates an inbound-only peer (browser, WebRTC connection, strict NAT) | ||
| if o.emptyUnderlays { | ||
| opts = libp2p.WithHostFactory( |
There was a problem hiding this comment.
This overwrites opts, losing any previously set fields like FullNode from o.libp2pOpts.
I think we should append to previous opts ?
There was a problem hiding this comment.
Yes, this makes emptyUnderlays option coupled with libp2pOpts in a not transparent way.
It is a problem with WithHostFactory function that, just returns the new libp2p.Options instead just to set the unexported field internally. This is just not practical, or composable.
I suggest to add in export_test.go:
func SetHostFactory(o *Options, factory func(...libp2pm.Option) (host.Host, error)) {
o.hostFactory = factory
}as more flexible alternative. WithHostFactory is not even named clearly, its name does not suggest that it is (one of) a constructor of the Options type.
| expectPeers(t, s2, overlay1) | ||
| expectPeersEventually(t, s1, overlay2) | ||
|
|
||
| // Verify s1 (has underlays) is persisted in s2's addressbook |
There was a problem hiding this comment.
I think we should add this test case:
s2 should not be saved in s1's addressbook
|
I'm not sure this is necessary. From browser connecting to testnet wss addresses is already possible. The browser node also has an (ephemeral) underlay. |
|
Without this Bee in browser fails to maintain connections. It is really necessary. I dont see any drawback to not support inbound-only peers |
@lat-murmeldjur Can you provide more details about ephemeral underlay? |
Yes. This is our ephemeral underlay in the browser, observed by the bee node, and sent back to the browser in the libp2p-identify protocol. The reason why getting it through this libp2p-protocol was necessary, is because connecting to a tls/ws address broke 1 thing: |
As I understand, with this PR, the browser client doesn't need the workaround of reading the observed underlay from libp2p-identify before the handshake. The handshake now accepts empty underlays, so the browser can complete the handshake immediately without waiting for the observed address. |
It's okay I'm not objecting to this change just wanted to make it clear that strictly speaking you don't need this to make connections work. |
|
With introducing the RemoteMultiaddr this PR is no longer required. |
|
@gacevicljubisa I'll comment here, so that we don't pollute the other PR. The problem IMO that needs to be resolved is that a solution should not require some multiaddr. Sure, it's nice to send an observed multiaddr from the connectee back to the connector, however for NAT-bound peers connecting out, it has no relevance (it does let them know that they are connected to a globally routable network, albeit via NAT, but the success of a connection is enough in and of itself to satisfy this). Subsequently, the address was being added to the peer store, which is suitable for the connectee, but pointless from a It seems odd to be signing a multiaddr, simply just trusting it, in order to satisfy the handshake, and actual fact, why can't the ack have a bzz address with no signed multiaddr in it (ie. the signature is over an empty Vec). Examples of where this is particularly painful is that with the introduction of the peer store waiting, this essentially causes all light clients behind NAT to get blocked for 10s on connection. |
|
Hi @mfw78, Thanks for taking the time to lay this out and explain your concerns. To make sure this gets the proper visibility and structured discussion it deserves, could you please open a dedicated issue in the repository outlining:
We’d like our research team to review this in more depth and evaluate whether this requirement should exist in its current form, as well as the protocol-level implications of changing it. Opening an issue will help us track the discussion properly without losing context in the PR thread. Appreciate the thoughtful feedback! |
Checklist
Description
Allow peers without dialable underlay addresses (e.g., browsers, WebRTC
clients, NAT-restricted nodes) to establish connections and use protocols
over existing streams.
Changes:
Inbound-only peers can participate in protocols (pricing, retrieval, pss)
but are excluded from Kademlia topology and hive gossip since they cannot
be dialed back.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):