Conversation
🤖 Kimi Code ReviewReview Summary: This PR implements recursive XMSS proof aggregation (leanSpec #510) and replaces the byte-limit-based block packing with a count-based limit. The cryptographic logic looks correct, but there are critical issues with determinism and error handling that must be addressed. Critical Issues1. Non-deterministic attestation ordering in
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewNow I have enough context to write a thorough review. Here it is: PR Review: feat: add proposer proof aggregationOverviewThis PR replaces the byte-budget cap on block attestation size with true recursive XMSS proof aggregation (leanSpec #510). When multiple proofs cover the same CorrectnessMessage consistency in recursive aggregation — OK The
The Empty proof handling — behavioral change with no safety net The old Test Coverage — Significant GapThe only test covering the duplicate-merging path (
The actual recursive aggregation path — where The
|
🤖 Codex Code Review
I did not run the Rust tests here because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR introduces true XMSS proof aggregation on the proposer side: a greedy proof-selection pass (
Confidence Score: 3/5Not safe to merge until the overlapping-participant aggregation issue is resolved; invalid proofs would cause cross-client verification failures in multi-aggregator scenarios. A P1 correctness issue exists in
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/store.rs | Adds greedy proof selection (extend_proofs_greedily), proof compaction (compact_attestations), and recursive proof aggregation into build_block; the greedy selector can pick proofs with overlapping participants, which may produce invalid aggregated proofs when passed to aggregate_proofs. |
| crates/common/crypto/src/lib.rs | Adds aggregate_proofs for recursive proof merging and fixes log_inv_rate from 1 to 2 for cross-client compatibility; minor clone overhead in aggregate_proofs but otherwise correct. |
| crates/blockchain/tests/forkchoice_spectests.rs | Updates build_signed_block to emit one AggregatedSignatureProof::empty per attestation so the signatures list length matches attestations; adds gossipAggregatedAttestation step handler routing through the no-verification path with a clear TODO. |
| CLAUDE.md | Documentation-only update reflecting the new log_inv_rate=2 convention and aggregation pipeline details; no code changes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[aggregated_payloads\ndata_root → AttestationData, Vec<Proof>] --> B[build_block loop\nfilter by source == current_justified]
B --> C[extend_proofs_greedily\ngreedy coverage selection]
C -->|may select overlapping proofs| D[aggregated_attestations\nmulti-entry per AttestationData]
C -->|same proofs| E[aggregated_signatures\nVec<AggregatedSignatureProof>]
D --> F[compact_attestations\ngroup by AttestationData]
E --> F
F -->|single entry| G[keep as-is]
F -->|multiple entries ≥2| H[union_aggregation_bits\nmerge participants]
H --> I[aggregate_proofs\nxmss_aggregate children\n⚠️ may have overlapping pubkeys]
I --> J[merged AggregatedSignatureProof]
G --> K[final block\ncompacted attestations + proofs]
J --> K
K --> L[process_block\nstate transition\ncompute state_root]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1085-1138
Comment:
**Greedy proof selection may pass overlapping participant sets to `aggregate_proofs`**
`extend_proofs_greedily` selects a proof as long as it covers at least one *new* validator (`best_count > 0`), but pushes the *original* proof — which may still include participants already covered by a previously selected proof. When two proofs for the same `AttestationData` overlap (e.g. local committee aggregation covered {1,2,3} and a received gossip proof covered {2,3,4}), both are selected and later passed as children to `aggregate_proofs` in `compact_attestations`. The `xmss_aggregate` call in `aggregate_proofs` receives child-pubkey lists `[pubkeys_for_{1,2,3}, pubkeys_for_{2,3,4}]` with validators 2 and 3 appearing in both children. Recursive aggregation of proofs with overlapping participants is likely unsupported by `xmss_aggregate`, which could produce an invalid proof that fails verification on peer nodes.
A straightforward fix is to restrict selection to proofs whose participants are fully disjoint from `covered`:
```rust
// Only select proof if ALL its participants are new (disjoint from covered)
let is_disjoint = proof.participant_indices().all(|vid| !covered.contains(&vid));
if !is_disjoint || best_count == 0 {
break;
}
```
Alternatively, restrict the greedy loop to select at most one proof and rely on `compact_attestations` to merge only from distinct, non-overlapping sources.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/common/crypto/src/lib.rs
Line: 148-155
Comment:
**Avoidable `AggregatedXMSS::clone()` per child proof**
The `children_refs` step borrows `deserialized` by shared reference and then clones each `AggregatedXMSS`. Since proof data can be large (potentially hundreds of KB), this allocates one full proof copy per child. The clone can be eliminated by consuming `deserialized` and splitting it:
```rust
let (pks_list, aggs): (Vec<Vec<LeanSigPubKey>>, Vec<AggregatedXMSS>) =
deserialized.into_iter().unzip();
let children_refs: Vec<(&[LeanSigPubKey], AggregatedXMSS)> =
pks_list.iter().map(|v| v.as_slice()).zip(aggs).collect();
```
This avoids the per-child clone while satisfying the borrow requirements of `xmss_aggregate`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1210-1234
Comment:
**Intermediate justification checks clone and run state transition on uncompacted attestations**
Inside the loop, `aggregated_attestations.clone().try_into()` is called and then `process_slots` + `process_block` are run on uncompacted data that may contain multiple entries per `AttestationData`. `process_attestations` in the state transition uses a per-validator boolean map keyed on `target.root`, so disjoint-participant duplicates are handled correctly, but the repeated full state-transition clones (`head_state.clone()` every iteration) are expensive. Consider accumulating just the newly added data roots and running a single speculative state transition after all data have been collected, rather than once per justification advance.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test: skip known failing tests" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -1098,10 +1116,6 @@ fn extend_proofs_greedily( | |||
| } | |||
|
|
|||
| let proof = &proofs[best_idx]; | |||
| let proof_bytes = proof.proof_data.len(); | |||
| if bytes_consumed + proof_bytes > remaining_bytes { | |||
| break; | |||
| } | |||
|
|
|||
| // Collect coverage only for the winning proof | |||
| let new_covered: Vec<u64> = proof | |||
| @@ -1120,10 +1134,7 @@ fn extend_proofs_greedily( | |||
|
|
|||
| covered.extend(new_covered); | |||
| remaining_indices.remove(&best_idx); | |||
| bytes_consumed += proof_bytes; | |||
| } | |||
|
|
|||
| bytes_consumed | |||
| } | |||
There was a problem hiding this comment.
aggregate_proofs
extend_proofs_greedily selects a proof as long as it covers at least one new validator (best_count > 0), but pushes the original proof — which may still include participants already covered by a previously selected proof. When two proofs for the same AttestationData overlap (e.g. local committee aggregation covered {1,2,3} and a received gossip proof covered {2,3,4}), both are selected and later passed as children to aggregate_proofs in compact_attestations. The xmss_aggregate call in aggregate_proofs receives child-pubkey lists [pubkeys_for_{1,2,3}, pubkeys_for_{2,3,4}] with validators 2 and 3 appearing in both children. Recursive aggregation of proofs with overlapping participants is likely unsupported by xmss_aggregate, which could produce an invalid proof that fails verification on peer nodes.
A straightforward fix is to restrict selection to proofs whose participants are fully disjoint from covered:
// Only select proof if ALL its participants are new (disjoint from covered)
let is_disjoint = proof.participant_indices().all(|vid| !covered.contains(&vid));
if !is_disjoint || best_count == 0 {
break;
}Alternatively, restrict the greedy loop to select at most one proof and rely on compact_attestations to merge only from distinct, non-overlapping sources.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1085-1138
Comment:
**Greedy proof selection may pass overlapping participant sets to `aggregate_proofs`**
`extend_proofs_greedily` selects a proof as long as it covers at least one *new* validator (`best_count > 0`), but pushes the *original* proof — which may still include participants already covered by a previously selected proof. When two proofs for the same `AttestationData` overlap (e.g. local committee aggregation covered {1,2,3} and a received gossip proof covered {2,3,4}), both are selected and later passed as children to `aggregate_proofs` in `compact_attestations`. The `xmss_aggregate` call in `aggregate_proofs` receives child-pubkey lists `[pubkeys_for_{1,2,3}, pubkeys_for_{2,3,4}]` with validators 2 and 3 appearing in both children. Recursive aggregation of proofs with overlapping participants is likely unsupported by `xmss_aggregate`, which could produce an invalid proof that fails verification on peer nodes.
A straightforward fix is to restrict selection to proofs whose participants are fully disjoint from `covered`:
```rust
// Only select proof if ALL its participants are new (disjoint from covered)
let is_disjoint = proof.participant_indices().all(|vid| !covered.contains(&vid));
if !is_disjoint || best_count == 0 {
break;
}
```
Alternatively, restrict the greedy loop to select at most one proof and rely on `compact_attestations` to merge only from distinct, non-overlapping sources.
How can I resolve this? If you propose a fix, please make it concise.
crates/common/crypto/src/lib.rs
Outdated
| // Build references for xmss_aggregate: &[(&[XmssPublicKey], AggregatedXMSS)] | ||
| let children_refs: Vec<(&[LeanSigPubKey], AggregatedXMSS)> = deserialized | ||
| .iter() | ||
| .map(|(pks, agg)| (pks.as_slice(), agg.clone())) | ||
| .collect(); | ||
|
|
||
| // No raw XMSS signatures; purely recursive merge of existing proofs. | ||
| let (_sorted_pubkeys, aggregate) = xmss_aggregate(&children_refs, vec![], &message.0, slot, 2); |
There was a problem hiding this comment.
AggregatedXMSS::clone() per child proof
The children_refs step borrows deserialized by shared reference and then clones each AggregatedXMSS. Since proof data can be large (potentially hundreds of KB), this allocates one full proof copy per child. The clone can be eliminated by consuming deserialized and splitting it:
let (pks_list, aggs): (Vec<Vec<LeanSigPubKey>>, Vec<AggregatedXMSS>) =
deserialized.into_iter().unzip();
let children_refs: Vec<(&[LeanSigPubKey], AggregatedXMSS)> =
pks_list.iter().map(|v| v.as_slice()).zip(aggs).collect();This avoids the per-child clone while satisfying the borrow requirements of xmss_aggregate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/crypto/src/lib.rs
Line: 148-155
Comment:
**Avoidable `AggregatedXMSS::clone()` per child proof**
The `children_refs` step borrows `deserialized` by shared reference and then clones each `AggregatedXMSS`. Since proof data can be large (potentially hundreds of KB), this allocates one full proof copy per child. The clone can be eliminated by consuming `deserialized` and splitting it:
```rust
let (pks_list, aggs): (Vec<Vec<LeanSigPubKey>>, Vec<AggregatedXMSS>) =
deserialized.into_iter().unzip();
let children_refs: Vec<(&[LeanSigPubKey], AggregatedXMSS)> =
pks_list.iter().map(|v| v.as_slice()).zip(aggs).collect();
```
This avoids the per-child clone while satisfying the borrow requirements of `xmss_aggregate`.
How can I resolve this? If you propose a fix, please make it concise.| @@ -1229,9 +1233,10 @@ fn build_block( | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Inside the loop, aggregated_attestations.clone().try_into() is called and then process_slots + process_block are run on uncompacted data that may contain multiple entries per AttestationData. process_attestations in the state transition uses a per-validator boolean map keyed on target.root, so disjoint-participant duplicates are handled correctly, but the repeated full state-transition clones (head_state.clone() every iteration) are expensive. Consider accumulating just the newly added data roots and running a single speculative state transition after all data have been collected, rather than once per justification advance.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 1210-1234
Comment:
**Intermediate justification checks clone and run state transition on uncompacted attestations**
Inside the loop, `aggregated_attestations.clone().try_into()` is called and then `process_slots` + `process_block` are run on uncompacted data that may contain multiple entries per `AttestationData`. `process_attestations` in the state transition uses a per-validator boolean map keyed on `target.root`, so disjoint-participant duplicates are handled correctly, but the repeated full state-transition clones (`head_state.clone()` every iteration) are expensive. Consider accumulating just the newly added data roots and running a single speculative state transition after all data have been collected, rather than once per justification advance.
How can I resolve this? If you propose a fix, please make it concise.1 participant
This PR adds true attestation aggregation on the proposer block building pipeline. As part of this, the attestation byte cap we added was removed since we now have the 16 attestation+proof cap.