Merge pull request #6018 from obycode/feat/propagate-vm-error

feat: include PC abort reason in tx receipt

Author: hstove

CHANGELOG.md

@@ -12,6 +12,7 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 - Added field `vm_error` to EventObserver transaction outputs
 - Added new `ValidateRejectCode` values to the `/v3/block_proposal` endpoint
 - Added `StateMachineUpdateContent::V1` to support a vector of `StacksTransaction` expected to be replayed in subsequent Stacks blocks
+- Include a reason string in the transaction receipt when a transaction is rolled back due to a post-condition. This should help users in understanding what went wrong.
 
 ### Changed
 

clarity/src/vm/clarity.rs

@@ -20,18 +20,30 @@ pub enum Error {
     Interpreter(InterpreterError),
     BadTransaction(String),
     CostError(ExecutionCost, ExecutionCost),
-    AbortedByCallback(Option<Value>, AssetMap, Vec<StacksTransactionEvent>),
+    AbortedByCallback {
+        /// What the output value of the transaction would have been.
+        /// This will be a Some for contract-calls, and None for contract initialization txs.
+        output: Option<Value>,
+        /// The asset map which was evaluated by the abort callback
+        assets_modified: AssetMap,
+        /// The events from the transaction processing
+        tx_events: Vec<StacksTransactionEvent>,
+        /// A human-readable explanation for aborting the transaction
+        reason: String,
+    },
 }
 
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match *self {
+        match self {
             Error::CostError(ref a, ref b) => {
-                write!(f, "Cost Error: {} cost exceeded budget of {} cost", a, b)
+                write!(f, "Cost Error: {a} cost exceeded budget of {b} cost")
             }
             Error::Analysis(ref e) => fmt::Display::fmt(e, f),
             Error::Parse(ref e) => fmt::Display::fmt(e, f),
-            Error::AbortedByCallback(..) => write!(f, "Post condition aborted transaction"),
+            Error::AbortedByCallback { reason, .. } => {
+                write!(f, "Post condition aborted transaction: {reason}")
+            }
             Error::Interpreter(ref e) => fmt::Display::fmt(e, f),
             Error::BadTransaction(ref s) => fmt::Display::fmt(s, f),
         }
@@ -42,7 +54,7 @@ impl std::error::Error for Error {
     fn cause(&self) -> Option<&dyn std::error::Error> {
         match *self {
             Error::CostError(ref _a, ref _b) => None,
-            Error::AbortedByCallback(..) => None,
+            Error::AbortedByCallback { .. } => None,
             Error::Analysis(ref e) => Some(e),
             Error::Parse(ref e) => Some(e),
             Error::Interpreter(ref e) => Some(e),
@@ -168,16 +180,17 @@ pub trait TransactionConnection: ClarityConnection {
     /// * the asset changes during `to_do` in an `AssetMap`
     /// * the Stacks events during the transaction
     ///
-    /// and a `bool` value which is `true` if the `abort_call_back` caused the changes to abort.
+    /// and an optional string value which is the result of `abort_call_back`,
+    /// containing a human-readable reason for aborting the transaction.
     ///
     /// If `to_do` returns an `Err` variant, then the changes are aborted.
     fn with_abort_callback<F, A, R, E>(
         &mut self,
         to_do: F,
         abort_call_back: A,
-    ) -> Result<(R, AssetMap, Vec<StacksTransactionEvent>, bool), E>
+    ) -> Result<(R, AssetMap, Vec<StacksTransactionEvent>, Option<String>), E>
     where
-        A: FnOnce(&AssetMap, &mut ClarityDatabase) -> bool,
+        A: FnOnce(&AssetMap, &mut ClarityDatabase) -> Option<String>,
         F: FnOnce(&mut OwnedEnvironment) -> Result<(R, AssetMap, Vec<StacksTransactionEvent>), E>,
         E: From<InterpreterError>;
 
@@ -283,16 +296,16 @@ pub trait TransactionConnection: ClarityConnection {
                     .stx_transfer(from, to, amount, memo)
                     .map_err(Error::from)
             },
-            |_, _| false,
+            |_, _| None,
         )
         .map(|(value, assets, events, _)| (value, assets, events))
     }
 
     /// Execute a contract call in the current block.
-    ///  If an error occurs while processing the transaction, its modifications will be rolled back.
-    /// abort_call_back is called with an AssetMap and a ClarityDatabase reference,
-    ///   if abort_call_back returns true, all modifications from this transaction will be rolled back.
-    ///      otherwise, they will be committed (though they may later be rolled back if the block itself is rolled back).
+    /// If an error occurs while processing the transaction, its modifications will be rolled back.
+    /// `abort_call_back` is called with an `AssetMap` and a `ClarityDatabase` reference,
+    /// If `abort_call_back` returns `Some(reason)`, all modifications from this transaction will be rolled back.
+    /// Otherwise, they will be committed (though they may later be rolled back if the block itself is rolled back).
     #[allow(clippy::too_many_arguments)]
     fn run_contract_call<F>(
         &mut self,
@@ -305,7 +318,7 @@ pub trait TransactionConnection: ClarityConnection {
         max_execution_time: Option<std::time::Duration>,
     ) -> Result<(Value, AssetMap, Vec<StacksTransactionEvent>), Error>
     where
-        F: FnOnce(&AssetMap, &mut ClarityDatabase) -> bool,
+        F: FnOnce(&AssetMap, &mut ClarityDatabase) -> Option<String>,
     {
         let expr_args: Vec<_> = args
             .iter()
@@ -331,20 +344,25 @@ pub trait TransactionConnection: ClarityConnection {
             },
             abort_call_back,
         )
-        .and_then(|(value, assets, events, aborted)| {
-            if aborted {
-                Err(Error::AbortedByCallback(Some(value), assets, events))
+        .and_then(|(value, assets_modified, tx_events, reason)| {
+            if let Some(reason) = reason {
+                Err(Error::AbortedByCallback {
+                    output: Some(value),
+                    assets_modified,
+                    tx_events,
+                    reason,
+                })
             } else {
-                Ok((value, assets, events))
+                Ok((value, assets_modified, tx_events))
             }
         })
     }
 
     /// Initialize a contract in the current block.
     ///  If an error occurs while processing the initialization, it's modifications will be rolled back.
-    /// abort_call_back is called with an AssetMap and a ClarityDatabase reference,
-    ///   if abort_call_back returns true, all modifications from this transaction will be rolled back.
-    ///      otherwise, they will be committed (though they may later be rolled back if the block itself is rolled back).
+    /// `abort_call_back` is called with an `AssetMap` and a `ClarityDatabase` reference,
+    /// If `abort_call_back` returns `Some(reason)`, all modifications from this transaction will be rolled back.
+    /// Otherwise, they will be committed (though they may later be rolled back if the block itself is rolled back).
     #[allow(clippy::too_many_arguments)]
     fn initialize_smart_contract<F>(
         &mut self,
@@ -357,9 +375,9 @@ pub trait TransactionConnection: ClarityConnection {
         max_execution_time: Option<std::time::Duration>,
     ) -> Result<(AssetMap, Vec<StacksTransactionEvent>), Error>
     where
-        F: FnOnce(&AssetMap, &mut ClarityDatabase) -> bool,
+        F: FnOnce(&AssetMap, &mut ClarityDatabase) -> Option<String>,
     {
-        let (_, asset_map, events, aborted) = self.with_abort_callback(
+        let (_, assets_modified, tx_events, reason) = self.with_abort_callback(
             |vm_env| {
                 if let Some(max_execution_time_duration) = max_execution_time {
                     vm_env
@@ -378,10 +396,15 @@ pub trait TransactionConnection: ClarityConnection {
             },
             abort_call_back,
         )?;
-        if aborted {
-            Err(Error::AbortedByCallback(None, asset_map, events))
+        if let Some(reason) = reason {
+            Err(Error::AbortedByCallback {
+                output: None,
+                assets_modified,
+                tx_events,
+                reason,
+            })
         } else {
-            Ok((asset_map, events))
+            Ok((assets_modified, tx_events))
         }
     }
 }

stackslib/src/chainstate/coordinator/tests.rs

@@ -391,7 +391,7 @@ pub fn setup_states_with_epochs(
                         Value::UInt(burnchain.pox_constants.reward_cycle_length as u128),
                         Value::UInt(burnchain.pox_constants.pox_rejection_fraction as u128),
                     ],
-                    |_, _| false,
+                    |_, _| None,
                     None,
                 )
                 .expect("Failed to set burnchain parameters in PoX contract");

stackslib/src/chainstate/nakamoto/mod.rs

@@ -4945,7 +4945,7 @@ impl NakamotoChainState {
                     &ast,
                     &contract_content,
                     None,
-                    |_, _| false,
+                    |_, _| None,
                     None,
                 )
                 .unwrap();

stackslib/src/chainstate/nakamoto/signer_set.rs

@@ -335,7 +335,7 @@ impl NakamotoSigners {
                         )
                     })
                 },
-                |_, _| false,
+                |_, _| None,
             )
             .expect("FATAL: failed to update signer stackerdb");
 

stackslib/src/chainstate/stacks/boot/contract_tests.rs

@@ -1177,7 +1177,7 @@ fn pox_2_delegate_extend_units() {
                     Value::UInt(25),
                     Value::UInt(0),
                 ],
-                |_, _| false,
+                |_, _| None,
                 None,
             )
         })
@@ -1780,15 +1780,7 @@ fn test_deploy_smart_contract(
     block.as_transaction(|tx| {
         let (ast, analysis) =
             tx.analyze_smart_contract(contract_id, version, content, ASTRules::PrecheckSize)?;
-        tx.initialize_smart_contract(
-            contract_id,
-            version,
-            &ast,
-            content,
-            None,
-            |_, _| false,
-            None,
-        )?;
+        tx.initialize_smart_contract(contract_id, version, &ast, content, None, |_, _| None, None)?;
         tx.save_analysis(contract_id, &analysis)?;
         return Ok(());
     })

stackslib/src/chainstate/stacks/boot/mod.rs

@@ -562,7 +562,7 @@ impl StacksChainState {
                             )
                         })
                     },
