Reduce usage of FromHex
#1565
Reduce usage of FromHex
#1565
Conversation
6d806d5 to
49b7053
Compare
Indeed, I knew about this. I think it could be simply a function, there's nothing a function can't do that such macro can. I planned to do it.
Why? That seems like a significant regression to me. It's slowed down by allocation and also requires an allocator which previously wasn't required. |
bitcoin/examples/taproot-psbt.rs
Outdated
| @@ -239,7 +238,7 @@ fn generate_bip86_key_spend_tx( | |||
| lock_time: absolute::LockTime::ZERO, | |||
| input: vec![TxIn { | |||
| previous_output: OutPoint { | |||
| txid: Txid::from_hex(input_utxo.txid)?, | |||
| txid: input_utxo.txid.parse::<Txid>()?, | |||
There was a problem hiding this comment.
Nit: Turbofish is not needed here.
bitcoin/src/address.rs
Outdated
| @@ -1143,11 +1143,11 @@ mod tests { | |||
|
|
|||
| #[test] | |||
| fn test_p2sh_address_58() { | |||
| let addr = Address::new(Bitcoin, Payload::ScriptHash(hex_into!("162c5ea71c0b23f5b9022ef047c4a86470a5b070"))); | |||
| let addr = Address::new(Bitcoin, Payload::ScriptHash("162c5ea71c0b23f5b9022ef047c4a86470a5b070".parse::<ScriptHash>().unwrap())); | |||
There was a problem hiding this comment.
A few more unneeded turbofishes.
| impl core::str::FromStr for ScriptBuf { | ||
| type Err = hex::Error; | ||
| #[inline] | ||
| fn from_str(s: &str) -> Result<Self, hex::Error> { | ||
| hex::FromHex::from_hex(s) | ||
| ScriptBuf::from_hex(s) |
There was a problem hiding this comment.
Oh, we had FromStr despite it not being inverse of Display. That's not great, I think we should remove it. (We could also parse human-friendly script but do we want to?)
9f340d3 to
1298554
Compare
|
Changes to the last 2 patches are significant. Patch |
FromHex
|
Changing the PR title to better express what it does. |
163117e to
c28c211
Compare
|
We probably want to rebase after #1576 because it will conflict with the first commit (which moves the hashbrown dep). |
c28c211 to
23d9605
Compare
|
Rebased on master, no changes required. |
|
Heh, needs rebase again |
23d9605 to
9257fde
Compare
|
I removed the leading 2 patches (from #1563) after noticing that this PR does not actually use those changes. Force push is rebase and dropping those two, no other changes. |
hashes/src/error.rs
Outdated
| /// Crate error type. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub enum Error { | ||
| /// Tried to create a fixed-length hash from a slice with the wrong size (expected, got). | ||
| InvalidLength(usize, usize), | ||
| /// A hexadecimal parsing error. | ||
| Hex(hex::Error), | ||
| } |
There was a problem hiding this comment.
This is not great, hex::Error already contains InvalidLength variant. I see this is because you wanted to use it in FromStr::Error. IMO we should have struct ParseError(hex::Error); for that.
This error type is provided only because we have redundant from_slice() methods for thing that can be achieved with from_inner(x.try_into()). I think we should consider removing both the type and the helper function.
There was a problem hiding this comment.
This is not great,
hex::Erroralready containsInvalidLengthvariant. I see this is because you wanted to use it inFromStr::Error. IMO we should havestruct ParseError(hex::Error);for that.
I just changed the Now I'm confused, the FromStr impls to use hex::Error instead of hashes::Error - any reason not to do that?FromStr impls do use hex::Error currently on master already.
This error type is provided only because we have redundant
from_slice()methods for thing that can be achieved withfrom_inner(x.try_into()). I think we should consider removing both the type and the helper function.
I played around with this, had to return core::alloc::TryFromSliceError - all was well until the derives on pstb::Error bit me. I'm going to leave this for now.
There was a problem hiding this comment.
I'm confused, the
FromStrimpls do usehex::Errorcurrently on master already.
Yes, I meant we should encapsulate it to not leak internal type.
But thinking about it, long term we will have hex crate with Error being just invalid char and odd length - no invalid length. And since the crate will be public we could then have:
enum ParseError {
Hex(hex::Error),
InvalidLength(InvalidLengthError),
}
#[non_exhaustive]
struct InvalidLengthError {
required: usize,
provided: usize
}until the derives on
pstb::Errorbit me
Oh, yeah, {Partial}Eq is annoying, but shouldn't PSBT code already supply the correct length, so just unwrapping is OK? Didn't read the code but when I did some deserialization stuff like this then unwrapping made more sense because the length was statically known.
hashes/src/error.rs
Outdated
| use self::Error::*; | ||
|
|
||
| match self { | ||
| InvalidLength(ref ell, ref ell2) => write!(f, "bad slice length {} (expected {})", ell2, ell), |
9257fde to
1c38486
Compare
|
I rebased and removed the new error variant, returning |
1c38486 to
bf976ed
Compare
Currently we have two things that are the same but use different identifiers, use `$reverse` instead of `$reversed`.
Currently we have incorrect feature gating on unit tests and imports leading to unused import warnings.
We have old Rust 1.29 error handling code still in `hashes`. Implement `std::error::Error` for the `hex::Error` and `error::Error` types in line with "modern" Rust 1.41.1 error handling.
It is easier to maintain code if macros use the fully qualified path to types and functions instead of relying on code that uses the macro to import said type or function.
There is no need to qualify `Script` and `ScriptBuf` with the `script::` prefix, remove it.
In preparation for modifying some unit test data structures, manually format the code so it is uniform. Move elements added to a vec with `vec!` onto a new line so they all line up and one can better see what fields go where. Refactor only, no logic changes.
Remove `FromHex` from hash and script types - Remove the `FromHex` implementation from hash types and `ScriptBuf` - Remove the `FromStr` implementation from `ScriptBuf` because it does not roundtrip with `Display`. - Implement a method `from_hex` on `ScriptBuf`. - Implement `FromStr` on hash types using a fixed size array. This leaves `FromHex` implementations only on `Vec` and fixed size arrays.
`Sighash` should be displayed forwards according to BIP143. Currently we are displaying it backwards (as we do for double SHA256). This is working because parse using `Vec::from_hex`. We have the means to parse hex strings directly for hashes, we no longer need `hex_from_slice`. BIP143 test vectors display double SHA256 forwards but we display backwards, this is acceptable because there is no fixed display in the ecosystem for double SHA256 hashes. In order to overcome this we parse test vector hex strings with into `Vec` when needed.
bf976ed to
32ca6cc
Compare
|
Rebase only, no other changes. |
|
ooo, two acks, can this one go in first please? |
This can be checked against the 0.29.x branch, and against the commit prior to rust-bitcoin#1659 (40c2467^) and you will see that it is consistent EXCEPT: * In rust-bitcoin 0.29.x we did not have multiple sighash types, only `Sighash`; we now have `LegacySighash`, `SegwitV0Sighash`, and `TapSighash`. * In rust-bitcoin#1565 we deliberately changed the display direction of the sighashes, to match BIP 143. Fixes rust-bitcoin#2495.
… in the library b816c0b hash_types: add unit tests for display of all hash types in the library (Andrew Poelstra) Pull request description: This can be checked against the 0.29.x branch, and against the commit prior to #1659 (40c2467^) and you will see that it is consistent EXCEPT: * In rust-bitcoin 0.29.x we did not have multiple sighash types, only `Sighash`; we now have `LegacySighash`, `SegwitV0Sighash`, and `TapSighash`. * In #1565 we deliberately changed the display direction of the sighashes, to match BIP 143. Fixes #2495. ACKs for top commit: tcharding: That's a win. ACK b816c0b Tree-SHA512: 5b44f40165699910ea9ba945657cd1f960cf00a0b4dfa44c513feb3b74cda33ed80d3551042c15b85d6e57c30a54242db202eefd9ec8c8b6e1498b5578e52800
This work started out, as the branch name suggests, as an effort to use the
hex_litcrate. But once I got to this stage it seems that thehex!macro we have provides different, useful, functionality than thehex_lit::hex!macro (it allows usage with non-consts). So I'm unsure if we want to remove it now.FromHex, please see git log for full descriptionhex_from_sliceand fixes a bug in how we displaySighash