Skip to content

feat(worker): disable l1 message skipping #1205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

feat(worker): disable l1 message skipping #1205

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions miner/scroll_worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,10 @@ func (w *worker) processTxns(txs types.OrderedTransactionSet) (bool, error) {
}

if tx.IsL1MessageTx() {
txs.Shift()
Copy link
Member

@georgehao georgehao Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if remove this, will ErrUnexpectedL1MessageIndex stop the L1 message to insert ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think tx.Shift() shouldn't be removed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understand. But I'm curious why it can be skipped to next before, but now we need to fix it immediately

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go-ethereum/miner/scroll_worker.go

Lines 816 to 819 in cdd77c1

if tx.IsL1MessageTx() && tx.AsL1MessageTx().QueueIndex != w.current.nextL1MsgIndex {
// Continue, we might still be able to include some L2 messages
return false, ErrUnexpectedL1MessageIndex
}

I think this error doesn't related to CCC

log.Error("Encountered error while processing L1MessageTx", "err", err, "blockNumber", w.current.header.Number, "queueIndex", tx.AsL1MessageTx().QueueIndex)
// Instead of skipping a message or stopping block building,
// we continue and build a block with no (more) L1 messages.
return false, nil
} else {
txs.Pop()
}
Expand Down Expand Up @@ -1061,13 +1064,8 @@ func (w *worker) onTxFailing(txIndex int, tx *types.Transaction, err error) {
return
}

queueIndex := tx.AsL1MessageTx().QueueIndex
log.Warn("Skipping L1 message", "queueIndex", queueIndex, "tx", tx.Hash().String(), "block",
w.current.header.Number, "reason", err)
rawdb.WriteSkippedTransaction(w.eth.ChainDb(), tx, nil, err.Error(),
w.current.header.Number.Uint64(), nil)
w.current.nextL1MsgIndex = queueIndex + 1
l1SkippedCounter.Inc(1)
// previously we would skip this transaction here,
// but now skipping is disabled.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add an error log such as "Unexpected case..." and print basic info of this L1 message.

Copy link
Author

@Thegaram Thegaram Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Preview

Copilot AI Jun 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
// 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.

} else if errors.Is(err, core.ErrInsufficientFunds) {
log.Trace("Skipping tx with insufficient funds", "tx", tx.Hash().String())
w.eth.TxPool().RemoveTx(tx.Hash(), true)
Expand Down
Loading