diff --git a/core/services/blockhashstore/delegate.go b/core/services/blockhashstore/delegate.go index f3008af7b8b..0342dff78b7 100644 --- a/core/services/blockhashstore/delegate.go +++ b/core/services/blockhashstore/delegate.go @@ -65,12 +65,6 @@ func (d *Delegate) ServicesForSpec(jb job.Job, qopts ...pg.QOpt) ([]job.ServiceC return nil, errors.New("log poller must be enabled to run blockhashstore") } - if jb.BlockhashStoreSpec.WaitBlocks < int32(chain.Config().EVM().FinalityDepth()) { - return nil, fmt.Errorf( - "waitBlocks must be greater than or equal to chain's finality depth (%d), currently %d", - chain.Config().EVM().FinalityDepth(), jb.BlockhashStoreSpec.WaitBlocks) - } - keys, err := d.ks.EnabledKeysForChain(chain.ID()) if err != nil { return nil, errors.Wrap(err, "getting sending keys") diff --git a/core/services/blockhashstore/delegate_test.go b/core/services/blockhashstore/delegate_test.go index b0456206861..3a9c01ae399 100644 --- a/core/services/blockhashstore/delegate_test.go +++ b/core/services/blockhashstore/delegate_test.go @@ -128,14 +128,6 @@ func TestDelegate_ServicesForSpec(t *testing.T) { assert.Error(t, err) }) - t.Run("WaitBlocks less than EvmFinalityDepth", func(t *testing.T) { - spec := job.Job{BlockhashStoreSpec: &job.BlockhashStoreSpec{ - WaitBlocks: defaultWaitBlocks - 1, - }} - _, err := delegate.ServicesForSpec(spec) - assert.Error(t, err) - }) - t.Run("missing EnabledKeysForChain", func(t *testing.T) { _, err := testData.ethKeyStore.Delete(testData.sendingKey.ID()) require.NoError(t, err) diff --git a/core/services/blockhashstore/feeder.go b/core/services/blockhashstore/feeder.go index 22bbf7f163d..14e51e68394 100644 --- a/core/services/blockhashstore/feeder.go +++ b/core/services/blockhashstore/feeder.go @@ -37,6 +37,7 @@ func NewFeeder( lookbackBlocks: lookbackBlocks, latestBlock: latestBlock, stored: make(map[uint64]struct{}), + storedTrusted: make(map[uint64]common.Hash), lastRunBlock: 0, wgStored: sync.WaitGroup{}, } @@ -54,12 +55,12 @@ type Feeder struct { lookbackBlocks int latestBlock func(ctx context.Context) (uint64, error) - stored map[uint64]struct{} - lastRunBlock uint64 - wgStored sync.WaitGroup - batchLock sync.Mutex - storedLock sync.RWMutex - errsLock sync.Mutex + stored map[uint64]struct{} // used for trustless feeder + storedTrusted map[uint64]common.Hash // used for trusted feeder + lastRunBlock uint64 + wgStored sync.WaitGroup + batchLock sync.Mutex + errsLock sync.Mutex } // Run the feeder. @@ -103,10 +104,10 @@ func (f *Feeder) Run(ctx context.Context) error { "block", block) errs = multierr.Append(errs, errors.Wrap(err, "checking if stored")) } else if stored { + // IsStored() can be based on unfinalized blocks. Therefore, f.stored mapping is not updated f.lggr.Infow("Blockhash already stored", "block", block, "latestBlock", latestBlock, "unfulfilledReqIDs", LimitReqIDs(unfulfilledReqs, 50)) - f.stored[block] = struct{}{} continue } @@ -161,13 +162,6 @@ func (f *Feeder) runTrusted( if len(unfulfilled) == 0 { return } - f.storedLock.RLock() - if _, ok := f.stored[block]; ok { - // Already stored - f.storedLock.RUnlock() - return - } - f.storedLock.RUnlock() // Do not store a block if it has been marked as stored; otherwise, store it even // if the RPC call errors, as to be conservative. @@ -185,9 +179,6 @@ func (f *Feeder) runTrusted( f.lggr.Infow("Blockhash already stored", "block", block, "latestBlock", latestBlock, "unfulfilledReqIDs", LimitReqIDs(unfulfilled, 50)) - f.storedLock.Lock() - f.stored[block] = struct{}{} - f.storedLock.Unlock() return } @@ -224,15 +215,23 @@ func (f *Feeder) runTrusted( // append its blockhash to our blockhashes we want to store. // If it is the log poller block pertaining to our recent block number, assig it. for _, b := range lpBlocks { + if b.BlockNumber == int64(latestBlock) { + latestBlockhash = b.BlockHash + } + if f.storedTrusted[uint64(b.BlockNumber)] == b.BlockHash { + // blockhash is already stored. skip to save gas + continue + } if _, ok := batch[uint64(b.BlockNumber)]; ok { blocksToStore = append(blocksToStore, uint64(b.BlockNumber)) blockhashesToStore = append(blockhashesToStore, b.BlockHash) } - if b.BlockNumber == int64(latestBlock) { - latestBlockhash = b.BlockHash - } } + if len(blocksToStore) == 0 { + f.lggr.Debugw("no blocks to store", "latestBlock", latestBlock) + return errs + } // Store the batch of blocks and their blockhashes. err = f.bhs.StoreTrusted(ctx, blocksToStore, blockhashesToStore, latestBlock, latestBlockhash) if err != nil { @@ -246,12 +245,15 @@ func (f *Feeder) runTrusted( errs = multierr.Append(errs, errors.Wrap(err, "checking if stored")) return errs } + for i, block := range blocksToStore { + f.storedTrusted[block] = blockhashesToStore[i] + } } - // Prune stored, anything older than fromBlock can be discarded. - for b := range f.stored { + // Prune storedTrusted, anything older than fromBlock can be discarded. + for b := range f.storedTrusted { if b < fromBlock { - delete(f.stored, b) + delete(f.storedTrusted, b) } } diff --git a/core/services/blockhashstore/feeder_test.go b/core/services/blockhashstore/feeder_test.go index 7efb7052985..e015253ba28 100644 --- a/core/services/blockhashstore/feeder_test.go +++ b/core/services/blockhashstore/feeder_test.go @@ -10,6 +10,7 @@ import ( "github.com/ethereum/go-ethereum/core/types" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "golang.org/x/exp/maps" mocklp "github.com/smartcontractkit/chainlink/v2/core/chains/evm/logpoller/mocks" evmtypes "github.com/smartcontractkit/chainlink/v2/core/chains/evm/types" @@ -46,61 +47,67 @@ var ( _ Coordinator = &TestCoordinator{} _ BHS = &TestBHS{} tests = []struct { - name string - requests []Event - fulfillments []Event - wait int - lookback int - latest uint64 - bhs TestBHS - expectedStored []uint64 - expectedErrMsg string + name string + requests []Event + fulfillments []Event + wait int + lookback int + latest uint64 + bhs TestBHS + expectedStored []uint64 + expectedStoredMapBlocks []uint64 // expected state of stored map in Feeder struct + expectedErrMsg string }{ { - name: "single unfulfilled request", - requests: []Event{{Block: 150, ID: "1000"}}, - wait: 25, - lookback: 100, - latest: 200, - expectedStored: []uint64{150}, + name: "single unfulfilled request", + requests: []Event{{Block: 150, ID: "1000"}}, + wait: 25, + lookback: 100, + latest: 200, + expectedStored: []uint64{150}, + expectedStoredMapBlocks: []uint64{150}, }, { - name: "single fulfilled request", - requests: []Event{{Block: 150, ID: "1000"}}, - fulfillments: []Event{{Block: 155, ID: "1000"}}, - wait: 25, - lookback: 100, - latest: 200, - expectedStored: []uint64{}, + name: "single fulfilled request", + requests: []Event{{Block: 150, ID: "1000"}}, + fulfillments: []Event{{Block: 155, ID: "1000"}}, + wait: 25, + lookback: 100, + latest: 200, + expectedStored: []uint64{}, + expectedStoredMapBlocks: []uint64{}, }, { - name: "single already fulfilled", - requests: []Event{{Block: 150, ID: "1000"}}, - wait: 25, - lookback: 100, - latest: 200, - bhs: TestBHS{Stored: []uint64{150}}, - expectedStored: []uint64{150}, + name: "single already fulfilled", + requests: []Event{{Block: 150, ID: "1000"}}, + wait: 25, + lookback: 100, + latest: 200, + bhs: TestBHS{Stored: []uint64{150}}, + expectedStored: []uint64{150}, + expectedStoredMapBlocks: []uint64{}, }, { - name: "error checking if stored, store anyway", - requests: []Event{{Block: 150, ID: "1000"}}, - wait: 25, - lookback: 100, - latest: 200, - bhs: TestBHS{ErrorsIsStored: []uint64{150}}, - expectedStored: []uint64{150}, - expectedErrMsg: "checking if stored: error checking if stored", + name: "error checking if stored, store anyway", + requests: []Event{{Block: 150, ID: "1000"}}, + wait: 25, + lookback: 100, + latest: 200, + bhs: TestBHS{ErrorsIsStored: []uint64{150}}, + expectedStored: []uint64{150}, + expectedStoredMapBlocks: []uint64{150}, + expectedErrMsg: "checking if stored: error checking if stored", }, { - name: "error storing, continue to next block anyway", - requests: []Event{{Block: 150, ID: "1000"}, {Block: 151, ID: "1000"}}, - wait: 25, - lookback: 100, - latest: 200, - bhs: TestBHS{ErrorsStore: []uint64{150}}, - expectedStored: []uint64{151}, - expectedErrMsg: "storing block: error storing", + name: "error storing, continue to next block anyway", + requests: []Event{{Block: 150, ID: "1000"}, {Block: 151, ID: "1000"}}, + wait: 25, + lookback: 100, + latest: 200, + bhs: TestBHS{ErrorsStore: []uint64{150}}, + expectedStored: []uint64{151}, + expectedStoredMapBlocks: []uint64{151}, + expectedErrMsg: "storing block: error storing", }, { name: "multiple requests same block, some fulfilled", @@ -111,10 +118,11 @@ var ( fulfillments: []Event{ {Block: 150, ID: "10001"}, {Block: 150, ID: "10003"}}, - wait: 25, - lookback: 100, - latest: 200, - expectedStored: []uint64{150}, + wait: 25, + lookback: 100, + latest: 200, + expectedStored: []uint64{150}, + expectedStoredMapBlocks: []uint64{150}, }, { name: "multiple requests same block, all fulfilled", @@ -126,52 +134,58 @@ var ( {Block: 150, ID: "10001"}, {Block: 150, ID: "10002"}, {Block: 150, ID: "10003"}}, - wait: 25, - lookback: 100, - latest: 200, - expectedStored: []uint64{}, + wait: 25, + lookback: 100, + latest: 200, + expectedStored: []uint64{}, + expectedStoredMapBlocks: []uint64{}, }, { - name: "fulfillment no matching request no error", - requests: []Event{{Block: 150, ID: "1000"}}, - fulfillments: []Event{{Block: 199, ID: "10002"}}, - wait: 25, - lookback: 100, - latest: 200, - expectedStored: []uint64{150}, + name: "fulfillment no matching request no error", + requests: []Event{{Block: 150, ID: "1000"}}, + fulfillments: []Event{{Block: 199, ID: "10002"}}, + wait: 25, + lookback: 100, + latest: 200, + expectedStored: []uint64{150}, + expectedStoredMapBlocks: []uint64{150}, }, { - name: "multiple unfulfilled requests", - requests: []Event{{Block: 150, ID: "10001"}, {Block: 151, ID: "10002"}}, - wait: 25, - lookback: 100, - latest: 200, - expectedStored: []uint64{150, 151}, + name: "multiple unfulfilled requests", + requests: []Event{{Block: 150, ID: "10001"}, {Block: 151, ID: "10002"}}, + wait: 25, + lookback: 100, + latest: 200, + expectedStored: []uint64{150, 151}, + expectedStoredMapBlocks: []uint64{150, 151}, }, { - name: "multiple fulfilled requests", - requests: []Event{{Block: 150, ID: "10001"}, {Block: 151, ID: "10002"}}, - fulfillments: []Event{{Block: 150, ID: "10001"}, {Block: 151, ID: "10002"}}, - wait: 25, - lookback: 100, - latest: 200, - expectedStored: []uint64{}, + name: "multiple fulfilled requests", + requests: []Event{{Block: 150, ID: "10001"}, {Block: 151, ID: "10002"}}, + fulfillments: []Event{{Block: 150, ID: "10001"}, {Block: 151, ID: "10002"}}, + wait: 25, + lookback: 100, + latest: 200, + expectedStored: []uint64{}, + expectedStoredMapBlocks: []uint64{}, }, { - name: "recent unfulfilled request do not store", - requests: []Event{{Block: 185, ID: "1000"}}, - wait: 25, - lookback: 100, - latest: 200, - expectedStored: []uint64{}, + name: "recent unfulfilled request do not store", + requests: []Event{{Block: 185, ID: "1000"}}, + wait: 25, + lookback: 100, + latest: 200, + expectedStored: []uint64{}, + expectedStoredMapBlocks: []uint64{}, }, { - name: "old unfulfilled request do not store", - requests: []Event{{Block: 99, ID: "1000"}, {Block: 57, ID: "1000"}}, - wait: 25, - lookback: 100, - latest: 200, - expectedStored: []uint64{}, + name: "old unfulfilled request do not store", + requests: []Event{{Block: 99, ID: "1000"}, {Block: 57, ID: "1000"}}, + wait: 25, + lookback: 100, + latest: 200, + expectedStored: []uint64{}, + expectedStoredMapBlocks: []uint64{}, }, { name: "mixed", @@ -204,18 +218,20 @@ var ( // Block 154 {Block: 154, ID: "10007"}}, - wait: 25, - lookback: 100, - latest: 200, - expectedStored: []uint64{150, 153}, + wait: 25, + lookback: 100, + latest: 200, + expectedStored: []uint64{150, 153}, + expectedStoredMapBlocks: []uint64{150, 153}, }, { - name: "lookback before 0th block", - requests: []Event{{Block: 20, ID: "1000"}}, - wait: 25, - lookback: 100, - latest: 50, - expectedStored: []uint64{20}, + name: "lookback before 0th block", + requests: []Event{{Block: 20, ID: "1000"}}, + wait: 25, + lookback: 100, + latest: 50, + expectedStored: []uint64{20}, + expectedStoredMapBlocks: []uint64{20}, }, } ) @@ -250,6 +266,7 @@ func TestFeeder(t *testing.T) { } require.ElementsMatch(t, test.expectedStored, test.bhs.Stored) + require.ElementsMatch(t, test.expectedStoredMapBlocks, maps.Keys(feeder.stored)) }) } } @@ -341,6 +358,7 @@ func TestFeederWithLogPollerVRFv1(t *testing.T) { require.EqualError(t, err, test.expectedErrMsg) } require.ElementsMatch(t, test.expectedStored, test.bhs.Stored) + require.ElementsMatch(t, test.expectedStoredMapBlocks, maps.Keys(feeder.stored)) }) } } @@ -436,6 +454,7 @@ func TestFeederWithLogPollerVRFv2(t *testing.T) { require.EqualError(t, err, test.expectedErrMsg) } require.ElementsMatch(t, test.expectedStored, test.bhs.Stored) + require.ElementsMatch(t, test.expectedStoredMapBlocks, maps.Keys(feeder.stored)) }) } } @@ -531,6 +550,7 @@ func TestFeederWithLogPollerVRFv2Plus(t *testing.T) { require.EqualError(t, err, test.expectedErrMsg) } require.ElementsMatch(t, test.expectedStored, test.bhs.Stored) + require.ElementsMatch(t, test.expectedStoredMapBlocks, maps.Keys(feeder.stored)) }) } }