Merge #2164

2164: refactor: Improve script error r=quake,zhangsoledad a=doitian

* Add information to help locating which script produces the error, including the first input or output index, whether it is lock or type script.
* Add extra info:
    * Add the upper limit in ExceededMaximumCycles
    * Add a link to the system script exit codes table for ValidationFailure

Co-authored-by: ian <ian@nervos.org>

script/src/error.rs

@@ -1,5 +1,8 @@
+use crate::types::{ScriptGroup, ScriptGroupType};
 use ckb_error::{Error, ErrorKind};
+use ckb_types::core::Cycle;
 use failure::Fail;
+use std::fmt;
 
 #[derive(Fail, Debug, PartialEq, Eq, Clone)]
 pub enum ScriptError {
@@ -8,24 +11,109 @@ pub enum ScriptError {
     InvalidCodeHash,
 
     /// The script consumes too much cycles
-    #[fail(display = "ExceededMaximumCycles")]
-    ExceededMaximumCycles,
+    #[fail(display = "ExceededMaximumCycles: expect cycles <= {}", _0)]
+    ExceededMaximumCycles(Cycle),
 
     /// `script.type_hash` hits multiple cells with different data
     #[fail(display = "MultipleMatches")]
     MultipleMatches,
 
     /// Non-zero exit code returns by script
-    #[fail(display = "ValidationFailure({})", _0)]
+    #[fail(
+        display = "ValidationFailure({}): the exit code is per script specific, for system scripts, please check https://github.com/nervosnetwork/ckb-system-scripts/wiki/Error-codes",
+        _0
+    )]
     ValidationFailure(i8),
 
     /// Known bugs are detected in transaction script outputs
     #[fail(display = "Known bugs encountered in output {}: {}", _1, _0)]
     EncounteredKnownBugs(String, usize),
+
+    /// Known bugs are detected in transaction script outputs
+    #[fail(display = "VM Internal Error: {}", _0)]
+    VMInternalError(String),
+}
+
+/// Locate the script using the first input index if possible, otherwise the first output index.
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub enum TransactionScriptErrorSource {
+    Inputs(usize, ScriptGroupType),
+    Outputs(usize, ScriptGroupType),
+    Unknown,
+}
+
+impl TransactionScriptErrorSource {
+    fn from_script_group(script_group: &ScriptGroup) -> Self {
+        if let Some(n) = script_group.input_indices.first() {
+            TransactionScriptErrorSource::Inputs(*n, script_group.group_type)
+        } else if let Some(n) = script_group.output_indices.first() {
+            TransactionScriptErrorSource::Outputs(*n, script_group.group_type)
+        } else {
+            TransactionScriptErrorSource::Unknown
+        }
+    }
+}
+
+impl fmt::Display for TransactionScriptErrorSource {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            TransactionScriptErrorSource::Inputs(n, field) => write!(f, "Inputs[{}].{}", n, field),
+            TransactionScriptErrorSource::Outputs(n, field) => {
+                write!(f, "Outputs[{}].{}", n, field)
+            }
+            TransactionScriptErrorSource::Unknown => write!(f, "Unknown"),
+        }
+    }
+}
+
+#[derive(Fail, Debug, PartialEq, Eq, Clone)]
+pub struct TransactionScriptError {
+    source: TransactionScriptErrorSource,
+    cause: ScriptError,
+}
+
+impl fmt::Display for TransactionScriptError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(
+            f,
+            "TransactionScriptError {{ source: {}, cause: {} }}",
+            self.source, self.cause
+        )
+    }
+}
+
+impl ScriptError {
+    pub(crate) fn source(self, script_group: &ScriptGroup) -> TransactionScriptError {
+        TransactionScriptError {
+            source: TransactionScriptErrorSource::from_script_group(script_group),
+            cause: self,
+        }
+    }
+
+    pub fn input_lock_script(self, index: usize) -> TransactionScriptError {
+        TransactionScriptError {
+            source: TransactionScriptErrorSource::Inputs(index, ScriptGroupType::Lock),
+            cause: self,
+        }
+    }
+
+    pub fn input_type_script(self, index: usize) -> TransactionScriptError {
+        TransactionScriptError {
+            source: TransactionScriptErrorSource::Inputs(index, ScriptGroupType::Type),
+            cause: self,
+        }
+    }
+
+    pub fn output_type_script(self, index: usize) -> TransactionScriptError {
+        TransactionScriptError {
+            source: TransactionScriptErrorSource::Outputs(index, ScriptGroupType::Type),
+            cause: self,
+        }
+    }
 }
 
