-correctly implement CS idempotency check on withdraw

3 files changed, 105 insertions, 60 deletions

diff --git a/src/exchangedb/exchange-0001.sql b/src/exchangedb/exchange-0001.sql
index 66856f60..1111f381 100644
--- a/src/exchangedb/exchange-0001.sql
+++ b/src/exchangedb/exchange-0001.sql
@@ -196,7 +196,8 @@ CREATE INDEX IF NOT EXISTS reserves_close_by_reserve_pub_index
 
 CREATE TABLE IF NOT EXISTS reserves_out
   (reserve_out_serial_id BIGSERIAL -- UNIQUE
-  ,h_blind_ev BYTEA PRIMARY KEY CHECK (LENGTH(h_blind_ev)=64)
+  ,wih BYTEA PRIMARY KEY CHECK (LENGTH(wih)=64)
+  ,h_blind_ev BYTEA CHECK (LENGTH(h_blind_ev)=64) -- UNIQUE
   ,denominations_serial INT8 NOT NULL REFERENCES denominations (denominations_serial)
   ,denom_sig BYTEA NOT NULL
   ,reserve_uuid INT8 NOT NULL -- REFERENCES reserves (reserve_uuid) ON DELETE CASCADE
@@ -205,9 +206,11 @@ CREATE TABLE IF NOT EXISTS reserves_out
   ,amount_with_fee_val INT8 NOT NULL
   ,amount_with_fee_frac INT4 NOT NULL
   )
-  PARTITION BY HASH (h_blind_ev);
+  PARTITION BY HASH (wih);
 COMMENT ON TABLE reserves_out
   IS 'Withdraw operations performed on reserves.';
+COMMENT ON COLUMN reserves_out.wih
+  IS 'Hash that uniquely identifies the withdraw request. Used to detect request replays (crucial for CS) and to check the withdraw existed during recoup.';
 COMMENT ON COLUMN reserves_out.h_blind_ev
   IS 'Hash of the blinded coin, used as primary key here so that broken clients that use a non-random coin or blinding factor fail to withdraw (otherwise they would fail on deposit when the coin is not unique there).';
 COMMENT ON COLUMN reserves_out.denominations_serial
@@ -637,7 +640,7 @@ COMMENT ON TABLE recoup
 COMMENT ON COLUMN recoup.known_coin_id
   IS 'Coin that is being debited in the recoup. Do not CASCADE ON DROP on the coin_pub, as we may keep the coin alive!';
 COMMENT ON COLUMN recoup.reserve_out_serial_id
-  IS 'Identifies the h_blind_ev of the recouped coin and provides the link to the credited reserve.';
+  IS 'Identifies the wih of the recouped coin and provides the link to the credited reserve.';
 COMMENT ON COLUMN recoup.coin_sig
   IS 'Signature by the coin affirming the recoup, of type TALER_SIGNATURE_WALLET_COIN_RECOUP';
 COMMENT ON COLUMN recoup.coin_blind
@@ -812,6 +815,7 @@ CREATE INDEX IF NOT EXISTS revolving_work_shards_by_job_name_active_last_attempt
 
 
 CREATE OR REPLACE FUNCTION exchange_do_withdraw(
+  IN in_wih BYTEA,
   IN amount_val INT8,
   IN amount_frac INT4,
   IN h_denom_pub BYTEA,
@@ -825,7 +829,8 @@ CREATE OR REPLACE FUNCTION exchange_do_withdraw(
   OUT balance_ok BOOLEAN,
   OUT kycok BOOLEAN,
   OUT account_uuid INT8,
-  OUT ruuid INT8)
+  OUT ruuid INT8,
+  OUT out_denom_sig BYTEA)
 LANGUAGE plpgsql
 AS $$
 DECLARE
@@ -838,7 +843,7 @@ DECLARE
   reserve_frac INT4;
 BEGIN
 -- Shards: reserves by reserve_pub (SELECT)
---         reserves_out (INSERT, with CONFLICT detection) by h_blind_ev
+--         reserves_out (INSERT, with CONFLICT detection) by wih
 --         reserves by reserve_pub (UPDATE)
 --         reserves_in by reserve_pub (SELECT)
 --         wire_targets by wire_target_serial_id
