From e56495d624d445177e32540aefc9302fb0e602b5 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sat, 11 Mar 2023 05:47:25 -0500 Subject: [PATCH] Prefix arbitrary data with 127 Since we cannot expect/guarantee a payment ID will be included, the previous position-based code for determining arbitrary data wasn't sufficient. --- coins/monero/src/wallet/extra.rs | 24 ++++++++++++--------- coins/monero/src/wallet/send/builder.rs | 4 ++-- coins/monero/src/wallet/send/mod.rs | 24 ++++++++++++++------- coins/monero/tests/add_data.rs | 13 +++++------ coins/monero/tests/wallet2_compatibility.rs | 5 +++-- 5 files changed, 42 insertions(+), 28 deletions(-) diff --git a/coins/monero/src/wallet/extra.rs b/coins/monero/src/wallet/extra.rs index 867c5189..b8b5f3c1 100644 --- a/coins/monero/src/wallet/extra.rs +++ b/coins/monero/src/wallet/extra.rs @@ -12,6 +12,14 @@ use crate::serialize::{ pub const MAX_TX_EXTRA_NONCE_SIZE: usize = 255; +pub const PAYMENT_ID_MARKER: u8 = 0; +pub const ENCRYPTED_PAYMENT_ID_MARKER: u8 = 1; +// Used as it's the highest value not interpretable as a continued VarInt +pub const ARBITRARY_DATA_MARKER: u8 = 127; + +// 1 byte is used for the marker +pub const MAX_ARBITRARY_DATA_SIZE: usize = MAX_TX_EXTRA_NONCE_SIZE - 1; + #[derive(Clone, Copy, PartialEq, Eq, Debug, Zeroize)] pub(crate) enum PaymentId { Unencrypted([u8; 32]), @@ -35,11 +43,11 @@ impl PaymentId { pub(crate) fn write(&self, w: &mut W) -> io::Result<()> { match self { PaymentId::Unencrypted(id) => { - w.write_all(&[0])?; + w.write_all(&[PAYMENT_ID_MARKER])?; w.write_all(id)?; } PaymentId::Encrypted(id) => { - w.write_all(&[1])?; + w.write_all(&[ENCRYPTED_PAYMENT_ID_MARKER])?; w.write_all(id)?; } } @@ -138,16 +146,12 @@ impl Extra { } pub(crate) fn data(&self) -> Vec> { - let mut first = true; let mut res = vec![]; for field in &self.0 { if let ExtraField::Nonce(data) = field { - // Skip the first Nonce, which should be the payment ID - if first { - first = false; - continue; + if data[0] == ARBITRARY_DATA_MARKER { + res.push(data[1 ..].to_vec()); } - res.push(data.clone()); } } res @@ -167,13 +171,13 @@ impl Extra { } #[rustfmt::skip] - pub(crate) fn fee_weight(outputs: usize, data: &[Vec]) -> usize { + pub(crate) fn fee_weight(outputs: usize, payment_id: bool, data: &[Vec]) -> usize { // PublicKey, key (1 + 32) + // PublicKeys, length, additional keys (1 + 1 + (outputs.saturating_sub(1) * 32)) + // PaymentId (Nonce), length, encrypted, ID - (1 + 1 + 1 + 8) + + (if payment_id { 1 + 1 + 1 + 8 } else { 0 }) + // Nonce, length, data (if existent) data.iter().map(|v| 1 + varint_len(v.len()) + v.len()).sum::() } diff --git a/coins/monero/src/wallet/send/builder.rs b/coins/monero/src/wallet/send/builder.rs index b137dcbf..ef70900a 100644 --- a/coins/monero/src/wallet/send/builder.rs +++ b/coins/monero/src/wallet/send/builder.rs @@ -6,7 +6,7 @@ use crate::{ Protocol, wallet::{ address::MoneroAddress, Fee, SpendableOutput, SignableTransaction, TransactionError, - extra::MAX_TX_EXTRA_NONCE_SIZE, + extra::MAX_ARBITRARY_DATA_SIZE, }, }; @@ -104,7 +104,7 @@ impl SignableTransactionBuilder { } pub fn add_data(&mut self, data: Vec) -> Result { - if data.len() > MAX_TX_EXTRA_NONCE_SIZE { + if data.len() > MAX_ARBITRARY_DATA_SIZE { Err(TransactionError::TooMuchData)?; } self.0.write().unwrap().add_data(data); diff --git a/coins/monero/src/wallet/send/mod.rs b/coins/monero/src/wallet/send/mod.rs index 3090af8a..aac69ee9 100644 --- a/coins/monero/src/wallet/send/mod.rs +++ b/coins/monero/src/wallet/send/mod.rs @@ -25,8 +25,10 @@ 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, extra::MAX_TX_EXTRA_NONCE_SIZE, + address::MoneroAddress, + SpendableOutput, Decoys, PaymentId, ExtraField, Extra, key_image_sort, uniqueness, shared_key, + commitment_mask, amount_encryption, + extra::{ARBITRARY_DATA_MARKER, MAX_ARBITRARY_DATA_SIZE}, }, }; @@ -196,7 +198,7 @@ impl SignableTransaction { fee_rate: Fee, ) -> Result { // Make sure there's only one payment ID - { + let mut has_payment_id = { let mut payment_ids = 0; let mut count = |addr: MoneroAddress| { if addr.payment_id().is_some() { @@ -212,7 +214,8 @@ impl SignableTransaction { if payment_ids > 1 { Err(TransactionError::MultiplePaymentIds)?; } - } + payment_ids == 1 + }; if inputs.is_empty() { Err(TransactionError::NoInputs)?; @@ -222,7 +225,7 @@ impl SignableTransaction { } for part in &data { - if part.len() > MAX_TX_EXTRA_NONCE_SIZE { + if part.len() > MAX_ARBITRARY_DATA_SIZE { Err(TransactionError::TooMuchData)?; } } @@ -235,9 +238,11 @@ impl SignableTransaction { Err(TransactionError::NoChange)?; } let outputs = payments.len() + usize::from(change); + // Add a dummy payment ID if there's only 2 payments + has_payment_id |= outputs == 2; // Calculate the extra length - let extra = Extra::fee_weight(outputs, data.as_ref()); + let extra = Extra::fee_weight(outputs, has_payment_id, data.as_ref()); // Calculate the fee. let mut fee = @@ -337,10 +342,13 @@ impl SignableTransaction { // Include data if present for part in self.data.drain(..) { - extra.push(ExtraField::Nonce(part)); + let mut arb = vec![ARBITRARY_DATA_MARKER]; + arb.extend(part); + extra.push(ExtraField::Nonce(arb)); } - let mut serialized = Vec::with_capacity(Extra::fee_weight(outputs.len(), self.data.as_ref())); + let mut serialized = + Vec::with_capacity(Extra::fee_weight(outputs.len(), id.is_some(), self.data.as_ref())); extra.write(&mut serialized).unwrap(); serialized }; diff --git a/coins/monero/tests/add_data.rs b/coins/monero/tests/add_data.rs index 918a51af..edb550ab 100644 --- a/coins/monero/tests/add_data.rs +++ b/coins/monero/tests/add_data.rs @@ -1,5 +1,5 @@ use monero_serai::{ - wallet::{TransactionError, extra::MAX_TX_EXTRA_NONCE_SIZE}, + wallet::{TransactionError, extra::MAX_ARBITRARY_DATA_SIZE}, transaction::Transaction, }; @@ -9,7 +9,7 @@ test!( add_single_data_less_than_max, ( |_, mut builder: Builder, addr| async move { - let arbitrary_data = vec![b'\0', (MAX_TX_EXTRA_NONCE_SIZE as u8) - 1]; + let arbitrary_data = vec![b'\0'; MAX_ARBITRARY_DATA_SIZE - 1]; // make sure we can add to tx let result = builder.add_data(arbitrary_data.clone()); @@ -30,7 +30,7 @@ test!( add_multiple_data_less_than_max, ( |_, mut builder: Builder, addr| async move { - let data = vec![b'\0', (MAX_TX_EXTRA_NONCE_SIZE as u8) - 1]; + let data = vec![b'\0'; MAX_ARBITRARY_DATA_SIZE - 1]; // Add tx multiple times for _ in 0 .. 5 { @@ -53,13 +53,14 @@ test!( add_single_data_more_than_max, ( |_, mut builder: Builder, addr| async move { - // Make a data that is bigger than 255 bytes - let mut data = vec![b'a'; MAX_TX_EXTRA_NONCE_SIZE + 1]; + // Make a data that is bigger than the maximum + let mut data = vec![b'a'; MAX_ARBITRARY_DATA_SIZE + 1]; // Make sure we get an error if we try to add it to the TX assert_eq!(builder.add_data(data.clone()), Err(TransactionError::TooMuchData)); - // Reduce data size and retry. The data will now be 255 bytes long, exactly + // Reduce data size and retry. The data will now be 255 bytes long (including the added + // marker), exactly data.pop(); assert!(builder.add_data(data.clone()).is_ok()); diff --git a/coins/monero/tests/wallet2_compatibility.rs b/coins/monero/tests/wallet2_compatibility.rs index 4e6586aa..1b028435 100644 --- a/coins/monero/tests/wallet2_compatibility.rs +++ b/coins/monero/tests/wallet2_compatibility.rs @@ -217,9 +217,10 @@ test!( 1000000, ); - // Make 2 data that is full 255 bytes + // Make 2 data that is the full 255 bytes for _ in 0 .. 2 { - let data = vec![b'a'; MAX_TX_EXTRA_NONCE_SIZE]; + // Subtract 1 since we prefix data with 127 + let data = vec![b'a'; MAX_TX_EXTRA_NONCE_SIZE - 1]; assert!(builder.add_data(data).is_ok()); }