RPC: admin_* peer-management endpoints improvements

[svlachakis]

Fixes Closes Resolves #11490

Summary

Wire-level breaking changes (all four endpoints)

• Return type string → bool on addPeer/removePeer (already bool on the trusted pair)
• Param renamed: addToStaticNodes → persistent, removeFromStaticNodes → persistent (consistent across all four)
• addTrustedPeer and removeTrustedPeer gained the new optional persistent parameter

Default-semantic changes (the same call now does something different)

• admin_addPeer(enode): now maintains the connection (Geth-equivalent); was a one-shot dial
• admin_removePeer(enode): now removes from static set + disconnects + idempotent; was pool-only and reported failure on unknown peers
• admin_addTrustedPeer(enode): no longer writes to trusted-nodes.json by default
• admin_removeTrustedPeer(enode): no longer writes to trusted-nodes.json by default; now idempotent on unknown peers

Validation (all four)

• Malformed enode now returns -32602 InvalidParams with "invalid enode: ..." instead of an opaque internal RPC error.

Further Refactoring

• Replaced Enode.IsEnode pre-check with TryParseAsNetworkNode/TryParseAsEnode helpers wrapping full-parse construction in try/catch (catches valid-scheme/invalid-content inputs)
• Plumbed IJsonRpcConfig-derived CancellationToken through
• IStaticNodesManager/ITrustedNodesManager/NodesManager.SaveFileAsync; dropped string-typed overloads of IStaticNodesManager.AddAsync/RemoveAsync/IsStatic (test-only, replaced with NetworkNode-typed) to eliminate the double-parse + double-DNS-lookup; consolidated 4 single-method malformed-enode tests into one parameterized [TestCase] covering both scheme-fail and content-fail paths
• Added _subscriptionManager null-guard, fixed field-assignment ordering, and removed the enode.ToString() round-trip in TrustedNodesManager.RemoveAsync

---

Changes

---

admin_addPeer

Behavior	Geth (only mode)	Nethermind BEFORE — default (addToStaticNodes=false)	Nethermind BEFORE — addToStaticNodes=true	Nethermind AFTER — default (persistent=false)	Nethermind AFTER — persistent=true
Adds to in-memory static set	✅	❌ pool dict only	✅	✅ matches Geth	✅
Maintains connection (auto-redial)	✅	❌ one-shot dial	✅	✅ matches Geth	✅
Writes to static-nodes.json	❌	❌	✅	❌ matches Geth	✅ (NM extension)
Validates enode upfront	✅ clean error	❌ throws on bad input → opaque	❌ same	✅ Enode.IsEnode → -32602	✅ same
Return type	bool	string (echoes enode)	string	bool matches Geth	bool

---

admin_removePeer

Behavior	Geth	NM BEFORE — default (removeFromStaticNodes=false)	NM BEFORE — removeFromStaticNodes=true	NM AFTER — default	NM AFTER — persistent=true
Removes from in-memory static set	✅	❌ pool only	✅	✅ matches Geth	✅
Disconnects active session	✅	✅ via direct TryRemove	✅ via event chain	✅ via event chain + direct TryRemove fallback	✅
Removes non-static (discovered) peer too	✅	✅	❌ static set only	✅ matches Geth (idempotent fallback)	✅
Writes to static-nodes.json	❌	❌	✅	❌ matches Geth	✅ (NM extension)
Idempotent on unknown peer	✅ returns true	❌ Fail	❌ Fail	✅ returns true matches Geth	✅
Validates enode upfront	✅	❌ throws	❌ throws	✅ -32602	✅
Return type	bool	string	string	bool matches Geth	bool

---

admin_addTrustedPeer

Behavior	Geth	NM BEFORE	NM AFTER — default	NM AFTER — persistent=true
Adds to in-memory trusted set	✅	✅	✅	✅
Writes to trusted-nodes.json	❌	✅ always (updateFile: true hardcoded)	❌ matches Geth	✅ (NM extension)
Synchronously inserts into peer pool	❌ relies on dialer	✅ _peerPool.GetOrAdd	✅ kept (NM nuance — avoids 1s FeedFromNodeSource throttle)	✅ kept
Idempotent on already-trusted	✅	✅ via IsTrusted short-circuit	✅ via ignoring AddAsync return	✅
Validates enode upfront	✅	❌ throws	✅ -32602	✅
Return type	bool	bool	bool matches Geth	bool

---

admin_removeTrustedPeer

