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)