[taler-wallet-core] branch master updated: fix totally broken p2p coin selection

[gnunet]

This is an automated email from the git hooks/post-receive script.

dold pushed a commit to branch master
in repository wallet-core.

The following commit(s) were added to refs/heads/master by this push:
     new 695a6a43e fix totally broken p2p coin selection
695a6a43e is described below

commit 695a6a43ea143475b2dddd070a2e16680b2bc9c7
Author: Florian Dold <florian@dold.me>
AuthorDate: Mon Mar 4 23:34:17 2024 +0100

    fix totally broken p2p coin selection
---
 packages/taler-util/src/http-impl.node.ts          |  11 +--
 .../taler-wallet-core/src/coinSelection.test.ts    | 101 ++++++++++++++++++---
 packages/taler-wallet-core/src/coinSelection.ts    |  79 ++++++++--------
 3 files changed, 129 insertions(+), 62 deletions(-)

diff --git a/packages/taler-util/src/http-impl.node.ts 
b/packages/taler-util/src/http-impl.node.ts
index ea2309504..b5c87843f 100644
--- a/packages/taler-util/src/http-impl.node.ts
+++ b/packages/taler-util/src/http-impl.node.ts
@@ -19,12 +19,8 @@
 /**
  * Imports.
  */
-import {
-  FollowOptions,
-  RedirectableRequest,
-  http,
-  https,
-} from "follow-redirects";
+import type { FollowOptions, RedirectableRequest } from "follow-redirects";
+import followRedirects from "follow-redirects";
 import type { ClientRequest, IncomingMessage } from "node:http";
 import { RequestOptions } from "node:http";
 import * as net from "node:net";
@@ -45,6 +41,9 @@ import {
   typedArrayConcat,
 } from "./index.js";
 
+const http = followRedirects.http;
+const https = followRedirects.https;
+
 // Work around a node v20.0.0, v20.1.0, and v20.2.0 bug. The issue was fixed
 // in v20.3.0.
 // https://github.com/nodejs/node/issues/47822#issuecomment-1564708870
