test: cover schnorrsig_sign_custom in constant-time tests #1893

Yudis-bit commented at 5:18 PM on July 17, 2026: contributor

Description

src/ctime_tests.c covers secp256k1_schnorrsig_sign32 and secp256k1_schnorrsig_sign_custom entry points under Valgrind/CHECKMEM.

secp256k1_schnorrsig_sign_custom accepts optional extraparams, including a custom nonce function, before calling the shared internal signer. Functional tests cover this API, but previously it was not exercised under the constant-time CHECKMEM harness.

This PR adds coverage for:

secp256k1_schnorrsig_sign_custom(..., NULL) using the default nonce path with NULL extraparams.
The default BIP340 nonce function with non-NULL extraparams.ndata.
The custom nonce callback dispatch path via a distinct nonce_function_custom callback that delegates to secp256k1_nonce_function_bip340.
Secret key material and keypair state under CHECKMEM for these signing paths.

Message and auxiliary data are not treated as secret inputs in these tests.

Coverage gap

The existing constant-time test suite exercised secp256k1_schnorrsig_sign32, but did not exercise the public secp256k1_schnorrsig_sign_custom entry point and its custom nonce callback dispatch path under CHECKMEM.

Local synthetic mutations were used during development to verify that the added coverage reaches these paths. These mutations are not included in this branch.

This is a test-coverage improvement. No production cryptographic code is modified, and this PR does not claim a production vulnerability in unmodified libsecp256k1.

Change

src/ctime_tests.c only
No production code changes
No new dependencies

Build / test

cmake -B build \
  -DSECP256K1_VALGRIND=ON \
  -DSECP256K1_BUILD_CTIME_TESTS=ON \
  -DSECP256K1_ENABLE_MODULE_SCHNORRSIG=ON \
  -DSECP256K1_ENABLE_MODULE_EXTRAKEYS=ON

cmake --build build --parallel
ctest --test-dir build --output-on-failure
valgrind --error-exitcode=42 ./build/bin/ctime_tests

Local validation after addressing review feedback:

207/207 CTest tests passed.
ctime_tests passed under Valgrind.
Valgrind reported ERROR SUMMARY: 0 errors.
Valgrind process exited with code 0.

Yudis-bit force-pushed on Jul 17, 2026

Yudis-bit marked this as ready for review on Jul 18, 2026

in src/ctime_tests.c:193 in c64c477b09

 188 | @@ -189,6 +189,17 @@ static void run_tests(secp256k1_context *ctx, unsigned char *key) {
 189 |      ret = secp256k1_schnorrsig_sign32(ctx, sig, msg, &keypair, NULL);
 190 |      SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
 191 |      CHECK(ret == 1);
 192 | +
 193 | +    /* Also cover the public sign_custom entry point (default nonce path). */

real-or-random commented at 7:37 AM on July 22, 2026:

nit: Let's remove this comment. This is typical LLM comment: it makes sense in the context of LLM conversation but it doesn't add anything for a reader that looks at the file without this context.

real-or-random commented at 7:38 AM on July 22, 2026: contributor

Good catch.

It would be nice to test also the "custom nonce" path (with a simple custom function).

real-or-random added the label assurance on Jul 22, 2026

real-or-random added the label side-channel on Jul 22, 2026

real-or-random added the label tweak/refactor on Jul 22, 2026

Yudis-bit commented at 2:00 PM on July 22, 2026: contributor

Addressed the review:

removed the redundant comment;
added a distinct custom nonce callback (nonce_function_custom) that delegates to secp256k1_nonce_function_bip340;
exercised the custom callback path with CHECKMEM-undefined secret inputs;
added non-NULL auxiliary-data coverage (extraparams.ndata);
used a variable-length message (sizeof(msg) - 1) for the custom path.

Validation:

ctime_tests build: pass
Valgrind/CHECKMEM clean run: pass (0 errors)
CTest test suites (207 tests): pass (100% passed)
Local-only secret-branch mutations (custom callback branch and non-NULL aux data branch) were detected by Valgrind (exit code 42) and removed before push

Yudis-bit commented at 3:41 AM on September 8, 2026: contributor

Hi @real-or-random , gentle ping on this when you have a chance.

I addressed your review in 81e434f, including the distinct custom nonce callback path, non-NULL auxiliary data, and variable-length message coverage. The branch is still limited to ctime_tests.c, and CI remains green.

Is there anything else you'd like changed here, or would this be ready for another look?

Thanks!

real-or-random requested review from Copilot on Sep 8, 2026

?

copilot_work_started real-or-random

Copilot commented at 9:08 AM on September 8, 2026: none

🟢 Approval recommended

The test-only changes introduce the intended coverage with no unresolved issues.

<details> <summary>Pull request overview</summary>

Adds constant-time CHECKMEM coverage for Schnorr custom-signing paths.

Changes:

Tests default and custom nonce callback paths.
Covers auxiliary randomness and variable-length secret messages.
Taints secret inputs to detect secret-dependent behavior.
</details>

<details> <summary>File summaries</summary>

File	Description
`src/ctime_tests.c`	Extends Schnorr custom-signing constant-time tests.

</details>

<details> <summary>Review details</summary>

Files reviewed: 1/1 changed files
Comments generated: 0
Review effort level: Balanced
</details>

💡 <a href="/bitcoin-core/secp256k1/new/master?filename=.github/skills/code-review/SKILL.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add a code-review agent skill</a> or configure MCP servers for context-aware, tailored reviews. <a href="https://docs.github.com/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review?tool=webui#mcp-servers-and-agent-skills" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn more in the docs.</a>

in src/ctime_tests.c:68 in 81e434f1af

  63 | +        key32,
  64 | +        xonly_pk32,
  65 | +        algo,
  66 | +        algolen,
  67 | +        data
  68 | +    );

