Merge rust-bitcoin/rust-bitcoin#669: PSBT: partial sig data type

14ace92666 Fix SchnorrSig type references in PSBT serialization macros (Dr Maxim Orlovsky)
2b530000d3 Use EcdsaSig in PSBT partial signatures instead of Vec<u8> (Dr Maxim Orlovsky)
141dbbd1b9 Add serde impl for EcdsaSig (Dr Maxim Orlovsky)
c92057d98f PSBT serialize/deserialize impl for EcdsaSig type (Dr Maxim Orlovsky)
0af1c3f320 Add Display and FromStr for EcdsaSig (Dr Maxim Orlovsky)
daf0eacf3d Improve NonStandardSigHashType (Dr Maxim Orlovsky)
c36a3da6f0 Add EcdsaSig::sighash_all convenience constructor (Dr Maxim Orlovsky)

Pull request description:

  Previously signatures were kept in PSBT as raw byte vec, without processing. This adds specific data type, capable of holding & serializing/deserializing partial signature with sighash flag information.

ACKs for top commit:
  apoelstra:
    ACK 14ace92666
  Kixunil:
    ACK 14ace92666

Tree-SHA512: f505df9d1990735fe3941092174a61e067a4e3db30d2d1b94b136da4607865b3b78b274e674b2cfb877aaf0ab58e007c1ab1738f3927d60a4a3ecc12cadc5a48

src/blockdata/transaction.rs

@@ -666,11 +666,11 @@ impl Decodable for Transaction {
 /// This type is consensus valid but an input including it would prevent the transaction from
 /// being relayed on today's Bitcoin network.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
-pub struct NonStandardSigHashType;
+pub struct NonStandardSigHashType(pub u32);
 
 impl fmt::Display for NonStandardSigHashType {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        write!(f, "Non standard sighash type")
+        write!(f, "Non standard sighash type {}", self.0)
     }
 }
 
@@ -789,7 +789,7 @@ impl EcdsaSigHashType {
              0x81 => Ok(EcdsaSigHashType::AllPlusAnyoneCanPay),
              0x82 => Ok(EcdsaSigHashType::NonePlusAnyoneCanPay),
              0x83 => Ok(EcdsaSigHashType::SinglePlusAnyoneCanPay),
-             _ => Err(NonStandardSigHashType)
+             non_standard => Err(NonStandardSigHashType(non_standard))
          }
      }
 
@@ -1151,7 +1151,7 @@ mod tests {
         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));
+        assert_eq!(EcdsaSigHashType::from_u32_standard(nonstandard_hashtype), Err(NonStandardSigHashType(0x04)));
     }
 
     // These test vectors were stolen from libbtc, which is Copyright 2014 Jonas Schnelli MIT

src/util/ecdsa.rs

@@ -25,11 +25,12 @@ use io;
 
 use secp256k1::{self, Secp256k1};
 use network::constants::Network;
-use hashes::{Hash, hash160};
+use hashes::{Hash, hash160, hex};
+use hashes::hex::FromHex;
 use hash_types::{PubkeyHash, WPubkeyHash};
 use util::base58;
 use util::key::Error;
-use blockdata::transaction::EcdsaSigHashType;
+use blockdata::transaction::{EcdsaSigHashType, NonStandardSigHashType};
 
 
 /// A Bitcoin ECDSA public key
@@ -417,6 +418,7 @@ impl<'de> ::serde::Deserialize<'de> for PublicKey {
 
 /// An ECDSA signature with the corresponding hash type.
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 pub struct EcdsaSig {
     /// The underlying ECDSA Signature
     pub sig: secp256k1::ecdsa::Signature,
@@ -425,13 +427,20 @@ pub struct EcdsaSig {
 }
 
 impl EcdsaSig {
+    /// Constructs ECDSA bitcoin signature for [`EcdsaSigHashType::All`]
+    pub fn sighash_all(sig: secp256k1::ecdsa::Signature) -> EcdsaSig {
+        EcdsaSig {
+            sig,
+            hash_ty: EcdsaSigHashType::All
+        }
+    }
 
     /// Deserialize from slice
     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)
-            .map_err(|_| EcdsaSigError::NonStandardSigHashType(*hash_ty))?;
+            .map_err(|_| EcdsaSigError::NonStandardSigHashType(*hash_ty as u32))?;
         let sig = secp256k1::ecdsa::Signature::from_der(sig)
             .map_err(EcdsaSigError::Secp256k1)?;
         Ok(EcdsaSig { sig, hash_ty })
@@ -446,11 +455,34 @@ impl EcdsaSig {
     }
 }
 
+impl fmt::Display for EcdsaSig {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        hex::format_hex(&self.sig.serialize_der(), f)?;
+        hex::format_hex(&[self.hash_ty as u8], f)
+    }
+}
+
+impl FromStr for EcdsaSig {
+    type Err = EcdsaSigError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let bytes = Vec::from_hex(s)?;
+        let (sighash_byte, signature) = bytes.split_last()
+            .ok_or(EcdsaSigError::EmptySignature)?;
+        Ok(EcdsaSig {
+            sig: secp256k1::ecdsa::Signature::from_der(signature)?,
+            hash_ty: EcdsaSigHashType::from_u32_standard(*sighash_byte as u32)?
+        })
+    }
+}
+
 /// A key-related error.
 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
 pub enum EcdsaSigError {
+    /// Hex encoding error
+    HexEncoding(hex::Error),
     /// Base58 encoding error
-    NonStandardSigHashType(u8),
+    NonStandardSigHashType(u32),
     /// Empty Signature
     EmptySignature,
     /// secp256k1-related error
@@ -467,6 +499,7 @@ impl fmt::Display for EcdsaSigError {
                 write!(f, "Invalid Ecdsa signature: {}", e),
             EcdsaSigError::EmptySignature =>
                 write!(f, "Empty ECDSA signature"),
+            EcdsaSigError::HexEncoding(e) => write!(f, "EcdsaSig hex encoding error: {}", e)
         }
     }
 }
