fix(blocksync): don't punish peers for duplicate block responses by QuantumExplorer · Pull Request #1393 · dashpay/tenderdash

QuantumExplorer · 2026-07-25T09:59:30Z

Issue being fixed or feature implemented

Synchronizer.removePeer re-requests every block it has already received from
the peer being removed, but leaves those responses in pendingToApply. When the
re-requested block arrives from a different peer, addBlock rejects it as a
duplicate and the synchronizer sends a PeerError against the peer that served
it correctly, evicting it.

Under peer churn this cascades: each eviction re-requests more heights, each of
those punishes another healthy peer, and block sync can stall long enough to hit
the 60s syncTimeout and switch to consensus without being caught up — which is
a one-way door, since nothing switches back to block sync.

Found while profiling block sync performance; it is independent of that work, so
it is split out here.

What was done?

removePeer now deletes the pending entry alongside the re-request, so the
re-fetched block is accepted instead of colliding with the stale one.
jobGenerator.pushBack dedupes, so a height is not fetched twice concurrently
(which produced the same duplicate collision by another route).
A duplicate response is now treated as benign: it is dropped with a debug log
and the peer is not reported. We asked more than one peer for that height, so
it is not the responder's fault. Non-duplicate addBlock failures still
report the peer as before.

Introduces a sentinel errDuplicateBlock so the duplicate case is distinguished
by errors.Is rather than by string.

How Has This Been Tested?

TestRemovePeer extended with a wantPending assertion: heights that were
re-requested must no longer be pending.
New TestConsumeDuplicateBlock: a duplicate response keeps the originally
stored response and sends no peer error. The client mock has no Send
expectation, so a report fails the test.
go build ./internal/... ./types/... ./node/... ./cmd/...
go test -race -tags=deadlock ./internal/blocksync/...
golangci-lint: identical issue set to the base branch, nothing introduced.

Built and tested against DOCKER/Dockerfile's deps stage (Go 1.26.4, BLS
prebuilt).

Breaking Changes

None.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

🤖 Generated with Claude Code

removePeer re-requested every block it had already received from the peer being removed, but left those responses in pendingToApply. When the re-requested block arrived from a different peer, addBlock rejected it as a duplicate and the synchronizer reported a PeerError against the peer that had served it correctly, evicting it. Under peer churn this cascades: each eviction re-requests more heights, each of those punishes another healthy peer, and block sync can stall long enough to hit the 60s syncTimeout and switch to consensus without being caught up. Delete the pending entry alongside the re-request so the re-fetched block is accepted, dedupe pushedBack so a height is not fetched twice concurrently, and treat a duplicate response as benign rather than as a peer fault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

coderabbitai · 2026-07-25T09:59:36Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2f6011bd-2388-4aba-94c4-d5866cc6ccb6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/blocksync-duplicate-block-peer-punishment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

QuantumExplorer · 2026-07-25T11:26:17Z

CI follow-up

`tests (01)` — flaky, not caused by this PR. Re-running.

The failure was --- FAIL: TestWALRoundsSkipper (60.07s) in internal/consensus
— a 60s timeout, not an assertion failure.

This PR cannot reach that test:

The diff is three files, all in internal/blocksync.
internal/consensus does not import internal/blocksync (the dependency runs
the other way — blocksync/reactor.go imports consensus). So no change here
is reachable from TestWALRoundsSkipper.

Ran it 8 consecutive times locally on this branch to check: all pass, 18.3s total
(~2.3s per run) against the 60s timeout it hit in CI. That gap points at a loaded
runner rather than anything in the code.

I have re-run the failed shard rather than changing anything.

`govulncheck` — pre-existing, needs a toolchain bump

GO-2026-5856 in the Go standard library crypto/tls@go1.26.4, fixed in
go1.26.5. It is reached via dash/core/client.go, rpc/jsonrpc/... and
internal/libs/autofile/..., none of which this PR touches, and it fails
identically on #1394.

Fixing it means bumping Go in go.mod, DOCKER/Dockerfile and CI — a repo-wide
decision, so I have deliberately left it alone. It will keep this PR red until
someone makes that call.

