Merge pull request #3763 from ethereum-optimism/jg/drop_invalid_batch

op-node,specs: Drop invalid batches

op-node/rollup/derive/engine_queue.go

@@ -9,7 +9,6 @@ import (
 
 	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
-	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/log"
 
@@ -404,21 +403,28 @@ func (eq *EngineQueue) forceNextSafeAttributes(ctx context.Context) error {
 		case BlockInsertPrestateErr:
 			return NewResetError(fmt.Errorf("need reset to resolve pre-state problem: %w", err))
 		case BlockInsertPayloadErr:
-			eq.log.Warn("could not process payload derived from L1 data", "err", err)
-			// filter everything but the deposits
-			var deposits []hexutil.Bytes
+			eq.log.Warn("could not process payload derived from L1 data, dropping batch", "err", err)
+			// Count the number of deposits to see if the tx list is deposit only.
+			depositCount := 0
 			for _, tx := range attrs.Transactions {
 				if len(tx) > 0 && tx[0] == types.DepositTxType {
-					deposits = append(deposits, tx)
+					depositCount += 1
 				}
 			}
-			if len(attrs.Transactions) > len(deposits) {
-				eq.log.Warn("dropping sequencer transactions from payload for re-attempt, batcher may have included invalid transactions",
-					"txs", len(attrs.Transactions), "deposits", len(deposits), "parent", eq.safeHead)
-				eq.safeAttributes[0].Transactions = deposits
-				return nil
+			// Deposit transaction execution errors are suppressed in the execution engine, but if the
+			// block is somehow invalid, there is nothing we can do to recover & we should exit.
+			// TODO: Can this be triggered by an empty batch with invalid data (like parent hash or gas limit?)
+			if len(attrs.Transactions) == depositCount {
+				eq.log.Error("deposit only block was invalid", "parent", eq.safeHead, "err", err)
+				return NewCriticalError(fmt.Errorf("failed to process block with only deposit transactions: %w", err))
 			}
-			return NewCriticalError(fmt.Errorf("failed to process block with only deposit transactions: %w", err))
+			// drop the payload without inserting it
+			eq.safeAttributes = eq.safeAttributes[1:]
+			// suppress the error b/c we want to retry with the next batch from the batch queue
+			// If there is no valid batch the node will eventually force a deposit only block. If
+			// the deposit only block fails, this will return the critical error above.
+			return nil
+
 		default:
 			return NewCriticalError(fmt.Errorf("unknown InsertHeadBlock error type %d: %w", errType, err))
 		}

specs/derivation.md

@@ -545,8 +545,9 @@ During the *Batch Buffering* stage, we reorder batches by their timestamps. If b
 slots][g-time-slot] and a valid batch with a higher timestamp exists, this stage also generates empty batches to fill
 the gaps.
 
-Batches are pushed to the next stage whenever there is one or more sequential batch(es) directly following the timestamp
+Batches are pushed to the next stage whenever there is one sequential batch directly following the timestamp
 of the current [safe L2 head][g-safe-l2-head] (the last block that can be derived from the canonical L1 chain).
+The parent hash of the batch must also match the hash of the current safe L2 head.
 
 Note that the presence of any gaps in the batches derived from L1 means that this stage will need to buffer for a whole
 [sequencing window][g-sequencing-window] before it can generate empty batches (because the missing batch(es) could have
@@ -653,6 +654,12 @@ If consolidation fails, the unsafe L2 head is reset to the safe L2 head.
 If the safe and unsafe L2 heads are identical (whether because of failed consolidation or not), we send the block to the
 execution engine to be converted into a proper L2 block, which will become both the new L2 safe and unsafe head.
 
+If a payload attributes created from a batch cannot be inserted into the chain because of a validation error (i.e. there
+was an invalid transaction or state transition in the block) the batch should be dropped & the safe head should not be
+advanced. The engine queue will attempt to use the next batch for that timestamp from the batch queue. If no valid batch
+is found, the rollup node will create a deposit only batch which should always pass validation because deposits are
+always valid.
+
 Interaction with the execution engine via the execution engine API is detailed in the [Communication with the Execution
 Engine][exec-engine-comm] section.