Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2992 +/- ##
==========================================
+ Coverage 58.08% 58.13% +0.04%
==========================================
Files 2109 2110 +1
Lines 173234 173473 +239
==========================================
+ Hits 100626 100844 +218
- Misses 63664 63679 +15
- Partials 8944 8950 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| // NOTE: amplification factor! | ||
| // small request results in up to maxMsgSize response | ||
| maxMsgSize = maxAddressSize * maxGetSelection | ||
| maxMsgSize = 1000 + maxAddressSize*p2p.MaxPexAddrs |
There was a problem hiding this comment.
How big is the average msg now? Where does the constant 1000 come from?
| Encode: func(m *handshakeMsg) *pb.Handshake { | ||
| var selfAddr *string | ||
| if addr, ok := m.SelfAddr.Get(); ok { | ||
| selfAddr = utils.Alloc(addr.String()) |
There was a problem hiding this comment.
When would it happen that you can't get selfAddr here?
There was a problem hiding this comment.
node doesn't have to configure an external (public) address in case it doesn't have one.
| if p.SelfAddr != nil { | ||
| addr, err := ParseNodeAddress(*p.SelfAddr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("SelfAddr: %w", err) |
There was a problem hiding this comment.
Would this ever happen during normal operations? DNS failures?
There was a problem hiding this comment.
Adversary node can send broken data. This is just a proto converter though, it doesn't assume the proto to be valid in any sense, other than specified by the proto message definiton
| for i, addrString := range p.PexAddrs { | ||
| addr, err := ParseNodeAddress(addrString) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("PexAddrs[%v]: %w", i, err) |
There was a problem hiding this comment.
If this may happen on a valid address, should we just ignore that one address and keep the others?
There was a problem hiding this comment.
wdym? Valid address is parseable. This is not a dynamic property.
| if err != nil { | ||
| return nil, fmt.Errorf("NodeAuthKey: %w", err) | ||
| } | ||
| nodeAuthSig, err := ed25519.SignatureFromBytes(p.NodeAuthSig) |
There was a problem hiding this comment.
Can anyone with zero stake send us address updates?
There was a problem hiding this comment.
yes, node keys do not have stake assigned. They are not validator keys
| func (r *Router) Advertise(maxAddrs int) []NodeAddress { | ||
| return r.peerManager.Advertise(maxAddrs) | ||
| addrs := r.peerManager.Advertise() | ||
| return addrs[:min(len(addrs), maxAddrs)] |
There was a problem hiding this comment.
nit: would we ever want randomly pick instead of always the front?
| // case listener does not have capacity for new connections. | ||
| // Dialer also could potentially send pex data, but there is no benefit from doing so: | ||
| // - if listener is full, then it won't use the new data. Listener also will not broadcast unverified data to anyone. | ||
| // - if it is not full, then the connection will be established and pex data will be sent the regular way using PEX protocol. |
There was a problem hiding this comment.
I'm confused, do you mean, if it is not full, Listener may broadcast unverified data to others?
There was a problem hiding this comment.
pex data is exchanged periodically both ways over every connection. But if the connection dies immediately after handshake, then there is no exchange obviously. Connection may die immediately after handshake if listener rejects it (maxconnected capacity reached), in which case it won't try to establish new connections any time soon anyway, so it doesn't need new peer addresses. Noone is sending unverified data to anyone.
3 participants
This PR allows nodes to learn more node addresses, even if the peer they dial is out of capacity for new connections. This works by making listener node send pex batch as part of the handshake (it might discard the connection just after handshake, in case it decides it does not have capacity for this connection).
This new "pex in handshake" is enabled only if reactor pex is enabled in the node - disabling pex reactor prevents a node from learning new addresses and is currently mainly used to prevent node from connecting to random peers (which is misleading indirect use of pex flag), and this pr maintains this semantics to avoid distruptions. I think we should change this semantics and require people to just set MaxConnected to 0 instead (which is a direct way to say: connect only to persistent peers).
Additionally a SelfAddress is added to the handshake message: nodes advertise addresses of nodes they are connected to. Until now they could only advertise the addresses of outbound connections (i.e. verified addresses), but with this PR also SelfAddress of inbound connections is included (each node declares just their own up to date address, so it is fine to gossip it).
Note that SelfAddress could have been also be extracted from the pex response (it is always included), but I wanted to make it more explicit that it is special.