feat: stop transaction rebroadcast while syncing - #2397
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2397 +/- ##
===========================================
+ Coverage 55.59% 55.60% +0.01%
===========================================
Files 918 918
Lines 167145 167168 +23
===========================================
+ Hits 92926 92959 +33
+ Misses 68740 68739 -1
+ Partials 5479 5470 -9
... and 20 files with indirect coverage changes
🚀 New features to boost your workflow:
|
vbhattaccmu
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The new Kurtosis observation script can exit prematurely under set -euo pipefail when the target service container is not found, preventing the intended error handling and making the scenario less reliable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR disables stuck-transaction rebroadcast as soon as the syncer detects a higher-TD peer (i.e., a catch-up sync is required), and only re-enables rebroadcast after a successful sync cycle, preventing out-of-date nodes from repeatedly clearing peers’ known-tx caches and re-announcing stale transactions. It also adds a Kurtosis-based local scenario to reproduce and observe the “hard-fork-stall + network delay” rebroadcast behavior.
Changes:
- Disable stuck-tx rebroadcast when
chainSyncer.nextSyncOpschedules a sync, and re-enable it viaenableSyncedFeatures()after successful sync. - Refactor stuck-tx rebroadcast into
handler.rebroadcastStuckTransactionsand update tests to validate the new gating behavior. - Add a repeatable Kurtosis scenario (README/params/observe script) to observe rebroadcast behavior under fork-stall and netem delay.
File summaries
| File | Description |
|---|---|
| tests/kurtosis/rebroadcast/README.md | Documents a local Kurtosis scenario for observing rebroadcast behavior on an out-of-sync node. |
| tests/kurtosis/rebroadcast/params.yml | Kurtosis params defining the devnet topology with a v2.9.0 baseline node and a candidate node. |
| tests/kurtosis/rebroadcast/observe.sh | Observation script that applies netem delay, optionally seeds a tx, and reports identified vs emitted rebroadcast batches. |
| eth/sync.go | Disables rebroadcast immediately when a higher-TD peer requires a sync. |
| eth/sync_test.go | Adds assertions that required sync disables rebroadcast and that a completed sync re-enables it. |
| eth/handler.go | Introduces rebroadcastOK and gates stuck-tx rebroadcast through rebroadcastStuckTransactions; enables rebroadcast after successful sync. |
| eth/handler_test.go | Updates tests to directly validate rebroadcast gating and that a tx message is emitted when rebroadcast is enabled. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@claude review |
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed compilation/type-mismatch issues in the new rebroadcast helper/tests plus a logic gap where benched higher-TD peers can still allow rebroadcast before a sync op is scheduled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
eth/handler_test.go:486
- After changing
rebroadcastStuckTransactionsto accept[]*types.Transaction(matchingcore.StuckTxsEvent.Txs), this test should pass a[]*types.Transactionslice instead oftypes.Transactions.
if handler.handler.rebroadcastStuckTransactions(types.Transactions{signedTx}) {
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Lite
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude review |
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes are small, well-scoped, and come with targeted unit tests and a reproducible Kurtosis scenario; only a minor test-strengthening nit was found.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
I reviewed this updated version and the bug hunt found no new bugs. The critical gap I flagged earlier — rebroadcastOK staying stuck disabled forever after a failed catch-up sync — is now fixed: onSyncDone re-evaluates the gate via updateRebroadcastStatus() on sync error, and nextSyncOp re-evaluates it on every loop iteration (peer events, force timer, retry timer), with a dedicated regression test (TestChainSyncerRebroadcastAfterSyncFailure) covering peer-drop, timeout, and cancellation cases. Because this touches sync-critical, CODEOWNER-owned code (eth/) with shared atomic state read from two goroutines, a human maintainer look is still worthwhile before merge.
Extended reasoning...
This is a follow-up review of a PR I previously reviewed. In my prior pass I flagged a critical correctness issue: rebroadcastOK was cleared before every sync attempt but only restored in enableSyncedFeatures, which is only reached on a successful LegacySync. A sync failure (peer drop, timeout, cancellation — all routine) would leave rebroadcastOK permanently false even once the node was caught up, silently disabling stuck-tx rebroadcast forever.
Since that review, commit 53a2dcc ("eth, tests/kurtosis: fix rebroadcast gating during peer backoff") directly addresses this: onSyncDone now calls cs.updateRebroadcastStatus() whenever err != nil, and nextSyncOp unconditionally calls updateRebroadcastStatus() at the top of every invocation — which happens on every iteration of chainSyncer.loop() (peer events, sync completion, the 10s force timer, and the retry timer), not just when a new sync is scheduled. This means the gate is periodically re-evaluated even without a fresh peer event, which also mitigates the earlier ruled-out concern about peerWithHighestTD(nil) not accounting for downloader backoff/jail state — a benched peer's stale TD claim will eventually be re-checked against current peer state on the next loop tick. A new test file (eth/sync_rebroadcast_test.go) adds TestChainSyncerRebroadcastAfterSyncFailure, which exercises exactly the scenario I flagged (initial-sync failure, catch-up peer drop, catch-up timeout, catch-up cancellation) and asserts the gate is correctly restored once no peer remains ahead, then re-disabled if a new higher-TD peer appears.
No new bugs were reported by this run's bug hunt, and the two ruled-out candidates from earlier runs (both about peerWithHighestTD(nil) ignoring backoff/jail) are effectively mitigated by the periodic re-evaluation added in this fix, since a stale claim from a backed-off peer is checked again on the next tick rather than being a one-shot decision.
Despite the fix being solid and test-backed, I am not approving. eth/ is listed in .github/CODEOWNERS under a named owner, and per this review's guidelines, CODEOWNER-owned paths should not be approved by this automated pass regardless of code quality. The change also touches concurrency-sensitive shared state (atomic.Bool read/written from the sync goroutine and the stuck-tx broadcast loop) in a consensus-adjacent path (transaction propagation during sync), which warrants a maintainer's sign-off even though the logic itself now looks correct and is well tested.
This review covers commit 53a2dcc, which is no longer the latest commit on this pull request; later commits are not covered by it.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes sync-driven transaction propagation behavior in the eth P2P path, which is operationally sensitive and should get final human review despite the added tests.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
eth/handler_test.go:487
- Grammar in this failure message is off; as written it reads like a present-tense verb phrase. Adjusting it makes the test output clearer when it fails.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Stops stuck-transaction rebroadcasting as soon as chain sync detects a higher-TD peer, and re-enables it only after a successful sync. This keeps the existing initial-sync transaction-acceptance behavior separate while preventing halted or lagging nodes from clearing every peer’s known-transaction cache and re-announcing stale transactions. Adds a repeatable local Kurtosis hard-fork-stall and netem-delay scenario.
Executed tests
make testmake test-integrationgo test -race ./eth -count=1 -timeout=20mmake lint-deps && make lint(0 issues)docker build -t bor:local --file Dockerfile .Repository-wide
go vet ./eth/...,gosec ./..., andmake vulncheckalso ran. They report pre-existing baseline findings: the tracerWriteTosignature warning, existing gosec findings, and Go 1.26.5 standard-library advisories fixed in Go 1.26.6. None point to changed files.Rollout notes
Backward-compatible and not consensus-affecting. No configuration or database migration is required. Out-of-sync nodes stop stuck-transaction rebroadcasts once a catch-up requirement is detected; normal rebroadcast eligibility resumes after a successful sync.