fix: unit tests for process_epoch #28

twoeths · 2025-07-19T10:48:46Z

Description
unit tests all functions in process_epoch plus:

fix: pass by references for CachedBeaconStateAllForks
allocate EpochTransionCache on stack
use output params whenever possible, this avoids heap allocation in function body and consumers have to deinit() it.
reuse rewards and penalties from EpochTransisionCache
address comments in feat: naive state transition #17, mainly name BeaconState method append* instead of add*

github-actions · 2025-07-19T10:51:09Z

Performance Report

✔️ no performance regression detected

Full benchmark results

Benchmark suite	Current: `3098a0f`	Previous: null	Ratio
get values - 1000	337.00 ns/op
get values - naive - 1000	563.00 ns/op
set values - 1000	346.00 ns/op
set values - naive - 1000	649.00 ns/op
get values - 1000000	875.00 ns/op
get values - naive - 1000000	1.4810 us/op
set values - 1000000	858.00 ns/op
set values - naive - 1000000	2.1750 us/op
JS - computeSyncCommitteeIndices - 16384 indices	320.22 ms/op
Zig - computeSyncCommitteeIndices - 16384 indices	4.0183 ms/op
JS - computeSyncCommitteeIndices - 250000 indices	293.17 ms/op
Zig - computeSyncCommitteeIndices - 250000 indices	17.008 ms/op
JS - computeSyncCommitteeIndices - 1000000 indices	308.86 ms/op
Zig - computeSyncCommitteeIndices - 1000000 indices	31.639 ms/op
JS - unshuffleList - 16384 indices	927.78 us/op
Zig - unshuffleList - 16384 indices	586.14 us/op
JS - unshuffleList - 250000 indices	13.816 ms/op
Zig - unshuffleList - 250000 indices	8.7618 ms/op
JS - unshuffleList - 1000000 indices	53.787 ms/op
Zig - unshuffleList - 1000000 indices	34.958 ms/op

by benchmarkbot/action

…te/fix_process_epoch

src/state_transition/epoch/process_sync_committee_updates.zig

src/state_transition/epoch/process_epoch.zig

src/state_transition/epoch/process_participation_flag_updates.zig

spiral-ladder

Some general thoughts on the state of the repo:

un-OOP the repo?

I notice in many places we have getters and setters into everything; I think this can be simplified a lot more. For example, when operating on pending deposits, we have these following functions:

getPendingDeposit(...) *const PendingDeposit { ... }
getPendingDeposits(...) []PendingDeposit { ... }
getPendingDepositCount(...) usize { ... }
setPendingDeposit(...) void { ... }
appendPendingDeposit(...) !void { ... }
setPendingDeposits(...) void { ... }

When really, this could just be all done with a single function:

pub fn pendingDeposits() *std.ArrayListUnmanaged(PendingDeposit) { ... }

Then, to replicate the above behaviour (in order):

_ = state.pendingDeposits().items[i];
_ = state.pendingDeposits().items;
_ = state.pendingDeposits().items.len;
state.pendingDeposits().items[i] = new_pending_deposit;
try state.pendingDeposits().append(new_pending_deposit);
state.pendingDeposits().items = new_pending_deposits.items;

Arguing that this is 'idiomatic zig' is lazy; instead I'll argue that: (1) it is cleaner that we have only one way to access a certain field in state, and then manipulating it through stdlib methods; and (2) reduce the amount of boilerplate code we already have due to types and tagged unions.

Another related point is that I noticed in some functions, the getter function is doing extra work other than just simply returning; I think this inconsistency might be harmful.

types - do they belong here?

I think it's odd that consensus types are within ssz-z - though i suppose this should be raised in that repo instead. It almost feels like ssz-z should just contain the primitives and not have knowledge of the consensus types that consume it.

testing

Testing can be simplified in this PR, as mentioned in my comment

spiral-ladder · 2025-07-29T07:46:37Z

src/state_transition/cache/epoch_cache.zig


+    /// Utility method to return EpochShuffling so that consumers don't have to deal with ".get()" call
+    /// Consumers borrow value, so they must not either modify or deinit it.
+    pub fn getPreviousShuffling(self: *const EpochCache) *const EpochShuffling {


I almost feel that we have too many of such getters/setters, and that we should probably just do self.previous_shuffling.get() (for example) at the callsite.

this is not a regular getter like in BeaconState
consumer does not need to know about ReferenceCount here
will leave a TODO for it to revisit again to avoid redoing it multiple times

spiral-ladder · 2025-07-29T07:57:38Z

src/state_transition/cache/epoch_cache.zig

+    pub fn addPubkey(self: *EpochCache, allocator: Allocator, index: ValidatorIndex, pubkey: Publickey) !void {
+        try self.pubkey_to_index.set(pubkey[0..], index);
+        // this is deinit() by application
+        const pk_ptr = try allocator.create(BLSPubkey);
+        pk_ptr.* = try BLSPubkey.fromBytes(&pubkey);
+        self.index_to_pubkey.items[index] = pk_ptr;


nit: Consider instead a doc comment that tells the consumer where the allocation happens, since this function does two things (though I suppose we can come back for doc comments at a later time)

Suggested change

pub fn addPubkey(self: *EpochCache, allocator: Allocator, index: ValidatorIndex, pubkey: Publickey) !void {

try self.pubkey_to_index.set(pubkey[0..], index);

// this is deinit() by application

const pk_ptr = try allocator.create(BLSPubkey);

pk_ptr.* = try BLSPubkey.fromBytes(&pubkey);

self.index_to_pubkey.items[index] = pk_ptr;

/// Sets `index` at `PublicKey` within the index to pubkey map and allocates and puts a new `PublicKey` at `index` within the set of validators.

pub fn addPubkey(self: *EpochCache, allocator: Allocator, index: ValidatorIndex, pubkey: Publickey) !void {

try self.pubkey_to_index.set(pubkey[0..], index);

const pk_ptr = try allocator.create(BLSPubkey);

pk_ptr.* = try BLSPubkey.fromBytes(&pubkey);

self.index_to_pubkey.items[index] = pk_ptr;

added the comment

spiral-ladder · 2025-07-29T07:58:17Z

src/state_transition/cache/epoch_cache.zig

-            self.previous_shuffling.get()
-        else if (epoch == self.epoch) self.current_shuffling.get() else if (epoch == self.next_epoch)
-            self.next_shuffling.get()
+            self.getPreviousShuffling()
+        else if (epoch == self.epoch) self.getCurrentShuffling() else if (epoch == self.next_epoch)
+            self.getNextEpochShuffling()


I actually prefer the deleted lines because IMO we don't need an extra function for the same effect

added a TODO for it to resolve later

spiral-ladder · 2025-07-29T08:01:47Z

src/state_transition/cache/pubkey_cache.zig

 const ssz = @import("consensus_types");
 const PublicKey = blst.PublicKey;
-const PubkeyIndexMap = @import("../utils/pubkey_index_map.zig").PubkeyIndexMap;
+const ValidatorIndex = ssz.primitive.ValidatorIndex.Type;


nit: We currently use the same ValidatorIndex across a few different files. We should probably consolidate types in a later PR - this is not important atm

spiral-ladder · 2025-07-29T08:06:16Z

src/state_transition/epoch/get_attestation_deltas.zig

+    @memset(rewards, 0);
+    @memset(penalties, 0);


if we're setting them to 0 anyway, should we not just pass them in as such to begin with?

not sure I understand this, these are output params that will set later
do you mean do not set it and only override in the for loop below?

spiral-ladder · 2025-07-29T11:46:08Z

src/state_transition/types/beacon_state.zig

        switch (self.*) {
            .phase0 => @panic("inactivity_scores is not available in phase0"),
-            else => |state| state.inactivity_scores.append(score),
+            inline .altair, .bellatrix, .capella, .deneb, .electra => |state| try state.inactivity_scores.append(allocator, score),


Suggested change

inline .altair, .bellatrix, .capella, .deneb, .electra => |state| try state.inactivity_scores.append(allocator, score),

inline else => |state| try state.inactivity_scores.append(allocator, score),

We can also do inline else (unless it's a conscious decision to explicitly name all the forks)

spiral-ladder · 2025-07-29T12:18:57Z

src/state_transition/types/beacon_state.zig

                if (start_index >= state.pending_consolidations.items.len) return error.IndexOutOfBounds;
-                const new_array = try std.ArrayListUnmanaged(ssz.electra.PendingConsolidation).initCapacity(state.pending_consolidations.items.len - start_index);
-                try new_array.appendSlice(state.pending_consolidations.items[start_index..]);
+                var new_array = try std.ArrayListUnmanaged(ssz.electra.PendingConsolidation.Type).initCapacity(allocator, state.pending_consolidations.items.len - start_index);


Suggested change

var new_array = try std.ArrayListUnmanaged(ssz.electra.PendingConsolidation.Type).initCapacity(allocator, state.pending_consolidations.items.len - start_index);

var new_array = try std.ArrayListUnmanaged(PendingConsolidation).initCapacity(allocator, state.pending_consolidations.items.len - start_index);

Use already imported PendingConsolidation.

spiral-ladder · 2025-07-29T12:19:15Z

src/state_transition/types/beacon_state.zig

    }

-    pub fn setPendingConsolidations(self: *BeaconStateAllForks, consolidations: std.ArrayListUnmanaged(ssz.electra.PendingConsolidation)) void {
+    pub fn setPendingConsolidations(self: *BeaconStateAllForks, consolidations: std.ArrayListUnmanaged(ssz.electra.PendingConsolidation.Type)) void {


Suggested change

pub fn setPendingConsolidations(self: *BeaconStateAllForks, consolidations: std.ArrayListUnmanaged(ssz.electra.PendingConsolidation.Type)) void {

pub fn setPendingConsolidations(self: *BeaconStateAllForks, consolidations: std.ArrayListUnmanaged(PendingConsolidation)) void {

spiral-ladder · 2025-07-29T12:30:53Z

src/state_transition/utils/pubkey_index_map.zig

+        pub fn set(self: *@This(), key: []const u8, value: Val) !void {
+            if (key.len != PUBKEY_INDEX_MAP_KEY_SIZE) {
+                return error.InvalidKeyLen;
+            }
+            var fixed_key: Key = undefined;
+            @memcpy(&fixed_key, key);
+            try self.map.put(fixed_key, value);


Seems to me that, given that we know keys are of a fixed length PUBKEY_INDEX_MAP_KEY_SIZE, we can make this take a Key instead of []const u8:

Suggested change

pub fn set(self: *@This(), key: []const u8, value: Val) !void {

if (key.len != PUBKEY_INDEX_MAP_KEY_SIZE) {

return error.InvalidKeyLen;

}

var fixed_key: Key = undefined;

@memcpy(&fixed_key, key);

try self.map.put(fixed_key, value);

pub fn set(self: *@This(), key: Key, value: Val) !void {

try self.map.put(key, value);

}

Which has 2 benefits:

puts the burden of ensuring that key is of the right length on the consumer, getting rid of the if statements here

makes the memcpys here explicit (in the sense that the burden of copying is on the conumer side)

The same suggestions apply for the rest of the method here.

created an issue for it #34

spiral-ladder · 2025-07-29T14:03:07Z

test/int/epoch/process_epoch.zig

+const state_transition = @import("state_transition");
+const ReusedEpochTransitionCache = state_transition.ReusedEpochTransitionCache;
+const EpochTransitionCache = state_transition.EpochTransitionCache;
+const testProcessEpoch = @import("./process_epoch_fn.zig").getTestProcessFn(state_transition.processEpoch, false, false, false).testProcessEpochFn;


I don't particularly like this approach for tests because of a few reasons:

boolean blindness, and

'hiding' the parameters to the function via an import

splitting many small test functions in separate files rather than one file

tackling boolean blindness and params being hidden

To tackle 1, we can have a ProcessEpochTestOpt struct that looks roughly like this:

pub const ProcessEpochTestOpt = struct { no_alloc: bool = false, no_err_return: bool = false, no_void_return: bool = false, };

Then, we can do:

const testProcessEpoch = @import("./process_epoch_fn.zig").getTestProcessFn; test "processEth1DataReset - sanity" { try testProcessEpoch(state_transition.processEth1DataReset, .{ .no_alloc = true, .no_err_return = true, // leave no_void_return empty; defaults to false }); }

with this pattern we also avoid directly importing and using a function with empty params. IMO, it's better to be explicit than implicit for such small tests.

The 3rd point might be more contentious. I do see the benefit of splitting tests by the function name (it's easy to tell from fuzzyfinder/file explorer that a test for a function is missing if the corresponding test/int/epoch/my_fn_name.zig does not exist), but it's also nice to have all the tests in a single file. I'm undecided on this 3rd point.

wemeetagain · 2025-07-29T15:54:38Z

I think it's odd that consensus types are within ssz-z - though i suppose this should be raised in that repo instead. It almost feels like ssz-z should just contain the primitives and not have knowledge of the consensus types that consume it.

Feel free to copy the types here. I want to keep the types in the ssz repo for testing purposes for now (they exercise many types that aren't tested by the generic tests -- and its hard to manually build up a test suite with the same level of coverage), but we don't have to use them here.

wemeetagain · 2025-07-29T16:01:37Z

un-OOP the repo?

definitely agree with the points you raised

spiral-ladder

some more testing comments

spiral-ladder · 2025-07-30T02:59:23Z

test/int/epoch/process_epoch_fn.zig

The other files are named process_**.zig (which fits the spec as well), consider renaming this to something other than process... for accessibility (perhaps test_runner.zig)?

spiral-ladder · 2025-07-30T03:01:29Z

test/int/epoch/process_epoch_fn.zig

+const ReusedEpochTransitionCache = state_transition.ReusedEpochTransitionCache;
+const EpochTransitionCache = state_transition.EpochTransitionCache;
+
+pub fn getTestProcessFn(process_epoch_fn: anytype, no_alloc: bool, no_err_return: bool, no_void_return: bool) type {


Suggested change

pub fn getTestProcessFn(process_epoch_fn: anytype, no_alloc: bool, no_err_return: bool, no_void_return: bool) type {

const TestOpt = struct {

alloc: bool = false,

err_return: bool = false,

void_return: bool = false,

};

pub fn TestRunner(process_epoch_fn: anytype, opt: TestOpt) type {

Two things here:

IMO, let's not overload the terminology get - this is doing a bunch of logic to decide how and what to run. Also nit: PascalCase for TestRunner since it's a type.

Consider not using the negative terms no_alloc, no_void_return, no_err_return and instead use alloc, void_return, err_return. These are already boolean values - we can avoid double negatives here

twoeths · 2025-07-31T07:18:25Z

un-OOP the repo?

opened an issue for it at #33
I found it too big to be addressed in this PR

twoeths · 2025-07-31T07:36:28Z

Feel free to copy the types here. I want to keep the types in the ssz repo for testing purposes for now (they exercise many types that aren't tested by the generic tests -- and its hard to manually build up a test suite with the same level of coverage), but we don't have to use them here.

created an issue for it here #35

spiral-ladder · 2025-08-21T07:25:34Z

Merging this with #17 for convenience's sake, since separating out this and #17 doesn't affect reviewability (they're both big PRs anyway)

twoeths added 4 commits July 19, 2025 17:13

fix: stack allocation EpochTransitionCache

cfb5706

fix: pass by reference in process_epoch() functions

1de70f3

fix: rewards and penalties as output params

105d74b

fix: unit test process_justification_and_finalization

efc7b02

twoeths added 25 commits July 19, 2025 20:57

fix: reuse reards and penalties from EpochTransisionCache

2b30c77

fix: process_epoch_fn compilation errors

2dea67a

fix: PubkeyIndexMap to support both u32 and u64

58f3d31

fix: processPendingDeposits compilation errors

98ff0c4

fix: processPendingConsolidations compilation errors

7ce81bd

Merge remote-tracking branch 'origin/te/naive_state_transition' into …

e9b2bfa

…te/fix_process_epoch

fix: processEffectiveBalanceUpdates compilation errors

84bc0c3

fix: processSlashingsReset compilation errors

f493c47

fix: processHistoricalSummariesUpdate compilation errors

158f9f1

fix: processHistoricalRootsUpdate compilation errors

07e9373

fix: processParticipationRecordUpdates compilation errors

a4a527c

fix: processParticipationFlagUpdates compiltation errors

fb1caff

fix: processSyncCommitteeUpdates compilation errors

d341b9d

feat: implement and consume getters in EpochCache

10547ac

feat: testProcessRewardsAndPenalties()

9104966

chore: test processInactivityUpdates()

8f63229

chore: test processRegistryUpdates()

d40594f

chore: test processSlashings()

39d3b76

fix: missing try in processEpoch()

e6f9c66

fix: testProcessEpochFn to handle no-err functions

ab19561

fix: unit test processPendingDeposits()

b12e8fc

fix: unit test processPendingConsolidations()

7e35a17

chore: unit test processEffectiveBalanceUpdates()

7304364

chore: unit test processSlashingsReset()

57f61ab

chore: unit test processRandaoMixesReset()

7c10cfc

chore: processSyncCommitteeUpdates unit tests

19a4ae4

twoeths mentioned this pull request Jul 29, 2025

feat: naive state transition #17

Merged

fix: append* instead of add*

8accc2f

twoeths marked this pull request as ready for review July 29, 2025 01:51

graphite-app bot reviewed Jul 29, 2025

View reviewed changes

src/state_transition/epoch/process_sync_committee_updates.zig Outdated Show resolved Hide resolved

src/state_transition/epoch/process_epoch.zig Outdated Show resolved Hide resolved

fix: unit test the main processEpoch() function

7749313

graphite-app bot reviewed Jul 29, 2025

View reviewed changes

src/state_transition/epoch/process_participation_flag_updates.zig Outdated Show resolved Hide resolved

twoeths added 2 commits July 29, 2025 09:21

fix: correct method name rotateEpochParticipations()

2edd274

fix: processSyncCommitteeUpdates next_sync_committee_pubkeys_slices[i]

e6d90f7

spiral-ladder reviewed Jul 29, 2025

View reviewed changes

spiral-ladder reviewed Jul 30, 2025

View reviewed changes

twoeths mentioned this pull request Jul 31, 2025

un-OOP the repo #33

Open

This was referenced Jul 31, 2025

PubkeyIndexMap: consider using [48]u8 as key #34

Open

Redefine state transition type in this repo #35

Closed

twoeths added 2 commits July 31, 2025 15:03

fix: inline else in beacon_state.zig

2f45853

fix: use imported type in beacon_state.zig

c08cb25

twoeths mentioned this pull request Jul 31, 2025

Drop isPost* method #36

Open

twoeths added 7 commits July 31, 2025 15:19

chore: add comment for ReferenceCount.get() usage

c721e5f

chore: avoid optional where possible

f134d1b

chore: avoid double negative in process_rewards_and_penalties

143b22c

chore: avoid optional in process_slashings

5b38d1f

fix: expicit params for getTestProcessFn()

365f04d

fix: avoid negative terms in getTestProcessFn() util

5c506cf

fix: TestRunner and TestOpt

75a16c3

spiral-ladder merged commit 34b86ef into te/naive_state_transition Aug 21, 2025
13 checks passed

	inline .altair, .bellatrix, .capella, .deneb, .electra => \|state\| try state.inactivity_scores.append(allocator, score),
	inline else => \|state\| try state.inactivity_scores.append(allocator, score),

	var new_array = try std.ArrayListUnmanaged(ssz.electra.PendingConsolidation.Type).initCapacity(allocator, state.pending_consolidations.items.len - start_index);
	var new_array = try std.ArrayListUnmanaged(PendingConsolidation).initCapacity(allocator, state.pending_consolidations.items.len - start_index);

	pub fn setPendingConsolidations(self: *BeaconStateAllForks, consolidations: std.ArrayListUnmanaged(ssz.electra.PendingConsolidation.Type)) void {
	pub fn setPendingConsolidations(self: *BeaconStateAllForks, consolidations: std.ArrayListUnmanaged(PendingConsolidation)) void {

-pub fn getTestProcessFn(process_epoch_fn: anytype, no_alloc: bool, no_err_return: bool, no_void_return: bool) type {
+const TestOpt  = struct {
+    alloc: bool = false,
+    err_return: bool = false,
+    void_return: bool = false,
+};
+pub fn TestRunner(process_epoch_fn: anytype, opt: TestOpt) type {

fix: unit tests for process_epoch #28

fix: unit tests for process_epoch #28

Uh oh!

Conversation

twoeths commented Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spiral-ladder left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

un-OOP the repo?

types - do they belong here?

testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tackling boolean blindness and params being hidden

Uh oh!

wemeetagain commented Jul 29, 2025

Uh oh!

wemeetagain commented Jul 29, 2025

Uh oh!

spiral-ladder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twoeths commented Jul 31, 2025

Uh oh!

twoeths commented Jul 31, 2025

Uh oh!

spiral-ladder commented Aug 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

twoeths commented Jul 19, 2025 •

edited

Loading

github-actions bot commented Jul 19, 2025 •

edited

Loading

spiral-ladder left a comment •

edited

Loading