autorelay: send addresses on eventbus; dont wrap address factory #3071
Conversation
sukunrt
force-pushed
the
sukun/autorelay-event
branch
2 times, most recently
from
2b114a2 to
762a43e
Compare
| @@ -9,6 +9,9 @@ import ( | |||
|
|
|||
| // This function cleans up a relay's address set to remove private addresses and curtail | |||
| // addrsplosion. | |||
| // TODO: Remove this, we don't need this. The current method tries to select the | |||
There was a problem hiding this comment.
Are we removing this for this PR?
There was a problem hiding this comment.
We should bound the number of addresses we re-advertise (which I'm assuming this
code does).
There was a problem hiding this comment.
will remove this in a follow up PR. Not related to the current change.
p2p/host/basic/basic_host.go
Outdated
| @@ -328,6 +340,21 @@ func NewHost(n network.Network, opts *HostOpts) (*BasicHost, error) { | |||
| } | |||
| } | |||
|
|
|||
| if opts.EnableAutoRelay { | |||
There was a problem hiding this comment.
I think this should be in config.go as part of an fx constructor for autorelay. Start should
be managed in the lifecycle there as well.
There was a problem hiding this comment.
The problem with this structure is that currently there's no way to get the autorelay object once it's constructed for debugging or any other reason.
I'm changing it though, let's make this change if we end up wanting access to this for debugging purposes.
There was a problem hiding this comment.
I don't think I understand. Could you use fx.Populate?
There was a problem hiding this comment.
Thanks for fx.Populate, but that will need #2962
My point was. Once you create a host, host, err := libp2p.New(), There's no way to access the AutoRelay object now for debugging purposes.
p2p/host/basic/basic_host.go
Outdated
| // Make a copy. Consumers can modify the slice elements | ||
| addrs := slices.Clone(h.AddrsFactory(h.AllAddrs())) | ||
| if h.autoNat != nil && h.autorelay != nil && h.autoNat.Status() == network.ReachabilityPrivate { |
There was a problem hiding this comment.
This check should be status != network.ReachabilityPublic instead. The current
code has this bug as well
There was a problem hiding this comment.
Will take this up in a separate PR:
#3172 (comment)
sukunrt
force-pushed
the
sukun/autorelay-event
branch
from
762a43e to
24204d9
Compare
sukunrt
force-pushed
the
sukun/autorelay-event
branch
3 times, most recently
from
8da42b0 to
7acb441
Compare
sukunrt
force-pushed
the
sukun/autorelay-event
branch
2 times, most recently
from
fdafe16 to
58eceee
Compare
sukunrt
force-pushed
the
sukun/autorelay-event
branch
from
58eceee to
d27d90e
Compare
p2p/host/basic/basic_host.go
Outdated
| updateLocalIPv4Backoff backoff.ExpBackoff | ||
| updateLocalIPv6Backoff backoff.ExpBackoff | ||
| filteredInterfaceAddrs []ma.Multiaddr | ||
| allInterfaceAddrs []ma.Multiaddr | ||
|
|
||
| relayAddrs atomic.Value |
There was a problem hiding this comment.
Slight preference to using the atomic.Pointer type since it's a little nicer to use.
| addrs = append(addrs, relayAddrs...) | ||
| } | ||
| } | ||
| addrs = slices.Clone(h.AddrsFactory(addrs)) |
There was a problem hiding this comment.
Let's change .addCertHashes to always return a copy, and remove this.
Otherwise we end up making two copies.
There was a problem hiding this comment.
I'll take that in a separate PR.
| rf.metricsTracer.RelayAddressUpdated() | ||
| // Sort the addresses. We depend on this order for checking diffs to send address update events. | ||
| slices.SortStableFunc(raddrs, func(a, b ma.Multiaddr) int { return bytes.Compare(a.Bytes(), b.Bytes()) }) | ||
| if len(raddrs) > maxRelayAddrs { |
There was a problem hiding this comment.
may as well do this check in the loop above too.
There was a problem hiding this comment.
We need to sort before limiting to the desired value to keep the list stable.
| ready := make(chan struct{}, 1) | ||
| for _, conn := range conns { | ||
| go func(conn network.Conn) { | ||
| select { | ||
| case <-rf.host.IDService().IdentifyWait(conn): | ||
| case <-hi.IDService().IdentifyWait(conn): |
There was a problem hiding this comment.
Ideally we'd do something like the draft conntracker logic to see which connections supports relaying. In practice this is probably fine as a relay server likely relays on all connections.
There was a problem hiding this comment.
Hmm, this is tricky indeed. It needs #2693 and maybe the ability to connect only on specific transports and not on everything in the peerstore.
| e := <-sub.Out() | ||
| evt := e.(event.EvtAutoRelayAddrsUpdated) | ||
| relayIds := []peer.ID{} | ||
| for _, r := range relays[1:] { |
There was a problem hiding this comment.
nit, this can move out of the eventually loop
Part of #2229