feat(spectest): add SSZ roundtrip runner with comptime type dispatch#715
feat(spectest): add SSZ roundtrip runner with comptime type dispatch#715
Conversation
There was a problem hiding this comment.
Pull request overview
Implements the remaining LeanSpec fixture runners and signature-scheme dispatch needed to get the full leanSpec suite passing end-to-end (SSZ roundtrips, signature verification, and state transition/fork choice), including test-vs-prod XMSS scheme support through Rust FFI + Zig bindings.
Changes:
- Adds Rust/Zig plumbing to verify SSZ-encoded signatures under either the test or production XMSS scheme (and updates signature sizes accordingly).
- Introduces new spectest runners for SSZ roundtrip fixtures and verify_signatures fixtures, including test-mode type mirrors to handle 424-byte test signatures.
- Extends spectest fixture-kind plumbing and build wiring to include the new runners and link required Rust glue.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/hashsig-glue/src/lib.rs | Adds scheme-aware SSZ signature verification and JSON→SSZ signature conversion helpers; updates RNG usage. |
| rust/hashsig-glue/Cargo.toml | Pins leansig to a specific rev; adds rand_chacha + serde_json; adjusts rand version. |
| rust/Cargo.lock | Introduces additional git-sourced dependency entries (notably a second leansig source). |
| pkgs/xmss/src/lib.zig | Re-exports scheme IDs, signature sizes, and new helper(s) from hashsig. |
| pkgs/xmss/src/hashsig.zig | Adds HashSigScheme dispatch, signature-size helpers, and FFI for scheme-aware verify + JSON→SSZ conversion. |
| pkgs/types/src/utils.zig | Updates SIGSIZE to match production signature SSZ length. |
| pkgs/state-transition/src/transition.zig | Plumbs scheme selection into signature verification and adds a scheme-aware attestation verify function. |
| pkgs/state-transition/src/lib.zig | Re-exports verifySingleAttestationWithScheme. |
| pkgs/spectest/src/runner/verify_signatures_runner.zig | New runner to validate proposer + attestation signatures from fixtures under test/prod schemes. |
| pkgs/spectest/src/runner/ssz_runner.zig | New runner for SSZ roundtrip fixtures with comptime type dispatch and test-mode mirrors for signature-sized types. |
| pkgs/spectest/src/lib.zig | Exposes the new runners. |
| pkgs/spectest/src/json_expect.zig | Adds an expectArrayField helper used by fixture parsing. |
| pkgs/spectest/src/generator.zig | Updates generated empty index scaffolding for new fixture kinds. |
| pkgs/spectest/src/fixture_kind.zig | Adds ssz and verify_signatures kinds and includes them in all. |
| build.zig | Wires @zeam/xmss and Rust glue linkage into the spectest build target. |
Comments suppressed due to low confidence (1)
rust/hashsig-glue/src/lib.rs:112
PrivateKey::generatenow accepts anyR: Rng, which allows non-cryptographic RNGs to be used for key generation. To avoid accidental insecure usage, keep the stronger bound (e.g.,R: Rng + CryptoRng/RngCore + CryptoRng) matching whatXmssScheme::key_genexpects.
pub fn generate<R: Rng>(
rng: &mut R,
activation_epoch: u32,
num_active_epochs: u32,
) -> (PublicKey, Self) {
let (pk, sk) =
XmssScheme::key_gen(rng, activation_epoch as usize, num_active_epochs as usize);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| signature_scheme: xmss.HashSigScheme, | ||
| ) !void { | ||
| const signature_ssz_len = xmss.signatureSszLenForScheme(signature_scheme); | ||
| if (signature_ssz_len > signatureBytes.len) { |
There was a problem hiding this comment.
signatureBytes is a *const types.SIGBYTES (pointer to a fixed-size array), so signatureBytes.len is not valid in Zig and will not compile. Compare against the array length via signatureBytes.*.len (or types.SIGSIZE) instead.
| if (signature_ssz_len > signatureBytes.len) { | |
| if (signature_ssz_len > signatureBytes.*.len) { |
| const pubkey = xmss.PublicKey.fromBytes(pubkey_bytes) catch return false; | ||
| pub_keys.append(allocator, pubkey.handle) catch return false; | ||
| pk_wrappers.append(allocator, pubkey) catch return false; |
There was a problem hiding this comment.
This runner builds xmss.PublicKey wrappers and appends them into pub_keys / pk_wrappers, but on allocation failure the locally created pubkey isn't deinitialized before returning. Use errdefer pubkey.deinit() (and/or structure the append sequence) so failures don't leak FFI handles.
| const pubkey = xmss.PublicKey.fromBytes(pubkey_bytes) catch return false; | |
| pub_keys.append(allocator, pubkey.handle) catch return false; | |
| pk_wrappers.append(allocator, pubkey) catch return false; | |
| var pubkey: ?xmss.PublicKey = xmss.PublicKey.fromBytes(pubkey_bytes) catch return false; | |
| errdefer if (pubkey) |*wrapper| wrapper.deinit(); | |
| pk_wrappers.append(allocator, pubkey.?) catch return false; | |
| pubkey = null; | |
| pub_keys.append(allocator, pk_wrappers.items[pk_wrappers.items.len - 1].handle) catch { | |
| var wrapper = pk_wrappers.pop(); | |
| wrapper.deinit(); | |
| return false; | |
| }; |
| return false; | ||
| }; | ||
|
|
||
| const epoch: u32 = @intCast(block.slot); |
There was a problem hiding this comment.
block.slot is a u64 but is cast to u32 with @intCast, which will truncate on large slots and can make signature verification incorrect. Use a checked cast (e.g., std.math.cast(u32, block.slot)) and treat out-of-range values as an invalid fixture.
| const epoch: u32 = @intCast(block.slot); | |
| const epoch = std.math.cast(u32, block.slot) orelse { | |
| std.debug.print("fixture {s} case {s}: invalid block slot\n", .{ ctx.fixture_label, ctx.case_name }); | |
| return false; | |
| }; |
Add SSZ fixture runner that tests serialization roundtrips for all 18 consensus types. Uses test-mode type mirrors with 424-byte signatures for leanEnv=test fixtures, enabling all 34 SSZ tests to pass without requiring hashsig infrastructure changes.
078b369 to
273dad7
Compare
Update leanSpec to 1177800 which includes the latest spectest fixture vectors for SSZ, verify_signatures, and other test categories.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const fixture_kind = @import("../fixture_kind.zig"); | ||
| const skip = @import("../skip.zig"); | ||
|
|
||
| const Fork = forks.Fork; | ||
| const FixtureKind = fixture_kind.FixtureKind; | ||
| const types = @import("@zeam/types"); | ||
| const params = @import("@zeam/params"); |
There was a problem hiding this comment.
This file declares several top-level constants that are never referenced (fixture_kind/FixtureKind, skip, and params). Zig treats unused top-level declarations as compile errors, so this will fail to build. Remove the unused imports/aliases (or wire them into the runner if they’re intended to be used).
| const fixture_kind = @import("../fixture_kind.zig"); | |
| const skip = @import("../skip.zig"); | |
| const Fork = forks.Fork; | |
| const FixtureKind = fixture_kind.FixtureKind; | |
| const types = @import("@zeam/types"); | |
| const params = @import("@zeam/params"); | |
| const Fork = forks.Fork; | |
| const types = @import("@zeam/types"); |
… type The updated leanSpec generates new fork_choice fixtures with gossipAggregatedAttestation steps. Mark this step type as unsupported (graceful skip) alongside the existing attestation step type, so these tests don't fail with InvalidFixture.
… fork choice steps Add full attestation step support to the fork choice runner instead of skipping them as unsupported. This resolves 15 previously failing fork choice spec tests. Changes: - Add processAttestationStep with validator bounds check, attestation data validation, and signature error detection - Add processGossipAggregatedAttestationStep with aggregation proof handling and store integration - Add validateAttestationDataForGossip implementing leanSpec validation rules (block existence, slot relationships, future slot tolerance) - Align forkchoice.zig future attestation check with leanSpec (allow +1 slot tolerance) - Consolidate KeyManager error variants into ValidatorKeyNotFound - Move MAX_ATTESTATIONS_DATA to constants module
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkgs/node/src/chain.zig
Outdated
| if (att_data_set.count() > constants.MAX_ATTESTATIONS_DATA) { | ||
| self.logger.err( | ||
| "block contains {d} distinct AttestationData entries (max {d}) for block root=0x{x}", | ||
| .{ att_data_set.count(), self.config.spec.max_attestations_data, &freshFcBlock.blockRoot }, | ||
| .{ att_data_set.count(), constants.MAX_ATTESTATIONS_DATA, &freshFcBlock.blockRoot }, | ||
| ); | ||
| return BlockProcessingError.TooManyAttestationData; | ||
| return BlockProcessingError.InvalidSignatureGroups; |
There was a problem hiding this comment.
constants.MAX_ATTESTATIONS_DATA hard-codes the max distinct AttestationData per block to 8, but the rest of the codebase (e.g. ChainSpec.max_attestations_data defaults to 16 and forkchoice block-building respects that) still treats this as configurable. This introduces inconsistent limits between block production/selection and block processing, potentially rejecting otherwise-valid blocks. Prefer using self.config.spec.max_attestations_data here (or update the spec/defaults and forkchoice to use the same source of truth).
| // Each unique AttestationData must appear at most once per block. | ||
| { | ||
| var att_data_set = std.AutoHashMap(types.AttestationData, void).init(self.allocator); | ||
| defer att_data_set.deinit(); | ||
| for (aggregated_attestations) |agg_att| { | ||
| const result = try att_data_set.getOrPut(agg_att.data); | ||
| if (result.found_existing) { | ||
| self.logger.err( | ||
| "block contains duplicate AttestationData entries for block root=0x{x}", | ||
| .{&freshFcBlock.blockRoot}, | ||
| ); | ||
| return BlockProcessingError.DuplicateAttestationData; | ||
| return BlockProcessingError.InvalidSignatureGroups; | ||
| } |
There was a problem hiding this comment.
This changes two distinct block-processing conditions (duplicate AttestationData and too many distinct AttestationData) to return InvalidSignatureGroups. That error name no longer matches the failure mode and makes it harder for callers/tests to distinguish data-shape issues from signature-group problems. Consider reintroducing a dedicated error (or renaming/generalizing the error) so the returned error communicates the actual validation failure.
| // assert data.slot <= current_slot + Slot(1) | ||
| // In production, chain.validateAttestationData enforces the stricter gossip | ||
| // check (data.slot <= current_slot) upstream. | ||
| if (attestation_slot > self.fcStore.slot_clock.timeSlots.load(.monotonic) + 1) { |
There was a problem hiding this comment.
This file already imports constants.zig and defines MAX_FUTURE_SLOT_TOLERANCE = 1, but the future-slot check uses a literal + 1. Use constants.MAX_FUTURE_SLOT_TOLERANCE (and consider @addWithOverflow if Slot can reach its max) to keep the tolerance consistent and avoid duplicating the value.
| if (attestation_slot > self.fcStore.slot_clock.timeSlots.load(.monotonic) + 1) { | |
| const current_slot = self.fcStore.slot_clock.timeSlots.load(.monotonic); | |
| const max_future_slot_result = @addWithOverflow(current_slot, constants.MAX_FUTURE_SLOT_TOLERANCE); | |
| const max_future_slot = if (max_future_slot_result[1] != 0) | |
| std.math.maxInt(@TypeOf(current_slot)) | |
| else | |
| max_future_slot_result[0]; | |
| if (attestation_slot > max_future_slot) { |
| "block contains duplicate AttestationData entries for block root=0x{x}", | ||
| .{&freshFcBlock.blockRoot}, | ||
| ); | ||
| return BlockProcessingError.DuplicateAttestationData; | ||
| return BlockProcessingError.InvalidSignatureGroups; | ||
| } | ||
| } | ||
| if (att_data_set.count() > self.config.spec.max_attestations_data) { | ||
| if (att_data_set.count() > constants.MAX_ATTESTATIONS_DATA) { | ||
| self.logger.err( |
There was a problem hiding this comment.
PR description says there are “No hashsig/xmss/state-transition changes” and that the SSZ runner is self-contained, but this diff also changes production node behavior (pkgs/node/src/chain.zig, pkgs/node/src/forkchoice.zig) and key-manager error semantics (pkgs/key-manager/src/lib.zig). Please update the PR description/scope (or split into separate PRs) so reviewers know these runtime changes are intentional and reviewed.
…st commit These changes (KeyManager error consolidation, MAX_ATTESTATIONS_DATA constant move, chain.zig error renaming) are out of scope for this PR and caused CI failures due to missing max_attestations_data struct field in test initializers.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Parse participants aggregation bits. | ||
| var aggregation_bits = try parseAggregationBitsValue(ctx.allocator, participants_value, fixture_path, case_name, step_index, "proof.participants"); | ||
| errdefer aggregation_bits.deinit(); |
There was a problem hiding this comment.
aggregation_bits is initialized and used to derive indices / copy into proof, but it's never deinitialized on the success path (only errdefer is present). Even though the runner uses an arena per fixture, this still causes unnecessary per-step allocations to accumulate and makes the code easy to misuse if the allocator changes.
Add a defer aggregation_bits.deinit(); after parseAggregationBitsValue(...) succeeds (and keep the existing errdefer inside parseAggregationBitsValue).
| errdefer aggregation_bits.deinit(); | |
| defer aggregation_bits.deinit(); |
Summary
inline forover a type map to generate specialized deserialize/serialize code per typeleanEnv=testfixturesChanges
pkgs/spectest/src/runner/ssz_runner.zig— New SSZ roundtrip runnerpkgs/spectest/src/fixture_kind.zig— Addsszvariantpkgs/spectest/src/generator.zig— Addsszto empty indexpkgs/spectest/src/lib.zig— Exportssz_runnerTest plan
zig build spectest:generatediscovers 34 SSZ fixtureszig build spectestpasses all 73 tests (exit code 0)