refactor: remove SortedMultiVec and use Terminal::SortedMulti

- Utilize the Miniscript parsing to handle sortedmulti as a Terminal.
- Deleted sortedmulti.rs (SortedMultiVec)
- Refactor Wsh to only wrap a Miniscript now that SortedMultiVec isn't used.
- Refactor ShInner to remove SortedMulti variant and only use the Ms variant for
  sortedmulti scripts

Author: trevarj

bitcoind-tests/tests/test_cpp.rs

@@ -147,10 +147,7 @@ pub fn test_from_cpp_ms(cl: &Client, testdata: &TestData) {
     for i in 0..psbts.len() {
         let wsh_derived = desc_vec[i].derived_descriptor(&secp);
         let ms = if let Descriptor::Wsh(wsh) = &wsh_derived {
-            match wsh.as_inner() {
-                miniscript::descriptor::WshInner::Ms(ms) => ms,
-                _ => unreachable!(),
-            }
+            wsh.as_inner()
         } else {
             unreachable!("Only Wsh descriptors are supported");
         };

bitcoind-tests/tests/test_desc.rs

@@ -225,29 +225,15 @@ pub fn test_desc_satisfy(
                 Descriptor::Pkh(pk) => find_sk_single_key(*pk.as_inner(), testdata),
                 Descriptor::Wpkh(pk) => find_sk_single_key(*pk.as_inner(), testdata),
                 Descriptor::Sh(sh) => match sh.as_inner() {
-                    miniscript::descriptor::ShInner::Wsh(wsh) => match wsh.as_inner() {
-                        miniscript::descriptor::WshInner::SortedMulti(ref smv) => {
-                            let ms = Miniscript::from_ast(smv.sorted_node()).unwrap();
-                            find_sks_ms(&ms, testdata)
-                        }
-                        miniscript::descriptor::WshInner::Ms(ref ms) => find_sks_ms(ms, testdata),
-                    },
+                    miniscript::descriptor::ShInner::Wsh(wsh) => {
+                        find_sks_ms(wsh.as_inner(), testdata)
+                    }
                     miniscript::descriptor::ShInner::Wpkh(pk) => {
                         find_sk_single_key(*pk.as_inner(), testdata)
                     }
-                    miniscript::descriptor::ShInner::SortedMulti(smv) => {
-                        let ms = Miniscript::from_ast(smv.sorted_node()).unwrap();
-                        find_sks_ms(&ms, testdata)
-                    }
                     miniscript::descriptor::ShInner::Ms(ms) => find_sks_ms(ms, testdata),
                 },
-                Descriptor::Wsh(wsh) => match wsh.as_inner() {
-                    miniscript::descriptor::WshInner::SortedMulti(ref smv) => {
-                        let ms = Miniscript::from_ast(smv.sorted_node()).unwrap();
-                        find_sks_ms(&ms, testdata)
-                    }
-                    miniscript::descriptor::WshInner::Ms(ref ms) => find_sks_ms(ms, testdata),
-                },
+                Descriptor::Wsh(wsh) => find_sks_ms(wsh.as_inner(), testdata),
                 Descriptor::Tr(_tr) => unreachable!("Tr checked earlier"),
             };
             let msg = psbt
@@ -319,8 +305,7 @@ fn find_sks_ms<Ctx: ScriptContext>(
 ) -> Vec<secp256k1::SecretKey> {
     let sks = &testdata.secretdata.sks;
     let pks = &testdata.pubdata.pks;
-    let sks = ms
-        .iter_pk()
+    let sks = ms.iter_pk()
         .filter_map(|pk| {
             let i = pks.iter().position(|&x| x.to_public_key() == pk);
             i.map(|idx| sks[idx])

src/descriptor/mod.rs

@@ -36,15 +36,13 @@ mod bare;
 mod iter;
 mod segwitv0;
 mod sh;
-mod sortedmulti;
 mod tr;
 
 // Descriptor Exports
 pub use self::bare::{Bare, Pkh};
 pub use self::iter::PkIter;
-pub use self::segwitv0::{Wpkh, Wsh, WshInner};
+pub use self::segwitv0::{Wpkh, Wsh};
 pub use self::sh::{Sh, ShInner};
-pub use self::sortedmulti::SortedMultiVec;
 pub use self::tr::{
     TapTree, TapTreeDepthError, TapTreeIter, TapTreeIterItem, Tr, TrSpendInfo, TrSpendInfoIter,
     TrSpendInfoIterItem,
@@ -256,18 +254,31 @@ impl<Pk: MiniscriptKey> Descriptor<Pk> {
             Descriptor::Pkh(ref pk) => PkIter::from_key(pk.as_inner().clone()),
             Descriptor::Wpkh(ref pk) => PkIter::from_key(pk.as_inner().clone()),
             Descriptor::Sh(ref sh) => match *sh.as_inner() {
-                ShInner::Wsh(ref wsh) => match wsh.as_inner() {
-                    WshInner::SortedMulti(ref sorted) => PkIter::from_sortedmulti(sorted.pks()),
-                    WshInner::Ms(ref ms) => PkIter::from_miniscript_segwit(ms),
-                },
+                ShInner::Wsh(ref wsh) => {
+                    let ms = wsh.as_inner();
+                    if let Terminal::SortedMulti(ref thresh) = ms.node {
+                        PkIter::from_sortedmulti(thresh.data())
+                    } else {
+                        PkIter::from_miniscript_segwit(ms)
+                    }
+                }
                 ShInner::Wpkh(ref pk) => PkIter::from_key(pk.as_inner().clone()),
-                ShInner::SortedMulti(ref sorted) => PkIter::from_sortedmulti(sorted.pks()),
-                ShInner::Ms(ref ms) => PkIter::from_miniscript_legacy(ms),
-            },
-            Descriptor::Wsh(ref wsh) => match wsh.as_inner() {
-                WshInner::SortedMulti(ref sorted) => PkIter::from_sortedmulti(sorted.pks()),
-                WshInner::Ms(ref ms) => PkIter::from_miniscript_segwit(ms),
+                ShInner::Ms(ref ms) => {
+                    if let Terminal::SortedMulti(ref thresh) = ms.node {
+                        PkIter::from_sortedmulti(thresh.data())
+                    } else {
+                        PkIter::from_miniscript_legacy(ms)
+                    }
+                }
             },
+            Descriptor::Wsh(ref wsh) => {
+                let ms = wsh.as_inner();
+                if let Terminal::SortedMulti(ref thresh) = ms.node {
+                    PkIter::from_sortedmulti(thresh.data())
+                } else {
+                    PkIter::from_miniscript_segwit(ms)
+                }
+            }
             Descriptor::Tr(ref tr) => PkIter::from_tr(tr),
         }
     }
@@ -313,18 +324,29 @@ impl<Pk: MiniscriptKey> Descriptor<Pk> {
             Descriptor::Pkh(ref _pkh) => DescriptorType::Pkh,
             Descriptor::Wpkh(ref _wpkh) => DescriptorType::Wpkh,
             Descriptor::Sh(ref sh) => match sh.as_inner() {
-                ShInner::Wsh(ref wsh) => match wsh.as_inner() {
-                    WshInner::SortedMulti(ref _smv) => DescriptorType::ShWshSortedMulti,
-                    WshInner::Ms(ref _ms) => DescriptorType::ShWsh,
-                },
+                ShInner::Wsh(ref wsh) => {
+                    if let Terminal::SortedMulti(..) = wsh.as_inner().node {
+                        DescriptorType::ShWshSortedMulti
+                    } else {
+                        DescriptorType::ShWsh
+                    }
+                }
                 ShInner::Wpkh(ref _wpkh) => DescriptorType::ShWpkh,
-                ShInner::SortedMulti(ref _smv) => DescriptorType::ShSortedMulti,
-                ShInner::Ms(ref _ms) => DescriptorType::Sh,
-            },
-            Descriptor::Wsh(ref wsh) => match wsh.as_inner() {
-                WshInner::SortedMulti(ref _smv) => DescriptorType::WshSortedMulti,
-                WshInner::Ms(ref _ms) => DescriptorType::Wsh,
+                ShInner::Ms(ref ms) => {
+                    if let Terminal::SortedMulti(..) = ms.node {
+                        DescriptorType::ShSortedMulti
+                    } else {
+                        DescriptorType::Sh
+                    }
+                }
             },
+            Descriptor::Wsh(ref wsh) => {
+                if let Terminal::SortedMulti(..) = wsh.as_inner().node {
+                    DescriptorType::WshSortedMulti
+                } else {
+                    DescriptorType::Wsh
+                }
+            }
             Descriptor::Tr(ref _tr) => DescriptorType::Tr,
         }
     }
@@ -1208,6 +1230,7 @@ mod tests {
 
     use super::{checksum, *};
     use crate::hex_script;
+    use crate::miniscript::context::ScriptContextError;
     #[cfg(feature = "compiler")]
     use crate::policy;
 
@@ -1257,7 +1280,7 @@ mod tests {
             StdDescriptor::from_str("sh(sortedmulti)")
                 .unwrap_err()
                 .to_string(),
-            "sortedmulti must have at least 1 children, but found 0"
+            "expected threshold, found terminal"
         ); //issue 202
         assert_eq!(
             StdDescriptor::from_str(&format!("sh(sortedmulti(2,{}))", &TEST_PK[3..69]))
@@ -2884,4 +2907,26 @@ pk(03f28773c2d975288bc7d1d205c3748651b075fbc6610e58cddeeddf8f19405aa8))";
         let definite: Result<Descriptor<DefiniteDescriptorKey>, _> = desc.try_into();
         assert!(matches!(definite, Err(NonDefiniteKeyError::Wildcard)));
     }
+
+    #[test]
+    fn too_many_pubkeys_for_p2sh() {
+        // Arbitrary 65-byte public key (66 with length prefix).
+        let pk = PublicKey::from_str(
+            "0400232a2acfc9b43fa89f1b4f608fde335d330d7114f70ea42bfb4a41db368a3e3be6934a4097dd25728438ef73debb1f2ffdb07fec0f18049df13bdc5285dc5b",
+        )
+        .unwrap();
+
+        // 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 script = Miniscript::<_, Legacy>::sortedmulti(thresh).encode();
+        let res = Miniscript::<_, Legacy>::decode(&script);
+
+        let error = res.expect_err("decoding should err");
+
+        match error {
+            Error::ContextError(ScriptContextError::MaxRedeemScriptSizeExceeded { .. }) => {} // ok
+            other => panic!("unexpected error: {:?}", other),
+        }
+    }
 }