Merge #849: several improvements and bugfixes to uncompressed key handling and sortedmulti

b5db340 sortedmulti: take Thresh as input to constructors (Andrew Poelstra)
276cf94 sortedmulti: simplify constructor and improve unit test (Andrew Poelstra)
3250e40 miniscript: take pubkey size into account in multi() type extdata (Andrew Poelstra)
8874640 miniscript: fix ExtData to correctly account for uncompressed keys (Andrew Poelstra)
040c29d miniscript: remove MalleablePkH script context rule (Andrew Poelstra)
a5c477b clippy: fix all nightly lints (Andrew Poelstra)

Pull request description:

  Removes the weird and incorrect `MalleablePkH` script context rule; fixes several bugs related to key size estimation in pkh and multisig fragments; improves tests of sortedmulti; uses the `Thresh` type in sortedmulti constructors.
  
  These bugs are obscure and only visible when using uncompressed pubkeys and I don't think they're worth backporting.
  
  This PR is starting to make "real" changes toward unifying our validation parameters. In particular we separate out validation errors in the `sortedmulti` constructors from threshold-construction errors (which the user is forced to deal with before calling a `sortedmulti` constructor). The PR is bigger than you might think because when testing this separation I found some bugs.
  
  This PR also includes a clippy fix which should unstick #805 (update nightly version) which has been stalled for months because the weekly retries don't trigger Github notifications and I didn't notice it.


ACKs for top commit:
  sanket1729:
    utACK b5db340


Tree-SHA512: 4c221f4efe1014001d4bbb99fd89728f5b56348c09d34df9163ff01766cd11e5d13834ecb3bc7ceb49ae89a99d6149587d83b0d901f2b73c64cc91c9a47684fc

Author: apoelstra

src/descriptor/key.rs

@@ -1203,9 +1203,7 @@ impl<K: InnerXKey> DescriptorXKey<K> {
         };
 
         if &compare_fingerprint == fingerprint
