Distinguish fee from necessary_fee in monero-wallet

If there's no change, the fee is difference of the inputs to the outputs. The
prior code wouldn't check that amount is greater than or equal to the necessary
fee, and returning the would-be change amount as the fee isn't necessarily
helpful.

Now the fee is validated in such cases and the necessary fee is returned,
enabling operating off of that.

coins/monero/wallet/src/send/mod.rs

@@ -193,18 +193,20 @@ pub enum SendError {
   /// This transaction could not pay for itself.
   #[cfg_attr(
     feature = "std",
-    error("not enough funds (inputs {inputs}, outputs {outputs}, fee {fee:?})")
+    error(
+      "not enough funds (inputs {inputs}, outputs {outputs}, necessary_fee {necessary_fee:?})"
+    )
   )]
   NotEnoughFunds {
     /// The amount of funds the inputs contributed.
     inputs: u64,
     /// The amount of funds the outputs required.
     outputs: u64,
-    /// The fee which would be paid on top.
+    /// The fee necessary to be paid on top.
     ///
     /// If this is None, it is because the fee was not calculated as the outputs alone caused this
     /// error.
-    fee: Option<u64>,
+    necessary_fee: Option<u64>,
   },
   /// This transaction is being signed with the wrong private key.
   #[cfg_attr(feature = "std", error("wrong spend private key"))]
@@ -321,22 +323,19 @@ impl SignableTransaction {
         InternalPayment::Change(_, _) => None,
       })
       .sum::<u64>();
