From 5b330cfa044b94704d5f47d0568d40260c22c9e9 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Tue, 17 Sep 2024 22:00:22 -0400 Subject: [PATCH 1/3] fix: run nakamoto inv, downloader, and natpunch state machines once per PeerNetwork::run() --- stackslib/src/net/p2p.rs | 117 ++++++++++++--------------------------- 1 file changed, 35 insertions(+), 82 deletions(-) diff --git a/stackslib/src/net/p2p.rs b/stackslib/src/net/p2p.rs index 45183cdf1b..7b36dc3c33 100644 --- a/stackslib/src/net/p2p.rs +++ b/stackslib/src/net/p2p.rs @@ -3560,8 +3560,8 @@ impl PeerNetwork { let prune = if cur_epoch.epoch_id >= StacksEpochId::Epoch30 { debug!("{:?}: run Nakamoto work loop", self.get_local_peer()); - // in Nakamoto epoch, so do Nakamoto things - let prune = self.do_network_work_nakamoto( + // in Nakamoto epoch, so we can always prune + self.do_network_work_nakamoto( burnchain_height, sortdb, chainstate, @@ -3593,9 +3593,10 @@ impl PeerNetwork { "{:?}: ran Epoch 2.x work loop in Nakamoto epoch", self.get_local_peer() ); - prune || epoch2_prune + epoch2_prune } else { - prune + // we can always prune in Nakamoto, since all state machines pin their connections + true } } else { // in epoch 2.x, so do epoch 2.x things @@ -3623,89 +3624,41 @@ impl PeerNetwork { chainstate: &mut StacksChainState, ibd: bool, network_result: &mut NetworkResult, - ) -> bool { - // do some Actual Work(tm) - let mut do_prune = false; - let mut did_cycle = false; - - while !did_cycle { - // always do an inv sync - let learned = self.do_network_inv_sync_nakamoto(sortdb, ibd); - debug!( - "{:?}: network work state is {:?}", - self.get_local_peer(), - &self.nakamoto_work_state; - "learned_new_blocks?" => learned - ); - - // always do block download - let new_blocks = self - .do_network_block_sync_nakamoto(burnchain_height, sortdb, chainstate, ibd) - .map_err(|e| { - warn!( - "{:?}: Failed to perform Nakamoto block sync: {:?}", - &self.get_local_peer(), - &e - ); - e - }) - .unwrap_or(HashMap::new()); - - network_result.consume_nakamoto_blocks(new_blocks); - - let cur_state = self.nakamoto_work_state; - match self.nakamoto_work_state { - PeerNetworkWorkState::GetPublicIP => { - if cfg!(test) && self.connection_opts.disable_natpunch { - self.nakamoto_work_state = PeerNetworkWorkState::BlockDownload; - } else { - // (re)determine our public IP address - let done = self.do_get_public_ip(); - if done { - self.nakamoto_work_state = PeerNetworkWorkState::BlockDownload; - } - } - } - PeerNetworkWorkState::BlockInvSync => { - // this state is useless in Nakamoto since we're always doing inv-syncs - self.nakamoto_work_state = PeerNetworkWorkState::BlockDownload; - } - PeerNetworkWorkState::BlockDownload => { - // this state is useless in Nakamoto since we're always doing download-syncs - self.nakamoto_work_state = PeerNetworkWorkState::AntiEntropy; - } - PeerNetworkWorkState::AntiEntropy => { - debug!( - "{:?}: Block anti-entropy for Nakamoto is not yet implemented", - self.get_local_peer() - ); - self.nakamoto_work_state = PeerNetworkWorkState::Prune; - } - PeerNetworkWorkState::Prune => { - // did one pass - did_cycle = true; - do_prune = true; + ) { + // always do an inv sync + let learned = self.do_network_inv_sync_nakamoto(sortdb, ibd); + debug!( + "{:?}: network work state is {:?}", + self.get_local_peer(), + &self.nakamoto_work_state; + "learned_new_blocks?" => learned + ); - // restart - self.nakamoto_work_state = PeerNetworkWorkState::GetPublicIP; - } - } + // always do block download + let new_blocks = self + .do_network_block_sync_nakamoto(burnchain_height, sortdb, chainstate, ibd) + .map_err(|e| { + warn!( + "{:?}: Failed to perform Nakamoto block sync: {:?}", + &self.get_local_peer(), + &e + ); + e + }) + .unwrap_or(HashMap::new()); - if self.nakamoto_work_state == cur_state { - // only break early if we can't make progress - break; - } - } + network_result.consume_nakamoto_blocks(new_blocks); - if did_cycle { - self.num_state_machine_passes += 1; - debug!( - "{:?}: Finished full p2p state-machine pass for Nakamoto ({})", - &self.local_peer, self.num_state_machine_passes - ); + // make sure our public IP is fresh (this self-throttles if we recently learned it). + if !self.connection_opts.disable_natpunch { + self.do_get_public_ip(); } - do_prune + self.num_state_machine_passes += 1; + debug!( + "{:?}: Finished full p2p state-machine pass for Nakamoto ({})", + &self.local_peer, self.num_state_machine_passes + ); } /// Do the actual work in the state machine. From 7c073faaa2051a903347a9e7cc642a2d60d9db07 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 18 Sep 2024 11:12:14 -0700 Subject: [PATCH 2/3] Cannot assume stacks transaction will get mined AFTER the burn block is mined Signed-off-by: Jacinta Ferrant --- .../src/tests/nakamoto_integrations.rs | 44 +++---------------- 1 file changed, 7 insertions(+), 37 deletions(-) diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index 32924ab7b7..7a71725f6c 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -5185,51 +5185,21 @@ fn clarity_burn_state() { vec![&Value::UInt(burn_block_height)], ); result.expect_result_ok().expect("Read-only call failed"); - - // Submit a tx for the next block (the next block will be a new tenure, so the burn block height will increment) - let call_tx = tests::make_contract_call( - &sender_sk, - sender_nonce, - tx_fee, - &sender_addr, - contract_name, - "bar", - &[Value::UInt(burn_block_height + 1)], - ); - sender_nonce += 1; - submit_tx(&http_origin, &call_tx); } let commits_before = commits_submitted.load(Ordering::SeqCst); - next_block_and_process_new_stacks_block(&mut btc_regtest_controller, 60, &coord_channel) - .unwrap(); + next_block_and_mine_commit( + &mut btc_regtest_controller, + 60, + &coord_channel, + &commits_submitted, + ) + .unwrap(); let info = get_chain_info(&naka_conf); burn_block_height = info.burn_block_height as u128; info!("Expecting burn block height to be {}", burn_block_height); - // Assert that the contract call was successful - test_observer::get_mined_nakamoto_blocks() - .last() - .unwrap() - .tx_events - .iter() - .for_each(|event| match event { - TransactionEvent::Success(TransactionSuccessEvent { result, fee, .. }) => { - // Ignore coinbase and tenure transactions - if *fee == 0 { - return; - } - - info!("Contract call result: {}", result); - result.clone().expect_result_ok().expect("Ok result"); - } - _ => { - info!("Unsuccessful event: {:?}", event); - panic!("Expected a successful transaction"); - } - }); - // mine the interim blocks for interim_block_ix in 0..inter_blocks_per_tenure { info!("Mining interim block {interim_block_ix}"); From d9c002c70f56a825ba318c2e4731914d360ca6e0 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 18 Sep 2024 12:42:53 -0700 Subject: [PATCH 3/3] CRC: ensure that the tenure change transaction and contract call get mined in the same stacks block Signed-off-by: Jacinta Ferrant --- .../stacks-node/src/nakamoto_node/miner.rs | 11 ++++ .../src/tests/nakamoto_integrations.rs | 63 ++++++++++++++++--- 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/testnet/stacks-node/src/nakamoto_node/miner.rs b/testnet/stacks-node/src/nakamoto_node/miner.rs index da1c75c708..1a5f4aa3c2 100644 --- a/testnet/stacks-node/src/nakamoto_node/miner.rs +++ b/testnet/stacks-node/src/nakamoto_node/miner.rs @@ -60,6 +60,8 @@ use crate::neon_node; use crate::run_loop::nakamoto::Globals; use crate::run_loop::RegisteredKey; +#[cfg(test)] +pub static TEST_MINE_STALL: std::sync::Mutex> = std::sync::Mutex::new(None); #[cfg(test)] pub static TEST_BROADCAST_STALL: std::sync::Mutex> = std::sync::Mutex::new(None); #[cfg(test)] @@ -291,6 +293,15 @@ impl BlockMinerThread { let mut attempts = 0; // now, actually run this tenure loop { + #[cfg(test)] + if *TEST_MINE_STALL.lock().unwrap() == Some(true) { + // Do an extra check just so we don't log EVERY time. + warn!("Mining is stalled due to testing directive"); + while *TEST_MINE_STALL.lock().unwrap() == Some(true) { + std::thread::sleep(std::time::Duration::from_millis(10)); + } + warn!("Mining is no longer stalled due to testing directive. Continuing..."); + } 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 diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index 7a71725f6c..6fa0dafc88 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -94,7 +94,9 @@ use wsts::net::Message; use super::bitcoin_regtest::BitcoinCoreController; use crate::config::{EventKeyType, EventObserverConfig, InitialBalance}; -use crate::nakamoto_node::miner::{TEST_BLOCK_ANNOUNCE_STALL, TEST_BROADCAST_STALL}; +use crate::nakamoto_node::miner::{ + TEST_BLOCK_ANNOUNCE_STALL, TEST_BROADCAST_STALL, TEST_MINE_STALL, +}; use crate::neon::{Counters, RunLoopCounter}; use crate::operations::BurnchainOpSigner; use crate::run_loop::boot_nakamoto; @@ -5185,21 +5187,68 @@ fn clarity_burn_state() { vec![&Value::UInt(burn_block_height)], ); result.expect_result_ok().expect("Read-only call failed"); + + // Pause mining to prevent the stacks block from being mined before the tenure change is processed + TEST_MINE_STALL.lock().unwrap().replace(true); + // Submit a tx for the next block (the next block will be a new tenure, so the burn block height will increment) + let call_tx = tests::make_contract_call( + &sender_sk, + sender_nonce, + tx_fee, + &sender_addr, + contract_name, + "bar", + &[Value::UInt(burn_block_height + 1)], + ); + sender_nonce += 1; + submit_tx(&http_origin, &call_tx); } let commits_before = commits_submitted.load(Ordering::SeqCst); - next_block_and_mine_commit( - &mut btc_regtest_controller, - 60, - &coord_channel, - &commits_submitted, - ) + let blocks_processed_before = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); + next_block_and(&mut btc_regtest_controller, 60, || { + Ok(commits_submitted.load(Ordering::SeqCst) > commits_before) + }) + .unwrap(); + TEST_MINE_STALL.lock().unwrap().replace(false); + wait_for(20, || { + Ok(coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed() + > blocks_processed_before) + }) .unwrap(); let info = get_chain_info(&naka_conf); burn_block_height = info.burn_block_height as u128; info!("Expecting burn block height to be {}", burn_block_height); + // Assert that the contract call was successful + test_observer::get_mined_nakamoto_blocks() + .last() + .unwrap() + .tx_events + .iter() + .for_each(|event| match event { + TransactionEvent::Success(TransactionSuccessEvent { result, fee, .. }) => { + // Ignore coinbase and tenure transactions + if *fee == 0 { + return; + } + + info!("Contract call result: {}", result); + result.clone().expect_result_ok().expect("Ok result"); + } + _ => { + info!("Unsuccessful event: {:?}", event); + panic!("Expected a successful transaction"); + } + }); + // mine the interim blocks for interim_block_ix in 0..inter_blocks_per_tenure { info!("Mining interim block {interim_block_ix}");