-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
chore: refactor block and state api utils #6963
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #6963 +/- ##
============================================
- Coverage 62.52% 62.49% -0.03%
============================================
Files 576 576
Lines 61171 61171
Branches 2140 2139 -1
============================================
- Hits 38245 38231 -14
- Misses 22888 22901 +13
- Partials 38 39 +1 |
Performance Report✔️ no performance regression detected Full benchmark results
|
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
Co-authored-by: Nico Flaig <nflaig@protonmail.com>
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
// TODO: This is not OK, head and headState must be fetched atomically | ||
const head = chain.forkChoice.getHead(); | ||
const headState = chain.getHeadState(); | ||
return {state: headState, executionOptimistic: isOptimisticBlock(head), finalized: false}; |
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.
@wemeetagain wondering here, we got the head state from getHeadState()
which called getClosestHeadState
so we would previouly do this
this.checkpointStateCache.getLatest(head.blockRoot, Infinity, opts) || |
which queried both blockStateCache
and checkpointStateCache
while now we would just query blockStateCache
lodestar/packages/beacon-node/src/chain/regen/queued.ts
Lines 79 to 80 in fed08fe
getStateSync(stateRoot: RootHex): CachedBeaconStateAllForks | null { | |
return this.blockStateCache.get(stateRoot, {dontTransferCache: 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.
@nflaig , the change actually made it better. Previously we also queried the head state pulled up to next epoch in the checkpointStateCache
which is only necessary for block processing while the "getHeadState()" api only wants the real head state. So it's better to only query blockStateCache
in this case
🎉 This PR is included in v1.21.0 🎉 |
Motivation
Description
resolveBlockId
andresolveStateId
)resolve*
functions return a concrete slot or root which can be fetched using the chaingetBlockResponse
andgetStateResponse
getStateResponseWithRegen
is created with unimplemented logic and a special return type.