Check minimal push encoding only when executed

src/lib.rs

@@ -78,7 +78,7 @@ pub struct Experimental {
 /// Used to fine-tune different variables during execution.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Options {
-    /// Require data pushes be minimally encoded.
+    /// Require executed data pushes and consumed numbers to be minimally encoded.
     pub require_minimal: bool, //TODO(stevenroose) double check all fRequireMinimal usage in Core
     /// Verify OP_CHECKLOCKTIMEVERIFY.
     pub verify_cltv: bool,
@@ -228,11 +228,9 @@ impl Exec {
 
         // We want to make sure the script is valid so we don't have to throw parsing errors
         // while executing.
-        let instructions = if opt.require_minimal {
-            script.instructions_minimal()
-        } else {
-            script.instructions()
-        };
+        // Minimal-push policy applies only when a push is executed. Parse
+        // syntax here so nonminimal pushes in skipped branches remain valid.
+        let instructions = script.instructions();
         if let Some(err) = instructions.clone().find_map(|res| res.err()) {
             return Err(Error::InvalidScript(err));
         }
@@ -245,11 +243,7 @@ impl Exec {
         // We box alocate the script to get a static Instructions iterator.
         // We will manually drop this allocation in the ops::Drop impl.
         let script = Box::leak(script.into_boxed_script()) as &'static Script;
-        let instructions = if opt.require_minimal {
-            script.instructions_minimal()
-        } else {
-            script.instructions()
-        };
+        let instructions = script.instructions();
 
         //TODO(stevenroose) make this more efficient
         let witness_size =
@@ -501,6 +495,15 @@ impl Exec {
                     return self.fail(ExecError::PushSize);
                 }
                 if exec {
+                    if self.opt.require_minimal
+                        && self.script[self.current_position..]
+                            .instructions_minimal()
+                            .next()
+                            .expect("already parsed instruction")
+                            .is_err()
+                    {
+                        return self.fail(ExecError::MinimalData);
+                    }
                     self.stack.pushstr(p.as_bytes());
                 }
             }

tests/minimal_pushes.rs

@@ -0,0 +1,217 @@
+use bitcoin::{hashes::Hash, ScriptBuf, TapLeafHash, Transaction};
+use bitcoin_scriptexec::{Error, Exec, ExecCtx, ExecError, Options, TxTemplate};
+
+const CONTEXTS: [ExecCtx; 3] = [ExecCtx::Legacy, ExecCtx::SegwitV0, ExecCtx::Tapscript];
+
+fn executor(
+    ctx: ExecCtx,
+    minimal: bool,
+    script: Vec<u8>,
+    witness: Vec<Vec<u8>>,
+) -> Result<Exec, Error> {
+    Exec::new(
+        ctx,
+        Options {
+            require_minimal: minimal,
+            verify_minimal_if: false,
+            ..Options::default()
+        },
+        TxTemplate {
+            tx: Transaction {
+                version: bitcoin::transaction::Version::TWO,
+                lock_time: bitcoin::absolute::LockTime::ZERO,
+                input: vec![],
+                output: vec![],
+            },
+            prevouts: vec![],
+            input_idx: 0,
+            taproot_annex_scriptleaf: Some((TapLeafHash::all_zeros(), None)),
+        },
+        ScriptBuf::from_bytes(script),
+        witness,
+    )
+}
+
+fn execute(ctx: ExecCtx, minimal: bool, script: Vec<u8>, witness: Vec<Vec<u8>>) -> Exec {
+    let mut exec = executor(ctx, minimal, script, witness)
+        .expect("syntactically valid scripts must construct independently of minimal-push policy");
+    while exec.exec_next().is_ok() {}
+    exec
+}
+
+fn nonminimal_pushes() -> Vec<Vec<u8>> {
+    vec![
+        vec![0x4c, 0], // PUSHDATA1 instead of OP_0
+        vec![1, 1],    // data push instead of OP_1
+        vec![1, 0x81], // data push instead of OP_1NEGATE
+        vec![0x4c, 1, 17],
+        vec![0x4d, 1, 0, 17],
+        vec![0x4e, 1, 0, 0, 0, 17],
+    ]
+}
+
+#[test]
+fn executed_nonminimal_pushes_obey_policy_at_execution() {
+    for ctx in CONTEXTS {
+        for minimal in [false, true] {
+            for mut script in nonminimal_pushes() {
+                script.extend([0x75, 0x51]); // DROP TRUE
+                let exec = execute(ctx, minimal, script, vec![]);
+                let result = exec.result().unwrap();
+                if minimal {
+                    assert_eq!(result.error, Some(ExecError::MinimalData));
+                    assert!(!result.success);
+                    assert!(
+                        exec.stack().is_empty(),
+                        "a rejected push must not mutate the stack"
+                    );
+                } else {
+                    assert_eq!(result.error, None);
+                    assert!(result.success);
+                }
+            }
+        }
+    }
+}
+
+#[test]
+fn nonminimal_pushes_in_unexecuted_branches_are_accepted() {
+    for ctx in CONTEXTS {
+        for minimal in [false, true] {
+            for push in nonminimal_pushes() {
+                // Both a false IF body and the ELSE arm of a true IF are skipped.
+                for (prefix, suffix) in [
+                    (vec![0x00, 0x63], vec![0x68, 0x51]),
+                    (vec![0x51, 0x63, 0x51, 0x67], vec![0x68]),
+                    (vec![0x00, 0x63, 0x63], vec![0x68, 0x68, 0x51]),
+                ] {
+                    let mut script = prefix;
+                    script.extend(&push);
+                    script.extend(suffix);
+                    let exec = execute(ctx, minimal, script, vec![]);
+                    assert_eq!(exec.result().unwrap().error, None);
+                    assert!(exec.result().unwrap().success);
+                }
+            }
+        }
+    }
+}
+
+#[test]
+fn minimal_pushes_at_encoding_boundaries_are_accepted() {
+    let mut pushes = vec![vec![0], vec![0x4f], vec![0x51], vec![0x60], vec![1, 17]];
+    for (size, mut prefix) in [
+        (75, vec![75]),
+        (76, vec![0x4c, 76]),
+        (255, vec![0x4c, 255]),
+        (256, vec![0x4d, 0, 1]),
+        (520, vec![0x4d, 8, 2]),
+    ] {
+        prefix.extend(vec![17; size]);
+        pushes.push(prefix);
+    }
+    for ctx in CONTEXTS {
+        for minimal in [false, true] {
+            for push in &pushes {
+                let mut script = push.clone();
+                script.extend([0x75, 0x51]);
+                let exec = execute(ctx, minimal, script, vec![]);
+                assert_eq!(exec.result().unwrap().error, None);
+                assert!(exec.result().unwrap().success);
+            }
+        }
+    }
+}
+
+#[test]
+fn malformed_pushes_remain_constructor_errors_even_in_dead_branches() {
+    for ctx in CONTEXTS {
+        for minimal in [false, true] {
+            for script in [
+                vec![0x4c],
+                vec![0x4c, 2, 17],
+                vec![2, 17],
+                vec![0x00, 0x63, 0x4d, 1],
+            ] {
+                assert!(matches!(
+                    executor(ctx, minimal, script, vec![]),
+                    Err(Error::InvalidScript(
+                        bitcoin::script::Error::EarlyEndOfScript
+                    ))
+                ));
+            }
+        }
+    }
+}
+
+#[test]
+fn numeric_minimality_is_still_checked_when_a_number_is_consumed() {
+    for ctx in CONTEXTS {
+        for minimal in [false, true] {
+            for (number, expected) in [(vec![0], 1), (vec![0x80], 1), (vec![1, 0], 2)] {
+                let exec = execute(
+                    ctx,
+                    minimal,
+                    vec![0x8b, 0x50 + expected, 0x9c],
+                    vec![number.clone()],
+                );
+                assert_eq!(
+                    exec.result().unwrap().error,
+                    minimal.then_some(ExecError::MinimalData)
+                );
+                assert_eq!(exec.result().unwrap().success, !minimal);
+                // Merely dropping the same data must not require numeric canonicalization.
+                let dropped = execute(ctx, minimal, vec![0x75, 0x51], vec![number]);
+                assert!(dropped.result().unwrap().success);
+            }
+            let overflow = execute(ctx, minimal, vec![0x8b], vec![vec![1, 0, 0, 0, 0]]);
+            assert_eq!(
+                overflow.result().unwrap().error,
+                Some(ExecError::ScriptIntNumericOverflow)
+            );
+        }
+    }
+}
+
+#[test]
+fn tapscript_minimal_if_remains_consensus_enforced_with_either_option() {
+    for minimal in [false, true] {
+        for condition in [vec![], vec![1], vec![0], vec![2], vec![0x80], vec![1, 0]] {
+            let valid = condition.is_empty() || condition == [1];
+            // IF TRUE ELSE TRUE ENDIF; verify_minimal_if is deliberately false.
+            let exec = execute(
+                ExecCtx::Tapscript,
+                minimal,
+                vec![0x63, 0x51, 0x67, 0x51, 0x68],
+                vec![condition],
+            );
+            assert_eq!(exec.result().unwrap().success, valid);
+            assert_eq!(
+                exec.result().unwrap().error,
+                (!valid).then_some(ExecError::TapscriptMinimalIf)
+            );
+        }
+    }
+}
+
+#[test]
+fn oversized_push_precedes_minimality_and_fails_in_skipped_branches() {
+    let mut push = vec![0x4e, 9, 2, 0, 0]; // nonminimal PUSHDATA4 of 521 bytes
+    push.extend(vec![17; 521]);
+    for ctx in CONTEXTS {
+        for minimal in [false, true] {
+            for skipped in [false, true] {
+                let mut script = if skipped { vec![0, 0x63] } else { vec![] };
+                script.extend(&push);
+                script.extend(if skipped {
+                    vec![0x68, 0x51]
+                } else {
+                    vec![0x75, 0x51]
+                });
+                let exec = execute(ctx, minimal, script, vec![]);
+                assert_eq!(exec.result().unwrap().error, Some(ExecError::PushSize));
+                assert!(!exec.result().unwrap().success);
+            }
+        }
+    }
+}