-                    |_, _| false,
+                    |_, _| None,
                 )
                 .expect("FATAL: failed to handle PoX unlock");
 

stackslib/src/chainstate/stacks/db/blocks.rs

@@ -4176,7 +4176,7 @@ impl StacksChainState {
                     &boot_code_id(active_pox_contract, mainnet),
                     "stack-stx",
                     &args,
-                    |_, _| false,
+                    |_, _| None,
                     None,
                 )
             });
@@ -4385,7 +4385,7 @@ impl StacksChainState {
                         until_burn_height_val,
                         reward_addr_val,
                     ],
-                    |_, _| false,
+                    |_, _| None,
                     None,
                 )
             });
@@ -4492,7 +4492,7 @@ impl StacksChainState {
                         Value::UInt(round.clone().into()),
                         Value::UInt(reward_cycle.clone().into()),
                     ],
-                    |_, _| false,
+                    |_, _| None,
                     None,
                 )
             });

stackslib/src/chainstate/stacks/db/mod.rs

@@ -1624,7 +1624,7 @@ impl StacksChainState {
                     &contract,
                     "set-burnchain-parameters",
                     &params,
-                    |_, _| false,
+                    |_, _| None,
                     None,
                 )
                 .expect("Failed to set burnchain parameters in PoX contract");