feat: impl rewards/merkle_proof/fork spec test by GrapeBaBa · Pull Request #81 · ChainSafe/lodestar-z

GrapeBaBa · 2025-11-12T10:05:58Z

Motivation

Implement rewards/merkel_proof/fork spec test in state transition zig.

Description

Add coverage for consensus rewards spec vectors, resolve latent bugs in rewards handling while enabling the suite.
Introduces merkle_proof runner and writer follow the current design .
Expands the spec test generator to support the new merkle_proof runner: registers the handler, ensures output directories exist, iterates nested suite/case folders, and emits generated cases via the new writer module.
Implement fork spec test based on transition upgrade functions

Copilot

Pull Request Overview

This PR implements spec test coverage for rewards, merkle_proof, and fork transitions in the state transition module. It introduces new test runners and writers following the existing test infrastructure pattern, adds necessary dependency imports, and fixes several latent bugs discovered during implementation.

Adds rewards spec test runner with delta accumulation logic for various reward/penalty types
Implements merkle proof verification test runner using persistent merkle trees
Adds fork transition test runner supporting upgrades from phase0 through electra
Refactors epoch computation logic to improve code reuse and correctness

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
zbuild.zon	Adds persistent_merkle_tree dependency for merkle proof tests
test/spec/writer/rewards.zig	New test writer for rewards spec tests with pyspec test suite support
test/spec/writer/merkle_proof.zig	New test writer for merkle proof spec tests with nested suite/case structure
test/spec/writer/fork.zig	New test writer for fork transition spec tests
test/spec/write_spec_tests.zig	Updates test generator to support new runners and nested test case directories
test/spec/runner_kind.zig	Adds new runner kinds and hasSuiteCase helper for merkle_proof
test/spec/runner/rewards.zig	Implements rewards test runner with delta loading and accumulation
test/spec/runner/merkle_proof.zig	Implements merkle proof test runner with YAML parsing and proof verification
test/spec/runner/fork.zig	Implements fork transition test runner with upgrade function dispatch
test/spec/root.zig	Registers new test modules in the test root
src/state_transition/utils/finality.zig	Refactors finality delay calculation to use computePreviousEpoch helper
src/state_transition/utils/epoch.zig	Extracts computePreviousEpoch helper function for code reuse
src/state_transition/slot/upgrade_state_to_altair.zig	Uses new computePreviousEpoch helper for consistency
src/state_transition/root.zig	Exports upgrade functions and getRewardsAndPenalties for test access
src/state_transition/cache/epoch_cache.zig	Uses computePreviousEpoch helper in shuffling lookup
src/ssz/type/byte_list.zig	Fixes memory leak by freeing allocated nodes array
build.zig	Adds persistent_merkle_tree and hex module imports for spec tests

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

Copilot · 2025-11-20T09:17:17Z

+    try writer.print(test_template, .{
+        @tagName(fork),
+        test_suite_name,
+        test_case_name,
+
+        @tagName(fork),
+        @tagName(handler),
+        test_suite_name,
+        test_case_name,
+
+        @tagName(fork),
+        @tagName(fork),
+    });


Format argument mismatch: The test template has 4 {s} placeholders on line 23, 4 on line 27, and 1 on line 32 (total 9 placeholders), but only 8 format arguments are provided. Lines 50-52 provide 3 args, lines 54-57 provide 4 args, and lines 59-60 provide 2 args. The test name on line 23 expects 4 arguments but only gets 3. The handler name appears to be missing. Line 51 should likely include @tagName(handler), after test_suite_name,.

Copilot · 2025-11-20T09:17:17Z

+            self.actual_rewards = epoch_cache.rewards;
+            self.actual_penalties = epoch_cache.penalties;


Memory safety issue: self.actual_rewards and self.actual_penalties are assigned to slices owned by epoch_cache (lines 176-177), but epoch_cache is destroyed via deinit() at scope exit (lines 169-172). This results in dangling pointers. When the test tries to read these slices at line 156, it will access freed memory. The rewards and penalties should be copied to owned memory, or the epoch_cache should not be freed until after the test completes.

These two fields were in the reusable cache which not be freed lines 169-172

Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>

wemeetagain

Looks good, only source change is preventing an underflow?

GrapeBaBa · 2025-11-21T04:42:13Z

Looks good, only source change is preventing an underflow?

you mean calc previous epoch?

Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>

GrapeBaBa · 2025-11-25T13:56:05Z

Looks good, only source change is preventing an underflow?

you mean calc previous epoch?

Not sure I get it, computePrevEpoch should be a helper function in the spec.

twoeths · 2025-11-29T07:58:55Z

Looks good, only source change is preventing an underflow?

