fix: prevent miner from reorging itself

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
   - `block-time`
   - `to-ascii?`
 - Added `contract_cost_limit_percentage` to the miner config file — sets the percentage of a block’s execution cost at which, if a large non-boot contract call would cause a BlockTooBigError, the miner will stop adding further non-boot contract calls and only include STX transfers and boot contract calls for the remainder of the block.
+- Fixed a bug caused by a miner winning a sortition with a block commit that pointed to a previous tip, which would cause the miner to try and reorg itself. [#6481](https://github.com/stacks-network/stacks-core/issues/6481)
 
 ### Changed
 

stacks-node/src/nakamoto_node/relayer.rs

@@ -50,6 +50,7 @@ use stacks::net::db::LocalPeer;
 use stacks::net::p2p::NetworkHandle;
 use stacks::net::relay::Relayer;
 use stacks::net::NetworkResult;
+use stacks::util_lib::db::Error as DbError;
 use stacks_common::types::chainstate::{
     BlockHeaderHash, BurnchainHeaderHash, StacksBlockId, StacksPublicKey, VRFSeed,
 };
@@ -82,6 +83,11 @@ pub static TEST_MINER_THREAD_STALL: LazyLock<TestFlag<bool>> = LazyLock::new(Tes
 pub static TEST_MINER_THREAD_START_STALL: LazyLock<TestFlag<bool>> =
     LazyLock::new(TestFlag::default);
 
+#[cfg(test)]
+/// Test flag to set the tip for the miner to commit to
+pub static TEST_MINER_COMMIT_TIP: LazyLock<TestFlag<Option<(ConsensusHash, BlockHeaderHash)>>> =
+    LazyLock::new(TestFlag::default);
+
 /// Command types for the Nakamoto relayer thread, issued to it by other threads
 #[allow(clippy::large_enum_variant)]
 pub enum RelayerDirective {
@@ -616,7 +622,10 @@ impl RelayerThread {
     /// Specifically:
     ///
     /// If we won the given sortition `sn`, then we can start mining immediately with a `BlockFound`
-    /// tenure-change.  Otherwise, if we won the tenure which started the ongoing Stacks tenure
+    /// tenure-change. The exception is if we won the sortition, but the sortition's winning commit
+    /// does not commit to the ongoing tenure. In this case, we instead extend the current tenure.
+    ///
+    /// Otherwise, if we did not win `sn`, if we won the tenure which started the ongoing Stacks tenure
     /// (i.e. we're the active miner), then we _may_ start mining after a timeout _if_ the winning
     /// miner (not us) fails to submit a `BlockFound` tenure-change block for `sn`.
     fn choose_directive_sortition_with_winner(
@@ -626,6 +635,60 @@ impl RelayerThread {
         committed_index_hash: StacksBlockId,
     ) -> Option<MinerDirective> {
         let won_sortition = sn.miner_pk_hash.as_ref() == Some(mining_pkh);
+
+        let (canonical_stacks_tip_ch, canonical_stacks_tip_bh) =
+            SortitionDB::get_canonical_stacks_chain_tip_hash(self.sortdb.conn())
+                .expect("FATAL: failed to query sortition DB for stacks tip");
+        let canonical_stacks_snapshot =
+            SortitionDB::get_block_snapshot_consensus(self.sortdb.conn(), &canonical_stacks_tip_ch)
+                .expect("FATAL: failed to query sortiiton DB for epoch")
+                .expect("FATAL: no sortition for canonical stacks tip");
+
+        // If we won the sortition, ensure that the sortition's winning commit actually commits to
+        // the ongoing tenure. If it does not (i.e. commit is "stale" and points to N-1 when we are
+        // currently in N), and if we are also the ongoing tenure's miner, then we must not attempt
+        // a tenure change (which would reorg our own signed blocks). Instead, we should immediately
+        // extend the tenure.
+        if won_sortition && !self.config.get_node_config(false).mock_mining {
+            let canonical_stacks_tip =
+                StacksBlockId::new(&canonical_stacks_tip_ch, &canonical_stacks_tip_bh);
+
+            let commits_to_tip_tenure = match Self::sortition_commits_to_stacks_tip_tenure(
+                &mut self.chainstate,
+                &canonical_stacks_tip,
+                &canonical_stacks_snapshot,
+                &sn,
+            ) {
+                Ok(b) => b,
+                Err(e) => {
+                    warn!(
+                        "Relayer: Failed to determine if winning sortition commits to current tenure: {e:?}";
+                        "sortition_ch" => %sn.consensus_hash,
+                        "stacks_tip_ch" => %canonical_stacks_tip_ch
+                    );
+                    false
+                }
+            };
+
+            if !commits_to_tip_tenure {
+                let won_ongoing_tenure_sortition =
+                    canonical_stacks_snapshot.miner_pk_hash.as_ref() == Some(mining_pkh);
+
+                if won_ongoing_tenure_sortition {
+                    info!(
+                        "Relayer: Won sortition, but commit does not target ongoing tenure. Will extend instead of starting a new tenure.";
+                        "winning_sortition" => %sn.consensus_hash,
+                        "ongoing_tenure" => %canonical_stacks_snapshot.consensus_hash,
+                        "commits_to_tip_tenure?" => commits_to_tip_tenure
+                    );
+                    // Extend tenure to the new burn view instead of attempting BlockFound
+                    return Some(MinerDirective::ContinueTenure {
+                        new_burn_view: sn.consensus_hash,
+                    });
+                }
+            }
+        }
+
         if won_sortition || self.config.get_node_config(false).mock_mining {
             // a sortition happenend, and we won
             info!("Won sortition; begin tenure.";
@@ -643,13 +706,6 @@ impl RelayerThread {
             "Relayer: did not win sortition {}, so stopping tenure",
             &sn.sortition
         );
-        let (canonical_stacks_tip_ch, _) =
-            SortitionDB::get_canonical_stacks_chain_tip_hash(self.sortdb.conn())
-                .expect("FATAL: failed to query sortition DB for stacks tip");
-        let canonical_stacks_snapshot =
-            SortitionDB::get_block_snapshot_consensus(self.sortdb.conn(), &canonical_stacks_tip_ch)
-                .expect("FATAL: failed to query sortiiton DB for epoch")
-                .expect("FATAL: no sortition for canonical stacks tip");
 
         let won_ongoing_tenure_sortition =
             canonical_stacks_snapshot.miner_pk_hash.as_ref() == Some(mining_pkh);
@@ -1637,6 +1693,31 @@ impl RelayerThread {
         false
     }
 
+    /// Get the canonical tip for the miner to commit to.
+    /// This is provided as a separate function so that it can be overridden for testing.
+    #[cfg(not(test))]
+    fn fault_injection_get_tip_for_commit(&self) -> Option<(ConsensusHash, BlockHeaderHash)> {
+        None
+    }
+
+    #[cfg(test)]
+    fn fault_injection_get_tip_for_commit(&self) -> Option<(ConsensusHash, BlockHeaderHash)> {
+        TEST_MINER_COMMIT_TIP.get()
+    }
+
+    fn get_commit_for_tip(&mut self) -> Result<(ConsensusHash, BlockHeaderHash), DbError> {
+        if let Some((consensus_hash, block_header_hash)) = self.fault_injection_get_tip_for_commit()
+        {
+            info!("Relayer: using test tip for commit";
+                "consensus_hash" => %consensus_hash,
+                "block_header_hash" => %block_header_hash,
+            );
+            Ok((consensus_hash, block_header_hash))
+        } else {
+            SortitionDB::get_canonical_stacks_chain_tip_hash(self.sortdb.conn())
+        }
+    }
+
     /// Generate and submit the next block-commit, and record it locally
     fn issue_block_commit(&mut self) -> Result<(), NakamotoNodeError> {
         if self.fault_injection_skip_block_commit() {
@@ -1645,10 +1726,7 @@ impl RelayerThread {
             );
             return Ok(());
         }
-        let (tip_block_ch, tip_block_bh) = SortitionDB::get_canonical_stacks_chain_tip_hash(
-            self.sortdb.conn(),
-        )
-        .unwrap_or_else(|e| {
+        let (tip_block_ch, tip_block_bh) = self.get_commit_for_tip().unwrap_or_else(|e| {
             panic!("Failed to load canonical stacks tip: {e:?}");
         });
         let mut last_committed = self.make_block_commit(&tip_block_ch, &tip_block_bh)?;

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

@@ -103,6 +103,7 @@ use stacks_signer::v0::tests::{
     TEST_SKIP_SIGNER_CLEANUP, TEST_STALL_BLOCK_VALIDATION_SUBMISSION,
 };
 use stacks_signer::v0::SpawnedSigner;
+use stdext::prelude::DurationExt;
 use tracing_subscriber::prelude::*;
 use tracing_subscriber::{fmt, EnvFilter};
 
@@ -114,6 +115,7 @@ use crate::nakamoto_node::miner::{
     fault_injection_stall_miner, fault_injection_unstall_miner, TEST_BLOCK_ANNOUNCE_STALL,
     TEST_BROADCAST_PROPOSAL_STALL, TEST_MINE_SKIP, TEST_P2P_BROADCAST_STALL,
 };
+use crate::nakamoto_node::relayer::TEST_MINER_COMMIT_TIP;
 use crate::nakamoto_node::stackerdb_listener::TEST_IGNORE_SIGNERS;
 use crate::neon::{Counters, RunLoopCounter};
 use crate::operations::BurnchainOpSigner;
@@ -192,6 +194,11 @@ impl<Z: SpawnedSignerTrait> SignerTest<Z> {
         })
         .expect("Timed out waiting for network to restart after 3.0 boundary reached");
 
+        if self.snapshot_path.is_some() {
+            info!("Booted to epoch 3.0, ready for snapshot.");
+            return;
+        }
+
         // Wait until we see the first block of epoch 3.0.
         // Note, we don't use `nakamoto_blocks_mined` counter, because there
         // could be other miners mining blocks.
@@ -18874,3 +18881,135 @@ fn signers_treat_signatures_as_precommits() {
     info!("------------------------- Shutdown -------------------------");
     signer_test.shutdown();
 }
+
+#[test]
+#[ignore]
+/// Scenario: same miner extends tenure when the block-commit for the next tenure still confirms N-1
+///
+/// Flow:
+/// - Miner A wins tenure N
+/// - Miner A submits a block-commit confirming N-1 (commit submitted before N's block gets approved)
+/// - Miner A mines at least 2 blocks in tenure N
+/// - Miner A wins tenure N+1 with the stale commit (confirming N-1)
+/// - Miner A cannot mine a normal tenure-change + coinbase in N+1 (would reorg its own N blocks)
+/// - Miner A should issue a TenureExtend on top of tenure N
+fn tenure_extend_after_stale_commit_same_miner() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+
+    tracing_subscriber::registry()
+        .with(fmt::layer())
+        .with(EnvFilter::from_default_env())
+        .init();
+
+    let num_signers = 5;
+    let sender_sk = Secp256k1PrivateKey::from_seed("sender".as_bytes());
+    let sender_addr = tests::to_addr(&sender_sk);
+    let send_amt = 1000;
+    let send_fee = 180;
+
+    let signer_test: SignerTest<SpawnedSigner> =
+        SignerTest::new_with_config_modifications_and_snapshot(
+            num_signers,
+            vec![(sender_addr.clone(), (send_amt + send_fee) * 10)],
+            |signer_cfg| {
+                signer_cfg.block_proposal_timeout = Duration::from_minutes(60);
+            },
+            |node_cfg| {
+                node_cfg.miner.block_commit_delay = Duration::from_secs(0);
+            },
+            None,
+            None,
+            Some(function_name!()),
+        );
+
+    if signer_test.bootstrap_snapshot() {
+        signer_test.shutdown_and_snapshot();
+        return;
+    }
+
+    let conf = &signer_test.running_nodes.conf;
+    let miner_pk =
+        StacksPublicKey::from_private(&conf.miner.mining_key.clone().expect("Missing mining key"));
+    let miner_pkh = Hash160::from_node_public_key(&miner_pk);
+    let sortdb = conf.get_burnchain().open_sortition_db(true).unwrap();
+
+    let pre_test_tenures = 4;
+    for i in 1..=pre_test_tenures {
+        info!("Mining pre-test tenure {i} of {pre_test_tenures}");
+        signer_test.mine_nakamoto_block(Duration::from_secs(30), true);
+    }
+
+    signer_test.mine_nakamoto_block(Duration::from_secs(30), true);
+    // We are now in "N-1"
+    let prev_tip = get_chain_info(&signer_test.running_nodes.conf);
+
+    info!("---- Waiting for block-commit to N-1 ----";
+        "Current height" => prev_tip.burn_block_height,
+    );
+
+    let Counters {
+        naka_skip_commit_op: skip_commit_op,
+        naka_submitted_commit_last_burn_height: last_commit_burn_height,
+        ..
+    } = signer_test.running_nodes.counters.clone();
+
+    wait_for(30, || {
+        let last_height = last_commit_burn_height.get();
+        Ok(last_height == prev_tip.burn_block_height)
+    })
+    .expect("Timed out waiting for block-commit to N-1");
+
+    skip_commit_op.set(true);
+
+    let prev_tip = get_chain_info(&signer_test.running_nodes.conf);
+
+    signer_test.mine_nakamoto_block_without_commit(Duration::from_secs(30), true);
+
+    TEST_MINER_COMMIT_TIP.set(Some((prev_tip.pox_consensus, prev_tip.stacks_tip)));
+
+    // Now in tenure N
+
+    // Mine a second block in tenure N to ensure that
+    // signers will reject a reorg attempt
+    let (_, transfer_nonce) = signer_test
+        .submit_transfer_tx(&sender_sk, send_fee, send_amt)
+        .unwrap();
+
+    signer_test
+        .wait_for_nonce_increase(&sender_addr, transfer_nonce)
+        .unwrap();
+
+    skip_commit_op.set(false);
+
+    info!("---- Waiting for block commit to N-1 ----");
+
+    wait_for(30, || {
+        let last_height = last_commit_burn_height.get();
+        Ok(last_height == prev_tip.burn_block_height)
+    })
+    .expect("Timed out waiting for block commit to N-1");
+
+    // Start a new tenure (N+1)
+
+    let info_before = get_chain_info(conf);
+    let stacks_height_before = info_before.stacks_tip_height;
+
+    signer_test.mine_bitcoin_block();
+
+    verify_sortition_winner(&sortdb, &miner_pkh);
+
+    info!("---- Waiting for a tenure extend block in tenure N+1 ----";
+        "stacks_height_before" => stacks_height_before,
+    );
+
+    wait_for_block_proposal(30, stacks_height_before + 1, &miner_pk)
+        .expect("Timed out waiting for block proposal in tenure N+1");
+
+    // Verify that the next block is a TenureExtend at the expected height
+    wait_for_tenure_change_tx(30, TenureChangeCause::Extended, stacks_height_before + 1)
+        .expect("Timed out waiting for a TenureExtend block atop tenure N in tenure N+1");
+
+    signer_test.shutdown();
+}

stacks-signer/src/chainstate/v1.rs

@@ -286,6 +286,7 @@ impl SortitionsView {
                         "Current miner behaved improperly, this signer views the miner as invalid.";
                         "proposed_block_consensus_hash" => %block.header.consensus_hash,
                         "signer_signature_hash" => %block.header.signer_signature_hash(),
+                        "current_sortition_miner_status" => ?sortition.miner_status,
                     );
                     return Err(RejectReason::InvalidMiner);
                 }