You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Recover transaction propagation after full gossip queues or initial sync, and recover block fetches after silent peers without timeout-based jailing.
Select bounded rebroadcast batches fairly by oldest attempt/admission time. Track attempts separately from queue acceptance, respect retry intervals and maximum age, and clean up tracking when transactions leave the pool.
Announce pending transactions to connected peers after initial sync, retrying queue-rejected hashes with bounded batches and per-peer cursors. Recheck pool membership and private/conditional filters before each retry.
Reserve privacy before local admission and commit it only on success. Private submissions bypass the public retry journal. Relay-enabled nodes still forward temporary local-pool rejections to producers; permanent rejection and producer-only rejection remain errors.
Bound header/body fetches to 16 per peer and 256 overall, retain alternative announcers, and release only matching request ownership. Discarding already-known or too-old queued blocks now clears every fetch stage and retained announcement credit.
Keep fresh block gather deadlines independent of deferred retries. New announcements can bring the timer forward without resetting existing backoff or scanning every retained hash.
Apply a bounded, non-escalating 10-second cooldown after block-fetch timeouts. Downloader timeouts retain their 10-second backoff without adding jail strikes. Non-timeout failure policy remains unchanged.
Latest review fixes
Actual peer-queue offers drive rebroadcast attempt accounting. Suppressed batches retain their retry opportunity; rejected queue offers still count for fairness.
Private transactions return already known for exact pool-resident duplicates without entering the public journal. Public re-admission while an absent transaction remains private returns a conflict.
Private markers expire after 30 minutes, with a one-minute sweep interval, allowing stalled pool-resident transactions to become publicly eligible. Pool retention is unchanged. Active admissions and refreshed markers remain protected. See private transaction lifetime.
Bodies waiting on cooling suppliers recover through healthy retained alternatives. Fresh body deadlines remain independent of deferred retry backoff.
Latest fixes: bdd681bab. Full short race suites for eth and eth/relay passed, final focused race regressions passed, and scoped golangci-lint v2.11.4 reports zero new issues. Diffguard against 27a2047b9 passed with all 18/18 mutations killed (tier-1 13/13). Fresh CI must validate this revision.
Full fetcher short race suite and focused scheduling/cleanup race tests pass.
Scoped lint reports zero issues; formatting and diff checks pass.
Diffguard with mutations enabled against c15ccd87b: tier-1 logic 11/11 (100%), total 25/27 (92.6%), exit 0. Two statement/branch-deletion survivors remain; the installed tool cannot apply statement deletions inside select clauses. No thresholds were changed.
The preceding full-branch Diffguard run against origin/develop also passed: tier-1 159/169 (94.1%), total 364/426 (85.4%). Earlier revisions passed race regressions for downloader, pool, relay, sync completion, and queue recovery. Existing churn warnings remain in large functions.
Current-head CI must validate repository-wide tests, integration/e2e, and security checks. Local regressions do not establish live-network performance or guarantee transaction inclusion.
Rollout
No consensus rule, hardfork activation, wire format, database migration, or operator configuration changes. Existing rebroadcast settings continue to apply. Timeout-only peers become eligible again after the 10-second cooldown. Private submissions now have a 30-minute window before public fallback; callers must not assume indefinite confidentiality. Exact pool-resident duplicates preserve the already known response used by preconfirmation relays.
❌ Patch coverage is 97.69585% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.95%. Comparing base (1cdf43c) to head (eaeae9c). ⚠️ Report is 4 commits behind head on develop.
The reason will be displayed to describe this comment to others. Learn more.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (1):
🔴 eth/relay/private_tx_store.go:197 — Private transactions that block producers reject can now stay hidden in the relay's pool indefinitely. They are never g…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
🔴 eth/fetcher/block_fetcher.go — Nodes that catch up through the downloader can permanently keep other peers' block announcements and their quota credits. Eventually announcements from new peers are refused. When a fetcher-queued block is popped as already known or too old, line 577 calls only forgetBlock. The stale f.completing[hash] keeps fetchInProgress true, so the hash's alternatives in f.announced are deferred forever. Fix: clear every stage for a discarded queued block (call forgetHash along with forgetBlock, as the done case does), so that alternatives and their f.announces credits are released.
Why this was flagged
The fetcher fetches block B (within maxQueueDist) and its body. B is enqueued with f.completing[B] set and waits because number > height+1 (block_fetcher.go:562). While B sits in completing/queued, other peers announce B. hasAnnouncer (block_fetcher_alternates.go:51) only rejects the same origin, so each new announcer is appended to f.announced[B] and f.announces[origin] is incremented (block_fetcher.go:634-635). The downloader then imports B. The import loop sees getBlock(B) != nil or number+maxUncleDist < height and calls only forgetBlock (block_fetcher.go:575-578). From then on, prepareHeaderFetch returns (nil, true) at fetchInProgress (block_fetcher_schedule.go:64-66) before readyToFetch can drop B, so the alternatives and their credits are never released. On the base branch the stale completing entry made the notify handler ignore further announcements of B, so no announcement or credit was retained.
Verification: When the queued B is popped as already known or too old, block_fetcher.go:575-577 calls only f.forgetBlock(hash). That removes queued/queues and leaves f.completing[B] behind permanently. Each other peer's announce for B is therefore appended, and f.announces[origin] = count is charged (block_fetcher.go:634-635).
The reason will be displayed to describe this comment to others. Learn more.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Fixed the out-of-diff queued-block cleanup finding in 2b88eb6. When a queued block is already known or too old, the import loop now calls both forgetHash and forgetBlock. This clears stale completion state, retained alternatives, and their announcement credits as well as queue ownership. TestBlockFetchDiscardReleasesAlternatives drives the real fetcher loop for both cases and verifies stage/quota cleanup.
The private-retention recovery finding remains open. The two reviews request conflicting policies: automatic public fallback versus preserving the private payload. I have asked the PR author to choose the policy; this commit does not silently publish private transactions.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The acknowledged private-transaction recovery policy after producer retry exhaustion still needs an explicit decision, implementation, and tests before merging.
The reason will be displayed to describe this comment to others. Learn more.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (1):
🔴 eth/relay/private_tx_store.go:197 — Private transactions that block producers reject can now stay hidden in the relay's pool indefinitely. They are never g…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The acknowledged private-transaction recovery policy after producer retries exhaust still needs agreement, implementation, and regression coverage before merging.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Bounded private-transaction recovery after producer retry exhaustion remains an acknowledged merge blocker requiring an agreed policy, implementation, and regression tests.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Cross-component retry scheduling and confidentiality changes require human sign-off and current-head CI validation.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Clarify privacy marker expiry for pool-resident transactions
eth/relay/CLAUDE.md:84
This guidance says privacy markers remain while the pool retains a transaction, but privateTxExpired now expires them after 30 minutes even for pool-resident transactions. The original payload then becomes eligible for public propagation without a replacement or public resubmission. Document the bounded privacy window and the in-flight admission exception here, consistent with docs/private-transactions.md, so the recovery guidance does not imply indefinite confidentiality.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Privacy-sensitive public fallback and concurrent fetch scheduling require human sign-off and current-head integration validation.
No open issues 🎉
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Recover transaction propagation after full gossip queues or initial sync, and recover block fetches after silent peers without timeout-based jailing.
Latest review fixes
already knownfor exact pool-resident duplicates without entering the public journal. Public re-admission while an absent transaction remains private returns a conflict.Latest fixes:
bdd681bab. Full short race suites forethandeth/relaypassed, final focused race regressions passed, and scoped golangci-lint v2.11.4 reports zero new issues. Diffguard against27a2047b9passed with all 18/18 mutations killed (tier-1 13/13). Fresh CI must validate this revision.Validation
For the latest follow-up:
c15ccd87b: tier-1 logic 11/11 (100%), total 25/27 (92.6%), exit 0. Two statement/branch-deletion survivors remain; the installed tool cannot apply statement deletions inside select clauses. No thresholds were changed.The preceding full-branch Diffguard run against
origin/developalso passed: tier-1 159/169 (94.1%), total 364/426 (85.4%). Earlier revisions passed race regressions for downloader, pool, relay, sync completion, and queue recovery. Existing churn warnings remain in large functions.Current-head CI must validate repository-wide tests, integration/e2e, and security checks. Local regressions do not establish live-network performance or guarantee transaction inclusion.
Rollout
No consensus rule, hardfork activation, wire format, database migration, or operator configuration changes. Existing rebroadcast settings continue to apply. Timeout-only peers become eligible again after the 10-second cooldown. Private submissions now have a 30-minute window before public fallback; callers must not assume indefinite confidentiality. Exact pool-resident duplicates preserve the
already knownresponse used by preconfirmation relays.