feat(dkg): FROST implementation

[emlautarom1]

Partially resolves #254.

---

Based on https://github.com/ZcashFoundation/frost, we build a library implementing FROST compatible with Coinbase's Kryptology (https://github.com/coinbase/kryptology/blob/v1.8.0/pkg/dkg/frost/README.md). No new dependencies are added to the workspace.

I've created some custom Go test cases in Charon that are not part of this PR that generate some fixtures with intermediate values which gives us some initial confidence that we can interop with Go nodes. Still, full compatibility requires further attention.

This PR requires nuanced review due to the challenging cryptography aspects of the code.

[emlautarom1]

@claude Review

[claude]

Claude finished @emlautarom1's task in 7m 8s —— View job

---

Code Review — feat(dkg): FROST implementation

Todo List

• Gather context and understand the request
• Read crates/frost/Cargo.toml and src/lib.rs
• Review crates/frost/src/curve.rs (elliptic curve abstractions)
• Review crates/frost/src/frost_core.rs (core FROST protocol)
• Review crates/frost/src/kryptology.rs (Kryptology compatibility layer)
• Review crates/frost/src/tests.rs and test fixtures
• Review crates/frost/tests/kryptology_interop.rs and kryptology_round_trip.rs
• Review crates/frost/dkg.md documentation
• Compile and post final review

---

This is a well-structured implementation. The protocol math is correct, the test coverage is solid, and the fixture-based cross-language interop is a nice confidence booster. Below are findings by severity.

---

🔴 High — Secret Material Not Zeroized

Round1Secret.coefficients (Vec<Scalar>), SigningShare (wrapping a Scalar), and KeyPackage.signing_share all hold secret key material but have no Drop implementation that clears the memory. Process memory dumps, swap files, or memory-safety bugs elsewhere in the process can recover key material.

// crates/frost/src/kryptology.rs:103-110
pub struct Round1Secret {
    id: u32,
    ctx: u8,
    coefficients: Vec<Scalar>,   // ← secret polynomial coefficients
    ...
}

// crates/frost/src/frost_core.rs:138
pub struct SigningShare(Scalar);  // ← secret scalar, no ZeroizeOnDrop

Recommendation: Add zeroize as a dependency and derive ZeroizeOnDrop for Round1Secret, SigningShare, and any other types that own secret scalars. Fix this →

---

🔴 High — Scalar::from_bytes_wide Does Not Actually Reduce 64 Bytes

The docstring says "Reduce 64 little-endian bytes modulo the scalar field order", but blst_scalar_from_le_bytes internally reads at most 32 bytes (the size of blst_scalar). The upper 32 bytes of the 64-byte input are silently dropped:

// crates/frost/src/curve.rs:71-79
pub fn from_bytes_wide(bytes: &[u8; 64]) -> Self {
    let mut scalar = blst_scalar::default();
    let mut fr = blst_fr::default();
    unsafe {
        blst_scalar_from_le_bytes(&mut scalar, bytes.as_ptr(), 64); // reads only [0..32]
        blst_fr_from_scalar(&mut fr, &scalar);
    }
    Scalar(fr)
}

Effect on Scalar::random (curve.rs:82-86): 64 random bytes are generated but only the first 32 are used. These 32 bytes represent a uniform draw from [0, 2^256). Since floor(2^256 / r) = 2 (BLS12-381 r ≈ 0.453 × 2^256), values in [0, 2^256 mod r) have ~50% higher probability — a statistical distance from uniform of ≈ 9%. For secret polynomial coefficients and Schnorr nonces, this is a meaningful bias.

Effect on kryptology_hash_to_scalar: Only 32 of the 48-byte XMD output are actually processed. The fixture tests passing strongly suggests kryptology's Go blst bindings exhibit the same truncation behavior, so the hash-to-scalar IS compatible — but the function is doing less work than advertised.

Recommendation: Either use rejection sampling (generate 32 bytes, call blst_scalar_fr_check, retry if out of range), or confirm and document that this matches kryptology's exact blst behavior (accepting the minor statistical bias). Also update the docstring to reflect reality. Fix this →

---

🟡 Medium — Missing std::error::Error + Display Impls

Neither DkgError nor FrostCoreError implements std::error::Error or std::fmt::Display. This prevents ergonomic use with ? into Box<dyn Error>, anyhow, or any crate that wraps errors.

// crates/frost/src/kryptology.rs:24
pub enum DkgError { ... }  // no Error, no Display

// crates/frost/src/frost_core.rs:18
pub enum FrostCoreError { ... }  // same

thiserror is already in the workspace. Fix this →

---

🟡 Medium — Round1Secret::from_raw Should Be Test-Only

from_raw exists solely to reconstruct a Round1Secret from fixture data — it has no valid production use case (in real DKG, the secret is always created by round1). It is currently public, which exposes a subtle escape hatch where the polynomial coefficients are faked ([own_share_scalar, 0, 0, ...]). If a caller uses it incorrectly, round2 will silently produce wrong key material.

// crates/frost/src/kryptology.rs:112-143
pub fn from_raw(...) -> Result<Self, DkgError> {
    let mut coefficients = vec![Scalar::ZERO; threshold as usize];
    coefficients[0] = own_share_scalar; // polynomial trick for test replay
    ...
}

Recommendation: Gate behind #[cfg(any(test, feature = "testing"))] or move to the test module. If kept public, document prominently that this is for testing only and that the coefficients stored are synthetic (not the real polynomial). Fix this →

---

🟡 Medium — BlsPartialSignature::from_key_package — Mismatched id Risk

The id parameter is separate from the KeyPackage, and there is no internal check that they agree:

