[Access] Indexer remove redundant header reads #8226
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| // we need loop here because it's possible for a height to be missed here, | ||
| // we should guarantee all heights are processed | ||
| for height := lastProcessedHeight + 1; height <= highestIndexedHeight; height++ { | ||
| header, err := i.indexer.headers.ByHeight(height) |
There was a problem hiding this comment.
This call might cause 30% CPU spikes on startup for hours, if there are lots of height to iterate.
We are not using the header, I don't think it's necessary to query it.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| if err != nil { | ||
| // if the execution data is available, the block must be locally finalized | ||
| i.log.Error().Err(err).Msgf("could not get header for height %d:", height) | ||
| return fmt.Errorf("could not get header for height %d: %w", height, err) |
There was a problem hiding this comment.
This error would not happen, because AN is only syncing execution data for sealed blocks, so an indexed height must be finalized already.
There was a problem hiding this comment.
AN is only syncing execution data for sealed blocks
this is currently true. this system will be replaced soon by one that does sync unsealed data, but this logic is not used.
| // return fmt.Errorf("could not get header for height %d: %w", height, err) | ||
| // } | ||
|
|
||
| i.OnBlockProcessed(height) |
There was a problem hiding this comment.
I'm actually not sure why this is needed. Do we remember the original issue that requires us to add this patch?
There was a problem hiding this comment.
I believe it was related to the execution data tracker/pruner, but I don't remember why we needed to explicitly check that the header existed locally.
This PR removes a unnecessary header reads that might cause CPU spikes during startup.