feat(crypto): integrate leanMultisig devnet5 + leanSpec PR #717 wire format#370
feat(crypto): integrate leanMultisig devnet5 + leanSpec PR #717 wire format#370MegaRedHand wants to merge 5 commits into
Conversation
Bump lean-multisig + leansig_wrapper to devnet5 HEAD (0242c909) and rewrite ethlambda-crypto on the new Type-1 / Type-2 API: - aggregate_signatures / aggregate_mixed / aggregate_proofs now wrap aggregate_type_1; per-attestation proof bytes are TypeOneMultiSignature compress_without_pubkeys(). - verify_aggregated_signature wraps verify_type_1 with an explicit (message, slot) binding check. - New merge_type_1s_into_type_2 (real cryptographic block-level merge), verify_type_2_signature (binding-checked verifier), and split_type_2_signature (disaggregation). Wire production paths to the real primitives: - propose_block wraps the proposer XMSS as a singleton Type-1 SNARK, resolves per-component pubkeys, and calls merge_type_1s_into_type_2. The SignedBlock.proof envelope now carries the real merged Type-2. - verify_block_signatures runs structural checks first, then crypto- verifies the Type-2 via verify_type_2_signature with bindings derived from the block body and proposer index. - signature_spectests' SKIP_TESTS list emptied: block-level crypto is back, so test_invalid_proposer_signature runs against the real path. TypeTwoMultiSignature::from_type_1s is kept as a documented test-only structural envelope (empty proof bytes) for fast-fail unit tests. Fixes the test signature scheme to the production Dim46 instantiation so SIG_SIZE_FE matches lean-multisig's assertion; the new merge/verify/split round-trip test passes end-to-end in ~13s release.
Slim the on-wire shape to match the spec PR's "aggregated block proof"
model. The verifier no longer relies on duplicated metadata inside the
proof envelope — message, slot, and bytecode_claim live solely on the
block body it already trusts.
TypeOneInfo: {message, slot, participants, bytecode_claim}
→ {participants, proof}
TypeOneMultiSignature: {info, proof} (info.proof == outer proof)
TypeTwoMultiSignature: {info, bytecode_claim, proof}
→ {info, proof}
The per-component Type-1 bytes now live inside TypeOneInfo.proof, so a
node receiving a block can recover a standalone Type-1 (e.g. for fork-
choice payload caching or re-broadcast) without running a fresh SNARK.
on_block_core wires this through: known_aggregated_payloads entries now
carry the real per-attestation Type-1 wire, not an empty placeholder.
verify_block_signatures drops the duplicate (message, slot) cross-check
on each info entry; bindings are rederived from block.body.attestations
+ (block_root, block.slot) and handed to verify_type_2_signature.
Disaggregation API swapped from split_type_2_signature(index) to
split_type_2_by_message, mirroring leanSpec's split_by_msg primitive.
The wrapper decompresses the Type-2, finds the unique component whose
internal native message matches, and delegates to lean_multisig's
split_type_2. New AggregationError::UnknownMessage / MultipleMessages
variants replace the now-unused SplitIndexOutOfBounds.
build_block_caps_attestation_data_entries: bump synthetic PROOF_SIZE
down from 253 KiB → 50 KiB to reflect that the envelope now carries N+1
copies of the per-component bytes, and to roughly match an expected
lean-multisig devnet5 Type-1 SNARK size.
leanSpec PR #717 embeds each component's standalone Type-1 SNARK inside
`TypeOneInfo.proof` so split-by-msg can recover a Type-1 without running
a fresh SNARK. With realistic lean-multisig devnet5 Type-1 sizes
(~225 KiB observed locally) bundling N+1 copies of those bytes plus the
merged Type-2 proof blows past the 1 MiB `ByteListMiB` cap on the outer
`SignedBlock.proof` envelope: the proposer panicked with
`OverCapacity { max: 1048576, got: 1354324 }` already at slot 6 with
5 attestations.
Strip `info[i].proof` to empty bytes when packing the Type-2 envelope
in `propose_block`. The merged proof bytes alone still bind the full
signature set, so `verify_block_signatures` keeps working. Recovery of
a standalone Type-1 is still possible via
`split_type_2_by_message`, which is SNARK-backed regardless.
On_block_core stops trying to read per-component bytes back; the
fork-choice payload-buffer entries it inserts are info-only, matching
their pre-PR-717 shape.
Verified with a 2-node ethlambda-only devnet run over 21 slots: every
block (attestation_count 0..7) is `Block Type-2 proof verified`,
`crypto_elapsed ~38 ms`, no panic. Finalization didn't advance with
only 2 validators in a single committee, but that's orthogonal —
fork-choice reorgs blocking 2/3+ vote accumulation, not the wire
format.
Block production previously ran two SNARKs on the actor thread (proposer
Type-1 wrap + merge_many_type_1). Each currently takes ~5 s, so the tick
handler at interval 0 stalls past interval 1 and validators never publish
attestations beyond slot 0. With no attestations after the first slot,
justification stays at 0 forever and the chain cannot finalize.
Add a `--crypto-merge-t1-into-t2` flag (default `false`) that gates both
SNARKs:
* off (default): proposer ships a metadata-only Type-2 envelope
(per-component `participants` + empty proof bytes), `propose_block`
stays fast, interval-1 attestations run on time. `verify_block_signatures`
detects the empty SNARK and skips `verify_type_2`, keeping the existing
structural checks. Per-attestation crypto still runs at gossip ingestion.
* on: full devnet5 cryptography (real proposer Type-1 + merge_many_type_1
+ verify_type_2 on import).
Default off until the SNARK work is moved off the actor thread —
spawn_blocking + result message, mirroring how the aggregator already
runs `aggregate_job` on a worker thread.
Single-node devnet (8 validators on ethlambda_0 with --is-aggregator,
flag default-off) finalizes:
Fork Choice Tree:
Finalized: slot 4 | root 1376f65e
Justified: slot 5 | root b89c21ad
Head: slot 7 | root f89ebf32
Justification at slot 2, finalization at slot 3 — chain progresses one
slot per slot from there.
🤖 Kimi Code ReviewThis PR introduces significant changes to the signature aggregation architecture (Type-1/Type-2 multisig migration per leanSpec PR #717). Overall the code is well-structured with proper error handling and security-conscious validation, but there are performance concerns and minor optimizations needed. Critical/Security IssuesNone identified. Bounds checking on validator indices is consistent, cryptographic bindings are properly verified, and the structural pre-check before expensive SNARK verification is correct. Performance & ConcurrencyItem 1. Blocking actor thread with SNARK operations Item 2. Redundant proposer pubkey decoding Item 3. Cloning proof data in compaction loop Code CorrectnessItem 4. Unused index variable let _ = idx; // index reserved for richer diagnostics if neededIf diagnostics are needed, implement them; otherwise remove the binding. Item 5. Error context in split deserialization Rust Best PracticesItem 6. Panic in test helper Item 7. Constant naming Architecture & MaintainabilityItem 8. Wire format change documentation Item 9. Dependency audit trail
Positive Acknowledgments
SummaryThe PR is approved with suggestions. Address Items 1–3 before enabling Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewNow I have a complete picture. Here is my review: PR #370 — feat(crypto): integrate leanMultisig devnet5 + leanSpec PR #717 wire formatOverviewThis PR bumps The overall design is sound. The known-limitations section in the PR description is thorough and accurate. Correctness and SecurityStructural pre-checks weakened on the stub path (important to track) The previous if info.slot != attestation.data.slot { return Err(...); }
if info.message != attestation.data.hash_tree_root() { return Err(...); }Both checks are now removed because In practice, the security exposure is bounded: (1) the participants bitfield is still checked ( Recommendation: Add a comment at the Proposer pubkey decoded twice when In let Ok(proposer_pubkey) = proposer_validator.get_proposal_pubkey()...
// ... used to build proposer_t1_bytes ...
// ↓ decoded again for merge_inputs
let proposer_pubkey = match proposer_validator.get_proposal_pubkey() {The second decode should just reuse the first. Both decode the same validator's proposal pubkey; if the second fails it would be a consistency bug (it won't, but the asymmetric error handling is confusing). Extract to a single
let mut resolve_failed = false;
for t1 in &type_one_proofs {
...
if resolve_failed { break; }
...
}
if resolve_failed {
metrics::inc_block_building_failures();
return;
}This is a valid Rust pattern but obscures the actual error (the loop continues to check Memory and PerformanceProof bytes stored twice in
Self {
info: TypeOneInfo {
participants,
proof: proof_data.clone(),
},
proof: proof_data,
}For lean-multisig devnet5, a real Type-1 weighs ~225 KiB. Every in-memory
The doc comment on
But .map(|t1| TypeOneInfo {
participants: t1.info.participants,
proof: ByteListMiB::default(), // ← intentionally empty
})The doc describes the leanSpec design intent, not what ethlambda currently sends. This is acknowledged in PR Known Limitation 3 (size cap pressure), but readers of the type definition alone will be confused. The comment should say that API and Idioms
LMType2::decompress_without_pubkeys(...)
.ok_or(AggregationError::ChildDeserializationFailed(0))?;
let _ = idx; // index reserved for richer diagnostics if neededIf for ((expected_msg, expected_slot), info) in expected_bindings.iter().zip(sig.info.iter()) {The comment about "richer diagnostics" is a future-work note; if useful later, add it then.
The test helper at Positive observations
SummaryThe PR is mergeable in its current form. The two highest-priority follow-ups are:
The stub-path security relaxation (dropping message/slot structural checks) is intentional and bounded, but deserves an inline comment so future readers understand what is and isn't checked on the empty-SNARK path. Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewFindings
Assumptions Testing Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR integrates the
Confidence Score: 3/5The default configuration ships blocks with no proposer-level cryptographic verification; any network peer can construct a structurally valid block claiming any elected proposer without supplying a valid XMSS signature over the block root. The default --crypto-merge-t1-into-t2=false path causes verify_block_signatures to skip all SNARK and XMSS checking on the block envelope, leaving proposer authentication entirely absent unless the state-transition function independently enforces it. This is intentional and documented for devnet throughput, but the security gap is real on the default path. Memory duplication in TypeOneMultiSignature::new and the misleading error variants are minor but add up over time. Focus on crates/blockchain/src/store.rs (verify_block_signatures empty-proof early return), crates/blockchain/src/lib.rs (proposer proof assembly when flag is off), and crates/common/types/src/block.rs (TypeOneMultiSignature::new clone duplication).
|
| Filename | Overview |
|---|---|
| crates/common/crypto/src/lib.rs | New Type-1/Type-2 aggregation and verification wrappers; split_type_2_by_message uses a misleading error variant on Type-2 decompression failure |
| crates/blockchain/src/lib.rs | Adds crypto_merge_t1_into_t2 flag; the default-off path ships empty proof bytes causing verify_block_signatures to skip all proposer signature crypto; redundant pubkey fetch when flag is on |
| crates/blockchain/src/store.rs | Rewrites verify_block_signatures to the new Type-2 API; correctly gates crypto on non-empty proof; slot-overflow error is mapped to the wrong StoreError variant |
| crates/common/types/src/block.rs | New slim wire format for TypeOneInfo/TypeTwoMultiSignature; TypeOneMultiSignature::new stores identical proof bytes in both info.proof and proof, doubling per-proof in-memory size |
| crates/blockchain/src/aggregation.rs | Aggregation worker updated to use new TypeOneInfo fields; logic unchanged, clean migration |
| bin/ethlambda/src/main.rs | Adds --crypto-merge-t1-into-t2 CLI flag (default false); wired correctly to BlockChain::spawn |
| crates/common/test-fixtures/src/verify_signatures.rs | Lossy and lossless TestSignedBlock conversions updated to new wire format; try_into_signed_block_with_proofs uses from_type_1s which produces an empty outer proof, so Hive fixtures will always exercise structural-only verification |
| crates/net/rpc/src/test_driver.rs | Minor import/usage updates to align with new verify_signatures path; no logic changes |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[propose_block] -->|crypto_merge_t1_into_t2=true| B[aggregate_signatures\nproposer Type-1 SNARK]
A -->|crypto_merge_t1_into_t2=false| C[proposer_proof_bytes = empty]
B --> D[merge_type_1s_into_type_2\nattestation T1s + proposer T1]
C --> E[merged_proof_bytes = empty]
D --> F[TypeTwoMultiSignature\nwith real SNARK proof]
E --> G[TypeTwoMultiSignature\nmetadata-only envelope]
F --> H[verify_block_signatures]
G --> H
H -->|merged.proof.is_empty| I[Structural checks only\nparticipant bitfields\nno crypto]
H -->|merged.proof non-empty| J[verify_type_2_signature\nfull SNARK + binding check]
I --> K[OK - default devnet path]
J --> L[OK - full crypto path]
Prompt To Fix All With AI
Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
crates/blockchain/src/store.rs:1244-1258
**Empty-proof path silently skips all proposer signature verification**
When `--crypto-merge-t1-into-t2=false` (the default), the proposer sets `merged_proof_bytes = ByteListMiB::default()` and the proposer's XMSS signature is never included in any cryptographically bound proof. The structural check at line 1236 only confirms that `proposer_bits == [block.proposer_index]` — it does *not* verify that the declared `proposer_index` actually signed `block_root`. Any peer that knows the expected proposer for a slot can submit a block with a valid-looking envelope and it will pass all signature checks here. It is worth confirming that `ethlambda_state_transition::state_transition` independently enforces that `block.proposer_index` matches the slot's elected proposer so the chain at least rejects impersonation at that layer.
### Issue 2 of 5
crates/common/crypto/src/lib.rs:387-389
`split_type_2_by_message` returns `ChildDeserializationFailed(0)` when the outer Type-2 proof fails to decompress. `ChildDeserializationFailed` carries an index implying a specific Type-1 child at position 0 failed — but this fires on the top-level Type-2 object before any child is accessed, which will mislead diagnostics.
```suggestion
let type_2 =
LMType2::decompress_without_pubkeys(proof_data.iter().as_slice(), pubkeys_per_info)
.ok_or(AggregationError::DeserializationFailed)?;
```
### Issue 3 of 5
crates/common/types/src/block.rs:118-129
**`TypeOneMultiSignature::new` stores identical proof bytes in both `info.proof` and `proof`**
Every standalone Type-1 costs ~2x its serialized size in memory: ~225 KiB is cloned into `info.proof` and again into `proof`. The `propose_block` path strips `info.proof` when packing the wire Type-2, so the duplication is never sent on the wire — but the redundant clone is live in the actor's heap for the full lifetime of each aggregated proof. Consider using an `Arc<ByteListMiB>` to share the allocation.
### Issue 4 of 5
crates/blockchain/src/store.rs:1292-1293
A slot `u64` to `u32` conversion failure is mapped to `ProposerSignatureVerificationFailed`, surfacing as a misleading "signature failed" error in logs. At ~4 billion slots this is unreachable in practice, but the error category is wrong and would make it hard to diagnose if it ever triggered.
```suggestion
let block_slot_u32 =
u32::try_from(block.slot).map_err(|_| StoreError::SlotOutOfRange(block.slot))?;
```
### Issue 5 of 5
crates/blockchain/src/lib.rs:377-450
**Proposer pubkey decoded twice when `crypto_merge_t1_into_t2=true`**
`proposer_validator.get_proposal_pubkey()` is called at line ~378 (to wrap the raw XMSS signature into a Type-1) and again at line ~413 (to build the merge inputs). Both calls occur on the actor thread's hot path and `get_proposal_pubkey` deserializes the key bytes each time. Compute the pubkey once and reuse it for both uses.
Reviews (1): Last reviewed commit: "feat(blockchain): gate Type-2 SNARK behi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| // Skip crypto when the merged proof carries no SNARK bytes (the stub path | ||
| // used while the actor-thread SNARK work is being moved off-thread — | ||
| // per-attestation crypto still runs at gossip ingestion). | ||
| if merged.proof.is_empty() { | ||
| let total_elapsed = total_start.elapsed(); | ||
| info!( | ||
| slot = block.slot, | ||
| attestation_count = attestations.len(), | ||
| ?structural_elapsed, | ||
| ?total_elapsed, | ||
| "Block Type-2 proof structural-only (empty SNARK bytes)" | ||
| ); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
When --crypto-merge-t1-into-t2=false (the default), the proposer sets merged_proof_bytes = ByteListMiB::default() and the proposer's XMSS signature is never included in any cryptographically bound proof. The structural check at line 1236 only confirms that proposer_bits == [block.proposer_index] — it does not verify that the declared proposer_index actually signed block_root. Any peer that knows the expected proposer for a slot can submit a block with a valid-looking envelope and it will pass all signature checks here. It is worth confirming that ethlambda_state_transition::state_transition independently enforces that block.proposer_index matches the slot's elected proposer so the chain at least rejects impersonation at that layer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1244-1258
Comment:
**Empty-proof path silently skips all proposer signature verification**
When `--crypto-merge-t1-into-t2=false` (the default), the proposer sets `merged_proof_bytes = ByteListMiB::default()` and the proposer's XMSS signature is never included in any cryptographically bound proof. The structural check at line 1236 only confirms that `proposer_bits == [block.proposer_index]` — it does *not* verify that the declared `proposer_index` actually signed `block_root`. Any peer that knows the expected proposer for a slot can submit a block with a valid-looking envelope and it will pass all signature checks here. It is worth confirming that `ethlambda_state_transition::state_transition` independently enforces that `block.proposer_index` matches the slot's elected proposer so the chain at least rejects impersonation at that layer.
How can I resolve this? If you propose a fix, please make it concise.| let type_2 = | ||
| LMType2::decompress_without_pubkeys(proof_data.iter().as_slice(), pubkeys_per_info) | ||
| .ok_or(AggregationError::ChildDeserializationFailed(0))?; |
There was a problem hiding this comment.
split_type_2_by_message returns ChildDeserializationFailed(0) when the outer Type-2 proof fails to decompress. ChildDeserializationFailed carries an index implying a specific Type-1 child at position 0 failed — but this fires on the top-level Type-2 object before any child is accessed, which will mislead diagnostics.
| let type_2 = | |
| LMType2::decompress_without_pubkeys(proof_data.iter().as_slice(), pubkeys_per_info) | |
| .ok_or(AggregationError::ChildDeserializationFailed(0))?; | |
| let type_2 = | |
| LMType2::decompress_without_pubkeys(proof_data.iter().as_slice(), pubkeys_per_info) | |
| .ok_or(AggregationError::DeserializationFailed)?; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/crypto/src/lib.rs
Line: 387-389
Comment:
`split_type_2_by_message` returns `ChildDeserializationFailed(0)` when the outer Type-2 proof fails to decompress. `ChildDeserializationFailed` carries an index implying a specific Type-1 child at position 0 failed — but this fires on the top-level Type-2 object before any child is accessed, which will mislead diagnostics.
```suggestion
let type_2 =
LMType2::decompress_without_pubkeys(proof_data.iter().as_slice(), pubkeys_per_info)
.ok_or(AggregationError::DeserializationFailed)?;
```
How can I resolve this? If you propose a fix, please make it concise.| impl TypeOneMultiSignature { | ||
| /// Build a Type-1 proof with the given participants, message, slot and | ||
| /// raw proof bytes. | ||
| pub fn new( | ||
| participants: AggregationBits, | ||
| message: H256, | ||
| slot: u64, | ||
| proof_data: ByteListMiB, | ||
| ) -> Self { | ||
| /// Build a Type-1 proof carrying the given participant bitfield and the | ||
| /// aggregated proof bytes. | ||
| pub fn new(participants: AggregationBits, proof_data: ByteListMiB) -> Self { | ||
| Self { | ||
| info: TypeOneInfo { | ||
| message, | ||
| slot, | ||
| participants, | ||
| bytecode_claim: BytecodeClaim::ZERO, | ||
| proof: proof_data.clone(), | ||
| }, | ||
| proof: proof_data, | ||
| } | ||
| } |
There was a problem hiding this comment.
TypeOneMultiSignature::new stores identical proof bytes in both info.proof and proof
Every standalone Type-1 costs ~2x its serialized size in memory: ~225 KiB is cloned into info.proof and again into proof. The propose_block path strips info.proof when packing the wire Type-2, so the duplication is never sent on the wire — but the redundant clone is live in the actor's heap for the full lifetime of each aggregated proof. Consider using an Arc<ByteListMiB> to share the allocation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/types/src/block.rs
Line: 118-129
Comment:
**`TypeOneMultiSignature::new` stores identical proof bytes in both `info.proof` and `proof`**
Every standalone Type-1 costs ~2x its serialized size in memory: ~225 KiB is cloned into `info.proof` and again into `proof`. The `propose_block` path strips `info.proof` when packing the wire Type-2, so the duplication is never sent on the wire — but the redundant clone is live in the actor's heap for the full lifetime of each aggregated proof. Consider using an `Arc<ByteListMiB>` to share the allocation.
How can I resolve this? If you propose a fix, please make it concise.| let block_slot_u32 = | ||
| u32::try_from(block.slot).map_err(|_| StoreError::ProposerSignatureVerificationFailed)?; |
There was a problem hiding this comment.
u64 to u32 conversion failure is mapped to ProposerSignatureVerificationFailed, surfacing as a misleading "signature failed" error in logs. At ~4 billion slots this is unreachable in practice, but the error category is wrong and would make it hard to diagnose if it ever triggered.
| let block_slot_u32 = | |
| u32::try_from(block.slot).map_err(|_| StoreError::ProposerSignatureVerificationFailed)?; | |
| let block_slot_u32 = | |
| u32::try_from(block.slot).map_err(|_| StoreError::SlotOutOfRange(block.slot))?; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1292-1293
Comment:
A slot `u64` to `u32` conversion failure is mapped to `ProposerSignatureVerificationFailed`, surfacing as a misleading "signature failed" error in logs. At ~4 billion slots this is unreachable in practice, but the error category is wrong and would make it hard to diagnose if it ever triggered.
```suggestion
let block_slot_u32 =
u32::try_from(block.slot).map_err(|_| StoreError::SlotOutOfRange(block.slot))?;
```
How can I resolve this? If you propose a fix, please make it concise.| let proposer_proof_bytes = if self.crypto_merge_t1_into_t2 { | ||
| let Ok(proposer_pubkey) = proposer_validator.get_proposal_pubkey().inspect_err( | ||
| |err| error!(%slot, %validator_id, %err, "Failed to decode proposer proposal pubkey"), | ||
| ) else { | ||
| metrics::inc_block_building_failures(); | ||
| return; | ||
| }; | ||
| let Ok(proposer_validator_signature) = | ||
| ValidatorSignature::from_bytes(&proposer_signature).inspect_err(|err| { | ||
| error!(%slot, %validator_id, %err, "Failed to decode proposer signature bytes") | ||
| }) | ||
| else { | ||
| metrics::inc_block_building_failures(); | ||
| return; | ||
| }; | ||
| let Ok(proposer_t1_bytes) = ethlambda_crypto::aggregate_signatures( | ||
| vec![proposer_pubkey.clone()], | ||
| vec![proposer_validator_signature], | ||
| &block_root, | ||
| slot as u32, | ||
| ) | ||
| .inspect_err(|err| { | ||
| error!(%slot, %validator_id, %err, "Failed to wrap proposer signature as Type-1") | ||
| }) else { | ||
| metrics::inc_block_building_failures(); | ||
| return; | ||
| }; | ||
| proposer_t1_bytes | ||
| } else { | ||
| ByteListMiB::default() | ||
| }; | ||
|
|
||
| let proposer_t1 = | ||
| TypeOneMultiSignature::for_proposer(validator_id, proposer_proof_bytes.clone()); | ||
|
|
||
| let merged_proof_bytes = if self.crypto_merge_t1_into_t2 { | ||
| let proposer_pubkey = match proposer_validator.get_proposal_pubkey() { | ||
| Ok(pk) => pk, | ||
| Err(err) => { | ||
| error!(%slot, %validator_id, %err, "Failed to decode proposer proposal pubkey"); | ||
| metrics::inc_block_building_failures(); | ||
| return; | ||
| } | ||
| }; | ||
| let mut merge_inputs: Vec<(Vec<ValidatorPublicKey>, ByteListMiB)> = | ||
| Vec::with_capacity(type_one_proofs.len() + 1); | ||
| let mut resolve_failed = false; | ||
| for t1 in &type_one_proofs { | ||
| let mut pubkeys = Vec::new(); | ||
| for vid in t1.participant_indices() { | ||
| let Some(validator) = validators.get(vid as usize) else { | ||
| error!(%slot, %validator_id, vid, "Participant out of range while resolving pubkeys"); | ||
| resolve_failed = true; | ||
| break; | ||
| }; | ||
| match validator.get_attestation_pubkey() { | ||
| Ok(pk) => pubkeys.push(pk), | ||
| Err(err) => { | ||
| error!(%slot, %validator_id, vid, %err, "Failed to decode attestation pubkey"); | ||
| resolve_failed = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if resolve_failed { | ||
| break; | ||
| } | ||
| merge_inputs.push((pubkeys, t1.proof.clone())); | ||
| } | ||
| if resolve_failed { | ||
| metrics::inc_block_building_failures(); | ||
| return; | ||
| } | ||
| merge_inputs.push((vec![proposer_pubkey], proposer_proof_bytes)); |
There was a problem hiding this comment.
crypto_merge_t1_into_t2=true
proposer_validator.get_proposal_pubkey() is called at line ~378 (to wrap the raw XMSS signature into a Type-1) and again at line ~413 (to build the merge inputs). Both calls occur on the actor thread's hot path and get_proposal_pubkey deserializes the key bytes each time. Compute the pubkey once and reuse it for both uses.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/lib.rs
Line: 377-450
Comment:
**Proposer pubkey decoded twice when `crypto_merge_t1_into_t2=true`**
`proposer_validator.get_proposal_pubkey()` is called at line ~378 (to wrap the raw XMSS signature into a Type-1) and again at line ~413 (to build the merge inputs). Both calls occur on the actor thread's hot path and `get_proposal_pubkey` deserializes the key bytes each time. Compute the pubkey once and reuse it for both uses.
How can I resolve this? If you propose a fix, please make it concise.
|
Reviewed locally: The PR splits cleanly into three layers and reads well:
Worth raising (rough priority)1. Off-default path skips proposer-signature verification entirely. With
2. Doc/wire-format inconsistency on 3. 4. Dead 5. 6. Unrealistic Type-1 size in the payload-buffer test. 7. Minor cleanup. Dropped-but-recomputed locals in test fixtures ( Things that look right
Test-plan gapThe "Local single-node devnet: finalizes one slot per slot for 360+ slots" checkbox was done with the default flag off — so it doesn't exercise the block-level crypto path. The |
* lib.rs (#5): decode proposer proposal pubkey once and reuse it for the singleton Type-1 wrap and the merge inputs; was deserialized twice on the crypto hot path. * store.rs (#4): block / attestation slot u64→u32 overflow now maps to a dedicated `SlotOutOfRange(u64)` variant instead of being misreported as `ProposerSignatureVerificationFailed`. * store.rs (#1): explicitly document the security caveat of the empty- SNARK structural-only branch (proposer XMSS not crypto-verified) and note the two upstream mitigations (STF `process_block_header` rejects wrong proposer_index; per-attestation crypto still runs at gossip ingestion). The structural log line now flags it explicitly. * crypto/src/lib.rs (#2): outer Type-2 decompression failure in `split_type_2_by_message` returns a new `DeserializationFailed` variant instead of `ChildDeserializationFailed(0)`, which had implied a child at index 0 had failed. * types/src/block.rs (#3): annotate the intentional duplication of proof bytes between `TypeOneMultiSignature::info.proof` and the outer `proof` field — mirrors leanSpec PR #717's shape so a Type-1 embedded inside a Type-2's info[i] reads the same as a standalone Type-1.
2 participants
Summary
lean-multisig/leansig_wrapperto devnet5 HEAD (0242c909) and rewriteethlambda-cryptoon the new Type-1 / Type-2 API (aggregate_type_1,verify_type_1,merge_many_type_1,verify_type_2,split_type_2_by_message).TypeOneInfo { participants, proof },TypeTwoMultiSignature { info, proof }—message/slot/bytecode_claimrederived from the block body during verify.--crypto-merge-t1-into-t2(default off). With the flag off, block production stays fast and the chain finalizes; with it on, full devnet5 crypto runs on the block-building hot path.Commits
1cd80ddpropose_block, realverify_type_2inverify_block_signatures, re-enabledtest_invalid_proposer_signature.2c9dec0TypeOneInfobecomes{participants, proof},TypeTwoMultiSignaturedropsbytecode_claim. Verifier rederives bindings from block body.split_type_2_signature(index)→split_type_2_by_message(message).5361136lean_multisigType-1s are ~225 KiB, so N+1 copies blow the 1 MiBByteListMiBcap. Outer merged proof still binds the full signature set; split is SNARK-backed.3199e7d--crypto-merge-t1-into-t2flag (default off). Off path ships a metadata-only Type-2 envelope;verify_block_signaturesdetects the empty SNARK and skipsverify_type_2. Per-attestation crypto still runs at gossip ingestion. Off by default until the SNARK work is moved off the actor thread (it currently takes ~5s and starves interval-1 attestation production).Crypto crate API
aggregate_signatures(pks, sigs, msg, slot)aggregate_type_1([], raw_xmss, …)aggregate_mixed(children, raw_pks, raw_sigs, msg, slot)aggregate_type_1(children, raw_xmss, …)aggregate_proofs(children, msg, slot)aggregate_type_1(children, [], …)verify_aggregated_signature(proof, pks, msg, slot)verify_type_1merge_type_1s_into_type_2(parts)merge_many_type_1verify_type_2_signature(proof, pks_per_component, expected_bindings)verify_type_2split_type_2_by_message(proof, pks_per_component, message)split_type_2(after locating index by message)split_by_msgType-1 / Type-2 proof bytes on the wire are$2^{32}$ ).
compress_without_pubkeys()form. Test scheme corrected from Dim64 to Dim46 to match productionValidatorPublicKey(Dim46-aborting-target-sum, lifetimeWire format (after PR #717 alignment)
Removed types:
BytecodeClaim(was a placeholderH256, alwaysZERO).This is not byte-compatible with the previous Type-1 / Type-2 envelope (PR #361 wire format). Cross-client interop on devnet5 requires the other clients to land equivalent slim shapes.
Devnet validation
Single-node 8-validator devnet (aggregator on, default
--crypto-merge-t1-into-t2=false):Chain advances one slot per slot from genesis, finalization tracks head minus ~2.
verify_block_signaturesruns the structural pre-checks (~10 µs) and skipsverify_type_2on the empty-SNARK path.A 4-minute pause was observed mid-run; tracing showed it was a wall-clock gap (host sleep / Docker daemon suspend) rather than a finalization fault — chain resumed cleanly at the next valid slot.
Crypto path is exercised via the ignored unit test
test_type_2_merge_verify_split_round_tripinethlambda-crypto, which produces 2 Type-1s, merges them into a Type-2, verifies the Type-2, then splits the first component back into a Type-1 and verifies it. Round-trip passes in ~13s release.Known limitations / follow-ups
propose_blockcallsaggregate_signatures(for the proposer Type-1) andmerge_type_1s_into_type_2synchronously. Each call is ~5 s, the actor's message loop is single-threaded, and interval-1 attestation ticks get starved. Off-load tospawn_blocking+ result message — mirror what the aggregator already does inrun_aggregation_worker. Until then the crypto path stays off by default.lean-quickstart/client-cmds/ethlambda-cmd.shhad stale CLI flags (--genesis,--validators, …). Updated locally to use--custom-network-config-dir. Should be upstreamed.info[i].prooffor cheap split" overflows our 1 MiB envelope cap at realistic Type-1 sizes. We strip the bytes on send; split-by-msg becomes SNARK-backed instead of free-by-deserialize. Worth flagging on the upstream PR.Test plan
cargo build --workspace --all-targets— greencargo fmt --check— greencargo clippy --workspace --all-targets -- -D warnings— greencargo test --workspace --release --lib— 101 pass / 0 fail / 7 ignoredcargo test --release -p ethlambda-crypto --lib -- --ignored— 5 pass (incl. Type-2 merge / verify / split round-trip)verify_signaturesdriver against the realverify_type_2(needs leanSpec PR #717 fixtures)