🤖 Addressed by Claude Code

lklimek · 2026-07-28T17:26:27Z

Audit Summary — PR #1393 `fix(blocksync): don't punish peers for duplicate block responses`

Reviewed by: Claude Code with a 3-agent team:

security-engineer-smythe (opus) — OWASP/CWE classification, concurrency & DoS surface, attacker-triggerable paths
project-reviewer-adams (opus) — cross-artifact consistency (PR narrative vs. code vs. comments vs. tests), code quality
qa-engineer-marvin (sonnet) — adversarial correctness, execution-verified (tests run, coverage profiles inspected, throwaway probes written)

Every finding was then independently re-validated by a 4th adversarial pass that re-ran the evidence rather than trusting the producers. All 12 findings came back valid (confidence 0.65–0.95).

Verdict: REQUEST CHANGES — 1 blocking finding.

Overall assessment

This PR fixes a real, well-scoped bug: an honest peer answering a duplicate (self-inflicted) block request was being reported and evicted. The core fix — sentinel-error duplicate classification plus a matching pendingToApply delete on peer removal — is correct, minimal, and closes that specific eviction loop.

However, the diff's own stated goal is only partially delivered. Removing the early return nil now lets a duplicate-sender's response fall through into applyBlock, and applyBlock still attributes any subsequent apply failure to whichever peer's response was last consumed — not to the peer that actually supplied the bad block. An honest duplicate-sender can therefore still be evicted, just for a different peer's block. That is SEC-002, the one blocking finding.

Everything else — a stale-entry/memory-pinning gap below s.height, an overstated dedup-invariant comment, a silent drop from Error to Debug logging, and a completely untested dedup branch that is the mechanism the whole PR relies on — is real and worth fixing, but does not block merge: none is required by the PR's explicit stated goal, none is newly-introduced-and-materially-broken, and all are naturally fixable in this PR or an immediate fast-follow.

Statistics: 14 raw findings from 3 reviewers → 12 after dedup (57% redundancy). Severity: 0 CRITICAL, 0 HIGH, 3 MEDIUM, 9 LOW. Merge class: 1 blocking, 8 non-blocking, 3 out-of-scope follow-up, 0 disputed.

Findings

#	Merge class	Severity	OWASP / CWE	Location	Issue
SEC-002	🔴 blocking	MEDIUM	A04:2021 Insecure Design · A08:2021 Data Integrity · CWE-841 · CWE-345 · CWE-400	`internal/blocksync/synchronizer.go:243-266`	`applyBlock` failure punishes the wrong peer — a malicious peer can poison `pendingToApply` and get honest peers evicted one per consumed response while sync stalls.
SEC-003	non-blocking	MEDIUM	A04:2021 Insecure Design · CWE-362 · CWE-401 · CWE-770	`internal/blocksync/synchronizer.go:341-354`	No `block.Height < s.height` floor guard: a stale/late duplicate for an already-applied height is accepted into `pendingToApply` and never read or freed again (block + commit pinned). Proven by execution — no race required.
SEC-005	non-blocking	LOW	A04:2021 Insecure Design · CWE-407 · CWE-400	`internal/blocksync/block_fetch_job.go:109-119`	Linear `slices.Contains` dedupe makes `removePeer` O(n·m) with n `jobGen.mtx` acquisitions inside the global `Synchronizer.mtx` critical section.
PROJ-001	non-blocking	LOW	observability / operator-visibility	`internal/blocksync/synchronizer.go:246-259`	Duplicate handling drops to `Debug`; default `log_level` is `info` and the package has no metrics, so the re-request cascade the PR describes becomes invisible in production.
PROJ-002	non-blocking	LOW	doc-code-mismatch	`internal/blocksync/block_fetch_job.go:109-119`	`pushBack` dedup comment (and PR text) claims it prevents concurrent double-fetch; `nextHeight` pops the height before dispatch, so the guard covers the queued-but-undispatched window only.
CODE-001	non-blocking	LOW	test-coverage	`internal/blocksync/block_fetch_job.go:112-119`	The dedup guard this PR relies on has zero coverage — raw profile shows the `return` branch at count 0. Delete the guard and the whole blocksync suite still passes.
CODE-002	non-blocking	LOW	superficial-test	`internal/blocksync/synchronizer_test.go:341-368`	`TestConsumeDuplicateBlock` seeds `pendingToApply[1]` on a synchronizer at height 2 — a below-current height, not the out-of-order/future case the PR targets. Asserts nothing about `jobGen` or subsequent apply progress; no concurrency under test.
CODE-003	non-blocking	LOW	DRY / test-structure	`internal/blocksync/synchronizer_test.go:344-367`	Rebuilds `TestConsumeJobResult`'s fixture verbatim instead of adding a sixth table case (its `mockFn` hook already exists for exactly this). `dupl` is an enabled linter; the sibling `TestRemovePeer` change in this same PR did it the right way.
CODE-004	non-blocking	LOW	comment-quality	`internal/blocksync/synchronizer.go:342-346`	5-line `removePeer` comment is written counterfactually and duplicates the commit message and PR description near-verbatim. Plus: `errDuplicateBlock`'s doc has the report direction backwards.

