Merge pull request #5801 from stacks-network/retry-mining-on-tip-change

feat: stop waiting for signatures if the chain tip advances

CHANGELOG.md

@@ -13,6 +13,8 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 
 ### Changed
 
+- Miner will stop waiting for signatures on a block if the Stacks tip advances (causing the block it had proposed to be invalid).
+
 ### Fixed
 
 - Error responses to /v2/transactions/fees are once again expressed as JSON ([#4145](https://github.com/stacks-network/stacks-core/issues/4145)).

stacks-signer/src/v0/signer.rs

@@ -207,6 +207,10 @@ impl SignerTrait<SignerMessage> for Signer {
                                 "block_height" => b.header.chain_length,
                                 "signer_sighash" => %b.header.signer_signature_hash(),
                             );
+                            #[cfg(any(test, feature = "testing"))]
+                            if self.test_skip_block_broadcast(b) {
+                                return;
+                            }
                             stacks_client.post_block_until_ok(self, b);
                         }
                         SignerMessage::MockProposal(mock_proposal) => {

testnet/stacks-node/src/nakamoto_node/miner.rs

@@ -65,11 +65,20 @@ use crate::run_loop::RegisteredKey;
 pub static TEST_MINE_STALL: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
 #[cfg(test)]
 /// Test flag to stall block proposal broadcasting
-pub static TEST_BROADCAST_STALL: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
+pub static TEST_BROADCAST_PROPOSAL_STALL: LazyLock<TestFlag<bool>> =
+    LazyLock::new(TestFlag::default);
 #[cfg(test)]
+// Test flag to stall the miner from announcing a block while this flag is true
 pub static TEST_BLOCK_ANNOUNCE_STALL: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
 #[cfg(test)]
-pub static TEST_SKIP_P2P_BROADCAST: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
+// Test flag to skip broadcasting blocks over the p2p network
+pub static TEST_P2P_BROADCAST_SKIP: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
+#[cfg(test)]
+// Test flag to stall broadcasting blocks over the p2p network
+pub static TEST_P2P_BROADCAST_STALL: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
+#[cfg(test)]
+// Test flag to skip pushing blocks to the signers
+pub static TEST_BLOCK_PUSH_SKIP: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
 
 /// If the miner was interrupted while mining a block, how long should the
 ///  miner thread sleep before trying again?
@@ -252,19 +261,19 @@ impl BlockMinerThread {
     }
 
     #[cfg(test)]
-    fn fault_injection_block_broadcast_stall(new_block: &NakamotoBlock) {
-        if TEST_BROADCAST_STALL.get() {
+    fn fault_injection_block_proposal_stall(new_block: &NakamotoBlock) {
+        if TEST_BROADCAST_PROPOSAL_STALL.get() {
             // Do an extra check just so we don't log EVERY time.
-            warn!("Fault injection: Broadcasting is stalled due to testing directive.";
+            warn!("Fault injection: Block proposal broadcast is stalled due to testing directive.";
                       "stacks_block_id" => %new_block.block_id(),
                       "stacks_block_hash" => %new_block.header.block_hash(),
                       "height" => new_block.header.chain_length,
                       "consensus_hash" => %new_block.header.consensus_hash
             );
-            while TEST_BROADCAST_STALL.get() {
+            while TEST_BROADCAST_PROPOSAL_STALL.get() {
                 std::thread::sleep(std::time::Duration::from_millis(10));
             }
-            info!("Fault injection: Broadcasting is no longer stalled due to testing directive.";
+            info!("Fault injection: Block proposal broadcast is no longer stalled due to testing directive.";
                   "block_id" => %new_block.block_id(),
                   "height" => new_block.header.chain_length,
                   "consensus_hash" => %new_block.header.consensus_hash
@@ -273,7 +282,7 @@ impl BlockMinerThread {
     }
 
     #[cfg(not(test))]
-    fn fault_injection_block_broadcast_stall(_ignored: &NakamotoBlock) {}
+    fn fault_injection_block_proposal_stall(_ignored: &NakamotoBlock) {}
 
     #[cfg(test)]
     fn fault_injection_block_announce_stall(new_block: &NakamotoBlock) {
@@ -301,17 +310,48 @@ impl BlockMinerThread {
 
     #[cfg(test)]
     fn fault_injection_skip_block_broadcast() -> bool {
-        if TEST_SKIP_P2P_BROADCAST.get() {
-            return true;
-        }
-        false
+        TEST_P2P_BROADCAST_SKIP.get()
     }
 
     #[cfg(not(test))]
     fn fault_injection_skip_block_broadcast() -> bool {
         false
     }
 
+    #[cfg(test)]
+    fn fault_injection_block_broadcast_stall(new_block: &NakamotoBlock) {
+        if TEST_P2P_BROADCAST_STALL.get() {
+            // Do an extra check just so we don't log EVERY time.
+            warn!("Fault injection: P2P block broadcast is stalled due to testing directive.";
+                      "stacks_block_id" => %new_block.block_id(),
+                      "stacks_block_hash" => %new_block.header.block_hash(),
+                      "height" => new_block.header.chain_length,
+                      "consensus_hash" => %new_block.header.consensus_hash
+            );
+            while TEST_P2P_BROADCAST_STALL.get() {
+                std::thread::sleep(std::time::Duration::from_millis(10));
+            }
+            info!("Fault injection: P2P block broadcast is no longer stalled due to testing directive.";
+                  "block_id" => %new_block.block_id(),
+                  "height" => new_block.header.chain_length,
+                  "consensus_hash" => %new_block.header.consensus_hash
+            );
+        }
+    }
+
+    #[cfg(not(test))]
+    fn fault_injection_block_broadcast_stall(_ignored: &NakamotoBlock) {}
+
+    #[cfg(test)]
+    fn fault_injection_skip_block_push() -> bool {
+        TEST_BLOCK_PUSH_SKIP.get()
+    }
+
+    #[cfg(not(test))]
+    fn fault_injection_skip_block_push() -> bool {
+        false
+    }
+
     /// Stop a miner tenure by blocking the miner and then joining the tenure thread
     #[cfg(test)]
     fn fault_injection_stall_miner() {
@@ -516,7 +556,7 @@ impl BlockMinerThread {
         };
 
         if let Some(mut new_block) = new_block {
-            Self::fault_injection_block_broadcast_stall(&new_block);
+            Self::fault_injection_block_proposal_stall(&new_block);
 
             let signer_signature = match self.propose_block(
                 coordinator,
@@ -532,7 +572,7 @@ impl BlockMinerThread {
                             "block_height" => new_block.header.chain_length,
                             "consensus_hash" => %new_block.header.consensus_hash,
                         );
-                        return Err(e);
+                        return Ok(());
                     }
                     NakamotoNodeError::BurnchainTipChanged => {
                         info!("Burnchain tip changed while waiting for signatures";
@@ -739,6 +779,7 @@ impl BlockMinerThread {
             );
             return Ok(());
         }
+        Self::fault_injection_block_broadcast_stall(block);
 
         let parent_block_info =
             NakamotoChainState::get_block_header(chain_state.db(), &block.header.parent_block_id)?
@@ -834,6 +875,14 @@ impl BlockMinerThread {
         let miners_contract_id = boot_code_id(MINERS_NAME, chain_state.mainnet);
         let mut miners_session = StackerDBSession::new(&rpc_socket.to_string(), miners_contract_id);
 
+        if Self::fault_injection_skip_block_push() {
+            warn!(
+                "Fault injection: Skipping block push for {}",
+                block.block_id()
+            );
+            return Ok(());
+        }
+
         SignerCoordinator::send_miners_message(
             miner_privkey,
             &sort_db,

testnet/stacks-node/src/nakamoto_node/signer_coordinator.rs

@@ -288,6 +288,7 @@ impl SignerCoordinator {
         self.get_block_status(
             &block.header.signer_signature_hash(),
             &block.block_id(),
+            block.header.parent_block_id,
             chain_state,
             sortdb,
             counters,
@@ -303,6 +304,7 @@ impl SignerCoordinator {
         &self,
         block_signer_sighash: &Sha512Trunc256Sum,
         block_id: &StacksBlockId,
+        parent_block_id: StacksBlockId,
         chain_state: &mut StacksChainState,
         sortdb: &SortitionDB,
         counters: &Counters,
@@ -319,6 +321,10 @@ impl SignerCoordinator {
                 )
             })?;
 
+        let parent_tenure_header =
+            NakamotoChainState::get_block_header(chain_state.db(), &parent_block_id)?
+                .ok_or(NakamotoNodeError::UnexpectedChainState)?;
+
         // this is used to track the start of the waiting cycle
         let rejections_timer = Instant::now();
         loop {
@@ -384,6 +390,18 @@ impl SignerCoordinator {
                         ));
                     }
 
+                    // Check if a new Stacks block has arrived in the parent tenure
+                    let highest_in_tenure =
+                        NakamotoChainState::get_highest_known_block_header_in_tenure(
+                            &mut chain_state.index_conn(),
+                            &parent_tenure_header.consensus_hash,
+                        )?
+                        .ok_or(NakamotoNodeError::UnexpectedChainState)?;
+                    if highest_in_tenure.index_block_hash() != parent_block_id {
+                        debug!("SignCoordinator: Exiting due to new stacks tip");
+                        return Err(NakamotoNodeError::StacksTipChanged);
+                    }
+
                     continue;
                 }
             };

testnet/stacks-node/src/tests/nakamoto_integrations.rs

@@ -98,7 +98,8 @@ use stacks_signer::v0::SpawnedSigner;
 
 use super::bitcoin_regtest::BitcoinCoreController;
 use crate::nakamoto_node::miner::{
-    TEST_BLOCK_ANNOUNCE_STALL, TEST_BROADCAST_STALL, TEST_MINE_STALL, TEST_SKIP_P2P_BROADCAST,
+    TEST_BLOCK_ANNOUNCE_STALL, TEST_BROADCAST_PROPOSAL_STALL, TEST_MINE_STALL,
+    TEST_P2P_BROADCAST_SKIP,
 };
 use crate::nakamoto_node::relayer::{RelayerThread, TEST_MINER_THREAD_STALL};
 use crate::neon::{Counters, RunLoopCounter};
@@ -5188,7 +5189,7 @@ fn forked_tenure_is_ignored() {
 
     // For the next tenure, submit the commit op but do not allow any stacks blocks to be broadcasted.
     // Stall the miner thread; only wait until the number of submitted commits increases.
-    TEST_BROADCAST_STALL.set(true);
+    TEST_BROADCAST_PROPOSAL_STALL.set(true);
     TEST_BLOCK_ANNOUNCE_STALL.set(true);
 
     let blocks_before = mined_blocks.load(Ordering::SeqCst);
@@ -5207,7 +5208,7 @@ fn forked_tenure_is_ignored() {
     // Unpause the broadcast of Tenure B's block, do not submit commits, and do not allow blocks to
     // be processed
     test_skip_commit_op.set(true);
-    TEST_BROADCAST_STALL.set(false);
+    TEST_BROADCAST_PROPOSAL_STALL.set(false);
 
     // Wait for a stacks block to be broadcasted.
     // However, it will not be processed.
@@ -9881,7 +9882,7 @@ fn skip_mining_long_tx() {
             })
             .unwrap();
 
-            TEST_SKIP_P2P_BROADCAST.set(true);
+            TEST_P2P_BROADCAST_SKIP.set(true);
             let tx = make_contract_publish(
                 &sender_2_sk,
                 0,
@@ -9908,7 +9909,7 @@ fn skip_mining_long_tx() {
             })
             .unwrap();
 
-            TEST_SKIP_P2P_BROADCAST.set(false);
+            TEST_P2P_BROADCAST_SKIP.set(false);
         } else {
             let transfer_tx = make_stacks_transfer(
                 &sender_1_sk,