Merge #259: Replace custom hex parsing with hex-conservative crate

775d7cd Migrate hex decoding implemenation with hex-conservative (Velnbur)

Pull request description:

  # Overview
  
  Add hex-conservative as a dependency and refactor hex parsing code to use it instead of the custom implementation. Update FromHex trait to use associated error types and standardize hex decoding across the codebase.
  
  Closes: #258


ACKs for top commit:
  apoelstra:
    ACK 775d7cd; successfully ran local tests


Tree-SHA512: 2c2a92b92f886909890b6c1b01d8313a956fbebd7978a195ca71e73bb3dc7f8e0cbc1db5942951cd2eaddcde26fd6c689cfd0ec784acff7b113956e46151d6ab

Author: apoelstra

Cargo-latest.lock

@@ -64,7 +64,7 @@ dependencies = [
  "bitcoin-io",
  "bitcoin-units",
  "bitcoin_hashes",
- "hex-conservative",
+ "hex-conservative 0.2.1",
  "hex_lit",
  "secp256k1",
  "serde",
@@ -108,7 +108,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "bb18c03d0db0247e147a21a6faafd5a7eb851c743db062de72018b6b7e8e4d16"
 dependencies = [
  "bitcoin-io",
- "hex-conservative",
+ "hex-conservative 0.2.1",
  "serde",
 ]
 
@@ -196,6 +196,7 @@ dependencies = [
  "bincode",
  "bitcoin",
  "getrandom 0.2.16",
+ "hex-conservative 1.0.0",
  "rand",
  "rand_chacha",
  "secp256k1-zkp",
@@ -274,6 +275,12 @@ dependencies = [
  "arrayvec",
 ]
 
+[[package]]
+name = "hex-conservative"
+version = "1.0.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9ee770c000993d17c185713463d5ebfbd1af9afae4c17cc295640104383bfbf0"
+
 [[package]]
 name = "hex_lit"
 version = "0.1.1"

Cargo.toml

@@ -32,6 +32,7 @@ serde_json = { version = "1.0", optional = true }
 actual-serde = { package = "serde", version = "1.0.103", features = [
     "derive",
 ], optional = true }
+hex-conservative = "1.0.0"
 
 
 [target.wasm32-unknown-unknown.dev-dependencies]

src/confidential.rs

@@ -720,6 +720,60 @@ impl<'de> Deserialize<'de> for Nonce {
     }
 }
 
+/// Error decoding hexadecimal string into tweak-like value.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum TweakHexDecodeError {
+    /// Invalid hexadecimal string.
+    InvalidHex(hex_conservative::DecodeFixedLengthBytesError),
+    /// Invalid tweak after decoding hexadecimal string.
+    InvalidTweak(secp256k1_zkp::Error),
+}
+
+impl fmt::Display for TweakHexDecodeError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            TweakHexDecodeError::InvalidHex(err) => {
+                write!(f, "Invalid hex: {}", err)
+            }
+            TweakHexDecodeError::InvalidTweak(err) => {
+                write!(f, "Invalid tweak: {}", err)
+            }
+        }
+    }
+}
+
+#[doc(hidden)]
+impl From<hex_conservative::DecodeFixedLengthBytesError> for TweakHexDecodeError {
+    fn from(err: hex_conservative::DecodeFixedLengthBytesError) -> Self {
+        TweakHexDecodeError::InvalidHex(err)
+    }
+}
+
+#[doc(hidden)]
+impl From<secp256k1_zkp::Error> for TweakHexDecodeError {
+    fn from(err: secp256k1_zkp::Error) -> Self {
+        TweakHexDecodeError::InvalidTweak(err)
+    }
+}
+
+impl From<TweakHexDecodeError> for encode::Error {
+    fn from(value: TweakHexDecodeError) -> Self {
+        match value {
+            TweakHexDecodeError::InvalidHex(err) => encode::Error::HexFixedError(err),
+            TweakHexDecodeError::InvalidTweak(err) => encode::Error::Secp256k1zkp(err),
+        }
+    }
+}
+
+impl std::error::Error for TweakHexDecodeError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        match self {
+            TweakHexDecodeError::InvalidHex(err) => Some(err),
+            TweakHexDecodeError::InvalidTweak(err) => Some(err),
+        }
+    }
+}
+
 /// Blinding factor used for asset commitments.
 #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash)]
 pub struct AssetBlindingFactor(pub(crate) Tweak);
