Merge rust-bitcoin/rust-bitcoin#796: Re-work sighash type conversion methods

8e2422f92b Add unit test for deserialize non-standard sighash (Tobin Harding)
e05776f176 Improve PsbtSigHashType conversion methods (Tobin Harding)
ac462897b1 Remove hungarian-ish notation (Tobin Harding)
564682627c Remove deprecated conversion method (Tobin Harding)
d1753d7ff1 Rename as_u32 -> to_u32 (Tobin Harding)
2bd71c3748 Remove From<EcdsaSigHashType> for u32 (Tobin Harding)

Pull request description:

  This PR has evolved into a full blown clean up of the conversion methods for all three sighash types based on the discussion below.

  Everything is split up into very small patches to aid review (and bikeshedding, this PR is almost totally just naming things).

  EDIT: I'm not convinced this should be an RC-fix, the changes are too wide spread now. It started as just a single method rename. Leaving label as is for others to consider.

ACKs for top commit:
  sanket1729:
    utACK 8e2422f92b.
  dr-orlovsky:
    ACK 8e2422f92b

Tree-SHA512: 8269bdf6d00a98b472a720b891f25728d5dedf3b2648aa60a461efdbdf37d883a1824a15f1838a792329ec9ac50a9c05618ab2a34cf201fcf63e4ad145955571

src/blockdata/transaction.rs

@@ -444,7 +444,7 @@ impl Transaction {
     }
 
     fn is_invalid_use_of_sighash_single(&self, sighash: u32, input_index: usize) -> bool {
-        let ty = EcdsaSigHashType::from_u32_consensus(sighash);
+        let ty = EcdsaSigHashType::from_consensus(sighash);
         ty == EcdsaSigHashType::Single && input_index >= self.output.len()
     }
 
@@ -806,21 +806,22 @@ impl EcdsaSigHashType {
         }
     }
 
