rust-bitcoin / rust-secp256k1

Rust language bindings for Bitcoin secp256k1 library.
Creative Commons Zero v1.0 Universal
350 stars 270 forks source link

Truncate secret hash #722

Closed tcharding closed 3 months ago

tcharding commented 3 months ago

The core::hash::Hasher and bitcoin_hashes hash types implement formatting traits slightly differently

Anywho, this PR fixes the secret display truncation.

tcharding commented 3 months ago

This PR fixes the question asked here: https://github.com/rust-bitcoin/rust-bitcoin/blob/b05cf43d22d36bddd90f9e512ce15158736c4812/bitcoin/src/crypto/key.rs#L1529

tcharding commented 3 months ago

This is non-urgent so I'll just wait till #699 goes in to debug CI failure.

Kixunil commented 3 months ago

hash is a 32 byte integer (I think) and for integral types precision is ignorded

It is but I think many people perceive it mainly as string. Though if we were to truncate it it's unclear if we should truncate the beginning or the end.

apoelstra commented 3 months ago

A hash is not an integer. In no sense is a hash an integer. You can interpret a hash as an integer but then you need to make a bunch of decisions about endianness.

A hash is an opaque bytestring. If you truncate it then you should take the prefix not the suffix. This is what git does, this is what Bitcoin does in multiple places (e.g. bip32 fingerprints, the sha256 "checksum" on base58 addresses, etc), and this is what's done to construct "truncated hashes".

Kixunil commented 3 months ago

@apoelstra this is also what GPG doesn't do - the short key identifiers are suffixes. ;) Yes, I hate it.

Anyway, I do think we should allow it.

apoelstra commented 3 months ago

Hah, TIL another crazy thing about GPG. (I guess I did know this actually, and have been annoyed about it because how hard it is visually to compare the short ID to the long fingerprint.)

tcharding commented 3 months ago

A hash is not an integer.

TIL, thanks. Raised issue: https://github.com/rust-bitcoin/hex-conservative/issues/101

tcharding commented 3 months ago

Buuuuut, we'd have to remove the range dependency on bitcoin_hashes.

apoelstra commented 3 months ago

Let's just remove the range dependency. It doesn't work properly anyway. See #673. I thought we'd removed it already because of that issue.

apoelstra commented 3 months ago

Not sure what's up with CI. I think maybe nightly has broken something xargo-related?

tcharding commented 3 months ago

Seems so, this works

RUSTUP_TOOLCHAIN=nightly-2024-08-04 xargo run --release --target=x86_64-unknown-linux-gnu
tcharding commented 3 months ago

That is the nightly version currently used in #699

Kixunil commented 3 months ago

WHAT?! What's the problem with ranged dependency?

Actually we made other changes that make #673 non-issue, but there's still #723. I'd like to use ranged dependencies for all private dependencies to minimize dependency duplication.