Followups to #716 (add musig2 API) #794
Conversation
apoelstra
force-pushed
the
2025-05_musig2-followups
branch
3 times, most recently
from
eecd9ab to
aa0f303
Compare
|
bacfb03 has a bunch of formatting changes in it. |
|
@tcharding oh, yeah, lemme separate those out into an initial commit.. This crate is supposed to always be formatted but #716 was not. |
src/musig.rs
Outdated
| /// # # [cfg(feature = "std")] | ||
| /// # # [cfg(feature = "rand")] { |
There was a problem hiding this comment.
In 48d5c8f this syntax is odd, is it meant to eb #![cfg(...)]?
There was a problem hiding this comment.
Yeah, something is messed up with my rustfmt setup..
There was a problem hiding this comment.
Oh I bet that is why CI is failing, I didn't look though.
There was a problem hiding this comment.
No, there shouldn't be a !. But there shouldn't be a space eithr.
There was a problem hiding this comment.
CI was failing because some of these doctests don't work when the secp256k1_fuzz "break all the crypto" flag is enabled. The spacing was fine.
Also, this weird spacing originated in #716 and I preserved it because I was copy/pasting lines to add more gates. Fixed now.
|
Reviewed aa0f303 |
|
Will revisit this tomorrow. |
|
Ohhh I see. We have And also investigate why CI is complaining about failures with the fuzz cfg set, when there are existing failures with that cfg and CI wasn't complaining.. |
apoelstra
force-pushed
the
2025-05_musig2-followups
branch
2 times, most recently
from
1de91d1 to
e03b54f
Compare
Claude identified a couple of the FFI bindings to MuSig that were incorrect.
Copy the list of whitelisted lints from BlockstreamResearch/simfony. The "uninlined format args" one was the only one that was currently triggering here.
The compiler doesn't like shared references to static mut variables, because it is worried that we'll share a reference while a mutation happens which is UB. We are only sharing references after using Once to do a one-time initialization, so this is fine, but in recent versions of the compiler there is an encapsulated form of this pattern called `OnceLock` and a variant called `LazyLock`. Neither of these are available on our MSRV so just whitelist the lint.
In rand 0.9 `thread_rng` has been renamed to `rng`. The recent Musig2 PR predates the transition to rand 0.9.
Pretty-much all of the doctests included in rust-bitcoin#716 corresponded to an old version of the API. I'm unsure why it is that my local CI accepted this. The Github CI did not.
This is just a convenience wrapper around SessionSecretRand::from_rng(&mut rng()). The name `new` disguises the fact that this is an impure function and actually calls a RNG every time it is called. It is also unused, in the docs or in the tests. Finally, clippy complains about it because it feels that a `new` method should always be accompanied by a `Default` impl...which is fair, but the impurity is veen more surprising for `default` than for `new`. Since `rand` renamed `thread_rng` to just `rng` the keystroke-saving value of this method is less. So just delete it.
This feels more natural as `secp.musig_sort_pubkeys` rather than `musig::pubkey_sort.
I decided not to rename the `serialize` functions to `to_byte_array`. Maybe we should do that, but we use the name `serialize` *all over* this library so we should do it in a separate PR that changes everything. Along the way we'll have to decide what to call the methods that produce e.g. a SerializedSignature; this is conceptually a byte array but it's actually not one.
…gate The existing code panics somewhere in ffi.rs trying to do pointer alignment stuff, and is only not UB by accident. Guard this with an explicit panic.
I asked Claude to create an initial set of unit tests, which it did. I then manually cleaned up a lot of its repeated logic (though not all of it, as you can tell) and added a whole bunch more failure cases. For example it did not bother trying to repeat or swap keys/nonces. When it generated the tests, the empty-pubkey-list bug (fixed in the previous commit) was still present. It failed to find it, I think because it was reading the code looking for panics to trigger, and there wasn't one. In future I will try giving it only the API, and try telling it to be more adversarial. We'll see.
apoelstra
force-pushed
the
2025-05_musig2-followups
branch
from
e03b54f to
8a43317
Compare
|
WASM and cross were both failing before this PR. |
|
cc @Kixunil can you take a look at this? |
| [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. |
There was a problem hiding this comment.
Can't this lints be whitelisted in the modules as needed rather than globally here.
Also if whitelisting locally (either in modules or specific code) I would advocate to using expect than allow.
There was a problem hiding this comment.
These lints are bad and I would rather not use clippy than deal with them. Global whitelisting is the right approach.
Using expect would require I know whether or not they trigger.
| 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. |
There was a problem hiding this comment.
Can't this lints be whitelisted in the modules as needed rather than globally here.
Also if whitelisting locally (either in modules or specific code) I would advocate to using expect than allow.
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.