Chilldkg

[aruokhai]

Implementation Of ChillDKG

[ghost]

🔍 Arkana Protocol Review — ChillDKG Implementation

Verdict: REQUEST CHANGES. This is protocol-critical code (threshold key generation + signing for a Bitcoin L2). Several findings require resolution before merge. Human sign-off mandatory.

Cross-repo impact: None. No other repos in the org currently import threshold-magic.

---

🔴 CRITICAL — Must Fix

C1. FROST signing produces non-BIP-340-compatible signatures despite -TR context string
threshold_frost/src/hasher.go:39-44
The H2 challenge hash uses SHA256(context_bytes || "BIP0340/challenge" || input), which is not a BIP-340 tagged hash (SHA256(SHA256(tag) || SHA256(tag) || msg)). Signatures produced by this implementation will not verify under standard BIP-340 verifiers (btcd schnorr.Verify, Bitcoin Core). If VTXOs use taproot addresses derived from the group key, this is a funds-loss bug.

C2. No taproot tweak in FROST signing flow
threshold_frost/src/signing.go (entire file)
Despite the -TR context string (FROST-secp256k1-SHA256-TR-v1), there is zero taproot tweak logic:

• ComputeChallenge uses vk.E directly — no tweak.
• Aggregate verifies against untweaked pubkeys.VerifyingKey.E.
• ComputeSignatureShare uses raw keyPackage.SecretShare — no secret key adjustment.
  Per RFC 9591 §6.5, the group public key must be tweaked. Without this, signatures are invalid for taproot outputs.

C3. RNG failure in nonce generation silently ignored → nonce reuse → key leak
threshold_frost/src/commitment.go:66

_, _ = io.ReadFull(rng, rb[:])

If ReadFull fails, rb is zeroed. The nonce degrades to H3(0...0 || secret) — deterministic. Two sessions with failed randomness reuse the same nonce, leaking the secret key via the standard Schnorr nonce-reuse attack. This must return an error or panic.

C4. gRPC transport has zero authentication — subscriber impersonation + sender spoofing

• grpc_server.go:116-134 — Subscribe accepts any caller for any address. No proof of identity. An attacker who knows a participant's identifier receives all their messages.
• grpc_server.go:92-113 — Send trusts the client-set From field. Any client can spoof any participant.
• grpc_client.go:28 — insecure.NewCredentials() — no TLS.

The ChillDKG protocol mitigates some attacks (encrypted shares, CertEq), but an active MITM can still cause targeted ceremony aborts, and a passive observer sees all unencrypted protocol messages. Must add TLS + caller authentication before any non-test deployment. If this is intentionally deferred, add prominent // TODO: INSECURE warnings and document the threat model.

---

🟠 HIGH — Should Fix

H1. elemSerializeCompressed does not reject the point at infinity
threshold_core/utils.go:34-48 (referenced in agent review)
If commitment coefficients cancel (producing identity), ToAffine() yields (0,0), which serializes to a compressed point that will be rejected on parse — but silently produces garbage output rather than erroring at the source. Add an explicit infinity check.

H2. bipTapTweak does not check for resulting identity point
threshold_core/chilldkg.go:717-729
After applying the taproot tweak, there's no check that the tweaked public key isn't the point at infinity. Astronomically unlikely from a hash, but should be an explicit error for defense-in-depth.

H3. No reconnection logic in gRPC client
grpc_client.go:44-67
A single stream break (network hiccup, server restart) kills the ceremony with no retry or backoff. Transient failures should not be fatal for a multi-party protocol that may take significant time.

H4. Signature UnmarshalJSON silently reduces Z mod N
threshold_core/dkg.go:75
z.SetByteSlice(zb) reduces mod N without checking overflow. A Z ≥ N in a proof-of-knowledge is invalid but gets silently normalized — potential malleability vector where two serializations produce the same proof.

---

🟡 MEDIUM

M1. No session ID in transport messages — transport.go:15-20
Messages carry Type/From/To/Payload but no session binding. If the relay ever serves concurrent ceremonies (natural evolution), cross-contamination is possible.

