Devnet4 Phase 4: network type cascade and test harness updates

[pablodeymo]

Motivation

Final PR implementing devnet4. Completes the type migration across the network layer and updates all test harnesses. After this PR, the full workspace compiles clean and all 34 unit tests pass.

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

Description

Network layer: type rename cascade

`SignedBlockWithAttestation` → `SignedBlock` and `.message.block.X` → `.message.X` across:

File	Changes
`crates/net/api/src/lib.rs`	Protocol message types
`crates/net/p2p/src/gossipsub/handler.rs`	Block decode/encode + field access
`crates/net/p2p/src/gossipsub/encoding.rs`	Ignored test type name
`crates/net/p2p/src/req_resp/messages.rs`	`ResponsePayload::BlocksByRoot`
`crates/net/p2p/src/req_resp/codec.rs`	Block decode + doc comment
`crates/net/p2p/src/req_resp/handlers.rs`	Response handling + field access

Test harness updates

• common.rs: removed `ProposerAttestation` type (no longer in block)
• types.rs: removed `proposer_attestation` from `BlockStepData`
• forkchoice_spectests.rs: simplified `build_signed_block()` — no attestation wrapper
• signature_spectests.rs: `TestSignedBlock` replaces `TestSignedBlockWithAttestation`
• signature_types.rs: removed `TestBlockWithAttestation` wrapper

Verification

```bash
cargo clippy --workspace --all-targets -- -D warnings # clean
cargo test -p ethlambda-types # 3 passed
cargo test -p ethlambda-blockchain --lib # 13 passed
cargo test -p ethlambda # 18 passed
```

