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>

include/secp256k1_frost_enrollment.h

@@ -106,19 +106,31 @@ extern "C" {
  *
  *  This function operates on public data only.
  *
- *  Returns: 0 if the arguments are invalid (duplicate ids, n_ids == 0, n_ids
+ *  This is also the natural place to pre-validate a parameter tuple: it
- *           greater than SECP256K1_FROST_MAX_PARTICIPANTS, unparseable
+ *  enforces exactly the same constraints as the four functions below, and
- *           thresh_pk), 1 otherwise
+ *  nothing else. Note that an unusable `thresh_pk` object is API MISUSE, not
+ *  an invalid parameter: like every other entry point in the library, this
+ *  function reports it through the illegal-argument callback rather than by
+ *  returning 0.
+ *
+ *  Returns: 0 if the parameters are invalid, 1 otherwise
  *  Args:            ctx: pointer to a context object
  *  Out:           out32: pointer to a 32-byte array for the hash. Set to zero
  *                        if this function returns 0.
  *  In:        thresh_pk: pointer to the threshold public key of the group
  *                    ids: array of the u helper identifiers. Every id must be
- *                        unique; the order is irrelevant.
+ *                        unique, smaller than n_participants and different
- *                 n_ids: number of helpers u
+ *                        from new_id; the order is irrelevant.
- *                new_id: identifier of the participant receiving the share
+ *                 n_ids: number of helpers u. Must be between threshold and
- *        n_participants: total number of participants n
+ *                        n_participants.
- *             threshold: threshold t
+ *                new_id: identifier of the participant receiving the share.
+ *                        Must equal n_participants (enrollment) or be smaller
+ *                        than it (repair).
+ *        n_participants: total number of participants n. Must be at most
+ *                        SECP256K1_FROST_MAX_PARTICIPANTS, and strictly
+ *                        smaller in enrollment mode.
+ *             threshold: threshold t. Must be at least 2 (see
+ *                        frost_enrollment.md) and at most n_participants.
  */
 SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_enrollment_params_hash(
     const secp256k1_context *ctx,
@@ -160,8 +172,10 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_enrollment_params
  *  Returns: 0 if the arguments are invalid, 1 otherwise
  *  Args:            ctx: pointer to a context object
  *  Out:    shares32_out: pointer to an array of u*32 bytes for the enrollment
- *                        shares, aligned with `ids`. Set to zero if this
+ *                        shares, aligned with `ids`. Zeroed if this function
- *                        function returns 0.
+ *                        returns 0 -- except when n_ids itself is out of
+ *                        range, in which case the buffer is not written at
+ *                        all, since its size is not known to be u*32.
  *      params_hash32_out: pointer to a 32-byte array for the parameters hash,
  *                        identical to what
  *                        `secp256k1_frost_enrollment_params_hash` returns for
@@ -208,11 +222,27 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_enrollment_shares
  *
  *  The function recomputes its own parameters hash from `thresh_pk` and the
  *  parameter tuple it is given, and compares every entry of
- *  `received_params_hashes32` against it. On the first disagreement it returns
+ *  `received_params_hashes32` against it. It then sums the shares.
- *  0 and, if `mismatch_id` is not NULL, stores the IDENTIFIER of the
+ *
- *  disagreeing helper there (not an array index, which would be ambiguous
+ *  `mismatch_id` reports fault attribution for BOTH ways a specific helper's
- *  because identifiers need not be 0..u-1). `mismatch_id` is set to
+ *  contribution can be at fault:
- *  UINT32_MAX when the failure has another cause.
+ *
+ *    - its parameters hash disagrees with the recomputed one, meaning that
+ *      helper ran round 1.1 on a different parameter tuple or in a different
+ *      group;
+ *    - its entry in `all_shares32` is not a valid scalar (it is not smaller
+ *      than the group order), meaning the value was corrupted in transit or
+ *      fabricated.
+ *
+ *  In both cases the function returns 0 and, if `mismatch_id` is not NULL,
+ *  stores the IDENTIFIER of the responsible helper there -- not an array
+ *  index, which would be ambiguous because identifiers need not be 0..u-1.
+ *  The two causes are not distinguished, so a caller should not report one of
+ *  them specifically. Note that the second cause can name the CALLER'S OWN
+ *  identifier, since the share kept locally is summed along with the rest.
+ *
+ *  `mismatch_id` is set to UINT32_MAX when the failure has neither cause,
+ *  which covers every invalid-parameter and API-misuse case.
  *
  *  Note the deliberately OPPOSITE own-slot conventions of the two u*32 input
  *  buffers, both of which are aligned with `ids`:
@@ -349,7 +379,10 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_enrollment_pubsha
  *                        from the helpers, or NULL to skip the comparison
  *      expected_pubshare: pointer to the expected public share, from
  *                        `secp256k1_frost_enrollment_pubshare_derive`, or NULL
- *                        to skip the verification (not recommended)
+ *                        to skip the verification (not recommended). When it
+ *                        is non-NULL, ctx must have been initialized for
+ *                        signing; when it is NULL, no context capability
+ *                        beyond the default is required.
  */
 SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_enrollment_secshare_gen(
     const secp256k1_context *ctx,

src/modules/frost_enrollment/enrollment_impl.h

@@ -215,7 +215,9 @@ static void secp256k1_frost_enrollment_lagrange_at(secp256k1_scalar *out, const
         secp256k1_scalar_add(&term, &term, &id_i);
         secp256k1_scalar_mul(&deno, &deno, &term);
     }
-    /* deno != 0 because the ids are distinct and new_id is not among them */
+    /* deno != 0 because the ids are distinct. (new_id not being among them
+     * is what keeps num nonzero, which is not required for correctness --
+     * lambda == 0 would simply mean this helper contributes nothing.) */
     VERIFY_CHECK(!secp256k1_scalar_is_zero(&deno));
     secp256k1_scalar_inverse_var(&deno, &deno);
     secp256k1_scalar_mul(out, &num, &deno);
@@ -289,13 +291,30 @@ int secp256k1_frost_enrollment_shares_gen(const secp256k1_context *ctx, unsigned
     ARG_CHECK(secshare32 != NULL);
     ARG_CHECK(thresh_pk != NULL);
     ARG_CHECK(ids != NULL);
-    /* By contract shares32_out holds n_ids entries, whatever n_ids is; the
+
-     * value itself is bounded by params_are_valid below. */
+    /* Validate BEFORE touching shares32_out, which is the only output in this
+     * module whose size is caller-supplied. A caller that passes a fixed
+     * buffer and an out-of-range n_ids -- relying on this API's "invalid
+     * ranges return 0" convention -- would otherwise have memory past the
+     * buffer zeroed before the call reported failure. The frost module
+     * validates counts first for the same reason
+     * (secp256k1_frost_trusted_dealer_keygen, keygen_impl.h:228). */
+    if (!secp256k1_frost_enrollment_params_are_valid(ids, n_ids, new_id, n_participants, threshold)) {
+        /* The seed is consumed on this path too, so that "a failed call
+         * cannot be retried with the same randomness" holds without
+         * exception. params_hash32_out was zeroed above; shares32_out is
+         * deliberately left untouched, which is the whole point of
+         * validating here. */
+        secp256k1_memzero_explicit(session_secrand32, 32);
+        return 0;
+    }
     memset(shares32_out, 0, n_ids * 32);
 
     hash_ctx = secp256k1_get_hash_context(ctx);
     memset(rand32, 0, sizeof(rand32));
 
+    /* Re-validates, which is cheap and keeps this the single place the hash
+     * encoding is produced. */
     if (!secp256k1_frost_enrollment_params_hash_checked(ctx, params_hash32_out, thresh_pk, ids, n_ids, new_id, n_participants, threshold)) {
         goto cleanup;
     }
@@ -379,9 +398,10 @@ int secp256k1_frost_enrollment_share_agg(const secp256k1_context *ctx, unsigned
 
     /* set_int, not scalar_clear: this accumulator is READ on the first add,
      * and secp256k1_memclear_explicit marks its target undefined in VERIFY
-     * builds (src/util.h:295) precisely to catch reads of cleared memory. */
+     * builds (src/util.h:295) precisely to catch reads of cleared memory.
+     * `term` is always written by set_b32 before it is read, so it needs no
+     * initializer -- but it is cleared on the way out. */
     secp256k1_scalar_set_int(&sigma, 0);
-    secp256k1_scalar_set_int(&term, 0);
     memset(params_hash32, 0, sizeof(params_hash32));
 
     /* The full parameter tuple is present here, so the mode and bounds are
@@ -487,11 +507,18 @@ int secp256k1_frost_enrollment_secshare_gen(const secp256k1_context *ctx, unsign
     ARG_CHECK(sigmas32 != NULL);
     ARG_CHECK(thresh_pk != NULL);
     ARG_CHECK(ids != NULL);
-    ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
+    /* The ecmult_gen context is needed only for the public-share check, so it
+     * is only required when that check will run: a caller passing
+     * expected_pubshare == NULL never reaches a generator multiplication and
+     * should not need a context built for one. The check stays here at the
+     * top, where returning early cannot skip the cleanup that wipes the
+     * secrets below. */
+    ARG_CHECK(expected_pubshare == NULL
+              || secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
 
-    /* set_int for the same reason as in share_agg above. */
+    /* set_int for the same reason as in share_agg above; `term` likewise
+     * needs no initializer. */
     secp256k1_scalar_set_int(&secshare, 0);
-    secp256k1_scalar_set_int(&term, 0);
     memset(params_hash32, 0, sizeof(params_hash32));
 
     /* Recomputed from the parameters the target believes and the group key it

src/modules/frost_enrollment/tests_impl.h

@@ -814,6 +814,143 @@ static void run_frost_enrollment_random_test(void) {
     }
 }
 
+/* Contract details that are easy to regress and that a caller can reasonably
+ * depend on. */
+static void run_frost_enrollment_contract_test(void) {
+    frost_enrollment_test_run r;
+    unsigned char guarded[4 * 32];
+    unsigned char hash32[32];
+    unsigned char secrand[32];
+    unsigned char sigma[32];
+    unsigned char out[32];
+    unsigned char all_shares[2 * 32];
+    unsigned char received[2 * 32];
+    uint32_t mismatch_id;
+    size_t i;
+
+    /* An out-of-range n_ids must be rejected WITHOUT writing shares32_out,
+     * whose size is only u*32 by contract: a caller passing a fixed buffer
+     * and a bad count would otherwise have memory past it zeroed. The seed is
+     * still consumed, so a failed call cannot be retried on it. */
+    frost_enrollment_test_deal(&r, 3, 2, 2, 3);
+    memset(guarded, 0xa5, sizeof(guarded));
+    testrand256(secrand);
+    memset(hash32, 0xff, sizeof(hash32));
+    CHECK(secp256k1_frost_enrollment_shares_gen(CTX, guarded, hash32, secrand, r.secshares[0], &r.thresh_pk, r.ids, SECP256K1_FROST_MAX_PARTICIPANTS + 1, r.ids[0], r.new_id, r.n, (uint32_t)r.t) == 0);
+    for (i = 0; i < sizeof(guarded); i++) {
+        CHECK(guarded[i] == 0xa5);
+    }
+    CHECK(secp256k1_is_zero_array(hash32, sizeof(hash32)));
+    CHECK(secp256k1_is_zero_array(secrand, sizeof(secrand)));
+
+    /* The same holds for an n_ids that is merely inconsistent with the rest
+     * of the tuple rather than out of the absolute range. */
+    memset(guarded, 0xa5, sizeof(guarded));
+    testrand256(secrand);
+    CHECK(secp256k1_frost_enrollment_shares_gen(CTX, guarded, hash32, secrand, r.secshares[0], &r.thresh_pk, r.ids, 1, r.ids[0], r.new_id, r.n, (uint32_t)r.t) == 0);
+    for (i = 0; i < sizeof(guarded); i++) {
+        CHECK(guarded[i] == 0xa5);
+    }
+    CHECK(secp256k1_is_zero_array(secrand, sizeof(secrand)));
+
+    /* A valid call does zero the buffer it is allowed to write, and only
+     * that part of it. */
+    memset(guarded, 0xa5, sizeof(guarded));
+    testrand256(secrand);
+    CHECK(secp256k1_frost_enrollment_shares_gen(CTX, guarded, hash32, secrand, r.secshares[0], &r.thresh_pk, r.ids, r.u, r.ids[0], r.new_id, r.n, (uint32_t)r.t) == 1);
+    for (i = r.u * 32; i < sizeof(guarded); i++) {
+        CHECK(guarded[i] == 0xa5);
+    }
+
+    /* secshare_gen needs a signing-capable context only when it is going to
+     * check the public share. With expected_pubshare == NULL it must work on
+     * the static context; with a public share it is API misuse there. */
+    frost_enrollment_test_full_run(&r, 3, 2, 2, 3);
+    memset(out, 0xff, sizeof(out));
+    CHECK(secp256k1_frost_enrollment_secshare_gen(STATIC_CTX, out, r.sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, r.params_hashes[0], NULL) == 1);
+    CHECK(secp256k1_memcmp_var(out, r.new_secshare, 32) == 0);
+    CHECK_ILLEGAL(STATIC_CTX, secp256k1_frost_enrollment_secshare_gen(STATIC_CTX, out, r.sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, r.params_hashes[0], &r.new_pubshare));
+
+    /* Both optional checks are genuinely optional: skipping either, or both,
+     * still produces the same share on an honest run. */
+    memset(out, 0xff, sizeof(out));
+    CHECK(secp256k1_frost_enrollment_secshare_gen(CTX, out, r.sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, NULL, &r.new_pubshare) == 1);
+    CHECK(secp256k1_memcmp_var(out, r.new_secshare, 32) == 0);
+    memset(out, 0xff, sizeof(out));
+    CHECK(secp256k1_frost_enrollment_secshare_gen(CTX, out, r.sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, NULL, NULL) == 1);
+    CHECK(secp256k1_memcmp_var(out, r.new_secshare, 32) == 0);
+
+    /* share_agg reports a share that is not a valid scalar the same way it
+     * reports a parameters disagreement: by naming the responsible helper.
+     * The helper set here is {0, 2}, so an implementation returning the array
+     * index rather than the identifier would be caught. */
+    frost_enrollment_test_deal(&r, 3, 2, 2, 1);
+    CHECK(r.ids[0] == 0 && r.ids[1] == 2);
+    frost_enrollment_test_round1_gen(&r);
+    frost_enrollment_test_collect(&r, 1, all_shares, received);
+    /* All-ones is larger than the group order. */
+    memset(&all_shares[0], 0xff, 32);
+    mismatch_id = 0;
+    memset(sigma, 0xff, sizeof(sigma));
+    CHECK(secp256k1_frost_enrollment_share_agg(CTX, sigma, &mismatch_id, all_shares, received, &r.thresh_pk, r.ids, r.u, r.ids[1], r.new_id, r.n, (uint32_t)r.t) == 0);
+    CHECK(mismatch_id == r.ids[0]);
+    CHECK(mismatch_id == 0);
+    CHECK(secp256k1_is_zero_array(sigma, sizeof(sigma)));
+
+    /* And at the caller's OWN slot, which the header calls out: the kept
+     * share is summed along with the rest, so the caller can be named. */
+    frost_enrollment_test_collect(&r, 1, all_shares, received);
+    memset(&all_shares[32], 0xff, 32);
+    mismatch_id = 0;
+    CHECK(secp256k1_frost_enrollment_share_agg(CTX, sigma, &mismatch_id, all_shares, received, &r.thresh_pk, r.ids, r.u, r.ids[1], r.new_id, r.n, (uint32_t)r.t) == 0);
+    CHECK(mismatch_id == r.ids[1]);
+    CHECK(mismatch_id == 2);
+
+    /* An out-of-range sigma is rejected by secshare_gen too, which has no
+     * attribution to offer. */
+    frost_enrollment_test_full_run(&r, 3, 2, 2, 3);
+    {
+        unsigned char sigmas[2 * 32];
+        memcpy(sigmas, r.sigmas, sizeof(sigmas));
+        memset(&sigmas[32], 0xff, 32);
+        memset(out, 0xff, sizeof(out));
+        CHECK(secp256k1_frost_enrollment_secshare_gen(CTX, out, sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, r.params_hashes[0], &r.new_pubshare) == 0);
+        CHECK(secp256k1_is_zero_array(out, sizeof(out)));
+    }
+
+    /* Sigmas summing to zero mod the group order are rejected: a zero share
+     * is not a usable secret key. secshare = 1 + (order - 1). */
+    {
+        unsigned char sigmas[2 * 32];
+        static const unsigned char order_minus_one[32] = {
+            0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
+            0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFE,
+            0xBA, 0xAE, 0xDC, 0xE6, 0xAF, 0x48, 0xA0, 0x3B,
+            0xBF, 0xD2, 0x5E, 0x8C, 0xD0, 0x36, 0x41, 0x40
+        };
+        memset(sigmas, 0, sizeof(sigmas));
+        sigmas[31] = 1;
+        memcpy(&sigmas[32], order_minus_one, 32);
+        memset(out, 0xff, sizeof(out));
+        CHECK(secp256k1_frost_enrollment_secshare_gen(CTX, out, sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, NULL, NULL) == 0);
+        CHECK(secp256k1_is_zero_array(out, sizeof(out)));
+    }
+
+    /* shares_gen rejects a secret share that is not a valid secret key. */
+    {
+        unsigned char bad_secshare[32];
+        memset(bad_secshare, 0, sizeof(bad_secshare));
+        testrand256(secrand);
+        CHECK(secp256k1_frost_enrollment_shares_gen(CTX, guarded, hash32, secrand, bad_secshare, &r.thresh_pk, r.ids, r.u, r.ids[0], r.new_id, r.n, (uint32_t)r.t) == 0);
+        CHECK(secp256k1_is_zero_array(guarded, r.u * 32));
+        CHECK(secp256k1_is_zero_array(secrand, sizeof(secrand)));
+        memset(bad_secshare, 0xff, sizeof(bad_secshare));
+        testrand256(secrand);
+        CHECK(secp256k1_frost_enrollment_shares_gen(CTX, guarded, hash32, secrand, bad_secshare, &r.thresh_pk, r.ids, r.u, r.ids[0], r.new_id, r.n, (uint32_t)r.t) == 0);
+        CHECK(secp256k1_is_zero_array(secrand, sizeof(secrand)));
+    }
+}
+
 static const struct tf_test_entry tests_frost_enrollment[] = {
     CASE1(run_frost_enrollment_vectors_test),
     CASE1(run_frost_enrollment_reconstruction_test),
@@ -823,6 +960,7 @@ static const struct tf_test_entry tests_frost_enrollment[] = {
     CASE1(run_frost_enrollment_fault_injection_test),
     CASE1(run_frost_enrollment_mismatch_test),
     CASE1(run_frost_enrollment_own_slot_test),
+    CASE1(run_frost_enrollment_contract_test),
     CASE1(run_frost_enrollment_invalid_params_test),
     CASE1(run_frost_enrollment_rejects_empty_set_test),
     CASE1(run_frost_enrollment_pubshare_derive_test),