Fix: Refactor Receipt Validation to ReceiptsProvider (#9417)

* Refactor Receipt Validation to ReceiptsProvider

* PR Comments

* tweaks

* remove client validation test; client no longer owns validation

* add comments

op-service/sources/eth_client.go

@@ -317,16 +317,11 @@ func (s *EthClient) FetchReceipts(ctx context.Context, blockHash common.Hash) (e
 		return nil, nil, fmt.Errorf("querying block: %w", err)
 	}
 
-	txHashes, block := eth.TransactionsToHashes(txs), eth.ToBlockID(info)
-	receipts, err := s.recProvider.FetchReceipts(ctx, block, txHashes)
+	txHashes, _ := eth.TransactionsToHashes(txs), eth.ToBlockID(info)
+	receipts, err := s.recProvider.FetchReceipts(ctx, info, txHashes)
 	if err != nil {
 		return nil, nil, err
 	}
-
-	if err := validateReceipts(block, info.ReceiptHash(), txHashes, receipts); err != nil {
-		return info, nil, fmt.Errorf("invalid receipts: %w", err)
-	}
-
 	return info, receipts, nil
 }
 

op-service/sources/eth_client_test.go

@@ -180,95 +180,49 @@ func TestEthClient_WrongInfoByHash(t *testing.T) {
 	m.Mock.AssertExpectations(t)
 }
 
