p2p/sentry: share one p2p.Server across all eth protocols

[lystopad]

Summary

Erigon now opens a single TCP listener on --port (30303 by default) carrying every configured eth protocol version, instead of one listener per protocol on 30303/30304/30305.

This fixes a discovery-DHT race that left inbound peers stuck at a fraction of --maxpeers for multi-protocol deployments.

The bug

Each entry in ProtocolVersion (today: [ETH69, ETH70, ETH71] on main) used to back its own p2p.Server bound to its own port from --p2p.allowed-ports. All those Servers share the same node key, so each one signed an ENR for the same Node ID but advertising its own port. The discv4 DHT keeps one ENR per Node ID — the one with the highest seq — so the Servers raced at startup and only one of the ENRs survived in the network's view.

Concrete repro on a live Hoodi node with maxpeers: 80 and ProtocolVersion=[ETH69, ETH68]:

• ENR seq numbers from erigon_nodeInfo are within a few milliseconds of each other.
• Whichever protocol won the race got 50+ inbound; the other got 0-4.
• Total peers plateaued around 30 instead of 80; outbound was identical between the two nodes I compared (capped at MaxPeers/DialRatio), so the entire delta was on the inbound side.

With three protocols (ETH69/70/71 on main) the race is three-way and a node can spend the whole session advertising whichever port is least useful.

The fix

node/components/sentry.Provider now:

1. Creates one sentry.GrpcServer per protocol (so the existing MultiClient / protocol-routing keeps working).
2. Collects the union of every GrpcServer's Protocols (dedup'd by name+version so wit isn't registered N times).
3. Builds one p2p.Server with that union and starts it on a single port.
4. Injects that shared Server back into every GrpcServer via a new SetP2PServer(srv) hook.

Result: one Node ID, one ENR, one TCP listener, one UDP discovery — multiple protocols multiplexed per peer connection (which is how p2p.Server was designed to work in the first place, and how geth has always done it).

Side fixes that fall out of the share

