Clean up hashes and related types ahead of bitcoin_hashes 1.0#278
Open
apoelstra wants to merge 13 commits into
Open
Clean up hashes and related types ahead of bitcoin_hashes 1.0#278apoelstra wants to merge 13 commits into
bitcoin_hashes 1.0#278apoelstra wants to merge 13 commits into
Conversation
Stop using `actual-serde` as a dependency; use `dep:` syntax. Also stop using
`#[macro_use]` on the import; instead import the macros like a normal symbol.
Also, rather than `use serde::{Serialize, Deserialize}`, fully-qualify the
trait names every time they're used. This helps avoid compiler confusion
between the serde methods and any other methods named serialize() that happen
to be in scope.
I'm not sure why we were using generic sha256::Hashes here, but it's the wrong type, and it will complicate our transition to hashes 1.0 if we keep using it.
The leaf_hash method is better-implemented as a passthrough to TapLeafHash::from_script (which saves a manual hash computation which is difficult to port to hashes 1.0). To do this we need to fix the type of leaf_version.
In hashes 1.0 we no longer have Hash::from_slice; we have Hash::from_byte_array and if the user has a slice, they should use one of the TryFrom impls in the stdlib to get an array from it. This replaces a hashes error with a stdlib error which is likely easier to deal with.
Now that we are using arrays everywhere for conversion to/from hashes, we no longer have runtime length errors.
While I was looking at this error enum I realized a bunch of these are out of date.
For several types we are going to rewrite them to no longer wrap sha256 midstates (or to be a newtype rather than an unwrapped sha256 midstate). To ensure this does not break serde, test round-tripping here.
…ropy In hashes 1.0, the sha256::Midstate type is no longer a raw 32-byte blob. It also contains a counter. This is necessary to safely move between a hash engine and a midstate, e.g. when using midstates as a "preinitialized hash state". Most users of the existing type can just stick some .0s onto the new type, swear at us a bit, and move on. Probably we can do that too. But in our case the correct thing to do is to introduce a fresh type for asset entropy, so we might as well take this opportunity to do it.
In general, I hate macros, but I'm going to use identical boilerplate for like half a dozen types through the next several commits, so I think that it's worth it. Putting this in its own commit so you can easily review it with `git show --color-moved-ws=ignore-all-space` -- though I changed some doc comments and also added LowerHex and UpperHex impls which I forgot to do in the previous commit.
In `hashes 1.0` the Midstate type is pretty annoying to work with. Better to just directly use an array backing. This also makes the string-encoding reversal logic more explicit. This also replaces the serde impls for AssetId, but because we added a regression test (a few commits ago) we can be sure that this didn't break anything. This also removes the AssetId::from_inner and to_inner methods, which are unused in the library and seem inappropriate for the public API. This also removes the Encodable/Decodable impls for sha256::Midstate, which won't work properly with hashes 1.0 and don't seem to be used anyway.
There are actually 3 separate Merkle roots at play here, which are all currently typed as sha256::Midstate. Had we designed this a few years later we likely would have domain-separated all of them to prevent them from ever being collided with each other, though given the nature of the data they hash, there should be no risk of that anyway. This change was surprisingly easy. This prevents these roots from being confused with each other or with other midstates. It changes the debug output of one of the merkle roots which for some reason we were unit testing.
We already re-export and use rust-bitcoin's PublicKey type. The two hashes of keys that are used in Script are identical in Bitcoin and Elements, and have convenience methods that come with the bitcoin PublicKey type. To use these convenience methods, drop the PubkeyHash and WPubkeyHash types and just re-export the rust-bitcoin ones. We keep the scripthash types separate because Elements Script is meaningfully different from Bitcoin Script. **Importantly**, because upstream our WPubkeyHash returns an error if you try to give it an uncompressed public key, we panic in this case. The 'correct' way to handle this would be to have distinct compressed/uncompressed public key types, but those are not yet available from rust-bitcoin. Our current behavior is to compute a "wpkh" output which is unspendable, which seems like a very serious loss-of-funds problem, so we panic instead.
This commit was originally just supposed to add constructors for WScriptHash and ScriptHash to allow them to be created from scripts without calling the generic `hash()` method (which will go away in 1.0). But in doing so, I noticed that the Address::p2shwpkh constructor is wrong. It computes the ScriptHash of a public key rather than a WPubkeyHash. Now, these two hashes are both just hash160s so there's no difference and no user-visible bug here. But jeez.
apoelstra
force-pushed
the
2026-06/hash-types
branch
from
b779dd3 to
f6d247e
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
I am going to PR soon to update to bitcoin-hashes 1.0. There are several big changes versus the old version of bitcoin-hashes that we were using:
Txid,Blockhash, etc) no longer have general hashing methods likehashthat let you hash up arbitrary data; the intention of these types is that they only be constructable from byte slices or by hashing the right kind of dataTryFrom<&[u8; N]> for &[u8]impl which is just a pointer cast, so you can get the old behavior by using thisTryFromsha256::Midstatetype now carries a length with it, so it behaves something like a deconstructed hash engine rather than being a "hash" which is computed from the sha256 compression function; you can no longer effectively wrap a midstate the way you would wrap a sha256 hash, so instead we directly wrap[u8; 32]s for the types that did thisThis PR makes a bunch of prepatory changes to make this transition smaller. In particular, it
sha256::Midstateuses by introducing newtypes for each midstate (asset entropy, dynafed elided params, etc)Address::p2wpkhandAddress::p2shwpkhpanic if you give them uncompressed keys rather than producing unspendable addresses; later we will clean this up further by type-separating compressed keys, but for now ISTM that a panic is better than silently creating black-hole addresses