Pre-existing / outside-diff

Found while walking the duplicate-block-response path, not introduced by this PR and correctly out of scope for #1393 — but real enough to deserve tracked follow-up rather than being silently dropped.

#	Severity	OWASP / CWE	Location	Issue
SEC-001	—	—	`internal/p2p/client/`	A pre-existing issue was identified in this area during review. Given its nature, it is being handled through a private security-disclosure channel rather than described in this public thread.
SEC-004	LOW	A07:2021 Ident./Auth Failures · A08:2021 Data Integrity · CWE-290 · CWE-345	`internal/p2p/client/client.go:339-359`	Block responses are matched by request ID alone, with no check that `envelope.From` is the peer we actually asked. Storing the target `peerID` alongside the pending channel is cheap and would make the blocksync blame logic sound by construction.
CODE-005	LOW	readability / go-idiom	`internal/blocksync/synchronizer.go:246-259`	The duplicate check is nested inside the error branch, inverting the happy path and hiding the fall-through into `applyBlock`. Two sibling guard clauses read better — and would have made SEC-002's fall-through obvious.

Suggested follow-up: SEC-004 (block responses matched by request ID alone, no check that envelope.From is the peer actually asked) is worth its own tracked issue.

Positive observations

The core fix is right. Sentinel-error classification (errDuplicateBlock) is the idiomatic Go way to distinguish "not this peer's fault" from a real protocol violation, and it is applied at exactly one place.
The removePeer delete + pushBack pairing is genuinely load-bearing and doubles as the mitigation that makes SEC-002's stall recoverable rather than permanent — once the poisoning peer is eventually removed, the poison is dropped and the height re-requested.
TestRemovePeer was extended correctly — a wantPending column added to the existing table rather than forking a new test. That is the pattern CODE-003 asks TestConsumeDuplicateBlock to follow.
The suite passes cleanly under -race (verified independently, -tags deadlock, 83.5% statement coverage in internal/blocksync).
The PR description is honest and specific about the cascade being fixed — it made the intent digest unambiguous, which is more than most PRs manage.

Inline comments for the 9 in-scope findings are attached as a draft review (not submitted) for the author's triage.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

…hment

…lock applyBlock drains pendingToApply from the current height, so the response that fails to apply is generally not the one just consumed. Attributing the failure to the consumed response let one peer poison a height, evict every honest peer that answered afterwards, and wedge the height for good, because the poisoned entry was never dropped either. applyBlock now returns the response that failed, and the caller removes that peer, which also deletes its pending entry so the height is fetched from someone else. A cancelled context no longer costs a peer its connection. addBlock now rejects responses for heights below the current one. Only the entry at the current height is ever read, so a straggler for an already applied height used to occupy pendingToApply for the rest of the process lifetime. RemovePeer collects the heights to re-fetch and hands them to a single batched pushBack after releasing the synchronizer lock, so peer removals no longer serialize status reads behind the job generator lock, and the only nested lock pair between the two mutexes is gone. The pushBack doc no longer claims to prevent concurrent double-fetching: dispatched heights leave pushedBack, so only the queued window is deduped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

thepastaclaw · 2026-07-30T10:05:04Z

⛔ Blockers found — Sonnet deferred (commit 64448e2)
Canonical validated blockers: 1

A duplicate response means a height was requested more than once, which is exactly the re-request cascade this branch is about. At debug level it was invisible on a default `info` node, and the package has no metrics to see it by, so the only evidence of a cascade was unreachable in production. The sub-case (already pending vs already applied) rides along as a reason field. Info per occurrence rather than an aggregate counter on the existing "block sync rate" line: a healthy sync produces no duplicates at all, a peer cannot amplify them because every response answers a job we issued, and the rate line only fires once the height advances by monitorInterval, so a counter would go quiet during precisely the wedged sync worth reporting. Also adds a jobGenerator.pushBack unit test covering both sides of the dedupe guard: queued heights are deduped, dispatched ones are not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

thepastaclaw

Preliminary review — Codex only

The duplicate-response handling and apply-failure attribution are correct at the exact head, and the resolved human review threads match the current code. One blocking issue remains: the new retry batching performs quadratic deduplication and preserves random map order over a completed-response backlog that is not bounded by the worker or per-peer request limits, allowing peer removal to delay the application-blocking height until block sync times out. Focused tests could not build on the review host because the native BLS header dashbls/bls.hpp is unavailable.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — tenderdash-consensus-security (completed)
Verifier: gpt-5.6-sol — verifier
Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `internal/blocksync/block_fetch_job.go`:
- [BLOCKING] internal/blocksync/block_fetch_job.go:112-124: Make retry insertion linear and prioritize the blocking height
  `dropPeer` supplies this function with heights collected by ranging over `pendingToApply`, so their order is nondeterministic. This loop scans the growing `pushedBack` slice once for every inserted height, making a batch of N distinct responses O(N²), and the FIFO subsequently retries those randomly ordered heights before whichever entry happens to contain the current application-blocking height. The 600-worker and 20-request-per-peer limits do not bound this batch: every completed future response decrements `numPending`, allowing the producer to issue a replacement while the completed response remains in `pendingToApply`. A peer can therefore hold height H until near the 15-second request deadline, rapidly answer H+1 onward with blocks and commits that pass basic structural validation, and finally return a cryptographically invalid H. When applying H removes that peer, a bandwidth-sized batch containing H is scanned quadratically and queued in random order. Fetching all entries placed before H can then consume the remaining 60-second no-progress window and recreate the synchronization stall this PR is intended to prevent. Build membership once and sort the retry queue so insertion is bounded and the lowest required height is dispatched first.

TestConsumeDuplicateBlock seeded a response below the synchronizer's height, which is not the situation this change is about and which the new already-applied floor guard now catches first. The real case is a retry race: a height is answered by two peers while an earlier height is still outstanding, so the duplicate arrives above the current height. Folds that into the TestConsumeJobResult table, which already has the mockFn hook for seeding pendingToApply, rather than rebuilding its fixture. What the table cannot express is the sequencing, so the remaining standalone test now covers that instead: after the duplicate is dropped, the arrival of the awaited height still drains both blocks. That exercises the fall-through from the duplicate branch into applyBlock, which the previous test never reached - it returned early because the awaited height was never pending. Both were checked by mutation: making the duplicate branch report the sender again fails the table case and the sequencing test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

pushBack queued heights in the order callers supplied them, and dropPeer supplies them by ranging pendingToApply, so the order is arbitrary. nextHeight pops from the front, so the height that actually unblocks applyBlock could sit behind a batch of higher heights that cannot be applied until it lands - spending the no-progress window on fetches that change nothing, which is the stall this branch exists to avoid. Keep the queue sorted so the lowest missing height is dispatched first. Membership is now built once per call instead of rescanning the queue for every inserted height, which was quadratic in the size of a batch. Reported by review on #1393. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

QuantumExplorer · 2026-08-04T04:39:16Z

tests (01) — infrastructure flake, not this PR. Re-running.

