Merge pull request #5111 from stacks-network/bugfix/end-of-tenure

Remove deadlock introduced in commit 1d83617

Author: jferrant

stackslib/src/chainstate/nakamoto/coordinator/mod.rs

@@ -188,7 +188,7 @@ impl<'a, T: BlockEventDispatcher> OnChainRewardSetProvider<'a, T> {
         debug_log: bool,
     ) -> Result<RewardSet, Error> {
         let Some(reward_set_block) = NakamotoChainState::get_header_by_coinbase_height(
-            &mut chainstate.index_tx_begin(),
+            &mut chainstate.index_conn(),
             block_id,
             coinbase_height_of_calculation,
         )?

stackslib/src/chainstate/nakamoto/coordinator/tests.rs

@@ -1086,8 +1086,6 @@ fn test_nakamoto_chainstate_getters() {
         let chainstate = &mut peer.stacks_node.as_mut().unwrap().chainstate;
         let sort_db = peer.sortdb.as_ref().unwrap();
 
-        let (mut stacks_db_tx, _) = chainstate.chainstate_tx_begin().unwrap();
-
         for coinbase_height in 0..=((tip
             .anchored_header
             .as_stacks_nakamoto()
@@ -1097,7 +1095,7 @@ fn test_nakamoto_chainstate_getters() {
             + 1)
         {
             let header_opt = NakamotoChainState::get_header_by_coinbase_height(
-                &mut stacks_db_tx,
+                &mut chainstate.index_conn(),
                 &tip.index_block_hash(),
                 coinbase_height,
             )

stackslib/src/chainstate/nakamoto/mod.rs

@@ -300,6 +300,13 @@ pub trait StacksDBIndexed {
     fn get(&mut self, tip: &StacksBlockId, key: &str) -> Result<Option<String>, DBError>;
     fn sqlite(&self) -> &Connection;
 
+    /// Get the ancestor block hash given a height
+    fn get_ancestor_block_id(
+        &mut self,
+        coinbase_height: u64,
+        tip_index_hash: &StacksBlockId,
+    ) -> Result<Option<StacksBlockId>, DBError>;
+
     /// Get the block ID for a specific coinbase height in the fork identified by `tip`
     fn get_nakamoto_block_id_at_coinbase_height(
         &mut self,
@@ -452,6 +459,14 @@ impl StacksDBIndexed for StacksDBConn<'_> {
     fn sqlite(&self) -> &Connection {
         self.conn()
     }
+
+    fn get_ancestor_block_id(
+        &mut self,
+        coinbase_height: u64,
+        tip_index_hash: &StacksBlockId,
+    ) -> Result<Option<StacksBlockId>, DBError> {
+        self.get_ancestor_block_hash(coinbase_height, tip_index_hash)
+    }
 }
 
 impl StacksDBIndexed for StacksDBTx<'_> {
@@ -462,6 +477,14 @@ impl StacksDBIndexed for StacksDBTx<'_> {
     fn sqlite(&self) -> &Connection {
         self.tx().deref()
     }
+
+    fn get_ancestor_block_id(
+        &mut self,
+        coinbase_height: u64,
+        tip_index_hash: &StacksBlockId,
+    ) -> Result<Option<StacksBlockId>, DBError> {
+        self.get_ancestor_block_hash(coinbase_height, tip_index_hash)
+    }
 }
 
 impl<'a> ChainstateTx<'a> {
@@ -2406,22 +2429,22 @@ impl NakamotoChainState {
     /// Return a Nakamoto StacksHeaderInfo at a given coinbase height in the fork identified by `tip_index_hash`.
     /// * For Stacks 2.x, this is the Stacks block's header
     /// * For Stacks 3.x (Nakamoto), this is the first block in the miner's tenure.
-    pub fn get_header_by_coinbase_height(
-        tx: &mut StacksDBTx,
+    pub fn get_header_by_coinbase_height<SDBI: StacksDBIndexed>(
+        conn: &mut SDBI,
         tip_index_hash: &StacksBlockId,
         coinbase_height: u64,
     ) -> Result<Option<StacksHeaderInfo>, ChainstateError> {
         // nakamoto block?
         if let Some(block_id) =
-            tx.get_nakamoto_block_id_at_coinbase_height(tip_index_hash, coinbase_height)?
+            conn.get_nakamoto_block_id_at_coinbase_height(tip_index_hash, coinbase_height)?
         {
-            return Self::get_block_header_nakamoto(tx.sqlite(), &block_id);
+            return Self::get_block_header_nakamoto(conn.sqlite(), &block_id);
         }
 
         // epcoh2 block?
-        let Some(ancestor_at_height) = tx
-            .get_ancestor_block_hash(coinbase_height, tip_index_hash)?
-            .map(|ancestor| Self::get_block_header(tx.tx(), &ancestor))
+        let Some(ancestor_at_height) = conn
+            .get_ancestor_block_id(coinbase_height, tip_index_hash)?
+            .map(|ancestor| Self::get_block_header(conn.sqlite(), &ancestor))
             .transpose()?
             .flatten()
         else {

stackslib/src/chainstate/nakamoto/tenure.rs

@@ -372,7 +372,7 @@ impl NakamotoChainState {
 
         let matured_coinbase_height = coinbase_height - MINER_REWARD_MATURITY;
         let matured_tenure_block_header = Self::get_header_by_coinbase_height(
-            chainstate_tx,
+            chainstate_tx.deref_mut(),
             &tip_index_hash,
             matured_coinbase_height,
         )?
@@ -964,7 +964,7 @@ impl NakamotoChainState {
 
         let total_coinbase = coinbase_at_block.saturating_add(accumulated_rewards);
         let parent_tenure_start_header: StacksHeaderInfo = Self::get_header_by_coinbase_height(
-            chainstate_tx,
+            chainstate_tx.deref_mut(),
             &block.header.parent_block_id,
             parent_coinbase_height,
         )?

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

@@ -1915,7 +1915,10 @@ fn end_of_tenure() {
     let http_origin = format!("http://{}", &signer_test.running_nodes.conf.node.rpc_bind);
     let long_timeout = Duration::from_secs(200);
     let short_timeout = Duration::from_secs(20);
-
+    let blocks_before = signer_test
+        .running_nodes
+        .nakamoto_blocks_mined
+        .load(Ordering::SeqCst);
     signer_test.boot_to_epoch_3();
     let curr_reward_cycle = signer_test.get_current_reward_cycle();
     // Advance to one before the next reward cycle to ensure we are on the reward cycle boundary
@@ -1928,15 +1931,26 @@ fn end_of_tenure() {
         - 2;
 
     // give the system a chance to mine a Nakamoto block
-    sleep_ms(30_000);
+    // But it doesn't have to mine one for this test to succeed?
+    let start = Instant::now();
+    while start.elapsed() <= short_timeout {
+        let mined_blocks = signer_test
+            .running_nodes
+            .nakamoto_blocks_mined
+            .load(Ordering::SeqCst);
+        if mined_blocks > blocks_before {
+            break;
+        }
+        sleep_ms(100);
+    }
 
     info!("------------------------- Test Mine to Next Reward Cycle Boundary  -------------------------");
     signer_test.run_until_burnchain_height_nakamoto(
         long_timeout,
         final_reward_cycle_height_boundary,
         num_signers,
     );
-    println!("Advanced to nexct reward cycle boundary: {final_reward_cycle_height_boundary}");
+    println!("Advanced to next reward cycle boundary: {final_reward_cycle_height_boundary}");
     assert_eq!(
         signer_test.get_current_reward_cycle(),
         final_reward_cycle - 1
@@ -1977,39 +1991,20 @@ fn end_of_tenure() {
         std::thread::sleep(Duration::from_millis(100));
     }
 
-    info!("Triggering a new block to be mined");
-
-    // Mine a block into the next reward cycle
-    let commits_before = signer_test
-        .running_nodes
-        .commits_submitted
-        .load(Ordering::SeqCst);
-    next_block_and(
-        &mut signer_test.running_nodes.btc_regtest_controller,
-        10,
-        || {
-            let commits_count = signer_test
-                .running_nodes
-                .commits_submitted
-                .load(Ordering::SeqCst);
-            Ok(commits_count > commits_before)
-        },
-    )
-    .unwrap();
-
-    // Mine a few blocks so we are well into the next reward cycle
-    for _ in 0..2 {
+    while signer_test.get_current_reward_cycle() != final_reward_cycle {
         next_block_and(
             &mut signer_test.running_nodes.btc_regtest_controller,
             10,
             || Ok(true),
         )
         .unwrap();
+        assert!(
+            start_time.elapsed() <= short_timeout,
+            "Timed out waiting to enter the next reward cycle"
+        );
+        std::thread::sleep(Duration::from_millis(100));
     }
 
-    sleep_ms(10_000);
-    assert_eq!(signer_test.get_current_reward_cycle(), final_reward_cycle);
-
     while test_observer::get_burn_blocks()
         .last()
         .unwrap()