feat: implement dkg/frost

[iamquang95]

Fix: #254

[iamquang95]

@claude

[claude]

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

---

Code Review

• Gather context and read changed files
• Review Go source for functional equivalence
• Analyze frost.rs implementation
• Post review findings

---

The implementation is well-structured and closely mirrors the Go source in charon/dkg/frost.go. Tests are meaningful and the in-memory transport is a clean port of Go's frostMemTransport. Below are my findings, ordered by severity.

---

Critical — Interoperability Gap

dkg_context_byte vs Go's actual context encoding (frost.rs:378–383)

In production, Go calls:

defHash := fmt.Sprintf("%#x", def.DefinitionHash)   // e.g. "0x3a7b..."
runFrostParallel(ctx, tp, ..., defHash)

Go's kryptology library takes this string and uses []byte(ctx)[0] — the first ASCII byte of the hex string — so for any "0x..." hash ctx would be 0x30 ('0').

Rust's implementation does:

fn dkg_context_byte(dkg_ctx: &str) -> u8 {
    dkg_ctx
        .parse::<isize>()
        .map(|value| value.to_le_bytes()[0])
        .unwrap_or_default()   // ← silently returns 0 for "0x3a7b..."
}

For a real cluster defHash the function silently returns 0 instead of 0x30. Since ctx is part of the FROST proof-of-knowledge challenge preimage (kryptology_challenge in crates/frost/src/kryptology.rs), this would cause a mismatch in challenge scalars and make cross-implementation DKG impossible.

The existing interop fixtures only cover ctx=0 scenarios, so this gap isn't caught by current tests. The fix needs to agree with Go kryptology's exact string-to-byte conversion (likely dkg_ctx.as_bytes().first().copied().unwrap_or(0)), or the callers need to convert the hash to a numeric string before passing it. Either way, this should be resolved and a fixture with ctx != 0 added.

Fix this →

---

Medium — Code Quality

validate_participant_inputs duplicates validate_round_parameters (frost.rs:362–376)

The private function is identical to validate_round_parameters in crates/frost/src/kryptology.rs — same three checks, same error types. Since kryptology::round1 already calls validate_round_parameters itself, the early validation in DkgParticipant::new doubles the work without adding safety. Either expose validate_round_parameters from the kryptology module and call it here, or rely on kryptology::round1 to do the validation and propagate the error.

Fix this →

Unnecessary clone-then-remove in DkgParticipant::round2 (frost.rs:168–171)

let mut bcasts = bcasts.clone();
let mut shares = shares.clone();
bcasts.remove(&self.id);
shares.remove(&self.id);

The full maps are cloned just to filter out the node's own entry. Since kryptology::round2 already rejects self-IDs with InvalidParticipantId, the removal is redundant (and the caller — get_round2_inputs — doesn't include self anyway since it collects from transport results which don't include the node's own data the same way). If the filtering is intentional and defensive, prefer passing filtered iterators or using filter() to avoid the O(n) clone.

---

Minor

#[allow(dead_code)] at the crate root (frost.rs:1)

All public-facing items are pub(crate) and only used in tests. This attribute is fine as scaffolding but should be removed (or replaced with per-item #[allow]) before integration, so warnings can catch unused code from the outside.

point_to_pubkey double-validates the compressed point (frost.rs:353–356)

fn point_to_pubkey(point: [u8; 48]) -> Result<PublicKey, FrostError> {
    G1Projective::from_compressed(&point).ok_or(FrostError::InvalidPublicKeyPoint)?;
    Ok(pubkey_from_bytes(&point)?)
}

from_compressed is called and immediately discarded; pubkey_from_bytes presumably validates the same bytes. The first call is only needed if pubkey_from_bytes doesn't validate the point, but if that's the case it should be documented. Either inline the check into the pubkey_from_bytes path or add a comment explaining why both are required.

Test: frost_dkg only covers 3-of-3 threshold (frost.rs:526–567)

The integration test runs with threshold = NODES = 3. A 2-of-3 threshold case (as seen in make_shares_sorts_by_validator_index_and_maps_public_shares) would exercise the partial-quorum code path and is the more realistic production configuration.

Test: FrostMemTransport::round1 source_id extraction is fragile (frost.rs:429–433)

let source_id = bcast.keys().next()
    .map(|key| key.source_id)
    .ok_or(FrostError::MissingRoundState)?;

All entries in bcast should share the same source_id, but this is never asserted. A debug_assert! that all key.source_id values are equal would surface bugs during development.

---

Overall

The structure is solid and the port faithfully mirrors the Go implementation. The critical item around dkg_context_byte needs resolution before this can be used in a cross-implementation DKG ceremony. The duplicate validation and unnecessary cloning are cleanup items but don't affect correctness within the current test coverage.

[iamquang95]

dkg_context_byte vs Go's actual context encoding (frost.rs:378–383) Seems not a correct finding.
Go flow:
defHash := fmt.Sprintf("%#x", def.DefinitionHash) (charon/dkg/dkg.go:205)
runFrostParallel(........, ctx = defHash) (charon/dkg/dkg.go:286-287)
And in https://github.com/coinbase/kryptology/blob/master/pkg/dkg/frost/participant.go#L55
strconv.Atoi(ctx) and ignores the error

There is no []byte(ctx)[0]

[varex83agent]

This PR implements the FROST DKG orchestration layer (crates/dkg/src/frost.rs) that parallels Go's dkg/frost.go. The implementation is functionally correct and the end-to-end test validates cryptographic correctness via threshold signature aggregation. Two major concerns block merge: the module is not yet wired into dkg.rs and the file-level #![allow(dead_code)] masks this integration gap; and the test transport's Default derive creates a silently broken zero-node state. Several minor findings cover test coverage gaps (no partial-quorum test, degenerate get_round2_inputs test, no cancellation test) and design concerns (FrostError recursive boxing, undocumented invariant in round2). No bugs found in the crypto path; Go parity is confirmed.

[varex83agent]

Integration gap: run_frost_parallel is not yet called from dkg.rs. The Go equivalent is dispatched at dkg/dkg.go:286 inside case "default", "frost":. The module is declared mod frost (private) in lib.rs and #![allow(dead_code)] suppresses the compiler warning, masking this gap entirely. Until dkg.rs dispatches to frost::run_frost_parallel, the entire module is dead in production.

This attribute should be removed once the wiring is in place (or this PR should include the wiring). Landing #![allow(dead_code)] on hand-written code as a permanent state is against Pluto conventions.

[iamquang95]

This is not included in this PR. Should be on other PR

AI review

[varex83agent]

The new commit addresses 13 of 14 original findings thoroughly. FTransport: Send + Sync fixed; TransportRound1/2 recursive boxing removed (direct ? propagation); round2 implicit contract documented with an explanatory comment and rewritten as a filter; dkg_context_byte annotated; FrostMemTransport::default() footgun removed; transport counters reset on reuse; partial-quorum test (2-of-3) added; get_round2_inputs test rewritten with realistic multi-source input; all nodes' outputs verified in make_shares test; cancellation test added; scalar_to_secret_share inlined; Round1Output type alias introduced.

One finding remains open: #![allow(dead_code)] is still at line 1 and run_frost_parallel is still not wired into dkg.rs. The module is dead in production. This must be removed before merge — either by wiring the call site or by tracking the gap with a // TODO: and a follow-up issue rather than a file-level lint suppression.

ai

[varex83]

LGTM overall, few nits and comments