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
Keep transactions eligible for rebroadcast when peer queues refuse them, and prevent downloader timeouts from escalating to peer jailing. Rebroadcast batches rotate fairly by last offer/admission time; actual queue offers enforce retry intervals while queue acceptance determines the maximum-age cutoff.
Keep offer and acceptance tracking separate, with bounded batch selection and cleanup when transactions leave the pool. Suppressed, private, or peerless batches do not consume an offer.
Preserve retries for old transactions until a peer queue accepts them, including transactions that become executable after a fee drop.
Keep downloader timeout backoff at 10 seconds without adding jail strikes. Non-timeout failure escalation remains intact.
Executed tests
On the reduced scope:
Full short race suites passed for core/txpool/legacypool (142.708s), core/txpool (2.662s), eth (122.794s), and eth/downloader (206.847s).
Scoped golangci-lint v2.11.4 reports zero issues; workflow actionlint and diff checks pass.
Diffguard with mutations enabled against the PR merge base exits 0: tier-1 logic 41/41 (100%), overall 93/101 (92.1%). Eight statement/call/boolean mutations survive; tier-2 and existing churn warnings remain. No thresholds were relaxed.
Fresh CI must validate the reduced revision.
Rollout notes
No consensus, wire-format, database, public API, or configuration changes. Existing rebroadcast interval, maximum-age, and batch-size settings apply. Downloader timeout-only failures retain their 10-second backoff without jail escalation.
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.82%. Comparing base (1cdf43c) to head (e7646d3). ⚠️ 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.
🔵 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.
I reviewed the latest revision and found no new bugs. Because this is a large change (76 files, about 7k lines) covering tx propagation, block-fetch scheduling and the private-transaction privacy lifecycle, a human look would still be worthwhile.
What was reviewed:
The rebroadcast max-age gate in core/txpool/legacypool/rebroadcast_selection.go now keys on lastRebroadcast, so a refused queue offer no longer uses up the one late propagation attempt.
The updated "Failure recovery" text in eth/relay/CLAUDE.md now describes the 30-minute marker expiry and public fallback, which matches private_tx_store.go and docs/private-transactions.md.
The hunt also covered the private admission reserve/commit path, fetcher slot bounds and release ownership, and post-sync announcement retries. It found nothing new there.
Extended reasoning...
The PR changes legacypool rebroadcast selection and attempt tracking, the post-sync pending announcements in eth, the block fetcher and downloader timeout/backoff handling, and private-transaction admission in eth/relay and internal/ethapi. The security-sensitive part is data exposure: private transactions now become eligible for public gossip after a 30-minute marker expiry, a design trade-off a maintainer should sign off on. The latest commit addresses the two issues raised in the previous automated review. Earlier threads were all resolved by the PR author rather than independently, and the size and peer-handling complexity rule out an automated approval.
The reason will be displayed to describe this comment to others. Learn more.
🟡 (optional) Transactions whose peer-queue offers were refused are now retried about every 60s, not every RebroadcastInterval (30s) as on the base branch. The attempt timestamp is written after the tick that selected the tx, so on the next tick now.Sub(last) < pool.config.RebroadcastInterval is still true and the tx is skipped. Fix: compare the tick or selection time to the tick or selection time, for example by passing the now from identifyStuckTransactions into the attempt record, or by allowing a small slack. The acceptance timestamp in lastRebroadcast has the same pre-existing skew, so the fix should cover both.
Why this was flagged
The pool loop's ticker (legacypool.go:461) fires at T. identifyStuckTransactions runs under RLock, and the batch then goes through rebroadcastAckFeed to stuckTxBroadcastLoop. queueTransactions calls the RebroadcastAttempt callback, which sets lastRebroadcastAttempt[hash] = now at T+δ (rebroadcast_attempt.go:36,44). δ covers the scan of every pending tx plus the handler's work, so it is milliseconds. The next tick computes now at about T+30s+ε (ε is wake-up jitter, microseconds). rebroadcastEligible (rebroadcast_selection.go:61) then sees now.Sub(last) = 30s-(δ-ε) < RebroadcastInterval and skips the tx. It becomes eligible only on the tick after that. On the base branch a refused tx left nothing recorded, so the next tick retried it after 30s. Now every refused offer waits two intervals, which halves retry frequency for stuck txs when peer queues are full and the pool holds fewer than RebroadcastBatchSize eligible txs. The tests set attempt times hours in the past (rebroadcast_scheduling_test.go:58,91,146), so none of them exercises the tick boundary.
Verification: Trigger: a stuck tx whose peer-queue offer is refused. identifyStuckTransactions takes now at tick T (rebroadcast_selection.go:41), and RebroadcastAttempt writes pool.lastRebroadcastAttempt[hash] later (rebroadcast_attempt.go:44), so now.Sub(last) < pool.config.RebroadcastInterval (line 61) is true on the next tick and the tx is skipped. On base, refused txs were eligible on every 30s tick.
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Summary
Keep transactions eligible for rebroadcast when peer queues refuse them, and prevent downloader timeouts from escalating to peer jailing. Rebroadcast batches rotate fairly by last offer/admission time; actual queue offers enforce retry intervals while queue acceptance determines the maximum-age cutoff.
Executed tests
On the reduced scope:
core/txpool/legacypool(142.708s),core/txpool(2.662s),eth(122.794s), andeth/downloader(206.847s).Fresh CI must validate the reduced revision.
Rollout notes
No consensus, wire-format, database, public API, or configuration changes. Existing rebroadcast interval, maximum-age, and batch-size settings apply. Downloader timeout-only failures retain their 10-second backoff without jail escalation.