@@ -887,6 +892,7 @@ END IF;
 -- the query successful due to idempotency.
 INSERT INTO reserves_out
   (h_blind_ev
+  ,wih
   ,denominations_serial
   ,denom_sig
   ,reserve_uuid
@@ -896,6 +902,7 @@ INSERT INTO reserves_out
   ,amount_with_fee_frac)
 VALUES
   (h_coin_envelope
+  ,in_wih
   ,denom_serial
   ,denom_sig
   ,ruuid
@@ -908,6 +915,25 @@ ON CONFLICT DO NOTHING;
 IF NOT FOUND
 THEN
   -- idempotent query, all constraints must be satisfied
+
+  SELECT
+     denom_sig
+  INTO
+     out_denom_sig
+  FROM reserves_in
+    WHERE wih=in_wih
+    LIMIT 1; -- limit 1 should not be required (without p2p transfers)
+
+  IF NOT FOUND
+  THEN
+    reserve_found=FALSE;
+    balance_ok=FALSE;
+    kycok=FALSE;
+    account_uuid=0;
+    ruuid=0;
+    ASSERT false, 'internal logic error';
+  END IF;
+
   reserve_found=TRUE;
   balance_ok=TRUE;
   kycok=TRUE;
@@ -967,9 +993,13 @@ SELECT
  WHERE reserve_pub=rpub
  LIMIT 1; -- limit 1 should not be required (without p2p transfers)
 
+-- Return denomination signature as result that
+-- was given as the argument.
+out_denom_sig=denom_sig;
+
 END $$;
 
-COMMENT ON FUNCTION exchange_do_withdraw(INT8, INT4, BYTEA, BYTEA, BYTEA, BYTEA, BYTEA, INT8, INT8)
+COMMENT ON FUNCTION exchange_do_withdraw(BYTEA, INT8, INT4, BYTEA, BYTEA, BYTEA, BYTEA, BYTEA, INT8, INT8)
   IS 'Checks whether the reserve has sufficient balance for a withdraw operation (or the request is repeated and was previously approved) and if so updates the database with the result';
 
 
diff --git a/src/exchangedb/plugin_exchangedb_postgres.c b/src/exchangedb/plugin_exchangedb_postgres.c
index ce184f48..98724fa0 100644
--- a/src/exchangedb/plugin_exchangedb_postgres.c
+++ b/src/exchangedb/plugin_exchangedb_postgres.c
@@ -560,13 +560,6 @@ prepare_statements (struct PostgresClosure *pg)
       "   ON (wire_source_serial_id = wire_target_serial_id)"
       " WHERE reserve_pub=$1;",
       1),
-    /* Lock withdraw table; NOTE: we may want to eventually shard the
-       deposit table to avoid this lock being the main point of
-       contention limiting transaction performance. */
-    GNUNET_PQ_make_prepare (
-      "lock_withdraw",
-      "LOCK TABLE reserves_out;",
-      0),
     /* Used in #postgres_do_withdraw() to store
        the signature of a blinded coin with the blinded coin's
        details before returning it during /reserve/withdraw. We store
@@ -582,9 +575,10 @@ prepare_statements (struct PostgresClosure *pg)
       ",kycok AS kyc_ok"
       ",account_uuid AS payment_target_uuid"
       ",ruuid"
+      ",out_denom_sig"
       " FROM exchange_do_withdraw"
-      " ($1,$2,$3,$4,$5,$6,$7,$8,$9);",
-      9),
+      " ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10);",
+      10),
     /* Used in #postgres_do_withdraw_limit_check() to check
        if the withdrawals remain below the limit under which
        KYC is not required. */
@@ -659,6 +653,7 @@ prepare_statements (struct PostgresClosure *pg)
       ",reserve_sig"
       ",reserves.reserve_pub"
       ",execution_date"
+      ",h_blind_ev"
       ",amount_with_fee_val"
       ",amount_with_fee_frac"
       ",denom.fee_withdraw_val"
@@ -668,7 +663,7 @@ prepare_statements (struct PostgresClosure *pg)
       "      USING (reserve_uuid)"
       "    JOIN denominations denom"
       "      USING (denominations_serial)"
