op-node: Simplify Receipts Fetching API (#3639)

* op-node: Simplify Receipts Fetching API

The previous API would return a receipts fetcher that the caller
was responsible for iterating through and then calling `Result` on.
This moves this inside the `Fetch` function. This simplifies usage
of the API and does not result in performance regression because
the receipts hash check requires all receipts to be fetched before

* Cache receipts fetcher rather than receipts

This modifies the refactor to be equivalent to the previous code.
Caching the receipts fetcher is actually critical for performance
because the fetcher caches intermediate results. The fetcher typically
makes lots of calls to fetch all the receipts so if all work is thrown
out on a single failure it will be very hard for the node to make progress.

* Rename Fetch -> FetchReceipts

op-node/rollup/derive/attributes.go

@@ -3,7 +3,6 @@ package derive
 import (
 	"context"
 	"fmt"
-	"io"
 
 	"github.com/ethereum-optimism/optimism/op-node/eth"
 	"github.com/ethereum-optimism/optimism/op-node/rollup"
@@ -15,7 +14,7 @@ import (
 // L1ReceiptsFetcher fetches L1 header info and receipts for the payload attributes derivation (the info tx and deposits)
 type L1ReceiptsFetcher interface {
 	InfoByHash(ctx context.Context, hash common.Hash) (eth.BlockInfo, error)
-	Fetch(ctx context.Context, blockHash common.Hash) (eth.BlockInfo, types.Transactions, eth.ReceiptsFetcher, error)
+	FetchReceipts(ctx context.Context, blockHash common.Hash) (eth.BlockInfo, types.Receipts, error)
 }
 
 // PreparePayloadAttributes prepares a PayloadAttributes template that is ready to build a L2 block with deposits only, on top of the given l2Parent, with the given epoch as L1 origin.
@@ -32,7 +31,7 @@ func PreparePayloadAttributes(ctx context.Context, cfg *rollup.Config, dl L1Rece
 	// case we need to fetch all transaction receipts from the L1 origin block so we can scan for
 	// user deposits.
 	if l2Parent.L1Origin.Number != epoch.Number {
-		info, _, receiptsFetcher, err := dl.Fetch(ctx, epoch.Hash)
+		info, receipts, err := dl.FetchReceipts(ctx, epoch.Hash)
 		if err != nil {
 			return nil, NewTemporaryError(fmt.Errorf("failed to fetch L1 block info and receipts: %w", err))
 		}
@@ -41,17 +40,7 @@ func PreparePayloadAttributes(ctx context.Context, cfg *rollup.Config, dl L1Rece
 				fmt.Errorf("cannot create new block with L1 origin %s (parent %s) on top of L1 origin %s",
 					epoch, info.ParentHash(), l2Parent.L1Origin))
 		}
-		for {
-			if err := receiptsFetcher.Fetch(ctx); err == io.EOF {
-				break
-			} else if err != nil {
-				return nil, NewTemporaryError(fmt.Errorf("failed to fetch more receipts: %w", err))
-			}
-		}
-		receipts, err := receiptsFetcher.Result()
-		if err != nil {
-			return nil, NewResetError(fmt.Errorf("fetched bad receipt data: %w", err))
-		}
+
 		deposits, err := DeriveDeposits(receipts, cfg.DepositContractAddress)
 		if err != nil {
 			// deposits may never be ignored. Failing to process them is a critical error.

op-node/rollup/derive/attributes_test.go

@@ -36,7 +36,7 @@ func TestPreparePayloadAttributes(t *testing.T) {
 		l1Info := testutils.RandomBlockInfo(rng)
 		l1Info.InfoNum = l2Parent.L1Origin.Number + 1
 		epoch := l1Info.ID()
-		l1Fetcher.ExpectFetch(epoch.Hash, l1Info, nil, nil, nil)
+		l1Fetcher.ExpectFetchReceipts(epoch.Hash, l1Info, nil, nil)
 		_, err := PreparePayloadAttributes(context.Background(), cfg, l1Fetcher, l2Parent, l2Time, epoch)
 		require.NotNil(t, err, "inconsistent L1 origin error expected")
 		require.ErrorIs(t, err, ErrReset, "inconsistent L1 origin transition must be handled like a critical error with reorg")
@@ -63,7 +63,7 @@ func TestPreparePayloadAttributes(t *testing.T) {
 		epoch := l2Parent.L1Origin
 		epoch.Number += 1
 		mockRPCErr := errors.New("mock rpc error")
-		l1Fetcher.ExpectFetch(epoch.Hash, nil, nil, nil, mockRPCErr)
+		l1Fetcher.ExpectFetchReceipts(epoch.Hash, nil, nil, mockRPCErr)
 		_, err := PreparePayloadAttributes(context.Background(), cfg, l1Fetcher, l2Parent, l2Time, epoch)
 		require.ErrorIs(t, err, mockRPCErr, "mock rpc error expected")
 		require.ErrorIs(t, err, ErrTemporary, "rpc errors should not be critical, it is not necessary to reorg")
@@ -93,7 +93,7 @@ func TestPreparePayloadAttributes(t *testing.T) {
 		epoch := l1Info.ID()
 		l1InfoTx, err := L1InfoDepositBytes(0, l1Info)
 		require.NoError(t, err)
-		l1Fetcher.ExpectFetch(epoch.Hash, l1Info, nil, nil, nil)
+		l1Fetcher.ExpectFetchReceipts(epoch.Hash, l1Info, nil, nil)
 		attrs, err := PreparePayloadAttributes(context.Background(), cfg, l1Fetcher, l2Parent, l2Time, epoch)
 		require.NoError(t, err)
 		require.NotNil(t, attrs)
@@ -129,9 +129,7 @@ func TestPreparePayloadAttributes(t *testing.T) {
 
 		l2Txs := append(append(make([]eth.Data, 0), l1InfoTx), usedDepositTxs...)
 
-		// txs are ignored, API is a bit bloated to previous approach. Only l1Info and receipts matter.
-		l1Txs := make(types.Transactions, len(receipts))
-		l1Fetcher.ExpectFetch(epoch.Hash, l1Info, l1Txs, receipts, nil)
+		l1Fetcher.ExpectFetchReceipts(epoch.Hash, l1Info, receipts, nil)
 		attrs, err := PreparePayloadAttributes(context.Background(), cfg, l1Fetcher, l2Parent, l2Time, epoch)
 		require.NoError(t, err)
 		require.NotNil(t, attrs)

op-node/rollup/driver/sequencer.go

@@ -16,7 +16,7 @@ import (
 
 type Downloader interface {
 	InfoByHash(ctx context.Context, hash common.Hash) (eth.BlockInfo, error)
-	Fetch(ctx context.Context, blockHash common.Hash) (eth.BlockInfo, types.Transactions, eth.ReceiptsFetcher, error)
+	FetchReceipts(ctx context.Context, blockHash common.Hash) (eth.BlockInfo, types.Receipts, error)
 }
 
 // Sequencer implements the sequencing interface of the driver: it starts and completes block building jobs.

op-node/sources/eth_client.go

@@ -3,6 +3,7 @@ package sources
 import (
 	"context"
 	"fmt"
+	"io"
 
 	"github.com/ethereum-optimism/optimism/op-node/client"
 	"github.com/ethereum-optimism/optimism/op-node/eth"
@@ -80,7 +81,8 @@ type EthClient struct {
 	log log.Logger
 
 	// cache receipts in bundles per block hash
-	// common.Hash -> types.Receipts
+	// We cache the receipts fetcher to not lose progress when we have to retry the `Fetch` call
+	// common.Hash -> eth.ReceiptsFetcher
 	receiptsCache *caching.LRUCache
 
 	// cache transactions in bundles per block hash
@@ -230,21 +232,42 @@ func (s *EthClient) PayloadByLabel(ctx context.Context, label eth.BlockLabel) (*
 	return s.payloadCall(ctx, "eth_getBlockByNumber", string(label))
 }
 
-func (s *EthClient) Fetch(ctx context.Context, blockHash common.Hash) (eth.BlockInfo, types.Transactions, eth.ReceiptsFetcher, error) {
+// FetchReceipts returns a block info and all of the receipts associated with transactions in the block.
+// It verifies the receipt hash in the block header against the receipt hash of the fetched receipts
+// to ensure that the execution engine did not fail to return any receipts.
+func (s *EthClient) FetchReceipts(ctx context.Context, blockHash common.Hash) (eth.BlockInfo, types.Receipts, error) {
 	info, txs, err := s.InfoAndTxsByHash(ctx, blockHash)
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, nil, err
 	}
+	// Try to reuse the receipts fetcher because is caches the results of intermediate calls. This means
+	// that if just one of many calls fail, we only retry the failed call rather than all of the calls.
+	// The underlying fetcher uses the receipts hash to verify receipt integrity.
+	var fetcher eth.ReceiptsFetcher
 	if v, ok := s.receiptsCache.Get(blockHash); ok {
-		return info, txs, v.(eth.ReceiptsFetcher), nil
+		fetcher = v.(eth.ReceiptsFetcher)
+	} else {
+		txHashes := make([]common.Hash, len(txs))
+		for i := 0; i < len(txs); i++ {
+			txHashes[i] = txs[i].Hash()
+		}
+		fetcher = NewReceiptsFetcher(info.ID(), info.ReceiptHash(), txHashes, s.client.BatchCallContext, s.maxBatchSize)
+		s.receiptsCache.Add(blockHash, fetcher)
 	}
-	txHashes := make([]common.Hash, len(txs))
-	for i := 0; i < len(txs); i++ {
-		txHashes[i] = txs[i].Hash()
+	// Fetch all receipts
+	for {
+		if err := fetcher.Fetch(ctx); err == io.EOF {
+			break
+		} else if err != nil {
+			return nil, nil, err
+		}
 	}
-	r := NewReceiptsFetcher(info.ID(), info.ReceiptHash(), txHashes, s.client.BatchCallContext, s.maxBatchSize)
-	s.receiptsCache.Add(blockHash, r)
-	return info, txs, r, nil
+	receipts, err := fetcher.Result()
+	if err != nil {
+		return nil, nil, err
+	}
+
+	return info, receipts, nil
 }
 
 func (s *EthClient) GetProof(ctx context.Context, address common.Address, blockTag string) (*eth.AccountResult, error) {

op-node/testutils/mock_eth_client.go

@@ -105,13 +105,13 @@ func (m *MockEthClient) ExpectPayloadByLabel(label eth.BlockLabel, payload *eth.
 	m.Mock.On("PayloadByLabel", label).Once().Return(payload, &err)
 }
 
-func (m *MockEthClient) Fetch(ctx context.Context, blockHash common.Hash) (eth.BlockInfo, types.Transactions, eth.ReceiptsFetcher, error) {
-	out := m.Mock.MethodCalled("Fetch", blockHash)
-	return *out[0].(*eth.BlockInfo), out[1].(types.Transactions), out[2].(eth.ReceiptsFetcher), *out[3].(*error)
+func (m *MockEthClient) FetchReceipts(ctx context.Context, blockHash common.Hash) (eth.BlockInfo, types.Receipts, error) {
+	out := m.Mock.MethodCalled("FetchReceipts", blockHash)
+	return *out[0].(*eth.BlockInfo), out[1].(types.Receipts), *out[2].(*error)
 }
 
-func (m *MockEthClient) ExpectFetch(hash common.Hash, info eth.BlockInfo, transactions types.Transactions, receipts types.Receipts, err error) {
-	m.Mock.On("Fetch", hash).Once().Return(&info, transactions, eth.FetchedReceipts(receipts), &err)
+func (m *MockEthClient) ExpectFetchReceipts(hash common.Hash, info eth.BlockInfo, receipts types.Receipts, err error) {
+	m.Mock.On("FetchReceipts", hash).Once().Return(&info, receipts, &err)
 }
 
 func (m *MockEthClient) GetProof(ctx context.Context, address common.Address, blockTag string) (*eth.AccountResult, error) {