From 5e62072a0f06fdc8790f9f3cb7690c5f249c02f8 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 11 Mar 2023 10:31:58 -0500 Subject: [PATCH] Fix #237 --- coins/monero/src/wallet/extra.rs | 31 ++- coins/monero/src/wallet/mod.rs | 4 +- coins/monero/src/wallet/send/builder.rs | 10 +- coins/monero/src/wallet/send/mod.rs | 255 +++++++++++++++----- coins/monero/src/wallet/send/multisig.rs | 21 +- coins/monero/tests/runner.rs | 10 +- coins/monero/tests/send.rs | 69 +++++- coins/monero/tests/wallet2_compatibility.rs | 35 +-- processor/src/coin/monero.rs | 7 +- 9 files changed, 342 insertions(+), 100 deletions(-) diff --git a/coins/monero/src/wallet/extra.rs b/coins/monero/src/wallet/extra.rs index b8b5f3c1..1b902109 100644 --- a/coins/monero/src/wallet/extra.rs +++ b/coins/monero/src/wallet/extra.rs @@ -21,7 +21,7 @@ pub const ARBITRARY_DATA_MARKER: u8 = 127; pub const MAX_ARBITRARY_DATA_SIZE: usize = MAX_TX_EXTRA_NONCE_SIZE - 1; #[derive(Clone, Copy, PartialEq, Eq, Debug, Zeroize)] -pub(crate) enum PaymentId { +pub enum PaymentId { Unencrypted([u8; 32]), Encrypted([u8; 8]), } @@ -31,6 +31,7 @@ impl BitXor<[u8; 8]> for PaymentId { fn bitxor(self, bytes: [u8; 8]) -> PaymentId { match self { + // Don't perform the xor since this isn't intended to be encrypted with xor PaymentId::Unencrypted(_) => self, PaymentId::Encrypted(id) => { PaymentId::Encrypted((u64::from_le_bytes(id) ^ u64::from_le_bytes(bytes)).to_le_bytes()) @@ -40,7 +41,7 @@ impl BitXor<[u8; 8]> for PaymentId { } impl PaymentId { - pub(crate) fn write(&self, w: &mut W) -> io::Result<()> { + pub fn write(&self, w: &mut W) -> io::Result<()> { match self { PaymentId::Unencrypted(id) => { w.write_all(&[PAYMENT_ID_MARKER])?; @@ -54,7 +55,7 @@ impl PaymentId { Ok(()) } - fn read(r: &mut R) -> io::Result { + pub fn read(r: &mut R) -> io::Result { Ok(match read_byte(r)? { 0 => PaymentId::Unencrypted(read_bytes(r)?), 1 => PaymentId::Encrypted(read_bytes(r)?), @@ -65,7 +66,7 @@ impl PaymentId { // Doesn't bother with padding nor MinerGate #[derive(Clone, PartialEq, Eq, Debug, Zeroize)] -pub(crate) enum ExtraField { +pub enum ExtraField { PublicKey(EdwardsPoint), Nonce(Vec), MergeMining(usize, [u8; 32]), @@ -73,7 +74,7 @@ pub(crate) enum ExtraField { } impl ExtraField { - fn write(&self, w: &mut W) -> io::Result<()> { + pub fn write(&self, w: &mut W) -> io::Result<()> { match self { ExtraField::PublicKey(key) => { w.write_all(&[1])?; @@ -96,7 +97,7 @@ impl ExtraField { Ok(()) } - fn read(r: &mut R) -> io::Result { + pub fn read(r: &mut R) -> io::Result { Ok(match read_byte(r)? { 1 => ExtraField::PublicKey(read_point(r)?), 2 => ExtraField::Nonce({ @@ -118,9 +119,9 @@ impl ExtraField { } #[derive(Clone, PartialEq, Eq, Debug, Zeroize)] -pub(crate) struct Extra(Vec); +pub struct Extra(Vec); impl Extra { - pub(crate) fn keys(&self) -> Option<(EdwardsPoint, Option>)> { + pub fn keys(&self) -> Option<(EdwardsPoint, Option>)> { let mut key = None; let mut additional = None; for field in &self.0 { @@ -136,7 +137,7 @@ impl Extra { key.map(|key| (key, additional)) } - pub(crate) fn payment_id(&self) -> Option { + pub fn payment_id(&self) -> Option { for field in &self.0 { if let ExtraField::Nonce(data) = field { return PaymentId::read::<&[u8]>(&mut data.as_ref()).ok(); @@ -145,7 +146,7 @@ impl Extra { None } - pub(crate) fn data(&self) -> Vec> { + pub fn data(&self) -> Vec> { let mut res = vec![]; for field in &self.0 { if let ExtraField::Nonce(data) = field { @@ -182,14 +183,20 @@ impl Extra { data.iter().map(|v| 1 + varint_len(v.len()) + v.len()).sum::() } - pub(crate) fn write(&self, w: &mut W) -> io::Result<()> { + pub fn write(&self, w: &mut W) -> io::Result<()> { for field in &self.0 { field.write(w)?; } Ok(()) } - pub(crate) fn read(r: &mut R) -> io::Result { + pub fn serialize(&self) -> Vec { + let mut buf = vec![]; + self.write(&mut buf).unwrap(); + buf + } + + pub fn read(r: &mut R) -> io::Result { let mut res = Extra(vec![]); let mut field; while { diff --git a/coins/monero/src/wallet/mod.rs b/coins/monero/src/wallet/mod.rs index f0fd398e..1f23406b 100644 --- a/coins/monero/src/wallet/mod.rs +++ b/coins/monero/src/wallet/mod.rs @@ -28,7 +28,9 @@ pub(crate) mod decoys; pub(crate) use decoys::Decoys; mod send; -pub use send::{Fee, TransactionError, SignableTransaction, SignableTransactionBuilder}; +pub use send::{Fee, TransactionError, Change, SignableTransaction, SignableTransactionBuilder}; +#[cfg(feature = "multisig")] +pub(crate) use send::InternalPayment; #[cfg(feature = "multisig")] pub use send::TransactionMachine; diff --git a/coins/monero/src/wallet/send/builder.rs b/coins/monero/src/wallet/send/builder.rs index ef70900a..d1e632cf 100644 --- a/coins/monero/src/wallet/send/builder.rs +++ b/coins/monero/src/wallet/send/builder.rs @@ -5,7 +5,7 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; use crate::{ Protocol, wallet::{ - address::MoneroAddress, Fee, SpendableOutput, SignableTransaction, TransactionError, + address::MoneroAddress, Fee, SpendableOutput, Change, SignableTransaction, TransactionError, extra::MAX_ARBITRARY_DATA_SIZE, }, }; @@ -17,14 +17,14 @@ struct SignableTransactionBuilderInternal { inputs: Vec, payments: Vec<(MoneroAddress, u64)>, - change_address: Option, + change_address: Option, 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: Fee, change_address: Option) -> Self { + fn new(protocol: Protocol, fee: Fee, change_address: Option) -> Self { Self { protocol, fee, inputs: vec![], payments: vec![], change_address, data: vec![] } } @@ -77,7 +77,7 @@ impl SignableTransactionBuilder { Self(self.0.clone()) } - pub fn new(protocol: Protocol, fee: Fee, change_address: Option) -> Self { + pub fn new(protocol: Protocol, fee: Fee, change_address: Option) -> Self { Self(Arc::new(RwLock::new(SignableTransactionBuilderInternal::new( protocol, fee, @@ -117,7 +117,7 @@ impl SignableTransactionBuilder { read.protocol, read.inputs.clone(), read.payments.clone(), - read.change_address, + read.change_address.clone(), read.data.clone(), read.fee, ) diff --git a/coins/monero/src/wallet/send/mod.rs b/coins/monero/src/wallet/send/mod.rs index aac69ee9..f8eec1e6 100644 --- a/coins/monero/src/wallet/send/mod.rs +++ b/coins/monero/src/wallet/send/mod.rs @@ -1,4 +1,4 @@ -use core::ops::Deref; +use core::{ops::Deref, fmt}; use thiserror::Error; @@ -8,7 +8,11 @@ use rand::seq::SliceRandom; use zeroize::{Zeroize, ZeroizeOnDrop, Zeroizing}; use group::Group; -use curve25519_dalek::{constants::ED25519_BASEPOINT_TABLE, scalar::Scalar, edwards::EdwardsPoint}; +use curve25519_dalek::{ + constants::{ED25519_BASEPOINT_POINT, ED25519_BASEPOINT_TABLE}, + scalar::Scalar, + edwards::EdwardsPoint, +}; use dalek_ff_group as dfg; #[cfg(feature = "multisig")] @@ -25,9 +29,9 @@ use crate::{ transaction::{Input, Output, Timelock, TransactionPrefix, Transaction}, rpc::{Rpc, RpcError}, wallet::{ - address::MoneroAddress, - SpendableOutput, Decoys, PaymentId, ExtraField, Extra, key_image_sort, uniqueness, shared_key, - commitment_mask, amount_encryption, + address::{Network, AddressSpec, MoneroAddress}, + ViewPair, SpendableOutput, Decoys, PaymentId, ExtraField, Extra, key_image_sort, uniqueness, + shared_key, commitment_mask, amount_encryption, extra::{ARBITRARY_DATA_MARKER, MAX_ARBITRARY_DATA_SIZE}, }, }; @@ -51,24 +55,23 @@ struct SendOutput { } impl SendOutput { - fn new( - r: &Zeroizing, + #[allow(non_snake_case)] + fn internal( unique: [u8; 32], output: (usize, (MoneroAddress, u64)), + ecdh_left: &Zeroizing, + ecdh_right: &EdwardsPoint, + R: EdwardsPoint, ) -> (SendOutput, Option<[u8; 8]>) { let o = output.0; let output = output.1; let (view_tag, shared_key, payment_id_xor) = - shared_key(Some(unique).filter(|_| output.0.is_guaranteed()), r, &output.0.view, o); + shared_key(Some(unique).filter(|_| output.0.is_guaranteed()), ecdh_left, ecdh_right, o); ( SendOutput { - R: if !output.0.is_subaddress() { - r.deref() * &ED25519_BASEPOINT_TABLE - } else { - r.deref() * output.0.spend - }, + R, view_tag, dest: ((&shared_key * &ED25519_BASEPOINT_TABLE) + output.0.spend), commitment: Commitment::new(commitment_mask(shared_key), output.1), @@ -80,6 +83,39 @@ impl SendOutput { .map(|id| (u64::from_le_bytes(id) ^ u64::from_le_bytes(payment_id_xor)).to_le_bytes()), ) } + + fn new( + r: &Zeroizing, + unique: [u8; 32], + output: (usize, (MoneroAddress, u64)), + ) -> (SendOutput, Option<[u8; 8]>) { + let address = output.1 .0; + SendOutput::internal( + unique, + output, + r, + &address.view, + if !address.is_subaddress() { + r.deref() * &ED25519_BASEPOINT_TABLE + } else { + r.deref() * address.spend + }, + ) + } + + fn change( + ecdh: &EdwardsPoint, + unique: [u8; 32], + output: (usize, (MoneroAddress, u64)), + ) -> (SendOutput, Option<[u8; 8]>) { + SendOutput::internal( + unique, + output, + &Zeroizing::new(Scalar::one()), + ecdh, + ED25519_BASEPOINT_POINT, + ) + } } #[derive(Clone, PartialEq, Eq, Debug, Error)] @@ -179,21 +215,66 @@ impl Fee { pub struct SignableTransaction { protocol: Protocol, inputs: Vec, - payments: Vec<(MoneroAddress, u64)>, + payments: Vec, data: Vec>, fee: u64, } +/// Specification for a change output. +#[derive(Clone, PartialEq, Eq, Zeroize)] +pub struct Change { + address: MoneroAddress, + view: Option>, +} + +impl fmt::Debug for Change { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.debug_struct("Change").field("address", &self.address).finish_non_exhaustive() + } +} + +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( + 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 { + Change { address, view: None } + } +} + +#[derive(Clone, PartialEq, Eq, Debug, Zeroize)] +pub(crate) enum InternalPayment { + Payment((MoneroAddress, u64)), + Change(Change, u64), +} + impl SignableTransaction { - /// Create a signable transaction. If the change address is specified, leftover funds will be - /// sent to it. If the change address isn't specified, up to 16 outputs may be specified, using - /// any leftover funds as a bonus to the fee. The optional data field will be embedded in TX - /// extra. + /// Create a signable transaction. + /// + /// Up to 16 outputs may be present, including the change output. + /// + /// If the change address is specified, leftover funds will be sent to it. + /// + /// Each chunk of data must not exceed MAX_ARBITRARY_DATA_SIZE. pub fn new( protocol: Protocol, inputs: Vec, mut payments: Vec<(MoneroAddress, u64)>, - change_address: Option, + change_address: Option, data: Vec>, fee_rate: Fee, ) -> Result { @@ -208,8 +289,8 @@ impl SignableTransaction { for payment in &payments { count(payment.0); } - if let Some(change) = change_address { - count(change); + if let Some(change) = change_address.as_ref() { + count(change.address); } if payment_ids > 1 { Err(TransactionError::MultiplePaymentIds)?; @@ -232,12 +313,11 @@ impl SignableTransaction { // TODO TX MAX SIZE - // If we don't have two outputs, as required by Monero, add a second - let mut change = payments.len() == 1; - if change && change_address.is_none() { + // If we don't have two outputs, as required by Monero, error + if (payments.len() == 1) && change_address.is_none() { Err(TransactionError::NoChange)?; } - let outputs = payments.len() + usize::from(change); + let outputs = payments.len() + usize::from(change_address.is_some()); // Add a dummy payment ID if there's only 2 payments has_payment_id |= outputs == 2; @@ -245,37 +325,24 @@ impl SignableTransaction { let extra = Extra::fee_weight(outputs, has_payment_id, data.as_ref()); // Calculate the fee. - let mut fee = - fee_rate.calculate(Transaction::fee_weight(protocol, inputs.len(), outputs, extra)); + let fee = fee_rate.calculate(Transaction::fee_weight(protocol, inputs.len(), outputs, extra)); // Make sure we have enough funds let in_amount = inputs.iter().map(|input| input.commitment().amount).sum::(); - let mut out_amount = payments.iter().map(|payment| payment.1).sum::() + fee; + let out_amount = payments.iter().map(|payment| payment.1).sum::() + fee; if in_amount < out_amount { Err(TransactionError::NotEnoughFunds(in_amount, out_amount))?; } - // If we have yet to add a change output, do so if it's economically viable - if (!change) && change_address.is_some() && (in_amount != out_amount) { - // Check even with the new fee, there's remaining funds - let change_fee = - fee_rate.calculate(Transaction::fee_weight(protocol, inputs.len(), outputs + 1, extra)) - - fee; - if (out_amount + change_fee) < in_amount { - change = true; - out_amount += change_fee; - fee += change_fee; - } - } - - if change { - payments.push((change_address.unwrap(), in_amount - out_amount)); - } - - if payments.len() > MAX_OUTPUTS { + if outputs > MAX_OUTPUTS { Err(TransactionError::TooManyOutputs)?; } + let mut payments = payments.drain(..).map(InternalPayment::Payment).collect::>(); + if let Some(change) = change_address { + payments.push(InternalPayment::Change(change, in_amount - out_amount)); + } + Ok(SignableTransaction { protocol, inputs, payments, data, fee }) } @@ -289,23 +356,93 @@ impl SignableTransaction { // Used for all non-subaddress outputs, or if there's only one subaddress output and a change let tx_key = Zeroizing::new(random_scalar(rng)); - // TODO: Support not needing additional when one subaddress and non-subaddress change - let additional = self.payments.iter().filter(|payment| payment.0.is_subaddress()).count() != 0; + let mut tx_public_key = tx_key.deref() * &ED25519_BASEPOINT_TABLE; + + // If any of these outputs are to a subaddress, we need keys distinct to them + // The only time this *does not* force having additional keys is when the only other output + // is a change output we have the view key for, enabling rewriting rA to aR + let mut has_change_view = false; + let subaddresses = self + .payments + .iter() + .filter(|payment| match *payment { + InternalPayment::Payment(payment) => payment.0.is_subaddress(), + InternalPayment::Change(change, _) => { + if change.view.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()); + } + change.address.is_subaddress() + } + }) + .count() != + 0; + + // We need additional keys if we have any subaddresses + let mut additional = subaddresses; + // Unless the above change view key path is taken + if (self.payments.len() == 2) && has_change_view { + additional = false; + } + let modified_change_ecdh = subaddresses && (!additional); + + // If we're using the aR rewrite, update tx_public_key from rG to rB + if modified_change_ecdh { + for payment in &self.payments { + match payment { + InternalPayment::Payment(payment) => { + // This should be the only payment and it should be a subaddress + debug_assert!(payment.0.is_subaddress()); + tx_public_key = tx_key.deref() * payment.0.spend; + } + InternalPayment::Change(_, _) => {} + } + } + debug_assert!(tx_public_key != (tx_key.deref() * &ED25519_BASEPOINT_TABLE)); + } // Actually create the outputs let mut outputs = Vec::with_capacity(self.payments.len()); let mut id = None; - for payment in self.payments.drain(..).enumerate() { - // If this is a subaddress, generate a dedicated r. Else, reuse the TX key - let dedicated = Zeroizing::new(random_scalar(&mut *rng)); - let use_dedicated = additional && payment.1 .0.is_subaddress(); - let r = if use_dedicated { &dedicated } else { &tx_key }; + for (o, mut payment) in self.payments.drain(..).enumerate() { + // Downcast the change output to a payment output if it doesn't require special handling + // regarding it's view key + payment = if !modified_change_ecdh { + if let InternalPayment::Change(change, amount) = &payment { + InternalPayment::Payment((change.address, *amount)) + } else { + payment + } + } else { + payment + }; - let (mut output, payment_id) = SendOutput::new(r, uniqueness, payment); - // If this used the tx_key, randomize its R - if !use_dedicated { - output.R = dfg::EdwardsPoint::random(&mut *rng).0; - } + let (output, payment_id) = match payment { + InternalPayment::Payment(payment) => { + // If this is a subaddress, generate a dedicated r. Else, reuse the TX key + let dedicated = Zeroizing::new(random_scalar(&mut *rng)); + let use_dedicated = additional && payment.0.is_subaddress(); + let r = if use_dedicated { &dedicated } else { &tx_key }; + + let (mut output, payment_id) = SendOutput::new(r, uniqueness, (o, payment)); + if modified_change_ecdh { + debug_assert_eq!(tx_public_key, output.R); + } + // If this used tx_key, randomize its R + if !use_dedicated { + output.R = dfg::EdwardsPoint::random(&mut *rng).0; + } + (output, payment_id) + } + 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))) + } + }; outputs.push(output); id = id.or(payment_id); @@ -330,7 +467,7 @@ impl SignableTransaction { // Create the TX extra let extra = { let mut extra = Extra::new( - tx_key.deref() * &ED25519_BASEPOINT_TABLE, + tx_public_key, if additional { outputs.iter().map(|output| output.R).collect() } else { vec![] }, ); diff --git a/coins/monero/src/wallet/send/multisig.rs b/coins/monero/src/wallet/send/multisig.rs index d665928e..8a1327e9 100644 --- a/coins/monero/src/wallet/send/multisig.rs +++ b/coins/monero/src/wallet/send/multisig.rs @@ -4,6 +4,8 @@ use std::{ collections::HashMap, }; +use zeroize::Zeroizing; + use rand_core::{RngCore, CryptoRng, SeedableRng}; use rand_chacha::ChaCha20Rng; @@ -29,7 +31,9 @@ use crate::{ }, transaction::{Input, Transaction}, rpc::Rpc, - wallet::{TransactionError, SignableTransaction, Decoys, key_image_sort, uniqueness}, + wallet::{ + TransactionError, InternalPayment, SignableTransaction, Decoys, key_image_sort, uniqueness, + }, }; /// FROST signing machine to produce a signed transaction. @@ -108,8 +112,19 @@ impl SignableTransaction { transcript.append_message(b"input_shared_key", input.key_offset().to_bytes()); } for payment in &self.payments { - transcript.append_message(b"payment_address", payment.0.to_string().as_bytes()); - transcript.append_message(b"payment_amount", payment.1.to_le_bytes()); + match payment { + InternalPayment::Payment(payment) => { + transcript.append_message(b"payment_address", payment.0.to_string().as_bytes()); + 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_view_key", Zeroizing::new(view.to_bytes())); + } + transcript.append_message(b"change_amount", amount.to_le_bytes()); + } + } } let mut key_images = vec![]; diff --git a/coins/monero/tests/runner.rs b/coins/monero/tests/runner.rs index 5eac16b5..5c3bb64f 100644 --- a/coins/monero/tests/runner.rs +++ b/coins/monero/tests/runner.rs @@ -155,7 +155,7 @@ macro_rules! test { use monero_serai::{ random_scalar, wallet::{ - address::{Network, AddressSpec}, ViewPair, Scanner, SignableTransaction, + address::{Network, AddressSpec}, ViewPair, Scanner, Change, SignableTransaction, SignableTransactionBuilder, }, }; @@ -196,7 +196,13 @@ macro_rules! test { let builder = SignableTransactionBuilder::new( rpc.get_protocol().await.unwrap(), rpc.get_fee().await.unwrap(), - Some(random_address().2), + Some(Change::new( + &ViewPair::new( + &random_scalar(&mut OsRng) * &ED25519_BASEPOINT_TABLE, + Zeroizing::new(random_scalar(&mut OsRng)) + ), + false + )), ); let sign = |tx: SignableTransaction| { diff --git a/coins/monero/tests/send.rs b/coins/monero/tests/send.rs index 84eb2bb0..66f8e593 100644 --- a/coins/monero/tests/send.rs +++ b/coins/monero/tests/send.rs @@ -1,6 +1,7 @@ use monero_serai::{ - wallet::{ReceivedOutput, SpendableOutput}, + wallet::{extra::Extra, address::SubaddressIndex, ReceivedOutput, SpendableOutput}, transaction::Transaction, + rpc::Rpc, }; mod runner; @@ -49,3 +50,69 @@ test!( }, ), ); + +test!( + // Ideally, this would be single_R, yet it isn't feasible to apply allow(non_snake_case) here + single_r_subaddress_send, + ( + // 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(), ()) + }, + |_, 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 + }, + ), + ( + |rpc: Rpc, _, _, mut outputs: Vec| async move { + let change_view = ViewPair::new( + &random_scalar(&mut OsRng) * &ED25519_BASEPOINT_TABLE, + Zeroizing::new(random_scalar(&mut OsRng)), + ); + + let mut builder = SignableTransactionBuilder::new( + rpc.get_protocol().await.unwrap(), + rpc.get_fee().await.unwrap(), + Some(Change::new(&change_view, false)), + ); + builder.add_input(SpendableOutput::from(&rpc, outputs.swap_remove(0)).await.unwrap()); + + // Send to a subaddress + let sub_view = ViewPair::new( + &random_scalar(&mut OsRng) * &ED25519_BASEPOINT_TABLE, + Zeroizing::new(random_scalar(&mut OsRng)), + ); + builder.add_payment( + sub_view + .address(Network::Mainnet, AddressSpec::Subaddress(SubaddressIndex::new(0, 1).unwrap())), + 1, + ); + (builder.build().unwrap(), (change_view, sub_view)) + }, + |_, tx: Transaction, _, views: (ViewPair, ViewPair)| async move { + // Make sure the change can pick up its output + let mut change_scanner = Scanner::from_view(views.0, Some(HashSet::new())); + assert!(change_scanner.scan_transaction(&tx).not_locked().len() == 1); + + // Make sure the subaddress can pick up its output + let mut sub_scanner = Scanner::from_view(views.1, Some(HashSet::new())); + sub_scanner.register_subaddress(SubaddressIndex::new(0, 1).unwrap()); + let sub_outputs = sub_scanner.scan_transaction(&tx).not_locked(); + assert!(sub_outputs.len() == 1); + assert_eq!(sub_outputs[0].commitment().amount, 1); + + // Make sure only one R was included in TX extra + assert!(Extra::read::<&[u8]>(&mut tx.prefix.extra.as_ref()) + .unwrap() + .keys() + .unwrap() + .1 + .is_none()); + }, + ), +); diff --git a/coins/monero/tests/wallet2_compatibility.rs b/coins/monero/tests/wallet2_compatibility.rs index 1b028435..113249c0 100644 --- a/coins/monero/tests/wallet2_compatibility.rs +++ b/coins/monero/tests/wallet2_compatibility.rs @@ -21,7 +21,7 @@ use monero_serai::{ transaction::Transaction, wallet::{ address::{Network, AddressSpec, SubaddressIndex, MoneroAddress}, - extra::MAX_TX_EXTRA_NONCE_SIZE, + extra::{MAX_TX_EXTRA_NONCE_SIZE, Extra}, Scanner, }, rpc::Rpc, @@ -56,7 +56,7 @@ async fn initialize_rpcs() -> (WalletClient, Rpc, monero_rpc::monero::Address) { let wallet_rpc_addr = if address_resp.is_ok() { address_resp.unwrap().address } else { - wallet_rpc.create_wallet("test_wallet".to_string(), None, "English".to_string()).await.unwrap(); + wallet_rpc.create_wallet("wallet".to_string(), None, "English".to_string()).await.unwrap(); let addr = wallet_rpc.get_address(0, None).await.unwrap().address; daemon_rpc.generate_blocks(&addr.to_string(), 70).await.unwrap(); addr @@ -64,7 +64,7 @@ async fn initialize_rpcs() -> (WalletClient, Rpc, monero_rpc::monero::Address) { (wallet_rpc, daemon_rpc, wallet_rpc_addr) } -async fn test_from_wallet_rpc_to_self(spec: AddressSpec) { +async fn from_wallet_rpc_to_self(spec: AddressSpec) { // initialize rpc let (wallet_rpc, daemon_rpc, wallet_rpc_addr) = initialize_rpcs().await; @@ -109,24 +109,23 @@ async fn test_from_wallet_rpc_to_self(spec: AddressSpec) { } async_sequential!( - async fn test_receipt_of_wallet_rpc_tx_standard() { - test_from_wallet_rpc_to_self(AddressSpec::Standard).await; + async fn receipt_of_wallet_rpc_tx_standard() { + from_wallet_rpc_to_self(AddressSpec::Standard).await; } - async fn test_receipt_of_wallet_rpc_tx_subaddress() { - test_from_wallet_rpc_to_self(AddressSpec::Subaddress(SubaddressIndex::new(0, 1).unwrap())) - .await; + async fn receipt_of_wallet_rpc_tx_subaddress() { + from_wallet_rpc_to_self(AddressSpec::Subaddress(SubaddressIndex::new(0, 1).unwrap())).await; } - async fn test_receipt_of_wallet_rpc_tx_integrated() { + async fn receipt_of_wallet_rpc_tx_integrated() { let mut payment_id = [0u8; 8]; OsRng.fill_bytes(&mut payment_id); - test_from_wallet_rpc_to_self(AddressSpec::Integrated(payment_id)).await; + from_wallet_rpc_to_self(AddressSpec::Integrated(payment_id)).await; } ); test!( - test_send_to_wallet_rpc_standard, + send_to_wallet_rpc_standard, ( |_, mut builder: Builder, _| async move { // initialize rpc @@ -151,7 +150,7 @@ test!( ); test!( - test_send_to_wallet_rpc_subaddress, + send_to_wallet_rpc_subaddress, ( |_, mut builder: Builder, _| async move { // initialize rpc @@ -173,12 +172,20 @@ test!( data.0.get_transfer(Hash::from_slice(&tx.hash()), None).await.unwrap().unwrap(); assert_eq!(transfer.amount.as_pico(), 1000000); assert_eq!(transfer.subaddr_index, Index { major: 0, minor: data.1 }); + + // Make sure only one R was included in TX extra + assert!(Extra::read::<&[u8]>(&mut tx.prefix.extra.as_ref()) + .unwrap() + .keys() + .unwrap() + .1 + .is_none()); }, ), ); test!( - test_send_to_wallet_rpc_integrated, + send_to_wallet_rpc_integrated, ( |_, mut builder: Builder, _| async move { // initialize rpc @@ -205,7 +212,7 @@ test!( ); test!( - test_send_to_wallet_rpc_with_arb_data, + send_to_wallet_rpc_with_arb_data, ( |_, mut builder: Builder, _| async move { // initialize rpc diff --git a/processor/src/coin/monero.rs b/processor/src/coin/monero.rs index 67329a74..9103c3b0 100644 --- a/processor/src/coin/monero.rs +++ b/processor/src/coin/monero.rs @@ -15,7 +15,7 @@ use monero_serai::{ wallet::{ ViewPair, Scanner, address::{Network, SubaddressIndex, AddressSpec, MoneroAddress}, - Fee, SpendableOutput, SignableTransaction as MSignableTransaction, TransactionMachine, + Fee, SpendableOutput, Change, SignableTransaction as MSignableTransaction, TransactionMachine, }, }; @@ -236,7 +236,8 @@ impl Coin for Monero { self.rpc.get_protocol().await.unwrap(), // TODO: Make this deterministic inputs.drain(..).map(|input| input.0).collect(), payments.to_vec(), - change.map(|change| self.address_internal(change, CHANGE_SUBADDRESS)), + change + .map(|change| Change::fingerprintable(self.address_internal(change, CHANGE_SUBADDRESS))), vec![], fee, ) @@ -316,7 +317,7 @@ impl Coin for Monero { self.rpc.get_protocol().await.unwrap(), outputs, vec![(address, amount - fee)], - Some(Self::test_address()), + Some(Change::new(&Self::test_view_pair(), true)), vec![], self.rpc.get_fee().await.unwrap(), )