feat: stop waiting for signatures if the chain tip advances

obycode · obycode · commit b21f156ac84c · 2025-02-04T15:46:34.000-05:00
This could happen at the beginning of a tenure, if the miner for tenure T+1 proposes its first block before it sees the last block from tenure T, so the block it proposed is reorging that last block. Once it sees this block, it should immediately stop waiting for signatures and instead build a new block off of the new tip. Fixes: #5751
diff --git a/.github/workflows/bitcoin-tests.yml b/.github/workflows/bitcoin-tests.yml
@@ -151,6 +151,7 @@ jobs:
           - tests::signer::v0::block_proposal_timeout
           - tests::signer::v0::rejected_blocks_count_towards_miner_validity
           - tests::signer::v0::allow_reorg_within_first_proposal_burn_block_timing_secs
+          - tests::signer::v0::interrupt_miner_on_new_stacks_tip
           - tests::nakamoto_integrations::burn_ops_integration_test
           - tests::nakamoto_integrations::check_block_heights
           - tests::nakamoto_integrations::clarity_burn_state
diff --git a/stacks-signer/src/v0/signer.rs b/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) => {
diff --git a/testnet/stacks-node/src/nakamoto_node/miner.rs b/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,
diff --git a/testnet/stacks-node/src/nakamoto_node/signer_coordinator.rs b/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,
@@ -384,6 +386,17 @@ impl SignerCoordinator {
                         ));
                     }
 
+                    // Check if a new Stacks block has arrived
+                    let (canonical_stacks_tip_ch, canonical_stacks_tip_bh) =
+                        SortitionDB::get_canonical_stacks_chain_tip_hash(sortdb.conn()).unwrap();
+                    let canonical_stacks_tip =
+                        StacksBlockId::new(&canonical_stacks_tip_ch, &canonical_stacks_tip_bh);
+                    info!("SignCoordinator: Current stacks tip: {canonical_stacks_tip}, parent: {parent_block_id}");
+                    if canonical_stacks_tip != parent_block_id {
+                        debug!("SignCoordinator: Exiting due to new stacks tip");
+                        return Err(NakamotoNodeError::StacksTipChanged);
+                    }
+
                     continue;
                 }
             };
diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs
@@ -97,7 +97,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};
@@ -5186,7 +5187,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);
@@ -5205,7 +5206,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.
@@ -9876,7 +9877,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,
@@ -9903,7 +9904,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,
diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs
