fix: isExecutionEnabled accepts block object#379
Conversation
`IBeaconStateView` (and all instances of calling `isExecutionEnabled` in lodestar) deals with the block object, so lets do the same.
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 refactors the Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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
|
spiral-ladder
force-pushed
the
bing/is-ex-enabled
branch
from
85bb524 to
aab639d
Compare
![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 refactors the isExecutionEnabled method in BeaconStateView.zig to accept a Lodestar-shaped block object instead of raw bytes, short-circuiting early on post-merge states without parsing the block. It also introduces a native parser executionPayloadFromJs to reconstruct the Bellatrix execution payload from JS objects. The code review identified critical issues: a potential silent truncation of bigints larger than 256 bits in readBigintU256, a missing implementation for the newly declared isMergeTransitionBlock method in the TypeScript bindings, and a violation of the 100-column line limit rule.
| fn readBigintU256(value: napi.Value) !u256 { | ||
| var sign_bit: c_int = 0; | ||
| var word_count: usize = 4; | ||
| var words: [4]u64 = .{ 0, 0, 0, 0 }; | ||
| try napi.status.check(napi.c.napi_get_value_bigint_words( | ||
| value.env, | ||
| value.value, | ||
| &sign_bit, | ||
| &word_count, | ||
| &words, | ||
| )); | ||
| if (sign_bit != 0) return error.NegativeBigint; | ||
| var result: u256 = 0; | ||
| var i: usize = 0; | ||
| while (i < @min(word_count, 4)) : (i += 1) { | ||
| result |= @as(u256, words[i]) << @intCast(i * 64); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
In readBigintU256, if the JS bigint is larger than 256 bits, napi_get_value_bigint_words will write the first 4 words and update word_count to the total number of words required (which will be greater than 4).
Because the loop condition uses @min(word_count, 4), any words beyond the 4th will be silently ignored, resulting in silent truncation of the bigint. To prevent silent truncation and potential consensus/correctness issues, we should explicitly check if word_count > 4 and return an error (e.g., error.BigintOverflow).
fn readBigintU256(value: napi.Value) !u256 {
var sign_bit: c_int = 0;
var word_count: usize = 4;
var words: [4]u64 = .{ 0, 0, 0, 0 };
try napi.status.check(napi.c.napi_get_value_bigint_words(
value.env,
value.value,
&sign_bit,
&word_count,
&words,
));
if (sign_bit != 0) return error.NegativeBigint;
if (word_count > 4) return error.BigintOverflow;
var result: u256 = 0;
var i: usize = 0;
while (i < word_count) : (i += 1) {
result |= @as(u256, words[i]) << @intCast(i * 64);
}
return result;
}
| /** True iff state is pre-merge AND the given block carries a non-default execution payload. Bellatrix-only. */ | ||
| isMergeTransitionBlock(signedBlockBytes: Uint8Array): boolean; |
There was a problem hiding this comment.
The method isMergeTransitionBlock is declared in the BeaconStateView class interface, but it is not implemented or exported in BeaconStateView.zig. Calling this method at runtime will result in a TypeError. Please implement this method in BeaconStateView.zig or remove it from the type definitions if it is not intended to be exposed.
| const merge_complete: bool = switch (fork_seq) { | ||
| inline .bellatrix, .capella, .deneb, .electra, .fulu => |f| st.isMergeTransitionComplete(f, cached_state.state.castToFork(f)), | ||
| else => unreachable, | ||
| }; |
There was a problem hiding this comment.
This line exceeds the 100-column limit specified in the Repository Style Guide (Rule 400). Please wrap the switch arm to keep it under 100 columns.
const merge_complete: bool = switch (fork_seq) {
inline .bellatrix, .capella, .deneb, .electra, .fulu => |f| st.isMergeTransitionComplete(
f,
cached_state.state.castToFork(f),
),
else => unreachable,
};
References
- Rule 400: Hard limit all line lengths, without exception, to at most 100 columns. (link)
| /// Read a JS bigint into u256. | ||
| /// | ||
| /// Throws on negative values; we never store signed u256 in consensus types. | ||
| fn readBigintU256(value: napi.Value) !u256 { |
There was a problem hiding this comment.
this has to be fixed on the zapi level first since casting a u1 to *c_int is UB due to alignment
spiral-ladder
force-pushed
the
bing/is-ex-enabled
branch
from
aab639d to
b784ca9
Compare
1 participant
IBeaconStateView(and all instances of callingisExecutionEnabledin lodestar) deals with the block object, so lets do the same.