chore: Expose ckb-vm Error type directly in ScriptError

Previously we were converting ckb-vm Error type into a string, which
makes it harder to inspect into ckb-vm Error for certain
values. However, upon further checking, it seems that ckb's Error type
can preserve enough information, so we don't always have to cast
ckb-vm's Error type into a string.

This commit fixes the code so we are preserving the original Error
type from ckb-vm when we need it. For exsiting tests cases leveraging
ckb_error::assert_error_eq, the code still passes without any
modifications needed.

A test case has also been added here, showcasing how we can
destructing a ckb_error::Error type step by step, resulting in the
inner-most ckb-vm's Error type.

script/src/error.rs

@@ -2,6 +2,7 @@ use crate::types::{ScriptGroup, ScriptGroupType};
 use ckb_error::{prelude::*, Error, ErrorKind};
 use ckb_types::core::{Cycle, ScriptHashType};
 use ckb_types::packed::{Byte32, Script};
+use ckb_vm::Error as VMInternalError;
 use std::{error::Error as StdError, fmt};
 
 /// Script execution error.
@@ -39,9 +40,13 @@ pub enum ScriptError {
     #[error("Invalid VM Version: {0}")]
     InvalidVmVersion(u8),
 
-    /// Known bugs are detected in transaction script outputs
-    #[error("VM Internal Error: {0}")]
-    VMInternalError(String),
+    /// Errors thrown by ckb-vm
+    #[error("VM Internal Error: {0:?}")]
+    VMInternalError(VMInternalError),
+
+    /// Other errors raised in script execution process
+    #[error("Other Error: {0}")]
+    Other(String),
 }
 
 /// Locate the script using the first input index if possible, otherwise the first output index.
@@ -83,6 +88,30 @@ pub struct TransactionScriptError {
     cause: ScriptError,
 }
 