• GrpcServer.Close() leaves an externally-injected Server alone (external == true skips Stop) so one sentry shutting down doesn't tear the listener out from under the others; Provider.Close owns stopping the shared server.
• A shared sentry.PeerStore is wired into every GrpcServer so wit/0 (deduped to a single sentry) and the negotiated eth/* (on a different sentry per peer) see the same PeerInfo and share its eth-ready signal. The field uses atomic.Pointer[PeerStore] so the swap is race-free.
• pi.protocol / pi.witProtocol reads go through locked accessors (EthProtocol(), WitProtocol()); the writers (SetEthProtocol, SetWitProtocol) already hold pi.lock. Verified with go test -race.
• pi.rw is split into per-subprotocol pi.ethRw / pi.witRw so writePeer can route by sentryproto.MessageId (globally unique) — necessary because eth and wit reuse low msgcodes (e.g. 0x01 is both eth.NewBlockHashesMsg and wit.NewWitnessHashesMsg).
• Per-sentry Peers() / SimplePeerCount() filter by the GrpcServer's own eth version (cached in ss.ethVersion), so the shared PeerStore doesn't N-fold-duplicate admin_peers aggregation.
• node/eth.NodesInfo deduplicates by Enode — every sentry now returns identical NodeInfo, so the dedup is what keeps admin_nodeInfo from listing the same node N times.
• NodeDatabase moves from per-protocol subdirs (nodes/eth68, nodes/eth69, …) to a single nodes/eth. Existing per-protocol dirs become inert on upgrade — peer discovery rebuilds from bootnodes within a few minutes.

User-visible changes

• Port surface shrinks: only --port is opened now, not --port, --port+1, --port+2. Firewall / monitoring / Kubernetes Service rules that explicitly opened 30304 and 30305 can drop those entries.
• --p2p.allowed-ports flag removed. Drop it from CLI args / config files; passing it now errors. --port is the only knob.
• --maxpeers caps total connections honestly. Before this fix, with N protocols enabled, each per-protocol Server enforced maxpeers independently, so the actual ceiling was ~N×maxpeers. The new ceiling matches what the flag's docs say.
• Default --maxpeers bumped 32 → 64 to compensate for the now-honest cap. (With 3 eth versions and the old multiplication, the effective ceiling was ~96; geth defaults to 50; 64 lands above geth with a small headroom.)
• Standalone sentry binary (cmd/sentry) and --sentry.api.addr (remote sentry over gRPC) are functionally unchanged — neither had the bug. The cmd/sentry flag surface shrinks alongside the main binary: it no longer accepts --p2p.allowed-ports.
• First run after upgrade loses the warm peer cache in nodes/eth{68,69,…}/; nothing on disk is deleted, the dirs are just no longer read. Discovery refills from bootnodes within a few minutes.

Why not the smaller fixes I considered

• Force seq ordering so the highest protocol always wins the DHT race — still publishes N ENRs, still wastes ports, still N-fold counts maxpeers.
• Disable discovery on all-but-one Server — kills inbound to the non-primary protocols entirely (fine when eth/68 is being deprecated, not fine for ETH69/70/71 living concurrently).

The shared-Server route is the structurally clean one and matches geth's model.

Verification

• make erigon integration builds clean.
• make lint clean (×3 — non-deterministic per CLAUDE.md).
• go test ./p2p/sentry/... ./node/components/sentry/... ./node/eth/... all green.
• go test -race ./p2p/sentry/ ./node/components/sentry/ clean.
• Manual repro of the original bug confirmed: ENR seqs from both sentries decoded; in_30303=0, in_30304>0 consistently after a restart that lost the eth/69 race. With this patch applied the race goes away because there's only one ENR.

Test plan

• Sync from snapshots on a public testnet (Hoodi / Sepolia) and confirm --maxpeers is reached.
• Confirm only --port shows up in netstat -ltnp | grep erigon (not --port+1/+2).
• Confirm admin_peers returns each peer once (no len(ProtocolVersion) duplication).
• Confirm admin_nodeInfo shows one enode (one port).
• Confirm standalone sentry binary still starts: ./build/bin/sentry --datadir=… --chain=….
• Confirm --sentry.api.addr=… mode still connects to an external sentry process.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR changes Erigon’s local sentry wiring to run one shared p2p.Server (single ENR / Node ID / TCP listener) across all configured ETH protocol versions, instead of one p2p.Server per protocol/port. This addresses the discovery ENR “last writer wins” race when multiple Servers sign ENRs with the same Node ID but different ports.

Changes:

• Add a GrpcServer.SetP2PServer(...) hook to inject an externally-managed, shared p2p.Server, plus lifecycle/peer-reporting controls for shared-server mode.
• Update the sentry Provider to build per-protocol GrpcServer instances, merge/dedupe their p2p.Protocols, start one shared p2p.Server, and inject it into all GrpcServers.
• Unify node discovery DB layout from per-protocol subdirs to a single nodes/eth directory in local multi-protocol mode.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
p2p/sentry/sentry_grpc_server.go	Adds shared-server injection (SetP2PServer), external ownership behavior in Close, peer-reporting gating for Peers/NodeInfo, and exports DNS discovery setup.
node/components/sentry/provider.go	Builds a shared p2p.Server across protocol GrpcServers, merges/dedupes protocol registrations, and centralizes node DB + listen-port selection.

Comments suppressed due to low confidence (1)

node/components/sentry/provider.go:405

• GrpcServer.Close intentionally does not Stop() externally-injected p2p.Servers. Since the Provider is the coordinator creating this shared server, it should retain a reference and Stop() it in Provider.Close; otherwise Close can leave the TCP listener and P2P goroutines running unless the caller also cancels SentryCtx.

	for i, ss := range p.Servers {
		// First sentry (highest configured protocol version) reports peers.
		ss.SetP2PServer(srv, i == 0)
	}
	return nil

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[yperbasis]

Overview

Fixes a real bug where erigon's local-mode sentry opened one p2p.Server per eth protocol version. Each Server signed its own ENR under the same Node ID but with a different listener port, racing in the discovery DHT — only the highest-seq ENR survives, so peers dial the "wrong" port and inbound peer count stalls at a fraction of --maxpeers.

The fix moves to geth's model: one p2p.Server with the union of all protocol caps multiplexed per peer connection. Side-effect cleanups (admin_peers de-duplication, MaxPeers honesty, externally-owned-Server lifecycle) fall out naturally.

Stats: +642 / −50, 4 files. Scope is well-contained to the sentry component.

What's strong about this PR

• Real bug with measurable impact. The PR body documents the discovery DHT race precisely, with concrete millisecond-level ENR seq timing and a Hoodi repro. Not a speculative refactor.
• Architectural alignment. Matches geth's design (one Server, multiplexed protocols). Reduces port surface and removes a class of races.
• Clean separation. buildSharedP2PConfig (pure) and startSharedP2PServer (lifecycle) are each small and testable; tests in provider_test.go exercise both directly. SetP2PServer on GrpcServer is a minimal, well-documented injection hook.
• Backwards-compatible. Standalone cmd/sentry and remote --sentry.api.addr mode are untouched — they continue to use the lazy-server path inside SetStatus. The new statusReady channel doesn't change behavior for those (the constructor still creates it; awaitStatus returns immediately if status is already set).
• Good test coverage. Eleven new tests across two packages, including: dedup, reporter wiring, idempotent SetP2PServer panic, external-server lifecycle, owned-server lifecycle, awaitStatus fast/timeout/unblock paths, empty-host port probing, all-ports-busy error path. All pass under go test -race.

I ran the full ./node/components/sentry/... and ./p2p/sentry/... test packages locally with -race -count=1 — all pass cleanly.

Concerns / suggestions

1. Doc/code mismatch: which sentry reports peers (p.Servers[0])

The PR description and the startSharedP2PServer doc comment both claim:

"Exactly one GrpcServer is marked as the peer-list reporter (the first one in p.Servers, which matches the highest entry in ProtocolVersion)"

But nodecfg/defaults.go:51 has:

ProtocolVersion: []uint{direct.ETH69, direct.ETH70, direct.ETH71},

— lowest first, highest last. So p.Servers[0] is ETH69, not the highest. The new test in provider_test.go puts eth/71 as first, which is the opposite of the production default ordering.

Functionally this doesn't matter — p2pServer.PeersInfo() returns the global list regardless of which sentry calls it. But the doc comment will mislead a reader. Either fix the comment ("the first configured protocol version, typically the lowest") or, if "highest" is intentional, sort p.Servers by Protocols[0].Version desc before the i == 0 reporter pick. The latter is cleaner if you care about which goodPeers map the wit/0 traffic lands in (see #4 below).

2. Unlocked reads of external / reportsPeers in Peers() / NodeInfo()

p2p/sentry/sentry_grpc_server.go Peers() and NodeInfo() read these fields without holding p2pServerLock:

p2pServer := ss.getP2PServer()    // takes RLock, releases it
if p2pServer == nil { ... }
if ss.external && !ss.reportsPeers {   // unlocked read
    return &sentryproto.PeersReply{}, nil
}

SetP2PServer writes both fields under Lock. In practice this is fine because SetP2PServer is called once during Initialize, strictly before any gRPC handler can fire — there's a happens-before edge through the multi-client wiring. go test -race didn't flag it because the tests don't drive that race.

But it's a latent risk for future changes (e.g., a hot-reconfig path). Cheap fix: capture both fields inside getP2PServer (rename to return a triple, or add a sibling getServerState() (*p2p.Server, bool, bool)).

3. SetP2PServer panics on double-call

if ss.p2pServer != nil {
    panic("sentry.GrpcServer: SetP2PServer called when p2pServer is already set")
}

The invariant ("ownership decided up front") is sound, but a panic in node startup is harsh. An error return would let the provider fail Initialize cleanly with the same diagnostic. Not a blocker — the panic message is descriptive — but worth reconsidering.

4. wit/0 dedup picks the first server's handler

In startSharedP2PServer, the dedup keeps the first wit/0 Protocol encountered (currently from p.Servers[0], i.e. the ETH69 sentry by default ordering). So every wit-protocol peer's runWitPeer runs on the ETH69 sentry's goodPeers map — even peers that negotiate eth/71 with the ETH71 sentry. That peer ends up in p.Servers[0].goodPeers with protocol=0, witProtocol=0 AND in p.Servers[2].goodPeers with protocol=71.

Effect on runPeerCountLogger: such peers get counted under protocol=0 in the ETH69 sentry's bucket and under their actual eth version in the highest sentry's bucket — double-counted across protocols in the log line. admin_peers is unaffected (gated to the reporter, uses PeersInfo()). Cosmetic logging quirk, not a correctness issue. Worth a one-line note in the comment, or — more cleanly — bind the deduped wit Protocol to the reporter sentry so wit state co-locates with the global peer view.

5. awaitStatus: 10s timeout is reasonable but undiscoverable

awaitStatusTimeout = 10 * time.Second is a package-private constant. It governs how long an inbound peer's Protocol.Run waits for the multi-client to broadcast its first SetStatus. The window between the listener coming up (in Initialize) and the multi-client calling SetStatus (after BuildMultiClient + Start) is normally sub-second, so 10s is plenty.

But if anything blocks the startup path (slow chain init, large snapshot reindex, debugger), peers connecting in the window get a silent DiscProtocolError. Worth a debug-level log on timeout so the operator can correlate "no inbound for the first N seconds" with the cause. The current code drops to nil silently and the caller logs PeerErrorLocalStatusNeeded — which doesn't make the timeout vs. the actual "core didn't send status" case distinguishable.

6. BootstrapNodes mutation lives in the wrong layer

startSharedP2PServer does:

if cfg.BootstrapNodes == nil && len(chainBootnodes) > 0 {
    bootstrapNodes, err := enode.ParseNodesFromURLs(chainBootnodes)
    ...
    cfg.BootstrapNodes = bootstrapNodes
    cfg.BootstrapNodesV5 = bootstrapNodes
}

This duplicates the exact logic in p2p/sentry/sentry_grpc_server.go makeP2PServer. Since you're already building the p2p.Server yourself in startSharedP2PServer, you can either:

• Reuse makeP2PServer (export it, or extract a buildBootstrapNodes helper), or
• Pull the DNS-discovery / bootnodes resolution into a single shared helper that both makeP2PServer and startSharedP2PServer call.

The current duplication is a future-divergence hazard.

7. Minor: awaitStatus doesn't return nil on ctx.Done() deterministically

select {
case <-ss.statusReady:
case <-time.After(maxWait):
case <-ss.ctx.Done():
}
return ss.GetStatus()

On ctx.Done(), the function returns whatever GetStatus() is — which could be non-nil if SetStatus ran concurrently and ctx.Done() won the select. Probably fine (we just return the latest status either way), but the doc comment says "returns nil on timeout so the caller can disconnect the peer" without mentioning the ctx.Done case. Tighten the comment or explicitly return nil when ctx is done.

8. Test gap: end-to-end Initialize with the new shared-Server path

The new unit tests cover buildSharedP2PConfig and startSharedP2PServer in isolation, but no test exercises Provider.Initialize end-to-end in local mode (which is what the bug actually lived in). I understand that requires a full ChainDB + chainspec stub which is non-trivial, but it would be the strongest regression guard for the original DHT-race scenario. At minimum, a test that asserts after Initialize with len(ProtocolVersion) >= 2:

• p.sharedP2PServer != nil
• len(p.Servers) == len(ProtocolVersion)
• p.Servers[0].GetP2PServer() == p.Servers[1].GetP2PServer() == p.sharedP2PServer
• p.Servers[0].IsPeerReporter() == true && p.Servers[1].IsPeerReporter() == false

would catch any future regression that breaks the wiring without a full network stand-up.

9. Backport plan

The PR notes that release/3.4 will need a separate cherry-pick PR landing the fix directly in node/eth/backend.go (no sentryProvider abstraction there). Given this is a peer-connectivity bug affecting users on 3.4 right now, that backport should be tracked as a follow-up immediately after merge. Per CLAUDE.md conventions, the title prefix is [r3.4].

Security / correctness

Nothing concerning. The new code:

• Doesn't introduce new network endpoints — actually closes off two (--port+1, --port+2).
• Doesn't change cryptographic material (same Node ID, same ENR signing).
• Doesn't loosen any peer admission checks.
• Bounds the new awaitStatus window so a stuck startup can't cause unbounded blocking.

Performance

• One less p2p.Server per protocol version: less goroutine overhead, one discovery instance instead of N.
• One DNS-discovery iterator per eth/N protocol still (created in each NewGrpcServer) — same DNS list. Could be deduped to one DNS iterator shared across protocols, but it's not visibly costly and matches geth's per-cap-iterator pattern. Out of scope for this PR.

Verdict

Solid PR. The core change is correct, well-tested, and addresses a documented production bug. My concerns are all polish (doc accuracy, future-proofing locking, factoring duplicated logic). Recommend:

1. Fix the "highest entry in ProtocolVersion" comment in provider.go and the PR body — it's the first configured entry (which is the lowest in the default).
2. Add a one-line debug log when awaitStatus times out.
3. (Optional but nice) Capture external/reportsPeers under p2pServerLock in Peers/NodeInfo for hygiene.
4. Track the [r3.4] backport as a follow-up.

Items 5-8 above are nice-to-haves that can land later. Nothing in this PR blocks merge.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.