Repository: rust-bitcoin/rust-bitcoin
Pull request #2081 (2023-09-18 23:18:52)

Feature: Instruction can read the script number
This new method on `Instruction` will allow for interpreting the Instruction as a script number (i32)

apoelstra: Looks good! But we have an existing `read_scriptint` function that I think can do the `PushBytes` case. (Or did you check that it wasn't applicable for some reason?)

Could you also add a unit test for a couple negative zeros :}?
apoelstra: Ok, c7ff61d072d35562fe15850b4f9631e08afdc2e6 looks good now! But what do you think of renaming `read_scriptint_internal` to `read_scriptint_nonminimal`?
junderw: Should I expose it publicly?

In which case, we'd still want the > 4 byte check for both.
apoelstra: @junderw yeah, I think it should be exposed publicly.
junderw: Should be good for re-review now.
junderw: If tests pass, this is ready for re-review
tcharding: Ouch, CI fail is linter because `if v.len() == 0` should be `if v.is_empty()`.

Scond time this week my review suggestions have failed to build.

Patch:
diff --git a/bitcoin/src/blockdata/script/instruction.rs b/bitcoin/src/blockdata/script/instruction.rs
index 7802a0e1..6c3da7bf 100644
--- a/bitcoin/src/blockdata/script/instruction.rs
+++ b/bitcoin/src/blockdata/script/instruction.rs
@@ -31,6 +31,31 @@ pub fn push_bytes(&self) -> Option<&PushBytes> {
         }
     }
 
