Merge pull request #5307 from stacks-network/fix/tenure-extend-burnchain-tip-check

jferrant · web-flow · commit ce46c93d5aca · 2024-10-17T17:12:39.000Z
Pass the correct burnchain tip to run_sign_v0 instead of the block election snapshot
diff --git a/.github/workflows/bitcoin-tests.yml b/.github/workflows/bitcoin-tests.yml
@@ -118,6 +118,7 @@ jobs:
           - tests::signer::v0::mine_2_nakamoto_reward_cycles
           - tests::signer::v0::signer_set_rollover
           - tests::signer::v0::signing_in_0th_tenure_of_reward_cycle
+          - tests::signer::v0::continue_after_tenure_extend
           - tests::nakamoto_integrations::burn_ops_integration_test
           - tests::nakamoto_integrations::check_block_heights
           - tests::nakamoto_integrations::clarity_burn_state
diff --git a/testnet/stacks-node/src/nakamoto_node/miner.rs b/testnet/stacks-node/src/nakamoto_node/miner.rs
@@ -531,22 +531,6 @@ impl BlockMinerThread {
             ))
         })?;
 
-        let tip = SortitionDB::get_block_snapshot_consensus(
-            sort_db.conn(),
-            &new_block.header.consensus_hash,
-        )
-        .map_err(|e| {
-            NakamotoNodeError::SigningCoordinatorFailure(format!(
-                "Failed to retrieve chain tip: {:?}",
-                e
-            ))
-        })
-        .and_then(|result| {
-            result.ok_or_else(|| {
-                NakamotoNodeError::SigningCoordinatorFailure("Failed to retrieve chain tip".into())
-            })
-        })?;
-
         let reward_set = self.load_signer_set()?;
 
         if self.config.get_node_config(false).mock_mining {
@@ -574,7 +558,7 @@ impl BlockMinerThread {
 
         let signature = coordinator.run_sign_v0(
             new_block,
-            &tip,
+            &self.burn_block,
             &self.burnchain,
             &sort_db,
             &mut chain_state,
@@ -1201,6 +1185,7 @@ 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
     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");
diff --git a/testnet/stacks-node/src/nakamoto_node/sign_coordinator.rs b/testnet/stacks-node/src/nakamoto_node/sign_coordinator.rs
@@ -251,11 +251,11 @@ impl SignCoordinator {
     }
 
     /// Check if the tenure needs to change
-    fn check_burn_tip_changed(sortdb: &SortitionDB, consensus_hash: &ConsensusHash) -> bool {
+    fn check_burn_tip_changed(sortdb: &SortitionDB, burn_block: &BlockSnapshot) -> bool {
         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 != *consensus_hash {
+        if cur_burn_chain_tip.consensus_hash != burn_block.consensus_hash {
             info!("SignCoordinator: Cancel signature aggregation; burnchain tip has changed");
             true
         } else {
@@ -365,7 +365,7 @@ impl SignCoordinator {
                 return Ok(stored_block.header.signer_signature);
             }
 
-            if Self::check_burn_tip_changed(&sortdb, &burn_tip.consensus_hash) {
+            if Self::check_burn_tip_changed(&sortdb, &burn_tip) {
                 debug!("SignCoordinator: Exiting due to new burnchain tip");
                 return Err(NakamotoNodeError::BurnchainTipChanged);
             }
diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs
@@ -6921,10 +6921,10 @@ fn continue_tenure_extend() {
     // setup sender + recipient for a test stx transfer
     let sender_addr = tests::to_addr(&sender_sk);
     let send_amt = 1000;
-    let send_fee = 100;
+    let send_fee = 200;
     naka_conf.add_initial_balance(
         PrincipalData::from(sender_addr.clone()).to_string(),
-        send_amt * 2 + send_fee,
+        (send_amt + send_fee) * 20,
     );
     let sender_signer_sk = Secp256k1PrivateKey::new();
     let sender_signer_addr = tests::to_addr(&sender_signer_sk);
@@ -6934,6 +6934,7 @@ fn continue_tenure_extend() {
     );
     let recipient = PrincipalData::from(StacksAddress::burn_address(false));
     let stacker_sk = setup_stacker(&mut naka_conf);
+    let mut transfer_nonce = 0;
 
     test_observer::spawn();
     test_observer::register_any(&mut naka_conf);
@@ -7054,7 +7055,7 @@ fn continue_tenure_extend() {
     // Submit a TX
     let transfer_tx = make_stacks_transfer(
         &sender_sk,
-        0,
+        transfer_nonce,
         send_fee,
         naka_conf.burnchain.chain_id,
         &recipient,
@@ -7116,6 +7117,26 @@ fn continue_tenure_extend() {
     })
     .unwrap();
 
+    // Mine 3 nakamoto blocks
+    for i in 0..3 {
+        info!("Triggering Nakamoto blocks after extend ({})", i + 1);
+        transfer_nonce += 1;
+        let transfer_tx = make_stacks_transfer(
+            &sender_sk,
+            transfer_nonce,
+            send_fee,
+            naka_conf.burnchain.chain_id,
+            &recipient,
+            send_amt,
+        );
+        submit_tx(&http_origin, &transfer_tx);
+        wait_for(10, || {
+            let sender_nonce = get_account(&http_origin, &to_addr(&sender_sk)).nonce;
+            Ok(sender_nonce >= transfer_nonce)
+        })
+        .expect("Timed out waiting for transfer TX to confirm");
+    }
+
     info!("Resuming commit ops to mine regular tenures.");
     test_skip_commit_op.0.lock().unwrap().replace(false);
 
@@ -7153,7 +7174,9 @@ fn continue_tenure_extend() {
     let mut tenure_extends = vec![];
     let mut tenure_block_founds = vec![];
     let mut transfer_tx_included = false;
+    let mut last_block_had_extend = false;
     for block in test_observer::get_blocks() {
+        let mut has_extend = false;
         for tx in block["transactions"].as_array().unwrap() {
             let raw_tx = tx["raw_tx"].as_str().unwrap();
             if raw_tx == &transfer_tx_hex {
@@ -7167,12 +7190,21 @@ fn continue_tenure_extend() {
             let parsed = StacksTransaction::consensus_deserialize(&mut &tx_bytes[..]).unwrap();
             match &parsed.payload {
                 TransactionPayload::TenureChange(payload) => match payload.cause {
-                    TenureChangeCause::Extended => tenure_extends.push(parsed),
-                    TenureChangeCause::BlockFound => tenure_block_founds.push(parsed),
+                    TenureChangeCause::Extended => {
+                        has_extend = true;
+                        tenure_extends.push(parsed);
+                    }
+                    TenureChangeCause::BlockFound => {
+                        if last_block_had_extend {
+                            panic!("Expected a Nakamoto block to happen after tenure extend block");
+                        }
+                        tenure_block_founds.push(parsed);
+                    }
                 },
                 _ => {}
             };
         }
+        last_block_had_extend = has_extend;
     }
     assert!(
         !tenure_extends.is_empty(),
diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs
@@ -34,6 +34,7 @@ use stacks::chainstate::nakamoto::{NakamotoBlock, NakamotoBlockHeader, NakamotoC
 use stacks::chainstate::stacks::address::PoxAddress;
 use stacks::chainstate::stacks::boot::MINERS_NAME;
 use stacks::chainstate::stacks::db::{StacksBlockHeaderTypes, StacksChainState, StacksHeaderInfo};
+use stacks::chainstate::stacks::{StacksTransaction, TenureChangeCause, TransactionPayload};
 use stacks::codec::StacksMessageCodec;
 use stacks::core::{StacksEpochId, CHAIN_ID_TESTNET};
 use stacks::libstackerdb::StackerDBChunkData;
@@ -42,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::MerkleHashFunc;
+use stacks::util::hash::{hex_bytes, MerkleHashFunc};
 use stacks::util::secp256k1::{Secp256k1PrivateKey, Secp256k1PublicKey};
 use stacks::util_lib::boot::boot_code_id;
 use stacks::util_lib::signed_structured_data::pox4::{
@@ -5077,6 +5078,143 @@ fn miner_recovers_when_broadcast_block_delay_across_tenures_occurs() {
     assert_ne!(block_n_2, block_n);
 }
 
+#[test]
+#[ignore]
+/// Test that we can mine a tenure extend and then continue mining afterwards.
+fn continue_after_tenure_extend() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+
+    tracing_subscriber::registry()
+        .with(fmt::layer())
+        .with(EnvFilter::from_default_env())
+        .init();
+
+    info!("------------------------- Test Setup -------------------------");
+    let num_signers = 5;
+    let sender_sk = Secp256k1PrivateKey::new();
+    let sender_addr = tests::to_addr(&sender_sk);
+    let recipient = PrincipalData::from(StacksAddress::burn_address(false));
+    let send_amt = 100;
+    let send_fee = 180;
+    let mut signer_test: SignerTest<SpawnedSigner> = SignerTest::new(
+        num_signers,
+        vec![(sender_addr.clone(), (send_amt + send_fee) * 5)],
+    );
+    let timeout = Duration::from_secs(200);
+    let coord_channel = signer_test.running_nodes.coord_channel.clone();
+    let http_origin = format!("http://{}", &signer_test.running_nodes.conf.node.rpc_bind);
+
+    signer_test.boot_to_epoch_3();
+
+    info!("------------------------- Mine Normal Tenure -------------------------");
+    signer_test.mine_and_verify_confirmed_naka_block(timeout, num_signers);
+
+    info!("------------------------- Extend Tenure -------------------------");
+    signer_test
+        .running_nodes
+        .nakamoto_test_skip_commit_op
+        .0
+        .lock()
+        .unwrap()
+        .replace(true);
+
+    // It's possible that we have a pending block commit already.
+    // Mine two BTC blocks to "flush" this commit.
+    let burn_height = signer_test
+        .stacks_client
+        .get_peer_info()
+        .expect("Failed to get peer info")
+        .burn_block_height;
+    for i in 0..2 {
+        info!(
+            "------------- After pausing commits, triggering 2 BTC blocks: ({} of 2) -----------",
+            i + 1
+        );
+
+        let blocks_processed_before = coord_channel
+            .lock()
+            .expect("Mutex poisoned")
+            .get_stacks_blocks_processed();
+        signer_test
+            .running_nodes
+            .btc_regtest_controller
+            .build_next_block(1);
+
+        wait_for(60, || {
+            let blocks_processed_after = coord_channel
+                .lock()
+                .expect("Mutex poisoned")
+                .get_stacks_blocks_processed();
+            Ok(blocks_processed_after > blocks_processed_before)
+        })
+        .expect("Timed out waiting for tenure extend block");
+    }
+
+    wait_for(30, || {
+        let new_burn_height = signer_test
+            .stacks_client
+            .get_peer_info()
+            .expect("Failed to get peer info")
+            .burn_block_height;
+        Ok(new_burn_height == burn_height + 2)
+    })
+    .expect("Timed out waiting for burnchain to advance");
+
+    // The last block should have a single instruction in it, the tenure extend
+    let blocks = test_observer::get_blocks();
+    let last_block = blocks.last().unwrap();
+    let transactions = last_block["transactions"].as_array().unwrap();
+    let tx = transactions.first().expect("No transactions in block");
+    let raw_tx = tx["raw_tx"].as_str().unwrap();
+    let tx_bytes = hex_bytes(&raw_tx[2..]).unwrap();
+    let parsed = StacksTransaction::consensus_deserialize(&mut &tx_bytes[..]).unwrap();
+    match &parsed.payload {
+        TransactionPayload::TenureChange(payload)
+            if payload.cause == TenureChangeCause::Extended => {}
+        _ => panic!("Expected tenure extend transaction, got {:?}", parsed),
+    };
+
+    // Verify that the miner can continue mining in the tenure with the tenure extend
+    info!("------------------------- Mine After Tenure Extend -------------------------");
+    let mut sender_nonce = 0;
+    let mut blocks_processed_before = coord_channel
+        .lock()
+        .expect("Mutex poisoned")
+        .get_stacks_blocks_processed();
+    for _ in 0..5 {
+        // submit a tx so that the miner will mine an extra block
+        let transfer_tx = make_stacks_transfer(
+            &sender_sk,
+            sender_nonce,
+            send_fee,
+            signer_test.running_nodes.conf.burnchain.chain_id,
+            &recipient,
+            send_amt,
+        );
+        sender_nonce += 1;
+        submit_tx(&http_origin, &transfer_tx);
+
+        info!("Submitted transfer tx and waiting for block proposal");
+        wait_for(30, || {
+            let blocks_processed_after = coord_channel
+                .lock()
+                .expect("Mutex poisoned")
+                .get_stacks_blocks_processed();
+            Ok(blocks_processed_after > blocks_processed_before)
+        })
+        .expect("Timed out waiting for block proposal");
+        blocks_processed_before = coord_channel
+            .lock()
+            .expect("Mutex poisoned")
+            .get_stacks_blocks_processed();
+        info!("Block {blocks_processed_before} processed, continuing");
+    }
+
+    signer_test.shutdown();
+}
+
 #[test]
 #[ignore]
 /// Test that signers can successfully sign a block proposal in the 0th tenure of a reward cycle

Original file line number	Diff line number	Diff line change
`@@ -251,11 +251,11 @@ impl SignCoordinator {`
`251`	`251`	`}`
`252`	`252`
`253`	`253`	`/// Check if the tenure needs to change`
`254`		`- fn check_burn_tip_changed(sortdb: &SortitionDB, consensus_hash: &ConsensusHash) -> bool {`
	`254`	`+ fn check_burn_tip_changed(sortdb: &SortitionDB, burn_block: &BlockSnapshot) -> bool {`
`255`	`255`	`let cur_burn_chain_tip = SortitionDB::get_canonical_burn_chain_tip(sortdb.conn())`
`256`	`256`	`.expect("FATAL: failed to query sortition DB for canonical burn chain tip");`
`257`	`257`
`258`		`- if cur_burn_chain_tip.consensus_hash != *consensus_hash {`
	`258`	`+ if cur_burn_chain_tip.consensus_hash != burn_block.consensus_hash {`
`259`	`259`	`info!("SignCoordinator: Cancel signature aggregation; burnchain tip has changed");`
`260`	`260`	`true`
`261`	`261`	`} else {`
`@@ -365,7 +365,7 @@ impl SignCoordinator {`
`365`	`365`	`return Ok(stored_block.header.signer_signature);`
`366`	`366`	`}`
`367`	`367`
`368`		`- if Self::check_burn_tip_changed(&sortdb, &burn_tip.consensus_hash) {`
	`368`	`+ if Self::check_burn_tip_changed(&sortdb, &burn_tip) {`
`369`	`369`	`debug!("SignCoordinator: Exiting due to new burnchain tip");`
`370`	`370`	`return Err(NakamotoNodeError::BurnchainTipChanged);`
`371`	`371`	`}`