it's more like a refactor, we have to handle underflow everywhere so it's more convenient to have a separate function for it

twoeths

I don't see we run merkle_proof at all on my generated code:

test "deneb merkle_proof single_merkle_proof BeaconBlockBody blob_kzg_commitment_merkle_proof__random_block_1" {
    const test_dir_name = try std.fs.path.join(allocator, &[_][]const u8{
        spec_test_options.spec_test_out_dir,
        spec_test_options.spec_test_version,
        @tagName(active_preset) ++ "/tests/" ++ @tagName(active_preset) ++ "/single_merkle_proof/merkle_proof/BeaconBlockBody/blob_kzg_commitment_merkle_proof__random_block_1/deneb",
    });
    defer allocator.free(test_dir_name);
    const test_dir = std.fs.cwd().openDir(test_dir_name, .{}) catch return error.SkipZigTest;

    try MerkleProof.TestCase(.deneb).execute(allocator, test_dir);
}

the test path seems to be incorrect
we need to improve spec test to error in that case instead of silently returning error.SkipZigTest

Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.

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

Copilot · 2025-11-30T08:58:49Z

+pub const Handler = enum {
+    single_merkle_proof,
+
+    pub fn suiteName(self: Handler) []const u8 {


The suiteName function should accept comptime self: Handler parameter for consistency with other handlers (rewards.zig) and because it's called in a comptime context in write_spec_tests.zig. Consider also adding the inline keyword like rewards.zig for better optimization.

Suggested change

pub fn suiteName(self: Handler) []const u8 {

pub inline fn suiteName(comptime self: Handler) []const u8 {

Copilot · 2025-11-30T08:58:50Z

+pub const Handler = enum {
+    fork,
+
+    pub fn suiteName(self: Handler) []const u8 {


The suiteName function should accept comptime self: Handler parameter for consistency with other handlers (rewards.zig) and because it's called in a comptime context in write_spec_tests.zig. Consider also adding the inline keyword like rewards.zig for better optimization.

Suggested change

pub fn suiteName(self: Handler) []const u8 {

pub inline fn suiteName(comptime self: Handler) []const u8 {

Copilot · 2025-11-30T08:58:50Z

+            return .{
+                .body = body,
+                .expect_proof = proof_data,
+                .actual_proof = undefined,


The actual_proof field is initialized to undefined but is unconditionally deinitialized in the deinit method (line 74). If an error occurs before actual_proof is properly assigned in the process method, deinit will attempt to free an undefined pointer, causing undefined behavior. Consider initializing actual_proof to a safe empty state (e.g., .{ .leaf = undefined, .leaf_gindex = Gindex.fromUint(1), .branch = &.{} }) or tracking whether it has been initialized with a separate boolean flag.

Suggested change

return .{

.body = body,

.expect_proof = proof_data,

.actual_proof = undefined,

// Initialize actual_proof to a safe empty state

var empty_branch = try allocator.alloc([66]u8, 0);

return .{

.body = body,

.expect_proof = proof_data,

.actual_proof = MerkleProof{

.leaf = undefined,

.leaf_gindex = Gindex.fromUint(1),

.branch = empty_branch,

},

Copilot · 2025-11-30T08:58:50Z

+pub fn writeTest(
+    writer: std.io.AnyWriter,
+    fork: ForkSeq,
+    handler: MerkleProof.Handler,


The handler parameter should be marked as comptime for consistency with other test writers (rewards.zig, fork.zig) and because it's passed from a comptime loop in write_spec_tests.zig. Change to comptime handler: MerkleProof.Handler,

Suggested change

handler: MerkleProof.Handler,

comptime handler: MerkleProof.Handler,

GrapeBaBa · 2025-11-30T09:01:33Z

I don't see we run merkle_proof at all on my generated code:

test "deneb merkle_proof single_merkle_proof BeaconBlockBody blob_kzg_commitment_merkle_proof__random_block_1" {
    const test_dir_name = try std.fs.path.join(allocator, &[_][]const u8{
        spec_test_options.spec_test_out_dir,
        spec_test_options.spec_test_version,
        @tagName(active_preset) ++ "/tests/" ++ @tagName(active_preset) ++ "/single_merkle_proof/merkle_proof/BeaconBlockBody/blob_kzg_commitment_merkle_proof__random_block_1/deneb",
    });
    defer allocator.free(test_dir_name);
    const test_dir = std.fs.cwd().openDir(test_dir_name, .{}) catch return error.SkipZigTest;

    try MerkleProof.TestCase(.deneb).execute(allocator, test_dir);
}

the test path seems to be incorrect we need to improve spec test to error in that case instead of silently returning error.SkipZigTest

Fixed the path issue, would like to check the error.SkipZigTest improvement in further PR.

twoeths

looks good to me

twoeths added 30 commits October 23, 2025 09:38

fix: implement computeIndexedAttestation() functions

58d5129

fix: clone aggregation_bits for phase0 PendingAttestation

1bd597a

fix: computeDomain()

0d59855

fix: handle processAttestationPhase0 failing partway

039886b

fix: equals api in isSlashableAttestationData()

242b0c9

fix: map isValidDepositSignature() exactly to the spec

847d265

fix: consume isValidDepositSignature() without try

29848c5

fix: getProposerSlashingSignatureSets signing root

d29c857

fix: processVoluntaryExit()

2be0eaa

feat: implement ChainConfig for spec tests

a493a2c

fix: free pubkeys in processAttestationsAltair()

991c0c8

fix: processAttestationsAltair for loop

11dc025

fix: bellatrix processExecutionPayload()

41dea08

fix: Operations test runner processExecutionPayload()

99f7725

fix: init correct root in processBlsToExecutionChange()

b36bd5c

fix: pass payload_withdrawals_root to processWithdrawals()

e8733fb

fix: update next_withdrawal_index in processWithdrawals()

ef300a6

fix: processWithdrawals() and getExpectedWithdrawals() misuse variables

1ceb59b

fix: use validator ptr in switchToCompoundingValidator()

a19884c

fix: cleanup resource when no post state

7312938

fix: write Operations test case without .valid

443faa8

fix: electra processWithdrawalRequest() validator ptr

dc05a5e

fix: Sanity test memory leak and remove .valid

3cd28bf

fix: only upgrade state if fork transition

a67e15f

fix: EffectiveBalanceIncrements usage

a65a7b0

fix: invalid state root due to processBlockHeader()

76551f3

fix: sanity BlocksTestCase deinit prestate inside blocks loop

aa304e7

fix: load slots.yaml

eb1b956

fix: use validator ptr where suitable

e096549

fix: BeaconState.rotateEpochParticipations()

3f6c9a5

GrapeBaBa requested review from a team, ensi321, spiral-ladder and twoeths November 20, 2025 09:14

Copilot finished reviewing on behalf of GrapeBaBa November 20, 2025 09:16

Copilot AI reviewed Nov 20, 2025

View reviewed changes

chore: sync zbuild

5536ea9

Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>

wemeetagain previously approved these changes Nov 20, 2025

View reviewed changes

fix: fix memory leak and writer wrong arg

9ff736e

Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>

GrapeBaBa dismissed wemeetagain’s stale review via 9ff736e November 21, 2025 04:46

twoeths reviewed Nov 29, 2025

View reviewed changes

Comment thread src/ssz/type/byte_list.zig

twoeths reviewed Nov 29, 2025

View reviewed changes

fix: fix merkle_proof spec test path issue and gindex bug

04829f3

Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>

GrapeBaBa requested review from Copilot, twoeths and wemeetagain November 30, 2025 08:54

Copilot started reviewing on behalf of GrapeBaBa November 30, 2025 08:54 View session

Copilot finished reviewing on behalf of GrapeBaBa November 30, 2025 08:58

Copilot AI reviewed Nov 30, 2025

View reviewed changes

twoeths approved these changes Dec 1, 2025

View reviewed changes

wemeetagain approved these changes Dec 1, 2025

View reviewed changes

wemeetagain merged commit b837ed3 into main Dec 1, 2025
11 checks passed

This was referenced Dec 8, 2025

Do not skip spec tests if cannot load them #117

Open

Implement fork spec test #93

Closed

matthewkeil mentioned this pull request Apr 15, 2026

Add grapebaba from Lodestar Team protocolguild/documentation#494

Merged

wemeetagain deleted the rewards_spec_test branch August 17, 2026 19:47

		self.actual_rewards = epoch_cache.rewards;
		self.actual_penalties = epoch_cache.penalties;

	pub fn suiteName(self: Handler) []const u8 {
	pub inline fn suiteName(comptime self: Handler) []const u8 {

	handler: MerkleProof.Handler,
	comptime handler: MerkleProof.Handler,

Uh oh!

Conversation

GrapeBaBa commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

GrapeBaBa Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wemeetagain left a comment

Choose a reason for hiding this comment

Uh oh!

GrapeBaBa commented Nov 21, 2025

Uh oh!

GrapeBaBa commented Nov 25, 2025

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

twoeths left a comment

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 Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

GrapeBaBa commented Nov 30, 2025

Uh oh!

twoeths left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GrapeBaBa commented Nov 12, 2025 •

edited

Loading

twoeths commented Nov 29, 2025 •

edited

Loading