Skip to content

Bump to secp256k1 0.20 and don't enable recovery of secp256k1 in the dependency declaration#545

Merged
sgeisler merged 2 commits intorust-bitcoin:masterfrom
thomaseizinger:update-secp
Jan 12, 2021
Merged

Bump to secp256k1 0.20 and don't enable recovery of secp256k1 in the dependency declaration#545
sgeisler merged 2 commits intorust-bitcoin:masterfrom
thomaseizinger:update-secp

Conversation

@thomaseizinger
Copy link
Copy Markdown
Contributor

@thomaseizinger thomaseizinger commented Jan 4, 2021 •
edited
Loading

rust-secp256k1 version 0.20 was recently released: rust-bitcoin/rust-secp256k1#257

This PR updates rust-bitcoin to this latest version.

@thomaseizinger
Bump to secp256k1 0.20
68840b6 
This version removes the fuzztarget and endomorphism features.
Kixunil
Kixunil reviewed Jan 4, 2021
View reviewed changes
@sgeisler sgeisler added the API break This PR requires a version bump for the next release label Jan 4, 2021
sgeisler
sgeisler previously approved these changes Jan 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

Looks good, but I'd like to hear @stevenroose's opinion.

@sgeisler sgeisler requested a review from stevenroose January 4, 2021 18:32
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Jan 4, 2021

I think we meant to turn off the default features in our secp dep.

sgeisler
sgeisler previously approved these changes Jan 5, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

ACK 8f071d116262b8596df4ee7890e9fe5c9405e6e8

apoelstra
apoelstra requested changes Jan 5, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

concept nack removing the feature - can you instead change this to disable the secp default features so that secp/recovery actually does something?

@sgeisler
Copy link
Copy Markdown
Contributor

sgeisler commented Jan 5, 2021

I'm a bit curious @apoelstra, what's the thought behind feature-gating key recovery? There aren't any more deps afaik. Is it so unstable? I'm pretty sure that LN invoices use this and it seems to be fine.

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Jan 5, 2021

@sgeisler it pulls in the key recovery module from libsecp. This is morally an extra dep, and on code with a questionable patent story and no non-legacy usecases.

If we had been better about feature-gating this historically maybe LN would not have used it :(

@sgeisler sgeisler dismissed their stale review January 5, 2021 17:44

Legal bullshit :(

@thomaseizinger thomaseizinger force-pushed the update-secp branch 2 times, most recently from 4dab9d1 to b5f1fda Compare January 6, 2021 01:23
@thomaseizinger thomaseizinger changed the title Bump to secp256k1 0.20 and remove secp-recovery feature Bump to secp256k1 0.20 and don't enable recovery of secp256k1 by default Jan 6, 2021
@thomaseizinger
Don't enable recovery of secp256k1 in the dependency declaration
e52e48e 
Enabling this feature in the dependency declaration defeats the point
of exposing a feature in rust-bitcoin that enables this because
cargo currently does not provide a way to disable a once activated feature.
@thomaseizinger thomaseizinger changed the title Bump to secp256k1 0.20 and don't enable recovery of secp256k1 by default Bump to secp256k1 0.20 and don't enable recovery of secp256k1 in the dependency declaration Jan 6, 2021
@thomaseizinger
Copy link
Copy Markdown
Contributor Author

thomaseizinger commented Jan 6, 2021

concept nack removing the feature - can you instead change this to disable the secp default features so that secp/recovery actually does something?

Is e52e48e what you had in mind?

@Kixunil
Copy link
Copy Markdown
Collaborator

Kixunil commented Jan 6, 2021

Side note: removing a feature from defaults is also semver breaking (although this PR is breaking anyway).

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Jan 7, 2021 •
edited
Loading

Yep, this looks great!

Can you also add a commit which sets RUSTFLAGS=--cfg=rust_secp_fuzz in the fuzz script?

@apoelstra apoelstra added this to the 0.26.0 milestone Jan 11, 2021
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Jan 11, 2021

Actually we don't need this. See rust-bitcoin/rust-secp256k1#269

apoelstra
apoelstra approved these changes Jan 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ack 8f071d116262b8596df4ee7890e9fe5c9405e6e8

@thomaseizinger
Copy link
Copy Markdown
Contributor Author

thomaseizinger commented Jan 11, 2021

ack 8f071d1

I am afraid that commit is no longer part of this PR, e52e48e is the latest commit.

apoelstra
apoelstra approved these changes Jan 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

I really wish github would flag this somehow. It's bad enough that to get the "review changes" button you have to click to a page which hides all the commit IDs.

ack e52e48e

sgeisler
sgeisler approved these changes Jan 12, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

ACK e52e48e

@sgeisler sgeisler removed the request for review from stevenroose January 12, 2021 10:42
@sgeisler sgeisler merged commit 026f2dd into rust-bitcoin:master Jan 12, 2021
@dr-orlovsky dr-orlovsky mentioned this pull request Jan 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kixunil Kixunil Kixunil left review comments

@apoelstra apoelstra apoelstra approved these changes

+1 more reviewer

@sgeisler sgeisler sgeisler approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

API break This PR requires a version bump for the next release

Projects

None yet

Milestone

0.26.0

Development

Successfully merging this pull request may close these issues.

4 participants

@thomaseizinger @apoelstra @sgeisler @Kixunil