feat(p2p): support inbound-only peers without underlay addresses #5326
Conversation
janos
left a comment
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 |
gacevicljubisa
requested review from
acud,
akrem-chabchoub,
janos and
martinconic
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. |
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):