From 240b23684d3f2b05b903b37f0a5fe3fc9e07497d Mon Sep 17 00:00:00 2001 From: Christian Grothoff Date: Mon, 23 Mar 2020 22:27:31 +0100 Subject: finish review of coins auditor analysis logic, improve error handling --- src/auditor/taler-helper-auditor-coins.c | 364 +++++++++++++++++-------------- 1 file changed, 196 insertions(+), 168 deletions(-) (limited to 'src/auditor') diff --git a/src/auditor/taler-helper-auditor-coins.c b/src/auditor/taler-helper-auditor-coins.c index af417215b..5ce3e77ec 100644 --- a/src/auditor/taler-helper-auditor-coins.c +++ b/src/auditor/taler-helper-auditor-coins.c @@ -910,13 +910,13 @@ reveal_data_cb (void *cls, report_row_inconsistency ("refresh_reveal", rctx->rowid, "denomination key not found"); - rctx->err = GNUNET_NO; /* terminate, but return "OK" */ + rctx->err = GNUNET_NO; /* terminate here, but return "OK" to commit transaction */ } else if (GNUNET_DB_STATUS_SUCCESS_ONE_RESULT != qs) { GNUNET_break (GNUNET_DB_STATUS_SOFT_ERROR == qs); rctx->qs = qs; - rctx->err = GNUNET_SYSERR; /* terminate, return GNUNET_SYSERR */ + rctx->err = GNUNET_SYSERR; /* terminate, return #GNUNET_SYSERR: abort transaction */ } } } @@ -1101,9 +1101,9 @@ refresh_session_cb (void *cls, if ( (GNUNET_DB_STATUS_SUCCESS_NO_RESULTS == qs) || (0 == reveal_ctx.num_freshcoins) ) { - /* This can happen if reveal was not yet called or only - with invalid data, even if the exchange is correctly - operating. We still report it. */ + /* This can legitimately happen if reveal was not yet called or only + with invalid data, even if the exchange is correctly operating. We + still report it. */ TALER_ARL_report (report_refreshs_hanging, json_pack ("{s:I, s:o, s:o}", "row", (json_int_t) rowid, @@ -1162,15 +1162,22 @@ refresh_session_cb (void *cls, amount_with_fee, &melt_fee)) { - // FIXME: handle properly! - GNUNET_break (0); - cc->qs = GNUNET_DB_STATUS_HARD_ERROR; - GNUNET_free_non_null (reveal_ctx.new_issues); - return GNUNET_SYSERR; + /* Melt fee higher than contribution of melted coin; this makes + no sense (exchange should never have accepted the operation) */ + report_amount_arithmetic_inconsistency ("melt contribution vs. fee", + rowid, + amount_with_fee, + &melt_fee, + -1); + /* To continue, best assumption is the melted coin contributed + nothing (=> all withdrawal amounts will be counted as losses) */ + GNUNET_assert (GNUNET_OK == + TALER_amount_get_zero (TALER_ARL_currency, + &amount_without_fee)); } } - /* check old coin covers complete expenses */ + /* check old coin covers complete expenses (of withdraw operations) */ if (1 == TALER_amount_cmp (&refresh_cost, &amount_without_fee)) { @@ -1184,7 +1191,7 @@ refresh_session_cb (void *cls, return GNUNET_OK; } - /* update outstanding denomination amounts */ + /* update outstanding denomination amounts for fresh coins withdrawn */ for (unsigned int i = 0; idenom_hash); if (NULL == dsi) { - GNUNET_break (0); - return GNUNET_SYSERR; + report_row_inconsistency ("refresh_reveal", + rowid, + "denomination key for fresh coin unknown to auditor"); + } + else + { + TALER_amount_ntoh (&value, + &reveal_ctx.new_issues[i]->value); + GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, + "Created fresh coin in denomination `%s' of value %s\n", + GNUNET_h2s (&reveal_ctx.new_issues[i]->denom_hash), + TALER_amount2s (&value)); + dsi->num_issued++; + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&dsi->denom_balance, + &dsi->denom_balance, + &value)); + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&dsi->denom_risk, + &dsi->denom_risk, + &value)); + GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, + "New balance of denomination `%s' is %s\n", + GNUNET_h2s (&reveal_ctx.new_issues[i]->denom_hash), + TALER_amount2s (&dsi->denom_balance)); + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&total_escrow_balance, + &total_escrow_balance, + &value)); + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&total_risk, + &total_risk, + &value)); } - TALER_amount_ntoh (&value, - &reveal_ctx.new_issues[i]->value); - GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, - "Created fresh coin in denomination `%s' of value %s\n", - GNUNET_h2s (&reveal_ctx.new_issues[i]->denom_hash), - TALER_amount2s (&value)); - dsi->num_issued++; - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&dsi->denom_balance, - &dsi->denom_balance, - &value)); - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&dsi->denom_risk, - &dsi->denom_risk, - &value)); - GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, - "New balance of denomination `%s' is %s\n", - GNUNET_h2s (&reveal_ctx.new_issues[i]->denom_hash), - TALER_amount2s (&dsi->denom_balance)); - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&total_escrow_balance, - &total_escrow_balance, - &value)); - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&total_risk, - &total_risk, - &value)); } GNUNET_free_non_null (reveal_ctx.new_issues); } @@ -1235,53 +1246,56 @@ refresh_session_cb (void *cls, &issue->denom_hash); if (NULL == dso) { - // FIXME: handle more nicely!?! - GNUNET_break (0); - return GNUNET_SYSERR; - } - if (GNUNET_SYSERR == - TALER_amount_subtract (&tmp, - &dso->denom_balance, - amount_with_fee)) - { - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&dso->denom_loss, - &dso->denom_loss, - amount_with_fee)); - dso->report_emergency = GNUNET_YES; - } - else - { - dso->denom_balance = tmp; - } - if (-1 == TALER_amount_cmp (&total_escrow_balance, - amount_with_fee)) - { - /* This can theoretically happen if for example the exchange - never issued any coins (i.e. escrow balance is zero), but - accepted a forged coin (i.e. emergency situation after - private key compromise). In that case, we cannot even - subtract the profit we make from the fee from the escrow - balance. Tested as part of test-auditor.sh, case #18 */report_amount_arithmetic_inconsistency ( - "subtracting refresh fee from escrow balance", - rowid, - &total_escrow_balance, - amount_with_fee, - 0); + report_row_inconsistency ("refresh_reveal", + rowid, + "denomination key for dirty coin unknown to auditor"); } else { - GNUNET_assert (GNUNET_SYSERR != - TALER_amount_subtract (&total_escrow_balance, - &total_escrow_balance, - amount_with_fee)); + if (GNUNET_SYSERR == + TALER_amount_subtract (&tmp, + &dso->denom_balance, + amount_with_fee)) + { + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&dso->denom_loss, + &dso->denom_loss, + amount_with_fee)); + dso->report_emergency = GNUNET_YES; + } + else + { + dso->denom_balance = tmp; + } + if (-1 == TALER_amount_cmp (&total_escrow_balance, + amount_with_fee)) + { + /* This can theoretically happen if for example the exchange + never issued any coins (i.e. escrow balance is zero), but + accepted a forged coin (i.e. emergency situation after + private key compromise). In that case, we cannot even + subtract the profit we make from the fee from the escrow + balance. Tested as part of test-auditor.sh, case #18 */// + report_amount_arithmetic_inconsistency ( + "subtracting refresh fee from escrow balance", + rowid, + &total_escrow_balance, + amount_with_fee, + 0); + } + else + { + GNUNET_assert (GNUNET_SYSERR != + TALER_amount_subtract (&total_escrow_balance, + &total_escrow_balance, + amount_with_fee)); + } + GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, + "New balance of denomination `%s' after melt is %s\n", + GNUNET_h2s (&issue->denom_hash), + TALER_amount2s (&dso->denom_balance)); } - GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, - "New balance of denomination `%s' after melt is %s\n", - GNUNET_h2s (&issue->denom_hash), - TALER_amount2s (&dso->denom_balance)); - /* update global melt fees */ { struct TALER_Amount rfee; @@ -1440,54 +1454,57 @@ deposit_cb (void *cls, &issue->denom_hash); if (NULL == ds) { - GNUNET_break (0); - // FIXME: handle/report more nicely!??! - return GNUNET_SYSERR; - } - if (GNUNET_SYSERR == - TALER_amount_subtract (&tmp, - &ds->denom_balance, - amount_with_fee)) - { - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&ds->denom_loss, - &ds->denom_loss, - amount_with_fee)); - ds->report_emergency = GNUNET_YES; + report_row_inconsistency ("deposit", + rowid, + "denomination key for deposited coin unknown to auditor"); } else { - ds->denom_balance = tmp; - } + if (GNUNET_SYSERR == + TALER_amount_subtract (&tmp, + &ds->denom_balance, + amount_with_fee)) + { + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&ds->denom_loss, + &ds->denom_loss, + amount_with_fee)); + ds->report_emergency = GNUNET_YES; + } + else + { + ds->denom_balance = tmp; + } - if (-1 == TALER_amount_cmp (&total_escrow_balance, - amount_with_fee)) - { - /* This can theoretically happen if for example the exchange - never issued any coins (i.e. escrow balance is zero), but - accepted a forged coin (i.e. emergency situation after - private key compromise). In that case, we cannot even - subtract the profit we make from the fee from the escrow - balance. Tested as part of test-auditor.sh, case #18 */// - report_amount_arithmetic_inconsistency ( - "subtracting deposit fee from escrow balance", - rowid, - &total_escrow_balance, - amount_with_fee, - 0); - } - else - { - GNUNET_assert (GNUNET_SYSERR != - TALER_amount_subtract (&total_escrow_balance, - &total_escrow_balance, - amount_with_fee)); - } + if (-1 == TALER_amount_cmp (&total_escrow_balance, + amount_with_fee)) + { + /* This can theoretically happen if for example the exchange + never issued any coins (i.e. escrow balance is zero), but + accepted a forged coin (i.e. emergency situation after + private key compromise). In that case, we cannot even + subtract the profit we make from the fee from the escrow + balance. Tested as part of test-auditor.sh, case #18 */// + report_amount_arithmetic_inconsistency ( + "subtracting deposit fee from escrow balance", + rowid, + &total_escrow_balance, + amount_with_fee, + 0); + } + else + { + GNUNET_assert (GNUNET_SYSERR != + TALER_amount_subtract (&total_escrow_balance, + &total_escrow_balance, + amount_with_fee)); + } - GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, - "New balance of denomination `%s' after deposit is %s\n", - GNUNET_h2s (&issue->denom_hash), - TALER_amount2s (&ds->denom_balance)); + GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, + "New balance of denomination `%s' after deposit is %s\n", + GNUNET_h2s (&issue->denom_hash), + TALER_amount2s (&ds->denom_balance)); + } /* update global deposit fees */ { @@ -1622,31 +1639,33 @@ refund_cb (void *cls, &issue->denom_hash); if (NULL == ds) { - GNUNET_break (0); - // FIXME: handle more nicely!?!? - return GNUNET_SYSERR; + report_row_inconsistency ("refund", + rowid, + "denomination key for refunded coin unknown to auditor"); + } + else + { + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&ds->denom_balance, + &ds->denom_balance, + &amount_without_fee)); + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&ds->denom_risk, + &ds->denom_risk, + &amount_without_fee)); + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&total_escrow_balance, + &total_escrow_balance, + &amount_without_fee)); + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&total_risk, + &total_risk, + &amount_without_fee)); + GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, + "New balance of denomination `%s' after refund is %s\n", + GNUNET_h2s (&issue->denom_hash), + TALER_amount2s (&ds->denom_balance)); } - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&ds->denom_balance, - &ds->denom_balance, - &amount_without_fee)); - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&ds->denom_risk, - &ds->denom_risk, - &amount_without_fee)); - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&total_escrow_balance, - &total_escrow_balance, - &amount_without_fee)); - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&total_risk, - &total_risk, - &amount_without_fee)); - GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, - "New balance of denomination `%s' after refund is %s\n", - GNUNET_h2s (&issue->denom_hash), - TALER_amount2s (&ds->denom_balance)); - /* update total refund fee balance */ GNUNET_assert (GNUNET_OK == TALER_amount_add (&total_refund_fee_income, @@ -1743,26 +1762,35 @@ check_recoup (struct CoinContext *cc, ds = get_denomination_summary (cc, issue, &issue->denom_hash); - if (GNUNET_NO == ds->was_revoked) + if (NULL == ds) { - /* Woopsie, we allowed recoup on non-revoked denomination!? */ - TALER_ARL_report (report_bad_sig_losses, - json_pack ("{s:s, s:I, s:o, s:o}", - "operation", - "recoup (denomination not revoked)", - "row", (json_int_t) rowid, - "loss", TALER_JSON_from_amount (amount), - "coin_pub", GNUNET_JSON_from_data_auto ( - &coin->coin_pub))); + report_row_inconsistency ("recoup", + rowid, + "denomination key for recouped coin unknown to auditor"); + } + else + { + if (GNUNET_NO == ds->was_revoked) + { + /* Woopsie, we allowed recoup on non-revoked denomination!? */ + TALER_ARL_report (report_bad_sig_losses, + json_pack ("{s:s, s:I, s:o, s:o}", + "operation", + "recoup (denomination not revoked)", + "row", (json_int_t) rowid, + "loss", TALER_JSON_from_amount (amount), + "coin_pub", GNUNET_JSON_from_data_auto ( + &coin->coin_pub))); + } + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&ds->denom_recoup, + &ds->denom_recoup, + amount)); + GNUNET_assert (GNUNET_OK == + TALER_amount_add (&total_recoup_loss, + &total_recoup_loss, + amount)); } - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&ds->denom_recoup, - &ds->denom_recoup, - amount)); - GNUNET_assert (GNUNET_OK == - TALER_amount_add (&total_recoup_loss, - &total_recoup_loss, - amount)); return GNUNET_OK; } -- cgit v1.2.3