Devnet4 Phase 3: dual-key verification and remove proposer attestation from store

[pablodeymo]

Motivation

Third of 4 PRs implementing devnet4. Updates signature verification to use dual keys and removes all proposer attestation handling from block processing.

Depends on: #230 (Phase 1), #231 (Phase 2)

Description

Signature verification: dual keys

• Attestation proofs: verified with `get_attestation_pubkey()` (was `get_pubkey()`)
• Proposer signature: verified with `get_proposal_pubkey()` over block root (was: over attestation data with single pubkey)
• Gossip attestation/aggregation verification: `get_attestation_pubkey()`

on_block_core: remove proposer attestation processing

Removed ~40 lines of proposer attestation handling:

• No more extracting proposer attestation from block
• No more inserting proposer gossip signature
• No more dummy proof for proposer in skip-verification mode
• No more circular weight advantage prevention (no longer needed)
• Removed `StoreError::ProposerAttestationMismatch` variant

Storage layer

• `write_signed_block()`: destructures `SignedBlock { message: block, signature }` directly
• `get_signed_block()`: deserializes `BlockSignatures` directly (no wrapper)
• Table docs updated: `BlockSignatures: H256 -> BlockSignatures`

Files changed

• `crates/blockchain/src/store.rs` — verification, on_block_core, gossip verification
• `crates/storage/src/store.rs` — block read/write
• `crates/storage/src/api/tables.rs` — table docs

PR chain

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

How to Test

Requires Phase 1+2 merged. Network layer still references old types — fixed in Phase 4.

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR introduces dual XMSS key pairs for validators (attestation + proposal keys) and removes the proposer attestation from blocks. The changes are well-structured and maintain security properties while improving performance by eliminating redundant signatures.

Issues Found

1. Metrics naming inconsistency (blockchain/src/key_manager.rs:84)

   • The new sign_with_proposal_key function uses time_pq_sig_attestation_signing() but should have its own metric for proposal signatures.

2. Checkpoint sync verification (bin/ethlambda/src/checkpoint_sync.rs:147-150)

   • The verification only checks pubkey equality but doesn't validate the cryptographic integrity of the keys themselves. Consider adding signature verification.

3. Error handling in genesis config (crates/common/types/src/genesis.rs:130-135)

   • The test comment mentions "state root should be non-zero" but this is too permissive. The actual expected values should be computed and pinned.

4. Missing proposal signature metric (blockchain/src/key_manager.rs:110-113)

   • No metric increment for proposal signatures like there is for attestation signatures.

Security Considerations

• ✅ Key separation: Proper separation of attestation and proposal keys prevents OTS exhaustion attacks
• ✅ Signature verification: All signatures are properly verified using the correct key type
• ✅ State consistency: Checkpoint sync correctly validates both key types

Performance Impact

• Positive: Eliminates proposer attestation storage and processing overhead
• Neutral: Dual key storage increases memory usage by ~2x per validator, but this is acceptable for security

Code Quality

• ✅ Clean migration from single to dual key structure
• ✅ Backward compatibility maintained through proper deserialization
• ✅ Clear separation of concerns between key types

Recommendations

1. Add dedicated metrics for proposal signatures
2. Consider adding cryptographic verification in checkpoint sync
3. Update test fixtures to include actual expected hash values once computed

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have a full picture of the diff. Let me write the review.

---

Code Review — PR #232: Dual-key verification, remove proposer attestation

Overall this is a clean, well-scoped refactor. The architecture simplification (removing BlockWithAttestation/BlockSignaturesWithAttestation wrapper types, having all validators attest via gossip) is the right direction. A few items worth addressing:

---

Correctness

1. Dead parameter _local_validator_ids in on_block_core

store.rs — on_block_core still takes _local_validator_ids: &[u64] but it's entirely unused now that all the proposer-attestation-in-subnet logic was deleted. Both call sites pass specific values into this dead slot. The parameter should be removed from the function signature and both callsites (on_block, on_block_without_verification).

2. Weakened state root pin test (genesis.rs)

The state_from_genesis_root test was previously a pinned exact-hash regression test; it's now just assert_ne!(root, H256::ZERO). This is noted with a "Will be recomputed" comment but there's no tracking issue. Given that Validator's SSZ layout changed (two pubkeys instead of one), this test should be re-pinned before merge, or at minimum have a tracking issue referenced. A non-zero check doesn't catch regressions.

