[wip] feat(bindings): align BeaconStateView with IBeaconStateView#347
[wip] feat(bindings): align BeaconStateView with IBeaconStateView#347spiral-ladder wants to merge 22 commits intomainfrom
BeaconStateView with IBeaconStateView#347Conversation
`IBeaconStateView` expects `RootHex` (66-char string) for these outputs see: https://github.com/ChainSafe/lodestar/blob/35940ffd61ad7e29f5de376e13587d044b27b246/packages/state-transition/src/stateView/interface.ts#L78-L82
`IBeaconStateView` expects `RootHex` (66-char string) for these outputs see: https://github.com/ChainSafe/lodestar/blob/35940ffd61ad7e29f5de376e13587d044b27b246/packages/state-transition/src/stateView/interface.ts#L78-L82
Summary of ChangesHello, 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 significantly expands the native BeaconStateView API to provide more comprehensive access to beacon state data and transition logic. It introduces new methods for querying shuffling, state roots, and validator information, while also refining existing serialization and execution-related functions to better align with the requirements of the Lodestar beacon node. 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. 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 expands the BeaconStateView N-API bindings to align with the IBeaconStateView interface, adding methods for shuffling data, validator/balance queries, and optimized execution-enabled checks. It also includes a fix for the argument order in isMergeTransitionBlock. The review feedback highlights critical risks of precision loss when converting u64 Gwei values to JS Number and recommends reverting these to BigInt. Furthermore, the reviewer suggests optimizing getValidatorsByStatus to avoid excessive N-API overhead and deduplicating the transition options parsing logic to improve maintainability.
| pub fn getBalance(self: *const BeaconStateView, index_arg: js.Number) !js.Number { | ||
| const cached_state = try self.requireState(); | ||
| const index_value: u64 = @intCast(try index_arg.toI64()); | ||
| const index_value: u64 = try index_arg.toU32(); | ||
| var balances = try cached_state.state.balances(); | ||
| const balance = try balances.get(index_value); | ||
| return js.BigInt.from(balance); | ||
| return js.Number.from(balance); | ||
| } |
There was a problem hiding this comment.
Changing the return type of getBalance from js.BigInt to js.Number introduces a risk of precision loss. Ethereum balances in Gwei are u64 values that can exceed Number.MAX_SAFE_INTEGER (2^53 - 1). For example, a total supply of 120M ETH is ~1.2e17 Gwei, which is significantly larger than 9e15. To ensure correctness and maintain compatibility with the existing IBeaconStateView interface, this should remain a BigInt.
pub fn getBalance(self: *const BeaconStateView, index_arg: js.Number) !js.BigInt {
const cached_state = try self.requireState();
const index_value: u64 = try index_arg.toU32();
var balances = try cached_state.state.balances();
const balance = try balances.get(index_value);
return js.BigInt.from(balance);
}
| for (validators) |*validator| { | ||
| const status = st.getValidatorStatus(validator, current_epoch); | ||
| const status_str = try env.createStringUtf8(status.toString()); | ||
| const has_result = try env.callFunction(has_fn, set_value, .{status_str}); | ||
| if (try has_result.getValueBool()) { | ||
| const v_napi = try sszValueToNapiValue(env, ct.phase0.Validator, validator); | ||
| try result.setElement(out_idx, v_napi); | ||
| out_idx += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of getValidatorsByStatus is highly inefficient. It performs multiple N-API calls (createStringUtf8, callFunction) inside a loop that iterates over the entire validator registry (which can contain over 1 million entries on mainnet). This will cause severe performance degradation. A better approach would be to pre-process the statuses_set into a Zig-native bitset or a small array of booleans corresponding to the ValidatorStatus enum values, and then perform the filtering entirely in Zig.
| try obj.setNamedProperty("attestations", try env.createDouble(@floatFromInt(rewards.attestations))); | ||
| try obj.setNamedProperty("syncAggregate", try env.createDouble(@floatFromInt(rewards.sync_aggregate))); | ||
| try obj.setNamedProperty("slashing", try env.createDouble(@floatFromInt(rewards.slashing))); |
There was a problem hiding this comment.
Similar to the getBalance issue, converting u64 reward values to JS Number via createDouble can lead to precision loss if the values exceed 2^53 - 1. These fields should continue to use BigInt to safely represent the full range of u64 Gwei values.
try obj.setNamedProperty("attestations", try env.createBigintUint64(rewards.attestations));
try obj.setNamedProperty("syncAggregate", try env.createBigintUint64(rewards.sync_aggregate));
try obj.setNamedProperty("slashing", try env.createBigintUint64(rewards.slashing));
| attestations: js.Number, | ||
| syncAggregate: js.Number, | ||
| slashing: js.Number, |
There was a problem hiding this comment.
Reverting these fields to js.BigInt to avoid precision loss for u64 Gwei values, consistent with the feedback in BeaconStateView.zig.
attestations: js.BigInt,
syncAggregate: js.BigInt,
slashing: js.BigInt,
There was a problem hiding this comment.
We use number in IBeaconStateView instead of big ints
| attestations: number; | ||
| syncAggregate: number; | ||
| slashing: number; |
There was a problem hiding this comment.
Reverting to bigint to prevent precision loss for Gwei values and maintain consistency with the native implementation.
| attestations: number; | |
| syncAggregate: number; | |
| slashing: number; | |
| attestations: bigint; | |
| syncAggregate: bigint; | |
| slashing: bigint; |
| effectiveBalanceIncrements: Uint16Array; | ||
| getEffectiveBalanceIncrementsZeroInactive(): Uint16Array; | ||
| getBalance(index: number): bigint; | ||
| getBalance(index: number): number; |
There was a problem hiding this comment.
Reverting the return type to bigint to ensure safe representation of Gwei balances.
| getBalance(index: number): number; | |
| getBalance(index: number): bigint; |
| if (options) |opt_val| { | ||
| const raw = opt_val.toValue(); | ||
| if (try raw.typeof() == .object) { | ||
| if (try raw.hasNamedProperty("verifyStateRoot")) | ||
| opts.verify_state_root = try (try raw.getNamedProperty("verifyStateRoot")).getValueBool(); | ||
| if (try raw.hasNamedProperty("verifyProposer")) | ||
| opts.verify_proposer = try (try raw.getNamedProperty("verifyProposer")).getValueBool(); | ||
| if (try raw.hasNamedProperty("verifySignatures")) | ||
| opts.verify_signatures = try (try raw.getNamedProperty("verifySignatures")).getValueBool(); | ||
| if (try raw.hasNamedProperty("transferCache")) | ||
| opts.transfer_cache = try (try raw.getNamedProperty("transferCache")).getValueBool(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for parsing transition options from a JS object is duplicated here. It would be better to share the parseOptions implementation from stateTransition.zig to ensure consistency and reduce maintenance overhead.
spiral-ladder
force-pushed
the
bing/native-stf
branch
from
b8a1fd7 to
a9ab3ec
Compare
spiral-ladder
force-pushed
the
bing/native-stf
branch
from
a9ab3ec to
4b70286
Compare
spiral-ladder
changed the title
BeaconStateView with IBeaconStateView
spiral-ladder
force-pushed
the
bing/native-stf
branch
from
4b70286 to
0525390
Compare
spiral-ladder
force-pushed
the
bing/native-stf
branch
from
0525390 to
fd1c0a9
Compare
Reverts the IBeaconStateView-shape object signature for getVoluntaryExitValidity and isValidVoluntaryExit. Bytes is one FFI hop (vs ~8 for field-walking), reuses the SSZ deserializer (no manual u64->u32 truncation), and is robust to schema changes. Tests already pass Uint8Array(112) so they work as-is.
extracted from #347 `IBeaconStateView` expects `RootHex` (66-char string) for these outputs. it also happens that it's cheaper to just create a string upfront than go through a typed array see: https://github.com/ChainSafe/lodestar/blob/35940ffd61ad7e29f5de376e13587d044b27b246/packages/state-transition/src/stateView/interface.ts#L78-L82
spiral-ladder
force-pushed
the
bing/native-stf
branch
from
04638b8 to
67084e3
Compare
spiral-ladder
force-pushed
the
bing/native-stf
branch
from
7caa682 to
342c1dc
Compare
spiral-ladder
force-pushed
the
bing/native-stf
branch
4 times, most recently
from
dfb1078 to
5dc9b65
Compare
spiral-ladder
force-pushed
the
bing/native-stf
branch
3 times, most recently
from
be8058e to
276cb64
Compare
- add bindings to it - don't heap allocate for `withdrawals_results` since it is capped at `preset.MAX_WITHDRAWALS_PER_PAYLOAD == 16`. This is about max ~800 bytes on mainnet preset - add assertions to assert the above
spiral-ladder
force-pushed
the
bing/native-stf
branch
from
ed6979b to
284e2bc
Compare
- add native `loadState` that mirrors TS implementation - add bindings - add benchmarks to bench naive + loadState difference part of #347
- add native `loadState` that mirrors TS implementation - add bindings - add benchmarks to bench naive + loadState difference part of #347
- add native `loadState` that mirrors TS implementation - add bindings - add benchmarks to bench naive + loadState difference part of #347
- add native `loadState` that mirrors TS implementation - add bindings - add benchmarks to bench naive + loadState difference part of #347
- add bindings to it - don't heap allocate for `withdrawals_results` since it is capped at `preset.MAX_WITHDRAWALS_PER_PAYLOAD == 16`. This is about max ~800 bytes on mainnet preset - add assertions to assert the above extracted from #347
1 participant
This PR aligns the bindings to
BeaconStateViewand its native implementation with the requirements of the typescript interface found atIBeaconStateView.A list of things:
fix some ergonomics stuff (using u32 instead of u64)Closed refactor(bindings): usetoU32()where it makes sense instead oftoI64()#349 bc it doesn't make senseHave broken this up into various smaller PRs for reviewability:
#342, #348, #350, #351