Check stacks height at the right moment

Signed-off-by: Jacinta Ferrant <[email protected]>

.github/workflows/bitcoin-tests.yml

@@ -120,6 +120,7 @@ jobs:
           - tests::signer::v0::signing_in_0th_tenure_of_reward_cycle
           - tests::signer::v0::continue_after_tenure_extend
           - tests::signer::v0::multiple_miners_with_custom_chain_id
+          - tests::signer::v0::continue_after_fast_block_no_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.rs

@@ -111,6 +111,8 @@ pub enum Error {
     SigningCoordinatorFailure(String),
     // The thread that we tried to send to has closed
     ChannelClosed,
+    // Missing the canonical stacks chain tip
+    StacksTipMissing,
 }
 
 impl StacksNode {

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

@@ -109,6 +109,12 @@ pub enum MinerReason {
         /// sortition.
         burn_view_consensus_hash: ConsensusHash,
     },
+    /// The miner thread was spawned to continue an existing empty tenure
+    ExtendedEmpty {
+        /// Current consensus hash on the underlying burnchain.  Corresponds to the last-seen
+        /// sortition.
+        burn_view_consensus_hash: ConsensusHash,
+    }
 }
 
 impl std::fmt::Display for MinerReason {
@@ -121,6 +127,12 @@ impl std::fmt::Display for MinerReason {
                 f,
                 "Extended: burn_view_consensus_hash = {burn_view_consensus_hash:?}",
             ),
+            MinerReason::ExtendedEmpty {
+                burn_view_consensus_hash,
+            } => write!(
+                f,
+                "ExtendedEmpty: burn_view_consensus_hash = {burn_view_consensus_hash:?}",
+            ),
         }
     }
 }
@@ -148,6 +160,8 @@ pub struct BlockMinerThread {
     event_dispatcher: EventDispatcher,
     /// The reason the miner thread was spawned
     reason: MinerReason,
+    /// Whether the miner thread is finished initializing
+    initialized: bool,
     /// Handle to the p2p thread for block broadcast
     p2p_handle: NetworkHandle,
     signer_set_cache: Option<RewardSet>,
@@ -177,6 +191,7 @@ impl BlockMinerThread {
             reason,
             p2p_handle: rt.get_p2p_handle(),
             signer_set_cache: None,
+            initialized: false,
         }
     }
 
@@ -434,6 +449,10 @@ impl BlockMinerThread {
                 Self::fault_injection_block_announce_stall(&new_block);
                 self.globals.coord().announce_new_stacks_block();
 
+                // If we are NOT an extended empty tenure miner, we need just ONE successful block broadcast
+                // If we are an extended empty tenure miner, we need at least TWO successful block broadcasts:
+                // the first contains the tenure change transaction, and the second contains the tenure extend transaction
+                self.initialized = !matches!(self.reason, MinerReason::ExtendedEmpty { .. }) || self.last_block_mined.is_some();
                 self.last_block_mined = Some(new_block);
             }
 
