From 6e0f58051cb45041f0438181a269c6fd03573cdc Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 01:26:08 +0530 Subject: [PATCH 01/14] fix: prevent discovery eviction from disconnecting active sync peer --- src/Nethermind/Nethermind.Network/PeerPool.cs | 11 ++++++++++- src/Nethermind/Nethermind.Network/Rlpx/RlpxHost.cs | 10 ++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index 629be9fa089c..f1bbbf84af9f 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -74,7 +74,16 @@ public PeerPool( _nodeSource.NodeRemoved += NodeSourceOnNodeRemoved; } - private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) => TryRemove(e.Node.Id, out _); + private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) + { + if (Peers.TryGetValue(e.Node.Id, out Peer? peer) + && (peer.InSession is not null || peer.OutSession is not null)) + { + return; + } + + TryRemove(e.Node.Id, out _); + } public Peer GetOrAdd(Node node) => Peers.GetOrAdd(node.Id, valueFactory: _createNewNodePeer, (node, _staticPeers)); diff --git a/src/Nethermind/Nethermind.Network/Rlpx/RlpxHost.cs b/src/Nethermind/Nethermind.Network/Rlpx/RlpxHost.cs index c0a64eb92c41..9a3d4f071f36 100644 --- a/src/Nethermind/Nethermind.Network/Rlpx/RlpxHost.cs +++ b/src/Nethermind/Nethermind.Network/Rlpx/RlpxHost.cs @@ -107,7 +107,7 @@ public RlpxHost( _nodeFilter = NodeFilter.Create(networkConfig.MaxActivePeers, networkConfig.FilterPeersByRecentIp, networkConfig.FilterPeersBySameSubnet, ips.ExternalIp); } - public bool ShouldContact(IPAddress ip, bool exactOnly = false) => _nodeFilter.TryAccept(ip, exactOnly); + public bool ShouldContact(IPAddress ip, bool exactOnly = false) => _nodeFilter.CanAccept(ip, exactOnly); public async Task Init() { @@ -257,7 +257,7 @@ internal SessionActivitySubscription TrackSessionActivity(ISession session) /// /// Rejects inbound connections from IPs already seen within the filter window. - /// Outgoing connections are filtered earlier by before . + /// Outgoing connections reserve their filter slot in once the TCP channel is open. /// private bool ShouldRejectInbound(ISession session, IChannel channel) { @@ -291,6 +291,12 @@ private void InitializeChannel(IChannel channel, ISession session) return; } + if (session.Direction == ConnectionDirection.Out + && channel.RemoteAddress is IPEndPoint outboundEndpoint) + { + _nodeFilter.TryAccept(outboundEndpoint.Address); + } + SetTcpSocketOptions(channel); TrackSessionActivity(session); From 7d1133008258caef7aa2ff3b88e6cf99f760cddb Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 01:51:56 +0530 Subject: [PATCH 02/14] revert: remove false positive NodeFilter fix from RlpxHost --- src/Nethermind/Nethermind.Network/Rlpx/RlpxHost.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/Nethermind/Nethermind.Network/Rlpx/RlpxHost.cs b/src/Nethermind/Nethermind.Network/Rlpx/RlpxHost.cs index 9a3d4f071f36..c0a64eb92c41 100644 --- a/src/Nethermind/Nethermind.Network/Rlpx/RlpxHost.cs +++ b/src/Nethermind/Nethermind.Network/Rlpx/RlpxHost.cs @@ -107,7 +107,7 @@ public RlpxHost( _nodeFilter = NodeFilter.Create(networkConfig.MaxActivePeers, networkConfig.FilterPeersByRecentIp, networkConfig.FilterPeersBySameSubnet, ips.ExternalIp); } - public bool ShouldContact(IPAddress ip, bool exactOnly = false) => _nodeFilter.CanAccept(ip, exactOnly); + public bool ShouldContact(IPAddress ip, bool exactOnly = false) => _nodeFilter.TryAccept(ip, exactOnly); public async Task Init() { @@ -257,7 +257,7 @@ internal SessionActivitySubscription TrackSessionActivity(ISession session) /// /// Rejects inbound connections from IPs already seen within the filter window. - /// Outgoing connections reserve their filter slot in once the TCP channel is open. + /// Outgoing connections are filtered earlier by before . /// private bool ShouldRejectInbound(ISession session, IChannel channel) { @@ -291,12 +291,6 @@ private void InitializeChannel(IChannel channel, ISession session) return; } - if (session.Direction == ConnectionDirection.Out - && channel.RemoteAddress is IPEndPoint outboundEndpoint) - { - _nodeFilter.TryAccept(outboundEndpoint.Address); - } - SetTcpSocketOptions(channel); TrackSessionActivity(session); From d9a5a13c491bea231ca968b419e7cd6bde59b58e Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 02:12:10 +0530 Subject: [PATCH 03/14] fix: race conditions --- src/Nethermind/Nethermind.Network/Peer.cs | 7 +++++-- .../Nethermind.Network/PeerManager.cs | 17 ++++++++++------- src/Nethermind/Nethermind.Network/PeerPool.cs | 12 +++++++++--- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/src/Nethermind/Nethermind.Network/Peer.cs b/src/Nethermind/Nethermind.Network/Peer.cs index 5df8d0b3d53e..57c82df41ad7 100644 --- a/src/Nethermind/Nethermind.Network/Peer.cs +++ b/src/Nethermind/Nethermind.Network/Peer.cs @@ -30,15 +30,18 @@ public sealed class Peer(Node node, INodeStats? stats = null) : IEquatable internal INodeStats? Stats { get; } = stats; + private volatile ISession? _inSession; + private volatile ISession? _outSession; + /// /// An incoming session to the Node which can be in one of many states. /// - public ISession? InSession { get; set; } + public ISession? InSession { get => _inSession; set => _inSession = value; } /// /// An outgoing session to the Node which can be in one of many states. /// - public ISession? OutSession { get; set; } + public ISession? OutSession { get => _outSession; set => _outSession = value; } public override string ToString() => $"[Peer|{Node:s}|{InSession}|{OutSession}]"; diff --git a/src/Nethermind/Nethermind.Network/PeerManager.cs b/src/Nethermind/Nethermind.Network/PeerManager.cs index 7c405d90563a..4cc8c50c2290 100644 --- a/src/Nethermind/Nethermind.Network/PeerManager.cs +++ b/src/Nethermind/Nethermind.Network/PeerManager.cs @@ -1075,14 +1075,17 @@ private bool CanAttachSessionDirectly(ISession session, Peer peer) private void AttachSession(Peer peer, ISession session, ConnectionDirection sessionDirection, bool disconnectOpposite) { - if (sessionDirection == ConnectionDirection.In) + lock (peer) { - peer.Stats.AddNodeStatsHandshakeEvent(ConnectionDirection.In); - peer.InSession = session; - } - else - { - peer.OutSession = session; + if (sessionDirection == ConnectionDirection.In) + { + peer.Stats.AddNodeStatsHandshakeEvent(ConnectionDirection.In); + peer.InSession = session; + } + else + { + peer.OutSession = session; + } } if (disconnectOpposite) diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index f1bbbf84af9f..f97661caea59 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -76,13 +76,19 @@ public PeerPool( private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) { - if (Peers.TryGetValue(e.Node.Id, out Peer? peer) - && (peer.InSession is not null || peer.OutSession is not null)) + if (!Peers.TryGetValue(e.Node.Id, out Peer? peer)) { + TryRemove(e.Node.Id, out _); return; } - TryRemove(e.Node.Id, out _); + lock (peer) + { + if (peer.InSession is not null || peer.OutSession is not null) + return; + + TryRemove(e.Node.Id, out _); + } } public Peer GetOrAdd(Node node) => Peers.GetOrAdd(node.Id, valueFactory: _createNewNodePeer, (node, _staticPeers)); From 31b2c4426c95735a0b64d70a8f46db2d19080701 Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 02:25:33 +0530 Subject: [PATCH 04/14] only remove when kamelia --- src/Nethermind/Nethermind.Network/PeerPool.cs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index f97661caea59..d932cfc63870 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -17,6 +17,7 @@ using Nethermind.Logging; using Nethermind.Network.Config; using Nethermind.Network.P2P; +using Nethermind.Network.StaticNodes; using Nethermind.Stats; using Nethermind.Stats.Model; @@ -76,19 +77,17 @@ public PeerPool( private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) { - if (!Peers.TryGetValue(e.Node.Id, out Peer? peer)) + if (sender is not StaticNodesManager and not TrustedNodesManager + && Peers.TryGetValue(e.Node.Id, out Peer? peer)) { - TryRemove(e.Node.Id, out _); - return; + lock (peer) + { + if (peer.InSession is not null || peer.OutSession is not null) + return; + } } - lock (peer) - { - if (peer.InSession is not null || peer.OutSession is not null) - return; - - TryRemove(e.Node.Id, out _); - } + TryRemove(e.Node.Id, out _); } public Peer GetOrAdd(Node node) => Peers.GetOrAdd(node.Id, valueFactory: _createNewNodePeer, (node, _staticPeers)); From 740e536daf8c1e95c4f122186126bc2c4709e3ce Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 02:33:04 +0530 Subject: [PATCH 05/14] fix race condition & tests --- src/Nethermind/Nethermind.Network/PeerPool.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index d932cfc63870..d28c1a01a71f 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -17,7 +17,7 @@ using Nethermind.Logging; using Nethermind.Network.Config; using Nethermind.Network.P2P; -using Nethermind.Network.StaticNodes; + using Nethermind.Stats; using Nethermind.Stats.Model; @@ -77,12 +77,12 @@ public PeerPool( private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) { - if (sender is not StaticNodesManager and not TrustedNodesManager + if (sender is not IStaticNodesManager and not ITrustedNodesManager && Peers.TryGetValue(e.Node.Id, out Peer? peer)) { lock (peer) { - if (peer.InSession is not null || peer.OutSession is not null) + if (peer.InSession is not null || peer.OutSession is not null || peer.IsAwaitingConnection) return; } } From a0e62d52cda723a2fe0e8a43ada981452f28fa59 Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 02:34:13 +0530 Subject: [PATCH 06/14] fix race condition & tests --- src/Nethermind/Nethermind.Network/PeerPool.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index d28c1a01a71f..2fec8d5f999b 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -77,8 +77,10 @@ public PeerPool( private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) { - if (sender is not IStaticNodesManager and not ITrustedNodesManager - && Peers.TryGetValue(e.Node.Id, out Peer? peer)) + if (!Peers.TryGetValue(e.Node.Id, out Peer? peer)) + return; + + if (sender is not IStaticNodesManager and not ITrustedNodesManager) { lock (peer) { From 453baaded16f55a0e93f5d033bb29ce11ad78780 Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 02:39:59 +0530 Subject: [PATCH 07/14] fix race condition & tests --- src/Nethermind/Nethermind.Network/PeerPool.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index 2fec8d5f999b..868590a93120 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -17,7 +17,6 @@ using Nethermind.Logging; using Nethermind.Network.Config; using Nethermind.Network.P2P; - using Nethermind.Stats; using Nethermind.Stats.Model; @@ -86,7 +85,10 @@ private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) { if (peer.InSession is not null || peer.OutSession is not null || peer.IsAwaitingConnection) return; + + TryRemove(e.Node.Id, out _); } + return; } TryRemove(e.Node.Id, out _); From 6aaeda3666a7ab033df31294927b2f8a215145f9 Mon Sep 17 00:00:00 2001 From: Sambhav Jain <136801346+DarkLord017@users.noreply.github.com> Date: Tue, 23 Jun 2026 02:47:56 +0530 Subject: [PATCH 08/14] Update src/Nethermind/Nethermind.Network/PeerPool.cs Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> --- src/Nethermind/Nethermind.Network/PeerPool.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index 868590a93120..fea90b7abb64 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -81,7 +81,13 @@ private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) if (sender is not IStaticNodesManager and not ITrustedNodesManager) { - lock (peer) + // Lock peer to make the session check and TryRemove atomic against AttachSession. + // If a session is being concurrently assigned, AttachSession also holds lock(peer), + // so only one thread will see null sessions and call TryRemove. Residual: TryGetValue + // is outside the lock, so a session assigned between TryGetValue and lock acquisition + // leaves the peer alive in ActivePeers but absent from Peers until rediscovery — + // strictly better than the original unconditional disconnect. + lock (peer) { if (peer.InSession is not null || peer.OutSession is not null || peer.IsAwaitingConnection) return; From 27a80d46ec6d1557d3fc193cb42c2dd3da1613c0 Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 02:50:14 +0530 Subject: [PATCH 09/14] fix linting --- src/Nethermind/Nethermind.Network/PeerPool.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index fea90b7abb64..6972e5d0c459 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -81,13 +81,13 @@ private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) if (sender is not IStaticNodesManager and not ITrustedNodesManager) { - // Lock peer to make the session check and TryRemove atomic against AttachSession. - // If a session is being concurrently assigned, AttachSession also holds lock(peer), - // so only one thread will see null sessions and call TryRemove. Residual: TryGetValue - // is outside the lock, so a session assigned between TryGetValue and lock acquisition - // leaves the peer alive in ActivePeers but absent from Peers until rediscovery — - // strictly better than the original unconditional disconnect. - lock (peer) + // Lock peer to make the session check and TryRemove atomic against AttachSession. + // If a session is being concurrently assigned, AttachSession also holds lock(peer), + // so only one thread will see null sessions and call TryRemove. Residual: TryGetValue + // is outside the lock, so a session assigned between TryGetValue and lock acquisition + // leaves the peer alive in ActivePeers but absent from Peers until rediscovery — + // strictly better than the original unconditional disconnect. + lock (peer) { if (peer.InSession is not null || peer.OutSession is not null || peer.IsAwaitingConnection) return; From fed6fc5d975f2380c3584f6b3dab2c2d9efd05b1 Mon Sep 17 00:00:00 2001 From: Sambhav Jain <136801346+DarkLord017@users.noreply.github.com> Date: Tue, 23 Jun 2026 17:13:12 +0530 Subject: [PATCH 10/14] Update src/Nethermind/Nethermind.Network/PeerPool.cs Co-authored-by: Lukasz Rozmej --- src/Nethermind/Nethermind.Network/PeerPool.cs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index 6972e5d0c459..868590a93120 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -81,12 +81,6 @@ private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) if (sender is not IStaticNodesManager and not ITrustedNodesManager) { - // Lock peer to make the session check and TryRemove atomic against AttachSession. - // If a session is being concurrently assigned, AttachSession also holds lock(peer), - // so only one thread will see null sessions and call TryRemove. Residual: TryGetValue - // is outside the lock, so a session assigned between TryGetValue and lock acquisition - // leaves the peer alive in ActivePeers but absent from Peers until rediscovery — - // strictly better than the original unconditional disconnect. lock (peer) { if (peer.InSession is not null || peer.OutSession is not null || peer.IsAwaitingConnection) From 29b21e76a33c9313293b6d13e1c29fba1e6405c1 Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 19:52:41 +0530 Subject: [PATCH 11/14] use events instead of checking sender and other req changes & tests --- .../Modules/DiscoveryModule.cs | 1 + .../PeerManagerTests.cs | 2 +- .../Nethermind.Network.Test/PeerPoolTests.cs | 31 ++++++++++++++++++ .../Nethermind.Network/NodeEventArgs.cs | 6 ++++ .../Nethermind.Network/NodesManager.cs | 17 ++++++++++ src/Nethermind/Nethermind.Network/Peer.cs | 11 +++++-- .../Nethermind.Network/PeerManager.cs | 2 +- src/Nethermind/Nethermind.Network/PeerPool.cs | 23 +++++++------ .../StaticNodes/StaticNodesManager.cs | 12 ++----- .../Nethermind.Network/TrustedNodesManager.cs | 32 +++---------------- 10 files changed, 83 insertions(+), 54 deletions(-) diff --git a/src/Nethermind/Nethermind.Init/Modules/DiscoveryModule.cs b/src/Nethermind/Nethermind.Init/Modules/DiscoveryModule.cs index fbb5c369fdce..a5f328879a8d 100644 --- a/src/Nethermind/Nethermind.Init/Modules/DiscoveryModule.cs +++ b/src/Nethermind/Nethermind.Init/Modules/DiscoveryModule.cs @@ -49,6 +49,7 @@ protected override void Load(ContainerBuilder builder) new TrustedNodesManager(initConfig.TrustedNodesPath.GetApplicationResourcePath(initConfig.DataDir), logManager)) .Bind() + .Bind() // Used by NodesLoader, and ProtocolsManager which add entry on sync peer connected .AddNetworkStorage(DbNames.PeersDb, DbNames.PeersDb) diff --git a/src/Nethermind/Nethermind.Network.Test/PeerManagerTests.cs b/src/Nethermind/Nethermind.Network.Test/PeerManagerTests.cs index a2282a4abddd..61112405ed98 100644 --- a/src/Nethermind/Nethermind.Network.Test/PeerManagerTests.cs +++ b/src/Nethermind/Nethermind.Network.Test/PeerManagerTests.cs @@ -652,7 +652,7 @@ public async Task Will_disconnect_on_remove_static_node() void DisconnectHandler(object o, DisconnectEventArgs e) => disconnections++; ctx.Sessions.ForEach(s => s.Disconnected += DisconnectHandler); - ctx.StaticNodesManager.NodeRemoved += Raise.EventWith(new NodeEventArgs( + ctx.StaticNodesManager.NodeRemoved += Raise.EventWith(new ExplicitNodeRemovalEventArgs( new Node(staticNodes.First()))); Assert.That(ctx.PeerManager.ActivePeers.Count(p => p.Node.IsStatic), Is.EqualTo(nodesCount - 1)); diff --git a/src/Nethermind/Nethermind.Network.Test/PeerPoolTests.cs b/src/Nethermind/Nethermind.Network.Test/PeerPoolTests.cs index d9339a6aefb6..7bb20f24bc7a 100644 --- a/src/Nethermind/Nethermind.Network.Test/PeerPoolTests.cs +++ b/src/Nethermind/Nethermind.Network.Test/PeerPoolTests.cs @@ -184,6 +184,37 @@ public async Task PeerPool_ShouldThrottleSource_WhenActivePeerPoolIsFull() } } + [Test] + public void PeerPool_DiscoveryEviction_KeepsPeer_WhenSessionActive() + { + ITrustedNodesManager trustedNodesManager = Substitute.For(); + TestNodeSource nodeSource = new(); + PeerPool pool = CreatePeerPool(nodeSource, trustedNodesManager, maxActivePeers: 10, maxCandidatePeerCount: 10); + + Node node = new(TestItem.PublicKeyA, "1.2.3.4", 1234); + Peer peer = pool.GetOrAdd(node); + peer.InSession = Substitute.For(); + + nodeSource.RemoveNode(node); + + Assert.That(pool.TryGet(node.Id, out _), Is.True); + } + + [Test] + public void PeerPool_DiscoveryEviction_RemovesPeer_WhenNoSession() + { + ITrustedNodesManager trustedNodesManager = Substitute.For(); + TestNodeSource nodeSource = new(); + PeerPool pool = CreatePeerPool(nodeSource, trustedNodesManager, maxActivePeers: 10, maxCandidatePeerCount: 10); + + Node node = new(TestItem.PublicKeyA, "1.2.3.4", 1234); + pool.GetOrAdd(node); + + nodeSource.RemoveNode(node); + + Assert.That(pool.TryGet(node.Id, out _), Is.False); + } + [Test] public void PeerPool_Replace_DoesNotInheritStaticFlag() { diff --git a/src/Nethermind/Nethermind.Network/NodeEventArgs.cs b/src/Nethermind/Nethermind.Network/NodeEventArgs.cs index 458f981771f9..390023f6c7b8 100644 --- a/src/Nethermind/Nethermind.Network/NodeEventArgs.cs +++ b/src/Nethermind/Nethermind.Network/NodeEventArgs.cs @@ -10,4 +10,10 @@ public class NodeEventArgs(Node node) : EventArgs { public Node Node { get; } = node; } + + /// + /// Raised when a node is explicitly removed by the operator (e.g. via admin_removePeer). + /// Receivers must disconnect any active P2P session unconditionally. + /// + public class ExplicitNodeRemovalEventArgs(Node node) : NodeEventArgs(node); } diff --git a/src/Nethermind/Nethermind.Network/NodesManager.cs b/src/Nethermind/Nethermind.Network/NodesManager.cs index fa6dfc5740f5..194bf82ec25b 100644 --- a/src/Nethermind/Nethermind.Network/NodesManager.cs +++ b/src/Nethermind/Nethermind.Network/NodesManager.cs @@ -13,6 +13,7 @@ using Nethermind.Core.Crypto; using Nethermind.Logging; using Nethermind.Serialization.Json; +using Nethermind.Stats.Model; namespace Nethermind.Network; @@ -132,6 +133,22 @@ protected virtual async Task> Parse return nodes; } + public event EventHandler? NodeRemoved; + + /// + /// Removes a node from the in-memory store and fires as an + /// so downstream listeners (e.g. PeerPool) + /// know to disconnect the peer unconditionally. + /// + protected bool TryRemoveNode(PublicKey nodeId) + { + if (!_nodes.TryRemove(nodeId, out NetworkNode? removed)) + return false; + + NodeRemoved?.Invoke(this, new ExplicitNodeRemovalEventArgs(new Node(removed))); + return true; + } + protected virtual Task SaveFileAsync(CancellationToken cancellationToken = default) { ArgumentException.ThrowIfNullOrWhiteSpace(path); diff --git a/src/Nethermind/Nethermind.Network/Peer.cs b/src/Nethermind/Nethermind.Network/Peer.cs index 57c82df41ad7..13464aefe303 100644 --- a/src/Nethermind/Nethermind.Network/Peer.cs +++ b/src/Nethermind/Nethermind.Network/Peer.cs @@ -20,8 +20,6 @@ namespace Nethermind.Network /// public sealed class Peer(Node node, INodeStats? stats = null) : IEquatable { - public bool IsAwaitingConnection { get; set; } - /// /// A physical network node with a network address combined with information about the client version /// and any extra attributes that we assign to a network node (static / trusted / bootnode). @@ -30,9 +28,18 @@ public sealed class Peer(Node node, INodeStats? stats = null) : IEquatable internal INodeStats? Stats { get; } = stats; + /// + /// Guards , , and + /// as a unit. Using a dedicated object avoids locking on a publicly visible instance. + /// + internal readonly object SessionLock = new(); + + private volatile bool _isAwaitingConnection; private volatile ISession? _inSession; private volatile ISession? _outSession; + public bool IsAwaitingConnection { get => _isAwaitingConnection; set => _isAwaitingConnection = value; } + /// /// An incoming session to the Node which can be in one of many states. /// diff --git a/src/Nethermind/Nethermind.Network/PeerManager.cs b/src/Nethermind/Nethermind.Network/PeerManager.cs index 4cc8c50c2290..8635758fc961 100644 --- a/src/Nethermind/Nethermind.Network/PeerManager.cs +++ b/src/Nethermind/Nethermind.Network/PeerManager.cs @@ -1075,7 +1075,7 @@ private bool CanAttachSessionDirectly(ISession session, Peer peer) private void AttachSession(Peer peer, ISession session, ConnectionDirection sessionDirection, bool disconnectOpposite) { - lock (peer) + lock (peer.SessionLock) { if (sessionDirection == ConnectionDirection.In) { diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index 6972e5d0c459..8bd1d4a4b7e8 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -79,21 +79,20 @@ private void NodeSourceOnNodeRemoved(object? sender, NodeEventArgs e) if (!Peers.TryGetValue(e.Node.Id, out Peer? peer)) return; - if (sender is not IStaticNodesManager and not ITrustedNodesManager) + if (e is not ExplicitNodeRemovalEventArgs) { - // Lock peer to make the session check and TryRemove atomic against AttachSession. - // If a session is being concurrently assigned, AttachSession also holds lock(peer), - // so only one thread will see null sessions and call TryRemove. Residual: TryGetValue - // is outside the lock, so a session assigned between TryGetValue and lock acquisition - // leaves the peer alive in ActivePeers but absent from Peers until rediscovery — - // strictly better than the original unconditional disconnect. - lock (peer) + // Only remove the peer if no P2P session is active. + // The dictionary removals are done inside SessionLock so the session check and + // removal are atomic against AttachSession. PeerRemoved is fired outside the lock + // to avoid holding it across arbitrary event handler code. + bool removed; + lock (peer.SessionLock) { - if (peer.InSession is not null || peer.OutSession is not null || peer.IsAwaitingConnection) - return; - - TryRemove(e.Node.Id, out _); + removed = peer.InSession is null && peer.OutSession is null && !peer.IsAwaitingConnection + && Peers.TryRemove(e.Node.Id, out _); + if (removed) _staticPeers.TryRemove(e.Node.Id, out _); } + if (removed) PeerRemoved?.Invoke(this, new PeerEventArgs(peer)); return; } diff --git a/src/Nethermind/Nethermind.Network/StaticNodes/StaticNodesManager.cs b/src/Nethermind/Nethermind.Network/StaticNodes/StaticNodesManager.cs index 7d91f9293c87..fb5434712fd6 100644 --- a/src/Nethermind/Nethermind.Network/StaticNodes/StaticNodesManager.cs +++ b/src/Nethermind/Nethermind.Network/StaticNodes/StaticNodesManager.cs @@ -52,20 +52,14 @@ public async Task AddAsync(NetworkNode networkNode, bool updateFile = true public async Task RemoveAsync(NetworkNode networkNode, bool updateFile = true, CancellationToken cancellationToken = default) { - if (!_nodes.TryRemove(networkNode.NodeId, out _)) + if (!TryRemoveNode(networkNode.NodeId)) { if (_logger.IsInfo) _logger.Info($"Static node was not found: {networkNode}"); return false; } if (_logger.IsInfo) _logger.Info($"Static node was removed: {networkNode}"); - Node node = new(networkNode); - NodeRemoved?.Invoke(this, new NodeEventArgs(node)); - if (updateFile) - { - await SaveFileAsync(cancellationToken); - } - + if (updateFile) await SaveFileAsync(cancellationToken); return true; } @@ -101,6 +95,4 @@ public async IAsyncEnumerable DiscoverNodes([EnumeratorCancellation] Cance } private event EventHandler? NodeAdded; - - public event EventHandler? NodeRemoved; } diff --git a/src/Nethermind/Nethermind.Network/TrustedNodesManager.cs b/src/Nethermind/Nethermind.Network/TrustedNodesManager.cs index 206c26f15b65..a6494c3d041f 100644 --- a/src/Nethermind/Nethermind.Network/TrustedNodesManager.cs +++ b/src/Nethermind/Nethermind.Network/TrustedNodesManager.cs @@ -84,40 +84,16 @@ public async Task AddAsync(Enode enode, bool updateFile = true, Cancellati public async Task RemoveAsync(Enode enode, bool updateFile = true, CancellationToken cancellationToken = default) { NetworkNode networkNode = new(enode); - if (!_nodes.TryRemove(networkNode.NodeId, out _)) + if (!TryRemoveNode(networkNode.NodeId)) { - if (_logger.IsInfo) - { - _logger.Info($"Trusted node was not found: {enode}"); - } + if (_logger.IsInfo) _logger.Info($"Trusted node was not found: {enode}"); return false; } - if (_logger.IsInfo) - { - _logger.Info($"Trusted node was removed: {enode}"); - } - - // Fire NodeRemoved (drives PeerPool disconnect via the event chain) BEFORE the file write, - // so a cancelled SaveFileAsync cannot leave the peer untrusted-in-memory yet still connected. - // Mirrors StaticNodesManager.RemoveAsync ordering. - OnNodeRemoved(networkNode); - - if (updateFile) - { - await SaveFileAsync(cancellationToken); - } - + if (_logger.IsInfo) _logger.Info($"Trusted node was removed: {enode}"); + if (updateFile) await SaveFileAsync(cancellationToken); return true; } public bool IsTrusted(Enode enode) => _nodes.ContainsKey(enode.PublicKey); - - public event EventHandler? NodeRemoved; - - private void OnNodeRemoved(NetworkNode node) - { - Node nodeForEvent = new(node); - NodeRemoved?.Invoke(this, new NodeEventArgs(nodeForEvent)); - } } From 659a5f266189de90f4d276c2fc0db300cb2dd7c6 Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 20:01:12 +0530 Subject: [PATCH 12/14] linting --- src/Nethermind/Nethermind.Network/TrustedNodesManager.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Nethermind/Nethermind.Network/TrustedNodesManager.cs b/src/Nethermind/Nethermind.Network/TrustedNodesManager.cs index a6494c3d041f..559811df32d2 100644 --- a/src/Nethermind/Nethermind.Network/TrustedNodesManager.cs +++ b/src/Nethermind/Nethermind.Network/TrustedNodesManager.cs @@ -5,7 +5,6 @@ using Nethermind.Core.Crypto; using Nethermind.Logging; using Nethermind.Stats.Model; -using System; using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; @@ -84,6 +83,8 @@ public async Task AddAsync(Enode enode, bool updateFile = true, Cancellati public async Task RemoveAsync(Enode enode, bool updateFile = true, CancellationToken cancellationToken = default) { NetworkNode networkNode = new(enode); + // Fire NodeRemoved BEFORE the file write: a cancelled SaveFileAsync must not leave + // the peer disconnected in-memory but still persisted as trusted. if (!TryRemoveNode(networkNode.NodeId)) { if (_logger.IsInfo) _logger.Info($"Trusted node was not found: {enode}"); From befc7fda2521031b8dcdc67212c5a1450fdbc315 Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 20:19:50 +0530 Subject: [PATCH 13/14] merge to cases --- .../Nethermind.Network.Test/PeerPoolTests.cs | 24 ++++--------------- src/Nethermind/Nethermind.Network/PeerPool.cs | 13 +++++----- 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/src/Nethermind/Nethermind.Network.Test/PeerPoolTests.cs b/src/Nethermind/Nethermind.Network.Test/PeerPoolTests.cs index 7bb20f24bc7a..794dcf0aa0cb 100644 --- a/src/Nethermind/Nethermind.Network.Test/PeerPoolTests.cs +++ b/src/Nethermind/Nethermind.Network.Test/PeerPoolTests.cs @@ -184,8 +184,9 @@ public async Task PeerPool_ShouldThrottleSource_WhenActivePeerPoolIsFull() } } - [Test] - public void PeerPool_DiscoveryEviction_KeepsPeer_WhenSessionActive() + [TestCase(true, true)] // active session → peer kept + [TestCase(false, false)] // no session → peer evicted + public void PeerPool_DiscoveryEviction(bool hasSession, bool expectPresent) { ITrustedNodesManager trustedNodesManager = Substitute.For(); TestNodeSource nodeSource = new(); @@ -193,26 +194,11 @@ public void PeerPool_DiscoveryEviction_KeepsPeer_WhenSessionActive() Node node = new(TestItem.PublicKeyA, "1.2.3.4", 1234); Peer peer = pool.GetOrAdd(node); - peer.InSession = Substitute.For(); - - nodeSource.RemoveNode(node); - - Assert.That(pool.TryGet(node.Id, out _), Is.True); - } - - [Test] - public void PeerPool_DiscoveryEviction_RemovesPeer_WhenNoSession() - { - ITrustedNodesManager trustedNodesManager = Substitute.For(); - TestNodeSource nodeSource = new(); - PeerPool pool = CreatePeerPool(nodeSource, trustedNodesManager, maxActivePeers: 10, maxCandidatePeerCount: 10); - - Node node = new(TestItem.PublicKeyA, "1.2.3.4", 1234); - pool.GetOrAdd(node); + if (hasSession) peer.InSession = Substitute.For(); nodeSource.RemoveNode(node); - Assert.That(pool.TryGet(node.Id, out _), Is.False); + Assert.That(pool.TryGet(node.Id, out _), Is.EqualTo(expectPresent)); } [Test] diff --git a/src/Nethermind/Nethermind.Network/PeerPool.cs b/src/Nethermind/Nethermind.Network/PeerPool.cs index 8bd1d4a4b7e8..7883f5005239 100644 --- a/src/Nethermind/Nethermind.Network/PeerPool.cs +++ b/src/Nethermind/Nethermind.Network/PeerPool.cs @@ -137,18 +137,19 @@ private Peer CreateNew(PublicKeyAsKey key, (NetworkNode Node, ConcurrentDictiona public bool TryRemove(PublicKey id, out Peer peer) { - if (Peers.TryRemove(id, out peer)) + if (!Peers.TryRemove(id, out peer)) + return false; + + _staticPeers.TryRemove(id, out _); + lock (peer.SessionLock) { - _staticPeers.TryRemove(id, out _); peer.InSession?.MarkDisconnected(DisconnectReason.PeerRemoved, DisconnectType.Local, "admin_removePeer"); peer.OutSession?.MarkDisconnected(DisconnectReason.PeerRemoved, DisconnectType.Local, "admin_removePeer"); peer.InSession = null; peer.OutSession = null; - PeerRemoved?.Invoke(this, new PeerEventArgs(peer)); - return true; } - - return false; + PeerRemoved?.Invoke(this, new PeerEventArgs(peer)); + return true; } public Peer Replace(ISession session) From 7b75f99ad84df3b5569ae9337b809d51b2eadebd Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Tue, 23 Jun 2026 20:26:22 +0530 Subject: [PATCH 14/14] add summary --- src/Nethermind/Nethermind.Network/NodesManager.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Nethermind/Nethermind.Network/NodesManager.cs b/src/Nethermind/Nethermind.Network/NodesManager.cs index 194bf82ec25b..679f513d54f6 100644 --- a/src/Nethermind/Nethermind.Network/NodesManager.cs +++ b/src/Nethermind/Nethermind.Network/NodesManager.cs @@ -133,6 +133,9 @@ protected virtual async Task> Parse return nodes; } + /// + /// Raised when a node is explicitly removed from this source. + /// public event EventHandler? NodeRemoved; ///