Fix problematic transaction handling

changelog.d/problematic-txs.fixed

@@ -0,0 +1 @@
+Mark a transaction as problematic when it hits the execution time limit.

clarity/src/vm/clarity.rs

@@ -61,6 +61,8 @@ pub enum ClarityError {
         /// A human-readable explanation for aborting the transaction
         reason: String,
     },
+    /// Transaction exceeded the maximum execution time allowed.
+    ExecutionTimeExpired,
 }
 
 impl fmt::Display for ClarityError {
@@ -76,6 +78,7 @@ impl fmt::Display for ClarityError {
             }
             ClarityError::Interpreter(e) => fmt::Display::fmt(e, f),
             ClarityError::BadTransaction(s) => fmt::Display::fmt(s, f),
+            ClarityError::ExecutionTimeExpired => write!(f, "Execution time expired"),
         }
     }
 }
@@ -89,6 +92,7 @@ impl std::error::Error for ClarityError {
             ClarityError::Parse(ref e) => Some(e),
             ClarityError::Interpreter(ref e) => Some(e),
             ClarityError::BadTransaction(ref _s) => None,
+            ClarityError::ExecutionTimeExpired => None,
         }
     }
 }
@@ -103,9 +107,7 @@ impl From<StaticCheckError> for ClarityError {
             StaticCheckErrorKind::MemoryBalanceExceeded(_a, _b) => {
                 ClarityError::CostError(ExecutionCost::max_value(), ExecutionCost::max_value())
             }
-            StaticCheckErrorKind::ExecutionTimeExpired => {
-                ClarityError::CostError(ExecutionCost::max_value(), ExecutionCost::max_value())
-            }
+            StaticCheckErrorKind::ExecutionTimeExpired => ClarityError::ExecutionTimeExpired,
             _ => ClarityError::StaticCheck(e),
         }
     }
@@ -152,7 +154,7 @@ impl From<VmExecutionError> for ClarityError {
                 ClarityError::CostError(ExecutionCost::max_value(), ExecutionCost::max_value())
             }
             VmExecutionError::RuntimeCheck(RuntimeCheckErrorKind::ExecutionTimeExpired) => {
-                ClarityError::CostError(ExecutionCost::max_value(), ExecutionCost::max_value())
+                ClarityError::ExecutionTimeExpired
             }
             _ => ClarityError::Interpreter(e),
         }
@@ -169,9 +171,7 @@ impl From<ParseError> for ClarityError {
             ParseErrorKind::MemoryBalanceExceeded(_a, _b) => {
                 ClarityError::CostError(ExecutionCost::max_value(), ExecutionCost::max_value())
             }
-            ParseErrorKind::ExecutionTimeExpired => {
-                ClarityError::CostError(ExecutionCost::max_value(), ExecutionCost::max_value())
-            }
+            ParseErrorKind::ExecutionTimeExpired => ClarityError::ExecutionTimeExpired,
             _ => ClarityError::Parse(e),
         }
     }

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

@@ -368,6 +368,7 @@ pub enum ClarityRuntimeTxError {
     },
     CostError(ExecutionCost, ExecutionCost),
     AnalysisError(RuntimeCheckErrorKind),
+    ExecutionTimeExpired,
     Rejectable(ClarityError),
 }
 
@@ -407,6 +408,7 @@ pub fn handle_clarity_runtime_error(error: ClarityError) -> ClarityRuntimeTxErro
             reason,
         },
         ClarityError::CostError(cost, budget) => ClarityRuntimeTxError::CostError(cost, budget),
+        ClarityError::ExecutionTimeExpired => ClarityRuntimeTxError::ExecutionTimeExpired,
         unhandled_error => ClarityRuntimeTxError::Rejectable(unhandled_error),
     }
 }