-      " WHERE h_blind_ev=$1;",
+      " WHERE wih=$1;",
       1),
     /* Used during #postgres_get_reserve_history() to
        obtain all of the /reserve/withdraw operations that
@@ -1671,16 +1666,16 @@ prepare_statements (struct PostgresClosure *pg)
       "      ON (denoms.denominations_serial = coins.denominations_serial)"
       " WHERE coins.coin_pub=$1;",
       1),
-    /* Used in #postgres_get_reserve_by_h_blind() */
+    /* Used in #postgres_get_reserve_by_wih() */
     GNUNET_PQ_make_prepare (
-      "reserve_by_h_blind",
+      "reserve_by_wih",
       "SELECT"
       " reserves.reserve_pub"
       ",reserve_out_serial_id"
       " FROM reserves_out"
       " JOIN reserves"
       "   USING (reserve_uuid)"
-      " WHERE h_blind_ev=$1"
+      " WHERE wih=$1"
       " LIMIT 1;",
       1),
     /* Used in #postgres_get_old_coin_by_h_blind() */
@@ -4304,8 +4299,7 @@ postgres_reserves_in_insert (void *cls,
  * key of the hash of the blinded message.
  *
  * @param cls the `struct PostgresClosure` with the plugin-specific state
- * @param h_blind hash of the blinded coin to be signed (will match
- *                `h_coin_envelope` in the @a collectable to be returned)
+ * @param wih hash that uniquely identifies the withdraw operation
  * @param collectable corresponding collectable coin (blind signature)
  *                    if a coin is found
  * @return statement execution status
@@ -4313,12 +4307,12 @@ postgres_reserves_in_insert (void *cls,
 static enum GNUNET_DB_QueryStatus
 postgres_get_withdraw_info (
   void *cls,
-  const struct TALER_BlindedCoinHash *h_blind,
+  const struct TALER_WithdrawIdentificationHash *wih,
   struct TALER_EXCHANGEDB_CollectableBlindcoin *collectable)
 {
   struct PostgresClosure *pg = cls;
   struct GNUNET_PQ_QueryParam params[] = {
-    GNUNET_PQ_query_param_auto_from_type (h_blind),
+    GNUNET_PQ_query_param_auto_from_type (wih),
     GNUNET_PQ_query_param_end
   };
   struct GNUNET_PQ_ResultSpec rs[] = {
@@ -4330,24 +4324,15 @@ postgres_get_withdraw_info (
                                           &collectable->reserve_sig),
     GNUNET_PQ_result_spec_auto_from_type ("reserve_pub",
                                           &collectable->reserve_pub),
+    GNUNET_PQ_result_spec_auto_from_type ("h_blind_ev",
+                                          &collectable->h_coin_envelope),
     TALER_PQ_RESULT_SPEC_AMOUNT ("amount_with_fee",
                                  &collectable->amount_with_fee),
     TALER_PQ_RESULT_SPEC_AMOUNT ("fee_withdraw",
                                  &collectable->withdraw_fee),
     GNUNET_PQ_result_spec_end
   };
-#if EXPLICIT_LOCKS
-  struct GNUNET_PQ_QueryParam no_params[] = {
-    GNUNET_PQ_query_param_end
-  };
-  enum GNUNET_DB_QueryStatus qs;
 
-  if (0 > (qs = GNUNET_PQ_eval_prepared_non_select (pg->conn,
-                                                    "lock_withdraw",
-                                                    no_params)))
-    return qs;
-#endif
-  collectable->h_coin_envelope = *h_blind;
   return GNUNET_PQ_eval_prepared_singleton_select (pg->conn,
                                                    "get_withdraw_info",
                                                    params,
@@ -4360,8 +4345,8 @@ postgres_get_withdraw_info (
  * and possibly persisting the withdrawal details.
  *
  * @param cls the `struct PostgresClosure` with the plugin-specific state
- * @param collectable corresponding collectable coin (blind signature)
- *                    if a coin is found
+ * @param wih hash that uniquely identifies the withdraw operation
+ * @param[in,out] collectable corresponding collectable coin (blind signature) if a coin is found; possibly updated if a (different) signature exists already
  * @param now current time (rounded)
  * @param[out] found set to true if the reserve was found
  * @param[out] balance_ok set to true if the balance was sufficient
@@ -4372,7 +4357,8 @@ postgres_get_withdraw_info (
 static enum GNUNET_DB_QueryStatus
 postgres_do_withdraw (
   void *cls,
-  const struct TALER_EXCHANGEDB_CollectableBlindcoin *collectable,
+  const struct TALER_WithdrawIdentificationHash *wih,
+  struct TALER_EXCHANGEDB_CollectableBlindcoin *collectable,
   struct GNUNET_TIME_Timestamp now,
   bool *found,
   bool *balance_ok,
@@ -4382,6 +4368,7 @@ postgres_do_withdraw (
   struct PostgresClosure *pg = cls;
   struct GNUNET_TIME_Timestamp gc;
   struct GNUNET_PQ_QueryParam params[] = {
+    GNUNET_PQ_query_param_auto_from_type (wih),
     TALER_PQ_query_param_amount (&collectable->amount_with_fee),
     GNUNET_PQ_query_param_auto_from_type (&collectable->denom_pub_hash),
     GNUNET_PQ_query_param_auto_from_type (&collectable->reserve_pub),
@@ -4392,6 +4379,9 @@ postgres_do_withdraw (
     GNUNET_PQ_query_param_timestamp (&gc),
     GNUNET_PQ_query_param_end
   };
+  enum GNUNET_DB_QueryStatus qs;
+  bool no_out_sig;
+  struct TALER_BlindedDenominationSignature out_sig;
   struct GNUNET_PQ_ResultSpec rs[] = {
     GNUNET_PQ_result_spec_bool ("reserve_found",
                                 found),
@@ -4403,18 +4393,33 @@ postgres_do_withdraw (
                                   &kyc->payment_target_uuid),
     GNUNET_PQ_result_spec_uint64 ("ruuid",
                                   ruuid),
+    GNUNET_PQ_result_spec_allow_null (
+      TALER_PQ_result_spec_blinded_denom_sig ("out_denom_sig",
+                                              &out_sig),
+      &no_out_sig),
     GNUNET_PQ_result_spec_end
   };
 
+#if 0
+  memset (&out_sig,
+          0,
+          sizeof (out_sig));
+#endif
   gc = GNUNET_TIME_absolute_to_timestamp (
     GNUNET_TIME_absolute_add (now.abs_time,
                               pg->legal_reserve_expiration_time));
   kyc->type = TALER_EXCHANGEDB_KYC_WITHDRAW;
-  return GNUNET_PQ_eval_prepared_singleton_select (pg->conn,
-                                                   "call_withdraw",
-                                                   params,
-                                                   rs);
-
+  qs = GNUNET_PQ_eval_prepared_singleton_select (pg->conn,
+                                                 "call_withdraw",
+                                                 params,
+                                                 rs);
+  if ( (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT == qs) &&
+       (! no_out_sig) )
+  {
+    TALER_blinded_denom_sig_free (&collectable->sig);
+    collectable->sig = out_sig;
+  }
+  return qs;
 }
 
 
@@ -9373,20 +9378,21 @@ postgres_select_reserve_closed_above_serial_id (
  * from given the hash of the blinded coin.
  *
  * @param cls closure
- * @param h_blind_ev hash of the blinded coin
+ * @param wih hash that uniquely identifies the withdraw request
  * @param[out] reserve_pub set to information about the reserve (on success only)
  * @param[out] reserve_out_serial_id set to row of the @a h_blind_ev in reserves_out
  * @return transaction status code
  */
 static enum GNUNET_DB_QueryStatus
-postgres_get_reserve_by_h_blind (void *cls,
-                                 const struct TALER_BlindedCoinHash *h_blind_ev,
-                                 struct TALER_ReservePublicKeyP *reserve_pub,
-                                 uint64_t *reserve_out_serial_id)
+postgres_get_reserve_by_wih (
+  void *cls,
+  const struct TALER_WithdrawIdentificationHash *wih,
+  struct TALER_ReservePublicKeyP *reserve_pub,
+  uint64_t *reserve_out_serial_id)
 {
   struct PostgresClosure *pg = cls;
   struct GNUNET_PQ_QueryParam params[] = {
-    GNUNET_PQ_query_param_auto_from_type (h_blind_ev),
+    GNUNET_PQ_query_param_auto_from_type (wih),
     GNUNET_PQ_query_param_end
   };
   struct GNUNET_PQ_ResultSpec rs[] = {
@@ -9398,7 +9404,7 @@ postgres_get_reserve_by_h_blind (void *cls,
   };
 
   return GNUNET_PQ_eval_prepared_singleton_select (pg->conn,
-                                                   "reserve_by_h_blind",
+                                                   "reserve_by_wih",
                                                    params,
                                                    rs);
 }
@@ -11663,8 +11669,8 @@ libtaler_plugin_exchangedb_postgres_init (void *cls)
     = &postgres_select_recoup_refresh_above_serial_id;
   plugin->select_reserve_closed_above_serial_id
     = &postgres_select_reserve_closed_above_serial_id;
-  plugin->get_reserve_by_h_blind
-    = &postgres_get_reserve_by_h_blind;
+  plugin->get_reserve_by_wih
+    = &postgres_get_reserve_by_wih;
   plugin->get_old_coin_by_h_blind
     = &postgres_get_old_coin_by_h_blind;
   plugin->insert_denomination_revocation
diff --git a/src/exchangedb/test_exchangedb.c b/src/exchangedb/test_exchangedb.c
index 9561df12..0622e069 100644
--- a/src/exchangedb/test_exchangedb.c
+++ b/src/exchangedb/test_exchangedb.c
@@ -1346,6 +1346,7 @@ run (void *cls)
   struct GNUNET_TIME_Timestamp now;
   struct TALER_WireSaltP salt;
   struct TALER_CoinPubHash c_hash;
+  struct TALER_WithdrawIdentificationHash wih;
   uint64_t known_coin_id;
   uint64_t rrc_serial;
   struct TALER_EXCHANGEDB_Refresh refresh;
@@ -1383,7 +1384,7 @@ run (void *cls)
       plugin->create_tables (plugin->cls))
   {
     result = 77;
-    goto drop;
+    goto cleanup;
   }
   plugin->preflight (plugin->cls);
   FAILIF (GNUNET_OK !=
@@ -1499,9 +1500,14 @@ run (void *cls)
                                          &cbc.denom_pub_hash,
                                          &cbc.h_coin_envelope));
       GNUNET_assert (GNUNET_OK ==
-                     TALER_denom_sign_blinded (&cbc.sig,
-                                               &dkp->priv,
-                                               &pd.blinded_planchet));
+                     TALER_withdraw_request_hash (&pd.blinded_planchet,
+                                                  &cbc.denom_pub_hash,
+                                                  &wih));      GNUNET_assert (
+        GNUNET_OK ==
+        TALER_denom_sign_blinded (
+          &cbc.sig,
+          &dkp->priv,
+          &pd.blinded_planchet));
       TALER_blinded_planchet_free (&pd.blinded_planchet);
     }
   }
@@ -1511,6 +1517,7 @@ run (void *cls)
   GNUNET_assert (GNUNET_OK ==
                  TALER_amount_set_zero (CURRENCY,
                                         &cbc.withdraw_fee));
+
   {
     bool found;
     bool balance_ok;
@@ -1519,6 +1526,7 @@ run (void *cls)
 
     FAILIF (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT !=
             plugin->do_withdraw (plugin->cls,
+                                 &wih,
                                  &cbc,
                                  now,
                                  &found,
@@ -1540,16 +1548,16 @@ run (void *cls)
                          value.fraction,
                          value.currency));
   FAILIF (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT !=
-          plugin->get_reserve_by_h_blind (plugin->cls,
-                                          &cbc.h_coin_envelope,
-                                          &reserve_pub3,
-                                          &reserve_out_serial_id));
+          plugin->get_reserve_by_wih (plugin->cls,
+                                      &wih,
+                                      &reserve_pub3,
+                                      &reserve_out_serial_id));
   FAILIF (0 != GNUNET_memcmp (&reserve_pub,
                               &reserve_pub3));
 
   FAILIF (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT !=
           plugin->get_withdraw_info (plugin->cls,
-                                     &cbc.h_coin_envelope,
+                                     &wih,
                                      &cbc2));
   FAILIF (0 != GNUNET_memcmp (&cbc2.reserve_sig,
                               &cbc.reserve_sig));
@@ -2400,6 +2408,7 @@ drop:
   rh = NULL;
   GNUNET_break (GNUNET_OK ==
                 plugin->drop_tables (plugin->cls));
+cleanup:
   if (NULL != dkp)
     destroy_denom_key_pair (dkp);
   if (NULL != revealed_coins)