Merge rust-bitcoin/rust-bitcoin#1007: Implement TryFrom

b29ff9b Rename SchnorrSighashType::from_u8 -> from_consensus_u8 (Tobin C. Harding)
af16286 Implement TryFrom sha256::Hash for TaprootMerkleBranch (Tobin C. Harding)
6b7b440 Implement TryFrom<Key> for ProprietaryKey (Tobin C. Harding)
5c49fe7 Implement TryFrom<TaprootBuilder> for TapTree (Tobin C. Harding)
632a5db Implement TryFrom for WitnessVersion (Tobin C. Harding)

Pull request description:

  Audit the whole codebase checking for any method that is of the form `from_foo` where foo is not an interesting identifier (like 'consensus' and 'standard'). Implement `TryFrom` for any such methods, deprecating the original.

  Done as separate patches so any can be easily dropped if not liked.

ACKs for top commit:
  apoelstra:
    ACK b29ff9b
  Kixunil:
    ACK b29ff9b

Tree-SHA512: 40f1d96b505891080df1f7a9b3507979b0279a9e0f9d7cd32598bdc16c866785e6b13d5cb1face5ba50e3bc8484a5cd9c7f430d7abc86db9609962476dacd467

Author: apoelstra

src/blockdata/script.rs

@@ -27,6 +27,7 @@ use crate::consensus::encode::MAX_VEC_SIZE;
 use crate::prelude::*;
 
 use crate::io;
+use core::convert::TryFrom;
 use core::{fmt, default::Default};
 use core::ops::Index;
 
@@ -498,7 +499,7 @@ impl Script {
     /// Returns witness version of the script, if any, assuming the script is a `scriptPubkey`.
     #[inline]
     pub fn witness_version(&self) -> Option<WitnessVersion> {
-        self.0.get(0).and_then(|opcode| WitnessVersion::from_opcode(opcodes::All::from(*opcode)).ok())
+        self.0.get(0).and_then(|opcode| WitnessVersion::try_from(opcodes::All::from(*opcode)).ok())
     }
 
     /// Checks whether a script pubkey is a P2SH output.
@@ -550,7 +551,7 @@ impl Script {
         }
         let ver_opcode = opcodes::All::from(self.0[0]); // Version 0 or PUSHNUM_1-PUSHNUM_16
         let push_opbyte = self.0[1]; // Second byte push opcode 2-40 bytes
-        WitnessVersion::from_opcode(ver_opcode).is_ok()
+        WitnessVersion::try_from(ver_opcode).is_ok()
             && push_opbyte >= opcodes::all::OP_PUSHBYTES_2.to_u8()
             && push_opbyte <= opcodes::all::OP_PUSHBYTES_40.to_u8()
             // Check that the rest of the script has the correct size

src/util/address.rs

@@ -34,6 +34,7 @@
 
 use crate::prelude::*;
 
+use core::convert::TryFrom;
 use core::fmt;
 use core::num::ParseIntError;
 use core::str::FromStr;
@@ -243,8 +244,8 @@ impl FromStr for WitnessVersion {
     type Err = Error;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let version = s.parse().map_err(Error::UnparsableWitnessVersion)?;
-        WitnessVersion::from_num(version)
+        let version: u8 = s.parse().map_err(Error::UnparsableWitnessVersion)?;
+        WitnessVersion::try_from(version)
     }
 }
 