diff --git a/packages/taler-wallet-core/src/coinSelection.test.ts 
b/packages/taler-wallet-core/src/coinSelection.test.ts
index 839cd22fb..4fac244fc 100644
--- a/packages/taler-wallet-core/src/coinSelection.test.ts
+++ b/packages/taler-wallet-core/src/coinSelection.test.ts
@@ -24,6 +24,7 @@ import {
 import test from "ava";
 import {
   AvailableDenom,
+  PeerCoinSelectionTally,
   testing_greedySelectPeer,
   testing_selectGreedy,
 } from "./coinSelection.js";
@@ -42,10 +43,10 @@ const inThePast = AbsoluteTime.toProtocolTimestamp(
 test("p2p: should select the coin", (t) => {
   const instructedAmount = Amounts.parseOrThrow("LOCAL:2");
   const tally = {
-    amountAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
+    amountRemaining: instructedAmount,
     depositFeesAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
     lastDepositFee: Amounts.zeroOfCurrency(instructedAmount.currency),
-  };
+  } satisfies PeerCoinSelectionTally;
   const coins = testing_greedySelectPeer(
     createCandidates([
       {
@@ -55,7 +56,6 @@ test("p2p: should select the coin", (t) => {
         fromExchange: "http://exchange.localhost/";,
       },
     ]),
-    instructedAmount,
     tally,
   );
 
@@ -75,7 +75,7 @@ test("p2p: should select the coin", (t) => {
   });
 
   t.deepEqual(tally, {
-    amountAcc: Amounts.parseOrThrow("LOCAL:2"),
+    amountRemaining: Amounts.parseOrThrow("LOCAL:0"),
     depositFeesAcc: Amounts.parseOrThrow("LOCAL:0.1"),
     lastDepositFee: Amounts.parseOrThrow("LOCAL:0.1"),
   });
@@ -84,10 +84,10 @@ test("p2p: should select the coin", (t) => {
 test("p2p: should select 3 coins", (t) => {
   const instructedAmount = Amounts.parseOrThrow("LOCAL:20");
   const tally = {
-    amountAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
+    amountRemaining: instructedAmount,
     depositFeesAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
     lastDepositFee: Amounts.zeroOfCurrency(instructedAmount.currency),
-  };
+  } satisfies PeerCoinSelectionTally;
   const coins = testing_greedySelectPeer(
     createCandidates([
       {
@@ -97,7 +97,6 @@ test("p2p: should select 3 coins", (t) => {
         fromExchange: "http://exchange.localhost/";,
       },
     ]),
-    instructedAmount,
     tally,
   );
 
@@ -107,9 +106,9 @@ test("p2p: should select 3 coins", (t) => {
       denomPubHash: "hash0",
       maxAge: 32,
       contributions: [
-        Amounts.parseOrThrow("LOCAL:9.9"),
-        Amounts.parseOrThrow("LOCAL:9.9"),
-        Amounts.parseOrThrow("LOCAL:0.5"),
+        Amounts.parseOrThrow("LOCAL:10"),
+        Amounts.parseOrThrow("LOCAL:10"),
+        Amounts.parseOrThrow("LOCAL:0.3"),
       ],
       expireDeposit: inTheDistantFuture,
       expireWithdraw: inTheDistantFuture,
@@ -117,7 +116,7 @@ test("p2p: should select 3 coins", (t) => {
   });
 
   t.deepEqual(tally, {
-    amountAcc: Amounts.parseOrThrow("LOCAL:20"),
+    amountRemaining: Amounts.parseOrThrow("LOCAL:0"),
     depositFeesAcc: Amounts.parseOrThrow("LOCAL:0.3"),
     lastDepositFee: Amounts.parseOrThrow("LOCAL:0.1"),
   });
@@ -126,10 +125,10 @@ test("p2p: should select 3 coins", (t) => {
 test("p2p: can't select since the instructed amount is too high", (t) => {
   const instructedAmount = Amounts.parseOrThrow("LOCAL:60");
   const tally = {
-    amountAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
+    amountRemaining: instructedAmount,
     depositFeesAcc: Amounts.zeroOfCurrency(instructedAmount.currency),
     lastDepositFee: Amounts.zeroOfCurrency(instructedAmount.currency),
-  };
+  } satisfies PeerCoinSelectionTally;
   const coins = testing_greedySelectPeer(
     createCandidates([
       {
@@ -139,14 +138,13 @@ test("p2p: can't select since the instructed amount is 
too high", (t) => {
         fromExchange: "http://exchange.localhost/";,
       },
     ]),
-    instructedAmount,
     tally,
   );
 
   t.is(coins, undefined);
 
   t.deepEqual(tally, {
-    amountAcc: Amounts.parseOrThrow("LOCAL:49"),
+    amountRemaining: Amounts.parseOrThrow("LOCAL:10.5"),
     depositFeesAcc: Amounts.parseOrThrow("LOCAL:0.5"),
     lastDepositFee: Amounts.parseOrThrow("LOCAL:0.1"),
   });
@@ -246,3 +244,76 @@ function createCandidates(
     };
   });
 }
+
+test("p2p: regression STATER", (t) => {
+  const candidates = [
+    {
+      denomPub: {
+        age_mask: 349441,
+        cipher: "RSA",
+        rsa_public_key:
+          
"040000WTR9ERP6FYDM4581C1WY4DX6EA6ZP0RKDEY1VCEG1HGZQDB1E1MT0HSPWKVWYY8GN99YG8JV2BQHCV608V3AP00HZ44M4R2RDK3MEG1HY3H5VP2YESFDXC8C2J0BT6E662JJYN4MCFR8Q8ZFD7ZCA8HGBNVG4JMTS5MBDTF9CX3JC25H702K1FG2C54HR48767D18F2H11HMVK7EEF51QRGE08T704VRCNZ6WTM3Z73Z5DW4W26GBEWTDZZ4HX94HRJEH8YENXAW5T5E39TQQN7MZ7HEPB59BQWB0DDMM8MAE274BV3HC2AJVCSXFJSKBAK1B9HKERPWF7Z5556VJG6YJ9236G5SFM3RC22PJM2SXHYBWFV1WBAYF1F2026C0CM5Q3RPQETHCWZTEX8KJ2J1K904002",
+      },
+      denomPubHash:
+        
"TF5S4VJ8P3NN0SM5R1KW5MP665KEFMGAT2RPR70BMG0WQ5A72J53GDDE0YSCTWEXHRW8FMMX3X27RQK4D1VH69GVJBYR5RSJY3X5FS8",
+      feeDeposit: "STATER:1",
+      feeRefresh: "STATER:0",
+      feeRefund: "STATER:0",
+      feeWithdraw: "STATER:0",
+      stampExpireDeposit: {
+        t_s: 1772722025,
+      },
+      stampExpireLegal: {
+        t_s: 1961938025,
+      },
+      stampExpireWithdraw: {
+        t_s: 1709650025,
+      },
+      stampStart: {
+        t_s: 1709045225,
+      },
+      value: "STATER:2",
+      exchangeBaseUrl: "https://exchange.taler.grothoff.org/";,
+      numAvailable: 6,
+      maxAge: 32,
+    },
+    {
+      denomPub: {
+        age_mask: 349441,
+        cipher: "RSA",
+        rsa_public_key:
+          
"040000Y84BTTQCZ28AS2KZ867V05WES3YPN34X51DNF14ADGW2HNG9YFXCCNVQ2JA9ZT3KSBD17ZN9Y71KGWAWEFYMHE0S61DW63WN58VWRXQ92440V1JSZDD7FDTYEVNGG8ZVARVZ4GGF1RCDM93R28M067S5CPRZFCCQBRFFM9YDK2W06WDXE96BDCB8MZEYPHSGK5CTDY6XJE18EMRWYRBAG0H8P6QGQS73REXX66PTJ3MRX3AK3ARZF8417QKMZZPNS1JV5EYPAC7X8R1F9G1GWAQXVVQ2XTA5NMVMNJDJ0KEM93AXD4W2C7XMVJFSQN8RVB9KZ8JXWGN1YJQK7P6476HV896THKQ05QK4F0C65P4HA7QDX84C91F42PZVMH8AMYMA2NBXEYXS0EV8NXZHMZ30JF04002",
+      },
+      denomPubHash:
+        
"WCMKBGR8ZKJ62YZXCRNT3EHPFQQ2M0B5CGZXW0PYA76G8PPXJMXZ7Q3WBP2DA3Z4BF21K3X9AG769RYCC39C3PT0R1DCTJA2PRTSHSR",
+      feeDeposit: "STATER:1",
+      feeRefresh: "STATER:0",
+      feeRefund: "STATER:0",
+      feeWithdraw: "STATER:0",
+      stampExpireDeposit: {
+        t_s: 1772722025,
+      },
+      stampExpireLegal: {
+        t_s: 1961938025,
+      },
+      stampExpireWithdraw: {
+        t_s: 1709650025,
+      },
+      stampStart: {
+        t_s: 1709045225,
+      },
+      value: "STATER:1",
+      exchangeBaseUrl: "https://exchange.taler.grothoff.org/";,
+      numAvailable: 1,
+      maxAge: 32,
+    },
+  ];
+  const instructedAmount = Amounts.parseOrThrow("STATER:1");
+  const tally = {
+    amountRemaining: instructedAmount,
+    depositFeesAcc: Amounts.parseOrThrow("STATER:0"),
+    lastDepositFee: Amounts.parseOrThrow("STATER:0"),
+  } satisfies PeerCoinSelectionTally;
+  const res = testing_greedySelectPeer(candidates as any, tally);
+  t.assert(!!res);
+});
diff --git a/packages/taler-wallet-core/src/coinSelection.ts 
b/packages/taler-wallet-core/src/coinSelection.ts
index 680e5faa1..3ece5546c 100644
--- a/packages/taler-wallet-core/src/coinSelection.ts
+++ b/packages/taler-wallet-core/src/coinSelection.ts
@@ -32,7 +32,6 @@ import {
   AllowedAuditorInfo,
   AllowedExchangeInfo,
   AmountJson,
-  AmountLike,
   Amounts,
   AmountString,
   checkDbInvariant,
@@ -64,11 +63,7 @@ import { getAutoRefreshExecuteThreshold } from "./common.js";
 import { DenominationRecord, WalletDbReadOnlyTransaction } from "./db.js";
 import { isWithdrawableDenom } from "./denominations.js";
 import { getExchangeWireDetailsInTx } from "./exchanges.js";
-import {
-  getDenomInfo,
-  InternalWalletState,
-  WalletExecutionContext,
-} from "./wallet.js";
+import { getDenomInfo, WalletExecutionContext } from "./wallet.js";
 
 const logger = new Logger("coinSelection.ts");
 
@@ -927,6 +922,9 @@ async function selectPayPeerCandidatesForExchange(
     );
 
   for (const coinAvail of myExchangeCoins) {
+    if (coinAvail.freshCoinCount <= 0) {
+      continue;
+    }
     const denom = await tx.denominations.get([
       coinAvail.exchangeBaseUrl,
       coinAvail.denomPubHash,
@@ -954,8 +952,8 @@ async function selectPayPeerCandidatesForExchange(
   return denoms;
 }
 
-interface PeerCoinSelectionTally {
-  amountAcc: AmountJson;
+export interface PeerCoinSelectionTally {
+  amountRemaining: AmountJson;
   depositFeesAcc: AmountJson;
   lastDepositFee: AmountJson;
 }
@@ -971,40 +969,37 @@ export function testing_greedySelectPeer(
 
 function greedySelectPeer(
   candidates: AvailableDenom[],
-  instructedAmount: AmountLike,
   tally: PeerCoinSelectionTally,
 ): SelResult | undefined {
   const selectedDenom: SelResult = {};
   for (const denom of candidates) {
     const contributions: AmountJson[] = [];
+    const feeDeposit = Amounts.parseOrThrow(denom.feeDeposit);
     for (
       let i = 0;
-      i < denom.numAvailable &&
-      Amounts.cmp(tally.amountAcc, instructedAmount) < 0;
+      i < denom.numAvailable && Amounts.isNonZero(tally.amountRemaining);
       i++
     ) {
-      const amountPayRemaining = Amounts.sub(
-        instructedAmount,
-        tally.amountAcc,
+      tally.depositFeesAcc = Amounts.add(
+        tally.depositFeesAcc,
+        feeDeposit,
+      ).amount;
+      tally.amountRemaining = Amounts.add(
+        tally.amountRemaining,
+        feeDeposit,
       ).amount;
-      // Maximum amount the coin could effectively contribute.
-      const maxCoinContrib = Amounts.sub(denom.value, denom.feeDeposit).amount;
+      tally.lastDepositFee = feeDeposit;
 
-      const coinSpend = Amounts.min(
-        Amounts.add(amountPayRemaining, denom.feeDeposit).amount,
-        maxCoinContrib,
+      const coinSpend = Amounts.max(
+        Amounts.min(tally.amountRemaining, denom.value),
+        denom.feeDeposit,
       );
 
-      tally.amountAcc = Amounts.add(tally.amountAcc, coinSpend).amount;
-      tally.amountAcc = Amounts.sub(tally.amountAcc, denom.feeDeposit).amount;
-
-      tally.depositFeesAcc = Amounts.add(
-        tally.depositFeesAcc,
-        denom.feeDeposit,
+      tally.amountRemaining = Amounts.sub(
+        tally.amountRemaining,
+        coinSpend,
       ).amount;
 
-      tally.lastDepositFee = Amounts.parseOrThrow(denom.feeDeposit);
-
       contributions.push(coinSpend);
     }
     if (contributions.length > 0) {
@@ -1027,14 +1022,12 @@ function greedySelectPeer(
       sd.contributions.push(...contributions);
       selectedDenom[avKey] = sd;
     }
-    if (Amounts.cmp(tally.amountAcc, instructedAmount) >= 0) {
-      break;
-    }
   }
 
-  if (Amounts.cmp(tally.amountAcc, instructedAmount) >= 0) {
+  if (Amounts.isZero(tally.amountRemaining)) {
     return selectedDenom;
   }
+
   return undefined;
 }
 
@@ -1071,8 +1064,11 @@ export async function selectPeerCoins(
           tx,
           exch.baseUrl,
         );
+        if (logger.shouldLogTrace()) {
+          logger.trace(`peer payment candidate coins: ${j2s(candidates)}`);
+        }
         const tally: PeerCoinSelectionTally = {
-          amountAcc: Amounts.zeroOfCurrency(currency),
+          amountRemaining: Amounts.parseOrThrow(instructedAmount),
           depositFeesAcc: Amounts.zeroOfCurrency(currency),
           lastDepositFee: Amounts.zeroOfCurrency(currency),
         };
@@ -1109,8 +1105,8 @@ export async function selectPeerCoins(
             });
             const depositFee = Amounts.parseOrThrow(denom.feeDeposit);
             tally.lastDepositFee = depositFee;
-            tally.amountAcc = Amounts.add(
-              tally.amountAcc,
+            tally.amountRemaining = Amounts.sub(
+              tally.amountRemaining,
               Amounts.sub(contrib, depositFee).amount,
             ).amount;
             tally.depositFeesAcc = Amounts.add(
@@ -1120,11 +1116,13 @@ export async function selectPeerCoins(
           }
         }
 
-        const selectedDenom = greedySelectPeer(
-          candidates,
-          instructedAmount,
-          tally,
-        );
+        if (logger.shouldLogTrace()) {
+          logger.trace(`candidates: ${j2s(candidates)}`);
+          logger.trace(`instructedAmount: ${j2s(instructedAmount)}`);
+          logger.trace(`tally: ${j2s(tally)}`);
+        }
+
+        const selectedDenom = greedySelectPeer(candidates, tally);
 
         if (selectedDenom) {
           let minAutorefreshExecuteThreshold = TalerProtocolTimestamp.never();
@@ -1180,10 +1178,9 @@ export async function selectPeerCoins(
           return { type: "success", result: res };
         }
 
-        const diff = Amounts.sub(instructedAmount, tally.amountAcc).amount;
         exchangeFeeGap[exch.baseUrl] = Amounts.add(
           tally.lastDepositFee,
-          diff,
+          tally.amountRemaining,
         ).amount;
 
         continue;

-- 
To stop receiving notification emails like this one, please contact
gnunet@gnunet.org.