refactor: Move common nonce helpers from musig to shared module #1938

fjahr commented at 9:49 PM on September 15, 2026: contributor

Factor the secret and public nonce (de)serialization, partial signature (de)serialization, effective nonce computation and secret clearing out of the musig module into a new nonce_common module. These helpers are not specific to musig and can be reused by other modules implementing interactive signing protocols with two-point nonces.

This came out of my work on the FullAgg/Dahlias PR which requires exactly these same helpers but since this refactoring idea can be discussed independently of the new module I am putting it here separately, as suggested by theStack.

theStack added the label tweak/refactor on Sep 22, 2026

in src/nonce_common.h:23 in e210f51657 outdated

  18 | + * (de)serialized so that passing an object to the wrong API fails. */
  19 | +
  20 | +/* Saves a secret nonce into a 132 byte buffer: magic (4 bytes) || k[0] (32
  21 | + * bytes) || k[1] (32 bytes) || pk (64 bytes). */
  22 | +static void secp256k1_secnonce_save(unsigned char *data, const unsigned char *magic, const secp256k1_scalar *k, const secp256k1_ge *pk);
  23 | +static int secp256k1_secnonce_load(const secp256k1_context *ctx, secp256k1_scalar *k, secp256k1_ge *pk, const unsigned char *data, const unsigned char *magic);

IsaqueFranklin commented at 1:56 PM on September 29, 2026:

nit: I think that a comment here for the _load, similar to the one for _save, would be nice to be consistent.

in src/nonce_common.h:45 in e210f51657 outdated

  40 | +static void secp256k1_nonce_points_serialize(unsigned char *out66, secp256k1_ge *ges);
  41 | +
  42 | +/* Saves a partial signature into a 36 byte buffer: magic (4 bytes) || s (32
  43 | + * bytes). */
  44 | +static void secp256k1_partial_sig_save(unsigned char *data, const unsigned char *magic, const secp256k1_scalar *s);
  45 | +static int secp256k1_partial_sig_load(const secp256k1_context *ctx, secp256k1_scalar *s, const unsigned char *data, const unsigned char *magic);

IsaqueFranklin commented at 1:56 PM on September 29, 2026:

nit: A comment for the _load here would be nice too.

fjahr commented at 2:12 PM on November 3, 2026:

Added

in src/nonce_common_impl.h:93 in e210f51657 outdated

  88 | +        secp256k1_ge_serialize33(&ges[i], &out66[33*i]);
  89 | +    }
  90 | +}
  91 | +
  92 | +static void secp256k1_partial_sig_save(unsigned char *data, const unsigned char *magic, const secp256k1_scalar *s) {
  93 | +    memcpy(&data[0], magic, 4);

IsaqueFranklin commented at 2:05 PM on September 29, 2026:

Are magic values expected to always be 4 bytes? Would future modules using these helpers also be expected to use 4 bytes for them? If so, would it make sense to define a constant for the magic size instead of hardcoding 4, in order to make that assumption explicit?

Maybe I'm missing something here.

fjahr commented at 2:12 PM on November 3, 2026:

Yeah, all magics in the library are 4 bytes and every use hardcodes the 4. I am keeping this as is for now since this would be another nice-to-have refactor where I am unclear if it would achieve conceptual agreement, and so staking these on top of each other doesn't seem ideal.

IsaqueFranklin commented at 2:13 PM on September 29, 2026: none

Concept ACK. I like the idea, and that it makes it easier for other possible future modules (e.g. Nested Musig 2, if it ever becomes a module) to use these helpers without having to duplicate code.

IsaqueFranklin commented at 10:38 AM on October 1, 2026: none

It also seems a bit weird to me to leave these nonce helper files "hanging" directly inside the modules directory. A helpers subdirectory under modules for files like these might be an idea, however, this also feels a bit weird. I'm not sure which direction is better.

fametrano commented at 8:13 PM on October 21, 2026: contributor

tACK e210f51

I checked that the refactor changes no behaviour, since the moved code handles the secret nonce. A moved-lines diff against the merge base shows the helper bodies unchanged. The test suite, BIP327 vectors included, passes through the moved helpers in a normal build and in a Debug build with VERIFY and -Werror, on Apple M5 arm64. Logs are available on request.

The non-move edits are the new data and magic parameters, the secp256k1_musig_partial_sign_clear rename to secp256k1_partial_sign_clear at its call sites, the parse and serialize loops now in two helpers, and one reworded comment. One suggestion, in the spirit of IsaqueFranklin's note on the hardcoded 4-byte magic: secp256k1_secnonce_invalidate went from sizeof(secnonce->data), which follows the type, to a bare 132 on an untyped pointer. A shared named size, or a length argument, would keep that tie.

refactor: Move common nonce helpers from musig to shared helpers

Factor the secret and public nonce (de)serialization, partial signature
(de)serialization, effective nonce computation and secret clearing out of
the musig module into a shared nonce_common file. These helpers are not
specific to musig and can be reused by other modules implementing
interactive signing protocols with two-point nonces.

This is a pure refactor without a change in behavior.

1a67459f7b

fjahr force-pushed on Nov 3, 2026

fjahr commented at 2:12 PM on November 3, 2026: contributor

Thanks for the reviews.

It also seems a bit weird to me to leave these nonce helper files "hanging" directly inside the modules directory.

Moved them to src/. They any module code, and src/modules/ now contains only modules again.

secp256k1_secnonce_invalidate went from sizeof(secnonce->data), which follows the type, to a bare 132 on an untyped pointer.

The same goes for sizeof(secp256k1_musig_secnonce_magic), which became 4. Save and load already used literal offsets, so I kept the literals in the helper. Instead, I added STATIC_ASSERT(sizeof(secnonce->data) == 132) to the musig wrapper, so a size mismatch would now fail the build.