View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0006200 | Taler | exchange | public | 2020-04-22 20:59 | 2021-09-02 18:14 |
Reporter | oec | Assigned To | Christian Grothoff | ||
Priority | normal | Severity | tweak | Reproducibility | have not tried |
Status | closed | Resolution | fixed | ||
Product Version | git (master) | ||||
Target Version | 0.7.1 | Fixed in Version | 0.7.1 | ||
Summary | 0006200: postgres_get_coin_transactions does not allow for multiple deposit transactions | ||||
Description | If I understand correctly, a coin can be used in multiple deposit-transactions. The exchange has to ensure that all transactions involving the same coin do not exceed the total value of the coin. For this purpose, the deposit handler calls TEH_plugin->get_coin_transactions which is basically a call to postgres_get_coin_transactions. That function calls a list of prepared-statements and the results are handled by callbacks: (from exchange/src/exchangedb/plugin_exchangedb_postgres.c) ``` 4488 for (unsigned int i = 0; NULL != work[i].statement; i++) 4489 { 4490 qs = GNUNET_PQ_eval_prepared_multi_select (session->conn, 4491 work[i].statement, 4492 params, 4493 work[i].cb, 4494 &chc); 4495 if ( (0 > qs) || 4496 (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT != chc.status) ) 4497 { 4498 if (NULL != chc.head) 4499 common_free_coin_transaction_list (cls, 4500 chc.head); 4501 *tlp = NULL; 4502 if (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT != chc.status) 4503 qs = chc.status; 4504 return qs; 4505 } 4506 } ``` In line 4496, it is required that each statement returns GNUNET_DB_STATUS_SUCCESS_ONE_RESULT. However, at least one of the callbacks, like `add_coin_deposit`, could have legitimately returned more than one result. So it seems that `postgres_get_coin_transactions` would fail when multiple deposits of the same coin (with different fractions not summing up to the total value of the coin) would be found in the deposit-table. | ||||
Tags | No tags attached. | ||||
|
I think you are mistaken. Admittedly, you could legitimately call the code confusing here. chc->status is initialized to 1 ( .status = GNUNET_DB_STATUS_SUCCESS_ONE_RESULT). -- even if there are zero results. Then, in the loop, all callbacks ONLY ever set chc->status to GNUNET_DB_STATUS_HARD_ERROR. The other possibilities (num results, soft error, no results) are never used for this 'status'. We COULD change 'status' to a bool (maybe call it "failed") and then change if (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT != chc.status) qs = chc.status; to if (chc.failed) qs = GNUNET_DB_STATUS_HARD_ERROR; I think that would be _clearer_ in terms of the logic -- albeit without actually changing the (already correct) semantics. Agreed? |
|
Ah, yes, confusing indeed. I agree with your suggestion to introduce chs.failed, that would make the logic clearer. |
|
Fixed in 09294481..a039bf4d |
|
Fix committed to master branch. |
Date Modified | Username | Field | Change |
---|---|---|---|
2020-04-22 20:59 | oec | New Issue | |
2020-04-22 20:59 | oec | Status | new => assigned |
2020-04-22 20:59 | oec | Assigned To | => Christian Grothoff |
2020-04-22 21:10 | Christian Grothoff | Note Added: 0015735 | |
2020-04-22 21:10 | Christian Grothoff | Status | assigned => feedback |
2020-04-22 21:19 | oec | Note Added: 0015736 | |
2020-04-22 21:19 | oec | Status | feedback => assigned |
2020-04-22 21:26 | Christian Grothoff | Note Added: 0015737 | |
2020-04-22 21:26 | Christian Grothoff | Status | assigned => resolved |
2020-04-22 21:26 | Christian Grothoff | Resolution | open => fixed |
2020-04-22 21:26 | Christian Grothoff | Fixed in Version | => 0.7.1 |
2020-04-22 21:27 | Christian Grothoff | Product Version | => git (master) |
2020-04-22 21:27 | Christian Grothoff | Target Version | => 0.7.1 |
2020-04-22 21:27 | Christian Grothoff | Severity | major => tweak |
2021-08-24 16:23 | Christian Grothoff | Status | resolved => closed |
2021-09-02 18:13 | Christian Grothoff | Changeset attached | => Taler-exchange master a039bf4d |
2021-09-02 18:14 | Christian Grothoff | Note Added: 0018258 |