From 92bb8dd039b058906a69b6c89dc393eb82a678b4 Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Thu, 21 Sep 2023 10:22:10 +0300 Subject: [PATCH 1/8] turn tx selector into an ongoing process with persistent state (wip: some tests are broken; selector is not used correctly by builder) --- mining/src/block_template/builder.rs | 4 +- mining/src/block_template/model/tx.rs | 17 ++-- mining/src/block_template/selector.rs | 137 +++++++++++++------------- 3 files changed, 79 insertions(+), 79 deletions(-) diff --git a/mining/src/block_template/builder.rs b/mining/src/block_template/builder.rs index 5348c653a..3f83919ed 100644 --- a/mining/src/block_template/builder.rs +++ b/mining/src/block_template/builder.rs @@ -1,4 +1,4 @@ -use super::{errors::BuilderResult, policy::Policy}; +use super::{errors::BuilderResult, policy::Policy, selector::TxSelector}; use crate::{block_template::selector::TransactionsSelector, model::candidate_tx::CandidateTransaction}; use kaspa_consensus_core::{ api::ConsensusApi, @@ -105,7 +105,7 @@ impl BlockTemplateBuilder { } pub(crate) fn reject_transaction(&mut self, transaction_id: TransactionId) { - self.selector.reject(transaction_id); + self.selector.reject_selection(transaction_id); } pub(crate) fn candidates_len(&self) -> usize { diff --git a/mining/src/block_template/model/tx.rs b/mining/src/block_template/model/tx.rs index dee461f63..b0c7e3f56 100644 --- a/mining/src/block_template/model/tx.rs +++ b/mining/src/block_template/model/tx.rs @@ -1,14 +1,11 @@ pub(crate) struct SelectableTransaction { pub(crate) gas_limit: u64, pub(crate) p: f64, - - /// Has this candidate been rejected by the consensus? - pub(crate) is_rejected: bool, } impl SelectableTransaction { pub(crate) fn new(tx_value: f64, gas_limit: u64, alpha: i32) -> Self { - Self { gas_limit, p: tx_value.powi(alpha), is_rejected: false } + Self { gas_limit, p: tx_value.powi(alpha) } } } @@ -22,6 +19,7 @@ pub(crate) struct Candidate { /// Range start in the candidate list total_p space pub(crate) start: f64, + /// Range end in the candidate list total_p space pub(crate) end: f64, @@ -35,6 +33,7 @@ impl Candidate { } } +#[derive(Default)] pub(crate) struct CandidateList { pub(crate) candidates: Vec, pub(crate) total_p: f64, @@ -45,12 +44,10 @@ impl CandidateList { let mut candidates = Vec::with_capacity(selectable_txs.len()); let mut total_p = 0.0; selectable_txs.iter().enumerate().for_each(|(i, tx)| { - if !tx.is_rejected { - let current_p = tx.p; - let candidate = Candidate::new(i, total_p, total_p + current_p); - candidates.push(candidate); - total_p += current_p; - } + let current_p = tx.p; + let candidate = Candidate::new(i, total_p, total_p + current_p); + candidates.push(candidate); + total_p += current_p; }); Self { candidates, total_p } } diff --git a/mining/src/block_template/selector.rs b/mining/src/block_template/selector.rs index 3348d7f2f..e55642a6a 100644 --- a/mining/src/block_template/selector.rs +++ b/mining/src/block_template/selector.rs @@ -1,9 +1,6 @@ use kaspa_core::{time::Stopwatch, trace}; use rand::Rng; -use std::{ - collections::{HashMap, HashSet}, - vec, -}; +use std::collections::HashMap; use crate::model::candidate_tx::CandidateTransaction; @@ -30,6 +27,11 @@ const ALPHA: i32 = 3; /// if REBALANCE_THRESHOLD is 0.95, there's a 1-in-20 chance of collision. const REBALANCE_THRESHOLD: f64 = 0.95; +pub trait TxSelector { + fn select_transactions(&mut self) -> Vec; + fn reject_selection(&mut self, invalid_tx_id: TransactionId); +} + pub(crate) struct TransactionsSelector { policy: Policy, /// Transaction store @@ -37,16 +39,20 @@ pub(crate) struct TransactionsSelector { /// Selectable transactions store selectable_txs: SelectableTransactions, - /// Indexes of transactions keys in stores - rejected_txs: HashSet, - - /// Number of transactions marked as rejected - committed_rejects: usize, - /// Indexes of selected transactions in stores selected_txs: Vec, + + /// Optional state for handling selection rejections. Maps from a selected tx id + /// to the index of the tx in the `transactions` vec + selected_txs_map: Option>, + + // Inner state of the selection process + candidate_list: CandidateList, + used_count: usize, + used_p: f64, total_mass: u64, total_fees: u64, + gas_usage_map: HashMap, } impl TransactionsSelector { @@ -59,23 +65,28 @@ impl TransactionsSelector { let mut selector = Self { policy, transactions, - selectable_txs: vec![], - rejected_txs: Default::default(), - committed_rejects: 0, - selected_txs: vec![], + selectable_txs: Default::default(), + selected_txs: Default::default(), + selected_txs_map: None, + candidate_list: Default::default(), + used_count: 0, + used_p: 0.0, total_mass: 0, total_fees: 0, + gas_usage_map: Default::default(), }; // Create the selectable transactions selector.selectable_txs = selector.transactions.iter().map(|x| SelectableTransaction::new(selector.calc_tx_value(x), 0, ALPHA)).collect(); + // Prepare the initial candidate list + selector.candidate_list = CandidateList::new(&selector.selectable_txs); selector } pub(crate) fn len(&self) -> usize { - self.transactions.len() - self.rejected_txs.len() - self.committed_rejects + self.transactions.len() } /// select_transactions implements a probabilistic transaction selection algorithm. @@ -102,29 +113,25 @@ impl TransactionsSelector { let _sw = Stopwatch::<15>::with_threshold("select_transaction op"); let mut rng = rand::thread_rng(); - self.reset(); - let mut candidate_list = CandidateList::new(&self.selectable_txs); - let mut used_count = 0; - let mut used_p = 0.0; - let mut gas_usage_map: HashMap = HashMap::new(); + self.reset_selection(); - while candidate_list.candidates.len() - used_count > 0 { + while self.candidate_list.candidates.len() - self.used_count > 0 { // Rebalance the candidates if it's required - if used_p >= REBALANCE_THRESHOLD * candidate_list.total_p { - candidate_list = candidate_list.rebalanced(&self.selectable_txs); - used_count = 0; - used_p = 0.0; + if self.used_p >= REBALANCE_THRESHOLD * self.candidate_list.total_p { + self.candidate_list = self.candidate_list.rebalanced(&self.selectable_txs); + self.used_count = 0; + self.used_p = 0.0; // Break if we now ran out of transactions - if candidate_list.is_empty() { + if self.candidate_list.is_empty() { break; } } // Select a candidate tx at random - let r = rng.gen::() * candidate_list.total_p; - let selected_candidate_idx = candidate_list.find(r); - let selected_candidate = candidate_list.candidates.get_mut(selected_candidate_idx).unwrap(); + let r = rng.gen::() * self.candidate_list.total_p; + let selected_candidate_idx = self.candidate_list.find(r); + let selected_candidate = self.candidate_list.candidates.get_mut(selected_candidate_idx).unwrap(); // If is_marked_for_deletion is set, it means we got a collision. // Ignore and select another Tx. @@ -145,7 +152,7 @@ impl TransactionsSelector { // Also check for overflow. if !selected_tx.tx.subnetwork_id.is_builtin_or_native() { let subnetwork_id = selected_tx.tx.subnetwork_id.clone(); - let gas_usage = gas_usage_map.entry(subnetwork_id.clone()).or_insert(0); + let gas_usage = self.gas_usage_map.entry(subnetwork_id.clone()).or_insert(0); let tx_gas = selected_tx.tx.gas; let next_gas_usage = (*gas_usage).checked_add(tx_gas); if next_gas_usage.is_none() || next_gas_usage.unwrap() > self.selectable_txs[selected_candidate.index].gas_limit { @@ -154,19 +161,19 @@ impl TransactionsSelector { selected_tx.tx.id(), subnetwork_id ); - for i in selected_candidate_idx..candidate_list.candidates.len() { - let transaction_index = candidate_list.candidates[i].index; - // candidateTxs are ordered by subnetwork, so we can safely assume - // that transactions after subnetworkID will not be relevant. + for i in selected_candidate_idx..self.candidate_list.candidates.len() { + let transaction_index = self.candidate_list.candidates[i].index; + // Candidate txs are ordered by subnetwork, so we can safely assume + // that transactions after subnetwork_id will not be relevant. if subnetwork_id < self.transactions[transaction_index].tx.subnetwork_id { break; } - let current = candidate_list.candidates.get_mut(i).unwrap(); + let current = self.candidate_list.candidates.get_mut(i).unwrap(); // Mark for deletion current.is_marked_for_deletion = true; - used_count += 1; - used_p += self.selectable_txs[transaction_index].p; + self.used_count += 1; + self.used_p += self.selectable_txs[transaction_index].p; } continue; } @@ -182,15 +189,15 @@ impl TransactionsSelector { self.total_fees += selected_tx.calculated_fee; trace!( - "Adding tx {0} (feePerMegaGram {1})", + "Adding tx {0} (fee per megagram: {1})", selected_tx.tx.id(), selected_tx.calculated_fee * 1_000_000 / selected_tx.calculated_mass ); // Mark for deletion selected_candidate.is_marked_for_deletion = true; - used_count += 1; - used_p += self.selectable_txs[selected_candidate.index].p; + self.used_count += 1; + self.used_p += self.selectable_txs[selected_candidate.index].p; } self.selected_txs.sort(); @@ -203,33 +210,11 @@ impl TransactionsSelector { self.selected_txs.iter().map(|x| self.transactions[*x].tx.as_ref().clone()).collect() } - pub(crate) fn reject(&mut self, transaction_id: TransactionId) { - self.rejected_txs.insert(transaction_id); - } - - fn commit_rejects(&mut self) { - let _sw = Stopwatch::<5>::with_threshold("commit_rejects op"); - if self.rejected_txs.is_empty() { - return; - } - for (index, tx) in self.transactions.iter().enumerate() { - if !self.selectable_txs[index].is_rejected && self.rejected_txs.remove(&tx.tx.id()) { - self.selectable_txs[index].is_rejected = true; - self.committed_rejects += 1; - if self.rejected_txs.is_empty() { - break; - } - } - } - assert!(self.rejected_txs.is_empty()); - } - - fn reset(&mut self) { + fn reset_selection(&mut self) { assert_eq!(self.transactions.len(), self.selectable_txs.len()); + // TODO: consider to min with the approximated amount of txs which fit into max block mass self.selected_txs = Vec::with_capacity(self.transactions.len()); - self.total_fees = 0; - self.total_mass = 0; - self.commit_rejects(); + self.selected_txs_map = None; } /// calc_tx_value calculates a value to be used in transaction selection. @@ -249,6 +234,25 @@ impl TransactionsSelector { } } +impl TxSelector for TransactionsSelector { + fn select_transactions(&mut self) -> Vec { + self.select_transactions() + } + + fn reject_selection(&mut self, invalid_tx_id: TransactionId) { + let selected_txs_map = self + .selected_txs_map + .get_or_insert_with(|| self.selected_txs.iter().map(|&x| (self.transactions[x].tx.id(), x)).collect()); + let tx_index = *selected_txs_map.get(&invalid_tx_id).expect("only previously selected txs can be rejected"); + let tx = &self.transactions[tx_index]; + self.total_mass -= tx.calculated_mass; + self.total_fees -= tx.calculated_fee; + if !tx.tx.subnetwork_id.is_builtin_or_native() { + *self.gas_usage_map.get_mut(&tx.tx.subnetwork_id).expect("previously selected txs have an entry") -= tx.tx.gas; + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -278,10 +282,9 @@ mod tests { let mut remaining_count = TX_INITIAL_COUNT; for i in 0..3 { let selected_txs = selector.select_transactions(); - selected_txs.iter().skip((i + 1) * 100).take(REJECT_COUNT).for_each(|x| selector.reject(x.id())); + selected_txs.iter().skip((i + 1) * 100).take(REJECT_COUNT).for_each(|x| selector.reject_selection(x.id())); remaining_count -= REJECT_COUNT; assert_eq!(selector.len(), remaining_count, "selector length matches remaining transaction count"); - selector.commit_rejects(); assert_eq!(selector.len(), remaining_count, "selector length matches remaining transaction count"); let selected_txs_2 = selector.select_transactions(); assert_eq!(selector.len(), remaining_count, "selector length matches remaining transaction count"); From 0e54b664e1ad66ff85ef46af213e4252be567ad0 Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Thu, 21 Sep 2023 11:51:28 +0300 Subject: [PATCH 2/8] use tx selector for BBT (wip: virtual processor retry logic) --- consensus/core/src/api/mod.rs | 8 ++- consensus/core/src/block.rs | 11 +++- consensus/src/consensus/mod.rs | 10 ++-- .../pipeline/virtual_processor/processor.rs | 11 +++- .../src/pipeline/virtual_processor/tests.rs | 29 +++++++++-- mining/src/block_template/builder.rs | 37 ++++---------- mining/src/block_template/selector.rs | 50 +++++++++---------- mining/src/manager.rs | 31 ++---------- mining/src/manager_tests.rs | 4 +- mining/src/testutils/consensus_mock.rs | 9 +++- simpa/src/simulator/miner.rs | 24 ++++++++- 11 files changed, 128 insertions(+), 96 deletions(-) diff --git a/consensus/core/src/api/mod.rs b/consensus/core/src/api/mod.rs index 316bfd3f5..c11ea8317 100644 --- a/consensus/core/src/api/mod.rs +++ b/consensus/core/src/api/mod.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use crate::{ acceptance_data::AcceptanceData, - block::{Block, BlockTemplate}, + block::{Block, BlockTemplate, TemplateTransactionSelector}, block_count::BlockCount, blockstatus::BlockStatus, coinbase::MinerData, @@ -27,7 +27,11 @@ pub type BlockValidationFuture = BoxFuture<'static, BlockProcessResult) -> Result { + fn build_block_template( + &self, + miner_data: MinerData, + tx_selector: Box, + ) -> Result { unimplemented!() } diff --git a/consensus/core/src/block.rs b/consensus/core/src/block.rs index acbdfcb88..21c0b531c 100644 --- a/consensus/core/src/block.rs +++ b/consensus/core/src/block.rs @@ -1,6 +1,10 @@ use std::sync::Arc; -use crate::{coinbase::MinerData, header::Header, tx::Transaction}; +use crate::{ + coinbase::MinerData, + header::Header, + tx::{Transaction, TransactionId}, +}; use kaspa_hashes::Hash; /// A mutable block structure where header and transactions within can still be mutated. @@ -64,6 +68,11 @@ impl Block { } } +pub trait TemplateTransactionSelector { + fn select_transactions(&mut self) -> Vec; + fn reject_selection(&mut self, tx_id: TransactionId); +} + /// A block template for miners. #[derive(Debug, Clone)] pub struct BlockTemplate { diff --git a/consensus/src/consensus/mod.rs b/consensus/src/consensus/mod.rs index 673a7823b..0071ea07f 100644 --- a/consensus/src/consensus/mod.rs +++ b/consensus/src/consensus/mod.rs @@ -41,7 +41,7 @@ use crate::{ use kaspa_consensus_core::{ acceptance_data::AcceptanceData, api::{BlockValidationFuture, ConsensusApi}, - block::{Block, BlockTemplate}, + block::{Block, BlockTemplate, TemplateTransactionSelector}, block_count::BlockCount, blockhash::BlockHashExtensions, blockstatus::BlockStatus, @@ -353,8 +353,12 @@ impl Consensus { } impl ConsensusApi for Consensus { - fn build_block_template(&self, miner_data: MinerData, txs: Vec) -> Result { - self.virtual_processor.build_block_template(miner_data, txs) + fn build_block_template( + &self, + miner_data: MinerData, + tx_selector: Box, + ) -> Result { + self.virtual_processor.build_block_template(miner_data, tx_selector) } fn validate_and_insert_block(&self, block: Block) -> BlockValidationFuture { diff --git a/consensus/src/pipeline/virtual_processor/processor.rs b/consensus/src/pipeline/virtual_processor/processor.rs index 499103a05..90720f5ac 100644 --- a/consensus/src/pipeline/virtual_processor/processor.rs +++ b/consensus/src/pipeline/virtual_processor/processor.rs @@ -48,7 +48,7 @@ use crate::{ }; use kaspa_consensus_core::{ acceptance_data::AcceptanceData, - block::{BlockTemplate, MutableBlock}, + block::{BlockTemplate, MutableBlock, TemplateTransactionSelector}, blockstatus::BlockStatus::{StatusDisqualifiedFromChain, StatusUTXOValid}, coinbase::MinerData, config::genesis::GenesisBlock, @@ -787,8 +787,15 @@ impl VirtualStateProcessor { Ok(()) } - pub fn build_block_template(&self, miner_data: MinerData, txs: Vec) -> Result { + pub fn build_block_template( + &self, + miner_data: MinerData, + mut tx_selector: Box, + ) -> Result { // TODO: tests + // + + let txs = tx_selector.select_transactions(); let virtual_read = self.virtual_stores.read(); let virtual_state = virtual_read.state.get().unwrap(); let virtual_utxo_view = &virtual_read.utxo_set; diff --git a/consensus/src/pipeline/virtual_processor/tests.rs b/consensus/src/pipeline/virtual_processor/tests.rs index 9c6f30927..119a57087 100644 --- a/consensus/src/pipeline/virtual_processor/tests.rs +++ b/consensus/src/pipeline/virtual_processor/tests.rs @@ -1,17 +1,37 @@ use crate::{consensus::test_consensus::TestConsensus, model::services::reachability::ReachabilityService}; use kaspa_consensus_core::{ api::ConsensusApi, - block::{Block, BlockTemplate, MutableBlock}, + block::{Block, BlockTemplate, MutableBlock, TemplateTransactionSelector}, blockhash, blockstatus::BlockStatus, coinbase::MinerData, config::{params::MAINNET_PARAMS, ConfigBuilder}, - tx::{ScriptPublicKey, ScriptVec}, + tx::{ScriptPublicKey, ScriptVec, Transaction}, BlockHashSet, }; use kaspa_hashes::Hash; use std::{collections::VecDeque, thread::JoinHandle}; +struct OnetimeTxSelector { + txs: Option>, +} + +impl OnetimeTxSelector { + fn new(txs: Vec) -> Self { + Self { txs: Some(txs) } + } +} + +impl TemplateTransactionSelector for OnetimeTxSelector { + fn select_transactions(&mut self) -> Vec { + self.txs.take().unwrap() + } + + fn reject_selection(&mut self, _tx_id: kaspa_consensus_core::tx::TransactionId) { + unimplemented!() + } +} + struct TestContext { consensus: TestConsensus, join_handles: Vec>, @@ -78,7 +98,10 @@ impl TestContext { } pub fn build_block_template(&self, nonce: u64, timestamp: u64) -> BlockTemplate { - let mut t = self.consensus.build_block_template(self.miner_data.clone(), Default::default()).unwrap(); + let mut t = self + .consensus + .build_block_template(self.miner_data.clone(), Box::new(OnetimeTxSelector::new(Default::default()))) + .unwrap(); t.block.header.timestamp = timestamp; t.block.header.nonce = nonce; t.block.header.finalize(); diff --git a/mining/src/block_template/builder.rs b/mining/src/block_template/builder.rs index 3f83919ed..a3468e8e2 100644 --- a/mining/src/block_template/builder.rs +++ b/mining/src/block_template/builder.rs @@ -1,11 +1,7 @@ -use super::{errors::BuilderResult, policy::Policy, selector::TxSelector}; +use super::{errors::BuilderResult, policy::Policy}; use crate::{block_template::selector::TransactionsSelector, model::candidate_tx::CandidateTransaction}; use kaspa_consensus_core::{ - api::ConsensusApi, - block::BlockTemplate, - coinbase::MinerData, - merkle::calc_hash_merkle_root, - tx::{TransactionId, COINBASE_TRANSACTION_INDEX}, + api::ConsensusApi, block::BlockTemplate, coinbase::MinerData, merkle::calc_hash_merkle_root, tx::COINBASE_TRANSACTION_INDEX, }; use kaspa_core::{ debug, @@ -14,15 +10,12 @@ use kaspa_core::{ pub(crate) struct BlockTemplateBuilder { policy: Policy, - selector: TransactionsSelector, } impl BlockTemplateBuilder { - pub(crate) fn new(max_block_mass: u64, transactions: Vec) -> Self { - let _sw = Stopwatch::<50>::with_threshold("BlockTemplateBuilder::new"); + pub(crate) fn new(max_block_mass: u64) -> Self { let policy = Policy::new(max_block_mass); - let selector = TransactionsSelector::new(policy.clone(), transactions); - Self { policy, selector } + Self { policy } } /// BuildBlockTemplate creates a block template for a miner to consume @@ -89,27 +82,15 @@ impl BlockTemplateBuilder { /// | <= policy.BlockMinSize) | | /// ----------------------------------- -- pub(crate) fn build_block_template( - &mut self, + &self, consensus: &dyn ConsensusApi, miner_data: &MinerData, + transactions: Vec, ) -> BuilderResult { let _sw = Stopwatch::<20>::with_threshold("build_block_template op"); - debug!("Considering {} transactions for a new block template", self.selector.len()); - let block_txs = self.selector.select_transactions(); - Ok(consensus.build_block_template(miner_data.clone(), block_txs)?) - } - - pub(crate) fn update_transactions(&mut self, transactions: Vec) { - let selector = TransactionsSelector::new(self.policy.clone(), transactions); - self.selector = selector; - } - - pub(crate) fn reject_transaction(&mut self, transaction_id: TransactionId) { - self.selector.reject_selection(transaction_id); - } - - pub(crate) fn candidates_len(&self) -> usize { - self.selector.len() + debug!("Considering {} transactions for a new block template", transactions.len()); + let selector = Box::new(TransactionsSelector::new(self.policy.clone(), transactions)); + Ok(consensus.build_block_template(miner_data.clone(), selector)?) } /// modify_block_template clones an existing block template, modifies it to the requested coinbase data and updates the timestamp diff --git a/mining/src/block_template/selector.rs b/mining/src/block_template/selector.rs index e55642a6a..53ec0a332 100644 --- a/mining/src/block_template/selector.rs +++ b/mining/src/block_template/selector.rs @@ -9,6 +9,7 @@ use super::{ policy::Policy, }; use kaspa_consensus_core::{ + block::TemplateTransactionSelector, subnets::SubnetworkId, tx::{Transaction, TransactionId}, }; @@ -27,11 +28,6 @@ const ALPHA: i32 = 3; /// if REBALANCE_THRESHOLD is 0.95, there's a 1-in-20 chance of collision. const REBALANCE_THRESHOLD: f64 = 0.95; -pub trait TxSelector { - fn select_transactions(&mut self) -> Vec; - fn reject_selection(&mut self, invalid_tx_id: TransactionId); -} - pub(crate) struct TransactionsSelector { policy: Policy, /// Transaction store @@ -85,10 +81,6 @@ impl TransactionsSelector { selector } - pub(crate) fn len(&self) -> usize { - self.transactions.len() - } - /// select_transactions implements a probabilistic transaction selection algorithm. /// The algorithm, roughly, is as follows: /// 1. We assign a probability to each transaction equal to: @@ -234,16 +226,16 @@ impl TransactionsSelector { } } -impl TxSelector for TransactionsSelector { +impl TemplateTransactionSelector for TransactionsSelector { fn select_transactions(&mut self) -> Vec { self.select_transactions() } - fn reject_selection(&mut self, invalid_tx_id: TransactionId) { + fn reject_selection(&mut self, tx_id: TransactionId) { let selected_txs_map = self .selected_txs_map .get_or_insert_with(|| self.selected_txs.iter().map(|&x| (self.transactions[x].tx.id(), x)).collect()); - let tx_index = *selected_txs_map.get(&invalid_tx_id).expect("only previously selected txs can be rejected"); + let tx_index = *selected_txs_map.get(&tx_id).expect("only previously selected txs can be rejected"); let tx = &self.transactions[tx_index]; self.total_mass -= tx.calculated_mass; self.total_fees -= tx.calculated_fee; @@ -264,31 +256,39 @@ mod tests { tx::{Transaction, TransactionId, TransactionInput, TransactionOutpoint, TransactionOutput}, }; use kaspa_txscript::{pay_to_script_hash_signature_script, test_helpers::op_true_script}; - use std::sync::Arc; + use std::{collections::HashSet, sync::Arc}; use crate::{mempool::config::DEFAULT_MINIMUM_RELAY_TRANSACTION_FEE, model::candidate_tx::CandidateTransaction}; #[test] fn test_reject_transaction() { const TX_INITIAL_COUNT: usize = 1_000; - const REJECT_COUNT: usize = 10; // Create a vector of transactions differing by output value so they have unique ids let transactions = (0..TX_INITIAL_COUNT).map(|i| create_transaction(SOMPI_PER_KASPA * (i + 1) as u64)).collect_vec(); let policy = Policy::new(100_000); let mut selector = TransactionsSelector::new(policy, transactions); - assert_eq!(selector.len(), TX_INITIAL_COUNT, "selector length matches initial transaction vector length"); - - let mut remaining_count = TX_INITIAL_COUNT; - for i in 0..3 { + let (mut kept, mut rejected) = (HashSet::new(), HashSet::new()); + let mut reject_count = 32; + for i in 0..10 { let selected_txs = selector.select_transactions(); - selected_txs.iter().skip((i + 1) * 100).take(REJECT_COUNT).for_each(|x| selector.reject_selection(x.id())); - remaining_count -= REJECT_COUNT; - assert_eq!(selector.len(), remaining_count, "selector length matches remaining transaction count"); - assert_eq!(selector.len(), remaining_count, "selector length matches remaining transaction count"); - let selected_txs_2 = selector.select_transactions(); - assert_eq!(selector.len(), remaining_count, "selector length matches remaining transaction count"); - assert_eq!(selected_txs.len(), selected_txs_2.len()); + if i > 0 { + assert_eq!( + selected_txs.len(), + reject_count, + "subsequent select calls are expected to only refill the previous rejections" + ); + reject_count /= 2; + } + for tx in selected_txs.iter() { + kept.insert(tx.id()).then_some(()).expect("selected txs should never repeat themselves"); + assert!(!rejected.contains(&tx.id()), "selected txs should never repeat themselves"); + } + selected_txs.iter().take(reject_count).for_each(|x| { + selector.reject_selection(x.id()); + kept.remove(&x.id()).then_some(()).expect("was just inserted"); + rejected.insert(x.id()).then_some(()).expect("was just verified"); + }); } } diff --git a/mining/src/manager.rs b/mining/src/manager.rs index 00c1f2872..f7fe03a77 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -85,17 +85,12 @@ impl MiningManager { debug!("Building a new block template..."); let _swo = Stopwatch::<22>::with_threshold("build_block_template full loop"); let mut attempts: u64 = 0; - let transactions = self.block_candidate_transactions(); - let mut block_template_builder = BlockTemplateBuilder::new(self.config.maximum_mass_per_block, transactions); loop { attempts += 1; - // TODO: consider a parameter forcing the consensus to build a template with the remaining successfully validated transactions - // - // let force_build = attempts == self.config.maximum_build_block_template_attempts; - // match block_template_builder.build_block_template(consensus, miner_data, force_build) { - - match block_template_builder.build_block_template(consensus, miner_data) { + let transactions = self.block_candidate_transactions(); + let block_template_builder = BlockTemplateBuilder::new(self.config.maximum_mass_per_block); + match block_template_builder.build_block_template(consensus, miner_data, transactions) { Ok(block_template) => { let block_template = cache_lock.set_immutable_cached_template(block_template); match attempts { @@ -125,21 +120,11 @@ impl MiningManager { return Ok(block_template.as_ref().clone()); } Err(BuilderError::ConsensusError(BlockRuleError::InvalidTransactionsInNewBlock(invalid_transactions))) => { - // Do not refetch candidates if not absolutely necessary so we do not lock the mempool - // and optimize for the quickest possible resolution - let keep_candidates = block_template_builder.candidates_len() - > self.config.ready_transactions_refetch_limit + invalid_transactions.len() - || attempts + 1 >= self.config.maximum_build_block_template_attempts; - let mut missing_outpoint: usize = 0; let mut invalid: usize = 0; let mut mempool_write = self.mempool.write(); invalid_transactions.iter().for_each(|(x, err)| { - if keep_candidates { - block_template_builder.reject_transaction(*x); - } - // On missing outpoints, the most likely is that the tx was already in a block accepted by // the consensus but not yet processed by handle_new_block_transactions(). Another possibility // is a double spend. In both cases, we simply remove the transaction but keep its redeemers. @@ -179,12 +164,6 @@ impl MiningManager { "Building a new block template failed for {} txs missing outpoint and {} invalid txs", missing_outpoint, invalid ); - - // Refetch candidates if asked to - if !keep_candidates { - let transactions = self.block_candidate_transactions(); - block_template_builder.update_transactions(transactions); - } } Err(err) => { warn!("Building a new block template failed: {}", err); @@ -204,8 +183,8 @@ impl MiningManager { } #[cfg(test)] - pub(crate) fn block_template_builder(&self, transactions: Vec) -> BlockTemplateBuilder { - BlockTemplateBuilder::new(self.config.maximum_mass_per_block, transactions) + pub(crate) fn block_template_builder(&self) -> BlockTemplateBuilder { + BlockTemplateBuilder::new(self.config.maximum_mass_per_block) } /// validate_and_insert_transaction validates the given transaction, and diff --git a/mining/src/manager_tests.rs b/mining/src/manager_tests.rs index 0ef9e53b4..ddcc05be1 100644 --- a/mining/src/manager_tests.rs +++ b/mining/src/manager_tests.rs @@ -846,8 +846,8 @@ mod tests { let miner_data_2 = generate_new_coinbase(address_prefix, second_op); // Build a fresh template for coinbase2 as a reference - let mut builder = mining_manager.block_template_builder(transactions); - let result = builder.build_block_template(consensus, &miner_data_2); + let builder = mining_manager.block_template_builder(); + let result = builder.build_block_template(consensus, &miner_data_2, transactions); assert!(result.is_ok(), "build block template failed for miner data 2"); let expected_template = result.unwrap(); diff --git a/mining/src/testutils/consensus_mock.rs b/mining/src/testutils/consensus_mock.rs index 7976bd1f0..c1f5cda4c 100644 --- a/mining/src/testutils/consensus_mock.rs +++ b/mining/src/testutils/consensus_mock.rs @@ -1,7 +1,7 @@ use super::coinbase_mock::CoinbaseManagerMock; use kaspa_consensus_core::{ api::ConsensusApi, - block::{BlockTemplate, MutableBlock}, + block::{BlockTemplate, MutableBlock, TemplateTransactionSelector}, coinbase::MinerData, constants::BLOCK_VERSION, errors::{ @@ -72,7 +72,12 @@ impl ConsensusMock { } impl ConsensusApi for ConsensusMock { - fn build_block_template(&self, miner_data: MinerData, mut txs: Vec) -> Result { + fn build_block_template( + &self, + miner_data: MinerData, + mut tx_selector: Box, + ) -> Result { + let mut txs = tx_selector.select_transactions(); let coinbase_manager = CoinbaseManagerMock::new(); let coinbase = coinbase_manager.expected_coinbase_transaction(miner_data.clone()); txs.insert(0, coinbase.tx); diff --git a/simpa/src/simulator/miner.rs b/simpa/src/simulator/miner.rs index 9bc3aae64..c69a0ca23 100644 --- a/simpa/src/simulator/miner.rs +++ b/simpa/src/simulator/miner.rs @@ -4,7 +4,7 @@ use kaspa_consensus::consensus::Consensus; use kaspa_consensus::model::stores::virtual_state::VirtualStateStoreReader; use kaspa_consensus::params::Params; use kaspa_consensus_core::api::ConsensusApi; -use kaspa_consensus_core::block::Block; +use kaspa_consensus_core::block::{Block, TemplateTransactionSelector}; use kaspa_consensus_core::coinbase::MinerData; use kaspa_consensus_core::sign::sign; use kaspa_consensus_core::subnets::SUBNETWORK_ID_NATIVE; @@ -22,6 +22,26 @@ use std::cmp::max; use std::iter::once; use std::sync::Arc; +struct OnetimeTxSelector { + txs: Option>, +} + +impl OnetimeTxSelector { + fn new(txs: Vec) -> Self { + Self { txs: Some(txs) } + } +} + +impl TemplateTransactionSelector for OnetimeTxSelector { + fn select_transactions(&mut self) -> Vec { + self.txs.take().unwrap() + } + + fn reject_selection(&mut self, _tx_id: kaspa_consensus_core::tx::TransactionId) { + unimplemented!() + } +} + pub struct Miner { // ID pub(super) id: u64, @@ -89,7 +109,7 @@ impl Miner { let session = self.consensus.acquire_session(); let mut block_template = self .consensus - .build_block_template(self.miner_data.clone(), txs) + .build_block_template(self.miner_data.clone(), Box::new(OnetimeTxSelector::new(txs))) .expect("simulation txs are selected in sync with virtual state and are expected to be valid"); drop(session); block_template.block.header.timestamp = timestamp; // Use simulation time rather than real time From d23113471ffb37872cb33d6f48358a3451b2de9b Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Thu, 21 Sep 2023 12:19:37 +0300 Subject: [PATCH 3/8] virtual processor selector retry logic --- .../pipeline/virtual_processor/processor.rs | 32 ++++++++++++++++--- mining/src/mempool/config.rs | 4 +-- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/consensus/src/pipeline/virtual_processor/processor.rs b/consensus/src/pipeline/virtual_processor/processor.rs index 90720f5ac..2979ab60d 100644 --- a/consensus/src/pipeline/virtual_processor/processor.rs +++ b/consensus/src/pipeline/virtual_processor/processor.rs @@ -88,7 +88,7 @@ use rayon::{ use rocksdb::WriteBatch; use std::{ cmp::min, - collections::{BinaryHeap, VecDeque}, + collections::{BinaryHeap, HashMap, VecDeque}, ops::Deref, sync::{atomic::Ordering, Arc}, }; @@ -795,13 +795,37 @@ impl VirtualStateProcessor { // TODO: tests // - let txs = tx_selector.select_transactions(); + // We call for the initial tx batch out of the virtual read lock + let mut txs = tx_selector.select_transactions(); + let virtual_read = self.virtual_stores.read(); let virtual_state = virtual_read.state.get().unwrap(); let virtual_utxo_view = &virtual_read.utxo_set; - // Validate the transactions in virtual's utxo context - self.validate_block_template_transactions(&txs, &virtual_state, virtual_utxo_view)?; + let mut invalid_transactions = HashMap::new(); + for tx in txs.iter() { + if let Err(e) = self.validate_block_template_transaction(tx, &virtual_state, virtual_utxo_view) { + invalid_transactions.insert(tx.id(), e); + tx_selector.reject_selection(tx.id()); + } + } + + if !invalid_transactions.is_empty() { + txs.retain(|tx| !invalid_transactions.contains_key(&tx.id())); + } + + while !invalid_transactions.is_empty() { + invalid_transactions.clear(); + let next_batch = tx_selector.select_transactions(); + for tx in next_batch { + if let Err(e) = self.validate_block_template_transaction(&tx, &virtual_state, virtual_utxo_view) { + invalid_transactions.insert(tx.id(), e); + tx_selector.reject_selection(tx.id()); + } else { + txs.push(tx); + } + } + } // At this point we can safely drop the read lock drop(virtual_read); diff --git a/mining/src/mempool/config.rs b/mining/src/mempool/config.rs index 2e3f68a2a..15b42bbfa 100644 --- a/mining/src/mempool/config.rs +++ b/mining/src/mempool/config.rs @@ -104,8 +104,8 @@ impl Config { Self { maximum_transaction_count: DEFAULT_MAXIMUM_TRANSACTION_COUNT, maximum_ready_transaction_count: DEFAULT_MAXIMUM_READY_TRANSACTION_COUNT, - maximum_build_block_template_attempts: DEFAULT_MAXIMUM_BUILD_BLOCK_TEMPLATE_ATTEMPTS, - ready_transactions_refetch_limit: DEFAULT_READY_TRANSACTIONS_REFETCH_LIMIT, + maximum_build_block_template_attempts: DEFAULT_MAXIMUM_BUILD_BLOCK_TEMPLATE_ATTEMPTS, // TODO + ready_transactions_refetch_limit: DEFAULT_READY_TRANSACTIONS_REFETCH_LIMIT, // TODO transaction_expire_interval_daa_score: DEFAULT_TRANSACTION_EXPIRE_INTERVAL_SECONDS * 1000 / target_milliseconds_per_block, transaction_expire_scan_interval_daa_score: DEFAULT_TRANSACTION_EXPIRE_SCAN_INTERVAL_SECONDS * 1000 / target_milliseconds_per_block, From 9a015d1320d1fa671748c217ae57dc8e211639b3 Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Thu, 21 Sep 2023 21:07:18 +0300 Subject: [PATCH 4/8] make BBT fallible by some selector criteria + comments and some docs --- consensus/core/src/block.rs | 11 +++++++++ consensus/core/src/errors/block.rs | 4 ++-- .../pipeline/virtual_processor/processor.rs | 24 ++++++++++++++----- .../src/pipeline/virtual_processor/tests.rs | 4 ++++ mining/src/block_template/selector.rs | 13 +++++++++- simpa/src/simulator/miner.rs | 4 ++++ 6 files changed, 51 insertions(+), 9 deletions(-) diff --git a/consensus/core/src/block.rs b/consensus/core/src/block.rs index 21c0b531c..74aa42186 100644 --- a/consensus/core/src/block.rs +++ b/consensus/core/src/block.rs @@ -68,9 +68,20 @@ impl Block { } } +/// An abstraction for a recallable transaction selector with persistent state pub trait TemplateTransactionSelector { + /// Expected to return a batch of transactions which were not previously selected. + /// The batch will typically contain sufficient transactions to fill the block + /// mass (along with the previously unrejected txs), or will drain the selector fn select_transactions(&mut self) -> Vec; + + /// Should be used to report invalid transactions obtained from the *most recent* + /// `select_transactions` call. Implementors should use this call to internally + /// track the selection state and discard the rejected tx from internal occupation calculations fn reject_selection(&mut self, tx_id: TransactionId); + + /// Determine whether this was an overall successful selection episode + fn is_successful(&self) -> bool; } /// A block template for miners. diff --git a/consensus/core/src/errors/block.rs b/consensus/core/src/errors/block.rs index e78d0f0f8..bc72136fe 100644 --- a/consensus/core/src/errors/block.rs +++ b/consensus/core/src/errors/block.rs @@ -1,4 +1,4 @@ -use std::fmt::Display; +use std::{collections::HashMap, fmt::Display}; use crate::{ constants, @@ -140,7 +140,7 @@ pub enum RuleError { InvalidTransactionsInUtxoContext(usize, usize), #[error("invalid transactions in new block template")] - InvalidTransactionsInNewBlock(Vec<(TransactionId, TxRuleError)>), + InvalidTransactionsInNewBlock(HashMap), #[error("DAA window data has only {0} entries")] InsufficientDaaWindowSize(usize), diff --git a/consensus/src/pipeline/virtual_processor/processor.rs b/consensus/src/pipeline/virtual_processor/processor.rs index 2979ab60d..2bde3b4c4 100644 --- a/consensus/src/pipeline/virtual_processor/processor.rs +++ b/consensus/src/pipeline/virtual_processor/processor.rs @@ -792,10 +792,13 @@ impl VirtualStateProcessor { miner_data: MinerData, mut tx_selector: Box, ) -> Result { + // // TODO: tests // - // We call for the initial tx batch out of the virtual read lock + // We call for the initial tx batch before acquiring the virtual read lock, + // optimizing for the common case where all txs are valid. Following selection calls + // are called within the lock in order to preserve validness of already validated txs let mut txs = tx_selector.select_transactions(); let virtual_read = self.virtual_stores.read(); @@ -814,19 +817,28 @@ impl VirtualStateProcessor { txs.retain(|tx| !invalid_transactions.contains_key(&tx.id())); } - while !invalid_transactions.is_empty() { - invalid_transactions.clear(); - let next_batch = tx_selector.select_transactions(); + let mut has_rejections = !invalid_transactions.is_empty(); + while has_rejections { + has_rejections = false; + let next_batch = tx_selector.select_transactions(); // Note that once next_batch is empty the loop will exit for tx in next_batch { if let Err(e) = self.validate_block_template_transaction(&tx, &virtual_state, virtual_utxo_view) { invalid_transactions.insert(tx.id(), e); tx_selector.reject_selection(tx.id()); + has_rejections = true; } else { txs.push(tx); } } } + // Check whether this was an overall successful selection episode. We pass this decision + // to the selector implementation which has the broadest picture and can use mempool config + // and context + if !tx_selector.is_successful() { + return Err(RuleError::InvalidTransactionsInNewBlock(invalid_transactions)); + } + // At this point we can safely drop the read lock drop(virtual_read); @@ -842,10 +854,10 @@ impl VirtualStateProcessor { ) -> Result<(), RuleError> { // Search for invalid transactions. This can happen since the mining manager calling this function is not atomically in sync with virtual state // TODO: process transactions in parallel - let mut invalid_transactions = Vec::new(); + let mut invalid_transactions = HashMap::new(); for tx in txs.iter() { if let Err(e) = self.validate_block_template_transaction(tx, virtual_state, utxo_view) { - invalid_transactions.push((tx.id(), e)) + invalid_transactions.insert(tx.id(), e); } } if !invalid_transactions.is_empty() { diff --git a/consensus/src/pipeline/virtual_processor/tests.rs b/consensus/src/pipeline/virtual_processor/tests.rs index 119a57087..e0b43b1b8 100644 --- a/consensus/src/pipeline/virtual_processor/tests.rs +++ b/consensus/src/pipeline/virtual_processor/tests.rs @@ -30,6 +30,10 @@ impl TemplateTransactionSelector for OnetimeTxSelector { fn reject_selection(&mut self, _tx_id: kaspa_consensus_core::tx::TransactionId) { unimplemented!() } + + fn is_successful(&self) -> bool { + true + } } struct TestContext { diff --git a/mining/src/block_template/selector.rs b/mining/src/block_template/selector.rs index 53ec0a332..369d3450d 100644 --- a/mining/src/block_template/selector.rs +++ b/mining/src/block_template/selector.rs @@ -44,6 +44,7 @@ pub(crate) struct TransactionsSelector { // Inner state of the selection process candidate_list: CandidateList, + overall_rejections: usize, used_count: usize, used_p: f64, total_mass: u64, @@ -65,6 +66,7 @@ impl TransactionsSelector { selected_txs: Default::default(), selected_txs_map: None, candidate_list: Default::default(), + overall_rejections: 0, used_count: 0, used_p: 0.0, total_mass: 0, @@ -234,14 +236,23 @@ impl TemplateTransactionSelector for TransactionsSelector { fn reject_selection(&mut self, tx_id: TransactionId) { let selected_txs_map = self .selected_txs_map + // We lazy-create the map only when there are actual rejections .get_or_insert_with(|| self.selected_txs.iter().map(|&x| (self.transactions[x].tx.id(), x)).collect()); - let tx_index = *selected_txs_map.get(&tx_id).expect("only previously selected txs can be rejected"); + let tx_index = selected_txs_map.remove(&tx_id).expect("only previously selected txs can be rejected (and only once)"); let tx = &self.transactions[tx_index]; self.total_mass -= tx.calculated_mass; self.total_fees -= tx.calculated_fee; if !tx.tx.subnetwork_id.is_builtin_or_native() { *self.gas_usage_map.get_mut(&tx.tx.subnetwork_id).expect("previously selected txs have an entry") -= tx.tx.gas; } + self.overall_rejections += 1; + } + + fn is_successful(&self) -> bool { + // TODO: comment + constants + self.transactions.is_empty() + || (self.total_mass as f64) > self.policy.max_block_mass as f64 * 0.8 + || (self.overall_rejections as f64) < self.transactions.len() as f64 * 0.2 } } diff --git a/simpa/src/simulator/miner.rs b/simpa/src/simulator/miner.rs index c69a0ca23..2cdf1af4b 100644 --- a/simpa/src/simulator/miner.rs +++ b/simpa/src/simulator/miner.rs @@ -40,6 +40,10 @@ impl TemplateTransactionSelector for OnetimeTxSelector { fn reject_selection(&mut self, _tx_id: kaspa_consensus_core::tx::TransactionId) { unimplemented!() } + + fn is_successful(&self) -> bool { + true + } } pub struct Miner { From 58e0ed5af4d705eff4bf2e78056dcec24f3b7024 Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Thu, 21 Sep 2023 21:30:13 +0300 Subject: [PATCH 5/8] add an infallible mode to virtual processor `build_block_template()` --- consensus/core/src/api/mod.rs | 3 ++- consensus/core/src/block.rs | 12 ++++++++++++ consensus/src/consensus/mod.rs | 5 +++-- .../src/pipeline/virtual_processor/processor.rs | 8 +++++--- consensus/src/pipeline/virtual_processor/tests.rs | 8 ++++++-- mining/src/block_template/builder.rs | 9 +++++++-- mining/src/block_template/selector.rs | 2 +- mining/src/manager.rs | 9 +++++++-- mining/src/manager_tests.rs | 3 ++- mining/src/testutils/consensus_mock.rs | 3 ++- simpa/src/simulator/miner.rs | 4 ++-- 11 files changed, 49 insertions(+), 17 deletions(-) diff --git a/consensus/core/src/api/mod.rs b/consensus/core/src/api/mod.rs index c11ea8317..880a27172 100644 --- a/consensus/core/src/api/mod.rs +++ b/consensus/core/src/api/mod.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use crate::{ acceptance_data::AcceptanceData, - block::{Block, BlockTemplate, TemplateTransactionSelector}, + block::{Block, BlockTemplate, TemplateBuildMode, TemplateTransactionSelector}, block_count::BlockCount, blockstatus::BlockStatus, coinbase::MinerData, @@ -31,6 +31,7 @@ pub trait ConsensusApi: Send + Sync { &self, miner_data: MinerData, tx_selector: Box, + build_mode: TemplateBuildMode, ) -> Result { unimplemented!() } diff --git a/consensus/core/src/block.rs b/consensus/core/src/block.rs index 74aa42186..77a363196 100644 --- a/consensus/core/src/block.rs +++ b/consensus/core/src/block.rs @@ -84,6 +84,18 @@ pub trait TemplateTransactionSelector { fn is_successful(&self) -> bool; } +/// Block template build mode +#[derive(Clone, Copy, Debug)] +pub enum TemplateBuildMode { + /// Block template build can possibly fail if `TemplateTransactionSelector::is_successful` deems the operation unsuccessful. + /// + /// In such a case, the build fails with `BlockRuleError::InvalidTransactionsInNewBlock`. + Standard, + + /// Block template build always succeeds. The built block contains only the validated transactions. + Infallible, +} + /// A block template for miners. #[derive(Debug, Clone)] pub struct BlockTemplate { diff --git a/consensus/src/consensus/mod.rs b/consensus/src/consensus/mod.rs index 0071ea07f..dd82c09d6 100644 --- a/consensus/src/consensus/mod.rs +++ b/consensus/src/consensus/mod.rs @@ -41,7 +41,7 @@ use crate::{ use kaspa_consensus_core::{ acceptance_data::AcceptanceData, api::{BlockValidationFuture, ConsensusApi}, - block::{Block, BlockTemplate, TemplateTransactionSelector}, + block::{Block, BlockTemplate, TemplateBuildMode, TemplateTransactionSelector}, block_count::BlockCount, blockhash::BlockHashExtensions, blockstatus::BlockStatus, @@ -357,8 +357,9 @@ impl ConsensusApi for Consensus { &self, miner_data: MinerData, tx_selector: Box, + build_mode: TemplateBuildMode, ) -> Result { - self.virtual_processor.build_block_template(miner_data, tx_selector) + self.virtual_processor.build_block_template(miner_data, tx_selector, build_mode) } fn validate_and_insert_block(&self, block: Block) -> BlockValidationFuture { diff --git a/consensus/src/pipeline/virtual_processor/processor.rs b/consensus/src/pipeline/virtual_processor/processor.rs index 2bde3b4c4..47d0cf679 100644 --- a/consensus/src/pipeline/virtual_processor/processor.rs +++ b/consensus/src/pipeline/virtual_processor/processor.rs @@ -48,7 +48,7 @@ use crate::{ }; use kaspa_consensus_core::{ acceptance_data::AcceptanceData, - block::{BlockTemplate, MutableBlock, TemplateTransactionSelector}, + block::{BlockTemplate, MutableBlock, TemplateBuildMode, TemplateTransactionSelector}, blockstatus::BlockStatus::{StatusDisqualifiedFromChain, StatusUTXOValid}, coinbase::MinerData, config::genesis::GenesisBlock, @@ -791,6 +791,7 @@ impl VirtualStateProcessor { &self, miner_data: MinerData, mut tx_selector: Box, + build_mode: TemplateBuildMode, ) -> Result { // // TODO: tests @@ -835,8 +836,9 @@ impl VirtualStateProcessor { // Check whether this was an overall successful selection episode. We pass this decision // to the selector implementation which has the broadest picture and can use mempool config // and context - if !tx_selector.is_successful() { - return Err(RuleError::InvalidTransactionsInNewBlock(invalid_transactions)); + match (build_mode, tx_selector.is_successful()) { + (TemplateBuildMode::Standard, false) => return Err(RuleError::InvalidTransactionsInNewBlock(invalid_transactions)), + (TemplateBuildMode::Standard, true) | (TemplateBuildMode::Infallible, _) => {} } // At this point we can safely drop the read lock diff --git a/consensus/src/pipeline/virtual_processor/tests.rs b/consensus/src/pipeline/virtual_processor/tests.rs index e0b43b1b8..fc6cb73da 100644 --- a/consensus/src/pipeline/virtual_processor/tests.rs +++ b/consensus/src/pipeline/virtual_processor/tests.rs @@ -1,7 +1,7 @@ use crate::{consensus::test_consensus::TestConsensus, model::services::reachability::ReachabilityService}; use kaspa_consensus_core::{ api::ConsensusApi, - block::{Block, BlockTemplate, MutableBlock, TemplateTransactionSelector}, + block::{Block, BlockTemplate, MutableBlock, TemplateBuildMode, TemplateTransactionSelector}, blockhash, blockstatus::BlockStatus, coinbase::MinerData, @@ -104,7 +104,11 @@ impl TestContext { pub fn build_block_template(&self, nonce: u64, timestamp: u64) -> BlockTemplate { let mut t = self .consensus - .build_block_template(self.miner_data.clone(), Box::new(OnetimeTxSelector::new(Default::default()))) + .build_block_template( + self.miner_data.clone(), + Box::new(OnetimeTxSelector::new(Default::default())), + TemplateBuildMode::Standard, + ) .unwrap(); t.block.header.timestamp = timestamp; t.block.header.nonce = nonce; diff --git a/mining/src/block_template/builder.rs b/mining/src/block_template/builder.rs index a3468e8e2..de3428a74 100644 --- a/mining/src/block_template/builder.rs +++ b/mining/src/block_template/builder.rs @@ -1,7 +1,11 @@ use super::{errors::BuilderResult, policy::Policy}; use crate::{block_template::selector::TransactionsSelector, model::candidate_tx::CandidateTransaction}; use kaspa_consensus_core::{ - api::ConsensusApi, block::BlockTemplate, coinbase::MinerData, merkle::calc_hash_merkle_root, tx::COINBASE_TRANSACTION_INDEX, + api::ConsensusApi, + block::{BlockTemplate, TemplateBuildMode}, + coinbase::MinerData, + merkle::calc_hash_merkle_root, + tx::COINBASE_TRANSACTION_INDEX, }; use kaspa_core::{ debug, @@ -86,11 +90,12 @@ impl BlockTemplateBuilder { consensus: &dyn ConsensusApi, miner_data: &MinerData, transactions: Vec, + build_mode: TemplateBuildMode, ) -> BuilderResult { let _sw = Stopwatch::<20>::with_threshold("build_block_template op"); debug!("Considering {} transactions for a new block template", transactions.len()); let selector = Box::new(TransactionsSelector::new(self.policy.clone(), transactions)); - Ok(consensus.build_block_template(miner_data.clone(), selector)?) + Ok(consensus.build_block_template(miner_data.clone(), selector, build_mode)?) } /// modify_block_template clones an existing block template, modifies it to the requested coinbase data and updates the timestamp diff --git a/mining/src/block_template/selector.rs b/mining/src/block_template/selector.rs index 369d3450d..9b34d964c 100644 --- a/mining/src/block_template/selector.rs +++ b/mining/src/block_template/selector.rs @@ -250,7 +250,7 @@ impl TemplateTransactionSelector for TransactionsSelector { fn is_successful(&self) -> bool { // TODO: comment + constants - self.transactions.is_empty() + self.overall_rejections == 0 || (self.total_mass as f64) > self.policy.max_block_mass as f64 * 0.8 || (self.overall_rejections as f64) < self.transactions.len() as f64 * 0.2 } diff --git a/mining/src/manager.rs b/mining/src/manager.rs index f7fe03a77..9a081bff0 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -21,7 +21,7 @@ use crate::{ use itertools::Itertools; use kaspa_consensus_core::{ api::ConsensusApi, - block::BlockTemplate, + block::{BlockTemplate, TemplateBuildMode}, coinbase::MinerData, errors::{block::RuleError as BlockRuleError, tx::TxRuleError}, tx::{MutableTransaction, Transaction, TransactionId, TransactionOutput}, @@ -90,7 +90,12 @@ impl MiningManager { let transactions = self.block_candidate_transactions(); let block_template_builder = BlockTemplateBuilder::new(self.config.maximum_mass_per_block); - match block_template_builder.build_block_template(consensus, miner_data, transactions) { + let build_mode = if attempts < self.config.maximum_build_block_template_attempts { + TemplateBuildMode::Standard + } else { + TemplateBuildMode::Infallible + }; + match block_template_builder.build_block_template(consensus, miner_data, transactions, build_mode) { Ok(block_template) => { let block_template = cache_lock.set_immutable_cached_template(block_template); match attempts { diff --git a/mining/src/manager_tests.rs b/mining/src/manager_tests.rs index ddcc05be1..776ef017f 100644 --- a/mining/src/manager_tests.rs +++ b/mining/src/manager_tests.rs @@ -16,6 +16,7 @@ mod tests { use kaspa_addresses::{Address, Prefix, Version}; use kaspa_consensus_core::{ api::ConsensusApi, + block::TemplateBuildMode, coinbase::MinerData, constants::{MAX_TX_IN_SEQUENCE_NUM, SOMPI_PER_KASPA, TX_VERSION}, errors::tx::{TxResult, TxRuleError}, @@ -847,7 +848,7 @@ mod tests { // Build a fresh template for coinbase2 as a reference let builder = mining_manager.block_template_builder(); - let result = builder.build_block_template(consensus, &miner_data_2, transactions); + let result = builder.build_block_template(consensus, &miner_data_2, transactions, TemplateBuildMode::Standard); assert!(result.is_ok(), "build block template failed for miner data 2"); let expected_template = result.unwrap(); diff --git a/mining/src/testutils/consensus_mock.rs b/mining/src/testutils/consensus_mock.rs index c1f5cda4c..ecf5319e0 100644 --- a/mining/src/testutils/consensus_mock.rs +++ b/mining/src/testutils/consensus_mock.rs @@ -1,7 +1,7 @@ use super::coinbase_mock::CoinbaseManagerMock; use kaspa_consensus_core::{ api::ConsensusApi, - block::{BlockTemplate, MutableBlock, TemplateTransactionSelector}, + block::{BlockTemplate, MutableBlock, TemplateBuildMode, TemplateTransactionSelector}, coinbase::MinerData, constants::BLOCK_VERSION, errors::{ @@ -76,6 +76,7 @@ impl ConsensusApi for ConsensusMock { &self, miner_data: MinerData, mut tx_selector: Box, + _build_mode: TemplateBuildMode, ) -> Result { let mut txs = tx_selector.select_transactions(); let coinbase_manager = CoinbaseManagerMock::new(); diff --git a/simpa/src/simulator/miner.rs b/simpa/src/simulator/miner.rs index 2cdf1af4b..2f144fc66 100644 --- a/simpa/src/simulator/miner.rs +++ b/simpa/src/simulator/miner.rs @@ -4,7 +4,7 @@ use kaspa_consensus::consensus::Consensus; use kaspa_consensus::model::stores::virtual_state::VirtualStateStoreReader; use kaspa_consensus::params::Params; use kaspa_consensus_core::api::ConsensusApi; -use kaspa_consensus_core::block::{Block, TemplateTransactionSelector}; +use kaspa_consensus_core::block::{Block, TemplateBuildMode, TemplateTransactionSelector}; use kaspa_consensus_core::coinbase::MinerData; use kaspa_consensus_core::sign::sign; use kaspa_consensus_core::subnets::SUBNETWORK_ID_NATIVE; @@ -113,7 +113,7 @@ impl Miner { let session = self.consensus.acquire_session(); let mut block_template = self .consensus - .build_block_template(self.miner_data.clone(), Box::new(OnetimeTxSelector::new(txs))) + .build_block_template(self.miner_data.clone(), Box::new(OnetimeTxSelector::new(txs)), TemplateBuildMode::Standard) .expect("simulation txs are selected in sync with virtual state and are expected to be valid"); drop(session); block_template.block.header.timestamp = timestamp; // Use simulation time rather than real time From 827b9fa7d1d8b2f865d946ce99843a1a7442eecd Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Thu, 21 Sep 2023 21:45:30 +0300 Subject: [PATCH 6/8] constants for tx selector successful decision --- mining/src/block_template/selector.rs | 9 ++++++--- mining/src/mempool/config.rs | 8 +------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/mining/src/block_template/selector.rs b/mining/src/block_template/selector.rs index 9b34d964c..a2ab7bad7 100644 --- a/mining/src/block_template/selector.rs +++ b/mining/src/block_template/selector.rs @@ -249,10 +249,13 @@ impl TemplateTransactionSelector for TransactionsSelector { } fn is_successful(&self) -> bool { - // TODO: comment + constants + const SUFFICIENT_MASS_THRESHOLD: f64 = 0.8; + const LOW_REJECTION_FRACTION: f64 = 0.2; + + // We consider the operation successful if either mass occupation is above 80% or rejection rate is below 20% self.overall_rejections == 0 - || (self.total_mass as f64) > self.policy.max_block_mass as f64 * 0.8 - || (self.overall_rejections as f64) < self.transactions.len() as f64 * 0.2 + || (self.total_mass as f64) > self.policy.max_block_mass as f64 * SUFFICIENT_MASS_THRESHOLD + || (self.overall_rejections as f64) < self.transactions.len() as f64 * LOW_REJECTION_FRACTION } } diff --git a/mining/src/mempool/config.rs b/mining/src/mempool/config.rs index 15b42bbfa..8c082d689 100644 --- a/mining/src/mempool/config.rs +++ b/mining/src/mempool/config.rs @@ -3,8 +3,6 @@ use kaspa_consensus_core::constants::TX_VERSION; pub(crate) const DEFAULT_MAXIMUM_TRANSACTION_COUNT: u64 = 1_000_000; pub(crate) const DEFAULT_MAXIMUM_READY_TRANSACTION_COUNT: u64 = 100_000; pub(crate) const DEFAULT_MAXIMUM_BUILD_BLOCK_TEMPLATE_ATTEMPTS: u64 = 5; -// TODO: revisit this value -pub(crate) const DEFAULT_READY_TRANSACTIONS_REFETCH_LIMIT: usize = 2_500; pub(crate) const DEFAULT_TRANSACTION_EXPIRE_INTERVAL_SECONDS: u64 = 60; pub(crate) const DEFAULT_TRANSACTION_EXPIRE_SCAN_INTERVAL_SECONDS: u64 = 10; @@ -34,7 +32,6 @@ pub struct Config { pub maximum_transaction_count: u64, pub maximum_ready_transaction_count: u64, pub maximum_build_block_template_attempts: u64, - pub ready_transactions_refetch_limit: usize, pub transaction_expire_interval_daa_score: u64, pub transaction_expire_scan_interval_daa_score: u64, pub transaction_expire_scan_interval_milliseconds: u64, @@ -58,7 +55,6 @@ impl Config { maximum_transaction_count: u64, maximum_ready_transaction_count: u64, maximum_build_block_template_attempts: u64, - ready_transactions_refetch_limit: usize, transaction_expire_interval_daa_score: u64, transaction_expire_scan_interval_daa_score: u64, transaction_expire_scan_interval_milliseconds: u64, @@ -79,7 +75,6 @@ impl Config { maximum_transaction_count, maximum_ready_transaction_count, maximum_build_block_template_attempts, - ready_transactions_refetch_limit, transaction_expire_interval_daa_score, transaction_expire_scan_interval_daa_score, transaction_expire_scan_interval_milliseconds, @@ -104,8 +99,7 @@ impl Config { Self { maximum_transaction_count: DEFAULT_MAXIMUM_TRANSACTION_COUNT, maximum_ready_transaction_count: DEFAULT_MAXIMUM_READY_TRANSACTION_COUNT, - maximum_build_block_template_attempts: DEFAULT_MAXIMUM_BUILD_BLOCK_TEMPLATE_ATTEMPTS, // TODO - ready_transactions_refetch_limit: DEFAULT_READY_TRANSACTIONS_REFETCH_LIMIT, // TODO + maximum_build_block_template_attempts: DEFAULT_MAXIMUM_BUILD_BLOCK_TEMPLATE_ATTEMPTS, transaction_expire_interval_daa_score: DEFAULT_TRANSACTION_EXPIRE_INTERVAL_SECONDS * 1000 / target_milliseconds_per_block, transaction_expire_scan_interval_daa_score: DEFAULT_TRANSACTION_EXPIRE_SCAN_INTERVAL_SECONDS * 1000 / target_milliseconds_per_block, From 08456c6719fc4834dc9dbca5577ee9bf26dc84fc Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Fri, 22 Sep 2023 01:14:19 +0300 Subject: [PATCH 7/8] avoid realloc --- mining/src/block_template/selector.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mining/src/block_template/selector.rs b/mining/src/block_template/selector.rs index a2ab7bad7..b65126caf 100644 --- a/mining/src/block_template/selector.rs +++ b/mining/src/block_template/selector.rs @@ -206,8 +206,9 @@ impl TransactionsSelector { fn reset_selection(&mut self) { assert_eq!(self.transactions.len(), self.selectable_txs.len()); + self.selected_txs.clear(); // TODO: consider to min with the approximated amount of txs which fit into max block mass - self.selected_txs = Vec::with_capacity(self.transactions.len()); + self.selected_txs.reserve_exact(self.transactions.len()); self.selected_txs_map = None; } From 099497b67793648a96e92d48926adc29755a7247 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Fri, 22 Sep 2023 14:46:14 +0300 Subject: [PATCH 8/8] Address review comments --- consensus/src/pipeline/virtual_processor/processor.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/consensus/src/pipeline/virtual_processor/processor.rs b/consensus/src/pipeline/virtual_processor/processor.rs index 47d0cf679..2718ebd91 100644 --- a/consensus/src/pipeline/virtual_processor/processor.rs +++ b/consensus/src/pipeline/virtual_processor/processor.rs @@ -814,11 +814,11 @@ impl VirtualStateProcessor { } } - if !invalid_transactions.is_empty() { + let mut has_rejections = !invalid_transactions.is_empty(); + if has_rejections { txs.retain(|tx| !invalid_transactions.contains_key(&tx.id())); } - let mut has_rejections = !invalid_transactions.is_empty(); while has_rejections { has_rejections = false; let next_batch = tx_selector.select_transactions(); // Note that once next_batch is empty the loop will exit @@ -848,14 +848,13 @@ impl VirtualStateProcessor { self.build_block_template_from_virtual_state(virtual_state, miner_data, txs) } - pub fn validate_block_template_transactions( + pub(crate) fn validate_block_template_transactions( &self, txs: &[Transaction], virtual_state: &VirtualState, utxo_view: &impl UtxoView, ) -> Result<(), RuleError> { - // Search for invalid transactions. This can happen since the mining manager calling this function is not atomically in sync with virtual state - // TODO: process transactions in parallel + // Search for invalid transactions let mut invalid_transactions = HashMap::new(); for tx in txs.iter() { if let Err(e) = self.validate_block_template_transaction(tx, virtual_state, utxo_view) {