Conversation
Summary of ChangesHello @twoeths, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural improvement by implementing a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the BeaconStateView interface and its implementation, providing a read-only facade over the CachedBeaconStateAllForks. This is a positive step towards encapsulating state access logic and improving maintainability. The changes also include refactoring RootCache and proposerShufflingDecisionRoot to utilize this new view, ensuring consistency. New types and utility functions for light client proofs and validator status mapping have also been added. Overall, the changes are well-structured and align with the goal of abstracting state access. There are a few areas identified for potential improvement regarding robustness and efficiency.
| const n2 = n1.left; | ||
| const n5 = n2.right; | ||
| const n10 = n5.left; | ||
| const n21 = n10.right; | ||
| const n43 = n21.right; |
There was a problem hiding this comment.
The direct access to n1.left, n2.right, etc., in getSyncCommitteesWitness relies heavily on the internal structure of the Tree and node properties. This makes the code brittle to future changes in the SSZ tree structure. If the SSZ structure is modified, these direct accesses could break silently or produce incorrect proofs, which is a critical correctness issue for light client proofs. It would be more robust to use explicit GINDEXes or a more abstract way to navigate the tree if available, or at least add comments explaining the gindex for each node access to make it easier to maintain.
| getValidatorsByStatus(statuses: Set<string>, currentEpoch: Epoch): phase0.Validator[] { | ||
| const validators: phase0.Validator[] = []; | ||
| const validatorsArr = this.cachedState.validators.getAllReadonlyValues(); | ||
|
|
||
| for (const validator of validatorsArr) { | ||
| const validatorStatus = getValidatorStatus(validator, currentEpoch); | ||
| if (statuses.has(validatorStatus) || statuses.has(mapToGeneralStatus(validatorStatus))) { | ||
| validators.push(validator); | ||
| } | ||
| } |
There was a problem hiding this comment.
The getValidatorsByStatus method creates a new array validatorsArr by calling this.cachedState.validators.getAllReadonlyValues() on every invocation. If this method is called frequently and the validator list is large, this could lead to unnecessary memory allocations and performance overhead. Consider iterating directly over the cachedState.validators view or caching the getAllReadonlyValues() result if appropriate to improve efficiency.
twoeths
force-pushed
the
te/beacon_state_view_2
branch
from
9bf8e0f to
6884d19
Compare
Performance Report✔️ no performance regression detected Full benchmark results
|
| import {BeaconStateAllForks, CachedBeaconStateAllForks} from "../types.js"; | ||
| import {SyncCommitteeWitness} from "./types.js"; | ||
|
|
||
| export function getSyncCommitteesWitness(fork: ForkName, state: BeaconStateAllForks): SyncCommitteeWitness { |
There was a problem hiding this comment.
this was moved from
although it does not look like it belongs to state-transition, it couples with BeaconStateAllForks and this is the only place we can leave it
twoeths
force-pushed
the
te/beacon_state_view_2
branch
from
1844dcf to
e1d30db
Compare
| /** | ||
| * A read-only view of the BeaconState. | ||
| */ | ||
| export interface IBeaconStateView { |
There was a problem hiding this comment.
We should be consistent with when we're using getters and when we're using functions/methods. Currently, I'm not sure when each is used. There's a few things here that are methods that seem like they should be getters.
Also should be organized very well. For example, latestExecutionPayloadHeader should probably go next to latest BlockHeader? proposersPrevEpoch next to proposers or proposersNextEpoch? Also add spaces / comment lines between "groupings" of properties. In any case, having a nice ordering here will be useful to keep the bindings aligned.
There was a problem hiding this comment.
- Something like this: chore: reorganize IBeaconStateView interface #8810
There was a problem hiding this comment.
We should be consistent with when we're using getters and when we're using functions/methods. Currently, I'm not sure when each is used
my rule is to use getters/fields when they're mandatory properties of the view, use functions if they're one-time use
it's easier for consumers to think that way. In terms of implementation/performance, they are the same
|
waiting for #8810 to be merged into this PR |
twoeths
force-pushed
the
te/beacon_state_view_2
branch
from
f2965d4 to
d151461
Compare
reorder and reorganize IBeaconStateView --------- Co-authored-by: Tuyen Nguyen <twoeths@users.noreply.github.com>
twoeths
force-pushed
the
te/beacon_state_view_2
branch
from
da644c7 to
782e1b1
Compare
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Review Summary
Overall: Approve ✅
This PR introduces a clean abstraction layer (IBeaconStateView) for state access, enabling future decoupling from the TypeScript CachedBeaconState implementation. Code quality is good.
Verified
- ✅ Lazy caching pattern is consistent across all fork-specific getters
- ✅ Fork guards use correct
ForkSeqcomparisons - ✅ Bug fix in
weakSubjectivity.tsis correct (using passedconfigparameter instead ofstate.config) - ✅ Light client proof extraction is a clean code move with no logic changes
- ✅ Type exports (
HistoricalSummaries,ExecutionPayloadBid,GeneralValidatorStatus) are correct
Minor observations (not blocking)
IBeaconStateViewJSDoc says "read-only view" but includesstateTransition/processSlots- these return new views (immutable pattern), so it's correct but comment could clarifycreateBeaconStateViewForHistoricalRegenintentionally passes emptyindex2pubkey: []- documented appropriatelygetExpectedWithdrawalsrelies on underlying function's fork guard - acceptable
Good architectural foundation for the lodestar-z migration path. 🚀
Reviewed with sub-agent assistance (codex-reviewer, gpt-advisor)
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Additional Findings from Sub-Agent Review
After deeper analysis, a few potential bugs were identified:
🔴 Critical
1. Missing Altair guard in getSyncCommitteesWitness
packages/state-transition/src/lightClient/proofs.ts - The function doesn't guard against pre-Altair forks. If called on a Phase0 state, it will crash accessing non-existent sync committee nodes.
Suggested fix:
if (!isForkPostAltair(fork)) {
throw new Error("Sync committees are not available before Altair");
}2. loadOtherState doesn't sync new validators' pubkeys
packages/state-transition/src/stateView/beaconStateView.ts:631-650 - Uses skipSyncPubkeys: true but doesn't manually sync new validators. If the loaded state has new validators not in the seed state, they won't be in pubkey caches.
Compare with loadCachedBeaconState in stateCache.ts which properly syncs new validators.
3. createBeaconStateViewForHistoricalRegen has empty index2pubkey
Passes index2pubkey: [] but methods like computeSyncCommitteeRewards() use it, which will fail.
🟡 Medium
4. Interface/Implementation mismatch - getExpectedWithdrawals interface is missing processedBuildersSweepCount from return type.
These may be acceptable depending on intended usage patterns, but worth flagging for review. Happy to discuss!
|
waiting for the final review from @wemeetagain |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #8773 +/- ##
============================================
+ Coverage 52.33% 52.35% +0.01%
============================================
Files 848 848
Lines 63363 63341 -22
Branches 4696 4696
============================================
- Hits 33163 33161 -2
+ Misses 30131 30111 -20
Partials 69 69 🚀 New features to boost your workflow:
|
4 participants
Motivation
Description
IBeaconStateViewinterfacetsBeaconStateViewblocked by #8728 to build successfully