From 663c40f592d039ef3e5f17381fd6694fe26bce44 Mon Sep 17 00:00:00 2001 From: Hansie Odendaal Date: Wed, 11 Oct 2023 11:14:17 +0200 Subject: [PATCH] review comments --- base_layer/core/src/blocks/block.rs | 1 + .../core/src/transactions/aggregated_body.rs | 3 +- base_layer/core/src/transactions/fee.rs | 4 +- .../core/src/transactions/test_helpers.rs | 25 +++- base_layer/core/tests/tests/mempool.rs | 135 +++++++++++++++++- 5 files changed, 158 insertions(+), 10 deletions(-) diff --git a/base_layer/core/src/blocks/block.rs b/base_layer/core/src/blocks/block.rs index 4a3bcdaa0d..a33f751b72 100644 --- a/base_layer/core/src/blocks/block.rs +++ b/base_layer/core/src/blocks/block.rs @@ -197,6 +197,7 @@ impl BlockBuilder { /// This function adds the provided transaction kernels to the block WITHOUT updating kernel_mmr_size in the header pub fn add_kernels(mut self, mut kernels: Vec) -> Self { for kernel in &kernels { + // Saturating add is used here to prevent overflow; invalid fees will be caught by block validation self.total_fee = self.total_fee.saturating_add(kernel.fee); } self.kernels.append(&mut kernels); diff --git a/base_layer/core/src/transactions/aggregated_body.rs b/base_layer/core/src/transactions/aggregated_body.rs index fe1e75a5d8..b20451b319 100644 --- a/base_layer/core/src/transactions/aggregated_body.rs +++ b/base_layer/core/src/transactions/aggregated_body.rs @@ -222,7 +222,8 @@ impl AggregateBody { pub fn get_total_fee(&self) -> MicroMinotari { let mut fee = MicroMinotari::from(0); for kernel in &self.kernels { - fee += kernel.fee; + // Saturating add is used here to prevent overflow; invalid fees will be caught by block validation + fee = fee.saturating_add(kernel.fee); } fee } diff --git a/base_layer/core/src/transactions/fee.rs b/base_layer/core/src/transactions/fee.rs index a910d9f3af..bcc234b5f9 100644 --- a/base_layer/core/src/transactions/fee.rs +++ b/base_layer/core/src/transactions/fee.rs @@ -52,7 +52,9 @@ impl Fee { num_outputs, rounded_features_and_scripts_byte_size, ); - MicroMinotari::from(weight) * fee_per_gram + // Saturating multiplication is used here to prevent overflow only; invalid values will be caught with + // validation + MicroMinotari::from(weight.saturating_mul(fee_per_gram.0)) } pub fn calculate_body(&self, fee_per_gram: MicroMinotari, body: &AggregateBody) -> std::io::Result { diff --git a/base_layer/core/src/transactions/test_helpers.rs b/base_layer/core/src/transactions/test_helpers.rs index df0709005e..5e96b089a9 100644 --- a/base_layer/core/src/transactions/test_helpers.rs +++ b/base_layer/core/src/transactions/test_helpers.rs @@ -65,7 +65,7 @@ use crate::{ WalletOutput, WalletOutputBuilder, }, - transaction_protocol::TransactionMetadata, + transaction_protocol::{transaction_initializer::SenderTransactionInitializer, TransactionMetadata}, weight::TransactionWeight, SenderTransactionProtocol, }, @@ -651,6 +651,22 @@ pub async fn create_stx_protocol( schema: TransactionSchema, key_manager: &TestKeyManager, ) -> (SenderTransactionProtocol, Vec) { + let mut outputs = Vec::with_capacity(schema.to.len()); + let stx_builder = create_stx_protocol_internal(schema, key_manager, &mut outputs).await; + + let stx_protocol = stx_builder.build().await.unwrap(); + let change_output = stx_protocol.get_change_output().unwrap().unwrap(); + + outputs.push(change_output); + (stx_protocol, outputs) +} + +#[allow(clippy::too_many_lines)] +pub async fn create_stx_protocol_internal( + schema: TransactionSchema, + key_manager: &TestKeyManager, + outputs: &mut Vec, +) -> SenderTransactionInitializer { let constants = ConsensusManager::builder(Network::LocalNet) .build() .unwrap() @@ -676,7 +692,6 @@ pub async fn create_stx_protocol( for tx_input in &schema.from { stx_builder.with_input(tx_input.clone()).await.unwrap(); } - let mut outputs = Vec::with_capacity(schema.to.len()); for val in schema.to { let (spending_key, _) = key_manager .get_next_key(TransactionKeyManagerBranch::CommitmentMask.get_branch_key()) @@ -741,11 +756,7 @@ pub async fn create_stx_protocol( stx_builder.with_output(utxo, sender_offset_key_id).await.unwrap(); } - let stx_protocol = stx_builder.build().await.unwrap(); - let change_output = stx_protocol.get_change_output().unwrap().unwrap(); - - outputs.push(change_output); - (stx_protocol, outputs) + stx_builder } pub async fn create_coinbase_kernel(spending_key_id: &TariKeyId, key_manager: &TestKeyManager) -> TransactionKernel { diff --git a/base_layer/core/tests/tests/mempool.rs b/base_layer/core/tests/tests/mempool.rs index ac1ad9052e..0ab97ded7d 100644 --- a/base_layer/core/tests/tests/mempool.rs +++ b/base_layer/core/tests/tests/mempool.rs @@ -28,15 +28,18 @@ use tari_common_types::types::{Commitment, PrivateKey, PublicKey, Signature}; use tari_comms_dht::domain_message::OutboundDomainMessage; use tari_core::{ base_node::state_machine_service::states::{ListeningInfo, StateInfo, StatusInfo}, - consensus::{ConsensusConstantsBuilder, ConsensusManager}, + blocks::{BlockHeader, NewBlockTemplate}, + consensus::{emission::Emission, ConsensusConstantsBuilder, ConsensusManager}, mempool::{Mempool, MempoolConfig, MempoolServiceConfig, TxStorageResponse}, proof_of_work::Difficulty, proto, transactions::{ + aggregated_body::AggregateBody, fee::Fee, key_manager::{TransactionKeyManagerBranch, TransactionKeyManagerInterface, TxoStage}, tari_amount::{uT, MicroMinotari, T}, test_helpers::{ + create_stx_protocol_internal, create_test_core_key_manager_with_memory_db, create_wallet_output_with_data, schema_to_transaction, @@ -77,6 +80,7 @@ use tempfile::tempdir; use crate::helpers::{ block_builders::{ chain_block, + create_coinbase, create_genesis_block, create_genesis_block_with_coinbase_value, find_header_with_achieved_difficulty, @@ -330,6 +334,135 @@ async fn test_time_locked() { assert_eq!(mempool.insert(tx2).await.unwrap(), TxStorageResponse::UnconfirmedPool); } +#[tokio::test] +#[allow(clippy::too_many_lines)] +async fn test_fee_overflow() { + let network = Network::LocalNet; + let (mut store, mut blocks, mut outputs, consensus_manager, key_manager) = create_new_blockchain(network).await; + let schemas = vec![txn_schema!( + from: vec![outputs[0][0].clone()], + to: vec![10 * T, 10 * T, 10 * T, 10 * T] + )]; + generate_new_block( + &mut store, + &mut blocks, + &mut outputs, + schemas, + &consensus_manager, + &key_manager, + ) + .await + .unwrap(); + + let schemas = vec![ + txn_schema!( + from: vec![outputs[1][0].clone()], + to: vec![1 * T, 1 * T, 1 * T, 1 * T] + ), + txn_schema!( + from: vec![outputs[1][1].clone()], + to: vec![1 * T, 1 * T, 1 * T, 1 * T] + ), + txn_schema!( + from: vec![outputs[1][2].clone()], + to: vec![1 * T, 1 * T, 1 * T, 1 * T] + ), + ]; + + let coinbase_value = consensus_manager + .emission_schedule() + .block_reward(store.get_height().unwrap() + 1); + + let mut transactions = Vec::new(); + let mut block_utxos = Vec::new(); + 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(); + transactions.push(tx); + block_utxos.append(&mut utxos); + } + + let (coinbase_utxo, coinbase_kernel, coinbase_output) = + create_coinbase(coinbase_value + fees, 100, None, &key_manager).await; + block_utxos.push(coinbase_output); + + outputs.push(block_utxos); + + let mut header = BlockHeader::from_previous(blocks.last().unwrap().header()); + header.version = consensus_manager + .consensus_constants(header.height) + .blockchain_version(); + let height = header.height; + + let mut transactions_new = Vec::with_capacity(transactions.len()); + for txn in transactions { + transactions_new.push(Transaction { + offset: txn.offset, + body: { + let mut inputs = Vec::with_capacity(txn.body.inputs().len()); + for input in txn.body.inputs().iter() { + inputs.push(input.clone()); + } + let mut outputs = Vec::with_capacity(txn.body.outputs().len()); + for output in txn.body.outputs().iter() { + outputs.push(output.clone()); + } + let mut kernels = Vec::with_capacity(txn.body.kernels().len()); + for kernel in txn.body.kernels().iter() { + kernels.push(TransactionKernel { + version: kernel.version, + features: kernel.features, + fee: (u64::MAX / 2).into(), + lock_height: kernel.lock_height, + excess_sig: kernel.excess_sig.clone(), + excess: kernel.excess.clone(), + burn_commitment: kernel.burn_commitment.clone(), + }); + } + AggregateBody::new(inputs, outputs, kernels) + }, + script_offset: txn.script_offset, + }); + } + let template = NewBlockTemplate::from_block( + header + .into_builder() + .with_transactions(transactions_new) + .with_coinbase_utxo(coinbase_utxo, coinbase_kernel) + .build(), + Difficulty::min(), + consensus_manager.get_block_reward_at(height), + ); + let new_block = store.prepare_new_block(template).unwrap(); + + // This will call `BlockBuilder::add_kernels(...)` and `AggregateBody::get_total_fee(...)`, which will overflow if + // regressed + let block_result = store.add_block(new_block.into()); + assert!(block_result.is_err()); + assert_eq!( + block_result.unwrap_err().to_string(), + "Validation error: The total value + fees of the block exceeds the maximum allowance on chain".to_string() + ); + + let schema = txn_schema!( + from: vec![outputs[1][3].clone()], + to: vec![], + fee: MicroMinotari(u64::MAX / 2), + lock: 0, + features: OutputFeatures::default() + ); + let stx_builder = create_stx_protocol_internal(schema, &key_manager, &mut Vec::new()).await; + + // This will call `Fee::calculate(...)`, which will overflow if regressed + let build_result = stx_builder.build().await; + assert!(build_result.is_err()); + assert_eq!( + format!("{:?}", build_result.unwrap_err()), + "You are spending (18446744073709.550781 T) more than you're providing (10.000000 T)." + ); +} + // maturities not being checked before #[tokio::test] #[allow(clippy::identity_op)]