View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0006170 | Taler | exchange | public | 2020-04-08 14:14 | 2021-09-02 16:14 |
| Reporter | fefe | Assigned To | Christian Grothoff | Resolution deadline | |
| Implementer | Reviewer | ||||
| Release signed by | Deployed by | ||||
| Verifier | Communicator | ||||
| Data protection authority notification required | No | ||||
| CSIRT notification required | No | ||||
| Estimated effort (hours) | Actual effort (hours) | 0 | |||
| Fast-track | No | ||||
| Work package | none | ||||
| Priority | normal | Severity | minor | Reproducibility | N/A |
| Status | closed | Resolution | fixed | ||
| Product Version | 0.7.0 | ||||
| Target Version | 0.7.1 | Fixed in Version | 0.7.1 | ||
| Summary | 0006170: confusing code in verify_reserve_balance | ||||
| Description | 1026 /* Check our reserve summary balance calculation shows that 1027 the reserve balance is acceptable (i.e. non-negative) */ 1028 GNUNET_assert (GNUNET_OK == 1029 TALER_amount_add (&balance, 1030 &rs->total_in, 1031 &rs->a_balance)); This does balance = rs->total_in + rs->a_balance. While I have an idea what total_in means, what is a_balance? Maybe rename it to balance_at_prev_audit or so? 1032 if (GNUNET_SYSERR == 1033 TALER_amount_subtract (&nbalance, 1034 &balance, 1035 &rs->total_out)) 1036 { Practically all other TALER_amount_subtract are in an assert that wants to make sure they did not return GNUNET_SYSERR. Why is this one different? And if the return was a SYSERR, why are we reacting not by logging an error but by doing some calculations? In fact the code looks like this is the expected code path. I'm confused by this. Please explain (and add comments in the code). | ||||
| Tags | No tags attached. | ||||
| Apparently subtract returns SYSERR of a-b would be negative. However it also returns SYSERR of the normalization failed. Maybe we meant the first case and forgot about the second one? | |
| TALER_amount_subtract also returns SYSERR if the currency was invalid. | |
| The intended case was "if result was negative". I actually forgot that we have the other two possibilities ;-). I'll try to clean up the code (later, still busy with 0006164). | |
| There is another instance of the pattern further down in the function: 1208 if (GNUNET_SYSERR == 1209 TALER_amount_subtract (&r, 1210 &total_escrow_balance, 1211 &rs->total_out)) 1212 { 1213 /* We could not reduce our total balance, i.e. exchange allowed IN TOTAL (!) 1214 to be withdrawn more than it was IN TOTAL ever given (exchange balance 1215 went negative!). Woopsie. Calculate how badly it went and log. */ This time there is a proper comment explaining what is happening. Recommendation: Add another return value for "value would become negative" instead of reusing SYSERR. | |
| Yes, at least. The TALER_amount_subtract() API's use of SYSERR for 'negative result' is one that actually has bitten me before... | |
| "a" in "a_balance" was supposed to be for "auditor_". But happy to rename as suggested, that is even clearer: 50bc862a..1554cc31 | |
| 1554cc31..84a40be0 introduces new return types for amount addition and subtraction, plus modifies the auditor to clearly distinguish between the different cases where applicable. | |
| Fix committed to master branch. | |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2020-04-08 14:14 | fefe | New Issue | |
| 2020-04-08 14:14 | fefe | Status | new => assigned |
| 2020-04-08 14:14 | fefe | Assigned To | => Christian Grothoff |
| 2020-04-08 14:16 | fefe | Note Added: 0015583 | |
| 2020-04-08 14:17 | fefe | Note Added: 0015584 | |
| 2020-04-08 15:00 | Christian Grothoff | Note Added: 0015585 | |
| 2020-04-08 15:05 | fefe | Note Added: 0015586 | |
| 2020-04-08 15:43 | Christian Grothoff | Note Added: 0015587 | |
| 2020-04-08 16:27 | Christian Grothoff | Note Added: 0015590 | |
| 2020-04-08 16:28 | Christian Grothoff | Note Edited: 0015590 | |
| 2020-04-08 21:57 | Christian Grothoff | Note Added: 0015597 | |
| 2020-04-08 21:57 | Christian Grothoff | Status | assigned => informing |
| 2020-04-08 21:57 | Christian Grothoff | Resolution | open => fixed |
| 2020-04-08 21:57 | Christian Grothoff | Fixed in Version | => 0.7.1 |
| 2020-04-08 21:58 | Christian Grothoff | Target Version | => 0.7.1 |
| 2021-08-24 14:23 | Christian Grothoff | Status | informing => closed |
| 2021-09-02 16:13 | Christian Grothoff | ||
| 2021-09-02 16:14 | Christian Grothoff | Note Added: 0018266 |