fix: operation and sanity spec tests v1.5.0 #75

twoeths · 2025-10-24T04:16:58Z

Motivation

fix operation and sanity spec tests

Description

merge chain config for spec test
ReusedEpochTransitionCache is (implicitly) inited and deinited once
fix various memory leak issues
track spec tests in a new CI job

2 remaining skipped tests are:

operation processBlockHeader test, will be fixed in fix: add Block functions #58
operation sync_aggregate, will fixed in fix: processSyncAggregate #73

part of #21

spiral-ladder

Looks amazing! Gave a preliminary look

test/spec/test_case.zig

src/config/beacon_config.zig

spiral-ladder · 2025-11-04T13:19:33Z

src/state_transition/block/process_attestation_phase0.zig

+    var cloned_aggregation_bits: s.BitListType(preset.MAX_VALIDATORS_PER_COMMITTEE).Type = undefined;
+    try s.BitListType(preset.MAX_VALIDATORS_PER_COMMITTEE).clone(allocator, &attestation.aggregation_bits, &cloned_aggregation_bits);
+    var appended: bool = false;
+    errdefer {
+        if (!appended) {
+            cloned_aggregation_bits.deinit(allocator);
+        }
+    }


Alternatively we can couple the deinit logic with the next 3 try's (which is up to where we're interested in deinit-ing), relying on a random appended bool seems prone to mistakes

src/state_transition/epoch/process_pending_consolidations.zig

spiral-ladder · 2025-11-04T13:31:27Z

src/state_transition/block/process_deposit.zig


