fix(tx-submitter): re-check submitter activity before each submit, and never rough-estimate past a revert - #1048
Conversation
…d never rough-estimate past a revert commitBatch / commitState / finalizeBatch all carry onlyActiveSubmitter on L1, but the activity probe only ran once, in PreCheck at Start(). A submitter can be removed, slashed, priced out by a raised minimum stake, or start withdrawing at any point afterwards, and the rollup / finalize loops kept submitting against a stale startup result. With rough_estimate_gas enabled this became a funds leak rather than a stall: eth_estimateGas fails on the revert, the rough fallback swallowed that failure and guessed a gas limit, and the tx was signed and sent anyway. The reverting tx refunds unused execution gas, but a commitBatch blob tx is charged its full blob fee regardless of revert. Nothing broke the cycle — the failed receipt only logs a warning, and once the tx leaves the pending pool the loop re-derives the same batch index and resends, with no low-balance guard. - extract ensureActiveSubmitter and call it at the top of rollup() and finalize(); an RPC failure on the probe also stops submission rather than assuming the wallet is still eligible - gate both rough-estimate fallbacks on utils.IsExecutionRevertErr, so the flag still covers a flaky/unreachable node but never guesses past a contract rejection Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesRollup submitter safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The change reduces repeated submissions after submitter deactivation and recognized execution reverts, but pending transaction replacements can still be sent after deactivation, and some contract-rejection responses may still be mistaken for temporary estimation failures. This can cause unauthorized or reverted fee-bearing transactions, so the PR needs follow-up or explicit owner acceptance before merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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. Comment |
2 participants
Problem
commitBatch/commitState/finalizeBatchall carryonlyActiveSubmitter(
Rollup.sol:119-120, applied at:257,:275,:598),but tx-submitter probed activity exactly once —
PreCheck()atStart()(
rollup.go:148). Therollup()andfinalize()loops then submitted foreveragainst that stale startup result.
A submitter can leave the active set at any point after startup:
removeSubmitter(Submitter.sol:83), self-servicewithdraw()(:120),slashafter a successful challenge (:146), orsetMinimumStakeraising thebar (
:92).With
rough_estimate_gasenabled this is a funds leak, not just a stall:eth_estimateGasfails on the revert.(
rollup.go:1013,:844).A revert refunds unused execution gas, but a
commitBatchblob tx(
createBlobTx,rollup.go:1088) is charged its full blob fee regardless ofwhether execution reverted — that is the real drain.
Nothing broke the cycle. The failed receipt only produces a
log.Warn(
rollup.go:696-706); after 6 confirmations the tx leaves the pending pool(
rollup.go:421), the loop re-derives the samebatchIndex(rollup.go:923-931)and resends.
MaxTxsInPendingPoolcaps in-flight txs, not cumulative attempts,and wallet balance is only exported as a metric — there is no low-balance guard.
Fix
ensureActiveSubmitter()and call it at the top ofrollup()andfinalize(). An RPC failure on the probe also stops submission, rather thanassuming the wallet is still eligible.
utils.IsExecutionRevertErr, sothe flag still does what it was added for (flaky / unreachable node) but never
guesses past a contract rejection.
The added per-tick
eth_callis one extra read per rollup/finalize interval.Tests
tx-submitter/services/rollup_submitter_activity_test.go,tx-submitter/utils/errors_test.go. Each new test was confirmed to fail with thefix reverted.
go test ./tx-submitter/...passes.mock.L1ClientWrappergains anEstimateGasErrfield so the estimate-failurepaths are reachable from tests.
Note on the second audit finding
The same audit reported, at LOW, that
InitAndSyncFromDatabase's committed-windowloop validates middle batches only against the persisted
Hashfield withoutindependently recomputing
header.Hash(). On verification this is a falsepositive and no change is included for it.
LoadAllSealedBatchesAndHeader— the first step ofInitAndSyncFromDatabase—already re-hashes every loaded header and compares it to that batch's persisted
Hash(common/batch/batch_storage.go:209-221). Composed with the window loop'sbatches[i].Hash == CommittedBatches(i), this yieldskeccak(headers[i]) == CommittedBatches(i)across the whole committed window.There is no gap, and adding a second re-hash in
batch_cache.gowould be deadweight.
🤖 Generated with Claude Code
Summary by CodeRabbit