fix: support new annotated_validators.yaml lean-quickstart format

[MegaRedHand]

The lean-quickstart devnet4 generator (generate-genesis.sh:655-672, DUAL_KEY_MODE=true branch) emits two entries per validator with pubkey_hex + privkey_file fields, distinguished by attester / proposer substring in the filename. zeam and lantern already parse this layout. ethlambda was the only client requiring an explicit attestation_privkey_file + proposal_privkey_file schema, forcing operators to hand-rewrite the file after every regeneration.

Switch the loader to consume the generator's native output:

• AnnotatedValidator now models one role-specific entry (single privkey_file + pubkey_hex).
• read_validator_keys groups entries per validator index and routes by filename substring, matching zeam (pkgs/cli/src/node.zig:540) and lantern (client_keys.c:606).
• pubkey_hex is parsed for hex-format validation (52-byte hex) but not cross-checked against the loaded secret key. Deriving the pubkey would require either an upstream leanSig accessor (no public_key() exists today on GeneralizedXMSSSecretKey) or burning an XMSS one-time slot per startup.

ValidatorKeyPair, KeyManager, and BlockChain::spawn signatures unchanged.

This PR also updates the lean-multisig version to the latest devnet4 commit and the log_inv_rate aggregation parameter to 2 (which is what other clients use).

