Add additional documentation

Enables using a locally backed decoy DB.

coins/monero/rpc/simple-request/tests/tests.rs

@@ -3,7 +3,6 @@ use monero_address::{Network, MoneroAddress};
 // monero-rpc doesn't include a transport
 // We can't include the simple-request crate there as then we'd have a cyclical dependency
 // Accordingly, we test monero-rpc here (implicitly testing the simple-request transport)
-use monero_rpc::*;
 use monero_simple_request_rpc::*;
 
 const ADDRESS: &str =
@@ -11,6 +10,8 @@ const ADDRESS: &str =
 
 #[tokio::test]
 async fn test_rpc() {
+  use monero_rpc::Rpc;
+
   let rpc =
     SimpleRequestRpc::new("http://serai:seraidex@127.0.0.1:18081".to_string()).await.unwrap();
 
@@ -52,17 +53,35 @@ async fn test_rpc() {
     }
     assert_eq!(blocks, actual_blocks);
   }
+}
+
+#[tokio::test]
+async fn test_decoy_rpc() {
+  use monero_rpc::{Rpc, DecoyRpc};
+
+  let rpc =
+    SimpleRequestRpc::new("http://serai:seraidex@127.0.0.1:18081".to_string()).await.unwrap();
 
   // Test get_output_distribution
   // It's documented to take two inclusive block numbers
   {
-    let height = rpc.get_height().await.unwrap();
+    let distribution_len = rpc.get_output_distribution_len().await.unwrap();
+    assert_eq!(distribution_len, rpc.get_height().await.unwrap());
 
-    rpc.get_output_distribution(0 ..= height).await.unwrap_err();
-    assert_eq!(rpc.get_output_distribution(0 .. height).await.unwrap().len(), height);
+    rpc.get_output_distribution(0 ..= distribution_len).await.unwrap_err();
+    assert_eq!(
+      rpc.get_output_distribution(0 .. distribution_len).await.unwrap().len(),
+      distribution_len
+    );
 
-    assert_eq!(rpc.get_output_distribution(0 .. (height - 1)).await.unwrap().len(), height - 1);
-    assert_eq!(rpc.get_output_distribution(1 .. height).await.unwrap().len(), height - 1);
+    assert_eq!(
+      rpc.get_output_distribution(0 .. (distribution_len - 1)).await.unwrap().len(),
+      distribution_len - 1
+    );
+    assert_eq!(
+      rpc.get_output_distribution(1 .. distribution_len).await.unwrap().len(),
+      distribution_len - 1
+    );
 
     assert_eq!(rpc.get_output_distribution(0 ..= 0).await.unwrap().len(), 1);
     assert_eq!(rpc.get_output_distribution(0 ..= 1).await.unwrap().len(), 2);

coins/monero/rpc/src/lib.rs

@@ -492,6 +492,129 @@ pub trait Rpc: Sync + Clone + Debug {
     Ok(res)
   }
 
+  /// Get the currently estimated fee rate from the node.
+  ///
+  /// This may be manipulated to unsafe levels and MUST be sanity checked.
+  ///
+  /// This MUST NOT be expected to be deterministic in any way.
+  // TODO: Take a sanity check argument
+  async fn get_fee_rate(&self, priority: FeePriority) -> Result<FeeRate, RpcError> {
+    #[derive(Deserialize, Debug)]
+    struct FeeResponse {
+      status: String,
+      fees: Option<Vec<u64>>,
+      fee: u64,
+      quantization_mask: u64,
+    }
+
+    let res: FeeResponse = self
+      .json_rpc_call(
+        "get_fee_estimate",
+        Some(json!({ "grace_blocks": GRACE_BLOCKS_FOR_FEE_ESTIMATE })),
+      )
+      .await?;
+
+    if res.status != "OK" {
+      Err(RpcError::InvalidFee)?;
+    }
+
+    if let Some(fees) = res.fees {
+      // https://github.com/monero-project/monero/blob/94e67bf96bbc010241f29ada6abc89f49a81759c/
+      // src/wallet/wallet2.cpp#L7615-L7620
+      let priority_idx = usize::try_from(if priority.fee_priority() >= 4 {
+        3
+      } else {
+        priority.fee_priority().saturating_sub(1)
+      })
+      .map_err(|_| RpcError::InvalidPriority)?;
+
+      if priority_idx >= fees.len() {
+        Err(RpcError::InvalidPriority)
+      } else {
+        FeeRate::new(fees[priority_idx], res.quantization_mask)
+      }
+    } else {
+      // https://github.com/monero-project/monero/blob/94e67bf96bbc010241f29ada6abc89f49a81759c/
+      //   src/wallet/wallet2.cpp#L7569-L7584
+      // https://github.com/monero-project/monero/blob/94e67bf96bbc010241f29ada6abc89f49a81759c/
+      //   src/wallet/wallet2.cpp#L7660-L7661
+      let priority_idx =
+        usize::try_from(if priority.fee_priority() == 0 { 1 } else { priority.fee_priority() - 1 })
+          .map_err(|_| RpcError::InvalidPriority)?;
+      let multipliers = [1, 5, 25, 1000];
+      if priority_idx >= multipliers.len() {
+        // though not an RPC error, it seems sensible to treat as such
+        Err(RpcError::InvalidPriority)?;
+      }
+      let fee_multiplier = multipliers[priority_idx];
+
+      FeeRate::new(res.fee * fee_multiplier, res.quantization_mask)
+    }
+  }
+
+  /// Publish a transaction.
+  async fn publish_transaction(&self, tx: &Transaction) -> Result<(), RpcError> {
+    #[allow(dead_code)]
+    #[derive(Deserialize, Debug)]
+    struct SendRawResponse {
+      status: String,
+      double_spend: bool,
+      fee_too_low: bool,
+      invalid_input: bool,
+      invalid_output: bool,
+      low_mixin: bool,
+      not_relayed: bool,
+      overspend: bool,
+      too_big: bool,
+      too_few_outputs: bool,
+      reason: String,
+    }
+
+    let res: SendRawResponse = self
+      .rpc_call(
+        "send_raw_transaction",
+        Some(json!({ "tx_as_hex": hex::encode(tx.serialize()), "do_sanity_checks": false })),
+      )
+      .await?;
+
+    if res.status != "OK" {
+      Err(RpcError::InvalidTransaction(tx.hash()))?;
+    }
+
+    Ok(())
+  }
+
+  /// Generate blocks, with the specified address receiving the block reward.
+  ///
+  /// Returns the hashes of the generated blocks and the last block's number.
+  async fn generate_blocks<const ADDR_BYTES: u128>(
+    &self,
+    address: &Address<ADDR_BYTES>,
+    block_count: usize,
+  ) -> Result<(Vec<[u8; 32]>, usize), RpcError> {
+    #[derive(Debug, Deserialize)]
+    struct BlocksResponse {
+      blocks: Vec<String>,
+      height: usize,
+    }
+
+    let res = self
+      .json_rpc_call::<BlocksResponse>(
+        "generateblocks",
+        Some(json!({
+          "wallet_address": address.to_string(),
+          "amount_of_blocks": block_count
+        })),
+      )
+      .await?;
+
+    let mut blocks = Vec::with_capacity(res.blocks.len());
+    for block in res.blocks {
+      blocks.push(hash_hex(&block)?);
+    }
+    Ok((blocks, res.height))
+  }
+
   /// Get the output indexes of the specified transaction.
   async fn get_o_indexes(&self, hash: [u8; 32]) -> Result<Vec<u64>, RpcError> {
     // Given the immaturity of Rust epee libraries, this is a homegrown one which is only validated
@@ -673,10 +796,57 @@ pub trait Rpc: Sync + Clone + Debug {
     })()
     .map_err(|e| RpcError::InvalidNode(format!("invalid binary response: {e:?}")))
   }
+}
+
+/// A trait for any object which can be used to select decoys.
+///
+/// An implementation is provided for any satisfier of `Rpc`. The benefit of this trait is the
+/// ability to select decoys off of a locally stored output distribution, preventing potential
+/// attacks a remote node can perform.
+#[async_trait]
+pub trait DecoyRpc: Sync + Clone + Debug {
+  /// Get the length of the output distribution.
+  ///
+  /// This is equivalent to the hight of the blockchain it's for. This is intended to be cheaper
+  /// than fetching the entire output distribution.
+  async fn get_output_distribution_len(&self) -> Result<usize, RpcError>;
 
   /// Get the output distribution.
   ///
   /// `range` is in terms of block numbers.
+  async fn get_output_distribution(
+    &self,
+    range: impl Send + RangeBounds<usize>,
+  ) -> Result<Vec<u64>, RpcError>;
+
+  /// Get the specified outputs from the RingCT (zero-amount) pool.
+  async fn get_outs(&self, indexes: &[u64]) -> Result<Vec<OutputResponse>, RpcError>;
+
+  /// Get the specified outputs from the RingCT (zero-amount) pool, but only return them if their
+  /// timelock has been satisfied.
+  ///
+  /// The timelock being satisfied is distinct from being free of the 10-block lock applied to all
+  /// Monero transactions.
+  ///
+  /// The node is trusted for if the output is unlocked unless `fingerprintable_canonical` is set
+  /// to true. If `fingerprintable_canonical` is set to true, the node's local view isn't used, yet
+  /// the transaction's timelock is checked to be unlocked at the specified `height`. This offers a
+  /// canonical decoy selection, yet is fingerprintable as time-based timelocks aren't evaluated
+  /// (and considered locked, preventing their selection).
+  async fn get_unlocked_outputs(
+    &self,
+    indexes: &[u64],
+    height: usize,
+    fingerprintable_canonical: bool,
+  ) -> Result<Vec<Option<[EdwardsPoint; 2]>>, RpcError>;
+}
+
+#[async_trait]
+impl<R: Rpc> DecoyRpc for R {
+  async fn get_output_distribution_len(&self) -> Result<usize, RpcError> {
+    <Self as Rpc>::get_height(self).await
+  }
+
   async fn get_output_distribution(
     &self,
     range: impl Send + RangeBounds<usize>,
@@ -743,7 +913,6 @@ pub trait Rpc: Sync + Clone + Debug {
     Ok(distribution)
   }
 
-  /// Get the specified outputs from the RingCT (zero-amount) pool.
   async fn get_outs(&self, indexes: &[u64]) -> Result<Vec<OutputResponse>, RpcError> {
     #[derive(Deserialize, Debug)]
     struct OutsResponse {
@@ -771,17 +940,6 @@ pub trait Rpc: Sync + Clone + Debug {
     Ok(res.outs)
   }
 
-  /// Get the specified outputs from the RingCT (zero-amount) pool, but only return them if their
-  /// timelock has been satisfied.
-  ///
-  /// The timelock being satisfied is distinct from being free of the 10-block lock applied to all
-  /// Monero transactions.
-  ///
-  /// The node is trusted for if the output is unlocked unless `fingerprintable_canonical` is set
-  /// to true. If `fingerprintable_canonical` is set to true, the node's local view isn't used, yet
-  /// the transaction's timelock is checked to be unlocked at the specified `height`. This offers a
-  /// canonical decoy selection, yet is fingerprintable as time-based timelocks aren't evaluated
-  /// (and considered locked, preventing their selection).
   async fn get_unlocked_outputs(
     &self,
     indexes: &[u64],
@@ -830,127 +988,4 @@ pub trait Rpc: Sync + Clone + Debug {
       })
       .collect()
   }
-
-  /// Get the currently estimated fee rate from the node.
-  ///
-  /// This may be manipulated to unsafe levels and MUST be sanity checked.
-  ///
-  /// This MUST NOT be expected to be deterministic in any way.
-  // TODO: Take a sanity check argument
-  async fn get_fee_rate(&self, priority: FeePriority) -> Result<FeeRate, RpcError> {
-    #[derive(Deserialize, Debug)]
-    struct FeeResponse {
-      status: String,
-      fees: Option<Vec<u64>>,
-      fee: u64,
-      quantization_mask: u64,
-    }
-
-    let res: FeeResponse = self
-      .json_rpc_call(
-        "get_fee_estimate",
-        Some(json!({ "grace_blocks": GRACE_BLOCKS_FOR_FEE_ESTIMATE })),
-      )
-      .await?;
-
-    if res.status != "OK" {
-      Err(RpcError::InvalidFee)?;
-    }
-
-    if let Some(fees) = res.fees {
-      // https://github.com/monero-project/monero/blob/94e67bf96bbc010241f29ada6abc89f49a81759c/
-      // src/wallet/wallet2.cpp#L7615-L7620
-      let priority_idx = usize::try_from(if priority.fee_priority() >= 4 {
-        3
-      } else {
-        priority.fee_priority().saturating_sub(1)
-      })
-      .map_err(|_| RpcError::InvalidPriority)?;
-
-      if priority_idx >= fees.len() {
-        Err(RpcError::InvalidPriority)
-      } else {
-        FeeRate::new(fees[priority_idx], res.quantization_mask)
-      }
-    } else {
-      // https://github.com/monero-project/monero/blob/94e67bf96bbc010241f29ada6abc89f49a81759c/
-      //   src/wallet/wallet2.cpp#L7569-L7584
-      // https://github.com/monero-project/monero/blob/94e67bf96bbc010241f29ada6abc89f49a81759c/
-      //   src/wallet/wallet2.cpp#L7660-L7661
-      let priority_idx =
-        usize::try_from(if priority.fee_priority() == 0 { 1 } else { priority.fee_priority() - 1 })
-          .map_err(|_| RpcError::InvalidPriority)?;
-      let multipliers = [1, 5, 25, 1000];
-      if priority_idx >= multipliers.len() {
-        // though not an RPC error, it seems sensible to treat as such
-        Err(RpcError::InvalidPriority)?;
-      }
-      let fee_multiplier = multipliers[priority_idx];
-
-      FeeRate::new(res.fee * fee_multiplier, res.quantization_mask)
-    }
-  }
-
-  /// Publish a transaction.
-  async fn publish_transaction(&self, tx: &Transaction) -> Result<(), RpcError> {
-    #[allow(dead_code)]
-    #[derive(Deserialize, Debug)]
-    struct SendRawResponse {
-      status: String,
-      double_spend: bool,
-      fee_too_low: bool,
-      invalid_input: bool,
-      invalid_output: bool,
-      low_mixin: bool,
-      not_relayed: bool,
-      overspend: bool,
-      too_big: bool,
-      too_few_outputs: bool,
-      reason: String,
-    }
-
-    let res: SendRawResponse = self
-      .rpc_call(
-        "send_raw_transaction",
-        Some(json!({ "tx_as_hex": hex::encode(tx.serialize()), "do_sanity_checks": false })),
-      )
-      .await?;
-
-    if res.status != "OK" {
-      Err(RpcError::InvalidTransaction(tx.hash()))?;
-    }
-
-    Ok(())
-  }
-
-  /// Generate blocks, with the specified address receiving the block reward.
-  ///
-  /// Returns the hashes of the generated blocks and the last block's number.
-  async fn generate_blocks<const ADDR_BYTES: u128>(
-    &self,
-    address: &Address<ADDR_BYTES>,
-    block_count: usize,
-  ) -> Result<(Vec<[u8; 32]>, usize), RpcError> {
-    #[derive(Debug, Deserialize)]
-    struct BlocksResponse {
-      blocks: Vec<String>,
-      height: usize,
-    }
-
-    let res = self
-      .json_rpc_call::<BlocksResponse>(
-        "generateblocks",
-        Some(json!({
-          "wallet_address": address.to_string(),
-          "amount_of_blocks": block_count
-        })),
-      )
-      .await?;
-
-    let mut blocks = Vec::with_capacity(res.blocks.len());
-    for block in res.blocks {
-      blocks.push(hash_hex(&block)?);
-    }
-    Ok((blocks, res.height))
-  }
 }

coins/monero/wallet/src/decoys.rs

@@ -12,7 +12,7 @@ use curve25519_dalek::{Scalar, EdwardsPoint};
 use crate::{
   DEFAULT_LOCK_WINDOW, COINBASE_LOCK_WINDOW, BLOCK_TIME,
   primitives::{Commitment, Decoys},
-  rpc::{RpcError, Rpc},
+  rpc::{RpcError, DecoyRpc},
   output::OutputData,
   WalletOutput,
 };
@@ -24,7 +24,7 @@ const TIP_APPLICATION: f64 = (DEFAULT_LOCK_WINDOW * BLOCK_TIME) as f64;
 
 async fn select_n(
   rng: &mut (impl RngCore + CryptoRng),
-  rpc: &impl Rpc,
+  rpc: &impl DecoyRpc,
   height: usize,
   real_output: u64,
   ring_len: usize,
@@ -33,7 +33,7 @@ async fn select_n(
   if height < DEFAULT_LOCK_WINDOW {
     Err(RpcError::InternalError("not enough blocks to select decoys".to_string()))?;
   }
-  if height > rpc.get_height().await? {
+  if height > rpc.get_output_distribution_len().await? {
     Err(RpcError::InternalError(
       "decoys being requested from blocks this node doesn't have".to_string(),
     ))?;
@@ -94,7 +94,8 @@ async fn select_n(
     let mut candidates = Vec::with_capacity(remaining);
     while candidates.len() != remaining {
       // Use a gamma distribution, as Monero does
-      // TODO: Cite these constants
+      // https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c45
+      //   /src/wallet/wallet2.cpp#L142-L143
       let mut age = Gamma::<f64>::new(19.28, 1.0 / 1.61).unwrap().sample(rng).exp();
       #[allow(clippy::cast_precision_loss)]
       if age > TIP_APPLICATION {
@@ -164,7 +165,7 @@ async fn select_n(
 
 async fn select_decoys<R: RngCore + CryptoRng>(
   rng: &mut R,
-  rpc: &impl Rpc,
+  rpc: &impl DecoyRpc,
   ring_len: usize,
   height: usize,
   input: &WalletOutput,
@@ -230,7 +231,7 @@ impl OutputWithDecoys {
   /// Select decoys for this output.
   pub async fn new(
     rng: &mut (impl Send + Sync + RngCore + CryptoRng),
-    rpc: &impl Rpc,
+    rpc: &impl DecoyRpc,
     ring_len: usize,
     height: usize,
     output: WalletOutput,
@@ -249,7 +250,7 @@ impl OutputWithDecoys {
   /// methodology.
   pub async fn fingerprintable_deterministic_new(
     rng: &mut (impl Send + Sync + RngCore + CryptoRng),
-    rpc: &impl Rpc,
+    rpc: &impl DecoyRpc,
     ring_len: usize,
     height: usize,
     output: WalletOutput,

coins/monero/wallet/src/extra.rs

@@ -226,7 +226,9 @@ impl Extra {
   }
 
   /// The payment ID embedded within this extra.
-  // TODO: Monero distinguishes encrypted/unencrypted payment ID retrieval
+  // Monero finds the first nonce field and reads the payment ID from it:
+  // https://github.com/monero-project/monero/blob/ac02af92867590ca80b2779a7bbeafa99ff94dcb/
+  //   src/wallet/wallet2.cpp#L2709-L2752
   pub fn payment_id(&self) -> Option<PaymentId> {
     for field in &self.0 {
       if let ExtraField::Nonce(data) = field {

coins/monero/wallet/src/output.rs

@@ -301,7 +301,7 @@ impl WalletOutput {
 
   /// The payment ID included with this output.
   ///
-  /// This field may be `Some` even if wallet would not return a payment ID. This will happen if
+  /// This field may be `Some` even if wallet2 would not return a payment ID. This will happen if
   /// the scanned output belongs to the subaddress which spent Monero within the transaction which
   /// created the output. If multiple subaddresses spent Monero within this transactions, the key
   /// image with the highest index is determined to be the subaddress considered as the one

coins/monero/wallet/src/scan.rs

@@ -336,8 +336,8 @@ impl InternalScanner {
     }
 
     // If the block's version is >= 12, drop all unencrypted payment IDs
-    // TODO: Cite rule
-    // TODO: What if TX extra had multiple payment IDs embedded?
+    // https://github.com/monero-project/monero/blob/ac02af92867590ca80b2779a7bbeafa99ff94dcb/
+    //   src/wallet/wallet2.cpp#L2739-L2744
     if block.header.hardfork_version >= 12 {
       for output in &mut res.0 {
         if matches!(output.metadata.payment_id, Some(PaymentId::Unencrypted(_))) {
@@ -354,8 +354,10 @@ impl InternalScanner {
 ///
 /// When an output is successfully scanned, the output key MUST be checked against the local
 /// database for lack of prior observation. If it was prior observed, that output is an instance
-/// of the burning bug (TODO: cite) and MAY be unspendable. Only the prior received output(s) or
-/// the newly received output will be spendable (as spending one will burn all of them).
+/// of the
+/// [burning bug](https://web.getmonero.org/2018/09/25/a-post-mortum-of-the-burning-bug.html) and
+/// MAY be unspendable. Only the prior received output(s) or the newly received output will be
+/// spendable (as spending one will burn all of them).
 ///
 /// Once checked, the output key MUST be saved to the local database so future checks can be
 /// performed.

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

@@ -22,8 +22,8 @@ use crate::{
     RctType, RctPrunable, RctProofs,
   },
   transaction::Transaction,
+  address::{Network, SubaddressIndex, MoneroAddress},
   extra::MAX_ARBITRARY_DATA_SIZE,
-  address::{Network, MoneroAddress},
   rpc::FeeRate,
   ViewPair, GuaranteedViewPair, OutputWithDecoys,
 };
@@ -44,58 +44,48 @@ pub(crate) fn key_image_sort(x: &EdwardsPoint, y: &EdwardsPoint) -> core::cmp::O
 
 #[derive(Clone, PartialEq, Eq, Zeroize)]
 enum ChangeEnum {
-  None,
   AddressOnly(MoneroAddress),
-  AddressWithView(MoneroAddress, Zeroizing<Scalar>),
+  Standard { view_pair: ViewPair, subaddress: Option<SubaddressIndex> },
+  Guaranteed { view_pair: GuaranteedViewPair, subaddress: Option<SubaddressIndex> },
 }
 
 impl fmt::Debug for ChangeEnum {
   fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
     match self {
-      ChangeEnum::None => f.debug_struct("ChangeEnum::None").finish_non_exhaustive(),
       ChangeEnum::AddressOnly(addr) => {
         f.debug_struct("ChangeEnum::AddressOnly").field("addr", &addr).finish()
       }
-      ChangeEnum::AddressWithView(addr, _) => {
-        f.debug_struct("ChangeEnum::AddressWithView").field("addr", &addr).finish_non_exhaustive()
-      }
+      ChangeEnum::Standard { subaddress, .. } => f
+        .debug_struct("ChangeEnum::Standard")
+        .field("subaddress", &subaddress)
+        .finish_non_exhaustive(),
+      ChangeEnum::Guaranteed { subaddress, .. } => f
+        .debug_struct("ChangeEnum::Guaranteed")
+        .field("subaddress", &subaddress)
+        .finish_non_exhaustive(),
     }
   }
 }
 
 /// Specification for a change output.
 #[derive(Clone, PartialEq, Eq, Debug, Zeroize)]
-pub struct Change(ChangeEnum);
+pub struct Change(Option<ChangeEnum>);
 
 impl Change {
   /// Create a change output specification.
   ///
   /// This take the view key as Monero assumes it has the view key for change outputs. It optimizes
   /// its wallet protocol accordingly.
-  pub fn new(view: &ViewPair) -> Change {
-    Change(ChangeEnum::AddressWithView(
-      // Which network doesn't matter as the derivations will all be the same
-      // TODO: Support subaddresses
-      view.legacy_address(Network::Mainnet),
-      view.view.clone(),
-    ))
+  pub fn new(view_pair: ViewPair, subaddress: Option<SubaddressIndex>) -> Change {
+    Change(Some(ChangeEnum::Standard { view_pair, subaddress }))
   }
 
   /// Create a change output specification for a guaranteed view pair.
   ///
   /// This take the view key as Monero assumes it has the view key for change outputs. It optimizes
   /// its wallet protocol accordingly.
-  pub fn guaranteed(view: &GuaranteedViewPair) -> Change {
-    Change(ChangeEnum::AddressWithView(
-      view.address(
-        // Which network doesn't matter as the derivations will all be the same
-        Network::Mainnet,
-        // TODO: Support subaddresses
-        None,
-        None,
-      ),
-      view.0.view.clone(),
-    ))
+  pub fn guaranteed(view_pair: GuaranteedViewPair, subaddress: Option<SubaddressIndex>) -> Change {
+    Change(Some(ChangeEnum::Guaranteed { view_pair, subaddress }))
   }
 
   /// Create a fingerprintable change output specification.
@@ -116,38 +106,34 @@ impl Change {
   ///    monero-wallet TX without change.
   pub fn fingerprintable(address: Option<MoneroAddress>) -> Change {
     if let Some(address) = address {
-      Change(ChangeEnum::AddressOnly(address))
+      Change(Some(ChangeEnum::AddressOnly(address)))
     } else {
-      Change(ChangeEnum::None)
+      Change(None)
     }
   }
 }
 
-#[derive(Clone, PartialEq, Eq, Zeroize)]
+#[derive(Clone, PartialEq, Eq, Debug, Zeroize)]
 enum InternalPayment {
   Payment(MoneroAddress, u64),
-  Change(MoneroAddress, Option<Zeroizing<Scalar>>),
+  Change(ChangeEnum),
 }
 
 impl InternalPayment {
-  fn address(&self) -> &MoneroAddress {
+  fn address(&self) -> MoneroAddress {
     match self {
-      InternalPayment::Payment(addr, _) | InternalPayment::Change(addr, _) => addr,
-    }
-  }
-}
-
-impl fmt::Debug for InternalPayment {
-  fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-    match self {
-      InternalPayment::Payment(addr, amount) => f
-        .debug_struct("InternalPayment::Payment")
-        .field("addr", &addr)
-        .field("amount", &amount)
-        .finish(),
-      InternalPayment::Change(addr, _) => {
-        f.debug_struct("InternalPayment::Change").field("addr", &addr).finish_non_exhaustive()
+      InternalPayment::Payment(addr, _) => *addr,
+      InternalPayment::Change(change) => match change {
+        ChangeEnum::AddressOnly(addr) => *addr,
+        // Network::Mainnet as the network won't effect the derivations
+        ChangeEnum::Standard { view_pair, subaddress } => match subaddress {
+          Some(subaddress) => view_pair.subaddress(Network::Mainnet, *subaddress),
+          None => view_pair.legacy_address(Network::Mainnet),
+        },
+        ChangeEnum::Guaranteed { view_pair, subaddress } => {
+          view_pair.address(Network::Mainnet, *subaddress, None)
         }
+      },
     }
   }
 }
@@ -252,7 +238,6 @@ impl SignableTransaction {
       Err(SendError::NoInputs)?;
     }
     for input in &self.inputs {
-      // TODO: Add a function for the ring length
       if input.decoys().len() !=
         match self.rct_type {
           RctType::ClsagBulletproof => 11,
@@ -276,7 +261,7 @@ impl SignableTransaction {
     {
       let mut change_count = 0;
       for payment in &self.payments {
-        change_count += usize::from(u8::from(matches!(payment, InternalPayment::Change(_, _))));
+        change_count += usize::from(u8::from(matches!(payment, InternalPayment::Change(_))));
       }
       if change_count > 1 {
         Err(SendError::MaliciousSerialization)?;
@@ -319,7 +304,7 @@ impl SignableTransaction {
       .iter()
       .filter_map(|payment| match payment {
         InternalPayment::Payment(_, amount) => Some(amount),
-        InternalPayment::Change(_, _) => None,
+        InternalPayment::Change(_) => None,
       })
       .sum::<u64>();
     let (weight, necessary_fee) = self.weight_and_necessary_fee();
@@ -331,11 +316,14 @@ impl SignableTransaction {
       })?;
     }
 
-    // The actual limit is half the block size, and for the minimum block size of 300k, that'd be
-    // 150k
-    // wallet2 will only create transactions up to 100k bytes however
-    // TODO: Cite
-    const MAX_TX_SIZE: usize = 100_000;
+    // The limit is half the no-penalty block size
+    // https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c454
+    //   /src/wallet/wallet2.cpp#L110766-L11085
+    // https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c454
+    //   /src/cryptonote_config.h#L61
+    // https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c454
+    //   /src/cryptonote_config.h#L64
+    const MAX_TX_SIZE: usize = (300_000 / 2) - 600;
     if weight >= MAX_TX_SIZE {
       Err(SendError::TooLargeTransaction)?;
     }
@@ -366,12 +354,9 @@ impl SignableTransaction {
       .into_iter()
       .map(|(addr, amount)| InternalPayment::Payment(addr, amount))
       .collect::<Vec<_>>();
-    match change.0 {
-      ChangeEnum::None => {}
-      ChangeEnum::AddressOnly(addr) => payments.push(InternalPayment::Change(addr, None)),
-      ChangeEnum::AddressWithView(addr, view) => {
-        payments.push(InternalPayment::Change(addr, Some(view)))
-      }
+
+    if let Some(change) = change.0 {
+      payments.push(InternalPayment::Change(change));
     }
 
     let mut res =
@@ -412,16 +397,36 @@ impl SignableTransaction {
           write_vec(write_byte, addr.to_string().as_bytes(), w)?;
           w.write_all(&amount.to_le_bytes())
         }
-        InternalPayment::Change(addr, change_view) => {
+        InternalPayment::Change(change) => match change {
+          ChangeEnum::AddressOnly(addr) => {
             w.write_all(&[1])?;
-          write_vec(write_byte, addr.to_string().as_bytes(), w)?;
-          if let Some(view) = change_view.as_ref() {
-            w.write_all(&[1])?;
-            write_scalar(view, w)
+            write_vec(write_byte, addr.to_string().as_bytes(), w)
+          }
+          ChangeEnum::Standard { view_pair, subaddress } => {
+            w.write_all(&[2])?;
+            write_point(&view_pair.spend(), w)?;
+            write_scalar(&view_pair.view, w)?;
+            if let Some(subaddress) = subaddress {
+              w.write_all(&subaddress.account().to_le_bytes())?;
+              w.write_all(&subaddress.address().to_le_bytes())
             } else {
-            w.write_all(&[0])
+              w.write_all(&0u32.to_le_bytes())?;
+              w.write_all(&0u32.to_le_bytes())
             }
           }
+          ChangeEnum::Guaranteed { view_pair, subaddress } => {
+            w.write_all(&[3])?;
+            write_point(&view_pair.spend(), w)?;
+            write_scalar(&view_pair.0.view, w)?;
+            if let Some(subaddress) = subaddress {
+              w.write_all(&subaddress.account().to_le_bytes())?;
+              w.write_all(&subaddress.address().to_le_bytes())
+            } else {
+              w.write_all(&0u32.to_le_bytes())?;
+              w.write_all(&0u32.to_le_bytes())
+            }
+          }
+        },
       }
     }
 
@@ -458,14 +463,17 @@ impl SignableTransaction {
     fn read_payment<R: io::Read>(r: &mut R) -> io::Result<InternalPayment> {
       Ok(match read_byte(r)? {
         0 => InternalPayment::Payment(read_address(r)?, read_u64(r)?),
-        1 => InternalPayment::Change(
-          read_address(r)?,
-          match read_byte(r)? {
-            0 => None,
-            1 => Some(Zeroizing::new(read_scalar(r)?)),
-            _ => Err(io::Error::other("invalid change view"))?,
-          },
-        ),
+        1 => InternalPayment::Change(ChangeEnum::AddressOnly(read_address(r)?)),
+        2 => InternalPayment::Change(ChangeEnum::Standard {
+          view_pair: ViewPair::new(read_point(r)?, Zeroizing::new(read_scalar(r)?))
+            .map_err(io::Error::other)?,
+          subaddress: SubaddressIndex::new(read_u32(r)?, read_u32(r)?),
+        }),
+        3 => InternalPayment::Change(ChangeEnum::Guaranteed {
+          view_pair: GuaranteedViewPair::new(read_point(r)?, Zeroizing::new(read_scalar(r)?))
+            .map_err(io::Error::other)?,
+          subaddress: SubaddressIndex::new(read_u32(r)?, read_u32(r)?),
+        }),
         _ => Err(io::Error::other("invalid payment"))?,
       })
     }

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

@@ -78,7 +78,7 @@ impl SignableTransaction {
     } else {
       // If there's no payment ID, we push a dummy (as wallet2 does) if there's only one payment
       if (self.payments.len() == 2) &&
-        self.payments.iter().any(|payment| matches!(payment, InternalPayment::Change(_, _)))
+        self.payments.iter().any(|payment| matches!(payment, InternalPayment::Change(_)))
       {
         let (_, payment_id_xor) = self
           .payments
@@ -292,7 +292,7 @@ impl SignableTransactionWithKeyImages {
             .intent
             .payments
             .iter()
-            .any(|payment| matches!(payment, InternalPayment::Change(_, _)))
+            .any(|payment| matches!(payment, InternalPayment::Change(_)))
           {
             // The necessary fee is the fee
             self.intent.weight_and_necessary_fee().1
@@ -306,7 +306,7 @@ impl SignableTransactionWithKeyImages {
               .iter()
               .filter_map(|payment| match payment {
                 InternalPayment::Payment(_, amount) => Some(amount),
-                InternalPayment::Change(_, _) => None,
+                InternalPayment::Change(_) => None,
               })
               .sum::<u64>();
             // Safe since the constructor checks inputs >= (payments + fee)

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

@@ -12,7 +12,7 @@ use crate::{
   primitives::{keccak256, Commitment},
   ringct::EncryptedAmount,
   SharedKeyDerivations, OutputWithDecoys,
-  send::{InternalPayment, SignableTransaction, key_image_sort},
+  send::{ChangeEnum, InternalPayment, SignableTransaction, key_image_sort},
 };
 
 impl SignableTransaction {
@@ -42,15 +42,13 @@ impl SignableTransaction {
   fn has_payments_to_subaddresses(&self) -> bool {
     self.payments.iter().any(|payment| match payment {
       InternalPayment::Payment(addr, _) => addr.is_subaddress(),
-      InternalPayment::Change(addr, view) => {
-        if view.is_some() {
-          // It should not be possible to construct a change specification to a subaddress with a
-          // view key
-          // TODO
-          debug_assert!(!addr.is_subaddress());
-        }
-        addr.is_subaddress()
-      }
+      InternalPayment::Change(change) => match change {
+        ChangeEnum::AddressOnly(addr) => addr.is_subaddress(),
+        // These aren't considered payments to subaddresses as we don't need to send to them as
+        // subaddresses
+        // We can calculate the shared key using the view key, as if we were receiving, instead
+        ChangeEnum::Standard { .. } | ChangeEnum::Guaranteed { .. } => false,
+      },
     })
   }
 
@@ -62,7 +60,10 @@ impl SignableTransaction {
 
     let has_change_view = self.payments.iter().any(|payment| match payment {
       InternalPayment::Payment(_, _) => false,
-      InternalPayment::Change(_, view) => view.is_some(),
+      InternalPayment::Change(change) => match change {
+        ChangeEnum::AddressOnly(_) => false,
+        ChangeEnum::Standard { .. } | ChangeEnum::Guaranteed { .. } => true,
+      },
     });
 
     /*
@@ -107,11 +108,17 @@ impl SignableTransaction {
 
       let ecdh = match payment {
         // If we don't have the view key, use the key dedicated for this address (r A)
-        InternalPayment::Payment(_, _) | InternalPayment::Change(_, None) => {
+        InternalPayment::Payment(_, _) |
+        InternalPayment::Change(ChangeEnum::AddressOnly { .. }) => {
           Zeroizing::new(key_to_use.deref() * addr.view())
         }
         // If we do have the view key, use the commitment to the key (a R)
-        InternalPayment::Change(_, Some(view)) => Zeroizing::new(view.deref() * tx_key_pub),
+        InternalPayment::Change(ChangeEnum::Standard { view_pair, .. }) => {
+          Zeroizing::new(view_pair.view.deref() * tx_key_pub)
+        }
+        InternalPayment::Change(ChangeEnum::Guaranteed { view_pair, .. }) => {
+          Zeroizing::new(view_pair.0.view.deref() * tx_key_pub)
+        }
       };
 
       res.push(ecdh);
@@ -172,9 +179,6 @@ impl SignableTransaction {
         panic!("filtered payment wasn't a payment")
       };
 
-      // TODO: Support subaddresses as change?
-      debug_assert!(addr.is_subaddress());
-
       return (tx_key.deref() * addr.spend(), vec![]);
     }
 
@@ -207,14 +211,14 @@ impl SignableTransaction {
     for (payment, shared_key_derivations) in self.payments.iter().zip(shared_key_derivations) {
       let amount = match payment {
         InternalPayment::Payment(_, amount) => *amount,
-        InternalPayment::Change(_, _) => {
+        InternalPayment::Change(_) => {
           let inputs = self.inputs.iter().map(|input| input.commitment().amount).sum::<u64>();
           let payments = self
             .payments
             .iter()
             .filter_map(|payment| match payment {
               InternalPayment::Payment(_, amount) => Some(amount),
-              InternalPayment::Change(_, _) => None,
+              InternalPayment::Change(_) => None,
             })
             .sum::<u64>();
           let necessary_fee = self.weight_and_necessary_fee().1;

coins/monero/wallet/src/view_pair.rs

@@ -27,7 +27,7 @@ pub enum ViewPairError {
 /// The pair of keys necessary to scan transactions.
 ///
 /// This is composed of the public spend key and the private view key.
-#[derive(Clone, Zeroize, ZeroizeOnDrop)]
+#[derive(Clone, PartialEq, Eq, Zeroize, ZeroizeOnDrop)]
 pub struct ViewPair {
   spend: EdwardsPoint,
   pub(crate) view: Zeroizing<Scalar>,
@@ -99,7 +99,7 @@ impl ViewPair {
 /// 'Guaranteed' outputs, or transactions outputs to the burning bug, are not officially specified
 /// by the Monero project. They should only be used if necessary. No support outside of
 /// monero-wallet is promised.
-#[derive(Clone, Zeroize)]
+#[derive(Clone, PartialEq, Eq, Zeroize)]
 pub struct GuaranteedViewPair(pub(crate) ViewPair);
 
 impl GuaranteedViewPair {

coins/monero/wallet/tests/decoys.rs

@@ -2,7 +2,7 @@ use monero_simple_request_rpc::SimpleRequestRpc;
 use monero_wallet::{
   DEFAULT_LOCK_WINDOW,
   transaction::Transaction,
-  rpc::{OutputResponse, Rpc},
+  rpc::{OutputResponse, Rpc, DecoyRpc},
   WalletOutput,
 };
 

coins/monero/wallet/tests/runner/mod.rs

@@ -254,10 +254,11 @@ macro_rules! test {
             rct_type,
             outgoing_view,
             Change::new(
-              &ViewPair::new(
+              ViewPair::new(
                 &Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE,
                 Zeroizing::new(Scalar::random(&mut OsRng))
               ).unwrap(),
+              None,
             ),
             rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(),
           );
@@ -267,6 +268,8 @@ macro_rules! test {
             #[cfg(feature = "multisig")]
             let keys = keys.clone();
 
+            assert_eq!(&SignableTransaction::read(&mut tx.serialize().as_slice()).unwrap(), &tx);
+
             let eventuality = Eventuality::from(tx.clone());
 
             let tx = if !multisig {

coins/monero/wallet/tests/send.rs

@@ -115,7 +115,7 @@ test!(
       let mut builder = SignableTransactionBuilder::new(
         rct_type,
         outgoing_view,
-        Change::new(&change_view),
+        Change::new(change_view.clone(), None),
         rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(),
       );
       add_inputs(rct_type, &rpc, vec![outputs.first().unwrap().clone()], &mut builder).await;
@@ -144,6 +144,8 @@ test!(
       assert!(sub_outputs.len() == 1);
       assert_eq!(sub_outputs[0].transaction(), tx.hash());
       assert_eq!(sub_outputs[0].commitment().amount, 1);
+      assert!(sub_outputs[0].subaddress().unwrap().account() == 0);
+      assert!(sub_outputs[0].subaddress().unwrap().address() == 1);
 
       // Make sure only one R was included in TX extra
       assert!(Extra::read::<&[u8]>(&mut tx.prefix().extra.as_ref())
@@ -333,3 +335,60 @@ test!(
     },
   ),
 );
+
+test!(
+  subaddress_change,
+  (
+    // 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(), ())
+    },
+    |rpc, block, tx: Transaction, mut scanner: Scanner, ()| async move {
+      let outputs = scanner.scan(&rpc, &block).await.unwrap().not_additionally_locked();
+      assert_eq!(outputs.len(), 1);
+      assert_eq!(outputs[0].transaction(), tx.hash());
+      assert_eq!(outputs[0].commitment().amount, 1000000000000);
+      outputs
+    },
+  ),
+  (
+    |rct_type, rpc: SimpleRequestRpc, _, _, outputs: Vec<WalletOutput>| async move {
+      use monero_wallet::rpc::FeePriority;
+
+      let view_priv = Zeroizing::new(Scalar::random(&mut OsRng));
+      let mut outgoing_view = Zeroizing::new([0; 32]);
+      OsRng.fill_bytes(outgoing_view.as_mut());
+      let change_view =
+        ViewPair::new(&Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE, view_priv.clone())
+          .unwrap();
+
+      let mut builder = SignableTransactionBuilder::new(
+        rct_type,
+        outgoing_view,
+        Change::new(change_view.clone(), Some(SubaddressIndex::new(0, 1).unwrap())),
+        rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(),
+      );
+      add_inputs(rct_type, &rpc, vec![outputs.first().unwrap().clone()], &mut builder).await;
+
+      // Send to a random address
+      let view = ViewPair::new(
+        &Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE,
+        Zeroizing::new(Scalar::random(&mut OsRng)),
+      )
+      .unwrap();
+      builder.add_payment(view.legacy_address(Network::Mainnet), 1);
+      (builder.build().unwrap(), change_view)
+    },
+    |rpc, block, _, _, change_view: ViewPair| async move {
+      // Make sure the change can pick up its output
+      let mut change_scanner = Scanner::new(change_view);
+      change_scanner.register_subaddress(SubaddressIndex::new(0, 1).unwrap());
+      let outputs = change_scanner.scan(&rpc, &block).await.unwrap().not_additionally_locked();
+      assert!(outputs.len() == 1);
+      assert!(outputs[0].subaddress().unwrap().account() == 0);
+      assert!(outputs[0].subaddress().unwrap().address() == 1);
+    },
+  ),
+);

tests/full-stack/src/tests/mint_and_burn.rs

@@ -389,7 +389,7 @@ async fn mint_and_burn_test() {
           ),
           1_100_000_000_000,
         )],
-        Change::new(&view_pair),
+        Change::new(view_pair.clone(), None),
         vec![Shorthand::transfer(None, serai_addr).encode()],
         rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(),
       )

tests/processor/src/networks.rs

@@ -474,7 +474,7 @@ impl Wallet {
           outgoing_view_key,
           inputs,
           vec![(to_addr, AMOUNT)],
-          Change::new(view_pair),
+          Change::new(view_pair.clone(), None),
           data,
           rpc.get_fee_rate(FeePriority::Unimportant).await.unwrap(),
         )