musig: Require generated secnonce for partial sig #35422

Previously, MuSig2SecNonce pre-allocated secure memory in its constructor, so IsValid() was true before secp256k1_musig_nonce_gen ran. secp256k1_musig_partial_sign could be reached with an uninitialized secp256k1_musig_secnonce causing libsecp to crash.

This patch defers secure allocation until CreateMuSig2Nonce succeeds, guards CreateMuSig2PartialSig when no secnonce was generated, and adds a unit test for the lifecycle.

Test plan

/build/bin/test_bitcoin --run_test=bip328_tests/secnonce_lifecycle

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35422.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

cc @real-or-random @jonasnick

It shouldn't be possible to hit any error with this as the nonce is not stored if nonce generation fails.

Instead of a bool that needs to track state, the unique ptr can be created in CreateMuSig2Nonce and set after the successful nonce generation.

Per BIP327, a secnonce consisting of only zero bytes is invalid for Sign and will cause it to fail.

I think you're confusing different abstraction levels here. secnonce in the BIP is a byte array of length 64. secp256k1_musig_secnonce is a C type. Blobs of that type need more than 64 bytes in memory. And yes, (purported) blobs of that type having an all-zero memory representation are indeed rejected by secp256k1_musig_partial_sign (by crashing). But this is not because they represent a secnonce of 64 zero bytes; it is simply because don't even represent a valid secp256k1_musig_secnonce. Crashing is just a courtesy of libsecp256k1 -- the library could also just invoke UB.

But none of this should matter for code in Bitcoin Core as a caller of libsecp256k1. The only thing you should make sure as a caller is to pass a secp256k1_musig_secnonce to secp256k1_musig_partial_sign only if secp256k1_musig_nonce_gen succeeded on it.

It shouldn't be possible to hit any error with this as the nonce is not stored if nonce generation fails.

That makes sense. I wasn't able to produce this error manually. It only surfaced during fuzz testing. Would it be helpful to provide the harness that I used?

Instead of a bool that needs to track state, the unique ptr can be created in CreateMuSig2Nonce and set after the successful nonce generation.

Okay, I've updated the code to take this approach.

But none of this should matter for code in Bitcoin Core as a caller of libsecp256k1. The only thing you should make sure as a caller is to pass a secp256k1_musig_secnonce to secp256k1_musig_partial_sign only if secp256k1_musig_nonce_gen succeeded on it.

I see now. Thanks for taking the time to explain it to me. I've removed this motivation and instead will focus solely on assuring that secnonce generation has already succeeded.

Rebased off master. Changed approach to that suggested by achow101. Updated motivation to remove misleading reference to BIP327.