Merge #794: Followups to #716 (add musig2 API)

d611a4f musig: weaken/simplify warnings about nonce reuse (Andrew Poelstra)
8a43317 musig: add a bunch of unit tests (Andrew Poelstra)
40a8b65 musig: explicitly panic when given an empty slice of pubkeys to aggregate (Andrew Poelstra)
ebdaec7 musig: clarify doc comment about aggregate nonce proxy (Andrew Poelstra)
dc04575 musig: a couple small improvements of byte array APIs (Andrew Poelstra)
c492c75 key: move pubkey_sort to method on Secp256k1; rename (Andrew Poelstra)
ec66003 musig: remove SessionSecretRand::new constructor (Andrew Poelstra)
6d938d3 musig: add missing Panics sections to docs (Andrew Poelstra)
00c8c75 musig: remove outdated doc references to ZeroSession error (Andrew Poelstra)
3b0232a musig: fix all the doctests (Andrew Poelstra)
4dd861f stop using deprecated thread_rng (Andrew Poelstra)
9615ec8 context: whitelist new compiler warning (Andrew Poelstra)
7c56bcc clippy: whitelist a bunch of lints (Andrew Poelstra)
07922fd musig: fix a couple FFI bindings (Andrew Poelstra)
f5f90af fmt: stop blacklisting secp256k1-sys; just fmt whole crate (Andrew Poelstra)

