feat: support native loadState - #351
Conversation
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 introduces a native 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
|
![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 a tree-reuse optimized loadOtherState method to BeaconStateView and a corresponding utility module in src/state_transition/utils/load_state.zig to improve state loading performance by sharing unchanged validator and inactivity score subtrees. The reviewer highlighted several violations of the repository style guide, specifically the use of recursion in findModifiedValidators and findModifiedInactivityScores, insufficient assertion density across the new utility functions, and a function length exceeding the 70-line limit in loadState. Additionally, a potential memory leak was identified regarding the deinitialization of TreeView objects returned by seed_view.get(i).
I am having trouble creating individual review comments. Click here to see my feedback.
src/state_transition/utils/load_state.zig (23-41)
The use of recursion in findModifiedValidators violates the repository style guide (Rule 32), which prohibits recursion to ensure bounded execution. Please refactor this to an iterative implementation. A simple loop over the validator chunks would be safer and likely sufficiently performant for this use case.
References
- Do not use recursion to ensure that all executions that should be bounded are bounded. (link)
src/state_transition/utils/load_state.zig (45-63)
The use of recursion in findModifiedInactivityScores violates the repository style guide (Rule 32). This should be refactored into an iterative loop.
References
- Do not use recursion to ensure that all executions that should be bounded are bounded. (link)
src/state_transition/utils/load_state.zig (138)
The seed_validator view obtained via seed_view.get(i) is likely leaking. In this architecture, get() typically returns a heap-allocated TreeView that the caller is responsible for deinitializing. Since it is only used as a temporary reference to fetch root nodes in loadValidator, it should be deinited at the end of each iteration.
src/state_transition/utils/load_state.zig (72-77)
This function lacks assertions for its arguments and invariants. According to the style guide (Rule 51, 54), every function must have a minimum of two assertions to detect programmer errors and document pre/postconditions. This is a widespread issue in this new file (e.g., loadValidators, loadInactivityScores, loadState).
References
- Assert all function arguments and return values, pre/postconditions and invariants. The assertion density of the code must average a minimum of two assertions per function. (link)
src/state_transition/utils/load_state.zig (268-347)
The loadState function is 80 lines long, exceeding the hard limit of 70 lines per function defined in the style guide (Rule 108). Consider refactoring the fork-specific logic into helper functions to improve readability and maintainability.
References
- Restrict the length of function bodies to reduce the probability of poorly structured code. We enforce a hard limit of 70 lines per function. (link)
- add native `loadState` that mirrors TS implementation - add bindings - add benchmarks to bench naive + loadState difference part of #347
e398aa0 to
1cb0367
Compare
|
native part has been implemented in #165 |
|
@GrapeBaBa missed this PR, sorry!taking a look |
Adds the bench requested by spiral-ladder on PR #165, mirroring PR #351's loadState perf test shape: - BeaconStateView.loadOtherStateBench (NAPI): bench-only entry that runs st.loadState end-to-end and tears down the result, no CachedBeaconState wrap or EpochCache build, so native vs TS comparisons isolate the SSZ tree-rebuild cost. - index.d.ts: TS surface declaration. - bindings/perf/loadState.test.ts: 4 cases (native/TS x with/without prebuilt seedValidatorsBytes) using @chainsafe/benchmark on a mainnet era state.
## Motivation Reproduces on `main` and every binding-feature branch (#165, #346, #351): any process that holds a `BeaconStateView` at module scope panics during process exit: ``` thread N panic: incorrect alignment src/persistent_merkle_tree/Node.zig:387:9 in unref src/ssz/tree_view/chunks.zig:38:30 in deinit src/ssz/tree_view/container.zig:97:49 in clearChildrenDataCache src/state_transition/cache/state_cache.zig:102:26 in deinit ``` Process exits with code 134 (SIGABRT) instead of 0. Root cause: NAPI env cleanup hook (`napi_add_env_cleanup_hook`) fires before module-level JS holders of native objects are finalized. The previous `cleanup` callback unconditionally called `pool.state.deinit()`, freeing the `Node.Pool`'s `MultiArrayList` storage. Live `BeaconStateView` instances rooted by module-level `const` had not yet been finalized; when their finalizers eventually ran, the chained `pool.unref(self.root)` walked freed memory and panicked. Module-scope is the standard pattern for benchmark fixtures (`@chainsafe/benchmark` + perf tests). Without the fix, every such test exits non-zero even when the bench itself succeeds. ## Description Wrap `Node.Pool` in the existing `RefCount(T)` (`src/state_transition/utils/ref_count.zig`): - Module init holds `ref = 1`. - Each `BeaconStateView` that owns a `cached_state` holds another ref. - `pool.state.deinit()` releases the module ref but only actually destroys the pool when the count reaches zero — deferring destruction past every live view's finalizer. Order in `BeaconStateView.deinit`: 1. `cached_state.deinit()` — does all the `pool.unref(...)` calls 2. `pool_rc.unref()` — releases this view's pool ref; if last, real `pool.deinit` runs here
|
Closing this in favour of #165, apologies @GrapeBaBa did not see your previous work |
2 participants
loadStatethat mirrors TS implementationpart of #347