M2. Dual subscriber race — grpc_server.go:124
Two subscribers to the same address split messages non-deterministically. Combined with C4, this is a griefing vector.

M3. FromUint16 test helper produces wrong values — threshold_core/test_helper.go:11-27
For input 1 it produces 3, for input 2 it produces 6. Only used in tests for identifier generation (values just need to be distinct + non-zero), but the function doesn't do what its name implies. Could mask test bugs.

M4. Misleading comment in commitment share — threshold_frost/src/commitment.go:50-58
Comment says B_i + b_i * H_i but code computes Hiding + rho * Binding. Code is correct per RFC 9591 §4.4; comment is backwards.

M5. Mul() mutates receiver in-place — fragile aliasing — threshold_frost/src/share.go:32-45
lambdaI.Mul(keyPackageScalar) mutates lambdaI. Safe today because passed by value, but if signatures ever change to pointer receivers, this becomes a corruption bug. Add explicit copies.

M6. Recovery data JSON determinism relies on encoding/json implementation detail
threshold_core/chill_recovery.go:136-174
Map key sorting in json.Marshal is not a language spec guarantee. For canonical serialization in a protocol-critical system, use explicit sorted serialization.

M7. Significant FROST test coverage gaps
Missing tests for: DetectCheater, ErrInvalidCommitment, ErrIncorrectNumberOfCommitments, identity-point commitments, different signer subsets (t < n), nonce reuse detection, taproot tweak, RFC test vectors.

---

🔵 LOW / INFO

• No secret scalar zeroization throughout codebase. Go GC makes this hard but ModNScalar.Zero() exists. Key material may persist in memory.
• validateStructure rejects t < 2 (chilldkg.go:96) — blocks 1-of-n configs. Intentional? Document if so. BIP path is unaffected.
• gRPC Serve error silently discarded — grpc_server.go:72.
• Host secret via env var in integration harness — integration/cmd/dkgnode/main.go:63. Test-only, but ensure this pattern doesn't leak into production templates.
• select {} in test harness has no shutdown mechanism — main.go:84. Fine for tests.
• generateNonce in hybrid path uses random (not deterministic) nonces — acceptable but less reproducible than the BIP path's deterministic nonces.

---

✅ What's Good

• RecoveryData design is solid: no secrets at rest, share recovered on-demand from host key + public blob.
• BIP vector conformance (51/51): strong evidence the core ChillDKG protocol is correct.
• Blame attribution is well-implemented: coordinator tampering correctly blamed, investigation sub-protocol pins the culprit.
• Fresh ephemeral material per ceremony: new private key + coefficients per Run(), never derived from host key.
• collectFromAll deduplication: prevents flooding attacks from displacing honest participants.

---

Required Actions Before Merge

1. Resolve C1+C2: Clarify whether FROST signing is intended to be BIP-340/taproot-compatible. If yes, fix the tagged hash construction and add taproot tweaking. If no, rename the context string to remove -TR and document that signatures are internal-only.
2. Fix C3: Propagate RNG errors in nonce generation. This is a textbook key-leak vector.
3. Address C4: At minimum, add // SECURITY: INSECURE TRANSPORT banners, document the threat model, and create a tracking issue for TLS + auth. Ideally add mTLS before merge.
4. Fix H1: Add infinity check in elemSerializeCompressed.
5. Add missing test coverage per M7.

⚠️ PROTOCOL-CRITICAL: This PR implements threshold key generation and signing for a Bitcoin L2. Human review and sign-off required before merge, per Arkana protocol rules.

[aruokhai]

What this PR achieves

This PR lands the ChillDKG (BIP-FROST-DKG) layer and an embeddable leader / follower distributed-key-generation SDK on top of the existing secp256k1 / FROST primitives.

1. Byte-exact BIP-FROST-DKG conformance (threshold_core)

• A complete BIP* code path (BIPParticipantStep1/Step2, BIPCoordinatorStep1/Finalize/Investigate, BIPParticipantFinalize, BIPRecover, BIPParamsID, BIPHostpubkeyGen) that is byte-identical to all 51 upstream BlockstreamResearch/bip-frost-dkg test vectors (key generation, recovery, and the investigation/blame verdicts) — vectors copied verbatim into testdata/*_vectors.json.
• ChillDKG = EncPedPop (additive ECDH-padded shares, safe over any transport) + CertEq (each party BIP340-signs a canonical transcript hash; N valid signatures form a success certificate, defeating an equivocating coordinator) + an identifiable blame / investigation sub-protocol that roots fault in the host keys.
• Deterministic params_id (no session nonce; per-session freshness comes from a fresh ephemeral key bound into every pad) and a canonical, public RecoveryData blob from which any member recovers its share with just its host key.

2. Leader / follower DKG SDK (threshold_sdk)

A threshold construction where one leader owns the group secret a0 and is designed to run inside a secure enclave (TEE), so the secret is hardware-isolated from the host and operator — the trust anchor is the attested enclave, not a custodian.

• The leader is both coordinator and a participant; every follower contributes the identity element (a0 = 0, i.e. the point at infinity), so the aggregate secret is the leader's a0 and the group key is g^(a0) (taproot-tweaked, deterministic in a0).
• a0 is generated once and persisted, so the group public key is stable across every membership change — only the committee, threshold, and shares move.
• Leadership is threaded as an explicit leader *Identifier argument to the BIP* functions (deliberately not part of params_id), so the no-leader path stays standard, byte-exact BIP.

3. Dynamic membership

• Join — a follower registers via a Join RPC, proving possession of its host key (BIP340 join proof); the leader admits it and re-keys.
• Ban — the leader evicts a member; survivors re-key and the evicted node is told to stop.
• Each change triggers a re-DKG that reuses the same a0 (the key never moves), with the threshold following the majority t = ⌊n/2⌋ + 1 of the current members.
• Authenticated leader commands — rekey/removed commands are BIP340-signed by the leader's host key over a tagged hash with a monotonic epoch (anti-replay); followers pin the leader key and verify before acting, independent of the (insecure) relay.
• Drop-unresponsive re-DKG: a member that doesn't contribute in time is evicted and the ceremony retries over the responsive remainder.

4. Transport & persistence

• Insecure gRPC relay (Server) + Client, with dynamic per-member subscriptions; an in-process loopback transport lets the leader run the coordinator and its own participant half in one process.
• Injectable Store (BadgerDB), holding only the public recovery blob, keyed by the group verifying key.

5. Docker integration test (threshold_sdk/integration)

A real distributed leader/follower DKG across containers (testcontainers) drives one long-lived leader through a full membership ramp — five followers join one at a time, then three are banned — asserting at every step that the group public key never moves and the signing threshold tracks the majority:

	n (leader + followers)	t
joins	2 → 3 → 4 → 5 → 6	2, 2, 3, 3, 4
bans	5 → 4 → 3	3, 3, 2

Group size and threshold are read from the leader (only advancing after a re-DKG fully succeeds), so the observed (size, t) sequence is itself proof each transition completed.

Also

• RFC 9591 FROST signing module (threshold_frost).
• Module consolidation: folded the transport-agnostic ChillDKG drivers into the thresholdsdk module.
• A top-level README.md documenting the architecture, the leader/follower + enclave trust model, and the integration test.

Notes

• Trust model is custodial-by-enclave, not trustless: the leader knows the full secret by construction and must run in an attested enclave; followers hold threshold shares for resilience/recovery.
• The gRPC transport ships insecure (no TLS); command authentication is end-to-end and independent of it.

[arkana-ai-bot]

PROTOCOL-CRITICAL: human review required.

This PR adds ~9k lines of ChillDKG / EncPedPop / CertEq / FROST-signing plumbing and rewires the whole SDK layer. It sits directly on Bitcoin key material, so a specialist in bip-frost-dkg must re-audit the wire encodings and hash tag domains against the reference vectors before this merges — vector-driven tests exist (threshold_core/testdata/*.json) but I could not execute them from this review environment.

Below are correctness / security findings from the diff read.

Protocol-correctness fixes I'm glad you shipped

• threshold_frost/src/signing.go:77-99, 88-99 and utils.go:33-45: iteration over s.Commitments is now sorted via sortedCommitmentIDs. The previous code (for id, comm := range groupCommitmentList inside encodeGroupCommitmentList, for id := range s.Commitments inside bindingFactorPreimages) fed a nondeterministic Go map iteration into the binding-factor preimage, so any two processes signing the same SigningPackage would derive different binding factors and the aggregate s = Σ z_i would not verify. Real distributed-signing bug, real fix.
• threshold_frost/src/signing.go:96-99: ComputeGroupCommitment error return is now actually checked.
• threshold_frost/src/signing.go:171-207: Aggregate used to call DetectCheater only when the signature verified and swallow the error path entirely (returning sig, err where err was the last stale err from ComputeGroupCommitment, i.e. nil). Now DetectCheater runs on verification failure, which is what the function name promises. Please add a targeted test for this path — see below.

Findings

1. gRPC relay does not authenticate the From field (High)

threshold_sdk/grpc_server.go:87-99 (Server.Send) forwards env.From verbatim; threshold_sdk/grpc_client.go:96-99 and loopback.go:37-38 set From client-side with no signature. threshold_sdk/leader.go:441-467 (Leader.collect) accepts the first message it sees for a given From and positions it into the pmsg1/pmsg2 array fed to BIPCoordinatorStep1 / BIPCoordinatorFinalize. Impact: any joined follower can race-send a malformed pmsg1 with another party's From and cause BIPCoordinatorStep1 to return BIPFaultyParticipantError{Participant: <honest>} (threshold_core/chilldkg.go:2120) — the leader then evicts the honest party in rekey's loop. The protocol's PoP + CertEq crypto stops the attacker from completing a run as the victim, but they can force false eviction / DoS. Recommend: wrap each pmsg1/pmsg2 in a host-key signature over (SessionID, epoch, msgType, payload) and drop unauthenticated messages at the relay boundary, or bind From to the Subscribe stream's req.Addr (per-connection identifier pinning).

2. Leader epoch is not persisted (High)

threshold_sdk/leader.go:83-116 initialises epoch to zero on every NewLeader, and only mutates it in-memory. The follower rejects cmd.Epoch <= curEpoch (follower.go:132). After a leader restart, followers that already saw epoch > 0 will silently discard every legitimate rekey/removed command until the leader climbs back past the highest previously-seen value. Recommend: persist epoch in Store (bump-then-write before Broadcast), or seed from time.Now().UnixNano().

3. Duplicate-join race in Leader.handleJoin (Medium)

threshold_sdk/leader.go:378-403: under l.mu, handleJoin scans l.committee for the host pubkey, then queues evJoin on l.events. applyJoin is only run later by the Serve loop, so two concurrent Join RPCs with the same host key both pass the duplicate check, both get distinct IDs from assignIDLocked, and both are added to the committee. The next rekey will then run with the same host key at two positions — SessionParams.validateStructure (threshold_core/chilldkg.go:1636-1650) actively rejects duplicate host pubs and returns ErrDuplicateHostKey, so the ceremony aborts. Fix: dedupe by host pubkey inside applyJoin under l.mu, or gate handleJoin on a set of pending pubkeys.

4. RecoveryData.Verify() does not cover EncShares (Medium — spec divergence)

The hybrid transcript in threshold_core/certeq.go:1082-1155 (serializeTranscript) omits EncShares — only SessionID, T, N, hostpubs, round1, ephPubs, group, vk are hashed. RecoveryData.EncShares is therefore not certified by the N-of-N cert; a mutated blob passes Verify() and only fails later inside RecoverShare → DecryptAndVerifyShare. Contrast the BIP-exact path (bipEqInput, chilldkg.go:2273-2291) which does include encSecshares. Either fold EncShares into serializeTranscript (aligns with BIP) or document loudly that Verify() is a certificate check, not a blob-integrity check. This also matters for Leader.rekey (leader.go:265-268) which stores the blob after CertEq passes without ever calling DecryptAndVerifyShare on every recipient.

5. ParseRecoveryData silently reduces oversized EncShares scalars (Low)

threshold_core/chill_recovery.go:1445-1446: s.SetByteSlice(sb) return value is ignored, so a 32-byte payload encoding a value ≥ n gets silently reduced. BIPRecover (chilldkg.go:2748-2752) explicitly rejects this with ErrInvalidRecoveryData. Make the JSON parser match. Same issue in RandomIdentifier (identifier.go:4234) — negligible bias for secp256k1 but violates uniformity; the reference uses full rejection sampling.

6. signing_test.go covers only the happy path (Low)

threshold_frost/src/signing_test.go:110-160 (TestSignAndAggregateEndToEnd) verifies one successful sign+aggregate. The Aggregate logic change is what should be regression-tested: add cases that (a) pass fewer commitments than MinSigners, (b) mismatch a SigningNonce against its commitment, (c) tamper one SignatureShare and confirm DetectCheater returns FaultyParticipant, and (d) tamper the message post-Sign and confirm the new challenge-based verification catches it. Also, the pre-fix nondeterministic ordering bug (finding above) would still slip past this test because Go map iteration is deterministic within one process — add a distributed-simulation test that constructs each participant's SigningPackage fresh from an unordered source (e.g. shuffled slice → map on each side).

7. Certeq / signing verifier internal consistency (spot-check needed)

Aggregate now open-codes the verification (z·G ?= R + c·Y) using ComputeChallenge (threshold_frost/src/signing.go:181-201) instead of pubkeys.VerifyingKey.Verify. The two must derive the challenge with the same tag/domain — please confirm ComputeChallenge and VerifyingKey.Verify (threshold_core/share.go) use identical hash tagging. If they diverge, Aggregate would accept signatures that VerifyingKey.Verify rejects (and vice versa). Untested here.

8. Module rename is monorepo-local — good

ArkLabsHQ/thresholdmagic/thresholdcore → ArkLabsHQ/threshold-magic/threshold_core touches only the three sibling go.mods (frost, nostrdkg, sdk) inside this repo. Grep across /srv/arkana/repos finds no external consumers — no cross-repo breakage.

Nits

• threshold_sdk/grpc_client.go:53: uses insecure.NewCredentials(); document explicitly that this transport must be tunnelled through TLS/mTLS in any real deployment. Currently only a // comment ("Insecure gRPC transport") in grpc_server.go:5.
• threshold_sdk/leader.go:275: l.notifyRemoved sends msgRemoved before dropping the sub; if the removed follower's Subscribe channel is full, the send blocks on case ch <- msg: in Server.SendTo until ctx cancels. Consider a short per-send timeout.
• chilldkg.go:1985-2010: role logic sets coeffs[0] = a0 after bipVSSGenerate has already derived a seed-driven coeffs[0]; the seed-derived value is then unused. Consider deriving coeffs[1:] separately for the leader path to avoid the wasted scalar (and remove any implication that the leader's a0 is deterministic from the seed).
• chilldkg.go:2004: overflow := a0.SetByteSlice(leaderSecret); the len != 32 guard is short-circuit-ORed with overflow. If callers ever pass 33+ bytes, SetByteSlice reads only the first 32 — length check is safely first, but worth adding a comment that the order is load-bearing.
• LeaderGroupPubkey in chilldkg.go:2422 — same order-load-bearing pattern; consider factoring into a parseLeaderSecret helper reused everywhere.

Suspicious content in inputs

Nothing in the PR title/body/diff attempted prompt injection. No further action.

Recommendation

Do not merge until (1) findings 1 and 2 are addressed (they are exploitable in the leader/follower deployment this PR advertises), (2) findings 4 and 6 have concrete resolution (fix or documented deferral), and (3) a bip-frost-dkg-literate reviewer signs off on the wire encodings against the upstream vectors. The core ChillDKG logic looks well-structured and the fixes to signing.go are welcome, but the surface area is large and the trust model of the SDK layer needs tightening.