Skip to content

fix(st): add errdefer chain for epoch cache functions #227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wemeetagain merged 4 commits into from
Mar 13, 2026
Merged

fix(st): add errdefer chain for epoch cache functions #227

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff33933
fix(st): add errdefer chain for epoch cache init
GrapeBaBa Feb 27, 2026
7d82411
fix: fix review comments
GrapeBaBa Feb 27, 2026
bd83835
fix: add errdefer chain in setSyncCommitteesIndexed
GrapeBaBa Feb 27, 2026
6794271
refactor(epoch-cache): ownership of `active_indices` (#229)
spiral-ladder Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 111 additions & 38 deletions src/state_transition/cache/epoch_cache.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,66 @@ pub const EpochCache = struct {

epoch: Epoch,

fn initEffectiveBalanceIncrementsRc(allocator: Allocator, validator_count: usize) !*EffectiveBalanceIncrementsRc {
var effective_balance_increments = try effectiveBalanceIncrementsInit(allocator, validator_count);
errdefer effective_balance_increments.deinit();
Comment on lines +140 to +141

@gemini-code-assist gemini-code-assist Bot Feb 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. 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)


return try EffectiveBalanceIncrementsRc.init(allocator, effective_balance_increments);
}
Comment on lines +139 to +144

@gemini-code-assist gemini-code-assist Bot Feb 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. Assert all function arguments and return values, pre/postconditions and invariants. A function must not operate blindly on data it has not checked. (link)


fn initEpochShufflingRc(
allocator: Allocator,
state: *AnyBeaconState,
active_indices: []const ValidatorIndex,
epoch: Epoch,
) !*EpochShufflingRc {
const owned_active_indices = try allocator.alloc(ValidatorIndex, active_indices.len);
errdefer allocator.free(owned_active_indices);
Comment thread
GrapeBaBa marked this conversation as resolved.
Resolved

std.mem.copyForwards(ValidatorIndex, owned_active_indices, active_indices);

const epoch_shuffling = try computeEpochShuffling(allocator, state, owned_active_indices, epoch);
Comment thread
GrapeBaBa marked this conversation as resolved.
Resolved
errdefer epoch_shuffling.deinit();

return try EpochShufflingRc.init(allocator, epoch_shuffling);
}

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);
}
Comment on lines +161 to +195

@gemini-code-assist gemini-code-assist Bot Feb 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.


