Update to hex-conservative 1.1; drop hex module#272
Open
apoelstra wants to merge 10 commits intoElementsProject:masterfrom
Open
Update to hex-conservative 1.1; drop hex module#272apoelstra wants to merge 10 commits intoElementsProject:masterfrom
hex module#272apoelstra wants to merge 10 commits intoElementsProject:masterfrom
Conversation
These are all very mechanical changes, and only affect tests. No API or behavior changes. Do this replacement across all of src/. There are some examples in examples/ and fuzz/ and elementsd-tests/ but we can't fix these yet because they don't have access to the hex-conservative dep. In a later commit we will delete the hex module and re-export hex-conservative as hex, and then change the other crates.
There are three types in this crate which implement FromHex: AssetBlindingFactor, ValueBlindingFactor, and Script. The first two are straightforward; these types are always encoded as hex and so we should use Display/FromStr rather than hex-specific traits. Script is a bit special because of its length prefix and we will address it in the next commit.
Scripts are sometimes hex-encoded with a length prefix (in particular, when they are part of a hex-encoding of a transaction, or as a test vector for a signature hash) and sometimes without a length prefix (in particular, in the output of most RPC commands). It is therefore unclear and bug-prone to have a from_hex method which doesn't specify whether a length prefix is expected. In rust-bitcoin we have solved this by splitting from_hex into two methods: from_hex_no_prefix and from_hex_prefixed. Add the former, but not the latter (since it is currently quite annoying to implement, and apparently it's not used anywhere). We will revisit the prefixed version later when we upgrade our consensus encoding traits. Also from Script::from_str. In addition to the length-prefix ambiguity, this is further ambiguous because there are a couple different "opcode dissambly" formats floating around. Better to make the user specify what format they want.
This eliminates crate::hex except in a couple of places: * examples/fuzz/doctests/elementsd-test, which need a re-export of hex-conservative * serde_utils, which will be updated in the next commit
…o From<Vec<u8>> The 'hex_bytes' module is somewhat confused: it demands AsRef<[u8]> to serialize, and does so by converting to bytes then hex-serializing the bytes. But to deserialize, it calls FromHex directly. This implicitly requires that the underlying type have a FromHex impl which is compatible with "interpret the type as raw bytes". In this crate, this module is used exclusively by the Vec<u8> and [u8] types, so this is true and things work. To make the requirements clearer, change the deserialization bound from FromHex to From<Vec<u8>>. This is more correct, and also will work with hex-conservative, which has no equivalent to the FromHex trait.
Despite the generic FromHex/ToHex traits being thrown around here, these are exclusively used for byte vectors and so we can just replace the generic calls with hex::decode_to_vec.
The next commit will clean this up a bit. This one just replaces the hex module with the hex-conservative crate and does mechanical fixes to the examples and doctests.
apoelstra
force-pushed
the
2026-05_hex-conservative
branch
from
71f2e9e to
44a6974
Compare
Member
Author
|
ACK 44a6974; successfully ran local tests |
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.
We have a
hexmodule which is loosely based on thehashes::hexmodule frombitcoin_hashes0.11 or so. rust-bitcoin has updated to thehex-conservativecrate, "hex with a conservative MSRV policy", which is maintained by the rust-bitcoin community.We recently have released version 1.1, which is finally feature-complete, after years of haggling over API questions.
This has a couple philosophical changes from the old
hexmodule:FromHextrait; the assumption is that if something is naturally hex-encoded then it should useFromStrto parse from hex, and there is little value in a trait describing "an object which can be parsed from hex" beyond this. For things likeScriptwhich have a hex-encoding but aren't "naturally" hex-encoded, they should have inherent methodsdecode_to_vecanddecode_to_arrayToHextrait, which previously was oriented around producing hex-encodedStrings, has been replaced withDisplayHex, a trait which produces a (typically) no-alloc wrapper around an object that can be hex-formatted. It is a trait rather than standalone function(s) so it can be cleanly implemented for both[u8]and[u8]-like things, but we don't expect users to implement this; they should useDisplayor inherent methods insteadIn addition, there are a pile of mechanical changes;
to_hexbecomesas_hexin most cases,Vec::from_hexbecomesdecode_to_vecorhex!, etc. So this PR has a large diff but it's mostly mechanical, and the changes serve both to clean up the code and to eliminate a bunch of allocations.A later PR will replace our encoding traits with the new
bitcoin-consensus-encodingtrait, which will further eliminate allocations when deserializing objects from hex.