test: fix flakiness in tests that use TEST_MINE_STALL

In CI, we would sometimes see the case where the test set this flag, to
stall the miner, then submits a transaction, expecting it not to
immediately be included in a block, but because of the timing, the miner
was already in a loop collecting transactions for the next block and
this transaction could be picked up before the miner reaches the stall.

This change creates a function for tests to use to set the stall and
then wait for the miner to reach the stall before returning, removing
this source of flakiness.

Author: obycode

stacks-node/src/nakamoto_node/miner.rs

@@ -67,9 +67,19 @@ use crate::nakamoto_node::VRF_MOCK_MINER_KEY;
 use crate::neon_node;
 use crate::run_loop::nakamoto::Globals;
 use crate::run_loop::RegisteredKey;
+
+#[cfg(test)]
+#[derive(Default, Clone, PartialEq)]
+enum TestMineStall {
+    #[default]
+    NotStalled,
+    Pending,
+    Stalled,
+}
+
 #[cfg(test)]
 /// Test flag to stall the miner thread
-pub static TEST_MINE_STALL: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
+static TEST_MINE_STALL: LazyLock<TestFlag<TestMineStall>> = LazyLock::new(TestFlag::default);
 #[cfg(test)]
 /// Test flag to stall block proposal broadcasting for the specified miner keys
 pub static TEST_BROADCAST_PROPOSAL_STALL: LazyLock<TestFlag<Vec<Secp256k1PublicKey>>> =
