Devnet4: dual-key validators, block signing, and type migration#233
Devnet4: dual-key validators, block signing, and type migration#233pablodeymo wants to merge 22 commits intodevnet4from
Conversation
🤖 Kimi Code ReviewReview SummaryThis 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
|
🤖 Codex Code Review
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 |
🤖 Claude Code ReviewHere is my review of PR #233: PR #233 — Devnet4 Phase 4: Network Type Cascade and Test Harness UpdatesOverall 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
|
| 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
pablodeymo
force-pushed
the
devnet4-phase4-network
branch
from
e3d4f96 to
cf396a4
Compare
Introduce the devnet4 type-level changes:
- Validator: single pubkey → attestation_pubkey + proposal_pubkey with
get_attestation_pubkey() and get_proposal_pubkey() methods
- SignedBlockWithAttestation → SignedBlock (message is Block directly)
- Delete BlockWithAttestation and BlockSignaturesWithAttestation wrappers
- Genesis config: GENESIS_VALIDATORS changes from list of hex strings to
list of {attestation_pubkey, proposal_pubkey} objects
- Test fixtures: Validator deserialization updated for dual pubkeys
NOTE: This is SSZ-breaking. Downstream crates will not compile until
subsequent phases update all call sites.
- KeyManager: introduce ValidatorKeyPair with attestation_key + proposal_key - sign_attestation() routes to attestation key, new sign_block_root() uses proposal key - propose_block(): sign block root with proposal key, remove proposer attestation from block (proposer now attests at interval 1 via gossip) - produce_attestations(): remove proposer skip, all validators attest - main.rs: read dual key files per validator (attestation_privkey_file + proposal_privkey_file) - checkpoint_sync: validate both attestation_pubkey and proposal_pubkey - Type cascade: SignedBlockWithAttestation → SignedBlock, fix field access NOTE: store.rs and network layer still reference old types — fixed in subsequent phases.
…r attestation - verify_signatures(): attestation proofs use get_attestation_pubkey(), proposer signature verified with get_proposal_pubkey() over block root - on_block_core(): remove proposer attestation processing (~40 lines) — no more gossip signature insert or dummy proof for proposer - Gossip attestation/aggregation verification: get_attestation_pubkey() - Remove StoreError::ProposerAttestationMismatch variant - Storage: write/read BlockSignatures directly (no wrapper), fix field access .message.block → .message - Table docs: BlockSignatures stores BlockSignatures (not WithAttestation)
- Network API: SignedBlockWithAttestation → SignedBlock in all protocols - Gossipsub handler: update block decode/encode and field access - Req/resp codec + handlers: update BlocksByRoot type and field access - Gossipsub encoding test: update ignored test for new type name - Fork choice spec tests: remove proposer_attestation from BlockStepData, simplify build_signed_block() - Signature spec tests: update to TestSignedBlock (no attestation wrapper) - Common test types: remove ProposerAttestation (no longer in block) This completes the devnet4 type migration. All 34 unit tests pass, cargo clippy clean with -D warnings.
- Add dedicated proposal signing metrics (lean_pq_sig_proposal_signatures_total
counter and lean_pq_sig_proposal_signing_time_seconds histogram) so proposal
key signing is tracked separately from attestation signing.
- Pin exact state root and block root hashes in the genesis test instead of
the weak != H256::ZERO check, catching any future SSZ layout regressions.
- Update the update_safe_target comment to reflect devnet4 attestation flow
(proposer no longer embeds its own attestation in the block body).
- CLAUDE.md: update SignedBlockWithAttestation to SignedBlock, fix genesis
config format to dual-key YAML, update BlockSignatures table type, fix
tick interval 1 description (all validators now attest).
- Architecture HTML: update type name in two places.
- checkpoint_sync: clarify pubkey mismatch error to mention both keys.
- SSZ test fixture: rename signed_block_with_attestation.ssz to signed_block.ssz.
- Genesis test: add proposal pubkey assertions for validators B and C.
- store.rs: fix stale proof wording in build_block docstring.
- Devnet log patterns: remove stale 'Skipping attestation for proposer' entry.
The previous pin (ad9a3226) added dual-key ValidatorKeyPair.from_dict() but still pointed KEY_DOWNLOAD_URLS at the old single-key release (leanSpec-bbbbf62-v2), causing CI fixture generation to fail with KeyError: 'attestation_public'. Commit 488518c updates the URLs to the leanSpec-ad9a3226 release which has the dual-key format keys.
… format
The new leanSpec fixture format flattens BlockStepData - block fields
(slot, proposerIndex, etc.) are now directly in the step's block object
instead of nested under a second block wrapper.
Changes:
- Bump LEAN_SPEC_COMMIT_HASH to 9c30436 (leanSpec HEAD)
- Flatten BlockStepData to match the new fixture JSON layout
- Add to_block() helper for converting BlockStepData to domain Block
- Skip test_reorg_with_slot_gaps: fixture relies on gossip proposer
attestation state not serialized into the JSON fixture
pablodeymo
force-pushed
the
devnet4-phase4-network
branch
from
39f9fa2 to
c8ea8d9
Compare
pablodeymo
changed the title
🤖 Kimi Code ReviewThis PR introduces a dual-key validator model (separate XMSS keys for attestation vs. block proposal), simplifying the block structure by removing the proposer attestation wrapper and having proposers sign block roots directly. Security & CorrectnessCritical: Key Usage Separation ✅
Block Verification Logic ✅
Fork Choice Impact ✅
Code QualityType Safety
Error Handling
Tests
Minor SuggestionsItem 1: Remove stale comment in Item 2: Explicit proposer validation (defense in depth) Item 3: Storage migration note Documentation
VerdictApprove — The refactoring is architecturally sound, maintains XMSS OTS security guarantees, and properly separates cryptographic domains. The SSZ encoding changes are consistent and tested. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
Aside from those points, the I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewNow I have enough context to write a thorough review. Let me compile the findings. Code Review: PR #233 — Devnet4: dual-key validators, block signing, and type migrationOverviewLarge, clean migration introducing separate attestation/proposal XMSS keys per validator. Removes the Security & CorrectnessDual-key verification routing — Attestation key path: XMSS OTS slot separation: the proposal key and attestation key advance their OTS indices independently, so signing both in slot N is safe. ✓ Unused pubkey fields in
if state_val.attestation_pubkey != expected_val.attestation_pubkey
|| state_val.proposal_pubkey != expected_val.proposal_pubkey✓ Potential Bugs / Issues
Old: D::Error::custom(format!("GENESIS_VALIDATORS[{idx}] is not valid hex: {s}"))New: D::Error::custom(format!("pubkey is not valid hex: {s}"))When a genesis config has a malformed key, the new message no longer tells the operator which validator entry is broken. This regression is a consequence of switching from a Suggested fix: pass the field name through context or keep the key prefix in the error string: D::Error::custom(format!("attestation_pubkey/proposal_pubkey is not valid hex: {s}"))Skipped test The skip reason is that the fixture relies on gossip attestation state not serialized into JSON. This is a latent correctness gap in reorg handling with slot gaps. There is no tracking issue referenced in the skip comment. At minimum, a link to an issue or a TODO noting what needs to happen for this test to be re-enabled would make it less likely to be forgotten. SSZ fixture
Minor Observations
let block = block_data.to_block();
if let Some(ref label) = block_data.block_root_label {
block_registry.insert(label.clone(), H256::from(block.tree_hash_root()));
}
let signed_block = build_signed_block_from(block, &block_data);Not a bug; minor style point. Positive Highlights
SummaryNo critical bugs. The dual-key routing through the signing and verification paths is correct, which is the most security-sensitive aspect of this change. The three items worth addressing before merge are (in order of priority):
Automated review by Claude (Anthropic) · sonnet · custom prompt |
Greptile SummaryThis PR completes the devnet4 migration from a single-key validator model to a dual-key model (separate attestation and proposal keys), cascading the type rename from Confidence Score: 5/5Safe to merge — dual-key migration is logically consistent and correct across all layers; only minor style inconsistencies remain. All findings are P2: a missing 0x-prefix strip in a startup helper and test-only proof_data replaced with empty proofs. No correctness, data-integrity, or security issues found in the consensus, signing, or storage paths. bin/ethlambda/src/main.rs (deser_pubkey_hex 0x handling) and crates/blockchain/tests/signature_types.rs (proof_data discarded silently).
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/key_manager.rs | Dual-key validator implementation with separate attestation and proposal keys; signing functions use correct keys with metrics; unit tests cover the error paths. |
| crates/blockchain/src/lib.rs | Block proposal correctly signs hash_tree_root(block) with the proposal key; all validators attest at interval 1 (proposer no longer skipped); logic is clean. |
| crates/common/types/src/block.rs | BlockSignatures now carries both attestation_signatures and proposer_signature; clean removal of the old proposer attestation wrapper. |
| crates/common/types/src/state.rs | Validator now has dual attestation_pubkey and proposal_pubkey; helper methods get_attestation_pubkey()/get_proposal_pubkey() added; state root pinned in tests. |
| crates/common/types/src/genesis.rs | GenesisValidatorEntry updated to dual-key YAML format; deser_pubkey_hex correctly strips 0x prefix; pinned genesis root hash test. |
| bin/ethlambda/src/main.rs | Loads dual attestation/proposal key pairs from annotated_validators.yaml; local deser_pubkey_hex lacks 0x prefix stripping unlike genesis.rs version, risking startup failure. |
| crates/blockchain/tests/signature_types.rs | Deserialises devnet4 SignedBlock from test fixtures; proof_data is silently discarded and replaced with empty proofs, so aggregated attestation proof verification is not exercised in spec tests. |
| crates/blockchain/tests/forkchoice_spectests.rs | test_reorg_with_slot_gaps skipped with clear explanation; build_signed_block uses default signatures correctly for verification-bypass path. |
| bin/ethlambda/src/checkpoint_sync.rs | Now validates both attestation_pubkey and proposal_pubkey in checkpoint state; error variant ValidatorPubkeyMismatch updated with clear message. |
| crates/blockchain/src/metrics.rs | New proposal-signing metrics added (counter + histogram); all metrics correctly registered in init(); naming follows lean_ prefix convention. |
Sequence Diagram
sequenceDiagram
participant BC as BlockChainServer
participant KM as KeyManager
participant ST as Store
participant P2P as P2P
Note over BC: Interval 0 (slot > 0)
BC->>ST: produce_block_with_signatures(slot, validator_id)
ST-->>BC: (block, attestation_signatures)
BC->>KM: sign_block_root(validator_id, slot, block_root)
KM-->>BC: proposer_signature via proposal_key
BC->>BC: assemble SignedBlock
BC->>ST: on_block(signed_block)
BC->>P2P: publish_block(signed_block)
Note over BC: Interval 1 (all validators)
BC->>KM: sign_attestation(validator_id, data)
KM-->>BC: signature via attestation_key
BC->>P2P: publish_attestation(SignedAttestation)
Note over P2P: On receiving gossip block
P2P->>BC: new_block(SignedBlock)
BC->>ST: on_block - verify proposer_signature via get_proposal_pubkey()
Comments Outside Diff (1)
-
bin/ethlambda/src/main.rs, line 251-263 (link)Missing
0xprefix stripping indeser_pubkey_hexThe local
deser_pubkey_hexcallshex::decode(&value)directly, while the equivalent function ingenesis.rsstrips the0xprefix first withs.strip_prefix("0x").unwrap_or(&s). Ifannotated_validators.yamlkeys are formatted with a0xprefix, startup will panic insideserde_yaml_ng::from_str(...).expect("Failed to parse validators file"). Consider aligning both functions or extracting a shared helper.Prompt To Fix With AI
This is a comment left during a code review. Path: bin/ethlambda/src/main.rs Line: 251-263 Comment: **Missing `0x` prefix stripping in `deser_pubkey_hex`** The local `deser_pubkey_hex` calls `hex::decode(&value)` directly, while the equivalent function in `genesis.rs` strips the `0x` prefix first with `s.strip_prefix("0x").unwrap_or(&s)`. If `annotated_validators.yaml` keys are formatted with a `0x` prefix, startup will panic inside `serde_yaml_ng::from_str(...).expect("Failed to parse validators file")`. Consider aligning both functions or extracting a shared helper. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/blockchain/tests/signature_types.rs
Line: 61-72
Comment:
**Attestation `proof_data` silently discarded in test conversion**
The `From<TestSignedBlock> for SignedBlock` conversion drops the actual `proof_data` from each `AttestationSignature` and replaces it with `AggregatedSignatureProof::empty(participants)`. The `ProofData { data: String }` field in the fixture is read but never used, so the signature spec tests do not exercise aggregated attestation proof verification — only the proposer XMSS signature is actually checked.
If the intent is a known limitation while aggregation proof format is stabilised, a comment explaining this would help future readers avoid mistaking the empty proofs for correct data.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: bin/ethlambda/src/main.rs
Line: 251-263
Comment:
**Missing `0x` prefix stripping in `deser_pubkey_hex`**
The local `deser_pubkey_hex` calls `hex::decode(&value)` directly, while the equivalent function in `genesis.rs` strips the `0x` prefix first with `s.strip_prefix("0x").unwrap_or(&s)`. If `annotated_validators.yaml` keys are formatted with a `0x` prefix, startup will panic inside `serde_yaml_ng::from_str(...).expect("Failed to parse validators file")`. Consider aligning both functions or extracting a shared helper.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Bump leanSpec to HEAD (9c30436) and adap..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let attestation_signatures: AttestationSignatures = value | ||
| .signature | ||
| .attestation_signatures | ||
| .data | ||
| .into_iter() | ||
| .map(|att_sig| { | ||
| // Convert participants bitfield | ||
| let participants: EthAggregationBits = att_sig.participants.into(); | ||
| // Create proof with participants but empty proof_data | ||
| AggregatedSignatureProof::empty(participants) | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .try_into() | ||
| .expect("too many attestation signatures"); |
There was a problem hiding this comment.
proof_data silently discarded in test conversion
The From<TestSignedBlock> for SignedBlock conversion drops the actual proof_data from each AttestationSignature and replaces it with AggregatedSignatureProof::empty(participants). The ProofData { data: String } field in the fixture is read but never used, so the signature spec tests do not exercise aggregated attestation proof verification — only the proposer XMSS signature is actually checked.
If the intent is a known limitation while aggregation proof format is stabilised, a comment explaining this would help future readers avoid mistaking the empty proofs for correct data.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/tests/signature_types.rs
Line: 61-72
Comment:
**Attestation `proof_data` silently discarded in test conversion**
The `From<TestSignedBlock> for SignedBlock` conversion drops the actual `proof_data` from each `AttestationSignature` and replaces it with `AggregatedSignatureProof::empty(participants)`. The `ProofData { data: String }` field in the fixture is read but never used, so the signature spec tests do not exercise aggregated attestation proof verification — only the proposer XMSS signature is actually checked.
If the intent is a known limitation while aggregation proof format is stabilised, a comment explaining this would help future readers avoid mistaking the empty proofs for correct data.
How can I resolve this? If you propose a fix, please make it concise.
bin/ethlambda/src/main.rs
Outdated
| #[serde(deserialize_with = "deser_pubkey_hex")] | ||
| _pubkey: ValidatorPubkeyBytes, | ||
| privkey_file: PathBuf, | ||
| _attestation_pubkey: ValidatorPubkeyBytes, |
bin/ethlambda/src/main.rs
Outdated
| std::fs::read(&secret_key_path).expect("Failed to read validator secret key file"); | ||
| let load_key = |path: &Path, purpose: &str| -> ValidatorSecretKey { | ||
| let bytes = std::fs::read(path).unwrap_or_else(|err| { | ||
| error!(node_id=%node_id, index=validator_index, file=?path, %err, "Failed to read {purpose} key file"); |
There was a problem hiding this comment.
| error!(node_id=%node_id, index=validator_index, file=?path, %err, "Failed to read {purpose} key file"); | |
| error!(%node_id, index=validator_index, file=?path, %err, "Failed to read {purpose} key file"); |
bin/ethlambda/src/main.rs
Outdated
| std::process::exit(1); | ||
| }); | ||
| ValidatorSecretKey::from_bytes(&bytes).unwrap_or_else(|err| { | ||
| error!(node_id=%node_id, index=validator_index, file=?path, ?err, "Failed to parse {purpose} key"); |
There was a problem hiding this comment.
| error!(node_id=%node_id, index=validator_index, file=?path, ?err, "Failed to parse {purpose} key"); | |
| error!(%node_id, index=validator_index, file=?path, ?err, "Failed to parse {purpose} key"); |
bin/ethlambda/src/main.rs
Outdated
| let load_key = |path: &Path, purpose: &str| -> ValidatorSecretKey { | ||
| let bytes = std::fs::read(path).unwrap_or_else(|err| { | ||
| error!(node_id=%node_id, index=validator_index, file=?path, %err, "Failed to read {purpose} key file"); | ||
| std::process::exit(1); |
There was a problem hiding this comment.
This should return an error
bin/ethlambda/src/main.rs
Outdated
| }); | ||
| ValidatorSecretKey::from_bytes(&bytes).unwrap_or_else(|err| { | ||
| error!(node_id=%node_id, index=validator_index, file=?path, ?err, "Failed to parse {purpose} key"); | ||
| std::process::exit(1); |
|
|
||
| // Tests where the fixture relies on gossip attestation behavior not serialized into the JSON. | ||
| // These pass in the Python spec but fail in our runner because we don't simulate gossip. | ||
| const SKIP_TESTS: &[&str] = &["test_reorg_with_slot_gaps"]; |
There was a problem hiding this comment.
Why skip this test? Can't we add gossip simulation?
Resolve merge conflicts from the single-AttestationData-per-block commit (2d5b273): - Makefile: keep phase4's leanSpec hash (9c30436) for dual-key fixtures - store.rs: keep DuplicateAttestationData error, drop ProposerAttestationMismatch, fix type access path (signed_block.message instead of signed_block.block.block) - key_manager.rs: fix secret_key references to key_pair.attestation_key for XMSS preparation window, add same preparation logic to sign_with_proposal_key
The auto-merged single-AttestationData tests (2d5b273) used pre-phase4 types: - SignedBlockWithAttestation/BlockWithAttestation -> SignedBlock with .message - Validator { pubkey } -> Validator { attestation_pubkey, proposal_pubkey } - Removed proposer_attestation fields from test block construction
…rt to test harness Resolve merge conflict in lib.rs: keep phase4's block-root signing (proposer attestation removed in devnet4), drop devnet4's post-block checkpoint attestation (PR #268) since the feature it improves no longer exists. Add attestation and gossipAggregatedAttestation step handlers to the fork choice test harness, with on_gossip_attestation_without_verification() that validates data, checks validator index, inserts into known payloads, and recalculates head. Patch leanSpec's test_lexicographic_tiebreaker to use single-depth forks (backport of upstream fix e545e14) and remove same-slot constraint from the lexicographicHeadAmong validation. Skip test_gossip_attestation_with_invalid_signature (no sig verification in tests). 7 headSlot-mismatch failures remain from fixture expectation changes between leanSpec d39d101 and 9c30436 - to be investigated separately.
## Summary
- Aggregator now calls `on_gossip_attestation()` locally before
publishing to gossip, ensuring its own validator's attestation enters
`gossip_signatures` and is included in aggregated proofs
- Adds `publish_best_effort()` to suppress the expected
`NoPeersSubscribedToTopic` warning when the aggregator publishes to
attestation subnets it subscribes to
- Threads `is_aggregator` from `SwarmConfig` through `BuiltSwarm` into
`P2PServer` so the gossipsub handler can use best-effort publishing
## Context
Gossipsub does not deliver messages back to the sender. The aggregator
subscribes to the attestation subnet and publishes via mesh, but in
small devnets no other node subscribes to attestation subnets. This
causes the aggregator's own attestation publish to fail silently with
`NoPeersSubscribedToTopic`, meaning its vote never enters
`gossip_signatures`, is never aggregated, and is never included in
blocks.
The reference spec handles this explicitly in `_produce_attestations()`:
```python
# Process attestation locally before publishing.
#
# Gossipsub does not deliver messages back to the sender.
# Without local processing, the aggregator node never sees its own
# validator's attestation in gossip_signatures, reducing the
# aggregation count below the 2/3 safe-target threshold.
self.sync_service.store = self.sync_service.store.on_gossip_attestation(
signed_attestation=signed_attestation,
is_aggregator=is_aggregator_role,
)
```
## Test plan
- [x] `cargo build` passes
- [x] `cargo clippy --workspace -- -D warnings` passes
- [x] `cargo test --workspace --release --lib` passes (all 32 unit
tests)
- [ ] Deploy to devnet and verify:
- Validator 3's attestation appears in "Attestation processed" logs on
the aggregator
- Blocks from all proposers include aggregator's vote in aggregated
proofs (higher attestation counts)
- No more `NoPeersSubscribedToTopic` warnings on the aggregator node
- Finalization continues normally
build_signed_block was creating an empty attestation_signatures list, so the zip in on_block_core discarded all in-block attestations before they reached fork choice. Now builds one empty AggregatedSignatureProof per attestation, matching each attestation's aggregation_bits. This fixes all 7 headSlot-mismatch failures in fork choice spec tests. All 48 forkchoice spec tests now pass.
… of exit
- Remove unused pubkey fields from AnnotatedValidator (already validated
during GenesisConfig parsing)
- Replace process::exit(1) with proper error returns in read_validator_keys
- Use tracing shorthand %node_id
2 participants
Motivation
Implements devnet4 (leanSpec#449): separate attestation and proposal keys for validators, replacing the single-key model and removing the proposer attestation wrapper.
Description
Phase 1: Types (#230)
Validatorgainsattestation_pubkey+proposal_pubkey(replaces singlepubkey)SignedBlockWithAttestation→SignedBlock,BlockWithAttestationdeletedPhase 2: Key manager + block proposal (#231)
ValidatorKeyPairwith separate attestation/proposal secret keyshash_tree_root(block)with proposal key (no proposer attestation)Phase 3: Store + verification (#232)
get_attestation_pubkey()/get_proposal_pubkey()as appropriateon_block_coreSignedBlockdirectlyPhase 4: Network + tests (#233)
ProposerAttestation, simplifiedbuild_signed_block()test_reorg_with_slot_gaps(fixture relies on gossip proposer attestation state)Supersedes
Closes #230, closes #231, closes #232
Blockers