Merge #859: typeck: clean up data structures

f3ea32c typeck: fix mistake in previous correction to thresh exec_stack_count (Andrew Poelstra)
1e6aead add unit tests for recent typeck bug fixes (Andrew Poelstra)
5f0072c typeck: move opcode limits into sat/dissat limits (Andrew Poelstra)
f57cac0 typeck: pull satisfaction/dissatisfaction limits into struct (Andrew Poelstra)

Pull request description:

  These simple commits just collect some related data into common data structures, allowing satisfaction/dissatisfaction data to all be `Option`al at once without any debug_asserts, and simplifying a bunch of code.
  
  This makes a couple obscure fixes that are not detected by tests and we have never received reports about. See the commit message for the first commit for details. With Claude 4's help I added unit tests for these and tweaked the fixes since my initial fixes didn't seem quite right.


ACKs for top commit:
  sanket1729:
    utACK f3ea32c


Tree-SHA512: a67f773e87d4bfe1ae9f53a614726445e353e429784b77e1639322ad7c0ee178b883ad76f38de9d89345f1d592cc6e915d92d7b2997a50a9dc862e1392e3828a

Author: apoelstra

src/miniscript/context.rs

@@ -436,7 +436,7 @@ impl ScriptContext for Legacy {
     fn check_local_consensus_validity<Pk: MiniscriptKey>(
         ms: &Miniscript<Pk, Self>,
     ) -> Result<(), ScriptContextError> {
-        match ms.ext.ops.op_count() {
+        match ms.ext.sat_op_count() {
             None => Err(ScriptContextError::ImpossibleSatisfaction),
             Some(op_count) if op_count > MAX_OPS_PER_SCRIPT => {
                 Err(ScriptContextError::MaxOpCountExceeded {
@@ -467,8 +467,7 @@ impl ScriptContext for Legacy {
     }
 
     fn max_satisfaction_size<Pk: MiniscriptKey>(ms: &Miniscript<Pk, Self>) -> Option<usize> {
-        // The scriptSig cost is the second element of the tuple
-        ms.ext.max_sat_size.map(|x| x.1)
+        ms.ext.sat_data.map(|data| data.max_script_sig_size)
     }
 
     fn pk_len<Pk: MiniscriptKey>(pk: &Pk) -> usize {
@@ -551,7 +550,7 @@ impl ScriptContext for Segwitv0 {
     fn check_local_consensus_validity<Pk: MiniscriptKey>(
         ms: &Miniscript<Pk, Self>,
     ) -> Result<(), ScriptContextError> {
-        match ms.ext.ops.op_count() {
+        match ms.ext.sat_op_count() {
             None => Err(ScriptContextError::ImpossibleSatisfaction),
             Some(op_count) if op_count > MAX_OPS_PER_SCRIPT => {
                 Err(ScriptContextError::MaxOpCountExceeded {
@@ -595,8 +594,7 @@ impl ScriptContext for Segwitv0 {
     }
 
     fn max_satisfaction_size<Pk: MiniscriptKey>(ms: &Miniscript<Pk, Self>) -> Option<usize> {
-        // The witness stack cost is the first element of the tuple
-        ms.ext.max_sat_size.map(|x| x.0)
+        ms.ext.sat_data.map(|data| data.max_witness_stack_size)
     }
 
     fn pk_len<Pk: MiniscriptKey>(_pk: &Pk) -> usize { 34 }
@@ -688,11 +686,10 @@ impl ScriptContext for Tap {
         // will have it's corresponding 64 bytes signature.
         // sigops budget = witness_script.len() + witness.size() + 50
         // Each signature will cover it's own cost(64 > 50) and thus will will never exceed the budget
-        if let (Some(s), Some(h)) = (ms.ext.exec_stack_elem_count_sat, ms.ext.stack_elem_count_sat)
-        {
-            if s + h > MAX_STACK_SIZE {
+        if let Some(data) = ms.ext.sat_data {
+            if data.max_witness_stack_count + data.max_exec_stack_count > MAX_STACK_SIZE {
                 return Err(ScriptContextError::StackSizeLimitExceeded {
-                    actual: s + h,
+                    actual: data.max_witness_stack_count + data.max_exec_stack_count,
                     limit: MAX_STACK_SIZE,
                 });
             }
@@ -714,8 +711,7 @@ impl ScriptContext for Tap {
     }
 
     fn max_satisfaction_size<Pk: MiniscriptKey>(ms: &Miniscript<Pk, Self>) -> Option<usize> {
-        // The witness stack cost is the first element of the tuple
-        ms.ext.max_sat_size.map(|x| x.0)
+        ms.ext.sat_data.map(|data| data.max_witness_stack_size)
     }
 
     fn sig_type() -> SigType { SigType::Schnorr }
@@ -787,7 +783,7 @@ impl ScriptContext for BareCtx {
     fn check_local_consensus_validity<Pk: MiniscriptKey>(
         ms: &Miniscript<Pk, Self>,
     ) -> Result<(), ScriptContextError> {
-        match ms.ext.ops.op_count() {
+        match ms.ext.sat_op_count() {
             None => Err(ScriptContextError::ImpossibleSatisfaction),
             Some(op_count) if op_count > MAX_OPS_PER_SCRIPT => {
                 Err(ScriptContextError::MaxOpCountExceeded {
@@ -812,8 +808,9 @@ impl ScriptContext for BareCtx {
     }
 
     fn max_satisfaction_size<Pk: MiniscriptKey>(ms: &Miniscript<Pk, Self>) -> Option<usize> {
-        // The witness stack cost is the first element of the tuple
-        ms.ext.max_sat_size.map(|x| x.1)
+        // For bare outputs the script appears in the scriptpubkey; its cost
+        // is the same as for a legacy scriptsig.
+        ms.ext.sat_data.map(|data| data.max_script_sig_size)
     }
 
     fn pk_len<Pk: MiniscriptKey>(pk: &Pk) -> usize {

src/miniscript/mod.rs

@@ -403,8 +403,8 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
     /// impossible to satisfy
     pub fn max_satisfaction_witness_elements(&self) -> Result<usize, Error> {
         self.ext
-            .stack_elem_count_sat
-            .map(|x| x + 1)
+            .sat_data
+            .map(|data| data.max_witness_stack_count + 1)
             .ok_or(Error::ImpossibleSatisfaction)
     }
 
@@ -1193,7 +1193,7 @@ mod tests {
                 assert_eq!(format!("{:x}", ms.encode()), expected_hex);
                 assert_eq!(ms.ty.mall.non_malleable, non_mal);
                 assert_eq!(ms.ty.mall.safe, need_sig);
-                assert_eq!(ms.ext.ops.op_count().unwrap(), ops);
+                assert_eq!(ms.ext.static_ops + ms.ext.sat_data.unwrap().max_exec_op_count, ops);
             }
             (Err(_), false) => {}
             _ => unreachable!(),
@@ -1883,6 +1883,53 @@ mod tests {
         Tapscript::decode_insane(&script.into_script()).unwrap_err();
     }
 
+    #[test]
+    fn test_or_d_exec_stack_count_fix() {
+        // Test for the or_d dissat_data.max_exec_stack_count fix
+        // The old code incorrectly added +1 to the exec stack count for or_d dissatisfaction
+        let ms_str = "or_d(pk(A),pk(B))";
+        let ms = Miniscript::<String, Segwitv0>::from_str_insane(ms_str).unwrap();
+
+        // With the fix, or_d dissatisfaction should not have the extra +1
+        // Both branches have exec_stack_count of 1, so dissat should be max(1,1) = 1, not 2
+        if let Some(dissat_data) = ms.ext.dissat_data {
+            assert_eq!(dissat_data.max_exec_stack_count, 1);
+        } else {
+            panic!("Expected dissat_data to be Some");
+        }
+    }
+
+    #[test]
+    fn test_threshold_exec_stack_count_max_not_sum() {
+        // Test for the threshold max_exec_stack_count fix
+        // The old code incorrectly summed exec stack counts, new code takes max
+        let ms_str = "thresh(2,pk(A),s:pk(B),s:pk(C))";
+        let ms = Miniscript::<String, Segwitv0>::from_str_insane(ms_str).unwrap();
+
+        // Each pk has exec_stack_count of 1, plus an extra stack element for the thresh accumulator.
+        // With the fix, threshold should take max(1,1,1) + 1 = 2, not sum 1+1+1 = 3
+        if let Some(sat_data) = ms.ext.sat_data {
+            assert_eq!(sat_data.max_exec_stack_count, 2);
+        } else {
+            panic!("Expected sat_data to be Some");
+        }
+
+        // Test with a more complex threshold, where the first child has a strictly higher
+        // exec_stack_count. This time, we take the maximum *without* adding +1 for the
+        // accumulator, since on the first child of `thresh` there is no accumulator yet
+        // (its initial value is the output value for the first child).
+        let complex_ms_str = "thresh(1,and_b(pk(A),s:pk(B)),s:pk(C))";
+        let complex_ms = Miniscript::<String, Segwitv0>::from_str_insane(complex_ms_str).unwrap();
+
+        // and_v has exec_stack_count of 2, pk has 1
+        // With the fix: max(2,1) = 2, old code would sum to 3
+        if let Some(sat_data) = complex_ms.ext.sat_data {
+            assert_eq!(sat_data.max_exec_stack_count, 2);
+        } else {
+            panic!("Expected sat_data to be Some");
+        }
+    }
+
     #[test]
     fn test_context_global_consensus() {
         // Test from string tests