Behavior	Geth	NM BEFORE	NM AFTER — default	NM AFTER — persistent=true
Removes from in-memory trusted set	✅	✅	✅	✅
Writes to trusted-nodes.json	❌	✅ always (updateFile: true hardcoded)	❌ matches Geth	✅ (NM extension)
Disconnects active session	❌ stays connected, loses privilege	✅ via event chain (NodeRemoved → PeerPool.TryRemove → MarkDisconnected)	✅ kept (NM-specific, deliberate)	✅ kept
Idempotent on unknown peer	✅ returns true	❌ Fail	✅ returns true matches Geth	✅
Validates enode upfront	✅	❌ throws	✅ -32602	✅
Return type	bool	bool	bool matches Geth	bool

---

Nethermind extensions Geth doesn't have

Extension	Endpoints	Effect
persistent: true flag	admin_addPeer, admin_removePeer	Also write/remove the entry to/from static-nodes.json so it survives restart
persistent: true flag	admin_addTrustedPeer, admin_removeTrustedPeer	Also write/remove the entry to/from trusted-nodes.json so trust survives restart
Synchronous pool insertion	admin_addTrustedPeer	Avoids the up-to-1s PeerPool.FeedFromNodeSource throttle delay; the peer is in the pool the moment the RPC returns
Disconnect on untrust	admin_removeTrustedPeer	Geth leaves the peer connected after untrusting; Nethermind disconnects via the existing NodeRemoved event chain — operationally cleaner ("force reconnect under non-trusted rules")

---

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

• Yes
• No

[claude]

Claude finished @svlachakis's task in 4m 28s —— View job

---

Code Review

• Gather context (rules, changed files)
• Review IAdminRpcModule.cs interface changes
• Review AdminRpcModule.cs implementation
• Review AdminModuleTests.cs test coverage
• Post inline comments and synthesize findings

---

Summary

The overall direction is good — aligning with Geth semantics, making the API idempotent, and adding upfront validation are all the right calls. The interface docs and PR description are thorough. One correctness issue must be resolved before merge.

---

Findings

High

Incomplete enode validation — unhandled exceptions bypass the -32602 guard (see inline comments on lines 112 and 125 of AdminRpcModule.cs)

Enode.IsEnode only checks Uri.TryCreate && scheme == "enode". Inputs like enode://badhex@127.0.0.1:30303 (valid scheme, invalid public key) pass that guard and then throw ArgumentException (or SocketException for DNS hostnames) inside new Enode(enode) / new NetworkNode(enode). The exception escapes the async Task<ResultWrapper<bool>> as a faulted task, returning an opaque internal error rather than the clean -32602 InvalidParams the PR claims to deliver.

All four methods are affected — admin_removePeer (explicit NetworkNode ctor), admin_addTrustedPeer and admin_removeTrustedPeer (explicit Enode ctor), and admin_addPeer (implicit, via StaticNodesManager.AddAsync which calls new NetworkNode(enode) internally).

Fix: wrap each object construction in a try/catch converting to ResultWrapper<bool>.Fail($"invalid enode: {e.Message}", ErrorCodes.InvalidParams).

---

Medium

1. admin_removePeer dual-removal ordering needs a comment (inline on line 113)
   StaticNodesManager.RemoveAsync fires NodeRemoved synchronously, which already calls TryRemove via the event chain. The subsequent direct _peerPool.TryRemove is a fallback for peers not in the static set. The intent is correct but looks redundant without a comment; a future reader may accidentally remove one of the two paths.

2. No CancellationToken on async RPC methods
   admin_addPeer, admin_removePeer, admin_addTrustedPeer, and admin_removeTrustedPeer perform file I/O (SaveFileAsync) when persistent = true, with no cancellation support. Per the repo's robustness rules, async methods that perform I/O must accept a CancellationToken. This is a pre-existing pattern, but a breaking-change release is a good time to fix it.

3. Missing test for valid-scheme / invalid-content enodes (inline on line 293 of AdminModuleTests.cs)
   Existing error-path tests only cover "not-an-enode" which fails the URI parse. There are no tests for enodes that pass IsEnode but fail the constructor (bad key hex, bad port, etc.). These are exactly the inputs that expose the High-severity issue above.

4. AdminPeerLifecycle_AddRemoveListBothArities_DoesNotThrow is a pure smoke test (inline on line 267)
   It doesn't verify any behaviour change introduced by this PR. Either add meaningful assertions or remove it in favour of the parameterised tests that already cover the paths.

