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>
Fifth of six commits. examples/frost_enrollment.c runs a 2-of-3 group
through an enrollment to 2-of-4, signs with the new participant, and then
repairs a lost share -- all roles in one process, following
examples/frost.c's structure.
The example exists mostly to demonstrate two things the API cannot
enforce and that a reader would otherwise have to reconstruct from the
documentation.
First, the verification flow, in the order that makes it non-circular:
1. obtain thresh_pk from a source authenticated INDEPENDENTLY of the
helpers (here, the dealer step, commented as the stand-in);
2. validate the helpers' public shares against it with
secp256k1_frost_threshold_info_validate;
3. derive the expected public share from those validated shares;
4. only then run round 2, passing the same authenticated thresh_pk.
Skip step 1 or 2 and every check in round 2 still passes -- on a share
from whatever polynomial t colluding helpers chose to present. The
example says so at the point where it would be tempting to skip them.
Second, the authorization gap. There is no authorization step in the
protocol: anyone who convinces t helpers to run it receives a valid
share, and in repair mode that is an existing participant's actual
share. The precondition sits in the comment on enroll(), where a reader
copying the function will see it, and again at the repair call site,
which is where it bites hardest.
Beyond that the example is a working reference for the parts that are
fiddly to get right from the header alone: the transposition between
round 1.1's output buffers and round 1.2's input buffer (helper j
collects entry j of every helper's buffer), the opposite own-slot
conventions of the two round-1.2 buffers, the n -> n+1 bookkeeping with
threshold_info_validate over the extended table, and the fact that the
resulting signature verifies against the group's ORIGINAL threshold
public key, since enrollment changes neither the polynomial nor any
existing share.
The repair half asserts byte equality with the original secret share and
the original public share, so a regression there fails the example rather
than passing quietly.
Wired into both build systems next to the iceberg example: Makefile.am
(noinst_PROGRAMS and TESTS under ENABLE_MODULE_FROST_ENROLLMENT) and
examples/CMakeLists.txt. The .gitignore entry landed with the Phase 1
scaffolding.
Verification. Autotools: builds warning-free and `make check` reports
12/12 PASS including frost_enrollment_example; five consecutive runs exit
0 (the key material is freshly random each time, so this exercises both
threshold-key parities in practice). CMake: with
-DSECP256K1_BUILD_EXAMPLES=ON, ctest runs all nine examples and
secp256k1.example.frost_enrollment passes; the full ctest suite is
523/523 with frost, chilldkg, iceberg, prefractal and frost-enrollment
all enabled. The source is clean under gcc -std=c89 -pedantic -Wall
-Wextra.
Note for anyone reproducing this: examples are OFF by default in both
build systems (--enable-examples for autotools,
-DSECP256K1_BUILD_EXAMPLES=ON for CMake), so a plain build will not
compile this file at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Port examples/iceberg.c from the source tree: a full Iceberg session
demonstrating the call order from the module docs -- distributed key
generation, pubshare_gen/pubkey_agg to obtain the group public key,
nonce_gen/nonce_agg into an ordinary MuSig2 public nonce, and
partial_sign/partial_sig_agg into an ordinary MuSig2 partial
signature.
One content adaptation: the secp256k1_musig_nonce_process call gains
a NULL adaptor argument, matching this repo's zkp musig variant.
Wired like the chilldkg example: autotools noinst_PROGRAMS +
TESTS entry under ENABLE_MODULE_ICEBERG (the example runs as part of
make check), CMake example target in examples/CMakeLists.txt, and
iceberg_example added to .gitignore.
Verified: ./iceberg_example runs to completion (exit 0) under both
build systems.
Final phase of the ChillDKG module: upstream test vectors, a
DKG->FROST integration test, boundary tests, full module
documentation and a runnable example.
Test vectors:
- tools/test_vectors_chilldkg_generate.py converts all 10 upstream
bip-frost-dkg JSON vector files into src/modules/chilldkg/vectors.h
(modeled on tools/test_vectors_frost_generate.py; takes the vectors
directory as an argument; upstream pinned to commit
a91896883f85b159415ecf298d5e844879af112d, recorded in the generated
header with the exact regeneration invocation; regeneration is
reproducible byte-for-byte).
- tests_impl.h vector runners execute 191 of 241 upstream cases
through the public API: hostpubkey_gen, params_hash,
participant_step1/step2/finalize/investigate,
coordinator_step1/finalize/investigate, recover. Happy paths are
byte-exact (pmsg1/cmsg1/pmsg2/cmsg2/dkg_output/recovery/cinv);
error cases assert both the fault enum and fault_index against
expectedError.participantId. The 50 skipped cases are
wrong-length/wrong-count inputs not expressible with the
fixed-size C API; each skip is documented in vectors.h.
Boundary/robustness tests: t=1, t=n, n=2, a full n=128/t=2 session
end-to-end with per-participant secshare*G == pubshare checks and a
recovery roundtrip, and a state1 memcpy roundtrip (step2 from a copied
state object).
DKG->FROST integration test (guarded by ENABLE_MODULE_FROST): a full
ChillDKG session (n=3, t=2) feeds (secshare, thresh_pk, pubshares)
directly into the frost module. ChillDKG's thresh_pk is already
TapTweak'ed, so frost_tweak_cache_init is called with no further
tweaks (frost's tweaked x-only key asserted equal to the x-only part
of the ChillDKG thresh_pk); signers 0 and 2 run nonce_gen, nonce_agg,
session_init with the shared x = id+1 convention, frost_sign,
partial_sig_verify and partial_sig_agg; the aggregate signature
verifies as a plain BIP-340 signature against the threshold key.
Example: examples/chilldkg.c runs a full 2-of-3 DKG session (host key
generation, params hash, participant/coordinator steps, finalize, and
a recovery roundtrip via participant_recover) with fixed-size buffers
and secret erasure. Wired into Makefile.am and
examples/CMakeLists.txt exactly like frost_example (runs as a TEST);
chilldkg_example binary added to .gitignore.
Docs: src/modules/chilldkg/chilldkg.md now documents the protocol
summary, message-flow table with exact byte sizes, blame taxonomy,
recovery workflow, security notes (host key reuse/retention, fresh
randomness per session, state secrecy, recovery-data sensitivity) and
the pinned reference commit; src/modules/frost/frost.md points at the
new module as the intended DKG.
Bug fix found by the vector runner (recover tcId 9): the internal
recover() passed a possibly-NULL fault_index from coordinator_recover
to certeq_verify, which dereferences it on failure; now uses a local.
Verified: make check 10/10 (3 test suites + 7 examples incl.
chilldkg_example, exit 0 when run); CMake ctest 428/428 with chilldkg
+ frost, and a no-frost build confirms the ENABLE_MODULE_FROST guard;
make distdir includes vectors.h, the example and the generator.
The module is feature-complete against bip-frost-dkg v0.3.0-dev at
a91896883f85b159415ecf298d5e844879af112d. The BIP is still a draft;
tagged hashes and wire formats may change upstream.
CI / x86_64: Linux (Debian stable) (clang, map[env_vars:map[CFLAGS:-O1 ECDH:yes ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (clang, map[env_vars:map[ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes WIDEMUL:int128]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (clang-snapshot, map[env_vars:map[BENCH:no BUILD:distcheck CTIMETESTS:no WITH_VALGRIND:no]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (clang-snapshot, map[env_vars:map[CFLAGS:-O1 ECDH:yes ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (clang-snapshot, map[env_vars:map[ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes WIDEMUL:int128]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (gcc, map[env_vars:map[CFLAGS:-O1 ECDH:yes ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (gcc, map[env_vars:map[ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes WIDEMUL:int128]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (gcc-snapshot, map[env_vars:map[BENCH:no BUILD:distcheck CTIMETESTS:no WITH_VALGRIND:no]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (gcc-snapshot, map[env_vars:map[CFLAGS:-O1 ECDH:yes ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (gcc-snapshot, map[env_vars:map[ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes WIDEMUL:int128]]) (push) Has been cancelled
CI / i686: Linux (Debian stable) (clang --target=i686-pc-linux-gnu -isystem /usr/i686-linux-gnu/include, map[env_vars:map[]]) (push) Has been cancelled
CI / MSan (clang, map[env_vars:map[CFLAGS:-fsanitize=memory -fsanitize-recover=memory -fsanitize-memory-param-retval -g CTIMETESTS:no]]) (push) Has been cancelled
CI / MSan (clang, map[env_vars:map[CFLAGS:-fsanitize=memory -fsanitize-recover=memory -g -O3 CTIMETESTS:yes ECMULTGENKB:2 ECMULTWINDOW:2]]) (push) Has been cancelled
CI / MSan (clang-snapshot, map[env_vars:map[CFLAGS:-fsanitize=memory -fsanitize-recover=memory -fsanitize-memory-param-retval -g CTIMETESTS:no]]) (push) Has been cancelled
CI / MSan (clang-snapshot, map[env_vars:map[CFLAGS:-fsanitize=memory -fsanitize-recover=memory -g -O3 CTIMETESTS:yes ECMULTGENKB:2 ECMULTWINDOW:2]]) (push) Has been cancelled
c232486d84 Revert "cmake: Set `ENVIRONMENT` property for examples on Windows" (Hennadii Stepanov)
26e4a7c214 cmake: Set top-level target output locations (Hennadii Stepanov)
Pull request description:
While testing https://github.com/bitcoin-core/secp256k1/pull/1551, I noticed that when cross-compiling a shared library with examples for Windows, the `ctest` fails to run examples with Wine. Adjusting the `PATH` variable in 4af241b320/examples/CMakeLists.txt (L16-L18) does not help because `WINEPATH` is expected.
Another issue with the current implementation is that the examples cannot run individually on Windows.
This PR resolves both issues by reverting the implementation from https://github.com/bitcoin-core/secp256k1/pull/1290 in favour of the reworked and improved implementation from https://github.com/bitcoin-core/secp256k1/pull/1233.
ACKs for top commit:
theuni:
Concept ACK and utACK c232486d84.
real-or-random:
utACK c232486d84
Tree-SHA512: 479b71d15d5d5670f6f69da3da599240c345711003383ca805c821b67065c9baaf269f987792cf1029211cdbfe799aecd401e6940a471539e3929b4a90e0781d
The previous code is correct and harmless to initialize an array with a
non-terminated character sequence using a string literal.
However, it requires exactly specifying the array size, which can be
cumbersome.
Also, GCC-15 may issue the -Wunterminated-string-initialization warning.
[1]
Fix both issues by using array initialization. This refactoring commit
does not change behavior.
[1] Example warning:
src/modules/schnorrsig/main_impl.h:48:46: error: initializer-string for array of 'unsigned char' is too long [-Werror=unterminated-string-initialization]
48 | static const unsigned char bip340_algo[13] = "BIP0340/nonce";
| ^~~~~~~~~~~~~~~
This should hopefully be useful as orientation for users implementing
the key exchange part of BIP324. Conceptually the example is not very
different to the ECDH one, so a lot of code/comments are just copied
(e.g. context creation, secret key generation, shared secret comparison,
console output, cleanup with secret key clearing).
This change simplifies running examples on Windows, because the DLL
must reside either in the same folder where the executable is or
somewhere in PATH.
c6cd2b15a0 ci: Add task for static library on Windows + CMake (Hennadii Stepanov)
020bf69a44 build: Add extensive docs on visibility issues (Tim Ruffing)
0196e8ade1 build: Introduce `SECP256k1_DLL_EXPORT` macro (Hennadii Stepanov)
9f1b1904a3 refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API` (Hennadii Stepanov)
ae9db95cea build: Introduce `SECP256K1_STATIC` macro for Windows users (Hennadii Stepanov)
Pull request description:
Previous attempts:
- https://github.com/bitcoin-core/secp256k1/pull/1346
- https://github.com/bitcoin-core/secp256k1/pull/1362
The result is as follows:
1. Simple, concise and extensively documented code.
2. Explicitly documented use cases with no ambiguities.
3. No workarounds for linker warnings.
4. Solves one item in https://github.com/bitcoin-core/secp256k1/issues/1235.
ACKs for top commit:
real-or-random:
utACK c6cd2b15a0
Tree-SHA512: d58694452d630aefbd047916033249891bc726b7475433aaaa7c3ea2a07ded8f185a598385b67c2ee3440ec5904ff9d9452c97b0961d84dcb2eb2cf46caa171e
It is a non-Libtool-specific way to explicitly specify the user's
intention to consume a static `libseck256k1`.
This change allows to get rid of MSVC linker warnings LNK4217 and
LNK4286. Also, it makes possible to merge the `SECP256K1_API` and
`SECP256K1_API_VAR` into one.
An executable target in the `COMMAND` option will automatically be
replaced by the location of the executable created at build time.
This change fixes tests for Windows binaries using Wine.
8e142ca410 Move `SECP256K1_INLINE` macro definition out from `include/secp256k1.h` (Hennadii Stepanov)
77445898a5 Remove `SECP256K1_INLINE` usage from examples (Hennadii Stepanov)
Pull request description:
From [IRC](https://gnusha.org/secp256k1/2023-01-31.log):
> 06:29 \< hebasto\> What are reasons to define the `SECP256K1_INLINE` macro in user's `include/secp256k1.h` header, while it is used internally only?
> 06:32 \< hebasto\> I mean, any other (or a new dedicated) header in `src` looks more appropriate, no?
> 06:35 \< sipa\> I think it may just predate any "utility" internal headers.
> 06:42 \< sipa\> I think it makes sense to move it to util.h
Pros:
- it is a step in direction to better organized headers (in context of #924, #1039)
Cons:
- code duplication for `SECP256K1_GNUC_PREREQ` macro
ACKs for top commit:
sipa:
utACK 8e142ca410
real-or-random:
utACK 8e142ca410
Tree-SHA512: 180e0ba7c2ef242b765f20698b67d06c492b7b70866c21db27c18d8b2e85c3e11f86c6cb99ffa88bbd23891ce3ee8a24bc528f2c91167ec2fddc167463f78eac
Besides improving the examples, this makes sure that the examples
import a variable (instead of a function), namely the static context,
from the library. This is helpful when testing MSVC builds, because
the MSVC linker tends to be awkward when importing variables.