@@ -258,8 +259,9 @@ impl WitnessVersion {
     /// # Errors
     /// If the integer does not correspond to any witness version, errors with
     /// [`Error::InvalidWitnessVersion`].
+    #[deprecated(since = "0.29.0", note = "use try_from instead")]
     pub fn from_u5(value: ::bech32::u5) -> Result<Self, Error> {
-        WitnessVersion::from_num(value.to_u8())
+        Self::try_from(value)
     }
 
     /// Converts an 8-bit unsigned integer value into [`WitnessVersion`] variant.
@@ -270,27 +272,9 @@ impl WitnessVersion {
     /// # Errors
     /// If the integer does not correspond to any witness version, errors with
     /// [`Error::InvalidWitnessVersion`].
+    #[deprecated(since = "0.29.0", note = "use try_from instead")]
     pub fn from_num(no: u8) -> Result<Self, Error> {
-        Ok(match no {
-            0 => WitnessVersion::V0,
-            1 => WitnessVersion::V1,
-            2 => WitnessVersion::V2,
-            3 => WitnessVersion::V3,
-            4 => WitnessVersion::V4,
-            5 => WitnessVersion::V5,
-            6 => WitnessVersion::V6,
-            7 => WitnessVersion::V7,
-            8 => WitnessVersion::V8,
-            9 => WitnessVersion::V9,
-            10 => WitnessVersion::V10,
-            11 => WitnessVersion::V11,
-            12 => WitnessVersion::V12,
-            13 => WitnessVersion::V13,
-            14 => WitnessVersion::V14,
-            15 => WitnessVersion::V15,
-            16 => WitnessVersion::V16,
-            wrong => return Err(Error::InvalidWitnessVersion(wrong)),
-        })
+        Self::try_from(no)
     }
 
     /// Converts bitcoin script opcode into [`WitnessVersion`] variant.
@@ -301,13 +285,9 @@ impl WitnessVersion {
     /// # Errors
     /// If the opcode does not correspond to any witness version, errors with
     /// [`Error::MalformedWitnessVersion`].
+    #[deprecated(since = "0.29.0", note = "use try_from instead")]
     pub fn from_opcode(opcode: opcodes::All) -> Result<Self, Error> {
-        match opcode.to_u8() {
-            0 => Ok(WitnessVersion::V0),
-            version if version >= opcodes::all::OP_PUSHNUM_1.to_u8() && version <= opcodes::all::OP_PUSHNUM_16.to_u8() =>
-                WitnessVersion::from_num(version - opcodes::all::OP_PUSHNUM_1.to_u8() + 1),
-            _ => Err(Error::MalformedWitnessVersion)
-        }
+        Self::try_from(opcode)
     }
 
     /// Converts bitcoin script [`Instruction`] (parsed opcode) into [`WitnessVersion`] variant.
@@ -319,12 +299,9 @@ impl WitnessVersion {
     /// # Errors
     /// If the opcode does not correspond to any witness version, errors with
     /// [`Error::MalformedWitnessVersion`] for the rest of opcodes.
+    #[deprecated(since = "0.29.0", note = "use try_from instead")]
     pub fn from_instruction(instruction: Instruction) -> Result<Self, Error> {
-        match instruction {
-            Instruction::Op(op) => WitnessVersion::from_opcode(op),
-            Instruction::PushBytes(bytes) if bytes.is_empty() => Ok(WitnessVersion::V0),
-            Instruction::PushBytes(_) => Err(Error::MalformedWitnessVersion),
-        }
+        Self::try_from(instruction)
     }
 
     /// Returns integer version number representation for a given [`WitnessVersion`] value.
@@ -355,6 +332,102 @@ impl WitnessVersion {
     }
 }
 
+impl TryFrom<bech32::u5> for WitnessVersion {
+    type Error = Error;
+
+    /// Converts 5-bit unsigned integer value matching single symbol from Bech32(m) address encoding
+    /// ([`bech32::u5`]) into [`WitnessVersion`] variant.
+    ///
+    /// # Returns
+    /// Version of the Witness program.
+    ///
+    /// # Errors
+    /// If the integer does not correspond to any witness version, errors with
+    /// [`Error::InvalidWitnessVersion`].
+    fn try_from(value: bech32::u5) -> Result<Self, Self::Error> {
+        Self::try_from(value.to_u8())
+    }
+}
+
+impl TryFrom<u8> for WitnessVersion {
+    type Error = Error;
+
+    /// Converts an 8-bit unsigned integer value into [`WitnessVersion`] variant.
+    ///
+    /// # Returns
+    /// Version of the Witness program.
+    ///
+    /// # Errors
+    /// If the integer does not correspond to any witness version, errors with
+    /// [`Error::InvalidWitnessVersion`].
+    fn try_from(no: u8) -> Result<Self, Self::Error> {
+        use WitnessVersion::*;
+
+        Ok(match no {
+            0 => V0,
+            1 => V1,
+            2 => V2,
+            3 => V3,
+            4 => V4,
+            5 => V5,
+            6 => V6,
+            7 => V7,
+            8 => V8,
+            9 => V9,
+            10 => V10,
+            11 => V11,
+            12 => V12,
+            13 => V13,
+            14 => V14,
+            15 => V15,
+            16 => V16,
+            wrong => return Err(Error::InvalidWitnessVersion(wrong)),
+        })
+    }
+}
+
+impl TryFrom<opcodes::All> for WitnessVersion {
+    type Error = Error;
+
+    /// Converts bitcoin script opcode into [`WitnessVersion`] variant.
+    ///
+    /// # Returns
+    /// Version of the Witness program (for opcodes in range of `OP_0`..`OP_16`).
+    ///
+    /// # Errors
+    /// If the opcode does not correspond to any witness version, errors with
+    /// [`Error::MalformedWitnessVersion`].
+    fn try_from(opcode: opcodes::All) -> Result<Self, Self::Error> {
+        match opcode.to_u8() {
+            0 => Ok(WitnessVersion::V0),
+            version if version >= opcodes::all::OP_PUSHNUM_1.to_u8() && version <= opcodes::all::OP_PUSHNUM_16.to_u8() =>
+                WitnessVersion::try_from(version - opcodes::all::OP_PUSHNUM_1.to_u8() + 1),
+            _ => Err(Error::MalformedWitnessVersion)
+        }
+    }
+}
+
+impl<'a> TryFrom<Instruction<'a>> for WitnessVersion {
+    type Error = Error;
+
+    /// Converts bitcoin script [`Instruction`] (parsed opcode) into [`WitnessVersion`] variant.
+    ///
+    /// # Returns
+    /// Version of the Witness program for [`Instruction::Op`] and [`Instruction::PushBytes`] with
+    /// byte value within `1..=16` range.
+    ///
+    /// # Errors
+    /// If the opcode does not correspond to any witness version, errors with
+    /// [`Error::MalformedWitnessVersion`] for the rest of opcodes.
+    fn try_from(instruction: Instruction) -> Result<Self, Self::Error> {
+        match instruction {
+            Instruction::Op(op) => WitnessVersion::try_from(op),
+            Instruction::PushBytes(bytes) if bytes.is_empty() => Ok(WitnessVersion::V0),
+            Instruction::PushBytes(_) => Err(Error::MalformedWitnessVersion),
+        }
+    }
+}
+
 impl From<WitnessVersion> for ::bech32::u5 {
     /// Converts [`WitnessVersion`] instance into corresponding Bech32(m) u5-value ([`bech32::u5`]).
     fn from(version: WitnessVersion) -> Self {
@@ -405,7 +478,7 @@ impl Payload {
             }
 
             Payload::WitnessProgram {
-                version: WitnessVersion::from_opcode(opcodes::All::from(script[0]))?,
+                version: WitnessVersion::try_from(opcodes::All::from(script[0]))?,
                 program: script[2..].to_vec(),
             }
         } else {
@@ -856,7 +929,7 @@ impl FromStr for Address {
             // Get the script version and program (converted from 5-bit to 8-bit)
             let (version, program): (WitnessVersion, Vec<u8>) = {
                 let (v, p5) = payload.split_at(1);
-                (WitnessVersion::from_u5(v[0])?, bech32::FromBase32::from_base32(p5)?)
+                (WitnessVersion::try_from(v[0])?, bech32::FromBase32::from_base32(p5)?)
             };
 
             if program.len() < 2 || program.len() > 40 {
@@ -1277,7 +1350,7 @@ mod tests {
         ];
         let segwit_payload = (0..=16).map(|version| {
             Payload::WitnessProgram {
-                version: WitnessVersion::from_num(version).unwrap(),
+                version: WitnessVersion::try_from(version).unwrap(),
                 program: vec![]
             }
         }).collect::<Vec<_>>();

src/util/psbt/map/global.rs

@@ -12,6 +12,8 @@
 // If not, see <http://creativecommons.org/publicdomain/zero/1.0/>.
 //
 
+use core::convert::TryFrom;
+
 use crate::prelude::*;
 
 use crate::io::{self, Cursor, Read};
@@ -191,7 +193,7 @@ impl PartiallySignedTransaction {
                                 return Err(Error::InvalidKey(pair.key).into())
                             }
                         }
-                        PSBT_GLOBAL_PROPRIETARY => match proprietary.entry(raw::ProprietaryKey::from_key(pair.key.clone())?) {
+                        PSBT_GLOBAL_PROPRIETARY => match proprietary.entry(raw::ProprietaryKey::try_from(pair.key.clone())?) {
                             btree_map::Entry::Vacant(empty_key) => {
                                 empty_key.insert(pair.value);
                             },

src/util/psbt/map/input.rs

@@ -16,6 +16,7 @@ use crate::prelude::*;
 use crate::io;
 use core::fmt;
 use core::str::FromStr;
+use core::convert::TryFrom;
 
 use secp256k1;
 use crate::blockdata::script::Script;
@@ -215,7 +216,7 @@ impl PsbtSighashType {
         if self.inner > 0xffu32 {
             Err(sighash::Error::InvalidSighashType(self.inner))
         } else {
-            SchnorrSighashType::from_u8(self.inner as u8)
+            SchnorrSighashType::from_consensus_u8(self.inner as u8)
         }
     }
 
@@ -356,7 +357,7 @@ impl Input {
                 }
             }
             PSBT_IN_PROPRIETARY => {
-                let key = raw::ProprietaryKey::from_key(raw_key.clone())?;
+                let key = raw::ProprietaryKey::try_from(raw_key.clone())?;
                 match self.proprietary.entry(key) {
                     btree_map::Entry::Vacant(empty_key) => {
                         empty_key.insert(raw_value);

src/util/psbt/map/output.rs

@@ -14,6 +14,7 @@
 
 use crate::prelude::*;
 use core;
+use core::convert::TryFrom;
 
 use crate::io;
 
@@ -158,16 +159,12 @@ impl TapTree {
     /// # Returns
     /// A [`TapTree`] iff the `builder` is complete, otherwise return [`IncompleteTapTree`]
     /// error with the content of incomplete `builder` instance.
+    #[deprecated(since = "0.29.0", note = "use try_from instead")]
     pub fn from_builder(builder: TaprootBuilder) -> Result<Self, IncompleteTapTree> {
-        if !builder.is_finalized() {
-            Err(IncompleteTapTree::NotFinalized(builder))
-        } else if builder.has_hidden_nodes() {
-            Err(IncompleteTapTree::HiddenParts(builder))
-        } else {
-            Ok(TapTree(builder))
-        }
+        Self::try_from(builder)
     }
 
+
     /// Converts self into builder [`TaprootBuilder`]. The builder is guaranteed to be finalized.
     pub fn into_builder(self) -> TaprootBuilder {
         self.0
@@ -194,6 +191,25 @@ impl TapTree {
     }
 }
 
+impl TryFrom<TaprootBuilder> for TapTree {
+    type Error = IncompleteTapTree;
+
+    /// Constructs [`TapTree`] from a [`TaprootBuilder`] if it is complete binary tree.
+    ///
+    /// # Returns
+    /// A [`TapTree`] iff the `builder` is complete, otherwise return [`IncompleteTapTree`]
+    /// error with the content of incomplete `builder` instance.
+    fn try_from(builder: TaprootBuilder) -> Result<Self, Self::Error> {
+        if !builder.is_finalized() {
+            Err(IncompleteTapTree::NotFinalized(builder))
+        } else if builder.has_hidden_nodes() {
+            Err(IncompleteTapTree::HiddenParts(builder))
+        } else {
+            Ok(TapTree(builder))
+        }
+    }
+}
+
 /// Iterator for a taproot script tree, operating in DFS order over leaf depth and
 /// leaf script pairs.
 pub struct TapTreeIter<'tree> {
@@ -233,7 +249,7 @@ impl Output {
                 }
             }
             PSBT_OUT_PROPRIETARY => {
-                let key = raw::ProprietaryKey::from_key(raw_key.clone())?;
+                let key = raw::ProprietaryKey::try_from(raw_key.clone())?;
                 match self.proprietary.entry(key) {
                     btree_map::Entry::Vacant(empty_key) => {
                         empty_key.insert(raw_value);

src/util/psbt/raw.rs

@@ -20,6 +20,7 @@
 
 use crate::prelude::*;
 use core::fmt;
+use core::convert::TryFrom;
 
 use crate::io;
 use crate::consensus::encode::{self, ReadExt, WriteExt, Decodable, Encodable, VarInt, serialize, deserialize, MAX_VEC_SIZE};
@@ -160,12 +161,9 @@ impl<Subtype> Decodable for ProprietaryKey<Subtype> where Subtype: Copy + From<u
 impl<Subtype> ProprietaryKey<Subtype> where Subtype: Copy + From<u8> + Into<u8> {
     /// Constructs [ProprietaryKey] from [Key]; returns
     /// [Error::InvalidProprietaryKey] if `key` do not starts with 0xFC byte
+    #[deprecated(since = "0.29.0", note = "use try_from instead")]
     pub fn from_key(key: Key) -> Result<Self, Error> {
-        if key.type_value != 0xFC {
-            return Err(Error::InvalidProprietaryKey)
-        }
-
-        Ok(deserialize(&key.key)?)
+        Self::try_from(key)
     }
 
     /// Constructs full [Key] corresponding to this proprietary key type
@@ -176,3 +174,21 @@ impl<Subtype> ProprietaryKey<Subtype> where Subtype: Copy + From<u8> + Into<u8>
         }
     }
 }
+
+impl<Subtype> TryFrom<Key> for ProprietaryKey<Subtype>
+where
+    Subtype:Copy + From<u8> + Into<u8> {
+    type Error = Error;
+
+    /// Constructs a [`ProprietaryKey`] from a [`Key`].
+    ///
+    /// # Errors
+    /// Returns [`Error::InvalidProprietaryKey`] if `key` does not start with `0xFC` byte.
+    fn try_from(key: Key) -> Result<Self, Self::Error> {
+        if key.type_value != 0xFC {
+            return Err(Error::InvalidProprietaryKey)
+        }
+
+        Ok(deserialize(&key.key)?)
+    }
+}