+impl TransactionScriptError {
+    /// Orginating script for the generated error
+    pub fn originating_script(&self) -> &TransactionScriptErrorSource {
+        &self.source
+    }
+
+    /// Actual error generated when verifying script
+    pub fn script_error(&self) -> &ScriptError {
+        &self.cause
+    }
+}
+
+/// It is a delibrate choice here to implement StdError directly, instead of
+/// implementing thiserror::Error on TransactionScriptError. This way, calling
+/// root_cause() on ckb_error::Error would return TransactionScriptError structure,
+/// providing us enough information to inspect on all kinds of errors generated when
+/// verifying a script.
+///
+/// This also means calling source() or cause() from std::error::Error on
+/// TransactionScriptError would return None values. One is expected to cast
+/// a value of `std::error::Error` type(possibly returned from root_cause) into
+/// concrete TransactionScriptError type, then use the defined methods to fetch
+/// originating script, as well as the actual script error. See the unit test defined
+/// at the end of this file for an example.
 impl StdError for TransactionScriptError {}
 
 impl fmt::Display for TransactionScriptError {
@@ -156,3 +185,36 @@ impl From<TransactionScriptError> for Error {
         ErrorKind::Script.because(error)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_downcast_error_to_vm_error() {
+        let vm_error = VMInternalError::ElfParseError("Foo bar baz".to_string());
+        let script_error = ScriptError::VMInternalError(vm_error.clone());
+        let error: Error = script_error.output_type_script(177).into();
+
+        let recovered_transaction_error: TransactionScriptError = error
+            .root_cause()
+            .downcast_ref()
+            .cloned()
+            .expect("downcasting transaction error");
+        assert_eq!(
+            recovered_transaction_error.originating_script(),
+            &TransactionScriptErrorSource::Outputs(177, ScriptGroupType::Type),
+        );
+
+        if let ScriptError::VMInternalError(recovered_vm_error) =
+            recovered_transaction_error.script_error()
+        {
+            assert_eq!(recovered_vm_error, &vm_error);
+        } else {
+            panic!(
+                "Invalid script type: {}",
+                recovered_transaction_error.script_error()
+            );
+        }
+    }
+}

script/src/types.rs

@@ -391,13 +391,13 @@ impl TryFrom<TransactionState> for TransactionSnapshot {
         let mut snaps = Vec::with_capacity(vms.len());
         for mut vm in vms {
             let snapshot = make_snapshot(&mut vm.machine_mut().machine)
-                .map_err(|e| ScriptError::VMInternalError(format!("{e:?}")).unknown_source())?;
+                .map_err(|e| ScriptError::VMInternalError(e).unknown_source())?;
             let cycles = vm.cycles();
             let resume_point = match vm {
                 ResumableMachine::Initial(_) => ResumePoint::Initial,
                 ResumableMachine::Spawn(_, data) => (&data)
                     .try_into()
-                    .map_err(|e| ScriptError::VMInternalError(format!("{e:?}")).unknown_source())?,
+                    .map_err(|e| ScriptError::VMInternalError(e).unknown_source())?,
             };
             snaps.push((snapshot, cycles, resume_point));
         }

script/src/verify.rs

@@ -641,10 +641,8 @@ impl<DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + C
         for (idx, (_hash, group)) in groups.iter().enumerate() {
             // vm should early return invalid cycles
             let remain_cycles = limit_cycles.checked_sub(cycles).ok_or_else(|| {
-                ScriptError::VMInternalError(format!(
-                    "expect invalid cycles {limit_cycles} {cycles}"
-                ))
-                .source(group)
+                ScriptError::Other(format!("expect invalid cycles {limit_cycles} {cycles}"))
+                    .source(group)
             })?;
 
             match self.verify_group_with_chunk(group, remain_cycles, &[]) {
@@ -690,7 +688,7 @@ impl<DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + C
         let mut current_used = 0;
 
         let (_hash, current_group) = self.groups().nth(snap.current).ok_or_else(|| {
-            ScriptError::VMInternalError(format!("snapshot group missing {:?}", snap.current))
+            ScriptError::Other(format!("snapshot group missing {:?}", snap.current))
                 .unknown_source()
         })?;
 
@@ -719,10 +717,8 @@ impl<DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + C
         let skip = snap.current + 1;
         for (idx, (_hash, group)) in self.groups().enumerate().skip(skip) {
             let remain_cycles = limit_cycles.checked_sub(current_used).ok_or_else(|| {
-                ScriptError::VMInternalError(format!(
-                    "expect invalid cycles {limit_cycles} {cycles}"
-                ))
-                .source(group)
+                ScriptError::Other(format!("expect invalid cycles {limit_cycles} {cycles}"))
+                    .source(group)
             })?;
 
             match self.verify_group_with_chunk(group, remain_cycles, &[]) {
@@ -776,8 +772,7 @@ impl<DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + C
         let mut cycles = current_cycles;
 
         let (_hash, current_group) = self.groups().nth(current).ok_or_else(|| {
-            ScriptError::VMInternalError(format!("snapshot group missing {current:?}"))
-                .unknown_source()
+            ScriptError::Other(format!("snapshot group missing {current:?}")).unknown_source()
         })?;
 
         let resumed_script_result = if vms.is_empty() {
@@ -805,10 +800,8 @@ impl<DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + C
 
         for (idx, (_hash, group)) in self.groups().enumerate().skip(current + 1) {
             let remain_cycles = limit_cycles.checked_sub(current_used).ok_or_else(|| {
-                ScriptError::VMInternalError(format!(
-                    "expect invalid cycles {limit_cycles} {cycles}"
-                ))
-                .source(group)
+                ScriptError::Other(format!("expect invalid cycles {limit_cycles} {cycles}"))
+                    .source(group)
             })?;
 
             match self.verify_group_with_chunk(group, remain_cycles, &[]) {
@@ -848,7 +841,7 @@ impl<DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + C
         let mut cycles = snap.current_cycles;
 
         let (_hash, current_group) = self.groups().nth(snap.current).ok_or_else(|| {
-            ScriptError::VMInternalError(format!("snapshot group missing {:?}", snap.current))
+            ScriptError::Other(format!("snapshot group missing {:?}", snap.current))
                 .unknown_source()
         })?;
 
@@ -878,7 +871,7 @@ impl<DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + C
 
         for (_hash, group) in self.groups().skip(snap.current + 1) {
             let remain_cycles = max_cycles.checked_sub(cycles).ok_or_else(|| {
-                ScriptError::VMInternalError(format!("expect invalid cycles {max_cycles} {cycles}"))
+                ScriptError::Other(format!("expect invalid cycles {max_cycles} {cycles}"))
                     .source(group)
             })?;
 
@@ -1032,7 +1025,7 @@ impl<DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + C
 
         let map_vm_internal_error = |error: VMInternalError| match error {
             VMInternalError::CyclesExceeded => ScriptError::ExceededMaximumCycles(max_cycles),
-            _ => ScriptError::VMInternalError(format!("{error:?}")),
+            _ => ScriptError::VMInternalError(error),
         };
 
         let bytes = machine
@@ -1067,7 +1060,7 @@ impl<DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + C
 
         let map_vm_internal_error = |error: VMInternalError| match error {
             VMInternalError::CyclesExceeded => ScriptError::ExceededMaximumCycles(max_cycles),
-            _ => ScriptError::VMInternalError(format!("{error:?}")),
+            _ => ScriptError::VMInternalError(error),
         };
 
         let machines = if !snaps.is_empty() {
@@ -1157,7 +1150,7 @@ impl<DL: CellDataProvider + HeaderProvider + ExtensionProvider + Send + Sync + C
             machine
                 .machine
                 .add_cycles_no_checking(program_bytes_cycles)
-                .map_err(|e| ScriptError::VMInternalError(format!("{e:?}")))?;
+                .map_err(ScriptError::VMInternalError)?;
             vec![ResumableMachine::initial(machine)]
         };
 
@@ -1175,14 +1168,14 @@ fn run_vms(
     let (mut exit_code, mut cycles, mut spawn_data) = (0, 0, None);
 
     if machines.is_empty() {
-        return Err(ScriptError::VMInternalError(
+        return Err(ScriptError::Other(
             "To resume VMs, at least one VM must be available!".to_string(),
         ));
     }
 
     let map_vm_internal_error = |error: VMInternalError| match error {
         VMInternalError::CyclesExceeded => ScriptError::ExceededMaximumCycles(max_cycles),
-        _ => ScriptError::VMInternalError(format!("{error:?}")),
+        _ => ScriptError::VMInternalError(error),
     };
 
     while let Some(mut machine) = machines.pop() {
@@ -1210,7 +1203,7 @@ fn run_vms(
                 VMInternalError::CyclesExceeded => {
                     let mut new_suspended_machines: Vec<_> = {
                         let mut context = context.lock().map_err(|e| {
-                            ScriptError::VMInternalError(format!("Failed to acquire lock: {}", e))
+                            ScriptError::Other(format!("Failed to acquire lock: {}", e))
                         })?;
                         context.suspended_machines.drain(..).collect()
                     };
@@ -1221,7 +1214,7 @@ fn run_vms(
                     machines.append(&mut new_suspended_machines);
                     return Ok(ChunkState::suspended(machines, Arc::clone(context)));
                 }
-                _ => return Err(ScriptError::VMInternalError(format!("{error:?}"))),
+                _ => return Err(ScriptError::VMInternalError(error)),
             },
         };
     }

script/src/verify/tests/ckb_latest/features_since_v2019.rs

@@ -1063,7 +1063,7 @@ fn load_code_to_stack_then_reuse_case1_load_and_write() {
     let result = verifier.verify_without_limit(script_version, &rtx);
     assert!(result.is_err());
     let vm_error = VmError::MemWriteOnExecutablePage;
-    let script_error = ScriptError::VMInternalError(format!("{vm_error:?}"));
+    let script_error = ScriptError::VMInternalError(vm_error);
     assert_error_eq!(result.unwrap_err(), script_error.input_lock_script(0));
 }
 

script/src/verify/tests/ckb_latest/features_since_v2021.rs

@@ -55,7 +55,7 @@ fn test_hint_instructions() {
             pc: 65_656,
             instruction: 36_906,
         };
-        let script_error = ScriptError::VMInternalError(format!("{vm_error:?}"));
+        let script_error = ScriptError::VMInternalError(vm_error);
         assert_error_eq!(result.unwrap_err(), script_error.input_lock_script(0));
     }
 }
@@ -105,7 +105,7 @@ fn test_b_extension() {
             pc: 0x10182,
             instruction: 0x60291913,
         };
-        let script_error = ScriptError::VMInternalError(format!("{vm_error:?}"));
+        let script_error = ScriptError::VMInternalError(vm_error);
         assert_error_eq!(result.unwrap_err(), script_error.input_lock_script(0));
     }
 }
@@ -1018,7 +1018,7 @@ fn load_code_into_global() {
     assert_eq!(result.is_ok(), script_version >= ScriptVersion::V1,);
     if script_version < ScriptVersion::V1 {
         let vm_error = VmError::MemWriteOnFreezedPage;
-        let script_error = ScriptError::VMInternalError(format!("{vm_error:?}"));
+        let script_error = ScriptError::VMInternalError(vm_error);
         assert_error_eq!(result.unwrap_err(), script_error.input_lock_script(0));
     }
 }

tx-pool/src/chunk_process.rs

@@ -357,7 +357,7 @@ fn exceeded_maximum_cycles_error<
         .nth(current)
         .map(|(_hash, group)| ScriptError::ExceededMaximumCycles(max_cycles).source(group))
         .unwrap_or_else(|| {
-            ScriptError::VMInternalError(format!("suspended state group missing {current:?}"))
+            ScriptError::Other(format!("suspended state group missing {current:?}"))
                 .unknown_source()
         })
         .into()