Merge branch 'develop' into feat/pox-4-tests-nakamoto

Author: jcnelson

.github/workflows/bitcoin-tests.yml

@@ -98,6 +98,7 @@ jobs:
           - tests::signer::v0::miner_forking
           - tests::signer::v0::reloads_signer_set_in
           - tests::signer::v0::signers_broadcast_signed_blocks
+          - tests::signer::v0::min_gap_between_blocks
           - tests::nakamoto_integrations::stack_stx_burn_op_integration_test
           - tests::nakamoto_integrations::check_block_heights
           - tests::nakamoto_integrations::clarity_burn_state

testnet/stacks-node/src/config.rs

@@ -86,6 +86,7 @@ pub const OP_TX_ANY_ESTIM_SIZE: u64 = fmax!(
 const DEFAULT_MAX_RBF_RATE: u64 = 150; // 1.5x
 const DEFAULT_RBF_FEE_RATE_INCREMENT: u64 = 5;
 const INV_REWARD_CYCLES_TESTNET: u64 = 6;
+const DEFAULT_MIN_TIME_BETWEEN_BLOCKS_MS: u64 = 1000;
 
 #[derive(Clone, Deserialize, Default, Debug)]
 pub struct ConfigFile {
@@ -2358,6 +2359,9 @@ pub struct MinerConfig {
     pub wait_on_signers: Duration,
     /// Whether to mock sign in Epoch 2.5 through the .miners and .signers contracts. This is used for testing purposes in Epoch 2.5 only.
     pub pre_nakamoto_mock_signing: bool,
+    /// The minimum time to wait between mining blocks in milliseconds. The value must be greater than or equal to 1000 ms because if a block is mined
+    /// within the same second as its parent, it will be rejected by the signers.
+    pub min_time_between_blocks_ms: u64,
 }
 
 impl Default for MinerConfig {
@@ -2389,6 +2393,7 @@ impl Default for MinerConfig {
             // TODO: update to a sane value based on stackerdb benchmarking
             wait_on_signers: Duration::from_secs(200),
             pre_nakamoto_mock_signing: false, // Should only default true if mining key is set
+            min_time_between_blocks_ms: DEFAULT_MIN_TIME_BETWEEN_BLOCKS_MS,
         }
     }
 }
@@ -2739,6 +2744,7 @@ pub struct MinerConfigFile {
     pub max_reorg_depth: Option<u64>,
     pub wait_on_signers_ms: Option<u64>,
     pub pre_nakamoto_mock_signing: Option<bool>,
+    pub min_time_between_blocks_ms: Option<u64>,
 }
 
 impl MinerConfigFile {
@@ -2850,6 +2856,12 @@ impl MinerConfigFile {
             pre_nakamoto_mock_signing: self
                 .pre_nakamoto_mock_signing
                 .unwrap_or(pre_nakamoto_mock_signing), // Should only default true if mining key is set
+                min_time_between_blocks_ms: self.min_time_between_blocks_ms.map(|ms| if ms < DEFAULT_MIN_TIME_BETWEEN_BLOCKS_MS {
+                warn!("miner.min_time_between_blocks_ms is less than the minimum allowed value of {DEFAULT_MIN_TIME_BETWEEN_BLOCKS_MS} ms. Using the default value instead.");
+                DEFAULT_MIN_TIME_BETWEEN_BLOCKS_MS
+            } else {
+                ms
+            }).unwrap_or(miner_default_config.min_time_between_blocks_ms),
         })
     }
 }

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

@@ -45,6 +45,7 @@ use stacks::chainstate::stacks::{
 use stacks::net::p2p::NetworkHandle;
 use stacks::net::stackerdb::StackerDBs;
 use stacks::net::{NakamotoBlocksData, StacksMessageType};
+use stacks::util::get_epoch_time_secs;
 use stacks::util::secp256k1::MessageSignature;
 use stacks_common::codec::read_next;
 use stacks_common::types::chainstate::{StacksAddress, StacksBlockId};
@@ -317,8 +318,17 @@ impl BlockMinerThread {
                         }
                     }
                 }
