Remove context from the API #844

tcharding · 2025-09-09T05:55:58Z

Tested in rust-bitcoin/rust-bitcoin#4960

TODO in this PR:

Remove the FIXMEs, I don't exactly understand the rerandomize_seed and what is ok to put in it?

While trying to work this out I created #847 and #848, apoelstra can you school me on exactly how random the seed needs to be please?

Follow up PRs:

Do we want to remove the SECP256K1, global-context (and global-context-less-secure) features in this PR instead of deprecating stuff?
Go over the docs and fix places that mention the context

apoelstra · 2025-09-09T15:26:20Z

Why put the changes on top of the release tracking PR? Shouldn't we do the changes first?

apoelstra · 2025-09-09T15:27:49Z

Thanks for doing this! Definite concept ACK. After these commits I guess we should look at the FIXMEs in the API tests and go over the PublicKey API to try to fix those.

apoelstra · 2025-09-09T15:29:14Z

...and no, I had no work here that you are stepping on (but now that you've started, I won't, so that means this API-overhaul-and-release project is blocked on you).

The soft goal is to get rid of all the context objects in rust-bitcoin except maybe in the signmessage. If that turns out to be hard or impossible that's okay; we should try to release this week or next with what we've got. Happy to see that you have a tracking PR in rust-bitcoin for this!

tcharding · 2025-09-09T22:43:32Z

so that means this API-overhaul-and-release project is blocked on you

No sweat, I got it.

Why put the changes on top of the release tracking PR? Shouldn't we do the changes first?

Yeah definitely, it was just for dev purposes. I'll push up a PR and rebase the release tracking one. Sorry if that wasn't obvious.

tcharding · 2025-09-09T22:44:14Z

The soft goal is to get rid of all the context objects in rust-bitcoin except maybe in the signmessage.

I'm pumped for this. First thing I've been excited about in rust-bitcoin for a good while.

tcharding · 2025-09-11T03:10:36Z

Phew, that was hard.

tcharding · 2025-09-11T22:30:37Z

I'm still unsure exactly what can be passed in as the random seed.

A secret key, because its essentially random data even if its fixed between calls?
A public key? Its essentially also random (in the same sense as the private key) but publicly known?
A tweak?

apoelstra · 2025-09-12T21:35:44Z