---

Low

• Double URI parse: IsEnode(enode, out _) discards the parsed Uri; new Enode(enode) parses the string again (including a potential DNS round-trip for hostname enodes). The out-parameter could be threaded through to avoid the second parse. Minor, but easy to fix alongside the try/catch work.
• using System.Linq is still imported in AdminRpcModule.cs for the LINQ chain in admin_peers (pre-existing, not introduced here).

---

Breaking-change acknowledgment

The wire-level changes (string → bool on addPeer/removePeer, param rename, new persistent flag) are clearly documented in the PR and the interface XML docs. No additional concern beyond ensuring release notes call this out explicitly.

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 3m 52s —— View job

---

Code Review (Re-review)

• Gather context (rules, changed files)
• Check if previous High findings were addressed
• Review AdminRpcModule.cs current state
• Review AdminModuleTests.cs current state
• Review supporting files (StaticNodesManager.cs, TrustedNodesManager.cs, etc.)
• Post updated findings

---

Previous Finding Status

All previous findings have been addressed:

Finding	Severity	Status
Incomplete enode validation — unhandled exceptions bypass -32602 guard	High	✅ Fixed — TryParseAsNetworkNode/TryParseAsEnode helpers wrap constructors in try/catch
Missing test for valid-scheme / invalid-content enodes	Medium	✅ Fixed — AdminPeerMethods_WithInvalidEnode_ReturnsInvalidParamsError now includes "enode://badhex@127.0.0.1:30303" cases for all four methods
Dual-removal ordering in admin_removePeer needs a comment	Medium	✅ Fixed — clear comment explaining the static-set event chain vs. discovered-peer fallback
AdminPeerLifecycle_AddRemoveListBothArities_DoesNotThrow is a pure smoke test	Medium	✅ Fixed — replaced with assertion-backed parameterized tests
Double URI parse (low)	Low	✅ Eliminated — IsEnode guard removed; constructors called directly inside try/catch

---

New Findings

Low

1. TrustedNodesManager.RemoveAsync uses new NetworkNode(enode.ToString()) while AddAsync uses new NetworkNode(enode) (TrustedNodesManager.cs:85). The string round-trip is harmless because NodeId is derived from the public key regardless, but the inconsistency between the two overloads is a minor code smell. Pre-existing, not introduced by this PR.

2. admin_addTrustedPeer calls _peerPool.GetOrAdd unconditionally, including when AddAsync returns false (already trusted). The result of AddAsync is intentionally discarded, so GetOrAdd fires on every call — this is by design ("synchronous pool insertion guarantee") but creates a new NetworkNode(enodeObj!) allocation on every idempotent re-trust. Harmless and intentional per the PR description.

3. IJsonRpcConfig null-guard ordering: _jsonRpcConfig is assigned after BuildNodeInfo() is called in the constructor (AdminRpcModule.cs:64-65). BuildNodeInfo does not use _jsonRpcConfig, so the ordering is safe, but moving the assignment before the call would be more defensive.

4. No null-guard on _subscriptionManager: All other constructor parameters have ?? throw new ArgumentNullException(...) guards, but _subscriptionManager does not (line 65). This is a pre-existing inconsistency.

---

Summary

The core concerns from the previous review are all resolved. The validation now correctly catches both URI-parse failures and constructor-level failures (bad key hex, bad port, DNS) for all four endpoints. Tests cover both failure classes. The dual-removal comment is clear. The smoke test has been properly replaced.

The remaining items are all low-severity and either pre-existing or intentional design choices documented in the PR description. This PR is ready to merge.

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 3m 5s —— View job

---