+    /// Returns the number interpretted by the script parser
+    /// if it can be coerced into a number.
+    ///
+    /// This does not require the script num to be minimal.
+    pub fn script_num(&self) -> Option<i64> {
+        match self {
+            Instruction::Op(op) => {
+                let v = op.to_u8();
+                match v {
+                    // OP_PUSHNUM_1 ..= OP_PUSHNUM_16
+                    0x51..=0x60 => Some(v as i64 - 0x50),
+                    // OP_PUSHNUM_NEG1
+                    0x4f => Some(-1),
+                    _ => None,
+                }
+            }
+            Instruction::PushBytes(bytes) => {
+                match super::read_scriptint_non_minimal(bytes.as_bytes()) {
+                    Ok(v) => Some(v),
+                    _ => None,
+                }
+            }
+        }
+    }
+
     /// Returns the number of bytes required to encode the instruction in script.
     pub(super) fn script_serialized_len(&self) -> usize {
         match self {
diff --git a/bitcoin/src/blockdata/script/mod.rs b/bitcoin/src/blockdata/script/mod.rs
index 0a5d53c4..9fb09360 100644
--- a/bitcoin/src/blockdata/script/mod.rs
+++ b/bitcoin/src/blockdata/script/mod.rs
@@ -170,20 +170,19 @@ fn from(script: &Script) -> WScriptHash { script.wscript_hash() }
 ///
 /// This code is based on the `CScriptNum` constructor in Bitcoin Core (see `script.h`).
 pub fn read_scriptint(v: &[u8]) -> Result<i64, Error> {
-    let len = v.len();
-    if len > 4 {
-        return Err(Error::NumericOverflow);
-    }
     let last = match v.last() {
         Some(last) => last,
         None => return Ok(0),
     };
+    if v.len() > 4 {
+        return Err(Error::NumericOverflow);
+    }
     // Comment and code copied from Bitcoin Core:
     // https://github.com/bitcoin/bitcoin/blob/447f50e4aed9a8b1d80e1891cda85801aeb80b4e/src/script/script.h#L247-L262
     // If the most-significant-byte - excluding the sign bit - is zero
     // then we're not minimal. Note how this test also rejects the
     // negative-zero encoding, 0x80.
-    if (last & 0x7f) == 0 {
+    if (*last & 0x7f) == 0 {
         // One exception: if there's more than one byte and the most
         // significant bit of the second-most-significant-byte is set
         // it would conflict with the sign bit. An example of this case
@@ -194,12 +193,33 @@ pub fn read_scriptint(v: &[u8]) -> Result<i64, Error> {
         }
     }
 
+    Ok(scriptint_parse(v))
+}
+
+/// Decodes an integer in script format without non-minimal error.
+///
+/// The overflow error for slices over 4 bytes long is still there.
+/// See [`read_scriptint`] for a description of some subtleties of
+/// this function.
+pub fn read_scriptint_non_minimal(v: &[u8]) -> Result<i64, Error> {
+    if v.is_empty() {
+        return Ok(0);
+    }
+    if v.len() > 4 {
+        return Err(Error::NumericOverflow);
+    }
+
+    Ok(scriptint_parse(v))
+}
+
+// Caller to guarantee that `v` is not empty.
+fn scriptint_parse(v: &[u8]) -> i64 {
     let (mut ret, sh) = v.iter().fold((0, 0), |(acc, sh), n| (acc + ((*n as i64) << sh), sh + 8));
-    if v[len - 1] & 0x80 != 0 {
+    if v[v.len() - 1] & 0x80 != 0 {
         ret &= (1 << (sh - 1)) - 1;
         ret = -ret;
     }
-    Ok(ret)
+    ret
 }
 
 /// Decodes a boolean.


Test patch:
diff --git a/bitcoin/src/blockdata/script/tests.rs b/bitcoin/src/blockdata/script/tests.rs
index 4b494a46..60105d3c 100644
--- a/bitcoin/src/blockdata/script/tests.rs
+++ b/bitcoin/src/blockdata/script/tests.rs
@@ -312,9 +312,13 @@ fn build_scriptint(n: i64) -> Vec<u8> {
     for &i in test_vectors.iter() {
         assert_eq!(Ok(i), read_scriptint(&build_scriptint(i)));
         assert_eq!(Ok(-i), read_scriptint(&build_scriptint(-i)));
+        assert_eq!(Ok(i), read_scriptint_non_minimal(&build_scriptint(i)));
+        assert_eq!(Ok(-i), read_scriptint_non_minimal(&build_scriptint(-i)));
     }
     assert!(read_scriptint(&build_scriptint(1 << 31)).is_err());
     assert!(read_scriptint(&build_scriptint(-(1 << 31))).is_err());
+    assert!(read_scriptint_non_minimal(&build_scriptint(1 << 31)).is_err());
+    assert!(read_scriptint_non_minimal(&build_scriptint(-(1 << 31))).is_err());
 }
 
 #[test]
@@ -323,6 +327,11 @@ fn non_minimal_scriptints() {
     assert_eq!(read_scriptint(&[0xff, 0x00]), Ok(0xff));
     assert_eq!(read_scriptint(&[0x8f, 0x00, 0x00]), Err(Error::NonMinimalPush));
     assert_eq!(read_scriptint(&[0x7f, 0x00]), Err(Error::NonMinimalPush));
+
+    assert_eq!(read_scriptint_non_minimal(&[0x80, 0x00]), Ok(0x80));
+    assert_eq!(read_scriptint_non_minimal(&[0xff, 0x00]), Ok(0xff));
+    assert_eq!(read_scriptint_non_minimal(&[0x8f, 0x00, 0x00]), Ok(0x8f));
+    assert_eq!(read_scriptint_non_minimal(&[0x7f, 0x00]), Ok(0x7f));
 }
 
 #[test]
@@ -769,3 +778,49 @@ fn read_scriptbool_non_zero_is_true() {
     let v: Vec<u8> = vec![0x01, 0x00, 0x00, 0x80]; // With sign bit set.
     assert!(read_scriptbool(&v));
 }
+
+#[test]
+fn instruction_script_num_parse() {
+    let push_bytes = [
+        (PushBytesBuf::from([]), Some(0)),
+        (PushBytesBuf::from([0x00]), Some(0)),
+        (PushBytesBuf::from([0x01]), Some(1)),
+        // Check all the negative 1s
+        (PushBytesBuf::from([0x81]), Some(-1)),
+        (PushBytesBuf::from([0x01, 0x80]), Some(-1)),
+        (PushBytesBuf::from([0x01, 0x00, 0x80]), Some(-1)),
+        (PushBytesBuf::from([0x01, 0x00, 0x00, 0x80]), Some(-1)),
+        // Check all the negative 0s
+        (PushBytesBuf::from([0x80]), Some(0)),
+        (PushBytesBuf::from([0x00, 0x80]), Some(0)),
+        (PushBytesBuf::from([0x00, 0x00, 0x80]), Some(0)),
+        (PushBytesBuf::from([0x00, 0x00, 0x00, 0x80]), Some(0)),
+        // Too long
+        (PushBytesBuf::from([0x01, 0x00, 0x00, 0x00, 0x80]), None),
+        // Check the position of all the bytes
+        (PushBytesBuf::from([0xef, 0xbe, 0xad, 0x5e]), Some(0x5eadbeef)),
+        // Add negative
+        (PushBytesBuf::from([0xef, 0xbe, 0xad, 0xde]), Some(-0x5eadbeef)),
+    ];
+    let ops = [
+        (Instruction::Op(opcodes::all::OP_PUSHDATA4), None),
+        (Instruction::Op(opcodes::all::OP_PUSHNUM_NEG1), Some(-1)),
+        (Instruction::Op(opcodes::all::OP_RESERVED), None),
+        (Instruction::Op(opcodes::all::OP_PUSHNUM_1), Some(1)),
+        (Instruction::Op(opcodes::all::OP_PUSHNUM_16), Some(16)),
+        (Instruction::Op(opcodes::all::OP_NOP), None),
+    ];
+    for (input, expected) in &push_bytes {
+        assert_eq!(Instruction::PushBytes(input).script_num(), *expected);
+    }
+    for (input, expected) in &ops {
+        assert_eq!(input.script_num(), *expected);
+    }
+
+    // script_num() is predicated on OP_0/OP_FALSE (0x00)
+    // being treated as an empty PushBytes
+    assert_eq!(
+        Script::from_bytes(&[0x00]).instructions().next(),
+        Some(Ok(Instruction::PushBytes(PushBytes::empty()))),
+    );
+}