Correct the fee amortization algorithm for an edge case

This is technically over-agressive, as a dropped output will reduce the fee,
yet this edge case is so minor the flow for it to not be over-aggressive (over
a few fractions of a cent) is by no means worth it.

Fixes the crash causable by the WIP send_test.

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

@@ -140,8 +140,11 @@ pub enum TransactionError {
   TooMuchData,
   #[cfg_attr(feature = "std", error("too many inputs/too much arbitrary data"))]
   TooLargeTransaction,
-  #[cfg_attr(feature = "std", error("not enough funds (in {0}, out {1})"))]
-  NotEnoughFunds(u64, u64),
+  #[cfg_attr(
+    feature = "std",
+    error("not enough funds (inputs {inputs}, outputs {outputs}, fee {fee})")
+  )]
+  NotEnoughFunds { inputs: u64, outputs: u64, fee: u64 },
   #[cfg_attr(feature = "std", error("wrong spend private key"))]
   WrongPrivateKey,
   #[cfg_attr(feature = "std", error("rpc error ({0})"))]
@@ -509,7 +512,7 @@ impl SignableTransaction {
     // Make sure we have enough funds
     let in_amount = inputs.iter().map(|(input, _)| input.commitment().amount).sum::<u64>();
     if in_amount < (out_amount + fee) {
-      Err(TransactionError::NotEnoughFunds(in_amount, out_amount + fee))?;
+      Err(TransactionError::NotEnoughFunds { inputs: in_amount, outputs: out_amount, fee })?;
     }
 
     // Sanity check we have the expected number of change outputs

processor/src/coins/mod.rs

@@ -22,7 +22,7 @@ pub mod monero;
 #[cfg(feature = "monero")]
 pub use monero::Monero;
 
-use crate::Plan;
+use crate::{Payment, Plan};
 
 #[derive(Clone, Copy, Error, Debug)]
 pub enum CoinError {
@@ -204,21 +204,45 @@ pub fn amortize_fee<C: Coin>(plan: &mut Plan<C>, tx_fee: u64) -> Vec<PostFeeBran
     return vec![];
   }
 
-  // Amortize the transaction fee across outputs
-  let payments_len = u64::try_from(plan.payments.len()).unwrap();
-  // Use a formula which will round up
-  let output_fee = (tx_fee + (payments_len - 1)) / payments_len;
+  let original_outputs = plan.payments.iter().map(|payment| payment.amount).sum::<u64>();
 
-  let mut branch_outputs = vec![];
-  for payment in plan.payments.iter_mut() {
-    let mut post_fee = payment.amount.checked_sub(output_fee);
+  // Amortize the transaction fee across outputs
+  let mut payments_len = u64::try_from(plan.payments.len()).unwrap();
+  // Use a formula which will round up
+  let per_output_fee = |payments| (tx_fee + (payments - 1)) / payments;
+
+  let post_fee = |payment: &Payment<C>, per_output_fee| {
+    let mut post_fee = payment.amount.checked_sub(per_output_fee);
     // If this is under our dust threshold, drop it
     if let Some(amount) = post_fee {
       if amount < C::DUST {
         post_fee = None;
       }
     }
+    post_fee
+  };
 
+  // If we drop outputs for being less than the fee, we won't successfully reduce the amount spent
+  // (dropping a 800 output due to a 1000 fee leaves 200 we still have to deduct)
+  // Do initial runs until the amount of output we will drop is known
+  while {
+    let last = payments_len;
+    payments_len = u64::try_from(
+      plan
+        .payments
+        .iter()
+        .filter(|payment| post_fee(payment, per_output_fee(payments_len)).is_some())
+        .count(),
+    )
+    .unwrap();
+    last != payments_len
+  } {}
+
+  // Now that we know how many outputs will survive, calculate the actual per_output_fee
+  let per_output_fee = per_output_fee(payments_len);
+  let mut branch_outputs = vec![];
+  for payment in plan.payments.iter_mut() {
+    let post_fee = post_fee(payment, per_output_fee);
     // Note the branch output, if this is one
     if payment.address == C::branch_address(plan.key) {
       branch_outputs.push(PostFeeBranch { expected: payment.amount, actual: post_fee });
@@ -227,6 +251,11 @@ pub fn amortize_fee<C: Coin>(plan: &mut Plan<C>, tx_fee: u64) -> Vec<PostFeeBran
   }
   // Drop payments now worth 0
   plan.payments = plan.payments.drain(..).filter(|payment| payment.amount != 0).collect();
+
+  // Sanity check the fee wa successfully amortized
+  let new_outputs = plan.payments.iter().map(|payment| payment.amount).sum::<u64>();
+  assert!((new_outputs + tx_fee) <= original_outputs);
+
   branch_outputs
 }
 

processor/src/coins/monero.rs

@@ -418,7 +418,7 @@ impl Coin for Monero {
 
     let inputs = spendable_outputs.into_iter().zip(decoys.into_iter()).collect::<Vec<_>>();
 
-    let signable = |plan: &mut Plan<Self>, tx_fee: Option<_>| {
+    let signable = |mut plan: Plan<Self>, tx_fee: Option<_>| {
       // Monero requires at least two outputs
       // If we only have one output planned, add a dummy payment
       let outputs = plan.payments.len() + usize::from(u8::from(plan.change.is_some()));
@@ -480,11 +480,14 @@ impl Coin for Monero {
           TransactionError::FrostError(_) => {
             panic!("supposedly unreachable (at this time) Monero error: {e}");
           }
-          TransactionError::NotEnoughFunds(_, _) => {
-            if tx_fee.is_none() {
-              Ok(None)
+          TransactionError::NotEnoughFunds { inputs, outputs, fee } => {
+            if let Some(tx_fee) = tx_fee {
+              panic!(
+                "{}. in: {inputs}, out: {outputs}, fee: {fee}, prior estimated fee: {tx_fee}",
+                "didn't have enough funds for a Monero TX",
+              );
             } else {
-              panic!("didn't have enough funds for a Monero TX");
+              Ok(None)
             }
           }
           TransactionError::RpcError(e) => {
@@ -495,7 +498,7 @@ impl Coin for Monero {
       }
     };
 
-    let tx_fee = match signable(&mut plan, None)? {
+    let tx_fee = match signable(plan.clone(), None)? {
       Some(tx) => tx.fee(),
       None => return Ok((None, drop_branches(&plan))),
     };
@@ -505,7 +508,7 @@ impl Coin for Monero {
     let signable = SignableTransaction {
       keys,
       transcript,
-      actual: match signable(&mut plan, Some(tx_fee))? {
+      actual: match signable(plan, Some(tx_fee))? {
         Some(signable) => signable,
         None => return Ok((None, branch_outputs)),
       },