It also adds a limit to the number of attestation data objects per block (leanEthereum/leanSpec#536)

[github-actions]

🤖 Kimi Code Review

The PR refactors validator key loading to support lean-quickstart's output format (routing by filename substring) and adds validation that each validator has exactly one attestation and one proposal key.

Security & Correctness

• Absolute path traversal (Minor): Line 293-297 (resolve_path) allows absolute paths in the YAML to bypass validator_keys_dir. While the config is typically trusted, consider rejecting absolute paths or restricting them to within the keys directory for defense-in-depth.
• Case sensitivity: classify_role (lines 266-276) uses case-sensitive substring matching (contains). Ensure lean-quickstart consistently generates lowercase filenames; otherwise keys may fail to load.

Code Quality

• Terminology consistency: The error message at line 326 uses "attester" while the ValidatorKeyRole variant and line 313 error use "Attestation". Consider aligning these (e.g., "attestation" throughout).
• Error type: Returning Result<..., String> (line 285) is consistent with the existing codebase, though a structured error type would enable better programmatic handling in the future.

Logic Validation

• Duplicate detection: Lines 308-316 correctly prevent duplicate role entries for the same validator index.
• Completeness check: Lines 324-329 ensure both roles are present before loading keys, preventing partial validator configurations.
• Deterministic ordering: Using BTreeMap (line 298) for grouping provides deterministic error messages and loading order, which aids debugging.

Consensus Safety

• The validation logic correctly enforces the protocol requirement that validators possess distinct attestation and proposal key pairs. No consensus-critical logic is modified.

Recommendation: Address the absolute path consideration if the YAML source is ever untrusted; otherwise the PR is sound and ready for merge.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have full context. Let me write the review.

---

PR #277 Review: fix: support new annotated_validators.yaml lean-quickstart format

Overall: The refactor is well-structured and the logic is sound. The two-pass approach (group by index, then validate completeness, then load) is clean and deterministic. A few minor issues worth addressing.

---

Issues

1. Unnecessary Clone derive on AnnotatedValidator

main.rs:245 — Clone was added to the derive list but is never used. validator_vec is only iterated by shared reference (for entry in validator_vec). This should be removed.

---

2. Misleading error message in classify_role when path has no filename component

main.rs:282-284:

.ok_or_else(|| format!("non-utf8 filename '{}'", file.display()))?;

file_name() returns None for two distinct reasons: the path ends in ../.// (i.e., has no file component), or to_str() fails due to non-UTF-8 bytes. The error message "non-utf8 filename" is only accurate for the second case. A clearer message would be:

let name = file.file_name()
    .ok_or_else(|| format!("path '{}' has no filename component", file.display()))?
    .to_str()
    .ok_or_else(|| format!("filename in '{}' is not valid UTF-8", file.display()))?;

---

3. deser_pubkey_hex is unnecessarily pub

main.rs:255 — This function is only used within main.rs. In a binary crate this is harmless, but it should be fn or at most pub(crate) for consistency with the rest of the module.

---

4. No cross-check between _pubkey_hex and loaded key (acknowledged limitation)

The PR description is clear that leansig lacks a public_key() getter, so a silent key/pubkey mismatch is possible if the YAML is malformed (e.g., pubkey from validator A paired with privkey from validator B). The _ prefix and doc comment make this intent clear at the struct level, which is acceptable. Worth tracking as a follow-up if leansig ever exposes a pubkey accessor.

---

Minor observations

• classify_role checks for "attester" / "proposer" as substrings (not requiring the underscores _attester_ / _proposer_ the PR description mentions). This is more permissive but still correct — the ambiguous case (true, true) is caught properly.
• Using BTreeMap<u64, RoleSlots> for grouped gives deterministic insertion/iteration order by validator index. Good choice over HashMap here.
• Hoisting resolve_path and load_key out of the loop (rather than re-declaring them per iteration as in the old code) is a clean improvement.
• Error messages for missing/duplicate roles (main.rs:341-367) are specific and actionable.

---

Summary: Points 1 and 2 are worth fixing before merge. Point 3 is cosmetic. The logic and error handling are otherwise correct.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

1. Hard compatibility break in the new YAML parser: bin/ethlambda/src/main.rs:250. The struct field is named _pubkey_hex, but there is no #[serde(rename = "...")]. Serde will therefore look for a YAML key literally named _pubkey_hex. If lean-quickstart emits pubkey_hex (which is what this code strongly suggests), serde_yaml_ng::from_str will fail and the node will never start. This looks like the main regression in the PR.

2. The new pubkey deserializer is less permissive than the rest of the repo: bin/ethlambda/src/main.rs:261 vs crates/common/types/src/genesis.rs:42. Here hex::decode(&value) rejects 0x-prefixed keys, while the existing genesis parser explicitly strips an optional prefix. If annotated_validators.yaml contains 0x... pubkeys, startup breaks even though other config loaders accept that format. Reusing the shared behavior would avoid another format-compatibility edge case.

No fork-choice, justification/finalization, STF, attestation-processing, XMSS verification, or SSZ logic is touched here; the risk is isolated to validator key/config loading. I could not run cargo check in this sandbox because rustup/cargo need write access outside the workspace and the environment blocks that.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[greptile-apps]

Greptile Summary

Switches annotated_validators.yaml parsing from a single dual-key-field entry per validator to the lean-quickstart generator's native format — two entries per validator (one attester, one proposer), with role inferred from filename substrings — eliminating the need for manual file rewrites after every genesis regeneration and aligning with zeam and lantern.

The implementation is clean: classify_role uses the same substring-matching heuristic as the reference clients, duplicate/missing role entries produce clear errors, and the known inability to cross-check pubkey_hex against the loaded secret key (leansig exposes no public_key() getter) is explicitly documented in code.

Confidence Score: 5/5

Safe to merge; the only finding is a trivial visibility annotation on a module-private helper.

No P0 or P1 issues found. The single comment is a P2 style suggestion (removing pub from a private helper function in a binary crate) with zero runtime impact.

No files require special attention.

Important Files Changed

Filename	Overview
bin/ethlambda/src/main.rs	Rewrites validator key loading to consume lean-quickstart's native two-entry-per-validator YAML format; new logic groups entries by index, routes via filename substring to attestation/proposal slots, and validates pubkey_hex for hex/length only (documented limitation).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["annotated_validators.yaml\n(2 entries per validator)"] --> B["read_validator_keys()"]
    B --> C["Lookup node_id\nBTreeMap<String, Vec<AnnotatedValidator>>"]
    C -->|not found| ERR1["Error: node_id not found"]
    C -->|found| D["Iterate Vec<AnnotatedValidator>"]
    D --> E["classify_role(privkey_file)"]
    E -->|contains 'attester'| F["ValidatorKeyRole::Attestation"]
    E -->|contains 'proposer'| G["ValidatorKeyRole::Proposal"]
    E -->|neither / both| ERR2["Error: ambiguous filename"]
    F --> H["grouped BTreeMap<u64, RoleSlots>"]
    G --> H
    H -->|duplicate role| ERR3["Error: duplicate role entry"]
    H --> I["For each (idx, RoleSlots)"]
    I -->|missing attestation| ERR4["Error: missing attester entry"]
    I -->|missing proposal| ERR5["Error: missing proposer entry"]
    I --> J["load_key(att_path)\nload_key(prop_path)"]
    J --> K["HashMap<u64, ValidatorKeyPair>"]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: bin/ethlambda/src/main.rs
Line: 255

Comment:
**Unnecessary `pub` visibility on module-private function**

`deser_pubkey_hex` is only referenced by the `#[serde(deserialize_with = "...")]` attribute on `AnnotatedValidator`, which is in the same module. In a binary crate there is no external caller, and `pub` adds no value here — the function would work identically without it.

```suggestion
fn deser_pubkey_hex<'de, D>(d: D) -> Result<ValidatorPubkeyBytes, D::Error>
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "compat: load annotated_validators.yaml i..." | Re-trigger Greptile}

[greptile-apps]

Unnecessary pub visibility on module-private function

deser_pubkey_hex is only referenced by the #[serde(deserialize_with = "...")] attribute on AnnotatedValidator, which is in the same module. In a binary crate there is no external caller, and pub adds no value here — the function would work identically without it.

Suggested change

	pub fn deser_pubkey_hex<'de, D>(d: D) -> Result<ValidatorPubkeyBytes, D::Error>
	fn deser_pubkey_hex<'de, D>(d: D) -> Result<ValidatorPubkeyBytes, D::Error>

Prompt To Fix With AI

This is a comment left during a code review.
Path: bin/ethlambda/src/main.rs
Line: 255

Comment:
**Unnecessary `pub` visibility on module-private function**

`deser_pubkey_hex` is only referenced by the `#[serde(deserialize_with = "...")]` attribute on `AnnotatedValidator`, which is in the same module. In a binary crate there is no external caller, and `pub` adds no value here — the function would work identically without it.

```suggestion
fn deser_pubkey_hex<'de, D>(d: D) -> Result<ValidatorPubkeyBytes, D::Error>
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!