feat: use bps instead of INTERVALS_PER_SLOT for deadlines#8091
feat: use bps instead of INTERVALS_PER_SLOT for deadlines#8091
INTERVALS_PER_SLOT for deadlines#8091Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
INTERVALS_PER_SLOT for deadlinesINTERVALS_PER_SLOT for deadlines
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #8091 +/- ##
============================================
+ Coverage 52.22% 52.26% +0.03%
============================================
Files 852 852
Lines 64895 64976 +81
Branches 4767 4770 +3
============================================
+ Hits 33893 33957 +64
- Misses 30932 30950 +18
+ Partials 70 69 -1 🚀 New features to boost your workflow:
|
This makes it easier to reason about the code
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring by replacing hardcoded time-based deadlines (using INTERVALS_PER_SLOT) with configurable basis points (BPS) values. This greatly improves the flexibility and clarity of the codebase.
The changes are extensive and have been applied consistently across configuration, core logic, tests, and metrics. The introduction of SLOT_DURATION_MS and new ForkConfig getters for deadlines is well-executed.
I've found a couple of minor spots where the old hardcoded logic remains, and I've left suggestions to align them with the new BPS-based approach for full consistency.
Overall, this is an excellent contribution that enhances the maintainability of the project.
packages/beacon-node/src/chain/stateCache/persistentCheckpointsCache.ts
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring to replace hardcoded slot interval timings with a more flexible and precise system based on basis points (BPS). The changes are systematic across the codebase, introducing new configuration constants for deadlines, deprecating SECONDS_PER_SLOT in favor of SLOT_DURATION_MS for better precision, and adding helper methods to centralize timing calculations. This improves the configurability and maintainability of the timing-sensitive logic. The updates to tests, metrics, and comments are consistent with these changes. Overall, this is a high-quality improvement with no apparent issues.
| const src = OpSource.gossip; | ||
| const data = indexedAttestation.data; | ||
| const epoch = computeEpochAtSlot(data.slot); | ||
| const fork = config.getForkName(data.slot); |
There was a problem hiding this comment.
Determining hard fork based on data.slot could potentially have issue at fork boundary.
So when we switch over to 6s slot, we will need to revisit all these timings.
There was a problem hiding this comment.
In this case it's what we want since we evaluate the attestation based on the fork it belongs to, we also use data.slot in the calculation to determine the delay.
Follow up on #8091 to clean up remaining usage of `SECONDS_PER_SLOT`. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
🎉 This PR is included in v1.35.0 🎉 |
Closes #8013