review comments

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<TransactionKernel>) -> 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);

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
     }

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<MicroMinotari> {

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<WalletOutput>) {
+    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<WalletOutput>,
+) -> SenderTransactionInitializer<TestKeyManager> {
     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 {

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)]