diff --git a/coins/monero/src/wallet/send/builder.rs b/coins/monero/src/wallet/send/builder.rs index 080504c0..eaa199c4 100644 --- a/coins/monero/src/wallet/send/builder.rs +++ b/coins/monero/src/wallet/send/builder.rs @@ -18,14 +18,14 @@ struct SignableTransactionBuilderInternal { r_seed: Option>, inputs: Vec<(SpendableOutput, Decoys)>, payments: Vec<(MoneroAddress, u64)>, - change_address: Option, + change_address: Change, data: Vec>, } impl SignableTransactionBuilderInternal { // Takes in the change address so users don't miss that they have to manually set one // If they don't, all leftover funds will become part of the fee - fn new(protocol: Protocol, fee_rate: Fee, change_address: Option) -> Self { + fn new(protocol: Protocol, fee_rate: Fee, change_address: Change) -> Self { Self { protocol, fee_rate, @@ -90,7 +90,7 @@ impl SignableTransactionBuilder { Self(self.0.clone()) } - pub fn new(protocol: Protocol, fee_rate: Fee, change_address: Option) -> Self { + pub fn new(protocol: Protocol, fee_rate: Fee, change_address: Change) -> Self { Self(Arc::new(RwLock::new(SignableTransactionBuilderInternal::new( protocol, fee_rate, diff --git a/coins/monero/src/wallet/send/mod.rs b/coins/monero/src/wallet/send/mod.rs index f770e270..61615866 100644 --- a/coins/monero/src/wallet/send/mod.rs +++ b/coins/monero/src/wallet/send/mod.rs @@ -292,7 +292,7 @@ impl FeePriority { #[derive(Clone, PartialEq, Eq, Debug, Zeroize)] pub(crate) enum InternalPayment { Payment((MoneroAddress, u64)), - Change(Change, u64), + Change((MoneroAddress, Option>), u64), } /// The eventual output of a SignableTransaction. @@ -324,7 +324,7 @@ pub struct SignableTransaction { /// Specification for a change output. #[derive(Clone, PartialEq, Eq, Zeroize)] pub struct Change { - address: MoneroAddress, + address: Option, view: Option>, } @@ -338,21 +338,21 @@ impl Change { /// Create a change output specification from a ViewPair, as needed to maintain privacy. pub fn new(view: &ViewPair, guaranteed: bool) -> Change { Change { - address: view.address( + address: Some(view.address( Network::Mainnet, if !guaranteed { AddressSpec::Standard } else { AddressSpec::Featured { subaddress: None, payment_id: None, guaranteed: true } }, - ), + )), view: Some(view.view.clone()), } } /// Create a fingerprintable change output specification which will harm privacy. Only use this /// if you know what you're doing. - pub fn fingerprintable(address: MoneroAddress) -> Change { + pub fn fingerprintable(address: Option) -> Change { Change { address, view: None } } } @@ -364,13 +364,13 @@ fn need_additional(payments: &[InternalPayment]) -> (bool, bool) { .filter(|payment| match *payment { InternalPayment::Payment(payment) => payment.0.is_subaddress(), InternalPayment::Change(change, _) => { - if change.view.is_some() { + if change.1.is_some() { has_change_view = true; // It should not be possible to construct a change specification to a subaddress with a // view key - debug_assert!(!change.address.is_subaddress()); + debug_assert!(!change.0.is_subaddress()); } - change.address.is_subaddress() + change.0.is_subaddress() } }) .count() != @@ -415,7 +415,7 @@ impl SignableTransaction { r_seed: Option>, inputs: Vec<(SpendableOutput, Decoys)>, payments: Vec<(MoneroAddress, u64)>, - change_address: Option, + change: Change, data: Vec>, fee_rate: Fee, ) -> Result { @@ -430,8 +430,8 @@ impl SignableTransaction { for payment in &payments { count(payment.0); } - if let Some(change) = change_address.as_ref() { - count(change.address); + if let Some(change_address) = change.address.as_ref() { + count(*change_address); } if payment_ids > 1 { Err(TransactionError::MultiplePaymentIds)?; @@ -459,24 +459,24 @@ impl SignableTransaction { } // If we don't have two outputs, as required by Monero, error - if (payments.len() == 1) && change_address.is_none() { + if (payments.len() == 1) && change.address.is_none() { Err(TransactionError::NoChange)?; } // Get the outgoing amount ignoring fees let out_amount = payments.iter().map(|payment| payment.1).sum::(); - let outputs = payments.len() + usize::from(change_address.is_some()); + let outputs = payments.len() + usize::from(change.address.is_some()); if outputs > MAX_OUTPUTS { Err(TransactionError::TooManyOutputs)?; } // Collect payments in a container that includes a change output if a change address is provided let mut payments = payments.into_iter().map(InternalPayment::Payment).collect::>(); - if change_address.is_some() { + if let Some(change_address) = change.address.as_ref() { // Push a 0 amount change output that we'll use to do fee calculations. // We'll modify the change amount after calculating the fee - payments.push(InternalPayment::Change(change_address.clone().unwrap(), 0)); + payments.push(InternalPayment::Change((*change_address, change.view.clone()), 0)); } // Determine if we'll need additional pub keys in tx extra @@ -517,21 +517,23 @@ impl SignableTransaction { } // Sanity check we have the expected number of change outputs - sanity_check_change_payment_quantity(&payments, change_address.is_some()); + sanity_check_change_payment_quantity(&payments, change.address.is_some()); // Modify the amount of the change output - if change_address.is_some() { + if let Some(change_address) = change.address.as_ref() { let change_payment = payments.last_mut().unwrap(); debug_assert!(matches!(change_payment, InternalPayment::Change(_, _))); - *change_payment = - InternalPayment::Change(change_address.clone().unwrap(), in_amount - out_amount - fee); + *change_payment = InternalPayment::Change( + (*change_address, change.view.clone()), + in_amount - out_amount - fee, + ); } // Sanity check the change again after modifying - sanity_check_change_payment_quantity(&payments, change_address.is_some()); + sanity_check_change_payment_quantity(&payments, change.address.is_some()); // Sanity check outgoing amount + fee == incoming amount - if change_address.is_some() { + if change.address.is_some() { debug_assert_eq!( payments .iter() @@ -551,7 +553,7 @@ impl SignableTransaction { r_seed, inputs, payments, - has_change: change_address.is_some(), + has_change: change.address.is_some(), data, fee, fee_rate, @@ -624,7 +626,7 @@ impl SignableTransaction { // regarding it's view key payment = if !modified_change_ecdh { if let InternalPayment::Change(change, amount) = &payment { - InternalPayment::Payment((change.address, *amount)) + InternalPayment::Payment((change.0, *amount)) } else { payment } @@ -656,8 +658,8 @@ impl SignableTransaction { InternalPayment::Change(change, amount) => { // Instead of rA, use Ra, where R is r * subaddress_spend_key // change.view must be Some as if it's None, this payment would've been downcast - let ecdh = tx_public_key * change.view.unwrap().deref(); - SendOutput::change(ecdh, uniqueness, (o, (change.address, amount))) + let ecdh = tx_public_key * change.1.unwrap().deref(); + SendOutput::change(ecdh, uniqueness, (o, (change.0, amount))) } }; @@ -954,8 +956,8 @@ impl Eventuality { } InternalPayment::Change(change, amount) => { w.write_all(&[1])?; - write_vec(write_byte, change.address.to_string().as_bytes(), w)?; - if let Some(view) = change.view.as_ref() { + write_vec(write_byte, change.0.to_string().as_bytes(), w)?; + if let Some(view) = change.1.as_ref() { w.write_all(&[1])?; write_scalar(view, w)?; } else { @@ -988,14 +990,14 @@ impl Eventuality { Ok(match read_byte(r)? { 0 => InternalPayment::Payment((read_address(r)?, read_u64(r)?)), 1 => InternalPayment::Change( - Change { - address: read_address(r)?, - view: match read_byte(r)? { + ( + read_address(r)?, + match read_byte(r)? { 0 => None, 1 => Some(Zeroizing::new(read_scalar(r)?)), _ => Err(io::Error::other("invalid change payment"))?, }, - }, + ), read_u64(r)?, ), _ => Err(io::Error::other("invalid payment"))?, diff --git a/coins/monero/src/wallet/send/multisig.rs b/coins/monero/src/wallet/send/multisig.rs index 39e79437..eecfd3fe 100644 --- a/coins/monero/src/wallet/send/multisig.rs +++ b/coins/monero/src/wallet/send/multisig.rs @@ -125,8 +125,8 @@ impl SignableTransaction { transcript.append_message(b"payment_amount", payment.1.to_le_bytes()); } InternalPayment::Change(change, amount) => { - transcript.append_message(b"change_address", change.address.to_string().as_bytes()); - if let Some(view) = change.view.as_ref() { + transcript.append_message(b"change_address", change.0.to_string().as_bytes()); + if let Some(view) = change.1.as_ref() { transcript.append_message(b"change_view_key", Zeroizing::new(view.to_bytes())); } transcript.append_message(b"change_amount", amount.to_le_bytes()); diff --git a/coins/monero/tests/runner.rs b/coins/monero/tests/runner.rs index e85268fd..cb0a3808 100644 --- a/coins/monero/tests/runner.rs +++ b/coins/monero/tests/runner.rs @@ -213,13 +213,13 @@ macro_rules! test { let builder = SignableTransactionBuilder::new( protocol, rpc.get_fee(protocol, FeePriority::Low).await.unwrap(), - Some(Change::new( + Change::new( &ViewPair::new( &random_scalar(&mut OsRng) * ED25519_BASEPOINT_TABLE, Zeroizing::new(random_scalar(&mut OsRng)) ), false - )), + ), ); let sign = |tx: SignableTransaction| { @@ -298,7 +298,11 @@ macro_rules! test { rpc.publish_transaction(&signed).await.unwrap(); mine_until_unlocked(&rpc, &random_address().2.to_string(), signed.hash()).await; let tx = rpc.get_transaction(signed.hash()).await.unwrap(); - check_weight_and_fee(&tx, fee_rate); + if stringify!($name) != "spend_one_input_to_two_outputs_no_change" { + // Skip weight and fee check for the above test because when there is no change, + // the change is added to the fee + check_weight_and_fee(&tx, fee_rate); + } #[allow(unused_assignments)] { let scanner = diff --git a/coins/monero/tests/send.rs b/coins/monero/tests/send.rs index b2abcb67..cd1b919d 100644 --- a/coins/monero/tests/send.rs +++ b/coins/monero/tests/send.rs @@ -111,7 +111,7 @@ test!( let mut builder = SignableTransactionBuilder::new( protocol, rpc.get_fee(protocol, FeePriority::Low).await.unwrap(), - Some(Change::new(&change_view, false)), + Change::new(&change_view, false), ); add_inputs(protocol, &rpc, vec![outputs.first().unwrap().clone()], &mut builder).await; @@ -273,3 +273,44 @@ test!( }, ), ); + +test!( + spend_one_input_to_two_outputs_no_change, + ( + |_, mut builder: Builder, addr| async move { + builder.add_payment(addr, 1000000000000); + (builder.build().unwrap(), ()) + }, + |_, tx: Transaction, mut scanner: Scanner, _| async move { + let mut outputs = scanner.scan_transaction(&tx).not_locked(); + outputs.sort_by(|x, y| x.commitment().amount.cmp(&y.commitment().amount)); + assert_eq!(outputs[0].commitment().amount, 1000000000000); + outputs + }, + ), + ( + |protocol, rpc: Rpc<_>, _, addr, outputs: Vec| async move { + use monero_serai::wallet::FeePriority; + + let mut builder = SignableTransactionBuilder::new( + protocol, + rpc.get_fee(protocol, FeePriority::Low).await.unwrap(), + Change::fingerprintable(None), + ); + add_inputs(protocol, &rpc, vec![outputs.first().unwrap().clone()], &mut builder).await; + builder.add_payment(addr, 10000); + builder.add_payment(addr, 50000); + + (builder.build().unwrap(), ()) + }, + |_, tx: Transaction, mut scanner: Scanner, _| async move { + let mut outputs = scanner.scan_transaction(&tx).not_locked(); + outputs.sort_by(|x, y| x.commitment().amount.cmp(&y.commitment().amount)); + assert_eq!(outputs[0].commitment().amount, 10000); + assert_eq!(outputs[1].commitment().amount, 50000); + + // The remainder should get shunted to fee, which is fingerprintable + assert_eq!(tx.rct_signatures.base.fee, 1000000000000 - 10000 - 50000); + }, + ), +); diff --git a/processor/src/networks/monero.rs b/processor/src/networks/monero.rs index 7bcf8097..d2ac279d 100644 --- a/processor/src/networks/monero.rs +++ b/processor/src/networks/monero.rs @@ -384,7 +384,7 @@ impl Monero { Some(Zeroizing::new(*plan_id)), inputs.clone(), payments, - change.clone().map(|change| Change::fingerprintable(change.into())), + Change::fingerprintable(change.as_ref().map(|change| change.clone().into())), vec![], fee_rate, ) { @@ -753,7 +753,7 @@ impl Network for Monero { None, inputs, vec![(address.into(), amount - fee)], - Some(Change::fingerprintable(Self::test_address().into())), + Change::fingerprintable(Some(Self::test_address().into())), vec![], self.rpc.get_fee(protocol, FeePriority::Low).await.unwrap(), ) diff --git a/tests/full-stack/src/tests/mint_and_burn.rs b/tests/full-stack/src/tests/mint_and_burn.rs index 79938596..856834fa 100644 --- a/tests/full-stack/src/tests/mint_and_burn.rs +++ b/tests/full-stack/src/tests/mint_and_burn.rs @@ -395,7 +395,7 @@ async fn mint_and_burn_test() { ), 1_100_000_000_000, )], - Some(Change::new(&view_pair, false)), + Change::new(&view_pair, false), vec![Shorthand::transfer(None, serai_addr).encode()], rpc.get_fee(Protocol::v16, FeePriority::Low).await.unwrap(), ) diff --git a/tests/processor/src/networks.rs b/tests/processor/src/networks.rs index 140f096e..db02686e 100644 --- a/tests/processor/src/networks.rs +++ b/tests/processor/src/networks.rs @@ -361,7 +361,7 @@ impl Wallet { None, these_inputs.drain(..).zip(decoys.drain(..)).collect(), vec![(to_addr, AMOUNT)], - Some(Change::new(view_pair, false)), + Change::new(view_pair, false), data, rpc.get_fee(Protocol::v16, FeePriority::Low).await.unwrap(), )