Merge #747: refactor: check the node syntax before size limit

3cdc711 test: ensure multi fragment panic before size limit panic (ChrisCho-H)
da806be refactor: check the node syntax before size limit (Chris Hyunhum Cho)

Pull request description:

  I’m not 100% sure if it’s intended, but the behavior of `check_global_consensus_validity`, which checks the script size limit first and then node could confuse the user-side.
  For example, If `sh` or `wsh` descriptor uses `multi_a` with 999 public key(e.g. `descriptor::Wsh::from_str("wsh(multi_a(pk1...pk999)")`), it will throw an error on the size limit, not the fragment `multi_a`. It might be a bit more efficient in case of size overflow, but

  1.  As miniscript functions as a language, syntax or grammar check should precedes stack limit. While the most of syntax are checked before `check_global_consensus_validity`, Syntax of which validity differ on context(e.g. `multi` and `multi_a`) are not.
  2. It’s more helpful to debug for user if blocked by wrong syntax first.
  3. It’s weird to check the script size which could be possibly wrong script upon context.

  While descriptor itself is kinda helper tool which uses miniscript, it would worth checking syntax first.

ACKs for top commit:
  apoelstra:
    ACK 3cdc711 successfully ran local tests

Tree-SHA512: 664bbd1fecba523a6b4a8e1ede46ef3129917bbd1602000560071321c73704fb2a686660d1f6a3cd903920514e1aac7109bfa045d235b249125dab81a58bb315

src/miniscript/context.rs