-impl From<ScriptError> for Error {
-    fn from(error: ScriptError) -> Self {
+impl From<TransactionScriptError> for Error {
+    fn from(error: TransactionScriptError) -> Self {
         error.context(ErrorKind::Script).into()
     }
 }

script/src/ill_transaction_checker.rs

@@ -1,6 +1,5 @@
 use crate::ScriptError;
 use byteorder::{ByteOrder, LittleEndian};
-use ckb_error::Error;
 use ckb_types::core::TransactionView;
 use ckb_vm::{
     instructions::{extract_opcode, i, m, rvc, Instruction, Itype, Stype},
@@ -20,7 +19,7 @@ impl<'a> IllTransactionChecker<'a> {
         IllTransactionChecker { tx }
     }
 
-    pub fn check(&self) -> Result<(), Error> {
+    pub fn check(&self) -> Result<(), ScriptError> {
         for (i, data) in self.tx.outputs_data().into_iter().enumerate() {
             IllScriptChecker::new(&data.raw_data(), i).check()?;
         }
@@ -38,7 +37,7 @@ impl<'a> IllScriptChecker<'a> {
         IllScriptChecker { data, index }
     }
 
-    pub fn check(&self) -> Result<(), Error> {
+    pub fn check(&self) -> Result<(), ScriptError> {
         if self.data.is_empty() {
             return Ok(());
         }
@@ -63,8 +62,7 @@ impl<'a> IllScriptChecker<'a> {
                                         return Err(ScriptError::EncounteredKnownBugs(
                                             CKB_VM_ISSUE_92.to_string(),
                                             self.index,
-                                        )
-                                        .into());
+                                        ));
                                     }
                                 }
                                 insts::OP_RVC_JALR => {
@@ -73,8 +71,7 @@ impl<'a> IllScriptChecker<'a> {
                                         return Err(ScriptError::EncounteredKnownBugs(
                                             CKB_VM_ISSUE_92.to_string(),
                                             self.index,
-                                        )
-                                        .into());
+                                        ));
                                     }
                                 }
                                 _ => (),
@@ -116,7 +113,6 @@ impl<'a> IllScriptChecker<'a> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use ckb_error::assert_error_eq;
     use std::fs::read;
     use std::path::Path;
 
@@ -132,7 +128,7 @@ mod tests {
         let data =
             read(Path::new(env!("CARGO_MANIFEST_DIR")).join("../script/testdata/defected_binary"))
                 .unwrap();
-        assert_error_eq!(
+        assert_eq!(
             IllScriptChecker::new(&data, 13).check().unwrap_err(),
             ScriptError::EncounteredKnownBugs(CKB_VM_ISSUE_92.to_string(), 13),
         );

script/src/lib.rs

@@ -3,8 +3,10 @@ mod error;
 mod ill_transaction_checker;
 mod syscalls;
 mod type_id;
+mod types;
 mod verify;
 
-pub use crate::error::ScriptError;
+pub use crate::error::{ScriptError, TransactionScriptError};
 pub use crate::ill_transaction_checker::IllTransactionChecker;
-pub use crate::verify::{ScriptGroup, ScriptGroupType, TransactionScriptsVerifier};
+pub use crate::types::{ScriptGroup, ScriptGroupType};
+pub use crate::verify::TransactionScriptsVerifier;

script/src/type_id.rs

@@ -1,5 +1,4 @@
 use crate::{ScriptError, ScriptGroup};
-use ckb_error::Error;
 use ckb_hash::new_blake2b;
 use ckb_types::{
     core::{cell::ResolvedTransaction, Cycle},
@@ -27,21 +26,21 @@ pub struct TypeIdSystemScript<'a> {
 }
 
 impl<'a> TypeIdSystemScript<'a> {
-    pub fn verify(&self) -> Result<Cycle, Error> {
+    pub fn verify(&self) -> Result<Cycle, ScriptError> {
         if self.max_cycles < TYPE_ID_CYCLES {
-            return Err(ScriptError::ExceededMaximumCycles.into());
+            return Err(ScriptError::ExceededMaximumCycles(self.max_cycles));
         }
         // TYPE_ID script should only accept one argument,
         // which is the hash of all inputs when creating
         // the cell.
         if self.script_group.script.args().len() != 32 {
-            return Err(ScriptError::ValidationFailure(ERROR_ARGS).into());
+            return Err(ScriptError::ValidationFailure(ERROR_ARGS));
         }
 
         // There could be at most one input cell and one
         // output cell with current TYPE_ID script.
         if self.script_group.input_indices.len() > 1 || self.script_group.output_indices.len() > 1 {
-            return Err(ScriptError::ValidationFailure(ERROR_TOO_MANY_CELLS).into());
+            return Err(ScriptError::ValidationFailure(ERROR_TOO_MANY_CELLS));
         }
 
         // If there's only one output cell with current
@@ -72,7 +71,7 @@ impl<'a> TypeIdSystemScript<'a> {
             blake2b.finalize(&mut ret);
 
             if ret[..] != self.script_group.script.args().raw_data()[..] {
-                return Err(ScriptError::ValidationFailure(ERROR_INVALID_INPUT_HASH).into());
+                return Err(ScriptError::ValidationFailure(ERROR_INVALID_INPUT_HASH));
             }
         }
         Ok(TYPE_ID_CYCLES)

script/src/types.rs

@@ -0,0 +1,49 @@
+use ckb_types::packed::Script;
+use serde::{Deserialize, Serialize};
+use std::fmt;
+
+// A script group is defined as scripts that share the same hash.
+// A script group will only be executed once per transaction, the
+// script itself should check against all inputs/outputs in its group
+// if needed.
+pub struct ScriptGroup {
+    pub script: Script,
+    pub group_type: ScriptGroupType,
+    pub input_indices: Vec<usize>,
+    pub output_indices: Vec<usize>,
+}
+
+impl ScriptGroup {
+    pub fn new(script: &Script, group_type: ScriptGroupType) -> Self {
+        Self {
+            group_type,
+            script: script.to_owned(),
+            input_indices: vec![],
+            output_indices: vec![],
+        }
+    }
+
+    pub fn from_lock_script(script: &Script) -> Self {
+        Self::new(script, ScriptGroupType::Lock)
+    }
+
+    pub fn from_type_script(script: &Script) -> Self {
+        Self::new(script, ScriptGroupType::Type)
+    }
+}
+
+#[derive(Copy, Clone, Serialize, Deserialize, PartialEq, Eq, Hash, Debug)]
+#[serde(rename_all = "snake_case")]
+pub enum ScriptGroupType {
+    Lock,
+    Type,
+}
+
+impl fmt::Display for ScriptGroupType {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            ScriptGroupType::Lock => write!(f, "Lock"),
+            ScriptGroupType::Type => write!(f, "Type"),
+        }
+    }
+}