From 1443e8ced1b108f67b17259f4bf2e32adcdcd02b Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Mon, 11 Nov 2024 12:36:17 -0800 Subject: [PATCH 01/30] Spend down the block budget limit by 25% every block Signed-off-by: Jacinta Ferrant --- clarity/src/vm/costs/mod.rs | 14 ++++++++++++++ stackslib/src/chainstate/stacks/miner.rs | 14 +++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/clarity/src/vm/costs/mod.rs b/clarity/src/vm/costs/mod.rs index 0751822ed0..2989305e7c 100644 --- a/clarity/src/vm/costs/mod.rs +++ b/clarity/src/vm/costs/mod.rs @@ -1170,6 +1170,7 @@ pub trait CostOverflowingMath { fn cost_overflow_mul(self, other: T) -> Result; fn cost_overflow_add(self, other: T) -> Result; fn cost_overflow_sub(self, other: T) -> Result; + fn cost_overflow_div(self, other: T) -> Result; } impl CostOverflowingMath for u64 { @@ -1185,6 +1186,10 @@ impl CostOverflowingMath for u64 { self.checked_sub(other) .ok_or_else(|| CostErrors::CostOverflow) } + fn cost_overflow_div(self, other: u64) -> Result { + self.checked_div(other) + .ok_or_else(|| CostErrors::CostOverflow) + } } impl ExecutionCost { @@ -1293,6 +1298,15 @@ impl ExecutionCost { Ok(()) } + pub fn divide(&mut self, divisor: u64) -> Result<()> { + self.runtime = self.runtime.cost_overflow_div(divisor)?; + self.read_count = self.read_count.cost_overflow_div(divisor)?; + self.read_length = self.read_length.cost_overflow_div(divisor)?; + self.write_length = self.write_length.cost_overflow_div(divisor)?; + self.write_count = self.write_count.cost_overflow_div(divisor)?; + Ok(()) + } + /// Returns whether or not this cost exceeds any dimension of the /// other cost. pub fn exceeds(&self, other: &ExecutionCost) -> bool { diff --git a/stackslib/src/chainstate/stacks/miner.rs b/stackslib/src/chainstate/stacks/miner.rs index c3b60c5da8..627f620b8c 100644 --- a/stackslib/src/chainstate/stacks/miner.rs +++ b/stackslib/src/chainstate/stacks/miner.rs @@ -2222,10 +2222,22 @@ impl StacksBlockBuilder { let mempool_settings = settings.mempool_settings.clone(); let ts_start = get_epoch_time_ms(); let stacks_epoch_id = epoch_tx.get_epoch(); - let block_limit = epoch_tx + let remaining_limit = epoch_tx .block_limit() .expect("Failed to obtain block limit from miner's block connection"); + // Attempt to spread the cost limit across the tenure by always using only 25% of the remaining budget + // until the remaining budget is less than 100 + let mut target_limit = remaining_limit.clone(); + let block_limit = if target_limit.divide(100).is_ok() { + target_limit.multiply(25).expect( + "BUG: Successfully divided by 100, but failed to multiply block limit by 25", + ); + target_limit + } else { + remaining_limit + }; + let mut tx_events = Vec::new(); for initial_tx in initial_txs.iter() { From 74da2c82d4ad63f2c79870ced51d747acd04daa8 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Mon, 11 Nov 2024 15:49:12 -0800 Subject: [PATCH 02/30] Add tenure_cost_limit_per_block_percentage config option and only apply it to Nakamoto tenures Signed-off-by: Jacinta Ferrant --- CHANGELOG.md | 1 + stackslib/src/chainstate/nakamoto/miner.rs | 131 +++++++++++------- .../src/chainstate/nakamoto/tests/node.rs | 1 + stackslib/src/chainstate/stacks/miner.rs | 14 +- stackslib/src/core/mempool.rs | 11 +- stackslib/src/net/api/postblock_proposal.rs | 1 + .../src/net/api/tests/postblock_proposal.rs | 1 + testnet/stacks-node/src/config.rs | 22 +++ .../src/tests/nakamoto_integrations.rs | 1 + 9 files changed, 117 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0470bab77b..e66d126390 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE - Remove the panic for reporting DB deadlocks (just error and continue waiting) - Add index to `metadata_table` in Clarity DB on `blockhash` - Add `block_commit_delay_ms` to the config file to control the time to wait after seeing a new burn block, before submitting a block commit, to allow time for the first Nakamoto block of the new tenure to be mined, allowing this miner to avoid the need to RBF the block commit. +- Add `tenure_cost_limit_per_block_percentage` to the miner config file to control the percentage remaining tenure cost limit to consume per nakamoto block. ## [3.0.0.0.1] diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 04401a0d9b..bf94313141 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -124,6 +124,8 @@ pub struct NakamotoBlockBuilder { txs: Vec, /// header we're filling in pub header: NakamotoBlockHeader, + /// The cost limit percentage to use when processing transactions + cost_limit_percentage: Option, } pub struct MinerTenureInfo<'a> { @@ -159,6 +161,7 @@ impl NakamotoBlockBuilder { bytes_so_far: 0, txs: vec![], header: NakamotoBlockHeader::genesis(), + cost_limit_percentage: None, } } @@ -176,6 +179,11 @@ impl NakamotoBlockBuilder { /// /// * `coinbase` - the coinbase tx if this is going to start a new tenure /// + /// * `bitvec_len` - the length of the bitvec of reward addresses that should be punished or not in this block. + /// + /// * `cost_limit_percentage` - the percentage of the remaining tenure cost limit to use when + /// processing transactions. Will use 100% if None. + /// pub fn new( parent_stacks_header: &StacksHeaderInfo, tenure_id_consensus_hash: &ConsensusHash, @@ -183,6 +191,7 @@ impl NakamotoBlockBuilder { tenure_change: Option<&StacksTransaction>, coinbase: Option<&StacksTransaction>, bitvec_len: u16, + cost_limit_percentage: Option, ) -> Result { let next_height = parent_stacks_header .anchored_header @@ -222,6 +231,7 @@ impl NakamotoBlockBuilder { .map(|b| b.timestamp) .unwrap_or(0), ), + cost_limit_percentage, }) } @@ -509,6 +519,9 @@ impl NakamotoBlockBuilder { tenure_info.tenure_change_tx(), tenure_info.coinbase_tx(), signer_bitvec_len, + settings + .mempool_settings + .tenure_cost_limit_per_block_percentage, )?; let ts_start = get_epoch_time_ms(); @@ -653,59 +666,31 @@ impl BlockBuilder for NakamotoBlockBuilder { ); return TransactionResult::problematic(&tx, Error::NetError(e)); } - let (fee, receipt) = match StacksChainState::process_transaction( - clarity_tx, tx, quiet, ast_rules, - ) { - Ok((fee, receipt)) => (fee, receipt), - Err(e) => { - let (is_problematic, e) = - TransactionResult::is_problematic(&tx, e, clarity_tx.get_epoch()); - if is_problematic { - return TransactionResult::problematic(&tx, e); - } else { - match e { - Error::CostOverflowError(cost_before, cost_after, total_budget) => { - clarity_tx.reset_cost(cost_before.clone()); - if total_budget.proportion_largest_dimension(&cost_before) - < TX_BLOCK_LIMIT_PROPORTION_HEURISTIC - { - warn!( - "Transaction {} consumed over {}% of block budget, marking as invalid; budget was {}", - tx.txid(), - 100 - TX_BLOCK_LIMIT_PROPORTION_HEURISTIC, - &total_budget - ); - let mut measured_cost = cost_after; - let measured_cost = if measured_cost.sub(&cost_before).is_ok() { - Some(measured_cost) - } else { - warn!( - "Failed to compute measured cost of a too big transaction" - ); - None - }; - return TransactionResult::error( - &tx, - Error::TransactionTooBigError(measured_cost), - ); - } else { - warn!( - "Transaction {} reached block cost {}; budget was {}", - tx.txid(), - &cost_after, - &total_budget - ); - return TransactionResult::skipped_due_to_error( - &tx, - Error::BlockTooBigError, - ); - } - } - _ => return TransactionResult::error(&tx, e), - } + if let Some(percentage) = self.cost_limit_percentage { + if percentage < 100 { + let remaining_limit = clarity_tx + .block_limit() + .expect("Failed to obtain block limit from miner's block connection"); + let mut block_limit = remaining_limit.clone(); + // Attempt to spread the cost limit across the tenure by always using only 25% of the remaining budget + // until the remaining budget is less than 100 in which case we just use 100% of the remaining budget + if block_limit.divide(100).is_ok() { + block_limit.multiply(25).expect( + "BUG: Successfully divided by 100, but failed to multiply block limit by 25", + ); + clarity_tx.reset_cost(block_limit); } } - }; + } + + let (fee, receipt) = + match StacksChainState::process_transaction(clarity_tx, tx, quiet, ast_rules) { + Ok(x) => x, + Err(e) => { + return parse_process_transaction_error(clarity_tx, tx, e); + } + }; + info!("Include tx"; "tx" => %tx.txid(), "payload" => tx.payload.name(), @@ -720,3 +705,47 @@ impl BlockBuilder for NakamotoBlockBuilder { result } } + +fn parse_process_transaction_error( + clarity_tx: &mut ClarityTx, + tx: &StacksTransaction, + e: Error, +) -> TransactionResult { + let (is_problematic, e) = TransactionResult::is_problematic(&tx, e, clarity_tx.get_epoch()); + if is_problematic { + TransactionResult::problematic(&tx, e) + } else { + match e { + Error::CostOverflowError(cost_before, cost_after, total_budget) => { + clarity_tx.reset_cost(cost_before.clone()); + if total_budget.proportion_largest_dimension(&cost_before) + < TX_BLOCK_LIMIT_PROPORTION_HEURISTIC + { + warn!( + "Transaction {} consumed over {}% of block budget, marking as invalid; budget was {}", + tx.txid(), + 100 - TX_BLOCK_LIMIT_PROPORTION_HEURISTIC, + &total_budget + ); + let mut measured_cost = cost_after; + let measured_cost = if measured_cost.sub(&cost_before).is_ok() { + Some(measured_cost) + } else { + warn!("Failed to compute measured cost of a too big transaction"); + None + }; + TransactionResult::error(&tx, Error::TransactionTooBigError(measured_cost)) + } else { + warn!( + "Transaction {} reached block cost {}; budget was {}", + tx.txid(), + &cost_after, + &total_budget + ); + TransactionResult::skipped_due_to_error(&tx, Error::BlockTooBigError) + } + } + _ => TransactionResult::error(&tx, e), + } + } +} diff --git a/stackslib/src/chainstate/nakamoto/tests/node.rs b/stackslib/src/chainstate/nakamoto/tests/node.rs index 6f929e0031..0645ecd15b 100644 --- a/stackslib/src/chainstate/nakamoto/tests/node.rs +++ b/stackslib/src/chainstate/nakamoto/tests/node.rs @@ -729,6 +729,7 @@ impl TestStacksNode { None }, 1, + None, ) .unwrap() } else { diff --git a/stackslib/src/chainstate/stacks/miner.rs b/stackslib/src/chainstate/stacks/miner.rs index 627f620b8c..c3b60c5da8 100644 --- a/stackslib/src/chainstate/stacks/miner.rs +++ b/stackslib/src/chainstate/stacks/miner.rs @@ -2222,22 +2222,10 @@ impl StacksBlockBuilder { let mempool_settings = settings.mempool_settings.clone(); let ts_start = get_epoch_time_ms(); let stacks_epoch_id = epoch_tx.get_epoch(); - let remaining_limit = epoch_tx + let block_limit = epoch_tx .block_limit() .expect("Failed to obtain block limit from miner's block connection"); - // Attempt to spread the cost limit across the tenure by always using only 25% of the remaining budget - // until the remaining budget is less than 100 - let mut target_limit = remaining_limit.clone(); - let block_limit = if target_limit.divide(100).is_ok() { - target_limit.multiply(25).expect( - "BUG: Successfully divided by 100, but failed to multiply block limit by 25", - ); - target_limit - } else { - remaining_limit - }; - let mut tx_events = Vec::new(); for initial_tx in initial_txs.iter() { diff --git a/stackslib/src/core/mempool.rs b/stackslib/src/core/mempool.rs index bf2b5aff57..46ff54924b 100644 --- a/stackslib/src/core/mempool.rs +++ b/stackslib/src/core/mempool.rs @@ -537,10 +537,13 @@ pub struct MemPoolWalkSettings { pub txs_to_consider: HashSet, /// Origins for transactions that we'll consider pub filter_origins: HashSet, + /// What percentage of the remaining cost limit should we consume before stopping the walk + /// None means we consume the entire cost limit ASAP + pub tenure_cost_limit_per_block_percentage: Option, } -impl MemPoolWalkSettings { - pub fn default() -> MemPoolWalkSettings { +impl Default for MemPoolWalkSettings { + fn default() -> Self { MemPoolWalkSettings { max_walk_time_ms: u64::MAX, consider_no_estimate_tx_prob: 5, @@ -554,8 +557,11 @@ impl MemPoolWalkSettings { .into_iter() .collect(), filter_origins: HashSet::new(), + tenure_cost_limit_per_block_percentage: None, } } +} +impl MemPoolWalkSettings { pub fn zero() -> MemPoolWalkSettings { MemPoolWalkSettings { max_walk_time_ms: u64::MAX, @@ -570,6 +576,7 @@ impl MemPoolWalkSettings { .into_iter() .collect(), filter_origins: HashSet::new(), + tenure_cost_limit_per_block_percentage: None, } } } diff --git a/stackslib/src/net/api/postblock_proposal.rs b/stackslib/src/net/api/postblock_proposal.rs index 517105515c..15516c9050 100644 --- a/stackslib/src/net/api/postblock_proposal.rs +++ b/stackslib/src/net/api/postblock_proposal.rs @@ -464,6 +464,7 @@ impl NakamotoBlockProposal { tenure_change, coinbase, self.block.header.pox_treatment.len(), + None, )?; let mut miner_tenure_info = diff --git a/stackslib/src/net/api/tests/postblock_proposal.rs b/stackslib/src/net/api/tests/postblock_proposal.rs index 4f553efd21..60edb63540 100644 --- a/stackslib/src/net/api/tests/postblock_proposal.rs +++ b/stackslib/src/net/api/tests/postblock_proposal.rs @@ -281,6 +281,7 @@ fn test_try_make_response() { None, None, 8, + None, ) .unwrap(); diff --git a/testnet/stacks-node/src/config.rs b/testnet/stacks-node/src/config.rs index 47e3baafc5..2e7383d6f8 100644 --- a/testnet/stacks-node/src/config.rs +++ b/testnet/stacks-node/src/config.rs @@ -90,6 +90,7 @@ const DEFAULT_MIN_TIME_BETWEEN_BLOCKS_MS: u64 = 1_000; const DEFAULT_FIRST_REJECTION_PAUSE_MS: u64 = 5_000; const DEFAULT_SUBSEQUENT_REJECTION_PAUSE_MS: u64 = 10_000; const DEFAULT_BLOCK_COMMIT_DELAY_MS: u64 = 20_000; +const DEFAULT_TENURE_COST_LIMIT_PER_BLOCK_PERCENTAGE: u8 = 25; #[derive(Clone, Deserialize, Default, Debug)] #[serde(deny_unknown_fields)] @@ -1076,6 +1077,8 @@ impl Config { candidate_retry_cache_size: miner_config.candidate_retry_cache_size, txs_to_consider: miner_config.txs_to_consider, filter_origins: miner_config.filter_origins, + tenure_cost_limit_per_block_percentage: miner_config + .tenure_cost_limit_per_block_percentage, }, miner_status, confirm_microblocks: false, @@ -1116,6 +1119,8 @@ impl Config { candidate_retry_cache_size: miner_config.candidate_retry_cache_size, txs_to_consider: miner_config.txs_to_consider, filter_origins: miner_config.filter_origins, + tenure_cost_limit_per_block_percentage: miner_config + .tenure_cost_limit_per_block_percentage, }, miner_status, confirm_microblocks: true, @@ -2148,6 +2153,8 @@ pub struct MinerConfig { pub subsequent_rejection_pause_ms: u64, /// Duration to wait for a Nakamoto block after seeing a burnchain block before submitting a block commit. pub block_commit_delay: Duration, + /// The percentage of the remaining tenure cost limit to consume each block. Defaults to 25%. + pub tenure_cost_limit_per_block_percentage: Option, } impl Default for MinerConfig { @@ -2181,6 +2188,9 @@ impl Default for MinerConfig { first_rejection_pause_ms: DEFAULT_FIRST_REJECTION_PAUSE_MS, subsequent_rejection_pause_ms: DEFAULT_SUBSEQUENT_REJECTION_PAUSE_MS, block_commit_delay: Duration::from_millis(DEFAULT_BLOCK_COMMIT_DELAY_MS), + tenure_cost_limit_per_block_percentage: Some( + DEFAULT_TENURE_COST_LIMIT_PER_BLOCK_PERCENTAGE, + ), } } } @@ -2551,6 +2561,7 @@ pub struct MinerConfigFile { pub first_rejection_pause_ms: Option, pub subsequent_rejection_pause_ms: Option, pub block_commit_delay_ms: Option, + pub tenure_cost_limit_per_block_percentage: Option, } impl MinerConfigFile { @@ -2561,6 +2572,16 @@ impl MinerConfigFile { .map(|x| Secp256k1PrivateKey::from_hex(x)) .transpose()?; let pre_nakamoto_mock_signing = mining_key.is_some(); + let valid_tenure_cost_limit = self + .tenure_cost_limit_per_block_percentage + .map(|p| p < 100 && p > 0) + .unwrap_or(true); + if !valid_tenure_cost_limit { + return Err( + "miner.tenure_cost_limit_per_block_percentage must be between 1 and 100" + .to_string(), + ); + }; Ok(MinerConfig { first_attempt_time_ms: self .first_attempt_time_ms @@ -2667,6 +2688,7 @@ impl MinerConfigFile { first_rejection_pause_ms: self.first_rejection_pause_ms.unwrap_or(miner_default_config.first_rejection_pause_ms), subsequent_rejection_pause_ms: self.subsequent_rejection_pause_ms.unwrap_or(miner_default_config.subsequent_rejection_pause_ms), block_commit_delay: self.block_commit_delay_ms.map(Duration::from_millis).unwrap_or(miner_default_config.block_commit_delay), + tenure_cost_limit_per_block_percentage: self.tenure_cost_limit_per_block_percentage, }) } } diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index 6ae34fce42..d9a9d6b961 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -2820,6 +2820,7 @@ fn block_proposal_api_endpoint() { tenure_change, coinbase, 1, + None, ) .expect("Failed to build Nakamoto block"); From 33091d7f4c4f8890fb57a9b857a7212d4c0da545 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 12 Nov 2024 10:27:23 -0800 Subject: [PATCH 03/30] Convert cost_limit_percentage to a soft limit Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 75 ++++++++++++---------- stackslib/src/chainstate/stacks/miner.rs | 56 +++++++++++----- 2 files changed, 79 insertions(+), 52 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index bf94313141..bee3f7ef06 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -620,26 +620,19 @@ impl BlockBuilder for NakamotoBlockBuilder { return TransactionResult::skipped_due_to_error(&tx, Error::BlockTooBigError); } + let non_boot_code_contract_call = match &tx.payload { + TransactionPayload::ContractCall(cc) => !cc.address.is_boot_code_addr(), + TransactionPayload::SmartContract(..) => true, + _ => false, + }; + match limit_behavior { BlockLimitFunction::CONTRACT_LIMIT_HIT => { - match &tx.payload { - TransactionPayload::ContractCall(cc) => { - // once we've hit the runtime limit once, allow boot code contract calls, but do not try to eval - // other contract calls - if !cc.address.is_boot_code_addr() { - return TransactionResult::skipped( - &tx, - "BlockLimitFunction::CONTRACT_LIMIT_HIT".to_string(), - ); - } - } - TransactionPayload::SmartContract(..) => { - return TransactionResult::skipped( - &tx, - "BlockLimitFunction::CONTRACT_LIMIT_HIT".to_string(), - ); - } - _ => {} + if non_boot_code_contract_call { + return TransactionResult::skipped( + &tx, + "BlockLimitFunction::CONTRACT_LIMIT_HIT".to_string(), + ); } } BlockLimitFunction::LIMIT_REACHED => { @@ -666,22 +659,6 @@ impl BlockBuilder for NakamotoBlockBuilder { ); return TransactionResult::problematic(&tx, Error::NetError(e)); } - if let Some(percentage) = self.cost_limit_percentage { - if percentage < 100 { - let remaining_limit = clarity_tx - .block_limit() - .expect("Failed to obtain block limit from miner's block connection"); - let mut block_limit = remaining_limit.clone(); - // Attempt to spread the cost limit across the tenure by always using only 25% of the remaining budget - // until the remaining budget is less than 100 in which case we just use 100% of the remaining budget - if block_limit.divide(100).is_ok() { - block_limit.multiply(25).expect( - "BUG: Successfully divided by 100, but failed to multiply block limit by 25", - ); - clarity_tx.reset_cost(block_limit); - } - } - } let (fee, receipt) = match StacksChainState::process_transaction(clarity_tx, tx, quiet, ast_rules) { @@ -691,6 +668,34 @@ impl BlockBuilder for NakamotoBlockBuilder { } }; + let mut soft_limit_reached = false; + // We only attempt to apply the soft limit to non-boot code contract calls. + if non_boot_code_contract_call { + if let Some(percentage) = self.cost_limit_percentage { + if percentage < 100 { + let remaining_limit = clarity_tx + .block_limit() + .expect("Failed to obtain block limit from miner's block connection"); + let mut block_limit = remaining_limit.clone(); + // Attempt to spread the cost limit across the tenure by always using only 25% of the remaining budget + // until the remaining budget is less than 100 in which case we just use 100% of the remaining budget + if block_limit.divide(100).is_ok() { + block_limit.multiply(25).expect( + "BUG: Successfully divided by 100, but failed to multiply block limit by 25", + ); + clarity_tx.reset_cost(block_limit); + soft_limit_reached = matches!( + StacksChainState::process_transaction( + clarity_tx, tx, quiet, ast_rules + ), + Err(Error::CostOverflowError(_, _, _)) + ); + clarity_tx.reset_cost(remaining_limit); + } + } + } + } + info!("Include tx"; "tx" => %tx.txid(), "payload" => tx.payload.name(), @@ -698,7 +703,7 @@ impl BlockBuilder for NakamotoBlockBuilder { // save self.txs.push(tx.clone()); - TransactionResult::success(&tx, fee, receipt) + TransactionResult::success(&tx, fee, receipt, soft_limit_reached) }; self.bytes_so_far += tx_len; diff --git a/stackslib/src/chainstate/stacks/miner.rs b/stackslib/src/chainstate/stacks/miner.rs index c3b60c5da8..8011ac7996 100644 --- a/stackslib/src/chainstate/stacks/miner.rs +++ b/stackslib/src/chainstate/stacks/miner.rs @@ -287,6 +287,8 @@ pub struct TransactionSuccess { /// The fee that was charged to the user for doing this transaction. pub fee: u64, pub receipt: StacksTransactionReceipt, + /// Whether the soft limit was reached after this transaction was processed. + pub soft_limit_reached: bool, } /// Represents a failed transaction. Something went wrong when processing this transaction. @@ -319,6 +321,7 @@ pub struct TransactionSuccessEvent { pub fee: u64, pub execution_cost: ExecutionCost, pub result: Value, + pub soft_limit_reached: bool, } /// Represents an event for a failed transaction. Something went wrong when processing this transaction. @@ -442,12 +445,14 @@ impl TransactionResult { transaction: &StacksTransaction, fee: u64, receipt: StacksTransactionReceipt, + soft_limit_reached: bool, ) -> TransactionResult { Self::log_transaction_success(transaction); Self::Success(TransactionSuccess { tx: transaction.clone(), fee, receipt, + soft_limit_reached, }) } @@ -499,14 +504,18 @@ impl TransactionResult { pub fn convert_to_event(&self) -> TransactionEvent { match &self { - TransactionResult::Success(TransactionSuccess { tx, fee, receipt }) => { - TransactionEvent::Success(TransactionSuccessEvent { - txid: tx.txid(), - fee: *fee, - execution_cost: receipt.execution_cost.clone(), - result: receipt.result.clone(), - }) - } + TransactionResult::Success(TransactionSuccess { + tx, + fee, + receipt, + soft_limit_reached, + }) => TransactionEvent::Success(TransactionSuccessEvent { + txid: tx.txid(), + fee: *fee, + execution_cost: receipt.execution_cost.clone(), + result: receipt.result.clone(), + soft_limit_reached: *soft_limit_reached, + }), TransactionResult::ProcessingError(TransactionError { tx, error }) => { TransactionEvent::ProcessingError(TransactionErrorEvent { txid: tx.txid(), @@ -540,11 +549,7 @@ impl TransactionResult { /// Otherwise crashes. pub fn unwrap(self) -> (u64, StacksTransactionReceipt) { match self { - TransactionResult::Success(TransactionSuccess { - tx: _, - fee, - receipt, - }) => (fee, receipt), + TransactionResult::Success(TransactionSuccess { fee, receipt, .. }) => (fee, receipt), _ => panic!("Tried to `unwrap` a non-success result."), } } @@ -1019,7 +1024,7 @@ impl<'a> StacksMicroblockBuilder<'a> { let quiet = !cfg!(test); match StacksChainState::process_transaction(clarity_tx, &tx, quiet, ast_rules) { - Ok((fee, receipt)) => Ok(TransactionResult::success(&tx, fee, receipt)), + Ok((fee, receipt)) => Ok(TransactionResult::success(&tx, fee, receipt, false)), Err(e) => { let (is_problematic, e) = TransactionResult::is_problematic(&tx, e, clarity_tx.get_epoch()); @@ -2366,7 +2371,12 @@ impl StacksBlockBuilder { let result_event = tx_result.convert_to_event(); match tx_result { - TransactionResult::Success(TransactionSuccess { receipt, .. }) => { + TransactionResult::Success(TransactionSuccess { + tx: _, + fee: _, + receipt, + soft_limit_reached, + }) => { if txinfo.metadata.time_estimate_ms.is_none() { // use i64 to avoid running into issues when storing in // rusqlite. @@ -2404,6 +2414,18 @@ impl StacksBlockBuilder { { mined_sponsor_nonces.insert(sponsor_addr, sponsor_nonce); } + if soft_limit_reached { + // done mining -- our soft limit execution budget is exceeded. + // Make the block from the transactions we did manage to get + debug!( + "Soft block budget exceeded on tx {}", + &txinfo.tx.txid() + ); + if block_limit_hit != BlockLimitFunction::CONTRACT_LIMIT_HIT { + debug!("Switch to mining stx-transfers only"); + block_limit_hit = BlockLimitFunction::CONTRACT_LIMIT_HIT; + } + } } TransactionResult::Skipped(TransactionSkipped { error, .. }) | TransactionResult::ProcessingError(TransactionError { @@ -2793,7 +2815,7 @@ impl BlockBuilder for StacksBlockBuilder { self.txs.push(tx.clone()); self.total_anchored_fees += fee; - TransactionResult::success(&tx, fee, receipt) + TransactionResult::success(&tx, fee, receipt, false) } else { // building up the microblocks if tx.anchor_mode != TransactionAnchorMode::OffChainOnly @@ -2885,7 +2907,7 @@ impl BlockBuilder for StacksBlockBuilder { self.micro_txs.push(tx.clone()); self.total_streamed_fees += fee; - TransactionResult::success(&tx, fee, receipt) + TransactionResult::success(&tx, fee, receipt, false) }; self.bytes_so_far += tx_len; From ef332d28ca170278ddd8a36a03c01cfbf90c7cb8 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 12 Nov 2024 10:37:59 -0800 Subject: [PATCH 04/30] Use the block limit BEFORE applying the hard check when running the soft check Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index bee3f7ef06..9903f19f99 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -660,6 +660,9 @@ impl BlockBuilder for NakamotoBlockBuilder { return TransactionResult::problematic(&tx, Error::NetError(e)); } + let block_limit_before = clarity_tx + .block_limit() + .expect("Failed to obtain block limit from miner's block connection"); let (fee, receipt) = match StacksChainState::process_transaction(clarity_tx, tx, quiet, ast_rules) { Ok(x) => x, @@ -676,7 +679,7 @@ impl BlockBuilder for NakamotoBlockBuilder { let remaining_limit = clarity_tx .block_limit() .expect("Failed to obtain block limit from miner's block connection"); - let mut block_limit = remaining_limit.clone(); + let mut block_limit = block_limit_before.clone(); // Attempt to spread the cost limit across the tenure by always using only 25% of the remaining budget // until the remaining budget is less than 100 in which case we just use 100% of the remaining budget if block_limit.divide(100).is_ok() { From ef4426ba250890ed050855f2c215cfa1ab1a7eb0 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 12 Nov 2024 11:18:11 -0800 Subject: [PATCH 05/30] Do not apply the spend down per transaction rather do per block Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 67 +++++++++++----------- 1 file changed, 34 insertions(+), 33 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 9903f19f99..eefb853217 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -124,8 +124,8 @@ pub struct NakamotoBlockBuilder { txs: Vec, /// header we're filling in pub header: NakamotoBlockHeader, - /// The cost limit percentage to use when processing transactions - cost_limit_percentage: Option, + /// The execution cost for the block + soft_limit: Option, } pub struct MinerTenureInfo<'a> { @@ -161,7 +161,7 @@ impl NakamotoBlockBuilder { bytes_so_far: 0, txs: vec![], header: NakamotoBlockHeader::genesis(), - cost_limit_percentage: None, + soft_limit: None, } } @@ -191,7 +191,7 @@ impl NakamotoBlockBuilder { tenure_change: Option<&StacksTransaction>, coinbase: Option<&StacksTransaction>, bitvec_len: u16, - cost_limit_percentage: Option, + soft_limit: Option, ) -> Result { let next_height = parent_stacks_header .anchored_header @@ -231,7 +231,7 @@ impl NakamotoBlockBuilder { .map(|b| b.timestamp) .unwrap_or(0), ), - cost_limit_percentage, + soft_limit, }) } @@ -519,9 +519,7 @@ impl NakamotoBlockBuilder { tenure_info.tenure_change_tx(), tenure_info.coinbase_tx(), signer_bitvec_len, - settings - .mempool_settings - .tenure_cost_limit_per_block_percentage, + None, )?; let ts_start = get_epoch_time_ms(); @@ -534,6 +532,24 @@ impl NakamotoBlockBuilder { .block_limit() .expect("Failed to obtain block limit from miner's block connection"); + let mut soft_limit = None; + if let Some(percentage) = settings + .mempool_settings + .tenure_cost_limit_per_block_percentage + { + if percentage < 100 { + let mut limit = block_limit.clone(); + if limit.divide(100).is_ok() { + limit.multiply(percentage.into()).expect( + "BUG: failed to multiply by {percentage} when previously divided by 100", + ); + soft_limit = Some(limit); + } + } + } + + builder.soft_limit = soft_limit; + let initial_txs: Vec<_> = [ tenure_info.tenure_change_tx.clone(), tenure_info.coinbase_tx.clone(), @@ -660,9 +676,6 @@ impl BlockBuilder for NakamotoBlockBuilder { return TransactionResult::problematic(&tx, Error::NetError(e)); } - let block_limit_before = clarity_tx - .block_limit() - .expect("Failed to obtain block limit from miner's block connection"); let (fee, receipt) = match StacksChainState::process_transaction(clarity_tx, tx, quiet, ast_rules) { Ok(x) => x, @@ -671,31 +684,19 @@ impl BlockBuilder for NakamotoBlockBuilder { } }; + let block_limit_before = clarity_tx + .block_limit() + .expect("Failed to obtain block limit from miner's block connection"); let mut soft_limit_reached = false; // We only attempt to apply the soft limit to non-boot code contract calls. if non_boot_code_contract_call { - if let Some(percentage) = self.cost_limit_percentage { - if percentage < 100 { - let remaining_limit = clarity_tx - .block_limit() - .expect("Failed to obtain block limit from miner's block connection"); - let mut block_limit = block_limit_before.clone(); - // Attempt to spread the cost limit across the tenure by always using only 25% of the remaining budget - // until the remaining budget is less than 100 in which case we just use 100% of the remaining budget - if block_limit.divide(100).is_ok() { - block_limit.multiply(25).expect( - "BUG: Successfully divided by 100, but failed to multiply block limit by 25", - ); - clarity_tx.reset_cost(block_limit); - soft_limit_reached = matches!( - StacksChainState::process_transaction( - clarity_tx, tx, quiet, ast_rules - ), - Err(Error::CostOverflowError(_, _, _)) - ); - clarity_tx.reset_cost(remaining_limit); - } - } + if let Some(soft_limit) = self.soft_limit.clone() { + clarity_tx.reset_cost(soft_limit); + soft_limit_reached = matches!( + StacksChainState::process_transaction(clarity_tx, tx, quiet, ast_rules), + Err(Error::CostOverflowError(_, _, _)) + ); + clarity_tx.reset_cost(block_limit_before); } } From 068d18f3c0dcbe870fb1e128d843ae54fb140258 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 12 Nov 2024 11:34:13 -0800 Subject: [PATCH 06/30] Fix build Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 2 +- stackslib/src/chainstate/stacks/miner.rs | 22 +++++++++++++++++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index eefb853217..3661032afb 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -707,7 +707,7 @@ impl BlockBuilder for NakamotoBlockBuilder { // save self.txs.push(tx.clone()); - TransactionResult::success(&tx, fee, receipt, soft_limit_reached) + TransactionResult::success_with_soft_limit(&tx, fee, receipt, soft_limit_reached) }; self.bytes_so_far += tx_len; diff --git a/stackslib/src/chainstate/stacks/miner.rs b/stackslib/src/chainstate/stacks/miner.rs index 8011ac7996..7a72cc1652 100644 --- a/stackslib/src/chainstate/stacks/miner.rs +++ b/stackslib/src/chainstate/stacks/miner.rs @@ -445,6 +445,22 @@ impl TransactionResult { transaction: &StacksTransaction, fee: u64, receipt: StacksTransactionReceipt, + ) -> TransactionResult { + Self::log_transaction_success(transaction); + Self::Success(TransactionSuccess { + tx: transaction.clone(), + fee, + receipt, + soft_limit_reached: false, + }) + } + + /// Creates a `TransactionResult` backed by `TransactionSuccess` with a soft limit reached. + /// This method logs "transaction success" as a side effect. + pub fn success_with_soft_limit( + transaction: &StacksTransaction, + fee: u64, + receipt: StacksTransactionReceipt, soft_limit_reached: bool, ) -> TransactionResult { Self::log_transaction_success(transaction); @@ -1024,7 +1040,7 @@ impl<'a> StacksMicroblockBuilder<'a> { let quiet = !cfg!(test); match StacksChainState::process_transaction(clarity_tx, &tx, quiet, ast_rules) { - Ok((fee, receipt)) => Ok(TransactionResult::success(&tx, fee, receipt, false)), + Ok((fee, receipt)) => Ok(TransactionResult::success(&tx, fee, receipt)), Err(e) => { let (is_problematic, e) = TransactionResult::is_problematic(&tx, e, clarity_tx.get_epoch()); @@ -2815,7 +2831,7 @@ impl BlockBuilder for StacksBlockBuilder { self.txs.push(tx.clone()); self.total_anchored_fees += fee; - TransactionResult::success(&tx, fee, receipt, false) + TransactionResult::success(&tx, fee, receipt) } else { // building up the microblocks if tx.anchor_mode != TransactionAnchorMode::OffChainOnly @@ -2907,7 +2923,7 @@ impl BlockBuilder for StacksBlockBuilder { self.micro_txs.push(tx.clone()); self.total_streamed_fees += fee; - TransactionResult::success(&tx, fee, receipt, false) + TransactionResult::success(&tx, fee, receipt) }; self.bytes_so_far += tx_len; From 7dbd45ca3f5f25f24270a5c927e8bdf8a171a28a Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 12 Nov 2024 12:39:35 -0800 Subject: [PATCH 07/30] Fix tests to not spend down the tenure limit Signed-off-by: Jacinta Ferrant --- testnet/stacks-node/src/tests/nakamoto_integrations.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index d9a9d6b961..c8ef5ca712 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -5242,6 +5242,7 @@ fn check_block_heights() { 3 * deploy_fee + (send_amt + send_fee) * tenure_count * inter_blocks_per_tenure, ); naka_conf.add_initial_balance(PrincipalData::from(sender_signer_addr).to_string(), 100000); + naka_conf.miner.tenure_cost_limit_per_block_percentage = None; let recipient = PrincipalData::from(StacksAddress::burn_address(false)); let stacker_sk = setup_stacker(&mut naka_conf); @@ -5984,6 +5985,7 @@ fn clarity_burn_state() { deploy_fee + tx_fee * tenure_count + tx_fee * tenure_count * inter_blocks_per_tenure, ); naka_conf.add_initial_balance(PrincipalData::from(sender_signer_addr).to_string(), 100000); + naka_conf.miner.tenure_cost_limit_per_block_percentage = None; let stacker_sk = setup_stacker(&mut naka_conf); test_observer::spawn(); @@ -7738,6 +7740,7 @@ fn check_block_info() { naka_conf.burnchain.chain_id = CHAIN_ID_TESTNET + 1; let http_origin = format!("http://{}", &naka_conf.node.rpc_bind); naka_conf.miner.wait_on_interim_blocks = Duration::from_secs(1); + naka_conf.miner.tenure_cost_limit_per_block_percentage = None; let sender_sk = Secp256k1PrivateKey::new(); let sender_signer_sk = Secp256k1PrivateKey::new(); let sender_signer_addr = tests::to_addr(&sender_signer_sk); From c9c950d7098251d17ff600eb3d013dfb65bf578f Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Tue, 12 Nov 2024 13:29:04 -0800 Subject: [PATCH 08/30] Fix comment Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 3661032afb..dcf60a600c 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -181,8 +181,7 @@ impl NakamotoBlockBuilder { /// /// * `bitvec_len` - the length of the bitvec of reward addresses that should be punished or not in this block. /// - /// * `cost_limit_percentage` - the percentage of the remaining tenure cost limit to use when - /// processing transactions. Will use 100% if None. + /// * `soft_limit` - an optional soft limit for the block's clarity cost for this block /// pub fn new( parent_stacks_header: &StacksHeaderInfo, From fbf5564094060d9858fe93f76c0fadfe1c0df39f Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 13 Nov 2024 12:25:57 -0800 Subject: [PATCH 09/30] WIP: incomplete test Signed-off-by: Jacinta Ferrant --- .github/workflows/bitcoin-tests.yml | 1 + stackslib/src/chainstate/nakamoto/miner.rs | 1 + stackslib/src/chainstate/stacks/miner.rs | 1 + .../src/tests/nakamoto_integrations.rs | 204 ++++++++++++++++++ 4 files changed, 207 insertions(+) diff --git a/.github/workflows/bitcoin-tests.yml b/.github/workflows/bitcoin-tests.yml index 2c2909bd45..0753f66837 100644 --- a/.github/workflows/bitcoin-tests.yml +++ b/.github/workflows/bitcoin-tests.yml @@ -139,6 +139,7 @@ jobs: - tests::nakamoto_integrations::utxo_check_on_startup_recover - tests::nakamoto_integrations::v3_signer_api_endpoint - tests::nakamoto_integrations::signer_chainstate + - tests::nakamoto_integrations::clarity_cost_spend_down # TODO: enable these once v1 signer is supported by a new nakamoto epoch # - tests::signer::v1::dkg # - tests::signer::v1::sign_request_rejected diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index dcf60a600c..9fd15b9bba 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -542,6 +542,7 @@ impl NakamotoBlockBuilder { limit.multiply(percentage.into()).expect( "BUG: failed to multiply by {percentage} when previously divided by 100", ); + debug!("Setting soft limit for clarity cost to {percentage}% of block limit ({block_limit}): {limit}"); soft_limit = Some(limit); } } diff --git a/stackslib/src/chainstate/stacks/miner.rs b/stackslib/src/chainstate/stacks/miner.rs index 7a72cc1652..3f95da057d 100644 --- a/stackslib/src/chainstate/stacks/miner.rs +++ b/stackslib/src/chainstate/stacks/miner.rs @@ -463,6 +463,7 @@ impl TransactionResult { receipt: StacksTransactionReceipt, soft_limit_reached: bool, ) -> TransactionResult { + debug!("SOFT LIMIT IS REACHED: {soft_limit_reached}"); Self::log_transaction_success(transaction); Self::Success(TransactionSuccess { tx: transaction.clone(), diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index c8ef5ca712..46f8deb271 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -9499,3 +9499,207 @@ fn skip_mining_long_tx() { run_loop_thread.join().unwrap(); } + +#[test] +#[ignore] +/// This test is testing that the clarity cost spend down works as expected, +/// spreading clarity contract calls across the tenure instead of including them all in +/// the first few blocks. It also asserts that this limit resets at the start of each tenure. +fn clarity_cost_spend_down() { + if env::var("BITCOIND_TEST") != Ok("1".into()) { + return; + } + + let mut signers = TestSigners::default(); + let (mut naka_conf, _miner_account) = naka_neon_integration_conf(None); + let http_origin = format!("http://{}", &naka_conf.node.rpc_bind); + naka_conf.miner.wait_on_interim_blocks = Duration::from_secs(1); + let sender_sks: Vec<_> = (0..5).map(|_| Secp256k1PrivateKey::new()).collect(); + let sender_signer_sks: Vec<_> = (0..5).map(|_| Secp256k1PrivateKey::new()).collect(); + let sender_signer_addrs: Vec<_> = sender_signer_sks.iter().map(tests::to_addr).collect(); + let sender_addrs: Vec<_> = sender_sks.iter().map(tests::to_addr).collect(); + let tenure_count = 5; + let inter_blocks_per_tenure = 30; + let nmb_txs = 25; + // setup sender + recipient for some test stx transfers + // these are necessary for the interim blocks to get mined at all + let tx_fee = 1000; + let deploy_fee = 580000; + let amount = deploy_fee + + tx_fee * tenure_count + + tx_fee * tenure_count * inter_blocks_per_tenure * nmb_txs; + for sender_addr in sender_addrs { + naka_conf.add_initial_balance(PrincipalData::from(sender_addr).to_string(), amount); + } + for sender_signer_addr in sender_signer_addrs { + naka_conf.add_initial_balance( + PrincipalData::from(sender_signer_addr).to_string(), + amount * 2, + ); + } + naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(1); + let stacker_sks: Vec<_> = (0..5).map(|_| setup_stacker(&mut naka_conf)).collect(); + + test_observer::spawn(); + test_observer::register(&mut naka_conf, &[EventKeyType::MinedBlocks]); + + 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, + naka_mined_blocks: mined_blocks, + .. + } = run_loop.counters(); + + let coord_channel = run_loop.coordinator_channels(); + + let run_loop_thread = thread::Builder::new() + .name("run_loop".into()) + .spawn(move || run_loop.start(None, 0)) + .unwrap(); + wait_for_runloop(&blocks_processed); + + boot_to_epoch_3( + &naka_conf, + &blocks_processed, + &stacker_sks, + &sender_signer_sks, + &mut Some(&mut signers), + &mut btc_regtest_controller, + ); + + info!("Bootstrapped to Epoch-3.0 boundary, starting nakamoto miner"); + + info!("Nakamoto miner started..."); + blind_signer(&naka_conf, &signers, proposals_submitted); + + wait_for_first_naka_block_commit(60, &commits_submitted); + + let mut sender_nonce = 0; + + // Create an expensive contract that will be republished multiple times + let contract = format!( + "(define-public (f) (begin {} (ok 1))) (begin (f))", + (0..3000) + .map(|_| format!( + "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", + boot_code_id("cost-voting", false), + boot_code_id("costs", false), + boot_code_id("costs", false), + )) + .collect::>() + .join(" ") + ); + + // Mine `tenure_count` nakamoto tenures + for tenure_ix in 0..tenure_count { + info!("Mining tenure {tenure_ix}"); + // Wait for the tenure change payload to be mined + let blocks_before = mined_blocks.load(Ordering::SeqCst); + let blocks_processed_before = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); + next_block_and(&mut btc_regtest_controller, 60, || { + let blocks_count = mined_blocks.load(Ordering::SeqCst); + let blocks_processed = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); + Ok(blocks_count > blocks_before && blocks_processed > blocks_processed_before) + }) + .unwrap(); + // mine the interim blocks + for interim_block_ix in 0..inter_blocks_per_tenure { + info!("Mining interim block {interim_block_ix}"); + let mined_before = test_observer::get_mined_nakamoto_blocks(); + let blocks_processed_before = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); + + // Pause mining so we can add all our transactions to the mempool at once. + TEST_MINE_STALL.lock().unwrap().replace(true); + let mut submitted_txs = vec![]; + for nmb_tx in 0..nmb_txs { + for sender_sk in sender_sks.iter() { + // Just keep redeploying large contracts + let contract_tx = make_contract_publish( + &sender_sk, + sender_nonce, + deploy_fee, + naka_conf.burnchain.chain_id, + &format!("expensive-contract-{sender_nonce}-{nmb_tx}"), + &contract, + ); + let txid = submit_tx(&http_origin, &contract_tx); + submitted_txs.push(txid); + } + sender_nonce += 1; + } + TEST_MINE_STALL.lock().unwrap().replace(false); + wait_for(120, || { + let blocks_processed = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); + let total_nmb_mined_txs = test_observer::get_mined_nakamoto_blocks() + .iter() + .map(|b| b.tx_events.len()) + .sum::(); + Ok(blocks_processed > blocks_processed_before + && test_observer::get_mined_nakamoto_blocks().len() > mined_before.len() + && total_nmb_mined_txs > nmb_txs as usize) + }) + .expect("Timed out waiting for interim blocks to be mined"); + + let mined_after = test_observer::get_mined_nakamoto_blocks(); + let mined_blocks = mined_after[mined_before.len()..].to_vec(); + assert!( + mined_blocks.len() > 1, + "Expected at least 2 blocks to be mined in the interim period" + ); + let soft_limit_reached = mined_blocks + .into_iter() + .map(|block| block.tx_events) + .flatten() + .any(|event| match event { + TransactionEvent::Success(TransactionSuccessEvent { + txid: _, + fee: _, + execution_cost: _, + result, + soft_limit_reached, + }) => { + info!("Contract call result: {result}"); + soft_limit_reached + } + _ => { + info!("Unsuccessful event: {event:?}"); + false + } + }); + assert!( + soft_limit_reached, + "Expected at least one block to have a soft limit reached event" + ); + } + } + + coord_channel + .lock() + .expect("Mutex poisoned") + .stop_chains_coordinator(); + run_loop_stopper.store(false, Ordering::SeqCst); + + run_loop_thread.join().unwrap(); +} From c7f65f83ac8d0472c4df1b6815ac9cf80943090e Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 13 Nov 2024 13:08:46 -0800 Subject: [PATCH 10/30] WIP: incomplete test Signed-off-by: Jacinta Ferrant --- .../src/tests/nakamoto_integrations.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index 46f8deb271..0c81e8b107 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -9510,17 +9510,22 @@ fn clarity_cost_spend_down() { return; } - let mut signers = TestSigners::default(); let (mut naka_conf, _miner_account) = naka_neon_integration_conf(None); let http_origin = format!("http://{}", &naka_conf.node.rpc_bind); + let num_signers = 30; naka_conf.miner.wait_on_interim_blocks = Duration::from_secs(1); - let sender_sks: Vec<_> = (0..5).map(|_| Secp256k1PrivateKey::new()).collect(); - let sender_signer_sks: Vec<_> = (0..5).map(|_| Secp256k1PrivateKey::new()).collect(); + let sender_sks: Vec<_> = (0..num_signers) + .map(|_| Secp256k1PrivateKey::new()) + .collect(); + let sender_signer_sks: Vec<_> = (0..num_signers) + .map(|_| Secp256k1PrivateKey::new()) + .collect(); let sender_signer_addrs: Vec<_> = sender_signer_sks.iter().map(tests::to_addr).collect(); let sender_addrs: Vec<_> = sender_sks.iter().map(tests::to_addr).collect(); let tenure_count = 5; let inter_blocks_per_tenure = 30; let nmb_txs = 25; + let mut signers = TestSigners::new(sender_signer_sks.clone()); // setup sender + recipient for some test stx transfers // these are necessary for the interim blocks to get mined at all let tx_fee = 1000; @@ -9538,7 +9543,9 @@ fn clarity_cost_spend_down() { ); } naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(1); - let stacker_sks: Vec<_> = (0..5).map(|_| setup_stacker(&mut naka_conf)).collect(); + let stacker_sks: Vec<_> = (0..num_signers) + .map(|_| setup_stacker(&mut naka_conf)) + .collect(); test_observer::spawn(); test_observer::register(&mut naka_conf, &[EventKeyType::MinedBlocks]); From 33ec9b87fcaf08620b7ce2382d849347d50661c1 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 13 Nov 2024 13:30:12 -0800 Subject: [PATCH 11/30] WIP: incomplete test Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/stacks/miner.rs | 1 + testnet/stacks-node/src/tests/nakamoto_integrations.rs | 8 ++++---- testnet/stacks-node/src/tests/neon_integrations.rs | 4 +++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/stackslib/src/chainstate/stacks/miner.rs b/stackslib/src/chainstate/stacks/miner.rs index 3f95da057d..4c4dc763fc 100644 --- a/stackslib/src/chainstate/stacks/miner.rs +++ b/stackslib/src/chainstate/stacks/miner.rs @@ -2469,6 +2469,7 @@ impl StacksBlockBuilder { } } Error::TransactionTooBigError(measured_cost) => { + debug!("TRANSACTION TOO BIG: {}", &txinfo.tx.txid()); if update_estimator { if let Some(measured_cost) = measured_cost { if let Err(e) = estimator.notify_event( diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index 0c81e8b107..9a0a08aab9 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -9529,7 +9529,7 @@ fn clarity_cost_spend_down() { // setup sender + recipient for some test stx transfers // these are necessary for the interim blocks to get mined at all let tx_fee = 1000; - let deploy_fee = 580000; + let deploy_fee = 650000; let amount = deploy_fee + tx_fee * tenure_count + tx_fee * tenure_count * inter_blocks_per_tenure * nmb_txs; @@ -9542,7 +9542,7 @@ fn clarity_cost_spend_down() { amount * 2, ); } - naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(1); + naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(100); let stacker_sks: Vec<_> = (0..num_signers) .map(|_| setup_stacker(&mut naka_conf)) .collect(); @@ -9596,7 +9596,7 @@ fn clarity_cost_spend_down() { // Create an expensive contract that will be republished multiple times let contract = format!( "(define-public (f) (begin {} (ok 1))) (begin (f))", - (0..3000) + (0..500) .map(|_| format!( "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", boot_code_id("cost-voting", false), @@ -9645,7 +9645,7 @@ fn clarity_cost_spend_down() { sender_nonce, deploy_fee, naka_conf.burnchain.chain_id, - &format!("expensive-contract-{sender_nonce}-{nmb_tx}"), + &format!("expensive-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), &contract, ); let txid = submit_tx(&http_origin, &contract_tx); diff --git a/testnet/stacks-node/src/tests/neon_integrations.rs b/testnet/stacks-node/src/tests/neon_integrations.rs index 167a66f7db..a1625b45a7 100644 --- a/testnet/stacks-node/src/tests/neon_integrations.rs +++ b/testnet/stacks-node/src/tests/neon_integrations.rs @@ -371,8 +371,10 @@ pub mod test_observer { inner_obj } else if let Some(inner_obj) = txevent_obj.get("Skipped") { inner_obj + } else if let Some(inner_obj) = txevent_obj.get("Problematic") { + inner_obj } else { - panic!("TransactionEvent object should have one of Success, ProcessingError, or Skipped") + panic!("TransactionEvent object should have one of Success, ProcessingError, Skipped, or Problematic. Had keys: {:?}", txevent_obj.keys().map(|x| x.to_string()).collect::>()); }; inner_obj .as_object() From 728c5f07f891a15a45e3e714e6213c4c793acdf7 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 13 Nov 2024 13:58:30 -0800 Subject: [PATCH 12/30] Fix soft limit calculation Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 19 ++++---- .../src/tests/nakamoto_integrations.rs | 44 ++++++++++++++----- 2 files changed, 44 insertions(+), 19 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 9fd15b9bba..c286b80753 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -537,14 +537,17 @@ impl NakamotoBlockBuilder { .tenure_cost_limit_per_block_percentage { if percentage < 100 { - let mut limit = block_limit.clone(); - if limit.divide(100).is_ok() { - limit.multiply(percentage.into()).expect( - "BUG: failed to multiply by {percentage} when previously divided by 100", - ); - debug!("Setting soft limit for clarity cost to {percentage}% of block limit ({block_limit}): {limit}"); - soft_limit = Some(limit); - } + let mut remaining_limit = block_limit.clone(); + let cost_so_far = tenure_tx.cost_so_far(); + if remaining_limit.sub(&cost_so_far).is_ok() { + if remaining_limit.divide(100).is_ok() { + remaining_limit.multiply(percentage.into()).expect( + "BUG: failed to multiply by {percentage} when previously divided by 100", + ); + debug!("Setting soft limit for clarity cost to {percentage}% of remaining block limit: {remaining_limit}"); + soft_limit = Some(remaining_limit); + } + }; } } diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index 9a0a08aab9..d0e329edf5 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -9529,10 +9529,9 @@ fn clarity_cost_spend_down() { // setup sender + recipient for some test stx transfers // these are necessary for the interim blocks to get mined at all let tx_fee = 1000; - let deploy_fee = 650000; - let amount = deploy_fee - + tx_fee * tenure_count - + tx_fee * tenure_count * inter_blocks_per_tenure * nmb_txs; + let small_deploy_fee = 3000; + let large_deploy_fee = 190200; + let amount = (large_deploy_fee + small_deploy_fee) * nmb_txs + tx_fee * tenure_count; for sender_addr in sender_addrs { naka_conf.add_initial_balance(PrincipalData::from(sender_addr).to_string(), amount); } @@ -9542,7 +9541,7 @@ fn clarity_cost_spend_down() { amount * 2, ); } - naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(100); + naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(25); let stacker_sks: Vec<_> = (0..num_signers) .map(|_| setup_stacker(&mut naka_conf)) .collect(); @@ -9593,10 +9592,22 @@ fn clarity_cost_spend_down() { let mut sender_nonce = 0; + let small_contract = format!( + "(define-public (f) (begin {} (ok 1))) (begin (f))", + (0..2) + .map(|_| format!( + "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", + boot_code_id("cost-voting", false), + boot_code_id("costs", false), + boot_code_id("costs", false), + )) + .collect::>() + .join(" ") + ); // Create an expensive contract that will be republished multiple times - let contract = format!( + let large_contract = format!( "(define-public (f) (begin {} (ok 1))) (begin (f))", - (0..500) + (0..1000) .map(|_| format!( "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", boot_code_id("cost-voting", false), @@ -9637,21 +9648,32 @@ fn clarity_cost_spend_down() { // Pause mining so we can add all our transactions to the mempool at once. TEST_MINE_STALL.lock().unwrap().replace(true); let mut submitted_txs = vec![]; - for nmb_tx in 0..nmb_txs { + for nmb_tx in 0..nmb_txs/2 { for sender_sk in sender_sks.iter() { // Just keep redeploying large contracts let contract_tx = make_contract_publish( &sender_sk, sender_nonce, - deploy_fee, + large_deploy_fee, naka_conf.burnchain.chain_id, &format!("expensive-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), - &contract, + &large_contract, + ); + let txid = submit_tx(&http_origin, &contract_tx); + submitted_txs.push(txid); + // Just keep redeploying large contracts + let contract_tx = make_contract_publish( + &sender_sk, + sender_nonce + 1, + small_deploy_fee, + naka_conf.burnchain.chain_id, + &format!("cheap-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), + &small_contract, ); let txid = submit_tx(&http_origin, &contract_tx); submitted_txs.push(txid); } - sender_nonce += 1; + sender_nonce += 2; } TEST_MINE_STALL.lock().unwrap().replace(false); wait_for(120, || { From f15b42687c66fa26e505f6fb3913bfa3d9c2887e Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 13 Nov 2024 14:55:09 -0800 Subject: [PATCH 13/30] Fix setting of soft limit Signed-off-by: Jacinta Ferrant --- clarity/src/vm/costs/mod.rs | 13 +++++++++ stackslib/src/chainstate/nakamoto/miner.rs | 20 ++++++++------ stackslib/src/chainstate/stacks/db/mod.rs | 6 +++++ stackslib/src/clarity_vm/clarity.rs | 9 +++++++ .../src/tests/nakamoto_integrations.rs | 27 ++++++++++--------- 5 files changed, 54 insertions(+), 21 deletions(-) diff --git a/clarity/src/vm/costs/mod.rs b/clarity/src/vm/costs/mod.rs index 2989305e7c..ae5b588bd0 100644 --- a/clarity/src/vm/costs/mod.rs +++ b/clarity/src/vm/costs/mod.rs @@ -896,6 +896,19 @@ impl LimitedCostTracker { Self::Free => ExecutionCost::max_value(), } } + // Set the limit on the Limited tracker, or do nothing if its free. + // Returns the previous limit if it was set. + pub fn set_limit(&mut self, limit: ExecutionCost) -> Option { + match self { + Self::Limited(ref mut data) => { + let old_limit = data.limit.clone(); + data.limit = limit; + Some(old_limit) + } + Self::Free => None, + } + } + pub fn get_memory(&self) -> u64 { match self { Self::Limited(TrackerData { memory, .. }) => *memory, diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index c286b80753..4cb5c7c7c2 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -25,7 +25,7 @@ use clarity::vm::analysis::{CheckError, CheckErrors}; use clarity::vm::ast::errors::ParseErrors; use clarity::vm::ast::ASTRules; use clarity::vm::clarity::TransactionConnection; -use clarity::vm::costs::ExecutionCost; +use clarity::vm::costs::{ExecutionCost, LimitedCostTracker, TrackerData}; use clarity::vm::database::BurnStateDB; use clarity::vm::errors::Error as InterpreterError; use clarity::vm::types::{QualifiedContractIdentifier, TypeSignature}; @@ -679,6 +679,7 @@ impl BlockBuilder for NakamotoBlockBuilder { return TransactionResult::problematic(&tx, Error::NetError(e)); } + let cost_before = clarity_tx.cost_so_far(); let (fee, receipt) = match StacksChainState::process_transaction(clarity_tx, tx, quiet, ast_rules) { Ok(x) => x, @@ -686,27 +687,30 @@ impl BlockBuilder for NakamotoBlockBuilder { return parse_process_transaction_error(clarity_tx, tx, e); } }; - - let block_limit_before = clarity_tx - .block_limit() - .expect("Failed to obtain block limit from miner's block connection"); + let cost_after = clarity_tx.cost_so_far(); let mut soft_limit_reached = false; // We only attempt to apply the soft limit to non-boot code contract calls. if non_boot_code_contract_call { if let Some(soft_limit) = self.soft_limit.clone() { - clarity_tx.reset_cost(soft_limit); + let old_limit = clarity_tx + .set_block_limit(soft_limit) + .expect("BUG: No old block limit set."); + clarity_tx.reset_cost(cost_before); soft_limit_reached = matches!( StacksChainState::process_transaction(clarity_tx, tx, quiet, ast_rules), Err(Error::CostOverflowError(_, _, _)) ); - clarity_tx.reset_cost(block_limit_before); + clarity_tx.set_block_limit(old_limit); + clarity_tx.reset_cost(cost_after); } } info!("Include tx"; "tx" => %tx.txid(), "payload" => tx.payload.name(), - "origin" => %tx.origin_address()); + "origin" => %tx.origin_address(), + "soft_limit_reached" => soft_limit_reached + ); // save self.txs.push(tx.clone()); diff --git a/stackslib/src/chainstate/stacks/db/mod.rs b/stackslib/src/chainstate/stacks/db/mod.rs index 6b6f523f88..d098b52a39 100644 --- a/stackslib/src/chainstate/stacks/db/mod.rs +++ b/stackslib/src/chainstate/stacks/db/mod.rs @@ -524,6 +524,12 @@ impl<'a, 'b> ClarityTx<'a, 'b> { self.block.block_limit() } + /// Sets the block limit for the block being created. + /// Returns the replaced block limit if there is one. + pub fn set_block_limit(&mut self, block_limit: ExecutionCost) -> Option { + self.block.set_block_limit(block_limit) + } + /// Run `todo` in this ClarityTx with `new_tracker`. /// Returns the result of `todo` and the `new_tracker` pub fn with_temporary_cost_tracker( diff --git a/stackslib/src/clarity_vm/clarity.rs b/stackslib/src/clarity_vm/clarity.rs index c89679f414..5ebeb4ae7e 100644 --- a/stackslib/src/clarity_vm/clarity.rs +++ b/stackslib/src/clarity_vm/clarity.rs @@ -226,6 +226,15 @@ impl<'a, 'b> ClarityBlockConnection<'a, 'b> { None => None, } } + + /// Sets the block limit for the block being created. + /// Returns the old block limit if there is one. + pub fn set_block_limit(&mut self, limit: ExecutionCost) -> Option { + match self.cost_track { + Some(ref mut track) => track.set_limit(limit), + None => None, + } + } } impl ClarityInstance { diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index d0e329edf5..3ab5026e68 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -9529,8 +9529,8 @@ fn clarity_cost_spend_down() { // setup sender + recipient for some test stx transfers // these are necessary for the interim blocks to get mined at all let tx_fee = 1000; - let small_deploy_fee = 3000; - let large_deploy_fee = 190200; + let small_deploy_fee = 190200; + let large_deploy_fee = 570200; let amount = (large_deploy_fee + small_deploy_fee) * nmb_txs + tx_fee * tenure_count; for sender_addr in sender_addrs { naka_conf.add_initial_balance(PrincipalData::from(sender_addr).to_string(), amount); @@ -9541,7 +9541,7 @@ fn clarity_cost_spend_down() { amount * 2, ); } - naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(25); + naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(1); let stacker_sks: Vec<_> = (0..num_signers) .map(|_| setup_stacker(&mut naka_conf)) .collect(); @@ -9594,7 +9594,7 @@ fn clarity_cost_spend_down() { let small_contract = format!( "(define-public (f) (begin {} (ok 1))) (begin (f))", - (0..2) + (0..1000) .map(|_| format!( "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", boot_code_id("cost-voting", false), @@ -9607,7 +9607,7 @@ fn clarity_cost_spend_down() { // Create an expensive contract that will be republished multiple times let large_contract = format!( "(define-public (f) (begin {} (ok 1))) (begin (f))", - (0..1000) + (0..3000) .map(|_| format!( "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", boot_code_id("cost-voting", false), @@ -9648,30 +9648,31 @@ fn clarity_cost_spend_down() { // Pause mining so we can add all our transactions to the mempool at once. TEST_MINE_STALL.lock().unwrap().replace(true); let mut submitted_txs = vec![]; - for nmb_tx in 0..nmb_txs/2 { + for nmb_tx in 0..nmb_txs / 2 { for sender_sk in sender_sks.iter() { // Just keep redeploying large contracts let contract_tx = make_contract_publish( &sender_sk, sender_nonce, - large_deploy_fee, + small_deploy_fee, naka_conf.burnchain.chain_id, - &format!("expensive-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), - &large_contract, + &format!("cheap-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), + &small_contract, ); let txid = submit_tx(&http_origin, &contract_tx); submitted_txs.push(txid); - // Just keep redeploying large contracts + let contract_tx = make_contract_publish( &sender_sk, sender_nonce + 1, - small_deploy_fee, + large_deploy_fee, naka_conf.burnchain.chain_id, - &format!("cheap-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), - &small_contract, + &format!("expensive-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), + &large_contract, ); let txid = submit_tx(&http_origin, &contract_tx); submitted_txs.push(txid); + // Just keep redeploying large contracts } sender_nonce += 2; } From 5f7d661a017292b3c057c1b8100791bac150cd5f Mon Sep 17 00:00:00 2001 From: Hank Stoever Date: Wed, 13 Nov 2024 16:13:22 -0800 Subject: [PATCH 14/30] wip: use contract-call and change soft_limit_reached logic --- stackslib/src/chainstate/nakamoto/miner.rs | 30 +-- .../src/tests/nakamoto_integrations.rs | 174 ++++++++++++++---- 2 files changed, 161 insertions(+), 43 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 4cb5c7c7c2..91f5cb2daf 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -692,16 +692,22 @@ impl BlockBuilder for NakamotoBlockBuilder { // We only attempt to apply the soft limit to non-boot code contract calls. if non_boot_code_contract_call { if let Some(soft_limit) = self.soft_limit.clone() { - let old_limit = clarity_tx - .set_block_limit(soft_limit) - .expect("BUG: No old block limit set."); - clarity_tx.reset_cost(cost_before); - soft_limit_reached = matches!( - StacksChainState::process_transaction(clarity_tx, tx, quiet, ast_rules), - Err(Error::CostOverflowError(_, _, _)) - ); - clarity_tx.set_block_limit(old_limit); - clarity_tx.reset_cost(cost_after); + let tx_cost = receipt.clone().execution_cost; + let mut cost_after = cost_before.clone(); + cost_after.add(&tx_cost).expect("Cost overflow"); + if cost_after.exceeds(&soft_limit) { + soft_limit_reached = true; + } + // let old_limit = clarity_tx + // .set_block_limit(soft_limit) + // .expect("BUG: No old block limit set."); + // clarity_tx.reset_cost(cost_before.clone()); + // soft_limit_reached = matches!( + // StacksChainState::process_transaction(clarity_tx, tx, quiet, ast_rules), + // Err(Error::CostOverflowError(_, _, _)) + // ); + // clarity_tx.set_block_limit(old_limit); + // clarity_tx.reset_cost(cost_after.clone()); } } @@ -709,7 +715,9 @@ impl BlockBuilder for NakamotoBlockBuilder { "tx" => %tx.txid(), "payload" => tx.payload.name(), "origin" => %tx.origin_address(), - "soft_limit_reached" => soft_limit_reached + "soft_limit_reached" => soft_limit_reached, + "cost_after" => %cost_after, + "cost_before" => %cost_before, ); // save diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index 3ab5026e68..7425bc377b 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -102,11 +102,11 @@ use crate::run_loop::boot_nakamoto; use crate::tests::neon_integrations::{ call_read_only, get_account, get_account_result, get_chain_info_opt, get_chain_info_result, get_neighbors, get_pox_info, next_block_and_wait, run_until_burnchain_height, submit_tx, - test_observer, wait_for_runloop, + submit_tx_fallible, test_observer, wait_for_runloop, }; use crate::tests::{ - gen_random_port, get_chain_info, make_contract_publish, make_contract_publish_versioned, - make_stacks_transfer, to_addr, + gen_random_port, get_chain_info, make_contract_call, make_contract_publish, + make_contract_publish_versioned, make_stacks_transfer, to_addr, }; use crate::{tests, BitcoinRegtestController, BurnchainController, Config, ConfigFile, Keychain}; @@ -9522,6 +9522,17 @@ fn clarity_cost_spend_down() { .collect(); let sender_signer_addrs: Vec<_> = sender_signer_sks.iter().map(tests::to_addr).collect(); let sender_addrs: Vec<_> = sender_sks.iter().map(tests::to_addr).collect(); + let deployer_sk = sender_sks[0]; + let deployer_addr = sender_addrs[0]; + let mut sender_nonces: HashMap = HashMap::new(); + + let mut get_and_increment_nonce = |sender_sk: &Secp256k1PrivateKey| { + // let mut sender_nonces = sender_nonces.lock().unwrap(); + let nonce = sender_nonces.get(&sender_sk.to_hex()).unwrap_or(&0); + let result = *nonce; + sender_nonces.insert(sender_sk.to_hex(), result + 1); + result + }; let tenure_count = 5; let inter_blocks_per_tenure = 30; let nmb_txs = 25; @@ -9590,24 +9601,35 @@ fn clarity_cost_spend_down() { wait_for_first_naka_block_commit(60, &commits_submitted); - let mut sender_nonce = 0; + // let mut sender_nonce = 0; let small_contract = format!( - "(define-public (f) (begin {} (ok 1))) (begin (f))", - (0..1000) - .map(|_| format!( - "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", - boot_code_id("cost-voting", false), - boot_code_id("costs", false), - boot_code_id("costs", false), - )) + r#" +(define-data-var my-var uint u0) +(define-public (f) (begin {} (ok 1))) (begin (f)) + "#, + (0..2000) + .map(|_| format!("(var-get my-var)")) .collect::>() .join(" ") ); + + // let small_contract = format!( + // "(define-public (f) (begin {} (ok 1))) (begin (f))", + // (0..1000) + // .map(|_| format!( + // "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", + // boot_code_id("cost-voting", false), + // boot_code_id("costs", false), + // boot_code_id("costs", false), + // )) + // .collect::>() + // .join(" ") + // ); // Create an expensive contract that will be republished multiple times let large_contract = format!( "(define-public (f) (begin {} (ok 1))) (begin (f))", - (0..3000) + (0..500) .map(|_| format!( "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", boot_code_id("cost-voting", false), @@ -9618,6 +9640,64 @@ fn clarity_cost_spend_down() { .join(" ") ); + // First, lets deploy the contract + let deployer_nonce = get_and_increment_nonce(&deployer_sk); + let small_contract_tx = make_contract_publish( + &deployer_sk, + deployer_nonce, + large_deploy_fee, + naka_conf.burnchain.chain_id, + "small-contract", + &small_contract, + ); + submit_tx(&http_origin, &small_contract_tx); + let deployer_nonce = get_and_increment_nonce(&deployer_sk); + let large_contract_tx = make_contract_publish( + &deployer_sk, + deployer_nonce, + large_deploy_fee, + naka_conf.burnchain.chain_id, + "big-contract", + &large_contract, + ); + submit_tx(&http_origin, &large_contract_tx); + + info!("----- Submitted deploy txs, mining BTC block -----"); + + let blocks_before = mined_blocks.load(Ordering::SeqCst); + let blocks_processed_before = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); + next_block_and(&mut btc_regtest_controller, 60, || { + let blocks_count = mined_blocks.load(Ordering::SeqCst); + let blocks_processed = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); + Ok(blocks_count > blocks_before && blocks_processed > blocks_processed_before) + }) + .unwrap(); + + let blocks_processed_before = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); + let mined_before = test_observer::get_mined_nakamoto_blocks(); + + info!("----- Waiting for deploy txs to be mined -----"); + wait_for(30, || { + let blocks_processed = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); + Ok(blocks_processed > blocks_processed_before + && test_observer::get_mined_nakamoto_blocks().len() > mined_before.len()) + }) + .expect("Timed out waiting for interim blocks to be mined"); + + info!("----- Mining interim blocks -----"); + // Mine `tenure_count` nakamoto tenures for tenure_ix in 0..tenure_count { info!("Mining tenure {tenure_ix}"); @@ -9648,33 +9728,63 @@ fn clarity_cost_spend_down() { // Pause mining so we can add all our transactions to the mempool at once. TEST_MINE_STALL.lock().unwrap().replace(true); let mut submitted_txs = vec![]; - for nmb_tx in 0..nmb_txs / 2 { + for _nmb_tx in 0..nmb_txs { for sender_sk in sender_sks.iter() { + let sender_nonce = get_and_increment_nonce(&sender_sk); // Just keep redeploying large contracts - let contract_tx = make_contract_publish( + let contract_tx = make_contract_call( &sender_sk, sender_nonce, - small_deploy_fee, - naka_conf.burnchain.chain_id, - &format!("cheap-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), - &small_contract, - ); - let txid = submit_tx(&http_origin, &contract_tx); - submitted_txs.push(txid); - - let contract_tx = make_contract_publish( - &sender_sk, - sender_nonce + 1, - large_deploy_fee, + tx_fee, naka_conf.burnchain.chain_id, - &format!("expensive-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), - &large_contract, + &deployer_addr, + "small-contract", + "f", + &[], ); - let txid = submit_tx(&http_origin, &contract_tx); - submitted_txs.push(txid); + // let contract_tx = make_contract_publish( + // &sender_sk, + // sender_nonce, + // small_deploy_fee, + // naka_conf.burnchain.chain_id, + // &format!("cheap-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), + // // &small_contract, + // &small_contract, + // ); + match submit_tx_fallible(&http_origin, &contract_tx) { + Ok(txid) => { + submitted_txs.push(txid); + } + Err(e) => { + warn!("Failed to submit tx: {e}"); + } + } + // let txid = submit_tx_fallible(&http_origin, &contract_tx); + // submitted_txs.push(txid); + + // let sender_nonce = get_and_increment_nonce(&sender_sk); + // // let contract_tx = make_contract_publish( + // // &sender_sk, + // // sender_nonce, + // // large_deploy_fee, + // // naka_conf.burnchain.chain_id, + // // &format!("expensive-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), + // // &large_contract, + // // ); + // let contract_tx = make_contract_call( + // &sender_sk, + // sender_nonce, + // tx_fee, + // naka_conf.burnchain.chain_id, + // &deployer_addr, + // "big-contract", + // "f", + // &[], + // ); + // let txid = submit_tx(&http_origin, &contract_tx); + // submitted_txs.push(txid); // Just keep redeploying large contracts } - sender_nonce += 2; } TEST_MINE_STALL.lock().unwrap().replace(false); wait_for(120, || { From edb64296e1c2f509e4b9101ac8421e3ea6cf60a6 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 13 Nov 2024 16:51:21 -0800 Subject: [PATCH 15/30] Cleanup Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 20 +++++--------------- stackslib/src/chainstate/stacks/db/mod.rs | 6 ------ stackslib/src/clarity_vm/clarity.rs | 9 --------- 3 files changed, 5 insertions(+), 30 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 91f5cb2daf..252ed06b8a 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -693,21 +693,11 @@ impl BlockBuilder for NakamotoBlockBuilder { if non_boot_code_contract_call { if let Some(soft_limit) = self.soft_limit.clone() { let tx_cost = receipt.clone().execution_cost; - let mut cost_after = cost_before.clone(); - cost_after.add(&tx_cost).expect("Cost overflow"); - if cost_after.exceeds(&soft_limit) { - soft_limit_reached = true; - } - // let old_limit = clarity_tx - // .set_block_limit(soft_limit) - // .expect("BUG: No old block limit set."); - // clarity_tx.reset_cost(cost_before.clone()); - // soft_limit_reached = matches!( - // StacksChainState::process_transaction(clarity_tx, tx, quiet, ast_rules), - // Err(Error::CostOverflowError(_, _, _)) - // ); - // clarity_tx.set_block_limit(old_limit); - // clarity_tx.reset_cost(cost_after.clone()); + let mut soft_cost = cost_before.clone(); + soft_cost.add(&tx_cost).expect( + "BUG: Cost overflow was already checked in the process_transaction call.", + ); + soft_limit_reached = soft_cost.exceeds(&soft_limit); } } diff --git a/stackslib/src/chainstate/stacks/db/mod.rs b/stackslib/src/chainstate/stacks/db/mod.rs index d098b52a39..6b6f523f88 100644 --- a/stackslib/src/chainstate/stacks/db/mod.rs +++ b/stackslib/src/chainstate/stacks/db/mod.rs @@ -524,12 +524,6 @@ impl<'a, 'b> ClarityTx<'a, 'b> { self.block.block_limit() } - /// Sets the block limit for the block being created. - /// Returns the replaced block limit if there is one. - pub fn set_block_limit(&mut self, block_limit: ExecutionCost) -> Option { - self.block.set_block_limit(block_limit) - } - /// Run `todo` in this ClarityTx with `new_tracker`. /// Returns the result of `todo` and the `new_tracker` pub fn with_temporary_cost_tracker( diff --git a/stackslib/src/clarity_vm/clarity.rs b/stackslib/src/clarity_vm/clarity.rs index 5ebeb4ae7e..c89679f414 100644 --- a/stackslib/src/clarity_vm/clarity.rs +++ b/stackslib/src/clarity_vm/clarity.rs @@ -226,15 +226,6 @@ impl<'a, 'b> ClarityBlockConnection<'a, 'b> { None => None, } } - - /// Sets the block limit for the block being created. - /// Returns the old block limit if there is one. - pub fn set_block_limit(&mut self, limit: ExecutionCost) -> Option { - match self.cost_track { - Some(ref mut track) => track.set_limit(limit), - None => None, - } - } } impl ClarityInstance { From 2803411fb795a09431e9c572f318b93134502e94 Mon Sep 17 00:00:00 2001 From: Hank Stoever Date: Wed, 13 Nov 2024 17:50:06 -0800 Subject: [PATCH 16/30] fix: proper `soft_limit` set, and fix test mined_blocks assertion --- stackslib/src/chainstate/nakamoto/miner.rs | 9 +- .../src/tests/nakamoto_integrations.rs | 258 +++++++++--------- 2 files changed, 144 insertions(+), 123 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 252ed06b8a..8290d3e5f6 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -544,7 +544,14 @@ impl NakamotoBlockBuilder { remaining_limit.multiply(percentage.into()).expect( "BUG: failed to multiply by {percentage} when previously divided by 100", ); - debug!("Setting soft limit for clarity cost to {percentage}% of remaining block limit: {remaining_limit}"); + remaining_limit.add(&cost_so_far).expect("BUG: unexpected overflow when adding cost_so_far, which was previously checked"); + // TODO: change to debug + info!( + "Setting soft limit for clarity cost to {percentage}% of remaining block limit: {remaining_limit}"; + "remaining_limit" => %remaining_limit, + "cost_so_far" => %cost_so_far, + "block_limit" => %block_limit, + ); soft_limit = Some(remaining_limit); } }; diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index 7425bc377b..bf13799348 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -9526,15 +9526,15 @@ fn clarity_cost_spend_down() { let deployer_addr = sender_addrs[0]; let mut sender_nonces: HashMap = HashMap::new(); - let mut get_and_increment_nonce = |sender_sk: &Secp256k1PrivateKey| { - // let mut sender_nonces = sender_nonces.lock().unwrap(); - let nonce = sender_nonces.get(&sender_sk.to_hex()).unwrap_or(&0); - let result = *nonce; - sender_nonces.insert(sender_sk.to_hex(), result + 1); - result - }; + let get_and_increment_nonce = + |sender_sk: &Secp256k1PrivateKey, sender_nonces: &mut HashMap| { + let nonce = sender_nonces.get(&sender_sk.to_hex()).unwrap_or(&0); + let result = *nonce; + sender_nonces.insert(sender_sk.to_hex(), result + 1); + result + }; let tenure_count = 5; - let inter_blocks_per_tenure = 30; + let _inter_blocks_per_tenure = 30; let nmb_txs = 25; let mut signers = TestSigners::new(sender_signer_sks.clone()); // setup sender + recipient for some test stx transfers @@ -9608,7 +9608,7 @@ fn clarity_cost_spend_down() { (define-data-var my-var uint u0) (define-public (f) (begin {} (ok 1))) (begin (f)) "#, - (0..2000) + (0..250) .map(|_| format!("(var-get my-var)")) .collect::>() .join(" ") @@ -9629,7 +9629,7 @@ fn clarity_cost_spend_down() { // Create an expensive contract that will be republished multiple times let large_contract = format!( "(define-public (f) (begin {} (ok 1))) (begin (f))", - (0..500) + (0..250) .map(|_| format!( "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", boot_code_id("cost-voting", false), @@ -9641,7 +9641,7 @@ fn clarity_cost_spend_down() { ); // First, lets deploy the contract - let deployer_nonce = get_and_increment_nonce(&deployer_sk); + let deployer_nonce = get_and_increment_nonce(&deployer_sk, &mut sender_nonces); let small_contract_tx = make_contract_publish( &deployer_sk, deployer_nonce, @@ -9651,7 +9651,7 @@ fn clarity_cost_spend_down() { &small_contract, ); submit_tx(&http_origin, &small_contract_tx); - let deployer_nonce = get_and_increment_nonce(&deployer_sk); + let deployer_nonce = get_and_increment_nonce(&deployer_sk, &mut sender_nonces); let large_contract_tx = make_contract_publish( &deployer_sk, deployer_nonce, @@ -9701,6 +9701,10 @@ fn clarity_cost_spend_down() { // Mine `tenure_count` nakamoto tenures for tenure_ix in 0..tenure_count { info!("Mining tenure {tenure_ix}"); + let nmb_txs_before = test_observer::get_mined_nakamoto_blocks() + .iter() + .map(|b| b.tx_events.len()) + .sum::(); // Wait for the tenure change payload to be mined let blocks_before = mined_blocks.load(Ordering::SeqCst); let blocks_processed_before = coord_channel @@ -9717,122 +9721,132 @@ fn clarity_cost_spend_down() { }) .unwrap(); // mine the interim blocks - for interim_block_ix in 0..inter_blocks_per_tenure { - info!("Mining interim block {interim_block_ix}"); - let mined_before = test_observer::get_mined_nakamoto_blocks(); - let blocks_processed_before = coord_channel - .lock() - .expect("Mutex poisoned") - .get_stacks_blocks_processed(); + // for interim_block_ix in 0..inter_blocks_per_tenure { + // info!("Mining interim blocks {interim_block_ix}"); + let mined_before = test_observer::get_mined_nakamoto_blocks(); + let blocks_processed_before = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); - // Pause mining so we can add all our transactions to the mempool at once. - TEST_MINE_STALL.lock().unwrap().replace(true); - let mut submitted_txs = vec![]; - for _nmb_tx in 0..nmb_txs { - for sender_sk in sender_sks.iter() { - let sender_nonce = get_and_increment_nonce(&sender_sk); - // Just keep redeploying large contracts - let contract_tx = make_contract_call( - &sender_sk, - sender_nonce, - tx_fee, - naka_conf.burnchain.chain_id, - &deployer_addr, - "small-contract", - "f", - &[], - ); - // let contract_tx = make_contract_publish( - // &sender_sk, - // sender_nonce, - // small_deploy_fee, - // naka_conf.burnchain.chain_id, - // &format!("cheap-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), - // // &small_contract, - // &small_contract, - // ); - match submit_tx_fallible(&http_origin, &contract_tx) { - Ok(txid) => { - submitted_txs.push(txid); - } - Err(e) => { - warn!("Failed to submit tx: {e}"); - } + // Pause mining so we can add all our transactions to the mempool at once. + TEST_MINE_STALL.lock().unwrap().replace(true); + let mut submitted_txs = vec![]; + for _nmb_tx in 0..nmb_txs { + for sender_sk in sender_sks.iter() { + let sender_nonce = get_and_increment_nonce(&sender_sk, &mut sender_nonces); + // Just keep redeploying large contracts + let contract_tx = make_contract_call( + &sender_sk, + sender_nonce, + tx_fee, + naka_conf.burnchain.chain_id, + &deployer_addr, + "small-contract", + "f", + &[], + ); + // let contract_tx = make_contract_publish( + // &sender_sk, + // sender_nonce, + // small_deploy_fee, + // naka_conf.burnchain.chain_id, + // &format!("cheap-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), + // // &small_contract, + // &small_contract, + // ); + match submit_tx_fallible(&http_origin, &contract_tx) { + Ok(txid) => { + submitted_txs.push(txid); + } + Err(_e) => { + // If we fail to submit a tx, we need to make sure we don't + // increment the nonce for this sender, so we don't end up + // skipping a tx. + sender_nonces.insert(sender_sk.to_hex(), sender_nonce); } - // let txid = submit_tx_fallible(&http_origin, &contract_tx); - // submitted_txs.push(txid); - - // let sender_nonce = get_and_increment_nonce(&sender_sk); - // // let contract_tx = make_contract_publish( - // // &sender_sk, - // // sender_nonce, - // // large_deploy_fee, - // // naka_conf.burnchain.chain_id, - // // &format!("expensive-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), - // // &large_contract, - // // ); - // let contract_tx = make_contract_call( - // &sender_sk, - // sender_nonce, - // tx_fee, - // naka_conf.burnchain.chain_id, - // &deployer_addr, - // "big-contract", - // "f", - // &[], - // ); - // let txid = submit_tx(&http_origin, &contract_tx); - // submitted_txs.push(txid); - // Just keep redeploying large contracts } + // let txid = submit_tx_fallible(&http_origin, &contract_tx); + // submitted_txs.push(txid); + + // let sender_nonce = get_and_increment_nonce(&sender_sk); + // // let contract_tx = make_contract_publish( + // // &sender_sk, + // // sender_nonce, + // // large_deploy_fee, + // // naka_conf.burnchain.chain_id, + // // &format!("expensive-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), + // // &large_contract, + // // ); + // let contract_tx = make_contract_call( + // &sender_sk, + // sender_nonce, + // tx_fee, + // naka_conf.burnchain.chain_id, + // &deployer_addr, + // "big-contract", + // "f", + // &[], + // ); + // let txid = submit_tx(&http_origin, &contract_tx); + // submitted_txs.push(txid); + // Just keep redeploying large contracts } - TEST_MINE_STALL.lock().unwrap().replace(false); - wait_for(120, || { - let blocks_processed = coord_channel - .lock() - .expect("Mutex poisoned") - .get_stacks_blocks_processed(); - let total_nmb_mined_txs = test_observer::get_mined_nakamoto_blocks() - .iter() - .map(|b| b.tx_events.len()) - .sum::(); - Ok(blocks_processed > blocks_processed_before - && test_observer::get_mined_nakamoto_blocks().len() > mined_before.len() - && total_nmb_mined_txs > nmb_txs as usize) - }) - .expect("Timed out waiting for interim blocks to be mined"); - - let mined_after = test_observer::get_mined_nakamoto_blocks(); - let mined_blocks = mined_after[mined_before.len()..].to_vec(); - assert!( - mined_blocks.len() > 1, - "Expected at least 2 blocks to be mined in the interim period" + } + TEST_MINE_STALL.lock().unwrap().replace(false); + wait_for(120, || { + let blocks_processed = coord_channel + .lock() + .expect("Mutex poisoned") + .get_stacks_blocks_processed(); + let total_nmb_mined_txs = test_observer::get_mined_nakamoto_blocks() + .iter() + .map(|b| b.tx_events.len()) + .sum::(); + info!("---- Total nmb mined txs: {total_nmb_mined_txs} ----"; + "blocks_processed" => blocks_processed, + "nakamoto_blocks_len" => test_observer::get_mined_nakamoto_blocks().len(), ); - let soft_limit_reached = mined_blocks - .into_iter() - .map(|block| block.tx_events) - .flatten() - .any(|event| match event { - TransactionEvent::Success(TransactionSuccessEvent { - txid: _, - fee: _, - execution_cost: _, - result, - soft_limit_reached, - }) => { - info!("Contract call result: {result}"); - soft_limit_reached - } - _ => { - info!("Unsuccessful event: {event:?}"); - false - } - }); - assert!( - soft_limit_reached, - "Expected at least one block to have a soft limit reached event" + Ok(blocks_processed > blocks_processed_before + && test_observer::get_mined_nakamoto_blocks().len() > mined_before.len() + && (total_nmb_mined_txs - nmb_txs_before) > nmb_txs as usize) + }) + .expect("Timed out waiting for interim blocks to be mined"); + + let mined_after = test_observer::get_mined_nakamoto_blocks(); + let mined_blocks = mined_after[mined_before.len()..].to_vec(); + info!("Mined blocks: {:?}", mined_blocks.len()); + let err_msg = format!( + "Expected at least 2 blocks to be mined in the interim period. Mined blocks: {}, Blocks before: {}", + mined_blocks.len(), + mined_before.len(), ); - } + assert!(mined_blocks.len() > 1, "{err_msg}"); + let soft_limit_reached = mined_blocks + .into_iter() + .map(|block| block.tx_events) + .flatten() + .any(|event| match event { + TransactionEvent::Success(TransactionSuccessEvent { + txid: _, + fee: _, + execution_cost: _, + result, + soft_limit_reached, + }) => { + info!("Contract call result: {result}"); + soft_limit_reached + } + _ => { + info!("Unsuccessful event: {event:?}"); + false + } + }); + assert!( + soft_limit_reached, + "Expected at least one block to have a soft limit reached event" + ); + // } } coord_channel From 3452aa71719faec257ff616b6e2fcb2407f7990d Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 13 Nov 2024 18:12:24 -0800 Subject: [PATCH 17/30] Cleanup Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/stacks/miner.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/stackslib/src/chainstate/stacks/miner.rs b/stackslib/src/chainstate/stacks/miner.rs index 4c4dc763fc..31c20b05c2 100644 --- a/stackslib/src/chainstate/stacks/miner.rs +++ b/stackslib/src/chainstate/stacks/miner.rs @@ -463,7 +463,6 @@ impl TransactionResult { receipt: StacksTransactionReceipt, soft_limit_reached: bool, ) -> TransactionResult { - debug!("SOFT LIMIT IS REACHED: {soft_limit_reached}"); Self::log_transaction_success(transaction); Self::Success(TransactionSuccess { tx: transaction.clone(), From 0c46dbe73c239d64e52bb75981f8cca7fbf11c44 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Wed, 13 Nov 2024 19:58:08 -0800 Subject: [PATCH 18/30] WIP: incomplete test Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 5 +- .../src/tests/nakamoto_integrations.rs | 134 ++++++------------ 2 files changed, 48 insertions(+), 91 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 8290d3e5f6..1fe2093699 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -545,9 +545,8 @@ impl NakamotoBlockBuilder { "BUG: failed to multiply by {percentage} when previously divided by 100", ); remaining_limit.add(&cost_so_far).expect("BUG: unexpected overflow when adding cost_so_far, which was previously checked"); - // TODO: change to debug - info!( - "Setting soft limit for clarity cost to {percentage}% of remaining block limit: {remaining_limit}"; + debug!( + "Setting soft limit for clarity cost to {percentage}% of remaining block limit"; "remaining_limit" => %remaining_limit, "cost_so_far" => %cost_so_far, "block_limit" => %block_limit, diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index bf13799348..be1b35c69c 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -9503,8 +9503,7 @@ fn skip_mining_long_tx() { #[test] #[ignore] /// This test is testing that the clarity cost spend down works as expected, -/// spreading clarity contract calls across the tenure instead of including them all in -/// the first few blocks. It also asserts that this limit resets at the start of each tenure. +/// spreading clarity contract calls across the tenure instead of all in the first block. fn clarity_cost_spend_down() { if env::var("BITCOIND_TEST") != Ok("1".into()) { return; @@ -9534,15 +9533,15 @@ fn clarity_cost_spend_down() { result }; let tenure_count = 5; - let _inter_blocks_per_tenure = 30; - let nmb_txs = 25; + let nmb_txs_per_signer = 6; let mut signers = TestSigners::new(sender_signer_sks.clone()); // setup sender + recipient for some test stx transfers // these are necessary for the interim blocks to get mined at all - let tx_fee = 1000; + let tx_fee = 10000; let small_deploy_fee = 190200; let large_deploy_fee = 570200; - let amount = (large_deploy_fee + small_deploy_fee) * nmb_txs + tx_fee * tenure_count; + let amount = + (large_deploy_fee + small_deploy_fee) + tx_fee * nmb_txs_per_signer + 100 * tenure_count; for sender_addr in sender_addrs { naka_conf.add_initial_balance(PrincipalData::from(sender_addr).to_string(), amount); } @@ -9552,7 +9551,7 @@ fn clarity_cost_spend_down() { amount * 2, ); } - naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(1); + naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(15); let stacker_sks: Vec<_> = (0..num_signers) .map(|_| setup_stacker(&mut naka_conf)) .collect(); @@ -9614,18 +9613,6 @@ fn clarity_cost_spend_down() { .join(" ") ); - // let small_contract = format!( - // "(define-public (f) (begin {} (ok 1))) (begin (f))", - // (0..1000) - // .map(|_| format!( - // "(unwrap! (contract-call? '{} submit-proposal '{} \"cost-old\" '{} \"cost-new\") (err 1))", - // boot_code_id("cost-voting", false), - // boot_code_id("costs", false), - // boot_code_id("costs", false), - // )) - // .collect::>() - // .join(" ") - // ); // Create an expensive contract that will be republished multiple times let large_contract = format!( "(define-public (f) (begin {} (ok 1))) (begin (f))", @@ -9684,7 +9671,7 @@ fn clarity_cost_spend_down() { .expect("Mutex poisoned") .get_stacks_blocks_processed(); let mined_before = test_observer::get_mined_nakamoto_blocks(); - + let commits_before = commits_submitted.load(Ordering::SeqCst); info!("----- Waiting for deploy txs to be mined -----"); wait_for(30, || { let blocks_processed = coord_channel @@ -9692,7 +9679,8 @@ fn clarity_cost_spend_down() { .expect("Mutex poisoned") .get_stacks_blocks_processed(); Ok(blocks_processed > blocks_processed_before - && test_observer::get_mined_nakamoto_blocks().len() > mined_before.len()) + && test_observer::get_mined_nakamoto_blocks().len() > mined_before.len() + && commits_submitted.load(Ordering::SeqCst) > commits_before) }) .expect("Timed out waiting for interim blocks to be mined"); @@ -9701,28 +9689,26 @@ fn clarity_cost_spend_down() { // Mine `tenure_count` nakamoto tenures for tenure_ix in 0..tenure_count { info!("Mining tenure {tenure_ix}"); - let nmb_txs_before = test_observer::get_mined_nakamoto_blocks() - .iter() - .map(|b| b.tx_events.len()) - .sum::(); // Wait for the tenure change payload to be mined let blocks_before = mined_blocks.load(Ordering::SeqCst); let blocks_processed_before = coord_channel .lock() .expect("Mutex poisoned") .get_stacks_blocks_processed(); + let commits_before = commits_submitted.load(Ordering::SeqCst); next_block_and(&mut btc_regtest_controller, 60, || { let blocks_count = mined_blocks.load(Ordering::SeqCst); let blocks_processed = coord_channel .lock() .expect("Mutex poisoned") .get_stacks_blocks_processed(); - Ok(blocks_count > blocks_before && blocks_processed > blocks_processed_before) + Ok(blocks_count > blocks_before + && blocks_processed > blocks_processed_before + && commits_submitted.load(Ordering::SeqCst) > commits_before) }) .unwrap(); + // mine the interim blocks - // for interim_block_ix in 0..inter_blocks_per_tenure { - // info!("Mining interim blocks {interim_block_ix}"); let mined_before = test_observer::get_mined_nakamoto_blocks(); let blocks_processed_before = coord_channel .lock() @@ -9732,10 +9718,10 @@ fn clarity_cost_spend_down() { // Pause mining so we can add all our transactions to the mempool at once. TEST_MINE_STALL.lock().unwrap().replace(true); let mut submitted_txs = vec![]; - for _nmb_tx in 0..nmb_txs { + for _nmb_tx in 0..nmb_txs_per_signer { for sender_sk in sender_sks.iter() { let sender_nonce = get_and_increment_nonce(&sender_sk, &mut sender_nonces); - // Just keep redeploying large contracts + // Fill up the mempool with contract calls let contract_tx = make_contract_call( &sender_sk, sender_nonce, @@ -9746,15 +9732,6 @@ fn clarity_cost_spend_down() { "f", &[], ); - // let contract_tx = make_contract_publish( - // &sender_sk, - // sender_nonce, - // small_deploy_fee, - // naka_conf.burnchain.chain_id, - // &format!("cheap-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), - // // &small_contract, - // &small_contract, - // ); match submit_tx_fallible(&http_origin, &contract_tx) { Ok(txid) => { submitted_txs.push(txid); @@ -9766,34 +9743,10 @@ fn clarity_cost_spend_down() { sender_nonces.insert(sender_sk.to_hex(), sender_nonce); } } - // let txid = submit_tx_fallible(&http_origin, &contract_tx); - // submitted_txs.push(txid); - - // let sender_nonce = get_and_increment_nonce(&sender_sk); - // // let contract_tx = make_contract_publish( - // // &sender_sk, - // // sender_nonce, - // // large_deploy_fee, - // // naka_conf.burnchain.chain_id, - // // &format!("expensive-contract-{sender_nonce}-{nmb_tx}-{tenure_ix}"), - // // &large_contract, - // // ); - // let contract_tx = make_contract_call( - // &sender_sk, - // sender_nonce, - // tx_fee, - // naka_conf.burnchain.chain_id, - // &deployer_addr, - // "big-contract", - // "f", - // &[], - // ); - // let txid = submit_tx(&http_origin, &contract_tx); - // submitted_txs.push(txid); - // Just keep redeploying large contracts } } TEST_MINE_STALL.lock().unwrap().replace(false); + let expected_nmb_txs = nmb_txs_per_signer as usize * sender_sks.len(); wait_for(120, || { let blocks_processed = coord_channel .lock() @@ -9806,10 +9759,11 @@ fn clarity_cost_spend_down() { info!("---- Total nmb mined txs: {total_nmb_mined_txs} ----"; "blocks_processed" => blocks_processed, "nakamoto_blocks_len" => test_observer::get_mined_nakamoto_blocks().len(), + "expected_nmb_txs" => expected_nmb_txs ); Ok(blocks_processed > blocks_processed_before && test_observer::get_mined_nakamoto_blocks().len() > mined_before.len() - && (total_nmb_mined_txs - nmb_txs_before) > nmb_txs as usize) + && total_nmb_mined_txs == expected_nmb_txs) }) .expect("Timed out waiting for interim blocks to be mined"); @@ -9822,31 +9776,35 @@ fn clarity_cost_spend_down() { mined_before.len(), ); assert!(mined_blocks.len() > 1, "{err_msg}"); - let soft_limit_reached = mined_blocks - .into_iter() - .map(|block| block.tx_events) - .flatten() - .any(|event| match event { - TransactionEvent::Success(TransactionSuccessEvent { - txid: _, - fee: _, - execution_cost: _, - result, + let mut last_tx_count = None; + for block in mined_blocks.into_iter() { + let tx_count = block.tx_events.len(); + if let Some(count) = last_tx_count { + assert!( + tx_count <= count, + "Expected fewer txs to be mined each block. Last block: {count}, Current block: {tx_count}" + ); + }; + last_tx_count = Some(tx_count); + + // All but the last transaction should hit the soft limit + for (j, tx_event) in block.tx_events.iter().enumerate() { + if let TransactionEvent::Success(TransactionSuccessEvent { soft_limit_reached, - }) => { - info!("Contract call result: {result}"); - soft_limit_reached - } - _ => { - info!("Unsuccessful event: {event:?}"); - false + .. + }) = tx_event + { + if j == block.tx_events.len() - 1 { + assert!( + !soft_limit_reached, + "Expected tx to not hit the soft limit in the very last block" + ); + } else { + assert!(soft_limit_reached, "Expected tx to hit the soft limit."); + } } - }); - assert!( - soft_limit_reached, - "Expected at least one block to have a soft limit reached event" - ); - // } + } + } } coord_channel From d78bf72a0b0be6ada1b69e88eb8c524ca2b96d88 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 14 Nov 2024 10:52:30 -0800 Subject: [PATCH 19/30] Make test a bit stricter about soft limit check Signed-off-by: Jacinta Ferrant --- clarity/src/vm/costs/mod.rs | 12 ------ .../src/tests/nakamoto_integrations.rs | 42 ++++++------------- 2 files changed, 13 insertions(+), 41 deletions(-) diff --git a/clarity/src/vm/costs/mod.rs b/clarity/src/vm/costs/mod.rs index ae5b588bd0..b3ee746fcf 100644 --- a/clarity/src/vm/costs/mod.rs +++ b/clarity/src/vm/costs/mod.rs @@ -896,18 +896,6 @@ impl LimitedCostTracker { Self::Free => ExecutionCost::max_value(), } } - // Set the limit on the Limited tracker, or do nothing if its free. - // Returns the previous limit if it was set. - pub fn set_limit(&mut self, limit: ExecutionCost) -> Option { - match self { - Self::Limited(ref mut data) => { - let old_limit = data.limit.clone(); - data.limit = limit; - Some(old_limit) - } - Self::Free => None, - } - } pub fn get_memory(&self) -> u64 { match self { diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index be1b35c69c..27d0280c55 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -9504,6 +9504,7 @@ fn skip_mining_long_tx() { #[ignore] /// This test is testing that the clarity cost spend down works as expected, /// spreading clarity contract calls across the tenure instead of all in the first block. +/// It also ensures that the clarity cost resets at the start of each tenure. fn clarity_cost_spend_down() { if env::var("BITCOIND_TEST") != Ok("1".into()) { return; @@ -9533,7 +9534,7 @@ fn clarity_cost_spend_down() { result }; let tenure_count = 5; - let nmb_txs_per_signer = 6; + let nmb_txs_per_signer = 2; let mut signers = TestSigners::new(sender_signer_sks.clone()); // setup sender + recipient for some test stx transfers // these are necessary for the interim blocks to get mined at all @@ -9551,7 +9552,7 @@ fn clarity_cost_spend_down() { amount * 2, ); } - naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(15); + naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(5); let stacker_sks: Vec<_> = (0..num_signers) .map(|_| setup_stacker(&mut naka_conf)) .collect(); @@ -9600,8 +9601,6 @@ fn clarity_cost_spend_down() { wait_for_first_naka_block_commit(60, &commits_submitted); - // let mut sender_nonce = 0; - let small_contract = format!( r#" (define-data-var my-var uint u0) @@ -9714,7 +9713,6 @@ fn clarity_cost_spend_down() { .lock() .expect("Mutex poisoned") .get_stacks_blocks_processed(); - // Pause mining so we can add all our transactions to the mempool at once. TEST_MINE_STALL.lock().unwrap().replace(true); let mut submitted_txs = vec![]; @@ -9746,38 +9744,24 @@ fn clarity_cost_spend_down() { } } TEST_MINE_STALL.lock().unwrap().replace(false); - let expected_nmb_txs = nmb_txs_per_signer as usize * sender_sks.len(); wait_for(120, || { let blocks_processed = coord_channel .lock() .expect("Mutex poisoned") .get_stacks_blocks_processed(); - let total_nmb_mined_txs = test_observer::get_mined_nakamoto_blocks() - .iter() - .map(|b| b.tx_events.len()) - .sum::(); - info!("---- Total nmb mined txs: {total_nmb_mined_txs} ----"; - "blocks_processed" => blocks_processed, - "nakamoto_blocks_len" => test_observer::get_mined_nakamoto_blocks().len(), - "expected_nmb_txs" => expected_nmb_txs - ); - Ok(blocks_processed > blocks_processed_before - && test_observer::get_mined_nakamoto_blocks().len() > mined_before.len() - && total_nmb_mined_txs == expected_nmb_txs) + Ok(blocks_processed >= blocks_processed_before + 7) }) .expect("Timed out waiting for interim blocks to be mined"); let mined_after = test_observer::get_mined_nakamoto_blocks(); - let mined_blocks = mined_after[mined_before.len()..].to_vec(); - info!("Mined blocks: {:?}", mined_blocks.len()); - let err_msg = format!( - "Expected at least 2 blocks to be mined in the interim period. Mined blocks: {}, Blocks before: {}", - mined_blocks.len(), - mined_before.len(), - ); - assert!(mined_blocks.len() > 1, "{err_msg}"); + let mined_blocks: Vec<_> = mined_after.iter().skip(mined_before.len()).collect(); + let total_nmb_txs = mined_after.iter().map(|b| b.tx_events.len()).sum::(); + let nmb_mined_blocks = mined_blocks.len(); + debug!( + "Mined a total of {total_nmb_txs} transactions across {nmb_mined_blocks} mined blocks" + ); let mut last_tx_count = None; - for block in mined_blocks.into_iter() { + for (i, block) in mined_blocks.into_iter().enumerate() { let tx_count = block.tx_events.len(); if let Some(count) = last_tx_count { assert!( @@ -9794,10 +9778,10 @@ fn clarity_cost_spend_down() { .. }) = tx_event { - if j == block.tx_events.len() - 1 { + if i == nmb_mined_blocks - 1 || j != block.tx_events.len() - 1 { assert!( !soft_limit_reached, - "Expected tx to not hit the soft limit in the very last block" + "Expected tx to not hit the soft limit in the very last block or in any txs but the last in all other blocks" ); } else { assert!(soft_limit_reached, "Expected tx to hit the soft limit."); From 9eb5a80ab263e1d81ee669f21f54af7cf377445b Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 14 Nov 2024 12:42:28 -0800 Subject: [PATCH 20/30] Fix failing tests Signed-off-by: Jacinta Ferrant --- testnet/stacks-node/src/tests/nakamoto_integrations.rs | 1 + testnet/stacks-node/src/tests/signer/v0.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index 27d0280c55..17e758d6d6 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -9323,6 +9323,7 @@ fn skip_mining_long_tx() { naka_conf.node.prometheus_bind = Some(prom_bind.clone()); naka_conf.miner.wait_on_interim_blocks = Duration::from_secs(1); naka_conf.miner.nakamoto_attempt_time_ms = 5_000; + naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(100); let sender_1_sk = Secp256k1PrivateKey::from_seed(&[30]); let sender_2_sk = Secp256k1PrivateKey::from_seed(&[31]); // setup sender + recipient for a test stx transfer diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index 5ac25f97bb..566ff6e499 100644 --- a/testnet/stacks-node/src/tests/signer/v0.rs +++ b/testnet/stacks-node/src/tests/signer/v0.rs @@ -1760,6 +1760,7 @@ fn miner_forking() { config.node.pox_sync_sample_secs = 30; config.burnchain.pox_reward_length = Some(max_sortitions as u32); config.miner.block_commit_delay = Duration::from_secs(0); + config.miner.tenure_cost_limit_per_block_percentage = Some(100); config.events_observers.retain(|listener| { let Ok(addr) = std::net::SocketAddr::from_str(&listener.endpoint) else { From 1e0fbba30a54fab3708d6b4219c3f5b3656f2499 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 14 Nov 2024 12:54:03 -0800 Subject: [PATCH 21/30] CRC: use cost_after to determine if soft limit was exceeded Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 1fe2093699..8fb200c54e 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -697,13 +697,8 @@ impl BlockBuilder for NakamotoBlockBuilder { let mut soft_limit_reached = false; // We only attempt to apply the soft limit to non-boot code contract calls. if non_boot_code_contract_call { - if let Some(soft_limit) = self.soft_limit.clone() { - let tx_cost = receipt.clone().execution_cost; - let mut soft_cost = cost_before.clone(); - soft_cost.add(&tx_cost).expect( - "BUG: Cost overflow was already checked in the process_transaction call.", - ); - soft_limit_reached = soft_cost.exceeds(&soft_limit); + if let Some(soft_limit) = self.soft_limit.as_ref() { + soft_limit_reached = cost_after.exceeds(soft_limit); } } From 975251b33d8bbafc03b2b690874c1e3ca8632acd Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 14 Nov 2024 12:55:20 -0800 Subject: [PATCH 22/30] CRC: cleanup comment for soft_limit option Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 8fb200c54e..07eaddf91c 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -124,7 +124,7 @@ pub struct NakamotoBlockBuilder { txs: Vec, /// header we're filling in pub header: NakamotoBlockHeader, - /// The execution cost for the block + /// Optional soft limit for this block's budget usage soft_limit: Option, } From 56087b028b8288cad587557c47f8681418f83e6b Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 14 Nov 2024 13:07:52 -0800 Subject: [PATCH 23/30] CRC: fix miner config parsing Signed-off-by: Jacinta Ferrant --- testnet/stacks-node/src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testnet/stacks-node/src/config.rs b/testnet/stacks-node/src/config.rs index 2e7383d6f8..d2332109c6 100644 --- a/testnet/stacks-node/src/config.rs +++ b/testnet/stacks-node/src/config.rs @@ -2574,7 +2574,7 @@ impl MinerConfigFile { let pre_nakamoto_mock_signing = mining_key.is_some(); let valid_tenure_cost_limit = self .tenure_cost_limit_per_block_percentage - .map(|p| p < 100 && p > 0) + .map(|p| p <= 100 && p > 0) .unwrap_or(true); if !valid_tenure_cost_limit { return Err( From 804b8b937589de25ec92bc520b21636d05086dc3 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 14 Nov 2024 13:08:49 -0800 Subject: [PATCH 24/30] Remove old log Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/stacks/miner.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/stackslib/src/chainstate/stacks/miner.rs b/stackslib/src/chainstate/stacks/miner.rs index 31c20b05c2..7a72cc1652 100644 --- a/stackslib/src/chainstate/stacks/miner.rs +++ b/stackslib/src/chainstate/stacks/miner.rs @@ -2468,7 +2468,6 @@ impl StacksBlockBuilder { } } Error::TransactionTooBigError(measured_cost) => { - debug!("TRANSACTION TOO BIG: {}", &txinfo.tx.txid()); if update_estimator { if let Some(measured_cost) = measured_cost { if let Err(e) = estimator.notify_event( From 21ed87413130dc8c4ab8f9265e777bdb619fceed Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Thu, 14 Nov 2024 13:11:51 -0800 Subject: [PATCH 25/30] Use ranges when checking percentage Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 2 +- testnet/stacks-node/src/config.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 07eaddf91c..4687afd4f7 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -536,7 +536,7 @@ impl NakamotoBlockBuilder { .mempool_settings .tenure_cost_limit_per_block_percentage { - if percentage < 100 { + if (1..100).contains(&percentage) { let mut remaining_limit = block_limit.clone(); let cost_so_far = tenure_tx.cost_so_far(); if remaining_limit.sub(&cost_so_far).is_ok() { diff --git a/testnet/stacks-node/src/config.rs b/testnet/stacks-node/src/config.rs index d2332109c6..903659d7b5 100644 --- a/testnet/stacks-node/src/config.rs +++ b/testnet/stacks-node/src/config.rs @@ -2574,7 +2574,7 @@ impl MinerConfigFile { let pre_nakamoto_mock_signing = mining_key.is_some(); let valid_tenure_cost_limit = self .tenure_cost_limit_per_block_percentage - .map(|p| p <= 100 && p > 0) + .map(|p| (1..=100).contains(&p)) .unwrap_or(true); if !valid_tenure_cost_limit { return Err( From 95ca231f5299e60467927424d2647072dcca71c2 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Fri, 15 Nov 2024 11:15:33 -0800 Subject: [PATCH 26/30] Default to 25% for tenure_cost_limit_per_block_percentage Signed-off-by: Jacinta Ferrant --- testnet/stacks-node/src/config.rs | 26 +++++++++++-------- .../src/tests/nakamoto_integrations.rs | 3 ++- testnet/stacks-node/src/tests/signer/v0.rs | 2 +- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/testnet/stacks-node/src/config.rs b/testnet/stacks-node/src/config.rs index 903659d7b5..b121a5b5a2 100644 --- a/testnet/stacks-node/src/config.rs +++ b/testnet/stacks-node/src/config.rs @@ -2572,16 +2572,20 @@ impl MinerConfigFile { .map(|x| Secp256k1PrivateKey::from_hex(x)) .transpose()?; let pre_nakamoto_mock_signing = mining_key.is_some(); - let valid_tenure_cost_limit = self - .tenure_cost_limit_per_block_percentage - .map(|p| (1..=100).contains(&p)) - .unwrap_or(true); - if !valid_tenure_cost_limit { - return Err( - "miner.tenure_cost_limit_per_block_percentage must be between 1 and 100" - .to_string(), - ); - }; + + let tenure_cost_limit_per_block_percentage = + if let Some(percentage) = self.tenure_cost_limit_per_block_percentage { + if (1..=100).contains(&percentage) { + Some(percentage) + } else { + return Err( + "miner.tenure_cost_limit_per_block_percentage must be between 1 and 100" + .to_string(), + ); + } + } else { + miner_default_config.tenure_cost_limit_per_block_percentage + }; Ok(MinerConfig { first_attempt_time_ms: self .first_attempt_time_ms @@ -2688,7 +2692,7 @@ impl MinerConfigFile { first_rejection_pause_ms: self.first_rejection_pause_ms.unwrap_or(miner_default_config.first_rejection_pause_ms), subsequent_rejection_pause_ms: self.subsequent_rejection_pause_ms.unwrap_or(miner_default_config.subsequent_rejection_pause_ms), block_commit_delay: self.block_commit_delay_ms.map(Duration::from_millis).unwrap_or(miner_default_config.block_commit_delay), - tenure_cost_limit_per_block_percentage: self.tenure_cost_limit_per_block_percentage, + tenure_cost_limit_per_block_percentage, }) } } diff --git a/testnet/stacks-node/src/tests/nakamoto_integrations.rs b/testnet/stacks-node/src/tests/nakamoto_integrations.rs index 17e758d6d6..ba2eca7d31 100644 --- a/testnet/stacks-node/src/tests/nakamoto_integrations.rs +++ b/testnet/stacks-node/src/tests/nakamoto_integrations.rs @@ -8707,6 +8707,7 @@ fn mock_mining() { let (mut naka_conf, _miner_account) = naka_neon_integration_conf(None); naka_conf.miner.wait_on_interim_blocks = Duration::from_secs(1); naka_conf.node.pox_sync_sample_secs = 30; + naka_conf.miner.tenure_cost_limit_per_block_percentage = None; let sender_sk = Secp256k1PrivateKey::new(); let sender_signer_sk = Secp256k1PrivateKey::new(); let sender_signer_addr = tests::to_addr(&sender_signer_sk); @@ -9323,7 +9324,7 @@ fn skip_mining_long_tx() { naka_conf.node.prometheus_bind = Some(prom_bind.clone()); naka_conf.miner.wait_on_interim_blocks = Duration::from_secs(1); naka_conf.miner.nakamoto_attempt_time_ms = 5_000; - naka_conf.miner.tenure_cost_limit_per_block_percentage = Some(100); + naka_conf.miner.tenure_cost_limit_per_block_percentage = None; let sender_1_sk = Secp256k1PrivateKey::from_seed(&[30]); let sender_2_sk = Secp256k1PrivateKey::from_seed(&[31]); // setup sender + recipient for a test stx transfer diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index 566ff6e499..30d25beb69 100644 --- a/testnet/stacks-node/src/tests/signer/v0.rs +++ b/testnet/stacks-node/src/tests/signer/v0.rs @@ -1760,7 +1760,7 @@ fn miner_forking() { config.node.pox_sync_sample_secs = 30; config.burnchain.pox_reward_length = Some(max_sortitions as u32); config.miner.block_commit_delay = Duration::from_secs(0); - config.miner.tenure_cost_limit_per_block_percentage = Some(100); + config.miner.tenure_cost_limit_per_block_percentage = None; config.events_observers.retain(|listener| { let Ok(addr) = std::net::SocketAddr::from_str(&listener.endpoint) else { From f27e9189c91cc55a589f8d05cde450c66610a2b9 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Fri, 15 Nov 2024 11:56:19 -0800 Subject: [PATCH 27/30] CRC: cleanup Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 36 ++++++++++------------ testnet/stacks-node/src/config.rs | 4 ++- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index 4687afd4f7..b155e95441 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -536,25 +536,23 @@ impl NakamotoBlockBuilder { .mempool_settings .tenure_cost_limit_per_block_percentage { - if (1..100).contains(&percentage) { - let mut remaining_limit = block_limit.clone(); - let cost_so_far = tenure_tx.cost_so_far(); - if remaining_limit.sub(&cost_so_far).is_ok() { - if remaining_limit.divide(100).is_ok() { - remaining_limit.multiply(percentage.into()).expect( - "BUG: failed to multiply by {percentage} when previously divided by 100", - ); - remaining_limit.add(&cost_so_far).expect("BUG: unexpected overflow when adding cost_so_far, which was previously checked"); - debug!( - "Setting soft limit for clarity cost to {percentage}% of remaining block limit"; - "remaining_limit" => %remaining_limit, - "cost_so_far" => %cost_so_far, - "block_limit" => %block_limit, - ); - soft_limit = Some(remaining_limit); - } - }; - } + let mut remaining_limit = block_limit.clone(); + let cost_so_far = tenure_tx.cost_so_far(); + if remaining_limit.sub(&cost_so_far).is_ok() { + if remaining_limit.divide(100).is_ok() { + remaining_limit.multiply(percentage.into()).expect( + "BUG: failed to multiply by {percentage} when previously divided by 100", + ); + remaining_limit.add(&cost_so_far).expect("BUG: unexpected overflow when adding cost_so_far, which was previously checked"); + debug!( + "Setting soft limit for clarity cost to {percentage}% of remaining block limit"; + "remaining_limit" => %remaining_limit, + "cost_so_far" => %cost_so_far, + "block_limit" => %block_limit, + ); + soft_limit = Some(remaining_limit); + } + }; } builder.soft_limit = soft_limit; diff --git a/testnet/stacks-node/src/config.rs b/testnet/stacks-node/src/config.rs index b121a5b5a2..18bb145ba5 100644 --- a/testnet/stacks-node/src/config.rs +++ b/testnet/stacks-node/src/config.rs @@ -2575,7 +2575,9 @@ impl MinerConfigFile { let tenure_cost_limit_per_block_percentage = if let Some(percentage) = self.tenure_cost_limit_per_block_percentage { - if (1..=100).contains(&percentage) { + if percentage == 100 { + None + } else if percentage > 0 && percentage < 100 { Some(percentage) } else { return Err( From 99d9a80063ad92d461cf79d6812df638206cc762 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Fri, 15 Nov 2024 12:01:35 -0800 Subject: [PATCH 28/30] Add an assert to account for a faulty test Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index b155e95441..fe8d8f193a 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -536,6 +536,11 @@ impl NakamotoBlockBuilder { .mempool_settings .tenure_cost_limit_per_block_percentage { + // Make sure we aren't actually going to multiply by 0 or attempt to increase the block limit. + assert!( + (1..100).contains(&percentage), + "BUG: tenure_cost_limit_per_block_percentage must be between between 1 and 99." + ); let mut remaining_limit = block_limit.clone(); let cost_so_far = tenure_tx.cost_so_far(); if remaining_limit.sub(&cost_so_far).is_ok() { From b2d6fad07cb2bf45b54018ba89e8019414e704e6 Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Fri, 15 Nov 2024 13:25:52 -0800 Subject: [PATCH 29/30] Cleanup Signed-off-by: Jacinta Ferrant --- testnet/stacks-node/src/config.rs | 2 +- testnet/stacks-node/src/tests/signer/v0.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/testnet/stacks-node/src/config.rs b/testnet/stacks-node/src/config.rs index 18bb145ba5..18c07e8953 100644 --- a/testnet/stacks-node/src/config.rs +++ b/testnet/stacks-node/src/config.rs @@ -2153,7 +2153,7 @@ pub struct MinerConfig { pub subsequent_rejection_pause_ms: u64, /// Duration to wait for a Nakamoto block after seeing a burnchain block before submitting a block commit. pub block_commit_delay: Duration, - /// The percentage of the remaining tenure cost limit to consume each block. Defaults to 25%. + /// The percentage of the remaining tenure cost limit to consume each block. pub tenure_cost_limit_per_block_percentage: Option, } diff --git a/testnet/stacks-node/src/tests/signer/v0.rs b/testnet/stacks-node/src/tests/signer/v0.rs index 30d25beb69..747900ea08 100644 --- a/testnet/stacks-node/src/tests/signer/v0.rs +++ b/testnet/stacks-node/src/tests/signer/v0.rs @@ -920,7 +920,9 @@ fn forked_tenure_testing( // need) TEST_SKIP_BLOCK_BROADCAST.lock().unwrap().replace(true); }, - |_| {}, + |config| { + config.miner.tenure_cost_limit_per_block_percentage = None; + }, None, None, ); From 7a3946d842b6d1f93d6ce02b5a079b9b06d0e76d Mon Sep 17 00:00:00 2001 From: Jacinta Ferrant Date: Fri, 15 Nov 2024 13:33:19 -0800 Subject: [PATCH 30/30] Fix assert Signed-off-by: Jacinta Ferrant --- stackslib/src/chainstate/nakamoto/miner.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stackslib/src/chainstate/nakamoto/miner.rs b/stackslib/src/chainstate/nakamoto/miner.rs index fe8d8f193a..0291b1dad2 100644 --- a/stackslib/src/chainstate/nakamoto/miner.rs +++ b/stackslib/src/chainstate/nakamoto/miner.rs @@ -538,8 +538,8 @@ impl NakamotoBlockBuilder { { // Make sure we aren't actually going to multiply by 0 or attempt to increase the block limit. assert!( - (1..100).contains(&percentage), - "BUG: tenure_cost_limit_per_block_percentage must be between between 1 and 99." + (1..=100).contains(&percentage), + "BUG: tenure_cost_limit_per_block_percentage: {percentage}%. Must be between between 1 and 100" ); let mut remaining_limit = block_limit.clone(); let cost_so_far = tenure_tx.cost_so_far();