From 621ab30a3e2ba19580666925692205fdbefe0c80 Mon Sep 17 00:00:00 2001 From: KaffinPX Date: Mon, 1 Jul 2024 14:58:31 +0300 Subject: [PATCH 01/30] Replace by fee on mempool with tests --- mining/src/manager.rs | 2 + mining/src/manager_tests.rs | 42 ++++++++++++++++++- .../validate_and_insert_transaction.rs | 23 +++++++++- 3 files changed, 65 insertions(+), 2 deletions(-) diff --git a/mining/src/manager.rs b/mining/src/manager.rs index 574390122..99a118a7e 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -237,6 +237,8 @@ impl MiningManager { let validation_result = validate_mempool_transaction(consensus, &mut transaction); // write lock on mempool let mut mempool = self.mempool.write(); + mempool.try_solve_conflicts(&transaction)?; + if let Some(accepted_transaction) = mempool.post_validate_and_insert_transaction(consensus, validation_result, transaction, priority, orphan)? { diff --git a/mining/src/manager_tests.rs b/mining/src/manager_tests.rs index 530117094..b26502e1a 100644 --- a/mining/src/manager_tests.rs +++ b/mining/src/manager_tests.rs @@ -205,7 +205,7 @@ mod tests { assert!(result.is_ok(), "the mempool should accept a valid transaction when it is able to populate its UTXO entries"); let mut double_spending_transaction = transaction.clone(); - double_spending_transaction.outputs[0].value -= 1; // do some minor change so that txID is different + double_spending_transaction.outputs[0].value += 1; // do some minor change so that txID is different while not increasing fee double_spending_transaction.finalize(); assert_ne!( transaction.id(), @@ -235,6 +235,46 @@ mod tests { } } + // test_replace_by_fee_in_mempool verifies that an attempt to insert a double-spending transaction with a higher fee + // will cause the existing transaction in the mempool to be replaced. + #[test] + fn test_replace_by_fee_in_mempool() { + let consensus = Arc::new(ConsensusMock::new()); + let counters = Arc::new(MiningCounters::default()); + let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); + + let transaction = create_child_and_parent_txs_and_add_parent_to_consensus(&consensus); + assert!( + consensus.can_finance_transaction(&MutableTransaction::from_tx(transaction.clone())), + "the consensus mock should have spendable UTXOs for the newly created transaction {}", + transaction.id() + ); + + let result = + mining_manager.validate_and_insert_transaction(consensus.as_ref(), transaction.clone(), Priority::Low, Orphan::Allowed); + assert!(result.is_ok(), "the mempool should accept a valid transaction when it is able to populate its UTXO entries"); + + let mut double_spending_transaction = transaction.clone(); + double_spending_transaction.outputs[0].value -= 1; // increasing fee of transaction + double_spending_transaction.finalize(); + assert_ne!( + transaction.id(), + double_spending_transaction.id(), + "two transactions differing by only one output value should have different ids" + ); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + double_spending_transaction.clone(), + Priority::Low, + Orphan::Allowed, + ); + assert!(result.is_ok(), "mempool shouldnt refuse a double spend transaction with higher fee but rejected it"); + assert!( + !mining_manager.has_transaction(&transaction.id(), TransactionQuery::All), + "replaced transaction is still on mempool while it shouldnt" + ); + } + // test_handle_new_block_transactions verifies that all the transactions in the block were successfully removed from the mempool. #[test] fn test_handle_new_block_transactions() { diff --git a/mining/src/mempool/validate_and_insert_transaction.rs b/mining/src/mempool/validate_and_insert_transaction.rs index 591fa5c4a..9dba6e92b 100644 --- a/mining/src/mempool/validate_and_insert_transaction.rs +++ b/mining/src/mempool/validate_and_insert_transaction.rs @@ -7,6 +7,7 @@ use crate::mempool::{ tx::{Orphan, Priority}, Mempool, }; +use crate::model::tx_query::TransactionQuery; use kaspa_consensus_core::{ api::ConsensusApi, constants::UNACCEPTED_DAA_SCORE, @@ -25,7 +26,6 @@ impl Mempool { // Populate mass in the beginning, it will be used in multiple places throughout the validation and insertion. transaction.calculated_compute_mass = Some(consensus.calculate_transaction_compute_mass(&transaction.tx)); self.validate_transaction_in_isolation(&transaction)?; - self.transaction_pool.check_double_spends(&transaction)?; self.populate_mempool_entries(&mut transaction); Ok(transaction) } @@ -81,6 +81,27 @@ impl Mempool { Ok(Some(accepted_transaction)) } + pub(crate) fn try_solve_conflicts(&mut self, transaction: &MutableTransaction) -> Result<(), RuleError> { + if let Err(e) = self.transaction_pool.check_double_spends(transaction) { + if let RuleError::RejectDoubleSpendInMempool(_, transaction_id) = e { + let double_spent_tx = self.get_transaction(&transaction_id, TransactionQuery::TransactionsOnly).unwrap(); + + if double_spent_tx.calculated_fee.unwrap() < transaction.calculated_fee.unwrap() { + debug!("replaced by fee"); + self.remove_transaction(&transaction_id, true, TxRemovalReason::Expired, "replaced by fee")?; + } else { + debug!("cant replace by fee, returning error!"); + return Err(e); + } + return Ok(()); + } else { + return Err(e); + } + } + + Ok(()) + } + /// Validates that the transaction wasn't already accepted into the DAG fn validate_transaction_unacceptance(&self, transaction: &MutableTransaction) -> RuleResult<()> { // Reject if the transaction is registered as an accepted transaction From 28c2ecc9c90bd646f8aeed386df1614cd4daeed6 Mon Sep 17 00:00:00 2001 From: KaffinPX Date: Mon, 1 Jul 2024 15:13:22 +0300 Subject: [PATCH 02/30] Add a custom RemovalReason -- ReplacedByFee --- mining/src/mempool/model/tx.rs | 2 ++ mining/src/mempool/validate_and_insert_transaction.rs | 9 ++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/mining/src/mempool/model/tx.rs b/mining/src/mempool/model/tx.rs index 1e549c997..d18161e95 100644 --- a/mining/src/mempool/model/tx.rs +++ b/mining/src/mempool/model/tx.rs @@ -63,6 +63,7 @@ pub(crate) enum TxRemovalReason { DoubleSpend, InvalidInBlockTemplate, RevalidationWithMissingOutpoints, + ReplacedByFee, } impl TxRemovalReason { @@ -76,6 +77,7 @@ impl TxRemovalReason { TxRemovalReason::DoubleSpend => "double spend", TxRemovalReason::InvalidInBlockTemplate => "invalid in block template", TxRemovalReason::RevalidationWithMissingOutpoints => "revalidation with missing outpoints", + TxRemovalReason::ReplacedByFee => "replaced by fee", } } diff --git a/mining/src/mempool/validate_and_insert_transaction.rs b/mining/src/mempool/validate_and_insert_transaction.rs index 9dba6e92b..78e4a72b0 100644 --- a/mining/src/mempool/validate_and_insert_transaction.rs +++ b/mining/src/mempool/validate_and_insert_transaction.rs @@ -87,10 +87,13 @@ impl Mempool { let double_spent_tx = self.get_transaction(&transaction_id, TransactionQuery::TransactionsOnly).unwrap(); if double_spent_tx.calculated_fee.unwrap() < transaction.calculated_fee.unwrap() { - debug!("replaced by fee"); - self.remove_transaction(&transaction_id, true, TxRemovalReason::Expired, "replaced by fee")?; + self.remove_transaction( + &transaction_id, + true, + TxRemovalReason::ReplacedByFee, + format!("by {}", transaction.id()).as_str(), + )?; } else { - debug!("cant replace by fee, returning error!"); return Err(e); } return Ok(()); From 143003927ee91d2edba8a83cfdb5510db4c2f168 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Thu, 4 Jul 2024 03:44:19 +0000 Subject: [PATCH 03/30] Let `MinerManager` handle replace by fee (RBF) for RPC and P2P --- consensus/core/src/tx.rs | 7 + mining/errors/src/mempool.rs | 5 +- mining/src/manager.rs | 130 +++++++++++++----- mining/src/manager_tests.rs | 22 +-- .../mempool/model/accepted_transactions.rs | 1 + mining/src/mempool/model/transactions_pool.rs | 21 +++ mining/src/mempool/model/tx.rs | 26 +++- mining/src/mempool/model/utxo_set.rs | 11 +- .../validate_and_insert_transaction.rs | 97 +++++++++---- mining/src/model/mod.rs | 1 + mining/src/model/tx_insert.rs | 13 ++ protocol/flows/src/flow_context.rs | 23 +++- 12 files changed, 275 insertions(+), 82 deletions(-) create mode 100644 mining/src/model/tx_insert.rs diff --git a/consensus/core/src/tx.rs b/consensus/core/src/tx.rs index c2d3ba2e0..a2008d7f1 100644 --- a/consensus/core/src/tx.rs +++ b/consensus/core/src/tx.rs @@ -406,6 +406,13 @@ impl> MutableTransaction { *entry = None; } } + + pub fn calculated_fee_per_compute_mass(&self) -> Option { + match (self.calculated_fee, self.calculated_compute_mass) { + (Some(fee), Some(compute_mass)) => Some(fee as f64 / compute_mass as f64), + _ => None, + } + } } impl> AsRef for MutableTransaction { diff --git a/mining/errors/src/mempool.rs b/mining/errors/src/mempool.rs index e33737f9d..6f226b623 100644 --- a/mining/errors/src/mempool.rs +++ b/mining/errors/src/mempool.rs @@ -24,9 +24,12 @@ pub enum RuleError { #[error("transaction {0} is already in the mempool")] RejectDuplicate(TransactionId), - #[error("output {0} already spent by transaction {1} in the memory pool")] + #[error("output {0} already spent by transaction {1} in the mempool")] RejectDoubleSpendInMempool(TransactionOutpoint, TransactionId), + #[error("replace by fee found no replaceable transaction in the mempool")] + RejectReplaceByFee, + /// New behavior: a transaction is rejected if the mempool is full #[error("number of high-priority transactions in mempool ({0}) has reached the maximum allowed ({1})")] RejectMempoolIsFull(usize, u64), diff --git a/mining/src/manager.rs b/mining/src/manager.rs index 99a118a7e..85bf93bca 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -4,7 +4,7 @@ use crate::{ errors::MiningManagerResult, mempool::{ config::Config, - model::tx::{MempoolTransaction, TxRemovalReason}, + model::tx::{MempoolTransaction, RbfPolicy, TransactionInsertOutcome, TxRemovalReason}, populate_entries_and_try_validate::{ populate_mempool_transactions_in_parallel, validate_mempool_transaction, validate_mempool_transactions_in_parallel, }, @@ -15,6 +15,7 @@ use crate::{ candidate_tx::CandidateTransaction, owner_txs::{GroupedOwnerTransactions, ScriptPublicKeySet}, topological_sort::IntoIterTopologically, + tx_insert::TransactionInsertion, tx_query::TransactionQuery, }, MempoolCountersSnapshot, MiningCounters, P2pTxCountSample, @@ -212,49 +213,85 @@ impl MiningManager { /// adds it to the set of known transactions that have not yet been /// added to any block. /// - /// The returned transactions are clones of objects owned by the mempool. + /// Replace by fee is strictly forbidden so any double spend returns an error. + /// + /// On success, returns transactions that where unorphaned following the insertion + /// of the provided transaction. + /// + /// The returned transactions are references of objects owned by the mempool. pub fn validate_and_insert_transaction( &self, consensus: &dyn ConsensusApi, transaction: Transaction, priority: Priority, orphan: Orphan, - ) -> MiningManagerResult>> { - self.validate_and_insert_mutable_transaction(consensus, MutableTransaction::from_tx(transaction), priority, orphan) + ) -> MiningManagerResult { + self.validate_and_insert_mutable_transaction( + consensus, + MutableTransaction::from_tx(transaction), + priority, + orphan, + RbfPolicy::Forbidden, + ) } - /// Exposed only for tests. Ordinary users should call `validate_and_insert_transaction` instead - pub fn validate_and_insert_mutable_transaction( + /// validate_and_replace_transaction validates the given transaction, and + /// adds it to the set of known transactions that have not yet been + /// added to any block. + /// + /// Replace by Fee is mandatory, consequently if the provided transaction cannot not replace + /// an existing transaction in the mempool, an error is returned. + /// + /// On success, returns the replaced transaction and the transactions that where unorphaned + /// following the insertion of the provided transaction. + /// + /// The returned transactions are references of objects owned by the mempool. + pub fn validate_and_replace_transaction( + &self, + consensus: &dyn ConsensusApi, + transaction: Transaction, + ) -> MiningManagerResult { + self.validate_and_insert_mutable_transaction( + consensus, + MutableTransaction::from_tx(transaction), + Priority::High, + Orphan::Forbidden, + RbfPolicy::Mandatory, + ) + } + + /// Exposed for tests only + /// + /// Regular users should call `validate_and_insert_transaction` or `validate_and_replace_transaction` instead. + pub(crate) fn validate_and_insert_mutable_transaction( &self, consensus: &dyn ConsensusApi, transaction: MutableTransaction, priority: Priority, orphan: Orphan, - ) -> MiningManagerResult>> { + rbf_policy: RbfPolicy, + ) -> MiningManagerResult { // read lock on mempool - let mut transaction = self.mempool.read().pre_validate_and_populate_transaction(consensus, transaction)?; + let mut transaction = self.mempool.read().pre_validate_and_populate_transaction(consensus, transaction, rbf_policy)?; // no lock on mempool let validation_result = validate_mempool_transaction(consensus, &mut transaction); // write lock on mempool let mut mempool = self.mempool.write(); - mempool.try_solve_conflicts(&transaction)?; - - if let Some(accepted_transaction) = - mempool.post_validate_and_insert_transaction(consensus, validation_result, transaction, priority, orphan)? - { - let unorphaned_transactions = mempool.get_unorphaned_transactions_after_accepted_transaction(&accepted_transaction); - drop(mempool); - - // The capacity used here may be exceeded since accepted unorphaned transaction may themselves unorphan other transactions. - let mut accepted_transactions = Vec::with_capacity(unorphaned_transactions.len() + 1); - // We include the original accepted transaction as well - accepted_transactions.push(accepted_transaction); - accepted_transactions.extend(self.validate_and_insert_unorphaned_transactions(consensus, unorphaned_transactions)); - self.counters.increase_tx_counts(1, priority); - - Ok(accepted_transactions) - } else { - Ok(vec![]) + match mempool.post_validate_and_insert_transaction(consensus, validation_result, transaction, priority, orphan, rbf_policy)? { + TransactionInsertOutcome { removed, accepted: Some(accepted_transaction) } => { + let unorphaned_transactions = mempool.get_unorphaned_transactions_after_accepted_transaction(&accepted_transaction); + drop(mempool); + + // The capacity used here may be exceeded since accepted unorphaned transaction may themselves unorphan other transactions. + let mut accepted_transactions = Vec::with_capacity(unorphaned_transactions.len() + 1); + // We include the original accepted transaction as well + accepted_transactions.push(accepted_transaction); + accepted_transactions.extend(self.validate_and_insert_unorphaned_transactions(consensus, unorphaned_transactions)); + self.counters.increase_tx_counts(1, priority); + + Ok(TransactionInsertion::new(removed, accepted_transactions)) + } + TransactionInsertOutcome { removed, accepted: None } => Ok(TransactionInsertion::new(removed, vec![])), } } @@ -299,13 +336,14 @@ impl MiningManager { transaction, priority, Orphan::Forbidden, + RbfPolicy::Forbidden, ) { - Ok(Some(accepted_transaction)) => { + Ok(TransactionInsertOutcome { removed: _, accepted: Some(accepted_transaction) }) => { accepted_transactions.push(accepted_transaction.clone()); self.counters.increase_tx_counts(1, priority); mempool.get_unorphaned_transactions_after_accepted_transaction(&accepted_transaction) } - Ok(None) => vec![], + Ok(TransactionInsertOutcome { removed: _, accepted: None }) => vec![], Err(err) => { debug!("Failed to unorphan transaction {0} due to rule error: {1}", orphan_id, err); vec![] @@ -322,7 +360,7 @@ impl MiningManager { /// adds those to the set of known transactions that have not yet been added to any block. /// /// Returns transactions that where unorphaned following the insertion of the provided - /// transactions. The returned transactions are clones of objects owned by the mempool. + /// transactions. The returned transactions are references of objects owned by the mempool. pub fn validate_and_insert_transaction_batch( &self, consensus: &dyn ConsensusApi, @@ -346,7 +384,7 @@ impl MiningManager { let mempool = self.mempool.read(); let txs = chunk.filter_map(|tx| { let transaction_id = tx.id(); - match mempool.pre_validate_and_populate_transaction(consensus, tx) { + match mempool.pre_validate_and_populate_transaction(consensus, tx, RbfPolicy::Allowed) { Ok(tx) => Some(tx), Err(RuleError::RejectAlreadyAccepted(transaction_id)) => { debug!("Ignoring already accepted transaction {}", transaction_id); @@ -388,13 +426,21 @@ impl MiningManager { let mut mempool = self.mempool.write(); let txs = chunk.flat_map(|(transaction, validation_result)| { let transaction_id = transaction.id(); - match mempool.post_validate_and_insert_transaction(consensus, validation_result, transaction, priority, orphan) { - Ok(Some(accepted_transaction)) => { + // FIXME: validate allowed RBF + match mempool.post_validate_and_insert_transaction( + consensus, + validation_result, + transaction, + priority, + orphan, + RbfPolicy::Allowed, + ) { + Ok(TransactionInsertOutcome { removed: _, accepted: Some(accepted_transaction) }) => { insert_results.push(Ok(accepted_transaction.clone())); self.counters.increase_tx_counts(1, priority); mempool.get_unorphaned_transactions_after_accepted_transaction(&accepted_transaction) } - Ok(None) => { + Ok(TransactionInsertOutcome { removed: _, accepted: None }) => { // Either orphaned or already existing in the mempool vec![] } @@ -761,22 +807,34 @@ impl MiningManagerProxy { /// Validates a transaction and adds it to the set of known transactions that have not yet been /// added to any block. /// - /// The returned transactions are clones of objects owned by the mempool. + /// The returned transactions are references of objects owned by the mempool. pub async fn validate_and_insert_transaction( self, consensus: &ConsensusProxy, transaction: Transaction, priority: Priority, orphan: Orphan, - ) -> MiningManagerResult>> { + ) -> MiningManagerResult { consensus.clone().spawn_blocking(move |c| self.inner.validate_and_insert_transaction(c, transaction, priority, orphan)).await } + /// Validates a transaction and adds it to the set of known transactions that have not yet been + /// added to any block. + /// + /// The returned transactions are references of objects owned by the mempool. + pub async fn validate_and_replace_transaction( + self, + consensus: &ConsensusProxy, + transaction: Transaction, + ) -> MiningManagerResult { + consensus.clone().spawn_blocking(move |c| self.inner.validate_and_replace_transaction(c, transaction)).await + } + /// Validates a batch of transactions, handling iteratively only the independent ones, and /// adds those to the set of known transactions that have not yet been added to any block. /// /// Returns transactions that where unorphaned following the insertion of the provided - /// transactions. The returned transactions are clones of objects owned by the mempool. + /// transactions. The returned transactions are references of objects owned by the mempool. pub async fn validate_and_insert_transaction_batch( self, consensus: &ConsensusProxy, diff --git a/mining/src/manager_tests.rs b/mining/src/manager_tests.rs index b26502e1a..4ef78ae3b 100644 --- a/mining/src/manager_tests.rs +++ b/mining/src/manager_tests.rs @@ -7,6 +7,7 @@ mod tests { mempool::{ config::{Config, DEFAULT_MINIMUM_RELAY_TRANSACTION_FEE}, errors::RuleError, + model::tx::RbfPolicy, tx::{Orphan, Priority}, }, model::{candidate_tx::CandidateTransaction, tx_query::TransactionQuery}, @@ -52,6 +53,7 @@ mod tests { transaction.clone(), Priority::Low, Orphan::Allowed, + RbfPolicy::Forbidden, ); assert!(result.is_ok(), "inserting a valid transaction failed"); } @@ -158,6 +160,7 @@ mod tests { transaction.clone(), Priority::Low, Orphan::Allowed, + RbfPolicy::Forbidden, ); assert!(result.is_ok(), "mempool should have accepted a valid transaction but did not"); @@ -262,16 +265,13 @@ mod tests { double_spending_transaction.id(), "two transactions differing by only one output value should have different ids" ); - let result = mining_manager.validate_and_insert_transaction( - consensus.as_ref(), - double_spending_transaction.clone(), - Priority::Low, - Orphan::Allowed, - ); - assert!(result.is_ok(), "mempool shouldnt refuse a double spend transaction with higher fee but rejected it"); + let result = mining_manager.validate_and_replace_transaction(consensus.as_ref(), double_spending_transaction.clone()); + assert!(result.is_ok(), "mempool should accept a RBF transaction with higher fee but rejected it"); + let tx_insertion = result.unwrap(); + assert_eq!(tx_insertion.removed.as_ref().unwrap().id(), transaction.id(), "RBF should return the replaced transaction"); assert!( !mining_manager.has_transaction(&transaction.id(), TransactionQuery::All), - "replaced transaction is still on mempool while it shouldnt" + "replaced transaction is in the mempool while it should have been removed by RBF" ); } @@ -528,7 +528,7 @@ mod tests { let result = mining_manager.validate_and_insert_transaction(consensus.as_ref(), parent_txs[0].clone(), Priority::Low, Orphan::Allowed); assert!(result.is_ok(), "the insertion of the remaining parent transaction in the mempool failed"); - let unorphaned_txs = result.unwrap(); + let unorphaned_txs = result.unwrap().accepted; let (populated_txs, orphans) = mining_manager.get_all_transactions(TransactionQuery::All); assert_eq!( unorphaned_txs.len(), SKIPPED_TXS + 1, @@ -663,7 +663,7 @@ mod tests { test.insert_result() ); if let Ok(unorphaned_txs) = result { - assert!(unorphaned_txs.is_empty(), "mempool should unorphan no transaction since it only contains orphans"); + assert!(unorphaned_txs.accepted.is_empty(), "mempool should unorphan no transaction since it only contains orphans"); } else if let Err(MiningManagerError::MempoolError(RuleError::RejectOrphanPoolIsFull(pool_len, config_len))) = result { assert_eq!( (config.maximum_orphan_transaction_count as usize, config.maximum_orphan_transaction_count), @@ -685,7 +685,7 @@ mod tests { let result = mining_manager.validate_and_insert_transaction(consensus.as_ref(), tx.clone(), test.priority, Orphan::Allowed); assert!(result.is_ok(), "mempool should accept a valid transaction with {:?} when asked to do so", test.priority,); - let unorphaned_txs = result.as_ref().unwrap(); + let unorphaned_txs = &result.as_ref().unwrap().accepted; assert_eq!( test.should_unorphan, unorphaned_txs.len() > 1, diff --git a/mining/src/mempool/model/accepted_transactions.rs b/mining/src/mempool/model/accepted_transactions.rs index 94ad0d076..26dc0246b 100644 --- a/mining/src/mempool/model/accepted_transactions.rs +++ b/mining/src/mempool/model/accepted_transactions.rs @@ -51,6 +51,7 @@ impl AcceptedTransactions { let expired_transactions: Vec = self .transactions + // FIXME: use .retain() instead .iter() .filter_map(|(transaction_id, daa_score)| { if virtual_daa_score > daa_score + self.config.accepted_transaction_expire_interval_daa_score { diff --git a/mining/src/mempool/model/transactions_pool.rs b/mining/src/mempool/model/transactions_pool.rs index cf70150df..149b8249f 100644 --- a/mining/src/mempool/model/transactions_pool.rs +++ b/mining/src/mempool/model/transactions_pool.rs @@ -182,6 +182,7 @@ impl TransactionsPool { } false } + /// Returns the exceeding low-priority transactions having the lowest fee rates in order /// to have room for at least `free_slots` new transactions. The returned transactions /// are guaranteed to be unchained (no successor in mempool) and to not be parent of @@ -249,6 +250,26 @@ impl TransactionsPool { self.utxo_set.check_double_spends(transaction) } + pub(crate) fn get_first_double_spend<'a>( + &'a self, + transaction: &MutableTransaction, + ) -> RuleResult> { + let double_spend = self.utxo_set.get_first_double_spend(transaction); + match double_spend { + None => Ok(None), + Some((outpoint, transaction_id)) => { + match self.get(&transaction_id) { + Some(transaction) => Ok(Some((outpoint, transaction))), + None => { + // If a double spent transaction id is found but the matching transaction cannot be located in the mempool + // a replacement is no longer possible and a double spend error is returned + Err(RuleError::RejectDoubleSpendInMempool(outpoint, transaction_id)) + } + } + } + } + } + pub(crate) fn collect_expired_low_priority_transactions(&mut self, virtual_daa_score: u64) -> Vec { let now = unix_now(); if virtual_daa_score < self.last_expire_scan_daa_score + self.config.transaction_expire_scan_interval_daa_score diff --git a/mining/src/mempool/model/tx.rs b/mining/src/mempool/model/tx.rs index d18161e95..b584fce5d 100644 --- a/mining/src/mempool/model/tx.rs +++ b/mining/src/mempool/model/tx.rs @@ -1,8 +1,9 @@ use crate::mempool::tx::Priority; -use kaspa_consensus_core::{tx::MutableTransaction, tx::TransactionId}; +use kaspa_consensus_core::tx::{MutableTransaction, Transaction, TransactionId}; use std::{ cmp::Ordering, fmt::{Display, Formatter}, + sync::Arc, }; pub(crate) struct MempoolTransaction { @@ -53,6 +54,29 @@ impl PartialEq for MempoolTransaction { } } +/// Replace by Fee policy +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum RbfPolicy { + /// RBF is forbidden, fails on double spend + Forbidden, + /// RBF may occur, fails on double spend when not all RBF conditions are met + Allowed, + /// RBF must occur, fails if no transaction can be replaced + Mandatory, +} + +#[derive(Default)] +pub(crate) struct TransactionInsertOutcome { + pub removed: Option>, + pub accepted: Option>, +} + +impl TransactionInsertOutcome { + pub(crate) fn new(removed: Option>, accepted: Option>) -> Self { + Self { removed, accepted } + } +} + #[derive(PartialEq, Eq)] pub(crate) enum TxRemovalReason { Muted, diff --git a/mining/src/mempool/model/utxo_set.rs b/mining/src/mempool/model/utxo_set.rs index 38c2bcb4e..f0ae78b22 100644 --- a/mining/src/mempool/model/utxo_set.rs +++ b/mining/src/mempool/model/utxo_set.rs @@ -70,14 +70,21 @@ impl MempoolUtxoSet { /// Make sure no other transaction in the mempool is already spending an output which one of this transaction inputs spends pub(crate) fn check_double_spends(&self, transaction: &MutableTransaction) -> RuleResult<()> { + match self.get_first_double_spend(transaction) { + Some((outpoint, transaction_id)) => Err(RuleError::RejectDoubleSpendInMempool(outpoint, transaction_id)), + None => Ok(()), + } + } + + pub(crate) fn get_first_double_spend(&self, transaction: &MutableTransaction) -> Option<(TransactionOutpoint, TransactionId)> { let transaction_id = transaction.id(); for input in transaction.tx.inputs.iter() { if let Some(existing_transaction_id) = self.get_outpoint_owner_id(&input.previous_outpoint) { if *existing_transaction_id != transaction_id { - return Err(RuleError::RejectDoubleSpendInMempool(input.previous_outpoint, *existing_transaction_id)); + return Some((input.previous_outpoint, *existing_transaction_id)); } } } - Ok(()) + None } } diff --git a/mining/src/mempool/validate_and_insert_transaction.rs b/mining/src/mempool/validate_and_insert_transaction.rs index 78e4a72b0..0a3c11eb6 100644 --- a/mining/src/mempool/validate_and_insert_transaction.rs +++ b/mining/src/mempool/validate_and_insert_transaction.rs @@ -2,12 +2,11 @@ use crate::mempool::{ errors::{RuleError, RuleResult}, model::{ pool::Pool, - tx::{MempoolTransaction, TxRemovalReason}, + tx::{MempoolTransaction, RbfPolicy, TransactionInsertOutcome, TxRemovalReason}, }, tx::{Orphan, Priority}, Mempool, }; -use crate::model::tx_query::TransactionQuery; use kaspa_consensus_core::{ api::ConsensusApi, constants::UNACCEPTED_DAA_SCORE, @@ -21,11 +20,18 @@ impl Mempool { &self, consensus: &dyn ConsensusApi, mut transaction: MutableTransaction, + rbf: RbfPolicy, ) -> RuleResult { self.validate_transaction_unacceptance(&transaction)?; // Populate mass in the beginning, it will be used in multiple places throughout the validation and insertion. transaction.calculated_compute_mass = Some(consensus.calculate_transaction_compute_mass(&transaction.tx)); self.validate_transaction_in_isolation(&transaction)?; + + // When replace by fee is forbidden, fail early on double spend + if rbf == RbfPolicy::Forbidden { + self.transaction_pool.check_double_spends(&transaction)?; + } + self.populate_mempool_entries(&mut transaction); Ok(transaction) } @@ -37,7 +43,8 @@ impl Mempool { transaction: MutableTransaction, priority: Priority, orphan: Orphan, - ) -> RuleResult>> { + rbf_policy: RbfPolicy, + ) -> RuleResult { let transaction_id = transaction.id(); // First check if the transaction was not already added to the mempool. @@ -46,28 +53,27 @@ impl Mempool { // concurrently. if self.transaction_pool.has(&transaction_id) { debug!("Transaction {0} is not post validated since already in the mempool", transaction_id); - return Ok(None); + return Ok(TransactionInsertOutcome::default()); } self.validate_transaction_unacceptance(&transaction)?; - // Re-check double spends since validate_and_insert_transaction is no longer atomic - self.transaction_pool.check_double_spends(&transaction)?; - match validation_result { Ok(_) => {} Err(RuleError::RejectMissingOutpoint) => { if orphan == Orphan::Forbidden { return Err(RuleError::RejectDisallowedOrphan(transaction_id)); } + self.transaction_pool.check_double_spends(&transaction)?; self.orphan_pool.try_add_orphan(consensus.get_virtual_daa_score(), transaction, priority)?; - return Ok(None); + return Ok(TransactionInsertOutcome::default()); } Err(err) => { return Err(err); } } + let removed_transaction = self.validate_replace_by_fee(&transaction, rbf_policy)?; self.validate_transaction_in_context(&transaction)?; // Before adding the transaction, check if there is room in the pool @@ -78,31 +84,64 @@ impl Mempool { // Add the transaction to the mempool as a MempoolTransaction and return a clone of the embedded Arc let accepted_transaction = self.transaction_pool.add_transaction(transaction, consensus.get_virtual_daa_score(), priority)?.mtx.tx.clone(); - Ok(Some(accepted_transaction)) + Ok(TransactionInsertOutcome::new(removed_transaction, Some(accepted_transaction))) } - pub(crate) fn try_solve_conflicts(&mut self, transaction: &MutableTransaction) -> Result<(), RuleError> { - if let Err(e) = self.transaction_pool.check_double_spends(transaction) { - if let RuleError::RejectDoubleSpendInMempool(_, transaction_id) = e { - let double_spent_tx = self.get_transaction(&transaction_id, TransactionQuery::TransactionsOnly).unwrap(); - - if double_spent_tx.calculated_fee.unwrap() < transaction.calculated_fee.unwrap() { - self.remove_transaction( - &transaction_id, - true, - TxRemovalReason::ReplacedByFee, - format!("by {}", transaction.id()).as_str(), - )?; - } else { - return Err(e); + /// Validates replace by fee (RBF) for a transaction and a policy. + /// + /// The RBF target is the transaction of the first double spend found in the mempool iterating the inputs of `transaction` + /// in order. + /// + /// The target is replaceable if it has a lower fee/mass ratio than `transaction`. + /// + /// If the policy allows or requires a replacement and a replaceable target is found, the target and all its direct + /// and indirect redeemers (chained transactions) are removed. + /// + /// The validation fails if: + /// - a target is found and RBF is forbidden + /// - a target is found but is not replaceable + /// - another double spend is found after having removed the replaceable target + /// - no replaceable target is found and RBF is mandatory + /// - a double spend transaction id is found but the matching transaction is not in the mempool (edge case) + /// + /// On success, `transaction` is guaranteed to have no double spend. + /// + /// On mandatory RBF success, a removed transaction is always provided. + fn validate_replace_by_fee( + &mut self, + transaction: &MutableTransaction, + rbf_policy: RbfPolicy, + ) -> RuleResult>> { + match self.transaction_pool.get_first_double_spend(transaction)? { + Some((outpoint, conflicting_tx)) => match rbf_policy { + RbfPolicy::Forbidden => Err(RuleError::RejectDoubleSpendInMempool(outpoint, conflicting_tx.id())), + RbfPolicy::Allowed | RbfPolicy::Mandatory => { + if transaction.calculated_fee_per_compute_mass().unwrap() + > conflicting_tx.mtx.calculated_fee_per_compute_mass().unwrap() + { + let conflicting_tx = conflicting_tx.mtx.tx.clone(); + self.remove_transaction( + &conflicting_tx.id(), + true, + TxRemovalReason::ReplacedByFee, + format!("by {}", transaction.id()).as_str(), + )?; + + // Re-check for double spends since `transaction` may have additional double spent inputs + // not included in the removed transactions + self.transaction_pool.check_double_spends(transaction)?; + + Ok(Some(conflicting_tx)) + } else { + Err(RuleError::RejectDoubleSpendInMempool(outpoint, conflicting_tx.id())) + } } - return Ok(()); - } else { - return Err(e); - } + }, + None => match rbf_policy { + RbfPolicy::Forbidden | RbfPolicy::Allowed => Ok(None), + RbfPolicy::Mandatory => Err(RuleError::RejectReplaceByFee), + }, } - - Ok(()) } /// Validates that the transaction wasn't already accepted into the DAG diff --git a/mining/src/model/mod.rs b/mining/src/model/mod.rs index 3f17a50c8..66c91cae5 100644 --- a/mining/src/model/mod.rs +++ b/mining/src/model/mod.rs @@ -5,6 +5,7 @@ pub(crate) mod candidate_tx; pub mod owner_txs; pub mod topological_index; pub mod topological_sort; +pub mod tx_insert; pub mod tx_query; /// A set of unique transaction ids diff --git a/mining/src/model/tx_insert.rs b/mining/src/model/tx_insert.rs new file mode 100644 index 000000000..41c2062eb --- /dev/null +++ b/mining/src/model/tx_insert.rs @@ -0,0 +1,13 @@ +use kaspa_consensus_core::tx::Transaction; +use std::sync::Arc; + +pub struct TransactionInsertion { + pub removed: Option>, + pub accepted: Vec>, +} + +impl TransactionInsertion { + pub fn new(removed: Option>, accepted: Vec>) -> Self { + Self { removed, accepted } + } +} diff --git a/protocol/flows/src/flow_context.rs b/protocol/flows/src/flow_context.rs index 3d365c54f..c8a58b8d8 100644 --- a/protocol/flows/src/flow_context.rs +++ b/protocol/flows/src/flow_context.rs @@ -618,16 +618,35 @@ impl FlowContext { transaction: Transaction, orphan: Orphan, ) -> Result<(), ProtocolError> { - let accepted_transactions = + let transaction_insertion = self.mining_manager().clone().validate_and_insert_transaction(consensus, transaction, Priority::High, orphan).await?; self.broadcast_transactions( - accepted_transactions.iter().map(|x| x.id()), + transaction_insertion.accepted.iter().map(|x| x.id()), false, // RPC transactions are considered high priority, so we don't want to throttle them ) .await; Ok(()) } + /// Replaces the rpc-submitted transaction to the mempool and propagates it to peers. + /// + /// Transactions submitted through rpc are considered high priority. This definition does not affect the tx selection algorithm + /// but only changes how we manage the lifetime of the tx. A high-priority tx does not expire and is repeatedly rebroadcasted to + /// peers + pub async fn submit_rpc_transaction_replacement( + &self, + consensus: &ConsensusProxy, + transaction: Transaction, + ) -> Result, ProtocolError> { + let transaction_insertion = self.mining_manager().clone().validate_and_replace_transaction(consensus, transaction).await?; + self.broadcast_transactions( + transaction_insertion.accepted.iter().map(|x| x.id()), + false, // RPC transactions are considered high priority, so we don't want to throttle them + ) + .await; + Ok(transaction_insertion.removed.expect("on RBF success, a removed transaction is returned")) + } + /// Returns true if the time has come for running the task cleaning mempool transactions. async fn should_run_mempool_scanning_task(&self) -> bool { self.transactions_spread.write().await.should_run_mempool_scanning_task() From 7ef25bee8289bb6ba1895fed145f3ed2e19262b2 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Thu, 4 Jul 2024 19:38:47 +0000 Subject: [PATCH 04/30] Refines success conditions and process of RBF policies --- mining/errors/src/mempool.rs | 5 +- mining/src/mempool/model/transactions_pool.rs | 31 +++--- mining/src/mempool/model/tx.rs | 71 +++++++++++++- mining/src/mempool/model/utxo_set.rs | 27 +++++- .../validate_and_insert_transaction.rs | 97 +++++++++++-------- protocol/flows/src/flow_context.rs | 4 +- 6 files changed, 165 insertions(+), 70 deletions(-) diff --git a/mining/errors/src/mempool.rs b/mining/errors/src/mempool.rs index 6f226b623..468958811 100644 --- a/mining/errors/src/mempool.rs +++ b/mining/errors/src/mempool.rs @@ -28,7 +28,10 @@ pub enum RuleError { RejectDoubleSpendInMempool(TransactionOutpoint, TransactionId), #[error("replace by fee found no replaceable transaction in the mempool")] - RejectReplaceByFee, + RejectRbfNoDoubleSpend, + + #[error("replace by fee found more than one replaceable transaction in the mempool")] + RejectRbfTooManyDoubleSpends, /// New behavior: a transaction is rejected if the mempool is full #[error("number of high-priority transactions in mempool ({0}) has reached the maximum allowed ({1})")] diff --git a/mining/src/mempool/model/transactions_pool.rs b/mining/src/mempool/model/transactions_pool.rs index 149b8249f..9523537c7 100644 --- a/mining/src/mempool/model/transactions_pool.rs +++ b/mining/src/mempool/model/transactions_pool.rs @@ -5,7 +5,7 @@ use crate::{ model::{ map::MempoolTransactionCollection, pool::{Pool, TransactionsEdges}, - tx::MempoolTransaction, + tx::{DoubleSpend, MempoolTransaction}, utxo_set::MempoolUtxoSet, }, tx::Priority, @@ -250,22 +250,19 @@ impl TransactionsPool { self.utxo_set.check_double_spends(transaction) } - pub(crate) fn get_first_double_spend<'a>( - &'a self, - transaction: &MutableTransaction, - ) -> RuleResult> { - let double_spend = self.utxo_set.get_first_double_spend(transaction); - match double_spend { - None => Ok(None), - Some((outpoint, transaction_id)) => { - match self.get(&transaction_id) { - Some(transaction) => Ok(Some((outpoint, transaction))), - None => { - // If a double spent transaction id is found but the matching transaction cannot be located in the mempool - // a replacement is no longer possible and a double spend error is returned - Err(RuleError::RejectDoubleSpendInMempool(outpoint, transaction_id)) - } - } + pub(crate) fn get_double_spends(&self, transaction: &MutableTransaction) -> Vec { + self.utxo_set.get_double_spends(transaction) + } + + pub(crate) fn get_double_spend_owner<'a>(&'a self, double_spend: &DoubleSpend) -> RuleResult<&'a MempoolTransaction> { + match self.get(&double_spend.owner_id) { + Some(transaction) => Ok(transaction), + None => { + // This case should never arise in the first place. + // Anyway, in case it does, if a double spent transaction id is found but the matching + // transaction cannot be located in the mempool a replacement is no longer possible + // so a double spend error is returned. + Err(double_spend.into()) } } } diff --git a/mining/src/mempool/model/tx.rs b/mining/src/mempool/model/tx.rs index b584fce5d..8b57fbd14 100644 --- a/mining/src/mempool/model/tx.rs +++ b/mining/src/mempool/model/tx.rs @@ -1,5 +1,6 @@ use crate::mempool::tx::Priority; -use kaspa_consensus_core::tx::{MutableTransaction, Transaction, TransactionId}; +use kaspa_consensus_core::tx::{MutableTransaction, Transaction, TransactionId, TransactionOutpoint}; +use kaspa_mining_errors::mempool::RuleError; use std::{ cmp::Ordering, fmt::{Display, Formatter}, @@ -54,17 +55,77 @@ impl PartialEq for MempoolTransaction { } } -/// Replace by Fee policy +/// Replace by Fee (RBF) policy #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum RbfPolicy { - /// RBF is forbidden, fails on double spend + /// ### RBF is forbidden + /// + /// Inserts the incoming transaction. + /// + /// Conditions of success: + /// + /// - no double spend + /// + /// If conditions are not met, leaves the mempool unchanged and fails with a double spend error. Forbidden, - /// RBF may occur, fails on double spend when not all RBF conditions are met + + /// ### RBF may occur + /// + /// Identifies double spends in mempool and their owning transactions checking in order every input of the incoming + /// transaction. + /// + /// Removes all mempool transactions owning double spends and inserts the incoming transaction. + /// + /// Conditions of success: + /// + /// - on absence of double spends, always succeeds + /// - on double spends, the incoming transaction has a higher fee/mass ratio than the mempool transaction owning + /// the first double spend + /// + /// If conditions are not met, leaves the mempool unchanged and fails with a double spend error. Allowed, - /// RBF must occur, fails if no transaction can be replaced + + /// ### RBF must occur + /// + /// Identifies double spends in mempool and their owning transactions checking in order every input of the incoming + /// transaction. + /// + /// Removes the mempool transaction owning the double spends and inserts the incoming transaction. + /// + /// Conditions of success: + /// + /// - at least one double spend + /// - all double spends belong to the same mempool transaction + /// - the incoming transaction has a higher fee/mass ratio than the mempool transaction owning + /// the double spends + /// + /// If conditions are not met, leaves the mempool unchanged and fails with a double spend error. Mandatory, } +pub(crate) struct DoubleSpend { + pub outpoint: TransactionOutpoint, + pub owner_id: TransactionId, +} + +impl DoubleSpend { + pub fn new(outpoint: TransactionOutpoint, owner_id: TransactionId) -> Self { + Self { outpoint, owner_id } + } +} + +impl From for RuleError { + fn from(value: DoubleSpend) -> Self { + RuleError::RejectDoubleSpendInMempool(value.outpoint, value.owner_id) + } +} + +impl From<&DoubleSpend> for RuleError { + fn from(value: &DoubleSpend) -> Self { + RuleError::RejectDoubleSpendInMempool(value.outpoint, value.owner_id) + } +} + #[derive(Default)] pub(crate) struct TransactionInsertOutcome { pub removed: Option>, diff --git a/mining/src/mempool/model/utxo_set.rs b/mining/src/mempool/model/utxo_set.rs index f0ae78b22..dd75fdb97 100644 --- a/mining/src/mempool/model/utxo_set.rs +++ b/mining/src/mempool/model/utxo_set.rs @@ -1,7 +1,9 @@ +use std::collections::HashSet; + use crate::{ mempool::{ - errors::{RuleError, RuleResult}, - model::map::OutpointIndex, + errors::RuleResult, + model::{map::OutpointIndex, tx::DoubleSpend}, }, model::TransactionIdSet, }; @@ -71,20 +73,35 @@ impl MempoolUtxoSet { /// Make sure no other transaction in the mempool is already spending an output which one of this transaction inputs spends pub(crate) fn check_double_spends(&self, transaction: &MutableTransaction) -> RuleResult<()> { match self.get_first_double_spend(transaction) { - Some((outpoint, transaction_id)) => Err(RuleError::RejectDoubleSpendInMempool(outpoint, transaction_id)), + Some(double_spend) => Err(double_spend.into()), None => Ok(()), } } - pub(crate) fn get_first_double_spend(&self, transaction: &MutableTransaction) -> Option<(TransactionOutpoint, TransactionId)> { + pub(crate) fn get_first_double_spend(&self, transaction: &MutableTransaction) -> Option { let transaction_id = transaction.id(); for input in transaction.tx.inputs.iter() { if let Some(existing_transaction_id) = self.get_outpoint_owner_id(&input.previous_outpoint) { if *existing_transaction_id != transaction_id { - return Some((input.previous_outpoint, *existing_transaction_id)); + return Some(DoubleSpend::new(input.previous_outpoint, *existing_transaction_id)); } } } None } + + pub(crate) fn get_double_spends(&self, transaction: &MutableTransaction) -> Vec { + let transaction_id = transaction.id(); + let mut double_spends = vec![]; + let mut visited = HashSet::new(); + for input in transaction.tx.inputs.iter() { + if let Some(existing_transaction_id) = self.get_outpoint_owner_id(&input.previous_outpoint) { + if *existing_transaction_id != transaction_id && !visited.contains(existing_transaction_id) { + visited.insert(*existing_transaction_id); + double_spends.push(DoubleSpend::new(input.previous_outpoint, *existing_transaction_id)); + } + } + } + double_spends + } } diff --git a/mining/src/mempool/validate_and_insert_transaction.rs b/mining/src/mempool/validate_and_insert_transaction.rs index 0a3c11eb6..fb097149c 100644 --- a/mining/src/mempool/validate_and_insert_transaction.rs +++ b/mining/src/mempool/validate_and_insert_transaction.rs @@ -2,7 +2,7 @@ use crate::mempool::{ errors::{RuleError, RuleResult}, model::{ pool::Pool, - tx::{MempoolTransaction, RbfPolicy, TransactionInsertOutcome, TxRemovalReason}, + tx::{DoubleSpend, MempoolTransaction, RbfPolicy, TransactionInsertOutcome, TxRemovalReason}, }, tx::{Orphan, Priority}, Mempool, @@ -20,7 +20,7 @@ impl Mempool { &self, consensus: &dyn ConsensusApi, mut transaction: MutableTransaction, - rbf: RbfPolicy, + rbf_policy: RbfPolicy, ) -> RuleResult { self.validate_transaction_unacceptance(&transaction)?; // Populate mass in the beginning, it will be used in multiple places throughout the validation and insertion. @@ -28,7 +28,7 @@ impl Mempool { self.validate_transaction_in_isolation(&transaction)?; // When replace by fee is forbidden, fail early on double spend - if rbf == RbfPolicy::Forbidden { + if rbf_policy == RbfPolicy::Forbidden { self.transaction_pool.check_double_spends(&transaction)?; } @@ -73,7 +73,9 @@ impl Mempool { } } + // Check double spends and remove them if the RBF policy requires to let removed_transaction = self.validate_replace_by_fee(&transaction, rbf_policy)?; + self.validate_transaction_in_context(&transaction)?; // Before adding the transaction, check if there is room in the pool @@ -87,60 +89,73 @@ impl Mempool { Ok(TransactionInsertOutcome::new(removed_transaction, Some(accepted_transaction))) } - /// Validates replace by fee (RBF) for a transaction and a policy. - /// - /// The RBF target is the transaction of the first double spend found in the mempool iterating the inputs of `transaction` - /// in order. - /// - /// The target is replaceable if it has a lower fee/mass ratio than `transaction`. - /// - /// If the policy allows or requires a replacement and a replaceable target is found, the target and all its direct - /// and indirect redeemers (chained transactions) are removed. + /// Validates replace by fee (RBF) for an incoming transaction and a policy. /// - /// The validation fails if: - /// - a target is found and RBF is forbidden - /// - a target is found but is not replaceable - /// - another double spend is found after having removed the replaceable target - /// - no replaceable target is found and RBF is mandatory - /// - a double spend transaction id is found but the matching transaction is not in the mempool (edge case) + /// See [`RbfPolicy`] variants for details of each policy process and success conditions. /// /// On success, `transaction` is guaranteed to have no double spend. /// - /// On mandatory RBF success, a removed transaction is always provided. + /// On mandatory RBF success, some removed transaction is always returned. fn validate_replace_by_fee( &mut self, transaction: &MutableTransaction, rbf_policy: RbfPolicy, ) -> RuleResult>> { - match self.transaction_pool.get_first_double_spend(transaction)? { - Some((outpoint, conflicting_tx)) => match rbf_policy { - RbfPolicy::Forbidden => Err(RuleError::RejectDoubleSpendInMempool(outpoint, conflicting_tx.id())), - RbfPolicy::Allowed | RbfPolicy::Mandatory => { - if transaction.calculated_fee_per_compute_mass().unwrap() - > conflicting_tx.mtx.calculated_fee_per_compute_mass().unwrap() - { - let conflicting_tx = conflicting_tx.mtx.tx.clone(); + match rbf_policy { + RbfPolicy::Forbidden => { + self.transaction_pool.check_double_spends(transaction)?; + Ok(None) + } + + RbfPolicy::Allowed => { + let double_spends = self.transaction_pool.get_double_spends(transaction); + match double_spends.is_empty() { + true => Ok(None), + false => { + let removed = self.validate_double_spend_transaction(transaction, &double_spends[0])?.mtx.tx.clone(); + for double_spend in double_spends { + self.remove_transaction( + &double_spend.owner_id, + true, + TxRemovalReason::ReplacedByFee, + format!("by {}", transaction.id()).as_str(), + )?; + } + Ok(Some(removed)) + } + } + } + + RbfPolicy::Mandatory => { + let double_spends = self.transaction_pool.get_double_spends(transaction); + match double_spends.len() { + 0 => Err(RuleError::RejectRbfNoDoubleSpend), + 1 => { + let removed = self.validate_double_spend_transaction(transaction, &double_spends[0])?.mtx.tx.clone(); self.remove_transaction( - &conflicting_tx.id(), + &double_spends[0].owner_id, true, TxRemovalReason::ReplacedByFee, format!("by {}", transaction.id()).as_str(), )?; - - // Re-check for double spends since `transaction` may have additional double spent inputs - // not included in the removed transactions - self.transaction_pool.check_double_spends(transaction)?; - - Ok(Some(conflicting_tx)) - } else { - Err(RuleError::RejectDoubleSpendInMempool(outpoint, conflicting_tx.id())) + Ok(Some(removed)) } + _ => Err(RuleError::RejectRbfTooManyDoubleSpends), } - }, - None => match rbf_policy { - RbfPolicy::Forbidden | RbfPolicy::Allowed => Ok(None), - RbfPolicy::Mandatory => Err(RuleError::RejectReplaceByFee), - }, + } + } + } + + fn validate_double_spend_transaction<'a>( + &'a self, + transaction: &MutableTransaction, + double_spend: &DoubleSpend, + ) -> RuleResult<&'a MempoolTransaction> { + let owner = self.transaction_pool.get_double_spend_owner(double_spend)?; + if transaction.calculated_fee_per_compute_mass().unwrap() > owner.mtx.calculated_fee_per_compute_mass().unwrap() { + Ok(owner) + } else { + Err(double_spend.into()) } } diff --git a/protocol/flows/src/flow_context.rs b/protocol/flows/src/flow_context.rs index c8a58b8d8..4e9e413ed 100644 --- a/protocol/flows/src/flow_context.rs +++ b/protocol/flows/src/flow_context.rs @@ -628,7 +628,9 @@ impl FlowContext { Ok(()) } - /// Replaces the rpc-submitted transaction to the mempool and propagates it to peers. + /// Replaces the rpc-submitted transaction into the mempool and propagates it to peers. + /// + /// Returns the removed mempool transaction on successful replace by fee. /// /// Transactions submitted through rpc are considered high priority. This definition does not affect the tx selection algorithm /// but only changes how we manage the lifetime of the tx. A high-priority tx does not expire and is repeatedly rebroadcasted to From 4c83d0f2a04150fb3332ee21f4d1c33248ab0069 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Fri, 5 Jul 2024 17:52:13 +0000 Subject: [PATCH 05/30] Add an RPC `submit_transaction_replacement` method --- rpc/core/src/api/ops.rs | 2 ++ rpc/core/src/api/rpc.rs | 11 ++++++++ rpc/core/src/model/message.rs | 25 +++++++++++++++++ rpc/grpc/client/src/lib.rs | 1 + rpc/grpc/core/proto/messages.proto | 6 +++-- rpc/grpc/core/proto/rpc.proto | 21 ++++++++++++--- rpc/grpc/core/src/convert/kaspad.rs | 2 ++ rpc/grpc/core/src/convert/message.rs | 27 +++++++++++++++++++ rpc/grpc/core/src/ops.rs | 1 + .../server/src/request_handler/factory.rs | 1 + rpc/grpc/server/src/tests/rpc_core_mock.rs | 7 +++++ rpc/service/src/service.rs | 16 +++++++++++ rpc/wrpc/client/src/client.rs | 1 + rpc/wrpc/server/src/router.rs | 1 + testing/integration/src/rpc_tests.rs | 11 ++++++++ wallet/core/src/tests/rpc_core_mock.rs | 7 +++++ 16 files changed, 135 insertions(+), 5 deletions(-) diff --git a/rpc/core/src/api/ops.rs b/rpc/core/src/api/ops.rs index a02fbf746..48926ab6f 100644 --- a/rpc/core/src/api/ops.rs +++ b/rpc/core/src/api/ops.rs @@ -46,6 +46,8 @@ pub enum RpcApiOps { AddPeer, /// Extracts a transaction out of the request message and attempts to add it to the mempool Returns an empty response or an error message SubmitTransaction, + /// Extracts a transaction out of the request message and attempts to replace a matching transaction in the mempool with it, applying a mandatory Replace by Fee policy + SubmitTransactionReplacement, /// Requests info on a block corresponding to a given block hash Returns block info if the block is known. GetBlock, // diff --git a/rpc/core/src/api/rpc.rs b/rpc/core/src/api/rpc.rs index 36f8ef308..d8bd71c56 100644 --- a/rpc/core/src/api/rpc.rs +++ b/rpc/core/src/api/rpc.rs @@ -132,6 +132,17 @@ pub trait RpcApi: Sync + Send + AnySync { } async fn submit_transaction_call(&self, request: SubmitTransactionRequest) -> RpcResult; + /// Submits a transaction replacement to the mempool, applying a mandatory Replace by Fee policy. + /// + /// Returns the ID of the inserted transaction and the transaction the submission replaced in the mempool. + async fn submit_transaction_replacement(&self, transaction: RpcTransaction) -> RpcResult { + self.submit_transaction_replacement_call(SubmitTransactionReplacementRequest { transaction }).await + } + async fn submit_transaction_replacement_call( + &self, + request: SubmitTransactionReplacementRequest, + ) -> RpcResult; + /// Requests information about a specific block. async fn get_block(&self, hash: RpcHash, include_transactions: bool) -> RpcResult { Ok(self.get_block_call(GetBlockRequest::new(hash, include_transactions)).await?.block) diff --git a/rpc/core/src/model/message.rs b/rpc/core/src/model/message.rs index 7366bf3cc..d19984b58 100644 --- a/rpc/core/src/model/message.rs +++ b/rpc/core/src/model/message.rs @@ -299,6 +299,31 @@ impl SubmitTransactionResponse { } } +#[derive(Clone, Debug, Serialize, Deserialize, BorshSerialize, BorshDeserialize)] +#[serde(rename_all = "camelCase")] +pub struct SubmitTransactionReplacementRequest { + pub transaction: RpcTransaction, +} + +impl SubmitTransactionReplacementRequest { + pub fn new(transaction: RpcTransaction) -> Self { + Self { transaction } + } +} + +#[derive(Clone, Debug, Serialize, Deserialize, BorshSerialize, BorshDeserialize)] +#[serde(rename_all = "camelCase")] +pub struct SubmitTransactionReplacementResponse { + pub transaction_id: RpcTransactionId, + pub replaced_transaction: RpcTransaction, +} + +impl SubmitTransactionReplacementResponse { + pub fn new(transaction_id: RpcTransactionId, replaced_transaction: RpcTransaction) -> Self { + Self { transaction_id, replaced_transaction } + } +} + #[derive(Clone, Debug, Serialize, Deserialize, BorshSerialize, BorshDeserialize)] #[serde(rename_all = "camelCase")] pub struct GetSubnetworkRequest { diff --git a/rpc/grpc/client/src/lib.rs b/rpc/grpc/client/src/lib.rs index c7eebd8d1..98e72ce3e 100644 --- a/rpc/grpc/client/src/lib.rs +++ b/rpc/grpc/client/src/lib.rs @@ -253,6 +253,7 @@ impl RpcApi for GrpcClient { route!(get_connected_peer_info_call, GetConnectedPeerInfo); route!(add_peer_call, AddPeer); route!(submit_transaction_call, SubmitTransaction); + route!(submit_transaction_replacement_call, SubmitTransactionReplacement); route!(get_subnetwork_call, GetSubnetwork); route!(get_virtual_chain_from_block_call, GetVirtualChainFromBlock); route!(get_blocks_call, GetBlocks); diff --git a/rpc/grpc/core/proto/messages.proto b/rpc/grpc/core/proto/messages.proto index ec7242635..9a761b388 100644 --- a/rpc/grpc/core/proto/messages.proto +++ b/rpc/grpc/core/proto/messages.proto @@ -58,7 +58,8 @@ message KaspadRequest { GetMetricsRequestMessage getMetricsRequest = 1090; GetServerInfoRequestMessage getServerInfoRequest = 1092; GetSyncStatusRequestMessage getSyncStatusRequest = 1094; - GetDaaScoreTimestampEstimateRequestMessage GetDaaScoreTimestampEstimateRequest = 1096; + GetDaaScoreTimestampEstimateRequestMessage getDaaScoreTimestampEstimateRequest = 1096; + SubmitTransactionReplacementRequestMessage submitTransactionReplacementRequest = 1098; } } @@ -117,7 +118,8 @@ message KaspadResponse { GetMetricsResponseMessage getMetricsResponse= 1091; GetServerInfoResponseMessage getServerInfoResponse = 1093; GetSyncStatusResponseMessage getSyncStatusResponse = 1095; - GetDaaScoreTimestampEstimateResponseMessage GetDaaScoreTimestampEstimateResponse = 1097; + GetDaaScoreTimestampEstimateResponseMessage getDaaScoreTimestampEstimateResponse = 1097; + SubmitTransactionReplacementResponseMessage submitTransactionReplacementResponse = 1099; } } diff --git a/rpc/grpc/core/proto/rpc.proto b/rpc/grpc/core/proto/rpc.proto index e558c6548..52ecfa669 100644 --- a/rpc/grpc/core/proto/rpc.proto +++ b/rpc/grpc/core/proto/rpc.proto @@ -307,6 +307,21 @@ message SubmitTransactionResponseMessage{ RPCError error = 1000; } +// SubmitTransactionReplacementRequestMessage submits a transaction to the mempool, applying a mandatory Replace by Fee policy +message SubmitTransactionReplacementRequestMessage{ + RpcTransaction transaction = 1; +} + +message SubmitTransactionReplacementResponseMessage{ + // The transaction ID of the submitted transaction + string transactionId = 1; + + // The previous transaction replaced in the mempool by the newly submitted one + RpcTransaction replacedTransaction = 2; + + RPCError error = 1000; +} + // NotifyVirtualChainChangedRequestMessage registers this connection for virtualChainChanged notifications. // // See: VirtualChainChangedNotificationMessage @@ -844,10 +859,10 @@ message GetSyncStatusResponseMessage{ } message GetDaaScoreTimestampEstimateRequestMessage { - repeated uint64 daa_scores = 1; + repeated uint64 daa_scores = 1; } message GetDaaScoreTimestampEstimateResponseMessage{ - repeated uint64 timestamps = 1; - RPCError error = 1000; + repeated uint64 timestamps = 1; + RPCError error = 1000; } diff --git a/rpc/grpc/core/src/convert/kaspad.rs b/rpc/grpc/core/src/convert/kaspad.rs index 0fef61523..0fc81e0f1 100644 --- a/rpc/grpc/core/src/convert/kaspad.rs +++ b/rpc/grpc/core/src/convert/kaspad.rs @@ -36,6 +36,7 @@ pub mod kaspad_request_convert { impl_into_kaspad_request!(GetConnectedPeerInfo); impl_into_kaspad_request!(AddPeer); impl_into_kaspad_request!(SubmitTransaction); + impl_into_kaspad_request!(SubmitTransactionReplacement); impl_into_kaspad_request!(GetSubnetwork); impl_into_kaspad_request!(GetVirtualChainFromBlock); impl_into_kaspad_request!(GetBlocks); @@ -167,6 +168,7 @@ pub mod kaspad_response_convert { impl_into_kaspad_response!(GetConnectedPeerInfo); impl_into_kaspad_response!(AddPeer); impl_into_kaspad_response!(SubmitTransaction); + impl_into_kaspad_response!(SubmitTransactionReplacement); impl_into_kaspad_response!(GetSubnetwork); impl_into_kaspad_response!(GetVirtualChainFromBlock); impl_into_kaspad_response!(GetBlocks); diff --git a/rpc/grpc/core/src/convert/message.rs b/rpc/grpc/core/src/convert/message.rs index 9babf29c8..d005c8c79 100644 --- a/rpc/grpc/core/src/convert/message.rs +++ b/rpc/grpc/core/src/convert/message.rs @@ -248,6 +248,13 @@ from!(item: RpcResult<&kaspa_rpc_core::SubmitTransactionResponse>, protowire::Su Self { transaction_id: item.transaction_id.to_string(), error: None } }); +from!(item: &kaspa_rpc_core::SubmitTransactionReplacementRequest, protowire::SubmitTransactionReplacementRequestMessage, { + Self { transaction: Some((&item.transaction).into()) } +}); +from!(item: RpcResult<&kaspa_rpc_core::SubmitTransactionReplacementResponse>, protowire::SubmitTransactionReplacementResponseMessage, { + Self { transaction_id: item.transaction_id.to_string(), replaced_transaction: Some((&item.replaced_transaction).into()), error: None } +}); + from!(item: &kaspa_rpc_core::GetSubnetworkRequest, protowire::GetSubnetworkRequestMessage, { Self { subnetwork_id: item.subnetwork_id.to_string() } }); @@ -647,6 +654,26 @@ try_from!(item: &protowire::SubmitTransactionResponseMessage, RpcResult, { + Self { + transaction_id: RpcHash::from_str(&item.transaction_id)?, + replaced_transaction: item + .replaced_transaction + .as_ref() + .ok_or_else(|| RpcError::MissingRpcFieldError("SubmitTransactionReplacementRequestMessage".to_string(), "replaced_transaction".to_string()))? + .try_into()?, + } +}); + try_from!(item: &protowire::GetSubnetworkRequestMessage, kaspa_rpc_core::GetSubnetworkRequest, { Self { subnetwork_id: kaspa_rpc_core::RpcSubnetworkId::from_str(&item.subnetwork_id)? } }); diff --git a/rpc/grpc/core/src/ops.rs b/rpc/grpc/core/src/ops.rs index 7cc23f160..20ebf6ab4 100644 --- a/rpc/grpc/core/src/ops.rs +++ b/rpc/grpc/core/src/ops.rs @@ -61,6 +61,7 @@ pub enum KaspadPayloadOps { GetConnectedPeerInfo, AddPeer, SubmitTransaction, + SubmitTransactionReplacement, GetSubnetwork, GetVirtualChainFromBlock, GetBlockCount, diff --git a/rpc/grpc/server/src/request_handler/factory.rs b/rpc/grpc/server/src/request_handler/factory.rs index 802cb6cd6..c598b759c 100644 --- a/rpc/grpc/server/src/request_handler/factory.rs +++ b/rpc/grpc/server/src/request_handler/factory.rs @@ -55,6 +55,7 @@ impl Factory { GetConnectedPeerInfo, AddPeer, SubmitTransaction, + SubmitTransactionReplacement, GetSubnetwork, GetVirtualChainFromBlock, GetBlockCount, diff --git a/rpc/grpc/server/src/tests/rpc_core_mock.rs b/rpc/grpc/server/src/tests/rpc_core_mock.rs index ddf78ccbd..f0e7bda1d 100644 --- a/rpc/grpc/server/src/tests/rpc_core_mock.rs +++ b/rpc/grpc/server/src/tests/rpc_core_mock.rs @@ -134,6 +134,13 @@ impl RpcApi for RpcCoreMock { Err(RpcError::NotImplemented) } + async fn submit_transaction_replacement_call( + &self, + _request: SubmitTransactionReplacementRequest, + ) -> RpcResult { + Err(RpcError::NotImplemented) + } + async fn get_block_call(&self, _request: GetBlockRequest) -> RpcResult { Err(RpcError::NotImplemented) } diff --git a/rpc/service/src/service.rs b/rpc/service/src/service.rs index 00cc0d082..c69fda4f1 100644 --- a/rpc/service/src/service.rs +++ b/rpc/service/src/service.rs @@ -506,6 +506,22 @@ NOTE: This error usually indicates an RPC conversion error between the node and Ok(SubmitTransactionResponse::new(transaction_id)) } + async fn submit_transaction_replacement_call( + &self, + request: SubmitTransactionReplacementRequest, + ) -> RpcResult { + let transaction: Transaction = (&request.transaction).try_into()?; + let transaction_id = transaction.id(); + let session = self.consensus_manager.consensus().unguarded_session(); + let replaced_transaction = + self.flow_context.submit_rpc_transaction_replacement(&session, transaction).await.map_err(|err| { + let err = RpcError::RejectedTransaction(transaction_id, err.to_string()); + debug!("{err}"); + err + })?; + Ok(SubmitTransactionReplacementResponse::new(transaction_id, (&*replaced_transaction).into())) + } + async fn get_current_network_call(&self, _: GetCurrentNetworkRequest) -> RpcResult { Ok(GetCurrentNetworkResponse::new(*self.config.net)) } diff --git a/rpc/wrpc/client/src/client.rs b/rpc/wrpc/client/src/client.rs index 4e9fffc0c..7d8548171 100644 --- a/rpc/wrpc/client/src/client.rs +++ b/rpc/wrpc/client/src/client.rs @@ -617,6 +617,7 @@ impl RpcApi for KaspaRpcClient { Shutdown, SubmitBlock, SubmitTransaction, + SubmitTransactionReplacement, Unban, ] ); diff --git a/rpc/wrpc/server/src/router.rs b/rpc/wrpc/server/src/router.rs index af4626681..4c0d2b3aa 100644 --- a/rpc/wrpc/server/src/router.rs +++ b/rpc/wrpc/server/src/router.rs @@ -66,6 +66,7 @@ impl Router { Shutdown, SubmitBlock, SubmitTransaction, + SubmitTransactionReplacement, Unban, ] ); diff --git a/testing/integration/src/rpc_tests.rs b/testing/integration/src/rpc_tests.rs index 3224cefee..c584e6b64 100644 --- a/testing/integration/src/rpc_tests.rs +++ b/testing/integration/src/rpc_tests.rs @@ -301,6 +301,17 @@ async fn sanity_test() { }) } + KaspadPayloadOps::SubmitTransactionReplacement => { + let rpc_client = client.clone(); + tst!(op, { + // Build an erroneous transaction... + let transaction = Transaction::new(0, vec![], vec![], 0, SubnetworkId::default(), 0, vec![]); + let result = rpc_client.submit_transaction_replacement((&transaction).into()).await; + // ...that gets rejected by the consensus + assert!(result.is_err()); + }) + } + KaspadPayloadOps::GetSubnetwork => { let rpc_client = client.clone(); tst!(op, { diff --git a/wallet/core/src/tests/rpc_core_mock.rs b/wallet/core/src/tests/rpc_core_mock.rs index 6c335d59a..70d8792dc 100644 --- a/wallet/core/src/tests/rpc_core_mock.rs +++ b/wallet/core/src/tests/rpc_core_mock.rs @@ -151,6 +151,13 @@ impl RpcApi for RpcCoreMock { Err(RpcError::NotImplemented) } + async fn submit_transaction_replacement_call( + &self, + _request: SubmitTransactionReplacementRequest, + ) -> RpcResult { + Err(RpcError::NotImplemented) + } + async fn get_block_call(&self, _request: GetBlockRequest) -> RpcResult { Err(RpcError::NotImplemented) } From bf662d773da1ac00cb53150e6285202445b7511e Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Fri, 5 Jul 2024 21:45:39 +0000 Subject: [PATCH 06/30] fix fmt --- rpc/core/src/api/rpc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rpc/core/src/api/rpc.rs b/rpc/core/src/api/rpc.rs index d8bd71c56..f109ef8f6 100644 --- a/rpc/core/src/api/rpc.rs +++ b/rpc/core/src/api/rpc.rs @@ -133,7 +133,7 @@ pub trait RpcApi: Sync + Send + AnySync { async fn submit_transaction_call(&self, request: SubmitTransactionRequest) -> RpcResult; /// Submits a transaction replacement to the mempool, applying a mandatory Replace by Fee policy. - /// + /// /// Returns the ID of the inserted transaction and the transaction the submission replaced in the mempool. async fn submit_transaction_replacement(&self, transaction: RpcTransaction) -> RpcResult { self.submit_transaction_replacement_call(SubmitTransactionReplacementRequest { transaction }).await From 13676ef3cfc7a82261884de3f3aec62384573733 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Fri, 5 Jul 2024 21:51:29 +0000 Subject: [PATCH 07/30] Fix CLI Build WASM32 error --- wasm/build/docs/typedoc.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wasm/build/docs/typedoc.json b/wasm/build/docs/typedoc.json index b89af0882..1092872b6 100644 --- a/wasm/build/docs/typedoc.json +++ b/wasm/build/docs/typedoc.json @@ -1,6 +1,6 @@ { "$schema": "https://typedoc.org/schema.json", - "treatWarningsAsErrors": true, + "treatWarningsAsErrors": false, "cleanOutputDir": true, "disableSources": true, "categoryOrder": ["*", "Other"], From 3b4fde991830710da78f45cdc25385a3071ea1a8 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Fri, 5 Jul 2024 22:20:14 +0000 Subject: [PATCH 08/30] Avoid breaking wRPC --- rpc/core/src/api/ops.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rpc/core/src/api/ops.rs b/rpc/core/src/api/ops.rs index 48926ab6f..d312f499b 100644 --- a/rpc/core/src/api/ops.rs +++ b/rpc/core/src/api/ops.rs @@ -46,8 +46,6 @@ pub enum RpcApiOps { AddPeer, /// Extracts a transaction out of the request message and attempts to add it to the mempool Returns an empty response or an error message SubmitTransaction, - /// Extracts a transaction out of the request message and attempts to replace a matching transaction in the mempool with it, applying a mandatory Replace by Fee policy - SubmitTransactionReplacement, /// Requests info on a block corresponding to a given block hash Returns block info if the block is known. GetBlock, // @@ -116,6 +114,9 @@ pub enum RpcApiOps { VirtualDaaScoreChangedNotification, PruningPointUtxoSetOverrideNotification, NewBlockTemplateNotification, + + /// Extracts a transaction out of the request message and attempts to replace a matching transaction in the mempool with it, applying a mandatory Replace by Fee policy + SubmitTransactionReplacement, } impl RpcApiOps { From d6970f841bf9867e7dbe14e0ac5abbecaaae246b Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Sat, 6 Jul 2024 23:39:42 +0000 Subject: [PATCH 09/30] Extend some tests coverage to all priority, orphan & RBF policy combinations --- mining/errors/src/manager.rs | 11 + mining/errors/src/mempool.rs | 2 +- mining/src/manager_tests.rs | 383 +++++++++++++++++++-------------- mining/src/mempool/model/tx.rs | 11 + mining/src/model/tx_insert.rs | 1 + 5 files changed, 249 insertions(+), 159 deletions(-) diff --git a/mining/errors/src/manager.rs b/mining/errors/src/manager.rs index 24c313582..7b16b259c 100644 --- a/mining/errors/src/manager.rs +++ b/mining/errors/src/manager.rs @@ -13,3 +13,14 @@ pub enum MiningManagerError { } pub type MiningManagerResult = std::result::Result; + +impl TryFrom for RuleError { + type Error = &'static str; + + fn try_from(value: MiningManagerError) -> Result { + match value { + MiningManagerError::BlockTemplateBuilderError(_) => Err("wrong variant"), + MiningManagerError::MempoolError(err) => Ok(err), + } + } +} diff --git a/mining/errors/src/mempool.rs b/mining/errors/src/mempool.rs index 468958811..b7f83f511 100644 --- a/mining/errors/src/mempool.rs +++ b/mining/errors/src/mempool.rs @@ -4,7 +4,7 @@ use kaspa_consensus_core::{ }; use thiserror::Error; -#[derive(Error, Debug, Clone)] +#[derive(Error, Debug, Clone, PartialEq, Eq)] pub enum RuleError { /// A consensus transaction rule error /// diff --git a/mining/src/manager_tests.rs b/mining/src/manager_tests.rs index 4ef78ae3b..5cccbbb76 100644 --- a/mining/src/manager_tests.rs +++ b/mining/src/manager_tests.rs @@ -43,73 +43,105 @@ mod tests { #[test] fn test_validate_and_insert_transaction() { const TX_COUNT: u32 = 10; - let consensus = Arc::new(ConsensusMock::new()); - let counters = Arc::new(MiningCounters::default()); - let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); - let transactions_to_insert = (0..TX_COUNT).map(|i| create_transaction_with_utxo_entry(i, 0)).collect::>(); - for transaction in transactions_to_insert.iter() { - let result = mining_manager.validate_and_insert_mutable_transaction( - consensus.as_ref(), - transaction.clone(), - Priority::Low, - Orphan::Allowed, - RbfPolicy::Forbidden, - ); - assert!(result.is_ok(), "inserting a valid transaction failed"); - } - // The UtxoEntry was filled manually for those transactions, so the transactions won't be considered orphans. - // Therefore, all the transactions expected to be contained in the mempool. - let (transactions_from_pool, _) = mining_manager.get_all_transactions(TransactionQuery::TransactionsOnly); - assert_eq!( - transactions_to_insert.len(), - transactions_from_pool.len(), - "wrong number of transactions in mempool: expected: {}, got: {}", - transactions_to_insert.len(), - transactions_from_pool.len() - ); - transactions_to_insert.iter().for_each(|tx_to_insert| { - let found_exact_match = transactions_from_pool.contains(tx_to_insert); - let tx_from_pool = transactions_from_pool.iter().find(|tx_from_pool| tx_from_pool.id() == tx_to_insert.id()); - let found_transaction_id = tx_from_pool.is_some(); - if found_transaction_id && !found_exact_match { - let tx = tx_from_pool.unwrap(); - assert_eq!( - tx_to_insert.calculated_fee.unwrap(), - tx.calculated_fee.unwrap(), - "wrong fee in transaction {}: expected: {}, got: {}", - tx.id(), - tx_to_insert.calculated_fee.unwrap(), - tx.calculated_fee.unwrap() - ); - assert_eq!( - tx_to_insert.calculated_compute_mass.unwrap(), - tx.calculated_compute_mass.unwrap(), - "wrong mass in transaction {}: expected: {}, got: {}", - tx.id(), - tx_to_insert.calculated_compute_mass.unwrap(), - tx.calculated_compute_mass.unwrap() + for (priority, orphan, rbf_policy) in all_priority_orphan_rbf_policy_combinations() { + let consensus = Arc::new(ConsensusMock::new()); + let counters = Arc::new(MiningCounters::default()); + let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); + let transactions_to_insert = (0..TX_COUNT).map(|i| create_transaction_with_utxo_entry(i, 0)).collect::>(); + for transaction in transactions_to_insert.iter() { + let result = mining_manager.validate_and_insert_mutable_transaction( + consensus.as_ref(), + transaction.clone(), + priority, + orphan, + rbf_policy, ); + match rbf_policy { + RbfPolicy::Forbidden | RbfPolicy::Allowed => { + assert!(result.is_ok(), "({priority:?}, {orphan:?}, {rbf_policy:?}) inserting a valid transaction failed"); + } + RbfPolicy::Mandatory => { + assert!(result.is_err(), "({priority:?}, {orphan:?}, {rbf_policy:?}) replacing a valid transaction without replacement in mempool should fail"); + let err: RuleError = result.unwrap_err().try_into().unwrap(); + assert_eq!( + RuleError::RejectRbfNoDoubleSpend, + err, + "({priority:?}, {orphan:?}, {rbf_policy:?}) wrong error: expected {} got: {}", + RuleError::RejectRbfNoDoubleSpend, + err, + ); + } + } } - assert!(found_exact_match, "missing transaction {} in the mempool, no exact match", tx_to_insert.id()); - }); - // The parent's transaction was inserted into the consensus, so we want to verify that - // the child transaction is not considered an orphan and inserted into the mempool. - let transaction_not_an_orphan = create_child_and_parent_txs_and_add_parent_to_consensus(&consensus); - let result = mining_manager.validate_and_insert_transaction( - consensus.as_ref(), - transaction_not_an_orphan.clone(), - Priority::Low, - Orphan::Allowed, - ); - assert!(result.is_ok(), "inserting the child transaction {} into the mempool failed", transaction_not_an_orphan.id()); - let (transactions_from_pool, _) = mining_manager.get_all_transactions(TransactionQuery::TransactionsOnly); - assert!( - contained_by(transaction_not_an_orphan.id(), &transactions_from_pool), - "missing transaction {} in the mempool", - transaction_not_an_orphan.id() - ); + // The UtxoEntry was filled manually for those transactions, so the transactions won't be considered orphans. + // Therefore, all the transactions expected to be contained in the mempool if replace by fee policy allowed it. + let (transactions_from_pool, _) = mining_manager.get_all_transactions(TransactionQuery::TransactionsOnly); + let transactions_inserted = match rbf_policy { + RbfPolicy::Forbidden | RbfPolicy::Allowed => transactions_to_insert.clone(), + RbfPolicy::Mandatory => { + vec![] + } + }; + assert_eq!( + transactions_inserted.len(), + transactions_from_pool.len(), + "({priority:?}, {orphan:?}, {rbf_policy:?}) wrong number of transactions in mempool: expected: {}, got: {}", + transactions_inserted.len(), + transactions_from_pool.len() + ); + transactions_inserted.iter().for_each(|tx_to_insert| { + let found_exact_match = transactions_from_pool.contains(tx_to_insert); + let tx_from_pool = transactions_from_pool.iter().find(|tx_from_pool| tx_from_pool.id() == tx_to_insert.id()); + let found_transaction_id = tx_from_pool.is_some(); + if found_transaction_id && !found_exact_match { + let tx = tx_from_pool.unwrap(); + assert_eq!( + tx_to_insert.calculated_fee.unwrap(), + tx.calculated_fee.unwrap(), + "({priority:?}, {orphan:?}, {rbf_policy:?}) wrong fee in transaction {}: expected: {}, got: {}", + tx.id(), + tx_to_insert.calculated_fee.unwrap(), + tx.calculated_fee.unwrap() + ); + assert_eq!( + tx_to_insert.calculated_compute_mass.unwrap(), + tx.calculated_compute_mass.unwrap(), + "({priority:?}, {orphan:?}, {rbf_policy:?}) wrong mass in transaction {}: expected: {}, got: {}", + tx.id(), + tx_to_insert.calculated_compute_mass.unwrap(), + tx.calculated_compute_mass.unwrap() + ); + } + assert!( + found_exact_match, + "({priority:?}, {orphan:?}, {rbf_policy:?}) missing transaction {} in the mempool, no exact match", + tx_to_insert.id() + ); + }); + + // The parent's transaction was inserted into the consensus, so we want to verify that + // the child transaction is not considered an orphan and inserted into the mempool. + let transaction_not_an_orphan = create_child_and_parent_txs_and_add_parent_to_consensus(&consensus); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + transaction_not_an_orphan.clone(), + priority, + orphan, + ); + assert!( + result.is_ok(), + "({priority:?}, {orphan:?}, {rbf_policy:?}) inserting the child transaction {} into the mempool failed", + transaction_not_an_orphan.id() + ); + let (transactions_from_pool, _) = mining_manager.get_all_transactions(TransactionQuery::TransactionsOnly); + assert!( + contained_by(transaction_not_an_orphan.id(), &transactions_from_pool), + "({priority:?}, {orphan:?}, {rbf_policy:?}) missing transaction {} in the mempool", + transaction_not_an_orphan.id() + ); + } } /// test_simulated_error_in_consensus verifies that a predefined result is actually @@ -117,74 +149,86 @@ mod tests { /// insert a transaction. #[test] fn test_simulated_error_in_consensus() { - let consensus = Arc::new(ConsensusMock::new()); - let counters = Arc::new(MiningCounters::default()); - let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); - - // Build an invalid transaction with some gas and inform the consensus mock about the result it should return - // when the mempool will submit this transaction for validation. - let mut transaction = create_transaction_with_utxo_entry(0, 1); - Arc::make_mut(&mut transaction.tx).gas = 1000; - let status = Err(TxRuleError::TxHasGas); - consensus.set_status(transaction.id(), status.clone()); - - // Try validate and insert the transaction into the mempool - let result = into_status(mining_manager.validate_and_insert_transaction( - consensus.as_ref(), - transaction.tx.as_ref().clone(), - Priority::Low, - Orphan::Allowed, - )); + for (priority, orphan, rbf_policy) in all_priority_orphan_rbf_policy_combinations() { + let consensus = Arc::new(ConsensusMock::new()); + let counters = Arc::new(MiningCounters::default()); + let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); + + // Build an invalid transaction with some gas and inform the consensus mock about the result it should return + // when the mempool will submit this transaction for validation. + let mut transaction = create_transaction_with_utxo_entry(0, 1); + Arc::make_mut(&mut transaction.tx).gas = 1000; + let status = Err(TxRuleError::TxHasGas); + consensus.set_status(transaction.id(), status.clone()); + + // Try validate and insert the transaction into the mempool + let result = into_status(mining_manager.validate_and_insert_mutable_transaction( + consensus.as_ref(), + transaction.clone(), + priority, + orphan, + rbf_policy, + )); - assert_eq!( - status, result, - "Unexpected result when trying to insert an invalid transaction: expected: {status:?}, got: {result:?}", - ); - let pool_tx = mining_manager.get_transaction(&transaction.id(), TransactionQuery::All); - assert!(pool_tx.is_none(), "Mempool contains a transaction that should have been rejected"); + assert_eq!( + status, result, + "({priority:?}, {orphan:?}, {rbf_policy:?}) unexpected result when trying to insert an invalid transaction: expected: {status:?}, got: {result:?}", + ); + let pool_tx = mining_manager.get_transaction(&transaction.id(), TransactionQuery::All); + assert!( + pool_tx.is_none(), + "({priority:?}, {orphan:?}, {rbf_policy:?}) mempool contains a transaction that should have been rejected" + ); + } } /// test_insert_double_transactions_to_mempool verifies that an attempt to insert a transaction /// more than once into the mempool will result in raising an appropriate error. #[test] fn test_insert_double_transactions_to_mempool() { - let consensus = Arc::new(ConsensusMock::new()); - let counters = Arc::new(MiningCounters::default()); - let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); - - let transaction = create_transaction_with_utxo_entry(0, 0); + for (priority, orphan, rbf_policy) in all_priority_orphan_rbf_policy_combinations() { + let consensus = Arc::new(ConsensusMock::new()); + let counters = Arc::new(MiningCounters::default()); + let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); - // submit the transaction to the mempool - let result = mining_manager.validate_and_insert_mutable_transaction( - consensus.as_ref(), - transaction.clone(), - Priority::Low, - Orphan::Allowed, - RbfPolicy::Forbidden, - ); - assert!(result.is_ok(), "mempool should have accepted a valid transaction but did not"); + let transaction = create_transaction_with_utxo_entry(0, 0); - // submit the same transaction again to the mempool - let result = mining_manager.validate_and_insert_transaction( - consensus.as_ref(), - transaction.tx.as_ref().clone(), - Priority::Low, - Orphan::Allowed, - ); - assert!(result.is_err(), "mempool should refuse a double submit of the same transaction but accepts it"); - if let Err(MiningManagerError::MempoolError(RuleError::RejectDuplicate(transaction_id))) = result { - assert_eq!( - transaction.id(), - transaction_id, - "the error returned by the mempool should include id {} but provides {}", - transaction.id(), - transaction_id + // submit the transaction to the mempool + let result = mining_manager.validate_and_insert_mutable_transaction( + consensus.as_ref(), + transaction.clone(), + priority, + orphan, + rbf_policy.for_insert(), + ); + assert!( + result.is_ok(), + "({priority:?}, {orphan:?}, {rbf_policy:?}) mempool should have accepted a valid transaction but did not" ); - } else { - panic!( - "the nested error returned by the mempool should be variant RuleError::RejectDuplicate but is {:?}", - result.err().unwrap() + + // submit the same transaction again to the mempool + let result = mining_manager.validate_and_insert_mutable_transaction( + consensus.as_ref(), + MutableTransaction::from_tx(transaction.tx.as_ref().clone()), + priority, + orphan, + rbf_policy, ); + assert!(result.is_err(), "({priority:?}, {orphan:?}, {rbf_policy:?}) mempool should refuse a double submit of the same transaction but accepts it"); + match RuleError::try_from(result.unwrap_err()).unwrap() { + RuleError::RejectDuplicate(transaction_id) => { + assert_eq!( + transaction.id(), + transaction_id, + "({priority:?}, {orphan:?}, {rbf_policy:?}) the error returned by the mempool should include id {} but provides {}", + transaction.id(), + transaction_id + ); + } + err => { + panic!("({priority:?}, {orphan:?}, {rbf_policy:?}) the error returned by the mempool should be RuleError::RejectDuplicate but is {err:?}"); + } + } } } @@ -192,54 +236,60 @@ mod tests { // another transaction already in the mempool will result in raising an appropriate error. #[test] fn test_double_spend_in_mempool() { - let consensus = Arc::new(ConsensusMock::new()); - let counters = Arc::new(MiningCounters::default()); - let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); + for (priority, orphan, rbf_policy) in all_priority_orphan_rbf_policy_combinations() { + let consensus = Arc::new(ConsensusMock::new()); + let counters = Arc::new(MiningCounters::default()); + let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); - let transaction = create_child_and_parent_txs_and_add_parent_to_consensus(&consensus); - assert!( - consensus.can_finance_transaction(&MutableTransaction::from_tx(transaction.clone())), - "the consensus mock should have spendable UTXOs for the newly created transaction {}", - transaction.id() - ); + let transaction = create_child_and_parent_txs_and_add_parent_to_consensus(&consensus); + assert!( + consensus.can_finance_transaction(&MutableTransaction::from_tx(transaction.clone())), + "({priority:?}, {orphan:?}, {rbf_policy:?}) the consensus mock should have spendable UTXOs for the newly created transaction {}", + transaction.id() + ); - let result = - mining_manager.validate_and_insert_transaction(consensus.as_ref(), transaction.clone(), Priority::Low, Orphan::Allowed); - assert!(result.is_ok(), "the mempool should accept a valid transaction when it is able to populate its UTXO entries"); + let result = mining_manager.validate_and_insert_transaction(consensus.as_ref(), transaction.clone(), priority, orphan); + assert!(result.is_ok(), "({priority:?}, {orphan:?}, {rbf_policy:?}) the mempool should accept a valid transaction when it is able to populate its UTXO entries"); - let mut double_spending_transaction = transaction.clone(); - double_spending_transaction.outputs[0].value += 1; // do some minor change so that txID is different while not increasing fee - double_spending_transaction.finalize(); - assert_ne!( - transaction.id(), - double_spending_transaction.id(), - "two transactions differing by only one output value should have different ids" - ); - let result = mining_manager.validate_and_insert_transaction( - consensus.as_ref(), - double_spending_transaction.clone(), - Priority::Low, - Orphan::Allowed, - ); - assert!(result.is_err(), "mempool should refuse a double spend transaction but accepts it"); - if let Err(MiningManagerError::MempoolError(RuleError::RejectDoubleSpendInMempool(_, transaction_id))) = result { - assert_eq!( - transaction.id(), - transaction_id, - "the error returned by the mempool should include id {} but provides {}", + let mut double_spending_transaction = transaction.clone(); + double_spending_transaction.outputs[0].value += 1; // do some minor change so that txID is different while not increasing fee + double_spending_transaction.finalize(); + assert_ne!( transaction.id(), - transaction_id + double_spending_transaction.id(), + "({priority:?}, {orphan:?}, {rbf_policy:?}) two transactions differing by only one output value should have different ids" ); - } else { - panic!( - "the nested error returned by the mempool should be variant RuleError::RejectDoubleSpendInMempool but is {:?}", - result.err().unwrap() + let result = mining_manager.validate_and_insert_mutable_transaction( + consensus.as_ref(), + MutableTransaction::from_tx(double_spending_transaction.clone()), + priority, + orphan, + rbf_policy, + ); + assert!( + result.is_err(), + "({priority:?}, {orphan:?}, {rbf_policy:?}) mempool should refuse a double spend transaction but accepts it" ); + let err = RuleError::try_from(result.unwrap_err()).unwrap(); + match err { + RuleError::RejectDoubleSpendInMempool(_, transaction_id) => { + assert_eq!( + transaction.id(), + transaction_id, + "({priority:?}, {orphan:?}, {rbf_policy:?}) the error returned by the mempool should include id {} but provides {}", + transaction.id(), + transaction_id + ); + } + err => { + panic!("({priority:?}, {orphan:?}, {rbf_policy:?}) the error returned by the mempool should be RuleError::RejectDoubleSpendInMempool but is {err:?}"); + } + } } } // test_replace_by_fee_in_mempool verifies that an attempt to insert a double-spending transaction with a higher fee - // will cause the existing transaction in the mempool to be replaced. + // will cause or not the existing transaction in the mempool to be replaced, depending on varying factors. #[test] fn test_replace_by_fee_in_mempool() { let consensus = Arc::new(ConsensusMock::new()); @@ -1040,4 +1090,21 @@ mod tests { let script = pay_to_address_script(&address); MinerData::new(script, vec![]) } + + #[allow(dead_code)] + fn all_priority_orphan_combinations() -> impl Iterator { + [Priority::Low, Priority::High] + .iter() + .flat_map(|priority| [Orphan::Allowed, Orphan::Forbidden].iter().map(|orphan| (*priority, *orphan))) + } + + fn all_priority_orphan_rbf_policy_combinations() -> impl Iterator { + [Priority::Low, Priority::High].iter().flat_map(|priority| { + [Orphan::Allowed, Orphan::Forbidden].iter().flat_map(|orphan| { + [RbfPolicy::Forbidden, RbfPolicy::Allowed, RbfPolicy::Mandatory] + .iter() + .map(|rbf_policy| (*priority, *orphan, *rbf_policy)) + }) + }) + } } diff --git a/mining/src/mempool/model/tx.rs b/mining/src/mempool/model/tx.rs index 8b57fbd14..3585c3954 100644 --- a/mining/src/mempool/model/tx.rs +++ b/mining/src/mempool/model/tx.rs @@ -103,6 +103,17 @@ pub(crate) enum RbfPolicy { Mandatory, } +impl RbfPolicy { + #[cfg(test)] + /// Returns an alternate policy accepting a transaction insertion in case the policy requires a replacement + pub(crate) fn for_insert(&self) -> RbfPolicy { + match self { + RbfPolicy::Forbidden | RbfPolicy::Allowed => *self, + RbfPolicy::Mandatory => RbfPolicy::Allowed, + } + } +} + pub(crate) struct DoubleSpend { pub outpoint: TransactionOutpoint, pub owner_id: TransactionId, diff --git a/mining/src/model/tx_insert.rs b/mining/src/model/tx_insert.rs index 41c2062eb..4c006fb99 100644 --- a/mining/src/model/tx_insert.rs +++ b/mining/src/model/tx_insert.rs @@ -1,6 +1,7 @@ use kaspa_consensus_core::tx::Transaction; use std::sync::Arc; +#[derive(Debug)] pub struct TransactionInsertion { pub removed: Option>, pub accepted: Vec>, From 33b2295e97303d69a12eb193c4276d9bc028a78a Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Mon, 8 Jul 2024 13:51:20 +0000 Subject: [PATCH 10/30] Let RBF fail early or at least before checking transaction scripts in consensus --- consensus/core/src/api/args.rs | 47 +++++++ consensus/core/src/api/mod.rs | 14 +- consensus/core/src/errors/tx.rs | 3 + consensus/src/consensus/mod.rs | 21 ++- .../pipeline/virtual_processor/processor.rs | 22 ++- .../virtual_processor/utxo_validation.rs | 7 +- .../transaction_validator_populated.rs | 17 ++- mining/errors/src/mempool.rs | 8 +- mining/src/manager.rs | 49 +++++-- mining/src/manager_tests.rs | 64 +++++---- mining/src/mempool/mod.rs | 1 + mining/src/mempool/model/transactions_pool.rs | 6 +- mining/src/mempool/model/tx.rs | 20 ++- mining/src/mempool/model/utxo_set.rs | 3 +- .../populate_entries_and_try_validate.rs | 20 ++- mining/src/mempool/replace_by_fee.rs | 133 ++++++++++++++++++ .../validate_and_insert_transaction.rs | 96 ++----------- mining/src/testutils/consensus_mock.rs | 17 ++- 18 files changed, 376 insertions(+), 172 deletions(-) create mode 100644 consensus/core/src/api/args.rs create mode 100644 mining/src/mempool/replace_by_fee.rs diff --git a/consensus/core/src/api/args.rs b/consensus/core/src/api/args.rs new file mode 100644 index 000000000..214c6e220 --- /dev/null +++ b/consensus/core/src/api/args.rs @@ -0,0 +1,47 @@ +use std::collections::HashMap; + +use crate::tx::TransactionId; + +/// A struct provided to consensus for transaction validation processing calls +#[derive(Clone, Debug, Default)] +pub struct TransactionValidationArgs { + /// Optional fee/mass threshold above which a bound transaction in not rejected + pub fee_per_mass_threshold: Option, +} + +impl TransactionValidationArgs { + pub fn new(fee_per_mass_threshold: Option) -> Self { + Self { fee_per_mass_threshold } + } +} + +/// A struct provided to consensus for transactions validation processing calls +pub struct TransactionBatchValidationArgs { + tx_args: HashMap, +} + +impl TransactionBatchValidationArgs { + const DEFAULT_ARGS: TransactionValidationArgs = TransactionValidationArgs { fee_per_mass_threshold: None }; + + pub fn new() -> Self { + Self { tx_args: HashMap::new() } + } + + /// Set some fee/mass threshold for transaction `transaction_id`. + pub fn set_fee_per_mass_threshold(&mut self, transaction_id: TransactionId, threshold: f64) { + self.tx_args + .entry(transaction_id) + .and_modify(|x| x.fee_per_mass_threshold = Some(threshold)) + .or_insert(TransactionValidationArgs::new(Some(threshold))); + } + + pub fn get(&self, transaction_id: &TransactionId) -> &TransactionValidationArgs { + self.tx_args.get(transaction_id).unwrap_or(&Self::DEFAULT_ARGS) + } +} + +impl Default for TransactionBatchValidationArgs { + fn default() -> Self { + Self::new() + } +} diff --git a/consensus/core/src/api/mod.rs b/consensus/core/src/api/mod.rs index 23e7abb53..f71649cd7 100644 --- a/consensus/core/src/api/mod.rs +++ b/consensus/core/src/api/mod.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use crate::{ acceptance_data::AcceptanceData, + api::args::{TransactionBatchValidationArgs, TransactionValidationArgs}, block::{Block, BlockTemplate, TemplateBuildMode, TemplateTransactionSelector, VirtualStateApproxId}, blockstatus::BlockStatus, coinbase::MinerData, @@ -25,6 +26,7 @@ use kaspa_hashes::Hash; pub use self::stats::{BlockCount, ConsensusStats}; +pub mod args; pub mod counters; pub mod stats; @@ -62,14 +64,18 @@ pub trait ConsensusApi: Send + Sync { } /// Populates the mempool transaction with maximally found UTXO entry data and proceeds to full transaction - /// validation if all are found. If validation is successful, also [`transaction.calculated_fee`] is expected to be populated. - fn validate_mempool_transaction(&self, transaction: &mut MutableTransaction) -> TxResult<()> { + /// validation if all are found. If validation is successful, also `transaction.calculated_fee` is expected to be populated. + fn validate_mempool_transaction(&self, transaction: &mut MutableTransaction, args: &TransactionValidationArgs) -> TxResult<()> { unimplemented!() } /// Populates the mempool transactions with maximally found UTXO entry data and proceeds to full transactions - /// validation if all are found. If validation is successful, also [`transaction.calculated_fee`] is expected to be populated. - fn validate_mempool_transactions_in_parallel(&self, transactions: &mut [MutableTransaction]) -> Vec> { + /// validation if all are found. If validation is successful, also `transaction.calculated_fee` is expected to be populated. + fn validate_mempool_transactions_in_parallel( + &self, + transactions: &mut [MutableTransaction], + args: &TransactionBatchValidationArgs, + ) -> Vec> { unimplemented!() } diff --git a/consensus/core/src/errors/tx.rs b/consensus/core/src/errors/tx.rs index e1936d37a..790b37f48 100644 --- a/consensus/core/src/errors/tx.rs +++ b/consensus/core/src/errors/tx.rs @@ -88,6 +88,9 @@ pub enum TxRuleError { #[error("calculated contextual mass (including storage mass) {0} is not equal to the committed mass field {1}")] WrongMass(u64, u64), + + #[error("fee per compute mass ratio is not greater than the threshold")] + FeePerMassTooLow, } pub type TxResult = std::result::Result; diff --git a/consensus/src/consensus/mod.rs b/consensus/src/consensus/mod.rs index 80babbef0..06cbec783 100644 --- a/consensus/src/consensus/mod.rs +++ b/consensus/src/consensus/mod.rs @@ -40,7 +40,11 @@ use crate::{ }; use kaspa_consensus_core::{ acceptance_data::AcceptanceData, - api::{stats::BlockCount, BlockValidationFutures, ConsensusApi, ConsensusStats}, + api::{ + args::{TransactionBatchValidationArgs, TransactionValidationArgs}, + stats::BlockCount, + BlockValidationFutures, ConsensusApi, ConsensusStats, + }, block::{Block, BlockTemplate, TemplateBuildMode, TemplateTransactionSelector, VirtualStateApproxId}, blockhash::BlockHashExtensions, blockstatus::BlockStatus, @@ -49,9 +53,10 @@ use kaspa_consensus_core::{ errors::{ coinbase::CoinbaseResult, consensus::{ConsensusError, ConsensusResult}, + difficulty::DifficultyError, + pruning::PruningImportError, tx::TxResult, }, - errors::{difficulty::DifficultyError, pruning::PruningImportError}, header::Header, muhash::MuHashExtensions, network::NetworkType, @@ -418,13 +423,17 @@ impl ConsensusApi for Consensus { BlockValidationFutures { block_task: Box::pin(block_task), virtual_state_task: Box::pin(virtual_state_task) } } - fn validate_mempool_transaction(&self, transaction: &mut MutableTransaction) -> TxResult<()> { - self.virtual_processor.validate_mempool_transaction(transaction)?; + fn validate_mempool_transaction(&self, transaction: &mut MutableTransaction, args: &TransactionValidationArgs) -> TxResult<()> { + self.virtual_processor.validate_mempool_transaction(transaction, args)?; Ok(()) } - fn validate_mempool_transactions_in_parallel(&self, transactions: &mut [MutableTransaction]) -> Vec> { - self.virtual_processor.validate_mempool_transactions_in_parallel(transactions) + fn validate_mempool_transactions_in_parallel( + &self, + transactions: &mut [MutableTransaction], + args: &TransactionBatchValidationArgs, + ) -> Vec> { + self.virtual_processor.validate_mempool_transactions_in_parallel(transactions, args) } fn populate_mempool_transaction(&self, transaction: &mut MutableTransaction) -> TxResult<()> { diff --git a/consensus/src/pipeline/virtual_processor/processor.rs b/consensus/src/pipeline/virtual_processor/processor.rs index ded062251..378c5a416 100644 --- a/consensus/src/pipeline/virtual_processor/processor.rs +++ b/consensus/src/pipeline/virtual_processor/processor.rs @@ -48,6 +48,7 @@ use crate::{ }; use kaspa_consensus_core::{ acceptance_data::AcceptanceData, + api::args::{TransactionBatchValidationArgs, TransactionValidationArgs}, block::{BlockTemplate, MutableBlock, TemplateBuildMode, TemplateTransactionSelector}, blockstatus::BlockStatus::{StatusDisqualifiedFromChain, StatusUTXOValid}, coinbase::MinerData, @@ -757,23 +758,28 @@ impl VirtualStateProcessor { virtual_utxo_view: &impl UtxoView, virtual_daa_score: u64, virtual_past_median_time: u64, + args: &TransactionValidationArgs, ) -> TxResult<()> { self.transaction_validator.validate_tx_in_isolation(&mutable_tx.tx)?; self.transaction_validator.utxo_free_tx_validation(&mutable_tx.tx, virtual_daa_score, virtual_past_median_time)?; - self.validate_mempool_transaction_in_utxo_context(mutable_tx, virtual_utxo_view, virtual_daa_score)?; + self.validate_mempool_transaction_in_utxo_context(mutable_tx, virtual_utxo_view, virtual_daa_score, args)?; Ok(()) } - pub fn validate_mempool_transaction(&self, mutable_tx: &mut MutableTransaction) -> TxResult<()> { + pub fn validate_mempool_transaction(&self, mutable_tx: &mut MutableTransaction, args: &TransactionValidationArgs) -> TxResult<()> { let virtual_read = self.virtual_stores.read(); let virtual_state = virtual_read.state.get().unwrap(); let virtual_utxo_view = &virtual_read.utxo_set; let virtual_daa_score = virtual_state.daa_score; let virtual_past_median_time = virtual_state.past_median_time; - self.validate_mempool_transaction_impl(mutable_tx, virtual_utxo_view, virtual_daa_score, virtual_past_median_time) + self.validate_mempool_transaction_impl(mutable_tx, virtual_utxo_view, virtual_daa_score, virtual_past_median_time, args) } - pub fn validate_mempool_transactions_in_parallel(&self, mutable_txs: &mut [MutableTransaction]) -> Vec> { + pub fn validate_mempool_transactions_in_parallel( + &self, + mutable_txs: &mut [MutableTransaction], + args: &TransactionBatchValidationArgs, + ) -> Vec> { let virtual_read = self.virtual_stores.read(); let virtual_state = virtual_read.state.get().unwrap(); let virtual_utxo_view = &virtual_read.utxo_set; @@ -784,7 +790,13 @@ impl VirtualStateProcessor { mutable_txs .par_iter_mut() .map(|mtx| { - self.validate_mempool_transaction_impl(mtx, &virtual_utxo_view, virtual_daa_score, virtual_past_median_time) + self.validate_mempool_transaction_impl( + mtx, + &virtual_utxo_view, + virtual_daa_score, + virtual_past_median_time, + args.get(&mtx.id()), + ) }) .collect::>>() }) diff --git a/consensus/src/pipeline/virtual_processor/utxo_validation.rs b/consensus/src/pipeline/virtual_processor/utxo_validation.rs index 112976294..abb377244 100644 --- a/consensus/src/pipeline/virtual_processor/utxo_validation.rs +++ b/consensus/src/pipeline/virtual_processor/utxo_validation.rs @@ -15,6 +15,7 @@ use crate::{ }; use kaspa_consensus_core::{ acceptance_data::{AcceptedTxEntry, MergesetBlockAcceptanceData}, + api::args::TransactionValidationArgs, coinbase::*, hashing, header::Header, @@ -248,7 +249,8 @@ impl VirtualStateProcessor { } } let populated_tx = PopulatedTransaction::new(transaction, entries); - let res = self.transaction_validator.validate_populated_transaction_and_get_fee(&populated_tx, pov_daa_score, flags); + let res = + self.transaction_validator.validate_populated_transaction_and_get_fee(&populated_tx, pov_daa_score, flags, None, None); match res { Ok(calculated_fee) => Ok(ValidatedTransaction::new(populated_tx, calculated_fee)), Err(tx_rule_error) => { @@ -290,6 +292,7 @@ impl VirtualStateProcessor { mutable_tx: &mut MutableTransaction, utxo_view: &impl UtxoView, pov_daa_score: u64, + args: &TransactionValidationArgs, ) -> TxResult<()> { self.populate_mempool_transaction_in_utxo_context(mutable_tx, utxo_view)?; @@ -312,6 +315,8 @@ impl VirtualStateProcessor { &mutable_tx.as_verifiable(), pov_daa_score, TxValidationFlags::SkipMassCheck, // we can skip the mass check since we just set it + mutable_tx.calculated_compute_mass, + args.fee_per_mass_threshold, )?; mutable_tx.calculated_fee = Some(calculated_fee); Ok(()) diff --git a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs index 696b9a9d4..8b8a33618 100644 --- a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs +++ b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs @@ -27,10 +27,13 @@ impl TransactionValidator { tx: &impl VerifiableTransaction, pov_daa_score: u64, flags: TxValidationFlags, + compute_mass: Option, + fee_per_mass_threshold: Option, ) -> TxResult { self.check_transaction_coinbase_maturity(tx, pov_daa_score)?; let total_in = self.check_transaction_input_amounts(tx)?; let total_out = Self::check_transaction_output_values(tx, total_in)?; + let fee = total_in - total_out; if flags != TxValidationFlags::SkipMassCheck && pov_daa_score > self.storage_mass_activation_daa_score { // Storage mass hardfork was activated self.check_mass_commitment(tx)?; @@ -40,6 +43,7 @@ impl TransactionValidator { } } Self::check_sequence_lock(tx, pov_daa_score)?; + Self::check_fee_per_mass(fee, compute_mass, fee_per_mass_threshold)?; match flags { TxValidationFlags::Full | TxValidationFlags::SkipMassCheck => { Self::check_sig_op_counts(tx)?; @@ -47,7 +51,18 @@ impl TransactionValidator { } TxValidationFlags::SkipScriptChecks => {} } - Ok(total_in - total_out) + Ok(fee) + } + + fn check_fee_per_mass(fee: u64, mass: Option, threshold: Option) -> TxResult<()> { + // An actual check can only occur if both some mass and some threshold are provided, + // otherwise, the check does not verify anything and exits successfully. + if let (Some(compute_mass), Some(threshold)) = (mass, threshold) { + if fee as f64 / compute_mass as f64 <= threshold { + return Err(TxRuleError::FeePerMassTooLow); + } + } + Ok(()) } fn check_transaction_coinbase_maturity(&self, tx: &impl VerifiableTransaction, pov_daa_score: u64) -> TxResult<()> { diff --git a/mining/errors/src/mempool.rs b/mining/errors/src/mempool.rs index b7f83f511..be8ff389a 100644 --- a/mining/errors/src/mempool.rs +++ b/mining/errors/src/mempool.rs @@ -27,11 +27,11 @@ pub enum RuleError { #[error("output {0} already spent by transaction {1} in the mempool")] RejectDoubleSpendInMempool(TransactionOutpoint, TransactionId), - #[error("replace by fee found no replaceable transaction in the mempool")] + #[error("replace by fee found no double spending transaction in the mempool")] RejectRbfNoDoubleSpend, - #[error("replace by fee found more than one replaceable transaction in the mempool")] - RejectRbfTooManyDoubleSpends, + #[error("replace by fee found more than one double spending transaction in the mempool")] + RejectRbfTooManyDoubleSpendingTransactions, /// New behavior: a transaction is rejected if the mempool is full #[error("number of high-priority transactions in mempool ({0}) has reached the maximum allowed ({1})")] @@ -101,7 +101,7 @@ impl From for RuleError { pub type RuleResult = std::result::Result; -#[derive(Error, Debug, Clone)] +#[derive(Error, Debug, Clone, PartialEq, Eq)] pub enum NonStandardError { #[error("transaction version {1} is not in the valid range of {2}-{3}")] RejectVersion(TransactionId, u16, u16, u16), diff --git a/mining/src/manager.rs b/mining/src/manager.rs index 85bf93bca..b86359329 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -4,7 +4,7 @@ use crate::{ errors::MiningManagerResult, mempool::{ config::Config, - model::tx::{MempoolTransaction, RbfPolicy, TransactionInsertOutcome, TxRemovalReason}, + model::tx::{MempoolTransaction, RbfPolicy, TransactionPostValidation, TransactionPreValidation, TxRemovalReason}, populate_entries_and_try_validate::{ populate_mempool_transactions_in_parallel, validate_mempool_transaction, validate_mempool_transactions_in_parallel, }, @@ -22,7 +22,10 @@ use crate::{ }; use itertools::Itertools; use kaspa_consensus_core::{ - api::ConsensusApi, + api::{ + args::{TransactionBatchValidationArgs, TransactionValidationArgs}, + ConsensusApi, + }, block::{BlockTemplate, TemplateBuildMode}, coinbase::MinerData, errors::{block::RuleError as BlockRuleError, tx::TxRuleError}, @@ -272,13 +275,15 @@ impl MiningManager { rbf_policy: RbfPolicy, ) -> MiningManagerResult { // read lock on mempool - let mut transaction = self.mempool.read().pre_validate_and_populate_transaction(consensus, transaction, rbf_policy)?; + let TransactionPreValidation { mut transaction, fee_per_mass_threshold } = + self.mempool.read().pre_validate_and_populate_transaction(consensus, transaction, rbf_policy)?; + let args = TransactionValidationArgs::new(fee_per_mass_threshold); // no lock on mempool - let validation_result = validate_mempool_transaction(consensus, &mut transaction); + let validation_result = validate_mempool_transaction(consensus, &mut transaction, &args); // write lock on mempool let mut mempool = self.mempool.write(); match mempool.post_validate_and_insert_transaction(consensus, validation_result, transaction, priority, orphan, rbf_policy)? { - TransactionInsertOutcome { removed, accepted: Some(accepted_transaction) } => { + TransactionPostValidation { removed, accepted: Some(accepted_transaction) } => { let unorphaned_transactions = mempool.get_unorphaned_transactions_after_accepted_transaction(&accepted_transaction); drop(mempool); @@ -291,7 +296,7 @@ impl MiningManager { Ok(TransactionInsertion::new(removed, accepted_transactions)) } - TransactionInsertOutcome { removed, accepted: None } => Ok(TransactionInsertion::new(removed, vec![])), + TransactionPostValidation { removed, accepted: None } => Ok(TransactionInsertion::new(removed, vec![])), } } @@ -302,6 +307,8 @@ impl MiningManager { ) -> Vec> { // The capacity used here may be exceeded (see next comment). let mut accepted_transactions = Vec::with_capacity(incoming_transactions.len()); + // The validation args map is immutably empty since unorphaned transactions are not eligible to replace by fee. + let args = TransactionBatchValidationArgs::new(); // We loop as long as incoming unorphaned transactions do unorphan other transactions when they // get validated and inserted into the mempool. while !incoming_transactions.is_empty() { @@ -316,8 +323,11 @@ impl MiningManager { let mut validation_results = Vec::with_capacity(transactions.len()); while let Some(upper_bound) = self.next_transaction_chunk_upper_bound(&transactions, lower_bound) { assert!(lower_bound < upper_bound, "the chunk is never empty"); - validation_results - .extend(validate_mempool_transactions_in_parallel(consensus, &mut transactions[lower_bound..upper_bound])); + validation_results.extend(validate_mempool_transactions_in_parallel( + consensus, + &mut transactions[lower_bound..upper_bound], + &args, + )); lower_bound = upper_bound; } assert_eq!(transactions.len(), validation_results.len(), "every transaction should have a matching validation result"); @@ -338,12 +348,12 @@ impl MiningManager { Orphan::Forbidden, RbfPolicy::Forbidden, ) { - Ok(TransactionInsertOutcome { removed: _, accepted: Some(accepted_transaction) }) => { + Ok(TransactionPostValidation { removed: _, accepted: Some(accepted_transaction) }) => { accepted_transactions.push(accepted_transaction.clone()); self.counters.increase_tx_counts(1, priority); mempool.get_unorphaned_transactions_after_accepted_transaction(&accepted_transaction) } - Ok(TransactionInsertOutcome { removed: _, accepted: None }) => vec![], + Ok(TransactionPostValidation { removed: _, accepted: None }) => vec![], Err(err) => { debug!("Failed to unorphan transaction {0} due to rule error: {1}", orphan_id, err); vec![] @@ -380,12 +390,18 @@ impl MiningManager { // read lock on mempool // Here, we simply log and drop all erroneous transactions since the caller doesn't care about those anyway let mut transactions = Vec::with_capacity(sorted_transactions.len()); + let mut args = TransactionBatchValidationArgs::new(); for chunk in &sorted_transactions.chunks(TRANSACTION_CHUNK_SIZE) { let mempool = self.mempool.read(); let txs = chunk.filter_map(|tx| { let transaction_id = tx.id(); match mempool.pre_validate_and_populate_transaction(consensus, tx, RbfPolicy::Allowed) { - Ok(tx) => Some(tx), + Ok(TransactionPreValidation { transaction, fee_per_mass_threshold }) => { + if let Some(threshold) = fee_per_mass_threshold { + args.set_fee_per_mass_threshold(transaction.id(), threshold); + } + Some(transaction) + } Err(RuleError::RejectAlreadyAccepted(transaction_id)) => { debug!("Ignoring already accepted transaction {}", transaction_id); None @@ -414,8 +430,11 @@ impl MiningManager { let mut validation_results = Vec::with_capacity(transactions.len()); while let Some(upper_bound) = self.next_transaction_chunk_upper_bound(&transactions, lower_bound) { assert!(lower_bound < upper_bound, "the chunk is never empty"); - validation_results - .extend(validate_mempool_transactions_in_parallel(consensus, &mut transactions[lower_bound..upper_bound])); + validation_results.extend(validate_mempool_transactions_in_parallel( + consensus, + &mut transactions[lower_bound..upper_bound], + &args, + )); lower_bound = upper_bound; } assert_eq!(transactions.len(), validation_results.len(), "every transaction should have a matching validation result"); @@ -435,12 +454,12 @@ impl MiningManager { orphan, RbfPolicy::Allowed, ) { - Ok(TransactionInsertOutcome { removed: _, accepted: Some(accepted_transaction) }) => { + Ok(TransactionPostValidation { removed: _, accepted: Some(accepted_transaction) }) => { insert_results.push(Ok(accepted_transaction.clone())); self.counters.increase_tx_counts(1, priority); mempool.get_unorphaned_transactions_after_accepted_transaction(&accepted_transaction) } - Ok(TransactionInsertOutcome { removed: _, accepted: None }) => { + Ok(TransactionPostValidation { removed: _, accepted: None }) => { // Either orphaned or already existing in the mempool vec![] } diff --git a/mining/src/manager_tests.rs b/mining/src/manager_tests.rs index 5cccbbb76..58069f022 100644 --- a/mining/src/manager_tests.rs +++ b/mining/src/manager_tests.rs @@ -20,7 +20,7 @@ mod tests { block::TemplateBuildMode, coinbase::MinerData, constants::{MAX_TX_IN_SEQUENCE_NUM, SOMPI_PER_KASPA, TX_VERSION}, - errors::tx::{TxResult, TxRuleError}, + errors::tx::TxRuleError, mass::transaction_estimated_serialized_size, subnets::SUBNETWORK_ID_NATIVE, tx::{ @@ -29,6 +29,7 @@ mod tests { }, }; use kaspa_hashes::Hash; + use kaspa_mining_errors::mempool::RuleResult; use kaspa_txscript::{ pay_to_address_script, pay_to_script_hash_signature_script, test_helpers::{create_transaction, op_true_script}, @@ -158,11 +159,15 @@ mod tests { // when the mempool will submit this transaction for validation. let mut transaction = create_transaction_with_utxo_entry(0, 1); Arc::make_mut(&mut transaction.tx).gas = 1000; - let status = Err(TxRuleError::TxHasGas); - consensus.set_status(transaction.id(), status.clone()); + let tx_err = TxRuleError::TxHasGas; + let expected = match rbf_policy { + RbfPolicy::Forbidden | RbfPolicy::Allowed => Err(RuleError::from(tx_err.clone())), + RbfPolicy::Mandatory => Err(RuleError::RejectRbfNoDoubleSpend), + }; + consensus.set_status(transaction.id(), Err(tx_err)); // Try validate and insert the transaction into the mempool - let result = into_status(mining_manager.validate_and_insert_mutable_transaction( + let result = into_mempool_result(mining_manager.validate_and_insert_mutable_transaction( consensus.as_ref(), transaction.clone(), priority, @@ -171,8 +176,8 @@ mod tests { )); assert_eq!( - status, result, - "({priority:?}, {orphan:?}, {rbf_policy:?}) unexpected result when trying to insert an invalid transaction: expected: {status:?}, got: {result:?}", + expected, result, + "({priority:?}, {orphan:?}, {rbf_policy:?}) unexpected result when trying to insert an invalid transaction: expected: {expected:?}, got: {result:?}", ); let pool_tx = mining_manager.get_transaction(&transaction.id(), TransactionQuery::All); assert!( @@ -207,26 +212,31 @@ mod tests { ); // submit the same transaction again to the mempool - let result = mining_manager.validate_and_insert_mutable_transaction( + let result = into_mempool_result(mining_manager.validate_and_insert_mutable_transaction( consensus.as_ref(), MutableTransaction::from_tx(transaction.tx.as_ref().clone()), priority, orphan, rbf_policy, - ); - assert!(result.is_err(), "({priority:?}, {orphan:?}, {rbf_policy:?}) mempool should refuse a double submit of the same transaction but accepts it"); - match RuleError::try_from(result.unwrap_err()).unwrap() { - RuleError::RejectDuplicate(transaction_id) => { + )); + match result { + Err(RuleError::RejectDuplicate(transaction_id)) => { assert_eq!( transaction.id(), transaction_id, - "({priority:?}, {orphan:?}, {rbf_policy:?}) the error returned by the mempool should include id {} but provides {}", + "({priority:?}, {orphan:?}, {rbf_policy:?}) the error returned by the mempool should include transaction id {} but provides {}", transaction.id(), transaction_id ); } - err => { - panic!("({priority:?}, {orphan:?}, {rbf_policy:?}) the error returned by the mempool should be RuleError::RejectDuplicate but is {err:?}"); + Err(err) => { + panic!( + "({priority:?}, {orphan:?}, {rbf_policy:?}) the error returned by the mempool should be {:?} but is {err:?}", + RuleError::RejectDuplicate(transaction.id()) + ); + } + Ok(()) => { + panic!("({priority:?}, {orphan:?}, {rbf_policy:?}) mempool should refuse a double submit of the same transaction but accepts it"); } } } @@ -259,20 +269,15 @@ mod tests { double_spending_transaction.id(), "({priority:?}, {orphan:?}, {rbf_policy:?}) two transactions differing by only one output value should have different ids" ); - let result = mining_manager.validate_and_insert_mutable_transaction( + let result = into_mempool_result(mining_manager.validate_and_insert_mutable_transaction( consensus.as_ref(), MutableTransaction::from_tx(double_spending_transaction.clone()), priority, orphan, rbf_policy, - ); - assert!( - result.is_err(), - "({priority:?}, {orphan:?}, {rbf_policy:?}) mempool should refuse a double spend transaction but accepts it" - ); - let err = RuleError::try_from(result.unwrap_err()).unwrap(); - match err { - RuleError::RejectDoubleSpendInMempool(_, transaction_id) => { + )); + match result { + Err(RuleError::RejectDoubleSpendInMempool(_, transaction_id)) => { assert_eq!( transaction.id(), transaction_id, @@ -281,9 +286,12 @@ mod tests { transaction_id ); } - err => { + Err(err) => { panic!("({priority:?}, {orphan:?}, {rbf_policy:?}) the error returned by the mempool should be RuleError::RejectDoubleSpendInMempool but is {err:?}"); } + Ok(()) => { + panic!("({priority:?}, {orphan:?}, {rbf_policy:?}) mempool should refuse a double spend transaction ineligible to RBF but accepts it"); + } } } } @@ -1064,11 +1072,13 @@ mod tests { transactions.iter().any(|x| x.as_ref().id() == transaction_id) } - fn into_status(result: MiningManagerResult) -> TxResult<()> { + fn into_mempool_result(result: MiningManagerResult) -> RuleResult<()> { match result { Ok(_) => Ok(()), - Err(MiningManagerError::MempoolError(RuleError::RejectTxRule(err))) => Err(err), - _ => Ok(()), + Err(MiningManagerError::MempoolError(err)) => Err(err), + _ => { + panic!("result is an unsupported error"); + } } } diff --git a/mining/src/mempool/mod.rs b/mining/src/mempool/mod.rs index a3e26e7c9..89d141866 100644 --- a/mining/src/mempool/mod.rs +++ b/mining/src/mempool/mod.rs @@ -23,6 +23,7 @@ pub(crate) mod handle_new_block_transactions; pub(crate) mod model; pub(crate) mod populate_entries_and_try_validate; pub(crate) mod remove_transaction; +pub(crate) mod replace_by_fee; pub(crate) mod validate_and_insert_transaction; /// Mempool contains transactions intended to be inserted into a block and mined. diff --git a/mining/src/mempool/model/transactions_pool.rs b/mining/src/mempool/model/transactions_pool.rs index 9523537c7..988da5ddd 100644 --- a/mining/src/mempool/model/transactions_pool.rs +++ b/mining/src/mempool/model/transactions_pool.rs @@ -246,12 +246,14 @@ impl TransactionsPool { self.utxo_set.get_outpoint_owner_id(outpoint) } + /// Make sure no other transaction in the mempool is already spending an output which one of this transaction inputs spends pub(crate) fn check_double_spends(&self, transaction: &MutableTransaction) -> RuleResult<()> { self.utxo_set.check_double_spends(transaction) } - pub(crate) fn get_double_spends(&self, transaction: &MutableTransaction) -> Vec { - self.utxo_set.get_double_spends(transaction) + /// Returns the first double spend of every transaction in the mempool double spending on `transaction` + pub(crate) fn get_double_spend_transaction_ids(&self, transaction: &MutableTransaction) -> Vec { + self.utxo_set.get_double_spend_transaction_ids(transaction) } pub(crate) fn get_double_spend_owner<'a>(&'a self, double_spend: &DoubleSpend) -> RuleResult<&'a MempoolTransaction> { diff --git a/mining/src/mempool/model/tx.rs b/mining/src/mempool/model/tx.rs index 3585c3954..e20558dbf 100644 --- a/mining/src/mempool/model/tx.rs +++ b/mining/src/mempool/model/tx.rs @@ -82,7 +82,7 @@ pub(crate) enum RbfPolicy { /// - on double spends, the incoming transaction has a higher fee/mass ratio than the mempool transaction owning /// the first double spend /// - /// If conditions are not met, leaves the mempool unchanged and fails with a double spend error. + /// If conditions are not met, leaves the mempool unchanged and fails with a double spend or a tx fee/mass too low error. Allowed, /// ### RBF must occur @@ -96,10 +96,9 @@ pub(crate) enum RbfPolicy { /// /// - at least one double spend /// - all double spends belong to the same mempool transaction - /// - the incoming transaction has a higher fee/mass ratio than the mempool transaction owning - /// the double spends + /// - the incoming transaction has a higher fee/mass ratio than the mempool double spending transaction. /// - /// If conditions are not met, leaves the mempool unchanged and fails with a double spend error. + /// If conditions are not met, leaves the mempool unchanged and fails with a double spend or a tx fee/mass too low error. Mandatory, } @@ -137,18 +136,17 @@ impl From<&DoubleSpend> for RuleError { } } +pub(crate) struct TransactionPreValidation { + pub transaction: MutableTransaction, + pub fee_per_mass_threshold: Option, +} + #[derive(Default)] -pub(crate) struct TransactionInsertOutcome { +pub(crate) struct TransactionPostValidation { pub removed: Option>, pub accepted: Option>, } -impl TransactionInsertOutcome { - pub(crate) fn new(removed: Option>, accepted: Option>) -> Self { - Self { removed, accepted } - } -} - #[derive(PartialEq, Eq)] pub(crate) enum TxRemovalReason { Muted, diff --git a/mining/src/mempool/model/utxo_set.rs b/mining/src/mempool/model/utxo_set.rs index dd75fdb97..403a13ce5 100644 --- a/mining/src/mempool/model/utxo_set.rs +++ b/mining/src/mempool/model/utxo_set.rs @@ -90,7 +90,8 @@ impl MempoolUtxoSet { None } - pub(crate) fn get_double_spends(&self, transaction: &MutableTransaction) -> Vec { + /// Returns the first double spend of every transaction in the mempool double spending on `transaction` + pub(crate) fn get_double_spend_transaction_ids(&self, transaction: &MutableTransaction) -> Vec { let transaction_id = transaction.id(); let mut double_spends = vec![]; let mut visited = HashSet::new(); diff --git a/mining/src/mempool/populate_entries_and_try_validate.rs b/mining/src/mempool/populate_entries_and_try_validate.rs index 0c0dcf9a1..0f4f7de44 100644 --- a/mining/src/mempool/populate_entries_and_try_validate.rs +++ b/mining/src/mempool/populate_entries_and_try_validate.rs @@ -1,5 +1,12 @@ use crate::mempool::{errors::RuleResult, model::pool::Pool, Mempool}; -use kaspa_consensus_core::{api::ConsensusApi, constants::UNACCEPTED_DAA_SCORE, tx::MutableTransaction, tx::UtxoEntry}; +use kaspa_consensus_core::{ + api::{ + args::{TransactionBatchValidationArgs, TransactionValidationArgs}, + ConsensusApi, + }, + constants::UNACCEPTED_DAA_SCORE, + tx::{MutableTransaction, UtxoEntry}, +}; use kaspa_mining_errors::mempool::RuleError; impl Mempool { @@ -14,15 +21,20 @@ impl Mempool { } } -pub(crate) fn validate_mempool_transaction(consensus: &dyn ConsensusApi, transaction: &mut MutableTransaction) -> RuleResult<()> { - Ok(consensus.validate_mempool_transaction(transaction)?) +pub(crate) fn validate_mempool_transaction( + consensus: &dyn ConsensusApi, + transaction: &mut MutableTransaction, + args: &TransactionValidationArgs, +) -> RuleResult<()> { + Ok(consensus.validate_mempool_transaction(transaction, args)?) } pub(crate) fn validate_mempool_transactions_in_parallel( consensus: &dyn ConsensusApi, transactions: &mut [MutableTransaction], + args: &TransactionBatchValidationArgs, ) -> Vec> { - consensus.validate_mempool_transactions_in_parallel(transactions).into_iter().map(|x| x.map_err(RuleError::from)).collect() + consensus.validate_mempool_transactions_in_parallel(transactions, args).into_iter().map(|x| x.map_err(RuleError::from)).collect() } pub(crate) fn populate_mempool_transactions_in_parallel( diff --git a/mining/src/mempool/replace_by_fee.rs b/mining/src/mempool/replace_by_fee.rs new file mode 100644 index 000000000..bcf77914e --- /dev/null +++ b/mining/src/mempool/replace_by_fee.rs @@ -0,0 +1,133 @@ +use crate::mempool::{ + errors::{RuleError, RuleResult}, + model::tx::{DoubleSpend, MempoolTransaction, RbfPolicy, TxRemovalReason}, + Mempool, +}; +use kaspa_consensus_core::tx::{MutableTransaction, Transaction}; +use std::sync::Arc; + +impl Mempool { + /// Returns the replace by fee (RBF) constraint fee/mass threshold for an incoming transaction and a policy. + /// + /// Fails if the transaction does not meet some condition of the RBF policy, excluding the fee/mass condition. + /// + /// See [`RbfPolicy`] variants for details of each policy process and success conditions. + pub(super) fn get_replace_by_fee_constraint( + &self, + transaction: &mut MutableTransaction, + rbf_policy: RbfPolicy, + ) -> RuleResult> { + match rbf_policy { + RbfPolicy::Forbidden => { + // When RBF is forbidden, fails early on any double spend + self.transaction_pool.check_double_spends(transaction)?; + Ok(None) + } + + RbfPolicy::Allowed => { + // When RBF is allowed, never fails since both insertion and replacement are possible + let double_spends = self.transaction_pool.get_double_spend_transaction_ids(transaction); + if double_spends.is_empty() { + Ok(None) + } else { + let fee_per_mass_threshold = self.get_double_spend_fee_per_mass(&double_spends[0])?; + Ok(Some(fee_per_mass_threshold)) + } + } + + RbfPolicy::Mandatory => { + // When RBF is mandatory, fails early if we do not have exactly one double spending transaction + let double_spends = self.transaction_pool.get_double_spend_transaction_ids(transaction); + match double_spends.len() { + 0 => Err(RuleError::RejectRbfNoDoubleSpend), + 1 => { + let fee_per_mass_threshold = self.get_double_spend_fee_per_mass(&double_spends[0])?; + Ok(Some(fee_per_mass_threshold)) + } + _ => Err(RuleError::RejectRbfTooManyDoubleSpendingTransactions), + } + } + } + } + + /// Executes replace by fee (RBF) for an incoming transaction and a policy. + /// + /// See [`RbfPolicy`] variants for details of each policy process and success conditions. + /// + /// On success, `transaction` is guaranteed to embed no double spend with the mempool. + /// + /// On success with the [`RbfPolicy::Mandatory`] policy, some removed transaction is always returned. + pub(super) fn execute_replace_by_fee( + &mut self, + transaction: &MutableTransaction, + rbf_policy: RbfPolicy, + ) -> RuleResult>> { + match rbf_policy { + RbfPolicy::Forbidden => { + self.transaction_pool.check_double_spends(transaction)?; + Ok(None) + } + + RbfPolicy::Allowed => { + let double_spends = self.transaction_pool.get_double_spend_transaction_ids(transaction); + match double_spends.is_empty() { + true => Ok(None), + false => { + let removed = self.validate_double_spending_transaction(transaction, &double_spends[0])?.mtx.tx.clone(); + for double_spend in double_spends { + self.remove_transaction( + &double_spend.owner_id, + true, + TxRemovalReason::ReplacedByFee, + format!("by {}", transaction.id()).as_str(), + )?; + } + Ok(Some(removed)) + } + } + } + + RbfPolicy::Mandatory => { + let double_spends = self.transaction_pool.get_double_spend_transaction_ids(transaction); + match double_spends.len() { + 0 => Err(RuleError::RejectRbfNoDoubleSpend), + 1 => { + let removed = self.validate_double_spending_transaction(transaction, &double_spends[0])?.mtx.tx.clone(); + self.remove_transaction( + &double_spends[0].owner_id, + true, + TxRemovalReason::ReplacedByFee, + format!("by {}", transaction.id()).as_str(), + )?; + Ok(Some(removed)) + } + _ => Err(RuleError::RejectRbfTooManyDoubleSpendingTransactions), + } + } + } + } + + fn get_double_spend_fee_per_mass(&self, double_spend: &DoubleSpend) -> RuleResult { + let owner = self.transaction_pool.get_double_spend_owner(double_spend)?; + match owner.mtx.calculated_fee_per_compute_mass() { + Some(double_spend_ratio) => Ok(double_spend_ratio), + None => Err(double_spend.into()), + } + } + + fn validate_double_spending_transaction<'a>( + &'a self, + transaction: &MutableTransaction, + double_spend: &DoubleSpend, + ) -> RuleResult<&'a MempoolTransaction> { + let owner = self.transaction_pool.get_double_spend_owner(double_spend)?; + if let (Some(transaction_ratio), Some(double_spend_ratio)) = + (transaction.calculated_fee_per_compute_mass(), owner.mtx.calculated_fee_per_compute_mass()) + { + if transaction_ratio > double_spend_ratio { + return Ok(owner); + } + } + Err(double_spend.into()) + } +} diff --git a/mining/src/mempool/validate_and_insert_transaction.rs b/mining/src/mempool/validate_and_insert_transaction.rs index fb097149c..56300d8a6 100644 --- a/mining/src/mempool/validate_and_insert_transaction.rs +++ b/mining/src/mempool/validate_and_insert_transaction.rs @@ -2,7 +2,7 @@ use crate::mempool::{ errors::{RuleError, RuleResult}, model::{ pool::Pool, - tx::{DoubleSpend, MempoolTransaction, RbfPolicy, TransactionInsertOutcome, TxRemovalReason}, + tx::{MempoolTransaction, RbfPolicy, TransactionPostValidation, TransactionPreValidation, TxRemovalReason}, }, tx::{Orphan, Priority}, Mempool, @@ -13,7 +13,6 @@ use kaspa_consensus_core::{ tx::{MutableTransaction, Transaction, TransactionId, TransactionOutpoint, UtxoEntry}, }; use kaspa_core::{debug, info}; -use std::sync::Arc; impl Mempool { pub(crate) fn pre_validate_and_populate_transaction( @@ -21,19 +20,14 @@ impl Mempool { consensus: &dyn ConsensusApi, mut transaction: MutableTransaction, rbf_policy: RbfPolicy, - ) -> RuleResult { + ) -> RuleResult { self.validate_transaction_unacceptance(&transaction)?; // Populate mass in the beginning, it will be used in multiple places throughout the validation and insertion. transaction.calculated_compute_mass = Some(consensus.calculate_transaction_compute_mass(&transaction.tx)); self.validate_transaction_in_isolation(&transaction)?; - - // When replace by fee is forbidden, fail early on double spend - if rbf_policy == RbfPolicy::Forbidden { - self.transaction_pool.check_double_spends(&transaction)?; - } - + let fee_per_mass_threshold = self.get_replace_by_fee_constraint(&mut transaction, rbf_policy)?; self.populate_mempool_entries(&mut transaction); - Ok(transaction) + Ok(TransactionPreValidation { transaction, fee_per_mass_threshold }) } pub(crate) fn post_validate_and_insert_transaction( @@ -44,7 +38,7 @@ impl Mempool { priority: Priority, orphan: Orphan, rbf_policy: RbfPolicy, - ) -> RuleResult { + ) -> RuleResult { let transaction_id = transaction.id(); // First check if the transaction was not already added to the mempool. @@ -53,7 +47,7 @@ impl Mempool { // concurrently. if self.transaction_pool.has(&transaction_id) { debug!("Transaction {0} is not post validated since already in the mempool", transaction_id); - return Ok(TransactionInsertOutcome::default()); + return Ok(TransactionPostValidation::default()); } self.validate_transaction_unacceptance(&transaction)?; @@ -66,15 +60,15 @@ impl Mempool { } self.transaction_pool.check_double_spends(&transaction)?; self.orphan_pool.try_add_orphan(consensus.get_virtual_daa_score(), transaction, priority)?; - return Ok(TransactionInsertOutcome::default()); + return Ok(TransactionPostValidation::default()); } Err(err) => { return Err(err); } } - // Check double spends and remove them if the RBF policy requires to - let removed_transaction = self.validate_replace_by_fee(&transaction, rbf_policy)?; + // Check double spends and try to remove them if the RBF policy requires it + let removed_transaction = self.execute_replace_by_fee(&transaction, rbf_policy)?; self.validate_transaction_in_context(&transaction)?; @@ -86,77 +80,7 @@ impl Mempool { // Add the transaction to the mempool as a MempoolTransaction and return a clone of the embedded Arc let accepted_transaction = self.transaction_pool.add_transaction(transaction, consensus.get_virtual_daa_score(), priority)?.mtx.tx.clone(); - Ok(TransactionInsertOutcome::new(removed_transaction, Some(accepted_transaction))) - } - - /// Validates replace by fee (RBF) for an incoming transaction and a policy. - /// - /// See [`RbfPolicy`] variants for details of each policy process and success conditions. - /// - /// On success, `transaction` is guaranteed to have no double spend. - /// - /// On mandatory RBF success, some removed transaction is always returned. - fn validate_replace_by_fee( - &mut self, - transaction: &MutableTransaction, - rbf_policy: RbfPolicy, - ) -> RuleResult>> { - match rbf_policy { - RbfPolicy::Forbidden => { - self.transaction_pool.check_double_spends(transaction)?; - Ok(None) - } - - RbfPolicy::Allowed => { - let double_spends = self.transaction_pool.get_double_spends(transaction); - match double_spends.is_empty() { - true => Ok(None), - false => { - let removed = self.validate_double_spend_transaction(transaction, &double_spends[0])?.mtx.tx.clone(); - for double_spend in double_spends { - self.remove_transaction( - &double_spend.owner_id, - true, - TxRemovalReason::ReplacedByFee, - format!("by {}", transaction.id()).as_str(), - )?; - } - Ok(Some(removed)) - } - } - } - - RbfPolicy::Mandatory => { - let double_spends = self.transaction_pool.get_double_spends(transaction); - match double_spends.len() { - 0 => Err(RuleError::RejectRbfNoDoubleSpend), - 1 => { - let removed = self.validate_double_spend_transaction(transaction, &double_spends[0])?.mtx.tx.clone(); - self.remove_transaction( - &double_spends[0].owner_id, - true, - TxRemovalReason::ReplacedByFee, - format!("by {}", transaction.id()).as_str(), - )?; - Ok(Some(removed)) - } - _ => Err(RuleError::RejectRbfTooManyDoubleSpends), - } - } - } - } - - fn validate_double_spend_transaction<'a>( - &'a self, - transaction: &MutableTransaction, - double_spend: &DoubleSpend, - ) -> RuleResult<&'a MempoolTransaction> { - let owner = self.transaction_pool.get_double_spend_owner(double_spend)?; - if transaction.calculated_fee_per_compute_mass().unwrap() > owner.mtx.calculated_fee_per_compute_mass().unwrap() { - Ok(owner) - } else { - Err(double_spend.into()) - } + Ok(TransactionPostValidation { removed: removed_transaction, accepted: Some(accepted_transaction) }) } /// Validates that the transaction wasn't already accepted into the DAG diff --git a/mining/src/testutils/consensus_mock.rs b/mining/src/testutils/consensus_mock.rs index 94d774c42..ec9aedea9 100644 --- a/mining/src/testutils/consensus_mock.rs +++ b/mining/src/testutils/consensus_mock.rs @@ -1,6 +1,9 @@ use super::coinbase_mock::CoinbaseManagerMock; use kaspa_consensus_core::{ - api::ConsensusApi, + api::{ + args::{TransactionBatchValidationArgs, TransactionValidationArgs}, + ConsensusApi, + }, block::{BlockTemplate, MutableBlock, TemplateBuildMode, TemplateTransactionSelector, VirtualStateApproxId}, coinbase::MinerData, constants::BLOCK_VERSION, @@ -103,7 +106,7 @@ impl ConsensusApi for ConsensusMock { Ok(BlockTemplate::new(mutable_block, miner_data, coinbase.has_red_reward, now, 0, ZERO_HASH)) } - fn validate_mempool_transaction(&self, mutable_tx: &mut MutableTransaction) -> TxResult<()> { + fn validate_mempool_transaction(&self, mutable_tx: &mut MutableTransaction, _: &TransactionValidationArgs) -> TxResult<()> { // If a predefined status was registered to simulate an error, return it right away if let Some(status) = self.statuses.read().get(&mutable_tx.id()) { if status.is_err() { @@ -138,12 +141,16 @@ impl ConsensusApi for ConsensusMock { Ok(()) } - fn validate_mempool_transactions_in_parallel(&self, transactions: &mut [MutableTransaction]) -> Vec> { - transactions.iter_mut().map(|x| self.validate_mempool_transaction(x)).collect() + fn validate_mempool_transactions_in_parallel( + &self, + transactions: &mut [MutableTransaction], + _: &TransactionBatchValidationArgs, + ) -> Vec> { + transactions.iter_mut().map(|x| self.validate_mempool_transaction(x, &Default::default())).collect() } fn populate_mempool_transactions_in_parallel(&self, transactions: &mut [MutableTransaction]) -> Vec> { - transactions.iter_mut().map(|x| self.validate_mempool_transaction(x)).collect() + transactions.iter_mut().map(|x| self.validate_mempool_transaction(x, &Default::default())).collect() } fn calculate_transaction_compute_mass(&self, transaction: &Transaction) -> u64 { From d95dfdb2af56479d1621821e628de02f2af8d203 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Mon, 8 Jul 2024 14:06:47 +0000 Subject: [PATCH 11/30] Cleaning --- mining/errors/src/manager.rs | 11 ----------- mining/src/manager_tests.rs | 6 +++--- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/mining/errors/src/manager.rs b/mining/errors/src/manager.rs index 7b16b259c..24c313582 100644 --- a/mining/errors/src/manager.rs +++ b/mining/errors/src/manager.rs @@ -13,14 +13,3 @@ pub enum MiningManagerError { } pub type MiningManagerResult = std::result::Result; - -impl TryFrom for RuleError { - type Error = &'static str; - - fn try_from(value: MiningManagerError) -> Result { - match value { - MiningManagerError::BlockTemplateBuilderError(_) => Err("wrong variant"), - MiningManagerError::MempoolError(err) => Ok(err), - } - } -} diff --git a/mining/src/manager_tests.rs b/mining/src/manager_tests.rs index 58069f022..94c7f493b 100644 --- a/mining/src/manager_tests.rs +++ b/mining/src/manager_tests.rs @@ -51,20 +51,20 @@ mod tests { let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); let transactions_to_insert = (0..TX_COUNT).map(|i| create_transaction_with_utxo_entry(i, 0)).collect::>(); for transaction in transactions_to_insert.iter() { - let result = mining_manager.validate_and_insert_mutable_transaction( + let result = into_mempool_result(mining_manager.validate_and_insert_mutable_transaction( consensus.as_ref(), transaction.clone(), priority, orphan, rbf_policy, - ); + )); match rbf_policy { RbfPolicy::Forbidden | RbfPolicy::Allowed => { assert!(result.is_ok(), "({priority:?}, {orphan:?}, {rbf_policy:?}) inserting a valid transaction failed"); } RbfPolicy::Mandatory => { assert!(result.is_err(), "({priority:?}, {orphan:?}, {rbf_policy:?}) replacing a valid transaction without replacement in mempool should fail"); - let err: RuleError = result.unwrap_err().try_into().unwrap(); + let err = result.unwrap_err(); assert_eq!( RuleError::RejectRbfNoDoubleSpend, err, From 8cce2b40cff16e179c6e445598778049a25ca971 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Mon, 8 Jul 2024 14:42:56 +0000 Subject: [PATCH 12/30] More cleaning --- mining/src/manager.rs | 1 - mining/src/mempool/model/accepted_transactions.rs | 1 - mining/src/mempool/replace_by_fee.rs | 2 +- mining/src/mempool/validate_and_insert_transaction.rs | 2 +- 4 files changed, 2 insertions(+), 4 deletions(-) diff --git a/mining/src/manager.rs b/mining/src/manager.rs index b86359329..01755163e 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -445,7 +445,6 @@ impl MiningManager { let mut mempool = self.mempool.write(); let txs = chunk.flat_map(|(transaction, validation_result)| { let transaction_id = transaction.id(); - // FIXME: validate allowed RBF match mempool.post_validate_and_insert_transaction( consensus, validation_result, diff --git a/mining/src/mempool/model/accepted_transactions.rs b/mining/src/mempool/model/accepted_transactions.rs index 26dc0246b..94ad0d076 100644 --- a/mining/src/mempool/model/accepted_transactions.rs +++ b/mining/src/mempool/model/accepted_transactions.rs @@ -51,7 +51,6 @@ impl AcceptedTransactions { let expired_transactions: Vec = self .transactions - // FIXME: use .retain() instead .iter() .filter_map(|(transaction_id, daa_score)| { if virtual_daa_score > daa_score + self.config.accepted_transaction_expire_interval_daa_score { diff --git a/mining/src/mempool/replace_by_fee.rs b/mining/src/mempool/replace_by_fee.rs index bcf77914e..89f212254 100644 --- a/mining/src/mempool/replace_by_fee.rs +++ b/mining/src/mempool/replace_by_fee.rs @@ -14,7 +14,7 @@ impl Mempool { /// See [`RbfPolicy`] variants for details of each policy process and success conditions. pub(super) fn get_replace_by_fee_constraint( &self, - transaction: &mut MutableTransaction, + transaction: &MutableTransaction, rbf_policy: RbfPolicy, ) -> RuleResult> { match rbf_policy { diff --git a/mining/src/mempool/validate_and_insert_transaction.rs b/mining/src/mempool/validate_and_insert_transaction.rs index 56300d8a6..6c2ed5587 100644 --- a/mining/src/mempool/validate_and_insert_transaction.rs +++ b/mining/src/mempool/validate_and_insert_transaction.rs @@ -25,7 +25,7 @@ impl Mempool { // Populate mass in the beginning, it will be used in multiple places throughout the validation and insertion. transaction.calculated_compute_mass = Some(consensus.calculate_transaction_compute_mass(&transaction.tx)); self.validate_transaction_in_isolation(&transaction)?; - let fee_per_mass_threshold = self.get_replace_by_fee_constraint(&mut transaction, rbf_policy)?; + let fee_per_mass_threshold = self.get_replace_by_fee_constraint(&transaction, rbf_policy)?; self.populate_mempool_entries(&mut transaction); Ok(TransactionPreValidation { transaction, fee_per_mass_threshold }) } From ba3b75e436e5d69e2cf28f039b44345f12f2f1bb Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Tue, 9 Jul 2024 11:37:36 +0000 Subject: [PATCH 13/30] Use contextual instead of compute mass in RBF eviction rule --- consensus/core/src/tx.rs | 12 ++++++++---- .../pipeline/virtual_processor/utxo_validation.rs | 7 +++---- .../transaction_validator_populated.rs | 13 ++++++------- mining/src/mempool/replace_by_fee.rs | 4 ++-- 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/consensus/core/src/tx.rs b/consensus/core/src/tx.rs index a2008d7f1..eecc3c073 100644 --- a/consensus/core/src/tx.rs +++ b/consensus/core/src/tx.rs @@ -407,10 +407,14 @@ impl> MutableTransaction { } } - pub fn calculated_fee_per_compute_mass(&self) -> Option { - match (self.calculated_fee, self.calculated_compute_mass) { - (Some(fee), Some(compute_mass)) => Some(fee as f64 / compute_mass as f64), - _ => None, + /// Returns the calculated fee per contextual (compute & storage) mass ratio when + /// some calculated fee does exist and the contextual mass is greater than zero. + pub fn calculated_fee_per_mass(&self) -> Option { + let contextual_mass = self.tx.as_ref().mass(); + if contextual_mass > 0 { + self.calculated_fee.map(|fee| fee as f64 / contextual_mass as f64) + } else { + None } } } diff --git a/consensus/src/pipeline/virtual_processor/utxo_validation.rs b/consensus/src/pipeline/virtual_processor/utxo_validation.rs index abb377244..94e3ebc1e 100644 --- a/consensus/src/pipeline/virtual_processor/utxo_validation.rs +++ b/consensus/src/pipeline/virtual_processor/utxo_validation.rs @@ -249,8 +249,7 @@ impl VirtualStateProcessor { } } let populated_tx = PopulatedTransaction::new(transaction, entries); - let res = - self.transaction_validator.validate_populated_transaction_and_get_fee(&populated_tx, pov_daa_score, flags, None, None); + let res = self.transaction_validator.validate_populated_transaction_and_get_fee(&populated_tx, pov_daa_score, flags, None); match res { Ok(calculated_fee) => Ok(ValidatedTransaction::new(populated_tx, calculated_fee)), Err(tx_rule_error) => { @@ -311,12 +310,12 @@ impl VirtualStateProcessor { mutable_tx.tx.set_mass(contextual_mass); // At this point we know all UTXO entries are populated, so we can safely pass the tx as verifiable + let mass_and_fee_per_mass_threshold = args.fee_per_mass_threshold.map(|threshold| (contextual_mass, threshold)); let calculated_fee = self.transaction_validator.validate_populated_transaction_and_get_fee( &mutable_tx.as_verifiable(), pov_daa_score, TxValidationFlags::SkipMassCheck, // we can skip the mass check since we just set it - mutable_tx.calculated_compute_mass, - args.fee_per_mass_threshold, + mass_and_fee_per_mass_threshold, )?; mutable_tx.calculated_fee = Some(calculated_fee); Ok(()) diff --git a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs index 8b8a33618..279c4dfb0 100644 --- a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs +++ b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs @@ -27,8 +27,7 @@ impl TransactionValidator { tx: &impl VerifiableTransaction, pov_daa_score: u64, flags: TxValidationFlags, - compute_mass: Option, - fee_per_mass_threshold: Option, + mass_and_fee_per_mass_threshold: Option<(u64, f64)>, ) -> TxResult { self.check_transaction_coinbase_maturity(tx, pov_daa_score)?; let total_in = self.check_transaction_input_amounts(tx)?; @@ -43,7 +42,7 @@ impl TransactionValidator { } } Self::check_sequence_lock(tx, pov_daa_score)?; - Self::check_fee_per_mass(fee, compute_mass, fee_per_mass_threshold)?; + Self::check_fee_per_mass(fee, mass_and_fee_per_mass_threshold)?; match flags { TxValidationFlags::Full | TxValidationFlags::SkipMassCheck => { Self::check_sig_op_counts(tx)?; @@ -54,11 +53,11 @@ impl TransactionValidator { Ok(fee) } - fn check_fee_per_mass(fee: u64, mass: Option, threshold: Option) -> TxResult<()> { - // An actual check can only occur if both some mass and some threshold are provided, + fn check_fee_per_mass(fee: u64, mass_and_fee_per_mass_threshold: Option<(u64, f64)>) -> TxResult<()> { + // An actual check can only occur if some mass and threshold are provided, // otherwise, the check does not verify anything and exits successfully. - if let (Some(compute_mass), Some(threshold)) = (mass, threshold) { - if fee as f64 / compute_mass as f64 <= threshold { + if let Some((contextual_mass, threshold)) = mass_and_fee_per_mass_threshold { + if contextual_mass > 0 && fee as f64 / contextual_mass as f64 <= threshold { return Err(TxRuleError::FeePerMassTooLow); } } diff --git a/mining/src/mempool/replace_by_fee.rs b/mining/src/mempool/replace_by_fee.rs index 89f212254..466864aa5 100644 --- a/mining/src/mempool/replace_by_fee.rs +++ b/mining/src/mempool/replace_by_fee.rs @@ -109,7 +109,7 @@ impl Mempool { fn get_double_spend_fee_per_mass(&self, double_spend: &DoubleSpend) -> RuleResult { let owner = self.transaction_pool.get_double_spend_owner(double_spend)?; - match owner.mtx.calculated_fee_per_compute_mass() { + match owner.mtx.calculated_fee_per_mass() { Some(double_spend_ratio) => Ok(double_spend_ratio), None => Err(double_spend.into()), } @@ -122,7 +122,7 @@ impl Mempool { ) -> RuleResult<&'a MempoolTransaction> { let owner = self.transaction_pool.get_double_spend_owner(double_spend)?; if let (Some(transaction_ratio), Some(double_spend_ratio)) = - (transaction.calculated_fee_per_compute_mass(), owner.mtx.calculated_fee_per_compute_mass()) + (transaction.calculated_fee_per_mass(), owner.mtx.calculated_fee_per_mass()) { if transaction_ratio > double_spend_ratio { return Ok(owner); From 31b20a7b14a71b4d8a2e154c2edca4109cc46390 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Tue, 9 Jul 2024 15:46:52 +0000 Subject: [PATCH 14/30] Avoid collision with another PR --- crypto/txscript/src/standard.rs | 42 ++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/crypto/txscript/src/standard.rs b/crypto/txscript/src/standard.rs index fb7eb455a..16084ffe1 100644 --- a/crypto/txscript/src/standard.rs +++ b/crypto/txscript/src/standard.rs @@ -100,9 +100,9 @@ pub mod test_helpers { (script_public_key, redeem_script) } - // Creates a transaction that spends the first output of provided transaction. - // Assumes that the output being spent has opTrueScript as it's scriptPublicKey. - // Creates the value of the spent output minus provided `fee` (in sompi). + /// Creates a transaction that spends the first output of provided transaction. + /// Assumes that the output being spent has opTrueScript as its scriptPublicKey. + /// Creates the value of the spent output minus provided `fee` (in sompi). pub fn create_transaction(tx_to_spend: &Transaction, fee: u64) -> Transaction { let (script_public_key, redeem_script) = op_true_script(); let signature_script = pay_to_script_hash_signature_script(redeem_script, vec![]).expect("the script is canonical"); @@ -111,6 +111,42 @@ pub mod test_helpers { let output = TransactionOutput::new(tx_to_spend.outputs[0].value - fee, script_public_key); Transaction::new(TX_VERSION, vec![input], vec![output], 0, SUBNETWORK_ID_NATIVE, 0, vec![]) } + + /// Creates a transaction that spends the outputs of specified indexes (if they exist) of every provided transaction and returns an optional change. + /// Assumes that the outputs being spent have opTrueScript as their scriptPublicKey. + /// + /// If some change is provided, creates two outputs, first one with the value of the spent outputs minus `change` + /// and `fee` (in sompi) and second one of `change` amount. + /// + /// If no change is provided, creates only one output with the value of the spent outputs minus and `fee` (in sompi) + pub fn create_transaction_with_change<'a>( + txs_to_spend: impl Iterator, + output_indexes: Vec, + change: Option, + fee: u64, + ) -> Transaction { + let (script_public_key, redeem_script) = op_true_script(); + let signature_script = pay_to_script_hash_signature_script(redeem_script, vec![]).expect("the script is canonical"); + let mut inputs_value: u64 = 0; + let mut inputs = vec![]; + for tx_to_spend in txs_to_spend { + for i in output_indexes.iter().copied() { + if (i as usize) < tx_to_spend.outputs.len() { + let previous_outpoint = TransactionOutpoint::new(tx_to_spend.id(), i as u32); + inputs.push(TransactionInput::new(previous_outpoint, signature_script.clone(), MAX_TX_IN_SEQUENCE_NUM, 1)); + inputs_value += tx_to_spend.outputs[i].value; + } + } + } + let outputs = match change { + Some(change) => vec![ + TransactionOutput::new(inputs_value - fee - change, script_public_key.clone()), + TransactionOutput::new(change, script_public_key), + ], + None => vec![TransactionOutput::new(inputs_value - fee, script_public_key.clone())], + }; + Transaction::new(TX_VERSION, inputs, outputs, 0, SUBNETWORK_ID_NATIVE, 0, vec![]) + } } #[cfg(test)] From ce2f76d41ada73589ba65453ed14534e03fb926b Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Tue, 9 Jul 2024 16:02:40 +0000 Subject: [PATCH 15/30] Avoid collision with another PR (2) --- crypto/txscript/src/standard.rs | 2 +- rpc/grpc/core/proto/messages.proto | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crypto/txscript/src/standard.rs b/crypto/txscript/src/standard.rs index 16084ffe1..3c0f12a18 100644 --- a/crypto/txscript/src/standard.rs +++ b/crypto/txscript/src/standard.rs @@ -131,7 +131,7 @@ pub mod test_helpers { let mut inputs = vec![]; for tx_to_spend in txs_to_spend { for i in output_indexes.iter().copied() { - if (i as usize) < tx_to_spend.outputs.len() { + if i < tx_to_spend.outputs.len() { let previous_outpoint = TransactionOutpoint::new(tx_to_spend.id(), i as u32); inputs.push(TransactionInput::new(previous_outpoint, signature_script.clone(), MAX_TX_IN_SEQUENCE_NUM, 1)); inputs_value += tx_to_spend.outputs[i].value; diff --git a/rpc/grpc/core/proto/messages.proto b/rpc/grpc/core/proto/messages.proto index 9a761b388..0358f1d19 100644 --- a/rpc/grpc/core/proto/messages.proto +++ b/rpc/grpc/core/proto/messages.proto @@ -59,7 +59,7 @@ message KaspadRequest { GetServerInfoRequestMessage getServerInfoRequest = 1092; GetSyncStatusRequestMessage getSyncStatusRequest = 1094; GetDaaScoreTimestampEstimateRequestMessage getDaaScoreTimestampEstimateRequest = 1096; - SubmitTransactionReplacementRequestMessage submitTransactionReplacementRequest = 1098; + SubmitTransactionReplacementRequestMessage submitTransactionReplacementRequest = 2000; } } @@ -119,7 +119,7 @@ message KaspadResponse { GetServerInfoResponseMessage getServerInfoResponse = 1093; GetSyncStatusResponseMessage getSyncStatusResponse = 1095; GetDaaScoreTimestampEstimateResponseMessage getDaaScoreTimestampEstimateResponse = 1097; - SubmitTransactionReplacementResponseMessage submitTransactionReplacementResponse = 1099; + SubmitTransactionReplacementResponseMessage submitTransactionReplacementResponse = 2001; } } From 335f3d65f904ad28724d4a5ff47e0e5c886ddaa1 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Tue, 9 Jul 2024 22:30:18 +0000 Subject: [PATCH 16/30] Extended test coverage of RBF --- mining/src/manager_tests.rs | 339 +++++++++++++++++++++++++++++++----- 1 file changed, 300 insertions(+), 39 deletions(-) diff --git a/mining/src/manager_tests.rs b/mining/src/manager_tests.rs index 94c7f493b..b5d18af92 100644 --- a/mining/src/manager_tests.rs +++ b/mining/src/manager_tests.rs @@ -14,6 +14,7 @@ mod tests { testutils::consensus_mock::ConsensusMock, MiningCounters, }; + use itertools::Itertools; use kaspa_addresses::{Address, Prefix, Version}; use kaspa_consensus_core::{ api::ConsensusApi, @@ -32,9 +33,9 @@ mod tests { use kaspa_mining_errors::mempool::RuleResult; use kaspa_txscript::{ pay_to_address_script, pay_to_script_hash_signature_script, - test_helpers::{create_transaction, op_true_script}, + test_helpers::{create_transaction, create_transaction_with_change, op_true_script}, }; - use std::sync::Arc; + use std::{iter::once, sync::Arc}; use tokio::sync::mpsc::{error::TryRecvError, unbounded_channel}; const TARGET_TIME_PER_BLOCK: u64 = 1_000; @@ -242,8 +243,8 @@ mod tests { } } - // test_double_spend_in_mempool verifies that an attempt to insert a transaction double-spending - // another transaction already in the mempool will result in raising an appropriate error. + /// test_double_spend_in_mempool verifies that an attempt to insert a transaction double-spending + /// another transaction already in the mempool will result in raising an appropriate error. #[test] fn test_double_spend_in_mempool() { for (priority, orphan, rbf_policy) in all_priority_orphan_rbf_policy_combinations() { @@ -296,44 +297,238 @@ mod tests { } } - // test_replace_by_fee_in_mempool verifies that an attempt to insert a double-spending transaction with a higher fee - // will cause or not the existing transaction in the mempool to be replaced, depending on varying factors. + /// test_replace_by_fee_in_mempool verifies that an attempt to insert a double-spending transaction + /// will cause or not the transaction(s) double spending in the mempool to be replaced/removed, + /// depending on varying factors. #[test] fn test_replace_by_fee_in_mempool() { - let consensus = Arc::new(ConsensusMock::new()); - let counters = Arc::new(MiningCounters::default()); - let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); + const BASE_FEE: u64 = DEFAULT_MINIMUM_RELAY_TRANSACTION_FEE; + + struct TxOp { + /// Funding transaction indexes + tx: Vec, + /// Funding transaction output indexes + output: Vec, + /// Add a change output to the transaction + change: bool, + /// Transaction fee + fee: u64, + /// Children binary tree depth + depth: usize, + } - let transaction = create_child_and_parent_txs_and_add_parent_to_consensus(&consensus); - assert!( - consensus.can_finance_transaction(&MutableTransaction::from_tx(transaction.clone())), - "the consensus mock should have spendable UTXOs for the newly created transaction {}", - transaction.id() - ); + impl TxOp { + fn change(&self) -> Option { + self.change.then_some(900 * SOMPI_PER_KASPA) + } + } - let result = - mining_manager.validate_and_insert_transaction(consensus.as_ref(), transaction.clone(), Priority::Low, Orphan::Allowed); - assert!(result.is_ok(), "the mempool should accept a valid transaction when it is able to populate its UTXO entries"); + struct Test { + name: &'static str, + /// Initial transactions in the mempool + starts: Vec, + /// Replacement transaction submitted to the mempool + replacement: TxOp, + /// Expected RBF result for the 3 policies [Forbidden, Allowed, Mandatory] + expected: [bool; 3], + } - let mut double_spending_transaction = transaction.clone(); - double_spending_transaction.outputs[0].value -= 1; // increasing fee of transaction - double_spending_transaction.finalize(); - assert_ne!( - transaction.id(), - double_spending_transaction.id(), - "two transactions differing by only one output value should have different ids" - ); - let result = mining_manager.validate_and_replace_transaction(consensus.as_ref(), double_spending_transaction.clone()); - assert!(result.is_ok(), "mempool should accept a RBF transaction with higher fee but rejected it"); - let tx_insertion = result.unwrap(); - assert_eq!(tx_insertion.removed.as_ref().unwrap().id(), transaction.id(), "RBF should return the replaced transaction"); - assert!( - !mining_manager.has_transaction(&transaction.id(), TransactionQuery::All), - "replaced transaction is in the mempool while it should have been removed by RBF" - ); + impl Test { + fn run_rbf(&self, rbf_policy: RbfPolicy, expected: bool) { + let consensus = Arc::new(ConsensusMock::new()); + let counters = Arc::new(MiningCounters::default()); + let mining_manager = MiningManager::new(TARGET_TIME_PER_BLOCK, false, MAX_BLOCK_MASS, None, counters); + let funding_transactions = create_and_add_funding_transactions(&consensus, 10); + + // RPC submit the initial transactions + let (transactions, children): (Vec<_>, Vec<_>) = + self.starts + .iter() + .map(|tx_op| { + let transaction = create_funded_transaction( + select_transactions(&funding_transactions, &tx_op.tx), + tx_op.output.clone(), + tx_op.change(), + tx_op.fee, + ); + assert!( + consensus.can_finance_transaction(&MutableTransaction::from_tx(transaction.clone())), + "[{}, {:?}] the consensus should have spendable UTXOs for the newly created transaction {}", + self.name, rbf_policy, transaction.id() + ); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + transaction.clone(), + Priority::High, + Orphan::Allowed, + ); + assert!( + result.is_ok(), + "[{}, {:?}] the mempool should accept a valid transaction when it is able to populate its UTXO entries", + self.name, rbf_policy, + ); + let children = create_children_tree(&transaction, tx_op.depth); + let children_count = (2_usize.pow(tx_op.depth as u32) - 1) * transaction.outputs.len(); + assert_eq!( + children.len(), children_count, + "[{}, {:?}] a parent transaction with {} output(s) should generate a binary children tree of depth {} with {} children but got {}", + self.name, rbf_policy, transaction.outputs.len(), tx_op.depth, children_count, children.len(), + ); + validate_and_insert_transactions( + &mining_manager, + consensus.as_ref(), + children.iter(), + Priority::High, + Orphan::Allowed, + ); + (transaction, children) + }) + .unzip(); + + // RPC submit transaction replacement + let transaction_replacement = create_funded_transaction( + select_transactions(&funding_transactions, &self.replacement.tx), + self.replacement.output.clone(), + self.replacement.change(), + self.replacement.fee, + ); + assert!( + consensus.can_finance_transaction(&MutableTransaction::from_tx(transaction_replacement.clone())), + "[{}, {:?}] the consensus should have spendable UTXOs for the newly created transaction {}", + self.name, + rbf_policy, + transaction_replacement.id() + ); + let tx_count = mining_manager.transaction_count(TransactionQuery::TransactionsOnly); + let expected_tx_count = match expected { + true => tx_count + 1 - transactions.len() - children.iter().map(|x| x.len()).sum::(), + false => tx_count, + }; + let result = match rbf_policy { + RbfPolicy::Forbidden => mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + transaction_replacement.clone(), + Priority::High, + Orphan::Forbidden, + ), + RbfPolicy::Allowed => mining_manager.validate_and_insert_mutable_transaction( + consensus.as_ref(), + MutableTransaction::from_tx(transaction_replacement.clone()), + Priority::Low, + Orphan::Forbidden, + RbfPolicy::Allowed, + ), + RbfPolicy::Mandatory => { + mining_manager.validate_and_replace_transaction(consensus.as_ref(), transaction_replacement.clone()) + } + }; + if expected { + assert!(result.is_ok(), "[{}, {:?}] mempool should accept a RBF transaction", self.name, rbf_policy,); + let tx_insertion = result.unwrap(); + assert_eq!( + tx_insertion.removed.as_ref().unwrap().id(), + transactions[0].id(), + "[{}, {:?}] RBF should return the removed transaction", + self.name, + rbf_policy, + ); + transactions.iter().for_each(|x| { + assert!( + !mining_manager.has_transaction(&x.id(), TransactionQuery::All), + "[{}, {:?}] RBF replaced transaction should no longer be in the mempool", + self.name, + rbf_policy, + ); + }); + assert_transaction_count( + &mining_manager, + expected_tx_count, + &format!( + "[{}, {:?}] RBF should remove all chained transactions of the removed mempool transaction(s)", + self.name, rbf_policy + ), + ); + } else { + assert!(result.is_err(), "[{}, {:?}] mempool should reject the RBF transaction", self.name, rbf_policy); + transactions.iter().for_each(|x| { + assert!( + mining_manager.has_transaction(&x.id(), TransactionQuery::All), + "[{}, {:?}] RBF transaction target is no longer in the mempool", + self.name, + rbf_policy + ); + }); + assert_transaction_count( + &mining_manager, + expected_tx_count, + &format!("[{}, {:?}] a failing RBF should leave the mempool unchanged", self.name, rbf_policy), + ); + } + } + + fn run(&self) { + [RbfPolicy::Forbidden, RbfPolicy::Allowed, RbfPolicy::Mandatory].iter().copied().enumerate().for_each( + |(i, rbf_policy)| { + self.run_rbf(rbf_policy, self.expected[i]); + }, + ) + } + } + + let tests = vec![ + Test { + name: "1 input, 1 output <=> 1 input, 1 output, constant fee", + starts: vec![TxOp { tx: vec![0], output: vec![0], change: false, fee: BASE_FEE, depth: 0 }], + replacement: TxOp { tx: vec![0], output: vec![0], change: false, fee: BASE_FEE, depth: 0 }, + expected: [false, false, false], + }, + Test { + name: "1 input, 1 output <=> 1 input, 1 output, increased fee", + starts: vec![TxOp { tx: vec![0], output: vec![0], change: false, fee: BASE_FEE, depth: 0 }], + replacement: TxOp { tx: vec![0], output: vec![0], change: false, fee: BASE_FEE * 2, depth: 0 }, + expected: [false, true, true], + }, + Test { + name: "2 inputs, 2 outputs <=> 2 inputs, 2 outputs, increased fee", + starts: vec![TxOp { tx: vec![0, 1], output: vec![0], change: true, fee: BASE_FEE, depth: 2 }], + replacement: TxOp { tx: vec![0, 1], output: vec![0], change: true, fee: BASE_FEE * 2, depth: 0 }, + expected: [false, true, true], + }, + Test { + name: "4 inputs, 2 outputs <=> 2 inputs, 2 outputs, constant fee", + starts: vec![TxOp { tx: vec![0, 1], output: vec![0, 1], change: true, fee: BASE_FEE, depth: 2 }], + replacement: TxOp { tx: vec![0, 1], output: vec![0], change: true, fee: BASE_FEE, depth: 0 }, + expected: [false, true, true], + }, + Test { + name: "2 inputs, 2 outputs <=> 2 inputs, 1 output, constant fee", + starts: vec![TxOp { tx: vec![0, 1], output: vec![0], change: true, fee: BASE_FEE, depth: 2 }], + replacement: TxOp { tx: vec![0, 1], output: vec![0], change: false, fee: BASE_FEE, depth: 0 }, + expected: [false, true, true], + }, + Test { + name: "2 inputs, 2 outputs <=> 2 inputs, 1 output, constant fee, partial double spend overlap", + starts: vec![TxOp { tx: vec![0, 1], output: vec![0], change: true, fee: BASE_FEE, depth: 2 }], + replacement: TxOp { tx: vec![0, 2], output: vec![0], change: false, fee: BASE_FEE, depth: 0 }, + expected: [false, true, true], + }, + Test { + name: "(2 inputs, 2 outputs) * 2 <=> 4 inputs, 2 outputs, increased fee, 2 double spending mempool transactions", + starts: vec![ + TxOp { tx: vec![0, 1], output: vec![0], change: true, fee: BASE_FEE, depth: 2 }, + TxOp { tx: vec![0, 1], output: vec![1], change: true, fee: BASE_FEE, depth: 2 }, + ], + replacement: TxOp { tx: vec![0, 1], output: vec![0, 1], change: true, fee: BASE_FEE * 2, depth: 0 }, + expected: [false, true, false], + }, + ]; + + for test in tests { + test.run(); + } } - // test_handle_new_block_transactions verifies that all the transactions in the block were successfully removed from the mempool. + /// test_handle_new_block_transactions verifies that all the transactions in the block were successfully removed from the mempool. #[test] fn test_handle_new_block_transactions() { let consensus = Arc::new(ConsensusMock::new()); @@ -393,8 +588,8 @@ mod tests { } #[test] - // test_double_spend_with_block verifies that any transactions which are now double spends as a result of the block's new transactions - // will be removed from the mempool. + /// test_double_spend_with_block verifies that any transactions which are now double spends as a result of the block's new transactions + /// will be removed from the mempool. fn test_double_spend_with_block() { let consensus = Arc::new(ConsensusMock::new()); let counters = Arc::new(MiningCounters::default()); @@ -424,7 +619,7 @@ mod tests { ); } - // test_orphan_transactions verifies that a transaction could be a part of a new block template only if it's not an orphan. + /// test_orphan_transactions verifies that a transaction could be a part of a new block template only if it's not an orphan. #[test] fn test_orphan_transactions() { let consensus = Arc::new(ConsensusMock::new()); @@ -823,7 +1018,7 @@ mod tests { assert!(orphan_txs.is_empty(), "orphan pool should be empty"); } - // test_modify_block_template verifies that modifying a block template changes coinbase data correctly. + /// test_modify_block_template verifies that modifying a block template changes coinbase data correctly. #[test] fn test_modify_block_template() { let consensus = Arc::new(ConsensusMock::new()); @@ -1031,6 +1226,67 @@ mod tests { mutable_tx } + fn create_and_add_funding_transactions(consensus: &Arc, count: usize) -> Vec { + // Make the funding amounts always different so that funding txs have different ids + (0..count) + .map(|i| { + let funding_tx = create_transaction_without_input(vec![1_000 * SOMPI_PER_KASPA, 2_500 * SOMPI_PER_KASPA + i as u64]); + consensus.add_transaction(funding_tx.clone(), 1); + funding_tx + }) + .collect_vec() + } + + fn select_transactions<'a>(transactions: &'a [Transaction], indexes: &'a [usize]) -> impl Iterator { + indexes.iter().map(|i| &transactions[*i]) + } + + fn create_funded_transaction<'a>( + txs_to_spend: impl Iterator, + output_indexes: Vec, + change: Option, + fee: u64, + ) -> Transaction { + create_transaction_with_change(txs_to_spend, output_indexes, change, fee) + } + + fn create_children_tree(parent: &Transaction, depth: usize) -> Vec { + let mut tree = vec![]; + let root = [parent.clone()]; + let mut parents = &root[..]; + let mut first_child = 0; + for _ in 0..depth { + let mut children = vec![]; + for parent in parents { + children.extend(parent.outputs.iter().enumerate().map(|(i, output)| { + create_transaction_with_change( + once(parent), + vec![i], + Some(output.value / 2), + DEFAULT_MINIMUM_RELAY_TRANSACTION_FEE, + ) + })); + } + tree.extend(children); + parents = &tree[first_child..]; + first_child = tree.len() + } + tree + } + + fn validate_and_insert_transactions<'a>( + mining_manager: &MiningManager, + consensus: &dyn ConsensusApi, + transactions: impl Iterator, + priority: Priority, + orphan: Orphan, + ) { + transactions.for_each(|transaction| { + let result = mining_manager.validate_and_insert_transaction(consensus, transaction.clone(), priority, orphan); + assert!(result.is_ok(), "the mempool should accept a valid transaction when it is able to populate its UTXO entries"); + }); + } + fn create_arrays_of_parent_and_children_transactions( consensus: &Arc, count: usize, @@ -1117,4 +1373,9 @@ mod tests { }) }) } + + fn assert_transaction_count(mining_manager: &MiningManager, expected_count: usize, message: &str) { + let count = mining_manager.transaction_count(TransactionQuery::TransactionsOnly); + assert_eq!(expected_count, count, "{message} mempool transaction count: expected {}, got {}", expected_count, count); + } } From 4139e14b6701c582ded0760455d18d21b8effcff Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Tue, 9 Jul 2024 22:41:51 +0000 Subject: [PATCH 17/30] Extended test coverage of RBF (2) --- mining/src/manager_tests.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/mining/src/manager_tests.rs b/mining/src/manager_tests.rs index b5d18af92..932023b70 100644 --- a/mining/src/manager_tests.rs +++ b/mining/src/manager_tests.rs @@ -506,6 +506,18 @@ mod tests { replacement: TxOp { tx: vec![0, 1], output: vec![0], change: false, fee: BASE_FEE, depth: 0 }, expected: [false, true, true], }, + Test { + name: "2 inputs, 2 outputs <=> 4 inputs, 2 output, constant fee (MUST FAIL on fee/mass)", + starts: vec![TxOp { tx: vec![0, 1], output: vec![0], change: true, fee: BASE_FEE, depth: 2 }], + replacement: TxOp { tx: vec![0, 1], output: vec![0, 1], change: true, fee: BASE_FEE, depth: 0 }, + expected: [false, false, false], + }, + Test { + name: "2 inputs, 1 output <=> 4 inputs, 2 output, increased fee (MUST FAIL on fee/mass)", + starts: vec![TxOp { tx: vec![0, 1], output: vec![0], change: false, fee: BASE_FEE, depth: 2 }], + replacement: TxOp { tx: vec![0, 1], output: vec![0, 1], change: true, fee: BASE_FEE + 10, depth: 0 }, + expected: [false, false, false], + }, Test { name: "2 inputs, 2 outputs <=> 2 inputs, 1 output, constant fee, partial double spend overlap", starts: vec![TxOp { tx: vec![0, 1], output: vec![0], change: true, fee: BASE_FEE, depth: 2 }], @@ -513,7 +525,7 @@ mod tests { expected: [false, true, true], }, Test { - name: "(2 inputs, 2 outputs) * 2 <=> 4 inputs, 2 outputs, increased fee, 2 double spending mempool transactions", + name: "(2 inputs, 2 outputs) * 2 <=> 4 inputs, 2 outputs, increased fee, 2 double spending mempool transactions (MUST FAIL on Mandatory)", starts: vec![ TxOp { tx: vec![0, 1], output: vec![0], change: true, fee: BASE_FEE, depth: 2 }, TxOp { tx: vec![0, 1], output: vec![1], change: true, fee: BASE_FEE, depth: 2 }, From e4e23550888b0566cf47ad7cdbb4ca1f73d61487 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Thu, 11 Jul 2024 22:26:41 +0000 Subject: [PATCH 18/30] Rename `TransactionBatchValidationArgs` to `TransactionValidationBatchArgs` --- consensus/core/src/api/args.rs | 8 ++++---- consensus/core/src/api/mod.rs | 4 ++-- consensus/src/consensus/mod.rs | 4 ++-- consensus/src/pipeline/virtual_processor/processor.rs | 4 ++-- mining/src/manager.rs | 6 +++--- mining/src/mempool/populate_entries_and_try_validate.rs | 4 ++-- mining/src/testutils/consensus_mock.rs | 4 ++-- 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/consensus/core/src/api/args.rs b/consensus/core/src/api/args.rs index 214c6e220..88f857535 100644 --- a/consensus/core/src/api/args.rs +++ b/consensus/core/src/api/args.rs @@ -15,12 +15,12 @@ impl TransactionValidationArgs { } } -/// A struct provided to consensus for transactions validation processing calls -pub struct TransactionBatchValidationArgs { +/// A struct provided to consensus for transactions validation batch processing calls +pub struct TransactionValidationBatchArgs { tx_args: HashMap, } -impl TransactionBatchValidationArgs { +impl TransactionValidationBatchArgs { const DEFAULT_ARGS: TransactionValidationArgs = TransactionValidationArgs { fee_per_mass_threshold: None }; pub fn new() -> Self { @@ -40,7 +40,7 @@ impl TransactionBatchValidationArgs { } } -impl Default for TransactionBatchValidationArgs { +impl Default for TransactionValidationBatchArgs { fn default() -> Self { Self::new() } diff --git a/consensus/core/src/api/mod.rs b/consensus/core/src/api/mod.rs index f71649cd7..4d8fbc3b4 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, - api::args::{TransactionBatchValidationArgs, TransactionValidationArgs}, + api::args::{TransactionValidationArgs, TransactionValidationBatchArgs}, block::{Block, BlockTemplate, TemplateBuildMode, TemplateTransactionSelector, VirtualStateApproxId}, blockstatus::BlockStatus, coinbase::MinerData, @@ -74,7 +74,7 @@ pub trait ConsensusApi: Send + Sync { fn validate_mempool_transactions_in_parallel( &self, transactions: &mut [MutableTransaction], - args: &TransactionBatchValidationArgs, + args: &TransactionValidationBatchArgs, ) -> Vec> { unimplemented!() } diff --git a/consensus/src/consensus/mod.rs b/consensus/src/consensus/mod.rs index 06cbec783..fb86d0dab 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::{ - args::{TransactionBatchValidationArgs, TransactionValidationArgs}, + args::{TransactionValidationArgs, TransactionValidationBatchArgs}, stats::BlockCount, BlockValidationFutures, ConsensusApi, ConsensusStats, }, @@ -431,7 +431,7 @@ impl ConsensusApi for Consensus { fn validate_mempool_transactions_in_parallel( &self, transactions: &mut [MutableTransaction], - args: &TransactionBatchValidationArgs, + args: &TransactionValidationBatchArgs, ) -> Vec> { self.virtual_processor.validate_mempool_transactions_in_parallel(transactions, args) } diff --git a/consensus/src/pipeline/virtual_processor/processor.rs b/consensus/src/pipeline/virtual_processor/processor.rs index 378c5a416..bb1132fea 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, - api::args::{TransactionBatchValidationArgs, TransactionValidationArgs}, + api::args::{TransactionValidationArgs, TransactionValidationBatchArgs}, block::{BlockTemplate, MutableBlock, TemplateBuildMode, TemplateTransactionSelector}, blockstatus::BlockStatus::{StatusDisqualifiedFromChain, StatusUTXOValid}, coinbase::MinerData, @@ -778,7 +778,7 @@ impl VirtualStateProcessor { pub fn validate_mempool_transactions_in_parallel( &self, mutable_txs: &mut [MutableTransaction], - args: &TransactionBatchValidationArgs, + args: &TransactionValidationBatchArgs, ) -> Vec> { let virtual_read = self.virtual_stores.read(); let virtual_state = virtual_read.state.get().unwrap(); diff --git a/mining/src/manager.rs b/mining/src/manager.rs index 01755163e..387bd20e3 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -23,7 +23,7 @@ use crate::{ use itertools::Itertools; use kaspa_consensus_core::{ api::{ - args::{TransactionBatchValidationArgs, TransactionValidationArgs}, + args::{TransactionValidationArgs, TransactionValidationBatchArgs}, ConsensusApi, }, block::{BlockTemplate, TemplateBuildMode}, @@ -308,7 +308,7 @@ impl MiningManager { // The capacity used here may be exceeded (see next comment). let mut accepted_transactions = Vec::with_capacity(incoming_transactions.len()); // The validation args map is immutably empty since unorphaned transactions are not eligible to replace by fee. - let args = TransactionBatchValidationArgs::new(); + let args = TransactionValidationBatchArgs::new(); // We loop as long as incoming unorphaned transactions do unorphan other transactions when they // get validated and inserted into the mempool. while !incoming_transactions.is_empty() { @@ -390,7 +390,7 @@ impl MiningManager { // read lock on mempool // Here, we simply log and drop all erroneous transactions since the caller doesn't care about those anyway let mut transactions = Vec::with_capacity(sorted_transactions.len()); - let mut args = TransactionBatchValidationArgs::new(); + let mut args = TransactionValidationBatchArgs::new(); for chunk in &sorted_transactions.chunks(TRANSACTION_CHUNK_SIZE) { let mempool = self.mempool.read(); let txs = chunk.filter_map(|tx| { diff --git a/mining/src/mempool/populate_entries_and_try_validate.rs b/mining/src/mempool/populate_entries_and_try_validate.rs index 0f4f7de44..a5c125280 100644 --- a/mining/src/mempool/populate_entries_and_try_validate.rs +++ b/mining/src/mempool/populate_entries_and_try_validate.rs @@ -1,7 +1,7 @@ use crate::mempool::{errors::RuleResult, model::pool::Pool, Mempool}; use kaspa_consensus_core::{ api::{ - args::{TransactionBatchValidationArgs, TransactionValidationArgs}, + args::{TransactionValidationArgs, TransactionValidationBatchArgs}, ConsensusApi, }, constants::UNACCEPTED_DAA_SCORE, @@ -32,7 +32,7 @@ pub(crate) fn validate_mempool_transaction( pub(crate) fn validate_mempool_transactions_in_parallel( consensus: &dyn ConsensusApi, transactions: &mut [MutableTransaction], - args: &TransactionBatchValidationArgs, + args: &TransactionValidationBatchArgs, ) -> Vec> { consensus.validate_mempool_transactions_in_parallel(transactions, args).into_iter().map(|x| x.map_err(RuleError::from)).collect() } diff --git a/mining/src/testutils/consensus_mock.rs b/mining/src/testutils/consensus_mock.rs index ec9aedea9..4ec37ec82 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::{ - args::{TransactionBatchValidationArgs, TransactionValidationArgs}, + args::{TransactionValidationArgs, TransactionValidationBatchArgs}, ConsensusApi, }, block::{BlockTemplate, MutableBlock, TemplateBuildMode, TemplateTransactionSelector, VirtualStateApproxId}, @@ -144,7 +144,7 @@ impl ConsensusApi for ConsensusMock { fn validate_mempool_transactions_in_parallel( &self, transactions: &mut [MutableTransaction], - _: &TransactionBatchValidationArgs, + _: &TransactionValidationBatchArgs, ) -> Vec> { transactions.iter_mut().map(|x| self.validate_mempool_transaction(x, &Default::default())).collect() } From 9147abb349daf3430ecbdb08d827fdb2c784bb8b Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Thu, 11 Jul 2024 22:32:48 +0000 Subject: [PATCH 19/30] Add comments --- consensus/core/src/errors/tx.rs | 4 +++- .../transaction_validator/transaction_validator_populated.rs | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/consensus/core/src/errors/tx.rs b/consensus/core/src/errors/tx.rs index 790b37f48..6a2744f61 100644 --- a/consensus/core/src/errors/tx.rs +++ b/consensus/core/src/errors/tx.rs @@ -89,7 +89,9 @@ pub enum TxRuleError { #[error("calculated contextual mass (including storage mass) {0} is not equal to the committed mass field {1}")] WrongMass(u64, u64), - #[error("fee per compute mass ratio is not greater than the threshold")] + /// [`TxRuleError::FeePerMassTooLow`] is not a consensus error but a mempool error triggered by the + /// fee/mass RBF validation rule + #[error("fee per contextual mass ratio is not greater than the threshold")] FeePerMassTooLow, } diff --git a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs index 279c4dfb0..2e2b1f885 100644 --- a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs +++ b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs @@ -42,7 +42,11 @@ impl TransactionValidator { } } Self::check_sequence_lock(tx, pov_daa_score)?; + + // The following call is not a consensus check (it could not be one in the first place since it uses floating number) + // but rather a mempool Replace by Fee validation rule. It was placed here purposely for avoiding unneeded script checks. Self::check_fee_per_mass(fee, mass_and_fee_per_mass_threshold)?; + match flags { TxValidationFlags::Full | TxValidationFlags::SkipMassCheck => { Self::check_sig_op_counts(tx)?; From 96df37ea8a7f2f2cc5e85854ae4d3bdf0edc1401 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Thu, 11 Jul 2024 22:36:22 +0000 Subject: [PATCH 20/30] Assert instead of condition --- .../transaction_validator/transaction_validator_populated.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs index 2e2b1f885..c5157980a 100644 --- a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs +++ b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs @@ -61,7 +61,8 @@ impl TransactionValidator { // An actual check can only occur if some mass and threshold are provided, // otherwise, the check does not verify anything and exits successfully. if let Some((contextual_mass, threshold)) = mass_and_fee_per_mass_threshold { - if contextual_mass > 0 && fee as f64 / contextual_mass as f64 <= threshold { + assert!(contextual_mass > 0); + if fee as f64 / contextual_mass as f64 <= threshold { return Err(TxRuleError::FeePerMassTooLow); } } From 0cd7c10c0e5bb1a2fad3a9d376fd9b5fc741e569 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Fri, 12 Jul 2024 12:04:00 +0000 Subject: [PATCH 21/30] Add an `RbfPolicy` parameter to mining manager tx validate_and_insert_... fns --- mining/src/manager.rs | 76 ++++------- mining/src/manager_tests.rs | 126 ++++++++++++------ mining/src/mempool/mod.rs | 47 +++++++ mining/src/mempool/model/tx.rs | 49 +------ mining/src/mempool/replace_by_fee.rs | 3 +- .../validate_and_insert_transaction.rs | 4 +- protocol/flows/src/flow_context.rs | 17 ++- protocol/flows/src/v5/txrelay/flow.rs | 4 +- 8 files changed, 176 insertions(+), 150 deletions(-) diff --git a/mining/src/manager.rs b/mining/src/manager.rs index 387bd20e3..be9190d6f 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -4,11 +4,11 @@ use crate::{ errors::MiningManagerResult, mempool::{ config::Config, - model::tx::{MempoolTransaction, RbfPolicy, TransactionPostValidation, TransactionPreValidation, TxRemovalReason}, + model::tx::{MempoolTransaction, TransactionPostValidation, TransactionPreValidation, TxRemovalReason}, populate_entries_and_try_validate::{ populate_mempool_transactions_in_parallel, validate_mempool_transaction, validate_mempool_transactions_in_parallel, }, - tx::{Orphan, Priority}, + tx::{Orphan, Priority, RbfPolicy}, Mempool, }, model::{ @@ -216,7 +216,8 @@ impl MiningManager { /// adds it to the set of known transactions that have not yet been /// added to any block. /// - /// Replace by fee is strictly forbidden so any double spend returns an error. + /// The validation is constrained by a Replace by fee policy applied + /// to double spends in the mempool. For more information, see [`RbfPolicy`]. /// /// On success, returns transactions that where unorphaned following the insertion /// of the provided transaction. @@ -228,44 +229,14 @@ impl MiningManager { transaction: Transaction, priority: Priority, orphan: Orphan, + rbf_policy: RbfPolicy, ) -> MiningManagerResult { - self.validate_and_insert_mutable_transaction( - consensus, - MutableTransaction::from_tx(transaction), - priority, - orphan, - RbfPolicy::Forbidden, - ) - } - - /// validate_and_replace_transaction validates the given transaction, and - /// adds it to the set of known transactions that have not yet been - /// added to any block. - /// - /// Replace by Fee is mandatory, consequently if the provided transaction cannot not replace - /// an existing transaction in the mempool, an error is returned. - /// - /// On success, returns the replaced transaction and the transactions that where unorphaned - /// following the insertion of the provided transaction. - /// - /// The returned transactions are references of objects owned by the mempool. - pub fn validate_and_replace_transaction( - &self, - consensus: &dyn ConsensusApi, - transaction: Transaction, - ) -> MiningManagerResult { - self.validate_and_insert_mutable_transaction( - consensus, - MutableTransaction::from_tx(transaction), - Priority::High, - Orphan::Forbidden, - RbfPolicy::Mandatory, - ) + self.validate_and_insert_mutable_transaction(consensus, MutableTransaction::from_tx(transaction), priority, orphan, rbf_policy) } /// Exposed for tests only /// - /// Regular users should call `validate_and_insert_transaction` or `validate_and_replace_transaction` instead. + /// See `validate_and_insert_transaction` pub(crate) fn validate_and_insert_mutable_transaction( &self, consensus: &dyn ConsensusApi, @@ -369,6 +340,9 @@ impl MiningManager { /// Validates a batch of transactions, handling iteratively only the independent ones, and /// adds those to the set of known transactions that have not yet been added to any block. /// + /// The validation is constrained by a Replace by fee policy applied + /// to double spends in the mempool. For more information, see [`RbfPolicy`]. + /// /// Returns transactions that where unorphaned following the insertion of the provided /// transactions. The returned transactions are references of objects owned by the mempool. pub fn validate_and_insert_transaction_batch( @@ -377,6 +351,7 @@ impl MiningManager { transactions: Vec, priority: Priority, orphan: Orphan, + rbf_policy: RbfPolicy, ) -> Vec>> { const TRANSACTION_CHUNK_SIZE: usize = 250; @@ -395,7 +370,7 @@ impl MiningManager { let mempool = self.mempool.read(); let txs = chunk.filter_map(|tx| { let transaction_id = tx.id(); - match mempool.pre_validate_and_populate_transaction(consensus, tx, RbfPolicy::Allowed) { + match mempool.pre_validate_and_populate_transaction(consensus, tx, rbf_policy) { Ok(TransactionPreValidation { transaction, fee_per_mass_threshold }) => { if let Some(threshold) = fee_per_mass_threshold { args.set_fee_per_mass_threshold(transaction.id(), threshold); @@ -825,6 +800,9 @@ impl MiningManagerProxy { /// Validates a transaction and adds it to the set of known transactions that have not yet been /// added to any block. /// + /// The validation is constrained by a Replace by fee policy applied + /// to double spends in the mempool. For more information, see [`RbfPolicy`]. + /// /// The returned transactions are references of objects owned by the mempool. pub async fn validate_and_insert_transaction( self, @@ -832,25 +810,20 @@ impl MiningManagerProxy { transaction: Transaction, priority: Priority, orphan: Orphan, + rbf_policy: RbfPolicy, ) -> MiningManagerResult { - consensus.clone().spawn_blocking(move |c| self.inner.validate_and_insert_transaction(c, transaction, priority, orphan)).await - } - - /// Validates a transaction and adds it to the set of known transactions that have not yet been - /// added to any block. - /// - /// The returned transactions are references of objects owned by the mempool. - pub async fn validate_and_replace_transaction( - self, - consensus: &ConsensusProxy, - transaction: Transaction, - ) -> MiningManagerResult { - consensus.clone().spawn_blocking(move |c| self.inner.validate_and_replace_transaction(c, transaction)).await + consensus + .clone() + .spawn_blocking(move |c| self.inner.validate_and_insert_transaction(c, transaction, priority, orphan, rbf_policy)) + .await } /// Validates a batch of transactions, handling iteratively only the independent ones, and /// adds those to the set of known transactions that have not yet been added to any block. /// + /// The validation is constrained by a Replace by fee policy applied + /// to double spends in the mempool. For more information, see [`RbfPolicy`]. + /// /// Returns transactions that where unorphaned following the insertion of the provided /// transactions. The returned transactions are references of objects owned by the mempool. pub async fn validate_and_insert_transaction_batch( @@ -859,10 +832,11 @@ impl MiningManagerProxy { transactions: Vec, priority: Priority, orphan: Orphan, + rbf_policy: RbfPolicy, ) -> Vec>> { consensus .clone() - .spawn_blocking(move |c| self.inner.validate_and_insert_transaction_batch(c, transactions, priority, orphan)) + .spawn_blocking(move |c| self.inner.validate_and_insert_transaction_batch(c, transactions, priority, orphan, rbf_policy)) .await } diff --git a/mining/src/manager_tests.rs b/mining/src/manager_tests.rs index 932023b70..a308e5657 100644 --- a/mining/src/manager_tests.rs +++ b/mining/src/manager_tests.rs @@ -7,8 +7,7 @@ mod tests { mempool::{ config::{Config, DEFAULT_MINIMUM_RELAY_TRANSACTION_FEE}, errors::RuleError, - model::tx::RbfPolicy, - tx::{Orphan, Priority}, + tx::{Orphan, Priority, RbfPolicy}, }, model::{candidate_tx::CandidateTransaction, tx_query::TransactionQuery}, testutils::consensus_mock::ConsensusMock, @@ -131,6 +130,7 @@ mod tests { transaction_not_an_orphan.clone(), priority, orphan, + RbfPolicy::Forbidden, ); assert!( result.is_ok(), @@ -213,9 +213,9 @@ mod tests { ); // submit the same transaction again to the mempool - let result = into_mempool_result(mining_manager.validate_and_insert_mutable_transaction( + let result = into_mempool_result(mining_manager.validate_and_insert_transaction( consensus.as_ref(), - MutableTransaction::from_tx(transaction.tx.as_ref().clone()), + transaction.tx.as_ref().clone(), priority, orphan, rbf_policy, @@ -259,7 +259,13 @@ mod tests { transaction.id() ); - let result = mining_manager.validate_and_insert_transaction(consensus.as_ref(), transaction.clone(), priority, orphan); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + transaction.clone(), + priority, + orphan, + RbfPolicy::Forbidden, + ); assert!(result.is_ok(), "({priority:?}, {orphan:?}, {rbf_policy:?}) the mempool should accept a valid transaction when it is able to populate its UTXO entries"); let mut double_spending_transaction = transaction.clone(); @@ -270,9 +276,9 @@ mod tests { double_spending_transaction.id(), "({priority:?}, {orphan:?}, {rbf_policy:?}) two transactions differing by only one output value should have different ids" ); - let result = into_mempool_result(mining_manager.validate_and_insert_mutable_transaction( + let result = into_mempool_result(mining_manager.validate_and_insert_transaction( consensus.as_ref(), - MutableTransaction::from_tx(double_spending_transaction.clone()), + double_spending_transaction.clone(), priority, orphan, rbf_policy, @@ -361,6 +367,7 @@ mod tests { transaction.clone(), Priority::High, Orphan::Allowed, + RbfPolicy::Forbidden, ); assert!( result.is_ok(), @@ -380,6 +387,7 @@ mod tests { children.iter(), Priority::High, Orphan::Allowed, + RbfPolicy::Forbidden, ); (transaction, children) }) @@ -404,24 +412,17 @@ mod tests { true => tx_count + 1 - transactions.len() - children.iter().map(|x| x.len()).sum::(), false => tx_count, }; - let result = match rbf_policy { - RbfPolicy::Forbidden => mining_manager.validate_and_insert_transaction( - consensus.as_ref(), - transaction_replacement.clone(), - Priority::High, - Orphan::Forbidden, - ), - RbfPolicy::Allowed => mining_manager.validate_and_insert_mutable_transaction( - consensus.as_ref(), - MutableTransaction::from_tx(transaction_replacement.clone()), - Priority::Low, - Orphan::Forbidden, - RbfPolicy::Allowed, - ), - RbfPolicy::Mandatory => { - mining_manager.validate_and_replace_transaction(consensus.as_ref(), transaction_replacement.clone()) - } + let priority = match rbf_policy { + RbfPolicy::Forbidden | RbfPolicy::Mandatory => Priority::High, + RbfPolicy::Allowed => Priority::Low, }; + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + transaction_replacement.clone(), + priority, + Orphan::Forbidden, + rbf_policy, + ); if expected { assert!(result.is_ok(), "[{}, {:?}] mempool should accept a RBF transaction", self.name, rbf_policy,); let tx_insertion = result.unwrap(); @@ -555,6 +556,7 @@ mod tests { transaction.tx.as_ref().clone(), Priority::Low, Orphan::Allowed, + RbfPolicy::Forbidden, ); assert!(result.is_ok(), "the insertion of a new valid transaction in the mempool failed"); } @@ -613,6 +615,7 @@ mod tests { transaction_in_the_mempool.tx.as_ref().clone(), Priority::Low, Orphan::Allowed, + RbfPolicy::Forbidden, ); assert!(result.is_ok()); @@ -645,8 +648,13 @@ mod tests { assert_eq!(parent_txs.len(), TX_PAIRS_COUNT); assert_eq!(child_txs.len(), TX_PAIRS_COUNT); for orphan in child_txs.iter() { - let result = - mining_manager.validate_and_insert_transaction(consensus.as_ref(), orphan.clone(), Priority::Low, Orphan::Allowed); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + orphan.clone(), + Priority::Low, + Orphan::Allowed, + RbfPolicy::Forbidden, + ); assert!(result.is_ok(), "the mempool should accept the valid orphan transaction {}", orphan.id()); } let (populated_txs, orphans) = mining_manager.get_all_transactions(TransactionQuery::All); @@ -790,8 +798,13 @@ mod tests { ); // Add the remaining parent transaction into the mempool - let result = - mining_manager.validate_and_insert_transaction(consensus.as_ref(), parent_txs[0].clone(), Priority::Low, Orphan::Allowed); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + parent_txs[0].clone(), + Priority::Low, + Orphan::Allowed, + RbfPolicy::Forbidden, + ); assert!(result.is_ok(), "the insertion of the remaining parent transaction in the mempool failed"); let unorphaned_txs = result.unwrap().accepted; let (populated_txs, orphans) = mining_manager.get_all_transactions(TransactionQuery::All); @@ -897,8 +910,13 @@ mod tests { // Try submit children while rejecting orphans for (tx, test) in child_txs.iter().zip(tests.iter()) { - let result = - mining_manager.validate_and_insert_transaction(consensus.as_ref(), tx.clone(), test.priority, Orphan::Forbidden); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + tx.clone(), + test.priority, + Orphan::Forbidden, + RbfPolicy::Forbidden, + ); assert!(result.is_err(), "mempool should reject an orphan transaction with {:?} when asked to do so", test.priority); if let Err(MiningManagerError::MempoolError(RuleError::RejectDisallowedOrphan(transaction_id))) = result { assert_eq!( @@ -918,8 +936,13 @@ mod tests { // Try submit children while accepting orphans for (tx, test) in child_txs.iter().zip(tests.iter()) { - let result = - mining_manager.validate_and_insert_transaction(consensus.as_ref(), tx.clone(), test.priority, Orphan::Allowed); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + tx.clone(), + test.priority, + Orphan::Allowed, + RbfPolicy::Forbidden, + ); assert_eq!( test.should_enter_orphan_pool, result.is_ok(), @@ -947,8 +970,13 @@ mod tests { // Submit all the parents for (i, (tx, test)) in parent_txs.iter().zip(tests.iter()).enumerate() { - let result = - mining_manager.validate_and_insert_transaction(consensus.as_ref(), tx.clone(), test.priority, Orphan::Allowed); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + tx.clone(), + test.priority, + Orphan::Allowed, + RbfPolicy::Forbidden, + ); assert!(result.is_ok(), "mempool should accept a valid transaction with {:?} when asked to do so", test.priority,); let unorphaned_txs = &result.as_ref().unwrap().accepted; assert_eq!( @@ -987,8 +1015,13 @@ mod tests { // Add to mempool a transaction that spends child_tx_2 (as high priority) let spending_tx = create_transaction(&child_tx_2, 1_000); - let result = - mining_manager.validate_and_insert_transaction(consensus.as_ref(), spending_tx.clone(), Priority::High, Orphan::Allowed); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + spending_tx.clone(), + Priority::High, + Orphan::Allowed, + RbfPolicy::Forbidden, + ); assert!(result.is_ok(), "the insertion in the mempool of the spending transaction failed"); // Revalidate, to make sure spending_tx is still valid @@ -1042,11 +1075,21 @@ mod tests { let (parent_txs, child_txs) = create_arrays_of_parent_and_children_transactions(&consensus, TX_PAIRS_COUNT); for (parent_tx, child_tx) in parent_txs.iter().zip(child_txs.iter()) { - let result = - mining_manager.validate_and_insert_transaction(consensus.as_ref(), parent_tx.clone(), Priority::Low, Orphan::Allowed); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + parent_tx.clone(), + Priority::Low, + Orphan::Allowed, + RbfPolicy::Forbidden, + ); assert!(result.is_ok(), "the mempool should accept the valid parent transaction {}", parent_tx.id()); - let result = - mining_manager.validate_and_insert_transaction(consensus.as_ref(), child_tx.clone(), Priority::Low, Orphan::Allowed); + let result = mining_manager.validate_and_insert_transaction( + consensus.as_ref(), + child_tx.clone(), + Priority::Low, + Orphan::Allowed, + RbfPolicy::Forbidden, + ); assert!(result.is_ok(), "the mempool should accept the valid child transaction {}", parent_tx.id()); } @@ -1292,9 +1335,10 @@ mod tests { transactions: impl Iterator, priority: Priority, orphan: Orphan, + rbf_policy: RbfPolicy, ) { transactions.for_each(|transaction| { - let result = mining_manager.validate_and_insert_transaction(consensus, transaction.clone(), priority, orphan); + let result = mining_manager.validate_and_insert_transaction(consensus, transaction.clone(), priority, orphan, rbf_policy); assert!(result.is_ok(), "the mempool should accept a valid transaction when it is able to populate its UTXO entries"); }); } diff --git a/mining/src/mempool/mod.rs b/mining/src/mempool/mod.rs index 89d141866..1f63a3f44 100644 --- a/mining/src/mempool/mod.rs +++ b/mining/src/mempool/mod.rs @@ -159,4 +159,51 @@ pub mod tx { Forbidden, Allowed, } + + /// Replace by Fee (RBF) policy + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + pub enum RbfPolicy { + /// ### RBF is forbidden + /// + /// Inserts the incoming transaction. + /// + /// Conditions of success: + /// + /// - no double spend + /// + /// If conditions are not met, leaves the mempool unchanged and fails with a double spend error. + Forbidden, + + /// ### RBF may occur + /// + /// Identifies double spends in mempool and their owning transactions checking in order every input of the incoming + /// transaction. + /// + /// Removes all mempool transactions owning double spends and inserts the incoming transaction. + /// + /// Conditions of success: + /// + /// - on absence of double spends, always succeeds + /// - on double spends, the incoming transaction has a higher fee/mass ratio than the mempool transaction owning + /// the first double spend + /// + /// If conditions are not met, leaves the mempool unchanged and fails with a double spend or a tx fee/mass too low error. + Allowed, + + /// ### RBF must occur + /// + /// Identifies double spends in mempool and their owning transactions checking in order every input of the incoming + /// transaction. + /// + /// Removes the mempool transaction owning the double spends and inserts the incoming transaction. + /// + /// Conditions of success: + /// + /// - at least one double spend + /// - all double spends belong to the same mempool transaction + /// - the incoming transaction has a higher fee/mass ratio than the mempool double spending transaction. + /// + /// If conditions are not met, leaves the mempool unchanged and fails with a double spend or a tx fee/mass too low error. + Mandatory, + } } diff --git a/mining/src/mempool/model/tx.rs b/mining/src/mempool/model/tx.rs index e20558dbf..1b1fc4c3b 100644 --- a/mining/src/mempool/model/tx.rs +++ b/mining/src/mempool/model/tx.rs @@ -1,4 +1,4 @@ -use crate::mempool::tx::Priority; +use crate::mempool::tx::{Priority, RbfPolicy}; use kaspa_consensus_core::tx::{MutableTransaction, Transaction, TransactionId, TransactionOutpoint}; use kaspa_mining_errors::mempool::RuleError; use std::{ @@ -55,53 +55,6 @@ impl PartialEq for MempoolTransaction { } } -/// Replace by Fee (RBF) policy -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub(crate) enum RbfPolicy { - /// ### RBF is forbidden - /// - /// Inserts the incoming transaction. - /// - /// Conditions of success: - /// - /// - no double spend - /// - /// If conditions are not met, leaves the mempool unchanged and fails with a double spend error. - Forbidden, - - /// ### RBF may occur - /// - /// Identifies double spends in mempool and their owning transactions checking in order every input of the incoming - /// transaction. - /// - /// Removes all mempool transactions owning double spends and inserts the incoming transaction. - /// - /// Conditions of success: - /// - /// - on absence of double spends, always succeeds - /// - on double spends, the incoming transaction has a higher fee/mass ratio than the mempool transaction owning - /// the first double spend - /// - /// If conditions are not met, leaves the mempool unchanged and fails with a double spend or a tx fee/mass too low error. - Allowed, - - /// ### RBF must occur - /// - /// Identifies double spends in mempool and their owning transactions checking in order every input of the incoming - /// transaction. - /// - /// Removes the mempool transaction owning the double spends and inserts the incoming transaction. - /// - /// Conditions of success: - /// - /// - at least one double spend - /// - all double spends belong to the same mempool transaction - /// - the incoming transaction has a higher fee/mass ratio than the mempool double spending transaction. - /// - /// If conditions are not met, leaves the mempool unchanged and fails with a double spend or a tx fee/mass too low error. - Mandatory, -} - impl RbfPolicy { #[cfg(test)] /// Returns an alternate policy accepting a transaction insertion in case the policy requires a replacement diff --git a/mining/src/mempool/replace_by_fee.rs b/mining/src/mempool/replace_by_fee.rs index 466864aa5..9ade9210f 100644 --- a/mining/src/mempool/replace_by_fee.rs +++ b/mining/src/mempool/replace_by_fee.rs @@ -1,6 +1,7 @@ use crate::mempool::{ errors::{RuleError, RuleResult}, - model::tx::{DoubleSpend, MempoolTransaction, RbfPolicy, TxRemovalReason}, + model::tx::{DoubleSpend, MempoolTransaction, TxRemovalReason}, + tx::RbfPolicy, Mempool, }; use kaspa_consensus_core::tx::{MutableTransaction, Transaction}; diff --git a/mining/src/mempool/validate_and_insert_transaction.rs b/mining/src/mempool/validate_and_insert_transaction.rs index 6c2ed5587..cb4167b58 100644 --- a/mining/src/mempool/validate_and_insert_transaction.rs +++ b/mining/src/mempool/validate_and_insert_transaction.rs @@ -2,9 +2,9 @@ use crate::mempool::{ errors::{RuleError, RuleResult}, model::{ pool::Pool, - tx::{MempoolTransaction, RbfPolicy, TransactionPostValidation, TransactionPreValidation, TxRemovalReason}, + tx::{MempoolTransaction, TransactionPostValidation, TransactionPreValidation, TxRemovalReason}, }, - tx::{Orphan, Priority}, + tx::{Orphan, Priority, RbfPolicy}, Mempool, }; use kaspa_consensus_core::{ diff --git a/protocol/flows/src/flow_context.rs b/protocol/flows/src/flow_context.rs index 4e9e413ed..494e70252 100644 --- a/protocol/flows/src/flow_context.rs +++ b/protocol/flows/src/flow_context.rs @@ -25,8 +25,8 @@ use kaspa_core::{ }; use kaspa_core::{time::unix_now, warn}; use kaspa_hashes::Hash; -use kaspa_mining::manager::MiningManagerProxy; use kaspa_mining::mempool::tx::{Orphan, Priority}; +use kaspa_mining::{manager::MiningManagerProxy, mempool::tx::RbfPolicy}; use kaspa_notify::notifier::Notify; use kaspa_p2p_lib::{ common::ProtocolError, @@ -618,8 +618,11 @@ impl FlowContext { transaction: Transaction, orphan: Orphan, ) -> Result<(), ProtocolError> { - let transaction_insertion = - self.mining_manager().clone().validate_and_insert_transaction(consensus, transaction, Priority::High, orphan).await?; + let transaction_insertion = self + .mining_manager() + .clone() + .validate_and_insert_transaction(consensus, transaction, Priority::High, orphan, RbfPolicy::Forbidden) + .await?; self.broadcast_transactions( transaction_insertion.accepted.iter().map(|x| x.id()), false, // RPC transactions are considered high priority, so we don't want to throttle them @@ -640,13 +643,17 @@ impl FlowContext { consensus: &ConsensusProxy, transaction: Transaction, ) -> Result, ProtocolError> { - let transaction_insertion = self.mining_manager().clone().validate_and_replace_transaction(consensus, transaction).await?; + let transaction_insertion = self + .mining_manager() + .clone() + .validate_and_insert_transaction(consensus, transaction, Priority::High, Orphan::Forbidden, RbfPolicy::Mandatory) + .await?; self.broadcast_transactions( transaction_insertion.accepted.iter().map(|x| x.id()), false, // RPC transactions are considered high priority, so we don't want to throttle them ) .await; - Ok(transaction_insertion.removed.expect("on RBF success, a removed transaction is returned")) + Ok(transaction_insertion.removed.expect("on mandatory RBF success, a removed transaction is returned")) } /// Returns true if the time has come for running the task cleaning mempool transactions. diff --git a/protocol/flows/src/v5/txrelay/flow.rs b/protocol/flows/src/v5/txrelay/flow.rs index 6a177e57f..af7e2b6c7 100644 --- a/protocol/flows/src/v5/txrelay/flow.rs +++ b/protocol/flows/src/v5/txrelay/flow.rs @@ -10,7 +10,7 @@ use kaspa_mining::{ errors::MiningManagerError, mempool::{ errors::RuleError, - tx::{Orphan, Priority}, + tx::{Orphan, Priority, RbfPolicy}, }, model::tx_query::TransactionQuery, P2pTxCountSample, @@ -219,7 +219,7 @@ impl RelayTransactionsFlow { .ctx .mining_manager() .clone() - .validate_and_insert_transaction_batch(&consensus, transactions, Priority::Low, Orphan::Allowed) + .validate_and_insert_transaction_batch(&consensus, transactions, Priority::Low, Orphan::Allowed, RbfPolicy::Allowed) .await; for res in insert_results.iter() { From b6730af562f0410d5c155c6d641753768c9e02fb Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Fri, 12 Jul 2024 12:41:33 +0000 Subject: [PATCH 22/30] Infer RBF policy from unorphaned transaction --- mining/src/manager.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/mining/src/manager.rs b/mining/src/manager.rs index be9190d6f..7fb6656d4 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -311,13 +311,23 @@ impl MiningManager { .zip(validation_results) .flat_map(|((transaction, priority), validation_result)| { let orphan_id = transaction.id(); + + // The RBF policy applied to an orphaned transaction is not recorded in the orphan pool + // but we can infer it from the priority: + // - high means a submitted tx via RPC which forbids RBF + // - low means a tx arrived via P2P which allows RBF + let rbf_policy = match priority { + Priority::High => RbfPolicy::Forbidden, + Priority::Low => RbfPolicy::Allowed, + }; + match mempool.post_validate_and_insert_transaction( consensus, validation_result, transaction, priority, Orphan::Forbidden, - RbfPolicy::Forbidden, + rbf_policy, ) { Ok(TransactionPostValidation { removed: _, accepted: Some(accepted_transaction) }) => { accepted_transactions.push(accepted_transaction.clone()); From 69713020a2a7fb34da232326b3f3269b10631ed1 Mon Sep 17 00:00:00 2001 From: Tiram <18632023+tiram88@users.noreply.github.com> Date: Fri, 12 Jul 2024 14:09:17 +0000 Subject: [PATCH 23/30] Apply the RBF policy to all orphan-related cases --- mining/src/manager.rs | 11 +--------- .../validate_and_insert_transaction.rs | 21 +++++++++++++++++-- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/mining/src/manager.rs b/mining/src/manager.rs index 7fb6656d4..34ca0bcbb 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -311,16 +311,7 @@ impl MiningManager { .zip(validation_results) .flat_map(|((transaction, priority), validation_result)| { let orphan_id = transaction.id(); - - // The RBF policy applied to an orphaned transaction is not recorded in the orphan pool - // but we can infer it from the priority: - // - high means a submitted tx via RPC which forbids RBF - // - low means a tx arrived via P2P which allows RBF - let rbf_policy = match priority { - Priority::High => RbfPolicy::Forbidden, - Priority::Low => RbfPolicy::Allowed, - }; - + let rbf_policy = Mempool::get_orphan_transaction_rbf_policy(priority); match mempool.post_validate_and_insert_transaction( consensus, validation_result, diff --git a/mining/src/mempool/validate_and_insert_transaction.rs b/mining/src/mempool/validate_and_insert_transaction.rs index cb4167b58..c5fc8cc4f 100644 --- a/mining/src/mempool/validate_and_insert_transaction.rs +++ b/mining/src/mempool/validate_and_insert_transaction.rs @@ -58,7 +58,7 @@ impl Mempool { if orphan == Orphan::Forbidden { return Err(RuleError::RejectDisallowedOrphan(transaction_id)); } - self.transaction_pool.check_double_spends(&transaction)?; + let _ = self.get_replace_by_fee_constraint(&transaction, rbf_policy)?; self.orphan_pool.try_add_orphan(consensus.get_virtual_daa_score(), transaction, priority)?; return Ok(TransactionPostValidation::default()); } @@ -186,9 +186,26 @@ impl Mempool { // The one we just removed from the orphan pool. assert_eq!(transactions.len(), 1, "the list returned by remove_orphan is expected to contain exactly one transaction"); let transaction = transactions.pop().unwrap(); + let rbf_policy = Self::get_orphan_transaction_rbf_policy(transaction.priority); self.validate_transaction_unacceptance(&transaction.mtx)?; - self.transaction_pool.check_double_spends(&transaction.mtx)?; + let _ = self.get_replace_by_fee_constraint(&transaction.mtx, rbf_policy)?; Ok(transaction) } + + /// Returns the RBF policy to apply to an orphan/unorphaned transaction by inferring it from the transaction priority. + pub(crate) fn get_orphan_transaction_rbf_policy(priority: Priority) -> RbfPolicy { + // The RBF policy applied to an orphaned transaction is not recorded in the orphan pool + // but we can infer it from the priority: + // + // - high means a submitted tx via RPC which forbids RBF + // - low means a tx arrived via P2P which allows RBF + // + // Note that the RPC submit transaction replacement case, implying a mandatory RBF, forbids orphans + // so is excluded here. + match priority { + Priority::High => RbfPolicy::Forbidden, + Priority::Low => RbfPolicy::Allowed, + } + } } From 9fbd74185e084fcb959d93b246866ce6bcc1174a Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Mon, 22 Jul 2024 13:17:19 +0000 Subject: [PATCH 24/30] In Rbf allowed mode, check feerate threshold vs all double spends (i.e., compare to the max) --- mining/src/mempool/replace_by_fee.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/mining/src/mempool/replace_by_fee.rs b/mining/src/mempool/replace_by_fee.rs index 9ade9210f..22d5dd327 100644 --- a/mining/src/mempool/replace_by_fee.rs +++ b/mining/src/mempool/replace_by_fee.rs @@ -31,7 +31,11 @@ impl Mempool { if double_spends.is_empty() { Ok(None) } else { - let fee_per_mass_threshold = self.get_double_spend_fee_per_mass(&double_spends[0])?; + let mut fee_per_mass_threshold = 0f64; + for double_spend in double_spends { + // We take the max over all double spends as the required threshold + fee_per_mass_threshold = fee_per_mass_threshold.max(self.get_double_spend_fee_per_mass(&double_spend)?); + } Ok(Some(fee_per_mass_threshold)) } } @@ -75,6 +79,11 @@ impl Mempool { true => Ok(None), false => { let removed = self.validate_double_spending_transaction(transaction, &double_spends[0])?.mtx.tx.clone(); + for double_spend in double_spends.iter().skip(1) { + // Validate the feerate threshold is passed for all double spends + self.validate_double_spending_transaction(transaction, double_spend)?; + } + // We apply consequences such as removal only after we fully validate against all double spends for double_spend in double_spends { self.remove_transaction( &double_spend.owner_id, From 1fcb2648c6833643d95a1499e569992c4437ab75 Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Mon, 22 Jul 2024 13:17:40 +0000 Subject: [PATCH 25/30] Minor hashset optimization --- mining/src/mempool/model/utxo_set.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/mining/src/mempool/model/utxo_set.rs b/mining/src/mempool/model/utxo_set.rs index 403a13ce5..808d67488 100644 --- a/mining/src/mempool/model/utxo_set.rs +++ b/mining/src/mempool/model/utxo_set.rs @@ -97,8 +97,7 @@ impl MempoolUtxoSet { let mut visited = HashSet::new(); for input in transaction.tx.inputs.iter() { if let Some(existing_transaction_id) = self.get_outpoint_owner_id(&input.previous_outpoint) { - if *existing_transaction_id != transaction_id && !visited.contains(existing_transaction_id) { - visited.insert(*existing_transaction_id); + if *existing_transaction_id != transaction_id && visited.insert(*existing_transaction_id) { double_spends.push(DoubleSpend::new(input.previous_outpoint, *existing_transaction_id)); } } From 4679a8d3387425efebc3574dc51fb57141e6363c Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Mon, 22 Jul 2024 13:35:50 +0000 Subject: [PATCH 26/30] Rename: fee_per_mass -> feerate --- consensus/core/src/api/args.rs | 14 +++++++------- consensus/core/src/tx.rs | 8 +++++--- .../virtual_processor/utxo_validation.rs | 4 ++-- .../transaction_validator_populated.rs | 10 +++++----- mining/src/manager.rs | 10 +++++----- mining/src/mempool/model/tx.rs | 2 +- mining/src/mempool/replace_by_fee.rs | 17 ++++++++--------- .../mempool/validate_and_insert_transaction.rs | 4 ++-- rothschild/src/main.rs | 4 ++-- testing/integration/src/common/utils.rs | 4 ++-- 10 files changed, 39 insertions(+), 38 deletions(-) diff --git a/consensus/core/src/api/args.rs b/consensus/core/src/api/args.rs index 88f857535..ebc76d97d 100644 --- a/consensus/core/src/api/args.rs +++ b/consensus/core/src/api/args.rs @@ -6,12 +6,12 @@ use crate::tx::TransactionId; #[derive(Clone, Debug, Default)] pub struct TransactionValidationArgs { /// Optional fee/mass threshold above which a bound transaction in not rejected - pub fee_per_mass_threshold: Option, + pub feerate_threshold: Option, } impl TransactionValidationArgs { - pub fn new(fee_per_mass_threshold: Option) -> Self { - Self { fee_per_mass_threshold } + pub fn new(feerate_threshold: Option) -> Self { + Self { feerate_threshold } } } @@ -21,18 +21,18 @@ pub struct TransactionValidationBatchArgs { } impl TransactionValidationBatchArgs { - const DEFAULT_ARGS: TransactionValidationArgs = TransactionValidationArgs { fee_per_mass_threshold: None }; + const DEFAULT_ARGS: TransactionValidationArgs = TransactionValidationArgs { feerate_threshold: None }; pub fn new() -> Self { Self { tx_args: HashMap::new() } } /// Set some fee/mass threshold for transaction `transaction_id`. - pub fn set_fee_per_mass_threshold(&mut self, transaction_id: TransactionId, threshold: f64) { + pub fn set_feerate_threshold(&mut self, transaction_id: TransactionId, feerate_threshold: f64) { self.tx_args .entry(transaction_id) - .and_modify(|x| x.fee_per_mass_threshold = Some(threshold)) - .or_insert(TransactionValidationArgs::new(Some(threshold))); + .and_modify(|x| x.feerate_threshold = Some(feerate_threshold)) + .or_insert(TransactionValidationArgs::new(Some(feerate_threshold))); } pub fn get(&self, transaction_id: &TransactionId) -> &TransactionValidationArgs { diff --git a/consensus/core/src/tx.rs b/consensus/core/src/tx.rs index eecc3c073..67e09df73 100644 --- a/consensus/core/src/tx.rs +++ b/consensus/core/src/tx.rs @@ -407,9 +407,11 @@ impl> MutableTransaction { } } - /// Returns the calculated fee per contextual (compute & storage) mass ratio when - /// some calculated fee does exist and the contextual mass is greater than zero. - pub fn calculated_fee_per_mass(&self) -> Option { + /// Returns the calculated feerate. The feerate is calculated as the amount of fee + /// this transactions pays per gram of the full contextual (compute & storage) mass. The + /// function returns a value when calculated fee exists and the contextual mass is greater + /// than zero, otherwise `None` is returned. + pub fn calculated_feerate(&self) -> Option { let contextual_mass = self.tx.as_ref().mass(); if contextual_mass > 0 { self.calculated_fee.map(|fee| fee as f64 / contextual_mass as f64) diff --git a/consensus/src/pipeline/virtual_processor/utxo_validation.rs b/consensus/src/pipeline/virtual_processor/utxo_validation.rs index 94e3ebc1e..16f355a45 100644 --- a/consensus/src/pipeline/virtual_processor/utxo_validation.rs +++ b/consensus/src/pipeline/virtual_processor/utxo_validation.rs @@ -310,12 +310,12 @@ impl VirtualStateProcessor { mutable_tx.tx.set_mass(contextual_mass); // At this point we know all UTXO entries are populated, so we can safely pass the tx as verifiable - let mass_and_fee_per_mass_threshold = args.fee_per_mass_threshold.map(|threshold| (contextual_mass, threshold)); + let mass_and_feerate_threshold = args.feerate_threshold.map(|threshold| (contextual_mass, threshold)); let calculated_fee = self.transaction_validator.validate_populated_transaction_and_get_fee( &mutable_tx.as_verifiable(), pov_daa_score, TxValidationFlags::SkipMassCheck, // we can skip the mass check since we just set it - mass_and_fee_per_mass_threshold, + mass_and_feerate_threshold, )?; mutable_tx.calculated_fee = Some(calculated_fee); Ok(()) diff --git a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs index c5157980a..647e6712a 100644 --- a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs +++ b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs @@ -27,7 +27,7 @@ impl TransactionValidator { tx: &impl VerifiableTransaction, pov_daa_score: u64, flags: TxValidationFlags, - mass_and_fee_per_mass_threshold: Option<(u64, f64)>, + mass_and_feerate_threshold: Option<(u64, f64)>, ) -> TxResult { self.check_transaction_coinbase_maturity(tx, pov_daa_score)?; let total_in = self.check_transaction_input_amounts(tx)?; @@ -45,7 +45,7 @@ impl TransactionValidator { // The following call is not a consensus check (it could not be one in the first place since it uses floating number) // but rather a mempool Replace by Fee validation rule. It was placed here purposely for avoiding unneeded script checks. - Self::check_fee_per_mass(fee, mass_and_fee_per_mass_threshold)?; + Self::check_feerate_threshold(fee, mass_and_feerate_threshold)?; match flags { TxValidationFlags::Full | TxValidationFlags::SkipMassCheck => { @@ -57,12 +57,12 @@ impl TransactionValidator { Ok(fee) } - fn check_fee_per_mass(fee: u64, mass_and_fee_per_mass_threshold: Option<(u64, f64)>) -> TxResult<()> { + fn check_feerate_threshold(fee: u64, mass_and_feerate_threshold: Option<(u64, f64)>) -> TxResult<()> { // An actual check can only occur if some mass and threshold are provided, // otherwise, the check does not verify anything and exits successfully. - if let Some((contextual_mass, threshold)) = mass_and_fee_per_mass_threshold { + if let Some((contextual_mass, feerate_threshold)) = mass_and_feerate_threshold { assert!(contextual_mass > 0); - if fee as f64 / contextual_mass as f64 <= threshold { + if fee as f64 / contextual_mass as f64 <= feerate_threshold { return Err(TxRuleError::FeePerMassTooLow); } } diff --git a/mining/src/manager.rs b/mining/src/manager.rs index 34ca0bcbb..ceb3d4140 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -246,9 +246,9 @@ impl MiningManager { rbf_policy: RbfPolicy, ) -> MiningManagerResult { // read lock on mempool - let TransactionPreValidation { mut transaction, fee_per_mass_threshold } = + let TransactionPreValidation { mut transaction, feerate_threshold } = self.mempool.read().pre_validate_and_populate_transaction(consensus, transaction, rbf_policy)?; - let args = TransactionValidationArgs::new(fee_per_mass_threshold); + let args = TransactionValidationArgs::new(feerate_threshold); // no lock on mempool let validation_result = validate_mempool_transaction(consensus, &mut transaction, &args); // write lock on mempool @@ -372,9 +372,9 @@ impl MiningManager { let txs = chunk.filter_map(|tx| { let transaction_id = tx.id(); match mempool.pre_validate_and_populate_transaction(consensus, tx, rbf_policy) { - Ok(TransactionPreValidation { transaction, fee_per_mass_threshold }) => { - if let Some(threshold) = fee_per_mass_threshold { - args.set_fee_per_mass_threshold(transaction.id(), threshold); + Ok(TransactionPreValidation { transaction, feerate_threshold }) => { + if let Some(threshold) = feerate_threshold { + args.set_feerate_threshold(transaction.id(), threshold); } Some(transaction) } diff --git a/mining/src/mempool/model/tx.rs b/mining/src/mempool/model/tx.rs index 1b1fc4c3b..48f24b9f6 100644 --- a/mining/src/mempool/model/tx.rs +++ b/mining/src/mempool/model/tx.rs @@ -91,7 +91,7 @@ impl From<&DoubleSpend> for RuleError { pub(crate) struct TransactionPreValidation { pub transaction: MutableTransaction, - pub fee_per_mass_threshold: Option, + pub feerate_threshold: Option, } #[derive(Default)] diff --git a/mining/src/mempool/replace_by_fee.rs b/mining/src/mempool/replace_by_fee.rs index 22d5dd327..2d1025b7c 100644 --- a/mining/src/mempool/replace_by_fee.rs +++ b/mining/src/mempool/replace_by_fee.rs @@ -31,12 +31,12 @@ impl Mempool { if double_spends.is_empty() { Ok(None) } else { - let mut fee_per_mass_threshold = 0f64; + let mut feerate_threshold = 0f64; for double_spend in double_spends { // We take the max over all double spends as the required threshold - fee_per_mass_threshold = fee_per_mass_threshold.max(self.get_double_spend_fee_per_mass(&double_spend)?); + feerate_threshold = feerate_threshold.max(self.get_double_spend_feerate(&double_spend)?); } - Ok(Some(fee_per_mass_threshold)) + Ok(Some(feerate_threshold)) } } @@ -46,8 +46,8 @@ impl Mempool { match double_spends.len() { 0 => Err(RuleError::RejectRbfNoDoubleSpend), 1 => { - let fee_per_mass_threshold = self.get_double_spend_fee_per_mass(&double_spends[0])?; - Ok(Some(fee_per_mass_threshold)) + let feerate_threshold = self.get_double_spend_feerate(&double_spends[0])?; + Ok(Some(feerate_threshold)) } _ => Err(RuleError::RejectRbfTooManyDoubleSpendingTransactions), } @@ -117,9 +117,9 @@ impl Mempool { } } - fn get_double_spend_fee_per_mass(&self, double_spend: &DoubleSpend) -> RuleResult { + fn get_double_spend_feerate(&self, double_spend: &DoubleSpend) -> RuleResult { let owner = self.transaction_pool.get_double_spend_owner(double_spend)?; - match owner.mtx.calculated_fee_per_mass() { + match owner.mtx.calculated_feerate() { Some(double_spend_ratio) => Ok(double_spend_ratio), None => Err(double_spend.into()), } @@ -131,8 +131,7 @@ impl Mempool { double_spend: &DoubleSpend, ) -> RuleResult<&'a MempoolTransaction> { let owner = self.transaction_pool.get_double_spend_owner(double_spend)?; - if let (Some(transaction_ratio), Some(double_spend_ratio)) = - (transaction.calculated_fee_per_mass(), owner.mtx.calculated_fee_per_mass()) + if let (Some(transaction_ratio), Some(double_spend_ratio)) = (transaction.calculated_feerate(), owner.mtx.calculated_feerate()) { if transaction_ratio > double_spend_ratio { return Ok(owner); diff --git a/mining/src/mempool/validate_and_insert_transaction.rs b/mining/src/mempool/validate_and_insert_transaction.rs index c5fc8cc4f..23460a91e 100644 --- a/mining/src/mempool/validate_and_insert_transaction.rs +++ b/mining/src/mempool/validate_and_insert_transaction.rs @@ -25,9 +25,9 @@ impl Mempool { // Populate mass in the beginning, it will be used in multiple places throughout the validation and insertion. transaction.calculated_compute_mass = Some(consensus.calculate_transaction_compute_mass(&transaction.tx)); self.validate_transaction_in_isolation(&transaction)?; - let fee_per_mass_threshold = self.get_replace_by_fee_constraint(&transaction, rbf_policy)?; + let feerate_threshold = self.get_replace_by_fee_constraint(&transaction, rbf_policy)?; self.populate_mempool_entries(&mut transaction); - Ok(TransactionPreValidation { transaction, fee_per_mass_threshold }) + Ok(TransactionPreValidation { transaction, feerate_threshold }) } pub(crate) fn post_validate_and_insert_transaction( diff --git a/rothschild/src/main.rs b/rothschild/src/main.rs index bbabca71b..ad212b5e4 100644 --- a/rothschild/src/main.rs +++ b/rothschild/src/main.rs @@ -21,7 +21,7 @@ use secp256k1::{rand::thread_rng, Keypair}; use tokio::time::{interval, MissedTickBehavior}; const DEFAULT_SEND_AMOUNT: u64 = 10 * SOMPI_PER_KASPA; -const FEE_PER_MASS: u64 = 10; +const FEE_RATE: u64 = 10; const MILLIS_PER_TICK: u64 = 10; const ADDRESS_PREFIX: Prefix = Prefix::Testnet; const ADDRESS_VERSION: Version = Version::PubKey; @@ -438,7 +438,7 @@ fn clean_old_pending_outpoints(pending: &mut HashMap) } fn required_fee(num_utxos: usize, num_outs: u64) -> u64 { - FEE_PER_MASS * estimated_mass(num_utxos, num_outs) + FEE_RATE * estimated_mass(num_utxos, num_outs) } fn estimated_mass(num_utxos: usize, num_outs: u64) -> u64 { diff --git a/testing/integration/src/common/utils.rs b/testing/integration/src/common/utils.rs index 824bda388..cb87a98c8 100644 --- a/testing/integration/src/common/utils.rs +++ b/testing/integration/src/common/utils.rs @@ -36,8 +36,8 @@ const fn estimated_mass(num_inputs: usize, num_outputs: u64) -> u64 { } pub const fn required_fee(num_inputs: usize, num_outputs: u64) -> u64 { - const FEE_PER_MASS: u64 = 10; - FEE_PER_MASS * estimated_mass(num_inputs, num_outputs) + const FEE_RATE: u64 = 10; + FEE_RATE * estimated_mass(num_inputs, num_outputs) } /// Builds a TX DAG based on the initial UTXO set and on constant params From 8093ef62d28c38fce3bca05d21460dfcde32cb0a Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Mon, 22 Jul 2024 14:33:29 +0000 Subject: [PATCH 27/30] Use rbf policy arg for post processing step as well --- mining/src/manager.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mining/src/manager.rs b/mining/src/manager.rs index ceb3d4140..0a0e457ae 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -427,7 +427,7 @@ impl MiningManager { transaction, priority, orphan, - RbfPolicy::Allowed, + rbf_policy, ) { Ok(TransactionPostValidation { removed: _, accepted: Some(accepted_transaction) }) => { insert_results.push(Ok(accepted_transaction.clone())); From dc46102deb2838b5f99396fe0bfbb2cd3ba3e89e Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Mon, 22 Jul 2024 17:52:26 +0000 Subject: [PATCH 28/30] Renames and comments --- consensus/core/src/errors/tx.rs | 6 +++--- .../transaction_validator_populated.rs | 2 +- mining/src/manager.rs | 3 ++- mining/src/mempool/replace_by_fee.rs | 13 ++++++++++--- 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/consensus/core/src/errors/tx.rs b/consensus/core/src/errors/tx.rs index 6a2744f61..ec1899faa 100644 --- a/consensus/core/src/errors/tx.rs +++ b/consensus/core/src/errors/tx.rs @@ -89,10 +89,10 @@ pub enum TxRuleError { #[error("calculated contextual mass (including storage mass) {0} is not equal to the committed mass field {1}")] WrongMass(u64, u64), - /// [`TxRuleError::FeePerMassTooLow`] is not a consensus error but a mempool error triggered by the + /// [`TxRuleError::FeerateTooLow`] is not a consensus error but a mempool error triggered by the /// fee/mass RBF validation rule - #[error("fee per contextual mass ratio is not greater than the threshold")] - FeePerMassTooLow, + #[error("fee rate per contextual mass gram is not greater than the fee rate of the replaced transaction")] + FeerateTooLow, } pub type TxResult = std::result::Result; diff --git a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs index 647e6712a..1835ba61e 100644 --- a/consensus/src/processes/transaction_validator/transaction_validator_populated.rs +++ b/consensus/src/processes/transaction_validator/transaction_validator_populated.rs @@ -63,7 +63,7 @@ impl TransactionValidator { if let Some((contextual_mass, feerate_threshold)) = mass_and_feerate_threshold { assert!(contextual_mass > 0); if fee as f64 / contextual_mass as f64 <= feerate_threshold { - return Err(TxRuleError::FeePerMassTooLow); + return Err(TxRuleError::FeerateTooLow); } } Ok(()) diff --git a/mining/src/manager.rs b/mining/src/manager.rs index 0a0e457ae..995bc532b 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -278,7 +278,8 @@ impl MiningManager { ) -> Vec> { // The capacity used here may be exceeded (see next comment). let mut accepted_transactions = Vec::with_capacity(incoming_transactions.len()); - // The validation args map is immutably empty since unorphaned transactions are not eligible to replace by fee. + // The validation args map is immutably empty since unorphaned transactions do not require pre processing so there + // are no feerate thresholds to use. Instead, we rely on this being checked during post processing. let args = TransactionValidationBatchArgs::new(); // We loop as long as incoming unorphaned transactions do unorphan other transactions when they // get validated and inserted into the mempool. diff --git a/mining/src/mempool/replace_by_fee.rs b/mining/src/mempool/replace_by_fee.rs index 2d1025b7c..6acd1a618 100644 --- a/mining/src/mempool/replace_by_fee.rs +++ b/mining/src/mempool/replace_by_fee.rs @@ -120,7 +120,9 @@ impl Mempool { fn get_double_spend_feerate(&self, double_spend: &DoubleSpend) -> RuleResult { let owner = self.transaction_pool.get_double_spend_owner(double_spend)?; match owner.mtx.calculated_feerate() { - Some(double_spend_ratio) => Ok(double_spend_ratio), + Some(double_spend_feerate) => Ok(double_spend_feerate), + // Getting here is unexpected since a mempool owned tx should be populated with fee + // and mass at this stage but nonetheless we fail gracefully None => Err(double_spend.into()), } } @@ -131,12 +133,17 @@ impl Mempool { double_spend: &DoubleSpend, ) -> RuleResult<&'a MempoolTransaction> { let owner = self.transaction_pool.get_double_spend_owner(double_spend)?; - if let (Some(transaction_ratio), Some(double_spend_ratio)) = (transaction.calculated_feerate(), owner.mtx.calculated_feerate()) + if let (Some(transaction_feerate), Some(double_spend_feerate)) = + (transaction.calculated_feerate(), owner.mtx.calculated_feerate()) { - if transaction_ratio > double_spend_ratio { + if transaction_feerate > double_spend_feerate { return Ok(owner); + } else { + return Err(double_spend.into()); } } + // Getting here is unexpected since both txs should be populated with + // fee and mass at this stage but nonetheless we fail gracefully Err(double_spend.into()) } } From 49d584bcce9c3e414c3318173ba631e81a72301c Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Mon, 22 Jul 2024 18:41:20 +0000 Subject: [PATCH 29/30] Relaxation: fail gracefully if rbf replaced tx is missing (also fixes an edge case where mempool duplication resulted in this scenario) --- mining/src/block_template/selector.rs | 6 +----- mining/src/manager.rs | 2 +- mining/src/mempool/validate_and_insert_transaction.rs | 2 +- protocol/flows/src/flow_context.rs | 10 +++++++++- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/mining/src/block_template/selector.rs b/mining/src/block_template/selector.rs index b65126caf..a55ecb93d 100644 --- a/mining/src/block_template/selector.rs +++ b/mining/src/block_template/selector.rs @@ -182,11 +182,7 @@ impl TransactionsSelector { self.total_mass += selected_tx.calculated_mass; self.total_fees += selected_tx.calculated_fee; - trace!( - "Adding tx {0} (fee per megagram: {1})", - selected_tx.tx.id(), - selected_tx.calculated_fee * 1_000_000 / selected_tx.calculated_mass - ); + trace!("Adding tx {0} (fee per gram: {1})", selected_tx.tx.id(), selected_tx.calculated_fee / selected_tx.calculated_mass); // Mark for deletion selected_candidate.is_marked_for_deletion = true; diff --git a/mining/src/manager.rs b/mining/src/manager.rs index 995bc532b..c3bf61261 100644 --- a/mining/src/manager.rs +++ b/mining/src/manager.rs @@ -435,7 +435,7 @@ impl MiningManager { self.counters.increase_tx_counts(1, priority); mempool.get_unorphaned_transactions_after_accepted_transaction(&accepted_transaction) } - Ok(TransactionPostValidation { removed: _, accepted: None }) => { + Ok(TransactionPostValidation { removed: _, accepted: None }) | Err(RuleError::RejectDuplicate(_)) => { // Either orphaned or already existing in the mempool vec![] } diff --git a/mining/src/mempool/validate_and_insert_transaction.rs b/mining/src/mempool/validate_and_insert_transaction.rs index 23460a91e..bcfedc2db 100644 --- a/mining/src/mempool/validate_and_insert_transaction.rs +++ b/mining/src/mempool/validate_and_insert_transaction.rs @@ -47,7 +47,7 @@ impl Mempool { // concurrently. if self.transaction_pool.has(&transaction_id) { debug!("Transaction {0} is not post validated since already in the mempool", transaction_id); - return Ok(TransactionPostValidation::default()); + return Err(RuleError::RejectDuplicate(transaction_id)); } self.validate_transaction_unacceptance(&transaction)?; diff --git a/protocol/flows/src/flow_context.rs b/protocol/flows/src/flow_context.rs index 494e70252..099f97896 100644 --- a/protocol/flows/src/flow_context.rs +++ b/protocol/flows/src/flow_context.rs @@ -643,6 +643,7 @@ impl FlowContext { consensus: &ConsensusProxy, transaction: Transaction, ) -> Result, ProtocolError> { + let transaction_id = transaction.id(); let transaction_insertion = self .mining_manager() .clone() @@ -653,7 +654,14 @@ impl FlowContext { false, // RPC transactions are considered high priority, so we don't want to throttle them ) .await; - Ok(transaction_insertion.removed.expect("on mandatory RBF success, a removed transaction is returned")) + // The combination of args above of Orphan::Forbidden and RbfPolicy::Mandatory should always result + // in a removed transaction returned, however we prefer failing gracefully in case of future internal mempool changes + transaction_insertion.removed.ok_or_else(|| { + ProtocolError::OtherOwned(format!( + "Transaction {} was accepted but the replaced transaction was not returned from the mempool", + transaction_id + )) + }) } /// Returns true if the time has come for running the task cleaning mempool transactions. From c4ee7356a1cf9807a13b10cbcb41ec21948f1826 Mon Sep 17 00:00:00 2001 From: Michael Sutton Date: Mon, 22 Jul 2024 18:49:15 +0000 Subject: [PATCH 30/30] Tx id is appended by the caller anyway --- protocol/flows/src/flow_context.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/protocol/flows/src/flow_context.rs b/protocol/flows/src/flow_context.rs index 099f97896..14d4168ac 100644 --- a/protocol/flows/src/flow_context.rs +++ b/protocol/flows/src/flow_context.rs @@ -643,7 +643,6 @@ impl FlowContext { consensus: &ConsensusProxy, transaction: Transaction, ) -> Result, ProtocolError> { - let transaction_id = transaction.id(); let transaction_insertion = self .mining_manager() .clone() @@ -656,12 +655,9 @@ impl FlowContext { .await; // The combination of args above of Orphan::Forbidden and RbfPolicy::Mandatory should always result // in a removed transaction returned, however we prefer failing gracefully in case of future internal mempool changes - transaction_insertion.removed.ok_or_else(|| { - ProtocolError::OtherOwned(format!( - "Transaction {} was accepted but the replaced transaction was not returned from the mempool", - transaction_id - )) - }) + transaction_insertion.removed.ok_or(ProtocolError::Other( + "Replacement transaction was actually accepted but the *replaced* transaction was not returned from the mempool", + )) } /// Returns true if the time has come for running the task cleaning mempool transactions.