@@ -450,11 +452,15 @@ fn log_unreachable_error(error: &ClarityError, txid: &Txid) {
 pub fn convert_clarity_error_to_transaction_result(
     clarity_tx: &mut ClarityTx,
     tx: &StacksTransaction,
+    cost_before: &ExecutionCost,
     error: Error,
 ) -> TransactionResult {
     let (is_problematic, error) =
         TransactionResult::is_problematic(tx, error, clarity_tx.get_epoch());
     if is_problematic {
+        // Roll back the cost accumulated by this transaction so it does not
+        // shrink the remaining block budget for subsequent honest txs.
+        clarity_tx.reset_cost(cost_before.clone());
         TransactionResult::problematic(tx, error)
     } else {
         match &error {
@@ -1289,6 +1295,16 @@ impl StacksChainState {
                                     )));
                                 }
                             }
+                            ClarityRuntimeTxError::ExecutionTimeExpired => {
+                                warn!("Transaction exceeded miner execution time limit; will be dropped from mempool";
+                                              "txid" => %tx.txid(),
+                                              "origin" => %origin_account.principal,
+                                              "origin_nonce" => %origin_account.nonce,
+                                               "contract_name" => %contract_id,
+                                               "function_name" => %contract_call.function_name,
+                                               "function_args" => %VecDisplay(&contract_call.function_args));
+                                return Err(Error::ExecutionTimeExpired);
+                            }
                             ClarityRuntimeTxError::Rejectable(e) => {
                                 error!("Unexpected error in validating transaction: if included, this will invalidate a block";
                                            "txid" => %tx.txid(),
@@ -1533,6 +1549,12 @@ impl StacksChainState {
                                     )));
                                 }
                             }
