Rdubois zy0nreview - #4
Open
rdubois-crypto wants to merge 20 commits into
Open
rdubois-crypto wants to merge 20 commits into
rdubois-crypto wants to merge 20 commits into
Conversation
…oseidon bounds
- babyfrost: compute deriveInterpolatingValue in the scalar field
(modOrder/invModOrder) instead of truncating integer division, which
produced wrong shares for non-contiguous signer subsets; add a 3-of-5
regression test that signs with the subset {1,2,4}
- module-index: use toReversed() so poseidon() no longer mutates caller input
- poseidon wrapper: accept up to 16 inputs to match poseidon-lite and the
error message; drop dead alias/comments; @ts-ignore -> @ts-expect-error
- dkg manager: wrap share decryption in a labeled error naming the dealer
- replace file-wide jsdoc/require-jsdoc disables with proper JSDoc
- fix typos: recievePartials -> receivePartials, particpantID -> participantID
- tighten types (any -> Uint8Array / bigint[]), drop redundant null check,
remove stale TODOs and commented-out code
DKGManager.getDecryptedShares decrypted each dealer share but never checked it against that dealer's Feldman commitments, so a malicious or buggy dealer could fold an off-polynomial share into the aggregate signing key undetected. AES-GCM AAD only authenticates transport against the commitment digest, not polynomial consistency. Call verifyFeldmanShare for every dealer and throw on mismatch. Add a regression test that forges an off-polynomial share under a matching AAD digest to isolate the Feldman check from the AEAD tag.
deriveViewKeyFromPK is intentionally a deterministic function of the broadcast dealer commitments so every share-holder derives the same shareable viewing key for engine view access. Document that this value is therefore not secret against anyone who observes those commitments.
finalize() only checked local signature shares (verifySignatureShare needs each signer's secret share, held only locally), so a forged or malformed remote partial produced an invalid aggregate returned without error. Verify the aggregate via eddsaBuild.verifyPoseidon before returning and throw on failure. Per-signer attribution would require exchanging public verification shares and is documented as out of scope. round1() also now clears partialsById so a fresh round cannot aggregate stale partials against newly generated nonces; remoteSigners is left to resetRoundState() because the exchange flow interleaves round1() and addRemoteSigner() across peers. Also fix the misspelled receivePartials method name.
Both copies of deriveInterpolatingValue (BabyFROST and TrustedDKG) rejected
a missing identifier with `!found`, which also rejects a legitimate
identifier of 0. Guard `found === undefined` instead.
Also adds manager-level regression coverage for non-consecutive signer
subsets ([1,2,5], [2,4,5], [1,3,5]), complementing the existing
TrustedDKG-level {1,2,4} case, and corrects the receivePartials spelling
in the README.
combineGroupPubkeyFromCommitments, verifyAllCommitmentsSubgroup, and verifyFeldmanShare tested subgroup membership with ScalarMult(P, order). ScalarMult reduces its scalar mod order, so order became 0 and mulPointEscalar(P, 0) is the identity for every point — the check always passed, accepting commitment points outside the prime-order subgroup (including via the verifyFeldmanShare path the manager now relies on). Call mulPointEscalar(P, order) directly, matching DeserializeElement. Found while writing adversarial tests; regression coverage added in test/trusted-dkg-adversarial.test.ts.
…managers Negative and guard-path tests for curve serialization (identity, length, non-subgroup, scalar range, Montgomery degenerate inputs), shareable and multisig key decode validation and round-trips, and DKG/signing manager input + flow-order guards. Suite 30 -> 97 tests; statements 88.5% -> 96.6%, branches 70.8% -> 83.6%.
The DKG flow state was tracked imperatively via `this.state = ...`
assignments scattered across six methods, and the "is the roster fully
covered?" completeness check was duplicated in three places. The single
linear enum also conflated two independent dimensions: local pipeline
progress (what this participant produced) and collection progress (which
peer commitments / encrypted shares have arrived).
Separate the two: local progress is a small DKGSteps flag record, and
collection progress is queried from the commitment / encrypted-share maps
via one rosterCovers() helper. getState() now derives the linear
DKGFlowState from both, so collecting the final artifact advances the
reported state with no separate bookkeeping. Guards call the derived
state, so behavior (and error strings) are unchanged.
Also:
- progress() exposes { state, awaitingCommitments, awaitingEncryptedShares }
so a driver/UI can see which dealers are still outstanding.
- commitmentRound() no longer self-adds its own commitment; the caller
feeds it back through addParticipantCommitments() like any peer's, one
uniform collection path (matches the documented flow).
- toJSON()/static fromJSON() give JSON-safe, versioned snapshots for
pause/resume across process restarts. Snapshots carry secret material.
- removed a dead validation branch in addEncryptedShares.
New test covers a mid-flow JSON snapshot/restore. Docs updated.
readyToFinalize() returned true at `available >= threshold`, but finalize() interpolates over the entire commitment list and requires a partial from every participant in it. When the participating set exceeded threshold (e.g. 5 signers with t=3), readyToFinalize() reported ready after only `threshold` partials while finalize() threw. Existing tests masked this by always using exactly `threshold` signers. readyToFinalize() now returns getMissingPartials().length === 0. Also harden round/orchestration handling: - Reject a second round1() without an intervening resetRoundState(): it regenerates local nonces while peers still hold the old commitments, so any partial produced is invalid. Tracked via round1CallsSinceReset and checked in sign()/finalize(). (An earlier per-remote epoch approach was wrong — the interleaved exchange lets a node receive all peer commitments before its own round1(), so remote-epoch staleness false-positives on the normal flow.) - addSigner() rejects duplicate identifiers (would corrupt aggregation). - sign() guards empty localBindings with a clear error. - Removed the unused sessions array / createSession() / SigningSession type and the stale todo. - hasId() simplified; sign()/finalize() params renamed msgHash -> message (they were never hashes; primitives hash internally). New tests cover set-exceeds-threshold readiness, the restart guard, and addSigner dedup. Docs updated.
Per-round signing state (local nonces, collected peer commitments, partials) lived in flat fields on the manager, so a single manager could drive only one round at a time and restart relied on the caller remembering resetRoundState(). Move that state into a SigningSession, keyed by id on the manager, so independent signings are isolated and one device can sign multiple messages concurrently. - SigningSession owns localBindings/remoteSigners/partialsById and the full round1 -> sign -> finalize flow; the manager keeps durable identity (signers, group public key, threshold) and a Map of sessions. - startSession()/session()/hasSession()/endSession() manage lifetimes. - The flat methods (round1/sign/finalize/...) are thin delegates to a lazily-created 'default' session, so the single-session API and all existing callers are unchanged. partialsById/remoteSigners/localBindings remain accessible as getters over the default session. - A session is single-commit: round1() throws if already committed (the clean replacement for the round1CallsSinceReset restart guard); reset() restarts it. - finalize(message) now asserts message matches what sign(message) signed. Tests updated for the new restart semantics; added concurrent two-session, re-commit-rejection, and message-mismatch coverage. Suite 101 -> 103.
Bring the signing manager to parity with the DKG manager's pause/resume support. toJSON()/static fromJSON() serialize the manager and every live session to a versioned, JSON-safe snapshot (bigints hex-encoded, points as [x, y] hex pairs) and rebuild them, so a signing can survive a process restart mid-flow. Crucially the snapshot captures each session's local nonces, so a restored manager produces partials that still match the commitments peers already hold. SigningSession gains toSnapshot()/fromSnapshot(); the manager serializes its session map. Snapshots contain secret material (signer shares and local nonces). New test snapshots after the commitment exchange, rebuilds from parsed JSON, and completes signing to a verifying signature. Suite 103 -> 104. Docs updated with a pause/resume section.
Clarify that the group public key and viewing key are deterministic in the input secrets, while individual signing shares are freshly random on every run by design (the higher-order polynomial coefficients come from a CSPRNG). Reproducible shares would be a replay/leakage footgun; recovery should use toJSON()/fromJSON() snapshots, not re-derivation. Pre-empts mistaking the random shares for a bug.
Identifier 0 is the point at which the sharing polynomial evaluates to the shared secret itself, so it can never be a participant identifier. Both copies of deriveInterpolatingValue relied on a falsy `!found` check to reject it by accident; switching that guard to `found === undefined` removed the rejection without putting anything in its place, and neither addSigner nor addRemoteSigner validated the identifier. Reject non-positive identifiers explicitly: - deriveInterpolatingValue (BabyFROST and TrustedDKG) rejects a non-positive x_i and any non-positive id in the signer set, while still rejecting an identifier absent from the set. - addSigner rejects non-integer and non-positive ids. - addRemoteSigner rejects a peer announcing identifier 0, which would otherwise enter the commitment list and shift every honest signer's Lagrange coefficients. Adds coverage for each guard, and for round1() discarding partials that arrived before this node committed (receivePartials has no ordering guard, and such a partial cannot have been computed against a commitment list containing ours). Suite 107 -> 118.
commitmentRound() no longer stores its own commitments; the caller feeds them back through addParticipantCommitments() like any peer's. A caller written against the older self-adding behaviour does not fail at the call site — it silently never reaches commitments-collected because it is waiting on itself, and getEncryptedShares reports only a generic state error. Detect that case and throw naming the participant's own id and the call required to fix it. Adds a regression test driving the old caller pattern and a migration section to the DKG manager docs.
The resumable snapshot serialized each committed session's local nonces so a restored manager could still produce partials matching the commitments peers hold. That is what makes it dangerous to restore twice: the same (hidingNonce, bindingNonce) pair then signs different messages under different challenges, and three such partials give three independent equations in d_i, e_i and s_i — enough to recover the signer's secret share. Nothing distinguished that payload from a harmless one, and the docs warned only about confidentiality, not use count. Separate the two by kind: - toJSON()/fromJSON() stay the default and carry no nonces. Committed sessions are dropped and their ids reported in omittedSessions, so the common pause/resume path cannot reuse a nonce however often it runs. - toMidRoundJSON()/fromMidRoundJSON() carry the nonces and are documented as single use, so resuming a signing already past round 1 has to be spelled out at the call site. Each entry point rejects the other's payload, naming the one to use. Snapshot version 1 -> 2 for the added discriminant. Docs rewritten with the nonce-reuse hazard stated explicitly. Suite 119 -> 122.
The FROST and DKG contexts both advertise FROST-EDBABYJUJUB-BLAKE512-v1, but RFC9591Hasher defaulted to BLAKE2b-512. BLAKE-512 (the original BLAKE, SHA-3 finalist) and BLAKE2b-512 share a digest length and nothing else, so every H1-H6 output, and therefore every binding factor and challenge, differed from what the ciphersuite string promises. An independent implementation written against the label produces partial signatures that aggregate into an invalid signature, with no diagnostic beyond a failed verification. curve.ts already imports the correct function from '@noble/hashes/blake1.js' and uses it for EdDSA-Poseidon key derivation, so FROST and EdDSA were running on two different base hashes within one library. Point the hasher at the same import: no new dependency, and the two paths now agree. Regenerate the frozen H1/H3/H4/H5 vectors in hashes.test.ts, which pinned the BLAKE2b outputs. Add test/blake512.test.ts with digest-level known answers, so that the base hash is pinned directly rather than only through the algebraic FROST tests, which pass under any 64-byte digest.
Round-1 nonce commitments arrive as already-decoded Point<bigint> pairs and never pass through DeserializeElement, so nothing checked them: not curve membership, not subgroup membership, not the identity. An off-curve pair was accepted by addRemoteSigner, encoded into the binding-factor preimage, and folded into the group commitment, where the Edwards addition law is undefined. Add RailJubCurvePoint.assertValidElement, mirroring the checks DeserializeElement already performs on the wire encoding, and call it from the three places a peer point enters: SigningSession.addRemoteSigner, snapshotToCommitment on the restore path, and encodeGroupCommitmentList for consumers driving BabyFROST without the manager. Aggregate verification already turned a malformed peer into a failed round, but without attribution: the honest participants could not tell whom to exclude. Validating at ingestion names the offending peer instead. computeGroupCommitment is left unguarded because computeBindingFactors, and therefore encodeGroupCommitmentList, always precedes it; duplicating the check would cost two scalar multiplications per signer per signature.
Author
|
6cba9e fix #3 |
@zk-kit/baby-jubjub pulled @zk-kit/utils, which pulled the buffer polyfill along with base64-js and ieee754. The package therefore shipped a Node-only dependency chain for what amounts to affine twisted Edwards arithmetic over the BN254 scalar field. After this change src/ contains no reference to Buffer at all, so the bundle runs unmodified in a browser or under wasm. src/babyjubjub.ts provides the same surface -- Point, Base8, r, order, subOrder, a, d, Fr, addPoint, mulPointEscalar, inCurve, packPoint, unpackPoint -- and is byte-for-byte compatible with the implementation it replaces, verified against 400 vectors covering scalar multiplication, addition, curve membership, compression round-trips, scalars at and above the subgroup order, points outside the prime-order subgroup, and the field comparison semantics that the compression sign bit depends on. @noble/curves was not used for the arithmetic. It rejects scalars outside [0, n), and DeserializeElement multiplies an untrusted point by the subgroup order precisely to test membership; reducing that scalar would make the check vacuous. verifyPoseidon has the same requirement for hm * 8. noble is used instead as a validation anchor in the new test, which pins the curve parameters -- including Point.BASE, which noble has changed historically -- and cross-checks arithmetic on every run. packPoint keeps circomlib's sign convention (x greater than (r-1)/2), which RAILGUN addresses and Circom witnesses expect, while SerializeElement keeps RFC 8032 parity for the FROST transcript. The two agree about half the time; a test asserts they still differ, so the encodings are not unified by mistake. Also repoint the signing-manager doc example at src/bytes, which already superseded the @zk-kit/utils import it still showed. No test vector changed. The 144 existing tests still pass, plus 16 new ones.
Author
|
21c5021 remove dependency to zkkit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix Blake512b->blake512 issue #2