-    // Necessary so weight_and_fee doesn't underflow
-    if in_amount < payments_amount {
-      Err(SendError::NotEnoughFunds { inputs: in_amount, outputs: payments_amount, fee: None })?;
-    }
-    let (weight, fee) = self.weight_and_fee();
-    if in_amount < (payments_amount + fee) {
+    let (weight, necessary_fee) = self.weight_and_necessary_fee();
+    if in_amount < (payments_amount + necessary_fee) {
       Err(SendError::NotEnoughFunds {
         inputs: in_amount,
         outputs: payments_amount,
-        fee: Some(fee),
+        necessary_fee: Some(necessary_fee),
       })?;
     }
 
     // The actual limit is half the block size, and for the minimum block size of 300k, that'd be
     // 150k
     // wallet2 will only create transactions up to 100k bytes however
+    // TODO: Cite
     const MAX_TX_SIZE: usize = 100_000;
     if weight >= MAX_TX_SIZE {
       Err(SendError::TooLargeTransaction)?;
@@ -394,9 +393,12 @@ impl SignableTransaction {
     self.fee_rate
   }
 
-  /// The fee this transaction will use.
-  pub fn fee(&self) -> u64 {
-    self.weight_and_fee().1
+  /// The fee this transaction requires.
+  ///
+  /// This is distinct from the fee this transaction will use. If no change output is specified,
+  /// all unspent coins will be shunted to the fee.
+  pub fn necessary_fee(&self) -> u64 {
+    self.weight_and_necessary_fee().1
   }
 
   /// Write a SignableTransaction.

coins/monero/wallet/src/send/tx.rs

@@ -105,7 +105,7 @@ impl SignableTransaction {
     serialized
   }
 
-  pub(crate) fn weight_and_fee(&self) -> (usize, u64) {
+  pub(crate) fn weight_and_necessary_fee(&self) -> (usize, u64) {
     /*
       This transaction is variable length to:
         - The decoy offsets (fixed)
@@ -223,22 +223,6 @@ impl SignableTransaction {
         1
     };
 
-    // If we don't have a change output, the difference is the fee
-    if !self.payments.iter().any(|payment| matches!(payment, InternalPayment::Change(_, _))) {
-      let inputs = self.inputs.iter().map(|input| input.0.commitment().amount).sum::<u64>();
-      let payments = self
-        .payments
-        .iter()
-        .filter_map(|payment| match payment {
-          InternalPayment::Payment(_, amount) => Some(amount),
-          InternalPayment::Change(_, _) => None,
-        })
-        .sum::<u64>();
-      // Safe since the constructor checks inputs > payments before any calls to weight_and_fee
-      let fee = inputs - payments;
-      return (base_weight + varint_len(fee), fee);
-    }
-
     // We now have the base weight, without the fee encoded
     // The fee itself will impact the weight as its encoding is [1, 9] bytes long
     let mut possible_weights = Vec::with_capacity(9);
@@ -255,17 +239,17 @@ impl SignableTransaction {
 
     // We now look for the fee whose length matches the length used to derive it
     let mut weight_and_fee = None;
-    for (len, possible_fee) in possible_fees.into_iter().enumerate() {
-      let len = 1 + len;
-      debug_assert!(1 <= len);
-      debug_assert!(len <= 9);
+    for (fee_len, possible_fee) in possible_fees.into_iter().enumerate() {
+      let fee_len = 1 + fee_len;
+      debug_assert!(1 <= fee_len);
+      debug_assert!(fee_len <= 9);
 
       // We use the first fee whose encoded length is not larger than the length used within this
       // weight
       // This should be because the lengths are equal, yet means if somehow none are equal, this
       // will still terminate successfully
-      if varint_len(possible_fee) <= len {
-        weight_and_fee = Some((base_weight + len, possible_fee));
+      if varint_len(possible_fee) <= fee_len {
+        weight_and_fee = Some((base_weight + fee_len, possible_fee));
         break;
       }
     }
@@ -304,7 +288,30 @@ impl SignableTransactionWithKeyImages {
       },
       proofs: Some(RctProofs {
         base: RctBase {
-          fee: self.intent.weight_and_fee().1,
+          fee: if self
+            .intent
+            .payments
+            .iter()
+            .any(|payment| matches!(payment, InternalPayment::Change(_, _)))
+          {
+            // The necessary fee is the fee
+            self.intent.weight_and_necessary_fee().1
+          } else {
+            // If we don't have a change output, the difference is the fee
+            let inputs =
+              self.intent.inputs.iter().map(|input| input.0.commitment().amount).sum::<u64>();
+            let payments = self
+              .intent
+              .payments
+              .iter()
+              .filter_map(|payment| match payment {
+                InternalPayment::Payment(_, amount) => Some(amount),
+                InternalPayment::Change(_, _) => None,
+              })
+              .sum::<u64>();
+            // Safe since the constructor checks inputs >= (payments + fee)
+            inputs - payments
+          },
           encrypted_amounts,
           pseudo_outs: vec![],
           commitments,

coins/monero/wallet/src/send/tx_keys.rs

@@ -217,9 +217,9 @@ impl SignableTransaction {
               InternalPayment::Change(_, _) => None,
             })
             .sum::<u64>();
-          let fee = self.weight_and_fee().1;
-          // Safe since the constructor checked this
-          inputs - (payments + fee)
+          let necessary_fee = self.weight_and_necessary_fee().1;
+          // Safe since the constructor checked this TX has enough funds for itself
+          inputs - (payments + necessary_fee)
         }
       };
       let commitment = Commitment::new(shared_key_derivations.commitment_mask(), amount);

processor/src/networks/monero.rs

@@ -159,7 +159,7 @@ impl EventualityTrait for Eventuality {
 pub struct SignableTransaction(MSignableTransaction);
 impl SignableTransactionTrait for SignableTransaction {
   fn fee(&self) -> u64 {
-    self.0.fee()
+    self.0.necessary_fee()
   }
 }
 
@@ -293,7 +293,7 @@ impl Monero {
     let fee = fees.get(fees.len() / 2).copied().unwrap_or(0);
 
     // TODO: Set a sane minimum fee
-    const MINIMUM_FEE: u64 = 5_000_000;
+    const MINIMUM_FEE: u64 = 50_000;
     Ok(FeeRate::new(fee.max(MINIMUM_FEE), 10000).unwrap())
   }
 
@@ -383,7 +383,7 @@ impl Monero {
     ) {
       Ok(signable) => Ok(Some({
         if calculating_fee {
-          MakeSignableTransactionResult::Fee(signable.fee())
+          MakeSignableTransactionResult::Fee(signable.necessary_fee())
         } else {
           MakeSignableTransactionResult::SignableTransaction(signable)
         }
@@ -405,17 +405,17 @@ impl Monero {
         SendError::MultiplePaymentIds => {
           panic!("multiple payment IDs despite not supporting integrated addresses");
         }
-        SendError::NotEnoughFunds { inputs, outputs, fee } => {
+        SendError::NotEnoughFunds { inputs, outputs, necessary_fee } => {
           log::debug!(
-            "Monero NotEnoughFunds. inputs: {:?}, outputs: {:?}, fee: {fee:?}",
+            "Monero NotEnoughFunds. inputs: {:?}, outputs: {:?}, necessary_fee: {necessary_fee:?}",
             inputs,
             outputs
           );
-          match fee {
-            Some(fee) => {
+          match necessary_fee {
+            Some(necessary_fee) => {
               // If we're solely calculating the fee, return the fee this TX will cost
               if calculating_fee {
-                Ok(Some(MakeSignableTransactionResult::Fee(fee)))
+                Ok(Some(MakeSignableTransactionResult::Fee(necessary_fee)))
               } else {
                 // If we're actually trying to make the TX, return None
                 Ok(None)