autonatv2: implement autonatv2 spec #2469
Conversation
sukunrt
force-pushed
the
sukun/autonat-v2-2
branch
from
5477354
to
fe5e13e
Compare
sukunrt
force-pushed
the
sukun/autonat-v2-2
branch
from
fe5e13e
to
7d524a9
Compare
p2p/protocol/autonatv2/autonat.go
Outdated
| return nil, err | ||
| } | ||
| } | ||
| sub, err := h.EventBus().Subscribe(new(event.EvtLocalReachabilityChanged)) |
There was a problem hiding this comment.
EvtLocalReachabilityChanged is emitted by AutoNAT v1. Why are we subscribing to that event here?
There was a problem hiding this comment.
I assume this is for the transition period where we have v1 and v2 running in parallel? If so, can you add comments explaining the transition plan?
There was a problem hiding this comment.
I copied this logic from AutoNATv1. Do we need to disable the server in case the node is private?
We can provide this service over a relay connection too since private nodes can dial out and check reachability. Even with amplification attack prevention this only takes 100kb.
There was a problem hiding this comment.
added a comment explaining the transition period.
p2p/protocol/autonatv2/autonat.go
Outdated
| func (an *AutoNAT) validPeer() peer.ID { | ||
| peers := an.host.Peerstore().Peers() | ||
| idx := 0 | ||
| for _, p := range an.host.Peerstore().Peers() { | ||
| if proto, err := an.host.Peerstore().SupportsProtocols(p, DialProtocol); len(proto) == 0 || err != nil { | ||
| continue | ||
| } | ||
| peers[idx] = p | ||
| idx++ | ||
| } | ||
| if idx == 0 { | ||
| return "" | ||
| } | ||
| peers = peers[:idx] | ||
| rand.Shuffle(len(peers), func(i, j int) { peers[i], peers[j] = peers[j], peers[i] }) | ||
| return peers[0] | ||
| } |
There was a problem hiding this comment.
Should we instead just keep our own map of peers that support DialProtocol? We would need to subscribe to the peer connected / disconnected event (or the identify completed event?), and check the supported protocols there.
This would allow us to not loop over all peers every single time we need one valid peer.
There was a problem hiding this comment.
I like this suggestion. I have incorporated it.
Should we process the connected peers on initialisation or just let the map build up and ignore the initially missed peers? This is only an issue if users separately attach AutoNATv2 to their hosts by calling the constructor explicitly.
p2p/protocol/autonatv2/server.go
Outdated
| s.SetDeadline(as.now().Add(1 * time.Second)) | ||
| b := make([]byte, 1) | ||
| s.Read(b) |
There was a problem hiding this comment.
Updated comment
Is there any clean way to close a stream with an ACK?
The problem here is, if we just do
stream.Write(msg)
stream.Close()
...
host.Network().ClosePeer(pid)
The last line will Reset the stream and discard the message.
We do want to remove the peer from the dialer host once the request is complete.
Yes, that part will go in the address pipeline. |
sukunrt
force-pushed
the
sukun/autonat-v2-2
branch
2 times, most recently
from
e09ef04
to
c55e2af
Compare
sukunrt
force-pushed
the
sukun/autonat-v2-2
branch
from
5bddfb0
to
acb1c88
Compare
sukunrt
force-pushed
the
sukun/autonat-v2-2
branch
5 times, most recently
from
6149b00
to
83babed
Compare
sukunrt
force-pushed
the
sukun/autonat-v2-2
branch
from
83babed
to
e080d99
Compare
sukunrt
force-pushed
the
sukun/autonat-v2-2
branch
from
e080d99
to
af0ac51
Compare
closes: #2422
Will add metrics for this in a separate PR.