[gzliudan]

Summary

SimNode.Start registers the services — and therefore appends to the p2p server's protocol slice — before the devp2p node is started. The simulations HTTP API marshals node info from other goroutines (the event stream as soon as a control event is handed over, the node handlers on request) without any synchronisation, so a node that is being started races with readers of p2p.Server.Protocols.

The new race job in #2797 exposed it as p2p/simulations failing on TestHTTPSnapshot:

WARNING: DATA RACE
Read at 0x00c00022a0c8 by goroutine 3856:
  p2p.(*Server).NodeInfo()          p2p/server.go:1128
  ...
Previous write at 0x00c00022a0c8 by goroutine 3862:
  node.(*Node).RegisterProtocols()  node/node.go:570

Change

• SimNode gains startMu, held for the whole of Start and taken shared in NodeInfo.
• The lock order stays startMu -> node.lock (the only other lock on the path), and nothing reached from Start calls NodeInfo, so there is no cycle.
• Only the in-memory adapter changes; the event and node response payloads are unchanged.

Verification

• 100 runs of TestHTTPSnapshot as four concurrent go test -race -short -count=25 processes: no race. The same command on the parent commit failed in 1–2 of the 4 processes.
• Dropping the read lock again makes 3 of the 4 processes fail, so the lock is what keeps it quiet.
• make all, go test ./p2p/simulations/..., golangci-lint 2.12.2 on ./p2p/simulations/... (no new findings) and make tidy (no untidy module files) pass.

Upstream

geth removed p2p/simulations in ethereum#30250 without fixing this, and both NodeInfo and RegisterProtocols are still unsynchronised there, so there is nothing to port.

Ref: #2797

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: fc457eae-8da2-461f-a777-bb844f2f98dd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gzliudan]

Disposition of the two open review items: both are won't-fix. No code change follows.

1. NodeInfo's blocking RLock stalling /events and then every net.lock holder. That chain is not reachable. Every SimNode.Start() runs inside Network.startWithSnapshots, which holds net.lock exclusively for the whole node.Start() call (p2p/simulations/network.go:179-190), and that is the only call site of SimNode.Start. A goroutine blocked in startMu.RLock() therefore implies net.lock is already held by the starter, so no other net.lock holder can be inside events.Send; the remaining wait is bounded by a single SimNode.Start. The upstream comparison also does not hold: geth's event/feed.go uses the same removeSub-only reflect.Select with no timeout.

2. Moving the srv.Protocols read under srv.lock. That does not fix the race: node.RegisterProtocols (node/node.go:563-570) appends under n.lock only, so srv.lock provides no happens-before with the writer. On top of that, srv.Start() holds srv.lock for the entire startup (p2p/server.go:453-506), so the read would block admin_nodeInfo for the whole server start. Narrowing the lock to the registration section is technically possible, but it would re-expose the 0.0.0.0:0 placeholder that Self() returns before setupLocalNode runs, for no practical gain.

The comment fix requested in the previous round landed in 300bf5e.

[gzliudan]

Follow-up on the third review item: the admin_nodeInfo path flagged in the latest review is not actually racy, so no code change is needed for it either.

Why: every mutation of the p2p server's protocol list and listen address happens inside SimNode.Start before node.Start -> openEndpoints -> startInProc registers the admin API on the in-process RPC server (serviceRegistry.registerName takes r.mu, rpc/service.go), and every dispatched request looks its callback up under that same mutex (h.reg.callback, rpc/handler.go). A dispatch that acquires the mutex before registration gets method-not-found without ever touching the server; one that acquires it after is ordered after all of the startup writes by the mutex release/acquire pair. After Stop there is no window either: rpc.Server.Stop clears the run flag permanently (nothing ever sets it back), trackCodec refuses new codecs and all tracked ones are closed, and doClose sets closedState so a node cannot be restarted at all (Start returns ErrNodeStopped).

Verified empirically on this exact head: a probe hammering admin_nodeInfo over the in-proc client before/during/after Start runs clean under -race (50 runs). As a negative control, a probe calling p2p.Server.NodeInfo directly — bypassing both the registry mutex and SimNode.NodeInfo's new read lock — reproduces the exact CI race pair (server.go:1128 read vs node.go:570 write), confirming the probe can detect the overlap when no synchronisation edge exists.

Net: all three review items are closed (two wont-fix with reasons above, one retracted), the branch is unchanged, and it is good to merge from my side.