-
-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add historical state regen #6033
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
packages/beacon-node/src/chain/historicalState/getHistoricalState.ts
Outdated
Show resolved
Hide resolved
Running into issues opening the db in the worker thread. It seems the leveldb way of allowing concurrency is have the consumer pass the pointer to the db handle to another thread. That's not currently possible with the leveldb package as it currently exists. |
Converting to draft until the multithreaded database access issue is resolved |
Any update on the status of this PR? Are we blocked on it somewhere externally? |
waiting on me to fix this in response to @matthewkeil 's review |
Ready for another round of review |
packages/beacon-node/src/chain/historicalState/getHistoricalState.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/historicalState/getHistoricalState.ts
Outdated
Show resolved
Hide resolved
this PR implements the fundamental work for historical state regen on worker thread, however I'm not sure how likely it could be fully functional in production, a request could take so long given we could explore storing incremental state diff with 2 purposes:
|
That is already in progress and will be complete very soon. |
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.
LGTM! I think all the important stuff was addressed 🎸
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.
LGTM - just a few minor comments / questions
} | ||
|
||
return {state: stateSerialized, executionOptimistic: false, finalized: true}; |
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.
we have access to the block here, shouldn't this rather be executionOptimistic: isOptimisticBlock(finalizedBlock)
, afaik a finalized block can still be optimistic
// TODO: Pass options from main thread for logging | ||
// TODO: Logging won't be visible in file loggers |
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.
aren't both of these todos stale? file logging works and opts are passed from main thread via workerData
regenTime: metricsRegister.histogram({ | ||
name: "lodestar_historical_state_regen_time_seconds", | ||
help: "Time to regenerate a historical state in seconds", | ||
// Historical state regen can take up to 3h as of Aug 2024 |
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.
We really need to tell people in the release notes that they should reduce archive state frequency if they want access to historical data, do we have a recommended value which keep state regen times below 30s while not blowing up storage requirements?
Also it might be a good time to unhide the flag as per #6908
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.
in my opinion we should not inform, we need to test this work carefully first
I really want to make sure we have a great performance before we inform
see #6033 (comment)
🎉 This PR is included in v1.21.0 🎉 |
Motivation
Description
TODO