feat: support spec test using test sig scheme #508
Conversation
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>
Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>
Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>
There was a problem hiding this comment.
Pull request overview
This PR adds support for testing signature verification using a test signature scheme alongside the existing production scheme. The test scheme uses smaller parameters (8-bit lifetime vs 32-bit) resulting in smaller signature sizes (424 bytes vs 3112 bytes), enabling more efficient testing.
Changes:
- Introduced dual signature scheme support (test and production) with corresponding SSZ length constants
- Added JSON-to-SSZ signature conversion functionality for spec test fixtures
- Implemented new
verify_signaturesspec test runner with scheme-aware verification
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/hashsig-glue/src/lib.rs | Added HashSigSchemeTest type alias and scheme-aware verification with JSON-to-SSZ conversion |
| rust/hashsig-glue/Cargo.toml | Updated leansig dependency revision |
| pkgs/xmss/src/lib.zig | Exported new scheme-related constants and functions |
| pkgs/xmss/src/hashsig.zig | Added HashSigScheme enum and scheme parameter to verification functions |
| pkgs/state-transition/src/transition.zig | Refactored signature verification to support scheme selection |
| pkgs/state-transition/src/lib.zig | Exported scheme-aware verification functions |
| pkgs/spectest/src/runner/verify_signatures_runner.zig | New spec test runner for signature verification with test/prod scheme support |
| pkgs/spectest/src/runner/state_transition_runner.zig | Updated to use new state transition options |
| pkgs/spectest/src/runner/fork_choice_runner.zig | Updated to use new state transition options |
| pkgs/spectest/src/json_expect.zig | Added expectArrayField helper function |
| pkgs/spectest/src/fixture_kind.zig | Registered verify_signatures as a new fixture kind |
| pkgs/key-manager/src/lib.zig | Added TestKeyManager and XmssTestConfig for test key management |
| leanSpec | Updated submodule to revision with test scheme support |
| build.zig | Added xmss module dependency for spectests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[repr(u8)] | ||
| enum HashSigSchemeId { | ||
| Test = 0, | ||
| Prod = 1, |
There was a problem hiding this comment.
The enum variant Prod should be Production for clarity and consistency with the documentation comments that use the full term 'production'.
| Prod = 1, | |
| Production = 1, |
| fn write_u32_le(dst: &mut [u8], offset: usize, v: u32) -> Option<()> { | ||
| let end = offset.checked_add(4)?; | ||
| dst.get_mut(offset..end)?.copy_from_slice(&v.to_le_bytes()); | ||
| Some(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
The function name write_u32_le uses 'le' abbreviation. Consider renaming to write_u32_little_endian for better clarity.
| fn write_u32_le(dst: &mut [u8], offset: usize, v: u32) -> Option<()> { | |
| let end = offset.checked_add(4)?; | |
| dst.get_mut(offset..end)?.copy_from_slice(&v.to_le_bytes()); | |
| Some(()) | |
| } | |
| fn write_u32_little_endian(dst: &mut [u8], offset: usize, value: u32) -> Option<()> { | |
| let end = offset.checked_add(4)?; | |
| dst.get_mut(offset..end)?.copy_from_slice(&value.to_le_bytes()); | |
| Some(()) | |
| } | |
| fn write_u32_le(dst: &mut [u8], offset: usize, v: u32) -> Option<()> { | |
| write_u32_little_endian(dst, offset, v) | |
| } |
Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>
Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let expected_len = match scheme_id { | ||
| x if x == HashSigSchemeId::Test as u8 => TEST_SIGNATURE_SSZ_LEN, | ||
| x if x == HashSigSchemeId::Prod as u8 => PROD_SIGNATURE_SSZ_LEN, | ||
| _ => return -1, | ||
| }; |
There was a problem hiding this comment.
The match arms use pattern guards unnecessarily. Since scheme_id is a u8, you can match directly on the enum discriminants without binding to x.
| let attempt: Result<bool, ()> = match scheme_id { | ||
| x if x == HashSigSchemeId::Test as u8 => { | ||
| verify_with_scheme::<HashSigSchemeTest>(pk_data, sig_data, epoch, message_array) | ||
| } | ||
| x if x == HashSigSchemeId::Prod as u8 => { | ||
| verify_with_scheme::<HashSigSchemeProd>(pk_data, sig_data, epoch, message_array) | ||
| } | ||
| _ => return -1, | ||
| }; |
There was a problem hiding this comment.
The match arms use pattern guards unnecessarily. Since scheme_id is a u8, you can match directly on the enum discriminants without binding to x.
| let sibling_size: usize = 8 * 4; | ||
| let hash_size: usize = 8 * 4; | ||
| let path_fixed_part: usize = 4; | ||
| let sig_fixed_part: usize = 36; |
There was a problem hiding this comment.
Magic numbers 8, 4, and 36 should be defined as named constants with explanatory comments indicating their meaning in the SSZ encoding structure.
Signed-off-by: grapebaba <grapebaba@grapebabadeMacBook-Pro.local>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
pkgs/key-manager/src/lib.zig:1
- This function lacks documentation. Add a doc comment explaining that it parses a hex-encoded public key string (with optional '0x' prefix) into a Bytes52 array.
const std = @import("std");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[repr(u8)] | ||
| enum HashSigSchemeId { | ||
| Test = 0, | ||
| Prod = 1, | ||
| } |
There was a problem hiding this comment.
The HashSigSchemeId enum lacks documentation. Add a doc comment explaining its purpose as a C-compatible FFI enum for distinguishing between test and production signature schemes.
| fn write_u32_le(dst: &mut [u8], offset: usize, v: u32) -> Option<()> { | ||
| let end = offset.checked_add(4)?; | ||
| dst.get_mut(offset..end)?.copy_from_slice(&v.to_le_bytes()); | ||
| Some(()) | ||
| } |
There was a problem hiding this comment.
This helper function lacks documentation. Add a doc comment explaining that it writes a u32 value in little-endian byte order to the specified offset in the destination buffer, returning None if the offset is out of bounds.
| fn parseKeyIndex(file_name: []const u8) !usize { | ||
| if (!std.mem.endsWith(u8, file_name, ".json")) { | ||
| return FixtureKeyError.InvalidKeyIndex; | ||
| } | ||
| const stem = file_name[0 .. file_name.len - ".json".len]; | ||
| if (stem.len == 0) { | ||
| return FixtureKeyError.InvalidKeyIndex; | ||
| } | ||
| return std.fmt.parseInt(usize, stem, 10) catch FixtureKeyError.InvalidKeyIndex; | ||
| } |
There was a problem hiding this comment.
This function lacks documentation. Add a doc comment explaining that it extracts a validator index from a JSON filename (e.g., '5.json' returns 5).
* feat: add API endpoint versioning Version API endpoints with /lean/v0/ prefix to align with leanSpec PR #323: - /health → /lean/v0/health - /lean/states/finalized → /lean/v0/states/finalized Updates endpoint routes, documentation, tests, and CLI help messages. * docs: update endpoint references to use versioned API paths Update documentation to reflect versioned API endpoints: - /health → /lean/v0/health - /lean/states/finalized → /lean/v0/states/finalized Updated files: - pkgs/api/README.md: API documentation and examples - resources/checkpoint-sync.md: Checkpoint sync guide - pkgs/cli/test/fixtures/README.md: Local devnet setup guide * feat: add /lean/v0/states/justified endpoint Implement the justified checkpoint endpoint to match leanSpec PR #323: - Add getJustifiedCheckpoint method to BeamChain - Add /lean/v0/states/justified route handler in api_server - Add handleJustifiedCheckpoint method to return JSON with checkpoint info - Update API documentation with new endpoint The endpoint returns the latest justified checkpoint as JSON with slot and root fields. Returns 503 if chain is not initialized. All tests passing (104/104). * test: add integration test for /lean/v0/states/justified endpoint Add comprehensive integration test that verifies: - /lean/v0/health endpoint returns 200 OK with healthy status - /lean/v0/states/justified endpoint returns JSON with checkpoint data - Response contains required fields: "root" and "slot" - JSON structure is valid and parseable - Slot value is a valid integer Test spins up a beam simulation node, waits for blocks to be processed, and validates both endpoints are functioning correctly. * fix: improve justified endpoint test robustness for CI - Increase initial wait time from 5s to 10s for chain initialization - Add retry logic if chain is not initialized (503 response) - Add detailed response logging for debugging - Handle 503 Service Unavailable gracefully with additional 10s wait - Ensures test passes in CI environment where initialization may be slower Fixes CI test failure where chain wasn't fully initialized when endpoint was first queried. * test: remove integration test for new API endpoints Remove the integration test for /lean/v0/health and /lean/v0/states/justified endpoints as it was causing timing issues in CI. The endpoints are still functional and can be tested manually. * update: leanSpec version --------- Co-authored-by: anshalshukla <shukla.anshal85@gmail.com>
…#518) Update API endpoint from /lean/v0/states/justified to /lean/v0/checkpoints/justified to match leanSpec PR #325. This makes the API semantically correct: - /lean/v0/states/finalized returns a State (SSZ binary) - /lean/v0/checkpoints/justified returns a Checkpoint (JSON) Changes: - Update endpoint route in api_server.zig - Update all documentation references in README.md - Update handler documentation comments
* fix: snappy frame decoding boundaries for req/resp * fix: cleanup * fix: remove unnecessary reference --------- Co-authored-by: Chen Kai <281165273grape@gmail.com>
* feat: add OCI labels for git commit and branch to docker images * fix: add branch label to auto-release * fix: add git commit label to auto-release
* chore: point to latest ssz commit * feat: add Sha256 ssz hashTreeRoot wrapper * refactor: use the updated hashTreeRoot wrapper --------- Co-authored-by: g11tech <gajinder@zeam.in>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 44 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
rust/hashsig-glue/src/lib.rs:1
- The function name
write_u32_lesuggests it writes a u32 in little-endian format, but there's no explicit documentation. Consider adding a doc comment to clarify the endianness and the function's purpose.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Signature structure constants from leansig | ||
| // path: 8 siblings, each is 8 u32 = 256 bytes | ||
| // rho: 7 u32 = 28 bytes | ||
| // hashes: 4 elements, each is 8 u32 = 128 bytes | ||
| // Total fixed size in fixture: 412 bytes, but actual SIGBYTES is 3112 bytes | ||
| // The remaining bytes are likely padding or additional OTS data |
There was a problem hiding this comment.
These hardcoded constants in comments (8 siblings, 7 u32, 4 elements) are specific to one signature scheme but don't clarify which one. Since the PR adds test vs. production scheme support, document which scheme these values correspond to, or remove if no longer applicable.
| // Signature structure constants from leansig | |
| // path: 8 siblings, each is 8 u32 = 256 bytes | |
| // rho: 7 u32 = 28 bytes | |
| // hashes: 4 elements, each is 8 u32 = 128 bytes | |
| // Total fixed size in fixture: 412 bytes, but actual SIGBYTES is 3112 bytes | |
| // The remaining bytes are likely padding or additional OTS data | |
| // Signature structure constants for the leansig-based XMSS test signature format | |
| // used by the JSON fixtures in this runner: | |
| // - path: 8 siblings, each is 8 u32 = 256 bytes | |
| // - rho: 7 u32 = 28 bytes | |
| // - hashes: 4 elements, each is 8 u32 = 128 bytes | |
| // Total fixed size explicitly serialized in the fixture is 412 bytes; the full | |
| // signature size (SIGBYTES) for the underlying leansig XMSS scheme is 3112 bytes, | |
| // with the remaining bytes handled internally by the signature implementation. |
| pub fn deinit(self: *Self) void { | ||
| if (self.owns_keypairs) { | ||
| var it = self.keys.iterator(); | ||
| while (it.next()) |entry| { | ||
| entry.value_ptr.deinit(); | ||
| } | ||
| } | ||
| self.clearEntries(); | ||
| self.keys.deinit(); | ||
| } |
There was a problem hiding this comment.
The clearEntries method is now called in deinit, which properly cleans up entries. However, the comment at line 324 in clearEntries should clarify that this method is idempotent and safe to call multiple times if needed.
| .ssz = .{ | ||
| .url = "git+https://github.com/blockblaz/ssz.zig#5ce7322fc45cab4f215021cae2579d1343e05d55", | ||
| .hash = "ssz-0.0.9-Lfwd61PEAgAUPJfUQiK4R5gRX_lTWOd_qYwNT-KAhRLA", | ||
| .url = "https://github.com/blockblaz/ssz.zig/archive/0ce92a8f093a321b0d0815eec6c90bd4e745d8e1.tar.gz", |
There was a problem hiding this comment.
The dependency changed from a git URL with fragment to a tarball archive URL. While valid, ensure this commit (0ce92a8) is stable and won't be force-pushed, as tarballs don't have the same ref protection as git tags.
| .url = "https://github.com/blockblaz/ssz.zig/archive/0ce92a8f093a321b0d0815eec6c90bd4e745d8e1.tar.gz", | |
| .url = "https://github.com/blockblaz/ssz.zig/archive/v0.0.9.tar.gz", |
No description provided.