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.

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

The current implementation of bitcoin-hashes alias is a significant footgun #562

Closed
Kixunil opened this issue Dec 8, 2022 · 2 comments · Fixed by #563
Closed

The current implementation of bitcoin-hashes alias is a significant footgun #562

Kixunil opened this issue Dec 8, 2022 · 2 comments · Fixed by #563

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 8, 2022

rust-bitcoin/rust-bitcoin#1450 (comment)

The idiomatic (and expected!) way to activate features is by the crate name. Which is bitcoin_hashes. The secp256k1 crate defines an alias, which is OK but then it itself MUST NOT use that alias. Otherwise the old name won't work and there's no easy way to tell for the users.

Alternatively there could be:

#[cfg(all(feature = "bitcoin_hashes", not(feature = "bitcoin-hashes")))]
compile_error!("Activate feature `bitcoin-hashes` instead of `bitcoin_hashes`");

But I don't see a good reason for forcing people to break the standard way of defining features.

@apoelstra
Copy link
Member

I think we should just audit the code for usage of the alias. We should fix this before the release that we're about to push out.

I wouldn't mind removing it ... but if we never gate stuff on it, it should be harmless.

@tcharding
Copy link
Member

tcharding commented Dec 8, 2022

I made this mess, I'll fix it. You fixed it for me already - thanks. Hilarious, there is a link literally right above this that says you fixed it. My tunnel vision never ceases to amaze me.

apoelstra added a commit that referenced this issue Dec 9, 2022
1d6a46e change bitcoin-hashes feature gates to bitcoin_hashes (Andrew Poelstra)

Pull request description:

  This leaves `bitcoin-hashes` in place everywhere in the documentation, but changes it to `bitcoin_hashes` on the actual feature gates, to ensure that both flags work.

  It's unfortunately a bit hard to test this because the library will be self-consistent pretty-much no matter what we do ... the issue is if the user enables the wrong flag we want to make sure that all the APIs we intend to be visible are actually visible.

  Fixes #562.

ACKs for top commit:
  tcharding:
    ACK 1d6a46e

Tree-SHA512: 1e060aeb2810ef1e23cf5c956023aca586550ba0287bbf7bc1108dc14091e17d7601aac3f057d0313fafd21351cdda1b08b4250f34ecde917be686d0b739e65a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants