feat: multiple underlay addresses #5204
Conversation
sbackend123
force-pushed
the
multiple-underlay-addresses
branch
3 times, most recently
from
cb33ff1 to
1e38f87
Compare
pkg/bzz/address.go
Outdated
| Signature string `json:"signature"` | ||
| Nonce string `json:"transaction"` | ||
| Overlay string `json:"overlay"` | ||
| Underlay []string `json:"underlay"` |
There was a problem hiding this comment.
Maybe plural in Field name and json tag name?
pkg/hive/hive.go
Outdated
| for _, underlay := range addr.Underlay { | ||
| if !s.allowPrivateCIDRs && manet.IsPrivateAddr(underlay) { | ||
| isPrivate = true | ||
| break |
There was a problem hiding this comment.
Actually, here, we should filter out private CIDRs and advetize only the public ones. Here we will not advertize any node that will have at leas one private mutliaddr. If there is at least one public multiaddr, the node should be advertized, if all underlays are private, than it should be skipped.
There was a problem hiding this comment.
Sorry, didn't get you
There was a problem hiding this comment.
This change will set as any node as "private" (isPrivate = true) that has at least one underlay address with a private IP address. But it may be and is very possible that that node has other underlay addresses that are not private, and the whole node will be skipped in advertizement thought he hive protocol (as per current state of the changeset).
Actually, the node should be considered as "private" only if all underlay addresses have a private IP address. Or node should not be considered as private if it has at least one underlay address with a public IP address.
In short, if allowPrivateCIDRs is false than filter out all underlay addresses that have private IP address and send only them through the hive message. If the length of such filtered underlay addresses is 0, do not advertize the node at all. If allowPrivateCIDRs is true, no filtering is needed and all nodeses are advetized.
| AddProtocol(ProtocolSpec) error | ||
| // Connect to a peer but do not notify topology about the established connection. | ||
| Connect(ctx context.Context, addr ma.Multiaddr) (address *bzz.Address, err error) | ||
| Connect(ctx context.Context, addrs []ma.Multiaddr) (address *bzz.Address, err error) |
There was a problem hiding this comment.
It is a question is the responsibility of the p2p Service implementation to choose which underlay address to use to connect to. One way to look at this is to have the same logic in one place transparent to the caller of the Connect function. This could be fine completely. I am just thinking would keeping the Connect function predictable to the caller with only one underlay address be more clear. In that case, the caller would know if the connection succeeded with the exactly the underlay that it passed to the Connect function. This may have a consequence that the logic of selecting the "desired" underlay address would need to be used always before Connect, and if it is always the same logic, than it makes sense to be hidden within the Connect function, but this can remove some flexibility that may be needed in some parts of the code. Again, it is nice to have libp2ppeer.AddrInfo mutliple underlay addresses and let it deal with the connectivity internally. I am just thinking, I still do not have the answer for this change.
There was a problem hiding this comment.
I think it depends on how we choose the transport for a node that supports both TCP and WSS. If we make that choice explicitly, then it makes sense to keep Connect unchanged. Otherwise, the functionality provided by libp2p frees us from having to perform sequential dials across different underlays (which can be quite slow) or implement parallel dials (which increases code complexity).
There was a problem hiding this comment.
Yes, the case of libp2p functionality that is already provided is a strong one.
pkg/p2p/libp2p/libp2p.go
Outdated
|
|
||
| if err := s.connectionBreaker.Execute(func() error { return s.host.Connect(ctx, *info) }); err != nil { | ||
| // libp2p will chose first ready address (possible multi dial). | ||
| info := libp2ppeer.AddrInfo{ID: id, Addrs: remotes} |
There was a problem hiding this comment.
remotes that are returned from the normalizeAddrsSamePeer have "decapsulated" hostAddr. Are such multiadds ok (complete) to be set as AddrInfo.Addrs?
There was a problem hiding this comment.
as far as I understand - yes, if we still have peer id
pkg/hive/hive.go
Outdated
|
|
||
| // check if the underlay is usable by doing a raw ping using libp2p | ||
| if _, err := s.streamer.Ping(ctx, multiUnderlay); err != nil { | ||
| // check if node is running in browser, and if it is, ping wss only |
There was a problem hiding this comment.
The comment refers to a particular situation, but the code is actually more general, trying to find any reachable underlay, which would work just fine in wss case as well. I think that the comment should be updated.
pkg/bzz/address.go
Outdated
| } | ||
|
|
||
| return a.Equal(b) | ||
| func IsUnderlayEqual(a, b []ma.Multiaddr) bool { |
There was a problem hiding this comment.
Would name AreUnderlaysEqual (or something similar with plurals) make more sense as it compares lists?
martinconic
left a comment
martinconic
left a comment
There was a problem hiding this comment.
Please fix all commit messages according with this guideline commits msg guidelines
martinconic
force-pushed
the
multiple-underlay-addresses
branch
from
604aa4b to
2378edd
Compare
… side and on kademlia
…dresses by mapstructure
martinconic
force-pushed
the
multiple-underlay-addresses
branch
from
604aa4b to
7d3f603
Compare
Checklist
Description
Second part of PR: #5199
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Part of: Support for multiple underlay addresses for different node types (in-browser, bee server) to choose from. Issue #5201
Related Issue (Optional)
Screenshots (if appropriate):