--- FAIL: TestTxSearch (0.00s)
    Received unexpected error:
    Post "http://127.0.0.1:36657": read tcp 127.0.0.1:37258->127.0.0.1:36657: read: connection reset by peer

The inspect test's local RPC server connection was reset — a port/timing failure on the runner, not an assertion. internal/inspect does not import internal/blocksync, so nothing in this PR is reachable from it. Ran it 10 times in a row locally under -race: all pass.

For the record, that is the fourth distinct flaky test seen across this stack — TestWALRoundsSkipper, TestReactorValidatorSetChanges (both internal/consensus, both 60s/120s timeouts) and now TestTxSearch. Each has cost a re-run. Might be worth tracking separately from this work.

🤖 Addressed by Claude Code

shumkov · 2026-08-08T12:14:33Z

@thepastaclaw review again

All comments addressed

Resolves the conflicts introduced when #1393 was squash-merged as a99ff2a. The squash dropped the shared ancestry between this branch and #1393, so the merge base fell back to 50f84e4 and every #1393 change resurfaced as a conflict against this branch's older copies of the same functions. Conflicts were in internal/blocksync/{block_fetch_job.go,synchronizer.go, synchronizer_test.go}, all between #1393's fixes and the pre-#1393 code this branch still carried. Resolved in favour of #1393 throughout: - block_fetch_job.go: taken from v1.6-dev unchanged. This branch needs no change here; keeping its copy would have reverted the sorted, single-pass pushBack and restored the quadratic scan and arrival-order retry. - synchronizer_test.go: taken from v1.6-dev unchanged. - synchronizer.go: keeps #1393's dropPeer batching, applyBlock returning the failing response so the supplying peer is charged, the addBlock floor guard for already-applied heights, and the context-cancellation carve-out; adds this branch's apply-path work on top (BlockResponse.Size, the per-stage timings in updateMonitor, and the RecordConsMetrics call). The resolved tree was verified byte-for-byte against a three-way merge of this branch with #1393's pre-squash tip, computed while that ancestry still existed. Net delta against v1.6-dev is this branch's apply-path work only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Resolves the conflicts introduced when #1394 was squash-merged as d3baf3f, the same way #1394 resolved the conflicts from #1393's squash as a99ff2a. Each squash drops the shared ancestry with the branch below it in the stack, so the merge base falls back past both parents and their changes resurface as conflicts against the older copies this branch still carries. Conflicts in internal/blocksync/{block_fetch_job.go,synchronizer.go, synchronizer_test.go} and types/block_meta.go, all between work already merged to v1.6-dev and this branch's stale copies. Resolved in favour of v1.6-dev throughout; this branch contributes no change to any of the four: - block_fetch_job.go, synchronizer_test.go: taken from v1.6-dev unchanged. - types/block_meta.go: taken from v1.6-dev unchanged. Keeping this branch's copy would have restored the eager block.Size() call, reintroducing the redundant serialization on the block sync apply path. - synchronizer.go: keeps #1393's dropPeer batching, applyBlock returning the failing response, and the addBlock floor guard, plus #1394's apply-path work, and layers this branch's failure-tracking and stall handling on top. The resolved tree was verified byte-for-byte against a three-way merge of this branch with #1394's pre-squash tip, computed while that ancestry still existed. Net delta against v1.6-dev is this branch's own seven files and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Three defects on the pending-request path, all reachable now that #1396 retains timed-out peers instead of evicting them. 1. Send on closed channel. removePending closed the response channel, so a resolver that had already loaded it panicked on the next send. iter calls resolve directly, outside recoveryP2PMessageHandler, so nothing recovered it and the process died. Deleting the close is correct, not a workaround. removePending has exactly one caller - the promise executor's deferred call - and the channel's only reader is that same executor's select, which has already returned by the time the defer runs. The close therefore signaled nobody. It was unsafe in the other direction too: a closed channel yields a zero result, whose nil Value would panic the type assertion on the receive side. Nothing should reinstate it. 2. A late response evicted the peer. With the entry deleted, a response arriving after the timeout fell through to "pending response not found", which iter turns into a PeerError - evicting at capacity and silently defeating blocksync's maxConsecutiveFailures policy. removePending now leaves a tombstone in place of the channel, so resolveMessage can tell a request we issued and retired (dropped, no error) from an ID we never issued (still reported). Tombstones live 2*reqTimeout on the injected clock and sit in an insertion-ordered list swept from addPending: one push and one pop per request, so no background goroutine and no full-map scan. What we retain is set by the requests we issue, not by anything a peer sends. Every retired request is tombstoned, not only expired ones. That also stops a duplicate BlockResponse for the same request ID from being reported - the client-side counterpart of #1393, which fixed only the height-level case in the synchronizer. 3. Blocking send parked the consumer. resolveMessage sent into a one-slot buffer whose reader may already be gone. Removing the close turned the interleaving that used to panic into a permanent hang instead, stopping every blocksync message until node shutdown. The send is now non-blocking: a full buffer means the answer already arrived, so the extra copy is dropped. Found by code review of the first two fixes, and reproduced before fixing. Tests, under -race -tags deadlock. Each one fails on the code it pins: TestRemovePendingKeepsChannelSendable before: panic: send on closed channel after: PASS TestLateResponseAfterTimeoutIsNotAnError before: pending response <uuid> not found after: PASS TestRetiredRequestIDsAreBounded before: Not equal: expected 100, actual 0 after: PASS TestDuplicateResponseDoesNotBlockResolver before: blocked on a response buffer nobody reads after: PASS TestUnsolicitedResponseIDIsAnError passes both before and after by design: it is the regression guard proving the new silence does not swallow fabricated response IDs. The pre-existing TestGetBlockTimeout still passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…throughput Follow-up to ca64f921c, which tombstoned every settled request so a late response could be told apart from an unsolicited one. That made retention scale with total throughput: removePending runs for every settled promise, so a node completing thousands of requests a second held every one of their IDs for the whole 30s window, tens of thousands of entries and megabytes of index, for a fix whose purpose is to stop a node falling over. Only a timed-out request now leaves a tombstone. Success and cancellation delete the entry outright - still without closing the channel, which remains correct for the reasons in ca64f921c. Timeouts are capped by how many requests can be outstanding at once, so retention follows concurrency rather than throughput. The cost is that a second response to an already-answered request is reported again; that requires a peer to answer one request twice, which is misbehaviour, and the synchronizer has handled height-level duplicates since #1393. The lifetime no longer depends on sweeping. Sweeps were driven only by addPending, so a client that stopped issuing requests never swept again and its last tombstones shielded their peers for as long as the node lived. Tombstones now carry their own deadline and resolveMessage enforces it on lookup, so an expired ID reports the peer whether or not a sweep has run. Sweeping also runs on retirement, which reclaims a burst without waiting for a request that may never come. Both remain O(1) amortized with no background goroutine for shutdown to join. Also folded in, from review of ca64f921c: - resolveMessage's unexpected-value branch logs instead of returning an error. It is unreachable today, but returning an error there would have iter raise a PeerError against whoever happened to answer, blaming a peer for our own bug. - Documented why the send select deliberately has no context arm, so it is not reinstated: with a default present the select cannot block, and a ctx arm would win a coin flip against delivering the response. - Corrected a comment claiming the response channel is collected along with the tombstone; it becomes garbage as soon as it leaves the map. Tests. Each new or changed test was verified red by mutating the specific behaviour it guards, then green again: TestRetentionTracksInFlightNotThroughput tombstone every retirement -> FAIL "settled requests must leave nothing behind" now PASS TestTimedOutRequestStopsShieldingPeerWhenIdle drop the lookup-time expiry -> FAIL "An error is expected but got nil" now PASS TestRetirementReclaimsExpiredTombstones remove the sweep on retire -> FAIL on retained count now PASS TestDuplicateResponseDoesNotBlockResolver now also asserts the first response survives; drop-both -> FAIL "the first response was dropped along with the duplicate", and a blocking send still -> FAIL on the 2s guard now PASS TestRetirePendingKeepsChannelSendable covers both retirement paths. Full package suite completes in ~10s under -race -tags deadlock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Three defects on the pending-request path, all reachable now that #1396 retains timed-out peers instead of evicting them. 1. Send on closed channel. removePending closed the response channel, so a resolver that had already loaded it panicked on the next send. iter calls resolve directly, outside recoveryP2PMessageHandler, so nothing recovered it and the process died. Deleting the close is correct, not a workaround. removePending has exactly one caller - the promise executor's deferred call - and the channel's only reader is that same executor's select, which has already returned by the time the defer runs. The close therefore signaled nobody. It was unsafe in the other direction too: a closed channel yields a zero result, whose nil Value would panic the type assertion on the receive side. Nothing should reinstate it. 2. A late response evicted the peer. With the entry deleted, a response arriving after the timeout fell through to "pending response not found", which iter turns into a PeerError - evicting at capacity and silently defeating blocksync's maxConsecutiveFailures policy. removePending now leaves a tombstone in place of the channel, so resolveMessage can tell a request we issued and retired (dropped, no error) from an ID we never issued (still reported). Tombstones live 2*reqTimeout on the injected clock and sit in an insertion-ordered list swept from addPending: one push and one pop per request, so no background goroutine and no full-map scan. What we retain is set by the requests we issue, not by anything a peer sends. Every retired request is tombstoned, not only expired ones. That also stops a duplicate BlockResponse for the same request ID from being reported - the client-side counterpart of #1393, which fixed only the height-level case in the synchronizer. 3. Blocking send parked the consumer. resolveMessage sent into a one-slot buffer whose reader may already be gone. Removing the close turned the interleaving that used to panic into a permanent hang instead, stopping every blocksync message until node shutdown. The send is now non-blocking: a full buffer means the answer already arrived, so the extra copy is dropped. Found by code review of the first two fixes, and reproduced before fixing. Tests, under -race -tags deadlock. Each one fails on the code it pins: TestRemovePendingKeepsChannelSendable before: panic: send on closed channel after: PASS TestLateResponseAfterTimeoutIsNotAnError before: pending response <uuid> not found after: PASS TestRetiredRequestIDsAreBounded before: Not equal: expected 100, actual 0 after: PASS TestDuplicateResponseDoesNotBlockResolver before: blocked on a response buffer nobody reads after: PASS TestUnsolicitedResponseIDIsAnError passes both before and after by design: it is the regression guard proving the new silence does not swallow fabricated response IDs. The pre-existing TestGetBlockTimeout still passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…throughput Follow-up to ca64f921c, which tombstoned every settled request so a late response could be told apart from an unsolicited one. That made retention scale with total throughput: removePending runs for every settled promise, so a node completing thousands of requests a second held every one of their IDs for the whole 30s window, tens of thousands of entries and megabytes of index, for a fix whose purpose is to stop a node falling over. Only a timed-out request now leaves a tombstone. Success and cancellation delete the entry outright - still without closing the channel, which remains correct for the reasons in ca64f921c. Timeouts are capped by how many requests can be outstanding at once, so retention follows concurrency rather than throughput. The cost is that a second response to an already-answered request is reported again; that requires a peer to answer one request twice, which is misbehaviour, and the synchronizer has handled height-level duplicates since #1393. The lifetime no longer depends on sweeping. Sweeps were driven only by addPending, so a client that stopped issuing requests never swept again and its last tombstones shielded their peers for as long as the node lived. Tombstones now carry their own deadline and resolveMessage enforces it on lookup, so an expired ID reports the peer whether or not a sweep has run. Sweeping also runs on retirement, which reclaims a burst without waiting for a request that may never come. Both remain O(1) amortized with no background goroutine for shutdown to join. Also folded in, from review of ca64f921c: - resolveMessage's unexpected-value branch logs instead of returning an error. It is unreachable today, but returning an error there would have iter raise a PeerError against whoever happened to answer, blaming a peer for our own bug. - Documented why the send select deliberately has no context arm, so it is not reinstated: with a default present the select cannot block, and a ctx arm would win a coin flip against delivering the response. - Corrected a comment claiming the response channel is collected along with the tombstone; it becomes garbage as soon as it leaves the map. Tests. Each new or changed test was verified red by mutating the specific behaviour it guards, then green again: TestRetentionTracksInFlightNotThroughput tombstone every retirement -> FAIL "settled requests must leave nothing behind" now PASS TestTimedOutRequestStopsShieldingPeerWhenIdle drop the lookup-time expiry -> FAIL "An error is expected but got nil" now PASS TestRetirementReclaimsExpiredTombstones remove the sweep on retire -> FAIL on retained count now PASS TestDuplicateResponseDoesNotBlockResolver now also asserts the first response survives; drop-both -> FAIL "the first response was dropped along with the duplicate", and a blocking send still -> FAIL on the 2s guard now PASS TestRetirePendingKeepsChannelSendable covers both retirement paths. Full package suite completes in ~10s under -race -tags deadlock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

