5 Commits

Author	SHA1	Message	Date
Kgothatso Ngako	69766dd331	frost_enrollment: close the review's test coverage gaps ... Six new tests and repairs to two that were confounded. The suite goes from 12 cases to 16. Two existing tests would have stayed green with the checks they target deleted, which is the worst kind of passing test: - The "helper id out of range" case also passed new_id = 5 > n = 4, which params_are_valid rejects several lines earlier. It now uses a valid enrollment target (new_id = 4) and an id of 5, so the id range check is the sole failing condition. - Every call in the empty-set test used (n = 1, t = 2), which fails on threshold > n_participants regardless of n_ids. It now uses (n = 2, t = 2, new_id = 2) -- a valid tuple in every respect except n_ids = 0. New coverage, in the order the review ranked it: - run_frost_enrollment_api_test: NULL for every ARG_NONNULL pointer on all five entry points, plus an unusable pubkey object on the four that take one (previously only params_hash was covered). Also asserts that a shares_gen call rejected at an ARG_CHECK does NOT consume the seed, since it never reaches the body -- the complement of the "a failed call always consumes it" property the previous commit pinned. - run_frost_enrollment_infinity_test: pubshare_derive's infinity rejection, which nothing reached before. With u = 2 and new_id = 2 the Lagrange coefficients are exactly -1 and 2, so P_0 = 2*P_1 makes the interpolation vanish; the same two points at a different target succeed, which is what distinguishes the infinity check from a parameter rejection. - run_frost_enrollment_max_size_test: full protocol runs at the largest sizes the API admits -- enrollment at n = 127 with u = 127, and repair in a full n = 128 group with u = 127. u cannot reach 128 in either mode (enrollment needs n < 128, repair excludes the target from the helper set), so these reach one entry below the fixed-size arrays' bound, which is as far as a valid tuple goes. Everything before this capped at n <= 7. Runs in 51 ms. - run_frost_enrollment_no_side_effects_test: the C analogue of the reference implementation's test_participant_not_in_dkg, which plan §1.2 listed and the Phase 3 list dropped. Every existing participant's secret share, public share and the group key are byte-identical before and after an enrollment, and the new share differs from all of them. - The pubshare_derive test now also pins the OTHER end of the polynomial. The public API cannot ask for x-coordinate 0 -- that is identifier -1, and new_id is a uint32_t bounded by n_participants -- so the convention there is checked by running frost's own derive_thresh_pubkey over the same loaded points and requiring it to reproduce the group key. With the existing check at x = new_id, both ends of the interpolation this module depends on are now fixed. Test-structure repairs the review called out: - The mismatch test's disagreement now enters where it would in reality, at helper 0's round 1.1 call (which is made with new_id = 3 while helper 1 uses 4), rather than by running a clean round and overwriting the outputs afterwards. - The oversized-helper-set test re-points a single dealt run at {0,1} and then {0,1,2} through a named helper, instead of struct-copying a ~540 KB run and hand-editing u and ids[2] -- which would have broken silently if deal()'s helper-selection rule changed. - frost_enrollment_test_run instances in the mismatch test are now static. That test needs four live at once, which was over 2 MB of stack. - The randomized test's `if (sub_ids[t-2] >= new_id) continue;` was unreachable: the loop bound gives sub_ids[t-2] <= n-1 and new_id == n in that branch. It is now the CHECK that states the invariant. - The pubshare_derive test's `k` was reset and reused as both helper counter and aligned-array index inside the same loop body. Verification: 16/16 pass at -i=16, -i=200 and -i=1000; ./tests, ./noverify_tests and ./exhaustive_tests exit 0; the module is clean under valgrind (0 errors from 0 contexts). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 10:21:13 +02:00
Kgothatso Ngako	266c6a7c4f	frost_enrollment: fix API contract issues found in review ... Five review findings, all non-blocking, all in the contract between the module and its callers rather than in the cryptography. Each fix comes with a regression test that fails without it. 1. shares_gen zeroed shares32_out before validating n_ids. shares32_out is the only output in this module whose size is caller-supplied. A caller that takes the helper count from a negotiated protocol message, passes a fixed buffer, and relies on this API's "invalid ranges return 0" convention would have memory past that buffer zeroed before the call reported failure -- turning a recoverable length-confusion bug into memory corruption. The frost module validates counts first for exactly this reason (trusted_dealer_keygen, keygen_impl.h:228). Validation now happens before the memset. The early return still wipes session_secrand32, because "a failed call cannot be retried on the same randomness" is a security property and an exception to it would be worse than the tidier control flow. The header's zeroing promise is scoped accordingly: the buffer is zeroed on failure except when n_ids itself is out of range, where it is not written at all. 2. mismatch_id had an undocumented second cause. The header said mismatch_id names the helper whose PARAMETERS HASH disagrees and is UINT32_MAX "when the failure has another cause", but share_agg also sets it when a helper's share is not a valid scalar. The example baked the wrong reading in, printing "Helper %u disagrees about the enrollment parameters" for what may be a corrupted transmission. Documented rather than removed: the attribution is genuinely useful for both causes, and this is API- and vector-compatible. The header now names both, says they are not distinguished so a caller must not report one specifically, and calls out that the second can name the CALLER'S OWN identifier, since the kept share is summed with the rest. The example's message is corrected in a following commit. 3. params_hash's doc claimed it returns 0 on an "unparseable thresh_pk". It does not, and cannot: secp256k1_pubkey_load (secp256k1.c:280) only ARG_CHECKs that x is nonzero, so a zeroed pubkey fires the illegal-argument callback and any other 64-byte content is accepted without curve validation. A caller writing input screening around the documented return 0 would abort on the first malformed input. The doc now states that an unusable pubkey object is API misuse, matching the pointer/value split the impl already follows. 4. params_hash's doc listed three of its ten validity conditions. It is the natural pre-validation entry point -- it enforces exactly what the other four enforce -- but the doc mentioned only duplicate ids and the two n_ids bounds, so the threshold >= 2 divergence and the mode-specific n bounds were discoverable only from the .md or the source. The parameter list now carries the same constraint lines as shares_gen. 5. secshare_gen required a signing context even when it would not sign. The ecmult_gen check was unconditional, but ecmult_gen is used only inside the expected_pubshare != NULL branch. A caller on a verification-only context passing NULL -- explicitly permitted -- hit the illegal-argument callback for a generator multiplication that would never happen. The check is now conditional on expected_pubshare being non-NULL, and stays at the top of the function rather than moving into the branch: ARG_CHECK returns directly, and from inside the branch that would skip the cleanup that wipes secshare and term. Documented in the header. Also in this commit, three comment/dead-code fixes the review noted: the redundant set_int of `term` in both aggregation loops (always written by set_b32 before it is read), the Lagrange denominator comment crediting new_id for something only id distinctness provides, and the comment that described the memset-before-validation ordering rather than justifying it -- now moot. The new run_frost_enrollment_contract_test also closes review coverage gaps 1, 2 and 8, which overlap these findings: malformed wire scalars into share_agg and secshare_gen, sigmas summing to zero mod the order, an invalid secshare32 into shares_gen (the only path exercising its declassify branch), mismatch_id asserted on a NON-CONTIGUOUS helper set {0, 2} so an implementation returning the array index would now be caught, mismatch_id at the caller's own slot, and successful runs with each optional secshare_gen check skipped and with both skipped. Both fixes were verified to be load-bearing by reverting them individually: the F1 test fails on `guarded[i] == 0xa5` and the F5 test fires the illegal-argument callback. ./tests, ./noverify_tests and ctime_tests pass; the module is clean under valgrind (0 errors from 0 contexts). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 10:15:26 +02:00
Kgothatso Ngako	303a7caeae	frost_enrollment: add the test suite and the regression vectors ... Fourth of six commits. Twelve tests replacing the Phase 2 smoke test, plus a vector generator and the frozen vectors it produces. The regression vectors are the one part of this worth being precise about, because they are easy to over-claim. FROST enrollment has no BIP and no published vectors, and the reference proof of concept draws its randomness from secrets.randbits, which is not seedable -- so there is nothing to cross-validate against. tools/test_vectors_frost_enrollment_generate.py therefore re-implements the math independently in stdlib-only Python, including the group arithmetic written from the secp256k1 parameters rather than borrowed, and freezes the output. What that buys: the two tag strings, the params hash serialization, the share-splitting derivation and the identifier conventions are now pinned, and changing any of them is a loud vector-breaking change. What it does not buy is evidence of protocol correctness. The generator header comment and the generated file both say so, as does frost_enrollment.md. The vectors passed on the first run against the C code, which is worth recording: two independent implementations agree byte for byte on the params hash, every delta, every sigma, the derived public share and the final share, across four cases (2-of-3 minimal, 2-of-3 oversized at u = 3 > t = 2, a 3-of-5 repair with a deliberately UNSORTED helper set, and a 4-of-6 enrollment), covering both threshold-key Y parities. The algebraic invariants are what actually carry correctness: - Reconstruction (PoC test_generate_frost_share): after a 2-of-3 group enrolls id 3, every pair {i, 3} reconstructs the original threshold secret, and so does the untouched pair {0, 1}. - Signing (PoC test_sign): a real BIP340 signature from {2, 3} verifying against the unchanged threshold public key, with every partial signature individually verified, plus the n -> n+1 bookkeeping -- secp256k1_frost_threshold_info_validate must accept the public share table extended with pubshare_derive's output at n+1. - Repair: byte-for-byte equality with the lost share, and the repaired participant keeps its old public share. - Oversized helper set: u = 3 and u = 2 over the same key material produce the same share and the same derived public share. - Randomized: COUNT iterations over 2 <= t <= u <= n <= 7, half enrollment and half repair, with EVERY HELPER GIVEN THE IDENTIFIER SET IN ITS OWN SHUFFLED ORDER. The params hash must come out identical while the delta buffers stay aligned per helper -- which is the whole point of canonicalizing ids inside the hash and nowhere else. Each iteration then checks every t-subset containing the new participant. The negative tests are organized around what each gate is actually for: - Fault injection flips a bit in one sigma. secshare_gen fails and wipes its output; the same call with expected_pubshare = NULL SUCCEEDS and returns a wrong share. That second assertion is the point -- it is the evidence that the parameter is load-bearing rather than decorative. Tampered public shares are caught earlier, by secp256k1_frost_threshold_info_validate, so the test exercises the recommended flow and not just the module. - Parameter mismatch, four angles: (a) one helper runs round 1.1 for a different target and every other helper's share_agg aborts naming it by identifier; (b) a caller that IGNORES that abort and finishes round 1.2 anyway still cannot produce a usable share, because the public-share check catches the inconsistent sum -- defence in depth, not a test of the test's own control flow; (c) the helpers agree with each other on new_id = 3 while the target expects 4, which round 1.2 cannot see and round 2's own recomputation does; (d) two groups with identical (t, n, ids, new_id) get different hashes, and a hash from one fails share_agg in the other. - Own-slot semantics: filling the caller's own slot of received_params_hashes32 with garbage changes nothing, because it is never read -- but the same garbage in a slot that IS read still aborts. That pair is what makes "recomputation, not string comparison" testable rather than merely asserted. - Invalid parameters, including both deliberate divergences: t = 1 refused, enrollment refused at n = 128 while repair at n = 128 is accepted, n_ids > 128 returning 0 with the output zeroed in a production build. Three bugs found while writing these, all in the tests, all worth naming: - pubshare_derive takes public shares ALIGNED WITH ids, and the test helper was handing it the participant-indexed table. Those coincide exactly when the helper set is 0..u-1, which every test until the repair case used, so the first non-contiguous helper set {0, 2} was what exposed it. There is now one helper that does the gather, with a comment saying which confusion it exists to prevent. - The fault-injection test compared against r.new_secshare without ever running round 2, and the mismatch test compared against r.params_hashes[0] one line before round 1.1 filled it. Both were reads of uninitialized memory that happened to pass; valgrind found both. The randomized test loops COUNT times so -i scales it, following the iceberg module (tests_impl.h:1322) rather than prefractal's run-once convention -- a fuzzing loop that ignores the iteration count is not much of one. Verification: all twelve tests pass at the default iteration count, at -i=200 and at -i=2000; ./tests, ./noverify_tests and ./exhaustive_tests exit 0 with all five FROST-stack modules enabled; the module runs clean under valgrind (0 errors from 0 contexts); ctime_tests is clean under valgrind; regenerating vectors.h reproduces it byte for byte. One note for anyone running these locally: ctime_tests must not be run against a CPPFLAGS='-DVERIFY' build. secp256k1_scalar_verify branches on scalar values, which ctime_tests deliberately marks secret, so every scalar operation in the library reports a finding -- 75997 of them, none in this module. The CI matrix already pairs -DVERIFY with CTIMETESTS: 'no' (.github/workflows/ci.yml:119, :596) for this reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 04:24:21 +02:00
Kgothatso Ngako	89b253b50e	frost_enrollment: implement the three rounds ... Third of six commits. Replaces the Phase 1 stubs with the real arithmetic, adds a smoke test that a 2-of-3 group really does grow into a working 2-of-4 one, and wires the entry points into ctime_tests. The Lagrange machinery is frost's, called in place. pubshare_derive is a skin over secp256k1_frost_derive_pubshare_at (src/modules/frost/keygen_impl.h:150) evaluated at identifier new_id, and the id canonicalization is secp256k1_frost_sort_ids, reached through the declaration the previous commit added. The one piece frost could not supply is the scalar Lagrange coefficient at an arbitrary point. frost's secp256k1_frost_derive_interpolating_value evaluates at x-coordinate 0, which is what reconstructing the group secret needs; enrollment needs the basis polynomial at the TARGET x-coordinate. secp256k1_frost_enrollment_lagrange_at is that, and it is deliberately the same product derive_pubshare_at applies to each pubshare, in the same identifier space -- so the scalar path and the point path agree by construction rather than by coincidence. Working in identifier space is what makes the id-to-x-coordinate +1 cancel: an x-coordinate difference x_j - x_i is the identifier difference id_j - id_i. Round 1.1 computes v = lambda * secshare and splits it. Every share but the one kept locally is masking randomness derived as Scalar.from_bytes_wrapping( TH("FROST enrollment/share_split", rand32 || params_hash32 || ser32(my_id) || ser32(recipient_id))) with rand32 = TH(same tag, session_secrand32) XOR secshare32; the kept share absorbs the remainder so the set sums to v. Three details: - The reduction wraps rather than rejects, chilldkg's from_bytes_wrapping (src/modules/chilldkg/util_impl.h:394). A 256-bit hash mod the group order is about 2^-128 from uniform; rejection sampling would buy that back in exchange for a variable-time loop. - Masking with the secret share is the secp256k1_frost_nonce_gen pattern (session_impl.h:340), so a broken RNG alone does not reveal the split. - The derivation is indexed by the recipient's IDENTIFIER, not by its position in the caller's ids array. The plan called for a counter; identifiers are unique, so they are one, and using them makes the split independent of the order a caller lists the helper set in. What the binding buys is DOMAIN SEPARATION only: params_hash32 carries the group key and the whole parameter tuple, so two runs sharing a seed but differing in either cannot produce the same deltas. It cannot detect a disagreement between helpers, because nothing cross-checks per-helper private randomness. That is the params hash's job. session_secrand32 is wiped whether the call succeeds or fails, so a caller cannot retry a failed run on the same randomness. Round 1.2 recomputes its own params hash from the group key and the tuple, compares every received hash against it, then sums. The slot at the caller's own position in received_params_hashes32 is skipped, while the same position in all_shares32 is read -- the asymmetry the header documents, and the thing that makes this a recomputation rather than a string comparison. The mode and bounds are re-validated here rather than trusted from the round 1.1 call site, since the full tuple is present. An out-of-range share is reported through mismatch_id the way secp256k1_frost_partial_sig_agg reports an unparseable partial signature. Round 2 compares the params hash against its own recomputation over the authenticated group key, sums, rejects a zero share, and checks secshare*G against the expected public share. Three deviations from the plan, all to match what the tree already does: - Value ranges return 0; only pointers get ARG_CHECK. The plan called for an ARG_CHECK on the n_ids bound, but the frost module's split is the one used here (secp256k1_frost_trusted_dealer_keygen, keygen_impl.h:227), and the header already documents these as return-0 conditions. The bound is still enforced in production builds -- params_are_valid requires 2 <= threshold <= n_ids <= n_participants <= 128 -- so it does not ride on the VERIFY_CHECK inside secp256k1_frost_sort_ids, which is what the plan was guarding against. - The public-share check declassifies the derived point and compares with secp256k1_ge_eq_var, rather than comparing 33 serialized bytes in constant time. There is no constant-time memcmp in this tree, and secshare*G is a public key: secp256k1_frost_sign declassifies exactly this quantity before exactly this comparison (src/modules/frost/session_impl.h:770, :789). Inventing a primitive to avoid following that precedent would be the worse trade. - params_hash's public entry point delegates to the same internal routine every gate uses, so the encoding has exactly one implementation to keep in step with the vectors. One real bug found by the tooling rather than by reading. Accumulators were initialized with secp256k1_scalar_clear, and secp256k1_memclear_explicit marks its target UNDEFINED in VERIFY builds (src/util.h:295) precisely so that reading cleared memory is caught. It was: valgrind reported 143752 errors in share_agg's summation loop. Accumulators now start at secp256k1_scalar_set_int(x, 0); scalar_clear is used only where it means "done with this secret". Worth stating plainly because the failure mode is invisible in a production build, where memclear_explicit only zeroes. ctime_tests gains a 2-of-3-enrolls-a-fourth block covering all three rounds, following prefractal's b66c757b. The threshold key, the secret shares, the session randomness and every delta and sigma on the wire are marked secret; the identifiers, public shares, group key, parameters hashes and derived public share are not. Under valgrind: 0 errors from 0 contexts, so no branch or memory access in the new code depends on secret data. Verification: ./tests, ./noverify_tests and ./exhaustive_tests exit 0; the frost_enrollment module runs clean under valgrind (0 errors); a separate CPPFLAGS='-DVERIFY' build compiles without warnings and passes; the module builds warning-free alongside frost, chilldkg, iceberg and prefractal. The smoke test is the substantive check: after a 2-of-3 group enrolls participant 3, every pair {i, 3} for i in 0..2 reconstructs the original threshold secret and matches the threshold public key, and the untouched pair {0, 1} still does too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 04:06:59 +02:00
Kgothatso Ngako	a7d4337778	build: wire the frost_enrollment module into both build systems ... Second of six commits adding the frost_enrollment module. This one is scaffolding only: the five entry points are stubs that validate their pointer arguments, zero their outputs and return 0. What is being verified here is that the module configures, compiles, links, exports its symbols and registers its test module in both build systems -- so that the next commit changes nothing but arithmetic. Ordering is the one thing in this commit that can go silently wrong, and it goes wrong in opposite directions in the two build systems: - configure.ac executes its `if` blocks in file order, and enable_module_frost defaults to no (configure.ac:243). A block placed after the frost block at :601 that sets enable_module_frost=yes flips the variable too late: AM_CONDITIONAL goes true, so the header is installed and the Makefile fragment is pulled in, but -DENABLE_MODULE_FROST=1 is never appended, so src/secp256k1.c never includes frost's implementation and every secp256k1_frost_* symbol fails to link. The new block therefore goes ahead of both the frost block and prefractal's, which documents the same trap. - src/CMakeLists.txt processes dependents FIRST, so the same block goes above the FROST block there, beside prefractal's. Verified rather than assumed: configuring with ONLY --enable-module-frost-enrollment emits -DENABLE_MODULE_FROST=1 alongside -DENABLE_MODULE_FROST_ENROLLMENT=1, and the CMake summary prints "frost ON" for the same configuration -- the latter is what the PARENT_SCOPE lift buys, since the summary runs after add_subdirectory(src) and would otherwise report a module it is compiling in as OFF. The dependency guard is prefractal's implies-frost idiom, copied verbatim along with its reasoning. frost is default-OFF, so the `test x"$enable_module_frost" = x"no"` / `DEFINED X AND NOT X` guard every other module uses -- which reads as "the user disabled it explicitly" for a default-ON dependency -- is true by default here and cannot tell an explicit --disable-module-frost from the default once both are in the cache. Enabling frost-enrollment simply implies frost, with no error. The one frost-module change in the whole series is in this commit: src/modules/frost/session.h gains a declaration for secp256k1_frost_sort_ids, which is defined at session_impl.h:517 and declared nowhere. The params hash needs it to canonicalize identifier order. Prefractal reaches frost's statics through translation-unit ordering alone; rather than inherit reuse-by-link-order, this declares the function where keygen.h:48 already declares derive_pubshare_at, so the reuse goes through an interface. No behavior change: it is a declaration for an existing static definition in the same TU. CI wiring is two files, and skipping either half fails quietly: - ci/ci.sh gets FROST_ENROLLMENT in the reproduction header's variable list and --enable-module-frost-enrollment="$FROST_ENROLLMENT" after the prefractal line. - .github/workflows/ci.yml gets FROST_ENROLLMENT at every PREFRACTAL site: the global default, 11 inline matrix entries and 10 job-level env blocks. Without the default, ci.sh runs under set -eux with an empty $FROST_ENROLLMENT, passes --enable-module-frost-enrollment="", `test x"" = x"yes"` is false, and the module is off in all of CI while ci.sh visibly has the plumbing. Verified programmatically over the parsed workflow: across the 106 effective job contexts, PREFRACTAL and FROST_ENROLLMENT now agree in every single one (45 set to yes, no mismatches), no context sets FROST_ENROLLMENT without FROST or without EXPERIMENTAL, and no context leaves it undefined. ci.sh passes sh -n. The stub test is not a placeholder that has to be deleted later: every entry point must reject an empty helper set and leave its output zeroed, which is true of the stubs and stays true of the finished implementation, so it doubles as the check that all five symbols are reachable from the test binary. Verification. Autotools: ./autogen.sh, then a frost-enrollment-only configure and a full configure with frost, chilldkg, iceberg, prefractal and frost-enrollment all on -- both build with zero warnings under the project's -Werror-grade flag set, ./tests and ./exhaustive_tests exit 0, and `./tests -l` lists the frost_enrollment module. CMake: configure with -DSECP256K1_EXPERIMENTAL=ON -DSECP256K1_ENABLE_MODULE_FROST_ENROLLMENT=ON builds clean and ctest passes 391 tests. nm shows the five new symbols exported from libsecp256k1.so; tools/symbol-check.py could not be run here because python3-lief is not installed in this environment, but all five carry the required secp256k1_ prefix. make dist succeeds and the tarball carries src/modules/frost_enrollment/frost_enrollment.md alongside the other module documents. One unrelated observation from this build: a stale src/ctime_tests-ctime_tests.o left over from an earlier configure with a different module set will fail to link, because automake does not track CPPFLAGS changes across reconfigures. make clean between configurations with different module sets, not a fault in this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 03:53:03 +02:00