Merge pull request #5717 from stacks-network/feat/burn-view-fix-burn-checks

Fix: explicit burnchain checks in miner thread

Author: kantai

.github/workflows/bitcoin-tests.yml

@@ -143,6 +143,8 @@ jobs:
           - tests::signer::v0::outgoing_signers_ignore_block_proposals
           - tests::signer::v0::injected_signatures_are_ignored_across_boundaries
           - tests::signer::v0::fast_sortition
+          - tests::signer::v0::single_miner_empty_sortition
+          - tests::signer::v0::multiple_miners_empty_sortition
           - tests::nakamoto_integrations::burn_ops_integration_test
           - tests::nakamoto_integrations::check_block_heights
           - tests::nakamoto_integrations::clarity_burn_state

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

@@ -82,6 +82,9 @@ pub enum MinerDirective {
         /// This is the block ID of the first block in the parent tenure
         parent_tenure_start: StacksBlockId,
         /// This is the snapshot that this miner won, and will produce a tenure for
+        election_block: BlockSnapshot,
+        /// This is the snapshot that caused the relayer to initiate this event (may be different
+        ///  than the election block in the case where the miner is trying to mine a late block).
         burnchain_tip: BlockSnapshot,
         /// This is `true` if the snapshot above is known not to be the the latest burnchain tip,
         /// but an ancestor of it (for example, the burnchain tip could be an empty flash block, but the
@@ -136,6 +139,15 @@ pub enum MinerReason {
     },
 }
 
