diff --git a/changelog.d/problematic-txs.fixed b/changelog.d/problematic-txs.fixed new file mode 100644 index 00000000000..ee95cbf28bb --- /dev/null +++ b/changelog.d/problematic-txs.fixed @@ -0,0 +1 @@ +Mark a transaction as problematic when it hits the execution time limit. diff --git a/clarity/src/vm/clarity.rs b/clarity/src/vm/clarity.rs index d07861b2905..8e6bc31296b 100644 --- a/clarity/src/vm/clarity.rs +++ b/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 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 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 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), } } diff --git a/stacks-node/src/tests/signer/v0/mod.rs b/stacks-node/src/tests/signer/v0/mod.rs index 4245ee6fa4e..924cbab0cfc 100644 --- a/stacks-node/src/tests/signer/v0/mod.rs +++ b/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 { @@ -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 { + 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, diff --git a/stacks-node/src/tests/signer/v0/reorg.rs b/stacks-node/src/tests/signer/v0/reorg.rs index 2366278eff0..022485c08da 100644 --- a/stacks-node/src/tests/signer/v0/reorg.rs +++ b/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); diff --git a/stacks-signer/changelog.d/block-proposal-max-tx-execution-time.added b/stacks-signer/changelog.d/block-proposal-max-tx-execution-time.added new file mode 100644 index 00000000000..2de6e2cf871 --- /dev/null +++ b/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. diff --git a/stackslib/src/chainstate/stacks/db/transactions.rs b/stackslib/src/chainstate/stacks/db/transactions.rs index 03dc9d3c73c..37eaece9b96 100644 --- a/stackslib/src/chainstate/stacks/db/transactions.rs +++ b/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" + ); + } } diff --git a/stackslib/src/chainstate/stacks/miner.rs b/stackslib/src/chainstate/stacks/miner.rs index 5b8a9fe5670..7c57822b7ed 100644 --- a/stackslib/src/chainstate/stacks/miner.rs +++ b/stackslib/src/chainstate/stacks/miner.rs @@ -44,8 +44,7 @@ use crate::chainstate::nakamoto::miner::make_mem_abort_callback; use crate::chainstate::stacks::address::StacksAddressExtensions; use crate::chainstate::stacks::db::blocks::SetupBlockResult; use crate::chainstate::stacks::db::transactions::{ - convert_clarity_error_to_transaction_result, handle_clarity_runtime_error, - ClarityRuntimeTxError, + finalize_failed_transaction, handle_clarity_runtime_error, ClarityRuntimeTxError, }; use crate::chainstate::stacks::db::unconfirmed::UnconfirmedState; use crate::chainstate::stacks::db::{ChainstateTx, ClarityTx, StacksChainState}; @@ -705,6 +704,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. @@ -719,6 +727,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) @@ -1091,9 +1108,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) => finalize_failed_transaction(clarity_tx, &tx, &cost_before, e), } } @@ -2492,11 +2510,12 @@ 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 finalize_failed_transaction(clarity_tx, tx, &cost_before, e); } }; info!("Include tx"; @@ -2535,11 +2554,12 @@ 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 finalize_failed_transaction(clarity_tx, tx, &cost_before, e); } }; debug!( diff --git a/stackslib/src/chainstate/stacks/mod.rs b/stackslib/src/chainstate/stacks/mod.rs index 9469d3a0603..3328251d715 100644 --- a/stackslib/src/chainstate/stacks/mod.rs +++ b/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 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", } } diff --git a/stackslib/src/config/mod.rs b/stackslib/src/config/mod.rs index b2669692e08..0f5fe2f1028 100644 --- a/stackslib/src/config/mod.rs +++ b/stackslib/src/config/mod.rs @@ -59,6 +59,7 @@ use crate::cost_estimates::{CostEstimator, FeeEstimator, PessimisticEstimator, U use crate::net::atlas::AtlasConfig; use crate::net::connection::{ ConnectionOptions, DEFAULT_BLOCK_PROPOSAL_MAX_AGE_SECS, + DEFAULT_BLOCK_PROPOSAL_MAX_TX_EXECUTION_TIME_SECS, DEFAULT_BLOCK_PROPOSAL_VALIDATION_TIMEOUT_SECS, }; use crate::net::{Neighbor, NeighborAddress, NeighborKey}; @@ -3723,6 +3724,18 @@ pub struct ConnectionOptionsFile { /// @default: [`DEFAULT_BLOCK_PROPOSAL_VALIDATION_TIMEOUT_SECS`] /// @units: seconds pub block_proposal_validation_timeout_secs: Option, + + /// Maximum time (in seconds) to spend executing a single transaction + /// during block proposal validation. This is a per-transaction cap that + /// is applied independently from the overall block validation timeout. + /// A transaction that exceeds this limit on its own is classified as + /// problematic; a transaction interrupted because the overall block + /// validation budget was exceeded is not. + /// --- + /// @default: [`DEFAULT_BLOCK_PROPOSAL_MAX_TX_EXECUTION_TIME_SECS`] + /// @units: seconds + pub block_proposal_max_tx_execution_time_secs: Option, + /// Maximum bytes a single transaction may allocate on the heap during /// block-proposal validation before it is rejected. /// `0` disables the limit. @@ -3887,6 +3900,9 @@ impl ConnectionOptionsFile { block_proposal_validation_timeout_secs: self .block_proposal_validation_timeout_secs .unwrap_or(DEFAULT_BLOCK_PROPOSAL_VALIDATION_TIMEOUT_SECS), + block_proposal_max_tx_execution_time_secs: self + .block_proposal_max_tx_execution_time_secs + .unwrap_or(DEFAULT_BLOCK_PROPOSAL_MAX_TX_EXECUTION_TIME_SECS), block_proposal_max_tx_mem_bytes: self .block_proposal_max_tx_mem_bytes .unwrap_or(default.block_proposal_max_tx_mem_bytes), diff --git a/stackslib/src/net/api/postblock_proposal.rs b/stackslib/src/net/api/postblock_proposal.rs index 7f23c61f807..d4928806add 100644 --- a/stackslib/src/net/api/postblock_proposal.rs +++ b/stackslib/src/net/api/postblock_proposal.rs @@ -355,6 +355,7 @@ impl NakamotoBlockProposal { connection_opts: &ConnectionOptions, ) -> Result, std::io::Error> { let timeout_secs = connection_opts.block_proposal_validation_timeout_secs; + let max_tx_execution_time_secs = connection_opts.block_proposal_max_tx_execution_time_secs; let max_tx_mem_bytes = connection_opts.block_proposal_max_tx_mem_bytes; let auth_token = connection_opts.auth_token.clone(); thread::Builder::new() @@ -365,6 +366,7 @@ impl NakamotoBlockProposal { &sortdb, &mut chainstate, timeout_secs, + max_tx_execution_time_secs, max_tx_mem_bytes, auth_token, ) @@ -541,6 +543,7 @@ impl NakamotoBlockProposal { sortdb: &SortitionDB, chainstate: &mut StacksChainState, // not directly used; used as a handle to open other chainstates timeout_secs: u64, + max_tx_execution_time_secs: u64, max_tx_mem_bytes: u64, auth_token: Option, ) -> Result { @@ -734,9 +737,25 @@ impl NakamotoBlockProposal { let mut tenure_tx = builder.tenure_begin(&burn_dbconn, &mut miner_tenure_info)?; let block_deadline = Instant::now() + Duration::from_secs(timeout_secs); + let per_tx_max_execution_time = Duration::from_secs(max_tx_execution_time_secs); let mut receipts_total = 0u64; for (i, tx) in self.block.txs.iter().enumerate() { - let remaining = block_deadline.saturating_duration_since(Instant::now()); + // Enforce the overall block validation budget between txs. A tx + // running over its own per-tx limit is the tx's fault and is + // handled below; running out of overall budget is the block's + // fault and shouldn't flag any specific tx as problematic. + if Instant::now() >= block_deadline { + warn!( + "Rejected block proposal"; + "reason" => "Block validation timed out", + "next_tx_index" => i, + ); + return Err(BlockValidateRejectReason { + reason: format!("Block validation timed out before tx {i} could be processed"), + reason_code: ValidateRejectCode::InvalidBlock, + failed_txid: None, + }); + } let tx_len = tx.tx_len(); @@ -749,7 +768,7 @@ impl NakamotoBlockProposal { tx, tx_len, &BlockLimitFunction::NO_LIMIT_HIT, - Some(remaining), + Some(per_tx_max_execution_time), &mut receipts_total, ); diff --git a/stackslib/src/net/connection.rs b/stackslib/src/net/connection.rs index f10467f6416..91db4e729ce 100644 --- a/stackslib/src/net/connection.rs +++ b/stackslib/src/net/connection.rs @@ -47,6 +47,10 @@ pub const DEFAULT_BLOCK_PROPOSAL_MAX_AGE_SECS: u64 = 600; /// The default maximum time to spend validating a block proposal in seconds pub const DEFAULT_BLOCK_PROPOSAL_VALIDATION_TIMEOUT_SECS: u64 = 60; +/// The default maximum time, in seconds, to spend executing a single +/// transaction during block proposal validation. +pub const DEFAULT_BLOCK_PROPOSAL_MAX_TX_EXECUTION_TIME_SECS: u64 = 30; + /// Receiver notification handle. /// When a message with the expected `seq` value arrives, send it to an expected receiver (possibly /// in another thread) via the given `receiver_input` channel. @@ -493,6 +497,13 @@ pub struct ConnectionOptions { /// Maximum time to spend validating a block proposal in seconds pub block_proposal_validation_timeout_secs: u64, + /// Maximum time, in seconds, to spend executing a single transaction + /// during block proposal validation. A transaction that exceeds this + /// limit on its own is classified as problematic; a transaction + /// interrupted because the overall block proposal validation budget was + /// exceeded is not. + pub block_proposal_max_tx_execution_time_secs: u64, + /// Maximum bytes a single transaction may allocate on the heap during /// block-proposal validation before it is rejected. Tracked via /// per-thread allocation counters in `TrackingAllocator`. @@ -611,6 +622,8 @@ impl std::default::Default for ConnectionOptions { read_only_max_execution_time_secs: 30, block_proposal_validation_timeout_secs: DEFAULT_BLOCK_PROPOSAL_VALIDATION_TIMEOUT_SECS, + block_proposal_max_tx_execution_time_secs: + DEFAULT_BLOCK_PROPOSAL_MAX_TX_EXECUTION_TIME_SECS, block_proposal_max_tx_mem_bytes: DEFAULT_PROPOSAL_MEMORY_BYTES, } }