From c7d9d36f57378ed53a7194eba11b902381e8b469 Mon Sep 17 00:00:00 2001 From: SW van Heerden Date: Mon, 18 Sep 2023 12:59:58 +0200 Subject: [PATCH 1/2] fix potential overflow --- .../src/ui/state/app_state.rs | 4 +-- .../src/commands/command/get_mempool_state.rs | 6 +++- .../src/base_node/comms_interface/error.rs | 3 ++ .../comms_interface/inbound_handlers.rs | 2 +- .../core/src/base_node/service/error.rs | 4 +++ .../core/src/blocks/new_block_template.rs | 18 ++++++++--- .../tests/blockchain_database.rs | 8 +++-- .../core/src/mempool/mempool_storage.rs | 22 +++++++++---- .../priority/prioritized_transaction.rs | 26 ++++++++------- .../src/mempool/unconfirmed_pool/error.rs | 4 +++ .../unconfirmed_pool/unconfirmed_pool.rs | 32 +++++++++++-------- .../core/src/transactions/aggregated_body.rs | 14 +++++--- .../transaction_components/error.rs | 2 ++ .../transaction_components/transaction.rs | 2 +- .../core/tests/helpers/block_builders.rs | 18 +++++++---- base_layer/core/tests/helpers/database.rs | 5 +-- .../core/tests/tests/block_validation.rs | 4 +-- base_layer/core/tests/tests/node_service.rs | 4 +-- .../wallet/src/transaction_service/handle.rs | 2 +- .../protocols/transaction_receive_protocol.rs | 5 ++- .../output_manager_service_tests/service.rs | 8 ++--- 21 files changed, 126 insertions(+), 67 deletions(-) diff --git a/applications/minotari_console_wallet/src/ui/state/app_state.rs b/applications/minotari_console_wallet/src/ui/state/app_state.rs index 55eec71c64..24ac99a9d8 100644 --- a/applications/minotari_console_wallet/src/ui/state/app_state.rs +++ b/applications/minotari_console_wallet/src/ui/state/app_state.rs @@ -58,7 +58,7 @@ use tari_comms::{ use tari_contacts::contacts_service::{handle::ContactsLivenessEvent, types::Contact}; use tari_core::transactions::{ tari_amount::{uT, MicroMinotari}, - transaction_components::{OutputFeatures, TemplateType}, + transaction_components::{OutputFeatures, TemplateType, TransactionError}, weight::TransactionWeight, }; use tari_shutdown::ShutdownSignal; @@ -1176,7 +1176,7 @@ impl CompletedTransactionInfo { pub fn from_completed_transaction( tx: CompletedTransaction, transaction_weighting: &TransactionWeight, - ) -> std::io::Result { + ) -> Result { let excess_signature = tx .transaction .first_kernel_excess_sig() diff --git a/applications/minotari_node/src/commands/command/get_mempool_state.rs b/applications/minotari_node/src/commands/command/get_mempool_state.rs index 07dce390a2..98222d9e9c 100644 --- a/applications/minotari_node/src/commands/command/get_mempool_state.rs +++ b/applications/minotari_node/src/commands/command/get_mempool_state.rs @@ -69,10 +69,14 @@ impl CommandContext { continue; } } else { + let fee = match tx.body.get_total_fee() { + Ok(fee) => format!("{}", fee), + Err(e) => e.to_string(), + }; println!( " {} Fee: {}, Outputs: {}, Kernels: {}, Inputs: {}, features_and_scripts: {} bytes", tx_sig, - tx.body.get_total_fee(), + fee, tx.body.outputs().len(), tx.body.kernels().len(), tx.body.inputs().len(), diff --git a/base_layer/core/src/base_node/comms_interface/error.rs b/base_layer/core/src/base_node/comms_interface/error.rs index ce27e93089..35bc42ce3b 100644 --- a/base_layer/core/src/base_node/comms_interface/error.rs +++ b/base_layer/core/src/base_node/comms_interface/error.rs @@ -31,6 +31,7 @@ use crate::{ consensus::ConsensusManagerError, mempool::MempoolError, proof_of_work::{monero_rx::MergeMineError, DifficultyError}, + transactions::transaction_components::TransactionError, }; #[derive(Debug, Error)] @@ -75,4 +76,6 @@ pub enum CommsInterfaceError { MergeMineError(#[from] MergeMineError), #[error("Invalid difficulty: {0}")] DifficultyError(#[from] DifficultyError), + #[error("Transaction error: {0}")] + TransactionError(#[from] TransactionError), } diff --git a/base_layer/core/src/base_node/comms_interface/inbound_handlers.rs b/base_layer/core/src/base_node/comms_interface/inbound_handlers.rs index 710990bdb1..ed61cc2d7b 100644 --- a/base_layer/core/src/base_node/comms_interface/inbound_handlers.rs +++ b/base_layer/core/src/base_node/comms_interface/inbound_handlers.rs @@ -300,7 +300,7 @@ where B: BlockchainBackend + 'static self.get_target_difficulty_for_next_block(request.algo, constants, prev_hash) .await?, self.consensus_manager.get_block_reward_at(height), - ); + )?; debug!(target: LOG_TARGET, "New template block: {}", block_template); debug!( diff --git a/base_layer/core/src/base_node/service/error.rs b/base_layer/core/src/base_node/service/error.rs index d620e47cea..43684c5b07 100644 --- a/base_layer/core/src/base_node/service/error.rs +++ b/base_layer/core/src/base_node/service/error.rs @@ -64,6 +64,10 @@ impl BaseNodeServiceError { reason: format!("Invalid block header: {}", e), ban_duration: Duration::from_secs(60), }), + CommsInterfaceError::TransactionError(e) => Some(BanReason { + reason: format!("Invalid transaction: {}", e), + ban_duration: Duration::from_secs(60), + }), CommsInterfaceError::InvalidRequest { request, details } => Some(BanReason { reason: format!("Invalid request: {} ({})", request, details), ban_duration: Duration::from_secs(60), diff --git a/base_layer/core/src/blocks/new_block_template.rs b/base_layer/core/src/blocks/new_block_template.rs index 7358cdfb16..1c05b433e0 100644 --- a/base_layer/core/src/blocks/new_block_template.rs +++ b/base_layer/core/src/blocks/new_block_template.rs @@ -27,7 +27,11 @@ use serde::{Deserialize, Serialize}; use crate::{ blocks::{new_blockheader_template::NewBlockHeaderTemplate, Block}, proof_of_work::Difficulty, - transactions::{aggregated_body::AggregateBody, tari_amount::MicroMinotari}, + transactions::{ + aggregated_body::AggregateBody, + tari_amount::MicroMinotari, + transaction_components::TransactionError, + }, }; /// The new block template is used constructing a new partial block, allowing a miner to added the coinbase utxo and as @@ -49,16 +53,20 @@ pub struct NewBlockTemplate { } impl NewBlockTemplate { - pub fn from_block(block: Block, target_difficulty: Difficulty, reward: MicroMinotari) -> Self { + pub fn from_block( + block: Block, + target_difficulty: Difficulty, + reward: MicroMinotari, + ) -> Result { let Block { header, body } = block; - let total_fees = body.get_total_fee(); - Self { + let total_fees = body.get_total_fee()?; + Ok(Self { header: NewBlockHeaderTemplate::from_header(header), body, target_difficulty, reward, total_fees, - } + }) } } diff --git a/base_layer/core/src/chain_storage/tests/blockchain_database.rs b/base_layer/core/src/chain_storage/tests/blockchain_database.rs index d0357fcacf..103a67d2ef 100644 --- a/base_layer/core/src/chain_storage/tests/blockchain_database.rs +++ b/base_layer/core/src/chain_storage/tests/blockchain_database.rs @@ -423,7 +423,7 @@ mod prepare_new_block { fn it_errors_for_genesis_block() { let db = setup(); let genesis = db.fetch_block(0, true).unwrap(); - let template = NewBlockTemplate::from_block(genesis.block().clone(), Difficulty::min(), 5000 * T); + let template = NewBlockTemplate::from_block(genesis.block().clone(), Difficulty::min(), 5000 * T).unwrap(); let err = db.prepare_new_block(template).unwrap_err(); assert!(matches!(err, ChainStorageError::InvalidArguments { .. })); } @@ -433,7 +433,8 @@ mod prepare_new_block { let db = setup(); let genesis = db.fetch_block(0, true).unwrap(); let next_block = BlockHeader::from_previous(genesis.header()); - let mut template = NewBlockTemplate::from_block(next_block.into_builder().build(), Difficulty::min(), 5000 * T); + let mut template = + NewBlockTemplate::from_block(next_block.into_builder().build(), Difficulty::min(), 5000 * T).unwrap(); // This would cause a panic if the sanity checks were not there template.header.height = 100; let err = db.prepare_new_block(template.clone()).unwrap_err(); @@ -448,7 +449,8 @@ mod prepare_new_block { let db = setup(); let genesis = db.fetch_block(0, true).unwrap(); let next_block = BlockHeader::from_previous(genesis.header()); - let template = NewBlockTemplate::from_block(next_block.into_builder().build(), Difficulty::min(), 5000 * T); + let template = + NewBlockTemplate::from_block(next_block.into_builder().build(), Difficulty::min(), 5000 * T).unwrap(); let block = db.prepare_new_block(template).unwrap(); assert_eq!(block.header.height, 1); } diff --git a/base_layer/core/src/mempool/mempool_storage.rs b/base_layer/core/src/mempool/mempool_storage.rs index e00d970975..ccc9b73e93 100644 --- a/base_layer/core/src/mempool/mempool_storage.rs +++ b/base_layer/core/src/mempool/mempool_storage.rs @@ -39,7 +39,10 @@ use crate::{ StatsResponse, TxStorageResponse, }, - transactions::{transaction_components::Transaction, weight::TransactionWeight}, + transactions::{ + transaction_components::{Transaction, TransactionError}, + weight::TransactionWeight, + }, validation::{TransactionValidator, ValidationError}, }; @@ -69,7 +72,7 @@ impl MempoolStorage { } /// Insert an unconfirmed transaction into the Mempool. - pub fn insert(&mut self, tx: Arc) -> std::io::Result { + pub fn insert(&mut self, tx: Arc) -> Result { let tx_id = tx .body .kernels() @@ -77,12 +80,19 @@ impl MempoolStorage { .map(|k| k.excess_sig.get_signature().to_hex()) .unwrap_or_else(|| "None?!".into()); let timer = Instant::now(); + debug!(target: LOG_TARGET, "Inserting tx into mempool: {}", tx_id); + let tx_fee = match tx.body.get_total_fee() { + Ok(fee) => fee, + Err(e) => { + warn!(target: LOG_TARGET, "Invalid transaction: {}", e); + return Ok(TxStorageResponse::NotStoredConsensus); + }, + }; // This check is almost free, so lets check this before we do any expensive validation. - if tx.body.get_total_fee().as_u64() < self.unconfirmed_pool.config.min_fee { + if tx_fee.as_u64() < self.unconfirmed_pool.config.min_fee { debug!(target: LOG_TARGET, "Tx: ({}) fee too low, rejecting",tx_id); return Ok(TxStorageResponse::NotStoredFeeTooLow); } - debug!(target: LOG_TARGET, "Inserting tx into mempool: {}", tx_id); match self.validator.validate(&tx) { Ok(()) => { debug!( @@ -147,7 +157,7 @@ impl MempoolStorage { } // Insert a set of new transactions into the UTxPool. - fn insert_txs(&mut self, txs: Vec>) -> std::io::Result<()> { + fn insert_txs(&mut self, txs: Vec>) -> Result<(), TransactionError> { for tx in txs { self.insert(tx)?; } @@ -341,7 +351,7 @@ impl MempoolStorage { } /// Gathers and returns the stats of the Mempool. - pub fn stats(&self) -> std::io::Result { + pub fn stats(&self) -> Result { let weighting = self.get_transaction_weighting(); Ok(StatsResponse { unconfirmed_txs: self.unconfirmed_pool.len() as u64, diff --git a/base_layer/core/src/mempool/priority/prioritized_transaction.rs b/base_layer/core/src/mempool/priority/prioritized_transaction.rs index 8840d9d6df..ba39ff2dca 100644 --- a/base_layer/core/src/mempool/priority/prioritized_transaction.rs +++ b/base_layer/core/src/mempool/priority/prioritized_transaction.rs @@ -29,7 +29,10 @@ use std::{ use tari_common_types::types::{HashOutput, PrivateKey, PublicKey}; use tari_utilities::{hex::Hex, ByteArray}; -use crate::transactions::{transaction_components::Transaction, weight::TransactionWeight}; +use crate::transactions::{ + transaction_components::{Transaction, TransactionError}, + weight::TransactionWeight, +}; /// Create a unique unspent transaction priority based on the transaction fee, maturity of the oldest input UTXO and the /// excess_sig. The excess_sig is included to ensure the the priority key unique so it can be used with a BTreeMap. @@ -38,12 +41,12 @@ use crate::transactions::{transaction_components::Transaction, weight::Transacti pub struct FeePriority(Vec); impl FeePriority { - pub fn new(transaction: &Transaction, insert_epoch: u64, weight: u64) -> Self { + pub fn new(transaction: &Transaction, insert_epoch: u64, weight: u64) -> Result { // The weights have been normalised, so the fee priority is now equal to the fee per gram ± a few pct points // Include 3 decimal places before flooring #[allow(clippy::cast_possible_truncation)] #[allow(clippy::cast_sign_loss)] - let fee_per_byte = ((transaction.body.get_total_fee().as_u64() as f64 / weight as f64) * 1000.0) as u64; + let fee_per_byte = ((transaction.body.get_total_fee()?.as_u64() as f64 / weight as f64) * 1000.0) as u64; // Big-endian used here, the MSB is in the starting index. The ordering for Vec is taken from elements left // to right and the unconfirmed pool expects the lowest priority to be sorted lowest to highest in the // BTreeMap @@ -66,7 +69,7 @@ impl FeePriority { ); priority[16..48].copy_from_slice(agg_sig.as_bytes()); priority[48..80].copy_from_slice(agg_nonce.as_bytes()); - Self(priority) + Ok(Self(priority)) } } @@ -87,7 +90,7 @@ impl PrioritizedTransaction { weighting: &TransactionWeight, transaction: Arc, dependent_outputs: Option>, - ) -> std::io::Result { + ) -> Result { let weight = transaction.calculate_weight(weighting)?; let insert_epoch = match SystemTime::now().duration_since(UNIX_EPOCH) { Ok(n) => n.as_secs(), @@ -95,8 +98,8 @@ impl PrioritizedTransaction { }; Ok(Self { key, - priority: FeePriority::new(&transaction, insert_epoch, weight), - fee_per_byte: ((transaction.body.get_total_fee() * 1000) / weight).as_u64(), + priority: FeePriority::new(&transaction, insert_epoch, weight)?, + fee_per_byte: ((transaction.body.get_total_fee()? * 1000) / weight).as_u64(), weight, transaction, dependent_output_hashes: dependent_outputs.unwrap_or_default(), @@ -136,10 +139,10 @@ mod tests { let weighting = TransactionWeight::latest(); let epoch = u64::MAX / 2; let tx = create_tx_with_fee(2 * uT, &key_manager).await; - let p1 = FeePriority::new(&tx, epoch, tx.calculate_weight(&weighting).expect("Failed to get tx")); + let p1 = FeePriority::new(&tx, epoch, tx.calculate_weight(&weighting).expect("Failed to get tx")).unwrap(); let tx = create_tx_with_fee(3 * uT, &key_manager).await; - let p2 = FeePriority::new(&tx, epoch, tx.calculate_weight(&weighting).expect("Failed to get tx")); + let p2 = FeePriority::new(&tx, epoch, tx.calculate_weight(&weighting).expect("Failed to get tx")).unwrap(); assert!(p2 > p1); } @@ -150,14 +153,15 @@ mod tests { let weighting = TransactionWeight::latest(); let epoch = u64::MAX / 2; let tx = create_tx_with_fee(2 * uT, &key_manager).await; - let p1 = FeePriority::new(&tx, epoch, tx.calculate_weight(&weighting).expect("Failed to get tx")); + let p1 = FeePriority::new(&tx, epoch, tx.calculate_weight(&weighting).expect("Failed to get tx")).unwrap(); let tx = create_tx_with_fee(2 * uT, &key_manager).await; let p2 = FeePriority::new( &tx, epoch - 1, tx.calculate_weight(&weighting).expect("Failed to get tx"), - ); + ) + .unwrap(); assert!(p2 > p1); } diff --git a/base_layer/core/src/mempool/unconfirmed_pool/error.rs b/base_layer/core/src/mempool/unconfirmed_pool/error.rs index 2f7906173f..df9b5e3655 100644 --- a/base_layer/core/src/mempool/unconfirmed_pool/error.rs +++ b/base_layer/core/src/mempool/unconfirmed_pool/error.rs @@ -22,10 +22,14 @@ use thiserror::Error; +use crate::transactions::transaction_components::TransactionError; + #[derive(Debug, Error)] pub enum UnconfirmedPoolError { #[error("The HashMap and BTreeMap are out of sync")] StorageOutofSync, #[error("Transaction has no kernels")] TransactionNoKernels, + #[error("Transaction error: `{0}`")] + TransactionError(#[from] TransactionError), } diff --git a/base_layer/core/src/mempool/unconfirmed_pool/unconfirmed_pool.rs b/base_layer/core/src/mempool/unconfirmed_pool/unconfirmed_pool.rs index e16cc3881c..d58fea9655 100644 --- a/base_layer/core/src/mempool/unconfirmed_pool/unconfirmed_pool.rs +++ b/base_layer/core/src/mempool/unconfirmed_pool/unconfirmed_pool.rs @@ -39,7 +39,11 @@ use crate::{ FeePerGramStat, MempoolError, }, - transactions::{tari_amount::MicroMinotari, transaction_components::Transaction, weight::TransactionWeight}, + transactions::{ + tari_amount::MicroMinotari, + transaction_components::{Transaction, TransactionError}, + weight::TransactionWeight, + }, }; pub const LOG_TARGET: &str = "c::mp::unconfirmed_pool::unconfirmed_pool_storage"; @@ -117,7 +121,7 @@ impl UnconfirmedPool { tx: Arc, dependent_outputs: Option>, transaction_weighting: &TransactionWeight, - ) -> std::io::Result<()> { + ) -> Result<(), TransactionError> { if tx .body .kernels() @@ -165,7 +169,7 @@ impl UnconfirmedPool { &mut self, txs: I, transaction_weighting: &TransactionWeight, - ) -> std::io::Result<()> { + ) -> Result<(), TransactionError> { for tx in txs { self.insert(tx, None, transaction_weighting)?; } @@ -227,7 +231,7 @@ impl UnconfirmedPool { &mut depended_on, &mut recompute, prioritized_transaction.fee_per_byte, - ); + )?; if curr_skip_count >= self.config.weight_tx_skip_count { break; } @@ -286,7 +290,7 @@ impl UnconfirmedPool { &mut depended_on, &mut recompute, 0, - ); + )?; } if !transactions_to_remove_and_recheck.is_empty() { // we need to remove all transactions that need to be rechecked. @@ -321,7 +325,7 @@ impl UnconfirmedPool { depended_on: &mut HashMap>, recompute: &mut HashSet<&'a TransactionKey>, fee_per_byte_threshold: u64, - ) { + ) -> Result<(), TransactionError> { while match potentional_to_add.peek() { Some((fee_per_byte, _)) => *fee_per_byte >= fee_per_byte_threshold, None => false, @@ -355,7 +359,7 @@ impl UnconfirmedPool { complete_transaction_branch, depended_on, recompute, - ); + )?; } selected_txs.extend(candidate_transactions_to_select); } @@ -369,15 +373,16 @@ impl UnconfirmedPool { complete_transaction_branch.remove(&tx_key); depended_on.remove(&tx_key); } + Ok(()) } - pub fn remove_transaction_from_the_dependants<'a>( + fn remove_transaction_from_the_dependants<'a>( &self, tx_key: TransactionKey, complete_transaction_branch: &mut CompleteTransactionBranch, depended_on: &mut HashMap>, recompute: &mut HashSet<&'a TransactionKey>, - ) { + ) -> Result<(), TransactionError> { if let Some(txs) = depended_on.remove(&tx_key) { let prioritized_transaction = self .tx_by_key @@ -393,13 +398,14 @@ impl UnconfirmedPool { { update_candidate_transactions_to_select.remove(&tx_key); *update_total_transaction_weight -= prioritized_transaction.weight; - *update_total_transaction_fees -= prioritized_transaction.transaction.body.get_total_fee().0; + *update_total_transaction_fees -= prioritized_transaction.transaction.body.get_total_fee()?.0; // We mark it as recompute, we don't have to update the Heap, because it will never be // better as it was (see the note at the top of the function). recompute.insert(tx); } } } + Ok(()) } pub fn retrieve_by_excess_sigs( @@ -474,7 +480,7 @@ impl UnconfirmedPool { .insert(transaction.key, transaction.transaction.clone()) .is_none() { - *total_fees += transaction.transaction.body.get_total_fee().0; + *total_fees += transaction.transaction.body.get_total_fee()?.0; *total_weight += transaction.weight; } @@ -695,7 +701,7 @@ impl UnconfirmedPool { } /// Returns the total weight of all transactions stored in the pool. - pub fn calculate_weight(&self, transaction_weight: &TransactionWeight) -> std::io::Result { + pub fn calculate_weight(&self, transaction_weight: &TransactionWeight) -> Result { let weights = self .tx_by_key .values() @@ -732,7 +738,7 @@ impl UnconfirmedPool { break; } - let total_tx_fee = tx.transaction.body.get_total_fee(); + let total_tx_fee = tx.transaction.body.get_total_fee()?; offset += 1; let fee_per_gram = total_tx_fee / weight; min_fee_per_gram = min_fee_per_gram.min(fee_per_gram); diff --git a/base_layer/core/src/transactions/aggregated_body.rs b/base_layer/core/src/transactions/aggregated_body.rs index fe1e75a5d8..3472bb3da1 100644 --- a/base_layer/core/src/transactions/aggregated_body.rs +++ b/base_layer/core/src/transactions/aggregated_body.rs @@ -219,12 +219,14 @@ impl AggregateBody { Ok(()) } - pub fn get_total_fee(&self) -> MicroMinotari { + pub fn get_total_fee(&self) -> Result { let mut fee = MicroMinotari::from(0); for kernel in &self.kernels { - fee += kernel.fee; + fee = fee.checked_add(kernel.fee).ok_or(TransactionError::InvalidKernel( + "Aggregated body has greater fee than u64::MAX".to_string(), + ))?; } - fee + Ok(fee) } /// Run through the outputs of the block and check that @@ -329,8 +331,10 @@ impl AggregateBody { } /// Returns the weight in grams of a body - pub fn calculate_weight(&self, transaction_weight: &TransactionWeight) -> std::io::Result { - transaction_weight.calculate_body(self) + pub fn calculate_weight(&self, transaction_weight: &TransactionWeight) -> Result { + transaction_weight + .calculate_body(self) + .map_err(|e| TransactionError::SerializationError(e.to_string())) } pub fn sum_features_and_scripts_size(&self) -> std::io::Result { diff --git a/base_layer/core/src/transactions/transaction_components/error.rs b/base_layer/core/src/transactions/transaction_components/error.rs index 2df43d6353..e1cbf3c14d 100644 --- a/base_layer/core/src/transactions/transaction_components/error.rs +++ b/base_layer/core/src/transactions/transaction_components/error.rs @@ -39,6 +39,8 @@ use crate::transactions::transaction_components::EncryptedDataError; pub enum TransactionError { #[error("Error building the transaction: {0}")] BuilderError(String), + #[error("Error serializing transaction: {0}")] + SerializationError(String), #[error("Signature is invalid: {0}")] InvalidSignatureError(String), #[error("A range proof construction or verification has produced an error: {0}")] diff --git a/base_layer/core/src/transactions/transaction_components/transaction.rs b/base_layer/core/src/transactions/transaction_components/transaction.rs index 36de10217c..7d8dc00ed2 100644 --- a/base_layer/core/src/transactions/transaction_components/transaction.rs +++ b/base_layer/core/src/transactions/transaction_components/transaction.rs @@ -76,7 +76,7 @@ impl Transaction { } /// Returns the byte size or weight of a transaction - pub fn calculate_weight(&self, transaction_weight: &TransactionWeight) -> std::io::Result { + pub fn calculate_weight(&self, transaction_weight: &TransactionWeight) -> Result { self.body.calculate_weight(transaction_weight) } diff --git a/base_layer/core/tests/helpers/block_builders.rs b/base_layer/core/tests/helpers/block_builders.rs index 9e4eb87a54..f97f26992e 100644 --- a/base_layer/core/tests/helpers/block_builders.rs +++ b/base_layer/core/tests/helpers/block_builders.rs @@ -134,7 +134,8 @@ async fn genesis_template( header.into_builder().with_coinbase_utxo(utxo, kernel).build(), Difficulty::min(), coinbase_value, - ); + ) + .unwrap(); (block, output) } @@ -300,6 +301,7 @@ pub async fn chain_block( Difficulty::min(), reward, ) + .unwrap() } /// Create a new block using the provided coinbase and transactions that adds to the blockchain given in `prev_block`. @@ -322,6 +324,7 @@ pub fn chain_block_with_coinbase( Difficulty::min(), consensus.get_block_reward_at(height), ) + .unwrap() } /// Create a new block using the provided coinbase and transactions that adds to the blockchain given in `prev_block`. @@ -336,7 +339,7 @@ pub async fn chain_block_with_new_coinbase( let mut coinbase_value = consensus_manager.emission_schedule().block_reward(height); coinbase_value += transactions .iter() - .fold(MicroMinotari(0), |acc, x| acc + x.body.get_total_fee()); + .fold(MicroMinotari(0), |acc, x| acc + x.body.get_total_fee().unwrap()); let (coinbase_utxo, coinbase_kernel, coinbase_output) = create_coinbase( coinbase_value, height + consensus_manager.consensus_constants(height).coinbase_min_maturity(), @@ -358,7 +361,8 @@ pub async fn chain_block_with_new_coinbase( .build(), Difficulty::min(), reward, - ); + ) + .unwrap(); (template, coinbase_output) } @@ -389,9 +393,9 @@ pub async fn append_block_with_coinbase( ) -> Result<(ChainBlock, WalletOutput), ChainStorageError> { let height = prev_block.height() + 1; let mut coinbase_value = consensus_manager.emission_schedule().block_reward(height); - coinbase_value += txns - .iter() - .fold(MicroMinotari(0), |acc, x| acc + x.body.get_total_fee()); + for tx in &txns { + coinbase_value += tx.body.get_total_fee()?; + } let (coinbase_utxo, coinbase_kernel, coinbase_output) = create_coinbase( coinbase_value, height + consensus_manager.consensus_constants(0).coinbase_min_maturity(), @@ -465,7 +469,7 @@ pub async fn generate_new_block_with_coinbase( let mut fees = MicroMinotari(0); for schema in schemas { let (tx, mut utxos) = spend_utxos(schema, key_manager).await; - fees += tx.body.get_total_fee(); + fees += tx.body.get_total_fee()?; txns.push(tx); block_utxos.append(&mut utxos); } diff --git a/base_layer/core/tests/helpers/database.rs b/base_layer/core/tests/helpers/database.rs index 17e6a68dac..2bb0ebc03c 100644 --- a/base_layer/core/tests/helpers/database.rs +++ b/base_layer/core/tests/helpers/database.rs @@ -44,7 +44,7 @@ pub async fn create_orphan_block( let lock_height = consensus.consensus_constants(block_height).coinbase_min_maturity(); coinbase_value += transactions .iter() - .fold(MicroMinotari(0), |acc, x| acc + x.body.get_total_fee()); + .fold(MicroMinotari(0), |acc, x| acc + x.body.get_total_fee().unwrap()); let (coinbase_utxo, coinbase_kernel, _coinbase_output) = create_coinbase(coinbase_value, block_height + lock_height, None, key_manager).await; let mut header = BlockHeader::new(consensus.consensus_constants(block_height).blockchain_version()); @@ -59,7 +59,8 @@ pub async fn create_orphan_block( .build(), Difficulty::min(), coinbase_value, - ); + ) + .unwrap(); Block::new(template.header.into(), template.body) } diff --git a/base_layer/core/tests/tests/block_validation.rs b/base_layer/core/tests/tests/block_validation.rs index 7b702a20b0..9ad1fa192d 100644 --- a/base_layer/core/tests/tests/block_validation.rs +++ b/base_layer/core/tests/tests/block_validation.rs @@ -414,7 +414,7 @@ async fn test_orphan_validator() { // let break coinbase lock height let (coinbase_utxo, coinbase_kernel, _) = create_coinbase( - rules.get_block_reward_at(1) + tx01.body.get_total_fee() + tx02.body.get_total_fee(), + rules.get_block_reward_at(1) + tx01.body.get_total_fee().unwrap() + tx02.body.get_total_fee().unwrap(), 1, None, &key_manager, @@ -988,7 +988,7 @@ async fn test_block_sync_body_validator() { // let break coinbase lock height let (coinbase_utxo, coinbase_kernel, _) = create_coinbase( - rules.get_block_reward_at(1) + tx01.body.get_total_fee() + tx02.body.get_total_fee(), + rules.get_block_reward_at(1) + tx01.body.get_total_fee().unwrap() + tx02.body.get_total_fee().unwrap(), 1 + rules.consensus_constants(1).coinbase_min_maturity(), None, &key_manager, diff --git a/base_layer/core/tests/tests/node_service.rs b/base_layer/core/tests/tests/node_service.rs index 17008373f2..ed483e5f75 100644 --- a/base_layer/core/tests/tests/node_service.rs +++ b/base_layer/core/tests/tests/node_service.rs @@ -600,7 +600,7 @@ async fn local_get_new_block_with_zero_conf() { .unwrap(); assert_eq!(block_template.header.height, 1); assert_eq!(block_template.body.kernels().len(), 4); - let coinbase_value = rules.get_block_reward_at(1) + block_template.body.get_total_fee(); + let coinbase_value = rules.get_block_reward_at(1) + block_template.body.get_total_fee().unwrap(); let (output, kernel, _) = create_coinbase( coinbase_value, rules.consensus_constants(1).coinbase_min_maturity() + 1, @@ -681,7 +681,7 @@ async fn local_get_new_block_with_combined_transaction() { .unwrap(); assert_eq!(block_template.header.height, 1); assert_eq!(block_template.body.kernels().len(), 4); - let coinbase_value = rules.get_block_reward_at(1) + block_template.body.get_total_fee(); + let coinbase_value = rules.get_block_reward_at(1) + block_template.body.get_total_fee().unwrap(); let (output, kernel, _) = create_coinbase( coinbase_value, rules.consensus_constants(1).coinbase_min_maturity() + 1, diff --git a/base_layer/wallet/src/transaction_service/handle.rs b/base_layer/wallet/src/transaction_service/handle.rs index e65f595a51..9f8c127916 100644 --- a/base_layer/wallet/src/transaction_service/handle.rs +++ b/base_layer/wallet/src/transaction_service/handle.rs @@ -781,7 +781,7 @@ impl TransactionServiceHandle { amount: MicroMinotari, message: String, ) -> Result<(), TransactionServiceError> { - let fee = tx.body.get_total_fee(); + let fee = tx.body.get_total_fee()?; match self .handle .call(TransactionServiceRequest::SubmitTransactionToSelf( diff --git a/base_layer/wallet/src/transaction_service/protocols/transaction_receive_protocol.rs b/base_layer/wallet/src/transaction_service/protocols/transaction_receive_protocol.rs index ecd9766531..b7942e337e 100644 --- a/base_layer/wallet/src/transaction_service/protocols/transaction_receive_protocol.rs +++ b/base_layer/wallet/src/transaction_service/protocols/transaction_receive_protocol.rs @@ -424,7 +424,10 @@ where self.source_address.clone(), self.resources.wallet_identity.address.clone(), inbound_tx.amount, - finalized_transaction.body.get_total_fee(), + finalized_transaction + .body + .get_total_fee() + .map_err(|e| TransactionServiceProtocolError::new(self.id, TransactionServiceError::from(e)))?, finalized_transaction.clone(), TransactionStatus::Completed, inbound_tx.message.clone(), diff --git a/base_layer/wallet/tests/output_manager_service_tests/service.rs b/base_layer/wallet/tests/output_manager_service_tests/service.rs index aff4db5e30..e8ac79f023 100644 --- a/base_layer/wallet/tests/output_manager_service_tests/service.rs +++ b/base_layer/wallet/tests/output_manager_service_tests/service.rs @@ -497,7 +497,7 @@ async fn test_utxo_selection_no_chain_metadata() { .expect("Failed to get default features and scripts size byte size") * 6, ); - assert_eq!(tx.body.get_total_fee(), expected_fee); + assert_eq!(tx.body.get_total_fee().unwrap(), expected_fee); assert_eq!(utxos_total_value, MicroMinotari::from(5_000)); // test that largest utxo was encumbered @@ -599,7 +599,7 @@ async fn test_utxo_selection_with_chain_metadata() { .expect("Failed to get default features and scripts size byte size") * 6, ); - assert_eq!(tx.body.get_total_fee(), expected_fee); + assert_eq!(tx.body.get_total_fee().unwrap(), expected_fee); // test that largest spendable utxo was encumbered let utxos = oms.get_unspent_outputs().await.unwrap(); @@ -1192,7 +1192,7 @@ async fn coin_split_with_change() { default_features_and_scripts_size_byte_size() .expect("Failed to get default features and scripts size byte size"), ); - assert_eq!(coin_split_tx.body.get_total_fee(), expected_fee); + assert_eq!(coin_split_tx.body.get_total_fee().unwrap(), expected_fee); // NOTE: assuming the LargestFirst strategy is used assert_eq!(amount, val3); } @@ -1234,7 +1234,7 @@ async fn coin_split_no_change() { .unwrap(); assert_eq!(coin_split_tx.body.inputs().len(), 3); assert_eq!(coin_split_tx.body.outputs().len(), split_count); - assert_eq!(coin_split_tx.body.get_total_fee(), expected_fee); + assert_eq!(coin_split_tx.body.get_total_fee().unwrap(), expected_fee); assert_eq!(amount, val1 + val2 + val3); } From acd1eae464565edeb3f95b9e48deea5903343fae Mon Sep 17 00:00:00 2001 From: SW van Heerden Date: Thu, 21 Sep 2023 16:59:30 +0200 Subject: [PATCH 2/2] fix mempool issues --- .../core/src/mempool/mempool_storage.rs | 10 +- .../priority/prioritized_transaction.rs | 8 +- .../src/mempool/unconfirmed_pool/error.rs | 2 + .../unconfirmed_pool/unconfirmed_pool.rs | 153 +++++++++++++----- 4 files changed, 118 insertions(+), 55 deletions(-) diff --git a/base_layer/core/src/mempool/mempool_storage.rs b/base_layer/core/src/mempool/mempool_storage.rs index ccc9b73e93..b7f99c6277 100644 --- a/base_layer/core/src/mempool/mempool_storage.rs +++ b/base_layer/core/src/mempool/mempool_storage.rs @@ -32,7 +32,7 @@ use crate::{ mempool::{ error::MempoolError, reorg_pool::ReorgPool, - unconfirmed_pool::UnconfirmedPool, + unconfirmed_pool::{UnconfirmedPool, UnconfirmedPoolError}, FeePerGramStat, MempoolConfig, StateResponse, @@ -72,7 +72,7 @@ impl MempoolStorage { } /// Insert an unconfirmed transaction into the Mempool. - pub fn insert(&mut self, tx: Arc) -> Result { + pub fn insert(&mut self, tx: Arc) -> Result { let tx_id = tx .body .kernels() @@ -157,7 +157,7 @@ impl MempoolStorage { } // Insert a set of new transactions into the UTxPool. - fn insert_txs(&mut self, txs: Vec>) -> Result<(), TransactionError> { + fn insert_txs(&mut self, txs: Vec>) -> Result<(), UnconfirmedPoolError> { for tx in txs { self.insert(tx)?; } @@ -177,7 +177,7 @@ impl MempoolStorage { // Move published txs to ReOrgPool and discard double spends let removed_transactions = self .unconfirmed_pool - .remove_published_and_discard_deprecated_transactions(published_block); + .remove_published_and_discard_deprecated_transactions(published_block)?; debug!( target: LOG_TARGET, "{} transactions removed from unconfirmed pool in {:.2?}, moving them to reorg pool for block #{} ({}) {}", @@ -220,7 +220,7 @@ impl MempoolStorage { ); let txs = self .unconfirmed_pool - .remove_published_and_discard_deprecated_transactions(failed_block); + .remove_published_and_discard_deprecated_transactions(failed_block)?; // Reinsert them to validate if they are still valid self.insert_txs(txs) diff --git a/base_layer/core/src/mempool/priority/prioritized_transaction.rs b/base_layer/core/src/mempool/priority/prioritized_transaction.rs index ba39ff2dca..3cb3bb2661 100644 --- a/base_layer/core/src/mempool/priority/prioritized_transaction.rs +++ b/base_layer/core/src/mempool/priority/prioritized_transaction.rs @@ -42,11 +42,7 @@ pub struct FeePriority(Vec); impl FeePriority { pub fn new(transaction: &Transaction, insert_epoch: u64, weight: u64) -> Result { - // The weights have been normalised, so the fee priority is now equal to the fee per gram ± a few pct points - // Include 3 decimal places before flooring - #[allow(clippy::cast_possible_truncation)] - #[allow(clippy::cast_sign_loss)] - let fee_per_byte = ((transaction.body.get_total_fee()?.as_u64() as f64 / weight as f64) * 1000.0) as u64; + let fee_per_byte = transaction.body.get_total_fee()?.as_u64().saturating_mul(1000) / weight; // Big-endian used here, the MSB is in the starting index. The ordering for Vec is taken from elements left // to right and the unconfirmed pool expects the lowest priority to be sorted lowest to highest in the // BTreeMap @@ -99,7 +95,7 @@ impl PrioritizedTransaction { Ok(Self { key, priority: FeePriority::new(&transaction, insert_epoch, weight)?, - fee_per_byte: ((transaction.body.get_total_fee()? * 1000) / weight).as_u64(), + fee_per_byte: transaction.body.get_total_fee()?.as_u64().saturating_mul(1000) / weight, weight, transaction, dependent_output_hashes: dependent_outputs.unwrap_or_default(), diff --git a/base_layer/core/src/mempool/unconfirmed_pool/error.rs b/base_layer/core/src/mempool/unconfirmed_pool/error.rs index df9b5e3655..15322e35c4 100644 --- a/base_layer/core/src/mempool/unconfirmed_pool/error.rs +++ b/base_layer/core/src/mempool/unconfirmed_pool/error.rs @@ -28,6 +28,8 @@ use crate::transactions::transaction_components::TransactionError; pub enum UnconfirmedPoolError { #[error("The HashMap and BTreeMap are out of sync")] StorageOutofSync, + #[error("Mempool encountered an internal error: {0}")] + InternalError(String), #[error("Transaction has no kernels")] TransactionNoKernels, #[error("Transaction error: `{0}`")] diff --git a/base_layer/core/src/mempool/unconfirmed_pool/unconfirmed_pool.rs b/base_layer/core/src/mempool/unconfirmed_pool/unconfirmed_pool.rs index d58fea9655..464b41ccb4 100644 --- a/base_layer/core/src/mempool/unconfirmed_pool/unconfirmed_pool.rs +++ b/base_layer/core/src/mempool/unconfirmed_pool/unconfirmed_pool.rs @@ -121,7 +121,7 @@ impl UnconfirmedPool { tx: Arc, dependent_outputs: Option>, transaction_weighting: &TransactionWeight, - ) -> Result<(), TransactionError> { + ) -> Result<(), UnconfirmedPoolError> { if tx .body .kernels() @@ -134,10 +134,10 @@ impl UnconfirmedPool { let new_key = self.get_next_key(); let prioritized_tx = PrioritizedTransaction::new(new_key, transaction_weighting, tx, dependent_outputs)?; if self.tx_by_key.len() >= self.config.storage_capacity { - if prioritized_tx.priority < *self.lowest_priority() { + if prioritized_tx.priority < *self.lowest_priority()? { return Ok(()); } - self.remove_lowest_priority_tx(); + self.remove_lowest_priority_tx()?; } self.tx_by_priority.insert(prioritized_tx.priority.clone(), new_key); @@ -169,7 +169,7 @@ impl UnconfirmedPool { &mut self, txs: I, transaction_weighting: &TransactionWeight, - ) -> Result<(), TransactionError> { + ) -> Result<(), UnconfirmedPoolError> { for tx in txs { self.insert(tx, None, transaction_weighting)?; } @@ -248,7 +248,12 @@ impl UnconfirmedPool { &mut total_transaction_fees, &mut unique_ids, )?; - let total_weight_after_candidates = curr_weight + total_transaction_weight; + let total_weight_after_candidates = + curr_weight + .checked_add(total_transaction_weight) + .ok_or(UnconfirmedPoolError::InternalError( + "Overflow when calculating transaction weights".to_string(), + ))?; if total_weight_after_candidates <= total_weight && potential_transactions_to_remove_and_recheck.is_empty() { for dependend_on_tx_key in candidate_transactions_to_select.keys() { @@ -260,7 +265,7 @@ impl UnconfirmedPool { .or_insert_with(|| vec![tx_key]); } } - let fee_per_byte = (total_transaction_fees * 1000) / total_transaction_weight; + let fee_per_byte = total_transaction_fees.saturating_mul(1000) / total_transaction_weight; complete_transaction_branch.insert( *tx_key, ( @@ -301,7 +306,7 @@ impl UnconfirmedPool { ); } for (tx_key, _) in &transactions_to_remove_and_recheck { - self.remove_transaction(*tx_key); + self.remove_transaction(*tx_key)?; } let results = RetrieveResults { @@ -325,13 +330,13 @@ impl UnconfirmedPool { depended_on: &mut HashMap>, recompute: &mut HashSet<&'a TransactionKey>, fee_per_byte_threshold: u64, - ) -> Result<(), TransactionError> { + ) -> Result<(), UnconfirmedPoolError> { while match potentional_to_add.peek() { Some((fee_per_byte, _)) => *fee_per_byte >= fee_per_byte_threshold, None => false, } { // If the current TXs has lower fee than the ones we already processed, we can add some. - let (_fee_per_byte, tx_key) = potentional_to_add.pop().unwrap(); // Safe, we already checked we have some. + let (_fee_per_byte, tx_key) = potentional_to_add.pop().ok_or(UnconfirmedPoolError::StorageOutofSync)?; if selected_txs.contains_key(&tx_key) { continue; } @@ -339,19 +344,29 @@ impl UnconfirmedPool { if recompute.contains(&tx_key) { recompute.remove(&tx_key); // So we recompute the total fees based on updated weights and fees. - let (_, total_transaction_weight, total_transaction_fees) = - complete_transaction_branch.get(&tx_key).unwrap(); - let fee_per_byte = (*total_transaction_fees * 1000) / *total_transaction_weight; + let (_, total_transaction_weight, total_transaction_fees) = complete_transaction_branch + .get(&tx_key) + .ok_or(UnconfirmedPoolError::StorageOutofSync)?; + let fee_per_byte = total_transaction_fees.saturating_mul(1000) / *total_transaction_weight; potentional_to_add.push((fee_per_byte, tx_key)); continue; } let (candidate_transactions_to_select, total_transaction_weight, _total_transaction_fees) = - complete_transaction_branch.remove(&tx_key).unwrap(); - - let total_weight_after_candidates = *curr_weight + total_transaction_weight; + complete_transaction_branch + .remove(&tx_key) + .ok_or(UnconfirmedPoolError::StorageOutofSync)?; + + let total_weight_after_candidates = + curr_weight + .checked_add(total_transaction_weight) + .ok_or(UnconfirmedPoolError::InternalError( + "Overflow when calculating total weights".to_string(), + ))?; if total_weight_after_candidates <= total_weight { if !UnconfirmedPool::find_duplicate_input(selected_txs, &candidate_transactions_to_select) { - *curr_weight += total_transaction_weight; + *curr_weight = curr_weight.checked_add(total_transaction_weight).ok_or( + UnconfirmedPoolError::InternalError("Overflow when calculating total weights".to_string()), + )?; // So we processed the transaction, let's mark the dependents to be recomputed. for tx_key in candidate_transactions_to_select.keys() { self.remove_transaction_from_the_dependants( @@ -382,13 +397,12 @@ impl UnconfirmedPool { complete_transaction_branch: &mut CompleteTransactionBranch, depended_on: &mut HashMap>, recompute: &mut HashSet<&'a TransactionKey>, - ) -> Result<(), TransactionError> { + ) -> Result<(), UnconfirmedPoolError> { if let Some(txs) = depended_on.remove(&tx_key) { let prioritized_transaction = self .tx_by_key .get(&tx_key) - .ok_or(UnconfirmedPoolError::StorageOutofSync) - .unwrap(); + .ok_or(UnconfirmedPoolError::StorageOutofSync)?; for tx in txs { if let Some(( update_candidate_transactions_to_select, @@ -397,8 +411,12 @@ impl UnconfirmedPool { )) = complete_transaction_branch.get_mut(tx) { update_candidate_transactions_to_select.remove(&tx_key); - *update_total_transaction_weight -= prioritized_transaction.weight; - *update_total_transaction_fees -= prioritized_transaction.transaction.body.get_total_fee()?.0; + *update_total_transaction_weight = update_total_transaction_weight + .checked_sub(prioritized_transaction.weight) + .ok_or(UnconfirmedPoolError::StorageOutofSync)?; + *update_total_transaction_fees = update_total_transaction_fees + .checked_sub(prioritized_transaction.transaction.body.get_total_fee()?.0) + .ok_or(UnconfirmedPoolError::StorageOutofSync)?; // We mark it as recompute, we don't have to update the Heap, because it will never be // better as it was (see the note at the top of the function). recompute.insert(tx); @@ -480,8 +498,16 @@ impl UnconfirmedPool { .insert(transaction.key, transaction.transaction.clone()) .is_none() { - *total_fees += transaction.transaction.body.get_total_fee()?.0; - *total_weight += transaction.weight; + *total_fees = total_fees + .checked_add(transaction.transaction.body.get_total_fee()?.0) + .ok_or(UnconfirmedPoolError::InternalError( + "Overflow when calculating total fees".to_string(), + ))?; + *total_weight = total_weight + .checked_add(transaction.weight) + .ok_or(UnconfirmedPoolError::InternalError( + "Overflow when calculating total weights".to_string(), + ))?; } Ok(()) @@ -528,17 +554,18 @@ impl UnconfirmedPool { false } - fn lowest_priority(&self) -> &FeePriority { + fn lowest_priority(&self) -> Result<&FeePriority, UnconfirmedPoolError> { self.tx_by_priority .keys() .next() - .expect("lowest_priority called on empty mempool") + .ok_or(UnconfirmedPoolError::StorageOutofSync) } - fn remove_lowest_priority_tx(&mut self) { + fn remove_lowest_priority_tx(&mut self) -> Result<(), UnconfirmedPoolError> { if let Some(tx_key) = self.tx_by_priority.values().next().copied() { - self.remove_transaction(tx_key); + self.remove_transaction(tx_key)?; } + Ok(()) } /// Remove all current mempool transactions from the UnconfirmedPoolStorage, returning that which have been removed @@ -553,7 +580,7 @@ impl UnconfirmedPool { pub fn remove_published_and_discard_deprecated_transactions( &mut self, published_block: &Block, - ) -> Vec> { + ) -> Result>, UnconfirmedPoolError> { trace!( target: LOG_TARGET, "Searching for transactions to remove from unconfirmed pool in block {} ({})", @@ -578,8 +605,12 @@ impl UnconfirmedPool { removed_transactions = to_remove .iter() - .filter_map(|key| self.remove_transaction(*key)) - .collect::>(); + .filter_map(|key| match self.remove_transaction(*key) { + Err(e) => Some(Err(e)), + Ok(Some(v)) => Some(Ok(v)), + Ok(None) => None, + }) + .collect::, _>>()?; debug!( target: LOG_TARGET, "Found {} transactions with matching kernel sigs from unconfirmed pool in {:.2?}", @@ -607,7 +638,17 @@ impl UnconfirmedPool { .map(|(key, _)| *key), ); - removed_transactions.extend(to_remove.iter().filter_map(|key| self.remove_transaction(*key))); + removed_transactions.extend( + to_remove + .iter() + .filter_map(|key| match self.remove_transaction(*key) { + Err(e) => Some(Err(e)), + Ok(Some(v)) => Some(Ok(v)), + Ok(None) => None, + }) + .collect::, _>>()? + .into_iter(), + ); debug!( target: LOG_TARGET, "Found {} transactions with matching inputs from unconfirmed pool in {:.2?}", @@ -631,7 +672,17 @@ impl UnconfirmedPool { .copied(), ); - removed_transactions.extend(to_remove.iter().filter_map(|key| self.remove_transaction(*key))); + removed_transactions.extend( + to_remove + .iter() + .filter_map(|key| match self.remove_transaction(*key) { + Err(e) => Some(Err(e)), + Ok(Some(v)) => Some(Ok(v)), + Ok(None) => None, + }) + .collect::, _>>()? + .into_iter(), + ); debug!( target: LOG_TARGET, "Found {} transactions with matching outputs from unconfirmed pool in {:.2?}", @@ -640,7 +691,7 @@ impl UnconfirmedPool { ); } - removed_transactions + Ok(removed_transactions) } /// Searches a block and transaction for matching inputs @@ -654,15 +705,21 @@ impl UnconfirmedPool { } /// Ensures that all transactions are safely deleted in order and from all storage - fn remove_transaction(&mut self, tx_key: TransactionKey) -> Option> { - let prioritized_transaction = self.tx_by_key.remove(&tx_key)?; + fn remove_transaction(&mut self, tx_key: TransactionKey) -> Result>, UnconfirmedPoolError> { + let prioritized_transaction = match self.tx_by_key.remove(&tx_key) { + Some(tx) => tx, + None => return Ok(None), + }; self.tx_by_priority.remove(&prioritized_transaction.priority); for kernel in prioritized_transaction.transaction.body.kernels() { let sig = kernel.excess_sig.get_signature(); if let Some(keys) = self.txs_by_signature.get_mut(sig) { - let pos = keys.iter().position(|k| *k == tx_key).expect("mempool out of sync"); + let pos = keys + .iter() + .position(|k| *k == tx_key) + .ok_or(UnconfirmedPoolError::StorageOutofSync)?; keys.remove(pos); if keys.is_empty() { self.txs_by_signature.remove(sig); @@ -687,7 +744,7 @@ impl UnconfirmedPool { "Deleted transaction: {}", &prioritized_transaction.transaction ); - Some(prioritized_transaction.transaction) + Ok(Some(prioritized_transaction.transaction)) } /// Returns the total number of unconfirmed transactions stored in the UnconfirmedPool. @@ -726,7 +783,7 @@ impl UnconfirmedPool { let mut stats = Vec::new(); let mut offset = 0usize; for start in 0..count { - let mut total_weight = 0; + let mut total_weight: u64 = 0; let mut total_fees = MicroMinotari::zero(); let mut min_fee_per_gram = MicroMinotari::from(u64::MAX); let mut max_fee_per_gram = MicroMinotari::zero(); @@ -734,7 +791,7 @@ impl UnconfirmedPool { let tx = self.tx_by_key.get(key).ok_or(UnconfirmedPoolError::StorageOutofSync)?; let weight = tx.weight; - if total_weight + weight > target_block_weight { + if total_weight.saturating_add(weight) > target_block_weight { break; } @@ -743,8 +800,16 @@ impl UnconfirmedPool { let fee_per_gram = total_tx_fee / weight; min_fee_per_gram = min_fee_per_gram.min(fee_per_gram); max_fee_per_gram = max_fee_per_gram.max(fee_per_gram); - total_fees += total_tx_fee; - total_weight += weight; + total_fees = total_fees + .checked_add(total_tx_fee) + .ok_or(UnconfirmedPoolError::InternalError( + "Overflow when calculating total fees".to_string(), + ))?; + total_weight = total_weight + .checked_add(weight) + .ok_or(UnconfirmedPoolError::InternalError( + "Overflow when calculating total weights".to_string(), + ))?; } if total_weight == 0 { break; @@ -799,7 +864,7 @@ impl UnconfirmedPool { "Shrunk reorg mempool memory usage ({}/{}) ~{}%", new, old, - (((old - new) as f32 / old as f32) * 100.0).round() as usize + (old - new).saturating_mul(100) / old as usize ); } } @@ -1214,14 +1279,14 @@ mod test { .unwrap() .first() .unwrap(); - unconfirmed_pool.remove_transaction(k); + unconfirmed_pool.remove_transaction(k).unwrap(); let k = *unconfirmed_pool .txs_by_signature .get(tx4.first_kernel_excess_sig().unwrap().get_signature()) .unwrap() .first() .unwrap(); - unconfirmed_pool.remove_transaction(k); + unconfirmed_pool.remove_transaction(k).unwrap(); let txns = vec![ Arc::new(tx2),