Devnet4: separate attestation and proposal keys

[pablodeymo]

Motivation

leanSpec commit ad9a322 introduces the devnet4 spec: separate attestation and proposal keys. Each validator now has two independent XMSS keys — one for attestation signing and one for block proposal signing.

This solves a fundamental OTS (one-time signature) conflict: in devnet3, a proposer needed to sign both its attestation and the block using the same XMSS key in the same slot. With dual keys, the attestation key and proposal key advance independently, eliminating this constraint.

Additionally, the proposer's attestation is removed from the block structure entirely. The proposer now attests through the normal gossip pipeline at interval 1, like every other validator. This simplifies block processing and removes a circular fork-choice weight advantage where the proposer's attestation could influence its own block's weight.

Description

Commit 1: Upgrade leanSpec to ad9a322

Updates LEAN_SPEC_COMMIT_HASH in the Makefile. Fixture generation is blocked until leansig-test-keys publishes keys in the new dual-key format.

Commit 2: Implement devnet4 dual-key model

23 files changed, 326 insertions, 500 deletions — net reduction of 174 lines because the proposer attestation wrapper was removed.

Changes organized by area:

Core Types (crates/common/types/)

Change	Details
Validator struct	Single pubkey → attestation_pubkey + proposal_pubkey. New methods get_attestation_pubkey() and get_proposal_pubkey()
SignedBlockWithAttestation → SignedBlock	message is now Block directly (was BlockWithAttestation)
BlockWithAttestation	Deleted — proposer attestation no longer embedded in block
BlockSignaturesWithAttestation	Deleted — storage uses BlockSignatures directly
BlockSignatures.proposer_signature	Semantics changed: now signs the block root with the proposal key (was: signed attestation data with single key)
Genesis config format	GENESIS_VALIDATORS changed from list of hex strings to list of {attestation_pubkey, proposal_pubkey} objects

Key Manager (crates/blockchain/src/key_manager.rs)

• Introduced ValidatorKeyPair struct holding attestation_key and proposal_key
• KeyManager stores HashMap<u64, ValidatorKeyPair> instead of single keys
• Added sign_block_root() method that uses the proposal key
• sign_attestation() now internally routes to the attestation key

Block Proposal (crates/blockchain/src/lib.rs)

• propose_block(): signs block root with proposal key, no longer creates proposer attestation
• produce_attestations(): removed proposer skip — all validators (including proposer) attest at interval 1
• All SignedBlockWithAttestation → SignedBlock type cascade
• All .message.block.X → .message.X field access fixes

Store & Verification (crates/blockchain/src/store.rs)

• verify_signatures(): attestation proofs verified with get_attestation_pubkey(), proposer signature verified with get_proposal_pubkey() over block root
• on_block_core(): removed entire proposer attestation processing block (~40 lines) — no more gossip signature insert, no more dummy proof for proposer
• Removed StoreError::ProposerAttestationMismatch variant
• Gossip attestation/aggregation verification uses get_attestation_pubkey()

Storage (crates/storage/src/store.rs)

• write_signed_block(): destructures SignedBlock { message: block, signature } directly, stores BlockSignatures without wrapper
• get_signed_block(): deserializes BlockSignatures directly, reconstructs SignedBlock
• Table docs updated

Network (crates/net/)

• Type rename across all 6 network files (api, gossipsub handler/encoding, req_resp codec/handlers/messages)
• Field access .message.block.X → .message.X in gossip handler and req_resp handlers

Binary (bin/ethlambda/src/main.rs)

• AnnotatedValidator: now expects attestation_pubkey_hex, proposal_pubkey_hex, attestation_privkey_file, proposal_privkey_file
• read_validator_keys(): reads two key files per validator, constructs ValidatorKeyPair
• Checkpoint sync: validates both attestation_pubkey and proposal_pubkey match

Tests

• Unit tests updated and passing (34 tests across types, blockchain, checkpoint_sync)
• Spec test types updated to compile with new format
• Fork choice test BlockStepData: removed proposer_attestation field
• Signature test TestSignedBlock: simplified from TestSignedBlockWithAttestation

Blockers

• Test fixtures: leansig-test-keys has not published dual-key format keys yet. Fixture generation (make leanSpec/fixtures) fails with KeyError: 'attestation_public'. Spec tests (fork_choice, verify_signatures, stf) cannot run until keys are available.
• lean-quickstart: needs to be updated to generate devnet4 genesis configs with dual keys and dual key files per validator

Potential split

This PR could be split into smaller parts if preferred:

1. Types only — Validator dual pubkeys, SignedBlock rename, genesis config format (Phase 1)
2. Key manager + proposal flow — ValidatorKeyPair, sign_block_root, proposer attestation removal (Phase 2)
3. Store + verification — dual-key verification, remove proposer attestation from on_block_core (Phase 3)
4. Network + tests — type cascade through network layer, test harness updates (Phase 4+5)

Note: Phase 1 is SSZ-breaking, so Phases 2-4 won't compile independently without Phase 1.