+                            ClarityRuntimeTxError::ExecutionTimeExpired => {
+                                warn!("Transaction exceeded miner execution time limit; will be dropped from mempool";
+                                              "txid" => %tx.txid(),
+                                              "contract" => %contract_id);
+                                return Err(Error::ExecutionTimeExpired);
+                            }
                             ClarityRuntimeTxError::Rejectable(e) => {
                                 error!("Unexpected error invalidating transaction: if included, this will invalidate a block";
                                            "txid" => %tx.txid(),
@@ -12126,4 +12148,140 @@ pub mod test {
             conn.commit_block();
         }
     }
+
+    #[test]
+    fn test_process_transaction_execution_time_expired() {
+        let privk = StacksPrivateKey::random();
+        let auth = TransactionAuth::from_p2pkh(&privk).unwrap();
+        let addr = auth.origin().address_testnet();
+        let balances = vec![(addr.clone(), 1_000_000_000)];
+
+        let mut chainstate =
+            instantiate_chainstate_with_balances(false, 0x80000000, function_name!(), balances);
+
+        let mut conn = chainstate.block_begin(
+            &TestBurnStateDB_21, // or whichever Epoch ≥ 2.1 stub fits
+            &FIRST_BURNCHAIN_CONSENSUS_HASH,
+            &FIRST_STACKS_BLOCK_HASH,
+            &ConsensusHash([1u8; 20]),
+            &BlockHeaderHash([1u8; 32]),
+        );
+
+        let foo = "
+            (define-public (foo)
+                (ok true)
+            )
+            (+ 1 3)
+            "
+        .to_string();
+        let mut tx = StacksTransaction::new(
+            TransactionVersion::Testnet,
+            auth.clone(),
+            TransactionPayload::new_smart_contract("foo", &foo, Some(ClarityVersion::Clarity1))
+                .unwrap(),
+        );
+
+        tx.post_condition_mode = TransactionPostConditionMode::Allow;
+        tx.chain_id = 0x80000000;
+        tx.set_tx_fee(1);
+        tx.set_origin_nonce(0);
+
+        let mut signer = StacksTransactionSigner::new(&tx);
+        signer.sign_origin(&privk).unwrap();
+
+        let signed_tx = signer.get_tx().unwrap();
+
+        let cost_before_deploy = conn.cost_so_far();
+
+        // set max_execution_time to something that will fire on the first eval()
+        let err = StacksChainState::process_transaction(
+            &mut conn,
+            &signed_tx,
+            false,
+            Some(std::time::Duration::from_nanos(1)),
+        )
+        .unwrap_err();
+
+        assert!(
+            matches!(err, Error::ExecutionTimeExpired),
+            "expected Error::ExecutionTimeExpired, got {err:?}",
+        );
+
+        // Exercise the miner-level wrapper: it should classify as Problematic and reset the cost.
+        let result = convert_clarity_error_to_transaction_result(
+            &mut conn,
+            &signed_tx,
+            &cost_before_deploy,
+            err,
+        );
+        assert!(
+            matches!(result, TransactionResult::Problematic(_)),
+            "expected Problematic verdict, got {result:?}",
+        );
+        assert_eq!(
+            cost_before_deploy,
+            conn.cost_so_far(),
+            "Expected transaction cost to be reverted on execution time expiry"
+        );
+
+        // allow that transaction to be processed with no max_execution_time, so that it gets
+        // committed to the chainstate
+        StacksChainState::process_transaction(&mut conn, &signed_tx, false, None).unwrap();
+
+        // check that the cost of the transaction was charged this time
+        let cost_after_deploy = conn.cost_so_far();
+        assert!(
+            cost_after_deploy.exceeds(&cost_before_deploy),
+            "Expected transaction cost to be charged after successful execution"
+        );
+
+        // Make a call to the contract to check the contract-call path also handles execution time
+        // expiry correctly
+        let mut call_tx = StacksTransaction::new(
+            TransactionVersion::Testnet,
+            auth.clone(),
+            TransactionPayload::new_contract_call(addr.clone(), "foo", "foo", vec![]).unwrap(),
+        );
+
+        call_tx.post_condition_mode = TransactionPostConditionMode::Allow;
+        call_tx.chain_id = 0x80000000;
+        call_tx.set_tx_fee(1);
+        call_tx.set_origin_nonce(1);
+
+        let mut signer = StacksTransactionSigner::new(&call_tx);
+        signer.sign_origin(&privk).unwrap();
+
+        let signed_call_tx = signer.get_tx().unwrap();
+
+        let cost_before_call = conn.cost_so_far();
+        let err = StacksChainState::process_transaction(
+            &mut conn,
+            &signed_call_tx,
+            false,
+            Some(std::time::Duration::from_nanos(1)),
+        )
+        .unwrap_err();
+
+        assert!(
+            matches!(err, Error::ExecutionTimeExpired),
+            "expected Error::ExecutionTimeExpired, got {err:?}",
+        );
+
+        // Exercise the miner-level wrapper for the contract-call path too.
+        let result = convert_clarity_error_to_transaction_result(
+            &mut conn,
+            &signed_call_tx,
+            &cost_before_call,
+            err,
+        );
+        assert!(
+            matches!(result, TransactionResult::Problematic(_)),
+            "expected Problematic verdict, got {result:?}",
+        );
+        assert_eq!(
+            cost_before_call,
+            conn.cost_so_far(),
+            "Expected transaction cost to be reverted on execution time expiry"
+        );
+    }
 }

stackslib/src/chainstate/stacks/miner.rs

@@ -697,6 +697,15 @@ impl TransactionResult {
                     tx_events,
                     reason,
                 }),
+                ClarityRuntimeTxError::ExecutionTimeExpired => {
+                    // This transaction took too long to execute. Consider it problematic.
+                    info!("Problematic transaction caused ExecutionTimeExpired";
+                          "txid" => %tx.txid(),
+                          "origin" => %tx.get_origin().get_address(false),
+                          "payload" => ?tx.payload,
+                    );
+                    return (true, Error::ExecutionTimeExpired);
+                }
             },
             Error::InvalidFee => {
                 // The transaction didn't have enough STX left over after it was run.
@@ -711,6 +720,15 @@ impl TransactionResult {
                 );
                 return (true, Error::InvalidFee);
             }