@@ -747,16 +801,13 @@ impl AssetBlindingFactor {
 }
 
 impl hex::FromHex for AssetBlindingFactor {
-    fn from_byte_iter<I>(iter: I) -> Result<Self, hex::Error>
-        where I: Iterator<Item=Result<u8, hex::Error>> +
-            ExactSizeIterator +
-            DoubleEndedIterator
-    {
-        let slice = <[u8; 32]>::from_byte_iter(iter.rev())?;
-        // Incorrect Return Error
-        // See: https://github.com/rust-bitcoin/bitcoin_hashes/issues/124
-        let inner = Tweak::from_inner(slice)
-            .map_err(|_e| hex::Error::InvalidChar(0))?;
+    type Err = TweakHexDecodeError;
+
+    fn from_hex(s: &str) -> Result<Self, Self::Err> {
+        let mut slice: [u8; 32] = hex_conservative::decode_to_array(s)?;
+        slice.reverse();
+
+        let inner = Tweak::from_inner(slice)?;
         Ok(AssetBlindingFactor(inner))
     }
 }
@@ -951,16 +1002,13 @@ impl Neg for ValueBlindingFactor {
 }
 
 impl hex::FromHex for ValueBlindingFactor {
-    fn from_byte_iter<I>(iter: I) -> Result<Self, hex::Error>
-        where I: Iterator<Item=Result<u8, hex::Error>> +
-            ExactSizeIterator +
-            DoubleEndedIterator
-    {
-        let slice = <[u8; 32]>::from_byte_iter(iter.rev())?;
-        // Incorrect Return Error
-        // See: https://github.com/rust-bitcoin/bitcoin_hashes/issues/124
-        let inner = Tweak::from_inner(slice)
-            .map_err(|_e| hex::Error::InvalidChar(0))?;
+    type Err = TweakHexDecodeError;
+
+    fn from_hex(s: &str) -> Result<Self, Self::Err> {
+        let mut slice: [u8; 32] = hex_conservative::decode_to_array(s)?;
+        slice.reverse();
+
+        let inner = Tweak::from_inner(slice)?;
         Ok(ValueBlindingFactor(inner))
     }
 }

src/encode.rs

@@ -19,6 +19,7 @@ use std::io::Cursor;
 use std::{any, error, fmt, io, mem};
 
 use bitcoin::ScriptBuf;
+use hex_conservative::{DecodeFixedLengthBytesError, DecodeVariableLengthBytesError};
 use secp256k1_zkp::{self, RangeProof, SurjectionProof, Tweak};
 
 use crate::hashes::{sha256, Hash};
@@ -54,8 +55,10 @@ pub enum Error {
     Secp256k1zkp(secp256k1_zkp::Error),
     /// Pset related Errors
     PsetError(pset::Error),
-    /// Hex parsing errors
-    HexError(crate::hex::Error),
+    /// Hex fixed parsing errors
+    HexFixedError(DecodeFixedLengthBytesError),
+    /// Hex variable parsing errors
+    HexVariableError(DecodeVariableLengthBytesError),
     /// Got a time-based locktime when expecting a height-based one, or vice-versa
     BadLockTime(crate::LockTime),
     /// `VarInt` was encoded in a non-minimal way.
@@ -83,7 +86,8 @@ impl fmt::Display for Error {
             Error::Secp256k1(ref e) => write!(f, "{}", e),
             Error::Secp256k1zkp(ref e) => write!(f, "{}", e),
             Error::PsetError(ref e) => write!(f, "Pset Error: {}", e),
-            Error::HexError(ref e) => write!(f, "Hex error {}", e),
+            Error::HexFixedError(ref e) => write!(f, "Hex fixed error: {}", e),
+            Error::HexVariableError(ref e) => write!(f, "Hex variable error: {}", e),
             Error::BadLockTime(ref lt) => write!(f, "Invalid locktime {}", lt),
             Error::NonMinimalVarInt => write!(f, "non-minimal varint"),
         }
@@ -134,9 +138,16 @@ impl From<secp256k1_zkp::Error> for Error {
 }
 
 #[doc(hidden)]
-impl From<crate::hex::Error> for Error {
-    fn from(e: crate::hex::Error) -> Self {
-        Error::HexError(e)
+impl From<DecodeFixedLengthBytesError> for Error {
+    fn from(e: DecodeFixedLengthBytesError) -> Self {
+        Error::HexFixedError(e)
+    }
+}
+
+#[doc(hidden)]
+impl From<DecodeVariableLengthBytesError> for Error {
+    fn from(e: DecodeVariableLengthBytesError) -> Self {
+        Error::HexVariableError(e)
     }
 }