Devnet4 Phase 2: dual-key KeyManager and block proposal flow

[pablodeymo]

Motivation

Second of 4 PRs implementing devnet4. Introduces dual-key management and rewrites the block proposal flow to sign the block root with the proposal key instead of embedding a proposer attestation.

Depends on: #230 (Phase 1: Types)

Description

KeyManager: dual keys per validator

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

• sign_attestation() → uses attestation key
• New sign_block_root() → uses proposal key
• Each key advances OTS preparation independently

Block proposal: sign block root, no proposer attestation

Before: proposer creates attestation → signs attestation data → embeds in SignedBlockWithAttestation
After: proposer signs block root with proposal key → assembles SignedBlock. Proposer attests at interval 1 through normal gossip.

Key behavioral change: produce_attestations() no longer skips the proposer — all validators attest at interval 1.

Binary: dual key loading

AnnotatedValidator now expects:

• attestation_pubkey_hex / proposal_pubkey_hex
• attestation_privkey_file / proposal_privkey_file

read_validator_keys() reads two key files per validator and constructs ValidatorKeyPair.

Checkpoint sync

Validates both attestation_pubkey and proposal_pubkey match between downloaded state and local genesis config.

Files changed

• crates/blockchain/src/key_manager.rs — ValidatorKeyPair, sign_block_root
• crates/blockchain/src/lib.rs — proposal flow, attestation flow, type cascade
• bin/ethlambda/src/main.rs — dual key loading
• bin/ethlambda/src/checkpoint_sync.rs — dual pubkey validation

PR chain

