feat(worker): disable l1 message skipping #1205
Conversation
WalkthroughThe error handling for L1 message transactions in the worker has been updated. Now, when an error occurs during processing, the system logs the error and stops including further L1 messages, rather than skipping or advancing past the problematic transaction. Automatic skipping of failing L1 messages has been disabled. Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
miner/scroll_worker.go (1)
1066-1069: Remove now-dead skip-path for L1 messagesThe comment correctly states that skipping is disabled, but the surrounding
else-ifladder still contains the empty branch, which is easy to misread as an
oversight. Dropping the branch (or gating it behind a// deprecatedbuild tag)
tightens the control-flow and avoids future re-introductions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
miner/scroll_worker.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
miner/scroll_worker.go (1)
798-803: Silent drop of failing L1 message can stall the L1 queue indefinitelyWhen
processTxnreturns an error for anL1MessageTx, the code now logs and immediately
returns(false, nil), leaving:
w.current.nextL1MsgIndexunchanged – the same failing message will be fetched again next
block and hit the exact same error.- The offending tx still in
txs(noPop/Shift), so any subsequent L1-messages in the same
slice are never examined this block.This achieves the “do not skip” objective, but also means a single permanently-failing message
(e.g. malformed calldata) can block the entire L1 queue forever, effectively halting L1 → L2
ingestion.Please double-check that every error reaching this point is transient (e.g. state-dependent
gas overflow) and that you have operational alerts/metrics to detect prolonged stagnation of
nextL1MsgIndex.
If some errors are unrecoverable, consider:+// after N consecutive failures we can escalate / circuit-break +if l1FailureCounter > maxRetries { + return false, fmt.Errorf("L1 message %d keeps failing: %w", tx.AsL1MessageTx().QueueIndex, err) +}or persisting the failure in DB with
rawdb.WriteSkippedTransactionbut without advancing the
index, so that manual intervention is still possible.
| w.current.nextL1MsgIndex = queueIndex + 1 | ||
| l1SkippedCounter.Inc(1) | ||
| // previously we would skip this transaction here, | ||
| // but now skipping is disabled. |
There was a problem hiding this comment.
Could add an error log such as "Unexpected case..." and print basic info of this L1 message.
There was a problem hiding this comment.
The other error log added in this PR will be printed after the call to this function. Or maybe I can move that error here.
| @@ -796,7 +796,10 @@ func (w *worker) processTxns(txs types.OrderedTransactionSet) (bool, error) { | |||
| } | |||
|
|
|||
| if tx.IsL1MessageTx() { | |||
| txs.Shift() | |||
There was a problem hiding this comment.
if remove this, will ErrUnexpectedL1MessageIndex stop the L1 message to insert ?
There was a problem hiding this comment.
I think tx.Shift() shouldn't be removed.
There was a problem hiding this comment.
Calling Shift in case of an error means that we skipped the transaction and continue with the next one, which we should not do.
Instead, we'll be stuck at the erroring transaction, and we must fix that error before we continue processing L1 messages.
There was a problem hiding this comment.
understand. But I'm curious why it can be skipped to next before, but now we need to fix it immediately
There was a problem hiding this comment.
Now that we disalbed ccc, this (skipping) should never happen. But if it does, it's better if we still produce a valid chain, i.e. stop processing L1 messages instead of skipping them.
There was a problem hiding this comment.
go-ethereum/miner/scroll_worker.go
Lines 816 to 819 in cdd77c1
I think this error doesn't related to CCC
There was a problem hiding this comment.
Pull Request Overview
This PR disables automatic skipping of L1 message transactions during block building to prevent chain corruption if unexpected errors occur.
- Removed the
txs.Shift()skip inprocessTxnsand replaced it with an error log and early block finalization. - Stripped out the skipping logic in
onTxFailingfor L1 messages, leaving a placeholder comment.
Comments suppressed due to low confidence (2)
miner/scroll_worker.go:802
- Consider adding a unit or integration test to verify that when an L1 message transaction fails,
processTxnsreturns(false, nil)and halts further L1 processing without skipping.
return false, nil
miner/scroll_worker.go:799
- The variable
erris not defined in this scope, which will cause a compilation error. You should use the error returned by the L1 message processing call or capture it into a variable before logging.
log.Error("Encountered error while processing L1MessageTx", "err", err, "blockNumber", w.current.header.Number, "queueIndex", tx.AsL1MessageTx().QueueIndex)
| w.current.nextL1MsgIndex = queueIndex + 1 | ||
| l1SkippedCounter.Inc(1) | ||
| // previously we would skip this transaction here, | ||
| // but now skipping is disabled. |
There was a problem hiding this comment.
[nitpick] With skipping disabled, it would be helpful to log the failure of an L1 message here so that developers can trace why the block building halted.
| // but now skipping is disabled. | |
| // but now skipping is disabled. | |
| log.Warn("L1 message transaction failed without skipping", "tx", tx.Hash().String(), "error", err.Error()) |
Copilot uses AI. Check for mistakes.
1. Purpose or design rationale of this PR
CCC is disabled, so it won't lead to skipping.
There are other error conditions that might lead to skipping L1 messages. To the best of our knowledge, these are all impossible. But if we do encounter such an error, we should avoid corrupting the chain by skipping an L1 message.
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit