Remove bitcoin-hashes feature #837
Open
+83
−168
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
apoelstra
force-pushed
the
push-pnskkuvsunrm
branch
from
1a0d9aa to
bd98c53
Compare
Closed
|
I believe line 30 and line 56 of the docs in |
apoelstra
force-pushed
the
push-pnskkuvsunrm
branch
2 times, most recently
from
2856bee to
426e338
Compare
|
tACK 426e338 Thanks for the clean up! |
|
cc @tcharding can I have an ack from you too? |
This is the only place we use a sha256 hash. We were using the 'hashes' crate which is a whole extra dependency just to bring in a hash function that already exists in libsecp256k1. libsecp256k1 doesn't expose sha256 directly, but they do expose default hash functions for EDCH, ECDSA and Schnorr, which are just sha2s with extra steps. Since we are not worried about having a specific canonical hash, just one that won't change for different keys, do this. (This change does mean that users will get different hashes when they upgrade this library; but they really shouldn't be persisting Debug output and expecting it to be consistent. I don't think this is a meaningful breaking change.)
We have a curious test where we confirm that we can match upstream's code with direct calls to sha256. This is nice, but completely unnecessary as a unit test. Secondly, we have example code where we demonstrate how to use bitcoin_hashes to write a custom ECDH nonce function. This is valuable, but not reason by itself to have an extra dependency, so we stick `ignore` on it to prevent the Rust compiler from attempting to compile it.
We had an impl of the (deprecated) ThirtyTwoByte hash trait, which we no longer use; we delete this. Also remove the 'hashes' feature gate from a unit test that no longer needs it. There is also a `pub extern crate hashes` in src/lib.rs which we should remove here, but it's used in some doctests so we'll remove it in the next commit.
This is a little annoying; the library has a Message type for ECDSA signing which is supposed to be a secure hash digest, but we provide no methods for producing such a digest. So our examples are necessarily a little wonky. Before this commit we would use `bitcoin_hashes` to hash some arbitrary data. This isn't really wise (signing unstructured data or using hashes that aren't domain-separated) but it was shaped similarly to proper usage. Now we just hardcode the "output" of hash functions and convert that. I added some comments to help.
apoelstra
force-pushed
the
push-pnskkuvsunrm
branch
from
426e338 to
f9ce31f
Compare
|
Force-pushed to fix formatting. Unsure why our CI is not enforcing this (my local CI is). |
|
Uh https://github.com/apoelstra/rust-secp256k1/actions/runs/17023922782/workflow may be one reason.. will fix this separately. |
tcharding
reviewed
Aug 17, 2025
| } | ||
| } | ||
|
|
||
| f.debug_tuple(stringify!($thing)).field(&output).finish() |
There was a problem hiding this comment.
Mind blown. All the time I've spent wrestling with formatter funtions ...
tcharding
approved these changes
Aug 17, 2025
|
BOOM! Nice bit of team work, killa. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This feature was used in examples and docs, and to obfuscate the Debug output of secrets. The latter can be done by other means, and the docs/examples can be modified or reduced.
Fixes #836