25 Commits

Author	SHA1	Message	Date
Kgothatso Ngako	3c962b00cc	frost_enrollment: fix the example's stale n and failure-path hygiene ... The last of the review findings, plus the comment and structure fixes it listed. The example's repair run used the pre-enrollment participant count. Two blocks earlier the example teaches that every participant must update its record of n from 3 to 4 after an enrollment, and the signing session duly uses N_PARTICIPANTS_AFTER. Then enroll() -- which hard-coded N_PARTICIPANTS -- ran the repair at n = 3. It worked only because the Lagrange math never involves n and every party in this single-process demo passed the same stale value. In a real post-enrollment repair it would not. n is bound into the parameters hash, so helpers feeding their updated n = 4 into shares_gen while the requester feeds n = 3 abort round 1.2 with no visible cause. enroll() now takes n_participants as a parameter, the repair passes N_PARTICIPANTS_AFTER, and both the function's contract comment and the repair call site say why. The repaired share is still byte-identical to the original, which is the point: n changes the hash, not the arithmetic. The example leaked secrets on its failure paths. enroll() erased the delta and sigma buffers only on success; four early returns left them live. sign_and_verify() returned from three places without erasing already-generated secnonces. Both now route every exit through a cleanup block. This example is otherwise more careful about erasure than its siblings, so the asymmetry was exactly what a reader copying it would carry into production -- on the fault paths where hygiene matters most. The double-wipe of session_secrand is gone with it: shares_gen and nonce_gen both wipe the seed on every path, and doing it again read as uncertainty about the contract. The comment now states the contract instead. The fill_random failure path does erase, since nothing else has touched the buffer there. The example's mismatch message asserted a cause it cannot know. It printed "Helper %u disagrees about the enrollment parameters" for what may equally be a corrupted share, per the previous commit's finding. It now says the helper "contributed a share this helper cannot use", with a comment noting that share_agg does not distinguish the two causes so neither can the message. Comment and structure fixes, all noted in the review: - The vector generator claimed case 4 was "the only case whose DERIVED public share has odd Y". It is not -- cases 1, 2 and 4 are odd and case 3 is even. The comment existed to justify a coverage choice and misinformed; both parity comments now describe the set accurately and say they document it rather than constrain it. Regenerating vectors.h still reproduces it byte for byte. - The secp256k1_frost_sort_ids declaration in frost/session.h no longer duplicates the definition's doc comment, which was two copies to keep in sync. It says what the function is for and points at the definition for the contract. - The t >= 2 rationale was stated in full in three places. The impl now states the conclusion and names frost_enrollment.md as the single place to edit if the policy moves. - The ctime_tests comments read ambiguously ("the parameters hash is public, the delta values are not" against a header calling deltas secret), and computing direct_hash without asserting anything invited a "forgotten assertion" reading. Both are now explicit. - The example moves next to frost_example in Makefile.am rather than after iceberg, matching the FROST-stack grouping used in configure.ac, ci.sh, ci.yml and README. - frost_enrollment.md now distinguishes what is unstable (the C API) from what is frozen (the wire-visible encodings), which the two statements together previously left easy to conflate. Not fixed, deliberately, and now recorded where the tree can see it: the plan called for a CHANGELOG.md entry. That file states in its first two lines that it is upstream libsecp256k1's changelog and not this fork's, and none of frost, chilldkg, iceberg or prefractal has an entry. Adding the first one is a decision about all five modules, not this one. The README link is the fork's actual convention for announcing a module and is in place. Verification: autotools builds warning-free and `make check` is 12/12 including the example; ctime_tests is clean under valgrind; `make dist` carries all nine frost_enrollment files; CMake with examples builds warning-free and ctest is 542/542; the example source is clean under gcc -std=c89 -pedantic -Wall -Wextra; regenerating vectors.h reproduces it byte for byte. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 10:27:17 +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	b66c757b7f	prefractal: document the deviations and add the constant-time test ... doc/prefractal.md writes down the three deliberate deviations from BIP 445 where a reviewer will find them, since none of them is visible from the API and two of them are actively counterintuitive: 1. b_frost does not commit to the message, because the target protocols publish the group's wire nonce before the message exists. The outer b_musig does commit to it and multiplies b_frost everywhere it appears. 2. There is no g_frost factor, and the reason is NOT that the tweak cache is the identity. The frost key-side factor is g*gacc; an identity cache gives gacc = 1, but g is still -1 for every odd-Y threshold key. The doc spells this out because "identity cache, therefore no key term" is the plausible wrong reason, and acting on it yields a signer that works for even-Y groups and fails for odd-Y ones. 3. The frost tweak cache must be the identity, checked at signing and verification and not only at aggregation, so the key a member signs under is tied to the cache that was validated. It also records the two caller obligations the module cannot enforce - one secnonce per signature, and round-two signers EQUAL to round-one contributors - and notes that iceberg tolerates a round-two subset where this module must not, since callers moving between the two would otherwise transpose the rule. The build section documents the three-way ordering constraint rather than leaving the next person to copy iceberg's positions, which are wrong for configure.ac. The ctime test adds a 2-of-2 nested group with a stock musig cosigner, taken as far as one signature share, marking the threshold key, the secret shares and the session randomness as secret and everything else as public. Nonce generation passes msg = NULL, which is how the module is actually driven. Verified: valgrind -q ./build/bin/ctime_tests exits 0, so nothing in the prefractal signing path branches on secret data. Full test suite green. The example program from the plan's optional list is not included; the test suite covers the same ground and the doc carries the usage rules.	2026-09-04 01:06:14 +02:00
Kgothatso Ngako	8c4bf548ed	iceberg: cover the module in ctime_tests ... Port the source tree's own iceberg ctime_tests block (guarded by ENABLE_MODULE_ICEBERG, after the chilldkg block): a 3-of-5 group run through shares_gen, a share serialize/parse roundtrip, share_cache_create (with a CHECKMEM_CHECK proving the cache holds no secrets), pubshare_gen, pubkey_agg, nonce_gen, nonce_agg and partial_sign, with share secrets undefined and all protocol outputs defined, following the frost/chilldkg annotation style. Both memory checkers pass with zero new declassifies needed -- the module's constant-time layering (vpss variable-time but secret-free, scalar_poly keeping secrets away from inversions) was already in place. Verified: valgrind ./ctime_tests exits 0, and the clang MemorySanitizer build exits 0 (with the chilldkg block unaffected).	2026-08-31 12:25:55 +02:00
Kgothatso Ngako	3b68633f59	chilldkg: CI wiring, ctime_tests coverage, declassify fixes ... CI: - ci/ci.sh: new CHILLDKG environment variable, passed to configure as --enable-module-chilldkg (mirroring FROST). - .github/workflows/ci.yml: default CHILLDKG: 'no' and CHILLDKG: 'yes' in every job that enables FROST, except the x86_64 matrix entry that deliberately builds without the ecdh module (chilldkg requires schnorrsig + ecdh; the configure-time dependency error would fire there). YAML validity and per-job dependency presence checked programmatically. ctime_tests: - src/ctime_tests.c: run a full ChillDKG session (n = 2, t = 2) through the public API under the memory checker: hostpubkey_gen, params_hash, participant_step1, coordinator_step1, participant_step2, coordinator_finalize, participant_finalize, participant_recover and recovery_ack_sign. Host secret keys, session randomness, aux randomness and the resulting secret shares are undefined (secret); all protocol messages, the certificate, threshold public key, public shares, recovery data, ack signature and the secret-free state1 objects are defined (public). state2 stays secret (contains the secret share). Constant-time fixes found by running the new block under MemorySanitizer (valgrind unavailable locally; MSan build via clang + CMake). All are missing declassifications of secret-derived but public (or public-outcome) values, following the frost module's secp256k1_declassify pattern with justification comments; no real constant-time bugs were found: - hostpubkey_gen: declassify the computed host public key before serialization (public output). - participant_step1: declassify the zero-randomness check result (only reveals "the RNG returned 32 zero bytes", which aborts the session). - encpedpop participant_step1: declassify the pubnonce point before serialization (public, part of pmsg1). - chilldkg_schnorrsig_sign: declassify the signer public key before normalization/parity branch, and declassify the return value (a failure only reveals a zero derived nonce, negligible probability). - vss_commit: declassify the VSS commitments before serialization (public, part of pmsg1). - vss_verify_secshare: declassify secshare*G before the infinity/eq checks (equals the public pubshare in honest runs; the discrete log is not revealed). - simplpedpop_participant_investigate (proactive audit; not reached by ctime_tests): declassify the secshare-sum comparison result (the public fault code reveals it anyway). Verified: MSan ctime_tests exits 0; autotools make check 10/10 (the local tree is configured without --enable-ctime-tests because neither valgrind nor an MSan-instrumented gcc build is available; CI runs ctime_tests under valgrind as before); CMake ctest 428/428; ./tests --target=chilldkg and ./chilldkg_example pass.	2026-08-31 10:25:14 +02:00
Kgothatso Ngako	7e73badcbd	frost: fix constant-time violations and C90 conformance ... The module's BIP 445 logic itself is unchanged and was independently validated against the pinned spec commit bb5396f (BIP v0.10.0), both via the checked-in test vectors and via differential testing against the Python reference over 360 randomised configurations (n up to 128, shuffled non-contiguous signer ids, mixed xonly/plain tweak chains, variable-length messages, pubshares present and absent). Every change below is structural: the differential harness produces byte-identical pubnonces, aggnonces, partial signatures and final signatures before and after. Two classes of problem prevented the module from passing CI. 1. Constant-time violations (ctime_tests) ----------------------------------------- The CI matrix enables FROST in rows that also run "valgrind --error-exitcode=42 ./ctime_tests" -- WITH_VALGRIND and CTIMETESTS both default to 'yes'. With the module enabled that job reported 639 "conditional jump depends on uninitialised value" errors, all originating from two sites: - secp256k1_frost_derive_coefficient returned !overflow && !secp256k1_scalar_is_zero(out) where the short-circuiting && branches on `overflow`, which is derived from the threshold secret key. The caller declassifies the return value, but the branch has already happened inside the callee. Replaced with a bitwise &, matching the existing idiom in secp256k1_scalar_set_b32_seckey (src/scalar_impl.h). - secp256k1_frost_sign_internal performs the self-verification recommended by BIP 445, which runs the *variable-time* secp256k1_ecmult over the partial signature s. nonce_pts and pk were already declassified ahead of that call; s was not. Since s is the public output of the function, declassifying it before the self-verification is both correct and sufficient. secp256k1_frost_deterministic_sign carried three more instances of the same class, invisible until now because ctime_tests did not exercise that path at all: - the `if (!valid)` check on secp256k1_scalar_set_b32_seckey lacked the declassify that the identical checks in secp256k1_frost_nonce_gen and secp256k1_frost_sign_internal already have; - secp256k1_frost_det_nonce_function used the same short-circuiting &&, here over the secret nonces; - the branch on that function's result was not declassified. The && in det_nonce_function is rewritten via two int locals rather than a bare bitwise &: clang's -Wbitwise-instead-of-logical fires when both operands are `!f(...)` expressions, which would break the -Werror clang builds. ctime_tests now also covers secp256k1_frost_deterministic_sign, so that path stays checked from here on. None of these leak anything of value in practice -- they reveal only negligible-probability events (a hash overflowing the curve order, a zero nonce) or whether a secret share is a valid secret key -- but they violate the project's declassification discipline and fail the ctime test. 2. C90 conformance (-Werror -pedantic-errors) --------------------------------------------- The project targets C90 (CMAKE_C_STANDARD 90, -std=c89 -pedantic) and CI passes WERROR_CFLAGS='-Werror -pedantic-errors'. Compiling src/tests.c with those flags produced 62 errors in three groups: - 40x "ISO C forbids empty initializer braces before C2X" in the generated vectors.h; empty {} initializers are C23-only. Fixed in tools/test_vectors_frost_generate.py so it survives regeneration: hexstr_to_intarray now emits "0" for an empty byte string (all six of its call sites wrap the result in braces), and init_group's `counted` helper emits "{ 0 }" for an empty group. In every affected slot the paired count/length field is 0, so the padding element is never read. - 1x "comma at end of enumerator list" (C99+), also in the generator. - 21x "initializer element is not computable at load time" across 11 lines of tests_impl.h. C90 requires constant expressions in initializers for automatic aggregates, so const secp256k1_frost_pubnonce *ptrs[2] = { &a, &b }; is invalid. Rewritten as a declaration plus assignments, the style the musig tests already use, which is why the pre-existing tree was green. vectors.h is regenerated from the spec's JSON vectors. Its hex payload is byte-identical (verified by hashing every 0xNN token) and the file still reproduces exactly from tools/test_vectors_frost_generate.py. Verification ------------ - gcc and clang, -std=c89 -pedantic-errors -Werror, with and without -DVERIFY: clean (was 62 errors) - ctime_tests under MemorySanitizer: 0 reports (was 639); exits 0 with halt_on_error=1 - tests, noverify_tests and frost_example: pass - vectors.h regenerates identically from the pinned spec vectors - 240 signing + 120 deterministic-signing differential cases against the BIP 445 Python reference: byte-identical to the pre-fix build Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-31 00:57:43 +02:00
Kgothatso Ngako	2d97cc2242	Frost Module logic.	2026-08-31 00:05:16 +02:00
mllwchrry	b1f9e6e360	Merge branch 'master' into sync-ebf59432	2026-07-02 18:43:56 +03:00
kallal79	a77dacad9a	test: enable -Wunused-function in test suite (Fix #1831)	2026-06-12 19:10:33 +05:30
mllwchrry	3b2ceb3e7a	Merge commits '14e56970 1605b02f cd49c57e 453949ab 57315a69 97de5120 c5da3bde 99ab4a10 d071aa56 1d146ac3 322d0a43 c7a7f732 ac561601 dfe042fe 3019186a 95e68158 10f546a2 c0a2aba0 ' into temp-merge-1811	2026-03-03 14:45:28 +02:00
DarkWindman	913be29ea2	Merge commits 'b161bffb 0cdc758a ec329c25 8deef00b f79f46c7 00774d07 2e3bf136 c0d9480f ' into temp-merge-1654	2026-02-18 15:57:18 +02:00
mllwchrry	8d443b8030	musig: Re-add adaptor signatures support	2026-02-13 15:07:52 +02:00
mllwchrry	248358f2bc	Merge commit '3660fe5e' into temp-merge-1479	2026-02-13 13:08:00 +02:00
mllwchrry	21c24fdc7a	musig: Remove module in preparation for upstream merge	2026-02-13 11:36:52 +02:00
8144225309	1bc74a22f8	test: show both Autotools and CMake usage for ctime_tests ... The existing message only shows the libtool command, which is specific to Autotools builds. Fixes #1697	2026-01-22 10:28:30 -05:00
Sebastian Falbesoner	c855581728	test, bench, precompute_ecmult: use EXIT_... constants for main return values	2025-02-14 18:03:36 +01:00
Jonas Nick	f411841a46	Add module "musig" that implements MuSig2 multi-signatures (BIP 327)	2024-10-07 14:03:42 +00:00
Jonas Nick	775f5e242b	Merge commits '1b13415d 374e2b54 96294c00 8d2960c8 ce765a5b b2f6712d eedd7810 b327abfc 5d8fa825 3d05c86d bcffeb14 de657c20 060e32cb 0ba2b945 48b1d939 6b9507ad 5373693e 2e6cf9ba 6ee14550 26a98992 4d7fe609 ea26b71c 65c79fe2 727bec5b 0b4640ae 199d27ce cbf3053f 49be5be9 b10ddd2b 4fd00f4b ba9cb6f3 ee7aaf21 ' into temp-merge-1395 ... - Replace fe_equal_var with fe_equal - Use CHECK_ILLEGAL instead of CHECK/ecount - Turn on secp256k1-zkp specific modules in CI	2023-09-20 09:38:36 +00:00
Tim Ruffing	db32a24761	ctimetests: Use new SECP256K1_CHECKMEM macros also for ellswift	2023-08-15 19:13:09 +02:00
Jonas Nick	74d9073414	Merge commits '83186db3 e9e4526a 5f7903c7 d373a721 09df0bfb 20a5da5f 908e02d5 d75dc59b debf3e5c bf29f8d0 60556c9f cb1a5927 67214f5f 45c5ca76 30574f22 0702ecb0 705ce7ed 3c1a0fd3 10836832 926dd3e9 ac43613d fd491ea1 799f4eec ' into temp-merge-1356	2023-07-27 16:31:40 +00:00
Jonas Nick	0d540ec942	Merge commits '88e80722 ff8edf89 f29a3270 a7a7bfaf a01a7d86 b1579cf5 ad7433b1 233822d8 5fbff5d3 2b77240b 1bff2005 e1817a6f 5596ec5c 8ebe5c52 1cca7c17 1b21aa51 cbd25559 09b1d466 57573187 8962fc95 9d1b458d eb8749fc 6048e6c0 ' into temp-merge-1222	2023-07-20 16:29:40 +00:00
Pieter Wuille	2d1d41acf8	Add ctime tests for ellswift module	2023-06-20 11:31:58 -04:00
Jonas Nick	8f51229e03	ctime_tests: improve output when CHECKMEM_RUNNING is not defined	2023-01-18 09:02:47 +00:00
Tim Ruffing	a7a7bfaf3d	Merge bitcoin-core/secp256k1#1190: Make all non-API functions (except main) static ... e03ef86559 Make all non-API functions (except main) static (Pieter Wuille) Pull request description: ACKs for top commit: real-or-random: utACK e03ef86559 hebasto: ACK e03ef86559. Tree-SHA512: 36a35d9a8da05411c88644aec81e79883febce3e08c9eb6b0ec95cfc3705fd6abfd66f7ee10dfa081ca20786d26b0a060ead7f5c8109bf02a73dde7ef811144b	2023-01-16 18:46:37 +01:00
Pieter Wuille	5048be17e9	Rename valgrind_ctime_test -> ctime_tests	2023-01-11 16:07:37 -05:00