fix(crypto): key a proof's signer set on (slot, message), not slot

[MegaRedHand]

Closes #619.

wire_keys builds the signer set a Type-2 proof is decoded and verified against. It merged claims by epoch alone and returned ConflictingMessages when a second claim at that slot carried a different message. It sits on the block import path, so a block holding two distinct AttestationData at one slot was rejected before leanVM ever saw it.

That is an ordinary block. Validators attest moments apart and can see justification advance in between, so disagreeing inside a slot is routine rather than equivocation: on an 11-hour gean-only run, 53% of slots carried more than one distinct AttestationData. On the mixed devnet in #619 a single gean block was enough. All six ethlambda nodes rejected it, orphaned everything built on it, re-fetched and re-rejected it, and the network split permanently with both sides frozen at the same finalized slot.

Why the restriction had no owner

layer	keys groups by	allows two messages at one slot
leanVM, since 48a90420	(epoch, message)	yes
this crate's build path (merge_type_1s_into_type_2 → one_group per claim)	(epoch, message)	yes
this crate's verify path (wire_keys)	epoch	no
leanSpec block_production.py / fork_choice.py	whole AttestationData	yes, and fork choice handles the tie

So the crate could build proofs its own verifier rejects. It had not surfaced only because no ethlambda proposer emitted such a block during the run.

The one-message-per-slot rule that genuinely exists is per key (xmss/interface.py:188: a secret key must never sign two different messages for one slot). Two validators with different keys signing different messages at one slot reuses nothing.

The change

Key the groups on the pair, in the match and in the sort:

.find(|group| group.epoch == component.slot && group.message == component.message.0)
...
groups.sort_unstable_by_key(|group| (group.epoch, group.message));

wire_keys becomes infallible, so ConflictingMessages and its two #[from] variants go with it. gean makes the same two edits in signature_claims (xmss/rust/multisig-glue/src/lib.rs), and both implementations order [u8; 32] lexicographically, so the digests agree.

The sort is the load-bearing half. With two groups at one slot, ordering on the epoch alone leaves them in the caller's order, which is block-body order and sorted by nothing.

Tests

wire_keys_keeps_two_messages_at_one_slot (fast, runs in CI) pins the whole shape: input is deliberately unsorted on both slot and message, and it asserts the (slot, message) sequence and which keys landed in which group.

