bkpr: remove currency support from balances.
What changed, and why it matters
This commit is a code cleanup in Core Lightning's bookkeeper plugin. It removes support for tracking multiple currencies per account and simplifies how account balances are calculated. The change is described by the developer as removing a concept they no longer want. There is no indication in the commit message or diff that this fixes a security vulnerability.
No security action required. Treat as a normal refactoring/cleanup commit. Reviewers may want to verify that the new single-currency assumption holds for all supported deployments and that the `plugin_err()` calls on overflow are acceptable fatal behavior.
Security signals we found
Removal of multi-currency balance aggregation
Change of error handling from returned error strings to fatal plugin errors on database overflow
SQL query simplification removing GROUP BY currency
No security-relevant keywords or fixes mentioned in commit message
Evidence from the diff
The patch refactors the bookkeeper plugin (plugins/bkpr). It replaces account_get_balance() (which returned an array of per-currency balances) with account_get_credit_debit() (which returns a single credit/debit pair and a boolean indicating whether any events exist). The SQL queries lose their GROUP BY ce.currency clauses, and callers now compute the net balance themselves. The struct acct_balance type is moved into the test file only. Error handling changes from returning error strings to calling plugin_err() for overflow conditions. A new include for chainparams.h is added so json_list_balances can report chainparams->lightning_hrp as the coin type.
Changed components
plugins/bkpr/bookkeeper.cplugins/bkpr/recorder.cplugins/bkpr/recorder.hplugins/bkpr/test/run-recorder.cInspect captured patch +108 / −198
diff --git a/plugins/bkpr/bookkeeper.c b/plugins/bkpr/bookkeeper.c
index 3747f49d..af398726 100644
--- a/plugins/bkpr/bookkeeper.c
+++ b/plugins/bkpr/bookkeeper.c
@@ -1,4 +1,5 @@
#include "config.h"
+#include <bitcoin/chainparams.h>
#include <ccan/array_size/array_size.h>
#include <ccan/cast/cast.h>
#include <ccan/json_escape/json_escape.h>
@@ -556,7 +557,6 @@ static struct command_result *json_list_balances(struct command *cmd,
{
struct json_stream *res;
struct account **accts;
- char *err;
if (!param(cmd, buf, params, NULL))
return command_param_failed();
@@ -568,18 +568,19 @@ static struct command_result *json_list_balances(struct command *cmd,
json_array_start(res, "accounts");
for (size_t i = 0; i < tal_count(accts); i++) {
- struct acct_balance **balances;
-
- err = account_get_balance(cmd, db,
- accts[i]->name,
- true,
- &balances);
+ struct amount_msat credit, debit, balance;
+ bool has_events;
- if (err)
+ has_events = account_get_credit_debit(cmd->plugin, db,
+ accts[i]->name,
+ &credit, &debit);
+ if (!amount_msat_sub(&balance, credit, debit)) {
plugin_err(cmd->plugin,
- "Get account balance returned err"
- " for account %s: %s",
- accts[i]->name, err);
+ "Account balance underflow for account %s (credit %s, debit %s)",
+ accts[i]->name,
+ fmt_amount_msat(tmpctx, credit),
+ fmt_amount_msat(tmpctx, debit));
+ }
/* Skip the external acct balance, it's effectively
* meaningless */
@@ -602,13 +603,15 @@ static struct command_result *json_list_balances(struct command *cmd,
accts[i]->onchain_resolved_block);
}
+ /* FIXME: This API is now overkill! */
json_array_start(res, "balances");
- for (size_t j = 0; j < tal_count(balances); j++) {
+ /* We expect no entry if account is not used. */
+ for (size_t j = 0; j < has_events; j++) {
json_object_start(res, NULL);
json_add_amount_msat(res, "balance_msat",
- balances[j]->balance);
+ balance);
json_add_string(res, "coin_type",
- balances[j]->currency);
+ chainparams->lightning_hrp);
json_object_end(res);
}
json_array_end(res);
@@ -951,8 +954,7 @@ static struct command_result *listpeerchannels_multi_done(struct command *cmd,
/* Let's register all these accounts! */
for (size_t i = 0; i < tal_count(new_accts); i++) {
struct new_account_info *info = new_accts[i];
- struct acct_balance **balances, *bal;
- struct amount_msat credit_diff, debit_diff;
+ struct amount_msat credit, debit, credit_diff, debit_diff;
char *err;
if (!new_missed_channel_account(cmd, buf, result,
@@ -966,25 +968,14 @@ static struct command_result *listpeerchannels_multi_done(struct command *cmd,
}
db_begin_transaction(db);
- err = account_get_balance(tmpctx, db, info->acct->name,
- false, &balances);
+ account_get_credit_debit(cmd->plugin, db,
+ info->acct->name,
+ &credit, &debit);
db_commit_transaction(db);
- if (err)
- plugin_err(cmd->plugin, "%s", err);
-
- /* FIXME: multiple currencies */
- if (tal_count(balances) > 0)
- bal = balances[0];
- else {
- bal = tal(tmpctx, struct acct_balance);
- bal->credit = AMOUNT_MSAT(0);
- bal->debit= AMOUNT_MSAT(0);
- }
-
err = msat_find_diff(info->curr_bal,
- bal->credit,
- bal->debit,
+ credit,
+ debit,
&credit_diff, &debit_diff);
if (err)
plugin_err(cmd->plugin, "%s", err);
@@ -1075,9 +1066,8 @@ static struct command_result *json_balance_snapshot(struct command *cmd,
db_begin_transaction(db);
json_for_each_arr(i, acct_tok, accounts_tok) {
- struct acct_balance **balances, *bal;
struct account *acct;
- struct amount_msat snap_balance, credit_diff, debit_diff;
+ struct amount_msat snap_balance, credit, debit, credit_diff, debit_diff;
char *acct_name, *currency;
bool existed;
@@ -1101,34 +1091,13 @@ static struct command_result *json_balance_snapshot(struct command *cmd,
fmt_amount_msat(tmpctx, snap_balance));
/* Find the account balances */
- err = account_get_balance(cmd, db, acct_name,
- /* Don't error if negative */
- false,
- &balances);
-
- if (err)
- plugin_err(cmd->plugin,
- "Get account balance returned err"
- " for account %s: %s",
- acct_name, err);
-
- /* multiple currency balances! */
- bal = NULL;
- for (size_t j = 0; j < tal_count(balances); j++) {
- if (streq(balances[j]->currency, currency))
- bal = balances[j];
- }
-
- if (!bal) {
- bal = tal(balances, struct acct_balance);
- bal->credit = AMOUNT_MSAT(0);
- bal->debit = AMOUNT_MSAT(0);
- }
+ account_get_credit_debit(cmd->plugin, db, acct_name,
+ &credit, &debit);
/* Figure out what the net diff is btw reported & actual */
err = msat_find_diff(snap_balance,
- bal->credit,
- bal->debit,
+ credit,
+ debit,
&credit_diff, &debit_diff);
if (err)
plugin_err(cmd->plugin,
@@ -1398,8 +1367,7 @@ listpeerchannels_done(struct command *cmd,
const jsmntok_t *result,
struct event_info *info)
{
- struct acct_balance **balances, *bal;
- struct amount_msat credit_diff, debit_diff;
+ struct amount_msat credit, debit, credit_diff, debit_diff;
const char *err;
if (new_missed_channel_account(cmd, buf, result,
@@ -1407,28 +1375,15 @@ listpeerchannels_done(struct command *cmd,
info->ev->currency,
info->ev->timestamp)) {
db_begin_transaction(db);
- err = account_get_balance(tmpctx, db, info->acct->name,
- false, &balances);
+ account_get_credit_debit(cmd->plugin, db, info->acct->name,
+ &credit, &debit);
db_commit_transaction(db);
- if (err)
- plugin_err(cmd->plugin, "%s", err);
-
- /* FIXME: multiple currencies per account? */
- if (tal_count(balances) > 0)
- bal = balances[0];
- else {
- bal = tal(balances, struct acct_balance);
- bal->credit = AMOUNT_MSAT(0);
- bal->debit = AMOUNT_MSAT(0);
- }
- assert(tal_count(balances) == 1);
-
/* The expected current balance is zero, since
* we just got the channel close event */
err = msat_find_diff(AMOUNT_MSAT(0),
- bal->credit,
- bal->debit,
+ credit,
+ debit,
&credit_diff, &debit_diff);
if (err)
plugin_err(cmd->plugin, "%s", err);
diff --git a/plugins/bkpr/recorder.c b/plugins/bkpr/recorder.c
index 18c84094..77d8096a 100644
--- a/plugins/bkpr/recorder.c
+++ b/plugins/bkpr/recorder.c
@@ -15,6 +15,7 @@
#include <plugins/bkpr/channel_event.h>
#include <plugins/bkpr/onchain_fee.h>
#include <plugins/bkpr/recorder.h>
+#include <plugins/libplugin.h>
static struct chain_event *stmt2chain_event(const tal_t *ctx, struct db_stmt *stmt)
@@ -917,103 +918,72 @@ static struct chain_event *find_chain_event(const tal_t *ctx,
return e;
}
-char *account_get_balance(const tal_t *ctx,
- struct db *db,
- const char *acct_name,
- bool calc_sum,
- struct acct_balance ***balances)
+bool account_get_credit_debit(struct plugin *plugin,
+ struct db *db,
+ const char *acct_name,
+ struct amount_msat *credit,
+ struct amount_msat *debit)
{
struct db_stmt *stmt;
+ bool exists;
+ /* Get sum from chain_events */
stmt = db_prepare_v2(db, SQL("SELECT"
" CAST(SUM(ce.credit) AS BIGINT) as credit"
", CAST(SUM(ce.debit) AS BIGINT) as debit"
- ", ce.currency"
" FROM chain_events ce"
" LEFT OUTER JOIN accounts a"
" ON a.id = ce.account_id"
- " WHERE a.name = ?"
- " GROUP BY ce.currency"));
-
+ " WHERE a.name = ?"));
db_bind_text(stmt, acct_name);
db_query_prepared(stmt);
- *balances = tal_arr(ctx, struct acct_balance *, 0);
-
- while (db_step(stmt)) {
- struct acct_balance *bal;
- bal = tal(*balances, struct acct_balance);
-
- bal->currency = db_col_strdup(bal, stmt, "ce.currency");
- bal->credit = db_col_amount_msat(stmt, "credit");
- bal->debit = db_col_amount_msat(stmt, "debit");
- tal_arr_expand(balances, bal);
+ db_step(stmt);
+ if (db_col_is_null(stmt, "credit")) {
+ db_col_ignore(stmt, "debit");
+ *credit = *debit = AMOUNT_MSAT(0);
+ exists = false;
+ } else {
+ *credit = db_col_amount_msat(stmt, "credit");
+ *debit = db_col_amount_msat(stmt, "debit");
+ exists = true;
}
tal_free(stmt);
+ /* Get sum from channel_events */
stmt = db_prepare_v2(db, SQL("SELECT"
" CAST(SUM(ce.credit) AS BIGINT) as credit"
", CAST(SUM(ce.debit) AS BIGINT) as debit"
- ", ce.currency"
" FROM channel_events ce"
" LEFT OUTER JOIN accounts a"
" ON a.id = ce.account_id"
- " WHERE a.name = ?"
- " GROUP BY ce.currency"));
+ " WHERE a.name = ?"));
db_bind_text(stmt, acct_name);
db_query_prepared(stmt);
+ db_step(stmt);
- while (db_step(stmt)) {
- struct amount_msat amt;
- struct acct_balance *bal = NULL;
- char *currency;
-
- currency = db_col_strdup(ctx, stmt, "ce.currency");
-
- /* Find the currency entry from above */
- for (size_t i = 0; i < tal_count(*balances); i++) {
- if (streq((*balances)[i]->currency, currency)) {
- bal = (*balances)[i];
- break;
- }
- }
-
- if (!bal) {
- bal = tal(*balances, struct acct_balance);
- bal->credit = AMOUNT_MSAT(0);
- bal->debit = AMOUNT_MSAT(0);
- bal->currency = tal_steal(bal, currency);
- tal_arr_expand(balances, bal);
- }
-
- amt = db_col_amount_msat(stmt, "credit");
- if (!amount_msat_accumulate(&bal->credit, amt)) {
- tal_free(stmt);
- return "overflow adding channel_event credits";
+ if (db_col_is_null(stmt, "credit")) {
+ db_col_ignore(stmt, "debit");
+ } else {
+ if (!amount_msat_accumulate(credit,
+ db_col_amount_msat(stmt, "credit"))) {
+ plugin_err(plugin, "db overflow: chain credit %s, adding channel credit %s",
+ fmt_amount_msat(tmpctx, *credit),
+ fmt_amount_msat(tmpctx,
+ db_col_amount_msat(stmt, "credit")));
}
- amt = db_col_amount_msat(stmt, "debit");
- if (!amount_msat_accumulate(&bal->debit, amt)) {
- tal_free(stmt);
- return "overflow adding channel_event debits";
+ if (!amount_msat_accumulate(debit,
+ db_col_amount_msat(stmt, "debit"))) {
+ plugin_err(plugin, "db overflow: chain debit %s, adding channel debit %s",
+ fmt_amount_msat(tmpctx, *debit),
+ fmt_amount_msat(tmpctx,
+ db_col_amount_msat(stmt, "debit")));
}
+ exists = true;
}
tal_free(stmt);
-
- if (!calc_sum)
- return NULL;
-
- for (size_t i = 0; i < tal_count(*balances); i++) {
- struct acct_balance *bal = (*balances)[i];
- if (!amount_msat_sub(&bal->balance, bal->credit, bal->debit))
- return tal_fmt(ctx,
- "%s channel balance is negative? %s - %s",
- bal->currency,
- fmt_amount_msat(ctx, bal->credit),
- fmt_amount_msat(ctx, bal->debit));
- }
-
- return NULL;
+ return exists;
}
struct channel_event **list_channel_events_timebox(const tal_t *ctx,
diff --git a/plugins/bkpr/recorder.h b/plugins/bkpr/recorder.h
index 596ef355..db1ef5a7 100644
--- a/plugins/bkpr/recorder.h
+++ b/plugins/bkpr/recorder.h
@@ -9,18 +9,12 @@ struct bitcoin_txid;
struct chain_event;
struct channel_event;
struct db;
+struct plugin;
enum mvt_tag;
struct onchain_fee;
#define SQLITE_MAX_UINT 0x7FFFFFFFFFFFFFFF
-struct acct_balance {
- char *currency;
- struct amount_msat credit;
- struct amount_msat debit;
- struct amount_msat balance;
-};
-
struct fee_sum {
u64 acct_db_id;
char *acct_name;
@@ -116,15 +110,14 @@ struct chain_event **get_chain_events_by_outpoint(const tal_t *ctx,
const struct bitcoin_outpoint *outpoint,
bool credits_only);
-/* Calculate the balances for an account
- *
- * @calc_sum - compute the total balance. error if negative
- * */
-char *account_get_balance(const tal_t *ctx,
- struct db *db,
- const char *acct_name,
- bool calc_sum,
- struct acct_balance ***balances);
+/* Get total credits and debits for this account: returns false if no entries at all
+ * (in which case, credit and debit will both be AMOUNT_MSAT(0)). */
+bool account_get_credit_debit(struct plugin *plugin,
+ struct db *db,
+ const char *acct_name,
+ struct amount_msat *credit,
+ struct amount_msat *debit);
+
/* Get chain fees for account */
struct onchain_fee **account_get_chain_fees(const tal_t *ctx, struct db *db,
diff --git a/plugins/bkpr/test/run-recorder.c b/plugins/bkpr/test/run-recorder.c
index bea42719..d5e09fe6 100644
--- a/plugins/bkpr/test/run-recorder.c
+++ b/plugins/bkpr/test/run-recorder.c
@@ -1185,14 +1185,32 @@ static bool test_chain_event_crud(const tal_t *ctx, struct plugin *p)
return true;
}
+struct acct_balance {
+ char *currency;
+ struct amount_msat credit;
+ struct amount_msat debit;
+ struct amount_msat balance;
+};
+
+static bool account_get_balance(struct plugin *plugin,
+ struct db *db,
+ const char *acct_name,
+ struct acct_balance *bal)
+{
+ account_get_credit_debit(plugin, db, acct_name,
+ &bal->credit, &bal->debit);
+
+ return amount_msat_sub(&bal->balance, bal->credit, bal->debit);
+}
+
static bool test_account_balances(const tal_t *ctx, struct plugin *p)
{
struct db *db = db_setup(ctx, p, tmp_dsn(ctx));
struct node_id peer_id;
struct account *acct, *acct2;
struct chain_event *ev1;
- struct acct_balance **balances;
- char *err;
+ struct acct_balance balance;
+ bool ok;
memset(&peer_id, 3, sizeof(struct node_id));
@@ -1201,10 +1219,8 @@ static bool test_account_balances(const tal_t *ctx, struct plugin *p)
db_begin_transaction(db);
/* Check that account does not exist yet */
- err = account_get_balance(ctx, db, acct->name, true,
- &balances);
-
- CHECK(!err);
+ ok = account_get_balance(NULL, db, acct->name, &balance);
+ CHECK(ok);
account_add(db, acct);
account_add(db, acct2);
@@ -1239,47 +1255,23 @@ static bool test_account_balances(const tal_t *ctx, struct plugin *p)
AMOUNT_MSAT(0),
'D'));
- /* +5000chf */
- ev1 = make_chain_event(ctx, "two",
- AMOUNT_MSAT(5000), AMOUNT_MSAT(0),
- AMOUNT_MSAT(5000), 1999,
- 'A', 3, '*');
- ev1->currency = "chf";
- log_chain_event(db, acct, ev1);
-
- /* Add same chain event to a different account, shouldn't show */
- log_chain_event(db, acct2, ev1);
-
- err = account_get_balance(ctx, db, acct->name, true,
- &balances);
- CHECK_MSG(!err, err);
+ ok = account_get_balance(NULL, db, acct->name, &balance);
+ CHECK(ok);
db_commit_transaction(db);
- /* Should have 2 balances */
- CHECK(tal_count(balances) == 2);
- CHECK(streq(balances[0]->currency, "btc"));
- CHECK(amount_msat_eq(balances[0]->balance, AMOUNT_MSAT(500 - 440 + 1)));
- CHECK(streq(balances[1]->currency, "chf"));
- CHECK(amount_msat_eq(balances[1]->balance, AMOUNT_MSAT(5000)));
+ CHECK(amount_msat_eq(balance.balance, AMOUNT_MSAT(500 - 440 + 1)));
/* Should error if account balance is negative */
db_begin_transaction(db);
- /* -5001chf */
+ /* -5001btc */
ev1 = make_chain_event(ctx, "two",
AMOUNT_MSAT(0), AMOUNT_MSAT(5001),
AMOUNT_MSAT(5001), 2020,
'A', 4, '*');
- ev1->currency = "chf";
log_chain_event(db, acct, ev1);
- err = account_get_balance(ctx, db, acct->name, true,
- &balances);
- CHECK_MSG(err != NULL, "Expected err message");
- CHECK(streq(err, "chf channel balance is negative? 5000msat - 5001msat"));
-
- err = account_get_balance(ctx, db, acct->name, false,
- &balances);
- CHECK_MSG(!err, err);
+ ok = account_get_balance(NULL, db, acct->name, &balance);
+ CHECK(!ok);
db_commit_transaction(db);
return true;
Why this scored 18/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.