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),
         }
     }

stacks-node/src/tests/signer/v0/mod.rs

@@ -911,6 +911,10 @@ impl MultipleMinerTest {
         )
     }
 
+    fn node_2_http(&self) -> String {
+        format!("http://{}", &self.conf_node_2.node.rpc_bind)
+    }
+
     /// Sends a transfer tx to the stacks node and waits for the stacks node to mine it
     /// Returns the txid of the transfer tx.
     pub fn send_and_mine_transfer_tx(&mut self, timeout_secs: u64) -> Result<String, String> {
@@ -929,7 +933,31 @@ impl MultipleMinerTest {
         contract_name: &str,
         contract_src: &str,
     ) -> String {
-        let http_origin = self.node_http();
+        self.send_contract_publish_to(&self.node_http(), sender_nonce, contract_name, contract_src)
+    }
+
+    /// Sends a contract publish tx to miner 2's stacks node.
+    pub fn send_contract_publish_to_node_2(
+        &mut self,
+        sender_nonce: u64,
+        contract_name: &str,
+        contract_src: &str,
+    ) -> String {
+        self.send_contract_publish_to(
+            &self.node_2_http(),
+            sender_nonce,
+            contract_name,
+            contract_src,
+        )
+    }
+
+    fn send_contract_publish_to(
+        &self,
+        http_origin: &str,
+        sender_nonce: u64,
+        contract_name: &str,
+        contract_src: &str,
+    ) -> String {
         let contract_tx = make_contract_publish(
             &self.sender_sk,
             sender_nonce,
@@ -938,7 +966,7 @@ impl MultipleMinerTest {
             contract_name,
             contract_src,
         );
-        submit_tx(&http_origin, &contract_tx)
+        submit_tx(http_origin, &contract_tx)
     }
 
     /// Sends a contract publish tx to the stacks node and waits for the stacks node to mine it
@@ -970,6 +998,34 @@ impl MultipleMinerTest {
         }
     }
 
+    /// Sends a contract publish tx to miner 2's stacks node and waits for it to be mined.
+    pub fn send_and_mine_contract_publish_to_node_2(
+        &mut self,
+        sender_nonce: u64,
+        contract_name: &str,
+        contract_src: &str,
+        timeout_secs: u64,
+    ) -> Result<String, String> {
+        let stacks_height_before = self.get_peer_stacks_tip_height();
+
+        let txid = self.send_contract_publish_to_node_2(sender_nonce, contract_name, contract_src);
+
+        // wait for the new block to be mined
+        wait_for(timeout_secs, || {
+            Ok(self.get_peer_stacks_tip_height() > stacks_height_before)
+        })
+        .unwrap();
+
+        // wait for the observer to see it
+        self.wait_for_test_observer_blocks(timeout_secs);
+
+        if last_block_contains_txid(&txid) {
+            Ok(txid)
+        } else {
+            Err(txid)
+        }
+    }
+
     pub fn send_contract_call(
         &mut self,
         sender_nonce: u64,

stacks-node/src/tests/signer/v0/reorg.rs

@@ -4744,8 +4744,16 @@ fn miner_rejection_by_contract_publish_execution_time_expired() {
 
     info!("------------------------- Miner 2 Mines Block N+1 -------------------------");
 
+    // Submit to miner 2's RPC: miner 1 (with `max_execution_time_secs = 0`)
+    // marked the tx as problematic and blacklisted it locally. Miner 2 has no
+    // execution time limit, so its mempool will accept and mine it.
     let _ = miners
-        .send_and_mine_contract_publish(sender_nonce + 1, "dummy-contract", dummy_contract_src, 60)
+        .send_and_mine_contract_publish_to_node_2(
+            sender_nonce + 1,
+            "dummy-contract",
+            dummy_contract_src,
+            60,
+        )
         .expect("Failed to publish contract in a new block");
 
     verify_sortition_winner(&sortdb, &miner_pkh_2);

stacks-signer/changelog.d/block-proposal-max-tx-execution-time.added

@@ -0,0 +1 @@
+Added the `block_proposal_max_tx_execution_time_secs` connection option (defaulting to 30 seconds), which caps the time a single transaction is allowed to run during block proposal validation. A transaction that exceeds this per-transaction cap on its own is reported as a `ProblematicTransaction`. If the overall block proposal validation budget (`block_proposal_validation_timeout_secs`) is exhausted before a transaction is processed, the block is rejected with `InvalidBlock` and no specific transaction is flagged, so a transaction is no longer marked problematic just because earlier transactions consumed the validation budget.

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),
     }
 }
@@ -447,22 +449,30 @@ fn log_unreachable_error(error: &ClarityError, txid: &Txid) {
     }
 }
 
-pub fn convert_clarity_error_to_transaction_result(
+/// Classify a failed transaction into a `TransactionResult`. For failures that
+/// charged execution cost before bailing (problematic txs and cost overflows),
+/// roll back the cost so the failure does not shrink the remaining block
+/// budget for subsequent honest txs.
+pub fn finalize_failed_transaction(
     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 {
-            Error::CostOverflowError(cost_before, cost_after, total_budget) => {
+            Error::CostOverflowError(overflow_cost_before, cost_after, total_budget) => {
                 // note: this path _does_ not perform the tx block budget % heuristic,
                 //  because this code path is not directly called with a mempool handle.
                 clarity_tx.reset_cost(cost_before.clone());
-                if total_budget.proportion_largest_dimension(cost_before)
+                if total_budget.proportion_largest_dimension(overflow_cost_before)
                     < TX_BLOCK_LIMIT_PROPORTION_HEURISTIC
                 {
                     warn!(
@@ -471,7 +481,7 @@ pub fn convert_clarity_error_to_transaction_result(
                         100 - TX_BLOCK_LIMIT_PROPORTION_HEURISTIC
                     );
                     let mut measured_cost = cost_after.clone();
-                    let measured_cost = if measured_cost.sub(cost_before).is_ok() {
+                    let measured_cost = if measured_cost.sub(overflow_cost_before).is_ok() {
                         Some(measured_cost)
                     } else {
                         warn!("Failed to compute measured cost of a too big transaction");
@@ -1289,6 +1299,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 +1553,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 +12152,131 @@ 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 = finalize_failed_transaction(&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 =
+            finalize_failed_transaction(&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"
+        );
+    }
 }