[Fix] Improve peer bootstrap connections #4263

Jim8y · 2025-11-01T15:21:36Z

Description

Restore healthy bootstrap behaviour after the stash fix by letting
Peer.OnTimer() pull a fuller batch of seed peers before honoring low
MinDesiredConnections. That way private-net consensus still converges
without depending on the accidental disconnects the race condition used to
cause.

Fixes # N/A

Type of change

Bug fix (non-breaking change which fixes an issue)
Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or
standard purpose)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality
to not work as expected)
This change requires a documentation update

Test Configuration:
dotnet test tests/Neo.UnitTests/Neo.UnitTests.csproj -c Release (fails
before running: MSBuild cannot bind its named pipe in this sandbox—
SocketException (permission denied)).

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature
works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream
modules

src/Neo/Network/P2P/Peer.cs

Co-authored-by: Shargon <[email protected]>

Copilot

Pull Request Overview

This PR refactors the OnTimer method in the Peer class to improve the logic for managing peer connections and adds a test case to verify that unconnected peers are not drained when the connecting capacity is zero.

Refactored OnTimer method to use a more sophisticated connection management strategy with multiple capacity checks
Added logic to handle bootstrap scenarios when there are no connected peers
Introduced a test case to verify behavior when connecting capacity is exhausted

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/Neo/Network/P2P/Peer.cs	Refactored `OnTimer` method with improved connection capacity logic and multiple guard clauses
tests/Neo.UnitTests/Network/P2P/UT_LocalNode.cs	Added test case to verify unconnected peers aren't modified when connecting capacity is zero

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-04T09:12:57Z

src/Neo/Network/P2P/Peer.cs

+
+            var maxConnections = Config.MaxConnections < 0
+                ? int.MaxValue
+                : Config.MaxConnections - ConnectedPeers.Count;


Potential negative value if ConnectedPeers.Count > Config.MaxConnections. While the subsequent check on line 386 guards against this, it would be clearer to ensure maxConnections is never negative: Math.Max(0, Config.MaxConnections - ConnectedPeers.Count).

Suggested change

: Config.MaxConnections - ConnectedPeers.Count;

: Math.Max(0, Config.MaxConnections - ConnectedPeers.Count);

Copilot · 2025-11-04T09:12:57Z

tests/Neo.UnitTests/Network/P2P/UT_LocalNode.cs

