From d88bd70f7a0eab43b02a847703bd5c672b708023 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 6 Jul 2024 22:22:32 -0400 Subject: [PATCH] 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 | 28 +++++++------ coins/monero/wallet/src/send/tx.rs | 55 ++++++++++++++----------- coins/monero/wallet/src/send/tx_keys.rs | 6 +-- processor/src/networks/monero.rs | 16 +++---- 4 files changed, 57 insertions(+), 48 deletions(-) diff --git a/coins/monero/wallet/src/send/mod.rs b/coins/monero/wallet/src/send/mod.rs index c477d3e7d..625157c9d 100644 --- a/coins/monero/wallet/src/send/mod.rs +++ b/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, + necessary_fee: Option, }, /// 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::(); - // 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. diff --git a/coins/monero/wallet/src/send/tx.rs b/coins/monero/wallet/src/send/tx.rs index c7b4988f7..237703164 100644 --- a/coins/monero/wallet/src/send/tx.rs +++ b/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::(); - let payments = self - .payments - .iter() - .filter_map(|payment| match payment { - InternalPayment::Payment(_, amount) => Some(amount), - InternalPayment::Change(_, _) => None, - }) - .sum::(); - // 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::(); + let payments = self + .intent + .payments + .iter() + .filter_map(|payment| match payment { + InternalPayment::Payment(_, amount) => Some(amount), + InternalPayment::Change(_, _) => None, + }) + .sum::(); + // Safe since the constructor checks inputs >= (payments + fee) + inputs - payments + }, encrypted_amounts, pseudo_outs: vec![], commitments, diff --git a/coins/monero/wallet/src/send/tx_keys.rs b/coins/monero/wallet/src/send/tx_keys.rs index b54212357..8cf141f79 100644 --- a/coins/monero/wallet/src/send/tx_keys.rs +++ b/coins/monero/wallet/src/send/tx_keys.rs @@ -217,9 +217,9 @@ impl SignableTransaction { InternalPayment::Change(_, _) => None, }) .sum::(); - 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); diff --git a/processor/src/networks/monero.rs b/processor/src/networks/monero.rs index b6be00e22..44cc2addf 100644 --- a/processor/src/networks/monero.rs +++ b/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)