Skip to content

chore: bump leanVM to f66d4a9#411

Merged
MegaRedHand merged 1 commit into
mainfrom
bump-leanvm-f66d4a9
Jun 3, 2026
Merged

chore: bump leanVM to f66d4a9#411
MegaRedHand merged 1 commit into
mainfrom
bump-leanvm-f66d4a9

Conversation

@MegaRedHand
Copy link
Copy Markdown
Collaborator

Motivation

Track upstream leanVM at f66d4a9 (previously 5eba3b1).

Description

Two upstream API changes required adaptation in ethlambda-crypto:

Upstream change Adaptation
xmss_aggregate now returns Result<_, AggregationError> instead of panicking on invalid child proofs New AggregationError::Upstream variant, ? at the 3 call sites
AggregatedXMSS::serialize/deserialize renamed to compress/decompress (lz4+postcard) Renamed calls in serialize_aggregate, deserialize_children, and verification

Notes:

  • The proof wire format changed (lz4+postcard compression), so proofs only interop with clients on a compatible leanVM rev.
  • Plonky3 p3-* crates are pinned back to the previously locked 82cfad73: newer Plonky3 commits use the unstable maybe_uninit_slice feature, which doesn't build on Rust 1.92.0.

The 24 forkchoice_spectests failures (missing field proofData) are pre-existing on main (stale local fixtures) and unrelated to this bump.

Adapts to two upstream API changes:
- xmss_aggregate now returns Result with an upstream AggregationError;
  surfaced via a new AggregationError::Upstream variant instead of the
  previous panic on invalid child proofs
- AggregatedXMSS::serialize/deserialize renamed to compress/decompress
  (lz4+postcard proof wire format)

Plonky3 p3-* crates are pinned back to the previously locked 82cfad73
because newer Plonky3 commits require unstable features unavailable on
Rust 1.92.0.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

🤖 Kimi Code Review

The PR updates the leanVM dependency to revision f66d4a9 and adapts to its changed XMSS aggregation API. The changes are correct and improve error handling.

crates/common/crypto/src/lib.rs

Line 9: Importing AggregationError as LeanAggregationError is correct for disambiguation.

Lines 46-48: Adding the Upstream error variant with #[from] is idiomatic and properly propagates upstream failures without panicking.

Lines 110, 163, 193: Using ? on xmss_aggregate calls correctly handles the new fallible API. This eliminates the previous panic behavior documented in the removed # Panics section (lines 121-125), improving client robustness.

Lines 209, 218, 253: The rename from deserialize/serialize to decompress/compress matches the upstream leanVM API changes. The error handling (ok_or) for decompress returning Option is correct.

Cargo.lock

The lockfile changes include transitive dependency downgrades (e.g., itertools 0.13.00.12.1, windows-sys 0.61.20.52.0). Verify these are intentional constraints from the new leanVM revision and don't introduce security advisories or build issues on Windows targets.

Security Consideration

Changing aggregation from panicking to returning Result is a security improvement for a consensus client—invalid attestations or malicious network inputs can no longer trigger DoS via panics in the signature aggregation path.

Approval: Changes are correct, safe, and follow Rust best practices.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

🤖 Claude Code Review

Now I have everything needed to write a thorough review.


PR #411 Review: chore: bump leanVM to f66d4a9

Summary

This is a dependency bump with two API adaptations in ethlambda-crypto. The changes are mechanical but touch the signature aggregation path, which is consensus-critical.


Code Review

Error propagation (lib.rs:48, 110, 163, 193)