Spec tests (fork_choice, verify_signatures, stf) await devnet4 fixtures from `leansig-test-keys`.

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 (Devnet4 Phase 3: dual-key verification and remove proposer attestation from store #232)
4. → Phase 4: Network + tests (this PR)

Blockers

• Test fixtures: `leansig-test-keys` has not published dual-key format keys yet
• lean-quickstart: needs devnet4 genesis config support for manual devnet testing

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR introduces dual XMSS key support for validators (separate attestation and proposal keys) and removes the proposer attestation wrapper from blocks. The changes are extensive but generally well-structured. Below are the key findings:

---

✅ Correctness & Consensus Safety

• Validator key separation logic is sound: The split into attestation_pubkey and proposal_pubkey prevents OTS reuse when signing both blocks and attestations in the same slot.
• Checkpoint sync verification updated correctly: verify_checkpoint_state now checks both keys (line 147 in checkpoint_sync.rs).
• Genesis config parsing updated: The new GenesisValidatorEntry struct properly handles dual keys.

---

⚠️ Potential Issues

1. Metrics Mislabeling (crates/blockchain/src/key_manager.rs:82)

• Line 82: time_pq_sig_attestation_signing() is used for block signing with the proposal key. This should use a separate metric (e.g., time_pq_sig_block_signing()).

2. Missing Proposal Key Verification in Tests

• Fork choice tests (forkchoice_spectests.rs) and signature tests (signature_spectests.rs) use Default::default() for proposer signatures. These should ideally verify the proposal key signature, even in test setups.

3. Checkpoint Sync Test Data

• **Line 249 in checkpoint_sync.rs**: The test validator generation uses proposal_pubkey: [i as u8 + 101; 52], which could overflow u8for largei. Use ((i + 101) as u8)` or a safer pattern.

4. Error Handling in read_validator_keys

• Line 287-291: The load_key closure calls std::process::exit(1) on failure. While acceptable for a binary, this is harsh for a library function. Consider propagating errors instead.

---

🔒 Security

• No direct security vulnerabilities introduced. The dual-key design improves security by preventing OTS reuse.
• Key file paths are resolved correctly (absolute vs. relative).

---

🧹 Code Quality & Maintainability

• Redundant types removed: BlockWithAttestation, BlockSignaturesWithAttestation, and ProposerAttestation are correctly eliminated.
• Consistent naming: SignedBlockWithAttestation → SignedBlock is a clean rename.
• SSZ compatibility: The Validator struct’s new layout is SSZ-compatible (fields are fixed-size).

---

📌 Minor Suggestions

• Line 230 in main.rs: The AnnotatedValidator struct has unused fields (_attestation_pubkey, _proposal_pubkey). Consider removing them or adding #[allow(dead_code)].
• Line 135 in key_manager.rs: The test test_sign_message_validator_not_found should be renamed to test_sign_attestation_validator_not_found for clarity.

---

✅ Conclusion

The PR is well-executed and aligns with the devnet4 spec. The dual-key design is correctly implemented, and the removal of the proposer attestation wrapper simplifies the block structure. Address the minor issues above (especially the metrics mislabeling) before merging.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

1. High: panic-on-input in block signature verification (remote DoS).
   crates/blockchain/src/store.rs:1175 and crates/blockchain/src/store.rs:1214 use try_into().expect("slot exceeds u32") on untrusted block data. A malicious peer can send a block/attestation with slot > u32::MAX and crash the node.
   Suggested fix: return a typed StoreError on conversion failure instead of expect.

2. High: silent slot truncation in proposer signing path.
   crates/blockchain/src/lib.rs:216 casts slot as u32 before signing, and crates/blockchain/src/key_manager.rs:65 bakes u32 into the API. After u32::MAX slots, signatures are produced for truncated slots and will fail verification / diverge behavior.
   Suggested fix: plumb u64 through KeyManager and perform checked conversion at the XMSS boundary with explicit error handling.

3. Medium: genesis root regression test lost its invariant check.
   crates/common/types/src/genesis.rs:137 replaced pinned root assertions with non-zero checks. This no longer detects SSZ layout/hash regressions, which is risky for consensus-critical types.
   Suggested fix: restore deterministic expected roots (or compute from fixed fixtures) and assert exact hashes.

4. Medium: validator key config is parsed but not validated against loaded secrets.
   bin/ethlambda/src/main.rs:229 / bin/ethlambda/src/main.rs:233 parse attestation/proposal pubkeys, but bin/ethlambda/src/main.rs:287-bin/ethlambda/src/main.rs:306 never checks the loaded secret keys derive those pubkeys. Misconfigured files will start normally but produce invalid signatures.
   Suggested fix: derive pubkeys from secret keys at load time and hard-fail on mismatch.

Overall direction (dual keys + block-root proposer signature) looks coherent, but the two slot-handling issues are blocking for safety/correctness.

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Here is my review of PR #233:

---

PR #233 — Devnet4 Phase 4: Network Type Cascade and Test Harness Updates

Overall this is a clean, well-scoped mechanical migration. The type rename cascade is handled consistently across all 22 files, and the architectural change (proposer signs block root with dedicated proposal key rather than embedding a proposer attestation) is correctly threaded through the full pipeline: key manager → proposal → verification → storage → network. A few issues worth addressing:

---

Wrong metrics label in sign_with_proposal_key

crates/blockchain/src/key_manager.rs, new sign_with_proposal_key:

let _timing = metrics::time_pq_sig_attestation_signing();

This reuses the attestation signing timing metric for proposal signing. The label is semantically wrong and will inflate lean_pq_sig_attestation_signing_* metrics with block proposal latency. A time_pq_sig_proposal_signing() metric should be added (or at minimum a separate counter inc_pq_sig_proposal_signatures()), mirroring what sign_with_attestation_key does.

---

Missing serialize_with for proposal_pubkey in Validator

crates/common/types/src/state.rs:

#[serde(serialize_with = "serialize_pubkey_hex")]
pub attestation_pubkey: ValidatorPubkeyBytes,

// no serialize_with here ↓
pub proposal_pubkey: ValidatorPubkeyBytes,

attestation_pubkey gets serialize_pubkey_hex but proposal_pubkey does not. This means JSON serialization of Validator (e.g., via the /lean/v0/fork_choice or /lean/v0/states/finalized endpoints) will produce inconsistent output — attestation_pubkey as hex but proposal_pubkey as raw bytes/array. Should be:

#[serde(serialize_with = "serialize_pubkey_hex")]
pub proposal_pubkey: ValidatorPubkeyBytes,

---

Inconsistent slot cast in block proposal vs. signature verification

crates/blockchain/src/lib.rs, propose_block:

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

vs. crates/blockchain/src/store.rs, verify_signatures:

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

The proposal path silently truncates an overflowing u64 slot to u32, while the verification path panics. For any slot ≥ 2³², the signature would be created with a different slot than what verification expects, causing verification failure. The as u32 cast should be replaced with try_into().expect("slot exceeds u32") to match the verification side.

---

ValidatorPubkeyMismatch error loses which key failed

bin/ethlambda/src/checkpoint_sync.rs:

if state_val.attestation_pubkey != expected_val.attestation_pubkey
    || state_val.proposal_pubkey != expected_val.proposal_pubkey
{
    return Err(CheckpointSyncError::ValidatorPubkeyMismatch { index: i });
}

The error doesn't say whether the attestation or proposal key mismatched. For operational debugging this is a minor usability regression. Could either add a field to the error variant or split into two checks with distinct variants/messages.

---

Weakened genesis state root test (acknowledged, temporary)

crates/common/types/src/genesis.rs, test_genesis_state_root_computation now only asserts != ZERO instead of checking the exact hash. The comment correctly explains this is intentional pending fixture regeneration from leansig-test-keys. This is acceptable given the PR chain context, but should be tracked to restore exact-hash assertions once the dual-key test fixtures are published.

---

Storage format is a breaking change (expected)

The BlockSignatures type is now stored directly instead of BlockSignaturesWithAttestation. Any existing database with the old encoding will fail to deserialize at startup. This is expected for a devnet breaking change, but it may be worth a log message at startup that instructs operators to wipe the database when migrating from devnet3 to devnet4.

---

Minor: deser_pubkey_hex error messages lost field context

Old: "GENESIS_VALIDATORS[{idx}] is not valid hex: {s}" (reported the validator index)
New: "pubkey is not valid hex: {s}" (no context which validator entry or which key field failed)

Since deser_pubkey_hex is now called once per field via #[serde(deserialize_with)], the outer serde error will include the field name path, so this is acceptably compensated — but the old index hint was helpful for large genesis configs.

---

Summary

Severity	Item
Bug (latent)	slot as u32 truncating cast in propose_block vs. try_into().expect() in verify_signatures
Bug	proposal_pubkey missing serialize_with on Validator
Metrics	sign_with_proposal_key uses attestation timing metric
Minor	ValidatorPubkeyMismatch doesn't identify which key type failed
Expected/tracked	Weakened state root test, storage format break

The core logic — dual-key signing, block root verification using proposal_pubkey, proposer now attesting independently via attestation_key at interval 1 — looks correct and consistent across all layers.

---

Automated review by Claude (Anthropic) · custom prompt