Optimize decoy selection

Saves roughly 0.8s when running the tests, which took 16.6s and now take 
15.8 (5%).

Removes the larger sample size, which replaced the closest selected 
decoy with the real spend, per advice of Rucknium.

coins/monero/src/wallet/decoys.rs

@@ -16,7 +16,8 @@ const BLOCK_TIME: usize = 120;
 const BLOCKS_PER_YEAR: usize = 365 * 24 * 60 * 60 / BLOCK_TIME;
 const TIP_APPLICATION: f64 = (LOCK_WINDOW * BLOCK_TIME) as f64;
 
-const DECOYS: usize = 11;
+const RING_LEN: usize = 11;
+const DECOYS: usize = RING_LEN - 1;
 
 lazy_static! {
   static ref GAMMA: Gamma<f64> = Gamma::new(19.28, 1.0 / 1.61).unwrap();
@@ -32,12 +33,6 @@ async fn select_n<R: RngCore + CryptoRng>(
   used: &mut HashSet<u64>,
   count: usize
 ) -> Result<Vec<(u64, [EdwardsPoint; 2])>, RpcError> {
-  // Panic if not enough decoys are available
-  // TODO: Simply create a TX with less than the target amount
-  if (high - MATURITY) < u64::try_from(DECOYS).unwrap() {
-    panic!("Not enough decoys available");
-  }
-
   let mut confirmed = Vec::with_capacity(count);
   while confirmed.len() != count {
     let remaining = count - confirmed.len();
@@ -79,11 +74,11 @@ async fn select_n<R: RngCore + CryptoRng>(
   Ok(confirmed)
 }
 
-fn offset(decoys: &[u64]) -> Vec<u64> {
+fn offset(ring: &[u64]) -> Vec<u64> {
-  let mut res = vec![decoys[0]];
+  let mut res = vec![ring[0]];
-  res.resize(decoys.len(), 0);
+  res.resize(ring.len(), 0);
-  for m in (1 .. decoys.len()).rev() {
+  for m in (1 .. ring.len()).rev() {
-    res[m] = decoys[m] - decoys[m - 1];
+    res[m] = ring[m] - ring[m - 1];
   }
   res
 }
@@ -128,68 +123,76 @@ impl Decoys {
       used.insert(o.0);
     }
 
+    // Panic if not enough decoys are available
+    // TODO: Simply create a TX with less than the target amount, or at least return an error
+    if (high - MATURITY) < u64::try_from(inputs.len() * RING_LEN).unwrap() {
+      panic!("Not enough decoys available");
+    }
+
+    // Select all decoys for this transaction, assuming we generate a sane transaction
+    // We should almost never naturally generate an insane transaction, hence why this doesn't bother
+    // with an overage
+    let mut decoys = select_n(
+      rng,
+      rpc,
+      height,
+      &distribution,
+      high,
+      per_second,
+      &mut used,
+      inputs.len() * DECOYS
+    ).await?;
+
     let mut res = Vec::with_capacity(inputs.len());
-    for (i, o) in outputs.iter().enumerate() {
+    for o in outputs {
-      // If there's only the target amount of decoys available, remove the index of the output we're spending
+      // Grab the decoys for this specific output
-      // So we don't infinite loop while ignoring it
+      let mut ring = decoys.drain((decoys.len() - DECOYS) ..).collect::<Vec<_>>();
-      // TODO: If we're spending 2 outputs of a possible 11 outputs, this will still fail
+      ring.push(o);
-      used.remove(&o.0);
+      ring.sort_by(|a, b| a.0.cmp(&b.0));
 
-      // Select the full amount of ring members in decoys, instead of just the actual decoys, in order
+      // Sanity checks are only run when 1000 outputs are available in Monero
-      // to increase sample size
+      // We run this check whenever the highest output index, which we acknowledge, is > 500
-      let mut decoys = select_n(rng, rpc, height, &distribution, high, per_second, &mut used, DECOYS).await?;
+      // This means we assume (for presumably test blockchains) the height being used has not had
-      decoys.sort_by(|a, b| a.0.cmp(&b.0));
+      // 500 outputs since while itself not being a sufficiently mature blockchain
-
+      // Considering Monero's p2p layer doesn't actually check transaction sanity, it should be
-      // Add back this output
+      // fine for us to not have perfectly matching rules, especially since this code will infinite
-      used.insert(o.0);
+      // loop if it can't determine sanity, which is possible with sufficient inputs on sufficiently
-
+      // small chains
-      // Make sure the TX passes the sanity check that the median output is within the last 40%
+      if high > 500 {
-      // This actually checks the median is within the last third, a slightly more aggressive boundary,
+        // Make sure the TX passes the sanity check that the median output is within the last 40%
-      // as the height used in this calculation will be slightly under the height this is sanity
+        // This actually checks the median is within the last third, a slightly more aggressive
-      // checked against
+        // boundary, as the height used in this calculation will be slightly under the height this is
-      let target_median = high * 2 / 3;
+        // sanity checked against
-
+        let target_median = high * 2 / 3;
-      // Sanity checks are only run when 1000 outputs are available
+        while ring[RING_LEN / 2].0 < target_median {
-      // We run this check whenever it's possible to satisfy
-      // This means we need the middle possible decoy to be above the target_median
-      // TODO: This will break if timelocks are used other than maturity on very small chains/chains
-      // of any size which use timelocks extremely frequently, as it'll try to satisfy an impossible
-      // condition
-      // Reduce target_median by each timelocked output found?
-      if (high - MATURITY) >= target_median {
-        while decoys[DECOYS / 2].0 < target_median {
           // If it's not, update the bottom half with new values to ensure the median only moves up
-          for m in 0 .. DECOYS / 2 {
+          for removed in ring.drain(0 .. (RING_LEN / 2)).collect::<Vec<_>>() {
-            // We could not remove this, saving CPU time and removing low values as possibilities, yet
+            // If we removed the real spend, add it back
-            // it'd increase the amount of decoys required to create this transaction and some banned
+            if removed.0 == o.0 {
-            // outputs may be the best options
+              ring.push(o);
-            used.remove(&decoys[m].0);
+            } else {
+              // We could not remove this, saving CPU time and removing low values as possibilities, yet
+              // it'd increase the amount of decoys required to create this transaction and some removed
+              // outputs may be the best option (as we drop the first half, not just the bottom n)
+              used.remove(&removed.0);
+            }
           }
 
-          decoys.splice(
+          // Select new outputs until we have a full sized ring again
-            0 .. DECOYS / 2,
+          ring.extend(
-            select_n(rng, rpc, height, &distribution, high, per_second, &mut used, DECOYS / 2).await?
+            select_n(rng, rpc, height, &distribution, high, per_second, &mut used, RING_LEN - ring.len()).await?
           );
-          decoys.sort_by(|a, b| a.0.cmp(&b.0));
+          ring.sort_by(|a, b| a.0.cmp(&b.0));
         }
+
+        // The other sanity check rule is about duplicates, yet we already enforce unique ring members
       }
 
-      // Replace the closest selected decoy with the actual
-      let mut replace = 0;
-      let mut distance = u64::MAX;
-      for m in 0 .. decoys.len() {
-        let diff = decoys[m].0.abs_diff(o.0);
-        if diff < distance {
-          replace = m;
-          distance = diff;
-        }
-      }
-
-      decoys[replace] = outputs[i];
       res.push(Decoys {
-        i: u8::try_from(replace).unwrap(),
+        // Binary searches for the real spend since we don't know where it sorted to
-        offsets: offset(&decoys.iter().map(|output| output.0).collect::<Vec<_>>()),
+        i: u8::try_from(ring.partition_point(|x| x.0 < o.0)).unwrap(),
-        ring: decoys.iter().map(|output| output.1).collect()
+        offsets: offset(&ring.iter().map(|output| output.0).collect::<Vec<_>>()),
+        ring: ring.iter().map(|output| output.1).collect()
       });
     }
 

coins/monero/tests/rpc.rs

@@ -25,10 +25,8 @@ pub async fn rpc() -> Rpc {
     PublicKey { point: (&random_scalar(&mut OsRng) * &ED25519_BASEPOINT_TABLE).compress() }
   ).to_string();
 
-  // Mine enough blocks decoy selection doesn't fail
+  // Mine 10 blocks so we have 10 decoys so decoy selection doesn't fail
-  for _ in 0 .. 1 {
+  mine_block(&rpc, &addr).await.unwrap();
-    mine_block(&rpc, &addr).await.unwrap();
-  }
 
   rpc
 }

coins/monero/tests/send.rs

@@ -113,6 +113,15 @@ async fn send_core(test: usize, multisig: bool) {
         continue;
       }
 
+      // We actually need 80 decoys for this transaction, so mine until then
+      // 80 + 60 (miner TX maturity) + 10 (lock blocks)
+      // It is possible for this to be lower, by noting maturity is sufficient regardless of lock
+      // blocks, yet that's not currently implemented
+      // TODO, if we care
+      while rpc.get_height().await.unwrap() < 160 {
+        mine_block(&rpc, &addr.to_string()).await.unwrap();
+      }
+
       for i in (start + 1) .. (start + 9) {
         let tx = rpc.get_block_transactions(i).await.unwrap().swap_remove(0);
         let output = tx.scan(view, spend_pub).swap_remove(0);