From 8b14bb54bbdb101f082874d75abcd520d9f2f34c Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 29 Jul 2023 06:55:55 -0400 Subject: [PATCH] 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 | 9 ++++-- processor/src/coins/mod.rs | 45 ++++++++++++++++++++++++----- processor/src/coins/monero.rs | 17 ++++++----- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/coins/monero/src/wallet/send/mod.rs b/coins/monero/src/wallet/send/mod.rs index 7fe03a18..af90954f 100644 --- a/coins/monero/src/wallet/send/mod.rs +++ b/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::(); 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 diff --git a/processor/src/coins/mod.rs b/processor/src/coins/mod.rs index fc46f4d1..06649cea 100644 --- a/processor/src/coins/mod.rs +++ b/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(plan: &mut Plan, tx_fee: u64) -> Vec(); - 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, 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(plan: &mut Plan, tx_fee: u64) -> Vec(); + assert!((new_outputs + tx_fee) <= original_outputs); + branch_outputs } diff --git a/processor/src/coins/monero.rs b/processor/src/coins/monero.rs index 208dfff7..c85f2550 100644 --- a/processor/src/coins/monero.rs +++ b/processor/src/coins/monero.rs @@ -418,7 +418,7 @@ impl Coin for Monero { let inputs = spendable_outputs.into_iter().zip(decoys.into_iter()).collect::>(); - let signable = |plan: &mut Plan, tx_fee: Option<_>| { + let signable = |mut plan: Plan, 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)), },