@@ -90,6 +100,23 @@ pub static TEST_P2P_BROADCAST_STALL: LazyLock<TestFlag<bool>> = LazyLock::new(Te
 // Test flag to skip pushing blocks to the signers
 pub static TEST_BLOCK_PUSH_SKIP: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
 
+#[cfg(test)]
+/// Set the `TEST_MINE_STALL` flag to `Pending` and block until the miner is stalled.
+pub fn fault_injection_stall_miner() {
+    if TEST_MINE_STALL.get() == TestMineStall::NotStalled {
+        TEST_MINE_STALL.set(TestMineStall::Pending);
+    }
+    while TEST_MINE_STALL.get() != TestMineStall::Stalled {
+        std::thread::sleep(std::time::Duration::from_millis(10));
+    }
+}
+
+#[cfg(test)]
+/// Unstall the miner by setting the `TEST_MINE_STALL` flag to `NotStalled`.
+pub fn fault_injection_unstall_miner() {
+    TEST_MINE_STALL.set(TestMineStall::NotStalled);
+}
+
 /// If the miner was interrupted while mining a block, how long should the
 ///  miner thread sleep before trying again?
 const ABORT_TRY_AGAIN_MS: u64 = 200;
@@ -394,21 +421,23 @@ impl BlockMinerThread {
         false
     }
 
-    /// Stop a miner tenure by blocking the miner and then joining the tenure thread
+    /// Stall the miner based on the `TEST_MINE_STALL` flag.
+    /// Block the miner until the test flag is set to `NotStalled`.
     #[cfg(test)]
-    fn fault_injection_stall_miner() {
-        if TEST_MINE_STALL.get() {
+    fn fault_injection_miner_stall() {
+        if TEST_MINE_STALL.get() != TestMineStall::NotStalled {
             // Do an extra check just so we don't log EVERY time.
             warn!("Mining is stalled due to testing directive");
-            while TEST_MINE_STALL.get() {
+            TEST_MINE_STALL.set(TestMineStall::Stalled);
+            while TEST_MINE_STALL.get() != TestMineStall::NotStalled {
                 std::thread::sleep(std::time::Duration::from_millis(10));
             }
             warn!("Mining is no longer stalled due to testing directive. Continuing...");
         }
     }
 
     #[cfg(not(test))]
-    fn fault_injection_stall_miner() {}
+    fn fault_injection_miner_stall() {}
 
     pub fn run_miner(
         mut self,
@@ -520,7 +549,7 @@ impl BlockMinerThread {
         last_block_rejected: &mut bool,
         reward_set: &RewardSet,
     ) -> Result<(), NakamotoNodeError> {
-        Self::fault_injection_stall_miner();
+        Self::fault_injection_miner_stall();
         let mut chain_state =
             neon_node::open_chainstate_with_faults(&self.config).map_err(|e| {
                 NakamotoNodeError::SigningCoordinatorFailure(format!(

stacks-node/src/tests/nakamoto_integrations.rs

@@ -109,8 +109,7 @@ use stacks_signer::v0::SpawnedSigner;
 
 use super::bitcoin_regtest::BitcoinCoreController;
 use crate::nakamoto_node::miner::{
-    TEST_BLOCK_ANNOUNCE_STALL, TEST_BROADCAST_PROPOSAL_STALL, TEST_MINE_STALL,
-    TEST_P2P_BROADCAST_SKIP, TEST_P2P_BROADCAST_STALL,
+    fault_injection_stall_miner, fault_injection_unstall_miner, TEST_BLOCK_ANNOUNCE_STALL, TEST_BROADCAST_PROPOSAL_STALL, TEST_P2P_BROADCAST_SKIP, TEST_P2P_BROADCAST_STALL
 };
 use crate::nakamoto_node::relayer::TEST_MINER_THREAD_STALL;
 use crate::neon::Counters;
@@ -6020,7 +6019,7 @@ fn nakamoto_attempt_time() {
 
             // Stall the miner to make sure it waits until all transactions are
             // submitted before it mines a block
-            TEST_MINE_STALL.set(true);
+            fault_injection_stall_miner();
 
             let mut sender_nonce = account.nonce;
             for _ in 0..txs_per_block {
@@ -6036,7 +6035,7 @@ fn nakamoto_attempt_time() {
                 submit_tx(&http_origin, &transfer_tx);
             }
 
-            TEST_MINE_STALL.set(false);
+            fault_injection_unstall_miner();
 
             // Miner should have made a new block by now
             let wait_start = Instant::now();
@@ -6304,7 +6303,7 @@ fn clarity_burn_state() {
             result.expect_result_ok().expect("Read-only call failed");
 
             // Pause mining to prevent the stacks block from being mined before the tenure change is processed
-            TEST_MINE_STALL.set(true);
+            fault_injection_stall_miner();
             // Submit a tx for the next block (the next block will be a new tenure, so the burn block height will increment)
             let call_tx = make_contract_call(
                 &sender_sk,
@@ -6329,7 +6328,7 @@ fn clarity_burn_state() {
             Ok(commits_submitted.load(Ordering::SeqCst) > commits_before)
         })
         .unwrap();
-        TEST_MINE_STALL.set(false);
+        fault_injection_unstall_miner();
         wait_for(20, || {
             Ok(coord_channel
                 .lock()
@@ -10368,7 +10367,7 @@ fn clarity_cost_spend_down() {
             .expect("Mutex poisoned")
             .get_stacks_blocks_processed();
         // Pause mining so we can add all our transactions to the mempool at once.
-        TEST_MINE_STALL.set(true);
+        fault_injection_stall_miner();
         for _nmb_tx in 0..nmb_txs_per_signer {
             for sender_sk in sender_sks.iter() {
                 let sender_nonce = get_and_increment_nonce(sender_sk, &mut sender_nonces);
@@ -10394,7 +10393,7 @@ fn clarity_cost_spend_down() {
                 }
             }
         }
-        TEST_MINE_STALL.set(false);
+        fault_injection_unstall_miner();
         wait_for(120, || {
             let blocks_processed = coord_channel
                 .lock()
@@ -10672,7 +10671,7 @@ fn test_tenure_extend_from_flashblocks() {
     assert_eq!(sort_tip.consensus_hash, election_tip.consensus_hash);
 
     // stop the relayer thread from starting a miner thread, and stop the miner thread from mining
-    TEST_MINE_STALL.set(true);
+    fault_injection_stall_miner();
     TEST_MINER_THREAD_STALL.set(true);
 
     // mine another Bitcoin block right away, and force it to be a flash block
@@ -10738,7 +10737,7 @@ fn test_tenure_extend_from_flashblocks() {
 
     // unstall miner thread and allow block-commits again
     counters.naka_skip_commit_op.set(false);
-    TEST_MINE_STALL.set(false);
+    fault_injection_unstall_miner();
 
     // wait for the miner directive to be processed
     wait_for(60, || {
@@ -11509,7 +11508,7 @@ fn large_mempool_base(strategy: MemPoolWalkStrategy, set_fee: impl Fn() -> u64)
     info!("Pause mining and fill the mempool with the transfers");
 
     // Pause block mining
-    TEST_MINE_STALL.set(true);
+    fault_injection_stall_miner();
 
     let db_tx = conn.transaction().unwrap();
     let timer = Instant::now();
@@ -11549,7 +11548,7 @@ fn large_mempool_base(strategy: MemPoolWalkStrategy, set_fee: impl Fn() -> u64)
     let proposed_blocks_before = test_observer::get_mined_nakamoto_blocks().len();
 
     // Unpause block mining
-    TEST_MINE_STALL.set(false);
+    fault_injection_unstall_miner();
 
     // Wait for the first block to be proposed.
     wait_for(30, || {
@@ -11846,7 +11845,7 @@ fn larger_mempool() {
     info!("Pause mining and fill the mempool with the transfers");
 
     // Pause block mining
-    TEST_MINE_STALL.set(true);
+    fault_injection_stall_miner();
 
     let timer = Instant::now();
 
@@ -11890,7 +11889,7 @@ fn larger_mempool() {
     let timer = Instant::now();
 
     // Unpause block mining
-    TEST_MINE_STALL.set(false);
+    fault_injection_unstall_miner();
 
     // Wait for the first block to be proposed.
     wait_for(10, || {
@@ -12184,7 +12183,7 @@ fn handle_considered_txs_foreign_key_failure() {
     let height_before = get_chain_info(&naka_conf).stacks_tip_height;
 
     // Initiate the transaction stall, then submit transactions.
-    TEST_MINE_STALL.set(true);
+    fault_injection_stall_miner();
     TEST_TX_STALL.set(true);
 
     let bad_transfer_tx = make_stacks_transfer_serialized(
@@ -12198,7 +12197,7 @@ fn handle_considered_txs_foreign_key_failure() {
     let txid = submit_tx(&http_origin, &bad_transfer_tx);
     info!("Bad transaction submitted: {txid}");
 
-    TEST_MINE_STALL.set(false);
+    fault_injection_unstall_miner();
 
     // Sleep long enough to ensure that the miner has started processing the tx
     sleep_ms(5_000);
@@ -12432,7 +12431,7 @@ fn miner_constructs_replay_block() {
 
     // Pause mining to prevent any of the submitted txs getting mined.
     info!("Stalling mining...");
-    TEST_MINE_STALL.set(true);
+    fault_injection_stall_miner();
     let burn_height_before = get_chain_info(&naka_conf).burn_block_height;
     // Mine 1 bitcoin block to trigger a new block found transaction
     next_block_and(&mut btc_regtest_controller, 60, || {
@@ -12515,7 +12514,7 @@ fn miner_constructs_replay_block() {
     let blocks_before = test_observer::get_blocks().len();
     assert_eq!(observed_before, 0);
     info!("Resuming mining...");
-    TEST_MINE_STALL.set(false);
+    fault_injection_unstall_miner();
 
     info!("Waiting for two stacks block to be mined...");
     wait_for(30, || {

stacks-node/src/tests/signer/commands/stacks_mining.rs

@@ -4,6 +4,7 @@ use madhouse::{Command, CommandWrapper};
 use proptest::prelude::{prop_oneof, Just, Strategy};
 
 use super::context::{SignerTestContext, SignerTestState};
+use crate::nakamoto_node::miner::{fault_injection_stall_miner, fault_injection_unstall_miner};
 
 /// Command to globally pause or resume Stacks block mining within the test environment.
 /// This command is used to simulate network-wide conditions where Stacks block production might halt or resume.
@@ -46,7 +47,11 @@ impl Command<SignerTestState, SignerTestContext> for ChainStacksMining {
             "Resuming Stacks mining"
         };
         info!("Applying: {}", operation_desc);
-        crate::tests::signer::v0::test_mine_stall_set(self.should_pause);
+        if self.should_pause {
+            fault_injection_stall_miner();
+        } else {
+            fault_injection_unstall_miner();
+        }
         state.mining_stalled = self.should_pause;
     }