-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
Precompute epoch transition #3383
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3383 +/- ##
==========================================
- Coverage 38.36% 38.07% -0.30%
==========================================
Files 303 304 +1
Lines 7738 7843 +105
Branches 1157 1189 +32
==========================================
+ Hits 2969 2986 +17
- Misses 4628 4716 +88
Partials 141 141 |
Code Climate has analyzed commit 48b8e51 and detected 0 issues on this pull request. View more on Code Climate. |
Performance Report✔️ no performance regression detected Full benchmark results
|
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
371622d
to
644b60b
Compare
@@ -538,5 +538,22 @@ export function createLodestarMetrics( | |||
name: "unhandeled_promise_rejections", | |||
help: "UnhandeledPromiseRejection count", | |||
}), | |||
|
|||
// Precompute epoch | |||
preComputeEpoch: { |
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.
The name preComputeEpoch
is bit too generic could we do precomputeNextEpochTransition
? If sounds good please rename files and classes on all the PR
// Precompute epoch | ||
preComputeEpoch: { | ||
count: register.counter<"result">({ | ||
name: "precompute_epoch_result", |
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.
See #3399
count: register.counter<"result">({ | ||
name: "precompute_epoch_result", | ||
labelNames: ["result"], | ||
help: "Number of precompute epoch skip/success/error", |
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.
Total number of precomputeNextEpochTransition runs by result
.getBlockSlotState(blockRoot, nextSlot, RegenCaller.preComputeEpoch) | ||
.then(() => { | ||
this.metrics?.preComputeEpoch.count.inc({result: "success"}, 1); | ||
const previousHits = this.chain.checkpointStateCache.updatePreComputedCheckpoint(blockRoot, nextEpoch); |
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.
Can you find a a way to track this without adding custom code in the checkpointStateCache? Ideally this class should have sufficient info to know if the previous run was re-orged or not.
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.
@dapplion even when our node is reorged, it does not mean the pre computed epoch transition is wasteful as some nodes can still send attestations with our old/obsolete target checkpoint and we still need that cached target checkpoint. So for this waste
metric, it does not depend on the node being re-orged or not, it depends on the produced checkpoint state is used or not.
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.
Yeah, that's true. Let's leave it like this then
13f97c0
to
48b8e51
Compare
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.
Can you make the naming consistent?
Either precompute or preCompute, not both
Other than that lgtm
Motivation
Description
Closes Early epoch transition when node is synced #3382
Test