Code Review (Re-review #2)

• Gather context (rules, changed files)
• Review AdminRpcModule.cs — validation helpers and logic
• Review AdminModuleTests.cs — test coverage
• Review StaticNodesManager.cs and TrustedNodesManager.cs
• Review supporting interfaces and DI wiring
• Post findings

---

Previous Finding Status

All findings from prior reviews are resolved:

Finding	Severity	Status
Incomplete enode validation — exceptions bypassed -32602 guard	High	✅ Fixed — TryParseAsNetworkNode/TryParseAsEnode catch all exceptions
Missing tests for valid-scheme / invalid-content enodes	Medium	✅ Fixed — AdminPeerMethods_WithInvalidEnode_ReturnsInvalidParamsError covers "enode://badhex@..." for all four methods
Dual-removal ordering in admin_removePeer needs a comment	Medium	✅ Fixed — comment explains static-set event chain vs. discovered-peer fallback
AdminPeerLifecycle_AddRemoveListBothArities_DoesNotThrow was a pure smoke test	Medium	✅ Fixed — replaced with assertion-backed parametrized tests
Double URI parse (discarded out _ from IsEnode)	Low	✅ Eliminated — IsEnode guard removed, constructors called directly inside try/catch
No CancellationToken on async methods	Medium	✅ Fixed — all four methods now thread a timeout CT via BuildTimeoutCancellationTokenSource()

---

New Findings

Low

TrustedNodesManager.RemoveAsync: OnNodeRemoved is skipped if file write is cancelled (TrustedNodesManager.cs:100-105)

if (updateFile)
{
    await SaveFileAsync(cancellationToken);   // <-- if CT fires here, throws
}
OnNodeRemoved(networkNode);                  // <-- never reached on cancellation

When persistent=true and the RPC timeout fires during SaveFileAsync, an OperationCanceledException propagates and OnNodeRemoved is never called. The peer is already removed from the in-memory _nodes dict (line 86) but the disconnect event chain is skipped — so the peer stays connected despite being un-trusted in memory.

StaticNodesManager.RemoveAsync has the opposite ordering (event fires before file write), which avoids this problem. The fix is to move OnNodeRemoved before the if (updateFile) block, or wrap with try/finally. Fix this →

admin_addTrustedPeer calls _peerPool.GetOrAdd unconditionally (AdminRpcModule.cs:97)

AddAsync return value is discarded, so GetOrAdd fires on every call including idempotent re-trusts — allocates a new NetworkNode(enodeObj!) each time. This is intentional per the PR description ("synchronous pool insertion guarantee") so no change needed, but the allocation is unnecessary on re-trust paths. Pre-existing design choice.

---

Summary

The core changes are sound. All High and Medium issues from prior reviews are resolved. Validation now correctly converts both URI-parse failures and constructor-level failures to -32602 InvalidParams across all four endpoints. Tests cover the full failure matrix. The idempotency contracts match Geth semantics throughout.

The only new finding is a Low-severity edge case in TrustedNodesManager.RemoveAsync where a cancelled file write can leave a peer connected after memory-removal. This affects only persistent=true calls that hit the RPC timeout during file I/O — a narrow window in practice. The fix is a one-line reorder. Whether to address before merge is at the author's discretion.

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 2m 59s —— View job

---

Code Review (Re-review #3)

• Gather context (rules, changed files)
• Read rules files
• Review AdminRpcModule.cs — validation helpers, logic, CT usage
• Review TrustedNodesManager.cs — OnNodeRemoved ordering
• Review AdminModuleTests.cs — test coverage
• Post findings

---

Previous Finding Status

All findings from prior reviews are resolved:

Finding	Severity	Status
Incomplete enode validation — exceptions bypassed -32602 guard	High	✅ Fixed — TryParseAsNetworkNode/TryParseAsEnode catch all exceptions
Missing tests for valid-scheme / invalid-content enodes	Medium	✅ Fixed — AdminPeerMethods_WithInvalidEnode_ReturnsInvalidParamsError covers "enode://badhex@..." for all four methods
Dual-removal ordering in admin_removePeer needs a comment	Medium	✅ Fixed — comment explains static-set event chain vs. discovered-peer fallback
AdminPeerLifecycle_AddRemoveListBothArities_DoesNotThrow pure smoke test	Medium	✅ Fixed — replaced with assertion-backed parameterized tests
No CancellationToken on async methods	Medium	✅ Fixed — all four methods thread a timeout CT via BuildTimeoutCancellationTokenSource()
OnNodeRemoved skipped if cancellation fires during SaveFileAsync	Low	✅ Fixed — TrustedNodesManager.RemoveAsync now calls OnNodeRemoved before the file write (TrustedNodesManager.cs:103)
new NetworkNode(enode.ToString()) string round-trip	Low	✅ Fixed — changed to new NetworkNode(enode)
Constructor null-guard ordering (_jsonRpcConfig assigned before BuildNodeInfo)	Low	✅ Fixed
No null-guard on _subscriptionManager	Low	✅ Fixed — ?? throw new ArgumentNullException(...) added (AdminRpcModule.cs:62)

---

New Findings

Low

admin_removePeer: discovered-peer disconnect skipped if cancellation fires during file write (AdminRpcModule.cs:86-87)

await _staticNodesManager.RemoveAsync(networkNode!, updateFile: persistent, timeout.Token);
_peerPool.TryRemove(networkNode!.NodeId, out _);   // fallback for non-static peers

If persistent=true and cancellation fires during SaveFileAsync inside RemoveAsync, an OperationCanceledException propagates and line 87 is never reached. Static peers are already disconnected by then (the NodeRemoved event fires before the file write in StaticNodesManager), but non-static (discovered) peers miss the TryRemove fallback.

This is a narrow window — it only matters for discovered peers when persistent=true and the file I/O hits the RPC timeout — but the intent of admin_removePeer is "disconnect the peer regardless of whether it was static". A try/finally would guarantee the fallback:

try
{
    await _staticNodesManager.RemoveAsync(networkNode!, updateFile: persistent, timeout.Token);
}
finally
{
    _peerPool.TryRemove(networkNode!.NodeId, out _);
}

Not blocking, but easy to address.

admin_addTrustedPeer unconditional GetOrAdd allocation (AdminRpcModule.cs:97)

new NetworkNode(enodeObj!) is allocated on every call (including idempotent re-trusts where AddAsync returns false). This is intentional per the PR description ("synchronous pool insertion guarantee") and benign in practice since admin_addTrustedPeer is not a hot path.

---

Summary

All High and Medium findings from prior reviews are fully resolved. Validation correctly converts both URI-parse failures and constructor-level failures to -32602 InvalidParams across all four endpoints. Tests cover the full failure matrix. The OnNodeRemoved-before-SaveFileAsync ordering fix in TrustedNodesManager is clean and consistent with StaticNodesManager. Constructor null-guards and ordering are correct.

The only remaining items are Low-severity edge cases or intentional design choices. This PR is ready to merge.

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 3m 14s —— View job

---

Code Review (Re-review #4)

• Gather context (rules, changed files)
• Review AdminRpcModule.cs — current state
• Review TrustedNodesManager.cs — cancellation ordering
• Review StaticNodesManager.cs — event/file ordering
• Review AdminModuleTests.cs + TrustedNodesManagerTests.cs
• Post findings

---

Previous Finding Status

All findings from prior reviews are resolved:

Finding	Severity	Status
Incomplete enode validation — exceptions bypassed -32602 guard	High	✅ Fixed — TryParseAsNetworkNode/TryParseAsEnode catch all exceptions
Missing tests for valid-scheme / invalid-content enodes	Medium	✅ Fixed — parameterized AdminPeerMethods_WithInvalidEnode_ReturnsInvalidParamsError covers "enode://badhex@..." for all four methods
Dual-removal ordering in admin_removePeer needs a comment	Medium	✅ Fixed — comment + try/finally guarantees fallback runs even on CT cancellation
AdminPeerLifecycle_AddRemoveListBothArities_DoesNotThrow was a pure smoke test	Medium	✅ Fixed — replaced with assertion-backed parameterized tests
No CancellationToken on async methods	Medium	✅ Fixed — all four methods thread a timeout CT via BuildTimeoutCancellationTokenSource()
OnNodeRemoved skipped if CT fires during SaveFileAsync	Low	✅ Fixed — TrustedNodesManager.RemoveAsync now calls OnNodeRemoved before the file write (TrustedNodesManager.cs:103)
new NetworkNode(enode.ToString()) string round-trip in RemoveAsync	Low	✅ Fixed — new NetworkNode(enode)
Constructor null-guard and ordering for _subscriptionManager / _jsonRpcConfig	Low	✅ Fixed — both guards added, assignments happen before BuildNodeInfo()
admin_removePeer discovered-peer disconnect skipped if CT fires during file write	Low	✅ Fixed — try/finally wraps RemoveAsync, guaranteeing _peerPool.TryRemove fallback

---

New Findings

Low

admin_addTrustedPeer finally allocates new NetworkNode(enodeObj!) unconditionally (AdminRpcModule.cs:113)

This allocation runs on every call — including idempotent re-trusts where AddAsync returns false without touching the channel or file. As noted in the PR description, this is intentional (guarantees synchronous pool insertion even after a CT-cancelled channel write). Not a hot path, so no action required.

TrustedNodesManagerTests doesn't assert node is absent from _nodes after cancellation (TrustedNodesManagerTests.cs:36-40)

The test correctly verifies that NodeRemoved fires before SaveFileAsync raises OperationCanceledException. It does not also assert that _nodes no longer contains the node after the exception. The implementation is correct (TryRemove runs before OnNodeRemoved), but a complementary assertion like manager.Nodes.Should().NotContain(...) would fully pin the invariant. Minor test-completeness gap; not blocking.

IsTrusted host/port comparison vs. _nodes public-key keying (TrustedNodesManager.cs:113-125) — pre-existing, not introduced here

_nodes is keyed by public key only, so TryAdd with a duplicate public key silently no-ops even if host/port differ. IsTrusted additionally checks host and port, meaning the stored entry and the query can diverge. This is a subtle pre-existing inconsistency; not introduced by this PR.

---

Summary

All High and Medium findings from prior reviews are fully resolved. Validation correctly converts both URI-parse failures and constructor-level failures to -32602 InvalidParams across all four endpoints. try/finally blocks ensure in-memory state (pool insertion / pool removal / disconnect event chain) is consistent even when file I/O is cancelled. TrustedNodesManager.RemoveAsync and StaticNodesManager.RemoveAsync both fire their disconnect events before the file write, matching the correct ordering. Tests cover both invalid-enode classes, all persistent/non-persistent branches, and the cancellation-ordering invariant via a dedicated TrustedNodesManagerTests.

The remaining items are all Low-severity pre-existing or intentional design choices. This PR is ready to merge.

[claude]

Claude finished @svlachakis's task in 2m 25s —— View job

---

Code Review (Final — ready_for_review)

• Read rules files
• Review AdminRpcModule.cs — current state
• Review AdminModuleTests.cs — test coverage
• Review TrustedNodesManager.cs / StaticNodesManager.cs
• Review supporting interfaces and DI wiring
• Post findings

---

Previous Finding Status

All findings from prior review rounds are confirmed resolved:

Finding	Severity	Status
Incomplete enode validation — exceptions bypassed -32602 guard	High	✅ Fixed — TryParseAsNetworkNode/TryParseAsEnode wrap constructors in try/catch
Missing tests for valid-scheme/invalid-content enodes	Medium	✅ Fixed — parameterized test covers "enode://badhex@..." for all four methods
admin_removePeer dual-removal ordering needs comment	Medium	✅ Fixed — clear comment + try/finally guarantees fallback on CT cancellation
Pure smoke test (no assertions)	Medium	✅ Fixed — replaced with assertion-backed parameterized tests
No CancellationToken on async methods	Medium	✅ Fixed — all four methods use BuildTimeoutCancellationTokenSource()
OnNodeRemoved skipped if CT fires during SaveFileAsync	Low	✅ Fixed — TrustedNodesManager.RemoveAsync calls OnNodeRemoved before file write
new NetworkNode(enode.ToString()) round-trip	Low	✅ Fixed — new NetworkNode(enode)
Constructor null-guard ordering and _subscriptionManager guard	Low	✅ Fixed — all guards present, assignments before BuildNodeInfo()
admin_removePeer discovered-peer disconnect skipped on CT	Low	✅ Fixed — try/finally wraps RemoveAsync, guaranteeing _peerPool.TryRemove fallback

---

New Findings

Low

TryParseAsNetworkNode/TryParseAsEnode catch all Exception (AdminRpcModule.cs:196-219)

Both helpers use catch (Exception ex) which catches OutOfMemoryException, ThreadAbortException, and other CLR-fatal exceptions alongside the expected ArgumentException/FormatException. For constructors that do pure string parsing this is standard defensive coding and will not mask real bugs in practice, but it violates the general principle of catching the narrowest type needed. Could be narrowed to:

catch (Exception ex) when (ex is ArgumentException or FormatException or SocketException)

Not blocking.

admin_addTrustedPeer always calls _peerPool.GetOrAdd in finally (AdminRpcModule.cs:111-114)

Allocates new NetworkNode(enodeObj!) on every call, including idempotent re-trusts where AddAsync returns false. This is intentional per the PR description ("synchronous pool insertion guarantee"). Not a hot path, so no action required.

---

Correctness Confirmation

The core behaviours introduced by this PR are correct:

• admin_addPeer: Adds to static set (auto-redial) by default; persistent=true also writes to static-nodes.json. Matches Geth semantics. ✅
• admin_removePeer: Removes from static set + disconnects via NodeRemoved event chain; try/finally guarantees direct pool removal fallback even for non-static (discovered) peers and on CT cancellation. Idempotent. ✅
• admin_addTrustedPeer: Adds to trusted in-memory set; GetOrAdd in finally guarantees pool insertion even if the channel write is cancelled. ✅
• admin_removeTrustedPeer: OnNodeRemoved fires before the file write, so a cancelled SaveFileAsync cannot leave a peer trusted in memory yet still connected. ✅
• Validation: Both URI-scheme failures ("not-an-enode") and constructor-level failures ("enode://badhex@...") correctly return -32602 InvalidParams for all four endpoints. ✅
• TrustedNodesManagerTests: Directly pins the NodeRemoved-before-SaveFileAsync ordering invariant, and also asserts the in-memory dict is cleared. ✅

Summary

All High and Medium findings from prior reviews are fully resolved. The implementation is correct, idempotent, and matches Geth semantics where specified. Test coverage is thorough, including the narrow edge cases. This PR is ready to merge.

[svlachakis]

@claude review again. This is intentional admin_addTrustedPeer always calls _peerPool.GetOrAdd in finally (AdminRpcModule.cs:111-114)

[claude]

Claude finished @svlachakis's task in 2m 24s —— View job

---

Code Review (Re-review #5)

• Gather context and rules
• Review AdminRpcModule.cs — current state
• Review TrustedNodesManager.cs / StaticNodesManager.cs
• Review AdminModuleTests.cs — test coverage
• Post findings

---

Previous Finding Status

All findings from prior reviews are confirmed resolved, including the GetOrAdd in finally:

Finding	Severity	Status
Incomplete enode validation — exceptions bypassed -32602 guard	High	✅ Fixed — TryParseAsNetworkNode/TryParseAsEnode wrap constructors in try/catch (ArgumentException or FormatException or SocketException)
Missing tests for valid-scheme/invalid-content enodes	Medium	✅ Fixed — parameterized test covers "enode://badhex@..." for all four methods
admin_removePeer dual-removal ordering needs comment	Medium	✅ Fixed — clear comment + try/finally guarantees fallback on CT cancellation
No CancellationToken on async methods	Medium	✅ Fixed — all four methods use BuildTimeoutCancellationTokenSource()
OnNodeRemoved skipped if CT fires during SaveFileAsync	Low	✅ Fixed — TrustedNodesManager.RemoveAsync calls OnNodeRemoved before file write
Constructor null-guard ordering and _subscriptionManager guard	Low	✅ Fixed
admin_removePeer discovered-peer disconnect skipped on CT	Low	✅ Fixed — try/finally wraps RemoveAsync

---

Acknowledged: admin_addTrustedPeer GetOrAdd in finally — intentional design

The unconditional _peerPool.GetOrAdd(new NetworkNode(enodeObj!)) in the finally block (AdminRpcModule.cs:114) is intentional. The comment on lines 104–107 explains the rationale clearly: if WriteAsync to _nodeChannel is cancelled, the node is in _nodes but was never published to the pool via the channel, so the direct GetOrAdd in finally closes that gap. This is correct and the test at line 196 asserts it.

---

New Findings

Low

admin_addTrustedPeer: OperationCanceledException propagates despite consistent in-memory state (AdminRpcModule.cs:108-117)

When the RPC timeout fires during _nodeChannel.Writer.WriteAsync, AddAsync throws OperationCanceledException. The finally runs (GetOrAdd succeeds — the node is in both _nodes and the pool), but the exception then propagates out of admin_addTrustedPeer as a faulted task. The JSON-RPC handler returns an internal error to the client. The caller sees a failure, but the operation actually succeeded. This is a narrow window (only relevant when persistent=false and the channel write hits the timeout), and pre-existing in the async-RPC pattern, but worth noting. No action needed before merge.

AdminAddTrustedPeer_WhenAlreadyTrusted_StillReturnsTrue doesn't assert GetOrAdd is called (AdminModuleTests.cs:200-211)

Since the unconditional-GetOrAdd behavior is now a documented invariant, a peerPool.Received(1).GetOrAdd(...) assertion in this test would pin the idempotent-path guarantee. Minor documentation gap, not blocking.

---

Correctness Confirmation

• Validation: Both "not-an-enode" (URI-scheme failure) and "enode://badhex@127.0.0.1:30303" (constructor-level failure) correctly return -32602 InvalidParams for all four endpoints via the when (ex is ArgumentException or FormatException or SocketException) filter. ✅
• admin_addPeer: Adds to static set + fires NodeAdded event (auto-redial) by default; persistent=true also writes to static-nodes.json. ✅
• admin_removePeer: try/finally guarantees _peerPool.TryRemove fallback runs even for non-static peers and on CT cancellation. Idempotent. ✅
• admin_addTrustedPeer: GetOrAdd in finally guarantees pool insertion even if channel write is cancelled. Intentional, documented, tested. ✅
• admin_removeTrustedPeer: OnNodeRemoved fires before file write; idempotent. ✅
• TrustedNodesManagerTests: Pins the NodeRemoved-before-SaveFileAsync ordering invariant. ✅

---

Summary

All High and Medium findings from prior reviews are fully resolved. The GetOrAdd in finally is intentional and correctly documented. The remaining items are Low-severity edge cases or documentation nits. This PR is ready to merge.

[svlachakis]

@claude please check the IsTrusted improvement.

Trust is keyed by public key only, matching geth's Server.trusted map (p2p/server.go:644 — trusted[enode.ID()] = true; enode.ID() is keccak256(pubkey), a 1:1 mapping with the pubkey).

Host and port are address-book metadata, not part of the trust identity: a trusted peer that reconnects from a different address must still be treated as trusted.

[claude]

Claude finished @svlachakis's task in 1m 18s —— View job

---

Code Review — IsTrusted public-key keying

• Gather context
• Read TrustedNodesManager.cs — IsTrusted and _nodes keying
• Read ITrustedNodesManager.cs — interface signature
• Verify alignment with Geth semantics
• Post findings

---

IsTrusted — confirmed correct

The implementation is correct and matches Geth's semantics exactly.

TrustedNodesManager.cs:113

public bool IsTrusted(Enode enode) => _nodes.ContainsKey(enode.PublicKey);

_nodes dictionary (NodesManager.cs:24):

protected ConcurrentDictionary<PublicKey, NetworkNode> _nodes = [];

Both AddAsync and RemoveAsync key by networkNode.NodeId, which is Enode.PublicKey (NetworkNode.cs:96). IsTrusted looks up by enode.PublicKey against the same key type. Host and port play no part in the lookup — a trusted peer reconnecting from a different address is correctly recognised as trusted.

The previous Low finding from review round #4 ("IsTrusted host/port comparison vs. _nodes public-key keying") is resolved by this PR.

Test coverage (TrustedNodesManagerTests.cs:49–62) directly pins the invariant:

[Test]
public async Task IsTrusted_AfterAdd_ReturnsTrueRegardlessOfHostOrPort()
{
    // EnodeStringSamePubkeyDifferentAddress uses same pubkey, but IP 10.0.0.1:31313 instead of 192.81.208.223:30303
    manager.IsTrusted(enodeSamePubkeyDifferentAddress).Should().BeTrue(
        because: "trust is keyed by public key only, matching geth's Server.trusted map (p2p/server.go:644 trusted[enode.ID()])...");
}

The test comment even cites the Geth source reference. No issues here. ✅

This PR is ready to merge.

[LukaszRozmej]

should we return result of this or always true?

[svlachakis]

Geth-parity. Server.RemovePeer is idempotent, admin.RemovePeer always returns true, nil on parsed input regardless of whether the peer existed. Returning TryRemove's bool would break that contract.

[LukaszRozmej]

Should we potentially skip this check and use result of _trustedNodesManager.AddAsync?

[svlachakis]

That's fast path. IsTrusted is one ContainsKey. Without it the re-trust path allocates a CTS, runs the async state machine, allocates a NetworkNode inside AddAsync only to fail TryAdd on duplicate, and allocates another NetworkNode for the GetOrAdd in finally. Restores the short-circuit the original IsTrusted(...) || AddAsync(...) had.

[LukaszRozmej]

should this happen only if _trustedNodesManager.AddAsync is true?

[svlachakis]

that's for Cancellation safety. If AddAsync is cancelled mid-await, control skips straight to finally, added would be false and conditional GetOrAdd would leave the peer in the trusted dict but not in the pool. Unconditional GetOrAdd is no-op when already pooled.

[LukaszRozmej]

should this always be true even if above calls fail?

[svlachakis]

I think it's only reachable on AddAsync returning. Throws propagate past the return, the JSON-RPC layer reports them as errors. AddAsync returning false (already trusted) is geth-equivalent success, not failure.