+            Error::ExecutionTimeExpired => {
+                // The transaction took too long to execute. Consider it problematic.
+                info!("Problematic transaction caused ExecutionTimeExpired";
+                      "txid" => %tx.txid(),
+                      "origin" => %tx.get_origin().get_address(false),
+                      "payload" => ?tx.payload,
+                );
+                return (true, Error::ExecutionTimeExpired);
+            }
             e => e,
         };
         (false, error)
@@ -1083,9 +1101,10 @@ impl<'a> StacksMicroblockBuilder<'a> {
         }
 
         let quiet = !cfg!(test);
+        let cost_before = clarity_tx.cost_so_far();
         match StacksChainState::process_transaction(clarity_tx, &tx, quiet, None) {
             Ok((_fee, receipt)) => TransactionResult::success(&tx, receipt),
-            Err(e) => convert_clarity_error_to_transaction_result(clarity_tx, &tx, e),
+            Err(e) => convert_clarity_error_to_transaction_result(clarity_tx, &tx, &cost_before, e),
         }
     }
 
@@ -2484,11 +2503,17 @@ impl BlockBuilder for StacksBlockBuilder {
                 );
                 return TransactionResult::problematic(tx, Error::NetError(e));
             }
+            let cost_before = clarity_tx.cost_so_far();
             let (fee, receipt) =
                 match StacksChainState::process_transaction(clarity_tx, tx, quiet, None) {
                     Ok((fee, receipt)) => (fee, receipt),
                     Err(e) => {
-                        return convert_clarity_error_to_transaction_result(clarity_tx, tx, e);
+                        return convert_clarity_error_to_transaction_result(
+                            clarity_tx,
+                            tx,
+                            &cost_before,
+                            e,
+                        );
                     }
                 };
             info!("Include tx";
@@ -2527,11 +2552,17 @@ impl BlockBuilder for StacksBlockBuilder {
                 );
                 return TransactionResult::problematic(tx, Error::NetError(e));
             }
+            let cost_before = clarity_tx.cost_so_far();
             let (fee, receipt) =
                 match StacksChainState::process_transaction(clarity_tx, tx, quiet, None) {
                     Ok((fee, receipt)) => (fee, receipt),
                     Err(e) => {
-                        return convert_clarity_error_to_transaction_result(clarity_tx, tx, e);
+                        return convert_clarity_error_to_transaction_result(
+                            clarity_tx,
+                            tx,
+                            &cost_before,
+                            e,
+                        );
                     }
                 };
             debug!(

stackslib/src/chainstate/stacks/mod.rs

@@ -126,6 +126,8 @@ pub enum Error {
     TxWouldNotFitError,
     /// This error indicates an internal state or condition that should never actually happen
     Expects(String),
+    /// This error indicates that a transaction execution was aborted because it exceeded the maximum allowed execution time.
+    ExecutionTimeExpired,
 }
 
 impl From<marf_error> for Error {
@@ -230,6 +232,7 @@ impl fmt::Display for Error {
             }
             Error::TenureTooBigError => write!(f, "Too much data in tenure"),
             Error::TxWouldNotFitError => write!(f, "Transaction would not fit in this block"),
+            Error::ExecutionTimeExpired => write!(f, "Transaction execution time expired"),
             Error::Expects(ref msg) => write!(f, "Unexpected state: {msg}"),
         }
     }
@@ -279,6 +282,7 @@ impl error::Error for Error {
             Error::NotInSameFork => None,
             Error::TenureTooBigError => None,
             Error::TxWouldNotFitError => None,
+            Error::ExecutionTimeExpired => None,
             Error::Expects(ref _msg) => None,
         }
     }
@@ -328,6 +332,7 @@ impl Error {
             Error::NotInSameFork => "NotInSameFork",
             Error::TenureTooBigError => "TenureTooBigError",
             Error::TxWouldNotFitError => "TxWouldNotFitError",
+            Error::ExecutionTimeExpired => "ExecutionTimeExpired",
             Error::Expects(_) => "Expects",
         }
     }