@@ -394,11 +394,8 @@ impl ScriptContext for Legacy {
     fn check_global_consensus_validity<Pk: MiniscriptKey>(
         ms: &Miniscript<Pk, Self>,
     ) -> Result<(), ScriptContextError> {
-        if ms.ext.pk_cost > MAX_SCRIPT_ELEMENT_SIZE {
-            return Err(ScriptContextError::MaxRedeemScriptSizeExceeded);
-        }
-
-        match ms.node {
+        // 1. Check the node first, throw an error on the language itself
+        let node_checked = match ms.node {
             Terminal::PkK(ref pk) => Self::check_pk(pk),
             Terminal::Multi(ref thresh) => {
                 for pk in thresh.iter() {
@@ -408,6 +405,17 @@ impl ScriptContext for Legacy {
             }
             Terminal::MultiA(..) => Err(ScriptContextError::MultiANotAllowed),
             _ => Ok(()),
+        };
+        // 2. After fragment and param check, validate the script size finally
+        match node_checked {
+            Ok(_) => {
+                if ms.ext.pk_cost > MAX_SCRIPT_ELEMENT_SIZE {
+                    Err(ScriptContextError::MaxRedeemScriptSizeExceeded)
+                } else {
+                    Ok(())
+                }
+            }
+            Err(_) => node_checked,
         }
     }
 
@@ -492,11 +500,8 @@ impl ScriptContext for Segwitv0 {
     fn check_global_consensus_validity<Pk: MiniscriptKey>(
         ms: &Miniscript<Pk, Self>,
     ) -> Result<(), ScriptContextError> {
-        if ms.ext.pk_cost > MAX_SCRIPT_SIZE {
-            return Err(ScriptContextError::MaxWitnessScriptSizeExceeded);
-        }
-
-        match ms.node {
+        // 1. Check the node first, throw an error on the language itself
+        let node_checked = match ms.node {
             Terminal::PkK(ref pk) => Self::check_pk(pk),
             Terminal::Multi(ref thresh) => {
                 for pk in thresh.iter() {
@@ -506,6 +511,17 @@ impl ScriptContext for Segwitv0 {
             }
             Terminal::MultiA(..) => Err(ScriptContextError::MultiANotAllowed),
             _ => Ok(()),
+        };
+        // 2. After fragment and param check, validate the script size finally
+        match node_checked {
+            Ok(_) => {
+                if ms.ext.pk_cost > MAX_SCRIPT_SIZE {
+                    Err(ScriptContextError::MaxWitnessScriptSizeExceeded)
+                } else {
+                    Ok(())
+                }
+            }
+            Err(_) => node_checked,
         }
     }
 
@@ -598,16 +614,8 @@ impl ScriptContext for Tap {
     fn check_global_consensus_validity<Pk: MiniscriptKey>(
         ms: &Miniscript<Pk, Self>,
     ) -> Result<(), ScriptContextError> {
-        // No script size checks for global consensus rules
-        // Should we really check for block limits here.
-        // When the transaction sizes get close to block limits,
-        // some guarantees are not easy to satisfy because of knapsack
-        // constraints
-        if ms.ext.pk_cost as u64 > Weight::MAX_BLOCK.to_wu() {
-            return Err(ScriptContextError::MaxWitnessScriptSizeExceeded);
-        }
-
-        match ms.node {
+        // 1. Check the node first, throw an error on the language itself
+        let node_checked = match ms.node {
             Terminal::PkK(ref pk) => Self::check_pk(pk),
             Terminal::MultiA(ref thresh) => {
                 for pk in thresh.iter() {
@@ -617,6 +625,22 @@ impl ScriptContext for Tap {
             }
             Terminal::Multi(..) => Err(ScriptContextError::TaprootMultiDisabled),
             _ => Ok(()),
+        };
+        // 2. After fragment and param check, validate the script size finally
+        match node_checked {
+            Ok(_) => {
+                // No script size checks for global consensus rules
+                // Should we really check for block limits here.
+                // When the transaction sizes get close to block limits,
+                // some guarantees are not easy to satisfy because of knapsack
+                // constraints
+                if ms.ext.pk_cost as u64 > Weight::MAX_BLOCK.to_wu() {
+                    Err(ScriptContextError::MaxWitnessScriptSizeExceeded)
+                } else {
+                    Ok(())
+                }
+            }
+            Err(_) => node_checked,
         }
     }
 
@@ -700,10 +724,8 @@ impl ScriptContext for BareCtx {
     fn check_global_consensus_validity<Pk: MiniscriptKey>(
         ms: &Miniscript<Pk, Self>,
     ) -> Result<(), ScriptContextError> {
-        if ms.ext.pk_cost > MAX_SCRIPT_SIZE {
-            return Err(ScriptContextError::MaxWitnessScriptSizeExceeded);
-        }
-        match ms.node {
+        // 1. Check the node first, throw an error on the language itself
+        let node_checked = match ms.node {
             Terminal::PkK(ref key) => Self::check_pk(key),
             Terminal::Multi(ref thresh) => {
                 for pk in thresh.iter() {
@@ -713,6 +735,17 @@ impl ScriptContext for BareCtx {
             }
             Terminal::MultiA(..) => Err(ScriptContextError::MultiANotAllowed),
             _ => Ok(()),
+        };
+        // 2. After fragment and param check, validate the script size finally
+        match node_checked {
+            Ok(_) => {
+                if ms.ext.pk_cost > MAX_SCRIPT_SIZE {
+                    Err(ScriptContextError::MaxWitnessScriptSizeExceeded)
+                } else {
+                    Ok(())
+                }
+            }
+            Err(_) => node_checked,
         }
     }
 

src/miniscript/mod.rs

@@ -829,7 +829,9 @@ mod tests {
     use crate::policy::Liftable;
     use crate::prelude::*;
     use crate::test_utils::{StrKeyTranslator, StrXOnlyKeyTranslator};
-    use crate::{hex_script, Error, ExtParams, RelLockTime, Satisfier, ToPublicKey};
+    use crate::{
+        hex_script, BareCtx, Error, ExtParams, Legacy, RelLockTime, Satisfier, ToPublicKey,
+    };
 
     type Segwitv0Script = Miniscript<bitcoin::PublicKey, Segwitv0>;
     type Tapscript = Miniscript<bitcoin::secp256k1::XOnlyPublicKey, Tap>;
@@ -1634,6 +1636,67 @@ mod tests {
         }
         Tapscript::parse_insane(&script.into_script()).unwrap_err();
     }
+
+    #[test]
+    fn test_context_global_consensus() {
+        // Test from string tests
+        type LegacyMs = Miniscript<String, Legacy>;
+        type Segwitv0Ms = Miniscript<String, Segwitv0>;
+        type BareMs = Miniscript<String, BareCtx>;
+
+        // multisig script of 20 pubkeys exceeds 520 bytes
+        let pubkey_vec_20: Vec<String> = (0..20).map(|x| x.to_string()).collect();
+        // multisig script of 300 pubkeys exceeds 10,000 bytes
+        let pubkey_vec_300: Vec<String> = (0..300).map(|x| x.to_string()).collect();
+
+        // wrong multi_a for non-tapscript, while exceeding consensus size limit
+        let legacy_multi_a_ms =
+            LegacyMs::from_str(&format!("multi_a(20,{})", pubkey_vec_20.join(",")));
+        let segwit_multi_a_ms =
+            Segwitv0Ms::from_str(&format!("multi_a(300,{})", pubkey_vec_300.join(",")));
+        let bare_multi_a_ms =
+            BareMs::from_str(&format!("multi_a(300,{})", pubkey_vec_300.join(",")));
+
+        // Should panic for wrong multi_a, even if it exceeds the max consensus size
+        assert_eq!(
+            legacy_multi_a_ms.unwrap_err().to_string(),
+            "Multi a(CHECKSIGADD) only allowed post tapscript"
+        );
+        assert_eq!(
+            segwit_multi_a_ms.unwrap_err().to_string(),
+            "Multi a(CHECKSIGADD) only allowed post tapscript"
+        );
+        assert_eq!(
+            bare_multi_a_ms.unwrap_err().to_string(),
+            "Multi a(CHECKSIGADD) only allowed post tapscript"
+        );
+
+        // multisig script of 20 pubkeys exceeds 520 bytes
+        let multi_ms = format!("multi(20,{})", pubkey_vec_20.join(","));
+        // other than legacy, and_v to build 15 nested 20-of-20 multisig script
+        // to exceed 10,000 bytes without violation of threshold limit(max: 20)
+        let and_v_nested_multi_ms =
+            format!("and_v(v:{},", multi_ms).repeat(14) + &multi_ms + "))))))))))))))";
+
+        // correct multi for non-tapscript, but exceeding consensus size limit
+        let legacy_multi_ms = LegacyMs::from_str(&multi_ms);
+        let segwit_multi_ms = Segwitv0Ms::from_str(&and_v_nested_multi_ms);
+        let bare_multi_ms = BareMs::from_str(&and_v_nested_multi_ms);
+
+        // Should panic for exceeding the max consensus size, as multi properly used
+        assert_eq!(
+            legacy_multi_ms.unwrap_err().to_string(),
+            "The Miniscript corresponding Script would be larger than MAX_SCRIPT_ELEMENT_SIZE bytes."
+        );
+        assert_eq!(
+            segwit_multi_ms.unwrap_err().to_string(),
+            "The Miniscript corresponding Script would be larger than MAX_STANDARD_P2WSH_SCRIPT_SIZE bytes."
+        );
+        assert_eq!(
+            bare_multi_ms.unwrap_err().to_string(),
+            "The Miniscript corresponding Script would be larger than MAX_STANDARD_P2WSH_SCRIPT_SIZE bytes."
+        );
+    }
 }
 
 #[cfg(bench)]