bkpr: create accounts for zero sat channels

we weren't making records for 'missed' accounts that had a zero balance
at snapshot time (if peer opens channel and is unused)

Fixes: #5502
Reported-By: https://github.com/niftynei/cln-logmaid

plugins/bkpr/bookkeeper.c

@@ -474,7 +474,8 @@ static struct command_result *json_list_balances(struct command *cmd,
 					  accts[i]->name,
 					  true,
 					  false, /* don't skip ignored */
-					  &balances);
+					  &balances,
+					  NULL);
 
 		if (err)
 			plugin_err(cmd->plugin,
@@ -888,7 +889,7 @@ listpeers_multi_done(struct command *cmd,
 
 		db_begin_transaction(db);
 		err = account_get_balance(tmpctx, db, info->acct->name,
-					  false, false, &balances);
+					  false, false, &balances, NULL);
 		db_commit_transaction(db);
 
 		if (err)
@@ -1001,6 +1002,7 @@ static struct command_result *json_balance_snapshot(struct command *cmd,
 		struct acct_balance **balances, *bal;
 		struct amount_msat snap_balance, credit_diff, debit_diff;
 		char *acct_name, *currency;
+		bool exists;
 
 		err = json_scan(cmd, buf, acct_tok,
 				"{account_id:%"
@@ -1029,7 +1031,8 @@ static struct command_result *json_balance_snapshot(struct command *cmd,
 					  /* Ignore non-clightning
 					   * balances items */
 					  true,
-					  &balances);
+					  &balances,
+					  &exists);
 
 		if (err)
 			plugin_err(cmd->plugin,
@@ -1056,7 +1059,8 @@ static struct command_result *json_balance_snapshot(struct command *cmd,
 				   "Unable to find_diff for amounts: %s",
 				   err);
 
-		if (!amount_msat_zero(credit_diff)
+		if (!exists
+		    || !amount_msat_zero(credit_diff)
 		    || !amount_msat_zero(debit_diff)) {
 			struct account *acct;
 			struct channel_event *ev;
@@ -1328,7 +1332,7 @@ listpeers_done(struct command *cmd, const char *buf,
 					info->ev->timestamp)) {
 		db_begin_transaction(db);
 		err = account_get_balance(tmpctx, db, info->acct->name,
-					  false, false, &balances);
+					  false, false, &balances, NULL);
 		db_commit_transaction(db);
 
 		if (err)

plugins/bkpr/incomestmt.c

@@ -235,6 +235,10 @@ static struct income_event *rebalance_fee(const tal_t *ctx,
 static struct income_event *maybe_channel_income(const tal_t *ctx,
 						 struct channel_event *ev)
 {
+	if (amount_msat_zero(ev->credit)
+	    && amount_msat_zero(ev->debit))
+		return NULL;
+
 	/* We record a +/- penalty adj, but we only count the credit */
 	if (streq(ev->tag, "penalty_adj")) {
 		if (!amount_msat_zero(ev->credit))

plugins/bkpr/recorder.c

@@ -786,7 +786,8 @@ char *account_get_balance(const tal_t *ctx,
 			  const char *acct_name,
 			  bool calc_sum,
 			  bool skip_ignored,
-			  struct acct_balance ***balances)
+			  struct acct_balance ***balances,
+			  bool *account_exists)
 {
 	struct db_stmt *stmt;
 
@@ -807,6 +808,8 @@ char *account_get_balance(const tal_t *ctx,
 	db_bind_int(stmt, 1, skip_ignored ? 1 : 2);
 	db_query_prepared(stmt);
 	*balances = tal_arr(ctx, struct acct_balance *, 0);
+	if (account_exists)
+		*account_exists = false;
 
 	while (db_step(stmt)) {
 		struct acct_balance *bal;
@@ -817,6 +820,9 @@ char *account_get_balance(const tal_t *ctx,
 		db_col_amount_msat(stmt, "credit", &bal->credit);
 		db_col_amount_msat(stmt, "debit", &bal->debit);
 		tal_arr_expand(balances, bal);
+
+		if (account_exists)
+			*account_exists = true;
 	}
 	tal_free(stmt);
 

plugins/bkpr/recorder.h

@@ -108,7 +108,8 @@ char *account_get_balance(const tal_t *ctx,
 			  const char *acct_name,
 			  bool calc_sum,
 			  bool skip_ignored,
-			  struct acct_balance ***balances);
+			  struct acct_balance ***balances,
+			  bool *account_exists);
 
 /* Get chain fees for account */
 struct onchain_fee **account_get_chain_fees(const tal_t *ctx, struct db *db,

plugins/bkpr/test/run-recorder.c

@@ -1195,6 +1195,7 @@ static bool test_account_balances(const tal_t *ctx, struct plugin *p)
 	struct account *acct, *acct2;
 	struct chain_event *ev1;
 	struct acct_balance **balances;
+	bool exists;
 	char *err;
 
 	memset(&peer_id, 3, sizeof(struct node_id));
@@ -1203,6 +1204,13 @@ static bool test_account_balances(const tal_t *ctx, struct plugin *p)
 	acct2 = new_account(ctx, tal_fmt(ctx, "wallet"), &peer_id);
 
 	db_begin_transaction(db);
+	/* Check that account does not exist yet */
+	err = account_get_balance(ctx, db, acct->name, true, false,
+				  &balances, &exists);
+
+	CHECK(!err);
+	CHECK_MSG(!exists, "expected account not to exist");
+
 	account_add(db, acct);
 	account_add(db, acct2);
 
@@ -1251,7 +1259,7 @@ static bool test_account_balances(const tal_t *ctx, struct plugin *p)
 	log_chain_event(db, acct2, ev1);
 
 	err = account_get_balance(ctx, db, acct->name, true, false,
-				  &balances);
+				  &balances, NULL);
 	CHECK_MSG(!err, err);
 	db_commit_transaction(db);
 	CHECK_MSG(!db_err, db_err);
@@ -1274,17 +1282,18 @@ static bool test_account_balances(const tal_t *ctx, struct plugin *p)
 	log_chain_event(db, acct, ev1);
 
 	err = account_get_balance(ctx, db, acct->name, true, false,
-				  &balances);
+				  &balances, &exists);
 	CHECK_MSG(err != NULL, "Expected err message");
 	CHECK(streq(err, "chf channel balance is negative? 5000msat - 5001msat"));
+	CHECK_MSG(exists, "expected account to exist");
 
 	err = account_get_balance(ctx, db, acct->name, false, false,
-				  &balances);
+				  &balances, NULL);
 	CHECK_MSG(!err, err);
 
 	/* Now with ignored events */
 	err = account_get_balance(ctx, db, acct->name, true, true,
-				  &balances);
+				  &balances, NULL);
 	CHECK(streq(balances[0]->currency, "btc"));
 	CHECK(amount_msat_eq(balances[0]->balance,
 			     AMOUNT_MSAT(500 - 440 + 1000)));

tests/test_bookkeeper.py

@@ -136,7 +136,7 @@ def test_bookkeeping_external_withdraws(node_factory, bitcoind):
 
     assert withdrawal[0]['amount'] == Decimal('0.00555555')
     incomes = l1.rpc.bkpr_listincome()['income_events']
-    # There should only be two income events: deposits to wallet
+    # There are two income events: deposits to wallet
     # for {amount}
     assert len(incomes) == 2
     for inc in incomes:
@@ -145,7 +145,7 @@ def test_bookkeeping_external_withdraws(node_factory, bitcoind):
         assert inc['credit_msat'] == amount_msat
     # The event should show up in the 'bkpr_listaccountevents' however
     events = l1.rpc.bkpr_listaccountevents()['events']
-    assert len(events) == 3
+    assert len(events) == 4
     external = [e for e in events if e['account'] == 'external'][0]
     assert external['credit_msat'] == Millisatoshi(amount // 2 * 1000)
 
@@ -162,8 +162,8 @@ def test_bookkeeping_external_withdraws(node_factory, bitcoind):
     assert len(find_tags(incomes, 'journal_entry')) == 0
     assert len(incomes) == 2
     events = l1.rpc.bkpr_listaccountevents()['events']
-    assert len(events) == 3
+    assert len(events) == 4
-    assert len(find_tags(events, 'journal_entry')) == 0
+    assert len(find_tags(events, 'journal_entry')) == 1
 
     # the wallet balance should be unchanged
     btc_balance = only_one(only_one(l1.rpc.bkpr_listbalances()['accounts'])['balances'])
@@ -214,17 +214,17 @@ def test_bookkeeping_external_withdraw_missing(node_factory, bitcoind):
     # Ok, now we send some funds to an external address
     l1.rpc.withdraw(waddr, amount // 2)
 
-    # There should only be two income events: deposits to wallet
+    # Only two income events: deposits
     assert len(l1.rpc.bkpr_listincome()['income_events']) == 2
-    # There are three account events: 2 wallet deposits, 1 external deposit
+    # 4 account events: empty wallet start, 2 wallet deposits, 1 external deposit
-    assert len(l1.rpc.bkpr_listaccountevents()['events']) == 3
+    assert len(l1.rpc.bkpr_listaccountevents()['events']) == 4
 
     # Stop node and remove the accounts data
     l1.stop()
     os.remove(os.path.join(basedir, TEST_NETWORK, 'accounts.sqlite3'))
     l1.start()
 
-    # the number of income events should be unchanged
+    # Number of income events should be unchanged
     assert len(l1.rpc.bkpr_listincome()['income_events']) == 2
     # we're now missing the external deposit
     events = l1.rpc.bkpr_listaccountevents()['events']
@@ -270,7 +270,7 @@ def test_bookkeeping_rbf_withdraw(node_factory, bitcoind):
     bitcoind.rpc.sendtoaddress(addr, amount / 10**8)
     bitcoind.generate_block(1)
     wait_for(lambda: len(l1.rpc.listfunds()['outputs']) == 1)
-    assert len(l1.rpc.bkpr_listaccountevents()['events']) == 1
+    assert len(l1.rpc.bkpr_listaccountevents()['events']) == 2
     assert len(l1.rpc.bkpr_listincome()['income_events']) == 1
 
     # Ok, now we send some funds to an external address
@@ -281,7 +281,7 @@ def test_bookkeeping_rbf_withdraw(node_factory, bitcoind):
     assert out1['txid'] in list(mempool.keys())
 
     # another account event, still one income event
-    assert len(l1.rpc.bkpr_listaccountevents()['events']) == 2
+    assert len(l1.rpc.bkpr_listaccountevents()['events']) == 3
     assert len(l1.rpc.bkpr_listincome()['income_events']) == 1
 
     # unreserve the existing output
@@ -294,7 +294,7 @@ def test_bookkeeping_rbf_withdraw(node_factory, bitcoind):
     assert out2['txid'] in list(mempool.keys())
 
     # another account event, still one income event
-    assert len(l1.rpc.bkpr_listaccountevents()['events']) == 3
+    assert len(l1.rpc.bkpr_listaccountevents()['events']) == 4
     assert len(l1.rpc.bkpr_listincome()['income_events']) == 1
 
     # ok now we mine a block
@@ -463,6 +463,76 @@ def test_bookkeeping_missed_chans_pushed(node_factory, bitcoind):
     _check_events(l2, channel_id, exp_events)
 
 
+@unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "turns off bookkeeper at start")
+@unittest.skipIf(TEST_NETWORK != 'regtest', "network fees hardcoded")
+@pytest.mark.openchannel('v1', 'Uses push-msat')
+def test_bookkeeping_missed_chans_pay_after(node_factory, bitcoind):
+    """
+    Route a payment through a channel that we didn't have open when the bookkeeper
+    was around
+    """
+    coin_mvt_plugin = Path(__file__).parent / "plugins" / "coin_movements.py"
+    l1, l2 = node_factory.get_nodes(2, opts={'disable-plugin': 'bookkeeper',
+                                             'plugin': str(coin_mvt_plugin)})
+
+    # Double check there's no bookkeeper plugin on
+    assert l1.daemon.opts['disable-plugin'] == 'bookkeeper'
+    assert l2.daemon.opts['disable-plugin'] == 'bookkeeper'
+
+    open_amt = 10**7
+    invoice_msat = 11000000
+
+    l1.fundwallet(200000000)
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+    txid = l1.rpc.fundchannel(l2.info['id'], open_amt)['txid']
+    bitcoind.generate_block(1, wait_for_mempool=[txid])
+    wait_for(lambda: l1.channel_state(l2) == 'CHANNELD_NORMAL')
+    scid = l1.get_channel_scid(l2)
+    l1.wait_channel_active(scid)
+    channel_id = first_channel_id(l1, l2)
+
+    # Now turn the bookkeeper on and restart
+    l1.stop()
+    l2.stop()
+    del l1.daemon.opts['disable-plugin']
+    del l2.daemon.opts['disable-plugin']
+    l1.start()
+    l2.start()
+
+    # Wait for the balance snapshot to fire/finish
+    l1.daemon.wait_for_log('Snapshot balances updated')
+    l2.daemon.wait_for_log('Snapshot balances updated')
+
+    # Should have channel in both, with balances
+    for n in [l1, l2]:
+        accts = [ba['account'] for ba in n.rpc.bkpr_listbalances()['accounts']]
+        assert channel_id in accts
+
+    # Send a payment, should be ok.
+    l1.wait_channel_active(scid)
+    l1.pay(l2, invoice_msat)
+    l1.daemon.wait_for_log(r'coin movement:.*\'invoice\'')
+
+    def _check_events(node, channel_id, exp_events):
+        chan_events = [ev for ev in node.rpc.bkpr_listaccountevents()['events'] if ev['account'] == channel_id]
+        assert len(chan_events) == len(exp_events)
+        for ev, exp in zip(chan_events, exp_events):
+            assert ev['tag'] == exp[0]
+            assert ev['credit_msat'] == Millisatoshi(exp[1])
+            assert ev['debit_msat'] == Millisatoshi(exp[2])
+
+    # l1 events
+    exp_events = [('channel_open', open_amt * 1000, 0),
+                  ('onchain_fee', 5257000, 0),
+                  ('invoice', 0, invoice_msat)]
+    _check_events(l1, channel_id, exp_events)
+
+    # l2 events
+    exp_events = [('channel_open', 0, 0),
+                  ('invoice', invoice_msat, 0)]
+    _check_events(l2, channel_id, exp_events)
+
+
 @unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "turns off bookkeeper at start")
 def test_bookkeeping_onchaind_txs(node_factory, bitcoind):
     """

tests/test_pay.py

@@ -1331,9 +1331,14 @@ def test_forward_pad_fees_and_cltv(node_factory, bitcoind):
     wait_for(lambda: len(_income_tagset(l1, tags)) == 2)
     incomes = _income_tagset(l1, tags)
     # the balance on l3 should equal the invoice
-    bal = only_one(only_one(l3.rpc.bkpr_listbalances()['accounts'])['balances'])['balance_msat']
+    accts = l3.rpc.bkpr_listbalances()['accounts']
+    assert len(accts) == 2
+    wallet = accts[0]
+    chan_acct = accts[1]
+    assert wallet['account'] == 'wallet'
+    assert only_one(wallet['balances'])['balance_msat'] == Millisatoshi(0)
     assert incomes[0]['tag'] == 'invoice'
-    assert Millisatoshi(bal) == incomes[0]['debit_msat']
+    assert only_one(chan_acct['balances'])['balance_msat'] == incomes[0]['debit_msat']
     inve = only_one([e for e in l1.rpc.bkpr_listaccountevents()['events'] if e['tag'] == 'invoice'])
     assert inve['debit_msat'] == incomes[0]['debit_msat'] + incomes[1]['debit_msat']