Merge pull request #6636 from jacinta-stacks/chore/fix-race-conditions-in-scenario-tests

Remove FromGlobalHeight from scenario tests as it has a race condition

Author: jacinta-stacks

stacks-node/src/tests/nakamoto_integrations.rs

@@ -1365,6 +1365,8 @@ pub fn setup_epoch_3_reward_set(
           "epoch_3_start_height" => {epoch_3_start_height},
     );
     for (stacker_sk, signer_sk) in stacker_sks.iter().zip(signer_sks.iter()) {
+        let address = StacksAddress::p2pkh(false, &StacksPublicKey::from_private(stacker_sk));
+        let nonce = get_account(&http_origin, &address).nonce;
         let pox_addr = PoxAddress::from_legacy(
             AddressHashMode::SerializeP2PKH,
             tests::to_addr(stacker_sk).bytes().clone(),
@@ -1387,7 +1389,7 @@ pub fn setup_epoch_3_reward_set(
         let signer_pk = StacksPublicKey::from_private(signer_sk);
         let stacking_tx = make_contract_call(
             stacker_sk,
-            0,
+            nonce,
             1000,
             naka_conf.burnchain.chain_id,
             &StacksAddress::burn_address(false),
@@ -1430,12 +1432,15 @@ pub fn boot_to_epoch_3_reward_set_calculation_boundary(
         num_stacking_cycles,
     );
 
-    let epochs = naka_conf.burnchain.epochs.clone().unwrap();
-    let epoch_3 = &epochs[StacksEpochId::Epoch30];
+    let epoch_3_start_height = naka_conf
+        .burnchain
+        .epochs
+        .as_ref()
+        .map(|epochs| epochs[StacksEpochId::Epoch30].start_height)
+        .unwrap();
     let reward_cycle_len = naka_conf.get_burnchain().pox_constants.reward_cycle_length as u64;
     let prepare_phase_len = naka_conf.get_burnchain().pox_constants.prepare_length as u64;
 
-    let epoch_3_start_height = epoch_3.start_height;
     assert!(
         epoch_3_start_height > 0,
         "Epoch 3.0 start height must be greater than 0"

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

@@ -26,7 +26,6 @@ pub struct ChainExpectNakaBlock {
 
 #[derive(Debug)]
 enum HeightStrategy {
-    FromGlobalHeight,
     FromMinerHeight,
     FromStateHeight,
 }
@@ -44,10 +43,6 @@ impl ChainExpectNakaBlock {
         }
     }
 
-    pub fn from_global_height(ctx: Arc<SignerTestContext>, miner_index: usize) -> Self {
-        Self::new(ctx, miner_index, HeightStrategy::FromGlobalHeight)
-    }
-
     pub fn from_miner_height(ctx: Arc<SignerTestContext>, miner_index: usize) -> Self {
         Self::new(ctx, miner_index, HeightStrategy::FromMinerHeight)
     }
@@ -76,33 +71,6 @@ impl Command<SignerTestState, SignerTestContext> for ChainExpectNakaBlock {
 
         // Calculate expected height based on the strategy
         match self.height_strategy {
-            HeightStrategy::FromGlobalHeight => {
-                // Use global height approach
-                let conf = self.ctx.get_node_config(self.miner_index);
-                let stacks_height_before = self.ctx.get_peer_stacks_tip_height();
-                let expected_height = stacks_height_before + 1;
-
-                let miner_block =
-                    wait_for_block_pushed_by_miner_key(30, expected_height, &miner_pk).expect(
-                        &format!(
-                            "Failed to get block for miner {} - Strategy: {:?}",
-                            self.miner_index, self.height_strategy
-                        ),
-                    );
-
-                let mined_block_height = miner_block.header.chain_length;
-
-                info!(
-                    "Miner {} mined Nakamoto block at height {}",
-                    self.miner_index, mined_block_height
-                );
-
-                let info_after = get_chain_info(&conf);
-
-                assert_eq!(info_after.stacks_tip, miner_block.header.block_hash());
-                assert_eq!(info_after.stacks_tip_height, mined_block_height);
-                assert_eq!(mined_block_height, expected_height);
-            }
             HeightStrategy::FromMinerHeight => {
                 // Use miner-specific height approach
                 let conf = self.ctx.get_node_config(self.miner_index);
@@ -175,7 +143,7 @@ impl Command<SignerTestState, SignerTestContext> for ChainExpectNakaBlock {
         (1usize..=2usize).prop_flat_map(move |miner_index| {
             prop_oneof![
                 Just(CommandWrapper::new(
-                    ChainExpectNakaBlock::from_global_height(ctx.clone(), miner_index)
+                    ChainExpectNakaBlock::from_state_height(ctx.clone(), miner_index)
                 )),
                 Just(CommandWrapper::new(
                     ChainExpectNakaBlock::from_miner_height(ctx.clone(), miner_index)
@@ -349,22 +317,23 @@ impl Command<SignerTestState, SignerTestContext> for ChainExpectStacksTenureChan
         true
     }
 
-    fn apply(&self, _state: &mut SignerTestState) {
+    fn apply(&self, state: &mut SignerTestState) {
         let miner_pk = self.ctx.get_miner_public_key(self.miner_index);
-        // TODO: this is a bug. You cannot gaurantee that the block has not already been processed
-        // by the node causing a potential off by one race condition.
-        // See https://github.com/stacks-network/stacks-core/issues/6221
-        let expected_height = self.ctx.get_peer_stacks_tip_height() + 1;
+        // Cannot use global height as this would result in a race condition. Cannot gaurantee
+        // that the node has not already processed the stacks block. Must use stored state.
+        let expected_height = state.last_stacks_block_height.expect(
+            "Cannot wait for a tenure change block if we haven't set the last_stacks_block_height",
+        ) + 1;
 
         info!(
-            "Applying: Waiting for tenure change block at height {} from miner {}",
-            expected_height, self.miner_index
+            "Applying: Waiting for tenure change block at height {expected_height} from miner {}",
+            self.miner_index
         );
 
         let block =
             wait_for_block_pushed_by_miner_key(30, expected_height, &miner_pk).expect(&format!(
-                "Failed to get tenure change block for miner {} at height {}",
-                self.miner_index, expected_height
+                "Failed to get tenure change block for miner {} at height {expected_height}",
+                self.miner_index
             ));
 
         // Verify this is a tenure change block
@@ -380,15 +349,14 @@ impl Command<SignerTestState, SignerTestContext> for ChainExpectStacksTenureChan
 
         assert!(
             is_tenure_change_block_found,
-            "Block at height {} from miner {} is not a proper tenure change block. Transactions: {:?}",
-            expected_height,
+            "Block at height {expected_height} from miner {} is not a proper tenure change block. Transactions: {:?}",
             self.miner_index,
             block.txs.iter().map(|tx| &tx.payload).collect::<Vec<_>>()
         );
 
         info!(
-            "Successfully verified tenure change block at height {} from miner {}",
-            expected_height, self.miner_index
+            "Successfully verified tenure change block at height {expected_height} from miner {}",
+            self.miner_index
         );
     }
 

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

@@ -2,6 +2,7 @@ use std::sync::Arc;
 
 use madhouse::{Command, CommandWrapper};
 use proptest::prelude::{Just, Strategy};
+use stacks::core::StacksEpochId;
 
 use super::context::{SignerTestContext, SignerTestState};
 use crate::tests::neon_integrations::get_chain_info;
@@ -32,17 +33,24 @@ impl Command<SignerTestState, SignerTestContext> for ChainBootToEpoch3 {
     fn apply(&self, state: &mut SignerTestState) {
         info!("Applying: Booting miners to Nakamoto");
 
-        self.ctx.miners.lock().unwrap().boot_to_epoch_3();
-
         // We can use miner 1 conf to get the chain info - it's the same for both miners
         let conf = self.ctx.get_node_config(1);
-        let burn_block_height = get_chain_info(&conf).burn_block_height;
-
-        state.epoch_3_start_block_height = Some(self.ctx.get_peer_stacks_tip_height());
-
-        // Epoch 3.0 is expected to start at burn block height 231
-        assert_eq!(burn_block_height, 231);
 
+        let epoch_3_start_height = conf
+            .burnchain
+            .epochs
+            .as_ref()
+            .map(|epochs| epochs[StacksEpochId::Epoch30].start_height)
+            .unwrap();
+        // If we have successfully bootstrapped already, don't bother doing it again.
+        if get_chain_info(&conf).burn_block_height < epoch_3_start_height {
+            self.ctx.miners.lock().unwrap().boot_to_epoch_3();
+            assert_eq!(
+                get_chain_info(&conf).burn_block_height,
+                epoch_3_start_height
+            );
+            state.epoch_3_start_block_height = Some(self.ctx.get_peer_stacks_tip_height());
+        }
         state.is_booted_to_nakamoto = true;
     }