Support including arbitrary data in TXs and return it with outputs

Fixes a bug where all payments identified as being to (0, 0) instead of 
their actual subaddress.

coins/monero/src/wallet/extra.rs

@@ -1,13 +1,13 @@
 use core::ops::BitXor;
-use std::io::{self, Read, Write};
+use std::io::{self, Read, Write, Cursor};
 
 use zeroize::Zeroize;
 
 use curve25519_dalek::edwards::EdwardsPoint;
 
 use crate::serialize::{
-  read_byte, read_bytes, read_varint, read_point, read_vec, write_byte, write_varint, write_point,
+  varint_len, read_byte, read_bytes, read_varint, read_point, read_vec, write_byte, write_varint,
-  write_vec,
+  write_point, write_vec,
 };
 
 #[derive(Clone, Copy, PartialEq, Eq, Debug, Zeroize)]
@@ -30,7 +30,7 @@ impl BitXor<[u8; 8]> for PaymentId {
 }
 
 impl PaymentId {
-  fn serialize<W: Write>(&self, w: &mut W) -> io::Result<()> {
+  pub(crate) fn serialize<W: Write>(&self, w: &mut W) -> io::Result<()> {
     match self {
       PaymentId::Unencrypted(id) => {
         w.write_all(&[0])?;
@@ -53,11 +53,11 @@ impl PaymentId {
   }
 }
 
+// Doesn't bother with padding nor MinerGate
 #[derive(Clone, PartialEq, Eq, Debug, Zeroize)]
 pub(crate) enum ExtraField {
-  Padding(Vec<u8>),
   PublicKey(EdwardsPoint),
-  PaymentId(PaymentId), // Technically Nonce, an arbitrary data field, yet solely used as PaymentId
+  Nonce(Vec<u8>),
   MergeMining(usize, [u8; 32]),
   PublicKeys(Vec<EdwardsPoint>),
 }
@@ -65,19 +65,13 @@ pub(crate) enum ExtraField {
 impl ExtraField {
   fn serialize<W: Write>(&self, w: &mut W) -> io::Result<()> {
     match self {
-      ExtraField::Padding(data) => {
-        w.write_all(&[0])?;
-        write_vec(write_byte, data, w)?;
-      }
       ExtraField::PublicKey(key) => {
         w.write_all(&[1])?;
         w.write_all(&key.compress().to_bytes())?;
       }
-      ExtraField::PaymentId(id) => {
+      ExtraField::Nonce(data) => {
         w.write_all(&[2])?;
-        let mut buf = Vec::with_capacity(1 + 8);
+        write_vec(write_byte, data, w)?;
-        id.serialize(&mut buf)?;
-        write_vec(write_byte, &buf, w)?;
       }
       ExtraField::MergeMining(height, merkle) => {
         w.write_all(&[3])?;
@@ -94,15 +88,14 @@ impl ExtraField {
 
   fn deserialize<R: Read>(r: &mut R) -> io::Result<ExtraField> {
     Ok(match read_byte(r)? {
-      0 => {
-        let res = read_vec(read_byte, r)?;
-        if res.len() > 255 {
-          Err(io::Error::new(io::ErrorKind::Other, "too long padding"))?;
-        }
-        ExtraField::Padding(res)
-      }
       1 => ExtraField::PublicKey(read_point(r)?),
-      2 => ExtraField::PaymentId(PaymentId::deserialize(r)?),
+      2 => ExtraField::Nonce({
+        let nonce = read_vec(read_byte, r)?;
+        if nonce.len() > 255 {
+          Err(io::Error::new(io::ErrorKind::Other, "too long nonce"))?;
+        }
+        nonce
+      }),
       3 => ExtraField::MergeMining(
         usize::try_from(read_varint(r)?)
           .map_err(|_| io::Error::new(io::ErrorKind::Other, "varint for height exceeds usize"))?,
@@ -131,16 +124,22 @@ impl Extra {
 
   pub(crate) fn payment_id(&self) -> Option<PaymentId> {
     for field in &self.0 {
-      if let ExtraField::PaymentId(id) = field {
+      if let ExtraField::Nonce(data) = field {
-        return Some(*id);
+        return PaymentId::deserialize(&mut Cursor::new(data)).ok();
       }
     }
     None
   }
 
   pub(crate) fn data(&self) -> Option<Vec<u8>> {
+    let mut first = true;
     for field in &self.0 {
-      if let ExtraField::Padding(data) = field {
+      if let ExtraField::Nonce(data) = field {
+        // Skip the first Nonce, which should be the payment ID
+        if first {
+          first = false;
+          continue;
+        }
         return Some(data.clone());
       }
     }
@@ -162,9 +161,16 @@ impl Extra {
     self.0.push(field);
   }
 
-  pub(crate) fn fee_weight(outputs: usize) -> usize {
+  #[rustfmt::skip]
-    // PublicKey, key, PublicKeys, length, additional keys, PaymentId, length, encrypted, ID
+  pub(crate) fn fee_weight(outputs: usize, data: Option<&Vec<u8>>) -> usize {
-    33 + 2 + (outputs.saturating_sub(1) * 32) + 11
+    // PublicKey, key
+    (1 + 32) +
+    // PublicKeys, length, additional keys
+    (1 + 1 + (outputs.saturating_sub(1) * 32)) +
+    // PaymentId (Nonce), length, encrypted, ID
+    (1 + 1 + 1 + 8) +
+    // Nonce, length, data (if existant)
+    data.map(|v| 1 + varint_len(v.len()) + v.len()).unwrap_or(0)
   }
 
   pub(crate) fn serialize<W: Write>(&self, w: &mut W) -> io::Result<()> {

coins/monero/src/wallet/scan.rs

@@ -6,7 +6,7 @@ use curve25519_dalek::{constants::ED25519_BASEPOINT_TABLE, scalar::Scalar, edwar
 
 use crate::{
   Commitment,
-  serialize::{read_byte, read_u32, read_u64, read_bytes, read_scalar, read_point},
+  serialize::{read_byte, read_u32, read_u64, read_bytes, read_scalar, read_point, read_raw_vec},
   transaction::{Timelock, Transaction},
   block::Block,
   rpc::{Rpc, RpcError},
@@ -69,19 +69,38 @@ pub struct Metadata {
   // 0xff was chosen as it'd be distinct from [0; 8], enabling atomically incrementing IDs (though
   // they should be randomly generated)
   pub payment_id: [u8; 8],
+  // Arbitrary data
+  pub arbitrary_data: Option<Vec<u8>>,
 }
 
 impl Metadata {
   pub fn serialize(&self) -> Vec<u8> {
-    let mut res = Vec::with_capacity(4 + 4 + 8);
+    let mut res = Vec::with_capacity(4 + 4 + 8 + 1);
     res.extend(self.subaddress.0.to_le_bytes());
     res.extend(self.subaddress.1.to_le_bytes());
     res.extend(self.payment_id);
+    if let Some(data) = self.arbitrary_data.as_ref() {
+      res.extend(&[1, u8::try_from(data.len()).unwrap()]);
+      res.extend(data);
+    } else {
+      res.extend(&[0]);
+    }
     res
   }
 
   pub fn deserialize<R: std::io::Read>(r: &mut R) -> std::io::Result<Metadata> {
-    Ok(Metadata { subaddress: (read_u32(r)?, read_u32(r)?), payment_id: read_bytes(r)? })
+    Ok(Metadata {
+      subaddress: (read_u32(r)?, read_u32(r)?),
+      payment_id: read_bytes(r)?,
+      arbitrary_data: {
+        if read_byte(r)? == 1 {
+          let len = read_byte(r)?;
+          Some(read_raw_vec(read_byte, usize::from(len), r)?)
+        } else {
+          None
+        }
+      },
+    })
   }
 }
 
@@ -251,13 +270,15 @@ impl Scanner {
         if subaddress.is_none() {
           continue;
         }
+        let subaddress = *subaddress.unwrap();
+
         // If it has torsion, it'll substract the non-torsioned shared key to a torsioned key
         // We will not have a torsioned key in our HashMap of keys, so we wouldn't identify it as
         // ours
         // If we did though, it'd enable bypassing the included burning bug protection
         debug_assert!(output_key.is_torsion_free());
 
-        let key_offset = shared_key + self.pair.subaddress(*subaddress.unwrap());
+        let key_offset = shared_key + self.pair.subaddress(subaddress);
         // Since we've found an output to us, get its amount
         let mut commitment = Commitment::zero();
 
@@ -288,7 +309,7 @@ impl Scanner {
 
             data: OutputData { key: output_key, key_offset, commitment },
 
-            metadata: Metadata { subaddress: (0, 0), payment_id },
+            metadata: Metadata { subaddress: subaddress, payment_id, arbitrary_data: extra.data() },
           });
 
           if let Some(burning_bug) = self.burning_bug.as_mut() {

coins/monero/src/wallet/send/mod.rs

@@ -88,6 +88,8 @@ pub enum TransactionError {
   NoChange,
   #[error("too many outputs")]
   TooManyOutputs,
+  #[error("too much data")]
+  TooMuchData,
   #[error("not enough funds (in {0}, out {1})")]
   NotEnoughFunds(u64, u64),
   #[error("wrong spend private key")]
@@ -171,6 +173,7 @@ pub struct SignableTransaction {
   protocol: Protocol,
   inputs: Vec<SpendableOutput>,
   payments: Vec<(Address, u64)>,
+  data: Option<Vec<u8>>,
   fee: u64,
 }
 
@@ -180,6 +183,7 @@ impl SignableTransaction {
     inputs: Vec<SpendableOutput>,
     mut payments: Vec<(Address, u64)>,
     change_address: Option<Address>,
+    data: Option<Vec<u8>>,
     fee_rate: Fee,
   ) -> Result<SignableTransaction, TransactionError> {
     // Make sure there's only one payment ID
@@ -208,6 +212,10 @@ impl SignableTransaction {
       Err(TransactionError::NoOutputs)?;
     }
 
+    if data.as_ref().map(|v| v.len()).unwrap_or(0) > 255 {
+      Err(TransactionError::TooMuchData)?;
+    }
+
     // TODO TX MAX SIZE
 
     // If we don't have two outputs, as required by Monero, add a second
@@ -218,7 +226,7 @@ impl SignableTransaction {
     let outputs = payments.len() + (if change { 1 } else { 0 });
 
     // Calculate the extra length
-    let extra = Extra::fee_weight(outputs);
+    let extra = Extra::fee_weight(outputs, data.as_ref());
 
     // Calculate the fee.
     let mut fee =
@@ -252,7 +260,7 @@ impl SignableTransaction {
       Err(TransactionError::TooManyOutputs)?;
     }
 
-    Ok(SignableTransaction { protocol, inputs, payments, fee })
+    Ok(SignableTransaction { protocol, inputs, payments, data, fee })
   }
 
   fn prepare_transaction<R: RngCore + CryptoRng>(
@@ -292,10 +300,16 @@ impl SignableTransaction {
     let extra = {
       let mut extra = Extra::new(outputs.iter().map(|output| output.R).collect());
 
-      // Additionally include a random payment ID
+      let mut id_vec = Vec::with_capacity(1 + 8);
-      extra.push(ExtraField::PaymentId(PaymentId::Encrypted(id)));
+      PaymentId::Encrypted(id).serialize(&mut id_vec).unwrap();
+      extra.push(ExtraField::Nonce(id_vec));
 
-      let mut serialized = Vec::with_capacity(Extra::fee_weight(outputs.len()));
+      // Include data if present
+      if let Some(data) = self.data.take() {
+        extra.push(ExtraField::Nonce(data));
+      }
+
+      let mut serialized = Vec::with_capacity(Extra::fee_weight(outputs.len(), self.data.as_ref()));
       extra.serialize(&mut serialized).unwrap();
       serialized
     };

coins/monero/tests/send.rs

@@ -134,6 +134,7 @@ async fn send_core(test: usize, multisig: bool) {
       outputs,
       vec![(addr, amount - 10000000000)],
       Some(addr),
+      None,
       fee,
     )
     .unwrap();

processor/src/coin/monero.rs

@@ -166,6 +166,7 @@ impl Coin for Monero {
         inputs.drain(..).map(|input| input.0).collect(),
         payments.to_vec(),
         Some(self.address(spend)),
+        None,
         fee,
       )
       .map_err(|_| CoinError::ConnectionError)?,
@@ -245,6 +246,7 @@ impl Coin for Monero {
       outputs,
       vec![(address, amount - fee)],
       Some(Self::empty_address()),
+      None,
       self.rpc.get_fee().await.unwrap(),
     )
     .unwrap()