Skip to content

Rename epoch to slot in XMSS signature API to align with leanSpec#184

Merged
MegaRedHand merged 3 commits intomainfrom
rename-epoch-to-slot
Mar 5, 2026
Merged

Rename epoch to slot in XMSS signature API to align with leanSpec#184
MegaRedHand merged 3 commits intomainfrom
rename-epoch-to-slot

Conversation

@pablodeymo
Copy link
Collaborator

Motivation

The leanSpec reference implementation renamed the XMSS signing/verification parameter from epoch to slot (commit 3b9d054, part of PR #412).

In ethlambda, the actual value passed to this parameter is always a slot number (e.g., attestation.data.slot as u32), so the epoch naming was misleading — in Ethereum consensus, an "epoch" typically means 32 slots, but leanConsensus has no epoch concept at all. The XMSS scheme uses this parameter as a one-time signature index to prevent key reuse, and leanConsensus uses the slot number for this purpose.

This is part of the post-devnet-3 alignment work tracked in #155 (pq-devnet-4 preparation).

Description

Pure parameter rename: epochslot across production code, documentation, and tests. No behavioral or wire-format change — the leansig crate uses positional arguments, so the external dependency is unaffected.

Changes by file

File Scope Changes
crates/common/types/src/signature.rs is_valid(), sign() Parameter + doc rename
crates/common/crypto/src/lib.rs aggregate_signatures(), verify_aggregated_signature() Parameter + doc rename
crates/common/crypto/src/lib.rs Unit tests + cross-client ream tests Variable, constant (REAM_EPOCHREAM_SLOT), test name, comment, and assert message renames
crates/blockchain/src/key_manager.rs sign_attestation(), sign_message() Parameter + doc + local variable rename
crates/blockchain/src/store.rs verify_signature(), verify_aggregated_attestation(), block verification Local variable rename at 4 call sites

What was NOT renamed

  • activation_epoch variables in test helpers — these are parameters to generate_keypair_and_sign() for leansig keypair setup, not part of our signing/verification API.
  • leansig internals — the external crate's parameter names are unchanged; we only pass our renamed variable positionally.

How to test

cargo check --workspace                      # Compiles clean
cargo clippy --workspace -- -D warnings      # No warnings
cargo test --workspace --release             # All 92 tests pass (5 pre-existing ignores)

Related issues

  The parameter named epoch actually receives a slot number everywhere.
  The leanSpec renamed this parameter from epoch to slot to reflect what
  it actually is — leanConsensus has no epoch concept unlike the Beacon
  Chain. Pure rename across production code, docs, and tests. No
  behavioral change.
@github-actions
Copy link

github-actions bot commented Mar 5, 2026

🤖 Kimi Code Review

Review Summary

This PR correctly fixes a naming inconsistency by replacing epoch with slot in XMSS signature contexts. The change is accurate because XMSS uses slot numbers (not epochs) for one-time signature indexing.

Issues Found

1. Critical: Potential panic in production code (Multiple locations)

  • Files: crates/blockchain/src/store.rs
  • Lines: 375, 438, 1163, 1204
  • Issue: Using expect("slot exceeds u32") on user-provided input can panic. This is a DoS vector as malicious actors could craft attestations with slots > u32::MAX.

Fix:

let slot: u32 = attestation.data.slot.try_into()
    .map_err(|_| StoreError::InvalidSlot(attestation.data.slot))?;

2. Minor: Incomplete variable rename (Line 1176)

  • File: crates/blockchain/src/store.rs
  • Line: 1176
  • Issue: Variable epoch was renamed to slot in the function signature but the local binding at line 1176 still uses epoch.

Fix:

let slot: u32 = attestation.data.slot.try_into().expect("slot exceeds u32");

3. Documentation consistency

  • File: crates/common/crypto/src/lib.rs
  • Lines: 65, 73, 138
  • Issue: Comments still mention "epoch" in a few places where "slot" would be more accurate.

Fix: Update comments to consistently use "slot" terminology.

Security Assessment

  • The variable rename itself is safe and correct
  • The expect calls are the only security concern, creating potential panic vectors
  • All cryptographic operations maintain their security properties

Performance Impact

  • No performance impact - this is purely a semantic rename
  • No additional allocations or computational overhead

Overall

The PR correctly addresses the naming inconsistency. The only blocking issue is the use of expect on untrusted input, which should be replaced with proper error handling before merge.


Automated review by Kimi (Moonshot AI) · custom prompt

@github-actions
Copy link

github-actions bot commented Mar 5, 2026

🤖 Codex Code Review

  1. Potential XMSS index truncation (correctness/security): in crates/blockchain/src/key_manager.rs:75, attestation_data.slot as u32 is a lossy cast.
    If slot > u32::MAX, this wraps and can reuse/shift XMSS signing indices, which is unsafe for one-time signatures.
    Suggested change: use try_into() and return a typed KeyManagerError instead of truncating.

  2. Panic on untrusted input in gossip/block validation paths (DoS risk): several conversions use .expect("slot exceeds u32") on slot values from network-processed objects:

A malformed/high slot can panic the process. Suggested change: map conversion failures to StoreError (e.g., InvalidSlotRange) and reject gracefully.

Overall, the PR is mostly a semantic rename (epoch -> slot) and improves naming clarity; I don’t see fork-choice/finalization/transition logic changes in this diff.


Automated review by OpenAI Codex · custom prompt

@github-actions
Copy link

github-actions bot commented Mar 5, 2026

🤖 Claude Code Review

PR 184 Review: Rename epoch to slot in XMSS Signature API

Summary: Pure parameter rename across 4 files — no behavioral changes, no wire-format impact. The external leansig crate uses positional arguments, so this is entirely cosmetic at the ABI level.


Correctness

The rename is complete and consistent:

  • ValidatorSignature::is_valid() and ValidatorSecretKey::sign() in signature.rs
  • aggregate_signatures() and verify_aggregated_signature() in crypto/src/lib.rs
  • sign_attestation() and sign_message() in key_manager.rs
  • All 4 call sites in store.rs (on_gossip_attestation, on_gossip_aggregated_attestation, verify_signatures ×2)

Intentional non-renames (correctly left alone): activation_epoch parameters in test helpers — these are semantically different (keypair generation index for leansig, not the signing slot). The PR description calls this out explicitly, which is good.

Documentation and Test Quality

  • All /// doc comments updated in lock-step with parameter renames.
  • Test function names updated (test_verify_wrong_epoch_failstest_verify_wrong_slot_fails, test_cross_client_ream_signature_rejects_wrong_slot).
  • Cross-client test constant REAM_EPOCHREAM_SLOT with accompanying comment update.
  • Assert messages updated ("Verification should have failed with wrong epoch" → "...wrong slot").

No Issues Found

The change is minimal, well-scoped, and accurately documents why the rename is correct (leanConsensus has no epoch concept — the XMSS one-time-use index is a slot number). The alignment with leanSpec commit 3b9d054 is a valid motivation.

Verdict: Looks good. Approve.


Automated review by Claude (Anthropic) · custom prompt

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 5, 2026

Greptile Summary

This PR is a pure naming alignment refactor, renaming the epoch parameter to slot across the XMSS signing/verification API (ValidatorSignature::is_valid, ValidatorSecretKey::sign, aggregate_signatures, verify_aggregated_signature) and all their call sites in key_manager.rs and store.rs. The change aligns ethlambda with leanSpec's updated terminology and removes the misleading implication that the parameter represents an Ethereum consensus epoch (32 slots) rather than a slot number used as a one-time-signature index.

Key observations:

  • No behavioral or wire-format change — the underlying leansig crate receives the same value positionally; only local variable and parameter names change.
  • All four production call sites in store.rs are updated, as are the public API signatures in signature.rs and crypto/src/lib.rs.
  • The test helper generate_keypair_and_sign in crates/common/crypto/src/lib.rs retains the old signing_epoch parameter name (line 179), creating a minor inconsistency since every call site now passes a variable named slot.
  • Cross-client test vectors (REAM_SLOT, assert messages) are correctly updated throughout.

Confidence Score: 4/5

  • Safe to merge — purely a local rename with no behavioral or wire-format impact, though one minor inconsistency remains in test helper naming.
  • The PR achieves its goal of aligning with leanSpec terminology. All production API surfaces and call sites are consistently updated. The only gap is the signing_epoch parameter name inside a private test helper function (generate_keypair_and_sign), which has no effect on test correctness or functionality since the value is passed positionally. However, this inconsistency undermines the clarity goal of the rename and should be fixed for completeness.
  • crates/common/crypto/src/lib.rs — test helper parameter name should be aligned with the renamed API terminology.

Important Files Changed

Filename Overview
crates/common/types/src/signature.rs Pure parameter rename of epochslot in ValidatorSignature::is_valid (line 44) and ValidatorSecretKey::sign (line 88). No logic changes. Clean and complete.
crates/common/crypto/src/lib.rs Rename is complete in production API (aggregate_signatures line 83, verify_aggregated_signature line 139) and most tests, but the test helper generate_keypair_and_sign retains the signing_epoch parameter name (lines 179, 190), creating a minor inconsistency with all its call sites that now pass a slot variable.
crates/blockchain/src/key_manager.rs Rename is complete and consistent across sign_attestation (line 75), sign_message (lines 84-95), and internal call chain (line 104). No issues.
crates/blockchain/src/store.rs All four epoch: u32 local variables renamed to slot: u32 at their respective call sites (line 172, 375-378, 440-446, 1163-1180). All production verification call sites use the renamed slot parameter correctly. Clean.

Sequence Diagram

sequenceDiagram
    participant Store as store.rs
    participant KM as key_manager.rs
    participant Crypto as crypto/lib.rs
    participant Sig as signature.rs
    participant Leansig as leansig crate

    Note over Store,Leansig: Signing flow
    KM->>KM: sign_attestation(validator_id, attestation_data)
    KM->>KM: slot = attestation_data.slot as u32
    KM->>Sig: secret_key.sign(slot, message_hash)
    Sig->>Leansig: LeanSignatureScheme::sign(&sk, slot, message)
    Leansig-->>Sig: LeanSigSignature
    Sig-->>KM: ValidatorSignature
    KM-->>Store: XmssSignature

    Note over Store,Leansig: Individual verification flow
    Store->>Store: slot = attestation.data.slot.try_into()
    Store->>Sig: signature.is_valid(&pubkey, slot, &data_root)
    Sig->>Leansig: LeanSignatureScheme::verify(&pk, slot, message, &sig)
    Leansig-->>Sig: bool
    Sig-->>Store: bool

    Note over Store,Leansig: Aggregated verification flow
    Store->>Crypto: verify_aggregated_signature(&proof, pubkeys, &data_root, slot)
    Crypto->>Leansig: xmss_verify_aggregated_signatures(&pks, message, &aggregate, slot)
    Leansig-->>Crypto: Result
    Crypto-->>Store: Result
Loading

Comments Outside Diff (1)

  1. crates/common/crypto/src/lib.rs, line 179-190 (link)

    Incomplete rename in test helper

    The generate_keypair_and_sign helper's parameter is still named signing_epoch (line 179) and is used as-is on line 190. However, all call sites now pass a variable named slot (e.g., lines 218, 252, 279, 301), creating a mismatch in terminology.

    Since the entire purpose of this PR is to rename epochslot for consistency, consider renaming this parameter as well:

    And on line 190:

    This ensures the test helper's signature is consistent with the renamed API terminology throughout the codebase.

Last reviewed commit: cfd6160

@MegaRedHand MegaRedHand merged commit 540200a into main Mar 5, 2026
2 checks passed
@MegaRedHand MegaRedHand deleted the rename-epoch-to-slot branch March 5, 2026 21:34
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