From 3df2ec7b36a7f605aab613b7152f20a58d955143 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Sat, 30 Dec 2023 23:58:19 -0500 Subject: [PATCH] feat: backport #4061 to master and make all miner config settings loadable at runtime --- .cargo/config | 1 + .../bitcoin-int-tests/Dockerfile.rustfmt | 3 +- CONTRIBUTING.md | 27 +++++- .../burnchains/bitcoin_regtest_controller.rs | 86 ++++++++++++++++--- testnet/stacks-node/src/config.rs | 58 ++++++++++--- testnet/stacks-node/src/neon_node.rs | 26 ++++-- testnet/stacks-node/src/run_loop/neon.rs | 18 +++- 7 files changed, 179 insertions(+), 40 deletions(-) diff --git a/.cargo/config b/.cargo/config index 3c72bd0ea1..f208bb1d2d 100644 --- a/.cargo/config +++ b/.cargo/config @@ -1,5 +1,6 @@ [alias] stacks-node = "run --package stacks-node --" +fmt-stacks = "fmt -- --config group_imports=StdExternalCrate,imports_granularity=Module" # Needed by perf to generate flamegraphs. #[target.x86_64-unknown-linux-gnu] diff --git a/.github/actions/bitcoin-int-tests/Dockerfile.rustfmt b/.github/actions/bitcoin-int-tests/Dockerfile.rustfmt index 793d8e4668..5d6455e2e6 100644 --- a/.github/actions/bitcoin-int-tests/Dockerfile.rustfmt +++ b/.github/actions/bitcoin-int-tests/Dockerfile.rustfmt @@ -4,9 +4,10 @@ WORKDIR /src COPY ./rust-toolchain . COPY ./Cargo.toml . +COPY ./.cargo . RUN rustup component add rustfmt COPY . . -RUN cargo fmt --all -- --check +RUN cargo fmt-stacks --check diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c121b9dbf..0101858b62 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -91,6 +91,24 @@ should reference the issue in the commit message. For example: fix: incorporate unlocks in mempool admitter, #3623 ``` +## Recommended developer setup +### Recommended githooks + +It is helpful to set up the pre-commit git hook set up, so that Rust formatting issues are caught before +you push your code. Follow these instruction to set it up: + +1. Rename `.git/hooks/pre-commit.sample` to `.git/hooks/pre-commit` +2. Change the content of `.git/hooks/pre-commit` to be the following +```sh +#!/bin/sh +git diff --name-only --staged | grep '\.rs$' | xargs -P 8 -I {} rustfmt {} --edition 2021 --check --config group_imports=StdExternalCrate,imports_granularity=Module || ( + echo 'rustfmt failed: run "cargo fmt-stacks"'; + exit 1 +) +``` +3. Make it executable by running `chmod +x .git/hooks/pre-commit` + That's it! Now your pre-commit hook should be configured on your local machine. + # Creating and Reviewing PRs This section describes some best practices on how to create and review PRs in this context. The target audience is people who have commit access to this repository (reviewers), and people who open PRs (submitters). This is a living document -- developers can and should document their own additional guidelines here. @@ -366,19 +384,20 @@ A test should be marked `#[ignore]` if: ## Formatting -This repository uses the default rustfmt formatting style. PRs will be checked against `rustfmt` and will _fail_ if not -properly formatted. +PRs will be checked against `rustfmt` and will _fail_ if not properly formatted. +Unfortunately, some config options that we require cannot currently be set in `.rustfmt` files, so arguments must be passed via the command line. +Therefore, we handle `rustfmt` configuration using a Cargo alias: `cargo fmt-stacks` You can check the formatting locally via: ```bash -cargo fmt --all -- --check --config group_imports=StdExternalCrate +cargo fmt-stacks --check ``` You can automatically reformat your commit via: ```bash -cargo fmt --all -- --config group_imports=StdExternalCrate +cargo fmt-stacks ``` ## Comments diff --git a/testnet/stacks-node/src/burnchains/bitcoin_regtest_controller.rs b/testnet/stacks-node/src/burnchains/bitcoin_regtest_controller.rs index 8b23d48c1f..efb82f8c2b 100644 --- a/testnet/stacks-node/src/burnchains/bitcoin_regtest_controller.rs +++ b/testnet/stacks-node/src/burnchains/bitcoin_regtest_controller.rs @@ -162,6 +162,64 @@ pub fn make_bitcoin_indexer(config: &Config) -> BitcoinIndexer { burnchain_indexer } +pub fn get_satoshis_per_byte(config: &Config) -> u64 { + match config.get_burnchain_config() { + Ok(s) => s.satoshis_per_byte, + Err(_) => { + info!("No config found. Using previous configuration."); + config.burnchain.satoshis_per_byte + } + } +} + +pub fn get_rbf_fee_increment(config: &Config) -> u64 { + match config.get_burnchain_config() { + Ok(s) => s.rbf_fee_increment, + Err(_) => { + info!("No config found. Using previous configuration."); + config.burnchain.rbf_fee_increment + } + } +} + +pub fn get_max_rbf(config: &Config) -> u64 { + match config.get_burnchain_config() { + Ok(s) => s.max_rbf, + Err(_) => { + info!("No config found. Using previous configuration."); + config.burnchain.max_rbf + } + } +} + +#[cfg(test)] +mod tests { + use crate::config::DEFAULT_SATS_PER_VB; + + use super::*; + use std::env::temp_dir; + use std::fs::File; + use std::io::Write; + + #[test] + fn test_get_satoshis_per_byte() { + let dir = temp_dir(); + let file_path = dir.as_path().join("config.toml"); + + let mut config = Config::default(); + + let satoshis_per_byte = get_satoshis_per_byte(&config); + assert_eq!(satoshis_per_byte, DEFAULT_SATS_PER_VB); + + let mut file = File::create(&file_path).unwrap(); + writeln!(file, "[burnchain]").unwrap(); + writeln!(file, "satoshis_per_byte = 51").unwrap(); + config.config_path = Some(file_path.to_str().unwrap().to_string()); + + assert_eq!(get_satoshis_per_byte(&config), 51); + } +} + impl LeaderBlockCommitFees { pub fn fees_from_previous_tx( &self, @@ -171,7 +229,7 @@ impl LeaderBlockCommitFees { let mut fees = LeaderBlockCommitFees::estimated_fees_from_payload(payload, config); fees.spent_in_attempts = cmp::max(1, self.spent_in_attempts); fees.final_size = self.final_size; - fees.fee_rate = self.fee_rate + config.burnchain.rbf_fee_increment; + fees.fee_rate = self.fee_rate + get_rbf_fee_increment(&config); fees.is_rbf_enabled = true; fees } @@ -190,7 +248,7 @@ impl LeaderBlockCommitFees { let value_per_transfer = payload.burn_fee / number_of_transfers; let sortition_fee = value_per_transfer * number_of_transfers; let spent_in_attempts = 0; - let fee_rate = config.burnchain.satoshis_per_byte; + let fee_rate = get_satoshis_per_byte(&config); let default_tx_size = config.burnchain.block_commit_tx_estimated_size; LeaderBlockCommitFees { @@ -802,8 +860,9 @@ impl BitcoinRegtestController { ) -> Option { let public_key = signer.get_public_key(); - let btc_miner_fee = self.config.burnchain.leader_key_tx_estimated_size - * self.config.burnchain.satoshis_per_byte; + // reload the config to find satoshis_per_byte changes + let satoshis_per_byte = get_satoshis_per_byte(&self.config); + let btc_miner_fee = self.config.burnchain.leader_key_tx_estimated_size * satoshis_per_byte; let budget_for_outputs = DUST_UTXO_LIMIT; let total_required = btc_miner_fee + budget_for_outputs; @@ -831,7 +890,7 @@ impl BitcoinRegtestController { tx.output = vec![consensus_output]; - let fee_rate = self.config.burnchain.satoshis_per_byte; + let fee_rate = get_satoshis_per_byte(&self.config); self.finalize_tx( epoch_id, @@ -925,7 +984,6 @@ impl BitcoinRegtestController { ) -> Option { let public_key = signer.get_public_key(); let max_tx_size = 230; - let (mut tx, mut utxos) = if let Some(utxo) = utxo_to_use { ( Transaction { @@ -943,7 +1001,7 @@ impl BitcoinRegtestController { self.prepare_tx( epoch_id, &public_key, - DUST_UTXO_LIMIT + max_tx_size * self.config.burnchain.satoshis_per_byte, + DUST_UTXO_LIMIT + max_tx_size * get_satoshis_per_byte(&self.config), None, None, 0, @@ -977,7 +1035,7 @@ impl BitcoinRegtestController { DUST_UTXO_LIMIT, 0, max_tx_size, - self.config.burnchain.satoshis_per_byte, + get_satoshis_per_byte(&self.config), &mut utxos, signer, )?; @@ -1026,7 +1084,7 @@ impl BitcoinRegtestController { self.prepare_tx( epoch_id, &public_key, - DUST_UTXO_LIMIT + max_tx_size * self.config.burnchain.satoshis_per_byte, + DUST_UTXO_LIMIT + max_tx_size * get_satoshis_per_byte(&self.config), None, None, 0, @@ -1060,7 +1118,7 @@ impl BitcoinRegtestController { DUST_UTXO_LIMIT, 0, max_tx_size, - self.config.burnchain.satoshis_per_byte, + get_satoshis_per_byte(&self.config), &mut utxos, signer, )?; @@ -1095,7 +1153,7 @@ impl BitcoinRegtestController { let public_key = signer.get_public_key(); let max_tx_size = 280; - let output_amt = DUST_UTXO_LIMIT + max_tx_size * self.config.burnchain.satoshis_per_byte; + let output_amt = DUST_UTXO_LIMIT + max_tx_size * get_satoshis_per_byte(&self.config); let (mut tx, mut utxos) = self.prepare_tx(epoch_id, &public_key, output_amt, None, None, 0)?; @@ -1124,7 +1182,7 @@ impl BitcoinRegtestController { output_amt, 0, max_tx_size, - self.config.burnchain.satoshis_per_byte, + get_satoshis_per_byte(&self.config), &mut utxos, signer, )?; @@ -1322,11 +1380,11 @@ impl BitcoinRegtestController { // Stop as soon as the fee_rate is ${self.config.burnchain.max_rbf} percent higher, stop RBF if ongoing_op.fees.fee_rate - > (self.config.burnchain.satoshis_per_byte * self.config.burnchain.max_rbf / 100) + > (get_satoshis_per_byte(&self.config) * get_max_rbf(&self.config) / 100) { warn!( "RBF'd block commits reached {}% satoshi per byte fee rate, not resubmitting", - self.config.burnchain.max_rbf + get_max_rbf(&self.config) ); self.ongoing_block_commit = Some(ongoing_op); return None; diff --git a/testnet/stacks-node/src/config.rs b/testnet/stacks-node/src/config.rs index c7b47f2aad..332e83e0d7 100644 --- a/testnet/stacks-node/src/config.rs +++ b/testnet/stacks-node/src/config.rs @@ -39,7 +39,7 @@ use stacks::util::secp256k1::Secp256k1PublicKey; use stacks::vm::costs::ExecutionCost; use stacks::vm::types::{AssetIdentifier, PrincipalData, QualifiedContractIdentifier}; -const DEFAULT_SATS_PER_VB: u64 = 50; +pub const DEFAULT_SATS_PER_VB: u64 = 50; const DEFAULT_MAX_RBF_RATE: u64 = 150; // 1.5x const DEFAULT_RBF_FEE_RATE_INCREMENT: u64 = 5; const LEADER_KEY_TX_ESTIM_SIZE: u64 = 290; @@ -48,6 +48,7 @@ const INV_REWARD_CYCLES_TESTNET: u64 = 6; #[derive(Clone, Deserialize, Default, Debug)] pub struct ConfigFile { + pub __path: Option, pub burnchain: Option, pub node: Option, pub ustx_balance: Option>, @@ -178,7 +179,9 @@ mod tests { impl ConfigFile { pub fn from_path(path: &str) -> Result { let content = fs::read_to_string(path).map_err(|e| format!("Invalid path: {}", &e))?; - Self::from_str(&content) + let mut f = Self::from_str(&content)?; + f.__path = Some(path.to_string()); + Ok(f) } pub fn from_str(content: &str) -> Result { @@ -353,6 +356,7 @@ impl ConfigFile { #[derive(Clone, Debug)] pub struct Config { + pub config_path: Option, pub burnchain: BurnchainConfig, pub node: NodeConfig, pub initial_balances: Vec, @@ -394,6 +398,28 @@ lazy_static! { } impl Config { + /// get the up-to-date burnchain from the config + pub fn get_burnchain_config(&self) -> Result { + if let Some(path) = &self.config_path { + let config_file = ConfigFile::from_path(path.as_str())?; + let config = Config::from_config_file(config_file)?; + Ok(config.burnchain) + } else { + Ok(self.burnchain.clone()) + } + } + + /// get the up-to-date miner options from the config + pub fn get_miner_config(&self) -> Result { + if let Some(path) = &self.config_path { + let config_file = ConfigFile::from_path(path.as_str())?; + let config = Config::from_config_file(config_file)?; + Ok(config.miner) + } else { + Ok(self.miner.clone()) + } + } + /// Apply any test settings to this burnchain config struct fn apply_test_settings(&self, burnchain: &mut Burnchain) { if self.burnchain.get_bitcoin_network().1 == BitcoinNetworkType::Mainnet { @@ -1148,6 +1174,7 @@ impl Config { }; Ok(Config { + config_path: config_file.__path, node, burnchain, initial_balances, @@ -1263,30 +1290,36 @@ impl Config { microblocks: bool, miner_status: Arc>, ) -> BlockBuilderSettings { + let miner_config = if let Ok(miner_config) = self.get_miner_config() { + miner_config + } else { + self.miner.clone() + }; + BlockBuilderSettings { max_miner_time_ms: if microblocks { - self.miner.microblock_attempt_time_ms + miner_config.microblock_attempt_time_ms } else if attempt <= 1 { // first attempt to mine a block -- do so right away - self.miner.first_attempt_time_ms + miner_config.first_attempt_time_ms } else { // second or later attempt to mine a block -- give it some time - self.miner.subsequent_attempt_time_ms + miner_config.subsequent_attempt_time_ms }, mempool_settings: MemPoolWalkSettings { - min_tx_fee: self.miner.min_tx_fee, + min_tx_fee: miner_config.min_tx_fee, max_walk_time_ms: if microblocks { - self.miner.microblock_attempt_time_ms + miner_config.microblock_attempt_time_ms } else if attempt <= 1 { // first attempt to mine a block -- do so right away - self.miner.first_attempt_time_ms + miner_config.first_attempt_time_ms } else { // second or later attempt to mine a block -- give it some time - self.miner.subsequent_attempt_time_ms + miner_config.subsequent_attempt_time_ms }, - consider_no_estimate_tx_prob: self.miner.probability_pick_no_estimate_tx, - nonce_cache_size: self.miner.nonce_cache_size, - candidate_retry_cache_size: self.miner.candidate_retry_cache_size, + consider_no_estimate_tx_prob: miner_config.probability_pick_no_estimate_tx, + nonce_cache_size: miner_config.nonce_cache_size, + candidate_retry_cache_size: miner_config.candidate_retry_cache_size, }, miner_status, } @@ -1308,6 +1341,7 @@ impl std::default::Default for Config { let estimation = FeeEstimationConfig::default(); Config { + config_path: None, burnchain, node, initial_balances: vec![], diff --git a/testnet/stacks-node/src/neon_node.rs b/testnet/stacks-node/src/neon_node.rs index 17eebf2c97..495a3e05e0 100644 --- a/testnet/stacks-node/src/neon_node.rs +++ b/testnet/stacks-node/src/neon_node.rs @@ -208,6 +208,7 @@ use stacks::vm::costs::ExecutionCost; use crate::burnchains::bitcoin_regtest_controller::BitcoinRegtestController; use crate::burnchains::bitcoin_regtest_controller::OngoingBlockCommit; use crate::burnchains::make_bitcoin_indexer; +use crate::config::MinerConfig; use crate::run_loop::neon::Counters; use crate::run_loop::neon::RunLoop; use crate::run_loop::RegisteredKey; @@ -1216,6 +1217,18 @@ impl MicroblockMinerThread { } } +fn reload_miner_config(config: &Config) -> MinerConfig { + let miner_config = if let Ok(miner_config) = config.get_miner_config().map_err(|e| { + warn!("Failed to load miner config: {:?}", &e); + e + }) { + miner_config + } else { + config.miner.clone() + }; + miner_config +} + impl BlockMinerThread { /// Instantiate the miner thread from its parent RelayerThread pub fn from_relayer_thread( @@ -1238,11 +1251,12 @@ impl BlockMinerThread { /// Get the coinbase recipient address, if set in the config and if allowed in this epoch fn get_coinbase_recipient(&self, epoch_id: StacksEpochId) -> Option { - if epoch_id < StacksEpochId::Epoch21 && self.config.miner.block_reward_recipient.is_some() { + let miner_config = reload_miner_config(&self.config); + if epoch_id < StacksEpochId::Epoch21 && miner_config.block_reward_recipient.is_some() { warn!("Coinbase pay-to-contract is not supported in the current epoch"); None } else { - self.config.miner.block_reward_recipient.clone() + miner_config.block_reward_recipient.clone() } } @@ -1716,7 +1730,6 @@ impl BlockMinerThread { } }; - // let burn_fee_cap = self.config.burnchain.burn_fee_cap; let burn_fee_cap = get_mining_spend_amount(self.globals.get_miner_status()); let sunset_burn = self.burnchain.expected_sunset_burn( self.burn_block.block_height + 1, @@ -1825,6 +1838,7 @@ impl BlockMinerThread { let burn_db_path = self.config.get_burn_db_file_path(); let stacks_chainstate_path = self.config.get_chainstate_path_str(); + let miner_config = reload_miner_config(&self.config); let cost_estimator = self .config @@ -2024,7 +2038,7 @@ impl BlockMinerThread { &self.burnchain, &burn_db, &chain_state, - self.config.miner.unprocessed_block_deadline_secs, + miner_config.unprocessed_block_deadline_secs, ); if stacks_tip.anchored_block_hash != anchored_block.header.parent_block || parent_block_info.parent_consensus_hash != stacks_tip.consensus_hash @@ -2986,11 +3000,13 @@ impl RelayerThread { return None; } + let miner_config = reload_miner_config(&self.config); + let has_unprocessed = BlockMinerThread::unprocessed_blocks_prevent_mining( &self.burnchain, self.sortdb_ref(), self.chainstate_ref(), - self.config.miner.unprocessed_block_deadline_secs, + miner_config.unprocessed_block_deadline_secs, ); if has_unprocessed { debug!( diff --git a/testnet/stacks-node/src/run_loop/neon.rs b/testnet/stacks-node/src/run_loop/neon.rs index 47b5df31ce..9c4493fa83 100644 --- a/testnet/stacks-node/src/run_loop/neon.rs +++ b/testnet/stacks-node/src/run_loop/neon.rs @@ -628,11 +628,16 @@ impl RunLoop { sortdb: &SortitionDB, last_stacks_pox_reorg_recover_time: &mut u128, ) { + let miner_config = if let Ok(miner_config) = config.get_miner_config() { + miner_config + } else { + config.miner.clone() + }; let delay = cmp::max( config.node.chain_liveness_poll_time_secs, cmp::max( - config.miner.first_attempt_time_ms, - config.miner.subsequent_attempt_time_ms, + miner_config.first_attempt_time_ms, + miner_config.subsequent_attempt_time_ms, ) / 1000, ); @@ -748,11 +753,16 @@ impl RunLoop { last_burn_pox_reorg_recover_time: &mut u128, last_announce_time: &mut u128, ) { + let miner_config = if let Ok(miner_config) = config.get_miner_config() { + miner_config + } else { + config.miner.clone() + }; let delay = cmp::max( config.node.chain_liveness_poll_time_secs, cmp::max( - config.miner.first_attempt_time_ms, - config.miner.subsequent_attempt_time_ms, + miner_config.first_attempt_time_ms, + miner_config.subsequent_attempt_time_ms, ) / 1000, );