autorelay: send addresses on eventbus; dont wrap address factory #3071
Conversation
sukunrt
force-pushed
the
sukun/autorelay-event
branch
from
b920ac5 to
2b114a2
Compare
sukunrt
force-pushed
the
sukun/autorelay-event
branch
from
2b114a2 to
762a43e
Compare
| } | ||
|
|
||
| if err := cfg.addAutoNAT(bh); err != nil { |
There was a problem hiding this comment.
This doesn't need a started host?
There was a problem hiding this comment.
Nope. It only needs the host to open a new stream and event bus.
| @@ -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.
| @@ -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.
| // 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)
| @@ -109,6 +110,8 @@ type BasicHost struct { | |||
| autoNat autonat.AutoNAT | |||
|
|
|||
| autonatv2 *autonatv2.AutoNAT | |||
| addrSub event.Subscription | |||
| autorelay *autorelay.AutoRelay | |||
There was a problem hiding this comment.
We don't need autorelay here if we can get the relay addrs via the eventbus
| @@ -808,3 +815,34 @@ func (rf *relayFinder) resetMetrics() { | |||
| rf.metricsTracer.RelayAddressCount(0) | |||
| rf.metricsTracer.ScheduledWorkUpdated(&scheduledWorkTimes{}) | |||
| } | |||
|
|
|||
| func haveAddrsDiff(a, b []ma.Multiaddr) bool { | |||
There was a problem hiding this comment.
We should change this to areSortedAddrsDifferent and uphold the invariant that
we maintain sorted addresses. That way this becomes O(n) instead of O(n^2)
| return rf.cachedAddrs | ||
| } | ||
|
|
||
| // This function computes the relay addrs when our status is private. |
There was a problem hiding this comment.
We run the relayfinder on unknown status as well. We should just say not public.
| peerChan <- peer.AddrInfo{ID: r1.ID(), Addrs: r1.Addrs()} | ||
| cl.AdvanceBy(time.Second) | ||
|
|
||
| require.Eventually(t, func() bool { |
There was a problem hiding this comment.
Prefer require.EventuallyWithT
There was a problem hiding this comment.
Is it for the fact that it collects errors and returns the last one?
Part of #2229