DescriptorPublicKey, second pass

[darosior]

This is a rebased, cleaned up and amended version of #93.

It's based on #130.

Here is the diff with a [rebased-on-master #93 ](https://github.com/darosior/rust-miniscript/commits/sgeisler_2020-06-descriptor-key_rebased) at commit cdfab02

diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml
index dac83c0..3d34f23 100644
--- a/.github/workflows/rust.yml
+++ b/.github/workflows/rust.yml
@@ -58,8 +58,8 @@ jobs:
           - 1.22.0
           - beta
           - stable
-    steps:      
-      - name: Checkout Crate 
+    steps:
+      - name: Checkout Crate
         uses: actions/checkout@v2
       - name: Checkout Toolchain
         uses: actions-rs/toolchain@v1
@@ -67,6 +67,8 @@ jobs:
           profile: minimal
           toolchain: ${{ matrix.rust }}
           override: true
+      - name: Pin cc if rust 1.22
+        if: matrix.rust == '1.22.0'
+        run: cargo generate-lockfile --verbose && cargo update -p cc --precise "1.0.41" --verbose
       - name: Running cargo
         run: ./contrib/test.sh
-          
diff --git a/Cargo.toml b/Cargo.toml
index 429d336..d4397e6 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -14,7 +14,7 @@ unstable = []
 default = []
 
 [dependencies]
-bitcoin = {git = "https://github.com/sgeisler/rust-bitcoin/", branch = "2020-06-bip32-derive-more"}
+bitcoin = "0.24"
 
 [dependencies.serde]
 version = "1.0"
diff --git a/src/descriptor/create_descriptor.rs b/src/descriptor/create_descriptor.rs
index b698dbc..d36a7cb 100644
--- a/src/descriptor/create_descriptor.rs
+++ b/src/descriptor/create_descriptor.rs
@@ -24,8 +24,7 @@ use ToPublicKey;
 /// NOTE: Miniscript pushes should only be either boolean, 1 or 0, signatures, and hash preimages.
 /// As per the current implementation, PUSH_NUM2 results in an error
 fn instr_to_stackelem<'txin>(
-    ins: &Result<Instruction<'txin>,
-    bitcoin::blockdata::script::Error>,
+    ins: &Result<Instruction<'txin>, bitcoin::blockdata::script::Error>,
 ) -> Result<StackElement<'txin>, Error> {
     match *ins {
         //Also covers the dissatisfied case as PushBytes0
@@ -322,8 +321,9 @@ mod tests {
         assert_eq!(stack, stack![Push(&sigs[0])]);
 
         //test wpkh
-        let script_pubkey =
-            bitcoin::Address::p2wpkh(&pks[1], bitcoin::Network::Bitcoin).unwrap().script_pubkey();
+        let script_pubkey = bitcoin::Address::p2wpkh(&pks[1], bitcoin::Network::Bitcoin)
+            .unwrap()
+            .script_pubkey();
         let script_sig = script::Builder::new().into_script();
         let witness = vec![sigs[1].clone(), pks[1].clone().to_bytes()];
         let (des, stack) = from_txin_with_witness_stack(&script_pubkey, &script_sig, &witness)
@@ -385,10 +385,12 @@ mod tests {
         assert_eq!(stack, stack![Push(&sigs[1]), Push(&sigs[3])]);
 
         //test shwpkh
-        let script_pubkey =
-            bitcoin::Address::p2shwpkh(&pks[2], bitcoin::Network::Bitcoin).unwrap().script_pubkey();
-        let redeem_script =
-            bitcoin::Address::p2wpkh(&pks[2], bitcoin::Network::Bitcoin).unwrap().script_pubkey();
+        let script_pubkey = bitcoin::Address::p2shwpkh(&pks[2], bitcoin::Network::Bitcoin)
+            .unwrap()
+            .script_pubkey();
+        let redeem_script = bitcoin::Address::p2wpkh(&pks[2], bitcoin::Network::Bitcoin)
+            .unwrap()
+            .script_pubkey();
         let script_sig = script::Builder::new()
             .push_slice(&redeem_script.to_bytes())
             .into_script();
diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs
index 6e857a0..6b251bf 100644
--- a/src/descriptor/mod.rs
+++ b/src/descriptor/mod.rs
@@ -23,16 +23,19 @@
 //! these with BIP32 paths, pay-to-contract instructions, etc.
 //!
 
-use bitcoin::blockdata::{opcodes, script};
-use bitcoin::{self, PublicKey, Script};
-#[cfg(feature = "serde")]
-use serde::{de, ser};
 use std::fmt;
 use std::str::{self, FromStr};
 
 use bitcoin::blockdata::{opcodes, script};
+use bitcoin::hashes::hash160;
+use bitcoin::hashes::hex::FromHex;
+use bitcoin::secp256k1;
+use bitcoin::util::bip32;
 use bitcoin::{self, Script};
 
+#[cfg(feature = "serde")]
+use serde::{de, ser};
+
 use expression;
 use miniscript;
 use miniscript::context::ScriptContextError;
@@ -50,12 +53,6 @@ pub use self::satisfied_constraints::Error as InterpreterError;
 pub use self::satisfied_constraints::SatisfiedConstraint;
 pub use self::satisfied_constraints::SatisfiedConstraints;
 pub use self::satisfied_constraints::Stack;
-use bitcoin::hashes::core::fmt::Formatter;
-use bitcoin::hashes::hash160;
-use bitcoin::hashes::hex::FromHex;
-use bitcoin::secp256k1::Secp256k1;
-use bitcoin::util::bip32::{ChildNumber, DerivationPath, Error as Bip32Error, ExtendedPubKey};
-use std::fmt::{Display, Write};
 
 /// Script descriptor
 #[derive(Clone, PartialEq, Eq, PartialOrd, Ord)]
@@ -79,35 +76,44 @@ pub enum Descriptor<Pk: MiniscriptKey> {
 }
 
 #[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Hash)]
-pub enum DescriptorKey {
-    PukKey(bitcoin::PublicKey),
+pub enum DescriptorPublicKey {
+    ShortPub(DescriptorShortPub),
     XPub(DescriptorXPub),
 }
 
+#[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Hash)]
+pub struct DescriptorShortPub {
+    origin: Option<(bip32::Fingerprint, bip32::DerivationPath)>,
+    key: bitcoin::PublicKey,
+}
+
 #[derive(Debug, Eq, PartialEq, Clone, Ord, PartialOrd, Hash)]
 pub struct DescriptorXPub {
-    source: Option<([u8; 4], DerivationPath)>,
-    xpub: bitcoin::util::bip32::ExtendedPubKey,
-    derivation_path: DerivationPath,
+    origin: Option<(bip32::Fingerprint, bip32::DerivationPath)>,
+    xpub: bip32::ExtendedPubKey,
+    derivation_path: bip32::DerivationPath,
     is_wildcard: bool,
 }
 
-#[derive(Debug)]
+#[derive(Debug, PartialEq)]
 pub struct DescriptorKeyParseError(&'static str);
 
-impl Display for DescriptorKey {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        match self {
-            DescriptorKey::PukKey(pk) => pk.fmt(f),
-            DescriptorKey::XPub(xpub) => {
-                if let Some((master_id, ref master_deriv)) = &xpub.source {
-                    f.write_char('[')?;
-                    for byte in master_id {
-                        write!(f, "{:02x}", byte)?;
-                    }
-                    fmt_derivation_path(f, master_deriv)?;
-                    f.write_char(']')?;
-                }
+impl fmt::Display for DescriptorKeyParseError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.write_str(self.0)
+    }
+}
+
+impl fmt::Display for DescriptorPublicKey {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match *self {
+            DescriptorPublicKey::ShortPub(ref pk) => {
+                maybe_fmt_master_id(f, &pk.origin)?;
+                pk.key.fmt(f)?;
+                Ok(())
+            }
+            DescriptorPublicKey::XPub(ref xpub) => {
+                maybe_fmt_master_id(f, &xpub.origin)?;
                 xpub.xpub.fmt(f)?;
                 fmt_derivation_path(f, &xpub.derivation_path)?;
                 if xpub.is_wildcard {
@@ -119,29 +125,52 @@ impl Display for DescriptorKey {
     }
 }
 
-fn fmt_derivation_path(f: &mut Formatter<'_>, path: &DerivationPath) -> std::fmt::Result {
+/// Writes the fingerprint of the origin, if there is one.
+fn maybe_fmt_master_id(
+    f: &mut fmt::Formatter,
+    origin: &Option<(bip32::Fingerprint, bip32::DerivationPath)>,
+) -> fmt::Result {
+    if let Some((ref master_id, ref master_deriv)) = *origin {
+        fmt::Formatter::write_str(f, "[")?;
+        for byte in master_id.into_bytes().iter() {
+            write!(f, "{:02x}", byte)?;
+        }
+        fmt_derivation_path(f, master_deriv)?;
+        fmt::Formatter::write_str(f, "]")?;
+    }
+
+    Ok(())
+}
+
+/// Writes a derivation path to the formatter, no leading 'm'
+fn fmt_derivation_path(f: &mut fmt::Formatter, path: &bip32::DerivationPath) -> fmt::Result {
     for child in path {
         write!(f, "/{}", child)?;
     }
     Ok(())
 }
 
-impl FromStr for DescriptorKey {
+impl FromStr for DescriptorPublicKey {
     type Err = DescriptorKeyParseError;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
+        // A "raw" public key without any origin is the least we accept.
         if s.len() < 66 {
-            Err(DescriptorKeyParseError(
+            return Err(DescriptorKeyParseError(
                 "Key too short (<66 char), doesn't match any format",
-            ))
-        } else if s.chars().next().unwrap() == '[' {
+            ));
+        }
+
+        // They may specify an origin
+        let mut origin = None;
+        if s.chars().next().unwrap() == '[' {
             let mut parts = s[1..].split(']');
-            let mut origin = parts
+            let mut raw_origin = parts
                 .next()
                 .ok_or(DescriptorKeyParseError("Unclosed '['"))?
                 .split('/');
 
-            let origin_id_hex = origin.next().ok_or(DescriptorKeyParseError(
+            let origin_id_hex = raw_origin.next().ok_or(DescriptorKeyParseError(
                 "No master fingerprint found after '['",
             ))?;
 
@@ -150,55 +179,59 @@ impl FromStr for DescriptorKey {
                     "Master fingerprint should be 8 characters long",
                 ));
             }
-
-            let origin_id: [u8; 4] = FromHex::from_hex(origin_id_hex).map_err(|_| {
+            let origin_raw_id: [u8; 4] = FromHex::from_hex(origin_id_hex).map_err(|_| {
                 DescriptorKeyParseError("Malformed master fingerprint, expected 8 hex chars")
             })?;
+            let parent_fingerprint = bip32::Fingerprint::from(origin_raw_id.as_ref());
 
-            let origin_path = origin
-                .map(|p| ChildNumber::from_str(p))
-                .collect::<Result<DerivationPath, Bip32Error>>()
+            let origin_path = raw_origin
+                .map(|p| bip32::ChildNumber::from_str(p))
+                .collect::<Result<bip32::DerivationPath, bip32::Error>>()
                 .map_err(|_| {
                     DescriptorKeyParseError("Error while parsing master derivation path")
                 })?;
+            origin = Some((parent_fingerprint, origin_path));
+        }
 
-            let key_deriv = parts.next().ok_or(DescriptorKeyParseError(
-                "No key found after origin description",
-            ))?;
+        let key_part = if origin == None {
+            Ok(s)
+        } else {
+            s.split(']')
+                .collect::<Vec<&str>>()
+                .pop()
+                .ok_or(DescriptorKeyParseError("No key after origin."))
+        }?;
 
-            let (xpub, derivation_path, is_wildcard) = Self::parse_xpub_deriv(key_deriv)?;
+        if key_part.starts_with("xpub") {
+            let (xpub, derivation_path, is_wildcard) = Self::parse_xpub_deriv(key_part)?;
 
-            Ok(DescriptorKey::XPub(DescriptorXPub {
-                source: Some((origin_id, origin_path)),
+            Ok(DescriptorPublicKey::XPub(DescriptorXPub {
+                origin,
                 xpub,
                 derivation_path,
                 is_wildcard,
             }))
-        } else if s.starts_with("02") || s.starts_with("03") || s.starts_with("04") {
-            let pk = PublicKey::from_str(s)
-                .map_err(|_| DescriptorKeyParseError("Error while parsing simple public key"))?;
-            Ok(DescriptorKey::PukKey(pk))
         } else {
-            let (xpub, derivation_path, is_wildcard) = Self::parse_xpub_deriv(s)?;
-            Ok(DescriptorKey::XPub(DescriptorXPub {
-                source: None,
-                xpub,
-                derivation_path,
-                is_wildcard,
+            let key = bitcoin::PublicKey::from_str(key_part)
+                .map_err(|_| DescriptorKeyParseError("Error while parsing simple public key"))?;
+            Ok(DescriptorPublicKey::ShortPub(DescriptorShortPub {
+                key,
+                origin,
             }))
         }
     }
 }
 
-impl DescriptorKey {
+impl DescriptorPublicKey {
+    /// Parse an extended public key concatenated to a derivation path.
     fn parse_xpub_deriv(
         key_deriv: &str,
-    ) -> Result<(ExtendedPubKey, DerivationPath, bool), DescriptorKeyParseError> {
+    ) -> Result<(bip32::ExtendedPubKey, bip32::DerivationPath, bool), DescriptorKeyParseError> {
         let mut key_deriv = key_deriv.split('/');
         let xpub_str = key_deriv.next().ok_or(DescriptorKeyParseError(
             "No key found after origin description",
         ))?;
-        let xpub = ExtendedPubKey::from_str(xpub_str)
+        let xpub = bip32::ExtendedPubKey::from_str(xpub_str)
             .map_err(|_| DescriptorKeyParseError("Error while parsing xpub."))?;
 
         let mut is_wildcard = false;
@@ -212,12 +245,12 @@ impl DescriptorKey {
                         "'*' may only appear as last element in a derivation path.",
                     )))
                 } else {
-                    Some(ChildNumber::from_str(p).map_err(|_| {
+                    Some(bip32::ChildNumber::from_str(p).map_err(|_| {
                         DescriptorKeyParseError("Error while parsing key derivation path")
                     }))
                 }
             })
-            .collect::<Result<DerivationPath, _>>()?;
+            .collect::<Result<bip32::DerivationPath, _>>()?;
 
         if (&derivation_path).into_iter().all(|c| c.is_normal()) {
             Ok((xpub, derivation_path, is_wildcard))
@@ -232,15 +265,15 @@ impl DescriptorKey {
     /// self.
     ///
     /// Panics if derivation path contains a hardened child number
-    pub fn derive(&self, path: &[ChildNumber]) -> DescriptorKey {
-        assert!(path.into_iter().all(|c| c.is_normal()));
+    pub fn derive(&self, path: &[bip32::ChildNumber]) -> DescriptorPublicKey {
+        debug_assert!(path.into_iter().all(|c| c.is_normal()));
 
-        match self {
-            DescriptorKey::PukKey(pk) => DescriptorKey::PukKey(*pk),
-            DescriptorKey::XPub(xpub) => {
+        match *self {
+            DescriptorPublicKey::ShortPub(ref pk) => DescriptorPublicKey::ShortPub(pk.clone()),
+            DescriptorPublicKey::XPub(ref xpub) => {
                 if xpub.is_wildcard {
-                    DescriptorKey::XPub(DescriptorXPub {
-                        source: xpub.source.clone(),
+                    DescriptorPublicKey::XPub(DescriptorXPub {
+                        origin: xpub.origin.clone(),
                         xpub: xpub.xpub.clone(),
                         derivation_path: (&xpub.derivation_path)
                             .into_iter()
@@ -257,17 +290,17 @@ impl DescriptorKey {
     }
 }
 
-impl MiniscriptKey for DescriptorKey {
+impl MiniscriptKey for DescriptorPublicKey {
     type Hash = hash160::Hash;
 
     fn to_pubkeyhash(&self) -> Self::Hash {
-        match self {
-            DescriptorKey::PukKey(pk) => pk.to_pubkeyhash(),
-            DescriptorKey::XPub(xpub) => {
-                let ctx = Secp256k1::verification_only();
+        match *self {
+            DescriptorPublicKey::ShortPub(ref spub) => spub.key.to_pubkeyhash(),
+            DescriptorPublicKey::XPub(ref xpub) => {
+                let ctx = secp256k1::Secp256k1::verification_only();
                 xpub.xpub
                     .derive_pub(&ctx, &xpub.derivation_path)
-                    .expect("Shouldn't fail, only normal derivations")
+                    .expect("Won't fail, only normal derivations")
                     .public_key
                     .to_pubkeyhash()
             }
@@ -275,12 +308,12 @@ impl MiniscriptKey for DescriptorKey {
     }
 }
 
-impl ToPublicKey for DescriptorKey {
-    fn to_public_key(&self) -> PublicKey {
-        match self {
-            DescriptorKey::PukKey(pk) => *pk,
-            DescriptorKey::XPub(xpub) => {
-                let ctx = Secp256k1::verification_only();
+impl ToPublicKey for DescriptorPublicKey {
+    fn to_public_key(&self) -> bitcoin::PublicKey {
+        match *self {
+            DescriptorPublicKey::ShortPub(ref spub) => spub.key.to_public_key(),
+            DescriptorPublicKey::XPub(ref xpub) => {
+                let ctx = secp256k1::Secp256k1::verification_only();
                 xpub.xpub
                     .derive_pub(&ctx, &xpub.derivation_path)
                     .expect("Shouldn't fail, only normal derivations")
@@ -294,12 +327,6 @@ impl ToPublicKey for DescriptorKey {
     }
 }
 
-impl Display for DescriptorKeyParseError {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        f.write_str(self.0)
-    }
-}
-
 impl<Pk: MiniscriptKey> Descriptor<Pk> {
     /// Convert a descriptor using abstract keys to one using specific keys
     /// This will panic if translatefpk returns an uncompressed key when
@@ -659,11 +686,14 @@ impl<Pk: MiniscriptKey + ToPublicKey> Descriptor<Pk> {
     }
 }
 
-impl Descriptor<DescriptorKey> {
+impl Descriptor<DescriptorPublicKey> {
     /// Derives all wildcard keys in the descriptor using the supplied `path`
-    pub fn derive(&self, path: &[ChildNumber]) -> Descriptor<DescriptorKey> {
-        self.translate_pk(|pk| Result::<_, ()>::Ok(pk.derive(path)), |pkh| Ok(*pkh))
-            .expect("Translation fn can't fail.")
+    pub fn derive(&self, path: &[bip32::ChildNumber]) -> Descriptor<DescriptorPublicKey> {
+        self.translate_pk(
+            |pk| Result::Ok::<DescriptorPublicKey, ()>(pk.derive(path)),
+            |pkh| Result::Ok::<hash160::Hash, ()>(*pkh),
+        )
+        .expect("Translation fn can't fail.")
     }
 }
 
@@ -793,15 +823,18 @@ serde_string_impl_pk!(Descriptor, "a script descriptor");
 
 #[cfg(test)]
 mod tests {
+    use super::DescriptorKeyParseError;
+
     use bitcoin::blockdata::opcodes::all::{OP_CLTV, OP_CSV};
     use bitcoin::blockdata::script::Instruction;
     use bitcoin::blockdata::{opcodes, script};
     use bitcoin::hashes::hex::FromHex;
     use bitcoin::hashes::{hash160, sha256};
-    use bitcoin::util::bip32::{ChildNumber, DerivationPath, ExtendedPubKey};
+    use bitcoin::util::bip32;
     use bitcoin::{self, secp256k1, PublicKey};
-    use descriptor::{DescriptorKey, DescriptorXPub};
+    use descriptor::{DescriptorPublicKey, DescriptorShortPub, DescriptorXPub};
     use miniscript::satisfy::BitcoinSig;
+    use policy;
     use std::collections::HashMap;
     use std::str::FromStr;
     use {Descriptor, DummyKey, Miniscript, Satisfier};
@@ -1320,53 +1353,137 @@ mod tests {
 
     #[test]
     fn parse_descriptor_key() {
-        let key = "[d34db33f/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*";
-        let expected = DescriptorKey::XPub(DescriptorXPub {
-            source: Some((
-                [0xd3, 0x4d, 0xb3, 0x3f],
+        // With a wildcard
+        let key = "[78412e3a/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*";
+        let expected = DescriptorPublicKey::XPub(DescriptorXPub {
+            origin: Some((
+                bip32::Fingerprint::from(&[0x78, 0x41, 0x2e, 0x3a][..]),
                 (&[
-                    ChildNumber::from_hardened_idx(44).unwrap(),
-                    ChildNumber::from_hardened_idx(0).unwrap(),
-                    ChildNumber::from_hardened_idx(0).unwrap(),
+                    bip32::ChildNumber::from_hardened_idx(44).unwrap(),
+                    bip32::ChildNumber::from_hardened_idx(0).unwrap(),
+                    bip32::ChildNumber::from_hardened_idx(0).unwrap(),
                 ][..])
                 .into(),
             )),
-            xpub: ExtendedPubKey::from_str("xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL").unwrap(),
-            derivation_path: (&[ChildNumber::from_normal_idx(1).unwrap()][..]).into(),
+            xpub: bip32::ExtendedPubKey::from_str("xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL").unwrap(),
+            derivation_path: (&[bip32::ChildNumber::from_normal_idx(1).unwrap()][..]).into(),
             is_wildcard: true,
         });
         assert_eq!(expected, key.parse().unwrap());
         assert_eq!(format!("{}", expected), key);
 
+        // Without origin
         let key = "xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1";
-        let expected = DescriptorKey::XPub(DescriptorXPub {
-            source: None,
-            xpub: ExtendedPubKey::from_str("xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL").unwrap(),
-            derivation_path: (&[ChildNumber::from_normal_idx(1).unwrap()][..]).into(),
+        let expected = DescriptorPublicKey::XPub(DescriptorXPub {
+            origin: None,
+            xpub: bip32::ExtendedPubKey::from_str("xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL").unwrap(),
+            derivation_path: (&[bip32::ChildNumber::from_normal_idx(1).unwrap()][..]).into(),
             is_wildcard: false,
         });
         assert_eq!(expected, key.parse().unwrap());
         assert_eq!(format!("{}", expected), key);
 
+        // Without derivation path
         let key = "xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL";
-        let expected = DescriptorKey::XPub(DescriptorXPub {
-            source: None,
-            xpub: ExtendedPubKey::from_str("xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL").unwrap(),
-            derivation_path: DerivationPath::from(&[][..]),
+        let expected = DescriptorPublicKey::XPub(DescriptorXPub {
+            origin: None,
+            xpub: bip32::ExtendedPubKey::from_str("xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL").unwrap(),
+            derivation_path: bip32::DerivationPath::from(&[][..]),
             is_wildcard: false,
         });
         assert_eq!(expected, key.parse().unwrap());
         assert_eq!(format!("{}", expected), key);
 
+        // Raw (compressed) pubkey
         let key = "03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8";
-        let expected = DescriptorKey::PukKey(
-            bitcoin::PublicKey::from_str(
+        let expected = DescriptorPublicKey::ShortPub(DescriptorShortPub {
+            key: bitcoin::PublicKey::from_str(
                 "03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8",
             )
             .unwrap(),
-        );
+            origin: None,
+        });
         assert_eq!(expected, key.parse().unwrap());
         assert_eq!(format!("{}", expected), key);
+
+        // Raw (uncompressed) pubkey
+        let key = "04f5eeb2b10c944c6b9fbcfff94c35bdeecd93df977882babc7f3a2cf7f5c81d3b09a68db7f0e04f21de5d4230e75e6dbe7ad16eefe0d4325a62067dc6f369446a";
+        let expected = DescriptorPublicKey::ShortPub(DescriptorShortPub {
+            key: bitcoin::PublicKey::from_str(
+                "04f5eeb2b10c944c6b9fbcfff94c35bdeecd93df977882babc7f3a2cf7f5c81d3b09a68db7f0e04f21de5d4230e75e6dbe7ad16eefe0d4325a62067dc6f369446a",
+            )
+            .unwrap(),
+            origin: None,
+        });
+        assert_eq!(expected, key.parse().unwrap());
+        assert_eq!(format!("{}", expected), key);
+
+        // Raw pubkey with origin
+        let desc =
+            "[78412e3a/0'/42/0']0231c7d3fc85c148717848033ce276ae2b464a4e2c367ed33886cc428b8af48ff8";
+        let expected = DescriptorPublicKey::ShortPub(DescriptorShortPub {
+            key: bitcoin::PublicKey::from_str(
+                "0231c7d3fc85c148717848033ce276ae2b464a4e2c367ed33886cc428b8af48ff8",
+            )
+            .unwrap(),
+            origin: Some((
+                bip32::Fingerprint::from(&[0x78, 0x41, 0x2e, 0x3a][..]),
+                (&[
+                    bip32::ChildNumber::from_hardened_idx(0).unwrap(),
+                    bip32::ChildNumber::from_normal_idx(42).unwrap(),
+                    bip32::ChildNumber::from_hardened_idx(0).unwrap(),
+                ][..])
+                    .into(),
+            )),
+        });
+        assert_eq!(expected, desc.parse().expect("Parsing desc"));
+        assert_eq!(format!("{}", expected), desc);
+    }
+
+    #[test]
+    fn parse_descriptor_key_errors() {
+        // We refuse creating descriptors which claim to be able to derive hardened childs
+        let desc = "[78412e3a/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/42'/*";
+        assert_eq!(
+            DescriptorPublicKey::from_str(desc),
+            Err(DescriptorKeyParseError(
+                "Hardened derivation is currently not supported."
+            ))
+        );
+
+        // And ones with misplaced wildcard
+        let desc = "[78412e3a/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*/44";
+        assert_eq!(
+            DescriptorPublicKey::from_str(desc),
+            Err(DescriptorKeyParseError(
+                "\'*\' may only appear as last element in a derivation path."
+            ))
+        );
+
+        // And ones with invalid fingerprints
+        let desc = "[NonHexor]xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*";
+        assert_eq!(
+            DescriptorPublicKey::from_str(desc),
+            Err(DescriptorKeyParseError(
+                "Malformed master fingerprint, expected 8 hex chars"
+            ))
+        );
+
+        // And ones with invalid xpubs..
+        let desc = "[78412e3a]xpub1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaLcgJvLJuZZvRcEL/1/*";
+        assert_eq!(
+            DescriptorPublicKey::from_str(desc),
+            Err(DescriptorKeyParseError("Error while parsing xpub."))
+        );
+
+        // ..or invalid raw keys
+        let desc = "[78412e3a]0208a117f3897c3a13c9384b8695eed98dc31bc2500feb19a1af424cd47a5d83/1/*";
+        assert_eq!(
+            DescriptorPublicKey::from_str(desc),
+            Err(DescriptorKeyParseError(
+                "Error while parsing simple public key"
+            ))
+        );
     }
 
     #[test]
@@ -1376,16 +1493,16 @@ mod tests {
 pk([d34db33f/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*),\
 pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1),\
 pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))";
-        let policy: crate::policy::concrete::Policy<DescriptorKey> =
-            descriptor_str.parse().unwrap();
+        let policy: policy::concrete::Policy<DescriptorPublicKey> = descriptor_str.parse().unwrap();
         let descriptor = Descriptor::Sh(policy.compile().unwrap());
-        let derived_descriptor = descriptor.derive(&[ChildNumber::from_normal_idx(42).unwrap()]);
+        let derived_descriptor =
+            descriptor.derive(&[bip32::ChildNumber::from_normal_idx(42).unwrap()]);
 
         let res_descriptor_str = "thresh(2,\
 pk([d34db33f/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/42),\
 pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1),\
 pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))";
-        let res_policy: crate::policy::concrete::Policy<DescriptorKey> =
+        let res_policy: policy::concrete::Policy<DescriptorPublicKey> =
             res_descriptor_str.parse().unwrap();
         let res_descriptor = Descriptor::Sh(res_policy.compile().unwrap());

@sgeisler i took the liberty to put myself as co-author of d84350d, which is the commit i mainly reworked - the rest were mostly rebase conflicts and fixes for 1.22.

Closes #93.

[darosior]

Added a commit to fix the 1.22.0 build (and fixed the actual build under 1.22)

[sgeisler]

I noticed you based your PR on #93 instead of #116. I guess this makes reviewing/merging quicker, I just hope it won't be too annoying for @afilini.

[darosior]

I noticed you based your PR on #93 instead of #116. I guess this makes reviewing/merging quicker, I just hope it won't be too annoying for @afilini.

I took #116 into consideration as well, i actually implemented the second item in the list you all seem to have agreed upon. A rebased and cleaned up version of generalistic descriptor keys seems to also be a requirement for it ?

[afilini]

Makes sense, considering that I would have to rebase on master anyways, I might as well rebase on top of this PR.

Working on it now..

[sgeisler]

@darosior do you consider the PR ready for another round of review?

[darosior]

Yes -------- Original Message --------

…

On Sep 13, 2020, 23:36, Sebastian wrote: ***@***.***(https://github.com/darosior) do you consider the PR ready for another round of review? — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#131 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3FZBR2RFHOVLBSKHFXDSFVCQVANCNFSM4RH3ERHQ).

[sanket1729]

Hi @darosior, I agree with the general idea of keeping things minimal in this PR so that easier for review.

I reviewed commit by commit, so of the issues had been addressed in later commits and you can safely ignore those.

[darosior]

Thanks for the review, pushed the fixes.

[sanket1729]

tAck 51a2136. Can merge after feedback from other reviewers as this already contains the commit from #130

[darosior]

Now we only derive descriptors from an actual child number and not a path anymore.

[sgeisler]

I think we reaches a pretty good state now (even though it doesn't do all I wanted anymore).

[apoelstra]

With this fuzztesting code

fn do_test(data: &[u8]) {
    let data_str = String::from_utf8_lossy(data);
    if let Ok(dpk) = DescriptorPublicKey::from_str(&data_str) {
        let output = dpk.to_string();
        assert_eq!(data_str.to_lowercase(), output.to_lowercase());
    }
}

I can quickly get a crash by indexing into UTF-8 data which isn't ASCII.

Also, it would be good to re-export the DescriptorPublicKey type in lib.rs

[apoelstra]

re adding a public key to Miniscript, I think this solution is really invasive and will break all existing code using PkH.

Why not just make the MiniscriptKey::Hash type for DescriptorPublicKey be DescriptorPublicKey again?

[sgeisler]

That sounds like an awesome idea. The current version is a bit convoluted indeed, but I didn't see another option. That trick might work though.

[darosior]

That sounds like an awesome idea. The current version is a bit convoluted indeed, but I didn't see another option. That trick might work though.

It does work with a new placeholder type, is way shorter and nicer than my previous hack.

Added the fuzz target, fixed the parser management of unprintable characters and made MiniscriptKey::Hash a DescriptorPublicKey instead of the two commits adding the Pk to PkH internals.

[darosior]

For what it worth, still running on this branch downstream without issue :)

[apoelstra]

ack 28158a3