pub fn createFromState(allocator: Allocator, state: *AnyBeaconState, immutable_data: EpochCacheImmutableData, option: ?EpochCacheOpts) !*EpochCache {
const config = immutable_data.config;
const pubkey_to_index = immutable_data.pubkey_to_index;
Expand All @@ -162,7 +222,10 @@ pub const EpochCache = struct {
try syncPubkeys(validators, pubkey_to_index, index_to_pubkey);
}

const effective_balance_increments = try effectiveBalanceIncrementsInit(allocator, validator_count);
const effective_balance_increments_rc = try initEffectiveBalanceIncrementsRc(allocator, validator_count);
errdefer effective_balance_increments_rc.release();

const effective_balance_increments = effective_balance_increments_rc.get();
const state_fork_seq = state.forkSeq();
const total_slashings_by_increment = switch (state_fork_seq) {
inline else => |f| try getTotalSlashingsByIncrement(f, state.castToFork(f)),
Expand Down Expand Up @@ -214,19 +277,31 @@ pub const EpochCache = struct {
}

// ownership of the active indices is transferred to EpochShuffling

@spiral-ladder spiral-ladder Mar 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@GrapeBaBa GrapeBaBa Mar 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reading the code, I feel you are right

@spiral-ladder spiral-ladder Mar 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const previous_active_indices = try allocator.alloc(ValidatorIndex, previous_active_indices_array_list.items.len);
std.mem.copyForwards(ValidatorIndex, previous_active_indices, previous_active_indices_array_list.items);
const previous_shuffling: *EpochShuffling = try computeEpochShuffling(allocator, state, previous_active_indices, previous_epoch);
const previous_shuffling_rc = try initEpochShufflingRc(
allocator,
state,
previous_active_indices_array_list.items,
previous_epoch,
);
errdefer previous_shuffling_rc.release();

// ownership of the active indices is transferred to EpochShuffling
const current_active_indices = try allocator.alloc(ValidatorIndex, current_active_indices_array_list.items.len);
std.mem.copyForwards(ValidatorIndex, current_active_indices, current_active_indices_array_list.items);
const current_shuffling: *EpochShuffling = try computeEpochShuffling(allocator, state, current_active_indices, current_epoch);
const current_shuffling_rc = try initEpochShufflingRc(
allocator,
state,
current_active_indices_array_list.items,
current_epoch,
);
errdefer current_shuffling_rc.release();

// ownership of the active indices is transferred to EpochShuffling
const next_active_indices = try allocator.alloc(ValidatorIndex, next_active_indices_array_list.items.len);
std.mem.copyForwards(ValidatorIndex, next_active_indices, next_active_indices_array_list.items);
const next_shuffling: *EpochShuffling = try computeEpochShuffling(allocator, state, next_active_indices, next_epoch);
const next_shuffling_rc = try initEpochShufflingRc(
allocator,
state,
next_active_indices_array_list.items,
next_epoch,
);
errdefer next_shuffling_rc.release();

// TODO: implement proposerLookahead in fulu
const fork_seq = config.forkSeqAtEpoch(current_epoch);
Expand All @@ -236,14 +311,14 @@ pub const EpochCache = struct {
}
var proposers = [_]ValidatorIndex{0} ** preset.SLOTS_PER_EPOCH;
var next_proposers: ?[preset.SLOTS_PER_EPOCH]ValidatorIndex = null;
if (current_shuffling.active_indices.len > 0) {
if (current_shuffling_rc.get().active_indices.len > 0) {
switch (fork_seq) {
inline else => |f| try computeProposers(
f,
allocator,
current_proposer_seed,
current_epoch,
current_shuffling.active_indices,
current_shuffling_rc.get().active_indices,
effective_balance_increments,
&proposers,
),
Expand Down Expand Up @@ -272,25 +347,21 @@ pub const EpochCache = struct {
const sync_proposer_reward: u64 = @intFromFloat(std.math.floor(sync_participant_reward_f64 * proposer_weight_factor));
const base_reward_pre_increment = computeBaseRewardPerIncrement(total_active_balance_increments);
const skip_sync_committee_cache = if (option) |opt| opt.skip_sync_committee_cache else !after_altair_fork;
var current_sync_committee_indexed = blk: {
if (skip_sync_committee_cache) break :blk SyncCommitteeCacheAllForks.initEmpty();
var current_sc_view = try state.currentSyncCommittee();
var current_sc: types.altair.SyncCommittee.Type = undefined;
try current_sc_view.toValue(allocator, &current_sc);
break :blk try SyncCommitteeCacheAllForks.initSyncCommittee(allocator, &current_sc, pubkey_to_index);
};
var next_sync_committee_indexed = blk: {
if (skip_sync_committee_cache) break :blk SyncCommitteeCacheAllForks.initEmpty();
var next_sc_view = try state.nextSyncCommittee();
var next_sc: types.altair.SyncCommittee.Type = undefined;
try next_sc_view.toValue(allocator, &next_sc);
break :blk try SyncCommitteeCacheAllForks.initSyncCommittee(allocator, &next_sc, pubkey_to_index);
};
const current_sync_committee_indexed = try initCurrentSyncCommitteeCacheRc(
allocator,
state,
pubkey_to_index,
skip_sync_committee_cache,
);
errdefer current_sync_committee_indexed.release();

errdefer {
current_sync_committee_indexed.deinit();
next_sync_committee_indexed.deinit();
}
const next_sync_committee_indexed = try initNextSyncCommitteeCacheRc(
allocator,
state,
pubkey_to_index,
skip_sync_committee_cache,
);
errdefer next_sync_committee_indexed.release();

// Precompute churnLimit for efficient initiateValidatorExit() during block proposing MUST be recompute everytime the
// active validator indices set changes in size. Validators change active status only when:
Expand All @@ -306,8 +377,9 @@ pub const EpochCache = struct {
// activeIndices size is dependent on the state epoch. The epoch is advanced after running the epoch transition, and
// the first block of the epoch process_block() call. So churnLimit must be computed at the end of the before epoch
// transition and the result is valid until the end of the next epoch transition
const churn_limit = getChurnLimit(config, current_shuffling.active_indices.len);
const activation_churn_limit = getActivationChurnLimit(config, fork_seq, current_shuffling.active_indices.len);
const churn_limit = getChurnLimit(config, current_shuffling_rc.get().active_indices.len);
const activation_churn_limit = getActivationChurnLimit(config, fork_seq, current_shuffling_rc.get().active_indices.len);

if (exit_queue_churn >= churn_limit) {
exit_queue_epoch += 1;
exit_queue_churn = 0;
Expand Down Expand Up @@ -351,10 +423,10 @@ pub const EpochCache = struct {
.previous_decision_root = previous_decision_root,
.current_decision_root = current_decision_root,
.next_decision_root = next_decision_root,
.previous_shuffling = try EpochShufflingRc.init(allocator, previous_shuffling),
.current_shuffling = try EpochShufflingRc.init(allocator, current_shuffling),
.next_shuffling = try EpochShufflingRc.init(allocator, next_shuffling),
.effective_balance_increments = try EffectiveBalanceIncrementsRc.init(allocator, effective_balance_increments),
.previous_shuffling = previous_shuffling_rc,
.current_shuffling = current_shuffling_rc,
.next_shuffling = next_shuffling_rc,
.effective_balance_increments = effective_balance_increments_rc,
.total_slashings_by_increment = total_slashings_by_increment,
.sync_participant_reward = sync_participant_reward,
.sync_proposer_reward = sync_proposer_reward,
Expand All @@ -366,8 +438,8 @@ pub const EpochCache = struct {
.exit_queue_churn = exit_queue_churn,
.current_target_unslashed_balance_increments = current_target_unslashed_balance_increments,
.previous_target_unslashed_balance_increments = previous_target_unslashed_balance_increments,
.current_sync_committee_indexed = try SyncCommitteeCacheRc.init(allocator, current_sync_committee_indexed),
.next_sync_committee_indexed = try SyncCommitteeCacheRc.init(allocator, next_sync_committee_indexed),
.current_sync_committee_indexed = current_sync_committee_indexed,
.next_sync_committee_indexed = next_sync_committee_indexed,
.sync_period = computeSyncPeriodAtEpoch(current_epoch),
.epoch = current_epoch,
};
Expand Down Expand Up @@ -433,6 +505,7 @@ pub const EpochCache = struct {

const epoch_cache_ptr = try allocator.create(EpochCache);
errdefer allocator.destroy(epoch_cache_ptr);

epoch_cache_ptr.* = epoch_cache;
return epoch_cache_ptr;
}
Expand Down