Skip to content

Update hashes to 0.16.0 #832

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rustaceanrob
Copy link

The 0.15.0 was a massive release for hashes in terms of API clean up and also adding new HKDF support. ElligatorSwift in combination with HKDF are the primitives required for encrypted message passing on bitcoin, so exporting these in tandum would save an additional hashes dependency.

I skip to 0.16.0 because hashes allows for hex-conservative as an optional dependency, and no listed changes elsewhere.

Reducing the scope of #783 and will take a stab at rand later.

@apoelstra
Copy link
Member

Thanks! Yeah, let's do this. I am tempted to cut a release of rust-secp256k1 anyway since @stevenroose also asked for this.

Was hoping to get #806 in first but it's blocked on lack of review, so let's just do it.

@apoelstra
Copy link
Member

cargo test --no-default-features --features 'alloc hashes'

fails claiming that Sha256 does not implement Display.

I'm not sure why the Display impl on Sha256 should depend on anything. I spent a couple minutes trying to trace through the macro soup that produces these impls but gave up.

The `0.15.0` was a massive release for `hashes` in terms of API clean
up and also adding new HKDF support. ElligatorSwift in combination with
HKDF are the primitives required for encrypted message passing on
bitcoin, so exporting these in tandum would save an additional
`hashes` dependency.

I skip to `0.16.0` because `hashes` allows for `hex-conservative` as an
optional dependency, and no listed changes elsewhere.
@rustaceanrob
Copy link
Author

Well, I came up with a solution lol. Using the Debug string works. This macro is implementing Debug, so this seems acceptable.

@apoelstra
Copy link
Member

Hah, thanks! I'd prefer to keep the PR here as-is as a canary, because I suspect that actually we broke hashes for nostd users, and we should fix that and cut a point release.

@rustaceanrob
Copy link
Author

Exploring hashes, it looks like the hash_trait_impls macro (internal_macros.rs) calls the impl_hex_string_traits macro (macros.rs) conditional on the hex feature. The 0.16.0 version makes hex optional, which is perhaps explaining the failure. Displaying a secret could can be gated on a hex feature or come with std, unless you have a proposed patch for hashes.

@tcharding
Copy link
Member

tcharding commented Aug 15, 2025

I did #833 while reviewing this.

f.debug_tuple(stringify!($thing)).field(&format_args!("#{:.16}", hash)).finish()
f.debug_tuple(stringify!($thing)).field(&format_args!("{:?}", hash)).finish()
Copy link
Member

@tcharding tcharding Aug 15, 2025

Choose a reason for hiding this comment

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

That 16 width was so its a fingerprint not the whole hash. But I cannot find a test for it. Can you add this and make it pass?

#[cfg(test)]
mod test {
    use super::*;

    #[test]
    #[cfg(feature = "hashes")]
    fn debug_secret_hashes_feature_enabled() {
        let key = "0000000000000000000000000000000000000000000000000000000000000001".parse::<SecretKey>().unwrap();
        
        // Normal debug hides value (`Display` is not implemented for `SecretKey`).
        let want = "SecretKey(#2518682f7819fb2d)";
        let got = format!("{:?}", key);
        assert_eq!(got, want);
    }

    #[test]
    #[cfg(not(feature = "hashes"))]
    fn debug_secret_no_hashes() {
        let key = "0000000000000000000000000000000000000000000000000000000000000001".parse::<SecretKey>().unwrap();
        
        // With "hashes" feature we just output a message.
        let want = "<secret key; enable `hashes` feature of `secp256k1` to display fingerprint>";
        let got = format!("{:?}", key);
        assert_eq!(got, want);
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the doc test was supposed to test it but it just includes a comment about it.

@rustaceanrob
Copy link
Author

Rebasing on #833 works. I will wait for that to go in and then add the tests.

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 this pull request may close these issues.

3 participants