feat(dkg): implement some helpers on dkg

[iamquang95]

Go function	Status	Rust mapping / missing deps
startSyncProtocol	✗	Missing deps: sync, p2p host wiring
signAndAggLockHash	✗	Missing deps: exchanger
signAndAggDepositData	✗	Missing deps: exchanger
signAndAggValidatorRegistrations	✗	Missing deps: exchanger
setupP2P	✗	Missing deps: other p2p packages
HasTestConfig	◐	crates/dkg/src/dkg.rs → Config::has_test_config. Missing deps: test hook surface
Run	◐	crates/dkg/src/dkg.rs → run. Missing deps: sync, exchanger, frost, nodesigs integration, setup_p2p, append flow
aggLockHashSig	✓	crates/dkg/src/aggregate.rs → agg_lock_hash_sig
signLockHash	✓	crates/dkg/src/signing.rs → sign_lock_hash
signDepositMsgs	✓	crates/dkg/src/signing.rs → sign_deposit_msgs
signValidatorRegistrations	✓	crates/dkg/src/signing.rs → sign_validator_registrations
aggDepositData	✓	crates/dkg/src/aggregate.rs → agg_deposit_data
aggValidatorRegistrations	✓	crates/dkg/src/aggregate.rs → agg_validator_registrations
createDistValidators	✓	crates/dkg/src/validators.rs → create_dist_validators
writeLockToAPI	✓	crates/dkg/src/publish.rs → write_lock_to_api
validateKeymanagerFlags	✓	crates/dkg/src/dkg.rs → validate_keymanager_flags
logPeerSummary	✓	crates/dkg/src/dkg.rs → log_peer_summary
builderRegistrationFromETH2	✓	crates/dkg/src/validators.rs → builder_registration_from_eth2
setRegistrationSignature	✓	crates/dkg/src/validators.rs → set_registration_signature
getExistingShares	✓	crates/dkg/src/dkg.rs → get_existing_shares

[iamquang95]

@claude

[claude]

Claude finished @iamquang95's task in 5m 7s —— View job

---

Code Review

• Gather context: read changed files
• Review validators.rs
• Review signing.rs
• Review aggregate.rs
• Review publish.rs
• Review dkg.rs and lib.rs changes
• Cross-check with Go source (Charon)
• Post final review

---

Overall the port is solid and functionally equivalent to the Go source. A few issues below.

---

Functional Issues

1. agg_validator_registrations crosses module boundaries with ValidatorsError [aggregate.rs:197]

let v1 = msg
    .0
    .v1
    .as_ref()
    .ok_or(ValidatorsError::MissingV1Registration)?;  // ValidatorsError in aggregate module

aggregate.rs imports and names a specific ValidatorsError variant directly. This works because AggregateError has Validators(#[from] ValidatorsError), but it's an awkward coupling. A dedicated AggregateError::MissingV1Registration variant or a helper on the registration type would be cleaner. In the Go source, msg.V1.Message is accessed without a check here because the type system guarantees V1 is present at this point — consider whether the same invariant holds in Rust and whether panicking on violation (like Go does implicitly) vs returning an error is the right choice.

Fix this →

2. create_dist_validators uses .remove() for deposit data lookup [validators.rs:115-117]

let partial_deposit_data = deposit_datas_map
    .remove(&share.pub_key)      // ← modifies the map
    .ok_or_else(...)?;

Go reads (depositDatasMap[pk]) without consuming entries. This means if two shares accidentally share a pubkey, the second will get DepositDataNotFound in Rust but succeed in Go. Not a real-world concern since shares must have unique pubkeys, but it is a behavioral divergence. Using .get() + .clone() would be a closer match to Go, or leave a comment explaining the deliberate ownership transfer.

3. Aggregation result order is non-deterministic

agg_deposit_data, agg_validator_registrations, and agg_lock_hash_sig all iterate over HashMap, so result ordering is non-deterministic. This matches Go (which has the same issue), but both should probably sort results by pubkey before returning so downstream consumers have stable output. Worth tracking as a follow-up even if out of scope here.

---

Missing Test Coverage

4. No test for sign_lock_hash [signing.rs]

sign_deposit_msgs and sign_validator_registrations both have tests, but sign_lock_hash has none. A simple test verifying it returns one partial signature per share with the correct share_idx would be good to add.

Fix this →

5. No happy-path test for agg_validator_registrations [aggregate.rs]

There's a rejection test (agg_validator_registrations_rejects_unknown_pubkeys) but no success-case test. agg_deposit_data has both. The success test should verify the aggregated signature verifies against the validator's pubkey and the registration signing root.

Fix this →

---

Minor Issues

6. Aggregation errors drop peer context that Go includes [aggregate.rs]

The Go source includes helpful debug fields on errors:

errors.New("invalid pubkey in lock hash partial signature from peer",
    z.Int("peerIdx", s.ShareIdx-1), z.Str("pubkey", pk.String()))

The Rust versions (e.g. AggregateError::InvalidPubKeyFromPeer { context: "lock hash" }) don't include share_idx or the pubkey. This makes debugging harder in production. Consider adding these fields to the error struct variants.

7. agg_validator_registrations shadows fork_version [aggregate.rs:185-186]

let fork_version: phase0::Version = fork_version
    .try_into()
    .map_err(|_| AggregateError::InvalidForkVersionLength)?;

Shadowing works and is idiomatic Rust, but it can be surprising. A fork_version_arr or similar distinct name would prevent confusion.

---

Positive Observations

• Input validation: Rust correctly checks shares.len() != withdrawal_addresses.len() / fee_recipients.len() and returns an error, whereas Go would panic on index out of bounds. Improvement.
• timestamp outside loop: In sign_validator_registrations, fork_version_to_genesis_time is called once before the loop. The Go source recomputes it on every iteration — the Rust version is more efficient.
• verify_threshold_partials helper: Extracting the common verify-and-collect logic from agg_deposit_data and agg_validator_registrations is clean and reduces duplication vs. the Go source.
• shares_by_pubkey helper: Same — nice extraction of the repeated index-building pattern.