-pub fn isValidDepositSignature(config: *const BeaconConfig, pubkey: BLSPubkey, withdrawal_credential: WithdrawalCredentials, amount: u64, deposit_signature: BLSSignature) !bool {
+/// refer to https://github.com/ethereum/consensus-specs/blob/v1.5.0/specs/electra/beacon-chain.md#new-is_valid_deposit_signature
+/// no need to return error union since consumer does not care about the reason of failure


i think it might be useful to know, even from a testing pov and even if the spec returns a bool, which portions of validation failed since we're uncompressing, validating the pubkey/sig and verifying against the message too. It seems like we're omitting useful error messages just to comply with the spec. Do we necessarily need it to return bool?

Yes, I think this is a similar thought to ChainSafe/lodestar#6330

Essentially, we may return an Error!void and only if absolutely necessary, wrap to convert to bool for purposes of spec compliance.

src/state_transition/block/process_consolidation_request.zig

spiral-ladder · 2025-11-04T13:35:51Z

src/state_transition/block/process_attestations.zig

        },
        .electra => |attestations_electra| {
-            try processAttestationsAltair(allocator, cached_state, ssz.electra.Attestation.Type, attestations_electra.items, verify_signatures);
+            try processAttestationsAltair(allocator, cached_state, attestations_electra.items, verify_signatures);


I think it's clearer and less prone to error if the reader of this code explicitly knows that we're passing in an attestation type of electra fork, rather than leaving it to zig to read the type internally within the process fn

spiral-ladder · 2025-11-04T13:37:36Z

src/state_transition/block/process_attestation_altair.zig

+    for (attestations) |*attestation| {
        const data = attestation.data;
-        try validateAttestation(AT, cached_state, attestation);
+        try validateAttestation(cached_state, attestation);


Same comment as the processAttestationAltair

spiral-ladder · 2025-11-04T13:44:57Z

src/state_transition/block/process_attestation_phase0.zig

 const CachedBeaconStateAllForks = @import("../cache/state_cache.zig").CachedBeaconStateAllForks;
 const BeaconStateAllForks = @import("../types/beacon_state.zig").BeaconStateAllForks;
 const ssz = @import("consensus_types");
+const s = @import("ssz");


I think in near future we should consider what we want to do with namespaces, right now we're using ssz for consensus types but then we run into situations like this where we can't use ssz the library.

Should we just use ct for consensus_types and ssz to mean ssz the library?

Prefer to reserve ssz for ssz library, and something else for consensus_types

ensi321

looks good. minor comments

ensi321 · 2025-11-05T22:22:26Z

src/config/beacon_config.zig


    pub fn getDomainForVoluntaryExit(self: *const BeaconConfig, state_slot: Slot, message_slot: ?Slot) ![32]u8 {
-        const domain = if (state_slot < self.chain.DENEB_FORK_EPOCH * preset.SLOTS_PER_EPOCH) {
+        const domain = if (state_slot / preset.SLOTS_PER_EPOCH < self.chain.DENEB_FORK_EPOCH) {


nit: probably better do @divFloor here

ensi321 · 2025-11-05T22:41:55Z

src/state_transition/block/process_attestation_phase0.zig

 const CachedBeaconStateAllForks = @import("../cache/state_cache.zig").CachedBeaconStateAllForks;
 const BeaconStateAllForks = @import("../types/beacon_state.zig").BeaconStateAllForks;
 const ssz = @import("consensus_types");
+const s = @import("ssz");


Prefer to reserve ssz for ssz library, and something else for consensus_types

ensi321 · 2025-11-05T23:05:56Z

src/state_transition/block/process_block.zig

+            const payload_withdrawals_root = switch (body) {
+                .regular => |b| blk: {
                    const actual_withdrawals = b.executionPayload().getWithdrawals();
-                    std.debug.assert(withdrawals_result.withdrawals.items.len == actual_withdrawals.items.len);


Why remove this assertion? We can potentially save one hashTreeRoot call

ensi321 · 2025-11-05T23:23:44Z

src/state_transition/block/process_withdrawals.zig

    }

    // Update the nextWithdrawalValidatorIndex
+    const nextWithdrawalValidatorIndex = state.nextWithdrawalValidatorIndex();


Suggested change

const nextWithdrawalValidatorIndex = state.nextWithdrawalValidatorIndex();

const new_withdrawal_validator_index = state.nextWithdrawalValidatorIndex();

twoeths added 27 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

twoeths changed the title ~~fix: operation spec tests v1.5.0~~ fix: operation and sanity spec tests v1.5.0 Oct 28, 2025

twoeths added 2 commits October 28, 2025 10:49

fix: load slots.yaml

eb1b956

fix: use validator ptr where suitable

e096549

twoeths added 11 commits October 29, 2025 15:04

fix: isValidIndexedAttestationIndices() check sort index

0ad403b

fix: computeSyncParticipantReward()

8eb7545

fix: proposer_weight_factor f64 const

f7e193c

fix: processSyncAggregate()

7c03a03

fix: processAttestationsAltair increase proposer reward

d67614b

fix: isExecutionEnabled()

8fcc8eb

fix: deinit withdrawals_result

8e6ddfa

feat: run spec tests on CI

bb93b56

fix: create test_case folder

f0260ff

chore: new spec test job

57a9fa7

fix: remove conditional statement

0560409

twoeths mentioned this pull request Oct 30, 2025

fix: add Block functions #58

Draft

twoeths added 8 commits October 30, 2025 15:39

fix: missing minimal config definition

91be677

fix: various fixes for minimal

ab4cc95

fix: ignore error if test/spec_test_case already exist

05c4fcd

fix: remove PENDING_PARTIAL_WITHDRAWALS_LIMIT from constant, use preset

5583474

fix: processEth1DataReset int test

b8c4dde

fix: processAttestation() minimal spec tests

5a80728

fix: active indices to compute proposers

df56c40

fix: run minimal spec tests on CI

cd09f44

twoeths marked this pull request as ready for review October 31, 2025 07:12

twoeths added 2 commits October 31, 2025 15:12

chore: loadPreState() and loadPostState()

e7f50b4

chore: lower case spec test file names

2d24e30

twoeths mentioned this pull request Nov 3, 2025

fix: epoch_processing spec test #77

Draft

spiral-ladder reviewed Nov 4, 2025

View reviewed changes

spiral-ladder added 3 commits November 5, 2025 12:33

chore: import FAR_FUTURE_EPOCH directly since it's the only import

90a86d0

chore: use @memcpy and copyForwards where possible

7f4574c

simplify loadPostState conditional flow

c19a5e3

ensi321 reviewed Nov 5, 2025

View reviewed changes

	const nextWithdrawalValidatorIndex = state.nextWithdrawalValidatorIndex();
	const new_withdrawal_validator_index = state.nextWithdrawalValidatorIndex();

fix: operation and sanity spec tests v1.5.0 #75

Are you sure you want to change the base?

fix: operation and sanity spec tests v1.5.0 #75

Uh oh!

Conversation

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

Uh oh!

Uh oh!

spiral-ladder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spiral-ladder Nov 4, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

ensi321 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

twoeths commented Oct 24, 2025 •

edited

Loading

spiral-ladder Nov 4, 2025 •

edited

Loading