@@ -1130,12 +1149,14 @@ impl BlockMinerThread {
     ) -> Result<NakamotoTenureInfo, NakamotoNodeError> {
         let current_miner_nonce = parent_block_info.coinbase_nonce;
         let Some(parent_tenure_info) = &parent_block_info.parent_tenure else {
+            debug!("NOPE HERE");
             return Ok(NakamotoTenureInfo {
                 coinbase_tx: None,
                 tenure_change_tx: None,
             });
         };
-        if self.last_block_mined.is_some() {
+        if self.last_block_mined.is_some() && self.initialized {
+            debug!("NOPE HERE 2");
             return Ok(NakamotoTenureInfo {
                 coinbase_tx: None,
                 tenure_change_tx: None,
@@ -1184,6 +1205,37 @@ impl BlockMinerThread {
                     self.generate_tenure_change_tx(current_miner_nonce, payload)?;
                 (Some(tenure_change_tx), None)
             }
+            MinerReason::ExtendedEmpty { burn_view_consensus_hash } => {
+                debug!("Miner: Extended empty tenure HERE");
+                if self.last_block_mined.is_none() {
+                    // This is the first block of the tenure, issue a tenure change tx and coinbase tx
+                    let tenure_change_tx =
+                        self.generate_tenure_change_tx(current_miner_nonce, payload)?;
+                    let coinbase_tx =
+                        self.generate_coinbase_tx(current_miner_nonce + 1, target_epoch_id, vrf_proof);
+                    (Some(tenure_change_tx), Some(coinbase_tx))
+                } else {
+                    // This is the subsequent block of the tenure, issue a tenure change tx only
+                    let num_blocks_so_far = NakamotoChainState::get_nakamoto_tenure_length(
+                        chainstate.db(),
+                        &parent_block_id,
+                    )
+                    .map_err(NakamotoNodeError::MiningFailure)?;
+                    info!("Miner: Extending empty tenure";
+                          "burn_view_consensus_hash" => %burn_view_consensus_hash,
+                          "parent_block_id" => %parent_block_id,
+                          "num_blocks_so_far" => num_blocks_so_far,
+                    );
+                    payload = payload.extend(
+                        *burn_view_consensus_hash,
+                        parent_block_id,
+                        num_blocks_so_far,
+                    );
+                    let tenure_change_tx =
+                        self.generate_tenure_change_tx(current_miner_nonce, payload)?;
+                    (Some(tenure_change_tx), None)
+                }
+            }
         };
 
         Ok(NakamotoTenureInfo {

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

@@ -385,6 +385,8 @@ impl RelayerThread {
         won_sortition: bool,
         committed_index_hash: StacksBlockId,
     ) -> Option<MinerDirective> {
+        debug!("MINER IS CHOOSING DIRECTIVE HERE");
+
         let directive = if sn.sortition {
             Some(
                 if won_sortition || config.get_node_config(false).mock_mining {
@@ -421,6 +423,7 @@ impl RelayerThread {
                 );
                 None
             } else {
+                debug!("HERE WE START WITH CONSENSUS HASH {}.", &sn.consensus_hash);
                 Some(MinerDirective::ContinueTenure {
                     new_burn_view: sn.consensus_hash,
                 })
@@ -445,6 +448,7 @@ impl RelayerThread {
         let sn = SortitionDB::get_block_snapshot_consensus(self.sortdb.conn(), &consensus_hash)
             .expect("FATAL: failed to query sortition DB")
             .expect("FATAL: unknown consensus hash");
+        debug!("SORTITION WINNER HERE {:?}", sn.miner_pk_hash);
 
         // always clear this even if this isn't the latest sortition
         let won_sortition = sn.sortition && self.last_commits.remove(&sn.winning_block_txid);
@@ -921,13 +925,37 @@ impl RelayerThread {
             return Ok(());
         }
 
+        let tip_info = NakamotoChainState::get_canonical_block_header(self.chainstate.db(), &self.sortdb).map_err(|e| {
+            error!("Relayer: failed to get canonical block header: {e:?}");
+            NakamotoNodeError::StacksTipMissing
+        })?
+        .ok_or_else(|| {
+            error!("Relayer: failed to get block snapshot for canonical tip");
+            NakamotoNodeError::StacksTipMissing
+        })?;
+
+        let last_non_empty_sortition_snapshot = SortitionDB::get_block_snapshot_consensus(self.sortdb.conn(), &tip_info.consensus_hash).map_err(|e| {
+            error!("Relayer: failed to get last non-empty sortition snapshot: {e:?}");
+            NakamotoNodeError::SnapshotNotFoundForChainTip
+        })?
+        .ok_or_else(|| {
+            error!("Relayer: failed to get last non-empty sortition snapshot");
+            NakamotoNodeError::SnapshotNotFoundForChainTip
+        })?;
+        let won_last_non_empty_sortition_snapshot = last_non_empty_sortition_snapshot.miner_pk_hash == Some(mining_pkh);
+
+        let reason = if !won_last_non_empty_sortition_snapshot {
+            debug!("Relayer: Failed to issue a tenure change payload in our last tenure. Issue a tenure change payload before issuing a continue extend.");
+            MinerReason::ExtendedEmpty { burn_view_consensus_hash: new_burn_view }
+        } else {
+            debug!("Relayer: Successfully issued a tenure change payload in our last tenure. Issue a continue extend.");
+            MinerReason::Extended { burn_view_consensus_hash: new_burn_view }
+        };
         match self.start_new_tenure(
             canonical_stacks_tip, // For tenure extend, we should be extending off the canonical tip
             block_election_snapshot,
             burn_tip,
-            MinerReason::Extended {
-                burn_view_consensus_hash: new_burn_view,
-            },
+            reason
         ) {
             Ok(()) => {
                 debug!("Relayer: successfully started new tenure.");

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

@@ -43,7 +43,7 @@ use stacks::net::api::postblock_proposal::{ValidateRejectCode, TEST_VALIDATE_STA
 use stacks::net::relay::fault_injection::set_ignore_block;
 use stacks::types::chainstate::{StacksAddress, StacksBlockId, StacksPrivateKey, StacksPublicKey};
 use stacks::types::PublicKey;
-use stacks::util::hash::{hex_bytes, MerkleHashFunc};
+use stacks::util::hash::{hex_bytes, Hash160, MerkleHashFunc};
 use stacks::util::secp256k1::{Secp256k1PrivateKey, Secp256k1PublicKey};
 use stacks::util_lib::boot::boot_code_id;
 use stacks::util_lib::signed_structured_data::pox4::{
@@ -5023,11 +5023,14 @@ fn miner_recovers_when_broadcast_block_delay_across_tenures_occurs() {
 /// Miner 1 wins the first Nakamoto tenure A. Miner 1 mines a regular stacks block N.
 /// Miner 2 wins the second Nakamoto tenure B and proposes block N+1, but it is rejected by the signers.
 /// An empty burn block is mined
-/// TODO: which behaviour do we want to enforce? Should we allow both? If so, we should force both explicitly
-/// Miner 1 should issue a tenure extend and propose block N+1' which is accepted by the signers OR
-/// Miner 2 should issue a new TenureChangePayload followed by a TenureExtend.
+/// Miner 2 issue a new TenureChangePayload in block N+1'
+/// Signers accept the new TenureChangePayload and the stacks tip should advance to N+1'
+/// Miner 2 issue a TenureExtend in block proposal N+2'
+/// Signers accept the TenureExtend and the stacks tip should advance to N+2'
 /// Asserts:
-/// - The stacks tip advances to N+1'
+/// - Block N+1' contains the TenureChangePayload
+/// - Block N+2 contains the TenureExtend
+/// - The stacks tip advances to N+2'
 #[test]
 #[ignore]
 fn continue_after_fast_block_no_sortition() {
@@ -5152,6 +5155,9 @@ fn continue_after_fast_block_no_sortition() {
 
     // Make sure Miner 2 cannot win a sortition at first.
     rl2_skip_commit_op.set(true);
+
+    info!("------------------------- Boot to Epoch 3.0 -------------------------");
+
     let run_loop_2_thread = thread::Builder::new()
         .name("run_loop_2".into())
         .spawn(move || run_loop_2.start(None, 0))
@@ -5170,6 +5176,11 @@ fn continue_after_fast_block_no_sortition() {
     })
     .expect("Timed out waiting for boostrapped node to catch up to the miner");
 
+    let mining_pkh_1 = Hash160::from_node_public_key(&StacksPublicKey::from_private(&conf.miner.mining_key.unwrap()));
+    let mining_pkh_2 = Hash160::from_node_public_key(&StacksPublicKey::from_private(&conf_node_2.miner.mining_key.unwrap()));
+    debug!("The miner key for miner 1 is {mining_pkh_1}");
+    debug!("The miner key for miner 2 is {mining_pkh_2}");
+
     info!("------------------------- Reached Epoch 3.0 -------------------------");
 
     let burnchain = signer_test.running_nodes.conf.get_burnchain();
@@ -5189,15 +5200,22 @@ fn continue_after_fast_block_no_sortition() {
     let starting_burn_height = get_burn_height();
     let mut btc_blocks_mined = 0;
 
-    info!("------------------------- Miner 1 Mines a Normal Tenure A -------------------------");
-    let blocks_processed_before_1 = blocks_mined1.load(Ordering::SeqCst);
     info!("------------------------- Pause Miner 1's Block Commit -------------------------");
-    // Make sure miner 1 doesn't submit a block commit for the next tenure BEFORE mining the bitcoin block
+    // Make sure miner 1 doesn't submit any further block commits for the next tenure BEFORE mining the bitcoin block
     signer_test
         .running_nodes
         .nakamoto_test_skip_commit_op
         .set(true);
 
+    info!("------------------------- Miner 1 Mines a Normal Tenure A -------------------------");
+    let blocks_processed_before_1 = blocks_mined1.load(Ordering::SeqCst);
+
+    let stacks_height_before = signer_test
+        .stacks_client
+        .get_peer_info()
+        .expect("Failed to get peer info")
+        .stacks_tip_height;
+
     signer_test
         .running_nodes
         .btc_regtest_controller
@@ -5210,7 +5228,12 @@ fn continue_after_fast_block_no_sortition() {
 
     // wait for the new block to be processed
     wait_for(60, || {
-        Ok(blocks_mined1.load(Ordering::SeqCst) > blocks_processed_before_1)
+        let stacks_height = signer_test
+            .stacks_client
+            .get_peer_info()
+            .expect("Failed to get peer info")
+            .stacks_tip_height;
+        Ok(blocks_mined1.load(Ordering::SeqCst) > blocks_processed_before_1 && stacks_height > stacks_height_before)
     })
     .unwrap();
 
@@ -5219,21 +5242,22 @@ fn continue_after_fast_block_no_sortition() {
         blocks_mined1.load(Ordering::SeqCst)
     );
 
+    info!("------------------------- Make Signers Reject All Subsequent Proposals -------------------------");
+
     let stacks_height_before = signer_test
         .stacks_client
         .get_peer_info()
         .expect("Failed to get peer info")
         .stacks_tip_height;
 
-    info!("------------------------- Make Signers Reject All Subsequent Proposals -------------------------");
     // Make all signers ignore block proposals
     let ignoring_signers = all_signers.to_vec();
     TEST_REJECT_ALL_BLOCK_PROPOSAL
         .lock()
         .unwrap()
         .replace(ignoring_signers.clone());
 
-    info!("------------------------- Unpause Miner 2's Block Commits -------------------------");
+    info!("------------------------- Submit Miner 2 Block Commit -------------------------");
     let rejections_before = signer_test
         .running_nodes
         .nakamoto_blocks_rejected
@@ -5243,7 +5267,6 @@ fn continue_after_fast_block_no_sortition() {
     // Unpause miner 2's block commits
     rl2_skip_commit_op.set(false);
 
-    info!("------------------------- Wait for Miner 2's Block Commit Submission -------------------------");
     // Ensure the miner 2 submits a block commit before mining the bitcoin block
     wait_for(30, || {
         Ok(rl2_commits.load(Ordering::SeqCst) > rl2_commits_before)
@@ -5337,7 +5360,7 @@ fn continue_after_fast_block_no_sortition() {
     let stacks_height_before = stacks_height;
 
     let blocks_processed_before_2 = blocks_mined2.load(Ordering::SeqCst);
-    info!("----- Enabling signers to approve proposals -----";
+    info!("------------------------- Enabling signers to approve proposals -------------------------";
         "stacks_height" => stacks_height_before,
     );
 
@@ -5348,6 +5371,7 @@ fn continue_after_fast_block_no_sortition() {
         .unwrap()
         .replace(Vec::new());
 
+    info!("------------------------- Mining Interim Block -------------------------");
     // submit a tx so that the miner will mine an extra block just in case due to timing constraints, the first block with the tenure extend was
     // rejected already by the signers
     let transfer_tx = make_stacks_transfer(
@@ -5383,6 +5407,10 @@ fn continue_after_fast_block_no_sortition() {
     )
     .expect("Timed out waiting for test observer to see new block");
 
+    info!(
+        "------------------------- Verify Tenure Extend Tx from Miner B -------------------------"
+    );
+
     let blocks = test_observer::get_blocks();
     let tenure_extend_block = if nmb_old_blocks + 1 == test_observer::get_blocks().len() {
         blocks.last().unwrap()