Expose ScriptError on C++ side (#195)

**This changes the C++ implementation.**

Provides richer errors, which also gives more precision when comparing
against the Rust impl.

This also removes the (now unused) `zcash_script_error_t`. The only case
other than `zcash_script_ERR_OK` that was still in use was
`zcash_script_ERR_VERIFY_SCRIPT`, so that case has been added to
`ScriptError`.

This avoids changing the Rust API, but potentially `Error` and
`ScriptError` on the Rust side could be collapsed into one `enum`. It
would just be a breaking change.

depend/zcash/src/script/script_error.h

@@ -53,6 +53,8 @@ typedef enum ScriptError_t
     /* softfork safeness */
     SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS,
 
+    SCRIPT_ERR_VERIFY_SCRIPT,
+
     SCRIPT_ERR_ERROR_COUNT
 } ScriptError;
 

depend/zcash/src/script/zcash_script.cpp

@@ -7,10 +7,11 @@
 #include "zcash_script.h"
 
 #include "script/interpreter.h"
+#include "script/script_error.h"
 #include "version.h"
 
 namespace {
-inline int set_error(zcash_script_error* ret, zcash_script_error serror)
+inline int set_error(ScriptError* ret, ScriptError serror)
 {
     if (ret)
         *ret = serror;
@@ -42,12 +43,10 @@ int zcash_script_verify_callback(
     const unsigned char* scriptSig,
     unsigned int scriptSigLen,
     unsigned int flags,
-    zcash_script_error* err)
+    ScriptError* script_err)
 {
     try {
-        set_error(err, zcash_script_ERR_OK);
         CScriptNum nLockTimeNum = CScriptNum(nLockTime);
-        ScriptError script_err = SCRIPT_ERR_OK;
         return VerifyScript(
             CScript(scriptSig, scriptSig + scriptSigLen),
             CScript(scriptPubKey, scriptPubKey + scriptPubKeyLen),
@@ -56,8 +55,8 @@ int zcash_script_verify_callback(
             // consensusBranchId is not longer used with the callback API; the argument
             // was left there to minimize changes to interpreter.cpp
             0,
-            &script_err);
+            script_err);
     } catch (const std::exception&) {
-        return set_error(err, zcash_script_ERR_VERIFY_SCRIPT);
+        return set_error(script_err, SCRIPT_ERR_VERIFY_SCRIPT);
     }
 }

depend/zcash/src/script/zcash_script.h

@@ -8,6 +8,7 @@
 #define ZCASH_SCRIPT_ZCASH_SCRIPT_H
 
 #include <stdint.h>
+#include "script_error.h"
 
 #if defined(BUILD_BITCOIN_INTERNAL) && defined(HAVE_CONFIG_H)
 #include "config/bitcoin-config.h"
@@ -36,19 +37,6 @@ extern "C" {
 
 #define ZCASH_SCRIPT_API_VER 4
 
-typedef enum zcash_script_error_t
-{
-    zcash_script_ERR_OK = 0,
-    zcash_script_ERR_TX_INDEX,
-    zcash_script_ERR_TX_SIZE_MISMATCH,
-    zcash_script_ERR_TX_DESERIALIZE,
-    // Defined since API version 3.
-    zcash_script_ERR_TX_VERSION,
-    zcash_script_ERR_ALL_PREV_OUTPUTS_SIZE_MISMATCH,
-    zcash_script_ERR_ALL_PREV_OUTPUTS_DESERIALIZE,
-    zcash_script_ERR_VERIFY_SCRIPT,
-} zcash_script_error;
-
 /** Script verification flags */
 enum
 {
@@ -93,8 +81,7 @@ EXPORT_SYMBOL unsigned int zcash_script_legacy_sigop_count_script(
 /// - flags: the script verification flags to use.
 /// - err: if not NULL, err will contain an error/success code for the operation.
 ///
-/// Note that script verification failure is indicated by err being set to
-/// zcash_script_ERR_OK and a return value of 0.
+/// Note that script verification failure is indicated by a return value of 0.
 EXPORT_SYMBOL int zcash_script_verify_callback(
     const void* ctx,
     void (*sighash)(unsigned char* sighash, unsigned int sighashLen, const void* ctx, const unsigned char* scriptCode, unsigned int scriptCodeLen, int hashType),
@@ -105,7 +92,7 @@ EXPORT_SYMBOL int zcash_script_verify_callback(
     const unsigned char* scriptSig,
     unsigned int scriptSigLen,
     unsigned int flags,
-    zcash_script_error* err);
+    ScriptError* err);
 
 #ifdef __cplusplus
 } // extern "C"

src/cxx.rs

@@ -15,7 +15,6 @@ include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
 mod tests {
     use std::ffi::{c_int, c_uint, c_void};
 
-    pub use super::zcash_script_error_t;
     use hex::FromHex;
 
     lazy_static::lazy_static! {

src/lib.rs

@@ -2,6 +2,7 @@
 
 #![doc(html_logo_url = "https://www.zfnd.org/images/zebra-icon.png")]
 #![doc(html_root_url = "https://docs.rs/zcash_script/0.3.0")]
+#![allow(non_snake_case)]
 #![allow(unsafe_code)]
 #[macro_use]
 extern crate enum_primitive;
@@ -24,12 +25,85 @@ pub use zcash_script::*;
 /// A tag to indicate that the C++ implementation of zcash_script should be used.
 pub enum CxxInterpreter {}
 
-impl From<zcash_script_error_t> for Error {
+impl From<cxx::ScriptError> for Error {
     #[allow(non_upper_case_globals)]
-    fn from(err_code: zcash_script_error_t) -> Self {
+    fn from(err_code: cxx::ScriptError) -> Self {
         match err_code {
-            zcash_script_error_t_zcash_script_ERR_OK => Error::Ok(None),
-            zcash_script_error_t_zcash_script_ERR_VERIFY_SCRIPT => Error::VerifyScript,
+            ScriptError_t_SCRIPT_ERR_OK => Error::Ok(script_error::ScriptError::Ok),
+            ScriptError_t_SCRIPT_ERR_UNKNOWN_ERROR => {
+                Error::Ok(script_error::ScriptError::UnknownError)
+            }
+            ScriptError_t_SCRIPT_ERR_EVAL_FALSE => Error::Ok(script_error::ScriptError::EvalFalse),
+            ScriptError_t_SCRIPT_ERR_OP_RETURN => Error::Ok(script_error::ScriptError::OpReturn),
+
+            ScriptError_t_SCRIPT_ERR_SCRIPT_SIZE => {
+                Error::Ok(script_error::ScriptError::ScriptSize)
+            }
+            ScriptError_t_SCRIPT_ERR_PUSH_SIZE => Error::Ok(script_error::ScriptError::PushSize),
+            ScriptError_t_SCRIPT_ERR_OP_COUNT => Error::Ok(script_error::ScriptError::OpCount),
+            ScriptError_t_SCRIPT_ERR_STACK_SIZE => Error::Ok(script_error::ScriptError::StackSize),
+            ScriptError_t_SCRIPT_ERR_SIG_COUNT => Error::Ok(script_error::ScriptError::SigCount),
+            ScriptError_t_SCRIPT_ERR_PUBKEY_COUNT => {
+                Error::Ok(script_error::ScriptError::PubKeyCount)
+            }
+
+            ScriptError_t_SCRIPT_ERR_VERIFY => Error::Ok(script_error::ScriptError::Verify),
+            ScriptError_t_SCRIPT_ERR_EQUALVERIFY => {
+                Error::Ok(script_error::ScriptError::EqualVerify)
+            }
+            ScriptError_t_SCRIPT_ERR_CHECKMULTISIGVERIFY => {
+                Error::Ok(script_error::ScriptError::CheckMultisigVerify)
+            }
+            ScriptError_t_SCRIPT_ERR_CHECKSIGVERIFY => {
+                Error::Ok(script_error::ScriptError::CheckSigVerify)
+            }
+            ScriptError_t_SCRIPT_ERR_NUMEQUALVERIFY => {
+                Error::Ok(script_error::ScriptError::NumEqualVerify)
+            }
+
+            ScriptError_t_SCRIPT_ERR_BAD_OPCODE => Error::Ok(script_error::ScriptError::BadOpcode),
+            ScriptError_t_SCRIPT_ERR_DISABLED_OPCODE => {
+                Error::Ok(script_error::ScriptError::DisabledOpcode)
+            }
+            ScriptError_t_SCRIPT_ERR_INVALID_STACK_OPERATION => {
+                Error::Ok(script_error::ScriptError::InvalidStackOperation)
+            }
+            ScriptError_t_SCRIPT_ERR_INVALID_ALTSTACK_OPERATION => {
+                Error::Ok(script_error::ScriptError::InvalidAltstackOperation)
+            }
+            ScriptError_t_SCRIPT_ERR_UNBALANCED_CONDITIONAL => {
+                Error::Ok(script_error::ScriptError::UnbalancedConditional)
+            }
+
+            ScriptError_t_SCRIPT_ERR_NEGATIVE_LOCKTIME => {
+                Error::Ok(script_error::ScriptError::NegativeLockTime)
+            }
+            ScriptError_t_SCRIPT_ERR_UNSATISFIED_LOCKTIME => {
+                Error::Ok(script_error::ScriptError::UnsatisfiedLockTime)
+            }
+
+            ScriptError_t_SCRIPT_ERR_SIG_HASHTYPE => {
+                Error::Ok(script_error::ScriptError::SigHashType)
+            }
+            ScriptError_t_SCRIPT_ERR_SIG_DER => Error::Ok(script_error::ScriptError::SigDER),
+            ScriptError_t_SCRIPT_ERR_MINIMALDATA => {
+                Error::Ok(script_error::ScriptError::MinimalData)
+            }
+            ScriptError_t_SCRIPT_ERR_SIG_PUSHONLY => {
+                Error::Ok(script_error::ScriptError::SigPushOnly)
+            }
+            ScriptError_t_SCRIPT_ERR_SIG_HIGH_S => Error::Ok(script_error::ScriptError::SigHighS),
+            ScriptError_t_SCRIPT_ERR_SIG_NULLDUMMY => {
+                Error::Ok(script_error::ScriptError::SigNullDummy)
+            }
+            ScriptError_t_SCRIPT_ERR_PUBKEYTYPE => Error::Ok(script_error::ScriptError::PubKeyType),
+            ScriptError_t_SCRIPT_ERR_CLEANSTACK => Error::Ok(script_error::ScriptError::CleanStack),
+
+            ScriptError_t_SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS => {
+                Error::Ok(script_error::ScriptError::DiscourageUpgradableNOPs)
+            }
+
+            ScriptError_t_SCRIPT_ERR_VERIFY_SCRIPT => Error::VerifyScript,
             unknown => Error::Unknown(unknown.into()),
         }
     }
@@ -166,7 +240,11 @@ pub fn check_verify_callback<T: ZcashScript, U: ZcashScript>(
 /// Convert errors that don’t exist in the C++ code into the cases that do.
 pub fn normalize_error(err: Error) -> Error {
     match err {
-        Error::Ok(Some(_)) => Error::Ok(None),
+        Error::Ok(serr) => Error::Ok(match serr {
+            script_error::ScriptError::ReadError { .. } => script_error::ScriptError::BadOpcode,
+            script_error::ScriptError::ScriptNumError(_) => script_error::ScriptError::UnknownError,
+            _ => serr,
+        }),
         _ => err,
     }
 }
@@ -310,11 +388,7 @@ mod tests {
         );
 
         assert_eq!(ret.0, ret.1.map_err(normalize_error));
-        // Checks the Rust result, because we have more information on the Rust side.
-        assert_eq!(
-            ret.1,
-            Err(Error::Ok(Some(script_error::ScriptError::EvalFalse)))
-        );
+        assert_eq!(ret.0, Err(Error::Ok(script_error::ScriptError::EvalFalse)));
     }
 
     #[test]
@@ -335,20 +409,14 @@ mod tests {
         );
 
         assert_eq!(ret.0, ret.1.map_err(normalize_error));
-        // Checks the Rust result, because we have more information on the Rust side.
-        assert_eq!(
-            ret.1,
-            Err(Error::Ok(Some(script_error::ScriptError::EvalFalse)))
-        );
+        assert_eq!(ret.0, Err(Error::Ok(script_error::ScriptError::EvalFalse)));
     }
 
     proptest! {
         #![proptest_config(ProptestConfig {
             cases: 20_000, .. ProptestConfig::default()
         })]
 
-        /// This test is very shallow, because we have only `()` for success and most errors have
-        /// been collapsed to `Error::Ok`. A deeper comparison, requires changes to the C++ code.
         #[test]
         fn test_arbitrary_scripts(
             lock_time in prop::num::u32::ANY,

src/script_error.rs

@@ -8,8 +8,8 @@ pub enum ScriptNumError {
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 #[repr(i32)]
 pub enum ScriptError {
-    // Ok = 0,
-    UnknownError = 1,
+    Ok = 0, // Unused (except in converting the C++ error to Rust)
+    UnknownError,
     EvalFalse,
     OpReturn,
 
@@ -44,8 +44,8 @@ pub enum ScriptError {
     SigDER,
     MinimalData,
     SigPushOnly,
-    // SigHighS,
-    SigNullDummy = 27,
+    SigHighS, // Unused (except in converting the C++ error to Rust)
+    SigNullDummy,
     PubKeyType,
     CleanStack,
 

src/zcash_script.rs

@@ -10,10 +10,7 @@ use super::script_error::*;
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 pub enum Error {
     /// Any failure that results in the script being invalid.
-    ///
-    /// __NB__: This is in `Option` because this type is used by both the C++ and Rust
-    ///         implementations, but the C++ impl doesn’t yet expose the original error.
-    Ok(Option<ScriptError>),
+    Ok(ScriptError),
     /// An exception was caught.
     VerifyScript,
     /// The script size can’t fit in a `u32`, as required by the C++ code.
@@ -87,6 +84,6 @@ impl ZcashScript for RustInterpreter {
                 is_final,
             },
         )
-        .map_err(|e| Error::Ok(Some(e)))
+        .map_err(Error::Ok)
     }
 }