fix(core): minor audit improvements (#5486)

Description
---
fix(core): make max_input_maturity fallible
add details on the usage of core hash domains
include details on all fallible transaction input methods
minor comment update in coinbase_builder
fix(core/aggregate_body): remove unused buggy function
min_input_maturity
tests(core/aggregate_body_validator): add test for unsorted aggregate
body and comment on new_sorted_unchecked call
chore(core/transaction_builder): remove unnecessary mutation for more
ergonomic api
chore(core): improve clarity of check_coinbase_output
fix(core): remove unused git link for monero

Motivation and Context
---
max_input_maturity: should be fallible rather than possibly return the
incorrect result.
min_input_maturity: returns an incorrect result (u64::MAX) if there are
no inputs. Removed because it was unused.
check_coinbase_output: had unwraps and a comment indicating something
_should_ happen to prevent the panic. For clarity, updated the code so
that it can easily be seen as correct.
transaction_builder: add components with a mutable reference, possibly
unclear how the references are mutated. More ergonomic and useful to
pass in an iterator.
monero dependency was using a git link and `version` ambiguously. Cargo
seemed to use the crate and ignore the git field.
Added an explicit test for validation failure on unordered tx components
in an aggregate body.

How Has This Been Tested?
---
Existing tests and new unit test

What process can a PR reviewer use to test or verify this change?
---

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->

---------

Co-authored-by: SW van Heerden <[email protected]>

base_layer/core/Cargo.toml

@@ -57,7 +57,7 @@ integer-encoding = "3.0.2"
 lmdb-zero = "0.4.4"
 log = "0.4"
 log-mdc = "0.1.0"
-monero = { git = "https://github.com/monero-rs/monero-rs.git", version = "0.18" , features = ["serde"], optional = true }
+monero = { version = "0.18.2" , features = ["serde"], optional = true }
 newtype-ops = "0.1.4"
 num-traits = "0.2.15"
 num-derive = "0.3.3"

base_layer/core/src/blocks/genesis_block.rs

@@ -61,7 +61,7 @@ fn add_faucet_utxos_to_genesis_block(file: &str, block: &mut Block) {
         counter += 1;
     }
     block.header.output_mmr_size += utxos.len() as u64;
-    block.body.add_outputs(&mut utxos);
+    block.body.add_outputs(utxos);
     block.body.sort();
 }
 

base_layer/core/src/covenants/test.rs

@@ -46,15 +46,15 @@ pub async fn create_outputs(
     for _i in 0..n {
         let params = TestParams::new(key_manager).await;
         let output = params.create_output(utxo_params.clone(), key_manager).await.unwrap();
-        outputs.push(output.as_transaction_output(key_manager).await.unwrap());
+        outputs.push(output.to_transaction_output(key_manager).await.unwrap());
     }
     outputs
 }
 
 pub async fn create_input(key_manager: &TestKeyManager) -> TransactionInput {
     let params = TestParams::new(key_manager).await;
     let output = params.create_output(Default::default(), key_manager).await.unwrap();
-    output.as_transaction_input(key_manager).await.unwrap()
+    output.to_transaction_input(key_manager).await.unwrap()
 }
 
 pub fn create_context<'a>(covenant: &Covenant, input: &'a TransactionInput, block_height: u64) -> CovenantContext<'a> {

base_layer/core/src/transactions/aggregated_body.rs

@@ -20,7 +20,7 @@
 // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
 // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 use std::{
-    cmp::{max, min},
+    cmp::max,
     fmt::{Display, Error, Formatter},
 };
 
@@ -30,7 +30,6 @@ use serde::{Deserialize, Serialize};
 use tari_common_types::types::PrivateKey;
 use tari_crypto::commitment::HomomorphicCommitmentFactory;
 
