Skip to content

refactor: optimize process_slashings #191

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 22 commits into from
Feb 2, 2026
Merged

refactor: optimize process_slashings #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
3ee0cc3
wip
wemeetagain Jan 20, 2026
4b2fd98
wip
wemeetagain Jan 20, 2026
1bd6cab
chore: remove unused imports
GrapeBaBa Jan 21, 2026
8c2b1dd
refactor: move beacon_state to fork types to prepare refactor
GrapeBaBa Jan 21, 2026
b80bba7
refactor: change some processXX to use comptime fork
GrapeBaBa Jan 21, 2026
1e37e44
refactor: continue change func using comptime fork
GrapeBaBa Jan 21, 2026
011ea81
chore: fork_types tweaks
wemeetagain Jan 21, 2026
064c413
chore: more processXX updates, top-level stateTransition update
wemeetagain Jan 21, 2026
a13ffb1
Merge main (int tests migration)
GrapeBaBa Jan 22, 2026
ab58507
refactor: continue use comptimee fork
GrapeBaBa Jan 22, 2026
1876bfe
refactor: continue to use comptime fork
GrapeBaBa Jan 22, 2026
600beaa
refactor: continue translate functions using comptime fork
GrapeBaBa Jan 22, 2026
a689e5f
chore: continue refactor
wemeetagain Jan 22, 2026
fbbb1a3
fix: fix state transition test
GrapeBaBa Jan 23, 2026
71b16d8
fix: fix spec test
GrapeBaBa Jan 23, 2026
a951492
fix: fix build compile error
GrapeBaBa Jan 23, 2026
8616e50
fix: fix bench
GrapeBaBa Jan 23, 2026
9f754ff
chore: use `ForkBeaconBlockBody` in stf util
GrapeBaBa Jan 25, 2026
6ba877c
refactor: optimize process_slashings
GrapeBaBa Jan 26, 2026
e0d4509
fix: fix review comments
GrapeBaBa Jan 31, 2026
b6d7f94
Merge branch 'main' into stf-slashings-penalties
GrapeBaBa Jan 31, 2026
2f8c62f
fix: fix merge error
GrapeBaBa Jan 31, 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
8 changes: 6 additions & 2 deletions bench/state_transition/process_epoch.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ fn ProcessRewardsAndPenaltiesBench(comptime fork: ForkSeq) type {
cloned.getEpochCache(),
cloned.state.castToFork(fork),
&cache,
null,
) catch unreachable;
}
};
Expand Down Expand Up @@ -122,12 +123,13 @@ fn ProcessSlashingsBench(comptime fork: ForkSeq) type {
}
var cache = EpochTransitionCache.init(allocator, cloned.config, cloned.getEpochCache(), cloned.state) catch unreachable;
defer cache.deinit();
state_transition.processSlashings(
_ = state_transition.processSlashings(
fork,
allocator,
cloned.getEpochCache(),
cloned.state.castToFork(fork),
&cache,
true,
) catch unreachable;
}
};
Expand Down Expand Up @@ -482,12 +484,13 @@ fn ProcessEpochSegmentedBench(comptime fork: ForkSeq) type {
recordSegment(.registry_updates, elapsedSince(registry_start));

const slashings_start = std.time.nanoTimestamp();
state_transition.processSlashings(
const slashing_penalties = state_transition.processSlashings(
fork,
allocator,
epoch_cache,
fork_state,
&cache,
false,
) catch unreachable;
recordSegment(.slashings, elapsedSince(slashings_start));

Expand All @@ -499,6 +502,7 @@ fn ProcessEpochSegmentedBench(comptime fork: ForkSeq) type {
epoch_cache,
fork_state,
&cache,
slashing_penalties,
) catch unreachable;
recordSegment(.rewards_and_penalties, elapsedSince(rewards_start));

Expand Down
54 changes: 36 additions & 18 deletions src/state_transition/cache/epoch_transition_cache.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,23 +68,36 @@ const ReusedEpochTransitionCache = struct {
current_epoch_participation: U8Array,
rewards: U64Array,
penalties: U64Array,

pub fn init(allocator: Allocator, validator_count: usize) !ReusedEpochTransitionCache {
return .{
.allocator = allocator,
.is_active_prev_epoch = try BoolArray.initCapacity(allocator, validator_count),
.is_active_current_epoch = try BoolArray.initCapacity(allocator, validator_count),
.is_active_next_epoch = try BoolArray.initCapacity(allocator, validator_count),
.proposer_indices = try UsizeArray.initCapacity(allocator, validator_count),
.inclusion_delays = try UsizeArray.initCapacity(allocator, validator_count),
.flags = try U8Array.initCapacity(allocator, validator_count),
.next_epoch_shuffling_active_validator_indices = try std.ArrayList(ValidatorIndex).initCapacity(allocator, validator_count),
.is_compounding_validator_arr = try BoolArray.initCapacity(allocator, validator_count),
.previous_epoch_participation = try U8Array.initCapacity(allocator, validator_count),
.current_epoch_participation = try U8Array.initCapacity(allocator, validator_count),
.rewards = try U64Array.initCapacity(allocator, validator_count),
.penalties = try U64Array.initCapacity(allocator, validator_count),
};
slashing_penalties: U64Array,

pub fn init(self: *ReusedEpochTransitionCache, allocator: Allocator, validator_count: usize) !void {
self.allocator = allocator;
self.is_active_prev_epoch = try BoolArray.initCapacity(allocator, validator_count);
errdefer self.is_active_prev_epoch.deinit();
self.is_active_current_epoch = try BoolArray.initCapacity(allocator, validator_count);
errdefer self.is_active_current_epoch.deinit();
self.is_active_next_epoch = try BoolArray.initCapacity(allocator, validator_count);
errdefer self.is_active_next_epoch.deinit();
self.proposer_indices = try UsizeArray.initCapacity(allocator, validator_count);
errdefer self.proposer_indices.deinit();
self.inclusion_delays = try UsizeArray.initCapacity(allocator, validator_count);
errdefer self.inclusion_delays.deinit();
self.flags = try U8Array.initCapacity(allocator, validator_count);
errdefer self.flags.deinit();
self.next_epoch_shuffling_active_validator_indices = try std.ArrayList(ValidatorIndex).initCapacity(allocator, validator_count);
errdefer self.next_epoch_shuffling_active_validator_indices.deinit();
self.is_compounding_validator_arr = try BoolArray.initCapacity(allocator, validator_count);
errdefer self.is_compounding_validator_arr.deinit();
self.previous_epoch_participation = try U8Array.initCapacity(allocator, validator_count);
errdefer self.previous_epoch_participation.deinit();
self.current_epoch_participation = try U8Array.initCapacity(allocator, validator_count);
errdefer self.current_epoch_participation.deinit();
self.rewards = try U64Array.initCapacity(allocator, validator_count);
errdefer self.rewards.deinit();
self.penalties = try U64Array.initCapacity(allocator, validator_count);
errdefer self.penalties.deinit();
self.slashing_penalties = try U64Array.initCapacity(allocator, validator_count);
errdefer self.slashing_penalties.deinit();
Comment on lines +73 to +100
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

The errdefer chain for cleanup is incomplete. If allocation of slashing_penalties succeeds (line 99) but then its errdefer (line 100) is triggered, all previously allocated fields from lines 75-97 will be properly cleaned up. However, there's no cleanup of self.allocator if an error occurs. While this is technically correct since allocator is just assigned (not allocated), the pattern is inconsistent. For clarity and to avoid potential future bugs, consider whether self.allocator should be set to undefined in the errdefer handlers, or add a comment explaining why it doesn't need cleanup.

Copilot uses AI. Check for mistakes.
}

pub fn resize(self: *ReusedEpochTransitionCache, validator_count: usize) !void {
Expand All @@ -100,6 +113,7 @@ const ReusedEpochTransitionCache = struct {
try self.current_epoch_participation.resize(validator_count);
try self.rewards.resize(validator_count);
try self.penalties.resize(validator_count);
try self.slashing_penalties.resize(validator_count);

@memset(self.is_active_prev_epoch.items, true);
@memset(self.is_active_current_epoch.items, true);
Expand All @@ -119,6 +133,8 @@ const ReusedEpochTransitionCache = struct {
self.current_epoch_participation.deinit();
self.rewards.deinit();
self.penalties.deinit();
self.slashing_penalties.deinit();
self.* = undefined;
}
};

Expand All @@ -138,7 +154,7 @@ fn getReusedEpochTransitionCache(allocator: Allocator, validator_count: usize) !
allocator.destroy(_reused_cache.?);
_reused_cache = null;
}
_reused_cache.?.* = try ReusedEpochTransitionCache.init(allocator, validator_count);
try _reused_cache.?.init(allocator, validator_count);
try _reused_cache.?.resize(validator_count);
return _reused_cache.?;
}
Expand Down Expand Up @@ -185,6 +201,7 @@ pub const EpochTransitionCache = struct {
is_compounding_validator_arr: *BoolArray,
rewards: []u64,
penalties: []u64,
slashing_penalties: []u64,
balances: ?U64Array,
next_shuffling_active_indices: []const ValidatorIndex,
// TODO: nextShufflingDecisionRoot may not needed as we don't use ShufflingCache
Expand Down Expand Up @@ -517,6 +534,7 @@ pub const EpochTransitionCache = struct {
.is_compounding_validator_arr = &reused_cache.is_compounding_validator_arr,
.rewards = reused_cache.rewards.items,
.penalties = reused_cache.penalties.items,
.slashing_penalties = reused_cache.slashing_penalties.items,
// Will be assigned in processRewardsAndPenalties()
.balances = null,
};
Expand Down
5 changes: 2 additions & 3 deletions src/state_transition/epoch/process_epoch.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,12 @@ pub fn processEpoch(
try processRegistryUpdates(fork, config, epoch_cache, state, cache);
try observeEpochTransitionStep(.{ .step = .process_registry_updates }, timer.read());

// TODO(bing): In lodestar-ts we accumulate slashing penalties and only update in processRewardsAndPenalties. Do the same?
timer = try Timer.start();
try processSlashings(fork, allocator, epoch_cache, state, cache);
const slashing_penalties = try processSlashings(fork, allocator, epoch_cache, state, cache, false);
try observeEpochTransitionStep(.{ .step = .process_slashings }, timer.read());

timer = try Timer.start();
try processRewardsAndPenalties(fork, allocator, config, epoch_cache, state, cache);
try processRewardsAndPenalties(fork, allocator, config, epoch_cache, state, cache, slashing_penalties);
try observeEpochTransitionStep(.{ .step = .process_rewards_and_penalties }, timer.read());

try processEth1DataReset(fork, state, cache);
Expand Down
14 changes: 11 additions & 3 deletions src/state_transition/epoch/process_rewards_and_penalties.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub fn processRewardsAndPenalties(
epoch_cache: *const EpochCache,
state: *BeaconState(fork),
cache: *const EpochTransitionCache,
slashing_penalties: ?[]const u64,
) !void {
// No rewards are applied at the end of `GENESIS_EPOCH` because rewards are for work done in the previous epoch
if (cache.current_epoch == GENESIS_EPOCH) {
Expand All @@ -30,9 +31,15 @@ pub fn processRewardsAndPenalties(
const balances = try state.balancesSlice(allocator);
defer allocator.free(balances);

for (rewards, penalties, balances) |reward, penalty, *balance| {
const result = balance.* + reward -| penalty;
balance.* = result;
if (slashing_penalties) |slashings| {
for (rewards, penalties, balances, 0..) |reward, penalty, *balance, i| {
const slashing: u64 = if (i < slashings.len) slashings[i] else 0;
balance.* = (try std.math.add(u64, balance.*, reward)) -| penalty -| slashing;
}
} else {
Comment on lines +34 to +39
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

The original code used wrapping addition (balance.* + reward) which would overflow without error if the sum exceeded u64 max. The new code uses std.math.add with try, which will return an error on overflow. While this is stricter and likely correct (rewards should never cause overflow in a valid system), it's a breaking change in error handling behavior. If rewards could legitimately cause overflow in some edge cases, this would cause the epoch transition to fail where it previously would have silently wrapped. Consider whether this change is intentional and document it if so.

Suggested change
if (slashing_penalties) |slashings| {
for (rewards, penalties, balances, 0..) |reward, penalty, *balance, i| {
const slashing: u64 = if (i < slashings.len) slashings[i] else 0;
balance.* = (try std.math.add(u64, balance.*, reward)) -| penalty -| slashing;
}
} else {
if (slashing_penalties) |slashings| {
// NOTE: The original implementation used wrapping addition (e.g. `balance.* + reward`),
// which would silently overflow. We now use checked addition via `std.math.add` and
// propagate any overflow as an error via `try`. Overflow of a validator balance due
// to rewards is considered a violation of protocol invariants, so we intentionally
// fail the epoch transition instead of wrapping on overflow.
for (rewards, penalties, balances, 0..) |reward, penalty, *balance, i| {
const slashing: u64 = if (i < slashings.len) slashings[i] else 0;
balance.* = (try std.math.add(u64, balance.*, reward)) -| penalty -| slashing;
}
} else {
// Same reasoning as above: treat any overflow in `balance + reward` as a hard error
// rather than allowing it to wrap.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@GrapeBaBa GrapeBaBa Jan 31, 2026

Choose a reason for hiding this comment

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

This is the correct behavior in the spec

for (rewards, penalties, balances) |reward, penalty, *balance| {
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

The loop at line 35 uses a numeric index but the loop at line 40 doesn't. This creates an inconsistency where the slashing_penalties slice might be shorter than the rewards/penalties/balances slices. If there are more validators than slashing_penalties entries, the check 'if (i < slashings.len)' will treat missing entries as 0, which could be correct behavior. However, this asymmetry creates confusion - consider using the same iteration pattern for both branches or adding a comment explaining why the lengths might differ.

Suggested change
for (rewards, penalties, balances) |reward, penalty, *balance| {
// Use the same indexed iteration pattern as the slashing branch to keep behavior and intent symmetric.
for (rewards, penalties, balances, 0..) |reward, penalty, *balance, _| {

Copilot uses AI. Check for mistakes.
balance.* = (try std.math.add(u64, balance.*, reward)) -| penalty;
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

Same behavioral change as line 37: the original code used wrapping addition, while the new code uses checked addition with std.math.add. This change should be applied consistently in both branches (with and without slashing_penalties), but the API contract change should be documented.

Copilot uses AI. Check for mistakes.
}
}

var balances_arraylist: std.ArrayListUnmanaged(u64) = .fromOwnedSlice(balances);
Expand Down Expand Up @@ -73,5 +80,6 @@ test "processRewardsAndPenalties - sanity" {
test_state.cached_state.getEpochCache(),
test_state.cached_state.state.castToFork(.electra),
test_state.epoch_transition_cache,
null,
);
}
23 changes: 18 additions & 5 deletions src/state_transition/epoch/process_slashings.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,23 @@ const PROPORTIONAL_SLASHING_MULTIPLIER_ALTAIR = preset.PROPORTIONAL_SLASHING_MUL
const PROPORTIONAL_SLASHING_MULTIPLIER_BELLATRIX = preset.PROPORTIONAL_SLASHING_MULTIPLIER_BELLATRIX;
const Node = @import("persistent_merkle_tree").Node;

/// TODO: consider returning number[] when we switch to TreeView
pub fn processSlashings(
comptime fork: ForkSeq,
allocator: std.mem.Allocator,
epoch_cache: *const EpochCache,
state: *BeaconState(fork),
cache: *const EpochTransitionCache,
) !void {
update_balance: bool,
) ![]const u64 {
const slashing_penalties = cache.slashing_penalties;
const empty_penalties = &[_]u64{};
if (!update_balance) {
@memset(slashing_penalties, 0);
}

// Return early if there no index to slash
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

Typo in comment: "there no index" should be "there are no indices" or "there is no index".

Suggested change
// Return early if there no index to slash
// Return early if there are no indices to slash

Copilot uses AI. Check for mistakes.
if (cache.indices_to_slash.items.len == 0) {
return;
return if (update_balance) empty_penalties else slashing_penalties;
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

When update_balance is false and there are no indices to slash, the function returns the slashing_penalties slice which has been zeroed at line 25. However, when update_balance is true with no indices to slash, it returns empty_penalties (an empty slice). This asymmetry could be confusing. Consider either: (1) always returning slashing_penalties when update_balance=false, even if empty, for consistency, or (2) documenting why the return values differ in this edge case.

Copilot uses AI. Check for mistakes.
}
const total_balance_by_increment = cache.total_active_stake_by_increment;
const proportional_slashing_multiplier: u64 =
Expand Down Expand Up @@ -51,8 +57,14 @@ pub fn processSlashings(
try penalties_by_effective_balance_increment.put(effective_balance_increment, p);
break :blk p;
};
try decreaseBalance(fork, state, index, penalty);
if (update_balance) {
try decreaseBalance(fork, state, index, penalty);
} else {
slashing_penalties[index] = penalty;
}
}

return if (update_balance) empty_penalties else slashing_penalties;
}

pub fn getTotalSlashingsByIncrement(
Expand Down Expand Up @@ -81,11 +93,12 @@ test "processSlashings - sanity" {
var test_state = try TestCachedBeaconState.init(allocator, &pool, 10_000);
defer test_state.deinit();

try processSlashings(
_ = try processSlashings(
.electra,
allocator,
test_state.cached_state.getEpochCache(),
test_state.cached_state.state.castToFork(.electra),
test_state.epoch_transition_cache,
true,
);
}
4 changes: 2 additions & 2 deletions test/spec/runner/epoch_processing.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ pub fn TestCase(comptime fork: ForkSeq, comptime epoch_process_fn: EpochProcessi
.participation_record_updates => try state_transition.processParticipationRecordUpdates(fork, fork_state),
.randao_mixes_reset => try state_transition.processRandaoMixesReset(fork, fork_state, &epoch_transition_cache),
.registry_updates => try state_transition.processRegistryUpdates(fork, config, epoch_cache, fork_state, &epoch_transition_cache),
.rewards_and_penalties => try state_transition.processRewardsAndPenalties(fork, allocator, config, epoch_cache, fork_state, &epoch_transition_cache),
.slashings => try state_transition.processSlashings(fork, allocator, epoch_cache, fork_state, &epoch_transition_cache),
.rewards_and_penalties => try state_transition.processRewardsAndPenalties(fork, allocator, config, epoch_cache, fork_state, &epoch_transition_cache, null),
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

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

In the test case for slashings, the function is called with update_balance=true, which means it will directly modify balances and return an empty slice. However, the return value is being discarded with '_'. While this is correct behavior for this test, it would be clearer to either remove the assignment or add a comment explaining that when update_balance=true, the function applies penalties directly and returns an empty slice. This helps maintain consistency with the new API contract.

Suggested change
.rewards_and_penalties => try state_transition.processRewardsAndPenalties(fork, allocator, config, epoch_cache, fork_state, &epoch_transition_cache, null),
.rewards_and_penalties => try state_transition.processRewardsAndPenalties(fork, allocator, config, epoch_cache, fork_state, &epoch_transition_cache, null),
// When update_balance = true, processSlashings applies penalties directly and
// returns an empty slice; the result is intentionally discarded here.

Copilot uses AI. Check for mistakes.
.slashings => _ = try state_transition.processSlashings(fork, allocator, epoch_cache, fork_state, &epoch_transition_cache, true),
.slashings_reset => try state_transition.processSlashingsReset(fork, epoch_cache, fork_state, &epoch_transition_cache),
.sync_committee_updates => try state_transition.processSyncCommitteeUpdates(fork, allocator, epoch_cache, fork_state),
.historical_summaries_update => try state_transition.processHistoricalSummariesUpdate(fork, fork_state, &epoch_transition_cache),
Expand Down