+
                 match self.mine_block(&stackerdbs) {
-                    Ok(x) => break Some(x),
+                    Ok(x) => {
+                        if !self.validate_timestamp(&x)? {
+                            info!("Block mined too quickly. Will try again.";
+                                  "block_timestamp" => x.header.timestamp,
+                            );
+                            continue;
+                        }
+                        break Some(x);
+                    }
                     Err(NakamotoNodeError::MiningFailure(ChainstateError::MinerAborted)) => {
                         info!("Miner interrupted while mining, will try again");
                         // sleep, and try again. if the miner was interrupted because the burnchain
@@ -1037,6 +1047,42 @@ impl BlockMinerThread {
         Some(vrf_proof)
     }
 
+    /// Check that the provided block is not mined too quickly after the parent block.
+    /// This is to ensure that the signers do not reject the block due to the block being mined within the same second as the parent block.
+    fn validate_timestamp(&self, x: &NakamotoBlock) -> Result<bool, NakamotoNodeError> {
+        let chain_state = neon_node::open_chainstate_with_faults(&self.config)
+            .expect("FATAL: could not open chainstate DB");
+        let stacks_parent_header =
+            NakamotoChainState::get_block_header(chain_state.db(), &x.header.parent_block_id)
+                .map_err(|e| {
+                    error!(
+                        "Could not query header info for parent block ID {}: {:?}",
+                        &x.header.parent_block_id, &e
+                    );
+                    NakamotoNodeError::ParentNotFound
+                })?
+                .ok_or_else(|| {
+                    error!(
+                        "No header info for parent block ID {}",
+                        &x.header.parent_block_id
+                    );
+                    NakamotoNodeError::ParentNotFound
+                })?;
+        let current_timestamp = get_epoch_time_secs();
+        let time_since_parent_ms =
+            current_timestamp.saturating_sub(stacks_parent_header.burn_header_timestamp) * 1000;
+        if time_since_parent_ms < self.config.miner.min_time_between_blocks_ms {
+            debug!("Parent block mined {time_since_parent_ms} ms ago. Required minimum gap between blocks is {} ms", self.config.miner.min_time_between_blocks_ms;
+                "current_timestamp" => current_timestamp,
+                "parent_block_id" => %stacks_parent_header.index_block_hash(),
+                "parent_block_height" => stacks_parent_header.stacks_block_height,
+                "parent_block_timestamp" => stacks_parent_header.burn_header_timestamp,
+            );
+            return Ok(false);
+        }
+        Ok(true)
+    }
+
     // TODO: add tests from mutation testing results #4869
     #[cfg_attr(test, mutants::skip)]
     /// Try to mine a Stacks block by assembling one from mempool transactions and sending a

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

@@ -44,6 +44,22 @@ impl BitcoinCoreController {
         }
     }
 
+    fn add_rpc_cli_args(&self, command: &mut Command) {
+        command.arg(format!("-rpcport={}", self.config.burnchain.rpc_port));
+
+        match (
+            &self.config.burnchain.username,
+            &self.config.burnchain.password,
+        ) {
+            (Some(username), Some(password)) => {
+                command
+                    .arg(format!("-rpcuser={username}"))
+                    .arg(format!("-rpcpassword={password}"));
+            }
+            _ => {}
+        }
+    }
+
     pub fn start_bitcoind(&mut self) -> BitcoinResult<()> {
         std::fs::create_dir_all(&self.config.get_burnchain_path_str()).unwrap();
 
@@ -58,30 +74,16 @@ impl BitcoinCoreController {
             .arg("-server=1")
             .arg("-listenonion=0")
             .arg("-rpcbind=127.0.0.1")
-            .arg(&format!("-port={}", self.config.burnchain.peer_port))
-            .arg(&format!(
-                "-datadir={}",
-                self.config.get_burnchain_path_str()
-            ))
-            .arg(&format!("-rpcport={}", self.config.burnchain.rpc_port));
+            .arg(format!("-port={}", self.config.burnchain.peer_port))
+            .arg(format!("-datadir={}", self.config.get_burnchain_path_str()));
 
-        match (
-            &self.config.burnchain.username,
-            &self.config.burnchain.password,
-        ) {
-            (Some(username), Some(password)) => {
-                command
-                    .arg(&format!("-rpcuser={}", username))
-                    .arg(&format!("-rpcpassword={}", password));
-            }
-            _ => {}
-        }
+        self.add_rpc_cli_args(&mut command);
 
-        eprintln!("bitcoind spawn: {:?}", command);
+        eprintln!("bitcoind spawn: {command:?}");
 
         let mut process = match command.spawn() {
             Ok(child) => child,
-            Err(e) => return Err(BitcoinCoreError::SpawnFailed(format!("{:?}", e))),
+            Err(e) => return Err(BitcoinCoreError::SpawnFailed(format!("{e:?}"))),
         };
 
         let mut out_reader = BufReader::new(process.stdout.take().unwrap());
@@ -108,17 +110,15 @@ impl BitcoinCoreController {
     pub fn stop_bitcoind(&mut self) -> Result<(), BitcoinCoreError> {
         if let Some(_) = self.bitcoind_process.take() {
             let mut command = Command::new("bitcoin-cli");
-            command
-                .stdout(Stdio::piped())
-                .arg("-rpcconnect=127.0.0.1")
-                .arg("-rpcport=8332")
-                .arg("-rpcuser=neon-tester")
-                .arg("-rpcpassword=neon-tester-pass")
-                .arg("stop");
+            command.stdout(Stdio::piped()).arg("-rpcconnect=127.0.0.1");
+
+            self.add_rpc_cli_args(&mut command);
+
+            command.arg("stop");
 
             let mut process = match command.spawn() {
                 Ok(child) => child,
-                Err(e) => return Err(BitcoinCoreError::SpawnFailed(format!("{:?}", e))),
+                Err(e) => return Err(BitcoinCoreError::SpawnFailed(format!("{e:?}"))),
             };
 
             let mut out_reader = BufReader::new(process.stdout.take().unwrap());
@@ -127,7 +127,7 @@ impl BitcoinCoreController {
                 if bytes_read == 0 {
                     break;
                 }
-                eprintln!("{}", &line);
+                eprintln!("{line}");
             }
         }
         Ok(())

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

@@ -2788,7 +2788,8 @@ fn signer_set_rollover() {
         .running_nodes
         .btc_regtest_controller
         .get_burnchain()
-        .reward_cycle_to_block_height(next_reward_cycle);
+        .reward_cycle_to_block_height(next_reward_cycle)
+        .saturating_add(1);
 
     info!("---- Mining to next reward set calculation -----");
     signer_test.run_until_burnchain_height_nakamoto(
@@ -2847,3 +2848,105 @@ fn signer_set_rollover() {
         assert!(signer.stop().is_none());
     }
 }
+
+#[test]
+#[ignore]
+/// This test checks that the signers will broadcast a block once they receive enough signatures.
+fn min_gap_between_blocks() {
+    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 send_amt = 100;
+    let send_fee = 180;
+    let recipient = PrincipalData::from(StacksAddress::burn_address(false));
+    let time_between_blocks_ms = 10_000;
+    let mut signer_test: SignerTest<SpawnedSigner> = SignerTest::new_with_config_modifications(
+        num_signers,
+        vec![(sender_addr.clone(), send_amt + send_fee)],
+        Some(Duration::from_secs(15)),
+        |_config| {},
+        |config| {
+            config.miner.min_time_between_blocks_ms = time_between_blocks_ms;
+        },
+        &[],
+    );
+
+    let http_origin = format!("http://{}", &signer_test.running_nodes.conf.node.rpc_bind);
+
+    signer_test.boot_to_epoch_3();
+
+    let proposals_before = signer_test
+        .running_nodes
+        .nakamoto_blocks_proposed
+        .load(Ordering::SeqCst);
+
+    let info_before = get_chain_info(&signer_test.running_nodes.conf);
+
+    // submit a tx so that the miner will mine a block
+    let sender_nonce = 0;
+    let transfer_tx =
+        make_stacks_transfer(&sender_sk, sender_nonce, send_fee, &recipient, send_amt);
+    submit_tx(&http_origin, &transfer_tx);
+
+    info!("Submitted transfer tx and waiting for block proposal. Ensure it does not arrive before the gap is exceeded");
+    let start_time = Instant::now();
+    loop {
+        let blocks_proposed = signer_test
+            .running_nodes
+            .nakamoto_blocks_proposed
+            .load(Ordering::SeqCst);
+        if blocks_proposed > proposals_before {
+            assert!(
+                start_time.elapsed().as_millis() >= time_between_blocks_ms.into(),
+                "Block proposed before gap was exceeded"
+            );
+            break;
+        }
+        std::thread::sleep(Duration::from_millis(100));
+    }
+
+    debug!("Ensure that the block is mined after the gap is exceeded");
+
+    let start = Instant::now();
+    let duration = 30;
+    let blocks_before = signer_test
+        .running_nodes
+        .nakamoto_blocks_mined
+        .load(Ordering::SeqCst);
+    loop {
+        let blocks_mined = signer_test
+            .running_nodes
+            .nakamoto_blocks_mined
+            .load(Ordering::SeqCst);
+
+        let info = get_chain_info(&signer_test.running_nodes.conf);
+        if blocks_mined > blocks_before
+            && info.stacks_tip_height == info_before.stacks_tip_height + 1
+        {
+            break;
+        }
+
+        debug!(
+            "blocks_mined: {},{}, stacks_tip_height: {},{}",
+            blocks_mined, blocks_before, info_before.stacks_tip_height, info.stacks_tip_height
+        );
+
+        std::thread::sleep(Duration::from_millis(100));
+        assert!(
+            start.elapsed() < Duration::from_secs(duration),
+            "Block not mined within timeout"
+        );
+    }
+
+    signer_test.shutdown();
+}