How to Test

# Verify compilation
cargo check --workspace

# Run unit tests (spec tests blocked on fixtures)
cargo test -p ethlambda-types
cargo test -p ethlambda-blockchain --lib
cargo test -p ethlambda

# Lint
cargo clippy --workspace --all-targets -- -D warnings

Full devnet testing requires lean-quickstart devnet4 support.

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR introduces dual XMSS key pairs for validators (attestation + proposal keys) and removes the proposer attestation wrapper from blocks. The changes are substantial and touch consensus-critical code.

Critical Issues

1. Checkpoint sync validation incomplete (bin/ethlambda/src/checkpoint_sync.rs:147)

   • Only validates pubkey equality but doesn't verify the keys actually correspond to the expected validators
   • Missing validation that checkpoint state's validator set matches expected genesis validators

2. Key loading security (bin/ethlambda/src/main.rs:275-290)

   • unwrap_or_else with std::process::exit(1) is dangerous in production
   • Should use proper error handling instead of terminating the process

3. Missing proposal key usage in tests (crates/blockchain/src/key_manager.rs:135-150)

   • Tests only verify attestation signing, missing tests for sign_block_root

Security Concerns

1. Validator key isolation - The dual key approach is good, but ensure:

   • Attestation keys can't be used for block proposals
   • Proposal keys can't be used for attestations
   • Both keys advance their OTS counters independently

2. Genesis config validation - New genesis format needs validation that:

   • Attestation and proposal pubkeys are different for each validator
   • Keys are valid XMSS public keys

Performance Impact

1. Memory usage doubled - Each validator now stores 104 bytes of pubkey data instead of 52
2. Storage migration - Existing databases will need migration for new validator format

Code Quality Issues

1. Inconsistent naming (crates/common/types/src/state.rs:70-77)

   • get_attestation_pubkey() vs get_proposal_pubkey() is good
   • But consider renaming ValidatorPubkeyBytes to be more specific

2. Dead code (crates/blockchain/src/store.rs:549)

   • Parameter _local_validator_ids is unused after removing proposer attestation processing

3. Test fixtures outdated (crates/common/test-fixtures/src/lib.rs)

   • Still has single pubkey format in some test data

Recommendations

1. Add comprehensive tests for the new dual-key system
2. Implement proper error handling for key loading
3. Add migration path for existing databases
4. Update all test fixtures to use new format
5. Consider adding key rotation mechanism for compromised keys

The changes appear architecturally sound for the dual-key requirement, but need the above issues addressed before production use.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

1. High: storage format change can panic on existing DB data (no migration path)
   In crates/storage/src/store.rs:785, get_signed_block now decodes Table::BlockSignatures as BlockSignatures and uses expect("valid signatures").
   If a node has pre-PR data encoded as the old BlockSignaturesWithAttestation, this will panic at runtime. For consensus clients, panic-on-read during startup/sync is a serious availability risk.
   Suggestion: add versioning/migration, or graceful decode fallback (old || new) plus explicit DB upgrade handling.

2. Medium: unchecked slot narrowing in proposer signing
   In crates/blockchain/src/lib.rs:216, slot is cast with slot as u32 before signing block roots.
   This is a silent truncation if slot exceeds u32::MAX. Even if far-future, consensus code should avoid narrowing casts silently.
   Suggestion: use u32::try_from(slot) and handle error explicitly (same style as verification path).

3. Medium: regression test weakened for SSZ/genesis determinism
   In crates/common/types/src/genesis.rs:137, the pinned expected state/block roots were replaced by != ZERO checks.
   This removes a strong invariant test for SSZ layout/root stability and can let consensus-breaking serialization changes slip through.
   Suggestion: recompute and pin new expected roots for the dual-key layout.

4. Low: proposal signing metrics are mislabeled and incomplete
   In crates/blockchain/src/key_manager.rs:108, proposal signing uses time_pq_sig_attestation_signing() and does not increment a proposal-signature counter.
   This skews telemetry and makes operational debugging harder.

Overall, the dual-key separation (attestation vs proposal key) and proposal-signature verification over block root look directionally correct; main concerns are upgrade safety and test/guardrail regressions.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have a complete picture of the PR. Let me write the review.

---

PR Review: Devnet4 Dual-Key Model

Overall assessment: The implementation correctly tracks the spec change and the design is clean — removing BlockWithAttestation/BlockSignaturesWithAttestation simplifies the type hierarchy considerably. A few issues below need attention before merge.

---

Correctness Issues

Wrong metric used for proposal signing

key_manager.rs — sign_with_proposal_key() reuses metrics::time_pq_sig_attestation_signing() instead of a proposal-specific timing metric:

let _timing = metrics::time_pq_sig_attestation_signing();  // <-- wrong
key_pair.proposal_key.sign(slot, message) ...

