Commit 6cdd6301 authored by Axel Kingsley's avatar Axel Kingsley Committed by GitHub

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
parent 962e5eca
...@@ -317,16 +317,11 @@ func (s *EthClient) FetchReceipts(ctx context.Context, blockHash common.Hash) (e ...@@ -317,16 +317,11 @@ func (s *EthClient) FetchReceipts(ctx context.Context, blockHash common.Hash) (e
return nil, nil, fmt.Errorf("querying block: %w", err) return nil, nil, fmt.Errorf("querying block: %w", err)
} }
txHashes, block := eth.TransactionsToHashes(txs), eth.ToBlockID(info) txHashes, _ := eth.TransactionsToHashes(txs), eth.ToBlockID(info)
receipts, err := s.recProvider.FetchReceipts(ctx, block, txHashes) receipts, err := s.recProvider.FetchReceipts(ctx, info, txHashes)
if err != nil { if err != nil {
return nil, nil, err 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 return info, receipts, nil
} }
......
...@@ -180,95 +180,49 @@ func TestEthClient_WrongInfoByHash(t *testing.T) { ...@@ -180,95 +180,49 @@ func TestEthClient_WrongInfoByHash(t *testing.T) {
m.Mock.AssertExpectations(t) m.Mock.AssertExpectations(t)
} }
type validateReceiptsTest struct { func newEthClientWithCaches(metrics caching.Metrics, cacheSize int) *EthClient {
desc string return &EthClient{
trustRPC bool transactionsCache: caching.NewLRUCache[common.Hash, types.Transactions](metrics, "txs", cacheSize),
mutReceipts func(types.Receipts) types.Receipts headersCache: caching.NewLRUCache[common.Hash, eth.BlockInfo](metrics, "headers", cacheSize),
expError string payloadsCache: caching.NewLRUCache[common.Hash, *eth.ExecutionPayloadEnvelope](metrics, "payloads", cacheSize),
}
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 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) require := require.New(t)
mrpc := new(mockRPC) mrpc := new(mockRPC)
mrp := new(mockReceiptsProvider) rp := NewRPCReceiptsFetcher(mrpc, nil, RPCReceiptsConfig{})
const numTxs = 4 const numTxs = 1
block, receipts := randomRpcBlockAndReceipts(rand.New(rand.NewSource(420)), numTxs) block, _ := randomRpcBlockAndReceipts(rand.New(rand.NewSource(420)), numTxs)
txHashes := receiptTxHashes(receipts) //txHashes := receiptTxHashes(receipts)
ctx := context.Background() ctx := context.Background()
if mut := test.mutReceipts; mut != nil { mrpc.On("CallContext",
receipts = mut(receipts) ctx,
} mock.Anything,
"eth_getTransactionReceipt",
mrpc.On("CallContext", ctx, mock.AnythingOfType("**sources.RPCBlock"), mock.Anything).
"eth_getBlockByHash", []any{block.Hash, true}). 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) { Run(func(args mock.Arguments) {
*(args[1].(**RPCBlock)) = block *(args[1].(**RPCBlock)) = block
}). }).
Return([]error{nil}).Once() Return([]error{nil})
mrp.On("FetchReceipts", ctx, block.BlockID(), txHashes).
Return(types.Receipts(receipts), error(nil)).Once()
ethcl := newEthClientWithCaches(nil, numTxs) ethcl := newEthClientWithCaches(nil, numTxs)
ethcl.client = mrpc ethcl.client = mrpc
ethcl.recProvider = mrp ethcl.recProvider = rp
ethcl.trustRPC = test.trustRPC ethcl.trustRPC = true
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)
}
func newEthClientWithCaches(metrics caching.Metrics, cacheSize int) *EthClient { _, _, err := ethcl.FetchReceipts(ctx, block.Hash)
return &EthClient{ require.ErrorContains(err, "unexpected nil block number")
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),
}
} }
...@@ -14,7 +14,7 @@ type ReceiptsProvider interface { ...@@ -14,7 +14,7 @@ type ReceiptsProvider interface {
// FetchReceipts returns a block info and all of the receipts associated with transactions in the block. // 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 // 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. // 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. // validateReceipts validates that the receipt contents are valid.
......
...@@ -31,7 +31,10 @@ func NewBasicRPCReceiptsFetcher(client rpcClient, maxBatchSize int) *BasicRPCRec ...@@ -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) call := f.getOrCreateBatchCall(block.Hash, txHashes)
// Fetch all receipts // Fetch all receipts
......
...@@ -35,7 +35,6 @@ func TestBasicRPCReceiptsFetcher_Reuse(t *testing.T) { ...@@ -35,7 +35,6 @@ func TestBasicRPCReceiptsFetcher_Reuse(t *testing.T) {
require := require.New(t) require := require.New(t)
batchSize, txCount := 2, uint64(4) batchSize, txCount := 2, uint64(4)
block, receipts := randomRpcBlockAndReceipts(rand.New(rand.NewSource(123)), txCount) block, receipts := randomRpcBlockAndReceipts(rand.New(rand.NewSource(123)), txCount)
blockid := block.BlockID()
txHashes := make([]common.Hash, 0, len(receipts)) txHashes := make([]common.Hash, 0, len(receipts))
recMap := make(map[common.Hash]*types.Receipt, len(receipts)) recMap := make(map[common.Hash]*types.Receipt, len(receipts))
for _, rec := range receipts { for _, rec := range receipts {
...@@ -75,8 +74,10 @@ func TestBasicRPCReceiptsFetcher_Reuse(t *testing.T) { ...@@ -75,8 +74,10 @@ func TestBasicRPCReceiptsFetcher_Reuse(t *testing.T) {
return err return err
} }
bInfo, _, _ := block.Info(true, true)
// 1st fetching should result in errors // 1st fetching should result in errors
recs, err := rp.FetchReceipts(ctx, blockid, txHashes) recs, err := rp.FetchReceipts(ctx, bInfo, txHashes)
require.Error(err) require.Error(err)
require.Nil(recs) require.Nil(recs)
require.EqualValues(2, numCalls.Load()) require.EqualValues(2, numCalls.Load())
...@@ -84,7 +85,7 @@ func TestBasicRPCReceiptsFetcher_Reuse(t *testing.T) { ...@@ -84,7 +85,7 @@ func TestBasicRPCReceiptsFetcher_Reuse(t *testing.T) {
// prepare 2nd fetching - all should succeed now // prepare 2nd fetching - all should succeed now
response[txHashes[2]] = true response[txHashes[2]] = true
response[txHashes[3]] = true response[txHashes[3]] = true
recs, err = rp.FetchReceipts(ctx, blockid, txHashes) recs, err = rp.FetchReceipts(ctx, bInfo, txHashes)
require.NoError(err) require.NoError(err)
require.NotNil(recs) require.NotNil(recs)
for i, rec := range recs { for i, rec := range recs {
...@@ -141,12 +142,13 @@ func runConcurrentFetchingTest(t *testing.T, rp ReceiptsProvider, numFetchers in ...@@ -141,12 +142,13 @@ func runConcurrentFetchingTest(t *testing.T, rp ReceiptsProvider, numFetchers in
} }
fetchResults := make(chan fetchResult, numFetchers) fetchResults := make(chan fetchResult, numFetchers)
barrier := make(chan struct{}) barrier := make(chan struct{})
bInfo, _, _ := block.Info(true, true)
ctx, done := context.WithTimeout(context.Background(), 10*time.Second) ctx, done := context.WithTimeout(context.Background(), 10*time.Second)
defer done() defer done()
for i := 0; i < numFetchers; i++ { for i := 0; i < numFetchers; i++ {
go func() { go func() {
<-barrier <-barrier
recs, err := rp.FetchReceipts(ctx, block.BlockID(), txHashes) recs, err := rp.FetchReceipts(ctx, bInfo, txHashes)
fetchResults <- fetchResult{rs: recs, err: err} fetchResults <- fetchResult{rs: recs, err: err}
}() }()
} }
......
...@@ -51,7 +51,10 @@ func (p *CachingReceiptsProvider) deleteFetchingLock(blockHash common.Hash) { ...@@ -51,7 +51,10 @@ func (p *CachingReceiptsProvider) deleteFetchingLock(blockHash common.Hash) {
delete(p.fetching, blockHash) 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 { if r, ok := p.cache.Get(block.Hash); ok {
return r, nil return r, nil
} }
...@@ -67,10 +70,11 @@ func (p *CachingReceiptsProvider) FetchReceipts(ctx context.Context, block eth.B ...@@ -67,10 +70,11 @@ func (p *CachingReceiptsProvider) FetchReceipts(ctx context.Context, block eth.B
return r, nil return r, nil
} }
r, err := p.inner.FetchReceipts(ctx, block, txHashes) r, err := p.inner.FetchReceipts(ctx, blockInfo, txHashes)
if err != nil { if err != nil {
return nil, err return nil, err
} }
p.cache.Add(block.Hash, r) p.cache.Add(block.Hash, r)
// result now in cache, can delete fetching lock // result now in cache, can delete fetching lock
p.deleteFetchingLock(block.Hash) p.deleteFetchingLock(block.Hash)
......
...@@ -17,7 +17,8 @@ type mockReceiptsProvider struct { ...@@ -17,7 +17,8 @@ type mockReceiptsProvider struct {
mock.Mock 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) args := m.Called(ctx, block, txHashes)
return args.Get(0).(types.Receipts), args.Error(1) return args.Get(0).(types.Receipts), args.Error(1)
} }
...@@ -35,8 +36,9 @@ func TestCachingReceiptsProvider_Caching(t *testing.T) { ...@@ -35,8 +36,9 @@ func TestCachingReceiptsProvider_Caching(t *testing.T) {
Return(types.Receipts(receipts), error(nil)). Return(types.Receipts(receipts), error(nil)).
Once() // receipts should be cached after first fetch Once() // receipts should be cached after first fetch
bInfo, _, _ := block.Info(true, true)
for i := 0; i < 4; i++ { for i := 0; i < 4; i++ {
gotRecs, err := rp.FetchReceipts(ctx, blockid, txHashes) gotRecs, err := rp.FetchReceipts(ctx, bInfo, txHashes)
require.NoError(t, err) require.NoError(t, err)
for i, gotRec := range gotRecs { for i, gotRec := range gotRecs {
requireEqualReceipt(t, receipts[i], gotRec) requireEqualReceipt(t, receipts[i], gotRec)
......
...@@ -70,11 +70,12 @@ func NewRPCReceiptsFetcher(client rpcClient, log log.Logger, config RPCReceiptsC ...@@ -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)) m := f.PickReceiptsMethod(len(txHashes))
block := eth.ToBlockID(blockInfo)
switch m { switch m {
case EthGetTransactionReceiptBatch: case EthGetTransactionReceiptBatch:
result, err = f.basic.FetchReceipts(ctx, block, txHashes) result, err = f.basic.FetchReceipts(ctx, blockInfo, txHashes)
case AlchemyGetTransactionReceipts: case AlchemyGetTransactionReceipts:
var tmp receiptsWrapper var tmp receiptsWrapper
err = f.client.CallContext(ctx, &tmp, "alchemy_getTransactionReceipts", blockHashParameter{BlockHash: block.Hash}) 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 ...@@ -104,6 +105,10 @@ func (f *RPCReceiptsFetcher) FetchReceipts(ctx context.Context, block eth.BlockI
return nil, err return nil, err
} }
if err = validateReceipts(block, blockInfo.ReceiptHash(), txHashes, result); err != nil {
return nil, err
}
return return
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment