fix(st): add errdefer chain for epoch cache functions by GrapeBaBa · Pull Request #227 · ChainSafe/lodestar-z

GrapeBaBa · 2026-02-27T10:34:35Z

Motivation

Make createFromState release resources correctly when error thrown

Signed-off-by: Chen Kai <281165273grape@gmail.com>

gemini-code-assist · 2026-02-27T10:34:48Z

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 enhances the reliability of the EpochCache initialization process. By implementing a robust errdefer chain, it guarantees that all allocated resources are properly deallocated even when errors are encountered during the complex setup of the cache. This change improves the stability and prevents potential memory leaks in error scenarios.

Highlights

Improved Error Handling: Introduced a chain of errdefer statements within the createFromState function to ensure that resources are correctly released if an error occurs during the initialization of the EpochCache.
Refactored Resource Initialization: Extracted the initialization logic for EffectiveBalanceIncrementsRc, EpochShufflingRc, and SyncCommitteeCacheRc into dedicated private helper functions, each incorporating its own errdefer for robust cleanup.
Simplified createFromState Logic: The main createFromState function now calls these new helper functions, making its structure cleaner and more focused on orchestrating the cache creation process while delegating resource management to the helpers.

🧠 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.

Changelog

src/state_transition/cache/epoch_cache.zig
- Added initEffectiveBalanceIncrementsRc function to initialize EffectiveBalanceIncrementsRc with errdefer.
- Added initEpochShufflingRc function to initialize EpochShufflingRc with errdefer.
- Added initCurrentSyncCommitteeCacheRc function to initialize SyncCommitteeCacheRc for the current sync committee with errdefer.
- Added initNextSyncCommitteeCacheRc function to initialize SyncCommitteeCacheRc for the next sync committee with errdefer.
- Refactored createFromState to use the new init...Rc helper functions for effective_balance_increments, previous_shuffling, current_shuffling, next_shuffling, current_sync_committee_indexed, and next_sync_committee_indexed.
- Updated access to active_indices from current_shuffling to current_shuffling_rc.get().

Activity

No specific activity has been recorded for this pull request yet.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request refactors createFromState to improve resource management during error scenarios by introducing helper functions that encapsulate resource initialization and cleanup using errdefer. This is a solid improvement for safety and directly addresses the PR's motivation. My review includes suggestions to further enhance maintainability by reducing code duplication and to align the new code with the repository's style guide concerning assertions and code formatting for resource management.

gemini-code-assist · 2026-02-27T10:37:40Z

+        var effective_balance_increments = try effectiveBalanceIncrementsInit(allocator, validator_count);
+        errdefer effective_balance_increments.deinit();


The style guide recommends using newlines to visually group resource allocation and deallocation. Please add a blank line before the resource allocation on line 140 to adhere to this rule. This comment also applies to the other new helper functions in this file where resource allocation is the first statement.

References

Use newlines to group resource allocation and deallocation, i.e. before the resource allocation and after the corresponding defer statement, to make leaks easier to spot. ^(link)

gemini-code-assist · 2026-02-27T10:37:40Z

+    fn initEffectiveBalanceIncrementsRc(allocator: Allocator, validator_count: usize) !*EffectiveBalanceIncrementsRc {
+        var effective_balance_increments = try effectiveBalanceIncrementsInit(allocator, validator_count);
+        errdefer effective_balance_increments.deinit();
+
+        return try EffectiveBalanceIncrementsRc.init(allocator, effective_balance_increments);
+    }


The style guide requires asserting all function arguments. This function is missing assertions for its arguments, such as allocator and validator_count. Please add assertions to ensure correctness and adherence to the style guide. This also applies to the other new helper functions in this file.

References

Assert all function arguments and return values, pre/postconditions and invariants. A function must not operate blindly on data it has not checked. ^(link)

gemini-code-assist · 2026-02-27T10:37:40Z

+    fn initCurrentSyncCommitteeCacheRc(
+        allocator: Allocator,
+        state: *AnyBeaconState,
+        pubkey_to_index: *const PubkeyIndexMap,
+        skip_sync_committee_cache: bool,
+    ) !*SyncCommitteeCacheRc {
+        var sync_committee_cache = blk: {
+            if (skip_sync_committee_cache) break :blk SyncCommitteeCacheAllForks.initEmpty();
+            var sync_committee_view = try state.currentSyncCommittee();
+            var sync_committee: types.altair.SyncCommittee.Type = undefined;
+            try sync_committee_view.toValue(allocator, &sync_committee);
+            break :blk try SyncCommitteeCacheAllForks.initSyncCommittee(allocator, &sync_committee, pubkey_to_index);
+        };
+        errdefer sync_committee_cache.deinit();
+
+        return try SyncCommitteeCacheRc.init(allocator, sync_committee_cache);
+    }
+
+    fn initNextSyncCommitteeCacheRc(
+        allocator: Allocator,
+        state: *AnyBeaconState,
+        pubkey_to_index: *const PubkeyIndexMap,
+        skip_sync_committee_cache: bool,
+    ) !*SyncCommitteeCacheRc {
+        var sync_committee_cache = blk: {
+            if (skip_sync_committee_cache) break :blk SyncCommitteeCacheAllForks.initEmpty();
+            var sync_committee_view = try state.nextSyncCommittee();
+            var sync_committee: types.altair.SyncCommittee.Type = undefined;
+            try sync_committee_view.toValue(allocator, &sync_committee);
+            break :blk try SyncCommitteeCacheAllForks.initSyncCommittee(allocator, &sync_committee, pubkey_to_index);
+        };
+        errdefer sync_committee_cache.deinit();
+
+        return try SyncCommitteeCacheRc.init(allocator, sync_committee_cache);
+    }


The functions initCurrentSyncCommitteeCacheRc and initNextSyncCommitteeCacheRc are nearly identical, with the only difference being the call to state.currentSyncCommittee() versus state.nextSyncCommittee(). To improve maintainability and reduce code duplication, consider refactoring this logic into a single helper function. This function could accept a function pointer to either currentSyncCommittee or nextSyncCommittee to handle both cases.

Copilot

Pull request overview

This PR improves error-path resource cleanup in EpochCache.createFromState by wrapping several intermediate allocations in reference-counted helpers and adding errdefer-based release chaining, so partially-initialized caches don’t leak when an error is thrown.

Changes:

Added helper initializers to build EffectiveBalanceIncrementsRc, EpochShufflingRc, and SyncCommitteeCacheRc with consistent errdefer cleanup.
Updated createFromState to use the new RC helpers and errdefer ...release() for safe unwinding on failure.
Simplified the final EpochCache struct initialization by directly storing the RC pointers created earlier.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

GrapeBaBa · 2026-02-27T11:11:48Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ff33933ead

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Signed-off-by: Chen Kai <281165273grape@gmail.com>

spiral-ladder

This PR LGTM, but re-reading the implementation led me to a question in a comment

spiral-ladder · 2026-03-03T09:20:59Z

@@ -214,19 +279,31 @@ pub const EpochCache = struct {
        }

        // ownership of the active indices is transferred to EpochShuffling


Re-reading this comment and thinking about how memory ownership works here again, but isn't this inaccurate if we're copying the items of the ArrayList into the EpochShuffling? Shouldn't we just use toOwnedSlice() here to pass into initEpochShufflingRc and take ownership of the items?

Re-reading the code, I feel you are right

#229 fixing this here, but i think we can merge this current PR as is

there was a misleading comment saying that `EpochShuffling` took ownership of the `active_indices` even though the implementation copies. This PR fixes that

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-05T04:22:19Z

+        errdefer next_sync_committee_indexed.deinit();
+
+        const next_sync_committee_indexed_rc = try SyncCommitteeCacheRc.init(self.allocator, next_sync_committee_indexed);
+        errdefer next_sync_committee_indexed_rc.release();
+
+        var current_sync_committee_indexed = try SyncCommitteeCacheAllForks.initValidatorIndices(self.allocator, next_sync_committee_indices);
+        errdefer current_sync_committee_indexed.deinit();
+
+        const current_sync_committee_indexed_rc = try SyncCommitteeCacheRc.init(self.allocator, current_sync_committee_indexed);


In setSyncCommitteesIndexed, next_sync_committee_indexed and current_sync_committee_indexed each have errdefer ...deinit() set before being wrapped into SyncCommitteeCacheRc. If a later try fails after SyncCommitteeCacheRc.init succeeds (e.g., allocating the second cache), both the value deinit() and the RC release() will run on the same underlying allocation (the union copy still points at the same *SyncCommitteeCacheAltair), causing a double-free. Consider moving the value->RC wrapping into a small helper (similar to initCurrentSyncCommitteeCacheRc) so ownership transfer and cleanup are handled in one place, or explicitly disarm the earlier errdefer after successful RC init (e.g., by resetting the union to initEmpty() before any subsequent fallible operations).

Suggested change

errdefer next_sync_committee_indexed.deinit();

const next_sync_committee_indexed_rc = try SyncCommitteeCacheRc.init(self.allocator, next_sync_committee_indexed);

errdefer next_sync_committee_indexed_rc.release();

var current_sync_committee_indexed = try SyncCommitteeCacheAllForks.initValidatorIndices(self.allocator, next_sync_committee_indices);

errdefer current_sync_committee_indexed.deinit();

const current_sync_committee_indexed_rc = try SyncCommitteeCacheRc.init(self.allocator, current_sync_committee_indexed);

const next_sync_committee_indexed_rc = SyncCommitteeCacheRc.init(self.allocator, next_sync_committee_indexed) catch |e| {

next_sync_committee_indexed.deinit();

return e;

};

errdefer next_sync_committee_indexed_rc.release();

var current_sync_committee_indexed = try SyncCommitteeCacheAllForks.initValidatorIndices(self.allocator, next_sync_committee_indices);

const current_sync_committee_indexed_rc = SyncCommitteeCacheRc.init(self.allocator, current_sync_committee_indexed) catch |e| {

current_sync_committee_indexed.deinit();

return e;

};

fix(st): add errdefer chain for epoch cache init

ff33933

Signed-off-by: Chen Kai <281165273grape@gmail.com>

Copilot AI review requested due to automatic review settings February 27, 2026 10:34

GrapeBaBa requested a review from a team as a code owner February 27, 2026 10:34

Copilot started reviewing on behalf of GrapeBaBa February 27, 2026 10:34 View session

gemini-code-assist Bot reviewed Feb 27, 2026

View reviewed changes

Copilot AI reviewed Feb 27, 2026

View reviewed changes

Comment thread src/state_transition/cache/epoch_cache.zig Outdated

chatgpt-codex-connector Bot reviewed Feb 27, 2026

View reviewed changes

Comment thread src/state_transition/cache/epoch_cache.zig Outdated

GrapeBaBa added 2 commits February 27, 2026 20:10

fix: fix review comments

7d82411

Signed-off-by: Chen Kai <281165273grape@gmail.com>

fix: add errdefer chain in setSyncCommitteesIndexed

bd83835

Signed-off-by: Chen Kai <281165273grape@gmail.com>

GrapeBaBa changed the title ~~fix(st): add errdefer chain for epoch cache init~~ fix(st): add errdefer chain for epoch cache functions Feb 27, 2026

spiral-ladder previously approved these changes Mar 3, 2026

View reviewed changes

refactor(epoch-cache): ownership of active_indices (#229)

6794271

there was a misleading comment saying that `EpochShuffling` took ownership of the `active_indices` even though the implementation copies. This PR fixes that

wemeetagain dismissed spiral-ladder’s stale review via 6794271 March 4, 2026 15:58

GrapeBaBa requested review from Copilot and spiral-ladder March 5, 2026 04:15

Copilot started reviewing on behalf of GrapeBaBa March 5, 2026 04:15 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

wemeetagain approved these changes Mar 13, 2026

View reviewed changes

wemeetagain merged commit 19745d4 into main Mar 13, 2026
14 checks passed

matthewkeil mentioned this pull request Apr 15, 2026

Add grapebaba from Lodestar Team protocolguild/documentation#494

Merged

wemeetagain deleted the GrapeBaBa/errdefer-epochcache branch August 17, 2026 19:48

		var effective_balance_increments = try effectiveBalanceIncrementsInit(allocator, validator_count);
		errdefer effective_balance_increments.deinit();

		@@ -214,19 +279,31 @@ pub const EpochCache = struct {
		}

		// ownership of the active indices is transferred to EpochShuffling

Uh oh!

Conversation

GrapeBaBa commented Feb 27, 2026

Uh oh!

gemini-code-assist Bot commented Feb 27, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

GrapeBaBa commented Feb 27, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

spiral-ladder left a comment

Choose a reason for hiding this comment

Uh oh!

spiral-ladder Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

GrapeBaBa Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

spiral-ladder Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

spiral-ladder Mar 4, 2026 •

edited

Loading