p2p/discover: add support for EIP-868 (v4 ENR extension) #19540
Conversation
|
I have verified this against aleth, which has support for EIP-868 on the master branch. One change I added during the interop test is that the findnode failure counter is now used in the bond check and reset to zero when findnode works. I think this improves resilience against misbehaving clients which don't remember the endpoint proof across restarts. Not sure if there is any downside. |
| // Try asking directly. This works if the node is still responding on the endpoint we have. | ||
| if rn, err := t.requestENR(n); err == nil { | ||
| return rn | ||
| } |
There was a problem hiding this comment.
This code modifies the behavior slightly, whereby previously we always used the local database if we've seen the node before, but now we always reach out to the network to do an ENR request. Are we sure there's no issue in this new network request based lookup?
There was a problem hiding this comment.
There is no issue with the request because it will deliver the most up-to-date version of the record if it works. If the request fails we just do what we did before.
There was a problem hiding this comment.
I was more concerned about the introduced network RTT.
p2p/discover/v4_udp.go
Outdated
| // This ensures there is a valid endpoint proof on the remote end. | ||
| func (t *UDPv4) ensureBond(toid enode.ID, toaddr *net.UDPAddr) { | ||
| tooOld := time.Since(t.db.LastPingReceived(toid, toaddr.IP)) > bondExpiration | ||
| if tooOld || t.db.FindFails(toid, toaddr.IP) > 5 { |
There was a problem hiding this comment.
Please separate out this 5 into it's own constant. Also, previously we didn't seem care about the number of failures. Any reason for asserting a 5 here? What's the rational? What's the expected behavioral difference?
There was a problem hiding this comment.
I just reused the existing maxFindnodeFailures constant now which was already five. There are two important differences in the new handling of findnode failures:
- we didn't reset the counter value but reduced it by one for every successful reply. If a remote node was unreachable for a while, the counter would go up to a very high value and it would take forever to come down below five again.
- the counter was not used for rebonding, i.e. if a node didn't respond to findnode we never even tried pinging it again. Doing ping/pong again in this case will make communication work if the node is actually live but just forgot about our endpoint proof.
| rm := t.sendPing(toid, toaddr, nil) | ||
| <-rm.errc | ||
| // Wait for them to ping back and process our pong. | ||
| time.Sleep(respTimeout) |
There was a problem hiding this comment.
Blind sleep? Seems a bit odd? Shouldn't we perhaps wake up as soon as the rely arrives and not wait until we reach the timeout?
There was a problem hiding this comment.
We had what you describe before f0c6f92#diff-67724ae2da807c2b2c15ec4cd9cd4c6eR341, but it was unsafe in case of packet reordering. The expectation here is that we send a ping to the remote node to make it ping back. When it does that, we need to reply to the ping first, then wait a bit, then send findnode.
This allows removing the packet type parameter from all methods dealing with packet structs because the type byte can be retrieved with the method instead.
This change implements EIP-868. The UDPv4 transport announces support for the extension in ping/pong and handles enrRequest messages. There are two uses of the extension: If a remote node announces support for EIP-868 in their pong, node revalidation pulls the node's record. The Resolve method requests the record unconditionally.
fjl
force-pushed
the
p2p-discover-enr-extension
branch
from
c9bfb00 to
4bdf1a4
Compare
| if respN.ID() != n.ID() { | ||
| return nil, fmt.Errorf("invalid ID in response record") | ||
| } | ||
| if err := netutil.CheckRelayIP(addr.IP, respN.IP()); err != nil { |
There was a problem hiding this comment.
As an attacker at this point, can I
a) supply many (millions?) ENR records all with the same IP (me) to populate the routing table with spam? (by adding junk to neighbours responses)
b) Then can I switch the IP addresses to focus traffic on some node?
Both would be attacks.
There was a problem hiding this comment.
No, these kinds of attacks are not possible. The response can only contain a single record and the regular IP limits on the table still apply even for record updates.
This change implements EIP-868. The UDPv4 transport announces support
for the extension in ping/pong and handles enrRequest messages.
There are two uses of the extension: If a remote node announces support
for EIP-868 in their pong, node revalidation pulls the node's record.
The Resolve method requests the record unconditionally.