feat: Implement latest signature spec test #458

GrapeBaBa · 2025-12-27T14:08:03Z

This PR is blocked by leanEthereum/leanSpec#243

Signed-off-by: Chen Kai <281165273grape@gmail.com>

Copilot

Pull request overview

This PR implements signature verification spec tests for the latest signature scheme, migrating from individual attestations to aggregated attestations with corresponding signature groups. The changes introduce SSZ-based signature parsing from JSON fixtures and add comprehensive debugging support for signature verification.

Key Changes:

Migrated from individual Attestation to AggregatedAttestation with aggregation bits
Restructured BlockSignatures to separate proposer signatures from attestation signature groups
Added Rust-based JSON-to-SSZ signature conversion utilities

Reviewed changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
`rust/hashsig-glue/src/lib.rs`	Added extensive debug logging, SSZ roundtrip validation, and JSON-to-SSZ signature conversion function
`rust/hashsig-glue/Cargo.toml`	Updated leansig dependency and added dev dependencies for Poseidon2 consistency testing
`pkgs/xmss/src/hashsig.zig`	Added FFI binding for JSON-to-SSZ signature conversion
`pkgs/types/src/block.zig`	Restructured attestations to use aggregation, refactored `BlockSignatures` structure, added aggregation logic
`pkgs/types/src/attestation.zig`	Added utility functions for aggregation bits manipulation and validator index extraction
`pkgs/types/src/state.zig`	Updated attestation processing to handle aggregated attestations and check for duplicates
`pkgs/state-transition/src/transition.zig`	Modified signature verification to validate per-validator signatures from aggregated groups
`pkgs/spectest/src/runner/verify_signatures_runner.zig`	New test runner for signature verification spec tests
Multiple test files	Updated to use `AggregatedAttestations` instead of `Attestations`

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

Copilot · 2025-12-27T14:10:32Z

rust/hashsig-glue/src/lib.rs

 use std::ffi::CStr;
 use std::os::raw::c_char;
 use std::ptr;
 use std::slice;


The import serde_json::Value is only used in newly added helper functions (json_u32, json_get, etc.) but is not used in the existing code or tests. While this is acceptable since these functions are part of the new hashsig_signature_ssz_from_json feature, consider grouping JSON-related imports together or adding a comment to clarify their purpose.

Suggested change

use std::slice;

use std::slice;

// Used by JSON helper functions (e.g., json_u32, json_get) for hashsig_signature_ssz_from_json.

Copilot · 2025-12-27T14:10:32Z

rust/hashsig-glue/src/lib.rs

+        // Debug: print first 36 bytes of signature
+        eprintln!("[hashsig_verify_ssz] pubkey_len={}, sig_len={}, epoch={}", pubkey_len, signature_len, epoch);
+        eprintln!("[hashsig_verify_ssz] sig first 36 bytes: {:02x?}", &sig_data[..36.min(sig_data.len())]);
+        eprintln!("[hashsig_verify_ssz] message: {:02x?}", message_array);


Extensive debug logging with eprintln! should be removed or placed behind a feature flag before merging to production. These debug statements (lines 395-518) will impact performance and clutter stderr in production environments.

Copilot · 2025-12-27T14:10:32Z

rust/hashsig-glue/src/lib.rs