1. Phase 1: Types (Devnet4 Phase 1: dual-key Validator, SignedBlock, genesis config format #230)
2. → Phase 2: Key manager + proposal (this PR)
3. Phase 3: Store + verification (`devnet4-phase3-store`)
4. Phase 4: Network + tests (`devnet4-phase4-network`)

How to Test

Requires Phase 1 merged first. Store and network still reference old types — fixed in Phase 3+4.

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR introduces dual XMSS key pairs for validators (separate attestation and proposal keys) to enable signing both attestations and block proposals in the same slot without violating OTS constraints. The changes are comprehensive and touch multiple components.

Issues Found

1. Metrics naming inconsistency (crates/blockchain/src/key_manager.rs:82 and crates/blockchain/src/key_manager.rs:111)

   • Both sign_with_attestation_key and sign_with_proposal_key use metrics::time_pq_sig_attestation_signing() for timing
   • The proposal key signing should have its own metric name (e.g., time_pq_sig_proposal_signing)

2. Missing proposal signature metrics (crates/blockchain/src/key_manager.rs:111)

   • Only attestation signatures are counted with metrics::inc_pq_sig_attestation_signatures()
   • Should add metrics::inc_pq_sig_proposal_signatures() for proposal signatures

3. Incomplete test coverage (crates/blockchain/src/key_manager.rs)

   • Tests only cover error cases (validator not found)
   • Missing tests for successful signing with both attestation and proposal keys

4. State root test regression (crates/common/types/src/genesis.rs:135-144)

   • The test now only checks for non-zero roots instead of the actual expected values
   • This creates a regression risk - the comment suggests this is temporary, but should be tracked

5. Potential deserialization issue (crates/common/types/src/genesis.rs:35-54)

   • The deser_pubkey_hex function is defined but never used in the genesis config
   • Genesis validators use GenesisValidatorEntry which has inline deserialization, making this function dead code

Security Considerations

• Key separation: The dual key approach is correctly implemented with separate keys for different purposes
• Path resolution: The validator key file path resolution in main.rs properly handles both relative and absolute paths
• Error handling: Good error handling with proper logging when key files can't be read or parsed

Performance Impact

• Memory usage: Doubles the memory footprint for validator keys (now storing two keys per validator)
• Storage: SSZ encoding of validators increases by 52 bytes per validator (additional pubkey)
• No performance regressions in signing operations as both keys are used independently

Code Quality

• Good separation of concerns: Clear distinction between attestation and proposal key usage
• Consistent naming: The attestation_pubkey/proposal_pubkey naming is consistent across the codebase
• Backward compatibility: Breaking changes are handled appropriately with updated test fixtures

Recommendations

1. Add the missing proposal signature metrics
2. Fix the metrics naming inconsistency
3. Add comprehensive tests for successful signing operations
4. Remove the unused deser_pubkey_hex function in genesis.rs
5. Consider adding a TODO/FIXME comment for the state root test regression

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Claude Code Review

PR Review: Devnet4 Phase 2 — Dual-key KeyManager and Block Proposal Flow

The overall design is clean and the refactoring is well-scoped. The transition from embedded proposer attestation to a signed block root is architecturally sound. A few concrete issues below.

---

Bug: Wrong metric timer in sign_with_proposal_key

crates/blockchain/src/key_manager.rs, ~line 306

fn sign_with_proposal_key(...) {
    let signature: ValidatorSignature = {
        let _timing = metrics::time_pq_sig_attestation_signing();  // ← wrong
        ...
    };

sign_with_proposal_key uses time_pq_sig_attestation_signing(). This pollutes attestation signing latency metrics with proposal signing time. A separate time_pq_sig_proposal_signing() timer should be used, or at minimum this should be renamed/shared correctly.

---

Bug: Missing metric counter in sign_with_proposal_key

crates/blockchain/src/key_manager.rs, ~line 313

sign_with_attestation_key calls metrics::inc_pq_sig_attestation_signatures() after signing, but sign_with_proposal_key has no equivalent counter. Block proposal signatures will go uncounted, making total signature throughput metrics inaccurate.

---

Pinned state root test weakened without a tracking issue

crates/common/types/src/genesis.rs, ~line 833

The state_from_genesis_root test previously pinned exact state root and block root hashes — a key regression guard for SSZ layout changes. It was replaced with:

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

The comment acknowledges this ("Will be recomputed once we can run this test"), but there's no tracking issue or follow-up PR reference. Until this is restored, silent SSZ layout regressions in Validator or State will not be caught by the test suite. This should be recomputed and pinned before the chain merges.

---

No cross-validation of pubkey vs privkey in AnnotatedValidator

bin/ethlambda/src/main.rs, ~line 65

_attestation_pubkey: ValidatorPubkeyBytes,
_proposal_pubkey: ValidatorPubkeyBytes,

Both public key fields are deserialized (triggering hex/length validation) but then discarded with _ prefixes — never compared against the public key derived from the loaded private key file. A misconfigured node with mismatched attestation_pubkey_hex/attestation_privkey_file pairs would start silently, sign with the wrong key, and produce signatures that fail remote verification.

This was a pre-existing issue with the single-key design, but dual-key config doubles the surface area. Either cross-check the derived pubkey or drop the deserialization entirely (if there's no intent to ever validate it). The misleading field names suggest there was some original intent to verify.

---

Missing test case: partial key mismatch in checkpoint sync

bin/ethlambda/src/checkpoint_sync.rs, ~line 230

The new ||-based check correctly rejects validators where either key mismatches, but the test suite only covers full-mismatch (both keys differ). Adding a test where attestation_pubkey matches but proposal_pubkey differs (and vice versa) would confirm the || logic is exercised for both fields independently.

---

Minor: slot as u32 cast in propose_block

crates/blockchain/src/lib.rs, ~line 430

.sign_block_root(validator_id, slot as u32, &block_root)

Same pattern as sign_attestation. For sufficiently large slot numbers (> u32::MAX ≈ 136 years at 4s/slot) this silently truncates. Pre-existing pattern, but worth noting for long-term correctness.

---

Nit: duplicate key values in test constants

crates/common/types/src/genesis.rs, ~line 741

const ATT_PUBKEY_C: &str = "b7b0f...";  // same bytes as PROP_PUBKEY_A

ATT_PUBKEY_C and PROP_PUBKEY_A share the same hex string (both are the old PUBKEY_B). This is likely intentional test data reuse, but the naming implies they're different keys — consider using distinct values or adding a comment.

---

What looks good

• The ValidatorKeyPair struct is clean and the routing to sign_with_attestation_key / sign_with_proposal_key is clear.
• Removing num_validators and the proposer-skip logic from produce_attestations simplifies the code correctly.
• The resolve_path + load_key closures in read_validator_keys are a readable improvement over the previous inline blocks.
• Checkpoint sync correctly validates both keys with a proper || condition.
• The BlockWithAttestation and BlockSignaturesWithAttestation removals are clean with no dead code left behind.

---

Automated review by Claude (Anthropic) · custom prompt

[github-actions]

🤖 Codex Code Review

Findings

1. Critical: API mismatch breaks block/signature verification path

• SignedBlock was changed to carry Block (no proposer_attestation) and Validator::get_pubkey() was removed, but verification code still uses both old APIs in store logic.
• References:
  • crates/common/types/src/block.rs:16
  • crates/common/types/src/state.rs:88
  • crates/blockchain/src/store.rs:1253
  • crates/blockchain/src/store.rs:1271
  • crates/blockchain/src/store.rs:173
• Why this matters: current verification still binds proposer signature to attestation data/pubkey flow, while producer now signs block root with proposal key. This is consensus-critical and should be updated atomically.

2. High: consensus-regression test weakened

• state_from_genesis_root replaced pinned hash assertions with only != ZERO.
• Reference: crates/common/types/src/genesis.rs:137
• Why this matters: SSZ/state-root drift can now slip through unnoticed. For consensus code, deterministic root pinning is essential. Recompute expected roots and keep strict equality checks.

3. Medium: wrong metrics path for proposal signing

• Proposal signing uses time_pq_sig_attestation_signing() and does not increment a proposal-signature counter.
• Reference: crates/blockchain/src/key_manager.rs:107
• Why this matters: operational/perf telemetry becomes misleading, making signature-path regressions harder to detect.

4. Medium: unchecked u64 -> u32 slot cast in proposer signing

• slot as u32 in proposer signing can truncate silently.
• Reference: crates/blockchain/src/lib.rs:216
• Why this matters: for XMSS OTS indexing, silent truncation is unsafe. Prefer try_into() with explicit error handling.

Overall direction (dual attestation/proposal keys, proposer signing block root) is good, but Item 1 and Item 2 should be fixed before merge.

---

Automated review by OpenAI Codex · custom prompt