-type validateReceiptsTest struct {
-	desc        string
-	trustRPC    bool
-	mutReceipts func(types.Receipts) types.Receipts
-	expError    string
-}
-
-func TestEthClient_validateReceipts(t *testing.T) {
-	mutBloom := func(rs types.Receipts) types.Receipts {
-		rs[2].Bloom[0] = 1
-		return rs
-	}
-	mutTruncate := func(rs types.Receipts) types.Receipts {
-		return rs[:len(rs)-1]
-	}
-
-	for _, tt := range []validateReceiptsTest{
-		{
-			desc: "valid",
-		},
-		{
-			desc:        "invalid-mut-bloom",
-			mutReceipts: mutBloom,
-			expError:    "invalid receipts: failed to fetch list of receipts: expected receipt root",
-		},
-		{
-			desc:        "invalid-truncated",
-			mutReceipts: mutTruncate,
-			expError:    "invalid receipts: got 3 receipts but expected 4",
-		},
-	} {
-		t.Run("no-trust-"+tt.desc, func(t *testing.T) {
-			testEthClient_validateReceipts(t, tt)
-		})
-		// trusting the rpc should still lead to failed validation
-		tt.trustRPC = true
-		t.Run("trust-"+tt.desc, func(t *testing.T) {
-			testEthClient_validateReceipts(t, tt)
-		})
+func newEthClientWithCaches(metrics caching.Metrics, cacheSize int) *EthClient {
+	return &EthClient{
+		transactionsCache: caching.NewLRUCache[common.Hash, types.Transactions](metrics, "txs", cacheSize),
+		headersCache:      caching.NewLRUCache[common.Hash, eth.BlockInfo](metrics, "headers", cacheSize),
+		payloadsCache:     caching.NewLRUCache[common.Hash, *eth.ExecutionPayloadEnvelope](metrics, "payloads", cacheSize),
 	}
 }
 
-func testEthClient_validateReceipts(t *testing.T, test validateReceiptsTest) {
+// TestReceiptValidation tests that the receipt validation is performed by the underlying RPCReceiptsFetcher
+func TestReceiptValidation(t *testing.T) {
 	require := require.New(t)
 	mrpc := new(mockRPC)
-	mrp := new(mockReceiptsProvider)
-	const numTxs = 4
-	block, receipts := randomRpcBlockAndReceipts(rand.New(rand.NewSource(420)), numTxs)
-	txHashes := receiptTxHashes(receipts)
+	rp := NewRPCReceiptsFetcher(mrpc, nil, RPCReceiptsConfig{})
+	const numTxs = 1
+	block, _ := randomRpcBlockAndReceipts(rand.New(rand.NewSource(420)), numTxs)
+	//txHashes := receiptTxHashes(receipts)
 	ctx := context.Background()
 
-	if mut := test.mutReceipts; mut != nil {
-		receipts = mut(receipts)
-	}
-
-	mrpc.On("CallContext", ctx, mock.AnythingOfType("**sources.RPCBlock"),
-		"eth_getBlockByHash", []any{block.Hash, true}).
+	mrpc.On("CallContext",
+		ctx,
+		mock.Anything,
+		"eth_getTransactionReceipt",
+		mock.Anything).
+		Run(func(args mock.Arguments) {
+		}).
+		Return([]error{nil})
+
+	// when the block is requested, the block is returned
+	mrpc.On("CallContext",
+		ctx,
+		mock.Anything,
+		"eth_getBlockByHash",
+		mock.Anything).
 		Run(func(args mock.Arguments) {
 			*(args[1].(**RPCBlock)) = block
 		}).
-		Return([]error{nil}).Once()
-
-	mrp.On("FetchReceipts", ctx, block.BlockID(), txHashes).
-		Return(types.Receipts(receipts), error(nil)).Once()
+		Return([]error{nil})
 
 	ethcl := newEthClientWithCaches(nil, numTxs)
 	ethcl.client = mrpc
-	ethcl.recProvider = mrp
-	ethcl.trustRPC = test.trustRPC
-
-	info, recs, err := ethcl.FetchReceipts(ctx, block.Hash)
-	if test.expError != "" {
-		require.ErrorContains(err, test.expError)
-	} else {
-		require.NoError(err)
-		expInfo, _, err := block.Info(true, false)
-		require.NoError(err)
-		require.Equal(expInfo, info)
-		require.Equal(types.Receipts(receipts), recs)
-	}
-
-	mrpc.AssertExpectations(t)
-	mrp.AssertExpectations(t)
-}
+	ethcl.recProvider = rp
+	ethcl.trustRPC = true
 
-func newEthClientWithCaches(metrics caching.Metrics, cacheSize int) *EthClient {
-	return &EthClient{
-		transactionsCache: caching.NewLRUCache[common.Hash, types.Transactions](metrics, "txs", cacheSize),
-		headersCache:      caching.NewLRUCache[common.Hash, eth.BlockInfo](metrics, "headers", cacheSize),
-		payloadsCache:     caching.NewLRUCache[common.Hash, *eth.ExecutionPayloadEnvelope](metrics, "payloads", cacheSize),
-	}
+	_, _, err := ethcl.FetchReceipts(ctx, block.Hash)
+	require.ErrorContains(err, "unexpected nil block number")
 }

op-service/sources/receipts.go

@@ -14,7 +14,7 @@ type ReceiptsProvider interface {
 	// 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.
-	FetchReceipts(ctx context.Context, block eth.BlockID, txHashes []common.Hash) (types.Receipts, error)
+	FetchReceipts(ctx context.Context, blockInfo eth.BlockInfo, txHashes []common.Hash) (types.Receipts, error)
 }
 
 // validateReceipts validates that the receipt contents are valid.

op-service/sources/receipts_basic.go

@@ -31,7 +31,10 @@ func NewBasicRPCReceiptsFetcher(client rpcClient, maxBatchSize int) *BasicRPCRec
 	}
 }
 
-func (f *BasicRPCReceiptsFetcher) FetchReceipts(ctx context.Context, block eth.BlockID, txHashes []common.Hash) (types.Receipts, error) {
+// FetchReceipts fetches receipts for the given block and transaction hashes
+// it does not validate receipts, and expects the caller to do so
+func (f *BasicRPCReceiptsFetcher) FetchReceipts(ctx context.Context, blockInfo eth.BlockInfo, txHashes []common.Hash) (types.Receipts, error) {
+	block := eth.ToBlockID(blockInfo)
 	call := f.getOrCreateBatchCall(block.Hash, txHashes)
 
 	// Fetch all receipts

op-service/sources/receipts_basic_test.go

@@ -35,7 +35,6 @@ func TestBasicRPCReceiptsFetcher_Reuse(t *testing.T) {
 	require := require.New(t)
 	batchSize, txCount := 2, uint64(4)
 	block, receipts := randomRpcBlockAndReceipts(rand.New(rand.NewSource(123)), txCount)
-	blockid := block.BlockID()
 	txHashes := make([]common.Hash, 0, len(receipts))
 	recMap := make(map[common.Hash]*types.Receipt, len(receipts))
 	for _, rec := range receipts {
@@ -75,16 +74,18 @@ func TestBasicRPCReceiptsFetcher_Reuse(t *testing.T) {
 		return err
 	}
 
+	bInfo, _, _ := block.Info(true, true)
+
 	// 1st fetching should result in errors
-	recs, err := rp.FetchReceipts(ctx, blockid, txHashes)
+	recs, err := rp.FetchReceipts(ctx, bInfo, txHashes)
 	require.Error(err)
 	require.Nil(recs)
 	require.EqualValues(2, numCalls.Load())
 
 	// prepare 2nd fetching - all should succeed now
 	response[txHashes[2]] = true
 	response[txHashes[3]] = true
-	recs, err = rp.FetchReceipts(ctx, blockid, txHashes)
+	recs, err = rp.FetchReceipts(ctx, bInfo, txHashes)
 	require.NoError(err)
 	require.NotNil(recs)
 	for i, rec := range recs {
@@ -141,12 +142,13 @@ func runConcurrentFetchingTest(t *testing.T, rp ReceiptsProvider, numFetchers in
 	}
 	fetchResults := make(chan fetchResult, numFetchers)
 	barrier := make(chan struct{})
+	bInfo, _, _ := block.Info(true, true)
 	ctx, done := context.WithTimeout(context.Background(), 10*time.Second)
 	defer done()
 	for i := 0; i < numFetchers; i++ {
 		go func() {
 			<-barrier
-			recs, err := rp.FetchReceipts(ctx, block.BlockID(), txHashes)
+			recs, err := rp.FetchReceipts(ctx, bInfo, txHashes)
 			fetchResults <- fetchResult{rs: recs, err: err}
 		}()
 	}

op-service/sources/receipts_caching.go

@@ -51,7 +51,10 @@ func (p *CachingReceiptsProvider) deleteFetchingLock(blockHash common.Hash) {
 	delete(p.fetching, blockHash)
 }
 
-func (p *CachingReceiptsProvider) FetchReceipts(ctx context.Context, block eth.BlockID, txHashes []common.Hash) (types.Receipts, error) {
+// FetchReceipts fetches receipts for the given block and transaction hashes
+// it expects that the inner FetchReceipts implementation handles validation
+func (p *CachingReceiptsProvider) FetchReceipts(ctx context.Context, blockInfo eth.BlockInfo, txHashes []common.Hash) (types.Receipts, error) {
+	block := eth.ToBlockID(blockInfo)
 	if r, ok := p.cache.Get(block.Hash); ok {
 		return r, nil
 	}
@@ -67,10 +70,11 @@ func (p *CachingReceiptsProvider) FetchReceipts(ctx context.Context, block eth.B
 		return r, nil
 	}
 
-	r, err := p.inner.FetchReceipts(ctx, block, txHashes)
+	r, err := p.inner.FetchReceipts(ctx, blockInfo, txHashes)
 	if err != nil {
 		return nil, err
 	}
+
 	p.cache.Add(block.Hash, r)
 	// result now in cache, can delete fetching lock
 	p.deleteFetchingLock(block.Hash)

op-service/sources/receipts_caching_test.go

@@ -17,7 +17,8 @@ type mockReceiptsProvider struct {
 	mock.Mock
 }
 
-func (m *mockReceiptsProvider) FetchReceipts(ctx context.Context, block eth.BlockID, txHashes []common.Hash) (types.Receipts, error) {
+func (m *mockReceiptsProvider) FetchReceipts(ctx context.Context, blockInfo eth.BlockInfo, txHashes []common.Hash) (types.Receipts, error) {
+	block := eth.ToBlockID(blockInfo)
 	args := m.Called(ctx, block, txHashes)
 	return args.Get(0).(types.Receipts), args.Error(1)
 }
@@ -35,8 +36,9 @@ func TestCachingReceiptsProvider_Caching(t *testing.T) {
 		Return(types.Receipts(receipts), error(nil)).
 		Once() // receipts should be cached after first fetch
 
+	bInfo, _, _ := block.Info(true, true)
 	for i := 0; i < 4; i++ {
-		gotRecs, err := rp.FetchReceipts(ctx, blockid, txHashes)
+		gotRecs, err := rp.FetchReceipts(ctx, bInfo, txHashes)
 		require.NoError(t, err)
 		for i, gotRec := range gotRecs {
 			requireEqualReceipt(t, receipts[i], gotRec)

op-service/sources/receipts_rcp.go → op-service/sources/receipts_rpc.go

@@ -70,11 +70,12 @@ func NewRPCReceiptsFetcher(client rpcClient, log log.Logger, config RPCReceiptsC
 	}
 }
 
-func (f *RPCReceiptsFetcher) FetchReceipts(ctx context.Context, block eth.BlockID, txHashes []common.Hash) (result types.Receipts, err error) {
+func (f *RPCReceiptsFetcher) FetchReceipts(ctx context.Context, blockInfo eth.BlockInfo, txHashes []common.Hash) (result types.Receipts, err error) {
 	m := f.PickReceiptsMethod(len(txHashes))
+	block := eth.ToBlockID(blockInfo)
 	switch m {
 	case EthGetTransactionReceiptBatch:
-		result, err = f.basic.FetchReceipts(ctx, block, txHashes)
+		result, err = f.basic.FetchReceipts(ctx, blockInfo, txHashes)
 	case AlchemyGetTransactionReceipts:
 		var tmp receiptsWrapper
 		err = f.client.CallContext(ctx, &tmp, "alchemy_getTransactionReceipts", blockHashParameter{BlockHash: block.Hash})
@@ -104,6 +105,10 @@ func (f *RPCReceiptsFetcher) FetchReceipts(ctx context.Context, block eth.BlockI
 		return nil, err
 	}
 
+	if err = validateReceipts(block, blockInfo.ReceiptHash(), txHashes, result); err != nil {
+		return nil, err
+	}
+
 	return
 }