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.

[iamquang95]

@claude

[claude]

Claude finished @iamquang95's task in 3m 22s —— View job

---

Code Review

• Read changed files
• Cross-check with Go source (Charon)
• Review aggregate.rs
• Review signing.rs
• Review validators.rs
• Review publish.rs
• Review dkg.rs changes
• Post final review

---

Good port overall — functionally equivalent to Charon for the covered functions. Several issues worth addressing before merge.

---

Functional Issues

1. write_lock_to_api emits info! where Go emits Debug [publish.rs:26]

info!(addr = publish_addr, "Published lock file");

Go uses log.Debug(ctx, "Published lock file to api"). Upgrading to info is a log-level divergence that will emit this message on every DKG run. Since this is routine, debug! matches Go and avoids log noise.

Fix this →

2. shares_by_pubkey misuses InvalidPubKeyFromPeer for a local data error [aggregate.rs:234-238]

let pub_key = PubKey::try_from(share.pub_key.as_slice()).map_err(|_| {
    AggregateError::InvalidPubKeyFromPeer {
        context: "local share",
    }
})?;

InvalidPubKeyFromPeer implies the bad key came from a remote peer. Here the key is from local state — a corrupted or malformed Share struct. A dedicated AggregateError::InvalidLocalSharePubKey variant (or even just panicking, since this indicates a programmer error) would be more accurate and avoids confusing error attribution in production logs.

Fix this →

3. verify_threshold_partials uses a function pointer, blocking contextual error data [aggregate.rs:249-270]

fn verify_threshold_partials(
    partials: &[ParSignedData],
    public_shares: &HashMap<u64, PublicKey>,
    message: &[u8],
    invalid_signature_error: fn() -> AggregateError,  // ← function pointer, not closure
) -> Result<HashMap<u8, Signature>> {

Because the parameter is fn() (not impl Fn()), callers can't pass closures that capture pub_key or share_idx. The Go source includes these fields in every verification error:

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

Changing the parameter to impl Fn() -> AggregateError alone won't help — callers would need to capture pub_key at the call site. A better fix is to make verify_threshold_partials return the share_idx and pubkey in the error, or accept them as extra parameters and build the error internally.

Fix this →

4. create_dist_validators .remove() diverges from Go's non-consuming read [validators.rs:115-117]

Go reads depositDatasMap[pk] without consuming the entry, so a duplicate pubkey would succeed on both iterations. Rust's .remove() means only the first share for a given pubkey gets deposit data; subsequent ones fail with DepositDataNotFound. Add a comment explaining the intentional ownership transfer and why it's safe (shares must have unique pubkeys).

---

Already Addressed (vs Prior Review)

The following items from the previous review have been resolved in this version:

• sign_lock_hash test — now present at signing.rs:262-282, verifies one partial sig per share, correct share_idx, and that the sig verifies against the share's public key. ✓
• agg_validator_registrations_accepts_valid_signatures test — now present at aggregate.rs:322-379. ✓
• ValidatorsError::MissingV1Registration cross-module usage — aggregate.rs now defines its own AggregateError::MissingV1Registration and calls registration_v1() from a local helper. ✓
• fork_version shadowing — fork_version_arr is now used as the name after conversion in agg_validator_registrations. ✓

---

Positive Observations

• logPeerSummary is safer than Go: Rust uses .get(idx) instead of direct indexing operators[i], so it won't panic if operators is shorter than peers. This is a deliberate and correct divergence.
• fork_version_to_genesis_time called outside loop in sign_validator_registrations: Go recomputes it on every iteration; Rust computes it once. More efficient and functionally identical since it's a pure function.
• validate_keymanager_flags returns error for empty addr + non-empty token: Same logic as Go, clean early return guard.
• registration_v1 helper: Extracts the V1-unwrap-or-error pattern used in both aggregate.rs and validators.rs cleanly. Each module has its own copy (avoiding cross-module coupling), which is the right call.

---

[mskrzypkows]

Suggested change

	let mut data = HashMap::new();
	let data = HashMap::from([(core_pub_key, partials)]);
	let msgs = HashMap::from([(core_pub_key, reg.clone())]);

[mskrzypkows]

Suggested change

	if let Some(address) = address {
	let you: Option<&str> = is_current_peer.then_some("⭐");
	info!(peer = peer.name, index = peer.index, address, you, "Peer summary");

[mskrzypkows]

this can replace 254 to 279 lines