Merge pull request #5752 from stacks-network/feat/better-win-detection

Fix: better win detection on restart

Author: hstove

.github/workflows/bitcoin-tests.yml

@@ -170,6 +170,7 @@ jobs:
           - tests::nakamoto_integrations::v3_blockbyheight_api_endpoint
           - tests::nakamoto_integrations::mine_invalid_principal_from_consensus_buff
           - tests::nakamoto_integrations::test_tenure_extend_from_flashblocks
+          - tests::nakamoto_integrations::restarting_miner
           # TODO: enable these once v1 signer is supported by a new nakamoto epoch
           # - tests::signer::v1::dkg
           # - tests::signer::v1::sign_request_rejected

CHANGELOG.md

@@ -18,6 +18,11 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 
 - Miner will include other transactions in blocks with tenure extend transactions (#5760)
 
+### Fixed
+
+- Miners who restart their nodes immediately before a winning tenure now correctly detect that
+  they won the tenure after their nodes restart ([#5750](https://github.com/stacks-network/stacks-core/issues/5750)).
+
 ## [3.1.0.0.4]
 
 ### Added

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

@@ -694,6 +694,9 @@ impl RelayerThread {
     /// this sortition matches the sortition tip and we have a parent to build atop.
     ///
     /// Otherwise, returns None, meaning no action will be taken.
+    // This method is covered by the e2e bitcoind tests, which do not show up
+    //  in mutant coverage.
+    #[cfg_attr(test, mutants::skip)]
     fn process_sortition(
         &mut self,
         consensus_hash: ConsensusHash,
@@ -705,8 +708,16 @@ impl RelayerThread {
             .expect("FATAL: unknown consensus hash");
 
         // always clear this even if this isn't the latest sortition
-        let cleared = self.last_commits.remove(&sn.winning_block_txid);
-        let won_sortition = sn.sortition && cleared;
+        let _cleared = self.last_commits.remove(&sn.winning_block_txid);
+        let was_winning_pkh = if let (Some(ref winning_pkh), Some(ref my_pkh)) =
+            (sn.miner_pk_hash, self.get_mining_key_pkh())
+        {
+            winning_pkh == my_pkh
+        } else {
+            false
+        };
+
+        let won_sortition = sn.sortition && was_winning_pkh;
         if won_sortition {
             increment_stx_blocks_mined_counter();
         }

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

@@ -1746,6 +1746,226 @@ fn simple_neon_integration() {
     run_loop_thread.join().unwrap();
 }
 
+#[test]
+#[ignore]
+/// Test a scenario in which a miner is restarted right before a tenure
+///  which they won. The miner, on restart, should begin mining the new tenure.
+fn restarting_miner() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+
+    let (mut naka_conf, _miner_account) = naka_neon_integration_conf(None);
+    let prom_bind = "127.0.0.1:6000".to_string();
+    naka_conf.node.prometheus_bind = Some(prom_bind.clone());
+    naka_conf.miner.activated_vrf_key_path =
+        Some(format!("{}/vrf_key", naka_conf.node.working_dir));
+    naka_conf.miner.wait_on_interim_blocks = Duration::from_secs(5);
+    let sender_sk = Secp256k1PrivateKey::from_seed(&[1, 2, 1, 2, 1, 2]);
+    // setup sender + recipient for a test stx transfer
+    let sender_addr = tests::to_addr(&sender_sk);
+    let send_amt = 1000;
+    let send_fee = 100;
+    naka_conf.add_initial_balance(
+        PrincipalData::from(sender_addr).to_string(),
+        send_amt * 2 + send_fee,
+    );
+    let sender_signer_sk = Secp256k1PrivateKey::from_seed(&[3, 2, 3, 2, 3, 2]);
+    let sender_signer_addr = tests::to_addr(&sender_signer_sk);
+    let mut signers = TestSigners::new(vec![sender_signer_sk]);
+    naka_conf.add_initial_balance(PrincipalData::from(sender_signer_addr).to_string(), 100000);
+    let stacker_sk = setup_stacker(&mut naka_conf);
+
+    test_observer::spawn();
+    test_observer::register_any(&mut naka_conf);
+
+    let mut btcd_controller = BitcoinCoreController::new(naka_conf.clone());
+    btcd_controller
+        .start_bitcoind()
+        .expect("Failed starting bitcoind");
+    let mut btc_regtest_controller = BitcoinRegtestController::new(naka_conf.clone(), None);
+    btc_regtest_controller.bootstrap_chain(201);
+
+    let mut run_loop = boot_nakamoto::BootRunLoop::new(naka_conf.clone()).unwrap();
+    let run_loop_stopper = run_loop.get_termination_switch();
+    let Counters {
+        blocks_processed,
+        naka_submitted_commits: commits_submitted,
+        naka_proposed_blocks: proposals_submitted,
+        ..
+    } = run_loop.counters();
+    let coord_channel = run_loop.coordinator_channels();
+
+    let mut run_loop_2 = boot_nakamoto::BootRunLoop::new(naka_conf.clone()).unwrap();
+    let _run_loop_2_stopper = run_loop.get_termination_switch();
+    let Counters {
+        blocks_processed: blocks_processed_2,
+        naka_submitted_commits: commits_submitted_2,
+        naka_proposed_blocks: proposals_submitted_2,
+        ..
+    } = run_loop_2.counters();
+    let coord_channel_2 = run_loop_2.coordinator_channels();
+
+    let run_loop_thread = thread::spawn(move || run_loop.start(None, 0));
+    wait_for_runloop(&blocks_processed);
+    boot_to_epoch_3(
+        &naka_conf,
+        &blocks_processed,
+        &[stacker_sk],
+        &[sender_signer_sk],
+        &mut Some(&mut signers),
+        &mut btc_regtest_controller,
+    );
+
+    info!("Bootstrapped to Epoch-3.0 boundary, starting nakamoto miner");
+
+    let burnchain = naka_conf.get_burnchain();
+    let sortdb = burnchain.open_sortition_db(true).unwrap();
+    let (chainstate, _) = StacksChainState::open(
+        naka_conf.is_mainnet(),
+        naka_conf.burnchain.chain_id,
+        &naka_conf.get_chainstate_path_str(),
+        None,
+    )
+    .unwrap();
+
+    let block_height_pre_3_0 =
+        NakamotoChainState::get_canonical_block_header(chainstate.db(), &sortdb)
+            .unwrap()
+            .unwrap()
+            .stacks_block_height;
+
+    info!("Nakamoto miner started...");
+    blind_signer_multinode(
+        &signers,
+        &[&naka_conf, &naka_conf],
+        vec![proposals_submitted, proposals_submitted_2],
+    );
+
+    wait_for_first_naka_block_commit(60, &commits_submitted);
+
+    // Mine 2 nakamoto tenures
+    for _i in 0..2 {
+        next_block_and_mine_commit(
+            &mut btc_regtest_controller,
+            60,
+            &coord_channel,
+            &commits_submitted,
+        )
+        .unwrap();
+    }
+
+    let last_tip = NakamotoChainState::get_canonical_block_header(chainstate.db(), &sortdb)
+        .unwrap()
+        .unwrap();
+    info!(
+        "Latest tip";
+        "height" => last_tip.stacks_block_height,
+        "is_nakamoto" => last_tip.anchored_header.as_stacks_nakamoto().is_some(),
+    );
+
+    // close the current miner
+    coord_channel
+        .lock()
+        .expect("Mutex poisoned")
+        .stop_chains_coordinator();
+    run_loop_stopper.store(false, Ordering::SeqCst);
+    run_loop_thread.join().unwrap();
+
+    // mine a bitcoin block -- this should include a winning commit from
+    //  the miner
+    btc_regtest_controller.build_next_block(1);
+
+    // start it back up
+
+    let _run_loop_thread = thread::spawn(move || run_loop_2.start(None, 0));
+    wait_for_runloop(&blocks_processed_2);
+
+    info!(" ================= RESTARTED THE MINER =================");
+
+    let tip = NakamotoChainState::get_canonical_block_header(chainstate.db(), &sortdb)
+        .unwrap()
+        .unwrap();
+    info!(
+        "Latest tip";
+        "height" => tip.stacks_block_height,
+        "is_nakamoto" => tip.anchored_header.as_stacks_nakamoto().is_some(),
+    );
+
+    wait_for(60, || {
+        let tip = NakamotoChainState::get_canonical_block_header(chainstate.db(), &sortdb)
+            .unwrap()
+            .unwrap();
+        Ok(tip.stacks_block_height > last_tip.stacks_block_height)
+    })
+    .unwrap_or_else(|e| {
+        let tip = NakamotoChainState::get_canonical_block_header(chainstate.db(), &sortdb)
+            .unwrap()
+            .unwrap();
+
+        error!(
+            "Failed to get a new block after restart";
+            "last_tip_height" => last_tip.stacks_block_height,
+            "latest_tip" => tip.stacks_block_height,
+            "error" => &e,
+        );
+
+        panic!("{e}")
+    });
+
+    // Mine 2 more nakamoto tenures
+    for _i in 0..2 {
+        next_block_and_mine_commit(
+            &mut btc_regtest_controller,
+            60,
+            &coord_channel_2,
+            &commits_submitted_2,
+        )
+        .unwrap();
+    }
+
+    // load the chain tip, and assert that it is a nakamoto block and at least 30 blocks have advanced in epoch 3
+    let tip = NakamotoChainState::get_canonical_block_header(chainstate.db(), &sortdb)
+        .unwrap()
+        .unwrap();
+    info!(
+        "=== Last tip ===";
+        "height" => tip.stacks_block_height,
+        "is_nakamoto" => tip.anchored_header.as_stacks_nakamoto().is_some(),
+    );
+
+    assert!(tip.anchored_header.as_stacks_nakamoto().is_some());
+
+    // Check that we aren't missing burn blocks
+    let bhh = u64::from(tip.burn_header_height);
+    // make sure every burn block after the nakamoto transition has a mined
+    //  nakamoto block in it.
+    let missing = test_observer::get_missing_burn_blocks(220..=bhh).unwrap();
+
+    // This test was flakey because it was sometimes missing burn block 230, which is right at the Nakamoto transition
+    // So it was possible to miss a burn block during the transition
+    // But I don't it matters at this point since the Nakamoto transition has already happened on mainnet
+    // So just print a warning instead, don't count it as an error
+    let missing_is_error: Vec<_> = missing
+        .into_iter()
+        .filter(|i| match i {
+            230 => {
+                warn!("Missing burn block {i}");
+                false
+            }
+            _ => true,
+        })
+        .collect();
+
+    if !missing_is_error.is_empty() {
+        panic!("Missing the following burn blocks: {missing_is_error:?}");
+    }
+
+    check_nakamoto_empty_block_heuristics();
+
+    assert!(tip.stacks_block_height >= block_height_pre_3_0 + 4);
+}
+
 #[test]
 #[ignore]
 /// This test spins up a nakamoto-neon node.