Merge #657: compiler: improve logic when deciding between conjunctions and multi/multi_a

apoelstra · apoelstra · commit f6ffc3ee8a65 · 2024-03-12T23:23:00.000Z
200991d compiler: improve logic for deciding between thresholds and ands (Andrew Poelstra) bc3b8dc policy: rename Threshold variant to Thresh (Andrew Poelstra) 78db616 consolidate some FIXMEs (Andrew Poelstra) 0666aef error: remove some unused variants (Andrew Poelstra) b2ec4a8 clippy: fix some nits introduced by #651 (Andrew Poelstra) Pull request description: The compiler logic when encountering thresholds of pks is currently a bit confused, and therefore chooses multi/multi_a in cases where this is not the most efficient compilation. This PR fixes that, and also brings in a few other cleanup commits that I had laying around. This does **not** fix #656, which I didn't know how to approach. ACKs for top commit: sanket1729: ACK 200991d Tree-SHA512: 252a60891cf1c1d1cd3ded88d97122fd1e76bd25807770f4843ae68bd2d854fc617518f26be86dcb57cd7fc369e1a4be81daa42ee1a6d4bc976dbad6dc1150f6
diff --git a/examples/taproot.rs b/examples/taproot.rs
@@ -45,7 +45,7 @@ fn main() {
     let desc = pol.compile_tr(Some("UNSPENDABLE_KEY".to_string())).unwrap();
 
     let expected_desc =
-        Descriptor::<String>::from_str("tr(Ca,{and_v(v:pk(In),older(9)),multi_a(2,hA,S)})")
+        Descriptor::<String>::from_str("tr(Ca,{and_v(v:pk(In),older(9)),and_v(v:pk(hA),pk(S))})")
             .unwrap();
     assert_eq!(desc, expected_desc);
 
@@ -73,7 +73,7 @@ fn main() {
         );
         assert_eq!(
             iter.next().unwrap(),
-            (1u8, &Miniscript::<String, Tap>::from_str("multi_a(2,hA,S)").unwrap())
+            (1u8, &Miniscript::<String, Tap>::from_str("and_v(v:pk(hA),pk(S))").unwrap())
         );
         assert_eq!(iter.next(), None);
     }
