fix(snap-sync): refresh pivot instead of punishing the only peer (#6803) by 0xDevNinja · Pull Request #11569 · NethermindEth/nethermind

0xDevNinja · 2026-05-12T10:29:28Z

Fixes #6803

Changes

Track in AnalyzeResponsePerPeer whether the recent result window contains entries from any peer other than the one being analyzed.
When the only peer observed exceeds AllowedInvalidResponses with no offsetting success, call _snapProvider.UpdatePivot() and clear the log instead of returning LesserQuality. Punishing the only available peer in that case just stalls sync.
Multi-peer behavior is untouched — the existing allLastFailures == peerLastFailures punishment branch still fires when other peers are healthy and only this peer is misbehaving.

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

Two new cases in AnalyzeResponsePerPeerTests:

Single_peer_with_consecutive_failures_refreshes_pivot_instead_of_punishing — exercises the Do not mark peer as low quality too soon if there is only one peer connected #6803 path: six consecutive failures from a sole peer trigger UpdatePivot and never return LesserQuality.
Single_peer_with_recent_success_is_not_punished_below_threshold — confirms a sub-threshold failure burst after a success neither punishes nor refreshes.

All four pre-existing tests continue to pass.

Documentation

Requires documentation update

Yes
No

Requires explanation in Release Notes

Yes
No

Remarks

Earlier attempts (#6818, #7067) were closed for being too simplistic or lacking tests. This iteration:

Uses the result log itself as the single-peer signal (no ISyncPeerPool injection needed), keeping SnapSyncFeed constructor unchanged.
Only refreshes the pivot when truly observing one peer with zero successes, leaving the multi-peer punishment branch intact.

Fixes NethermindEth#6803. Track if other peers appear in the recent window; when only one peer is observed and there are no successes, refresh the pivot instead of marking it LesserQuality.

Covers NethermindEth#6803: consecutive failures from a sole peer trigger UpdatePivot, and a sub-threshold burst after a success neither punishes nor refreshes.

asdacap · 2026-05-12T23:15:07Z

                {
                    foreach ((PeerInfo peer, AddRangeResult result) item in _resultLog)
                    {
+                        if (item.peer != peer)


What if item.peer is the first peer?

Good catch — you are right. With the in-loop check, a newcomer that fails its first 6 requests while healthy peers sit further back in the log would never reach those older entries before seenOtherPeer got evaluated, so the guard would refresh the pivot instead of punishing the newcomer.

Moved seenOtherPeer to a pre-scan over the full window in 83a0327946 and added a regression test (New_peer_failing_burst_is_punished_when_log_holds_other_healthy_peers) that exercises exactly this case.

@asdacap

Addresses @asdacap's review on NethermindEth#11569: the in-loop seenOtherPeer check could fire before reaching entries from older healthy peers, so a newcomer's first AllowedInvalidResponses failures would refresh the pivot instead of being demoted. Compute seenOtherPeer over the full result window before the analysis loop. Adds a regression test for the multi-peer-with-newcomer case.

LukaszRozmej · 2026-05-14T09:40:50Z

@claude review

claude · 2026-05-14T09:41:05Z

Claude finished @LukaszRozmej's task in 4m 31s —— View job

PR Review: fix(snap-sync): refresh pivot instead of punishing the only peer

Summary

The fix is logically correct and addresses the original stall scenario (#6803) without disturbing multi-peer behavior. The reviewer-caught race (in-loop seenOtherPeer check firing prematurely for a newcomer) was properly addressed by the pre-scan in commit 83a0327.

Correctness trace

Key invariants verified by manual trace:

Scenario	Expected	Result
Single peer, ≥6 consecutive failures, no successes in log	`UpdatePivot` + return `OK`	✓ condition 1 fires before condition 2
Single peer, ≤5 failures	No action	✓ threshold never exceeded
New peer failing 6×, healthy peer's OK further back in log	Punish (`LesserQuality`)	✓ pre-scan sets `seenOtherPeer=true`, falls to condition 2
Multi-peer, one peer consistently bad	Punish	✓ condition 2 unchanged
Multi-peer, global failure stream	`UpdatePivot`	✓ condition 3 unchanged

The ordering of conditions 1 → 2 → 3 is correct: condition 1 is a strict single-peer sub-case that is checked first, preventing condition 2 from incorrectly punishing the sole available peer.

Findings

Medium

Weak test assertion at line 141 — Is.Not.EqualTo(LesserQuality) passes for NoProgress, InternalError, etc. Since the input is DifferentRootHash, the expected result after a pivot refresh is OK. The assertion should be Is.EqualTo(SyncResponseHandlingResult.OK). (inline comment posted)

Low

Duplicated constant at line 195 — private const int AllowedInvalidResponses = 5 must stay in sync with the production constant. If the production value changes, the two new tests silently use wrong loop bounds. Consider exposing the constant as internal or adding a tracking note. (inline comment posted)

Pre-existing issues (not introduced by this PR)

The while (_resultLog.Count > maxSize) trimming loop reads _resultLog.Count outside lock (_syncLock) — a data race on LinkedList<T>. Not in scope here.
The two-lock structure (add in one lock, analyze in a second) means another thread can modify the log between the two acquisitions. Also pre-existing.

Overall

The production logic is sound and the three new tests meaningfully cover the bug scenario and its regression cases. The only blocking-adjacent issue is the imprecise test assertion (Medium); the constant duplication is a minor hygiene point. Both are in tests, not production code.

claude · 2026-05-14T09:45:18Z

+                lastResult = feed.AnalyzeResponsePerPeer(AddRangeResult.DifferentRootHash, peer);
+            }
+
+            Assert.That(lastResult, Is.Not.EqualTo(SyncResponseHandlingResult.LesserQuality));


Medium — Weak assertion masks potential regressions

Assert.That(lastResult, Is.Not.EqualTo(SyncResponseHandlingResult.LesserQuality));

This passes for any result other than LesserQuality — including NoProgress or InternalError. Since the input is DifferentRootHash (not ExpiredRootHash), the expected post-refresh return value is OK. The assertion should be exact:

Suggested change

Assert.That(lastResult, Is.Not.EqualTo(SyncResponseHandlingResult.LesserQuality));

Assert.That(lastResult, Is.EqualTo(SyncResponseHandlingResult.OK));

Fix this →

Tightened in a5495b409b. Single-peer refresh path returns OK after UpdatePivot, so the assertion now checks that exact value.

claude · 2026-05-14T09:45:26Z

+            snapProvider.DidNotReceive().UpdatePivot();
+        }
+
+        private const int AllowedInvalidResponses = 5;


Low — Duplicated constant drifts silently

AllowedInvalidResponses = 5 must stay in sync with the production constant in SnapSyncFeed. If the production value is changed, the loop bounds in Single_peer_with_recent_success_is_not_punished_below_threshold and New_peer_failing_burst_is_punished_when_log_holds_other_healthy_peers silently test the wrong thresholds.

One approach — expose the production constant as internal (with InternalsVisibleTo already wired in the test project) and reference it directly. Alternatively, add a note that this value must track the production constant.

Done in a5495b409b — promoted SnapSyncFeed.AllowedInvalidResponses to internal (with [assembly: InternalsVisibleTo("Nethermind.Synchronization.Test")] added) and the test file now references it directly. The local duplicate is gone, so changing the production value updates both loop bounds automatically.

Assert exact OK on single-peer pivot refresh; expose production constant as internal so test loop bounds cannot drift from SnapSyncFeed.

InternalsVisibleTo("Nethermind.Synchronization.Test") is already declared in PeerInfo.cs (and other files) in the same assembly, so the duplicate attribute and its System.Runtime.CompilerServices using are unnecessary. Rename the pre-scan tuple element from p to peer so probe.peer matches the naming used by item.peer in the main loop below. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

0xDevNinja added 2 commits May 12, 2026 15:58

fix(snap-sync): don't punish lone peer on burst failures

cb4b332

Fixes NethermindEth#6803. Track if other peers appear in the recent window; when only one peer is observed and there are no successes, refresh the pivot instead of marking it LesserQuality.

test(snap-sync): single-peer pivot-refresh regression

4984e3c

Covers NethermindEth#6803: consecutive failures from a sole peer trigger UpdatePivot, and a sub-threshold burst after a success neither punishes nor refreshes.

0xDevNinja requested review from LukaszRozmej, asdacap, benaadams, flcl42 and marcindsobczak as code owners May 12, 2026 10:29

github-actions Bot added bug fix + reliability snap sync sync labels May 12, 2026

asdacap reviewed May 12, 2026

View reviewed changes

0xDevNinja requested a review from asdacap May 13, 2026 17:52

claude Bot reviewed May 14, 2026

View reviewed changes

test(snap-sync): tighten assertion and share AllowedInvalidResponses

a5495b4

Assert exact OK on single-peer pivot refresh; expose production constant as internal so test loop bounds cannot drift from SnapSyncFeed.

asdacap approved these changes May 15, 2026

View reviewed changes

LukaszRozmej approved these changes May 15, 2026

View reviewed changes

svlachakis approved these changes May 15, 2026

View reviewed changes

smartprogrammer93 approved these changes May 15, 2026

View reviewed changes

LukaszRozmej merged commit e217f11 into NethermindEth:master May 15, 2026
547 of 551 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(snap-sync): refresh pivot instead of punishing the only peer (#6803)#11569

fix(snap-sync): refresh pivot instead of punishing the only peer (#6803)#11569
LukaszRozmej merged 5 commits into
NethermindEth:masterfrom
0xDevNinja:feature/6803-peer-quality-guard

0xDevNinja commented May 12, 2026

Uh oh!

asdacap May 12, 2026

Uh oh!

0xDevNinja May 13, 2026

Uh oh!

LukaszRozmej commented May 14, 2026

Uh oh!

claude Bot commented May 14, 2026 •

edited

Loading

Uh oh!

claude Bot May 14, 2026

Uh oh!

0xDevNinja May 14, 2026

Uh oh!

claude Bot May 14, 2026

Uh oh!

0xDevNinja May 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	Assert.That(lastResult, Is.Not.EqualTo(SyncResponseHandlingResult.LesserQuality));
	Assert.That(lastResult, Is.EqualTo(SyncResponseHandlingResult.OK));

Conversation

0xDevNinja commented May 12, 2026

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Documentation

Requires documentation update

Requires explanation in Release Notes

Remarks

Uh oh!

asdacap May 12, 2026

Choose a reason for hiding this comment

Uh oh!

0xDevNinja May 13, 2026

Choose a reason for hiding this comment

Uh oh!

LukaszRozmej commented May 14, 2026

Uh oh!

claude Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: fix(snap-sync): refresh pivot instead of punishing the only peer

Summary

Correctness trace

Findings

Pre-existing issues (not introduced by this PR)

Overall

Uh oh!

claude Bot May 14, 2026

Choose a reason for hiding this comment

Uh oh!

0xDevNinja May 14, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 14, 2026

Choose a reason for hiding this comment

Uh oh!

0xDevNinja May 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

claude Bot commented May 14, 2026 •

edited

Loading