diff --git a/coins/monero/wallet/src/send/mod.rs b/coins/monero/wallet/src/send/mod.rs index 210d18a0..f63915d4 100644 --- a/coins/monero/wallet/src/send/mod.rs +++ b/coins/monero/wallet/src/send/mod.rs @@ -22,8 +22,8 @@ use crate::{ RctType, RctPrunable, RctProofs, }, transaction::Transaction, + address::{Network, SubaddressIndex, MoneroAddress}, extra::MAX_ARBITRARY_DATA_SIZE, - address::{Network, MoneroAddress}, rpc::FeeRate, ViewPair, GuaranteedViewPair, OutputWithDecoys, }; @@ -44,58 +44,48 @@ pub(crate) fn key_image_sort(x: &EdwardsPoint, y: &EdwardsPoint) -> core::cmp::O #[derive(Clone, PartialEq, Eq, Zeroize)] enum ChangeEnum { - None, AddressOnly(MoneroAddress), - AddressWithView(MoneroAddress, Zeroizing), + Standard { view_pair: ViewPair, subaddress: Option }, + Guaranteed { view_pair: GuaranteedViewPair, subaddress: Option }, } impl fmt::Debug for ChangeEnum { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { - ChangeEnum::None => f.debug_struct("ChangeEnum::None").finish_non_exhaustive(), ChangeEnum::AddressOnly(addr) => { f.debug_struct("ChangeEnum::AddressOnly").field("addr", &addr).finish() } - ChangeEnum::AddressWithView(addr, _) => { - f.debug_struct("ChangeEnum::AddressWithView").field("addr", &addr).finish_non_exhaustive() - } + ChangeEnum::Standard { subaddress, .. } => f + .debug_struct("ChangeEnum::Standard") + .field("subaddress", &subaddress) + .finish_non_exhaustive(), + ChangeEnum::Guaranteed { subaddress, .. } => f + .debug_struct("ChangeEnum::Guaranteed") + .field("subaddress", &subaddress) + .finish_non_exhaustive(), } } } /// Specification for a change output. #[derive(Clone, PartialEq, Eq, Debug, Zeroize)] -pub struct Change(ChangeEnum); +pub struct Change(Option); impl Change { /// Create a change output specification. /// /// This take the view key as Monero assumes it has the view key for change outputs. It optimizes /// its wallet protocol accordingly. - pub fn new(view: &ViewPair) -> Change { - Change(ChangeEnum::AddressWithView( - // Which network doesn't matter as the derivations will all be the same - // TODO: Support subaddresses - view.legacy_address(Network::Mainnet), - view.view.clone(), - )) + pub fn new(view_pair: ViewPair, subaddress: Option) -> Change { + Change(Some(ChangeEnum::Standard { view_pair, subaddress })) } /// Create a change output specification for a guaranteed view pair. /// /// This take the view key as Monero assumes it has the view key for change outputs. It optimizes /// its wallet protocol accordingly. - pub fn guaranteed(view: &GuaranteedViewPair) -> Change { - Change(ChangeEnum::AddressWithView( - view.address( - // Which network doesn't matter as the derivations will all be the same - Network::Mainnet, - // TODO: Support subaddresses - None, - None, - ), - view.0.view.clone(), - )) + pub fn guaranteed(view_pair: GuaranteedViewPair, subaddress: Option) -> Change { + Change(Some(ChangeEnum::Guaranteed { view_pair, subaddress })) } /// Create a fingerprintable change output specification. @@ -116,38 +106,34 @@ impl Change { /// monero-wallet TX without change. pub fn fingerprintable(address: Option) -> Change { if let Some(address) = address { - Change(ChangeEnum::AddressOnly(address)) + Change(Some(ChangeEnum::AddressOnly(address))) } else { - Change(ChangeEnum::None) + Change(None) } } } -#[derive(Clone, PartialEq, Eq, Zeroize)] +#[derive(Clone, PartialEq, Eq, Debug, Zeroize)] enum InternalPayment { Payment(MoneroAddress, u64), - Change(MoneroAddress, Option>), + Change(ChangeEnum), } impl InternalPayment { - fn address(&self) -> &MoneroAddress { + fn address(&self) -> MoneroAddress { match self { - InternalPayment::Payment(addr, _) | InternalPayment::Change(addr, _) => addr, - } - } -} - -impl fmt::Debug for InternalPayment { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - InternalPayment::Payment(addr, amount) => f - .debug_struct("InternalPayment::Payment") - .field("addr", &addr) - .field("amount", &amount) - .finish(), - InternalPayment::Change(addr, _) => { - f.debug_struct("InternalPayment::Change").field("addr", &addr).finish_non_exhaustive() - } + InternalPayment::Payment(addr, _) => *addr, + InternalPayment::Change(change) => match change { + ChangeEnum::AddressOnly(addr) => *addr, + // Network::Mainnet as the network won't effect the derivations + ChangeEnum::Standard { view_pair, subaddress } => match subaddress { + Some(subaddress) => view_pair.subaddress(Network::Mainnet, *subaddress), + None => view_pair.legacy_address(Network::Mainnet), + }, + ChangeEnum::Guaranteed { view_pair, subaddress } => { + view_pair.address(Network::Mainnet, *subaddress, None) + } + }, } } } @@ -276,7 +262,7 @@ impl SignableTransaction { { let mut change_count = 0; for payment in &self.payments { - change_count += usize::from(u8::from(matches!(payment, InternalPayment::Change(_, _)))); + change_count += usize::from(u8::from(matches!(payment, InternalPayment::Change(_)))); } if change_count > 1 { Err(SendError::MaliciousSerialization)?; @@ -319,7 +305,7 @@ impl SignableTransaction { .iter() .filter_map(|payment| match payment { InternalPayment::Payment(_, amount) => Some(amount), - InternalPayment::Change(_, _) => None, + InternalPayment::Change(_) => None, }) .sum::(); let (weight, necessary_fee) = self.weight_and_necessary_fee(); @@ -366,12 +352,9 @@ impl SignableTransaction { .into_iter() .map(|(addr, amount)| InternalPayment::Payment(addr, amount)) .collect::>(); - match change.0 { - ChangeEnum::None => {} - ChangeEnum::AddressOnly(addr) => payments.push(InternalPayment::Change(addr, None)), - ChangeEnum::AddressWithView(addr, view) => { - payments.push(InternalPayment::Change(addr, Some(view))) - } + + if let Some(change) = change.0 { + payments.push(InternalPayment::Change(change)); } let mut res = @@ -412,16 +395,36 @@ impl SignableTransaction { write_vec(write_byte, addr.to_string().as_bytes(), w)?; w.write_all(&amount.to_le_bytes()) } - InternalPayment::Change(addr, change_view) => { - w.write_all(&[1])?; - write_vec(write_byte, addr.to_string().as_bytes(), w)?; - if let Some(view) = change_view.as_ref() { + InternalPayment::Change(change) => match change { + ChangeEnum::AddressOnly(addr) => { w.write_all(&[1])?; - write_scalar(view, w) - } else { - w.write_all(&[0]) + write_vec(write_byte, addr.to_string().as_bytes(), w) } - } + ChangeEnum::Standard { view_pair, subaddress } => { + w.write_all(&[2])?; + write_point(&view_pair.spend(), w)?; + write_scalar(&view_pair.view, w)?; + if let Some(subaddress) = subaddress { + w.write_all(&subaddress.account().to_le_bytes())?; + w.write_all(&subaddress.address().to_le_bytes()) + } else { + w.write_all(&0u32.to_le_bytes())?; + w.write_all(&0u32.to_le_bytes()) + } + } + ChangeEnum::Guaranteed { view_pair, subaddress } => { + w.write_all(&[3])?; + write_point(&view_pair.spend(), w)?; + write_scalar(&view_pair.0.view, w)?; + if let Some(subaddress) = subaddress { + w.write_all(&subaddress.account().to_le_bytes())?; + w.write_all(&subaddress.address().to_le_bytes()) + } else { + w.write_all(&0u32.to_le_bytes())?; + w.write_all(&0u32.to_le_bytes()) + } + } + }, } } @@ -458,14 +461,17 @@ impl SignableTransaction { fn read_payment(r: &mut R) -> io::Result { Ok(match read_byte(r)? { 0 => InternalPayment::Payment(read_address(r)?, read_u64(r)?), - 1 => InternalPayment::Change( - read_address(r)?, - match read_byte(r)? { - 0 => None, - 1 => Some(Zeroizing::new(read_scalar(r)?)), - _ => Err(io::Error::other("invalid change view"))?, - }, - ), + 1 => InternalPayment::Change(ChangeEnum::AddressOnly(read_address(r)?)), + 2 => InternalPayment::Change(ChangeEnum::Standard { + view_pair: ViewPair::new(read_point(r)?, Zeroizing::new(read_scalar(r)?)) + .map_err(io::Error::other)?, + subaddress: SubaddressIndex::new(read_u32(r)?, read_u32(r)?), + }), + 3 => InternalPayment::Change(ChangeEnum::Guaranteed { + view_pair: GuaranteedViewPair::new(read_point(r)?, Zeroizing::new(read_scalar(r)?)) + .map_err(io::Error::other)?, + subaddress: SubaddressIndex::new(read_u32(r)?, read_u32(r)?), + }), _ => Err(io::Error::other("invalid payment"))?, }) } diff --git a/coins/monero/wallet/src/send/tx.rs b/coins/monero/wallet/src/send/tx.rs index 522ee868..b4ea3970 100644 --- a/coins/monero/wallet/src/send/tx.rs +++ b/coins/monero/wallet/src/send/tx.rs @@ -78,7 +78,7 @@ impl SignableTransaction { } else { // If there's no payment ID, we push a dummy (as wallet2 does) if there's only one payment if (self.payments.len() == 2) && - self.payments.iter().any(|payment| matches!(payment, InternalPayment::Change(_, _))) + self.payments.iter().any(|payment| matches!(payment, InternalPayment::Change(_))) { let (_, payment_id_xor) = self .payments @@ -292,7 +292,7 @@ impl SignableTransactionWithKeyImages { .intent .payments .iter() - .any(|payment| matches!(payment, InternalPayment::Change(_, _))) + .any(|payment| matches!(payment, InternalPayment::Change(_))) { // The necessary fee is the fee self.intent.weight_and_necessary_fee().1 @@ -306,7 +306,7 @@ impl SignableTransactionWithKeyImages { .iter() .filter_map(|payment| match payment { InternalPayment::Payment(_, amount) => Some(amount), - InternalPayment::Change(_, _) => None, + InternalPayment::Change(_) => None, }) .sum::(); // Safe since the constructor checks inputs >= (payments + fee) diff --git a/coins/monero/wallet/src/send/tx_keys.rs b/coins/monero/wallet/src/send/tx_keys.rs index 09362b47..628f643e 100644 --- a/coins/monero/wallet/src/send/tx_keys.rs +++ b/coins/monero/wallet/src/send/tx_keys.rs @@ -12,7 +12,7 @@ use crate::{ primitives::{keccak256, Commitment}, ringct::EncryptedAmount, SharedKeyDerivations, OutputWithDecoys, - send::{InternalPayment, SignableTransaction, key_image_sort}, + send::{ChangeEnum, InternalPayment, SignableTransaction, key_image_sort}, }; impl SignableTransaction { @@ -42,15 +42,13 @@ impl SignableTransaction { fn has_payments_to_subaddresses(&self) -> bool { self.payments.iter().any(|payment| match payment { InternalPayment::Payment(addr, _) => addr.is_subaddress(), - InternalPayment::Change(addr, view) => { - if view.is_some() { - // It should not be possible to construct a change specification to a subaddress with a - // view key - // TODO - debug_assert!(!addr.is_subaddress()); - } - addr.is_subaddress() - } + InternalPayment::Change(change) => match change { + ChangeEnum::AddressOnly(addr) => addr.is_subaddress(), + // These aren't considered payments to subaddresses as we don't need to send to them as + // subaddresses + // We can calculate the shared key using the view key, as if we were receiving, instead + ChangeEnum::Standard { .. } | ChangeEnum::Guaranteed { .. } => false, + }, }) } @@ -62,7 +60,10 @@ impl SignableTransaction { let has_change_view = self.payments.iter().any(|payment| match payment { InternalPayment::Payment(_, _) => false, - InternalPayment::Change(_, view) => view.is_some(), + InternalPayment::Change(change) => match change { + ChangeEnum::AddressOnly(_) => false, + ChangeEnum::Standard { .. } | ChangeEnum::Guaranteed { .. } => true, + }, }); /* @@ -107,11 +108,17 @@ impl SignableTransaction { let ecdh = match payment { // If we don't have the view key, use the key dedicated for this address (r A) - InternalPayment::Payment(_, _) | InternalPayment::Change(_, None) => { + InternalPayment::Payment(_, _) | + InternalPayment::Change(ChangeEnum::AddressOnly { .. }) => { Zeroizing::new(key_to_use.deref() * addr.view()) } // If we do have the view key, use the commitment to the key (a R) - InternalPayment::Change(_, Some(view)) => Zeroizing::new(view.deref() * tx_key_pub), + InternalPayment::Change(ChangeEnum::Standard { view_pair, .. }) => { + Zeroizing::new(view_pair.view.deref() * tx_key_pub) + } + InternalPayment::Change(ChangeEnum::Guaranteed { view_pair, .. }) => { + Zeroizing::new(view_pair.0.view.deref() * tx_key_pub) + } }; res.push(ecdh); @@ -172,9 +179,6 @@ impl SignableTransaction { panic!("filtered payment wasn't a payment") }; - // TODO: Support subaddresses as change? - debug_assert!(addr.is_subaddress()); - return (tx_key.deref() * addr.spend(), vec![]); } @@ -207,14 +211,14 @@ impl SignableTransaction { for (payment, shared_key_derivations) in self.payments.iter().zip(shared_key_derivations) { let amount = match payment { InternalPayment::Payment(_, amount) => *amount, - InternalPayment::Change(_, _) => { + InternalPayment::Change(_) => { let inputs = self.inputs.iter().map(|input| input.commitment().amount).sum::(); let payments = self .payments .iter() .filter_map(|payment| match payment { InternalPayment::Payment(_, amount) => Some(amount), - InternalPayment::Change(_, _) => None, + InternalPayment::Change(_) => None, }) .sum::(); let necessary_fee = self.weight_and_necessary_fee().1; diff --git a/coins/monero/wallet/src/view_pair.rs b/coins/monero/wallet/src/view_pair.rs index eb89d96d..3b09f088 100644 --- a/coins/monero/wallet/src/view_pair.rs +++ b/coins/monero/wallet/src/view_pair.rs @@ -27,7 +27,7 @@ pub enum ViewPairError { /// The pair of keys necessary to scan transactions. /// /// This is composed of the public spend key and the private view key. -#[derive(Clone, Zeroize, ZeroizeOnDrop)] +#[derive(Clone, PartialEq, Eq, Zeroize, ZeroizeOnDrop)] pub struct ViewPair { spend: EdwardsPoint, pub(crate) view: Zeroizing, @@ -99,7 +99,7 @@ impl ViewPair { /// 'Guaranteed' outputs, or transactions outputs to the burning bug, are not officially specified /// by the Monero project. They should only be used if necessary. No support outside of /// monero-wallet is promised. -#[derive(Clone, Zeroize)] +#[derive(Clone, PartialEq, Eq, Zeroize)] pub struct GuaranteedViewPair(pub(crate) ViewPair); impl GuaranteedViewPair { diff --git a/coins/monero/wallet/tests/runner/mod.rs b/coins/monero/wallet/tests/runner/mod.rs index 679f748e..f8c1a4ea 100644 --- a/coins/monero/wallet/tests/runner/mod.rs +++ b/coins/monero/wallet/tests/runner/mod.rs @@ -254,10 +254,11 @@ macro_rules! test { rct_type, outgoing_view, Change::new( - &ViewPair::new( + ViewPair::new( &Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE, Zeroizing::new(Scalar::random(&mut OsRng)) ).unwrap(), + None, ), rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(), ); @@ -267,6 +268,8 @@ macro_rules! test { #[cfg(feature = "multisig")] let keys = keys.clone(); + assert_eq!(&SignableTransaction::read(&mut tx.serialize().as_slice()).unwrap(), &tx); + let eventuality = Eventuality::from(tx.clone()); let tx = if !multisig { diff --git a/coins/monero/wallet/tests/send.rs b/coins/monero/wallet/tests/send.rs index 0641760d..86a801d7 100644 --- a/coins/monero/wallet/tests/send.rs +++ b/coins/monero/wallet/tests/send.rs @@ -115,7 +115,7 @@ test!( let mut builder = SignableTransactionBuilder::new( rct_type, outgoing_view, - Change::new(&change_view), + Change::new(change_view.clone(), None), rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(), ); add_inputs(rct_type, &rpc, vec![outputs.first().unwrap().clone()], &mut builder).await; @@ -144,6 +144,8 @@ test!( assert!(sub_outputs.len() == 1); assert_eq!(sub_outputs[0].transaction(), tx.hash()); assert_eq!(sub_outputs[0].commitment().amount, 1); + assert!(sub_outputs[0].subaddress().unwrap().account() == 0); + assert!(sub_outputs[0].subaddress().unwrap().address() == 1); // Make sure only one R was included in TX extra assert!(Extra::read::<&[u8]>(&mut tx.prefix().extra.as_ref()) @@ -333,3 +335,60 @@ test!( }, ), ); + +test!( + subaddress_change, + ( + // Consume this builder for an output we can use in the future + // This is needed because we can't get the input from the passed in builder + |_, mut builder: Builder, addr| async move { + builder.add_payment(addr, 1000000000000); + (builder.build().unwrap(), ()) + }, + |rpc, block, tx: Transaction, mut scanner: Scanner, ()| async move { + let outputs = scanner.scan(&rpc, &block).await.unwrap().not_additionally_locked(); + assert_eq!(outputs.len(), 1); + assert_eq!(outputs[0].transaction(), tx.hash()); + assert_eq!(outputs[0].commitment().amount, 1000000000000); + outputs + }, + ), + ( + |rct_type, rpc: SimpleRequestRpc, _, _, outputs: Vec| async move { + use monero_wallet::rpc::FeePriority; + + let view_priv = Zeroizing::new(Scalar::random(&mut OsRng)); + let mut outgoing_view = Zeroizing::new([0; 32]); + OsRng.fill_bytes(outgoing_view.as_mut()); + let change_view = + ViewPair::new(&Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE, view_priv.clone()) + .unwrap(); + + let mut builder = SignableTransactionBuilder::new( + rct_type, + outgoing_view, + Change::new(change_view.clone(), Some(SubaddressIndex::new(0, 1).unwrap())), + rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(), + ); + add_inputs(rct_type, &rpc, vec![outputs.first().unwrap().clone()], &mut builder).await; + + // Send to a random address + let view = ViewPair::new( + &Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE, + Zeroizing::new(Scalar::random(&mut OsRng)), + ) + .unwrap(); + builder.add_payment(view.legacy_address(Network::Mainnet), 1); + (builder.build().unwrap(), change_view) + }, + |rpc, block, _, _, change_view: ViewPair| async move { + // Make sure the change can pick up its output + let mut change_scanner = Scanner::new(change_view); + change_scanner.register_subaddress(SubaddressIndex::new(0, 1).unwrap()); + let outputs = change_scanner.scan(&rpc, &block).await.unwrap().not_additionally_locked(); + assert!(outputs.len() == 1); + assert!(outputs[0].subaddress().unwrap().account() == 0); + assert!(outputs[0].subaddress().unwrap().address() == 1); + }, + ), +); diff --git a/tests/full-stack/src/tests/mint_and_burn.rs b/tests/full-stack/src/tests/mint_and_burn.rs index a81575c9..3bb9e11f 100644 --- a/tests/full-stack/src/tests/mint_and_burn.rs +++ b/tests/full-stack/src/tests/mint_and_burn.rs @@ -389,7 +389,7 @@ async fn mint_and_burn_test() { ), 1_100_000_000_000, )], - Change::new(&view_pair), + Change::new(view_pair.clone(), None), vec![Shorthand::transfer(None, serai_addr).encode()], rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(), ) diff --git a/tests/processor/src/networks.rs b/tests/processor/src/networks.rs index 1992000b..d6ccb1f5 100644 --- a/tests/processor/src/networks.rs +++ b/tests/processor/src/networks.rs @@ -474,7 +474,7 @@ impl Wallet { outgoing_view_key, inputs, vec![(to_addr, AMOUNT)], - Change::new(view_pair), + Change::new(view_pair.clone(), None), data, rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(), )