+impl MinerReason {
+    pub fn is_late_block(&self) -> bool {
+        match self {
+            Self::BlockFound { ref late } => *late,
+            Self::Extended { .. } => false,
+        }
+    }
+}
+
 impl std::fmt::Display for MinerReason {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
@@ -171,7 +183,7 @@ pub struct BlockMinerThread {
     burn_election_block: BlockSnapshot,
     /// Current burnchain tip as of the last TenureChange
     /// * if the last tenure-change was a BlockFound, then this is the same as the
-    /// `burn_election_block`.
+    /// `burn_election_block` (and it is also the `burn_view`)
     /// * otherwise, if the last tenure-change is an Extend, then this is the sortition of the burn
     /// view consensus hash in the TenureChange
     burn_block: BlockSnapshot,
@@ -186,6 +198,12 @@ pub struct BlockMinerThread {
     signer_set_cache: Option<RewardSet>,
     /// The time at which tenure change/extend was attempted
     tenure_change_time: Instant,
+    /// The current tip when this miner thread was started.
+    /// This *should not* be passed into any block building code, as it
+    ///  is not necessarily the burn view for the block being constructed.
+    /// Rather, this burn block is used to determine whether or not a new
+    ///  burn block has arrived since this thread started.
+    burn_tip_at_start: ConsensusHash,
     /// flag to indicate an abort driven from the relayer
     abort_flag: Arc<AtomicBool>,
 }
@@ -198,6 +216,7 @@ impl BlockMinerThread {
         burn_election_block: BlockSnapshot,
         burn_block: BlockSnapshot,
         parent_tenure_id: StacksBlockId,
+        burn_tip_at_start: &ConsensusHash,
         reason: MinerReason,
     ) -> BlockMinerThread {
         BlockMinerThread {
@@ -215,6 +234,7 @@ impl BlockMinerThread {
             reason,
             p2p_handle: rt.get_p2p_handle(),
             signer_set_cache: None,
+            burn_tip_at_start: burn_tip_at_start.clone(),
             tenure_change_time: Instant::now(),
             abort_flag: Arc::new(AtomicBool::new(false)),
         }
@@ -339,18 +359,17 @@ impl BlockMinerThread {
             self.burnchain.pox_constants.clone(),
         )
         .expect("FATAL: could not open sortition DB");
-        let burn_tip = SortitionDB::get_canonical_burn_chain_tip(sortdb.conn())
-            .expect("FATAL: failed to query sortition DB for canonical burn chain tip");
 
         // Start the signer coordinator
         let mut coordinator = SignerCoordinator::new(
             self.event_dispatcher.stackerdb_channel.clone(),
             self.globals.should_keep_running.clone(),
             &reward_set,
-            &burn_tip,
+            &self.burn_election_block,
             &self.burnchain,
             miner_privkey,
             &self.config,
+            &self.burn_tip_at_start,
         )
         .map_err(|e| {
             NakamotoNodeError::SigningCoordinatorFailure(format!(
@@ -414,6 +433,16 @@ impl BlockMinerThread {
                     "Failed to open chainstate DB. Cannot mine! {e:?}"
                 ))
             })?;
+        // Late block tenures are initiated only to issue the BlockFound
+        //  tenure change tx (because they can be immediately extended to
+        //  the next burn view). This checks whether or not we're in such a
+        //  tenure and have produced a block already. If so, it exits the
+        //  mining thread to allow the tenure extension thread to take over.
+        if self.last_block_mined.is_some() && self.reason.is_late_block() {
+            info!("Miner: finished mining a late tenure");
+            return Err(NakamotoNodeError::StacksTipChanged);
+        }
+
         let new_block = loop {
             // If we're mock mining, we may not have processed the block that the
             // actual tenure winner committed to yet. So, before attempting to
@@ -423,7 +452,7 @@ impl BlockMinerThread {
                 let mut burn_db =
                     SortitionDB::open(&burn_db_path, true, self.burnchain.pox_constants.clone())
                         .expect("FATAL: could not open sortition DB");
-                let burn_tip_changed = self.check_burn_tip_changed(&burn_db, &mut chain_state);
+                let burn_tip_changed = self.check_burn_tip_changed(&burn_db);
                 match burn_tip_changed
                     .and_then(|_| self.load_block_parent_info(&mut burn_db, &mut chain_state))
                 {
@@ -568,10 +597,7 @@ impl BlockMinerThread {
         let wait_start = Instant::now();
         while wait_start.elapsed() < self.config.miner.wait_on_interim_blocks {
             thread::sleep(Duration::from_millis(ABORT_TRY_AGAIN_MS));
-            if self
-                .check_burn_tip_changed(&sort_db, &mut chain_state)
-                .is_err()
-            {
+            if self.check_burn_tip_changed(&sort_db).is_err() {
                 return Err(NakamotoNodeError::BurnchainTipChanged);
             }
         }
@@ -599,13 +625,12 @@ impl BlockMinerThread {
             })?;
         coordinator.propose_block(
             new_block,
-            &self.burn_block,
             &self.burnchain,
             sortdb,
             &mut chain_state,
             stackerdbs,
             &self.globals.counters,
-            &self.burn_election_block.consensus_hash,
+            &self.burn_election_block,
         )
     }
 
@@ -1113,7 +1138,7 @@ impl BlockMinerThread {
         let mut chain_state = neon_node::open_chainstate_with_faults(&self.config)
             .expect("FATAL: could not open chainstate DB");
 
-        self.check_burn_tip_changed(&burn_db, &mut chain_state)?;
+        self.check_burn_tip_changed(&burn_db)?;
         neon_node::fault_injection_long_tenure();
 
         let mut mem_pool = self
@@ -1217,7 +1242,7 @@ impl BlockMinerThread {
         // last chance -- confirm that the stacks tip is unchanged (since it could have taken long
         // enough to build this block that another block could have arrived), and confirm that all
         // Stacks blocks with heights higher than the canonical tip are processed.
-        self.check_burn_tip_changed(&burn_db, &mut chain_state)?;
+        self.check_burn_tip_changed(&burn_db)?;
         Ok(block)
     }
 
@@ -1356,26 +1381,14 @@ impl BlockMinerThread {
     /// Check if the tenure needs to change -- if so, return a BurnchainTipChanged error
     /// The tenure should change if there is a new burnchain tip with a valid sortition,
     /// or if the stacks chain state's burn view has advanced beyond our burn view.
-    fn check_burn_tip_changed(
-        &self,
-        sortdb: &SortitionDB,
-        _chain_state: &mut StacksChainState,
-    ) -> Result<(), NakamotoNodeError> {
-        if let MinerReason::BlockFound { late } = &self.reason {
-            if *late && self.last_block_mined.is_none() {
-                // this is a late BlockFound tenure change that ought to be appended to the Stacks
-                // chain tip, and we haven't submitted it yet.
-                return Ok(());
-            }
-        }
-
+    fn check_burn_tip_changed(&self, sortdb: &SortitionDB) -> Result<(), NakamotoNodeError> {
         let cur_burn_chain_tip = SortitionDB::get_canonical_burn_chain_tip(sortdb.conn())
             .expect("FATAL: failed to query sortition DB for canonical burn chain tip");
 
-        if cur_burn_chain_tip.consensus_hash != self.burn_block.consensus_hash {
+        if cur_burn_chain_tip.consensus_hash != self.burn_tip_at_start {
             info!("Miner: Cancel block assembly; burnchain tip has changed";
                 "new_tip" => %cur_burn_chain_tip.consensus_hash,
-                "local_tip" => %self.burn_block.consensus_hash);
+                "local_tip" => %self.burn_tip_at_start);
             self.globals.counters.bump_missed_tenures();
             Err(NakamotoNodeError::BurnchainTipChanged)
         } else {

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

@@ -525,7 +525,8 @@ impl RelayerThread {
                       "winning_sortition" => %sn.consensus_hash);
                 return Some(MinerDirective::BeginTenure {
                     parent_tenure_start: committed_index_hash,
-                    burnchain_tip: sn,
+                    burnchain_tip: sn.clone(),
+                    election_block: sn,
                     late: false,
                 });
             }
@@ -663,7 +664,8 @@ impl RelayerThread {
                     parent_tenure_start: StacksBlockId(
                         last_winning_snapshot.winning_stacks_block_hash.clone().0,
                     ),
-                    burnchain_tip: last_winning_snapshot,
+                    burnchain_tip: sn,
+                    election_block: last_winning_snapshot,
                     late: true,
                 });
             }
@@ -1049,6 +1051,7 @@ impl RelayerThread {
         burn_tip: BlockSnapshot,
         parent_tenure_id: StacksBlockId,
         reason: MinerReason,
+        burn_tip_at_start: &ConsensusHash,
     ) -> Result<BlockMinerThread, NakamotoNodeError> {
         if fault_injection_skip_mining(&self.config.node.rpc_bind, burn_tip.block_height) {
             debug!(
@@ -1065,14 +1068,8 @@ impl RelayerThread {
 
         let burn_chain_tip = burn_chain_sn.burn_header_hash;
 
-        let allow_late = if let MinerReason::BlockFound { late } = &reason {
-            *late
-        } else {
-            false
-        };
-
-        if burn_chain_tip != burn_header_hash && !allow_late {
-            debug!(
+        if &burn_chain_sn.consensus_hash != burn_tip_at_start {
+            info!(
                 "Relayer: Drop stale RunTenure for {burn_header_hash}: current sortition is for {burn_chain_tip}"
             );
             self.globals.counters.bump_missed_tenures();
@@ -1095,6 +1092,7 @@ impl RelayerThread {
             burn_election_block,
             burn_tip,
             parent_tenure_id,
+            burn_tip_at_start,
             reason,
         );
         Ok(miner_thread_state)
@@ -1106,6 +1104,7 @@ impl RelayerThread {
         block_election_snapshot: BlockSnapshot,
         burn_tip: BlockSnapshot,
         reason: MinerReason,
+        burn_tip_at_start: &ConsensusHash,
     ) -> Result<(), NakamotoNodeError> {
         // when starting a new tenure, block the mining thread if its currently running.
         // the new mining thread will join it (so that the new mining thread stalls, not the relayer)
@@ -1126,6 +1125,7 @@ impl RelayerThread {
             burn_tip.clone(),
             parent_tenure_start,
             reason,
+            burn_tip_at_start,
         )?;
         let miner_abort_flag = new_miner_state.get_abort_flag();
 
@@ -1457,14 +1457,15 @@ impl RelayerThread {
             StacksBlockId::new(&canonical_stacks_tip_ch, &canonical_stacks_tip_bh);
 
         let reason = MinerReason::Extended {
-            burn_view_consensus_hash: new_burn_view,
+            burn_view_consensus_hash: new_burn_view.clone(),
         };
 
         if let Err(e) = self.start_new_tenure(
             canonical_stacks_tip.clone(),
             canonical_stacks_tip_election_snapshot.clone(),
             burn_tip.clone(),
             reason.clone(),
+            &new_burn_view,
         ) {
             error!("Relayer: Failed to start new tenure: {e:?}");
         } else {
@@ -1500,12 +1501,14 @@ impl RelayerThread {
             MinerDirective::BeginTenure {
                 parent_tenure_start,
                 burnchain_tip,
+                election_block,
                 late,
             } => match self.start_new_tenure(
                 parent_tenure_start,
-                burnchain_tip.clone(),
-                burnchain_tip.clone(),
+                election_block.clone(),
+                election_block.clone(),
                 MinerReason::BlockFound { late },
+                &burnchain_tip.consensus_hash,
             ) {
                 Ok(()) => {
                     debug!("Relayer: successfully started new tenure.";