Restore minimum Monero fee from develop

If there's no change, the fee is difference of the inputs to the outputs. The
prior code wouldn't check that amount is greater than or equal to the necessary
fee, and returning the would-be change amount as the fee isn't necessarily
helpful.

Now the fee is validated in such cases and the necessary fee is returned,
enabling operating off of that.

.github/workflows/monero-tests.yaml

@@ -42,7 +42,6 @@ jobs:
           GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-seed --lib
           GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package polyseed --lib
           GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-wallet-util --lib
-          GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-serai-verify-chain --lib
 
       # Doesn't run unit tests with features as the tests workflow will
 
@@ -67,7 +66,6 @@ jobs:
           GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-simple-request-rpc --test '*'
           GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-wallet --test '*'
           GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-wallet-util --test '*'
-          GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-serai-verify-chain --test '*'
 
       - name: Run Integration Tests
         # Don't run if the the tests workflow also will
@@ -77,4 +75,3 @@ jobs:
           GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-simple-request-rpc --test '*'
           GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-wallet --all-features --test '*'
           GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-wallet-util --all-features --test '*'
-          GITHUB_CI=true RUST_BACKTRACE=1 cargo test --package monero-serai-verify-chain --test '*'

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/decoys.rs

@@ -163,7 +163,7 @@ async fn select_decoys<R: RngCore + CryptoRng>(
   distribution.truncate(height);
 
   if distribution.len() < DEFAULT_LOCK_WINDOW {
-    Err(RpcError::InternalError("not enough decoy candidates".to_string()))?;
+    Err(RpcError::InternalError("not enough blocks to select decoys".to_string()))?;
   }
 
   #[allow(clippy::cast_precision_loss)]
@@ -181,10 +181,12 @@ async fn select_decoys<R: RngCore + CryptoRng>(
 
   // TODO: Create a TX with less than the target amount, as allowed by the protocol
   let high = distribution[distribution.len() - DEFAULT_LOCK_WINDOW];
+  // This assumes that each miner TX had one output (as sane) and checks we have sufficient
+  // outputs even when excluding them (due to their own timelock requirements)
   if high.saturating_sub(u64::try_from(COINBASE_LOCK_WINDOW).unwrap()) <
     u64::try_from(inputs.len() * ring_len).unwrap()
   {
-    Err(RpcError::InternalError("not enough coinbase candidates".to_string()))?;
+    Err(RpcError::InternalError("not enough decoy candidates".to_string()))?;
   }
 
   // Select all decoys for this transaction, assuming we generate a sane transaction

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

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

@@ -193,18 +193,20 @@ pub enum SendError {
   /// This transaction could not pay for itself.
   #[cfg_attr(
     feature = "std",
-    error("not enough funds (inputs {inputs}, outputs {outputs}, fee {fee:?})")
+    error(
+      "not enough funds (inputs {inputs}, outputs {outputs}, necessary_fee {necessary_fee:?})"
+    )
   )]
   NotEnoughFunds {
     /// The amount of funds the inputs contributed.
     inputs: u64,
     /// The amount of funds the outputs required.
     outputs: u64,
-    /// The fee which would be paid on top.
+    /// The fee necessary to be paid on top.
     ///
     /// If this is None, it is because the fee was not calculated as the outputs alone caused this
     /// error.
-    fee: Option<u64>,
+    necessary_fee: Option<u64>,
   },
   /// This transaction is being signed with the wrong private key.
   #[cfg_attr(feature = "std", error("wrong spend private key"))]
@@ -251,6 +253,7 @@ impl SignableTransaction {
       Err(SendError::NoInputs)?;
     }
     for (_, decoys) in &self.inputs {
+      // TODO: Add a function for the ring length
       if decoys.len() !=
         match self.rct_type {
           RctType::ClsagBulletproof => 11,
@@ -320,22 +323,19 @@ impl SignableTransaction {
         InternalPayment::Change(_, _) => None,
       })
       .sum::<u64>();
-    // Necessary so weight_and_fee doesn't underflow
-    if in_amount < payments_amount {
-      Err(SendError::NotEnoughFunds { inputs: in_amount, outputs: payments_amount, fee: None })?;
-    }
-    let (weight, fee) = self.weight_and_fee();
-    if in_amount < (payments_amount + fee) {
+    let (weight, necessary_fee) = self.weight_and_necessary_fee();
+    if in_amount < (payments_amount + necessary_fee) {
       Err(SendError::NotEnoughFunds {
         inputs: in_amount,
         outputs: payments_amount,
-        fee: Some(fee),
+        necessary_fee: Some(necessary_fee),
       })?;
     }
 
     // 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;
     if weight >= MAX_TX_SIZE {
       Err(SendError::TooLargeTransaction)?;
@@ -393,9 +393,12 @@ impl SignableTransaction {
     self.fee_rate
   }
 
-  /// The fee this transaction will use.
-  pub fn fee(&self) -> u64 {
-    self.weight_and_fee().1
+  /// The fee this transaction requires.
+  ///
+  /// This is distinct from the fee this transaction will use. If no change output is specified,
+  /// all unspent coins will be shunted to the fee.
+  pub fn necessary_fee(&self) -> u64 {
+    self.weight_and_necessary_fee().1
   }
 
   /// Write a SignableTransaction.

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

@@ -105,7 +105,7 @@ impl SignableTransaction {
     serialized
   }
 
-  pub(crate) fn weight_and_fee(&self) -> (usize, u64) {
+  pub(crate) fn weight_and_necessary_fee(&self) -> (usize, u64) {
     /*
       This transaction is variable length to:
         - The decoy offsets (fixed)
@@ -122,7 +122,14 @@ impl SignableTransaction {
         key_images.push(ED25519_BASEPOINT_POINT);
         clsags.push(Clsag {
           D: ED25519_BASEPOINT_POINT,
-          s: vec![Scalar::ZERO; 16],
+          s: vec![
+            Scalar::ZERO;
+            match self.rct_type {
+              RctType::ClsagBulletproof => 11,
+              RctType::ClsagBulletproofPlus => 16,
+              _ => unreachable!("unsupported RCT type"),
+            }
+          ],
           c1: Scalar::ZERO,
         });
         pseudo_outs.push(ED25519_BASEPOINT_POINT);
@@ -216,22 +223,6 @@ impl SignableTransaction {
         1
     };
 
-    // If we don't have a change output, the difference is the fee
-    if !self.payments.iter().any(|payment| matches!(payment, InternalPayment::Change(_, _))) {
-      let inputs = self.inputs.iter().map(|input| input.0.commitment().amount).sum::<u64>();
-      let payments = self
-        .payments
-        .iter()
-        .filter_map(|payment| match payment {
-          InternalPayment::Payment(_, amount) => Some(amount),
-          InternalPayment::Change(_, _) => None,
-        })
-        .sum::<u64>();
-      // Safe since the constructor checks inputs > payments before any calls to weight_and_fee
-      let fee = inputs - payments;
-      return (base_weight + varint_len(fee), fee);
-    }
-
     // We now have the base weight, without the fee encoded
     // The fee itself will impact the weight as its encoding is [1, 9] bytes long
     let mut possible_weights = Vec::with_capacity(9);
@@ -248,17 +239,17 @@ impl SignableTransaction {
 
     // We now look for the fee whose length matches the length used to derive it
     let mut weight_and_fee = None;
-    for (len, possible_fee) in possible_fees.into_iter().enumerate() {
-      let len = 1 + len;
-      debug_assert!(1 <= len);
-      debug_assert!(len <= 9);
+    for (fee_len, possible_fee) in possible_fees.into_iter().enumerate() {
+      let fee_len = 1 + fee_len;
+      debug_assert!(1 <= fee_len);
+      debug_assert!(fee_len <= 9);
 
       // We use the first fee whose encoded length is not larger than the length used within this
       // weight
       // This should be because the lengths are equal, yet means if somehow none are equal, this
       // will still terminate successfully
-      if varint_len(possible_fee) <= len {
-        weight_and_fee = Some((base_weight + len, possible_fee));
+      if varint_len(possible_fee) <= fee_len {
+        weight_and_fee = Some((base_weight + fee_len, possible_fee));
         break;
       }
     }
@@ -297,7 +288,30 @@ impl SignableTransactionWithKeyImages {
       },
       proofs: Some(RctProofs {
         base: RctBase {
-          fee: self.intent.weight_and_fee().1,
+          fee: if self
+            .intent
+            .payments
+            .iter()
+            .any(|payment| matches!(payment, InternalPayment::Change(_, _)))
+          {
+            // The necessary fee is the fee
+            self.intent.weight_and_necessary_fee().1
+          } else {
+            // If we don't have a change output, the difference is the fee
+            let inputs =
+              self.intent.inputs.iter().map(|input| input.0.commitment().amount).sum::<u64>();
+            let payments = self
+              .intent
+              .payments
+              .iter()
+              .filter_map(|payment| match payment {
+                InternalPayment::Payment(_, amount) => Some(amount),
+                InternalPayment::Change(_, _) => None,
+              })
+              .sum::<u64>();
+            // Safe since the constructor checks inputs >= (payments + fee)
+            inputs - payments
+          },
           encrypted_amounts,
           pseudo_outs: vec![],
           commitments,

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

@@ -217,9 +217,9 @@ impl SignableTransaction {
               InternalPayment::Change(_, _) => None,
             })
             .sum::<u64>();
-          let fee = self.weight_and_fee().1;
-          // Safe since the constructor checked this
-          inputs - (payments + fee)
+          let necessary_fee = self.weight_and_necessary_fee().1;
+          // Safe since the constructor checked this TX has enough funds for itself
+          inputs - (payments + necessary_fee)
         }
       };
       let commitment = Commitment::new(shared_key_derivations.commitment_mask(), amount);

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

@@ -125,8 +125,10 @@ pub async fn rpc() -> SimpleRequestRpc {
   let rpc =
     SimpleRequestRpc::new("http://serai:seraidex@127.0.0.1:18081".to_string()).await.unwrap();
 
+  const BLOCKS_TO_MINE: usize = 110;
+
   // Only run once
-  if rpc.get_height().await.unwrap() != 1 {
+  if rpc.get_height().await.unwrap() > BLOCKS_TO_MINE {
     return rpc;
   }
 
@@ -137,8 +139,8 @@ pub async fn rpc() -> SimpleRequestRpc {
     &Scalar::random(&mut OsRng) * ED25519_BASEPOINT_TABLE,
   );
 
-  // Mine 80 blocks to ensure decoy availability
-  rpc.generate_blocks(&addr, 80).await.unwrap();
+  // Mine enough blocks to ensure decoy availability
+  rpc.generate_blocks(&addr, BLOCKS_TO_MINE).await.unwrap();
 
   rpc
 }

coins/monero/wallet/util/tests/tests.rs

@@ -0,0 +1,3 @@
+// TODO
+#[test]
+fn test() {}

orchestration/dev/coins/monero/run.sh

@@ -7,5 +7,5 @@ RPC_PASS="${RPC_PASS:=seraidex}"
 monerod --non-interactive --regtest --offline --fixed-difficulty=1 \
   --no-zmq --rpc-bind-ip=0.0.0.0 --rpc-bind-port=18081 --confirm-external-bind \
   --rpc-access-control-origins "*" --disable-rpc-ban \
-  --rpc-login=$RPC_USER:$RPC_PASS \
+  --rpc-login=$RPC_USER:$RPC_PASS --log-level 2 \
   $1

orchestration/src/coins/monero.rs

@@ -69,14 +69,7 @@ CMD ["/run.sh"]
 }
 
 pub fn monero(orchestration_path: &Path, network: Network) {
-  monero_internal(
-    network,
-    if network == Network::Dev { Os::Alpine } else { Os::Debian },
-    orchestration_path,
-    "monero",
-    "monerod",
-    "18080 18081",
-  )
+  monero_internal(network, Os::Debian, orchestration_path, "monero", "monerod", "18080 18081")
 }
 
 pub fn monero_wallet_rpc(orchestration_path: &Path) {

processor/src/networks/monero.rs

@@ -159,7 +159,7 @@ impl EventualityTrait for Eventuality {
 pub struct SignableTransaction(MSignableTransaction);
 impl SignableTransactionTrait for SignableTransaction {
   fn fee(&self) -> u64 {
-    self.0.fee()
+    self.0.necessary_fee()
   }
 }
 
@@ -293,7 +293,8 @@ impl Monero {
     let fee = fees.get(fees.len() / 2).copied().unwrap_or(0);
 
     // TODO: Set a sane minimum fee
-    Ok(FeeRate::new(fee.max(1500000), 10000).unwrap())
+    const MINIMUM_FEE: u64 = 1_500_000;
+    Ok(FeeRate::new(fee.max(MINIMUM_FEE), 10000).unwrap())
   }
 
   async fn make_signable_transaction(
@@ -382,7 +383,7 @@ impl Monero {
     ) {
       Ok(signable) => Ok(Some({
         if calculating_fee {
-          MakeSignableTransactionResult::Fee(signable.fee())
+          MakeSignableTransactionResult::Fee(signable.necessary_fee())
         } else {
           MakeSignableTransactionResult::SignableTransaction(signable)
         }
@@ -404,17 +405,17 @@ impl Monero {
         SendError::MultiplePaymentIds => {
           panic!("multiple payment IDs despite not supporting integrated addresses");
         }
-        SendError::NotEnoughFunds { inputs, outputs, fee } => {
+        SendError::NotEnoughFunds { inputs, outputs, necessary_fee } => {
           log::debug!(
-            "Monero NotEnoughFunds. inputs: {:?}, outputs: {:?}, fee: {fee:?}",
+            "Monero NotEnoughFunds. inputs: {:?}, outputs: {:?}, necessary_fee: {necessary_fee:?}",
             inputs,
             outputs
           );
-          match fee {
-            Some(fee) => {
+          match necessary_fee {
+            Some(necessary_fee) => {
               // If we're solely calculating the fee, return the fee this TX will cost
               if calculating_fee {
-                Ok(Some(MakeSignableTransactionResult::Fee(fee)))
+                Ok(Some(MakeSignableTransactionResult::Fee(necessary_fee)))
               } else {
                 // If we're actually trying to make the TX, return None
                 Ok(None)
@@ -475,7 +476,6 @@ impl Network for Monero {
   const MAX_OUTPUTS: usize = 16;
 
   // 0.01 XMR
-  // TODO: Set a sane dust
   const DUST: u64 = 10000000000;
 
   // TODO