Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: potential overflow #5778

Merged
merged 6 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1176,7 +1176,7 @@ impl CompletedTransactionInfo {
pub fn from_completed_transaction(
tx: CompletedTransaction,
transaction_weighting: &TransactionWeight,
) -> std::io::Result<Self> {
) -> Result<Self, TransactionError> {
let excess_signature = tx
.transaction
.first_kernel_excess_sig()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
3 changes: 3 additions & 0 deletions base_layer/core/src/base_node/comms_interface/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use crate::{
consensus::ConsensusManagerError,
mempool::MempoolError,
proof_of_work::{monero_rx::MergeMineError, DifficultyError},
transactions::transaction_components::TransactionError,
};

#[derive(Debug, Error)]
Expand Down Expand Up @@ -75,4 +76,6 @@ pub enum CommsInterfaceError {
MergeMineError(#[from] MergeMineError),
#[error("Invalid difficulty: {0}")]
DifficultyError(#[from] DifficultyError),
#[error("Transaction error: {0}")]
TransactionError(#[from] TransactionError),
}
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
4 changes: 4 additions & 0 deletions base_layer/core/src/base_node/service/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
18 changes: 13 additions & 5 deletions base_layer/core/src/blocks/new_block_template.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<Self, TransactionError> {
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,
}
})
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 { .. }));
}
Expand All @@ -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();
Expand All @@ -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);
}
Expand Down
22 changes: 16 additions & 6 deletions base_layer/core/src/mempool/mempool_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ use crate::{
StatsResponse,
TxStorageResponse,
},
transactions::{transaction_components::Transaction, weight::TransactionWeight},
transactions::{
transaction_components::{Transaction, TransactionError},
weight::TransactionWeight,
},
validation::{TransactionValidator, ValidationError},
};

Expand Down Expand Up @@ -69,20 +72,27 @@ impl MempoolStorage {
}

/// Insert an unconfirmed transaction into the Mempool.
pub fn insert(&mut self, tx: Arc<Transaction>) -> std::io::Result<TxStorageResponse> {
pub fn insert(&mut self, tx: Arc<Transaction>) -> Result<TxStorageResponse, TransactionError> {
let tx_id = tx
.body
.kernels()
.first()
.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!(
Expand Down Expand Up @@ -147,7 +157,7 @@ impl MempoolStorage {
}

// Insert a set of new transactions into the UTxPool.
fn insert_txs(&mut self, txs: Vec<Arc<Transaction>>) -> std::io::Result<()> {
fn insert_txs(&mut self, txs: Vec<Arc<Transaction>>) -> Result<(), TransactionError> {
for tx in txs {
self.insert(tx)?;
}
Expand Down Expand Up @@ -341,7 +351,7 @@ impl MempoolStorage {
}

/// Gathers and returns the stats of the Mempool.
pub fn stats(&self) -> std::io::Result<StatsResponse> {
pub fn stats(&self) -> Result<StatsResponse, TransactionError> {
let weighting = self.get_transaction_weighting();
Ok(StatsResponse {
unconfirmed_txs: self.unconfirmed_pool.len() as u64,
Expand Down
26 changes: 15 additions & 11 deletions base_layer/core/src/mempool/priority/prioritized_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -38,12 +41,12 @@ use crate::transactions::{transaction_components::Transaction, weight::Transacti
pub struct FeePriority(Vec<u8>);

impl FeePriority {
pub fn new(transaction: &Transaction, insert_epoch: u64, weight: u64) -> Self {
pub fn new(transaction: &Transaction, insert_epoch: u64, weight: u64) -> Result<Self, TransactionError> {
// 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;
SWvheerden marked this conversation as resolved.
Show resolved Hide resolved
// Big-endian used here, the MSB is in the starting index. The ordering for Vec<u8> is taken from elements left
// to right and the unconfirmed pool expects the lowest priority to be sorted lowest to highest in the
// BTreeMap
Expand All @@ -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))
}
}

Expand All @@ -87,16 +90,16 @@ impl PrioritizedTransaction {
weighting: &TransactionWeight,
transaction: Arc<Transaction>,
dependent_outputs: Option<Vec<HashOutput>>,
) -> std::io::Result<PrioritizedTransaction> {
) -> Result<PrioritizedTransaction, TransactionError> {
let weight = transaction.calculate_weight(weighting)?;
let insert_epoch = match SystemTime::now().duration_since(UNIX_EPOCH) {
Ok(n) => n.as_secs(),
Err(_) => 0,
};
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(),
SWvheerden marked this conversation as resolved.
Show resolved Hide resolved
weight,
transaction,
dependent_output_hashes: dependent_outputs.unwrap_or_default(),
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down
4 changes: 4 additions & 0 deletions base_layer/core/src/mempool/unconfirmed_pool/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}