Also, sign_with_attestation_key calls metrics::inc_pq_sig_attestation_signatures() after signing, but sign_with_proposal_key has no corresponding counter increment. This means proposal signatures inflate the attestation timing histogram but are not counted anywhere. Either add a separate time_pq_sig_proposal_signing() / inc_pq_sig_proposal_signatures(), or rename the existing ones to be key-agnostic (e.g., lean_pq_sig_signing_seconds with a key_type label).

---

Stale SSZ fixture in gossipsub encoding test

crates/net/p2p/src/gossipsub/encoding.rs:

let block_bytes = include_bytes!("../../test_data/signed_block_with_attestation.ssz");
let _block = SignedBlock::from_ssz_bytes(block_bytes).unwrap();

The binary fixture was captured from Zeam with the old SignedBlockWithAttestation SSZ layout (which included proposer_attestation inside the message). Decoding that blob as the new SignedBlock layout will very likely fail, since the message field no longer contains the attestation wrapper. The file also still carries the old name. This test will panic at runtime unless the fixture is updated or the test is temporarily disabled with a comment.

---

Weakened genesis root pinning test

crates/common/types/src/genesis.rs — state_from_genesis_root:

assert_ne!(root, crate::primitives::H256::ZERO, "state root should be non-zero");

The previous test pinned exact byte values for both state_root and block_root, which would catch any accidental SSZ layout change. The replacement only verifies non-zero, which would pass even if the Merkle tree is completely wrong. The comment acknowledges this is temporary pending fixture regeneration — that's fine — but it should include a // TODO: restore exact hash once devnet4 fixtures are available marker so it doesn't silently stay this way.

---

Design / API Issues

Unused parameter should be removed, not prefixed

crates/blockchain/src/store.rs:

fn on_block_core(
    store: &mut Store,
    signed_block: SignedBlock,
    verify: bool,
    _local_validator_ids: &[u64],   // <-- effectively dead
) -> Result<(), StoreError> {

All three call sites (on_block, on_block_without_verification, internal tests) can be updated to drop this argument. Keeping it as _local_validator_ids signals intent to revisit, but there is no corresponding // TODO comment and the entire subnet-selection code block is gone. If this is truly dead, remove the parameter; if it will return in a later devnet, add a TODO comment explaining why.

---

ValidatorKeyPair fields are public with no invariant enforcement

key_manager.rs:

pub struct ValidatorKeyPair {
    pub attestation_key: ValidatorSecretKey,
    pub proposal_key: ValidatorSecretKey,
}

The whole motivation of the dual-key model is to prevent a single XMSS key from being used for two different purposes in the same slot. There is no check preventing attestation_key and proposal_key from being set to the same value (or the same key material appearing in both roles). This is more of a defense-in-depth concern than a bug today, but at minimum a debug_assert! in KeyManager::new would document the invariant.

---

AnnotatedValidator pubkeys are parsed but silently discarded

bin/ethlambda/src/main.rs:

#[serde(rename = "attestation_pubkey_hex")]
#[serde(deserialize_with = "deser_pubkey_hex")]
_attestation_pubkey: ValidatorPubkeyBytes,

#[serde(rename = "proposal_pubkey_hex")]
#[serde(deserialize_with = "deser_pubkey_hex")]
_proposal_pubkey: ValidatorPubkeyBytes,

These bytes are deserialized correctly but never compared against the public key derived from the loaded secret key. The intent appears to be "parse to validate format", but if the key file and the hex string belong to different validators (misconfiguration) this will go undetected. A comparison of the pubkey derived from the secret key against the annotated hex value would catch config mismatches at startup — the same way checkpoint sync already validates attestation_pubkey and proposal_pubkey against state.

---

Minor Nits

• genesis.rs test constants: ATT_PUBKEY_C has the same hex string as PROP_PUBKEY_A. Not a bug (test data), but it can make the test hard to read. Using distinct dummy hex strings would be clearer.

• sign_block_root passes slot as u32 from the u64 call site in lib.rs. The expect("slot exceeds u32") in verify_signatures correctly guards the cast there; the cast in lib.rs is a silent truncation (slot as u32). Should be slot.try_into().expect(...) for consistency.

---

What Looks Good

• The type rename cascade (SignedBlockWithAttestation → SignedBlock, BlockWithAttestation removed) is thorough and consistent across all 6 network files, storage, and tests.
• Removing the proposer-attestation-in-block block (~40 lines in on_block_core) is correct — the old code had a subtle ordering constraint (fork choice update must precede proposer attestation insertion) that is now gone.
• All attestation verification paths correctly route to get_attestation_pubkey() and proposer signature verification correctly uses get_proposal_pubkey().
• The checkpoint sync validation (verify_checkpoint_state) correctly validates both keys.
• produce_attestations correctly removes the proposer skip — all validators including the proposer now attest at interval 1.
• resolve_path closure in read_validator_keys is a clean refactor.

---

Automated review by Claude (Anthropic) · custom prompt

[pablodeymo]

Splitting into 4 smaller PRs targeting devnet4 branch. See linked PRs below.