real-or-random commented at 7:02 AM on September 9, 2026:

nit: Our style is to have these long function declarations/calls on a single line.

in src/ctime_tests.c:193 in 81e434f1af

 217 | @@ -190,7 +218,6 @@ static void run_tests(secp256k1_context *ctx, unsigned char *key) {
 218 |      SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
 219 |      CHECK(ret == 1);
 220 |  
 221 | -    /* Also cover the public sign_custom entry point (default nonce path). */

real-or-random commented at 7:03 AM on September 9, 2026:

Good to remove this comment but maybe don't even add it in the first commit.

real-or-random approved

real-or-random commented at 7:03 AM on September 9, 2026: contributor

some nits

in src/ctime_tests.c:239 in 81e434f1af

 234 | +    ret = secp256k1_keypair_create(ctx, &keypair, key);
 235 | +    SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
 236 | +    CHECK(ret == 1);
 237 | +    SECP256K1_CHECKMEM_UNDEFINE(&keypair, sizeof(keypair));
 238 | +    SECP256K1_CHECKMEM_UNDEFINE(msg, 32);
 239 | +    SECP256K1_CHECKMEM_UNDEFINE(aux_rand, sizeof(aux_rand));

real-or-random commented at 7:06 AM on September 9, 2026:

Please remove these. msg and aux_rand are not secrets (same below in the second commit)

in src/ctime_tests.c:253 in 81e434f1af

 248 | +    SECP256K1_CHECKMEM_DEFINE(&ret, sizeof(ret));
 249 | +    CHECK(ret == 1);
 250 | +    SECP256K1_CHECKMEM_UNDEFINE(&keypair, sizeof(keypair));
 251 | +    SECP256K1_CHECKMEM_UNDEFINE(msg, sizeof(msg) - 1);
 252 | +    SECP256K1_CHECKMEM_UNDEFINE(aux_rand, sizeof(aux_rand));
 253 | +    ret = secp256k1_schnorrsig_sign_custom(ctx, sig, msg, sizeof(msg) - 1, &keypair, &extraparams);

real-or-random commented at 7:06 AM on September 9, 2026:

Is there are specific reason why sizeof(msg) - 1 instead of sizeof(msg)?

test: cover schnorrsig_sign_custom in constant-time tests 2a3780d73f

test: cover custom Schnorr nonce callback in ctime tests

Exercise schnorrsig_sign_custom with non-NULL auxiliary data and with a distinct custom nonce callback under CHECKMEM. The callback delegates to the BIP340 nonce function so the custom dispatch path is tested without introducing an unsafe test nonce.

38255a943f

Yudis-bit force-pushed on Sep 9, 2026

real-or-random approved

real-or-random commented at 8:08 AM on September 9, 2026: contributor

utACK 38255a943f09831c7615bad854e9b827f6c994fa

real-or-random merged this on Sep 9, 2026

real-or-random closed this on Sep 9, 2026