Skip to content
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

Jump to bottom

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

Merged
merged 2 commits into from
Jan 12, 2021
Merged

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

merged 2 commits into from
Jan 12, 2021

Conversation

thomaseizinger
Copy link
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
Cargo.toml Show resolved Hide resolved
@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
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.

Cargo.toml Show resolved Hide resolved
@sgeisler sgeisler requested a review from stevenroose January 4, 2021 18:32
@apoelstra
Copy link
Member

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
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 8f071d1

apoelstra
apoelstra requested changes Jan 5, 2021
View reviewed changes
Copy link
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
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
Member

@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
Contributor Author

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
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
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
Member

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
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 8f071d1

@thomaseizinger
Copy link
Contributor Author

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
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
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

@sgeisler sgeisler sgeisler approved these changes

@apoelstra apoelstra apoelstra approved these changes

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