QuantumExplorer requested a review from lklimek as a code owner July 25, 2026 09:59

QuantumExplorer mentioned this pull request Jul 25, 2026

perf(blocksync): remove redundant work from the block apply path #1394

Merged

5 tasks

This was referenced Jul 25, 2026

chore: bump Go to 1.26.5 #1395

Merged

fix(blocksync): survive transient peer failures and stalls #1396

Merged

lklimek mentioned this pull request Jul 28, 2026

build(deps): Bump github.com/adlio/schema from 1.5.0 to 1.5.1 #1381

Merged

lklimek previously requested changes Jul 28, 2026

View reviewed changes

lklimek and others added 2 commits July 30, 2026 10:05

Merge branch 'v1.6-dev' into fix/blocksync-duplicate-block-peer-punis…

403541e

…hment

thepastaclaw suggested changes Aug 3, 2026

View reviewed changes

Comment thread internal/blocksync/block_fetch_job.go

QuantumExplorer and others added 2 commits August 4, 2026 11:28

shumkov approved these changes Aug 8, 2026

View reviewed changes

shumkov enabled auto-merge (squash) August 8, 2026 13:31

shumkov merged commit a99ff2a into v1.6-dev Aug 8, 2026
22 of 23 checks passed

shumkov deleted the fix/blocksync-duplicate-block-peer-punishment branch August 8, 2026 13:31