3. Storage format break — no migration

store.rs — get_signed_block now decodes BlockSignatures bytes directly from the BlockSignatures table, but existing data was encoded as BlockSignaturesWithAttestation. SSZ isn't self-describing, so existing persisted blocks (if any) will silently fail or misparse after upgrade. This is probably acceptable for a devnet-targeting PR chain with no persistence guarantees between devnets, but it's worth explicitly noting in release notes or a migration guard.

---

Metrics

4. Proposal signing reuses attestation metric labels

key_manager.rs, sign_with_proposal_key (~line 96):

let _timing = metrics::time_pq_sig_attestation_signing();
// ...
metrics::inc_pq_sig_attestation_signatures();

Both calls use attestation metrics. Proposal signing is now a distinct operation on a different key — mixing them obscures observability (e.g., if proposal signing is slow or fails, the attestation metric is inflated). Either add time_pq_sig_proposal_signing / inc_pq_sig_proposal_signatures, or at minimum add a comment acknowledging the reuse is intentional.

---

Error messages

5. deser_pubkey_hex loses field context

genesis.rs (~line 52): The old per-field deser_hex_pubkeys produced errors like "GENESIS_VALIDATORS[2] has length 51 (expected 52)". The new deser_pubkey_hex produces "pubkey has length 51 (expected 52)" — no field name, no validator index. This will make genesis config misconfigurations significantly harder to diagnose. Consider threading context via the caller or using a wrapper type with a custom deserializer that adds field context.

---

Test coverage

6. proposal_pubkey of validators 1 and 2 not asserted in genesis test

genesis.rs, test_parse_genesis_config: Only config.genesis_validators[0].proposal_pubkey is checked. Validators 1 and 2's proposal_pubkey values are never asserted. Since the config format changed, it's worth covering all six pubkey fields.

---

Minor

7. ATT_PUBKEY_C / PROP_PUBKEY_A have identical values in test data

genesis.rs tests: PROP_PUBKEY_A and ATT_PUBKEY_C resolve to the same hex string. Not a bug, but it can mask cross-key confusion in tests — if serialization accidentally swapped attestation and proposal keys, some assertions would still pass. Using fully distinct test values is preferable.

---

What's good

• The removal of BlockWithAttestation / BlockSignaturesWithAttestation is clean and the call sites updated consistently.
• Having proposers attest via normal gossip (interval 1) like all other validators is architecturally cleaner.
• The verify_signatures path now correctly uses get_proposal_pubkey() for the block-root signature and get_attestation_pubkey() for all attestation proofs — this is the critical security path and it looks correct.
• write_signed_block / get_signed_block storage layer simplification is straightforward and correct for the new type layout.
• New test test_sign_block_root_validator_not_found mirrors the existing attestation test — good coverage addition.

---

Automated review by Claude (Anthropic) · custom prompt

[github-actions]

🤖 Codex Code Review

1. High: possible startup/runtime panic on existing DB data after signature format change
   In crates/storage/src/store.rs:785, get_signed_block now decodes Table::BlockSignatures as BlockSignatures with expect("valid signatures").
   This table previously stored BlockSignaturesWithAttestation; without an explicit schema/version migration, older entries will fail decode and panic the node (DoS / availability risk).
   Suggestion: add DB schema versioning + migration (or graceful fallback decode path) before switching the on-disk type.

2. Medium: consensus regression detection weakened in genesis tests
   In crates/common/types/src/genesis.rs:137, pinned expected roots were removed and replaced with != ZERO checks (142, 151).
   This no longer catches accidental SSZ/state-root regressions, which is high-risk for consensus code.
   Suggestion: re-pin deterministic expected roots for current fixture data (or compute from a locked test vector) rather than non-zero assertions.

3. Low: incorrect metrics for proposal signing path
   In crates/blockchain/src/key_manager.rs:108, block proposal signing uses time_pq_sig_attestation_signing(), and unlike attestation signing there is no proposal-signature counter increment.
   This distorts operational telemetry and can hide proposal-signing performance issues.
   Suggestion: add dedicated proposal-signing timer/counter metrics.

Briefly: the dual-key split (attestation vs proposal) and signature verification direction look structurally sound; main concern is storage compatibility/panic behavior plus restoring stronger consensus-root test coverage.

I did not run tests locally in this review pass.

---

Automated review by OpenAI Codex · custom prompt