Add traces backfill option #615
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,6 +163,39 @@ func (b *Bootstrap) StartTraceDownloader(ctx context.Context) error { | |
| ) | ||
|
|
||
| StartEngine(ctx, b.traces, l) | ||
|
|
||
| if b.config.TracesBackfillStartHeight > 0 { | ||
|
There was a problem hiding this comment. How do we prevent from backfilling it over and over again after every restart? There was a problem hiding this comment. That can be done by completely removing the provided values for these 2 flags, upon restarting, or simply by shifting their values to not account for past heights, which we know were back-filled. There was a problem hiding this comment. Yeah, it's better that we could somehow remember that we have back-filled, and skip it next time. There was a problem hiding this comment. That's true, but soon enough the traces will be generated on demand from the EVM Gateway, so there will be no need for downloading from a GCP bucket. I see this merely as a band-aid to unblock partners that rely on the missing traces. There was a problem hiding this comment. I was thinking we would just remove the flag from the config after starting it up. We could build a more intelligent solution, but I think this is going away in the next couple months so not worth spending much time on it. worst case it will scan for all of the traces, and skip downloading them because they already exist in the db |
||
| startHeight := b.config.TracesBackfillStartHeight | ||
| if _, err := b.storages.Blocks.GetByHeight(startHeight); err != nil { | ||
| return fmt.Errorf("failed to get provided start height %d in db: %w", startHeight, err) | ||
| } | ||
|
|
||
| cadenceStartHeight, err := b.storages.Blocks.GetCadenceHeight(startHeight) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get cadence height for backfill start height %d: %w", startHeight, err) | ||
| } | ||
|
|
||
| if cadenceStartHeight < b.config.InitCadenceHeight { | ||
| b.logger.Warn(). | ||
| Uint64("evm-start-height", startHeight). | ||
| Uint64("cadence-start-height", cadenceStartHeight). | ||
| Uint64("init-cadence-height", b.config.InitCadenceHeight). | ||
| Msg("backfill start height is before initial cadence height. data may be missing from configured traces bucket") | ||
| } | ||
|
|
||
| endHeight := b.config.TracesBackfillEndHeight | ||
| if endHeight == 0 { | ||
| endHeight, err = b.storages.Blocks.LatestEVMHeight() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get latest EVM height: %w", err) | ||
| } | ||
| } else if _, err := b.storages.Blocks.GetByHeight(endHeight); err != nil { | ||
| return fmt.Errorf("failed to get provided end height %d in db: %w", endHeight, err) | ||
| } | ||
|
|
||
| go b.traces.Backfill(startHeight, endHeight) | ||
| } | ||
|
Comment on lines
+167
to
+197
There was a problem hiding this comment. Add validation to ensure Currently, there is no check to confirm that Consider adding the following validation after determining if startHeight > endHeight {
return fmt.Errorf("startHeight (%d) cannot be greater than endHeight (%d)", startHeight, endHeight)
}There was a problem hiding this comment. this is checked in another file which runs before this code There was a problem hiding this comment. @peterargue, understood, thank you for the clarification. I've noted that the validation for (_/) ✏️ Learnings added
|
||
|
|
||
| return nil | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,11 +84,11 @@ func (e *Engine) Notify(block *models.Block) { | |
| return | ||
| } | ||
|
|
||
| go e.indexBlockTraces(block, cadenceID, false) | ||
| } | ||
|
|
||
| // indexBlockTraces iterates the block transaction hashes and tries to download the traces | ||
| func (e *Engine) indexBlockTraces(evmBlock *models.Block, cadenceBlockID flow.Identifier, skipExisting bool) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), downloadTimeout) | ||
| defer cancel() | ||
|
|
||
|
|
@@ -107,9 +107,17 @@ func (e *Engine) indexBlockTraces(evmBlock *models.Block, cadenceBlockID flow.Id | |
|
|
||
| l := e.logger.With(). | ||
| Str("tx-id", h.String()). | ||
| Uint64("evm-height", evmBlock.Height). | ||
| Str("cadence-block-id", cadenceBlockID.String()). | ||
| Logger() | ||
|
|
||
| if skipExisting { | ||
| if _, err := e.traces.GetTransaction(h); err == nil { | ||
| l.Debug().Msg("trace already downloaded") | ||
| return | ||
| } | ||
| } | ||
|
|
||
| err := retry.Fibonacci(ctx, time.Second*1, func(ctx context.Context) error { | ||
| trace, err := e.downloader.Download(h, cadenceBlockID) | ||
| if err != nil { | ||
|
|
@@ -140,3 +148,46 @@ func (e *Engine) Error() <-chan error { | |
| func (e *Engine) Stop() { | ||
| e.MarkStopped() | ||
| } | ||
|
|
||
| // Backfill redownloads traces for blocks from EVM start to end height. | ||
| func (e *Engine) Backfill(start uint64, end uint64) { | ||
| select { | ||
| case <-e.Ready(): | ||
| case <-e.Done(): | ||
| return | ||
| } | ||
|
|
||
| lg := e.logger.With().Uint64("start", start).Uint64("end", end).Logger() | ||
|
|
||
| lg.Info().Msg("backfilling traces") | ||
| for height := start; height <= end; height++ { | ||
| select { | ||
| case <-e.Done(): | ||
| return | ||
| case <-e.Stopped(): | ||
| return | ||
| default: | ||
| } | ||
|
|
||
| l := lg.With().Uint64("evm-height", height).Logger() | ||
|
|
||
| block, err := e.blocks.GetByHeight(height) | ||
| if err != nil { | ||
| l.Error().Err(err).Msg("failed to get block by height") | ||
| return | ||
| } | ||
|
Comment on lines
+175
to
+178
There was a problem hiding this comment. Modify error handling to continue backfilling despite retrieval errors. Currently, the backfill process exits if it encounters an error when retrieving a block or its Cadence ID (lines 175-178 and 185-188). This means that an error on a single block halts the entire backfill process. To improve robustness, consider logging the error and continuing with the next block instead of returning. Apply this diff to adjust the error handling: --- a/services/traces/engine.go
+++ b/services/traces/engine.go
@@ -176,7 +176,7 @@
if err != nil {
l.Error().Err(err).Msg("failed to get block by height")
- return
+ continue
}
@@ -186,7 +186,7 @@
if err != nil {
l.Error().Err(err).Msg("failed to get cadence block ID")
- return
+ continue
}Also applies to: 185-188 There was a problem hiding this comment. returning is intentional. if these blocks don't exist, it's unlikely future blocks will There was a problem hiding this comment. @peterargue, understood. If returning is intentional due to future blocks being unlikely to exist, no changes are necessary. (_/) ✏️ Learnings added
|
||
|
|
||
| if len(block.TransactionHashes) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| cadenceID, err := e.blocks.GetCadenceID(block.Height) | ||
| if err != nil { | ||
| l.Error().Err(err).Msg("failed to get cadence block ID") | ||
| return | ||
| } | ||
|
|
||
| e.indexBlockTraces(block, cadenceID, true) | ||
|
There was a problem hiding this comment. 🛠️ Refactor suggestion Consider managing concurrency during backfill to prevent resource exhaustion. When calling There was a problem hiding this comment.
There was a problem hiding this comment. @peterargue, thank you for the clarification. It's good to know that 🐰😔 ✏️ Learnings added
|
||
| } | ||
| lg.Info().Msg("done backfilling traces") | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed something that could be problematic, though not really related to this PR.
The
traces-gcp-bucketaccepts a single bucket name, but formainnetwe already have 2:mainnet26-evm-execution-traces1mainnet25-evm-execution-traces1We need to make sure that when we back-fill, the provided heights exist in the appropriate bucket, but I don't know what is contained in those 2 buckets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a check that will log a warning if the start height is before the init-cadence height (378e60f). this may not work for nodes that have been up for a while, but not sure what else we can do without adding a bunch of complexity to the startup logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍 Not much we can do for the time being. Just thought of mentioning it.