Combined discovery & fork-filtering fixes (discv5 ENR refresh, startup races, DoS bounds, EIP-2124/eth2 correctness) - #43
Merged
Merged
Conversation
The handler tracked every distinct node ID it ever saw in a map that was never evicted and had no size limit. getOrCreateNode inserted unconditionally, including one entry per record in every inbound NEIGHBORS packet with no bond gate, so the map grew without bound over normal operation and an unauthenticated peer could accelerate it to memory exhaustion by flooding NEIGHBORS records with fabricated public keys. Add a hard cap (MaxNodes, default 50000) enforced on insert: once full a new node is still returned so the packet is handled but is not retained. Evict stale unbonded nodes in the existing periodic cleanup (NodeTTL, default 5m); bonded nodes are kept until their bond expires. Both are configurable and default when unset, so existing call sites are unchanged.
The generic node published its v4Node and v5Node pointers with no synchronization. SetV4 and SetV5 wrote them lock free while V4, V5, HasV4, HasV5, PeerID, Enode, UpdateENR and CalculateScore read them lock free. These run on different goroutines in normal operation, for example protocol detection updating the pointers while a scoring sweep, a database batch or the web UI reads them, so the access is a data race. The check then deref readers are worse: SetV5(nil) can clear the pointer between the nil check and the call, so PeerID and CalculateScore can dereference nil and crash the daemon. Guard both pointers with the node mutex that already protects addr. Simple accessors take the read lock. SetV4 and SetV5 take the write lock around the pointer store only. The check then deref readers capture the pointer under the read lock and release before the call, so a concurrent clear can no longer be observed mid call.
Response handlers delivered a matched response by sending on the pending request's channel with a plain send. That channel is buffered with size one and is read at most once by the waiter, so a duplicate, replayed or late PONG, NEIGHBORS or ENRRESPONSE found the buffer full or the waiter gone and parked the packet dispatch goroutine forever. An unauthenticated peer could leak goroutines by replaying responses. Route the three sends through a helper that uses a non blocking send, so an extra response is dropped instead of parking the goroutine. The first legitimate response still lands in the empty buffer and is delivered.
handleNeighbors accumulated node records for any sender into a per node slice with no cap, and refreshed the entry timestamp on every packet, so the periodic cleanup never evicted it. An unauthenticated peer could send a steady stream of NEIGHBORS and grow memory without limit, and each packet scheduled its own delivery goroutine. Only accept NEIGHBORS in response to a FINDNODE we actually sent to that node, so unsolicited responses are dropped. Cap the accumulated nodes at one k-bucket. Schedule delivery once, on the first packet. Base cleanup on the entry creation time instead of the last received time, so a stream of packets can no longer keep an entry alive.
…oadInitialNodesFromDB
Bumps the dependencies group with 4 updates in the / directory: [github.com/ethereum/go-ethereum](https://github.com/ethereum/go-ethereum), [github.com/pk910/dynamic-ssz](https://github.com/pk910/dynamic-ssz), [github.com/pressly/goose/v3](https://github.com/pressly/goose) and [github.com/prometheus/client_golang](https://github.com/prometheus/client_golang). Updates `github.com/ethereum/go-ethereum` from 1.17.3 to 1.17.4 - [Release notes](https://github.com/ethereum/go-ethereum/releases) - [Commits](ethereum/go-ethereum@v1.17.3...v1.17.4) Updates `github.com/pk910/dynamic-ssz` from 1.3.1 to 1.3.2 - [Release notes](https://github.com/pk910/dynamic-ssz/releases) - [Changelog](https://github.com/pk910/dynamic-ssz/blob/master/CHANGELOG.md) - [Commits](pk910/dynamic-ssz@v1.3.1...v1.3.2) Updates `github.com/pressly/goose/v3` from 3.27.1 to 3.27.2 - [Release notes](https://github.com/pressly/goose/releases) - [Changelog](https://github.com/pressly/goose/blob/main/CHANGELOG.md) - [Commits](pressly/goose@v3.27.1...v3.27.2) Updates `github.com/prometheus/client_golang` from 1.23.2 to 1.24.0 - [Release notes](https://github.com/prometheus/client_golang/releases) - [Changelog](https://github.com/prometheus/client_golang/blob/v1.24.0/CHANGELOG.md) - [Commits](prometheus/client_golang@v1.23.2...v1.24.0) Updates `golang.org/x/crypto` from 0.50.0 to 0.53.0 - [Commits](golang/crypto@v0.50.0...v0.53.0) Updates `golang.org/x/net` from 0.53.0 to 0.56.0 - [Commits](golang/net@v0.53.0...v0.56.0) --- updated-dependencies: - dependency-name: github.com/ethereum/go-ethereum dependency-version: 1.17.4 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: dependencies - dependency-name: github.com/pk910/dynamic-ssz dependency-version: 1.3.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: dependencies - dependency-name: github.com/pressly/goose/v3 dependency-version: 3.27.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: dependencies - dependency-name: github.com/prometheus/client_golang dependency-version: 1.24.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: dependencies - dependency-name: golang.org/x/crypto dependency-version: 0.53.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: dependencies - dependency-name: golang.org/x/net dependency-version: 0.56.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: dependencies ... Signed-off-by: dependabot[bot] <support@github.com>
- enforce the NEIGHBORS cap before records enter the node map - remove a completed FINDNODE's pending request on every exit path - stamp last-seen at node creation so cleanup honors NodeTTL
- reserve NEIGHBORS room before decoding and tombstone delivered entries, keyed by request hash, so the persistence cap is exact under concurrent dispatch - guard discv5 node record/addr/tcpPort/stats behind a mutex; same for the discv4 node enr/addr/stats and the generic node enr - bulk DB load stops at the active-pool soft cap - Ping/RequestENR clear their pending request on every exit path - handler stats snapshot reads maps under their own mutexes
EL: - ForkFilter keeps its genesis time; GetCurrentForkID no longer gathers forks with a hardcoded 0 cutoff, so the current-era id matches go-ethereum - Filter() ports go-ethereum's EIP-2124 validation ruleset (subset/superset/ stale-Next) evaluated at a static head instead of accepting any known checksum; the unused FilterStrict stub is gone - GetAllForkIDsWithNames uses the stored genesis time and groups names per deduplicated activation, so shared-activation rows stay aligned - admission sites record accept/reject counts and log the remote fork id CL: - GetAllForkDigests/Infos and the previous-fork getters enumerate boundary epochs through GetForkDigestForEpoch, so the admission set and the live digest share one code path and same-epoch phantom digests are gone - BLOB_SCHEDULE is sorted at parse; GetBlobParamsForEpoch's early break relied on an ordering the YAML never guaranteed - rejectedExpired (unreachable: expired digests fall through to historical acceptance) is replaced by an acceptedHistorical accessor - the blocking StartPeriodicUpdate is replaced by Update() on a shared tick ENR: - eth/eth2 are refreshed on a maintenance tick so a long-running bootnode stops advertising a stale fork past a scheduled transition; UpdateENR is a no-op when nothing changed, so the sequence only moves at real transitions - lookup completion counts fork rejections separately from pool rejections
Config.LocalNode was declared but never read, so nothing excluded us from discovery: a peer returning our record in a NODES/NEIGHBORS response made us FINDNODE ourselves every round, and two concurrent self-handshakes collided on the per-nodeID+addr challenge key, producing invalid handshake signature warnings against our own address. - Config.LocalNode is replaced by LocalIDs, which carries every identity's node ID (two when separate EL and CL keys are configured) - lookups seed the seen map with all local IDs, skip them when selecting query targets, and skip them in the discv4 ENR-request fan-out - LoadInitialNodesFromDB applies the self check Add already had, so a persisted record of ourselves can no longer sit in the active pool for the process lifetime - the bootnode-connect paths only persist a node the table actually admitted, and skip an ENR request aimed at our own enode
The bb3655b refactor dropped Service.GetStats(), leaving ~25 overview fields declared but never assigned, so the page rendered zeros for lookups, pings, sessions, pending handshakes and packets next to fully populated node tables. - PingService counters get the mutex they always needed: PingMultiple fans pings out across goroutines, so they raced each other even before a web UI reader was added - a typed Service.GetStats() aggregates lookups over both layers and pings, discv5 handler counters, session cache totals and transport packet metrics over every identity, counting a shared socket once - discv4's protocol handler gains a typed GetStats(); the map form now derives from it, so the web UI no longer type-asserts interface{} values - the fork panel shows real admission numbers: CL from the digest filter, EL from the counters added with the fork-id fix - fields with no real source are removed rather than left lying: BucketsFilled (hardcoded 0 for the flat table, rendered nowhere) and its NumBucketsFilled accessor, and the unreachable Rejected (Expired) row is replaced by the Accepted (Historical) category the filter actually produces
Two defects the fork-refresh tick would have exposed at every transition: - discv4 SetLocalENR rebuilt the whole protocol handler, discarding bonds, known nodes, pending requests and counters, orphaning the old handler's cleanup goroutine, and delivering replies to a handler nobody waits on. The handler now holds its own mutex-guarded record that SetLocalENR replaces in place, and Service.Handler() reads the pointer under the service mutex. - --cl-genesis-time was parsed, logged and then dropped: SetGenesisTime was never called, so every epoch-derived value (current digest, next-fork info, the published eth2 entry) kept using the YAML time. An override that crosses a fork boundary published the wrong digest.
…dmission Both found by the devnet validation run: - lookup rejections lumped consensus nodes in with execution nodes on an incompatible fork. On the devnet 100% of rejected_fork was healthy cross-layer traffic, and on a real network an EL lookup mostly sees CL records, so the counter reads exactly like the malfunction this batch fixed. Records with no eth/eth2 entry are now AdmissionRejectedLayer, logged as rejected_layer and stored as not_el/not_cl, and they no longer pollute the fork filter's accept/reject counters. - the overview picked the CL filter or the EL filter with an else-if, so on any dual-layer bootnode the EL admission counters were unreachable. They now have their own card and AJAX wiring alongside the CL fork panel.
OldDigests was declared and had a template card gated on it, but was never assigned, so the card never rendered. The fork test showed the data is real: AcceptedOld was in the thousands across three transitions while the panel stayed blank. GetOldForkDigests already returns the exact shape the field needs, sorted by remaining grace time.
All found by the live multi-fork devnet test (Electra -> Fulu -> BPO1). - eth2.next_fork_epoch was written and read big-endian. The entry is an SSZ ENRForkID, so the epoch is a little-endian uint64: a conformant peer read epoch 1 as 2^56. Both sides were consistently wrong, so bootnodoor's own filter and tests could not see it, and FAR_FUTURE_EPOCH is byte-palindromic so it stayed invisible until a fork was actually scheduled. Regression test added for exactly that reason. - nextForkInfo returned the version of unscheduled (FAR_FUTURE_EPOCH) forks, so after the last fork bootnodoor advertised an unscheduled fork's version where the spec wants the current one. - addDiscv4Node returned early for a node already in the table, so Add() never got the chance to install its newer ENR. Post-fork records were left stale and served to every FINDNODE querier - the failure a bootnode exists to prevent. The insertion log still only fires for genuinely new nodes. - EL admission counters no longer count records without an eth entry, so the rejection count stops tracking cross-layer traffic (9544615 fixed the log line but not the stats surface). - a CL fork activation now logs instead of carrying a 'Log would go here' placeholder.
Three findings from review of the fork-test fixes: - the post-fork record refresh was still unreachable. onNodeSeenV4 returns as soon as a node is in the table, so checkAndAddNodeV4 never ran for known nodes; PONG's newer-ENR detection only updates the handler's own node and discards the result. A known node whose record advanced now goes through admission again so Add() installs it. - the layer gates used Record.Eth(), which is also false for a malformed or empty eth entry, so a broken execution record was filed as cross-layer instead of a fork rejection. They now test key presence with Has(). - the EL admission AJAX updates sat inside the CL filter's conditional, so on an EL-only deployment the card never refreshed and could not appear without a page reload.
A peer we cannot bond with has no refresh channel of its own: PING/PONG, the handshake and the v4 ENR request all need us to reach it. The one remaining path is another discv5 peer relaying its signed record, and the lookup dropped those - every table node was pre-seeded into `seen`, so a known ID could never reach OnNodeFound. Nodes are not evicted below capacity, so the first-contact record stuck for the process lifetime and was persisted to the DB. Replace `seen`/`allDiscovered` with per-lookup bookkeeping that keeps the best record per node plus first-observation order. Refreshes are gated to v5-sourced records: a v4 wrapper's ENR came from dialing the peer, so it is not a relay, and re-admitting one re-runs the blocking v5 support probe. Eligibility is checked before the dedupe branch so a later duplicate cannot bypass that gate, and admission re-checks the live table entry in case a direct refresh overtook us mid-lookup. Also move queryHistory, lookupsV5 and lookupsV4 writes to ls.mu; they were written under the per-lookup mutex but read under ls.mu.
The handshake record is optional in discv5 v5.1 - a peer omits it when our WHOAREYOU advertised an ENRSeq it has nothing newer than - but it was the only source of the sender's key, so those handshakes were rejected. We honour the same optionality outbound, so we emitted handshakes we would refuse. Reachable when a session with a node attached lands between the session lookup and sendWHOAREYOU. Carry the record our ENRSeq came from on the pending challenge and fall back to it, resolving the whole node rather than a bare key: the session and OnHandshakeComplete both hang off it, and a node-less session can never refresh its ENR. Also require the key to derive the claimed source node ID. The session is keyed by that ID while the signature only proves possession of the key, so a peer could authenticate as itself and be filed under another identity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Combined discovery / fork-filtering branch consolidating the in-flight discovery hardening PRs plus a series of follow-up correctness fixes on top of them. Authored primarily by @MysticRyuujin (Chase Wright), with the four DoS-hardening changes by @damilolaedwards and one dependency bump from dependabot.
Please merge with a merge commit — do NOT squash. The per-change history (and authorship) is intentionally preserved so each fix stays independently reviewable and bisectable.
Base: current
master(0 commits behind).go build ./...andgo test ./...pass. Validated against go-ethereum's real discv4/discv5 stack (see Testing).Included open PRs
These already-open PRs are contained in this branch (as their original commits + merges):
v4Node/v5Nodepointer race that could nil-crash the daemon.OnHandshakeComplete/OnNodeSeenfire after a restart), lockLoadInitialNodesFromDB, re-admit demoted-but-alive nodes on traffic.Additional fixes on top
Fork / CL correctness
fix(fork): spec-correct eth2 encoding and post-fork record refresh— writenext_fork_epoch(and blob-param inputs) as SSZ little-endian (was big-endian; only latent because the scheduled epoch wasFAR_FUTURE), and driveForkDigestFilter.Update()+UpdateENRfrom a 1-minute refresh tick so the published digest no longer goes stale after a fork/BPO boundary.fix(fork): correct fork-id/digest admission and publication— EL admission now runs full EIP-2124 validation instead of hash-set membership. See Reviewer notes (behavior change).fix: update discv4 ENR in place and honor --cl-genesis-time—SetLocalENRno longer rebuilds the whole discv4 handler (which discarded bonds/known-nodes/pending-requests/counters and orphaned the cleanup goroutine); it now swaps a mutex-guarded record in place.--cl-genesis-timeis now actually applied (SetGenesisTimewas never called).fix: distinguish cross-layer from wrong-fork rejections, surface EL admission— bad-node reasons/counters distinguish "not this layer" from "wrong fork"; EL admission stats recorded only when anethentry is present.fix: refresh known v4 records, tighten layer gating, unnest EL stats.Discovery / lookup / session
fix(discv5): accept a record-less handshake, bind key to claimed ID— the handshake record is optional in discv5 v5.1 (a peer omits it when our WHOAREYOU advertised an ENRSeq it has nothing newer than); fall back to the record our ENRSeq came from, and require the handshake key to derive the claimed source node ID (the session is keyed by that ID while the signature only proves key possession — closes an identity-confusion hole).fix(lookup): admit newer relayed records for known nodes— install a newer, signature-verified, ID-bound record for a known node even when learned via another node's NODES response.fix(discovery): never dial our own identities— skip self by node ID.fix: race and cap fixes from the develop-vs-master review— add missing mutexes on the discv4/discv5 node types, make the NEIGHBORS persistence cap exact (reserve-before-decode + tombstone), stop bulk DB load overfilling the active pool.fix(discv4): address review feedback on #34 and #38.Web UI / stats
fix(webui): report real discovery stats instead of hardcoded zeros,fix(webui): populate the grace-period digest card— plus additive stats plumbing (no effect on discovery/serve behavior).Testing
go build ./...,go vet ./...,go test ./...— all pass.discover.UDPv5/UDPv4: discv5 PING / RequestENR (FINDNODE[0]) / lookup all succeed; the bootnode'sethENR entry decodes correctly via geth's EIP-2124 loader.Reviewer notes (please read before merging)
Two items are worth explicit attention:
Behavior change — EL admission tightened.
fix(fork): correct fork-id/digest admissionreplaces the previous permissive "hash matches any canonical fork-id" check with full go-ethereum EIP-2124 validation (static head). This correctly rejects genuinely-stale EL peers, but it will also reject a peer publishing a malformed/zeroedNextthat the old code accepted on hash alone — and, via the (unchanged) dual-stack serve path, such a requester is then served CL-only instead of both layers. Intended as a correctness fix, but it is a real narrowing of who is admitted.Bound discv4 handler node map to prevent unbounded growth #34 still returns a non-retained node when the map is full.
getOrCreateNodereturning a throwaway node under saturation means a genuinely-new peer's inbound PING bonds a discarded object, so under a flood of distinct signature-recovered IDs a new peer can fail to bond. Recommend not applying the cap to the dispatchfromNode(or evict-one-stale-on-insert) before/at merge.Not included / planned follow-ups
For reviewer awareness, these known issues are not addressed here and are tracked as separate follow-ups:
enr.Record.EncodeRLPdoesn't satisfyrlp.Encoder, so the record encodes asc0; go-ethereum rejects it with "record contains less than two list elements"). Confirmed still failing on this branch. Separate fix incoming.DecodeRLPBytesis recommended.