-    /// Reads a 4-byte uint32 as a sighash type.
+    /// Creates a [`EcdsaSigHashType`] from a raw `u32`.
-    #[deprecated(since = "0.26.1", note = "please use `from_u32_consensus` or `from_u32_standard` instead")]
+    #[deprecated(since="0.28.0", note="please use `from_consensus`")]
-    pub fn from_u32(n: u32) -> EcdsaSigHashType {
+    pub fn from_u32_consensus(n: u32) -> EcdsaSigHashType {
-        Self::from_u32_consensus(n)
+        EcdsaSigHashType::from_consensus(n)
     }
 
-    /// Reads a 4-byte uint32 as a sighash type.
+    /// Creates a [`EcdsaSigHashType`] from a raw `u32`.
     ///
     /// **Note**: this replicates consensus behaviour, for current standardness rules correctness
-    /// you probably want [Self::from_u32_standard].
+    /// you probably want [`Self::from_standard`].
+    ///
     /// This might cause unexpected behavior because it does not roundtrip. That is,
-    /// `EcdsaSigHashType::from_u32_consensus(n) as u32 != n` for non-standard values of
+    /// `EcdsaSigHashType::from_consensus(n) as u32 != n` for non-standard values of `n`. While
-    /// `n`. While verifying signatures, the user should retain the `n` and use it compute the
+    /// verifying signatures, the user should retain the `n` and use it compute the signature hash
-    /// signature hash message.
+    /// message.
-    pub fn from_u32_consensus(n: u32) -> EcdsaSigHashType {
+    pub fn from_consensus(n: u32) -> EcdsaSigHashType {
         // In Bitcoin Core, the SignatureHash function will mask the (int32) value with
         // 0x1f to (apparently) deactivate ACP when checking for SINGLE and NONE bits.
         // We however want to be matching also against on ACP-masked ALL, SINGLE, and NONE.
@@ -840,9 +841,18 @@ impl EcdsaSigHashType {
         }
     }
 
-    /// Read a 4-byte uint32 as a standard sighash type, returning an error if the type
+    /// Creates a [`EcdsaSigHashType`] from a raw `u32`.
-    /// is non standard.
+    #[deprecated(since="0.28.0", note="please use `from_standard`")]
     pub fn from_u32_standard(n: u32) -> Result<EcdsaSigHashType, NonStandardSigHashType> {
+        EcdsaSigHashType::from_standard(n)
+    }
+
+    /// Creates a [`EcdsaSigHashType`] from a raw `u32`.
+    ///
+    /// # Errors
+    ///
+    /// If `n` is a non-standard sighash value.
+    pub fn from_standard(n: u32) -> Result<EcdsaSigHashType, NonStandardSigHashType> {
         match n {
             // Standard sighashes, see https://github.com/bitcoin/bitcoin/blob/b805dbb0b9c90dadef0424e5b3bf86ac308e103e/src/script/interpreter.cpp#L189-L198
             0x01 => Ok(EcdsaSigHashType::All),
@@ -855,14 +865,10 @@ impl EcdsaSigHashType {
         }
     }
 
-    /// Converts to a u32
+    /// Converts [`EcdsaSigHashType`] to a `u32` sighash flag.
-    pub fn as_u32(self) -> u32 { self as u32 }
+    ///
-}
+    /// The returned value is guaranteed to be a valid according to standardness rules.
-
+    pub fn to_u32(self) -> u32 { self as u32 }
-impl From<EcdsaSigHashType> for u32 {
-    fn from(t: EcdsaSigHashType) -> u32 {
-        t.as_u32()
-    }
 }
 
 /// Error returned when parsing `SigHashType` fails.
@@ -1194,7 +1200,6 @@ mod tests {
     fn test_sighashtype_standard() {
         let nonstandard_hashtype = 0x04;
         // This type is not well defined, by consensus it becomes ALL
-        assert_eq!(EcdsaSigHashType::from_u32(nonstandard_hashtype), EcdsaSigHashType::All);
         assert_eq!(EcdsaSigHashType::from_u32_consensus(nonstandard_hashtype), EcdsaSigHashType::All);
         // But it's policy-invalid to use it!
         assert_eq!(EcdsaSigHashType::from_u32_standard(nonstandard_hashtype), Err(NonStandardSigHashType(0x04)));

src/util/ecdsa.rs

@@ -47,7 +47,7 @@ impl EcdsaSig {
     pub fn from_slice(sl: &[u8]) -> Result<Self, EcdsaSigError> {
         let (hash_ty, sig) = sl.split_last()
             .ok_or(EcdsaSigError::EmptySignature)?;
-        let hash_ty = EcdsaSigHashType::from_u32_standard(*hash_ty as u32)
+        let hash_ty = EcdsaSigHashType::from_standard(*hash_ty as u32)
             .map_err(|_| EcdsaSigError::NonStandardSigHashType(*hash_ty as u32))?;
         let sig = secp256k1::ecdsa::Signature::from_der(sig)
             .map_err(EcdsaSigError::Secp256k1)?;
@@ -80,7 +80,7 @@ impl FromStr for EcdsaSig {
             .ok_or(EcdsaSigError::EmptySignature)?;
         Ok(EcdsaSig {
             sig: secp256k1::ecdsa::Signature::from_der(signature)?,
-            hash_ty: EcdsaSigHashType::from_u32_standard(*sighash_byte as u32)?
+            hash_ty: EcdsaSigHashType::from_standard(*sighash_byte as u32)?
         })
     }
 }

src/util/psbt/map/input.rs

@@ -171,7 +171,7 @@ impl PsbtSigHashType {
     /// Returns the [`EcdsaSigHashType`] if the [`PsbtSigHashType`] can be
     /// converted to one.
     pub fn ecdsa_hash_ty(self) -> Result<EcdsaSigHashType, NonStandardSigHashType> {
-        EcdsaSigHashType::from_u32_standard(self.inner)
+        EcdsaSigHashType::from_standard(self.inner)
     }
 
     /// Returns the [`SchnorrSigHashType`] if the [`PsbtSigHashType`] can be
@@ -184,8 +184,19 @@ impl PsbtSigHashType {
         }
     }
 
-    /// Obtains the inner sighash byte from this [`PsbtSigHashType`].
+    /// Creates a [`PsbtSigHashType`] from a raw `u32`.
-    pub fn inner(self) -> u32 {
+    ///
+    /// Allows construction of a non-standard or non-valid sighash flag
+    /// ([`EcdsaSigHashType`], [`SchnorrSigHashType`] respectively).
+    pub fn from_u32(n: u32) -> PsbtSigHashType {
+        PsbtSigHashType { inner: n }
+    }
+
+
+    /// Converts [`PsbtSigHashType`] to a raw `u32` sighash flag.
+    ///
+    /// No guarantees are made as to the standardness or validity of the returned value.
+    pub fn to_u32(self) -> u32 {
         self.inner
     }
 }

src/util/psbt/serialize.rs

@@ -193,7 +193,7 @@ impl Deserialize for Vec<u8> {
 
 impl Serialize for PsbtSigHashType {
     fn serialize(&self) -> Vec<u8> {
-        serialize(&self.inner())
+        serialize(&self.to_u32())
     }
 }
 
@@ -367,3 +367,15 @@ impl Deserialize for TapTree {
 fn key_source_len(key_source: &KeySource) -> usize {
     4 + 4 * (key_source.1).as_ref().len()
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn can_deserialize_non_standard_psbt_sig_hash_type() {
+        let non_standard_sighash = [222u8, 0u8, 0u8, 0u8]; // 32 byte value.
+        let sighash = PsbtSigHashType::deserialize(&non_standard_sighash);
+        assert!(sighash.is_ok())
+    }
+}

src/util/sighash.rs

@@ -600,7 +600,7 @@ impl<R: Deref<Target=Transaction>> SigHashCache<R> {
         }
 
         self.tx.lock_time.consensus_encode(&mut writer)?;
-        sighash_type.as_u32().consensus_encode(&mut writer)?;
+        sighash_type.to_u32().consensus_encode(&mut writer)?;
         Ok(())
     }