Basically anything. Though there is not much value in adding publicly known stuff (you might as well pass in all 0s if you're gonna do this; the effect is basically just to ratchet the rng), so we shouldn't encourage people to do that.

Docs are not linted, this `Secp256k1` is not needed.

As is customary. Internal change only.

We are removing the context from APIs, bit by bit.

tcharding · 2025-09-15T03:22:31Z

I've passed in a local variable of all-zeroes anywhere there isn't secret data. There are other solutions, can we merge this as is and iterate because this PR is painful since its so big.

In musig::new_nonce_pair I got creative, please check for sanity.

apoelstra · 2025-09-15T13:39:31Z

Yeah, I'll leave comments here and just open a followup

apoelstra · 2025-09-15T13:40:11Z

src/key/mod.rs

-        other: &Scalar,
-    ) -> Result<PublicKey, Error> {
+    pub fn mul_tweak(mut self, other: &Scalar) -> Result<PublicKey, Error> {
+        // We have no seed here but we want rerandomiziation to happen for `rand` users.


In f8bf19d:

There is no secret data. We don't want to rerandomize here. It slows things down for no purpose.

Same with every other instance, even KeyPair, because while we manipulate secret data there, it's not in a way that rerandomization would have any effect on.

This commit is still present in the latest push (but I can address it in a followup).

My bad, I missed this comment yesterday. Fixed in #855

apoelstra · 2025-09-15T13:48:14Z

src/lib.rs

+#[cfg(all(feature = "rand", feature = "std"))]
 fn test_random_keypair() -> (key::SecretKey, key::PublicKey) { generate_keypair(&mut rand::rng()) }

-/// Constructor for unit testing.


In 0f557a4:

Ok, I can't accept this commit. It deletes a test-only function and then disables like half the unit tests because of it except when rand is enabled. I'm not sure why this is done. The commit message is also misleading, saying that it reduces coverage "because it tightens feature guards".

But actually it reduces coverage because it deletes functionality that was just added in 362495b in #806 among other things, specifically to test the global context API that this PR purports to be expanding usage of.

The bulk of this patch seems to be disabling unit tests and I didn't read it in detail.

Stopping here because I suspect that I will need to re-review everything anyway after this patch is removed.

apoelstra · 2025-09-15T22:08:15Z

Re musig_nonce_gen you have

   ┊191┊+    let seed = if let Some(key) = sec_key {
   ┊192┊+        key.to_secret_bytes()
   ┊193┊+    } else if let Some(rand) = extra_rand {
   ┊194┊+        rand
   ┊195┊+    } else {
   ┊196┊+        session_secrand.to_byte_array()
   ┊197┊+    };

I think we should start with session_secrand and xor in each of the other two things if we've got it.

As we did for `PublicKey` get rid of the context.

Done for both `schnorr` and `ecdsa`.

We are removing the context everywhere we can, do so for the `musig` module. N.B. the seed initialization logic in `new_nonce_pair`.

We are removing the context everywhere we can, do so for the `ellswift` module.

Take the `sort_pubkeys` function off of the context and make it stand alone. Re-export it at the crate root because the `key` module is private.

Somehow this got missed, trivially remove it. Internal change only, no logic change.

apoelstra · 2025-09-16T15:19:37Z

Will ACK this. Followups:

Go through and stop passing dummy data to rerandomization function when we don't need to rerandomize
Go through all the newly one-liner functions and consider deprecating/removing them
Delete existing Secp256k1 object and replace with a dummy object that reimplements the existing API (to the extent possible) by passing through to global functions

Then I think we can cut a release. Then we can fix the API tests by doing deprecate-and-rename on a bunch of methods, and do another major release.

apoelstra

ACK 3f5c666; successfully ran local tests

99171f8 Don't re-randomize unnecessarily (Tobin C. Harding) 0e0eb60 musig: Use secret bytes from keypair to rerandomize (Tobin C. Harding) Pull request description: Follow up of #844 Fix rerandomization stuff. - Use keypair secret data to rerandomize in musig when doing partial siging. - Remove all the zero seed arrays and use `None` to disable rerandomization when no secret data used. ACKs for top commit: apoelstra: ACK 99171f8; successfully ran local tests Tree-SHA512: 4bc68d9e819b2a0fb94ade67aa29c25c2454f94dcdd623f4fb67f9aa74caac75146bccb8683ed1ed636591d8e1dfb181c76cc16d33b0b57885af9b2b94c8b931

3f5c666182b13f9a450f793138833d32b8d1c2e2 Remove unneeded import (Tobin C. Harding) 38a7d9e87f2e27f4b6a59e213f25602864c2ffaf Remove context sort_pubkeys (Tobin C. Harding) 4fe0101b6dfdc850b2e2cccbc48029963ae35e98 ellswift: Remove context (Tobin C. Harding) 70e928cb6a28598276b1c2d51b36fedfcab9d0ca musig: Remove context (Tobin C. Harding) 58c7ba21ff70507d991e6b03cf9cf87aa7a5298f Remove context from signing APIs (Tobin C. Harding) 403f5414d9d2948f36c102a5df69d1da2f49d98b Remove context from verification functions (Tobin C. Harding) 074a4ac8eabb7a753ea07bb04941e3619fc126b8 Remove context from Keypair functions (Tobin C. Harding) f8bf19d9135d3c9cb42e6f6f751d6ead62ebc1e0 Remove context from tweaking functions (Tobin C. Harding) 792103bd8f775761b2d93b33a3112448c7b4a78b Remove context from PublicKey::from_secret_key (Tobin C. Harding) 95e2e2587ec72ab53e6c6e02ae458185158c5aa1 Use super::* in tests (Tobin C. Harding) b2cdc9849f7629cb1d9fc9085cd4c9aa51225da8 docs: Remove one instance of context construction (Tobin C. Harding) Pull request description: Tested in rust-bitcoin/rust-bitcoin#4960 ### TODO in this PR: Remove the FIXMEs, I don't exactly understand the `rerandomize_seed` and what is ok to put in it? While trying to work this out I created #847 and #848, apoelstra can you school me on _exactly_ how random the seed needs to be please? ### Follow up PRs: - Do we want to remove the `SECP256K1`, `global-context` (and `global-context-less-secure`) features in this PR instead of deprecating stuff? - Go over the docs and fix places that mention the context ACKs for top commit: apoelstra: ACK 3f5c666182b13f9a450f793138833d32b8d1c2e2; successfully ran local tests Tree-SHA512: ea0667fa950182ddb4cf90f0de96824bb244000cdc9f4a8fd243b29e6c5c8180b4f269d91d918fa035bfe58b48d1b7367cc2f7234e21ea4c6cc7f5a448e92594

tcharding mentioned this pull request Sep 9, 2025

Test secp context removal work rust-bitcoin/rust-bitcoin#4960

Closed

tcharding force-pushed the push-mvsrwnoxylmx branch 2 times, most recently from d8bb241 to 7e638fc Compare September 10, 2025 04:40

tcharding changed the title ~~Start removing the context from a bunch of places~~ Remove context from the API Sep 10, 2025

tcharding force-pushed the push-mvsrwnoxylmx branch 5 times, most recently from 7eee6ee to 1cef39c Compare September 10, 2025 05:32

tcharding mentioned this pull request Sep 11, 2025

Review usage of ffi::secp256k1_context_no_precomp #846

Open

tcharding force-pushed the push-mvsrwnoxylmx branch 5 times, most recently from e25ee7d to 02b9c22 Compare September 11, 2025 03:04

tcharding mentioned this pull request Sep 11, 2025

Finish off removing context #850

Open

tcharding added 4 commits September 15, 2025 13:20

docs: Remove one instance of context construction

b2cdc98

Docs are not linted, this `Secp256k1` is not needed.

Use super::* in tests

95e2e25

As is customary. Internal change only.

Remove context from PublicKey::from_secret_key

792103b

We are removing the context from APIs, bit by bit.

Remove context from tweaking functions

f8bf19d

tcharding force-pushed the push-mvsrwnoxylmx branch from 02b9c22 to 4562ae2 Compare September 15, 2025 03:21

tcharding marked this pull request as ready for review September 15, 2025 03:21

tcharding force-pushed the push-mvsrwnoxylmx branch from 4562ae2 to f48d5a9 Compare September 15, 2025 03:24

apoelstra reviewed Sep 15, 2025

View reviewed changes

tcharding added 7 commits September 16, 2025 12:51

Remove context from Keypair functions

074a4ac

As we did for `PublicKey` get rid of the context.

Remove context from verification functions

403f541

Done for both `schnorr` and `ecdsa`.

Remove context from signing APIs

58c7ba2

musig: Remove context

70e928c

We are removing the context everywhere we can, do so for the `musig` module. N.B. the seed initialization logic in `new_nonce_pair`.

ellswift: Remove context

4fe0101

We are removing the context everywhere we can, do so for the `ellswift` module.

Remove context sort_pubkeys

38a7d9e

Take the `sort_pubkeys` function off of the context and make it stand alone. Re-export it at the crate root because the `key` module is private.

Remove unneeded import

3f5c666

Somehow this got missed, trivially remove it. Internal change only, no logic change.

tcharding force-pushed the push-mvsrwnoxylmx branch from f48d5a9 to 3f5c666 Compare September 16, 2025 02:51

apoelstra approved these changes Sep 16, 2025

View reviewed changes

apoelstra merged commit 0fb6a2a into rust-bitcoin:master Sep 16, 2025
28 checks passed

This was referenced Sep 17, 2025

Fix rerandomization seed usage #855

Merged

Follow up #844 #856

Open

tcharding mentioned this pull request Sep 24, 2025

Follow up 844 #858

Draft

Remove context from the API #844

Remove context from the API #844

Uh oh!

Conversation

tcharding commented Sep 9, 2025 • edited by apoelstra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO in this PR:

Follow up PRs:

Uh oh!

apoelstra commented Sep 9, 2025

Uh oh!

apoelstra commented Sep 9, 2025

Uh oh!

apoelstra commented Sep 9, 2025

Uh oh!

tcharding commented Sep 9, 2025

Uh oh!

tcharding commented Sep 9, 2025

Uh oh!

tcharding commented Sep 11, 2025

Uh oh!

tcharding commented Sep 11, 2025

Uh oh!

apoelstra commented Sep 12, 2025

Uh oh!

tcharding commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Sep 15, 2025

Uh oh!

apoelstra Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

tcharding Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Sep 16, 2025

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tcharding commented Sep 9, 2025 •

edited by apoelstra

Loading

tcharding commented Sep 15, 2025 •

edited

Loading

apoelstra commented Sep 15, 2025 •

edited

Loading