fix: handling of block cost exceeded in validation

Author: hstove

stackslib/src/core/test_util.rs

@@ -5,6 +5,7 @@ use clarity::codec::StacksMessageCodec;
 use clarity::types::chainstate::{
     BlockHeaderHash, ConsensusHash, StacksAddress, StacksPrivateKey, StacksPublicKey,
 };
+use clarity::vm::costs::ExecutionCost;
 use clarity::vm::tests::BurnStateDB;
 use clarity::vm::types::PrincipalData;
 use clarity::vm::{ClarityName, ClarityVersion, ContractName, Value};
@@ -524,3 +525,25 @@ pub fn insert_tx_in_mempool(
         .execute(sql, args)
         .expect("Failed to insert transaction into mempool");
 }
+
+/// Generate source code for a contract that exposes a public function
+/// `big-tx`. This function uses `proportion` of read_count when called
+pub fn make_big_read_count_contract(limit: ExecutionCost, proportion: u64) -> String {
+    let read_count = (limit.read_count * proportion) / 100;
+
+    let read_lines = (0..read_count)
+        .map(|_| format!("(var-get my-var)"))
+        .collect::<Vec<_>>()
+        .join("\n");
+
+    format!(
+        "
+(define-data-var my-var uint u0)
+(define-public (big-tx)
+(begin
+{}
+(ok true)))
+        ",
+        read_lines
+    )
+}

stackslib/src/net/api/postblock_proposal.rs