-use super::transaction_components::OutputFeatures;
 use crate::transactions::{
     crypto_factories::CryptoFactories,
     tari_amount::MicroTari,
@@ -49,9 +48,11 @@ use crate::transactions::{
 pub const LOG_TARGET: &str = "c::tx::aggregated_body";
 
 /// The components of the block or transaction. The same struct can be used for either, since in Mimblewimble,
-/// cut-through means that blocks and transactions have the same structure.
+/// blocks consist of inputs, outputs and kernels, rather than transactions.
 #[derive(Clone, Debug, Serialize, Deserialize, BorshSerialize, BorshDeserialize)]
 pub struct AggregateBody {
+    /// This flag indicates if the inputs, outputs and kernels have been sorted internally, that is, the sort() method
+    /// has been called. This may be false even if all components are sorted.
     #[borsh_skip]
     sorted: bool,
     /// List of inputs spent by the transaction.
@@ -135,8 +136,8 @@ impl AggregateBody {
     }
 
     /// Add a series of inputs to the existing aggregate body
-    pub fn add_inputs(&mut self, inputs: &mut Vec<TransactionInput>) {
-        self.inputs.append(inputs);
+    pub fn add_inputs<I: IntoIterator<Item = TransactionInput>>(&mut self, inputs: I) {
+        self.inputs.extend(inputs);
         self.sorted = false;
     }
 
@@ -146,9 +147,9 @@ impl AggregateBody {
         self.sorted = false;
     }
 
-    /// Add an output to the existing aggregate body
-    pub fn add_outputs(&mut self, outputs: &mut Vec<TransactionOutput>) {
-        self.outputs.append(outputs);
+    /// Add a series of outputs to the existing aggregate body
+    pub fn add_outputs<I: IntoIterator<Item = TransactionOutput>>(&mut self, outputs: I) {
+        self.outputs.extend(outputs);
         self.sorted = false;
     }
 
@@ -157,9 +158,9 @@ impl AggregateBody {
         self.kernels.push(kernel);
     }
 
-    /// Add a kernels to the existing aggregate body
-    pub fn add_kernels(&mut self, new_kernels: &mut Vec<TransactionKernel>) {
-        self.kernels.append(new_kernels);
+    /// Add a series of kernels to the existing aggregate body
+    pub fn add_kernels<I: IntoIterator<Item = TransactionKernel>>(&mut self, new_kernels: I) {
+        self.kernels.extend(new_kernels);
         self.sorted = false;
     }
 
@@ -253,8 +254,8 @@ impl AggregateBody {
 
     /// Run through the outputs of the block and check that
     /// 1. There is exactly ONE coinbase output
-    /// 1. The output's maturity is correctly set
-    /// 1. The amount is correct.
+    /// 1. The coinbase output's maturity is correctly set
+    /// 1. The reward amount is correct.
     pub fn check_coinbase_output(
         &self,
         reward: MicroTari,
@@ -272,7 +273,7 @@ impl AggregateBody {
                     warn!(target: LOG_TARGET, "Coinbase {} found with maturity set too low", utxo);
                     return Err(TransactionError::InvalidCoinbaseMaturity);
                 }
-                coinbase_utxo = Some(utxo.clone());
+                coinbase_utxo = Some(utxo);
             }
         }
         if coinbase_counter > 1 {
@@ -283,31 +284,35 @@ impl AggregateBody {
             return Err(TransactionError::MoreThanOneCoinbase);
         }
 
-        if coinbase_counter == 0 {
+        if coinbase_utxo.is_none() || coinbase_counter == 0 {
             return Err(TransactionError::NoCoinbase);
         }
 
+        let coinbase_utxo = coinbase_utxo.expect("coinbase_utxo: none checked");
+
         let mut coinbase_counter = 0; // there should be exactly 1 coinbase kernel as well
         for kernel in self.kernels() {
             if kernel.features.contains(KernelFeatures::COINBASE_KERNEL) {
                 coinbase_counter += 1;
-                coinbase_kernel = Some(kernel.clone());
+                coinbase_kernel = Some(kernel);
             }
         }
-        if coinbase_counter != 1 {
+        if coinbase_kernel.is_none() || coinbase_counter != 1 {
             warn!(
                 target: LOG_TARGET,
                 "{} coinbase kernels found in body. Only a single coinbase kernel is permitted.", coinbase_counter,
             );
             return Err(TransactionError::MoreThanOneCoinbase);
         }
-        // Unwrap used here are fine as they should have an amount in them by here. If the coinbase's are missing the
-        // counters should be 0 and the fn should have returned an error by now.
-        let utxo = coinbase_utxo.unwrap();
-        let rhs =
-            &coinbase_kernel.unwrap().excess + &factories.commitment.commit_value(&PrivateKey::default(), reward.0);
-        if rhs != utxo.commitment {
-            warn!(target: LOG_TARGET, "Coinbase {} amount validation failed", utxo);
+
+        let coinbase_kernel = coinbase_kernel.expect("coinbase_kernel: none checked");
+
+        let rhs = &coinbase_kernel.excess + &factories.commitment.commit_value(&PrivateKey::default(), reward.0);
+        if rhs != coinbase_utxo.commitment {
+            warn!(
+                target: LOG_TARGET,
+                "Coinbase {} amount validation failed", coinbase_utxo
+            );
             return Err(TransactionError::InvalidCoinbase);
         }
         Ok(())
@@ -371,36 +376,13 @@ impl AggregateBody {
         )
     }
 
-    /// Returns the minimum maturity of the input UTXOs
-    pub fn min_input_maturity(&self) -> u64 {
-        self.inputs().iter().fold(u64::MAX, |min_maturity, input| {
-            min(
-                min_maturity,
-                input
-                    .features()
-                    .unwrap_or(&OutputFeatures {
-                        maturity: u64::MAX,
-                        ..Default::default()
-                    })
-                    .maturity,
-            )
-        })
-    }
-
-    /// Returns the maximum maturity of the input UTXOs
-    pub fn max_input_maturity(&self) -> u64 {
-        self.inputs().iter().fold(0, |max_maturity, input| {
-            max(
-                max_maturity,
-                input
-                    .features()
-                    .unwrap_or(&OutputFeatures {
-                        maturity: 0,
-                        ..Default::default()
-                    })
-                    .maturity,
-            )
-        })
+    /// Returns the maximum maturity of the input UTXOs.
+    /// This function panics if any of the inputs are compact.
+    pub fn max_input_maturity(&self) -> Result<u64, TransactionError> {
+        self.inputs()
+            .iter()
+            .map(|i| i.features())
+            .try_fold(0, |max_maturity, features| Ok(max(max_maturity, features?.maturity)))
     }
 
     pub fn max_kernel_timelock(&self) -> u64 {
@@ -411,8 +393,8 @@ impl AggregateBody {
 
     /// Returns the height of the minimum height where the body is spendable. This is calculated from the
     /// kernel lock_heights and the maturity of the input UTXOs.
-    pub fn min_spendable_height(&self) -> u64 {
-        max(self.max_kernel_timelock(), self.max_input_maturity())
+    pub fn min_spendable_height(&self) -> Result<u64, TransactionError> {
+        Ok(max(self.max_kernel_timelock(), self.max_input_maturity()?))
     }
 
     /// Return a cloned version of self with TransactionInputs in their compact form

base_layer/core/src/transactions/coinbase_builder.rs

@@ -99,7 +99,7 @@ impl<TKeyManagerInterface> CoinbaseBuilder<TKeyManagerInterface>
 where TKeyManagerInterface: TransactionKeyManagerInterface
 {
     /// Start building a new Coinbase transaction. From here you can build the transaction piecemeal with the builder
-    /// methods, or pass in a block to `using_block` to determine most of the coinbase parameters automatically.
+    /// methods.
     pub fn new(key_manager: TKeyManagerInterface) -> Self {
         CoinbaseBuilder {
             key_manager,
@@ -113,7 +113,7 @@ where TKeyManagerInterface: TransactionKeyManagerInterface
         }
     }
 
-    /// Assign the block height. This is used to determine the lock height of the transaction.
+    /// Assign the block height. This is used to determine the coinbase maturity and reward.
     pub fn with_block_height(mut self, height: u64) -> Self {
         self.block_height = Some(height);
         self
@@ -161,10 +161,6 @@ where TKeyManagerInterface: TransactionKeyManagerInterface
     /// block height. The other parameters (keys, nonces etc.) are provided by the caller. Other data is
     /// automatically set: Coinbase transactions have an offset of zero, no fees, the `COINBASE_OUTPUT` flags are set
     /// on the output and kernel, and the maturity schedule is set from the consensus rules.
-    ///
-    /// After `build` is called, the struct is destroyed and the memory zeroed
-    /// out (by virtue of the zero_on_drop crate).
-
     pub async fn build(
         self,
         constants: &ConsensusConstants,
@@ -179,9 +175,6 @@ where TKeyManagerInterface: TransactionKeyManagerInterface
     /// etc.) are provided by the caller. Other data is automatically set: Coinbase transactions have an offset of
     /// zero, no fees, the `COINBASE_OUTPUT` flags are set on the output and kernel, and the maturity schedule is
     /// set from the consensus rules.
-    ///
-    /// After `build_with_reward` is called, the struct is destroyed and the
-    /// memory zeroed out (by virtue of the zero_on_drop crate).
     #[allow(clippy::too_many_lines)]
     #[allow(clippy::erasing_op)] // This is for 0 * uT
     pub async fn build_with_reward(
@@ -199,7 +192,7 @@ where TKeyManagerInterface: TransactionKeyManagerInterface
 
         let kernel_features = KernelFeatures::create_coinbase();
         let metadata = TransactionMetadata::new_with_features(0.into(), 0, kernel_features);
-        // generate kernel siganture
+        // generate kernel signature
         let kernel_version = TransactionKernelVersion::get_current_version();
         let kernel_message = TransactionKernel::build_kernel_signature_message(
             &kernel_version,
@@ -285,7 +278,7 @@ where TKeyManagerInterface: TransactionKeyManagerInterface
         )
         .await?;
         let output = wallet_output
-            .as_transaction_output(&self.key_manager)
+            .to_transaction_output(&self.key_manager)
             .await
             .map_err(|e| CoinbaseBuildError::BuildError(e.to_string()))?;
         let kernel = KernelBuilder::new()
@@ -300,7 +293,9 @@ where TKeyManagerInterface: TransactionKeyManagerInterface
         let mut builder = TransactionBuilder::new();
         builder
             .add_output(output)
+            // A coinbase must have 0 offset or the reward balance check will fail.
             .add_offset(PrivateKey::default())
+            // Coinbase has no script offset https://rfc.tari.com/RFC-0201_TariScript.html#script-offset
             .add_script_offset(PrivateKey::default())
             .with_reward(total_reward)
             .with_kernel(kernel);

base_layer/core/src/transactions/mod.rs

@@ -21,7 +21,6 @@ pub use format_currency::format_currency;
 pub mod transaction_protocol;
 pub use transaction_protocol::{recipient::ReceiverTransactionProtocol, sender::SenderTransactionProtocol};
 
-pub mod types;
 pub mod weight;
 
 pub mod key_manager;
@@ -30,12 +29,11 @@ pub mod key_manager;
 #[cfg(feature = "base_node")]
 pub mod test_helpers;
 
+// Hash domain for all transaction-related hashes, including the script signature challenge, transaction hash and kernel
+// signature challenge
 hash_domain!(TransactionHashDomain, "com.tari.base_layer.core.transactions", 0);
-hash_domain!(
-    TransactionFixedNonceKdfDomain,
-    "com.tari.base_layer.core.transactions.fixed_nonce_kdf",
-    0
-);
+
+// Hash domain used to derive the final AEAD encryption key for encrypted data in UTXOs
 hash_domain!(
     TransactionSecureNonceKdfDomain,
     "com.tari.base_layer.core.transactions.secure_nonce_kdf",

base_layer/core/src/transactions/tari_amount.rs

@@ -285,6 +285,7 @@ impl Tari {
     }
 
     pub fn to_currency_string(&self, sep: char) -> String {
+        // UNWRAP: MAX_I128_REPR > u64::MAX and scale is within bounds (see Decimal::from_parts)
         let d = Decimal::from_parts(u128::from(self.0.as_u64()), 6, false).unwrap();
         format!("{} T", format_currency(&d.to_string(), sep))
     }