feat: use binary diff to persist finalized states #7005
+2,375
−454
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/differential-archive #7005 +/- ##
================================================================
- Coverage 50.91% 50.90% -0.01%
================================================================
Files 594 594
Lines 39609 39602 -7
Branches 2245 2254 +9
================================================================
- Hits 20165 20160 -5
+ Misses 19444 19442 -2 |
Performance Report✔️ no performance regression detected Full benchmark results
|
twoeths
reviewed
Aug 15, 2024
twoeths
reviewed
Aug 15, 2024
twoeths
reviewed
Aug 15, 2024
twoeths
reviewed
Aug 15, 2024
packages/beacon-node/src/chain/historicalState/historicalState.ts
Outdated
Show resolved
Hide resolved
twoeths
reviewed
Aug 15, 2024
packages/beacon-node/src/chain/historicalState/historicalState.ts
Outdated
Show resolved
Hide resolved
twoeths
reviewed
Aug 15, 2024
| case StateArchiveStrategy.Diff: { | ||
| const {diffState} = await getDiffState({slot, skipSlotDiff: true}, {db, metrics, logger, diffLayers, codec}); | ||
|
|
||
| if (!diffState) return; |
There was a problem hiding this comment.
Yes a lot of comments need to be added, working on it.
twoeths
reviewed
Aug 15, 2024
twoeths
reviewed
Aug 15, 2024
docs/pages/contribution/advanced-topics/historical-state-regen.md
Outdated
Show resolved
Hide resolved
twoeths
reviewed
Aug 15, 2024
docs/pages/contribution/advanced-topics/historical-state-regen.md
Outdated
Show resolved
Hide resolved
twoeths
reviewed
Aug 15, 2024
| import {SLOTS_PER_EPOCH} from "@lodestar/params"; | ||
| import {StateArchiveStrategy} from "./types.js"; | ||
|
|
||
| export const DEFAULT_DIFF_LAYERS = "4, 64, 256, 1024"; |
There was a problem hiding this comment.
need a comment and benchmark/evidence on how do we come up with this default value and what does this mean
twoeths
reviewed
Aug 15, 2024
twoeths
reviewed
Aug 15, 2024
| return activeState; | ||
| } | ||
|
|
||
| export async function getDiffState( |
There was a problem hiding this comment.
add comment for this method, does this return a state bytes or a state diff?
twoeths
reviewed
Aug 15, 2024
| } | ||
|
|
||
| try { | ||
| const diffState = await replayStateDiffs({diffs: nonEmptyDiffs, snapshotState}, {codec}); |
There was a problem hiding this comment.
confirm the loaded state bytes using getStateSlotFromBytes() util and compare to slot
twoeths
reviewed
Aug 15, 2024
| import {Bucket, getBucketNameByValue} from "../buckets.js"; | ||
| import {getRootIndexKey} from "./stateArchiveIndex.js"; | ||
|
|
||
| export class StateDiffArchiveRepository extends Repository<Slot, Uint8Array> { |
There was a problem hiding this comment.
add comment to this class, noting that only putBinary(), getBinary() are used so no need a type?
twoeths
reviewed
Aug 15, 2024
| {dontTransferCache: true}, | ||
| RegenCaller.historicalState | ||
| ); | ||
| await this.historicalSates?.storeHistoricalState(state.slot, state.serialize()); |
There was a problem hiding this comment.
be careful with this memory allocation as it could cause gc to run for > 1s
need to use BufferPool and an util similar to
in the future I'll implement serializeInto() api in ssz to make it more convenient
There was a problem hiding this comment.
Should we duplicate this logic into utility or expose an interface from the regen?
There was a problem hiding this comment.
I refactored to have a utility for it in this PR #7042
you can rebased against unstable when that's merged @nazarhussain
twoeths
reviewed
Aug 15, 2024
There was a problem hiding this comment.
this looks great, I guess I need much more time to review through. Need to add much more comments on how the strategy works and the comment for classes/methods to make sure we have a maintainable code. I suggest to have this style of comment to make it easier to understand for everyone
also need more insight for this work:
- how much disc space does it need for storing states with the current approach, compared to the default config of the new approach
- need benchmark results to come up with the default config
- the time to compute state diff and apply it, and the disc space needed
- need to download mainnet/holesky states for a good estimate
twoeths
reviewed
Aug 15, 2024
| @@ -4,7 +4,7 @@ export {BlobSidecarsArchiveRepository} from "./blobSidecarsArchive.js"; | |||
| export {BlockRepository} from "./block.js"; | |||
| export {BlockArchiveRepository} from "./blockArchive.js"; | |||
| export type {BlockArchiveBatchPutBinaryItem, BlockFilterOptions} from "./blockArchive.js"; | |||
| export {StateArchiveRepository} from "./stateArchive.js"; | |||
There was a problem hiding this comment.
also remove stateArchive.ts because we don't use it anymore? if yes I suppose we need a new cli to prune it, this helps users a lot of saving disc space
twoeths
reviewed
Aug 15, 2024
| let queueMetrics: QueueMetrics | undefined; | ||
| if (metricsRegister) { | ||
| const closeMetrics = collectNodeJSMetrics(metricsRegister, "lodestar_historical_state_worker_"); | ||
| abortController.signal.addEventListener("abort", closeMetrics, {once: true}); | ||
|
|
||
| historicalStateRegenMetrics = { |
There was a problem hiding this comment.
make sure we have equivalent metrics with new approach if applicable
There was a problem hiding this comment.
yes added new metrics related to it.
- State Diff Size
- State Snapshot Size
There is still some work left to optimize the diff size:
Afterwards we can have more realistic disk space estimation. |
could you add TODOs in the description? |
twoeths
reviewed
Aug 21, 2024
| @@ -27,56 +15,15 @@ export interface StatesArchiverOpts { | |||
| */ | |||
| export class StatesArchiver { | |||
| constructor( | |||
| private readonly historicalSates: IHistoricalStateRegen | undefined, | |||
There was a problem hiding this comment.
Suggested change
| private readonly historicalSates: IHistoricalStateRegen | undefined, | |
| private readonly historicalStates: IHistoricalStateRegen | undefined, |
@nazarhussain |
nazarhussain
force-pushed
the
nh/state-diff-2
branch
from
2800dce
to
bad4039
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Reduce the storage requirement and improve performance for the state regeneration for archival node.
Description
Steps to test or reproduce