fix(eth): make peer removal idempotent to stop unregister flood

[gzliudan]

Proposed changes

When a peer disconnects, my node will instantly flash about 150+duplicate alarms::

WARN Failed to unregister sync peer  peer=191c74d66358dcb2 err="peer is not registered"
DEBUG Removing Ethereum peer  ×9
DEBUG Peer removal failed ...

Concurrent removePeer callers (BFT broadcast loops, DAO fork timers, normal teardown, and the downloader/fetcher drop callbacks) can all pass the Peer(id) lookup before the first caller unregisters the peer, so each re-runs the unregister equence and floods the logs with "peer is not registered" warnings on every peer drop.

Mark peer removal with an atomic flag so the unregister sequence runs exactly once, and let downloader.UnregisterPeer treat an unregistered peer as a silent no-op. In the same race window, handle() could register a peer in the downloader after its removal was claimed, leaving a stale entry that blocks a reconnect of the same node id; re-check the flag after
registering and undo the registration.

Types of changes

What types of changes does your code introduce to XDC network?
Put an ✅ in the boxes that apply

• build: Changes that affect the build system or external dependencies
• ci: Changes to CI configuration files and scripts
• chore: Changes that don't change source code or tests
• docs: Documentation only changes
• feat: A new feature
• fix: A bug fix
• perf: A code change that improves performance
• refactor: A code change that neither fixes a bug nor adds a feature
• revert: Revert something
• style: Changes that do not affect the meaning of the code
• test: Adding missing tests or correcting existing tests

Impacted Components

Which parts of the codebase does this PR touch?
Put an ✅ in the boxes that apply

• Consensus
• Account
• Network
• Geth
• Smart Contract
• External components
• Not sure (Please specify below)

Checklist

Put an ✅ in the boxes once you have confirmed below actions (or provide reasons on not doing so) that

• This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
• Tested on a private network from the genesis block and monitored the chain operating correctly for multiple epochs.
• Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
• Tested the backwards compatibility.
• Tested with XDC nodes running this version co-exist with those running the previous version.
• Relevant documentation has been updated as part of this PR
• N/A

[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

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 735f8e5d-1a37-4434-98f9-1535cd6426d8

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

---

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.}

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

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

Suppressed comments (4)

eth/handler_test.go:850

• This test only checks the final peer-set size, which was already zero before this change even when every concurrent caller reran the unregister sequence. It therefore does not regress the reported warning flood or verify the new exactly-once side effects. Add instrumentation or log capture that asserts downloader/peer-set unregistration occurs once under synchronized concurrent calls.

			pm.removePeer(tp.id)

eth/downloader/downloader_test.go:2326

• The implementation logs this case at Debug level, not as a warning. Update the test documentation so it matches the behavior being locked in.

// logging a warning about the state inconsistency.

eth/peer_test.go:43

• peerSet.Register starts two broadcaster goroutines, but this peer has a nil term channel and Unregister does not stop them, so this test leaks both goroutines until the test process exits. Initialize term and close it during cleanup.

	p := &peer{id: "twice"}

eth/downloader/downloader.go:369

• This at-most-once requirement contradicts both the implementation and TestDownloaderUnregisterPeerIdempotent: a repeated call also produces errNotRegistered and returns nil, so the function cannot infer that the peer was genuinely never registered. Document the actual idempotent contract instead.

// The caller must guarantee at-most-once semantics per peer. Today only
// ProtocolManager.removePeer calls this, and its markRemoved guard ensures a
// peer is unregistered at most once; as long as that invariant holds, an
// errNotRegistered here means the peer was genuinely never registered rather
// than a benign repeat call. If a second caller is ever added, it must uphold

[Copilot]

Pull request overview

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