From 09ac96be908697a5d1a65d9a1c75067b973ea1b7 Mon Sep 17 00:00:00 2001 From: Greg Pfeil 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 | 50 ++++++++++++++++++++++++------------ src/zcash_script.rs | 17 ++++++------ 3 files changed, 45 insertions(+), 25 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..615ff419e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -24,11 +24,11 @@ pub use zcash_script::*; /// A tag to indicate that the C++ implementation of zcash_script should be used. pub enum Cxx {} -impl From for Error { +impl<'a> From for Error<'a> { #[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()), } @@ -64,14 +64,14 @@ extern "C" fn sighash_callback( /// This steals a bit of the wrapper code from zebra_script, to provide the API that they want. impl ZcashScript for Cxx { - fn verify_callback( + fn verify_callback<'a>( sighash: SighashCalculator, lock_time: i64, is_final: bool, script_pub_key: &[u8], signature_script: &[u8], flags: VerificationFlags, - ) -> Result<(), Error> { + ) -> Result<(), Error<'a>> { let mut err = 0; // SAFETY: The `script` fields are created from a valid Rust `slice`. @@ -131,14 +131,14 @@ fn check_legacy_sigop_count_script( /// Runs both the C++ and Rust implementations of `ZcashScript::verify_callback` and returns both /// results. This is more useful for testing than the impl that logs a warning if the results differ /// and always returns the C++ result. -pub fn check_verify_callback( +pub fn check_verify_callback<'a, T: ZcashScript, U: ZcashScript>( sighash: SighashCalculator, lock_time: i64, is_final: bool, script_pub_key: &[u8], script_sig: &[u8], flags: VerificationFlags, -) -> (Result<(), Error>, Result<(), Error>) { +) -> (Result<(), Error<'a>>, Result<(), Error<'a>>) { ( T::verify_callback( sighash, @@ -173,14 +173,14 @@ impl ZcashScript for (T, U) { cxx } - fn verify_callback( + fn verify_callback<'a>( sighash: SighashCalculator, lock_time: i64, is_final: bool, script_pub_key: &[u8], script_sig: &[u8], flags: VerificationFlags, - ) -> Result<(), Error> { + ) -> Result<(), Error<'a>> { let (cxx, rust) = check_verify_callback::( sighash, lock_time, @@ -207,6 +207,14 @@ impl 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.clone().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.clone().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.clone().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.clone().map_err(normalize_error), + "original Rust result: {:?}", ret.1); } } } diff --git a/src/zcash_script.rs b/src/zcash_script.rs index 4a19e02a9..b075eb14f 100644 --- a/src/zcash_script.rs +++ b/src/zcash_script.rs @@ -2,15 +2,16 @@ 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 /// happen. -#[derive(Copy, Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] #[repr(u32)] -pub enum Error { +pub enum Error<'a> { /// Any failure that results in the script being invalid. - Ok = 0, + Ok(Option>) = 0, /// An exception was caught. VerifyScript = 7, /// The script size can’t fit in a `u32`, as required by the C++ code. @@ -40,14 +41,14 @@ pub trait ZcashScript { /// - flags: the script verification flags to use. /// /// Note that script verification failure is indicated by `Err(Error::Ok)`. - fn verify_callback( + fn verify_callback<'a>( sighash_callback: SighashCalculator, lock_time: i64, is_final: bool, script_pub_key: &[u8], script_sig: &[u8], flags: VerificationFlags, - ) -> Result<(), Error>; + ) -> Result<(), Error<'a>>; /// Returns the number of transparent signature operations in the input or /// output script pointed to by script. @@ -64,14 +65,14 @@ impl ZcashScript for Rust { Ok(cscript.get_sig_op_count(false)) } - fn verify_callback( + fn verify_callback<'a>( sighash: SighashCalculator, lock_time: i64, is_final: bool, script_pub_key: &[u8], script_sig: &[u8], flags: VerificationFlags, - ) -> Result<(), Error> { + ) -> Result<(), Error<'a>> { let lock_time_num = ScriptNum(lock_time); verify_script( &Script(script_sig), @@ -83,6 +84,6 @@ impl ZcashScript for Rust { is_final, }, ) - .map_err(|_| Error::Ok) + .map_err(|e| Error::Ok(Some(e))) } }