Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't disconnect peers on fast sync criteria #5293

Merged
merged 5 commits into from
Mar 9, 2023
Merged

Don't disconnect peers on fast sync criteria #5293

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
143b04d
Don't disconnect peers on fast sync criteria
LukaszRozmej Feb 15, 2023
1c33fe5
Remove obsolete flag
LukaszRozmej Feb 15, 2023
eb35da6
remove unused method
LukaszRozmej Feb 15, 2023
48ea994
Keep DropWorstPeer
LukaszRozmej Mar 8, 2023
0e4b722
Merge remote-tracking branch 'origin/master' into fix/dont-disconnect…
LukaszRozmej Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/Nethermind/Nethermind.Network.Stats/Model/InitiateDisconnectReason.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ public enum InitiateDisconnectReason : byte
TxFlooding,
NoCapabilityMatched,

UselessInFastBlocks,
DropWorstPeer,
PeerRefreshFailed,

Expand Down Expand Up @@ -72,8 +71,6 @@ public static DisconnectReason ToDisconnectReason(this InitiateDisconnectReason
case InitiateDisconnectReason.NoCapabilityMatched:
return DisconnectReason.UselessPeer;

case InitiateDisconnectReason.UselessInFastBlocks:
return DisconnectReason.UselessPeer;
case InitiateDisconnectReason.DropWorstPeer:
return DisconnectReason.TooManyPeers;
case InitiateDisconnectReason.PeerRefreshFailed:
Expand Down
52 changes: 0 additions & 52 deletions src/Nethermind/Nethermind.Synchronization.Test/SyncPeerPoolTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,16 +169,6 @@ public async Task Cannot_add_when_not_started()
}
}

[Test]
public async Task Will_disconnect_one_when_at_max()
{
await using Context ctx = new();
var peers = await SetupPeers(ctx, 25);
await WaitForPeersInitialization(ctx);
ctx.Pool.DropUselessPeers(true);
Assert.True(peers.Any(p => p.DisconnectRequested));
}

[TestCase(true, false)]
[TestCase(false, true)]
public async Task Will_disconnect_when_refresh_exception_is_not_cancelled(bool isExceptionOperationCanceled, bool isDisconnectRequested)
Expand All @@ -192,48 +182,6 @@ public async Task Will_disconnect_when_refresh_exception_is_not_cancelled(bool i
peer.DisconnectRequested.Should().Be(isDisconnectRequested);
}

[TestCase(0)]
[TestCase(10)]
[TestCase(24)]
public async Task Will_not_disconnect_any_priority_peer_if_their_amount_is_lower_than_max(byte number)
{
const int peersMaxCount = 25;
const int priorityPeersMaxCount = 25;
await using Context ctx = new();
ctx.Pool = new SyncPeerPool(ctx.BlockTree, ctx.Stats, ctx.PeerStrategy, peersMaxCount, priorityPeersMaxCount, 50, LimboLogs.Instance);
var peers = await SetupPeers(ctx, peersMaxCount);

// setting priority to all peers except one - peers[number]
for (int i = 0; i < priorityPeersMaxCount; i++)
{
if (i != number)
{
ctx.Pool.SetPeerPriority(peers[i].Id);
}
}
await WaitForPeersInitialization(ctx);
ctx.Pool.DropUselessPeers(true);
Assert.True(peers[number].DisconnectRequested);
}

[Test]
public async Task Can_disconnect_priority_peer_if_their_amount_is_max()
{
const int peersMaxCount = 25;
const int priorityPeersMaxCount = 25;
await using Context ctx = new();
ctx.Pool = new SyncPeerPool(ctx.BlockTree, ctx.Stats, ctx.PeerStrategy, peersMaxCount, priorityPeersMaxCount, 50, LimboLogs.Instance);
var peers = await SetupPeers(ctx, peersMaxCount);

foreach (SimpleSyncPeerMock peer in peers)
{
ctx.Pool.SetPeerPriority(peer.Id);
}
await WaitForPeersInitialization(ctx);
ctx.Pool.DropUselessPeers(true);
Assert.True(peers.Any(p => p.DisconnectRequested));
}

[Test]
public async Task Should_increment_PriorityPeerCount_when_added_priority_peer_and_decrement_after_removal()
{
Expand Down
118 changes: 0 additions & 118 deletions src/Nethermind/Nethermind.Synchronization/Peers/SyncPeerPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,123 +477,6 @@ private bool CanBeUsefulForFastBlocks(long blockNumber)
lowestInsertedHeader > 1 && lowestInsertedHeader < blockNumber;
}

