Implement code review suggestions

* Make push_size function
* Fix CI errors
* Remove `bitcoin::PublicKey` trait bounds
* Make `SortedMultiVec` methods `pub` and copy `Miniscript` doc strings

src/descriptor/mod.rs

@@ -39,7 +39,10 @@ use bitcoin::{self, Script};
 use expression;
 use miniscript;
 use miniscript::context::{ScriptContext, ScriptContextError};
-use miniscript::{Legacy, Miniscript, Segwitv0};
+use miniscript::{decode::Terminal, satisfy, Legacy, Miniscript, Segwitv0};
+use policy;
+use push_size;
+use script_num_size;
 use Error;
 use MiniscriptKey;
 use Satisfier;
@@ -48,8 +51,6 @@ use ToPublicKey;
 mod create_descriptor;
 mod satisfied_constraints;
 
-use crate::{expression::Tree, miniscript::satisfy, script_num_size, Terminal};
-
 pub use self::create_descriptor::from_txin_with_witness_stack;
 pub use self::satisfied_constraints::Error as InterpreterError;
 pub use self::satisfied_constraints::SatisfiedConstraint;
@@ -557,7 +558,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Descriptor<Pk> {
     /// Returns satisfying witness and scriptSig to spend an
     /// output controlled by the given descriptor if it possible to
     /// construct one using the satisfier.
-    pub fn get_satisfication<S: Satisfier<Pk> + Satisfier<bitcoin::PublicKey>>(
+    pub fn get_satisfication<S: Satisfier<Pk>>(
         &self,
         satisfier: S,
     ) -> Result<(Vec<Vec<u8>>, Script), Error> {
@@ -739,17 +740,8 @@ impl<Pk: MiniscriptKey + ToPublicKey> Descriptor<Pk> {
             Descriptor::ShWpkh(ref pk) => 4 * 24 + 1 + 73 + pk.serialized_len(),
             Descriptor::Sh(ref ms) => {
                 let ss = ms.script_size();
-                let push_size = if ss < 76 {
-                    1
-                } else if ss < 0x100 {
-                    2
-                } else if ss < 0x10000 {
-                    3
-                } else {
-                    5
-                };
-
-                let scriptsig_len = push_size + ss + ms.max_satisfaction_size(1);
+                let ps = push_size(ss);
+                let scriptsig_len = ps + ss + ms.max_satisfaction_size(1);
                 4 * (varint_len(scriptsig_len) + scriptsig_len)
             }
             Descriptor::Wsh(ref ms) => {
@@ -770,17 +762,8 @@ impl<Pk: MiniscriptKey + ToPublicKey> Descriptor<Pk> {
             }
             Descriptor::ShSortedMulti(ref smv) => {
                 let ss = smv.script_size();
-                let push_size = if ss < 76 {
-                    1
-                } else if ss < 0x100 {
-                    2
-                } else if ss < 0x10000 {
-                    3
-                } else {
-                    5
-                };
-
-                let scriptsig_len = push_size + ss + smv.max_satisfaction_size(1);
+                let ps = push_size(ss);
+                let scriptsig_len = ps + ss + smv.max_satisfaction_size(1);
                 4 * (varint_len(scriptsig_len) + scriptsig_len)
             }
             Descriptor::WshSortedMulti(ref smv) => {
@@ -1015,7 +998,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
         })
     }
     /// Parse an expression tree into a SortedMultiVec
-    fn from_tree(tree: &Tree) -> Result<Self, Error>
+    fn from_tree(tree: &expression::Tree) -> Result<Self, Error>
     where
         <Pk as FromStr>::Err: ToString,
     {
@@ -1029,7 +1012,10 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
 }
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
-    fn translate_pk<FPk, Q, FuncError>(
+    /// This will panic if translatefpk returns an uncompressed key when
+    /// converting to a Segwit descriptor. To prevent this panic, ensure
+    /// translatefpk returns an error in this case instead.
+    pub fn translate_pk<FPk, Q, FuncError>(
         &self,
         translatefpk: &mut FPk,
     ) -> Result<SortedMultiVec<Q, Ctx>, FuncError>
@@ -1048,31 +1034,42 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
 
 impl<Pk: MiniscriptKey + ToPublicKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     /// Create Terminal::Multi containing sorted pubkeys
-    pub fn sorted_node(&self) -> Terminal<bitcoin::PublicKey, Ctx> {
-        let mut pks: Vec<bitcoin::PublicKey> =
-            self.pks.iter().map(|pk| pk.to_public_key()).collect();
+    pub fn sorted_node(&self) -> Terminal<Pk, Ctx> {
+        let mut pks = self.pks.clone();
         // Sort pubkeys lexigraphically according to BIP 67
-        pks.sort_by(|a, b| a.to_string().partial_cmp(&b.to_string()).unwrap());
+        pks.sort_by(|a, b| {
+            a.to_public_key()
+                .to_string()
+                .partial_cmp(&b.to_public_key().to_string())
+                .unwrap()
+        });
         Terminal::Multi(self.k, pks)
     }
 
-    fn encode(&self) -> script::Script {
+    /// Encode as a Bitcoin script
+    pub fn encode(&self) -> script::Script {
         self.sorted_node()
             .encode(script::Builder::new())
             .into_script()
     }
 
-    // FIXME: is this trait bound right?
     /// Attempt to produce a satisfying witness for the
     /// witness script represented by the parse tree
-    fn satisfy<S: Satisfier<bitcoin::PublicKey>>(&self, satisfier: S) -> Option<Vec<Vec<u8>>> {
+    pub fn satisfy<S: Satisfier<Pk>>(&self, satisfier: S) -> Option<Vec<Vec<u8>>> {
         match satisfy::Satisfaction::satisfy(&self.sorted_node(), &satisfier).stack {
             satisfy::Witness::Stack(stack) => Some(stack),
             satisfy::Witness::Unavailable => None,
         }
     }
 
-    fn script_size(&self) -> usize {
+    /// Size, in bytes of the script-pubkey. If this Miniscript is used outside
+    /// of segwit (e.g. in a bare or P2SH descriptor), this quantity should be
+    /// multiplied by 4 to compute the weight.
+    ///
+    /// In general, it is not recommended to use this function directly, but
+    /// to instead call the corresponding function on a `Descriptor`, which
+    /// will handle the segwit/non-segwit technicalities for you.
+    pub fn script_size(&self) -> usize {
         script_num_size(self.k)
             + 1
             + script_num_size(self.pks.len())
@@ -1083,25 +1080,47 @@ impl<Pk: MiniscriptKey + ToPublicKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx
                 .sum::<usize>()
     }
 
-    fn max_satisfaction_witness_elements(&self) -> usize {
+    /// Maximum number of witness elements used to satisfy the Miniscript
+    /// fragment, including the witness script itself. Used to estimate
+    /// the weight of the `VarInt` that specifies this number in a serialized
+    /// transaction.
+    ///
+    /// This function may panic on misformed `Miniscript` objects which do
+    /// not correspond to semantically sane Scripts. (Such scripts should be
+    /// rejected at parse time. Any exceptions are bugs.)
+    pub fn max_satisfaction_witness_elements(&self) -> usize {
         2 + self.k
     }
 
-    fn max_satisfaction_size(&self, _: usize) -> usize {
+    /// Maximum size, in bytes, of a satisfying witness. For Segwit outputs
+    /// `one_cost` should be set to 2, since the number `1` requires two
+    /// bytes to encode. For non-segwit outputs `one_cost` should be set to
+    /// 1, since `OP_1` is available in scriptSigs.
+    ///
+    /// In general, it is not recommended to use this function directly, but
+    /// to instead call the corresponding function on a `Descriptor`, which
+    /// will handle the segwit/non-segwit technicalities for you.
+    ///
+    /// All signatures are assumed to be 73 bytes in size, including the
+    /// length prefix (segwit) or push opcode (pre-segwit) and sighash
+    /// postfix.
+    ///
+    /// This function may panic on misformed `Miniscript` objects which do not
+    /// correspond to semantically sane Scripts. (Such scripts should be
+    /// rejected at parse time. Any exceptions are bugs.)
+    pub fn max_satisfaction_size(&self, _: usize) -> usize {
         1 + 73 * self.k
     }
 }
 
-impl<Pk: MiniscriptKey, Ctx: ScriptContext> crate::policy::Liftable<Pk>
-    for SortedMultiVec<Pk, Ctx>
-{
-    fn lift(&self) -> Result<crate::policy::semantic::Policy<Pk>, Error> {
-        let ret = crate::policy::semantic::Policy::Threshold(
+impl<Pk: MiniscriptKey, Ctx: ScriptContext> policy::Liftable<Pk> for SortedMultiVec<Pk, Ctx> {
+    fn lift(&self) -> Result<policy::semantic::Policy<Pk>, Error> {
+        let ret = policy::semantic::Policy::Threshold(
             self.k,
             self.pks
                 .clone()
                 .into_iter()
-                .map(|k| crate::policy::semantic::Policy::KeyHash(k.to_pubkeyhash()))
+                .map(|k| policy::semantic::Policy::KeyHash(k.to_pubkeyhash()))
                 .collect(),
         );
         Ok(ret)

src/descriptor/satisfied_constraints.rs

@@ -12,8 +12,11 @@
 // If not, see <http://creativecommons.org/publicdomain/zero/1.0/>.
 //
 
-use bitcoin::hashes::{hash160, ripemd160, sha256, sha256d, Hash};
 use bitcoin::{self, secp256k1};
+use bitcoin::{
+    hashes::{hash160, ripemd160, sha256, sha256d, Hash},
+    PublicKey,
+};
 use fmt;
 use miniscript::context::Any;
 use miniscript::ScriptContext;
@@ -342,7 +345,7 @@ where
                     verify_sig: verify_sig,
                     public_key: None,
                     state: vec![NodeEvaluationState {
-                        node: &Any::from_smv_segwitv0(smv),
+                        node: &Any::from_smv_segwitv0::<PublicKey>(smv),
                         n_evaluated: 0,
                         n_satisfied: 0,
                     }],

src/lib.rs

@@ -506,6 +506,19 @@ pub fn script_num_size(n: usize) -> usize {
     }
 }
 
+/// FIXME
+pub fn push_size(script_size: usize) -> usize {
+    if script_size < 76 {
+        1
+    } else if script_size < 0x100 {
+        2
+    } else if script_size < 0x10000 {
+        3
+    } else {
+        5
+    }
+}
+
 /// Helper function used by tests
 #[cfg(test)]
 fn hex_script(s: &str) -> bitcoin::Script {

src/miniscript/context.rs

@@ -17,8 +17,8 @@ use miniscript::types::extra_props::{
 };
 use std::fmt;
 
-use crate::descriptor::SortedMultiVec;
-use {Miniscript, MiniscriptKey, Terminal};
+use bitcoin;
+use {descriptor::SortedMultiVec, Miniscript, MiniscriptKey, Terminal, ToPublicKey};
 
 /// Error for Script Context
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
@@ -224,23 +224,21 @@ impl Any {
         }
     }
 
-    pub(crate) fn from_smv_legacy(
-        smv: &SortedMultiVec<bitcoin::PublicKey, Legacy>,
-    ) -> &Miniscript<bitcoin::PublicKey, Any> {
+    pub(crate) fn from_smv_legacy<Pk: MiniscriptKey + ToPublicKey>(
+        smv: &SortedMultiVec<Pk, Legacy>,
+    ) -> &Miniscript<Pk, Any> {
         let ms = &Miniscript::from_ast(smv.sorted_node()).unwrap();
         // The fields Miniscript<Pk, Legacy> and Miniscript<Any, Legacy> only
         // differ in PhantomData. This unsafe assumes that the unlerlying byte
         // representation of the both should be the same. There is a Miri test
         // checking the same.
         unsafe {
             use std::mem::transmute;
-            transmute::<&Miniscript<bitcoin::PublicKey, Legacy>, &Miniscript<bitcoin::PublicKey, Any>>(
-                ms,
-            )
+            transmute::<&Miniscript<Pk, Legacy>, &Miniscript<Pk, Any>>(ms)
         }
     }
 
-    pub(crate) fn from_smv_segwitv0(
+    pub(crate) fn from_smv_segwitv0<Pk: MiniscriptKey + ToPublicKey>(
         smv: &SortedMultiVec<bitcoin::PublicKey, Segwitv0>,
     ) -> &Miniscript<bitcoin::PublicKey, Any> {
         let ms = &Miniscript::from_ast(smv.sorted_node()).unwrap();