+        let pk_roundtrip = pk.as_ssz_bytes();
+        if pk_roundtrip != pk_data {


Duplicate SSZ roundtrip check for pubkey. The same roundtrip validation is performed at lines 427-443 and again at lines 465-489. Consider removing the duplicate check or consolidating the logic.

Copilot · 2025-12-27T14:10:32Z

rust/hashsig-glue/src/lib.rs

+        let sig_roundtrip = sig.as_ssz_bytes();
+        if sig_roundtrip != sig_data {


Duplicate SSZ roundtrip check for signature. The same roundtrip validation is performed at lines 445-461 and again at lines 491-515. Consider removing the duplicate check or consolidating the logic.

Copilot · 2025-12-27T14:10:33Z

pkgs/types/src/block.zig


-        // Serialize attestations list
        var attestations_array = json.Array.init(allocator);
+        errdefer attestations_array.deinit();


Good defensive programming: the errdefer ensures cleanup on error paths. However, verify that all error paths after line 64 properly trigger this cleanup, especially in the loop at line 66-67 which appends to the array.

Copilot · 2025-12-27T14:10:33Z

pkgs/types/src/state.zig

+            const stored_target_root = try self.historical_block_hashes.get(target_slot);
+            const has_correct_source_root = std.mem.eql(u8, &attestation_data.source.root, &stored_source_root);
+            const has_correct_target_root = std.mem.eql(u8, &attestation_data.target.root, &stored_target_root);
+            const has_known_root = has_correct_source_root or has_correct_target_root;


The logic change from requiring both has_correct_source_root AND has_correct_target_root to requiring only has_known_root (OR condition) represents a significant behavioral change. The original logic at line 388 required !has_correct_source_root or !has_correct_target_root to skip, which meant both had to be correct to proceed. The new logic allows proceeding if either is correct. Ensure this relaxed validation aligns with the protocol specification.

Suggested change

const has_known_root = has_correct_source_root or has_correct_target_root;

const has_known_root = has_correct_source_root and has_correct_target_root;

Copilot · 2025-12-27T14:10:33Z

pkgs/spectest/src/runner/verify_signatures_runner.zig

+    // Debug: print signature info
+    const sig = &signed_block.signature.proposer_signature;
+    std.debug.print("fixture {s}: signature first 32 bytes: {x}\n", .{ ctx.fixture_label, sig[0..32].* });
+    std.debug.print("fixture {s}: signature last 32 bytes: {x}\n", .{ ctx.fixture_label, sig[sig.len - 32 ..].* });
+
+    // Debug: print proposer attestation data and computed message hash
+    const proposer_att = signed_block.message.proposer_attestation;
+    std.debug.print("fixture {s}: proposer_attestation.validator_id: {d}\n", .{ ctx.fixture_label, proposer_att.validator_id });


Extensive debug printing (lines 206-236) should be removed or placed behind a debug flag. These print statements will clutter test output and impact test execution performance.

Copilot · 2025-12-27T14:10:33Z

pkgs/spectest/src/runner/verify_signatures_runner.zig

+        if (att_sigs_obj.get("data")) |data_val| {
+            const arr = try expect.expectArrayValue(FixtureError, data_val, ctx, "signature.attestationSignatures.data");
+            for (arr.items) |_| {
+                // TODO: Parse actual attestation signatures if needed


The TODO comment indicates incomplete implementation. Non-empty attestation signatures are not supported, which limits test coverage. Consider tracking this limitation in an issue and linking it here.

Suggested change

// TODO: Parse actual attestation signatures if needed

// Limitation: non-empty attestation signatures are currently unsupported in spectest verification.

// Tracked in issue: https://github.com/<org>/<repo>/issues/<id>

Copilot · 2025-12-27T14:10:33Z

pkgs/spectest/src/runner/verify_signatures_runner.zig

+                for (arr.items) |_| {
+                    // TODO: Parse actual attestations if needed
+                    std.debug.print("fixture {s} case {s}: non-empty attestations not yet supported\n", .{ ctx.fixture_label, ctx.case_name });


Another TODO indicating incomplete implementation. Block body attestations are not parsed, limiting test coverage for blocks with attestations. Consider tracking this limitation.

Suggested change

for (arr.items) |_| {

// TODO: Parse actual attestations if needed

std.debug.print("fixture {s} case {s}: non-empty attestations not yet supported\n", .{ ctx.fixture_label, ctx.case_name });

if (arr.items.len != 0) {

// Block body attestations are currently not parsed by this runner.

// Treat fixtures with non-empty attestations as unsupported to make this

// limitation explicit and avoid silently ignoring attestation data.

Signed-off-by: Chen Kai <281165273grape@gmail.com>

GrapeBaBa added 3 commits December 22, 2025 12:49

feat: impl aggr sig spec

0ab84cb

Signed-off-by: Chen Kai <281165273grape@gmail.com>

feat: impl sig spec test

61efac2

Signed-off-by: Chen Kai <281165273grape@gmail.com>

feat: add signature spec test

52dee63

Signed-off-by: Chen Kai <281165273grape@gmail.com>

Copilot AI review requested due to automatic review settings December 27, 2025 14:08

Copilot AI reviewed Dec 27, 2025

View reviewed changes

GrapeBaBa added 2 commits January 3, 2026 20:59

chore(leanSpec): bump submodule to 90d6111

8a18469

fix: fix signature test

40df3ca

Signed-off-by: Chen Kai <281165273grape@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Implement latest signature spec test #458

feat: Implement latest signature spec test #458

Uh oh!

GrapeBaBa commented Dec 27, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Copilot AI Dec 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	use std::slice;
	use std::slice;
	// Used by JSON helper functions (e.g., json_u32, json_get) for hashsig_signature_ssz_from_json.

		let pk_roundtrip = pk.as_ssz_bytes();
		if pk_roundtrip != pk_data {

		let sig_roundtrip = sig.as_ssz_bytes();
		if sig_roundtrip != sig_data {

	const has_known_root = has_correct_source_root or has_correct_target_root;
	const has_known_root = has_correct_source_root and has_correct_target_root;

	// TODO: Parse actual attestation signatures if needed
	// Limitation: non-empty attestation signatures are currently unsupported in spectest verification.
	// Tracked in issue: https://github.com/<org>/<repo>/issues/<id>

-                for (arr.items) |_| {
-                    // TODO: Parse actual attestations if needed
-                    std.debug.print("fixture {s} case {s}: non-empty attestations not yet supported\n", .{ ctx.fixture_label, ctx.case_name });
+                if (arr.items.len != 0) {
+                    // Block body attestations are currently not parsed by this runner.
+                    // Treat fixtures with non-empty attestations as unsupported to make this
+                    // limitation explicit and avoid silently ignoring attestation data.

feat: Implement latest signature spec test #458

Are you sure you want to change the base?

feat: Implement latest signature spec test #458

Uh oh!

Conversation

GrapeBaBa commented Dec 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants