[Access] Bootstrap register db and start indexer #4780
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4780 +/- ##
==========================================
+ Coverage 55.78% 57.17% +1.38%
==========================================
Files 939 773 -166
Lines 86902 72771 -14131
==========================================
- Hits 48480 41607 -6873
+ Misses 34771 27998 -6773
+ Partials 3651 3166 -485
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // other components from starting while bootstrapping the register db since it may | ||
| // take hours to complete. | ||
|
|
||
| pdb, err := pStorage.OpenRegisterPebbleDB(builder.registersDBPath) |
There was a problem hiding this comment.
How come this is not created as a module?
There was a problem hiding this comment.
it could be, but we only use it here so I figured it wasn't necessary.
| // since indexing is done using execution data, make sure that execution data | ||
| // will be available starting at the checkpoint height. otherwise, the indexer | ||
| // will fail to progress. | ||
| if builder.checkpointHeight < builder.executionDataConfig.InitialBlockHeight { |
There was a problem hiding this comment.
This is out of the scope of this PR but is there no way to get the height of the checkpoint from the checkpoint? This feels like we could set it to the wrong value or skip some heights by mistake. cc @zhangchiqing @koko1123
Also, is the check sufficient? What if the checkpoint height is bigger than the initial height of exe data, isn't that unexpected too? Shouldn't the height always be -1 the exe data height? What is the case where this is not true?
There was a problem hiding this comment.
This check is sufficient for root block, but is not a check we should keep if we're using a checkpoint generated by dynamic bootstrap later, but we're at least 3 months away from that
There was a problem hiding this comment.
is there no way to get the height of the checkpoint from the checkpoint?
my understanding is that checkpoints currently do not include the height. That is coming in a future improvement from the storehouse work.
Also, is the check sufficient? What if the checkpoint height is bigger than the initial height of exe data, isn't that unexpected too? Shouldn't the height always be -1 the exe data height? What is the case where this is not true?
At the beginning of the spork, checkpointHeight should the the same as InitialBlockHeight. Both should be the root height. For nodes starting after the spork, or using a smaller execution data retention window, they may not be the same.
My 2 cents is it's better to be flexible where possible (and safe). that way the software can handle situations we may not anticipated, like recovering from an outage. In this case, we just need to make sure that the configuration allows for indexing to work. As long as there is execution data available from checkpoint + 1 and higher, the indexer will be able to progress.
|
|
||
| return builder.ExecutionDataRequester, nil | ||
| }) | ||
|
|
||
| if builder.executionDataIndexingEnabled { |
There was a problem hiding this comment.
I see there are a couple of scenarios where building a component depends on a boolean flag, would it make sense to have a builder option for it, something like so:
builder.
Requires(builder.executionDataIndexingEnabled).
Module("...") // ...If the value is false the whole procedure would stop. Likely unrelated to this PR, just as a thought.
There was a problem hiding this comment.
yea, that's a cool idea. It would require a bit of refactoring in the node builder, but could make some of the bootstrapping logic cleaner.
There aren't a ton of places we load conditional modules/components like this though. mostly in the AN/ON codebase since they have more user options
|
|
||
| checkpointFile := builder.checkpointFile | ||
| if checkpointFile == cmd.NotSet { | ||
| checkpointFile = path.Join(builder.BootstrapDir, bootstrap.PathRootCheckpoint) |
There was a problem hiding this comment.
should we make this file bootstrap.PathRootCheckpoint to be used by the bootstraping checkpoint as well? what is this file used for otherwise? for producing checkpoint on demand?
There was a problem hiding this comment.
that's a good question. what's your take @zhangchiqing ?
Closes: #4778
Bootstrap the register db and start the indexer on Access nodes.