// crates/frost/src/kryptology.rs:483
pub fn from_key_package(id: u32, key_package: &KeyPackage, msg: &[u8]) -> BlsPartialSignature {
    // id is used as the Lagrange x-coordinate but KeyPackage.identifier() is never consulted

If a caller passes the wrong id, Lagrange interpolation will silently produce an incorrect combined signature. The signature won't verify, but there is no runtime diagnostic.

Recommendation: Read id from key_package.identifier() instead of taking it as a parameter, or assert agreement: debug_assert_eq!(Identifier::from_u32(id).map(|i| i.to_scalar()), Ok(key_package.identifier().to_scalar())). Fix this →

---

🟢 Low — expand_msg_xmd Panics Instead of Returning Errors

// crates/frost/src/kryptology.rs:165-168
assert!(ell <= 255, "RFC 9380: ell must be at most 255");
assert!(len_in_bytes <= 65535, ...);
assert!(dst.len() <= 255, ...);

These assertions will panic! if called with oversized inputs. Since the DST is a hardcoded constant and len_in_bytes is always 48, this is never reachable in production — but it makes expand_msg_xmd a footgun for any future caller. Consider returning a Result or making the function pub(crate) and documenting the preconditions.

---

🟢 Low — Blanket #![allow(non_snake_case)] in lib.rs

// crates/frost/src/lib.rs:6
#![allow(non_snake_case)]

This covers the entire crate. The FROST naming convention (R, A0, G) only applies to a handful of local variables in kryptology.rs. Prefer per-function #[allow(non_snake_case)] attributes. Fix this →

---

🟢 Low — blst_p1_mult / blst_p2_mult nbits=255 Should Have a Comment

// crates/frost/src/curve.rs:217
blst_p1_mult(&mut out, &self.0, scalar.b.as_ptr(), 255);
// crates/frost/src/kryptology.rs:613
blst_p2_mult(&mut out, point, s.b.as_ptr(), 255);

This is correct (BLS12-381 r has its highest bit at position 254, so 255 bits suffices), but a one-line comment would save the next reviewer from having to verify it independently.

---

🟢 Low — Glob Re-exports Obscure the Public API Surface

// crates/frost/src/lib.rs
pub use curve::*;
pub use frost_core::*;

This makes it hard to determine the intended public API without reading each module. Explicit re-exports would be clearer for semver stability and documentation. Consider using explicit pub use curve::{Scalar, G1Affine, G1Projective}; etc.

---

🟢 Low — Missing Test Coverage

• validate_num_of_signers with min_signers < 2 and min_signers > max_signers
• from_partial_signatures with duplicate identifiers
• BLS signing with a non-consecutive subset of signers (e.g., participants 1 and 3 out of 1-2-3-4)
• Identifier::from_u32(0) — error case

---

✅ Positive Observations

• Protocol math is correct: The Schnorr proof wi = k + a0·ci with verification R' = wi·G − ci·A0 is consistent throughout.
• RFC 9380 test vectors: expand_msg_xmd is validated against the spec.
• Cross-language fixtures: Fixture interop gives real confidence in wire format compatibility.
• Subgroup check on deserialization: blst_p1_affine_in_g1 + identity rejection in G1Projective::from_compressed prevents small-subgroup attacks.
• Horner's method in evaluate_polynomial: efficient and correct for threshold-degree polynomials.
• Culprit identification in error variants (InvalidProof { culprit }, InvalidShare { culprit }) — important for operational diagnostics.
• Round1Secret not Clone/Debug: prevents accidental log leakage of the secret polynomial.
• missing_docs = "deny" and strict clippy lints (unwrap_used, cast_lossless, etc.) in Cargo.toml — good hygiene.

---

[mskrzypkows]

It can be done without manually reversing:

fn kryptology_hash_to_scalar(msg: &[u8]) -> Scalar {
    let xmd = expand_msg_xmd(msg, KRYPTOLOGY_DST, 48);
    Scalar::from_be_bytes_wide(&xmd)
}

and new Scalar function needed:

    /// Reduce big-endian bytes modulo the scalar field order.
    pub(crate) fn from_be_bytes_wide(bytes: &[u8]) -> Self {
        let mut scalar = blst_scalar::default();
        let mut fr = blst_fr::default();
        unsafe {
            blst_scalar_from_be_bytes(&mut scalar, bytes.as_ptr(), bytes.len());
            blst_fr_from_scalar(&mut fr, &scalar);
        }
        Scalar(fr)
    }

[mskrzypkows]

Round1Secret::from_raw misses the validation logic (lines 282 - 290). What do you think about extracting it to the separate function and using in both places?

[mskrzypkows]

We could check secret.id == sender_id and return an error to prevent some wrong usage of the round2 function.

[mskrzypkows]

Why is it nested ?

[mskrzypkows]

Would be better to limit it to some specific scopes.

[mskrzypkows]

Would be better to limit it to some specific scopes.

[mskrzypkows]

This can be possibly optimized if we would keep u32 value at the construction time:

#[derive(Copy, Clone, Debug)]
pub struct Identifier {
    id: u32,
    scalar: Scalar,
}

impl Identifier {
    pub fn from_u32(id: u32) -> Result<Self, FrostCoreError> {
        let scalar = Scalar::from(u64::from(id));
        if scalar == Scalar::ZERO {
            Err(FrostCoreError::InvalidZeroScalar)
        } else {
            Ok(Self { id, scalar })
        }
    }

    pub fn to_scalar(&self) -> Scalar {
        self.scalar
    }
}

impl Ord for Identifier {
    fn cmp(&self, other: &Self) -> Ordering {
        self.id.cmp(&other.id)
    }
}

[mskrzypkows]

Would be nice to cover all errors with tests.