Skip to content

Followups to #716 (add musig2 API) #794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Jun 11, 2025
Merged

Followups to #716 (add musig2 API) #794

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f5f90af
fmt: stop blacklisting secp256k1-sys; just fmt whole crate
apoelstra May 16, 2025
07922fd
musig: fix a couple FFI bindings
apoelstra May 15, 2025
7c56bcc
clippy: whitelist a bunch of lints
apoelstra May 15, 2025
9615ec8
context: whitelist new compiler warning
apoelstra May 15, 2025
4dd861f
stop using deprecated thread_rng
apoelstra May 15, 2025
3b0232a
musig: fix all the doctests
apoelstra May 15, 2025
00c8c75
musig: remove outdated doc references to ZeroSession error
apoelstra May 15, 2025
6d938d3
musig: add missing Panics sections to docs
apoelstra May 15, 2025
ec66003
musig: remove SessionSecretRand::new constructor
apoelstra May 15, 2025
c492c75
key: move pubkey_sort to method on Secp256k1; rename
apoelstra May 15, 2025
dc04575
musig: a couple small improvements of byte array APIs
apoelstra May 15, 2025
ebdaec7
musig: clarify doc comment about aggregate nonce proxy
apoelstra May 15, 2025
40a8b65
musig: explicitly panic when given an empty slice of pubkeys to aggre…
apoelstra May 15, 2025
8a43317
musig: add a bunch of unit tests
apoelstra May 15, 2025
d611a4f
musig: weaken/simplify warnings about nonce reuse
apoelstra May 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ pub mod global {
type Target = Secp256k1<All>;

#[allow(unused_mut)] // Unused when `rand` + `std` is not enabled.
#[allow(static_mut_refs)] // The "proper" way to do this is with OnceLock (MSRV 1.70) or LazyLock (MSRV 1.80)
// See https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might still be UB if we're creating &mut rather than using raw pointers. I think we really need SyncUnsafeCell.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we focus on #806 instead which will entirely remove this?

If you are really concerned about soundness then we can try to fold that into this upcoming release -- but all this PR does is whitelist the clippy lint. If we think the existing code is actually unsound then we should have a separate issue to track that. (But my feeling is that it's probably fine in practice so it's ok to continue with our intended removal of this code and not worry about backporting.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we look at how hard it is to replace with SyncUnsafeCell and if it's easy enough we replace it. Also IIRC the only sound way to use static muts is to use raw pointers to them only (obtained using raw pointer operator) with core::ptr::* operations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"How hard it is" appears to be "impossible" since the whole SyncUnsafeCell API is unstable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we can easily write our own struct SyncUnsafeCell<T>(UnsafeCell<T>); unsafe impl<T> Sync for UnsafeCell<T> {}

Copy link
Member Author

@apoelstra apoelstra Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I misunderstood your point -- I thought you were proposing we replace an UnsafeCell with a SyncUnsafeCell (and couldn't figure out how that would make unsound code sound).

Actually you're just proposing we replace no cell at all with an UnsafeCell, which needs to be Sync or else the compiler won't allow it in a static.

I'm not totally convinced you're correct, but it seems easy to do and definitely won't make things worse. So we might as well try.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I remembered now that the assignment calls the drop impl on Option which takes &mut self so it's implicitly creating a mutable reference to a static which IIRC is UB, so I'm quite convinced we have to do this. Sure, we could rewrite it to use ptr::write instead but I think the explicitness of UnsafeCell beats it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, good catch.

I wonder whether we should file a bug/feature request to clippy requesting that any assignments to static mut types which are Drop be linted. Since they are probably UB.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to #820. I believe that is the last followup from this PR.

fn deref(&self) -> &Self::Target {
static ONCE: Once = Once::new();
static mut CONTEXT: Option<Secp256k1<All>> = None;
Expand Down