-            && compare_path
-                .into_iter()
-                .eq(path_excluding_wildcard.into_iter())
+            && compare_path.into_iter().eq(&path_excluding_wildcard)
         {
             Some(path_excluding_wildcard)
         } else {

src/descriptor/mod.rs

@@ -23,12 +23,13 @@ use sync::Arc;
 
 use crate::expression::FromTree as _;
 use crate::miniscript::decode::Terminal;
+use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
 use crate::miniscript::{satisfy, Legacy, Miniscript, Segwitv0};
 use crate::plan::{AssetProvider, Plan};
 use crate::prelude::*;
 use crate::{
     expression, hash256, BareCtx, Error, ForEachKey, FromStrKey, MiniscriptKey, ParseError,
-    Satisfier, ToPublicKey, TranslateErr, Translator,
+    Satisfier, Threshold, ToPublicKey, TranslateErr, Translator,
 };
 
 mod bare;
@@ -217,21 +218,27 @@ impl<Pk: MiniscriptKey> Descriptor<Pk> {
     /// Create a new sh sortedmulti descriptor with threshold `k`
     /// and Vec of `pks`.
     /// Errors when miniscript exceeds resource limits under p2sh context
-    pub fn new_sh_sortedmulti(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
-        Ok(Descriptor::Sh(Sh::new_sortedmulti(k, pks)?))
+    pub fn new_sh_sortedmulti(
+        thresh: Threshold<Pk, MAX_PUBKEYS_PER_MULTISIG>,
+    ) -> Result<Self, Error> {
+        Ok(Descriptor::Sh(Sh::new_sortedmulti(thresh)?))
     }
 
     /// Create a new sh wrapped wsh sortedmulti descriptor from threshold
     /// `k` and Vec of `pks`
     /// Errors when miniscript exceeds resource limits under segwit context
-    pub fn new_sh_wsh_sortedmulti(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
-        Ok(Descriptor::Sh(Sh::new_wsh_sortedmulti(k, pks)?))
+    pub fn new_sh_wsh_sortedmulti(
+        thresh: Threshold<Pk, MAX_PUBKEYS_PER_MULTISIG>,
+    ) -> Result<Self, Error> {
+        Ok(Descriptor::Sh(Sh::new_wsh_sortedmulti(thresh)?))
     }
 
     /// Create a new wsh sorted multi descriptor
     /// Errors when miniscript exceeds resource limits under p2sh context
-    pub fn new_wsh_sortedmulti(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
-        Ok(Descriptor::Wsh(Wsh::new_sortedmulti(k, pks)?))
+    pub fn new_wsh_sortedmulti(
+        thresh: Threshold<Pk, MAX_PUBKEYS_PER_MULTISIG>,
+    ) -> Result<Self, Error> {
+        Ok(Descriptor::Wsh(Wsh::new_sortedmulti(thresh)?))
     }
 
     /// Create new tr descriptor
@@ -288,7 +295,7 @@ impl<Pk: MiniscriptKey> Descriptor<Pk> {
     ///
     /// If the descriptor is not a Taproot descriptor, **or** if the descriptor is a
     /// Taproot descriptor containing only a keyspend, returns an empty iterator.
-    pub fn tap_tree_iter(&self) -> tr::TapTreeIter<Pk> {
+    pub fn tap_tree_iter(&self) -> tr::TapTreeIter<'_, Pk> {
         if let Descriptor::Tr(ref tr) = self {
             if let Some(tree) = tr.tap_tree() {
                 return tree.leaves();

src/descriptor/segwitv0.rs

@@ -14,14 +14,15 @@ use super::SortedMultiVec;
 use crate::descriptor::{write_descriptor, DefiniteDescriptorKey};
 use crate::expression::{self, FromTree};
 use crate::miniscript::context::{ScriptContext, ScriptContextError};
+use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
 use crate::miniscript::satisfy::{Placeholder, Satisfaction, Witness};
 use crate::plan::AssetProvider;
 use crate::policy::{semantic, Liftable};
 use crate::prelude::*;
 use crate::util::varint_len;
 use crate::{
-    Error, ForEachKey, FromStrKey, Miniscript, MiniscriptKey, Satisfier, Segwitv0, ToPublicKey,
-    TranslateErr, Translator,
+    Error, ForEachKey, FromStrKey, Miniscript, MiniscriptKey, Satisfier, Segwitv0, Threshold,
+    ToPublicKey, TranslateErr, Translator,
 };
 /// A Segwitv0 wsh descriptor
 #[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
@@ -45,10 +46,10 @@ impl<Pk: MiniscriptKey> Wsh<Pk> {
     }
 
     /// Create a new sortedmulti wsh descriptor
-    pub fn new_sortedmulti(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
+    pub fn new_sortedmulti(thresh: Threshold<Pk, MAX_PUBKEYS_PER_MULTISIG>) -> Result<Self, Error> {
         // The context checks will be carried out inside new function for
         // sortedMultiVec
-        Ok(Self { inner: WshInner::SortedMulti(SortedMultiVec::new(k, pks)?) })
+        Ok(Self { inner: WshInner::SortedMulti(SortedMultiVec::new(thresh)?) })
     }
 
     /// Get the descriptor without the checksum

src/descriptor/sh.rs

@@ -17,14 +17,15 @@ use super::{SortedMultiVec, Wpkh, Wsh};
 use crate::descriptor::{write_descriptor, DefiniteDescriptorKey};
 use crate::expression::{self, FromTree};
 use crate::miniscript::context::ScriptContext;
+use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
 use crate::miniscript::satisfy::{Placeholder, Satisfaction};
 use crate::plan::AssetProvider;
 use crate::policy::{semantic, Liftable};
 use crate::prelude::*;
 use crate::util::{varint_len, witness_to_scriptsig};
 use crate::{
     push_opcode_size, Error, ForEachKey, FromStrKey, Legacy, Miniscript, MiniscriptKey, Satisfier,
-    Segwitv0, ToPublicKey, TranslateErr, Translator,
+    Segwitv0, Threshold, ToPublicKey, TranslateErr, Translator,
 };
 
 /// A Legacy p2sh Descriptor
@@ -125,10 +126,10 @@ impl<Pk: MiniscriptKey> Sh<Pk> {
 
     /// Create a new p2sh sortedmulti descriptor with threshold `k`
     /// and Vec of `pks`.
-    pub fn new_sortedmulti(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
+    pub fn new_sortedmulti(thresh: Threshold<Pk, MAX_PUBKEYS_PER_MULTISIG>) -> Result<Self, Error> {
         // The context checks will be carried out inside new function for
         // sortedMultiVec
-        Ok(Self { inner: ShInner::SortedMulti(SortedMultiVec::new(k, pks)?) })
+        Ok(Self { inner: ShInner::SortedMulti(SortedMultiVec::new(thresh)?) })
     }
 
     /// Create a new p2sh wrapped wsh descriptor with the raw miniscript
@@ -152,10 +153,12 @@ impl<Pk: MiniscriptKey> Sh<Pk> {
 
     /// Create a new p2sh wrapped wsh sortedmulti descriptor from threshold
     /// `k` and Vec of `pks`
-    pub fn new_wsh_sortedmulti(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
+    pub fn new_wsh_sortedmulti(
+        thresh: Threshold<Pk, MAX_PUBKEYS_PER_MULTISIG>,
+    ) -> Result<Self, Error> {
         // The context checks will be carried out inside new function for
         // sortedMultiVec
-        Ok(Self { inner: ShInner::Wsh(Wsh::new_sortedmulti(k, pks)?) })
+        Ok(Self { inner: ShInner::Wsh(Wsh::new_sortedmulti(thresh)?) })
     }
 
     /// Create a new p2sh wrapped wpkh from `Pk`

src/descriptor/sortedmulti.rs

@@ -33,13 +33,10 @@ pub struct SortedMultiVec<Pk: MiniscriptKey, Ctx: ScriptContext> {
 
 impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     fn constructor_check(mut self) -> Result<Self, Error> {
+        let ms = Miniscript::<Pk, Ctx>::multi(self.inner);
         // Check the limits before creating a new SortedMultiVec
         // For example, under p2sh context the scriptlen can only be
         // upto 520 bytes.
-        let term: Terminal<Pk, Ctx> = Terminal::Multi(self.inner);
-        let ms = Miniscript::from_ast(term)?;
-        // This would check all the consensus rules for p2sh/p2wsh and
-        // even tapscript in future
         Ctx::check_local_validity(&ms)?;
         if let Terminal::Multi(inner) = ms.node {
             self.inner = inner;
@@ -52,9 +49,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
     /// Create a new instance of `SortedMultiVec` given a list of keys and the threshold
     ///
     /// Internally checks all the applicable size limits and pubkey types limitations according to the current `Ctx`.
-    pub fn new(k: usize, pks: Vec<Pk>) -> Result<Self, Error> {
-        let ret =
-            Self { inner: Threshold::new(k, pks).map_err(Error::Threshold)?, phantom: PhantomData };
+    pub fn new(thresh: Threshold<Pk, MAX_PUBKEYS_PER_MULTISIG>) -> Result<Self, Error> {
+        let ret = Self { inner: thresh, phantom: PhantomData };
         ret.constructor_check()
     }
 
@@ -229,31 +225,27 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> fmt::Display for SortedMultiVec<Pk,
 mod tests {
     use core::str::FromStr as _;
 
-    use bitcoin::secp256k1::PublicKey;
+    use bitcoin::PublicKey;
 
     use super::*;
-    use crate::miniscript::context::Legacy;
+    use crate::miniscript::context::{Legacy, ScriptContextError};
 
     #[test]
-    fn too_many_pubkeys() {
-        // Arbitrary pubic key.
+    fn too_many_pubkeys_for_p2sh() {
+        // Arbitrary 65-byte public key (66 with length prefix).
         let pk = PublicKey::from_str(
-            "02e6642fd69bd211f93f7f1f36ca51a26a5290eb2dd1b0d8279a87bb0d480c8443",
+            "0400232a2acfc9b43fa89f1b4f608fde335d330d7114f70ea42bfb4a41db368a3e3be6934a4097dd25728438ef73debb1f2ffdb07fec0f18049df13bdc5285dc5b",
         )
         .unwrap();
 
-        let over = 1 + MAX_PUBKEYS_PER_MULTISIG;
-
-        let mut pks = Vec::new();
-        for _ in 0..over {
-            pks.push(pk);
-        }
-
-        let res: Result<SortedMultiVec<PublicKey, Legacy>, Error> = SortedMultiVec::new(0, pks);
+        // This is legal for CHECKMULTISIG, but the 8 keys consume the whole 520 bytes
+        // allowed by P2SH, meaning that the full script goes over the limit.
+        let thresh = Threshold::new(2, vec![pk; 8]).expect("the thresh is ok..");
+        let res: Result<SortedMultiVec<PublicKey, Legacy>, Error> = SortedMultiVec::new(thresh);
         let error = res.expect_err("constructor should err");
 
         match error {
-            Error::Threshold(_) => {} // ok
+            Error::ContextError(ScriptContextError::MaxRedeemScriptSizeExceeded { .. }) => {} // ok
             other => panic!("unexpected error: {:?}", other),
         }
     }

src/descriptor/tr/mod.rs

@@ -108,7 +108,7 @@ impl<Pk: MiniscriptKey> Tr<Pk> {
     ///
     /// The yielded elements include the Miniscript for each leave as well as its depth
     /// in the tree, which is the data required by PSBT (BIP 371).
-    pub fn leaves(&self) -> TapTreeIter<Pk> {
+    pub fn leaves(&self) -> TapTreeIter<'_, Pk> {
         match self.tree {
             Some(ref t) => t.leaves(),
             None => TapTreeIter::empty(),

src/descriptor/tr/spend_info.rs

@@ -181,7 +181,7 @@ impl<Pk: ToPublicKey> TrSpendInfo<Pk> {
     /// This yields the same leaves in the same order as [`super::Tr::leaves`] on the original
     /// [`super::Tr`]. However, in addition to yielding the leaves and their depths, it also
     /// yields their scripts, leafhashes, and control blocks.
-    pub fn leaves(&self) -> TrSpendInfoIter<Pk> {
+    pub fn leaves(&self) -> TrSpendInfoIter<'_, Pk> {
         TrSpendInfoIter {
             spend_info: self,
             index: 0,

src/descriptor/tr/taptree.rs

@@ -53,7 +53,7 @@ impl<Pk: MiniscriptKey> TapTree<Pk> {
     ///
     /// The yielded elements include the Miniscript for each leave as well as its depth
     /// in the tree, which is the data required by PSBT (BIP 371).
-    pub fn leaves(&self) -> TapTreeIter<Pk> { TapTreeIter::from_tree(self) }
+    pub fn leaves(&self) -> TapTreeIter<'_, Pk> { TapTreeIter::from_tree(self) }
 
     /// Converts keys from one type of public key to another.
     pub fn translate_pk<T>(

src/miniscript/context.rs

@@ -21,10 +21,6 @@ use crate::{hash256, Error, ForEachKey, Miniscript, MiniscriptKey, Terminal};
 /// Error for Script Context
 #[derive(Clone, PartialEq, Eq, Debug)]
 pub enum ScriptContextError {
-    /// Script Context does not permit PkH for non-malleability
-    /// It is not possible to estimate the pubkey size at the creation
-    /// time because of uncompressed pubkeys
-    MalleablePkH,
     /// Script Context does not permit OrI for non-malleability
     /// Legacy fragments allow non-minimal IF which results in malleability
     MalleableOrI,
@@ -74,8 +70,7 @@ impl error::Error for ScriptContextError {
         use self::ScriptContextError::*;
 
         match self {
-            MalleablePkH
-            | MalleableOrI
+            MalleableOrI
             | MalleableDupIf
             | CompressedOnly(_)
             | XOnlyKeysNotAllowed(_, _)
@@ -97,7 +92,6 @@ impl error::Error for ScriptContextError {
 impl fmt::Display for ScriptContextError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match *self {
-            ScriptContextError::MalleablePkH => write!(f, "PkH is malleable under Legacy rules"),
             ScriptContextError::MalleableOrI => write!(f, "OrI is malleable under Legacy rules"),
             ScriptContextError::MalleableDupIf => {
                 write!(f, "DupIf is malleable under Legacy rules")
@@ -380,8 +374,6 @@ impl ScriptContext for Legacy {
         frag: &Terminal<Pk, Self>,
     ) -> Result<(), ScriptContextError> {
         match *frag {
-            Terminal::PkH(ref _pkh) => Err(ScriptContextError::MalleablePkH),
-            Terminal::RawPkH(ref _pk) => Err(ScriptContextError::MalleablePkH),
             Terminal::OrI(ref _a, ref _b) => Err(ScriptContextError::MalleableOrI),
             Terminal::DupIf(ref _ms) => Err(ScriptContextError::MalleableDupIf),
             _ => Ok(()),

src/miniscript/iter.rs

@@ -18,12 +18,12 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
     /// Creates a new [Iter] iterator that will iterate over all [Miniscript] items within
     /// AST by traversing its branches. For the specific algorithm please see
     /// [Iter::next] function.
-    pub fn iter(&self) -> Iter<Pk, Ctx> { Iter::new(self) }
+    pub fn iter(&self) -> Iter<'_, Pk, Ctx> { Iter::new(self) }
 
     /// Creates a new [PkIter] iterator that will iterate over all plain public keys (and not
     /// key hash values) present in [Miniscript] items within AST by traversing all its branches.
     /// For the specific algorithm please see [PkIter::next] function.
-    pub fn iter_pk(&self) -> PkIter<Pk, Ctx> { PkIter::new(self) }
+    pub fn iter_pk(&self) -> PkIter<'_, Pk, Ctx> { PkIter::new(self) }
 
     /// Enumerates all child nodes of the current AST node (`self`) and returns a `Vec` referencing
     /// them.