test: update test runner to verify signatures#310
Conversation
Greptile SummaryThis PR restores changes accidentally dropped from #304, wiring the fork-choice spec-test runner to execute the full attestation flow — including XMSS signature verification — for both individual ( Confidence Score: 5/5Safe to merge; all remaining findings are P2 style suggestions in test utilities only. The production code change is a clean removal of a test-bypass function and a minor borrow-ordering refactor with no behavioral difference. The only flagged item is a low-context panic message in a test helper. No files require special attention.
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/store.rs | Removes test-only on_gossip_attestation_without_verification bypass; refactors on_gossip_aggregated_attestation to read stats before consuming the proof (borrow-checker fix); updates doc comment to reflect the stored data model. |
| crates/blockchain/tests/forkchoice_spectests.rs | Test runner now builds real SignedAttestation and SignedAggregatedAttestation objects with actual signatures from fixtures, replacing the old signature-bypass codepath. |
| crates/blockchain/tests/signature_types.rs | Removes duplicate deser_xmss_hex definition (now imported from super::common); no functional change to verify-signatures test infrastructure. |
| crates/blockchain/tests/types.rs | Adds is_aggregator to ForkChoiceStep, promotes signature to Option<XmssSignature>, and introduces ProofStepData/HexByteList for aggregated attestation fixtures. |
| crates/common/test-fixtures/src/lib.rs | Adds the canonical deser_xmss_hex helper migrated from signature_types.rs, enabling deduplication across test crates. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/blockchain/tests/types.rs
Line: 91-96
Comment:
**Panic on invalid hex in test fixture**
`From<HexByteList>` calls `expect` on hex decoding, so a malformed `0x…` string in a fixture will produce an opaque panic rather than a structured serde/test error. The context string `"invalid hex in proof data"` gives no indication of which step or test case failed.
This is test code, so a panic is not catastrophic, but surfacing the offending string in the message would make fixture authoring failures much easier to diagnose.
```suggestion
hex::decode(stripped)
.unwrap_or_else(|_| panic!("invalid hex in proof data: {:?}", value.data))
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test: update test runner to verify signa..." | Re-trigger Greptile
🤖 Kimi Code ReviewOverall Assessment: Good refactoring that removes unsafe test bypasses and improves aggregation logic. The move from per-validator to per-attestation-data storage is correct for consensus, but verify the store implementation aggregates participant bits rather than overwriting. Critical: Aggregation Storage Logic
The change stores one entry per unique attestation data rather than per validator. Ensure Suggested verification: Check that Safety: Removal of Verification Bypass
Removing
The test harness now uses real signature verification. Ensure the Correctness: Aggregated Attestation Construction
The Error Handling
impl From<HexByteList> for Vec<u8> {
fn from(value: HexByteList) -> Self {
let stripped = value.data.strip_prefix("0x").unwrap_or(&value.data);
hex::decode(stripped).expect("invalid hex in proof data") // Panic risk
}
}Issue: hex::decode(stripped).map_err(|e| serde::de::Error::custom(format!("invalid hex: {e}")))?However, since this is test-only code and the Code Quality
Excellent deduplication using
Good practice reading fields ( Minor: Documentation
The updated comment clarifies the aggregation strategy, but explicitly mention that participant bits are aggregated (OR'd) if an entry exists, not just "carried in the proof itself." Summary
The signature verification wiring and removal of bypass functions significantly improves consensus safety. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| impl From<HexByteList> for Vec<u8> { | ||
| fn from(value: HexByteList) -> Self { | ||
| let stripped = value.data.strip_prefix("0x").unwrap_or(&value.data); | ||
| hex::decode(stripped).expect("invalid hex in proof data") | ||
| } | ||
| } |
There was a problem hiding this comment.
From<HexByteList> calls expect on hex decoding, so a malformed 0x… string in a fixture will produce an opaque panic rather than a structured serde/test error. The context string "invalid hex in proof data" gives no indication of which step or test case failed.
This is test code, so a panic is not catastrophic, but surfacing the offending string in the message would make fixture authoring failures much easier to diagnose.
| impl From<HexByteList> for Vec<u8> { | |
| fn from(value: HexByteList) -> Self { | |
| let stripped = value.data.strip_prefix("0x").unwrap_or(&value.data); | |
| hex::decode(stripped).expect("invalid hex in proof data") | |
| } | |
| } | |
| hex::decode(stripped) | |
| .unwrap_or_else(|_| panic!("invalid hex in proof data: {:?}", value.data)) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/tests/types.rs
Line: 91-96
Comment:
**Panic on invalid hex in test fixture**
`From<HexByteList>` calls `expect` on hex decoding, so a malformed `0x…` string in a fixture will produce an opaque panic rather than a structured serde/test error. The context string `"invalid hex in proof data"` gives no indication of which step or test case failed.
This is test code, so a panic is not catastrophic, but surfacing the offending string in the message would make fixture authoring failures much easier to diagnose.
```suggestion
hex::decode(stripped)
.unwrap_or_else(|_| panic!("invalid hex in proof data: {:?}", value.data))
```
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewNow I have everything I need. Here is the full review: PR #310 Review: Update test runner to verify signaturesOverviewThis PR restores changes that were accidentally dropped from PR #304. It wires real XMSS signature verification into the fork choice spec test runner, replacing the now-deleted CorrectnessMove-before-borrow fix in
Potential Issues
let proof_data = ByteList::try_from(proof_bytes)
.expect("aggregated proof data fits in ByteListMiB");If a fixture ever carries a proof larger than the
Code QualityDRY improvement is significant. The three duplicated 15-line
Doc comment update on Removed SummaryThe PR is clean, well-scoped, and achieves its goal. The borrow fix (move without clone) is a genuine correctness/performance improvement. The test helper deduplication is a meaningful DRY win. No logic regressions are visible. The Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewFindings:
The I couldn’t run the Rust test target here because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
2 participants
This PR adds some changes that were mistakenly removed from #304. It updates the test runner to run the full attestation flow (signature verification included), which removes some verification-less paths that are no longer necessary. Before, we weren't doing signature verification, and had a wrong implementation for aggregated proofs processing.