bookkeeper: don't set origin account to "null" if that's specified as transfer_from.
What the developer wrote
Authored by Rusty Russell
bookkeeper: don't set origin account to "null" if that's specified as transfer_from.
Before bkpr_listaccountevents() gave entries with origin like:
{'account': "nifty's secret stash",
'blockheight': 111,
'credit_msat': 180000000,
'currency': 'bcrt',
'debit_msat': 0,
'origin': 'null',
'outpoint': 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:0',
'tag': 'deposit',
'timestamp': 1679955976,
'type': 'chain'},
Changelog-Changed: Plugins: "utxo_deposit" is allows to have missing `transfer_from`, and null is not considered an account name.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
What changed, and why it matters
This is a small bug-fix in Core Lightning's bookkeeping plugin. Previously, if a user sent a deposit notification without specifying where the funds came from, the plugin would incorrectly record the origin account as the literal string "null". The fix makes the transfer_from field optional and treats a JSON null value as 'no origin account' rather than an account named "null". This is a data-correctness issue, not a direct way for an attacker to steal funds or take over a node.
No urgent security action required. Treat as a normal bug fix. Operators relying on bookkeeper origin account data should ensure they are on a version containing this commit if they use custom deposit notifications.
Security signals we found
Data-integrity bug: literal 'null' string stored as account origin
Plugin RPC parameter handling change: optional field with null handling
Test added/updated to assert corrected behavior
Evidence from the diff
The commit modifies json_utxo_deposit in plugins/bkpr/bookkeeper.c so that the transfer_from field is optional (transfer_from?:%) and is parsed with a custom json_to_tok helper. If the field is absent or explicitly JSON null, ev->origin_acct is set to NULL; otherwise it is duplicated from the token. This prevents the literal string “null” from being stored as an origin account name. A test is updated to assert that the returned event no longer contains an origin key when transfer_from is not supplied.
Changed components
plugins/bkpr/bookkeeper.ctests/test_bookkeeper.pyInspect captured patch +61 / −4
diff --git a/plugins/bkpr/bookkeeper.c b/plugins/bkpr/bookkeeper.c
index 45258abb..e7e1409b 100644
--- a/plugins/bkpr/bookkeeper.c
+++ b/plugins/bkpr/bookkeeper.c
@@ -1723,6 +1723,12 @@ static char *parse_tags(const tal_t *ctx,
return NULL;
}
+static bool json_to_tok(const char *buffer, const jsmntok_t *tok, const jsmntok_t **ret)
+{
+ *ret = tok;
+ return true;
+}
+
static struct command_result *json_utxo_deposit(struct command *cmd, const char *buf, const jsmntok_t *params)
{
const char *move_tag ="utxo_deposit";
@@ -1730,18 +1736,20 @@ static struct command_result *json_utxo_deposit(struct command *cmd, const char
struct account *acct;
const char *err;
struct bkpr *bkpr = bkpr_of(cmd->plugin);
+ const jsmntok_t *transfer_from;
+ transfer_from = NULL;
err = json_scan(tmpctx, buf, params,
"{utxo_deposit:{"
"account:%"
- ",transfer_from:%"
+ ",transfer_from?:%"
",outpoint:%"
",amount_msat:%"
",timestamp:%"
",blockheight:%"
"}}",
- JSON_SCAN_TAL(tmpctx, json_strdup, &ev->acct_name),
- JSON_SCAN_TAL(tmpctx, json_strdup, &ev->origin_acct),
+ JSON_SCAN_TAL(ev, json_strdup, &ev->acct_name),
+ JSON_SCAN(json_to_tok, &transfer_from),
JSON_SCAN(json_to_outpoint, &ev->outpoint),
JSON_SCAN(json_to_msat, &ev->credit),
JSON_SCAN(json_to_u64, &ev->timestamp),
@@ -1753,6 +1761,11 @@ static struct command_result *json_utxo_deposit(struct command *cmd, const char
move_tag, err, json_tok_full_len(params),
json_tok_full(buf, params));
+ if (!transfer_from || json_tok_is_null(buf, transfer_from))
+ ev->origin_acct = NULL;
+ else
+ ev->origin_acct = json_strdup(ev, buf, transfer_from);
+
/* Log the thing */
db_begin_transaction(bkpr->db);
acct = find_or_create_account(cmd, bkpr, ev->acct_name);
diff --git a/tests/test_bookkeeper.py b/tests/test_bookkeeper.py
index f590c0e8..27a339fe 100644
--- a/tests/test_bookkeeper.py
+++ b/tests/test_bookkeeper.py
@@ -891,7 +891,7 @@ def test_rebalance_tracking(node_factory, bitcoind):
assert outbound_ev['payment_id'] == pay_hash
-def test_bookkeeper_custom_notifs(node_factory):
+def test_bookkeeper_custom_notifs(node_factory, chainparams):
# FIXME: what happens if we send internal funds to 'external' wallet?
plugin = os.path.join(
os.path.dirname(__file__), "plugins", "bookkeeper_custom_coins.py"
@@ -939,6 +939,50 @@ def test_bookkeeper_custom_notifs(node_factory):
assert len(onchain_fees) == 2
assert onchain_fees[0]['credit_msat'] == onchain_fee_one
assert onchain_fees[1]['debit_msat'] == withdraw_amt
+ assert events == [{'account': "nifty's secret stash",
+ 'blockheight': 111,
+ 'credit_msat': 180000000,
+ 'currency': chainparams['bip173_prefix'],
+ 'debit_msat': 0,
+ 'outpoint': 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:0',
+ 'tag': 'deposit',
+ 'timestamp': 1679955976,
+ 'type': 'chain'},
+ {'account': "nifty's secret stash",
+ 'blockheight': 111,
+ 'credit_msat': 0,
+ 'currency': chainparams['bip173_prefix'],
+ 'debit_msat': 180000000,
+ 'outpoint': 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:0',
+ 'tag': 'withdrawal',
+ 'timestamp': 1679955976,
+ 'txid': 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb',
+ 'type': 'chain'},
+ {'account': "nifty's secret stash",
+ 'blockheight': 111,
+ 'credit_msat': 124443000,
+ 'currency': chainparams['bip173_prefix'],
+ 'debit_msat': 0,
+ 'outpoint': 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb:0',
+ 'tag': 'deposit',
+ 'timestamp': 1679955976,
+ 'type': 'chain'},
+ {'account': "nifty's secret stash",
+ 'credit_msat': 55557000,
+ 'currency': chainparams['bip173_prefix'],
+ 'debit_msat': 0,
+ 'tag': 'onchain_fee',
+ 'timestamp': 1679955976,
+ 'txid': 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb',
+ 'type': 'onchain_fee'},
+ {'account': "nifty's secret stash",
+ 'credit_msat': 0,
+ 'currency': chainparams['bip173_prefix'],
+ 'debit_msat': 55555000,
+ 'tag': 'onchain_fee',
+ 'timestamp': 1679955976,
+ 'txid': 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb',
+ 'type': 'onchain_fee'}]
# This should not blow up
incomes = l1.rpc.bkpr_listincome()['income_events']
Why this scored 21/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.