@@ -97,19 +97,19 @@ fn main() {
     let real_desc = desc.translate_pk(&mut t).unwrap();
 
     // Max satisfaction weight for compilation, corresponding to the script-path spend
-    // `multi_a(2,PUBKEY_1,PUBKEY_2) at tap tree depth 1, having:
+    // `and_v(PUBKEY_1,PUBKEY_2) at tap tree depth 1, having:
     //
     //     max_witness_size = varint(control_block_size) + control_block size +
     //                        varint(script_size) + script_size + max_satisfaction_size
-    //                      = 1 + 65 + 1 + 70 + 132 = 269
+    //                      = 1 + 65 + 1 + 68 + 132 = 269
     let max_sat_wt = real_desc.max_weight_to_satisfy().unwrap();
-    assert_eq!(max_sat_wt, 269);
+    assert_eq!(max_sat_wt, 267);
 
     // Compute the bitcoin address and check if it matches
     let network = Network::Bitcoin;
     let addr = real_desc.address(network).unwrap();
     let expected_addr = bitcoin::Address::from_str(
-        "bc1pcc8ku64slu3wu04a6g376d2s8ck9y5alw5sus4zddvn8xgpdqw2swrghwx",
+        "bc1p4l2xzq7js40965s5w0fknd287kdlmt2dljte37zsc5a34u0h9c4q85snyd",
     )
     .unwrap()
     .assume_checked();
diff --git a/src/descriptor/sortedmulti.rs b/src/descriptor/sortedmulti.rs
@@ -198,7 +198,7 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> SortedMultiVec<Pk, Ctx> {
 
 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(
+        let ret = policy::semantic::Policy::Thresh(
             self.k,
             self.pks
                 .iter()
diff --git a/src/descriptor/tr.rs b/src/descriptor/tr.rs
@@ -616,7 +616,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for TapTree<Pk> {
     fn lift(&self) -> Result<Policy<Pk>, Error> {
         fn lift_helper<Pk: MiniscriptKey>(s: &TapTree<Pk>) -> Result<Policy<Pk>, Error> {
             match *s {
-                TapTree::Tree { ref left, ref right, height: _ } => Ok(Policy::Threshold(
+                TapTree::Tree { ref left, ref right, height: _ } => Ok(Policy::Thresh(
                     1,
                     vec![Arc::new(lift_helper(left)?), Arc::new(lift_helper(right)?)],
                 )),
@@ -632,7 +632,7 @@ impl<Pk: MiniscriptKey> Liftable<Pk> for TapTree<Pk> {
 impl<Pk: MiniscriptKey> Liftable<Pk> for Tr<Pk> {
     fn lift(&self) -> Result<Policy<Pk>, Error> {
         match &self.tree {
-            Some(root) => Ok(Policy::Threshold(
+            Some(root) => Ok(Policy::Thresh(
                 1,
                 vec![
                     Arc::new(Policy::Key(self.internal_key.clone())),
diff --git a/src/lib.rs b/src/lib.rs
@@ -443,8 +443,6 @@ pub enum Error {
     Unexpected(String),
     /// Name of a fragment contained `:` multiple times
     MultiColon(String),
-    /// Name of a fragment contained `@` multiple times
-    MultiAt(String),
     /// Name of a fragment contained `@` but we were not parsing an OR
     AtOutsideOr(String),
     /// Encountered a wrapping character that we don't recognize
@@ -453,16 +451,8 @@ pub enum Error {
     NonTopLevel(String),
     /// Parsed a miniscript but there were more script opcodes after it
     Trailing(String),
-    /// Failed to parse a push as a public key
-    BadPubkey(bitcoin::key::Error),
-    /// Could not satisfy a script (fragment) because of a missing hash preimage
-    MissingHash(sha256::Hash),
     /// Could not satisfy a script (fragment) because of a missing signature
     MissingSig(bitcoin::PublicKey),
-    /// Could not satisfy, relative locktime not met
-    RelativeLocktimeNotMet(u32),
-    /// Could not satisfy, absolute locktime not met
-    AbsoluteLocktimeNotMet(u32),
     /// General failure to satisfy
     CouldNotSatisfy,
     /// Typechecking failed
@@ -482,8 +472,6 @@ pub enum Error {
     ContextError(miniscript::context::ScriptContextError),
     /// Recursion depth exceeded when parsing policy/miniscript from string
     MaxRecursiveDepthExceeded,
-    /// Script size too large
-    ScriptSizeTooLarge,
     /// Anything but c:pk(key) (P2PK), c:pk_h(key) (P2PKH), and thresh_m(k,...)
     /// up to n=3 is invalid by standardness (bare)
     NonStandardBareScript,
@@ -495,12 +483,8 @@ pub enum Error {
     BareDescriptorAddr,
     /// PubKey invalid under current context
     PubKeyCtxError(miniscript::decode::KeyParseError, &'static str),
-    /// Attempted to call function that requires PreComputed taproot info
-    TaprootSpendInfoUnavialable,
     /// No script code for Tr descriptors
     TrNoScriptCode,
-    /// No explicit script for Tr descriptors
-    TrNoExplicitScript,
     /// At least two BIP389 key expressions in the descriptor contain tuples of
     /// derivation indexes of different lengths.
     MultipathDescLenMismatch,
@@ -512,8 +496,6 @@ pub enum Error {
 
 // https://github.com/sipa/miniscript/pull/5 for discussion on this number
 const MAX_RECURSION_DEPTH: u32 = 402;
-// https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki
-const MAX_SCRIPT_SIZE: u32 = 10000;
 
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -531,23 +513,12 @@ impl fmt::Display for Error {
             Error::UnexpectedStart => f.write_str("unexpected start of script"),
             Error::Unexpected(ref s) => write!(f, "unexpected «{}»", s),
             Error::MultiColon(ref s) => write!(f, "«{}» has multiple instances of «:»", s),
-            Error::MultiAt(ref s) => write!(f, "«{}» has multiple instances of «@»", s),
             Error::AtOutsideOr(ref s) => write!(f, "«{}» contains «@» in non-or() context", s),
             Error::UnknownWrapper(ch) => write!(f, "unknown wrapper «{}:»", ch),
             Error::NonTopLevel(ref s) => write!(f, "non-T miniscript: {}", s),
             Error::Trailing(ref s) => write!(f, "trailing tokens: {}", s),
-            Error::MissingHash(ref h) => write!(f, "missing preimage of hash {}", h),
             Error::MissingSig(ref pk) => write!(f, "missing signature for key {:?}", pk),
-            Error::RelativeLocktimeNotMet(n) => {
-                write!(f, "required relative locktime CSV of {} blocks, not met", n)
-            }
-            Error::AbsoluteLocktimeNotMet(n) => write!(
-                f,
-                "required absolute locktime CLTV of {} blocks, not met",
-                n
-            ),
             Error::CouldNotSatisfy => f.write_str("could not satisfy"),
-            Error::BadPubkey(ref e) => fmt::Display::fmt(e, f),
             Error::TypeCheck(ref e) => write!(f, "typecheck: {}", e),
             Error::BadDescriptor(ref e) => write!(f, "Invalid descriptor: {}", e),
             Error::Secp(ref e) => fmt::Display::fmt(e, f),
@@ -561,11 +532,6 @@ impl fmt::Display for Error {
                 "Recursive depth over {} not permitted",
                 MAX_RECURSION_DEPTH
             ),
-            Error::ScriptSizeTooLarge => write!(
-                f,
-                "Standardness rules imply bitcoin than {} bytes",
-                MAX_SCRIPT_SIZE
-            ),
             Error::NonStandardBareScript => write!(
                 f,
                 "Anything but c:pk(key) (P2PK), c:pk_h(key) (P2PKH), and thresh_m(k,...) \
@@ -579,9 +545,7 @@ impl fmt::Display for Error {
                 write!(f, "Pubkey error: {} under {} scriptcontext", pk, ctx)
             }
             Error::MultiATooManyKeys(k) => write!(f, "MultiA too many keys {}", k),
-            Error::TaprootSpendInfoUnavialable => write!(f, "Taproot Spend Info not computed."),
             Error::TrNoScriptCode => write!(f, "No script code for Tr descriptors"),
-            Error::TrNoExplicitScript => write!(f, "No script code for Tr descriptors"),
             Error::MultipathDescLenMismatch => write!(f, "At least two BIP389 key expressions in the descriptor contain tuples of derivation indexes of different lengths"),
             Error::AbsoluteLockTime(ref e) => e.fmt(f),
             Error::RelativeLockTime(ref e) => e.fmt(f),
@@ -605,30 +569,22 @@ impl error::Error for Error {
             | UnexpectedStart
             | Unexpected(_)
             | MultiColon(_)
-            | MultiAt(_)
             | AtOutsideOr(_)
             | UnknownWrapper(_)
             | NonTopLevel(_)
             | Trailing(_)
-            | MissingHash(_)
             | MissingSig(_)
-            | RelativeLocktimeNotMet(_)
-            | AbsoluteLocktimeNotMet(_)
             | CouldNotSatisfy
             | TypeCheck(_)
             | BadDescriptor(_)
             | MaxRecursiveDepthExceeded
-            | ScriptSizeTooLarge
             | NonStandardBareScript
             | ImpossibleSatisfaction
             | BareDescriptorAddr
-            | TaprootSpendInfoUnavialable
             | TrNoScriptCode
-            | TrNoExplicitScript
             | MultipathDescLenMismatch => None,
             Script(e) => Some(e),
             AddrError(e) => Some(e),
-            BadPubkey(e) => Some(e),
             Secp(e) => Some(e),
             #[cfg(feature = "compiler")]
             CompilerError(e) => Some(e),
diff --git a/src/miniscript/astelem.rs b/src/miniscript/astelem.rs
@@ -139,7 +139,7 @@ fn fmt_1<D: fmt::Debug + fmt::Display>(
     a: &D,
     is_debug: bool,
 ) -> fmt::Result {
-    f.write_str(&name)?;
+    f.write_str(name)?;
     conditional_fmt(f, a, is_debug)?;
     f.write_str(")")
 }
@@ -150,7 +150,7 @@ fn fmt_2<D: fmt::Debug + fmt::Display>(
     b: &D,
     is_debug: bool,
 ) -> fmt::Result {
-    f.write_str(&name)?;
+    f.write_str(name)?;
     conditional_fmt(f, a, is_debug)?;
     f.write_str(",")?;
     conditional_fmt(f, b, is_debug)?;
@@ -163,7 +163,7 @@ fn fmt_n<D: fmt::Debug + fmt::Display>(
     list: &[D],
     is_debug: bool,
 ) -> fmt::Result {
-    f.write_str(&name)?;
+    f.write_str(name)?;
     write!(f, "{}", first)?;
     for el in list {
         f.write_str(",")?;
diff --git a/src/miniscript/decode.rs b/src/miniscript/decode.rs
@@ -11,11 +11,10 @@ use core::marker::PhantomData;
 use std::error;
 
 use bitcoin::hashes::{hash160, ripemd160, sha256, Hash};
-use bitcoin::Weight;
 use sync::Arc;
 
 use crate::miniscript::lex::{Token as Tk, TokenIter};
-use crate::miniscript::limits::MAX_PUBKEYS_PER_MULTISIG;
+use crate::miniscript::limits::{MAX_PUBKEYS_IN_CHECKSIGADD, MAX_PUBKEYS_PER_MULTISIG};
 use crate::miniscript::types::extra_props::ExtData;
 use crate::miniscript::types::Type;
 use crate::miniscript::ScriptContext;
@@ -451,10 +450,9 @@ pub fn parse<Ctx: ScriptContext>(
                     },
                     // MultiA
                     Tk::NumEqual, Tk::Num(k) => {
-                        let max = Weight::MAX_BLOCK.to_wu() / 32;
                         // Check size before allocating keys
-                        if k as u64 > max {
-                            return Err(Error::MultiATooManyKeys(max))
+                        if k as usize > MAX_PUBKEYS_IN_CHECKSIGADD {
+                            return Err(Error::MultiATooManyKeys(MAX_PUBKEYS_IN_CHECKSIGADD as u64))
                         }
                         let mut keys = Vec::with_capacity(k as usize); // atleast k capacity
                         while tokens.peek() == Some(&Tk::CheckSigAdd) {
diff --git a/src/miniscript/limits.rs b/src/miniscript/limits.rs
@@ -33,3 +33,5 @@ pub const MAX_BLOCK_WEIGHT: usize = 4000000;
 /// Maximum pubkeys as arguments to CHECKMULTISIG
 // https://github.com/bitcoin/bitcoin/blob/6acda4b00b3fc1bfac02f5de590e1a5386cbc779/src/script/script.h#L30
 pub const MAX_PUBKEYS_PER_MULTISIG: usize = 20;
+/// Maximum pubkeys in a CHECKSIGADD construction.
+pub const MAX_PUBKEYS_IN_CHECKSIGADD: usize = (bitcoin::Weight::MAX_BLOCK.to_wu() / 32) as usize;
diff --git a/src/miniscript/types/mod.rs b/src/miniscript/types/mod.rs
@@ -220,7 +220,7 @@ impl Type {
 
     /// Constructor for the type of the `a:` fragment.
     pub const fn cast_alt(self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
+        // FIXME need to do manual `?` because ? is not supported in constfns. (Also below.)
         Ok(Type {
             corr: match Correctness::cast_alt(self.corr) {
                 Ok(x) => x,
@@ -232,7 +232,6 @@ impl Type {
 
     /// Constructor for the type of the `s:` fragment.
     pub const fn cast_swap(self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::cast_swap(self.corr) {
                 Ok(x) => x,
@@ -255,7 +254,6 @@ impl Type {
 
     /// Constructor for the type of the `d:` fragment.
     pub const fn cast_dupif(self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::cast_dupif(self.corr) {
                 Ok(x) => x,
@@ -267,7 +265,6 @@ impl Type {
 
     /// Constructor for the type of the `v:` fragment.
     pub const fn cast_verify(self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::cast_verify(self.corr) {
                 Ok(x) => x,
@@ -279,7 +276,6 @@ impl Type {
 
     /// Constructor for the type of the `j:` fragment.
     pub const fn cast_nonzero(self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::cast_nonzero(self.corr) {
                 Ok(x) => x,
@@ -291,7 +287,6 @@ impl Type {
 
     /// Constructor for the type of the `n:` fragment.
     pub const fn cast_zeronotequal(self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::cast_zeronotequal(self.corr) {
                 Ok(x) => x,
@@ -303,7 +298,6 @@ impl Type {
 
     /// Constructor for the type of the `t:` fragment.
     pub const fn cast_true(self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::cast_true(self.corr) {
                 Ok(x) => x,
@@ -315,7 +309,6 @@ impl Type {
 
     /// Constructor for the type of the `u:` fragment.
     pub const fn cast_unlikely(self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::cast_or_i_false(self.corr) {
                 Ok(x) => x,
@@ -327,7 +320,6 @@ impl Type {
 
     /// Constructor for the type of the `l:` fragment.
     pub const fn cast_likely(self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::cast_or_i_false(self.corr) {
                 Ok(x) => x,
@@ -339,7 +331,6 @@ impl Type {
 
     /// Constructor for the type of the `and_b` fragment.
     pub const fn and_b(left: Self, right: Self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::and_b(left.corr, right.corr) {
                 Ok(x) => x,
@@ -351,7 +342,6 @@ impl Type {
 
     /// Constructor for the type of the `and_v` fragment.
     pub const fn and_v(left: Self, right: Self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::and_v(left.corr, right.corr) {
                 Ok(x) => x,
@@ -363,7 +353,6 @@ impl Type {
 
     /// Constructor for the type of the `or_b` fragment.
     pub const fn or_b(left: Self, right: Self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::or_b(left.corr, right.corr) {
                 Ok(x) => x,
@@ -375,7 +364,6 @@ impl Type {
 
     /// Constructor for the type of the `or_b` fragment.
     pub const fn or_d(left: Self, right: Self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::or_d(left.corr, right.corr) {
                 Ok(x) => x,
@@ -387,7 +375,6 @@ impl Type {
 
     /// Constructor for the type of the `or_c` fragment.
     pub const fn or_c(left: Self, right: Self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::or_c(left.corr, right.corr) {
                 Ok(x) => x,
@@ -410,7 +397,6 @@ impl Type {
 
     /// Constructor for the type of the `and_or` fragment.
     pub const fn and_or(a: Self, b: Self, c: Self) -> Result<Self, ErrorKind> {
-        // FIXME need to do manual `?` because ? is not supported in constfns.
         Ok(Type {
             corr: match Correctness::and_or(a.corr, b.corr, c.corr) {
                 Ok(x) => x,
diff --git a/src/policy/compiler.rs b/src/policy/compiler.rs
diff --git a/src/policy/concrete.rs b/src/policy/concrete.rs
diff --git a/src/policy/mod.rs b/src/policy/mod.rs
diff --git a/src/policy/semantic.rs b/src/policy/semantic.rs
