From b195db0929704e29b2db7db7c923efd9e31fc33a Mon Sep 17 00:00:00 2001
From: Luke Parker <lukeparker5132@gmail.com>
Date: Tue, 4 Jul 2023 18:07:27 -0400
Subject: [PATCH] Fix the known issue with the DSA

I wrote it to only select TXs with a timelock, not only TXs which are unlocked.
This most likely explains why it so heavily selected coinbases.

Also moves an InternalError which would've never been hit on mainnet, yet
technically isn't an invariant, to only exist when cfg(test).
---
 coins/monero/src/rpc/mod.rs       | 12 +++++-------
 coins/monero/src/wallet/decoys.rs | 15 ++++++++++-----
 coins/monero/src/wallet/scan.rs   |  6 +++++-
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/coins/monero/src/rpc/mod.rs b/coins/monero/src/rpc/mod.rs
index daae57a2..224b1b35 100644
--- a/coins/monero/src/rpc/mod.rs
+++ b/coins/monero/src/rpc/mod.rs
@@ -138,7 +138,7 @@ impl<R: RpcConnection> Rpc<R> {
       )
       .map_err(|_| RpcError::InvalidNode)?,
     )
-    .map_err(|_| RpcError::InternalError("Failed to parse JSON response"))
+    .map_err(|_| RpcError::InvalidNode)
   }
 
   /// Perform a JSON-RPC call with the specified method with the provided parameters
@@ -538,12 +538,10 @@ impl<R: RpcConnection> Rpc<R> {
       .iter()
       .enumerate()
       .map(|(i, out)| {
-        Ok(Some([rpc_point(&out.key)?, rpc_point(&out.mask)?]).filter(|_| {
-          match txs[i].prefix.timelock {
-            Timelock::Block(t_height) => t_height <= height,
-            _ => false,
-          }
-        }))
+        Ok(
+          Some([rpc_point(&out.key)?, rpc_point(&out.mask)?])
+            .filter(|_| Timelock::Block(height) >= txs[i].prefix.timelock),
+        )
       })
       .collect()
   }
diff --git a/coins/monero/src/wallet/decoys.rs b/coins/monero/src/wallet/decoys.rs
index 8c8f6ebb..81eeedab 100644
--- a/coins/monero/src/wallet/decoys.rs
+++ b/coins/monero/src/wallet/decoys.rs
@@ -47,9 +47,11 @@ async fn select_n<'a, R: RngCore + CryptoRng, RPC: RpcConnection>(
   count: usize,
 ) -> Result<Vec<(u64, [EdwardsPoint; 2])>, RpcError> {
   if height >= rpc.get_height().await? {
+    // TODO: Don't use InternalError for the caller's failure
     Err(RpcError::InternalError("decoys being requested from too young blocks"))?;
   }
 
+  #[cfg(test)]
   let mut iters = 0;
   let mut confirmed = Vec::with_capacity(count);
   // Retries on failure. Retries are obvious as decoys, yet should be minimal
@@ -57,10 +59,13 @@ async fn select_n<'a, R: RngCore + CryptoRng, RPC: RpcConnection>(
     let remaining = count - confirmed.len();
     let mut candidates = Vec::with_capacity(remaining);
     while candidates.len() != remaining {
-      iters += 1;
-      // This is cheap and on fresh chains, thousands of rounds may be needed
-      if iters == 10000 {
-        Err(RpcError::InternalError("not enough decoy candidates"))?;
+      #[cfg(test)]
+      {
+        iters += 1;
+        // This is cheap and on fresh chains, a lot of rounds may be needed
+        if iters == 100 {
+          Err(RpcError::InternalError("hit decoy selection round limit"))?;
+        }
       }
 
       // Use a gamma distribution
@@ -183,7 +188,7 @@ impl Decoys {
       used.insert(o.0);
     }
 
-    // TODO: Simply create a TX with less than the target amount
+    // TODO: Create a TX with less than the target amount, as allowed by the protocol
     if (high - MATURITY) < u64::try_from(inputs.len() * ring_len).unwrap() {
       Err(RpcError::InternalError("not enough decoy candidates"))?;
     }
diff --git a/coins/monero/src/wallet/scan.rs b/coins/monero/src/wallet/scan.rs
index 1a29d21e..4816ed86 100644
--- a/coins/monero/src/wallet/scan.rs
+++ b/coins/monero/src/wallet/scan.rs
@@ -273,7 +273,11 @@ impl<O: Clone + Zeroize> Timelocked<O> {
   /// Returns None if the Timelocks aren't comparable. Returns Some(vec![]) if none are unlocked.
   pub fn unlocked(&self, timelock: Timelock) -> Option<Vec<O>> {
     // If the Timelocks are comparable, return the outputs if they're now unlocked
-    self.0.partial_cmp(&timelock).filter(|_| self.0 <= timelock).map(|_| self.1.clone())
+    if self.0 <= timelock {
+      Some(self.1.clone())
+    } else {
+      None
+    }
   }
 
   pub fn ignore_timelock(&self) -> Vec<O> {