internal void DropUselessPeers(bool force = false)
{
if (!force && DateTime.UtcNow - _lastUselessPeersDropTime < TimeSpan.FromSeconds(30))
// give some time to monitoring nodes
// (monitoring nodes are nodes that are investigating the network but are not synced themselves)
return;

if (_logger.IsTrace) _logger.Trace($"Reviewing {PeerCount} peer usefulness");

int peersDropped = 0;
_lastUselessPeersDropTime = DateTime.UtcNow;

long ourNumber = _blockTree.BestSuggestedHeader?.Number ?? 0L;
UInt256 ourDifficulty = _blockTree.BestSuggestedHeader?.TotalDifficulty ?? UInt256.Zero;
foreach (PeerInfo peerInfo in NonStaticPeers)
{
if (peerInfo.HeadNumber == 0
&& peerInfo.IsInitialized
&& ourNumber != 0
&& peerInfo.PeerClientType != NodeClientType.Nethermind
&& peerInfo.PeerClientType != NodeClientType.Trinity)
// we know that Nethermind reports 0 HeadNumber when it is in sync (and it can still serve a lot of data to other nodes)
{
if (!CanBeUsefulForFastBlocks(peerInfo.HeadNumber))
{
peersDropped++;
peerInfo.SyncPeer.Disconnect(InitiateDisconnectReason.UselessInFastBlocks, "PEER REVIEW / HEAD 0");
}
}
else if (peerInfo.HeadNumber == 1920000 && _blockTree.NetworkId == BlockchainIds.Mainnet) // mainnet, stuck Geth nodes
{
if (!CanBeUsefulForFastBlocks(peerInfo.HeadNumber))
{
peersDropped++;
peerInfo.SyncPeer.Disconnect(InitiateDisconnectReason.UselessInFastBlocks, "PEER REVIEW / 1920000");
}
}
else if (peerInfo.HeadNumber == 7280022 && _blockTree.NetworkId == BlockchainIds.Mainnet) // mainnet, stuck Geth nodes
{
if (!CanBeUsefulForFastBlocks(peerInfo.HeadNumber))
{
peersDropped++;
peerInfo.SyncPeer.Disconnect(InitiateDisconnectReason.UselessInFastBlocks, "PEER REVIEW / 7280022");
}
}
else if (peerInfo.HeadNumber > ourNumber + 1024L && _betterPeerStrategy.IsLowerThanTerminalTotalDifficulty(peerInfo.TotalDifficulty) && peerInfo.TotalDifficulty < ourDifficulty)
smartprogrammer93 marked this conversation as resolved.
Show resolved Hide resolved
{
if (!CanBeUsefulForFastBlocks(MainnetSpecProvider.Instance.DaoBlockNumber ?? 0))
{
// probably Ethereum Classic nodes tht remain connected after we went pass the DAO
// worth to find a better way to discard them at the right time
peersDropped++;
peerInfo.SyncPeer.Disconnect(InitiateDisconnectReason.UselessInFastBlocks, "STRAY PEER");
}
}
}

if (PeerCount == PeerMaxCount)
{
peersDropped += DropWorstPeer();
}

LukaszRozmej marked this conversation as resolved.
Show resolved Hide resolved
if (_logger.IsDebug) _logger.Debug($"Dropped {peersDropped} useless peers");
}

private int DropWorstPeer()
{
string? IsPeerWorstWithReason(PeerInfo currentPeer, PeerInfo toCompare)
{
if (toCompare.HeadNumber < currentPeer.HeadNumber)
{
return "LOWEST NUMBER";
}

if (toCompare.TotalDifficulty < currentPeer.TotalDifficulty)
{
return "LOWEST DIFFICULTY";
}

if ((_stats.GetOrAdd(toCompare.SyncPeer.Node).GetAverageTransferSpeed(TransferSpeedType.Latency) ?? long.MaxValue) >
(_stats.GetOrAdd(currentPeer.SyncPeer.Node).GetAverageTransferSpeed(TransferSpeedType.Latency) ?? long.MaxValue))
{
return "HIGHEST PING";
}

return null;
}

bool canDropPriorityPeer = PriorityPeerCount >= PriorityPeerMaxCount;

PeerInfo? worstPeer = null;
string? worstReason = "DEFAULT";

foreach (PeerInfo peerInfo in NonStaticPeers)
{
if (peerInfo.SyncPeer.IsPriority && !canDropPriorityPeer)
{
continue;
}

if (worstPeer is null)
{
worstPeer = peerInfo;
}

string? peerWorstReason = IsPeerWorstWithReason(worstPeer, peerInfo);
if (peerWorstReason is not null)
{
worstPeer = peerInfo;
worstReason = peerWorstReason;
}
}

worstPeer?.SyncPeer.Disconnect(InitiateDisconnectReason.DropWorstPeer, $"PEER REVIEW / {worstReason}");
return 1;
}

public void SignalPeersChanged()
{
if (!_signals.SafeWaitHandle.IsClosed)
Expand Down Expand Up @@ -674,7 +557,6 @@ await firstToComplete.ContinueWith(
/// <exception cref="InvalidOperationException">Thrown if an irreplaceable allocation is being replaced by this method (internal implementation error).</exception>
private void UpgradeAllocations()
{
DropUselessPeers();
WakeUpPeerThatSleptEnough();
foreach ((SyncPeerAllocation allocation, _) in _replaceableAllocations)
{
Expand Down