-
-
Notifications
You must be signed in to change notification settings - Fork 287
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: use binary diff to persist finalized states #7005
base: feature/differential-archive
Are you sure you want to change the base?
Conversation
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
|
packages/beacon-node/src/chain/historicalState/historicalState.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/historicalState/historicalState.ts
Outdated
Show resolved
Hide resolved
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need some logs here
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.
Yes a lot of comments need to be added, working on it.
docs/pages/contribution/advanced-topics/historical-state-regen.md
Outdated
Show resolved
Hide resolved
docs/pages/contribution/advanced-topics/historical-state-regen.md
Outdated
Show resolved
Hide resolved
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need a comment and benchmark/evidence on how do we come up with this default value and what does this mean
return activeState; | ||
} | ||
|
||
export async function getDiffState( |
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.
add comment for this method, does this return a state bytes or a state diff?
} | ||
|
||
try { | ||
const diffState = await replayStateDiffs({diffs: nonEmptyDiffs, snapshotState}, {codec}); |
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.
confirm the loaded state bytes using getStateSlotFromBytes()
util and compare to slot
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment to this class, noting that only putBinary(), getBinary()
are used so no need a type?
{dontTransferCache: true}, | ||
RegenCaller.historicalState | ||
); | ||
await this.historicalSates?.storeHistoricalState(state.slot, state.serialize()); |
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.
be careful with this memory allocation as it could cause gc
to run for > 1s
need to use BufferPool
and an util similar to
lodestar/packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
Line 766 in 20c18ad
private serializeState(state: CachedBeaconStateAllForks): BufferWithKey | null { |
in the future I'll implement serializeInto()
api in ssz to make it more convenient
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.
Should we duplicate this logic into utility or expose an interface from the regen?
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 refactored to have a utility for it in this PR #7042
you can rebased against unstable
when that's merged @nazarhussain
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.
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
* Persist states every some epochs to |
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
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure we have equivalent metrics with new approach if applicable
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.
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? |
@@ -27,56 +15,15 @@ export interface StatesArchiverOpts { | |||
*/ | |||
export class StatesArchiver { | |||
constructor( | |||
private readonly historicalSates: IHistoricalStateRegen | undefined, |
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.
private readonly historicalSates: IHistoricalStateRegen | undefined, | |
private readonly historicalStates: IHistoricalStateRegen | undefined, |
@nazarhussain
|
7eb7c17
to
d22df00
Compare
Motivation
Reduce the storage requirement and improve performance for the state regeneration for archival node.
Description
Steps to test or reproduce