From e5ce823c609e5b9a37ca5e20d4284d9c26da9c5d Mon Sep 17 00:00:00 2001 From: Hansie Odendaal Date: Wed, 27 Mar 2024 11:50:23 +0200 Subject: [PATCH 1/2] Prevent mempool panic Under certain conditions it is possible to let the mempool panic when is inserting an empty transaction. This PR prevents it. --- .../priority/prioritized_transaction.rs | 23 +++++++++++++++++++ .../transaction_components/error.rs | 2 ++ base_layer/core/src/transactions/weight.rs | 7 ++++++ 3 files changed, 32 insertions(+) diff --git a/base_layer/core/src/mempool/priority/prioritized_transaction.rs b/base_layer/core/src/mempool/priority/prioritized_transaction.rs index d656a8cfd6..d90249279a 100644 --- a/base_layer/core/src/mempool/priority/prioritized_transaction.rs +++ b/base_layer/core/src/mempool/priority/prioritized_transaction.rs @@ -88,6 +88,9 @@ impl PrioritizedTransaction { dependent_outputs: Option>, ) -> Result { let weight = transaction.calculate_weight(weighting)?; + if weight == 0 { + return Err(TransactionError::ZeroWeight); + } let insert_epoch = match SystemTime::now().duration_since(UNIX_EPOCH) { Ok(n) => n.as_secs(), Err(_) => 0, @@ -162,4 +165,24 @@ mod tests { assert!(p2 > p1); } + + #[test] + fn empty_transaction() { + let weighting = TransactionWeight::latest(); + match PrioritizedTransaction::new( + 0, + &weighting, + Arc::new(Transaction::new( + vec![], + vec![], + vec![], + Default::default(), + Default::default(), + )), + None, + ) { + Ok(_) => panic!("Empty transaction should not be valid"), + Err(e) => assert_eq!(e, TransactionError::ZeroWeight), + } + } } diff --git a/base_layer/core/src/transactions/transaction_components/error.rs b/base_layer/core/src/transactions/transaction_components/error.rs index f2d4b84b34..ab038bffa5 100644 --- a/base_layer/core/src/transactions/transaction_components/error.rs +++ b/base_layer/core/src/transactions/transaction_components/error.rs @@ -75,6 +75,8 @@ pub enum TransactionError { EncryptedDataError(String), #[error("Ledger device error: {0}")] LedgerDeviceError(#[from] LedgerDeviceError), + #[error("Transaction has a zero weight, not possible")] + ZeroWeight, } impl From for TransactionError { diff --git a/base_layer/core/src/transactions/weight.rs b/base_layer/core/src/transactions/weight.rs index 8d9fa9d87a..72f91f0409 100644 --- a/base_layer/core/src/transactions/weight.rs +++ b/base_layer/core/src/transactions/weight.rs @@ -163,4 +163,11 @@ mod test { ); } } + + #[test] + fn empty_body_weight() { + let weighting = TransactionWeight::latest(); + let body = AggregateBody::empty(); + assert_eq!(weighting.calculate_body(&body).unwrap(), 0); + } } From f90d99d9e92bab5614825dad140ae37e3987e7ff Mon Sep 17 00:00:00 2001 From: Hansie Odendaal Date: Thu, 28 Mar 2024 05:26:50 +0200 Subject: [PATCH 2/2] review comments --- .../priority/prioritized_transaction.rs | 34 +++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/base_layer/core/src/mempool/priority/prioritized_transaction.rs b/base_layer/core/src/mempool/priority/prioritized_transaction.rs index d90249279a..0c78db88b7 100644 --- a/base_layer/core/src/mempool/priority/prioritized_transaction.rs +++ b/base_layer/core/src/mempool/priority/prioritized_transaction.rs @@ -42,7 +42,13 @@ pub struct FeePriority(Vec); impl FeePriority { pub fn new(transaction: &Transaction, insert_epoch: u64, weight: u64) -> Result { - let fee_per_byte = transaction.body.get_total_fee()?.as_u64().saturating_mul(1000) / weight; + let fee_per_byte = transaction + .body + .get_total_fee()? + .as_u64() + .saturating_mul(1000) + .checked_div(weight) + .ok_or(TransactionError::ZeroWeight)?; // 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 @@ -88,9 +94,6 @@ impl PrioritizedTransaction { dependent_outputs: Option>, ) -> Result { let weight = transaction.calculate_weight(weighting)?; - if weight == 0 { - return Err(TransactionError::ZeroWeight); - } let insert_epoch = match SystemTime::now().duration_since(UNIX_EPOCH) { Ok(n) => n.as_secs(), Err(_) => 0, @@ -98,7 +101,13 @@ impl PrioritizedTransaction { Ok(Self { key, priority: FeePriority::new(&transaction, insert_epoch, weight)?, - fee_per_byte: transaction.body.get_total_fee()?.as_u64().saturating_mul(1000) / weight, + fee_per_byte: transaction + .body + .get_total_fee()? + .as_u64() + .saturating_mul(1000) + .checked_div(weight) + .ok_or(TransactionError::ZeroWeight)?, weight, transaction, dependent_output_hashes: dependent_outputs.unwrap_or_default(), @@ -167,7 +176,7 @@ mod tests { } #[test] - fn empty_transaction() { + fn prioritized_from_empty_transaction() { let weighting = TransactionWeight::latest(); match PrioritizedTransaction::new( 0, @@ -185,4 +194,17 @@ mod tests { Err(e) => assert_eq!(e, TransactionError::ZeroWeight), } } + + #[test] + fn fee_priority_with_zero_weight() { + let weight = 0; + match FeePriority::new( + &Transaction::new(vec![], vec![], vec![], Default::default(), Default::default()), + SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs(), + weight, + ) { + Ok(_) => panic!("Empty transaction should not be valid"), + Err(e) => assert_eq!(e, TransactionError::ZeroWeight), + } + } }