@@ -481,6 +514,18 @@ impl From<secp256k1::Error> for EcdsaSigError {
     }
 }
 
+impl From<NonStandardSigHashType> for EcdsaSigError {
+    fn from(err: NonStandardSigHashType) -> Self {
+        EcdsaSigError::NonStandardSigHashType(err.0)
+    }
+}
+
+impl From<hex::Error> for EcdsaSigError {
+    fn from(err: hex::Error) -> Self {
+        EcdsaSigError::HexEncoding(err)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use io;

src/util/psbt/map/input.rs

@@ -14,7 +14,7 @@
 
 use prelude::*;
 
-use io;
+use ::{EcdsaSig, io};
 
 use blockdata::script::Script;
 use blockdata::transaction::{EcdsaSigHashType, Transaction, TxOut};
@@ -29,7 +29,7 @@ use util::psbt::raw;
 use util::psbt::serialize::Deserialize;
 use util::psbt::{Error, error};
 
-use schnorr;
+use ::{SchnorrSig};
 use util::taproot::{ControlBlock, LeafVersion, TapLeafHash, TapBranchHash};
 
 /// Type: Non-Witness UTXO PSBT_IN_NON_WITNESS_UTXO = 0x00
@@ -87,9 +87,8 @@ pub struct Input {
     /// including P2SH embedded ones.
     pub witness_utxo: Option<TxOut>,
     /// A map from public keys to their corresponding signature as would be
-    /// pushed to the stack from a scriptSig or witness.
+    /// pushed to the stack from a scriptSig or witness for a non-taproot inputs.
-    #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_byte_values"))]
+    pub partial_sigs: BTreeMap<PublicKey, EcdsaSig>,
-    pub partial_sigs: BTreeMap<PublicKey, Vec<u8>>,
     /// The sighash type to be used for this input. Signatures for this input
     /// must use the sighash type.
     pub sighash_type: Option<EcdsaSigHashType>,
@@ -121,10 +120,10 @@ pub struct Input {
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_byte_values"))]
     pub hash256_preimages: BTreeMap<sha256d::Hash, Vec<u8>>,
     /// Serialized schnorr signature with sighash type for key spend
-    pub tap_key_sig: Option<schnorr::SchnorrSig>,
+    pub tap_key_sig: Option<SchnorrSig>,
     /// Map of <xonlypubkey>|<leafhash> with signature
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_as_seq"))]
-    pub tap_script_sigs: BTreeMap<(XOnlyPublicKey, TapLeafHash), schnorr::SchnorrSig>,
+    pub tap_script_sigs: BTreeMap<(XOnlyPublicKey, TapLeafHash), SchnorrSig>,
     /// Map of Control blocks to Script version pair
     #[cfg_attr(feature = "serde", serde(with = "::serde_utils::btreemap_as_seq"))]
     pub tap_scripts: BTreeMap<ControlBlock, (Script, LeafVersion)>,
@@ -163,7 +162,7 @@ impl Map for Input {
             }
             PSBT_IN_PARTIAL_SIG => {
                 impl_psbt_insert_pair! {
-                    self.partial_sigs <= <raw_key: PublicKey>|<raw_value: Vec<u8>>
+                    self.partial_sigs <= <raw_key: PublicKey>|<raw_value: EcdsaSig>
                 }
             }
             PSBT_IN_SIGHASH_TYPE => {
@@ -210,12 +209,12 @@ impl Map for Input {
             }
             PSBT_IN_TAP_KEY_SIG => {
                 impl_psbt_insert_pair! {
-                    self.tap_key_sig <= <raw_key: _>|<raw_value: schnorr::SchnorrSig>
+                    self.tap_key_sig <= <raw_key: _>|<raw_value: SchnorrSig>
                 }
             }
             PSBT_IN_TAP_SCRIPT_SIG => {
                 impl_psbt_insert_pair! {
-                    self.tap_script_sigs <= <raw_key: (XOnlyPublicKey, TapLeafHash)>|<raw_value: schnorr::SchnorrSig>
+                    self.tap_script_sigs <= <raw_key: (XOnlyPublicKey, TapLeafHash)>|<raw_value: SchnorrSig>
                 }
             }
             PSBT_IN_TAP_LEAF_SCRIPT=> {
@@ -267,7 +266,7 @@ impl Map for Input {
         }
 
         impl_psbt_get_pair! {
-            rv.push(self.partial_sigs as <PSBT_IN_PARTIAL_SIG, PublicKey>|<Vec<u8>>)
+            rv.push(self.partial_sigs as <PSBT_IN_PARTIAL_SIG, PublicKey>|<EcdsaSig>)
         }
 
         impl_psbt_get_pair! {
@@ -311,11 +310,11 @@ impl Map for Input {
         }
 
         impl_psbt_get_pair! {
-            rv.push(self.tap_key_sig as <PSBT_IN_TAP_KEY_SIG, _>|<Vec<u8>>)
+            rv.push(self.tap_key_sig as <PSBT_IN_TAP_KEY_SIG, _>|<SchnorrSig>)
         }
 
         impl_psbt_get_pair! {
-            rv.push(self.tap_script_sigs as <PSBT_IN_TAP_SCRIPT_SIG, (XOnlyPublicKey, TapLeafHash)>|<Vec<u8>>)
+            rv.push(self.tap_script_sigs as <PSBT_IN_TAP_SCRIPT_SIG, (XOnlyPublicKey, TapLeafHash)>|<SchnorrSig>)
         }
 
         impl_psbt_get_pair! {

src/util/psbt/mod.rs

@@ -477,7 +477,7 @@ mod tests {
                 witness_script: None,
                 partial_sigs: vec![(
                     "0339880dc92394b7355e3d0439fa283c31de7590812ea011c4245c0674a685e883".parse().unwrap(),
-                    vec![8, 5, 4],
+                    "304402204f67e2afb76142d44fae58a2495d33a3419daa26cd0db8d04f3452b63289ac0f022010762a9fb67e94cc5cad9026f6dc99ff7f070f4278d30fbc7d0c869dd38c7fe701".parse().unwrap(),
                 )].into_iter().collect(),
                 bip32_derivation: keypaths.clone(),
                 final_script_witness: Some(vec![vec![1, 3], vec![5]]),

src/util/psbt/serialize.rs

@@ -28,7 +28,7 @@ use consensus::encode::{self, serialize, Decodable, Encodable, deserialize_parti
 use secp256k1::{self, XOnlyPublicKey};
 use util::bip32::{ChildNumber, Fingerprint, KeySource};
 use hashes::{hash160, ripemd160, sha256, sha256d, Hash};
-use util::ecdsa::PublicKey;
+use util::ecdsa::{PublicKey, EcdsaSig};
 use util::psbt;
 use util::taproot::{TapBranchHash, TapLeafHash, ControlBlock, LeafVersion};
 use schnorr;
@@ -89,6 +89,30 @@ impl Deserialize for PublicKey {
     }
 }
 
+impl Serialize for EcdsaSig {
+    fn serialize(&self) -> Vec<u8> {
+        let mut buf = Vec::with_capacity(72);
+        buf.extend(self.sig.serialize_der().iter());
+        buf.push(self.hash_ty as u8);
+        buf
+    }
+}
+
+impl Deserialize for EcdsaSig {
+    fn deserialize(bytes: &[u8]) -> Result<Self, encode::Error> {
+        let (sighash_byte, signature) = bytes.split_last()
+            .ok_or(encode::Error::ParseFailed("empty partial signature data"))?;
+        Ok(EcdsaSig {
+            sig: secp256k1::ecdsa::Signature::from_der(signature)
+                .map_err(|_| encode::Error::ParseFailed("non-DER encoded signature"))?,
+            // NB: Since BIP-174 says "the signature as would be pushed to the stack from
+            // a scriptSig or witness" we should use a consensus deserialization and do
+            // not error on a non-standard values.
+            hash_ty: EcdsaSigHashType::from_u32_consensus(*sighash_byte as u32)
+        })
+    }
+}
+
 impl Serialize for KeySource {
     fn serialize(&self) -> Vec<u8> {
         let mut rv: Vec<u8> = Vec::with_capacity(key_source_len(&self));