XORBytes was added to package crypto/subtle in Go 1.20, and it's faster
than our bitutil.XORBytes. There is only one use of this function
across go-ethereum so we can simply deprecate the custom implementation.
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
The random-port retry loop in addAnyPortMapping shadowed the err
variable, causing the function to return (0, nil) when all attempts
failed. This change removes the shadowing and preserves the last error
across both the fixed-port and random-port retries, ensuring failures
are reported to callers correctly.
This fixes a regression introduced in #32518. In that PR, we removed the
slowdown logic that would throttle lookups when the table runs empty.
Said logic was originally added in #20389.
Usually it's fine, but there exist pathological cases, such as hive
tests, where the node can only discover one other node, so it can only
ever query that node and won't get any results. In cases like these, we
need to throttle the creation of lookups to avoid crazy CPU usage.
Description:
We found a occasionally node hang issue on BSC, I think Geth may
also have the issue, so pick the fix patch here.
The fix on BSC repo: https://github.com/bnb-chain/bsc/pull/3347
When the hang occurs, there are two routines stuck.
- routine 1: AsyncFilter(...)
On node start, it will run part of the DiscoveryV4 protocol, which could
take considerable time, here is its hang callstack:
```
goroutine 9711 [chan receive]: // this routine was stuck on read channel: `<-f.slots`
github.com/ethereum/go-ethereum/p2p/enode.AsyncFilter.func1()
github.com/ethereum/go-ethereum/p2p/enode/iter.go:206 +0x125
created by github.com/ethereum/go-ethereum/p2p/enode.AsyncFilter in goroutine 1
github.com/ethereum/go-ethereum/p2p/enode/iter.go:192 +0x205
```
- Routine 2: Node Stop
It is the main routine to shutdown the process, but it got stuck when it
tries to shutdown the discovery components, as it tries to drain the
channel of `<-f.slots`, but the extra 1 slot will never have chance to
be resumed.
```
goroutine 11796 [chan receive]:
github.com/ethereum/go-ethereum/p2p/enode.(*asyncFilterIter).Close.func1()
github.com/ethereum/go-ethereum/p2p/enode/iter.go:248 +0x5c
sync.(*Once).doSlow(0xc032a97cb8?, 0xc032a97d18?)
sync/once.go:78 +0xab
sync.(*Once).Do(...)
sync/once.go:69
github.com/ethereum/go-ethereum/p2p/enode.(*asyncFilterIter).Close(0xc092ff8d00?)
github.com/ethereum/go-ethereum/p2p/enode/iter.go:244 +0x36
github.com/ethereum/go-ethereum/p2p/enode.(*bufferIter).Close.func1()
github.com/ethereum/go-ethereum/p2p/enode/iter.go:299 +0x24
sync.(*Once).doSlow(0x11a175f?, 0x2bfe63e?)
sync/once.go:78 +0xab
sync.(*Once).Do(...)
sync/once.go:69
github.com/ethereum/go-ethereum/p2p/enode.(*bufferIter).Close(0x30?)
github.com/ethereum/go-ethereum/p2p/enode/iter.go:298 +0x36
github.com/ethereum/go-ethereum/p2p/enode.(*FairMix).Close(0xc0004bfea0)
github.com/ethereum/go-ethereum/p2p/enode/iter.go:379 +0xb7
github.com/ethereum/go-ethereum/eth.(*Ethereum).Stop(0xc000997b00)
github.com/ethereum/go-ethereum/eth/backend.go:960 +0x4a
github.com/ethereum/go-ethereum/node.(*Node).stopServices(0xc0001362a0, {0xc012e16330, 0x1, 0xc000111410?})
github.com/ethereum/go-ethereum/node/node.go:333 +0xb3
github.com/ethereum/go-ethereum/node.(*Node).Close(0xc0001362a0)
github.com/ethereum/go-ethereum/node/node.go:263 +0x167
created by github.com/ethereum/go-ethereum/cmd/utils.StartNode.func1.1 in goroutine 9729
github.com/ethereum/go-ethereum/cmd/utils/cmd.go:101 +0x78
```
The rootcause of the hang is caused by the extra 1 slot, which was
designed to make sure the routines in `AsyncFilter(...)` can be
finished. This PR fixes it by making sure the extra 1 shot can always be
resumed when node shutdown.
The lookup would add self into the replyBuffer if returned by another node.
Avoid doing that by marking self as seen.
With the changed initialization behavior of lookup, the lookupIterator needs to yield the
buffer right after creation. This fixes the smallNetConvergence test, where all results
are straight out of the local table.
Refresh is doing some lookups and thus it could block for some time. We
do not want the initializer of an iterator to block. If there is
something blocking, it should happen when calling Next.
Here, next will start a lookup, which will wait if needed (no nodes),
making sure the iterator's Next is not creating a busy loop.
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Supersedes #32470.
### What
- snap: shorten stall watchdog in `eth/protocols/snap/sync_test.go` from
1m to 10s.
- discover/v5: consolidate FINDNODE negative tests into a single
table-driven test:
- `TestUDPv5_findnodeCall_InvalidNodes` covers:
- invalid IP (unspecified `0.0.0.0`) → ignored
- low UDP port (`<=1024`) → ignored
### Why
- Addresses TODOs:
- “Make tests smaller” (reduce long 1m timeout).
- “check invalid IPs”; also cover low port per `verifyResponseNode`
rules (UDP must be >1024).
### How it’s validated
- Test-only changes; no production code touched.
- Local runs:
- `go test ./p2p/discover -count=1 -timeout=300s` → ok
- `go test ./eth/protocols/snap -count=1 -timeout=600s` → ok
- Lint:
- `go run build/ci.go lint` → 0 issues on modified files.
### Notes
- The test harness uses `enode.ValidSchemesForTesting` (which includes
the “null” scheme), so records signed with `enode.SignNull` are
signature-valid; failures here are due to IP/port validation in
`verifyResponseNode` and `netutil.CheckRelayAddr`.
- Tests are written as a single table-driven function for clarity; no
helpers or environment switching.
---------
Co-authored-by: lightclient <lightclient@protonmail.com>
Replace manual byte-by-byte XOR implementation with the optimized
bitutil.XORBytes function. This improves performance by using word-sized
operations on supported architectures while maintaining the same
functionality. The optimized version processes data in bulk rather than
one byte at a time
---------
Co-authored-by: Felix Lange <fjl@twurst.com>
This PR improves the speed of Disc/v4 and Disc/v5 based discovery by
adding a prefetch buffer to discovery sources, eliminating slowdowns
due to timeouts and rate mismatch between the two processes.
Since we now want to filter the discv4 nodes iterator, it is being removed
from the default discovery mix in p2p.Server. To keep backwards-compatibility,
the default unfiltered discovery iterator will be utilized by the server when
no protocol-specific discovery is configured.
---------
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
This adds support for naming the source iterators of FairMix, like so:
mix.AddSource(enode.WithSourceName("mySource", iter))
The source that produced the latest node is returned by the new NodeSource method.
Our metrics related to dial errors were off. The original error was not
wrapped, so the caller function had no chance of picking it up.
Therefore the most common error, which is "TooManyPeers", was not
correctly counted.
The metrics were originally introduced in
https://github.com/ethereum/go-ethereum/pull/27621
I was thinking of various possible solutions.
- the one proposed here wraps both the new error and the origial error.
It is not a pattern we use in other parts of the code, but works. This
is maybe the smallest possible change.
- as an alternate, I could write a proper `errProtoHandshakeError` with
it's own wrapped error
- finally, I'm not even sure we need `errProtoHandshakeError`, maybe we
could just pass up the original error.
---------
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
As of now, Geth disconnects peers only on protocol error or timeout,
meaning once connection slots are filled, the peerset is largely fixed.
As mentioned in https://github.com/ethereum/go-ethereum/issues/31321,
Geth should occasionally disconnect peers to ensure some churn.
What/when to disconnect could depend on:
- the state of geth (e.g. sync or not)
- current number of peers
- peer level metrics
This PR adds a very slow churn using a random drop.
---------
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
Our previous success metrics gave success even if a peer disconnected
right after connection. These metrics only count peers that stayed
connected for at least 1 min. The 1 min limit is an arbitrary choice. We do
not use this for decision logic, only statistics.
Make UPnP more robust
- Once a random port was mapped, we try to stick to it even if a UPnP
refresh fails. Previously we were immediately moving back to try the
default port, leading to frequent ENR changes.
- We were deleting port mappings before refresh as a possible
workaround. This created issues in some UPnP servers. The UPnP (and PMP)
specification is explicit about the refresh requirements, and delete is
clearly not needed (see
https://github.com/ethereum/go-ethereum/pull/30265#issuecomment-2766987859).
From now on we only delete when closing.
- We were trying to add port mappings only once, and then moved on to
random ports. Now we insist a bit more, so that a simple failed request
won't lead to ENR changes.
Fixes https://github.com/ethereum/go-ethereum/issues/31418
---------
Signed-off-by: Csaba Kiraly <csaba.kiraly@gmail.com>
Co-authored-by: Felix Lange <fjl@twurst.com>
