From 4c34b3ed40c3e426b4843e1253cd93ead5b14da2 Mon Sep 17 00:00:00 2001
From: Greg Pfeil <greg@technomadic.org>
Date: Thu, 17 Oct 2024 12:39:38 -0600
Subject: [PATCH] Preserve richer errors on the Rust side

For now, the underlying errors are discarded when comparing against the
C++ results, but there are corresponding changes on the C++ side in a
separate branch.
---
 fuzz/fuzz_targets/compare.rs |  3 ++-
 src/lib.rs                   | 38 ++++++++++++++++++++++++++----------
 src/zcash_script.rs          |  8 ++++++--
 3 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/fuzz/fuzz_targets/compare.rs b/fuzz/fuzz_targets/compare.rs
index ec80c550a..1fd8875ed 100644
--- a/fuzz/fuzz_targets/compare.rs
+++ b/fuzz/fuzz_targets/compare.rs
@@ -20,5 +20,6 @@ fuzz_target!(|tup: (i64, bool, &[u8], &[u8], u32)| {
         sig,
         testing::repair_flags(VerificationFlags::from_bits_truncate(flags)),
     );
-    assert_eq!(ret.0, ret.1);
+    assert_eq!(ret.0, ret.1.clone().map_err(testing::normalize_error),
+               "original Rust result: {:?}", ret.1);
 });
diff --git a/src/lib.rs b/src/lib.rs
index 7d59ed47e..c2d24b5cc 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -10,7 +10,7 @@ mod cxx;
 mod external;
 mod interpreter;
 mod script;
-mod script_error;
+pub mod script_error;
 mod zcash_script;
 
 use std::os::raw::{c_int, c_uint, c_void};
@@ -26,9 +26,9 @@ pub enum Cxx {}
 
 impl From<zcash_script_error_t> for Error {
     #[allow(non_upper_case_globals)]
-    fn from(err_code: zcash_script_error_t) -> Error {
+    fn from(err_code: zcash_script_error_t) -> Self {
         match err_code {
-            zcash_script_error_t_zcash_script_ERR_OK => Error::Ok,
+            zcash_script_error_t_zcash_script_ERR_OK => Error::Ok(None),
             zcash_script_error_t_zcash_script_ERR_VERIFY_SCRIPT => Error::VerifyScript,
             unknown => Error::Unknown(unknown.into()),
         }
@@ -207,6 +207,14 @@ impl<T: ZcashScript, U: ZcashScript> ZcashScript for (T, U) {
 pub mod testing {
     use super::*;
 
+    /// 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),
+            _ => err,
+        }
+    }
+
     /// Ensures that flags represent a supported state. This avoids crashes in the C++ code, which
     /// break various tests.
     pub fn repair_flags(flags: VerificationFlags) -> VerificationFlags {
@@ -271,7 +279,7 @@ mod tests {
             flags,
         );
 
-        assert_eq!(ret.0, ret.1);
+        assert_eq!(ret.0, ret.1.map_err(normalize_error));
         assert!(ret.0.is_ok());
     }
 
@@ -292,8 +300,12 @@ mod tests {
             flags,
         );
 
-        assert_eq!(ret.0, ret.1);
-        assert_eq!(ret.0, Err(Error::Ok));
+        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)))
+        );
     }
 
     #[test]
@@ -313,8 +325,12 @@ mod tests {
             flags,
         );
 
-        assert_eq!(ret.0, ret.1);
-        assert_eq!(ret.0, Err(Error::Ok));
+        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)))
+        );
     }
 
     proptest! {
@@ -340,7 +356,8 @@ mod tests {
                 &sig[..],
                 repair_flags(VerificationFlags::from_bits_truncate(flags)),
             );
-            prop_assert_eq!(ret.0, ret.1);
+            prop_assert_eq!(ret.0, ret.1.map_err(normalize_error),
+                            "original Rust result: {:?}", ret.1);
         }
 
         /// Similar to `test_arbitrary_scripts`, but ensures the `sig` only contains pushes.
@@ -363,7 +380,8 @@ mod tests {
                 repair_flags(VerificationFlags::from_bits_truncate(flags))
                     | VerificationFlags::SigPushOnly,
             );
-            prop_assert_eq!(ret.0, ret.1);
+            prop_assert_eq!(ret.0, ret.1.map_err(normalize_error),
+                            "original Rust result: {:?}", ret.1);
         }
     }
 }
diff --git a/src/zcash_script.rs b/src/zcash_script.rs
index 9c24beb76..a18574d27 100644
--- a/src/zcash_script.rs
+++ b/src/zcash_script.rs
@@ -2,6 +2,7 @@ use std::num::TryFromIntError;
 
 use super::interpreter::*;
 use super::script::*;
+use super::script_error::*;
 
 /// This maps to `zcash_script_error_t`, but most of those cases aren’t used any more. This only
 /// replicates the still-used cases, and then an `Unknown` bucket for anything else that might
@@ -10,7 +11,10 @@ use super::script::*;
 #[repr(u32)]
 pub enum Error {
     /// Any failure that results in the script being invalid.
-    Ok = 0,
+    ///
+    /// __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>) = 0,
     /// An exception was caught.
     VerifyScript = 7,
     /// The script size can’t fit in a `u32`, as required by the C++ code.
@@ -84,6 +88,6 @@ impl ZcashScript for Rust {
                 is_final,
             },
         )
-        .map_err(|_| Error::Ok)
+        .map_err(|e| Error::Ok(Some(e)))
     }
 }