Pull request description:

  This PR needs to be merged before the next release because the existing code has one instance of UB when passing an empty array to the aggregate nonce function. (Ok, there's a rust panic in our alignment code so maybe no bad pointers make it across the C boundary and we're ok. But it's near-UB.)

  This PR is the first one I created using jujutsu. One thing I notice is that the tool encourages you to produce way more commits than you would with git. Most of these are small. Let me know if you want me to squash any.

ACKs for top commit:
  jlest01:
    ACK d611a4f
  jonasnick:
    ACK d611a4f modulo my comments on the PR (secret dependent branches) and that I only looked at the musig/libsecp-relevant bits.

Tree-SHA512: a504912639bcb6296bd6fdf7a0533464ce9e9064d1c2bf06bf142b7749b4aaf75ed4bae10f9912b92191c64a453bcf56bbd001e3c99ab383e02de4676c7c6a69

Author: apoelstra

Cargo.toml

@@ -73,3 +73,13 @@ required-features = ["rand", "std"]
 [workspace]
 members = ["secp256k1-sys"]
 exclude = ["no_std_test"]
+
+[lints.clippy]
+# Exclude lints we don't think are valuable.
+large_enum_variant = "allow" # docs say "measure before paying attention to this"; why is it on by default??
+similar_names = "allow" # Too many (subjectively) false positives.
+uninlined_format_args = "allow" # This is a subjective style choice.
+indexing_slicing = "allow" # Too many false positives ... would be cool though
+match_bool = "allow" # Adds extra indentation and LOC.
+match_same_arms = "allow" # Collapses things that are conceptually unrelated to each other.
+must_use_candidate = "allow" # Useful for audit but many false positives.

examples/musig.rs

@@ -4,11 +4,11 @@ use secp256k1::musig::{
     new_nonce_pair, AggregatedNonce, KeyAggCache, PartialSignature, PublicNonce, Session,
     SessionSecretRand,
 };
-use secp256k1::{pubkey_sort, Keypair, Message, PublicKey, Scalar, Secp256k1, SecretKey};
+use secp256k1::{Keypair, Message, PublicKey, Scalar, Secp256k1, SecretKey};
 
 fn main() {
     let secp = Secp256k1::new();
-    let mut rng = rand::thread_rng();
+    let mut rng = rand::rng();
 
     let (seckey1, pubkey1) = secp.generate_keypair(&mut rng);
 
@@ -19,7 +19,7 @@ fn main() {
     let mut pubkeys_ref: Vec<&PublicKey> = pubkeys.iter().collect();
     let pubkeys_ref = pubkeys_ref.as_mut_slice();
 
-    pubkey_sort(&secp, pubkeys_ref);
+    secp.musig_sort_pubkeys(pubkeys_ref);
 
     let mut musig_key_agg_cache = KeyAggCache::new(&secp, pubkeys_ref);
 

rustfmt.toml

@@ -1,8 +1,3 @@
-# Eventually this shoud be: ignore = []
-ignore = [
-       "secp256k1-sys"
-]
-
 hard_tabs = false
 tab_spaces = 4
 newline_style = "Auto"

secp256k1-sys/Cargo.toml

@@ -35,3 +35,13 @@ alloc = []
 
 [lints.rust]
 unexpected_cfgs = { level = "deny", check-cfg = ['cfg(bench)', 'cfg(secp256k1_fuzz)', 'cfg(rust_secp_no_symbol_renaming)'] }
+
+[lints.clippy]
+# Exclude lints we don't think are valuable.
+large_enum_variant = "allow" # docs say "measure before paying attention to this"; why is it on by default??
+similar_names = "allow" # Too many (subjectively) false positives.
+uninlined_format_args = "allow" # This is a subjective style choice.
+indexing_slicing = "allow" # Too many false positives ... would be cool though
+match_bool = "allow" # Adds extra indentation and LOC.
+match_same_arms = "allow" # Collapses things that are conceptually unrelated to each other.
+must_use_candidate = "allow" # Useful for audit but many false positives.

secp256k1-sys/build.rs

@@ -16,21 +16,22 @@ use std::env;
 fn main() {
     // Actual build
     let mut base_config = cc::Build::new();
-    base_config.include("depend/secp256k1/")
-               .include("depend/secp256k1/include")
-               .include("depend/secp256k1/src")
-               .flag_if_supported("-Wno-unused-function") // some ecmult stuff is defined but not used upstream
-               .flag_if_supported("-Wno-unused-parameter") // patching out printf causes this warning
-               .define("SECP256K1_API", Some(""))
-               .define("ENABLE_MODULE_ECDH", Some("1"))
-               .define("ENABLE_MODULE_SCHNORRSIG", Some("1"))
-               .define("ENABLE_MODULE_EXTRAKEYS", Some("1"))
-               .define("ENABLE_MODULE_ELLSWIFT", Some("1"))
-               .define("ENABLE_MODULE_MUSIG", Some("1"))
-               // upstream sometimes introduces calls to printf, which we cannot compile
-               // with WASM due to its lack of libc. printf is never necessary and we can
-               // just #define it away.
-               .define("printf(...)", Some(""));
+    base_config
+        .include("depend/secp256k1/")
+        .include("depend/secp256k1/include")
+        .include("depend/secp256k1/src")
+        .flag_if_supported("-Wno-unused-function") // some ecmult stuff is defined but not used upstream
+        .flag_if_supported("-Wno-unused-parameter") // patching out printf causes this warning
+        .define("SECP256K1_API", Some(""))
+        .define("ENABLE_MODULE_ECDH", Some("1"))
+        .define("ENABLE_MODULE_SCHNORRSIG", Some("1"))
+        .define("ENABLE_MODULE_EXTRAKEYS", Some("1"))
+        .define("ENABLE_MODULE_ELLSWIFT", Some("1"))
+        .define("ENABLE_MODULE_MUSIG", Some("1"))
+        // upstream sometimes introduces calls to printf, which we cannot compile
+        // with WASM due to its lack of libc. printf is never necessary and we can
+        // just #define it away.
+        .define("printf(...)", Some(""));
 
     if cfg!(feature = "lowmemory") {
         base_config.define("ECMULT_WINDOW_SIZE", Some("4")); // A low-enough value to consume negligible memory
@@ -45,15 +46,15 @@ fn main() {
 
     // WASM headers and size/align defines.
     if env::var("CARGO_CFG_TARGET_ARCH").unwrap() == "wasm32" {
-        base_config.include("wasm/wasm-sysroot")
-                   .file("wasm/wasm.c");
+        base_config.include("wasm/wasm-sysroot").file("wasm/wasm.c");
     }
 
     // secp256k1
-    base_config.file("depend/secp256k1/contrib/lax_der_parsing.c")
-               .file("depend/secp256k1/src/precomputed_ecmult_gen.c")
-               .file("depend/secp256k1/src/precomputed_ecmult.c")
-               .file("depend/secp256k1/src/secp256k1.c");
+    base_config
+        .file("depend/secp256k1/contrib/lax_der_parsing.c")
+        .file("depend/secp256k1/src/precomputed_ecmult_gen.c")
+        .file("depend/secp256k1/src/precomputed_ecmult.c")
+        .file("depend/secp256k1/src/secp256k1.c");
 
     if base_config.try_compile("libsecp256k1.a").is_err() {
         // Some embedded platforms may not have, eg, string.h available, so if the build fails
@@ -63,4 +64,3 @@ fn main() {
         base_config.compile("libsecp256k1.a");
     }
 }
-