diff --git a/plugins/bkpr/bookkeeper.c b/plugins/bkpr/bookkeeper.c index b823a3959..e1ebdbd13 100644 --- a/plugins/bkpr/bookkeeper.c +++ b/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) diff --git a/plugins/bkpr/incomestmt.c b/plugins/bkpr/incomestmt.c index c017f34d0..1a9502a48 100644 --- a/plugins/bkpr/incomestmt.c +++ b/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)) diff --git a/plugins/bkpr/recorder.c b/plugins/bkpr/recorder.c index 301cac060..79a70f8b6 100644 --- a/plugins/bkpr/recorder.c +++ b/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); diff --git a/plugins/bkpr/recorder.h b/plugins/bkpr/recorder.h index ece697fde..1e3278281 100644 --- a/plugins/bkpr/recorder.h +++ b/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, diff --git a/plugins/bkpr/test/run-recorder.c b/plugins/bkpr/test/run-recorder.c index 3382f6a9a..393c052a1 100644 --- a/plugins/bkpr/test/run-recorder.c +++ b/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))); diff --git a/tests/test_bookkeeper.py b/tests/test_bookkeeper.py index 9453b362d..ec53035ed 100644 --- a/tests/test_bookkeeper.py +++ b/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(find_tags(events, 'journal_entry')) == 0 + assert len(events) == 4 + 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 - assert len(l1.rpc.bkpr_listaccountevents()['events']) == 3 + # 4 account events: empty wallet start, 2 wallet deposits, 1 external deposit + 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): """ diff --git a/tests/test_pay.py b/tests/test_pay.py index 6042ddf8a..8360b81f5 100644 --- a/tests/test_pay.py +++ b/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']