test_type_2_two_messages_at_one_slot_round_trip (#[ignore], real XMSS and real proving, ~35s) is the interop case end to end: two validators, one slot, two messages, merged into a Type-2 and verified. It hands the claims over with the larger message first, so only a correct sort can reproduce leanVM's canonical order.

Verified against the three variants:

variant	result
before this PR	unit test fails, ConflictingMessages { slot: 6 }
match fixed, sort left on epoch alone	round-trip fails, DeserializationFailed on a valid proof
this PR	both pass, and test_type_2_merge_verify_split_round_trip is unregressed

The middle row is why the sort gets its own comment: leanVM turns a mis-ordered signer set away at decode, so a missed sort reads as a corrupt proof rather than a wrong signer set.

wire_keys_rejects_two_messages_at_one_slot asserted the removed behaviour and is replaced. wire_keys_groups_by_slot_and_sorts still passes unchanged (its two slot-9 entries share a message); it is renamed to wire_keys_unions_keys_sharing_a_group_and_sorts, since what it actually covers is key-unioning and ordering.

Module docs and the Aggregation shape section of CLAUDE.md said a slot carries one message. Both updated.

Related

• Can an XMSS aggregate carry two messages at one epoch? (claims keyed (epoch, key), unlike SPHINCS) leanEthereum/leanVM#278, the upstream change that lifted the constraint
• Blocks may carry two AttestationData for one slot, which no leanVM proof can express leanEthereum/leanSpec#1209, open: whether the spec should state this explicitly

[github-actions]

🤖 Kimi Code Review

I'll review this PR which changes the aggregation grouping from per-slot to per-(epoch, message) pair, allowing multiple messages at a single slot.

Security & Correctness

Critical: merge_type_1s_into_type_2 may produce invalid proofs (crates/common/crypto/src/lib.rs)

The function at line ~480 still uses BTreeMap<u32, Vec<usize>> keyed by slot only:

let mut by_slot: BTreeMap<u32, Vec<usize>> = BTreeMap::new();
// ...
by_slot.entry(components[i].slot).or_default().push(i);

Bug: When two type-1 proofs at the same slot have different messages, this merges them under a single slot key, then iterates over by_slot.values() which gives arbitrary message ordering (hash map iteration order of the underlying IndexMap in BTreeMap's values). The resulting type-2 proof will have groups in an unpredictable order, causing verification failures.

Fix: Change to BTreeMap<(u32, H256), Vec<usize>> or similar, keyed by (slot, message).

split_type_2_by_message comment is misleading (line ~555)

// Groups are keyed on `(epoch, message)`, so a message that appears at all
// appears in exactly one group unless two slots signed the very same bytes.

This comment is now incorrect. The same message can appear at multiple different slots — the uniqueness is per (epoch, message) pair, not per message. The old comment about "one group per slot" was actually closer to the uniqueness property; the new comment conflates message uniqueness with pair uniqueness.

Performance

wire_keys allocation pattern (line ~168)

let mut groups: Vec<XmssClaimGroup> = Vec::with_capacity(components.len());

The capacity hint is now pessimistic. With multiple messages per slot, components.len() is closer to the actual group count, but still potentially over-allocates. Minor issue.

sort_unstable_by_key with (u32, [u8; 32]) (line ~204)

This allocates a tuple per comparison. For large signer sets, prefer:

groups.sort_unstable_by(|a, b| a.epoch.cmp(&b.epoch).then_with(|| a.message.cmp(&b.message)));

Rust Idioms

H256 field access (line ~662, ~689)

assert_eq!(shape, vec![(6, msg_low.0), (6, msg_high.0), (8, msg_low.0)]);

Direct .0 access on H256 is fragile. Use *msg_low or msg_low.into() for clarity, or ensure H256 has a proper accessor. This pattern repeats in tests.

wire_keys return type change

Good: Removing Result wrapper where error case no longer exists is correct simplification.

Consensus-Critical Concerns

Fork choice / attestation validity

The CLAUDE.md and code comments correctly note that validators "attest moments apart and justification can advance in between." This is correct for Ethereum consensus — attestations at the same slot with different AttestationData (due to different head/root views) are not slashable as equivocations because they share the same slot but different beacon block roots.

However, ensure this interacts correctly with:

• Slashing protection: The PR removes ConflictingMessages as an error, but slashing conditions in consensus still require double-vote detection (same validator, same slot, different attestation data where data is (target, source, head)). This is a database/validator client concern, not this crypto code, but verify no slashing guard was removed upstream.

XMSS one-time signature safety (line ~912 in test comment)

// Two keys, so neither signs twice at one slot: XMSS one-time-signature
// safety is per key, and two validators disagreeing reuses nothing.

This comment is correct but incomplete. The safety property is per (key, message) in XMSS with WOTS+? Verify that leanVM's XMSS implementation doesn't have additional constraints on key reuse across different messages at the same epoch. The comment should reference the specific XMSS variant's security assumption.

Testing

test_type_2_two_messages_at_one_slot_round_trip (line ~896)

Good: This is the critical interop test. However, it's marked #[ignore = "too slow"]. This is dangerous — the exact bug this PR fixes (decode failure on valid proofs with multiple messages per slot) will not be caught in CI.

Recommendation:

1. Add a fast unit test that verifies wire_keys sorting without full proof generation
2. Consider running ignored tests in a slower CI job, or use a smaller XMSS parameter set for this test

Missing test: merge_type_1s_into_type_2 with same-slot-different-messages

No test covers the merge function with this scenario, which is where the BTreeMap<u32, ...> bug manifests.

Documentation

CLAUDE.md line ~287

- **A slot can carry several messages.** Validators attesting moments apart
  disagree within a slot, so two distinct `AttestationData` at one slot is
  ordinary rather than equivocation and a block routinely carries both.

This is correct. Minor grammar: "is ordinary" → "are ordinary" (plural subject "two distinct AttestationData").

Summary

Priority	Issue	Location
Critical	merge_type_1s_into_type_2 uses slot-only key, breaking multi-message-per-slot merges	crates/common/crypto/src/lib.rs ~line 480, by_slot
High	#[ignore] on only interop test; add fast unit test for merge path	test at line ~896
Medium	split_type_2_by_message comment incorrectly states message uniqueness	line ~555
Low	sort_unstable_by_key tuple allocation	line ~204
Low	H256.0 direct field access in tests	lines ~662, ~689

---

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

[github-actions]

🤖 Codex Code Review

1. split_type_2_by_message still identifies the target group by message alone, even though this PR changes the canonical grouping key to (slot, message). See crates/common/crypto/src/lib.rs. Today’s only caller passes attestation roots, so it happens to be safe because AttestationData already commits to slot, but the public API is now looser than the underlying model. A future caller that legitimately reuses the same 32-byte message in two slots will get MultipleMessages for a valid aggregate. I’d tighten the API to split by (slot, message) or by the full SignerSet identity.

2. The public docs/comments around type-2 merging still describe the pre-PR invariant, which now contradicts the implementation. In particular, crates/common/crypto/src/lib.rs still says claims are “grouped by slot” and that two messages at one slot “cannot be merged”, and crates/common/crypto/src/lib.rs still lists “a slot carrying two messages” as a malformed request. That is a maintainability risk for consensus code: future callers may preserve obsolete guards or mis-handle leanVM errors based on stale contract docs.

Beyond that, the actual wire_keys change looks sound: sorting by (epoch, message) matches the new leanVM requirement, and I don’t see a fork-choice / attestation-processing regression in the touched logic.

I could not run cargo test in this sandbox because cargo needs writable/fetchable dependency state for leanVM.

---

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

[github-actions]

🤖 Claude Code Review

Review: PR 620 — key a proof's signer set on (slot, message)

Summary: This correctly fixes a real network-splitting bug: wire_keys grouped claims by epoch alone and rejected two distinct AttestationData at one slot, while leanVM's prover and this crate's build path (merge_type_1s_into_type_2) already allowed it via (epoch, message) grouping. The fix — matching and sorting on the (epoch, message) pair instead of epoch alone — is minimal, correct, and well-tested.

Strengths

• The core fix (wire_keys in crates/common/crypto/src/lib.rs:180-209) is exactly right: matching on (slot, message) and sorting on the same pair. The commit message correctly identifies that the sort is the load-bearing half — a decode failure on a valid proof is a much subtler bug than an outright rejection, since it manifests as DeserializationFailed rather than a clear signal.
• ConflictingMessages and its two #[from] error variants are cleanly removed with no dangling references or exhaustive matches left behind (verified — no code outside this file matched on those variants).
• Test coverage is strong: wire_keys_keeps_two_messages_at_one_slot pins the exact shape/ordering with deliberately unsorted input, and test_type_2_two_messages_at_one_slot_round_trip is a genuine end-to-end interop test (real XMSS, real proving) that specifically exercises the "sort on epoch alone" failure mode by handing claims in reverse canonical order.
• Module docs and CLAUDE.md's "Aggregation shape" section are updated to match the new invariant.

Finding

crates/common/crypto/src/lib.rs:464-468 — The doc comment on merge_type_1s_into_type_2 is now stale and contradicts the fix:

/// The returned blob is the `to_bytes_without_pubkeys()` form of the merged
/// aggregate, whose signer set is the union of the claims grouped by slot. A
/// verifier decoding it back needs the same claims, in any order.
///
/// Two claims at one slot under different messages cannot be merged at all:
/// leanVM rejects the pair rather than producing a proof (see the module docs).
pub fn merge_type_1s_into_type_2(

This block wasn't touched by the diff, but it directly contradicts what the PR just proved: test_type_2_two_messages_at_one_slot_round_trip (line 895) successfully merges two claims at one slot under different messages through this exact function, and the PR description itself states "The build path has always allowed it, since it hands leanVM one claim at a time." A future reader trusting this comment could reintroduce the restriction the rest of the PR just removed, or waste time debugging on a false premise. Worth updating in this PR since it's the same misconception the rest of the diff fixes elsewhere.

Other notes

• wire_keys's linear find per component is unchanged in complexity (still O(n²) in the number of claims) — fine given the bounded per-block attestation count, not a regression from this PR.
• Confirmed callers (store.rs:1251, block_builder.rs:1016, reaggregate.rs:145) all build one SignerSet per attestation independently, so the two-messages-per-slot scenario is real and reachable in normal operation (not just theoretical), matching the PR's devnet incident description.

---

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