Deep review

Findings below are observations, not pushed changes — please decide which to apply.

Important (real defects, but not blockers)

1. KBucketTree.cs:59,153 — events fire under McsLock. OnNodeAdded / OnNodeRemoved are invoked synchronously while _lock is held. The internal subscriber (KademliaNodeSource.Handler) uses non-blocking Channel.Writer.TryWrite, but DiscoveryApp.OnKademliaNodeRemoved re-raises a public NodeRemoved event with external (peer-manager) subscribers — any one re-entering the tree deadlocks. Fix: capture the node, release the using scope, then Invoke.

2. LookupKNearestNeighbour.cs:65,107 — racy TaskCompletionSource reassignment, and the token is misused. Multiple workers race on if (roundComplete.TrySetResult()) roundComplete = new TaskCompletionSource(token); outside any lock. Also new TaskCompletionSource(token) treats CancellationToken as opaque state — cancellation is not actually wired in. Fix: Interlocked.Exchange for the swap, and drop the (token) arg or register cancellation via RegisterToCompletionSource.

3. Kademlia.cs:103-115 — Bootstrap only catches OperationCanceledException. Any non-cancellation exception from Ping (transient network error, etc.) propagates through Parallel.ForEachAsync, kills Bootstrap, kills Run's while (true), and lands in ActivateAsync.catch(Exception) (DiscoveryApp.cs:244). Discovery then dies for the rest of the process lifetime. Wrap individual pings the way IteratorNodeLookup.FindNeighbour does, or catch+log inside Run's outer loop and continue.

4. EnrResponseHandler.cs:12 — accepts any unsolicited ENR response. Unlike PongMsgHandler (which validates pong.PingMdc == ping.Mdc), this handler takes the first ENR seen on the (nodeId, EnrResponse) key. EnrResponseMsg already carries RequestKeccak; capture the request hash on send and compare.

5. KademliaNodeSource.cs:118-122 — finally awaits discoverTask before unsubscribing. If anything inside the consumer await foreach throws before cancellation, the producer keeps running and OnNodeAdded stays subscribed until cancel. Either unsubscribe first, or ch.Writer.TryComplete() before the await.

6. KBucket.GetAllWithHash() / DoubleEndedLru.GetAllWithHash() return the live LinkedList<>. All current callers happen to go through KBucketTree._lock, but IRoutingTable.IterateBuckets() exposes KBucket to the outside — a future caller iterating outside the lock will hit InvalidOperationException: Collection was modified. Either materialize under lock or document the lock requirement.

Nits

• Typo on public surface: LookupFindNeighbourHardTimout in KademliaConfig and DiscV4KademliaModule.cs:43 — rename to …Timeout.
• KademliaNodeSource ctor param lookup2 — leftover name from refactor.
• KBucketTree.GetAllAtDistanceRecursive and KBucketTree.GetKNearestNeighbour use LINQ .Where/.Select/.Take chains on hot lookup paths — convert to manual foreach per project style (.agents/rules/coding-style.md rule: no LINQ on hot paths).
• KBucketTree.cs:130 .Reverse() on IEnumerable allocates a buffer on every split.
• Hash256XorUtils.MaxDistance can be public const int instead of a property.
• Kademlia.cs:61 comment: "logid" → "logic".
• KademliaDiscv4Adapter.cs:175 comment: "does not seems" → "does not seem".

TODO assessment

Discv4/DiscV4KademliaModule.cs:43 — LookupFindNeighbourHardTimout = SendNodeTimeout; // TODO: This seems very low. — Doable now. SendNodeTimeout defaults to 500ms, used here as the per-FindNode hard timeout inside Kademlia lookup. At 500ms many slow peers get cut off before they respond. Add a dedicated KademliaLookupHardTimeoutMs on IDiscoveryConfig (default ~2000ms), or use 2 * SendNodeTimeout. Combine with the Timout typo rename.

Kademlia/Hash256XorUtils.cs:70 — // TODO: Just add a min/max range per bucket and randomized between them. — Defer. Needs threading per-bucket prefix ranges through IRoutingTable/KBucketTree and changing IKeyOperator.CreateRandomKeyAtDistance signatures. No correctness risk today — the function is covered by Hash256XorUtilsTests.TestGetRandomHash. Code-shape TODO, follow-up.

Kademlia/KBucketTree.cs:28 — // TODO: Double check and probably make lockless. — Defer. Non-trivial lockfree design, would need a lockless DoubleEndedLru and new concurrent stress tests. The lock itself is correct; throughput is the concern. But the concrete defect the TODO hints at — event invocation under the lock — is Important #1 above and can be fixed independently of going lockless.

Test coverage gaps worth filling

• KBucketTree.SplitBucket LRU order preservation (the .Reverse() claim) — no assertion.
• KBucketTree.GetAllAtDistance depth > targetDepth "collect all leaves" branch — untested.
• NodeHealthTracker.TryRefresh — entire Task.Run path (ping head → refresh on success → evict on failure) not exercised.
• LookupKNearestNeighbour — no mid-flight cancellation test; only one Alpha value implicitly exercised.
• KademliaDiscv4Adapter.RemoveMessageHandler — CAS retry loop never stressed with concurrent add/remove.
• EnrResponseHandler — no test that an unsolicited ENR response is rejected (would catch Important #4).
• DiscoveryPersistenceManager.LoadPersistedNodes — Node construction throwing and ping throwing paths not asserted to skip and continue.