Upgrade to secp256k1 v0.23.0 #1066

tcharding · 2022-06-28T02:51:16Z

We recently released a new version of rust-secp256k1, upgrade to use it.

tcharding · 2022-06-28T03:36:04Z

CI is failing because of a bug in secp, fixed in: rust-bitcoin/rust-secp256k1#466

0c15c01 Use fuzzing not feature = "fuzzing" (Tobin C. Harding) Pull request description: Currently the following command fails `RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS='--cfg=fuzzing' cargo test --all --all-features` This is because `fuzzing` is not a feature, we should be using `fuzzing` directly not `feature = "fuzzing"`. ~I have no idea how this got past CI~, found while trying to [upgrade secp in bitcoin](rust-bitcoin/rust-bitcoin#1066). This got past CI because of the feature gate combination `#[cfg(all(test, feature = "unstable"))]`, we never run tests on CI with both DO_FEATURE_MATRIX and DO_BENCH. ``` if [ "$DO_FEATURE_MATRIX" = true ]; then ... if [ "$DO_BENCH" = true ]; then # proxy for us having a nightly compiler cargo test --all --all-features RUSTFLAGS='--cfg=fuzzing' RUSTDOCFLAGS='--cfg=fuzzing' cargo test --all --all-features fi fi ``` ACKs for top commit: apoelstra: ACK 0c15c01 Tree-SHA512: 08ada4eb20c3b7b128a225ed66cc621af097367f8ca19128b868d1b5de897f46d19f3a96a06ebd5dfaa288bc4477046f5d1214f0cdc33237b0ace079c539fc9e

sanket1729 · 2022-06-28T17:35:56Z

expect that needs removing.

Unfortunately, the expect will have to stay. Whenever we hash a point, with a low probability it will fail to be a scalar (1/2^128). The previous code would have panicked at "Tap Tweak failed".

tcharding · 2022-06-28T23:11:57Z

Oh, thanks man, I see it now

let parity = output_key.tweak_add_assign(secp, &tweak_value).expect("Tap tweak failed");

Why did we elect to keep this as a panic and not return the error? Is it because it is statistically unlikely?

apoelstra · 2022-06-28T23:14:24Z

It's more than statistically unlikely. There is literally no aspect of your reality more likely to be real than that this .expect will pass every time.

tcharding · 2022-06-28T23:42:32Z

So definitely not worth adding an error for :) I'll remove the Option and add back in an expect with a comment.

tcharding · 2022-06-29T02:01:15Z

Added this comment

// This is statistically extremely unlikely to panic (1/2^128).

I prefer your comment @apoelstra but it didn't read as well once it was in the code :)

apoelstra

ACK 171c5d3

Kixunil

The code looks correct, just nits/ideas.

Kixunil · 2022-07-08T15:08:24Z

src/util/taproot.rs

+
+    /// Converts a `TapTweakHash` into a `Scalar` ready for use with key tweaking API.
+    pub fn to_scalar(&self) -> Scalar {
+        // This is statistically extremely unlikely to panic (1/2^128).


Note that wile you're correct it's unlikely, the probability is not 1/2^128 but 432420386565659656852420866394968145599 / 2^256; still same order of magnitude. :)

I copied that from @sanket1729 and do not know where either of these numbers come from, can someone please justify their number so I can understand and I'll fix the comment.

For now I have just removed the probability.

The number is the difference between our group's size and 2^256, I believe.

Isn't the group size equal to p?

p = 2^256 - 2^32 - 2^9 - 2^8 - 2^7 - 2^6 - 2^4 - 1 = 4294968237

ref: https://en.bitcoin.it/wiki/Secp256k1

It's equal to n from the same page. Yes, it's confusing but there are actually two groups/fields in secp256k1 - one for coordinates, the other for points/scalars. Unsurprisingly, this is not the first time I've seen people being confused. :)

(Writing my own toy secp256k1 lib was a great learning experience, 100% recommend!)

one for coordinates, the other for points/scalars

I'm totally lost by this statement, added writing a toy secp lib to my todo list, good idea!

Kixunil · 2022-07-08T15:13:33Z

src/util/taproot.rs

@@ -90,6 +90,12 @@ impl TapTweakHash {
        }
        TapTweakHash::from_engine(eng)
    }
