ethclient/authclient/engine.go (1)

60-77: LGTM! Implementation is correct.

The method correctly implements the parent-hash-based assembly flow, properly marshaling transactions and calling the corresponding RPC endpoint.

The transaction marshaling logic (lines 65-72) is duplicated from AssembleL2Block (lines 16-23). Consider extracting this to a helper function to reduce duplication:

🔎 Optional refactor to extract transaction marshaling

+// marshalTransactions converts a slice of transactions to binary format
+func marshalTransactions(transactions types.Transactions) ([][]byte, error) {
+	txs := make([][]byte, 0, len(transactions))
+	for i, tx := range transactions {
+		bz, err := tx.MarshalBinary()
+		if err != nil {
+			return nil, fmt.Errorf("failed to marshal tx, index: %d, error: %v", i, err)
+		}
+		txs = append(txs, bz)
+	}
+	return txs, nil
+}
+
 // AssembleL2Block assembles L2 Block used for L2 sequencer to propose a block in L2 consensus progress
 func (ec *Client) AssembleL2Block(ctx context.Context, number *big.Int, transactions types.Transactions) (*catalyst.ExecutableL2Data, error) {
-	txs := make([][]byte, 0, len(transactions))
-	for i, tx := range transactions {
-		bz, err := tx.MarshalBinary()
-		if err != nil {
-			return nil, fmt.Errorf("failed to marshal tx, index: %d, error: %v", i, err)
-		}
-		txs = append(txs, bz)
-	}
+	txs, err := marshalTransactions(transactions)
+	if err != nil {
+		return nil, err
+	}
 	var result catalyst.ExecutableL2Data
-	err := ec.c.CallContext(ctx, &result, "engine_assembleL2Block", &catalyst.AssembleL2BlockParams{
+	err = ec.c.CallContext(ctx, &result, "engine_assembleL2Block", &catalyst.AssembleL2BlockParams{
 		Number:       number.Uint64(),
 		Transactions: txs,
 	})
 	return &result, err
 }

Then apply the same helper to AssembleL2BlockV2:

 func (ec *Client) AssembleL2BlockV2(ctx context.Context, parentHash common.Hash, transactions types.Transactions) (*catalyst.ExecutableL2Data, error) {
-	txs := make([][]byte, 0, len(transactions))
-	for i, tx := range transactions {
-		bz, err := tx.MarshalBinary()
-		if err != nil {
-			return nil, fmt.Errorf("failed to marshal tx, index: %d, error: %v", i, err)
-		}
-		txs = append(txs, bz)
-	}
+	txs, err := marshalTransactions(transactions)
+	if err != nil {
+		return nil, err
+	}
 	var result catalyst.ExecutableL2Data
-	err := ec.c.CallContext(ctx, &result, "engine_assembleL2BlockV2", parentHash, txs)
+	err = ec.c.CallContext(ctx, &result, "engine_assembleL2BlockV2", parentHash, txs)
 	return &result, err
 }

eth/catalyst/l2_api.go (1)

364-413: LGTM! Implementation correctly enables parent-hash-based block assembly.

The method properly handles parent lookup, transaction decoding, block building, and result caching. The nil check for the parent header (lines 372-374) prevents potential panics.

The implementation duplicates significant logic from AssembleL2Block (lines 67-114), including transaction decoding (lines 377-384 vs 75-82) and ExecutableL2Data construction (lines 395-412 vs 96-113). Consider extracting shared logic to reduce maintenance burden:

🔎 Recommended refactor to extract common logic

Add helper methods to reduce duplication:

// decodeTransactionsList decodes a slice of binary-encoded transactions
func decodeTransactionsList(encodedTxs [][]byte) (types.Transactions, error) {
	transactions := make(types.Transactions, 0, len(encodedTxs))
	for i, otx := range encodedTxs {
		var tx types.Transaction
		if err := tx.UnmarshalBinary(otx); err != nil {
			return nil, fmt.Errorf("transaction %d is not valid: %v", i, err)
		}
		transactions = append(transactions, &tx)
	}
	return transactions, nil
}

// buildExecutableL2Data constructs ExecutableL2Data from a block build result
func (api *l2ConsensusAPI) buildExecutableL2Data(newBlockResult *miner.BlockResult, procTime time.Duration) *ExecutableL2Data {
	withdrawTrieRoot := api.writeVerified(newBlockResult.State, newBlockResult.Block, newBlockResult.Receipts, procTime)
	return &ExecutableL2Data{
		ParentHash:   newBlockResult.Block.ParentHash(),
		Number:       newBlockResult.Block.NumberU64(),
		Miner:        newBlockResult.Block.Coinbase(),
		Timestamp:    newBlockResult.Block.Time(),
		GasLimit:     newBlockResult.Block.GasLimit(),
		BaseFee:      newBlockResult.Block.BaseFee(),
		Transactions: encodeTransactions(newBlockResult.Block.Transactions()),

		StateRoot:          newBlockResult.Block.Root(),
		GasUsed:            newBlockResult.Block.GasUsed(),
		ReceiptRoot:        newBlockResult.Block.ReceiptHash(),
		LogsBloom:          newBlockResult.Block.Bloom().Bytes(),
		NextL1MessageIndex: newBlockResult.Block.Header().NextL1MsgIndex,
		WithdrawTrieRoot:   withdrawTrieRoot,

		Hash: newBlockResult.Block.Hash(),
	}
}

Then refactor both methods:

 func (api *l2ConsensusAPI) AssembleL2Block(params AssembleL2BlockParams) (*ExecutableL2Data, error) {
 	log.Info("Assembling block", "block number", params.Number)
 	parent := api.eth.BlockChain().CurrentHeader()
 	expectedBlockNumber := parent.Number.Uint64() + 1
 	if params.Number != expectedBlockNumber {
 		log.Warn("Cannot assemble block with discontinuous block number", "expected number", expectedBlockNumber, "actual number", params.Number)
 		return nil, fmt.Errorf("cannot assemble block with discontinuous block number %d, expected number is %d", params.Number, expectedBlockNumber)
 	}
-	transactions := make(types.Transactions, 0, len(params.Transactions))
-	for i, otx := range params.Transactions {
-		var tx types.Transaction
-		if err := tx.UnmarshalBinary(otx); err != nil {
-			return nil, fmt.Errorf("transaction %d is not valid: %v", i, err)
-		}
-		transactions = append(transactions, &tx)
-	}
+	transactions, err := decodeTransactionsList(params.Transactions)
+	if err != nil {
+		return nil, err
+	}

 	start := time.Now()
 	newBlockResult, err := api.eth.Miner().BuildBlock(parent.Hash(), time.Now(), transactions)
 	if err != nil {
 		return nil, err
 	}

 	procTime := time.Since(start)
-	withdrawTrieRoot := api.writeVerified(newBlockResult.State, newBlockResult.Block, newBlockResult.Receipts, procTime)
-	return &ExecutableL2Data{
-		ParentHash:   newBlockResult.Block.ParentHash(),
-		Number:       newBlockResult.Block.NumberU64(),
-		Miner:        newBlockResult.Block.Coinbase(),
-		Timestamp:    newBlockResult.Block.Time(),
-		GasLimit:     newBlockResult.Block.GasLimit(),
-		BaseFee:      newBlockResult.Block.BaseFee(),
-		Transactions: encodeTransactions(newBlockResult.Block.Transactions()),
-
-		StateRoot:          newBlockResult.Block.Root(),
-		GasUsed:            newBlockResult.Block.GasUsed(),
-		ReceiptRoot:        newBlockResult.Block.ReceiptHash(),
-		LogsBloom:          newBlockResult.Block.Bloom().Bytes(),
-		NextL1MessageIndex: newBlockResult.Block.Header().NextL1MsgIndex,
-		WithdrawTrieRoot:   withdrawTrieRoot,
-
-		Hash: newBlockResult.Block.Hash(),
-	}, nil
+	return api.buildExecutableL2Data(newBlockResult, procTime), nil
 }

 func (api *l2ConsensusAPI) AssembleL2BlockV2(parentHash common.Hash, txs [][]byte) (*ExecutableL2Data, error) {
 	log.Debug("AssembleL2BlockV2", "parentHash", parentHash.Hex())

 	parent := api.eth.BlockChain().GetHeaderByHash(parentHash)
 	if parent == nil {
 		return nil, fmt.Errorf("parent block not found: %s", parentHash.Hex())
 	}

-	transactions := make(types.Transactions, 0, len(txs))
-	for i, otx := range txs {
-		var tx types.Transaction
-		if err := tx.UnmarshalBinary(otx); err != nil {
-			return nil, fmt.Errorf("transaction %d is not valid: %v", i, err)
-		}
-		transactions = append(transactions, &tx)
-	}
+	transactions, err := decodeTransactionsList(txs)
+	if err != nil {
+		return nil, err
+	}

 	start := time.Now()
 	newBlockResult, err := api.eth.Miner().BuildBlock(parentHash, time.Now(), transactions)
 	if err != nil {
 		return nil, err
 	}

 	procTime := time.Since(start)
-	withdrawTrieRoot := api.writeVerified(newBlockResult.State, newBlockResult.Block, newBlockResult.Receipts, procTime)
-
-	return &ExecutableL2Data{
-		ParentHash:   newBlockResult.Block.ParentHash(),
-		Number:       newBlockResult.Block.NumberU64(),
-		Miner:        newBlockResult.Block.Coinbase(),
-		Timestamp:    newBlockResult.Block.Time(),
-		GasLimit:     newBlockResult.Block.GasLimit(),
-		BaseFee:      newBlockResult.Block.BaseFee(),
-		Transactions: encodeTransactions(newBlockResult.Block.Transactions()),
-
-		StateRoot:          newBlockResult.Block.Root(),
-		GasUsed:            newBlockResult.Block.GasUsed(),
-		ReceiptRoot:        newBlockResult.Block.ReceiptHash(),
-		LogsBloom:          newBlockResult.Block.Bloom().Bytes(),
-		NextL1MessageIndex: newBlockResult.Block.Header().NextL1MsgIndex,
-		WithdrawTrieRoot:   withdrawTrieRoot,
-
-		Hash: newBlockResult.Block.Hash(),
-	}, nil
+	return api.buildExecutableL2Data(newBlockResult, procTime), nil
 }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

ethclient/authclient/engine.go (1)

69-81: Consider extracting shared transaction marshaling logic.

The transaction marshaling loop (lines 70-77) is identical to the one in AssembleL2Block (lines 16-23). Consider extracting this into a helper function to reduce duplication and simplify future maintenance.

♻️ Suggested helper extraction

+func marshalTransactions(transactions types.Transactions) ([][]byte, error) {
+	txs := make([][]byte, 0, len(transactions))
+	for i, tx := range transactions {
+		bz, err := tx.MarshalBinary()
+		if err != nil {
+			return nil, fmt.Errorf("failed to marshal tx, index: %d, error: %v", i, err)
+		}
+		txs = append(txs, bz)
+	}
+	return txs, nil
+}
+
 func (ec *Client) AssembleL2BlockV2(ctx context.Context, parentHash common.Hash, transactions types.Transactions) (*catalyst.ExecutableL2Data, error) {
-	txs := make([][]byte, 0, len(transactions))
-	for i, tx := range transactions {
-		bz, err := tx.MarshalBinary()
-		if err != nil {
-			return nil, fmt.Errorf("failed to marshal tx, index: %d, error: %v", i, err)
-		}
-		txs = append(txs, bz)
+	txs, err := marshalTransactions(transactions)
+	if err != nil {
+		return nil, err
 	}
 	var result catalyst.ExecutableL2Data
-	err := ec.c.CallContext(ctx, &result, "engine_assembleL2BlockV2", parentHash, txs)
+	err = ec.c.CallContext(ctx, &result, "engine_assembleL2BlockV2", parentHash, txs)
 	return &result, err
 }

The same helper could also be used by AssembleL2Block.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ethclient/authclient/engine.go` around lines 69 - 81, The transaction
marshaling loop in AssembleL2BlockV2 is duplicated with AssembleL2Block; extract
it into a shared helper (e.g., marshalTransactions or encodeTransactions) that
accepts transactions types.Transactions and returns ([][]byte, error),
performing the MarshalBinary per tx and returning a descriptive error on
failure, then replace the loop in both AssembleL2BlockV2 and AssembleL2Block to
call this helper and pass its result into ec.c.CallContext.

eth/catalyst/l2_api.go (1)

396-442: Significant code duplication with AssembleL2Block.

The new AssembleL2BlockV2 method shares substantial logic with AssembleL2Block:

• Transaction decoding (lines 406-413 vs 75-82)
• ExecutableL2Data construction (lines 424-441 vs 96-113)

Consider extracting shared logic into helper functions to improve maintainability. For example:

1. decodeTransactions already exists but a helper for building the ExecutableL2Data response could be added.
2. The common post-build flow (writeVerified + constructing return value) could be unified.

♻️ Suggested refactor for shared ExecutableL2Data construction

+func buildExecutableL2Data(block *types.Block, withdrawTrieRoot common.Hash) *ExecutableL2Data {
+	return &ExecutableL2Data{
+		ParentHash:         block.ParentHash(),
+		Number:             block.NumberU64(),
+		Miner:              block.Coinbase(),
+		Timestamp:          block.Time(),
+		GasLimit:           block.GasLimit(),
+		BaseFee:            block.BaseFee(),
+		Transactions:       encodeTransactions(block.Transactions()),
+		StateRoot:          block.Root(),
+		GasUsed:            block.GasUsed(),
+		ReceiptRoot:        block.ReceiptHash(),
+		LogsBloom:          block.Bloom().Bytes(),
+		NextL1MessageIndex: block.Header().NextL1MsgIndex,
+		WithdrawTrieRoot:   withdrawTrieRoot,
+		Hash:               block.Hash(),
+	}
+}

This helper can then be used by both AssembleL2Block and AssembleL2BlockV2.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@eth/catalyst/l2_api.go` around lines 396 - 442, AssembleL2BlockV2 duplicates
transaction decoding and ExecutableL2Data assembly from AssembleL2Block; extract
a shared helper(s) such as decodeTransactions (reuse existing
decodeTransactions) and a new buildExecutableL2Data(parentHash, newBlockResult,
procTime) that calls writeVerified(newBlockResult.State, newBlockResult.Block,
newBlockResult.Receipts, procTime) and constructs/returns the ExecutableL2Data
(setting ParentHash, Number, Miner, Timestamp, GasLimit, BaseFee, Transactions
via encodeTransactions, StateRoot, GasUsed, ReceiptRoot, LogsBloom,
NextL1MessageIndex, WithdrawTrieRoot and Hash). Replace the duplicated blocks in
both AssembleL2Block and AssembleL2BlockV2 to call decodeTransactions for tx
decoding and buildExecutableL2Data for the post-build flow.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

eth/catalyst/l2_api.go (1)

420-424: ⚠️ Potential issue | 🟠 Major

Use one explicit block timestamp for both the fork check and BuildBlock.

These two independent time.Now() reads can disagree at the Jade boundary: the guard can evaluate one second while BuildBlock seals the next. That makes fork-format selection non-deterministic and also means identical (parentHash, txs) inputs can produce different payloads depending on local clock time. Please thread a caller-supplied timestamp through V2 and use that same value in both places.

Suggested fix

-func (api *l2ConsensusAPI) AssembleL2BlockV2(parentHash common.Hash, txs [][]byte) (*ExecutableL2Data, error) {
+func (api *l2ConsensusAPI) AssembleL2BlockV2(parentHash common.Hash, timestamp uint64, txs [][]byte) (*ExecutableL2Data, error) {
 	api.newBlockLock.Lock()
 	defer api.newBlockLock.Unlock()

 	log.Info("AssembleL2BlockV2", "parentHash", parentHash.Hex())
+	blockTime := time.Unix(int64(timestamp), 0)

 	// Get parent block by hash
 	if api.eth.BlockChain().GetHeaderByHash(parentHash) == nil {
 		return nil, fmt.Errorf("parent block not found: %s", parentHash.Hex())
 	}
@@
-	if api.eth.BlockChain().Config().IsJadeFork(uint64(time.Now().Unix())) == api.eth.BlockChain().Config().Morph.UseZktrie {
+	if api.eth.BlockChain().Config().IsJadeFork(timestamp) == api.eth.BlockChain().Config().Morph.UseZktrie {
 		return nil, fmt.Errorf("cannot assemble block for fork, useZKtrie: %v, please switch geth", api.eth.BlockChain().Config().Morph.UseZktrie)
 	}

-	newBlockResult, err := api.eth.Miner().BuildBlock(parentHash, time.Now(), transactions)
+	newBlockResult, err := api.eth.Miner().BuildBlock(parentHash, blockTime, transactions)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@eth/catalyst/l2_api.go` around lines 420 - 424, The code uses two independent
time.Now() calls for the Jade fork check and for BuildBlock causing
nondeterminism; modify the API to accept a caller-supplied timestamp (e.g., ts
or blockTime) and thread that value into both the fork check and the call to
api.eth.Miner().BuildBlock(parentHash, ..., transactions) so you call
api.eth.BlockChain().Config().IsJadeFork(uint64(ts.Unix())) and pass the same ts
to BuildBlock instead of calling time.Now() twice; update the V2 entrypoint to
require this timestamp and propagate it through to where parentHash and
transactions are used.

eth/catalyst/l2_api.go (1)

396-449: Extract the shared assembly pipeline into one helper.

AssembleL2BlockV2 now duplicates most of AssembleL2Block's decode/build/cache/response flow, and the two entry points already diverged on locking. Pulling the common path behind a private helper would make future fixes land in one place instead of drifting again.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@eth/catalyst/l2_api.go` around lines 396 - 449, AssembleL2BlockV2 duplicates
most of AssembleL2Block; extract the shared decode/build/cache/response pipeline
into a private helper (e.g. func (api *l2ConsensusAPI)
assembleL2BlockCommon(parentHash common.Hash, timestamp *uint64, txs [][]byte)
(*ExecutableL2Data, error)) that performs: parent header check, transaction
unmarshalling into types.Transactions, timestamp resolution, call to
api.eth.Miner().BuildBlock, call to writeVerified(newBlockResult.State,...), and
construction of the ExecutableL2Data (use encodeTransactions,
NextL1MessageIndex, WithdrawTrieRoot, etc. as in current code). Keep locking
semantics in the two public entry points (AssembleL2Block and AssembleL2BlockV2)
by leaving api.newBlockLock.Lock()/Unlock() where they currently are and simply
call the new helper from each; ensure error paths and returned values are
preserved exactly.