Document v2 TX/RCT output relation assumed when scanning

coins/monero/primitives/src/lib.rs

@@ -107,6 +107,33 @@ impl Commitment {
   pub fn calculate(&self) -> EdwardsPoint {
     EdwardsPoint::vartime_double_scalar_mul_basepoint(&Scalar::from(self.amount), &H(), &self.mask)
   }
+
+  /// Write the Commitment.
+  ///
+  /// This is not a Monero protocol defined struct, and this is accordingly not a Monero protocol
+  /// defined serialization.
+  pub fn write<W: io::Write>(&self, w: &mut W) -> io::Result<()> {
+    w.write_all(&self.mask.to_bytes())?;
+    w.write_all(&self.amount.to_le_bytes())
+  }
+
+  /// Serialize the Commitment to a `Vec<u8>`.
+  ///
+  /// This is not a Monero protocol defined struct, and this is accordingly not a Monero protocol
+  /// defined serialization.
+  pub fn serialize(&self) -> Vec<u8> {
+    let mut res = Vec::with_capacity(32 + 8);
+    self.write(&mut res).unwrap();
+    res
+  }
+
+  /// Read a Commitment.
+  ///
+  /// This is not a Monero protocol defined struct, and this is accordingly not a Monero protocol
+  /// defined serialization.
+  pub fn read<R: io::Read>(r: &mut R) -> io::Result<Commitment> {
+    Ok(Commitment::new(read_scalar(r)?, read_u64(r)?))
+  }
 }
 
 /// Decoy data, as used for producing Monero's ring signatures.

coins/monero/wallet/src/output.rs

@@ -119,9 +119,7 @@ impl OutputData {
   fn write<W: Write>(&self, w: &mut W) -> io::Result<()> {
     w.write_all(&self.key.compress().to_bytes())?;
     w.write_all(&self.key_offset.to_bytes())?;
-    // TODO: Commitment::write?
-    w.write_all(&self.commitment.mask.to_bytes())?;
-    w.write_all(&self.commitment.amount.to_le_bytes())?;
+    self.commitment.write(w)?;
     self.additional_timelock.write(w)
   }
 
@@ -133,7 +131,7 @@ impl OutputData {
     Ok(OutputData {
       key: read_point(r)?,
       key_offset: read_scalar(r)?,
-      commitment: Commitment::new(read_scalar(r)?, read_u64(r)?),
+      commitment: Commitment::read(r)?,
       additional_timelock: Timelock::read(r)?,
     })
   }

coins/monero/wallet/src/scan.rs

@@ -9,7 +9,7 @@ use monero_rpc::{RpcError, Rpc};
 use monero_serai::{
   io::*,
   primitives::Commitment,
-  transaction::{Input, Timelock, Transaction},
+  transaction::{Timelock, Transaction},
   block::Block,
 };
 use crate::{
@@ -111,7 +111,8 @@ impl InternalScanner {
     tx_start_index_on_blockchain: u64,
     tx: &Transaction,
   ) -> Result<Timelocked, RpcError> {
-    // Only scan RCT TXs since we can only spend RCT outputs
+    // Only scan TXs creating RingCT outputs
+    // For the full details on why this check is equivalent, please see the documentation in `scan`
     if tx.version() != 2 {
       return Ok(Timelocked(vec![]));
     }
@@ -254,18 +255,72 @@ impl InternalScanner {
 
     let block_hash = block.hash();
 
-    // We get the output indexes for the miner transaction as a reference point
-    // TODO: Are miner transactions since v2 guaranteed to have an output?
-    let mut tx_start_index_on_blockchain = *rpc
-      .get_o_indexes(block.miner_transaction.hash())
-      .await?
-      .first()
-      .ok_or(RpcError::InvalidNode("miner transaction without outputs".to_string()))?;
-
     // We obtain all TXs in full
     let mut txs = vec![block.miner_transaction.clone()];
     txs.extend(rpc.get_transactions(&block.transactions).await?);
 
+    /*
+      Requesting the output index for each output we sucessfully scan would cause a loss of privacy
+      We could instead request the output indexes for all outputs we scan, yet this would notably
+      increase the amount of RPC calls we make.
+
+      We solve this by requesting the output index for the first RingCT output in the block, which
+      should be within the miner transaction. Then, as we scan transactions, we update the output
+      index ourselves.
+
+      Please note we only will scan RingCT outputs so we only need to track the RingCT output
+      index. This decision was made due to spending CN outputs potentially having burdensome
+      requirements (the need to make a v1 TX due to insufficient decoys).
+
+      We bound ourselves to only scanning RingCT outputs by only scanning v2 transactions. This is
+      safe and correct since:
+
+      1) v1 transactions cannot create RingCT outputs.
+
+         https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c454
+           /src/cryptonote_basic/cryptonote_format_utils.cpp#L866-L869
+
+      2) v2 miner transactions implicitly create RingCT outputs.
+
+         https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c454
+           /src/blockchain_db/blockchain_db.cpp#L232-L241
+
+      3) v2 transactions must create RingCT outputs.
+
+         https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c45
+           /src/cryptonote_core/blockchain.cpp#L3055-L3065
+
+         That does bound on the hard fork version being >= 3, yet all v2 TXs have a hard fork
+         version > 3.
+
+         https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c454
+           /src/cryptonote_core/blockchain.cpp#L3417
+    */
+
+    // Get the starting index
+    let mut tx_start_index_on_blockchain = {
+      let mut tx_start_index_on_blockchain = None;
+      for tx in &txs {
+        // If this isn't a RingCT output, or there are no outputs, move to the next TX
+        if (!matches!(tx, Transaction::V2 { .. })) || tx.prefix().outputs.is_empty() {
+          continue;
+        }
+
+        let index = *rpc.get_o_indexes(tx.hash()).await?.first().ok_or_else(|| {
+          RpcError::InvalidNode(
+            "requested output indexes for a TX with outputs and got none".to_string(),
+          )
+        })?;
+        tx_start_index_on_blockchain = Some(index);
+        break;
+      }
+      let Some(tx_start_index_on_blockchain) = tx_start_index_on_blockchain else {
+        // Block had no RingCT outputs
+        return Ok(Timelocked(vec![]));
+      };
+      tx_start_index_on_blockchain
+    };
+
     let mut res = Timelocked(vec![]);
     for tx in txs {
       // Push all outputs into our result
@@ -278,20 +333,10 @@ impl InternalScanner {
         res.0.extend(this_txs_outputs);
       }
 
-      // Update the TX start index for the next TX
-      tx_start_index_on_blockchain += u64::try_from(
-        tx.prefix()
-          .outputs
-          .iter()
-          // Filter to v2 miner TX outputs/RCT outputs since we're tracking the RCT output index
-          .filter(|output| {
-            let is_v2_miner_tx =
-              (tx.version() == 2) && matches!(tx.prefix().inputs.first(), Some(Input::Gen(..)));
-            is_v2_miner_tx || output.amount.is_none()
-          })
-          .count(),
-      )
-      .unwrap()
+      // Update the RingCT starting index for the next TX
+      if matches!(tx, Transaction::V2 { .. }) {
+        tx_start_index_on_blockchain += u64::try_from(tx.prefix().outputs.len()).unwrap()
+      }
     }
 
     // If the block's version is >= 12, drop all unencrypted payment IDs