@@ -50,7 +50,8 @@ use crate::chainstate::nakamoto::{NakamotoBlock, NakamotoChainState, NAKAMOTO_BL
 use crate::chainstate::stacks::db::blocks::MINIMUM_TX_FEE_RATE_PER_BYTE;
 use crate::chainstate::stacks::db::{StacksBlockHeaderTypes, StacksChainState};
 use crate::chainstate::stacks::miner::{
-    BlockBuilder, BlockLimitFunction, TransactionError, TransactionResult,
+    BlockBuilder, BlockLimitFunction, TransactionError, TransactionProblematic, TransactionResult,
+    TransactionSkipped,
 };
 use crate::chainstate::stacks::{
     Error as ChainError, StacksBlock, StacksBlockHeader, StacksTransaction, TransactionPayload,
@@ -579,6 +580,10 @@ impl NakamotoBlockProposal {
                     if replay_tx.txid() == tx.txid() {
                         break;
                     }
+
+                    // The included tx doesn't match the next tx in the
+                    // replay set. Check to see if the tx is skipped because
+                    // it was unmineable.
                     let tx_result = builder.try_mine_tx_with_len(
                         &mut tenure_tx,
                         &replay_tx,
@@ -588,30 +593,42 @@ impl NakamotoBlockProposal {
                         None,
                     );
                     match tx_result {
-                        TransactionResult::ProcessingError(e) => {
-                            // The tx wasn't able to be mined. Check `TransactionError`, to
-                            // see if we should error or allow the tx to be dropped from the replay set.
-
-                            // TODO: handle TransactionError cases
-                            match e.error {
-                                ChainError::BlockCostExceeded => {
+                        TransactionResult::Skipped(TransactionSkipped { error, .. })
+                        | TransactionResult::ProcessingError(TransactionError { error, .. })
+                        | TransactionResult::Problematic(TransactionProblematic {
+                            error, ..
+                        }) => {
+                            // The tx wasn't able to be mined. Check the underlying error, to
+                            // see if we should reject the block or allow the tx to be
+                            // dropped from the replay set.
+
+                            match error {
+                                ChainError::CostOverflowError(..)
+                                | ChainError::BlockTooBigError => {
                                     // block limit reached; add tx back to replay set.
                                     // BUT we know that the block should have ended at this point, so
                                     // return an error.
+                                    let txid = replay_tx.txid();
                                     replay_txs.push_front(replay_tx);
 
+                                    warn!("Rejecting block proposal. Next replay tx exceeds cost limits, so should have been in the next block.";
+                                        "error" => %error,
+                                        "txid" => %txid,
+                                    );
+
                                     return Err(BlockValidateRejectReason {
                                         reason_code: ValidateRejectCode::InvalidTransactionReplay,
                                         reason: "Transaction is not in the replay set".into(),
                                     });
                                 }
+                                // TODO: handle other ChainError cases
                                 _ => {
                                     // it's ok, drop it
                                     continue;
                                 }
                             }
                         }
-                        _ => {
+                        TransactionResult::Success(_) => {
                             // Tx should have been included
                             return Err(BlockValidateRejectReason {
                                 reason_code: ValidateRejectCode::InvalidTransactionReplay,

stackslib/src/net/api/tests/postblock_proposal.rs

@@ -34,7 +34,11 @@ use crate::chainstate::nakamoto::NakamotoChainState;
 use crate::chainstate::stacks::db::StacksChainState;
 use crate::chainstate::stacks::miner::{BlockBuilder, BlockLimitFunction};
 use crate::chainstate::stacks::test::make_codec_test_nakamoto_block;
-use crate::core::test_util::{make_stacks_transfer_tx, to_addr};
+use crate::core::test_util::{
+    make_big_read_count_contract, make_contract_call, make_contract_publish,
+    make_stacks_transfer_tx, to_addr,
+};
+use crate::core::BLOCK_LIMIT_MAINNET_21;
 use crate::net::api::postblock_proposal::{
     BlockValidateOk, BlockValidateReject, TEST_REPLAY_TRANSACTIONS,
 };
@@ -867,3 +871,81 @@ fn replay_validation_test_transaction_mineable_mismatch_series_2() {
         }
     }
 }
+
+#[test]
+#[ignore]
+/// Replay set has [deploy, big_a, big_b, c]
+/// The block has [deploy, big_a, c]
+///
+/// The block should have ended at big_a, because big_b would
+/// have cost too much to include.
+fn replay_validation_test_budget_exceeded() {
+    let result = replay_validation_test(|rpc_test| {
+        let miner_privk = &rpc_test.peer_1.miner.nakamoto_miner_key();
+        let miner_addr = to_addr(miner_privk);
+
+        let contract_code = make_big_read_count_contract(BLOCK_LIMIT_MAINNET_21, 50);
+
+        let deploy_tx_bytes = make_contract_publish(
+            miner_privk,
+            36,
+            1000,
+            CHAIN_ID_TESTNET,
+            &"big-contract",
+            &contract_code,
+        );
+
+        let big_a_bytes = make_contract_call(
+            miner_privk,
+            37,
+            1000,
+            CHAIN_ID_TESTNET,
+            &miner_addr,
+            &"big-contract",
+            "big-tx",
+            &vec![],
+        );
+
+        let big_b_bytes = make_contract_call(
+            miner_privk,
+            38,
+            1000,
+            CHAIN_ID_TESTNET,
+            &miner_addr,
+            &"big-contract",
+            "big-tx",
+            &vec![],
+        );
+
+        let deploy_tx =
+            StacksTransaction::consensus_deserialize(&mut deploy_tx_bytes.as_slice()).unwrap();
+        let big_a = StacksTransaction::consensus_deserialize(&mut big_a_bytes.as_slice()).unwrap();
+        let big_b = StacksTransaction::consensus_deserialize(&mut big_b_bytes.as_slice()).unwrap();
+
+        let transfer_tx = make_stacks_transfer_tx(
+            miner_privk,
+            38,
+            1000,
+            CHAIN_ID_TESTNET,
+            &StandardPrincipalData::transient().into(),
+            100,
+        );
+
+        (
+            vec![deploy_tx.clone(), big_a.clone(), big_b.clone()].into(),
+            vec![deploy_tx, big_a, transfer_tx],
+        )
+    });
+
+    match result {
+        Ok(_) => {
+            panic!("Expected validation to be rejected, but got Ok");
+        }
+        Err(rejection) => {
+            assert_eq!(
+                rejection.reason_code,
+                ValidateRejectCode::InvalidTransactionReplay
+            );
+        }
+    }
+}