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

@@ -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<W: 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<Vec<u8>> {
-    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 data[0] == ARBITRARY_DATA_MARKER {
-        if first {
+          res.push(data[1 ..].to_vec());
-          first = false;
-          continue;
         }
-        res.push(data.clone());
       }
     }
     res
@@ -167,13 +171,13 @@ impl Extra {
   }
 
   #[rustfmt::skip]
-  pub(crate) fn fee_weight(outputs: usize, data: &[Vec<u8>]) -> usize {
+  pub(crate) fn fee_weight(outputs: usize, payment_id: bool, data: &[Vec<u8>]) -> 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::<usize>()
   }

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<u8>) -> Result<Self, TransactionError> {
-    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);

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,
+    address::MoneroAddress,
-    uniqueness, shared_key, commitment_mask, amount_encryption, extra::MAX_TX_EXTRA_NONCE_SIZE,
+    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<SignableTransaction, TransactionError> {
     // 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
     };

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
+      // Make a data that is bigger than the maximum
-      let mut data = vec![b'a'; MAX_TX_EXTRA_NONCE_SIZE + 1];
+      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());
 

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());
       }