Address the second half of @daira’s ZcashFoundation#174 review

src/interpreter.rs

@@ -1095,6 +1095,7 @@ pub fn eval_script(
                             if sigs_count > keys_count {
                                 return set_error(ScriptError::SigCount);
                             };
+                            assert!(i <= 22);
                             i += 1;
                             let mut isig = i;
                             i += sigs_count;
@@ -1132,11 +1133,7 @@ pub fn eval_script(
                             }
 
                             // Clean up stack of actual arguments
-                            while {
-                                let res = i > 1;
-                                i -= 1;
-                                res
-                            } {
+                            for _ in 1..i {
                                 stack.pop()?;
                             }
 
@@ -1231,7 +1228,7 @@ impl SignatureChecker for CallbackTransactionSignatureChecker<'_> {
     }
 
     fn check_lock_time(&self, lock_time: &ScriptNum) -> bool {
-        // There are two times of nLockTime: lock-by-blockheight
+        // There are two kinds of nLockTime: lock-by-blockheight
         // and lock-by-blocktime, distinguished by whether
         // nLockTime < LOCKTIME_THRESHOLD.
         //
@@ -1317,8 +1314,7 @@ pub fn verify_script(
     // as the non-P2SH evaluation of a P2SH script will obviously not result in
     // a clean stack (the P2SH inputs remain).
     if flags.contains(VerificationFlags::CleanStack) {
-        // Disallow CLEANSTACK without P2SH, as otherwise a switch CLEANSTACK->P2SH+CLEANSTACK
-        // would be possible, which is not a softfork (and P2SH should be one).
+        // Disallow CLEANSTACK without P2SH, because Bitcoin did.
         assert!(flags.contains(VerificationFlags::P2SH));
         if stack.size() != 1 {
             return set_error(ScriptError::CleanStack);

src/lib.rs

@@ -54,7 +54,9 @@ extern "C" fn sighash_callback(
     // SAFETY: `ctx` is a valid `SighashCalculator` constructed in `verify_callback`
     // which forwards it to the `CallbackTransactionSignatureChecker`.
     let callback = unsafe { *(ctx as *const SighashCalculator) };
-    // We don’t need to handle strictness here, because … something
+    // We don’t need to handle strictness here, because it is checked (when necessary) by
+    // `CheckSignatureEncoding` before `CallbackTransactionSignatureChecker` calls this callback.
+    // And we don’t have access to the flags here to determine if it should be checked.
     if let Some(sighash) = HashType::from_bits(hash_type, false)
         .ok()
         .and_then(|ht| callback(script_code_vec, ht))
@@ -209,7 +211,7 @@ impl ZcashScript for CxxRustComparisonInterpreter {
         if rust.map_err(normalize_error) != cxx {
             // probably want to distinguish between
             // - C++ succeeding when Rust fails (bad),
-            // - Rust succeeding when C++ fals (worse), and
+            // - Rust succeeding when C++ fails (worse), and
             // - differing error codes (maybe not bad).
             warn!(
                 "The Rust Zcash Script interpreter had a different result ({:?}) from the C++ one ({:?}).",

src/script.rs

@@ -156,7 +156,7 @@ pub enum Operation {
 
     // expansion
     OP_NOP1 = 0xb0,
-    OP_NOP2 = 0xb1,
+    OP_CHECKLOCKTIMEVERIFY = 0xb1,
     OP_NOP3 = 0xb2,
     OP_NOP4 = 0xb3,
     OP_NOP5 = 0xb4,
@@ -172,8 +172,6 @@ pub enum Operation {
 
 use Operation::*;
 
-pub const OP_CHECKLOCKTIMEVERIFY: Operation = OP_NOP2;
-
 impl From<Opcode> for u8 {
     fn from(value: Opcode) -> Self {
         match value {
@@ -292,15 +290,14 @@ impl ScriptNum {
             // 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 (vch.last().unwrap_or_else(|| unreachable!()) & 0x7F) == 0 {
+            if (vch.last().expect("not empty") & 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
-                // is +-255, which encode to 0xff00 and 0xff80 respectively.
-                // (big-endian).
-                if vch.len() <= 1 {
-                    return Err(ScriptNumError::NegativeZero);
-                } else if (vch[vch.len() - 2] & 0x80) == 0 {
+                // then it would have conflicted with the sign bit if one
+                // fewer byte were used, and so such encodings are minimal.
+                // An example of this is +-255, which have minimal encodings
+                // [0xff, 0x00] and [0xff, 0x80] respectively.
+                if vch.len() <= 1 || (vch[vch.len() - 2] & 0x80) == 0 {
                     return Err(ScriptNumError::NonMinimalEncoding);
                 }
             }
@@ -328,42 +325,35 @@ impl ScriptNum {
         }
 
         if *value == i64::MIN {
-            // The code below is buggy, and produces the "wrong" result for
-            // INT64_MIN. To avoid undefined behavior while attempting to
-            // negate a value of INT64_MIN, we intentionally return the result
-            // that the code below would produce on an x86_64 system.
+            // The code below was based on buggy C++ code, that produced the
+            // "wrong" result for INT64_MIN. In that case we intentionally return
+            // the result that the C++ code as compiled for zcashd (with `-fwrapv`)
+            // originally produced on an x86_64 system.
             return vec![0, 0, 0, 0, 0, 0, 0, 128, 128];
         }
 
         let mut result = Vec::new();
         let neg = *value < 0;
-        let mut absvalue = value.abs();
+        let mut absvalue = value.unsigned_abs();
 
         while absvalue != 0 {
-            result.push(
-                (absvalue & 0xff)
-                    .try_into()
-                    .unwrap_or_else(|_| unreachable!()),
-            );
+            result.push((absvalue & 0xff).try_into().expect("fits in u8"));
             absvalue >>= 8;
         }
 
-        //    - If the most significant byte is >= 0x80 and the value is positive, push a
-        //    new zero-byte to make the significant byte < 0x80 again.
-
-        //    - If the most significant byte is >= 0x80 and the value is negative, push a
-        //    new 0x80 byte that will be popped off when converting to an integral.
-
-        //    - If the most significant byte is < 0x80 and the value is negative, add
-        //    0x80 to it, since it will be subtracted and interpreted as a negative when
-        //    converting to an integral.
+        // - If the most significant byte is >= 0x80 and the value is positive, push a
+        //   new zero-byte to make the significant byte < 0x80 again.
+        // - If the most significant byte is >= 0x80 and the value is negative, push a
+        //   new 0x80 byte that will be popped off when converting to an integral.
+        // - If the most significant byte is < 0x80 and the value is negative, add 0x80
+        //   to it, since it will be subtracted and interpreted as a negative when
+        //   converting to an integral.
 
-        if result.last().map_or(true, |last| last & 0x80 != 0) {
+        let result_back = result.last_mut().expect("not empty");
+        if *result_back & 0x80 != 0 {
             result.push(if neg { 0x80 } else { 0 });
         } else if neg {
-            if let Some(last) = result.last_mut() {
-                *last |= 0x80;
-            }
+            *result_back |= 0x80;
         }
 
         result
@@ -374,14 +364,15 @@ impl ScriptNum {
             None => Ok(0),
             Some(vch_back) => {
                 if *vch == vec![0, 0, 0, 0, 0, 0, 0, 128, 128] {
-                    // On an x86_64 system, the code below would actually decode the buggy
-                    // INT64_MIN encoding correctly. However in this case, it would be
-                    // performing left shifts of a signed type by 64, which has undefined
-                    // behavior.
+                    // Match the behaviour of the C++ code, which special-cased this
+                    // encoding to avoid an undefined shift of a signed type by 64 bits.
                     return Ok(i64::MIN);
                 };
 
-                // Guard against undefined behavior. INT64_MIN is the only allowed 9-byte encoding.
+                // Ensure defined behaviour (in Rust, left shift of `i64` by 64 bits
+                // is an arithmetic overflow that may panic or give an unspecified
+                // result). The above encoding of `i64::MIN` is the only allowed
+                // 9-byte encoding.
                 if vch.len() > 8 {
                     return Err(ScriptNumError::Overflow {
                         max_num_size: 8,
@@ -457,27 +448,23 @@ impl Add for ScriptNum {
     type Output = Self;
 
     fn add(self, other: Self) -> Self {
-        let rhs = other.0;
-        assert!(
-            rhs == 0
-                || (rhs > 0 && self.0 <= i64::MAX - rhs)
-                || (rhs < 0 && self.0 >= i64::MIN - rhs)
-        );
-        Self(self.0 + rhs)
+        Self(
+            self.0
+                .checked_add(other.0)
+                .expect("caller should avoid overflow"),
+        )
     }
 }
 
 impl Sub for ScriptNum {
     type Output = Self;
 
     fn sub(self, other: Self) -> Self {
-        let rhs = other.0;
-        assert!(
-            rhs == 0
-                || (rhs > 0 && self.0 >= i64::MIN + rhs)
-                || (rhs < 0 && self.0 <= i64::MAX + rhs)
-        );
-        Self(self.0 - rhs)
+        Self(
+            self.0
+                .checked_sub(other.0)
+                .expect("caller should avoid underflow"),
+        )
     }
 }
 
@@ -507,7 +494,7 @@ impl Script<'_> {
             });
         }
 
-        // Empty the provided buffer, if any
+        // Empty the provided buffer.
         buffer.truncate(0);
 
         let leading_byte = Opcode::from(script[0]);

src/script_error.rs

@@ -1,6 +1,5 @@
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub enum ScriptNumError {
-    NegativeZero,
     NonMinimalEncoding,
     Overflow { max_num_size: usize, actual: usize },
 }