+            typeof(Peer).GetField("UnconnectedPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)
+                .SetValue(localNode, unconnected);
+            typeof(Peer).GetField("ConnectingPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)
+                .SetValue(localNode, connecting);


[nitpick] Repeated reflection binding flags should be extracted into a constant to reduce duplication and improve readability. Consider: const BindingFlags privateInstance = BindingFlags.Instance | BindingFlags.NonPublic;

Copilot · 2025-11-04T09:12:57Z

tests/Neo.UnitTests/Network/P2P/UT_LocalNode.cs

+            typeof(Peer).GetField("UnconnectedPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)
+                .SetValue(localNode, unconnected);
+            typeof(Peer).GetField("ConnectingPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)
+                .SetValue(localNode, connecting);
+
+            typeof(Peer).GetMethod("OnTimer", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)
+                .Invoke(localNode, null);
+
+            var updatedUnconnected = (ImmutableHashSet<IPEndPoint>)typeof(Peer)
+                .GetField("UnconnectedPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)
+                .GetValue(localNode);
+            var updatedConnecting = (ImmutableHashSet<IPEndPoint>)typeof(Peer)
+                .GetField("ConnectingPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)
+                .GetValue(localNode);


Missing null-check on reflection result. GetField can return null if the field doesn't exist. Consider adding null-forgiving operator or assertion: typeof(Peer).GetField(...)?.SetValue(...) or add an assertion after retrieval to fail early if the field is not found.

Suggested change

typeof(Peer).GetField("UnconnectedPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)

.SetValue(localNode, unconnected);

typeof(Peer).GetField("ConnectingPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)

.SetValue(localNode, connecting);

typeof(Peer).GetMethod("OnTimer", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)

.Invoke(localNode, null);

var updatedUnconnected = (ImmutableHashSet<IPEndPoint>)typeof(Peer)

.GetField("UnconnectedPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)

.GetValue(localNode);

var updatedConnecting = (ImmutableHashSet<IPEndPoint>)typeof(Peer)

.GetField("ConnectingPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)

.GetValue(localNode);

var unconnectedPeersField = typeof(Peer).GetField("UnconnectedPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic);

Assert.IsNotNull(unconnectedPeersField, "Field 'UnconnectedPeers' not found in Peer");

unconnectedPeersField.SetValue(localNode, unconnected);

var connectingPeersField = typeof(Peer).GetField("ConnectingPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic);

Assert.IsNotNull(connectingPeersField, "Field 'ConnectingPeers' not found in Peer");

connectingPeersField.SetValue(localNode, connecting);

typeof(Peer).GetMethod("OnTimer", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)

.Invoke(localNode, null);

var unconnectedPeersField2 = typeof(Peer).GetField("UnconnectedPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic);

Assert.IsNotNull(unconnectedPeersField2, "Field 'UnconnectedPeers' not found in Peer");

var updatedUnconnected = (ImmutableHashSet<IPEndPoint>)unconnectedPeersField2.GetValue(localNode);

var connectingPeersField2 = typeof(Peer).GetField("ConnectingPeers", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic);

Assert.IsNotNull(connectingPeersField2, "Field 'ConnectingPeers' not found in Peer");

var updatedConnecting = (ImmutableHashSet<IPEndPoint>)connectingPeersField2.GetValue(localNode);

shargon

I think this should go to master

vncoelho · 2025-11-04T11:50:48Z

src/Neo/Network/P2P/Peer.cs

+            if (deficit > 0 && UnconnectedPeers.Count == 0)
+            {
+                NeedMorePeers(deficit);
+                if (UnconnectedPeers.Count == 0)


Is NeedMorePeers a Sync function?
I think assyncrounous I think. So, the if check does not would make much sense here, it was just verified above.

vncoelho · 2025-11-04T11:51:58Z

src/Neo/Network/P2P/Peer.cs

-                NeedMorePeers(Config.MinDesiredConnections - ConnectedPeers.Count);
+                return;
+
+            var maxConnections = Config.MaxConnections < 0


Config.MaxConnections < 0, how can this can lower than 0?

To me maxConnections here should be something like currentMaxConnectionsLimit

vncoelho · 2025-11-04T11:52:37Z

src/Neo/Network/P2P/Peer.cs

+            var maxConnections = Config.MaxConnections < 0
+                ? int.MaxValue
+                : Config.MaxConnections - ConnectedPeers.Count;
+            if (maxConnections <= 0)


ConnectedPeers.Count should never be great than Config.MaxConnections, this case should not happen

src/Neo/Network/P2P/Peer.cs

vncoelho · 2025-11-04T11:59:26Z

src/Neo/Network/P2P/Peer.cs

+            if (connectingCapacity <= 0)
+                return;
+
+            var toConnect = Math.Max(deficit, 0);


if deficit is 0 and you reached here it is because ConnectingPeers.Count > 0

So, perhaps this needs to be considered

vncoelho · 2025-11-04T12:02:21Z

src/Neo/Network/P2P/Peer.cs

+                return;
+
+            var maxConnections = Config.MaxConnections < 0
+                ? int.MaxValue


int.MaxValue ?

vncoelho

The fix looks good @Jim8y, there are some checks and parameters that could improve and some are a little strange to me.

Jim8y · 2025-11-04T14:28:48Z

The fix looks good @Jim8y, there are some checks and parameters that could improve and some are a little strange to me.

i checked your comments, many of them are reasonable range checks, will udpate.

Improve peer bootstrap connections

4d45405

Wi1l-B0t reviewed Nov 1, 2025

View reviewed changes

src/Neo/Network/P2P/Peer.cs Outdated Show resolved Hide resolved

Merge branch 'dev' into fix/peer-bootstrap

ace6918

shargon reviewed Nov 1, 2025

View reviewed changes

src/Neo/Network/P2P/Peer.cs Outdated Show resolved Hide resolved

src/Neo/Network/P2P/Peer.cs Outdated Show resolved Hide resolved

Jim8y and others added 7 commits November 2, 2025 10:34

Merge branch 'dev' into fix/peer-bootstrap

b11f631

Merge branch 'dev' into fix/peer-bootstrap

029e23b

Update src/Neo/Network/P2P/Peer.cs

b1bead9

Co-authored-by: Shargon <[email protected]>

Update src/Neo/Network/P2P/Peer.cs

9bb938a

Co-authored-by: Shargon <[email protected]>

Adjust bootstrap connection capacity

1d3aefe

Add unit test for connection capacity handling

d33493e

Apply dotnet format

b034b67

Jim8y added the Waiting for Review label Nov 4, 2025

Jim8y requested review from Wi1l-B0t and shargon November 4, 2025 02:38

shargon requested a review from Copilot November 4, 2025 09:08

shargon changed the base branch from dev to master November 4, 2025 09:09

shargon changed the base branch from master to dev November 4, 2025 09:10

Copilot AI reviewed Nov 4, 2025

View reviewed changes

move condition

deb8c30

shargon reviewed Nov 4, 2025

View reviewed changes

vncoelho reviewed Nov 4, 2025

View reviewed changes

src/Neo/Network/P2P/Peer.cs Show resolved Hide resolved

vncoelho reviewed Nov 4, 2025

View reviewed changes

src/Neo/Network/P2P/Peer.cs

return;

var maxConnections = Config.MaxConnections < 0

? int.MaxValue

Copy link

Member

vncoelho Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int.MaxValue ?

vncoelho reviewed Nov 4, 2025

View reviewed changes

	: Config.MaxConnections - ConnectedPeers.Count;
	: Math.Max(0, Config.MaxConnections - ConnectedPeers.Count);

[Fix] Improve peer bootstrap connections #4263

Are you sure you want to change the base?

[Fix] Improve peer bootstrap connections #4263

Conversation

Jim8y commented Nov 1, 2025

Description

Type of change

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

shargon left a comment

Choose a reason for hiding this comment

Uh oh!

vncoelho Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

vncoelho Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

vncoelho Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vncoelho Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vncoelho Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

vncoelho Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

vncoelho left a comment

Choose a reason for hiding this comment

Uh oh!

Jim8y commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vncoelho Nov 4, 2025 •

edited

Loading