shumkov mentioned this pull request Aug 17, 2026

feat: consensus, p2p and sync hardening for v1.7.0 #1408

Merged

Navigation Menu

Uh oh!

fix(blocksync): don't punish peers for duplicate block responses - #1393

fix(blocksync): don't punish peers for duplicate block responses#1393
shumkov merged 6 commits into
v1.6-devfrom
fix/blocksync-duplicate-block-peer-punishment

QuantumExplorer commented Jul 25, 2026

Uh oh!

coderabbitai Bot commented Jul 25, 2026 •

edited

Loading

Review skipped

Uh oh!

QuantumExplorer commented Jul 25, 2026

Uh oh!

lklimek commented Jul 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thepastaclaw commented Jul 30, 2026 •

edited

Loading

Uh oh!

thepastaclaw left a comment

Uh oh!

Uh oh!

QuantumExplorer commented Aug 4, 2026

Uh oh!

shumkov commented Aug 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

QuantumExplorer commented Jul 25, 2026

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

coderabbitai Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

QuantumExplorer commented Jul 25, 2026

CI follow-up

tests (01) — flaky, not caused by this PR. Re-running.

govulncheck — pre-existing, needs a toolchain bump

Uh oh!

lklimek commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Audit Summary — PR #1393 fix(blocksync): don't punish peers for duplicate block responses

Overall assessment

Findings

Pre-existing / outside-diff

Positive observations

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thepastaclaw commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Preliminary review — Codex only

Review provenance

Uh oh!

Uh oh!

QuantumExplorer commented Aug 4, 2026

Uh oh!

shumkov commented Aug 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coderabbitai Bot commented Jul 25, 2026 •

edited

Loading

`tests (01)` — flaky, not caused by this PR. Re-running.

`govulncheck` — pre-existing, needs a toolchain bump

lklimek commented Jul 28, 2026 •

edited

Loading

Audit Summary — PR #1393 `fix(blocksync): don't punish peers for duplicate block responses`

thepastaclaw commented Jul 30, 2026 •

edited

Loading