The new Upstream(#[from] LeanAggregationError) variant and ? propagation at all three xmss_aggregate call sites is correct. Replacing what was previously a panic with a Result return is strictly better — a node processing a block with an invalid child proof will now return an error rather than crash, which is the right behavior for consensus-critical code. The #[from] usage is appropriate because LeanAggregationError maps to exactly one variant.

compress/decompress rename (lib.rs:209, 218, 253)

Applied consistently at all four sites (serialize_aggregate, deserialize_children ×1, verify_aggregated_signature ×1). No missed call sites.

Validation ordering in aggregate_signatures (lib.rs:87–97)

CountMismatch is checked before EmptyInput. When both inputs are empty, 0 == 0 passes the count check, then EmptyInput fires correctly. Ordering is fine.

aggregate_mixed precondition (lib.rs:141)

if raw_public_keys.is_empty() && children.len() < 2 {

Correctly handles the lone-child case (1 child + 0 raw sigs = InsufficientChildren(1); 1 child + ≥1 raw sigs = allowed). No change here but it's still correct after the refactor.

deserialize_children returning Option (lib.rs:209–210)

let aggregate = AggregatedXMSS::decompress(proof_data.iter().as_slice())
    .ok_or(AggregationError::ChildDeserializationFailed(i))?;

This is correct. A None from decompress on a mismatched wire format produces ChildDeserializationFailed(i) with the index, which is useful for debugging. Same pattern for verify_aggregated_signature at line 253.


Wire Format Break

The PR description calls this out, but it's worth emphasizing for operators: the lz4+postcard compression change means all nodes must upgrade simultaneously. There is no version negotiation here — old-format proofs will fail decompress and return ChildDeserializationFailed / DeserializationFailed. This is a graceful failure rather than a crash or, worse, silent acceptance of garbage, which is correct behavior. No code change needed, but rolling upgrades across a heterogeneous devnet will break block import until all nodes are on a compatible leanVM rev.


Plonky3 pin (Cargo.lock)

Pinning p3-* back to 82cfad73 to avoid maybe_uninit_slice (requires nightly, unavailable on Rust 1.92.0) is the right call. This is a defensive move and documented in the PR notes.


New indirect dependencies (Cargo.lock)

objc2, objc2-foundation, system-info, and zk-alloc appear as new transitive dependencies. The objc2 crates are macOS-specific and should be behind target_os = "macos" in upstream; they'll be compiled out on Linux CI. zk-alloc is from the leanVM ZK stack. None of these are directly instantiated in ethlambda code, so the risk is bounded to supply chain — worth a quick check that they come from trusted upstream sources, but not a blocker for this PR.


Verdict

The changes are correct and minimal. No consensus logic was altered; the only behavior change is replacing panics with proper Result propagation, which is an improvement. The rename is applied consistently with no missed sites.

No blocking issues. The wire format break is expected and documented.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

🤖 Codex Code Review

  • High: crates/common/crypto/src/lib.rs, lib.rs, lib.rs switch the on-wire proof bytes from serialize/deserialize to compress/decompress, but AggregatedSignatureProof::proof_data remains an opaque SSZ ByteListMiB with no versioning or migration path (block.rs). That means proofs already buffered/stored locally or received from an older node become unreadable after upgrade, breaking both recursive aggregation and block import verification. I’d keep a legacy deserialize fallback on read, or add an explicit storage/network migration before changing the wire encoding.

No other repo-local correctness issues stood out in the diff.

I could not run a compile/test sanity check here because the new leanVM git dependency revision is not cached locally and network access is restricted in this environment.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 3, 2026

Greptile Summary

Tracks the leanVM upstream at commit f66d4a9 (from 5eba3b1) and adapts ethlambda-crypto to two breaking API changes in that bump.

  • xmss_aggregate now returns Result<_, AggregationError> instead of panicking; all three call sites have been updated with ?, and a new AggregationError::Upstream(#[from] LeanAggregationError) variant threads the upstream error type through correctly.
  • AggregatedXMSS::serialize/deserializecompress/decompress (lz4+postcard): all four call sites (serialize_aggregate, deserialize_children, verify_aggregated_signature) are updated. The wire-format change means proofs are not interoperable with nodes on the old revision, as documented in the PR description.

Confidence Score: 5/5

Safe to merge. All call sites are correctly updated, error propagation is properly wired, and the wire-format change is intentional and documented.

The adaptation is complete and consistent: all three xmss_aggregate call sites use ?, both compress/decompress renames are applied at every call site, and the new Upstream error variant correctly uses #[from] for automatic conversion. New transitive dependencies (zk-alloc, system-info, objc2-foundation) come from upstream leanVM and don't introduce any code changes in this repo.

No files require special attention.

Important Files Changed

Filename Overview
crates/common/crypto/src/lib.rs Adapts to two upstream API changes: xmss_aggregate now returns Result (handled with ? at all 3 call sites + new Upstream error variant), and serialize/deserialize renamed to compress/decompress (updated in serialize_aggregate, deserialize_children, and verify_aggregated_signature).
crates/common/crypto/Cargo.toml Bumps lean-multisig and leansig_wrapper git revisions from 5eba3b1 to f66d4a9, consistent with the Cargo.lock update.
Cargo.lock All leanVM crates updated to f66d4a9. Transitive additions include system-info, zk-alloc, include_dir, objc2-foundation, and several windows-sys downgrades. objc2/objc2-foundation appear as new transitive deps of rec_aggregation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["aggregate_signatures / aggregate_mixed / aggregate_proofs"] -->|"xmss_aggregate(...)?"|B{Result}
    B -->|Ok| C["serialize_aggregate\naggregate.compress()"]
    B -->|Err LeanAggregationError| D["AggregationError::Upstream"]
    C --> E["ByteListMiB (lz4+postcard wire format)"]
    E -->|"AggregatedXMSS::decompress()"| F["verify_aggregated_signature\ndeserialize_children"]
    F -->|"Ok(AggregatedXMSS)"| G["xmss_verify_aggregation"]
    F -->|"None"| H["DeserializationFailed / ChildDeserializationFailed"]
Loading

Reviews (1): Last reviewed commit: "chore: bump leanVM to f66d4a9" | Re-trigger Greptile

@MegaRedHand MegaRedHand merged commit 161245f into main Jun 3, 2026
7 of 8 checks passed
@MegaRedHand MegaRedHand deleted the bump-leanvm-f66d4a9 branch June 3, 2026 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants