Feature/scribe chain backfill #146

CryptoMaxPlanck · 2022-08-28T21:00:08Z

Description
Each contract backfiller belongs to a chain backfiller. The chain represents one chain and hosts our contracts on it, which we add to a config and use to create a contract backfiller for each one. The job of the chain backfiller is to aggregate the storing of logs on all of these contract backfillers to populate the same event database.

- chain backfiller type - chain backfiller tests - alter testing functions

codecov · 2022-08-28T21:01:24Z

Codecov Report

Merging #146 (0409f3d) into master (49cac98) will increase coverage by 0.12633%.
The diff coverage is 37.93103%.

@@                 Coverage Diff                 @@
##              master        #146         +/-   ##
===================================================
+ Coverage   51.28562%   51.41195%   +0.12632%     
===================================================
  Files            137         138          +1     
  Lines           5756        5843         +87     
  Branches          73          73                 
===================================================
+ Hits            2952        3004         +52     
- Misses          2511        2545         +34     
- Partials         293         294          +1

Impacted Files	Coverage Δ
services/scribe/db/datastore/sql/base/log.go	`0.00000% <0.00000%> (ø)`
services/scribe/db/datastore/sql/base/receipt.go	`0.00000% <0.00000%> (ø)`
...rvices/scribe/db/datastore/sql/base/transaction.go	`0.00000% <0.00000%> (ø)`
services/scribe/backfill/chain.go	`75.00000% <75.00000%> (ø)`
services/scribe/backfill/contract.go	`64.55696% <90.00000%> (+7.81937%)`	⬆️
services/scribe/db/datastore/sql/base/model.go	`100.00000% <100.00000%> (ø)`
agents/indexer/domain.go	`67.27273% <0.00000%> (+3.63636%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

CryptoMaxPlanck · 2022-08-28T22:38:05Z

services/scribe/backfill/chain_test.go

+	// Backfill the first batch of events.
+	latestBlock, err := simulatedChain.BlockNumber(b.GetTestContext())
+	Nil(b.T(), err)
+	err = chainBackfiller.Backfill(b.GetTestContext(), latestBlock-uint64(chainConfig.ConfirmationThreshold))


setting the endHeight of the backfiller will be the job of the ScribeBackfiller which is the aggregator of chain backfillers. This is just to test the method that the ScribeBackfiller is going to use for setting that end blockheight. But when actually calling it, there will be protective checks around making sure endHeight doesn't wrap around if threshold > currentBlock. This is a placeholder/duplicate of what will be done in the ScribeTest level. this is more so a proof of concept / sanity check

trajan0x · 2022-08-28T22:44:21Z

services/scribe/backfill/chain.go

+// from a slice of ContractBackfillers.
+type ChainBackfiller struct {
+	// contracts is the list of contracts to get logs for
+	contracts []contracts.DeployedContract


We definitely shouldn't be using this interface here, I'd define a new one. There are Accessor methods we don't have access to here/we don't need any of these:

From #114, I'm pretty sure the only thing we need here is:

Address

ChainID

StartBlock

Owner, DeployTX, and ContractHandle will all be nil here and throw an error

Actually - we can probably just get this from chainConfig, right?

trajan0x · 2022-08-28T22:45:55Z

services/scribe/backfill/chain.go

+		if err != nil {
+			return nil, fmt.Errorf("could not create contract backfiller: %w", err)
+		}
+		contractBackfillers[i] = *contractBackfiller


I don't think this is safe (risks nil dereference. Let's set the type in the slice to just be []*ContractBackfiller

trajan0x · 2022-08-28T22:48:04Z

services/scribe/backfill/chain.go

+		startHeight := c.chainConfig.Contracts[contractBackfiller.contract.Address().String()].StartBlock
+		// call Backfill concurrently
+		g.Go(func() error {
+			err := contractBackfiller.Backfill(ctx, startHeight, endHeight)


Still not sure about this error handling behavior here. An error on a contract in a given chain shouldn't throw an error on every contract on that chain. Maybe we need to throw this in a for-loop w/ a backoff? If you want to handle in a different pr can definitely make an issue and handle later, but definitely needs to be addressed

One thing to remember here is RPCs can be super flaky. Rate limits, data-availability issues, etc can all cause any of our networked calls (eth_getLogs, eth_getTransactionByHash, eth_getReceipt) to fail so we want to design these individual loops to be resilient.

The goal here is to keep these things indexing as long as we can hit our most important gurantee- that we're not marking a range as indexed if it's not indexed

trajan0x · 2022-08-28T22:51:51Z

services/scribe/backfill/chain_test.go

+	// Check that the events were written to the database.
+	for _, contract := range contracts {
+		// Check the storage of logs.
+		logs, err := b.testDB.RetrieveAllLogs_Test(b.GetTestContext(), true, chainConfig.ChainID, contract.Address().String())


One minor thing here, can we rename this to UnsafeRetrieveAllLogs, just a standard go pattern

trajan0x · 2022-08-28T22:52:55Z

services/scribe/db/datastore/sql/base/log.go

@@ -103,11 +112,22 @@ func (s Store) RetrieveLogs(ctx context.Context, txHash common.Hash, chainID uin
 // RetrieveAllLogs_Test retrieves all logs in the database. This is only used for testing.
 //
 //nolint:golint, revive, stylecheck
-func (s Store) RetrieveAllLogs_Test(ctx context.Context) (logs []*types.Log, err error) {
+func (s Store) RetrieveAllLogs_Test(ctx context.Context, specific bool, chainID uint32, address string) (logs []*types.Log, err error) {


Any reason address isn't a string here? Would like to try to keep the interface native

trajan0x · 2022-08-28T22:53:45Z

services/scribe/db/event.go

@@ -14,13 +14,13 @@ type EventDB interface {
 	// RetrieveLogs retrieves logs that match a tx hash and chain id
 	RetrieveLogs(ctx context.Context, txHash common.Hash, chainID uint32) (logs []*types.Log, err error)
 	// RetrieveAllLogs_Test retrieves all logs in the database. This is only used for testing.
-	RetrieveAllLogs_Test(ctx context.Context) (logs []*types.Log, err error)
+	RetrieveAllLogs_Test(ctx context.Context, specific bool, chainID uint32, address string) (logs []*types.Log, err error)


common.Address

trajan0x · 2022-08-28T22:54:43Z

services/scribe/db/event.go

@@ -14,13 +14,13 @@ type EventDB interface {
 	// RetrieveLogs retrieves logs that match a tx hash and chain id
 	RetrieveLogs(ctx context.Context, txHash common.Hash, chainID uint32) (logs []*types.Log, err error)
 	// RetrieveAllLogs_Test retrieves all logs in the database. This is only used for testing.
-	RetrieveAllLogs_Test(ctx context.Context) (logs []*types.Log, err error)
+	RetrieveAllLogs_Test(ctx context.Context, specific bool, chainID uint32, address string) (logs []*types.Log, err error)
 	// StoreReceipt stores a receipt
 	StoreReceipt(ctx context.Context, receipt types.Receipt, chainID uint32) error
 	// RetrieveReceipt retrieves a receipt by tx hash and chain id
 	RetrieveReceipt(ctx context.Context, txHash common.Hash, chainID uint32) (receipt types.Receipt, err error)
 	// RetrieveAllReceipts_Test retrieves all receipts in the database. This is only used for testing.


What's specific here? Throw in comment

trajan0x · 2022-08-29T00:58:25Z

services/scribe/backfill/contract.go

 		for {
 			select {
 			case <-ctx.Done():
 				return nil
 			case log := <-logChan:
+				// check if either attempt count has exceeded maxAttempts
+				if storeAttempt > maxAttempts {


let's ditch max attempts here

CryptoMaxPlanck added 2 commits August 28, 2022 14:42

add:

83cfb26

- chain backfiller type - chain backfiller tests - alter testing functions

fix test errors

786b32a

github-actions bot added the go Pull requests that update Go code label Aug 28, 2022

add testConfirmations test

410c38d

CryptoMaxPlanck commented Aug 28, 2022

View reviewed changes

trajan0x requested changes Aug 28, 2022

View reviewed changes

CryptoMaxPlanck added 3 commits August 28, 2022 20:39

fixes based on trajan comments

96018be

fix comment

bc3803b

reset backoff

6414c62

trajan0x reviewed Aug 29, 2022

View reviewed changes

remove maxattempts for store loop

0409f3d

trajan0x approved these changes Aug 29, 2022

View reviewed changes

trajan0x merged commit ebfc61a into master Aug 29, 2022

trajan0x deleted the feature/scribe_chain_backfill branch August 29, 2022 01:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/scribe chain backfill #146

Feature/scribe chain backfill #146

CryptoMaxPlanck commented Aug 28, 2022

codecov bot commented Aug 28, 2022 •

edited

Loading

CryptoMaxPlanck Aug 28, 2022

trajan0x Aug 28, 2022

trajan0x Aug 28, 2022

trajan0x Aug 28, 2022

trajan0x Aug 28, 2022

trajan0x Aug 28, 2022

trajan0x Aug 28, 2022

trajan0x Aug 28, 2022

trajan0x Aug 28, 2022

trajan0x Aug 29, 2022

Feature/scribe chain backfill #146

Feature/scribe chain backfill #146

Conversation

CryptoMaxPlanck commented Aug 28, 2022

codecov bot commented Aug 28, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 28, 2022 •

edited

Loading