+
+    /// Converts a `TapTweakHash` into a `Scalar` ready for use with key tweaking API.
+    pub fn to_scalar(&self) -> Scalar {


Perhaps motivating to add something similar to ThirtyTwoByteHash but for scalar. (trait ToScalar { fn to_scalar(&self) -> Scalar; } impl<T: ThirtyTwoByteHash> ToScalar for T { /* ... */ }) and accept T: ToScalar in tweaking fns. But maybe it's too many generics?

I'm going to leave this for now, mainly because I have a weeks worth of notifications to work through.

Kixunil · 2022-07-08T15:21:28Z

src/util/bip32.rs

-        let mut sk = secp256k1::SecretKey::from_slice(&hmac_result[..32])?;
-        sk.add_assign(&self.private_key[..])?;
+        let sk = secp256k1::SecretKey::from_slice(&hmac_result[..32])?;
+        let tweaked = sk.add_tweak(&self.private_key.into())?;


Since both are cryptographically unlikely maybe panic would be better?

Agreed, though don't feel very strongly..

It's somewhat "unfortunate" that Rust makes it easier to propagate impossible-to-hit error conditions than it does to assert on them.

Used expect as suggested.

Kixunil · 2022-07-08T15:33:53Z

Jump straight to semvar patch version 3

Generally, forcing newest version if not required is not good.

tcharding · 2022-07-10T02:36:53Z

Generally, forcing newest version if not required is not good.

Did you mean in general because in this instance it is required since we use new functionality that does not exist in lower versions. Or am I missing something?

tcharding · 2022-07-10T02:48:12Z

Changes in force push:

Rebase to pick up clippy fix
Use expect instead of returning error
Remove the probability from docs

Kixunil · 2022-07-10T20:22:51Z

we use new functionality that does not exist in lower versions

Do we? I didn't notice and I'm really exhausted rn, so I can't re-check myself.

tcharding · 2022-07-10T21:47:37Z

Uses the new functional style methods

let tweaked = self.public_key.add_exp_tweak(secp, &sk.into())?;

apoelstra

ACK 4f753b2

Kixunil · 2022-07-11T15:21:32Z

That one is available since 0.23.0

tcharding · 2022-07-11T23:12:04Z

~~Oh bother, I notice in the linked docs I've used the wrong deprecation string a bunch of times, it should have read since = "0.23.3".~~

Ok, I seen now you are correct and I was confused, I got mixed up between the three point releases we did and the 23.0 release, my bad. Will fix.

We recently released a new version of `rust-secp256k1`, upgrade to use it.

tcharding · 2022-07-11T23:38:56Z

Changes in force push:

Upgrade to 0.23.0 instead of 0.23.3
Fix commit log, PR title and PR description

apoelstra

ACK 36f29d4

Kixunil

ACK 36f29d4

36f29d4 Upgrade to secp256k1 v0.23.0 (Tobin C. Harding) Pull request description: We recently released a new version of `rust-secp256k1`, upgrade to use it. ACKs for top commit: apoelstra: ACK 36f29d4 Kixunil: ACK 36f29d4 Tree-SHA512: 46a909dec8bc59daa78acdb76824d93f4f1da0e9736cf6ca443d3bbadfa43867e720293bb7c4919cb0658e75ec59daeffea080611f0e7eed4df439ddac0305de

tcharding force-pushed the 06-28-upgrade-secp branch from 515c4a2 to d77fd11 Compare June 28, 2022 03:00

tcharding mentioned this pull request Jun 28, 2022

Hot Fix: Use fuzzing not feature = "fuzzing" rust-bitcoin/rust-secp256k1#466

Merged

tcharding mentioned this pull request Jun 28, 2022

Derive Hash on a bunch of types #990

Merged

tcharding added this to the 0.29.0 milestone Jun 28, 2022

tcharding force-pushed the 06-28-upgrade-secp branch from d77fd11 to 7e02e64 Compare June 28, 2022 23:11

tcharding force-pushed the 06-28-upgrade-secp branch 2 times, most recently from 72e0eea to 2194f0d Compare June 29, 2022 02:00

tcharding force-pushed the 06-28-upgrade-secp branch from 2194f0d to 171c5d3 Compare June 29, 2022 23:48

tcharding changed the title ~~Upgrade to secp256k1 v0.23.2~~ Upgrade to secp256k1 v0.23.3 Jun 29, 2022

tcharding marked this pull request as ready for review June 30, 2022 00:02

apoelstra previously approved these changes Jun 30, 2022

View reviewed changes

Kixunil reviewed Jul 8, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Jul 9, 2022

derive Hash for Invoice lightningdevkit/rust-lightning#1575

Merged

tcharding dismissed apoelstra’s stale review via 4f753b2 July 10, 2022 02:47

tcharding force-pushed the 06-28-upgrade-secp branch from 171c5d3 to 4f753b2 Compare July 10, 2022 02:47

apoelstra previously approved these changes Jul 11, 2022

View reviewed changes

Upgrade to secp256k1 v0.23.0

36f29d4

We recently released a new version of `rust-secp256k1`, upgrade to use it.

tcharding dismissed apoelstra’s stale review via 36f29d4 July 11, 2022 23:23

tcharding force-pushed the 06-28-upgrade-secp branch from 4f753b2 to 36f29d4 Compare July 11, 2022 23:23

tcharding changed the title ~~Upgrade to secp256k1 v0.23.3~~ Upgrade to secp256k1 v0.23.0 Jul 11, 2022

apoelstra approved these changes Jul 12, 2022

View reviewed changes

Kixunil approved these changes Jul 12, 2022

View reviewed changes

Kixunil added the API break This PR requires a version bump for the next release label Jul 12, 2022

apoelstra merged commit 4965495 into rust-bitcoin:master Jul 12, 2022

tcharding deleted the 06-28-upgrade-secp branch July 12, 2022 23:38

Kixunil mentioned this pull request Jul 13, 2022

Use infallible conversions from hashes to secp256k1::Message #824

Closed

notmandatory mentioned this pull request Aug 26, 2022

Problem at use bdk 0.19 with wasm bitcoindevkit/bdk#646

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to secp256k1 v0.23.0 #1066

Upgrade to secp256k1 v0.23.0 #1066

tcharding commented Jun 28, 2022 •

edited

tcharding commented Jun 28, 2022

sanket1729 commented Jun 28, 2022

tcharding commented Jun 28, 2022

apoelstra commented Jun 28, 2022

tcharding commented Jun 28, 2022

tcharding commented Jun 29, 2022 •

edited

apoelstra left a comment

Kixunil left a comment

Kixunil Jul 8, 2022

tcharding Jul 10, 2022

tcharding Jul 10, 2022

apoelstra Jul 11, 2022

tcharding Jul 11, 2022

Kixunil Jul 12, 2022

tcharding Jul 12, 2022

Kixunil Jul 8, 2022

tcharding Jul 10, 2022

Kixunil Jul 8, 2022

apoelstra Jul 8, 2022

tcharding Jul 10, 2022

Kixunil commented Jul 8, 2022

tcharding commented Jul 10, 2022

tcharding commented Jul 10, 2022

Kixunil commented Jul 10, 2022

tcharding commented Jul 10, 2022

apoelstra left a comment

Kixunil commented Jul 11, 2022

tcharding commented Jul 11, 2022 •

edited

tcharding commented Jul 11, 2022

apoelstra left a comment

Kixunil left a comment

Upgrade to secp256k1 v0.23.0 #1066

Upgrade to secp256k1 v0.23.0 #1066

Conversation

tcharding commented Jun 28, 2022 • edited

tcharding commented Jun 28, 2022

sanket1729 commented Jun 28, 2022

tcharding commented Jun 28, 2022

apoelstra commented Jun 28, 2022

tcharding commented Jun 28, 2022

tcharding commented Jun 29, 2022 • edited

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil commented Jul 8, 2022

tcharding commented Jul 10, 2022

tcharding commented Jul 10, 2022

Kixunil commented Jul 10, 2022

tcharding commented Jul 10, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil commented Jul 11, 2022

tcharding commented Jul 11, 2022 • edited

tcharding commented Jul 11, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

tcharding commented Jun 28, 2022 •

edited

tcharding commented Jun 29, 2022 •

edited

tcharding commented Jul 11, 2022 •

edited