diff --git a/plugins/bkpr/bookkeeper.c b/plugins/bkpr/bookkeeper.c index e23dd708e..964dc70fc 100644 --- a/plugins/bkpr/bookkeeper.c +++ b/plugins/bkpr/bookkeeper.c @@ -1072,6 +1072,18 @@ parse_and_log_chain_move(struct command *cmd, "Unable to update onchain fees %s", err); + /* If this is a spend confirmation event, it's possible + * that it we've got an external deposit that's now + * confirmed */ + if (e->spending_txid) { + db_begin_transaction(db); + /* Go see if there's any deposits to an external + * that are now confirmed */ + /* FIXME: might need updating when we can splice? */ + maybe_closeout_external_deposits(db, e); + db_commit_transaction(db); + } + /* If this is an account close event, it's possible * that we *never* got the open event. (This happens * if you add the plugin *after* you've closed the channel) */ diff --git a/plugins/bkpr/incomestmt.c b/plugins/bkpr/incomestmt.c index 7139aacec..ffc074a94 100644 --- a/plugins/bkpr/incomestmt.c +++ b/plugins/bkpr/incomestmt.c @@ -133,6 +133,12 @@ static struct income_event *maybe_chain_income(const tal_t *ctx, /* deposit to external is cost to us */ if (streq(ev->acct_name, EXTERNAL_ACCT)) { struct income_event *iev; + + /* External deposits w/o a blockheight + * aren't confirmed yet */ + if (ev->blockheight == 0) + return NULL; + iev = chain_to_income(ctx, ev, ev->origin_acct, ev->debit, diff --git a/plugins/bkpr/recorder.c b/plugins/bkpr/recorder.c index cb4ac7b0f..7cfd7a643 100644 --- a/plugins/bkpr/recorder.c +++ b/plugins/bkpr/recorder.c @@ -1638,6 +1638,44 @@ finished: return err; } +void maybe_closeout_external_deposits(struct db *db, + struct chain_event *ev) +{ + struct db_stmt *stmt; + + assert(ev->spending_txid); + stmt = db_prepare_v2(db, SQL("SELECT " + " e.id" + " FROM chain_events e" + " LEFT OUTER JOIN accounts a" + " ON e.account_id = a.id" + " WHERE e.blockheight = ?" + " AND e.utxo_txid = ?" + " AND a.name = ?")); + + /* Blockheight for unconfirmeds is zero */ + db_bind_int(stmt, 0, 0); + db_bind_txid(stmt, 1, ev->spending_txid); + db_bind_text(stmt, 2, EXTERNAL_ACCT); + db_query_prepared(stmt); + + while (db_step(stmt)) { + struct db_stmt *update_stmt; + u64 id; + + id = db_col_u64(stmt, "e.id"); + update_stmt = db_prepare_v2(db, SQL("UPDATE chain_events SET" + " blockheight = ?" + " WHERE id = ?")); + + db_bind_int(update_stmt, 0, ev->blockheight); + db_bind_u64(update_stmt, 1, id); + db_exec_prepared_v2(take(update_stmt)); + } + + tal_free(stmt); +} + bool log_chain_event(struct db *db, const struct account *acct, struct chain_event *e) diff --git a/plugins/bkpr/recorder.h b/plugins/bkpr/recorder.h index f54c74c45..228ce07b7 100644 --- a/plugins/bkpr/recorder.h +++ b/plugins/bkpr/recorder.h @@ -171,6 +171,14 @@ char *update_channel_onchain_fees(const tal_t *ctx, * The point of this is to allow us to prune data, eventually */ void maybe_mark_account_onchain(struct db *db, struct account *acct); +/* When we make external deposits from the wallet, we don't + * count them until any output that was spent *into* them is + * confirmed onchain. + * + * This method updates the blockheight on these events to the + * height an input was spent into */ +void maybe_closeout_external_deposits(struct db *db, struct chain_event *ev); + /* Log a channel event */ void log_channel_event(struct db *db, const struct account *acct, diff --git a/tests/test_bookkeeper.py b/tests/test_bookkeeper.py index 091fe60a7..cb0a12920 100644 --- a/tests/test_bookkeeper.py +++ b/tests/test_bookkeeper.py @@ -1,17 +1,31 @@ from fixtures import * # noqa: F401,F403 +from decimal import Decimal from pyln.client import Millisatoshi +from fixtures import TEST_NETWORK from utils import ( - sync_blockheight + sync_blockheight, wait_for, only_one ) +import os import pytest +import unittest + + +def find_tags(evs, tag): + return [e for e in evs if e['tag'] == tag] + + +def find_first_tag(evs, tag): + ev = find_tags(evs, tag) + assert len(ev) > 0 + return ev[0] @pytest.mark.developer("dev-ignore-htlcs") def test_closing_trimmed_htlcs(node_factory, bitcoind, executor): l1, l2 = node_factory.line_graph(2) - # give l2 an output!? + # Send l2 funds via the channel l1.pay(l2, 11000000) l1.rpc.dev_ignore_htlcs(id=l2.info['id'], ignore=True) @@ -33,29 +47,21 @@ def test_closing_trimmed_htlcs(node_factory, bitcoind, executor): sync_blockheight(bitcoind, [l1]) l1.daemon.wait_for_log(r'All outputs resolved.*') - def _find_tags(evs, tag): - return [e for e in evs if e['tag'] == tag] - - def _find_first_tag(evs, tag): - ev = _find_tags(evs, tag) - assert len(ev) > 0 - return ev[0] - evs = l1.rpc.listaccountevents()['events'] - close = _find_first_tag(evs, 'channel_close') - delayed_to = _find_first_tag(evs, 'delayed_to_us') + close = find_first_tag(evs, 'channel_close') + delayed_to = find_first_tag(evs, 'delayed_to_us') # find the chain fee entry for the channel close - fees = _find_tags(evs, 'onchain_fee') + fees = find_tags(evs, 'onchain_fee') close_fee = [e for e in fees if e['txid'] == close['txid']] assert len(close_fee) == 1 assert Millisatoshi(close_fee[0]['credit']) + Millisatoshi(delayed_to['credit']) == Millisatoshi(close['debit']) # l2's fees should equal the trimmed htlc out evs = l2.rpc.listaccountevents()['events'] - close = _find_first_tag(evs, 'channel_close') - deposit = _find_first_tag(evs, 'deposit') - fees = _find_tags(evs, 'onchain_fee') + close = find_first_tag(evs, 'channel_close') + deposit = find_first_tag(evs, 'deposit') + fees = find_tags(evs, 'onchain_fee') close_fee = [e for e in fees if e['txid'] == close['txid']] assert len(close_fee) == 1 # sent htlc was too small, we lose it, rounded up to nearest sat @@ -80,30 +86,248 @@ def test_closing_subsat_htlcs(node_factory, bitcoind, chainparams): l1.daemon.wait_for_log('Broadcasting OUR_DELAYED_RETURN_TO_WALLET') bitcoind.generate_block(80) - def _find_tags(evs, tag): - return [e for e in evs if e['tag'] == tag] - - def _find_first_tag(evs, tag): - ev = _find_tags(evs, tag) - assert len(ev) > 0 - return ev[0] - sync_blockheight(bitcoind, [l1, l2]) evs = l1.rpc.listaccountevents()['events'] # check that closing equals onchain deposits + fees - close = _find_first_tag(evs, 'channel_close') - delayed_to = _find_first_tag(evs, 'delayed_to_us') - fees = _find_tags(evs, 'onchain_fee') + close = find_first_tag(evs, 'channel_close') + delayed_to = find_first_tag(evs, 'delayed_to_us') + fees = find_tags(evs, 'onchain_fee') close_fee = [e for e in fees if e['txid'] == close['txid']] assert len(close_fee) == 1 assert Millisatoshi(close_fee[0]['credit']) + Millisatoshi(delayed_to['credit']) == Millisatoshi(close['debit']) evs = l2.rpc.listaccountevents()['events'] - close = _find_first_tag(evs, 'channel_close') - deposit = _find_first_tag(evs, 'deposit') - fees = _find_tags(evs, 'onchain_fee') + close = find_first_tag(evs, 'channel_close') + deposit = find_first_tag(evs, 'deposit') + fees = find_tags(evs, 'onchain_fee') close_fee = [e for e in fees if e['txid'] == close['txid']] assert len(close_fee) == 1 # too small to fit, we lose them as miner fees assert close_fee[0]['credit'] == '333msat' assert Millisatoshi(close_fee[0]['credit']) + Millisatoshi(deposit['credit']) == Millisatoshi(close['debit']) + + +def test_bookkeeping_external_withdraws(node_factory, bitcoind): + """ Withdrawals to an external address shouldn't be included + in the income statements until confirmed""" + l1 = node_factory.get_node() + addr = l1.rpc.newaddr()['bech32'] + + amount = 1111111 + amount_msat = Millisatoshi(amount * 1000) + bitcoind.rpc.sendtoaddress(addr, amount / 10**8) + bitcoind.rpc.sendtoaddress(addr, amount / 10**8) + + bitcoind.generate_block(1) + wait_for(lambda: len(l1.rpc.listfunds()['outputs']) == 2) + + waddr = l1.bitcoin.rpc.getnewaddress() + + # Ok, now we send some funds to an external address + out = l1.rpc.withdraw(waddr, amount // 2) + + # Make sure bitcoind received the withdrawal + unspent = l1.bitcoin.rpc.listunspent(0) + withdrawal = [u for u in unspent if u['txid'] == out['txid']] + + assert withdrawal[0]['amount'] == Decimal('0.00555555') + incomes = l1.rpc.listincome()['income_events'] + # There should only be two income events: deposits to wallet + # for {amount} + assert len(incomes) == 2 + for inc in incomes: + assert inc['account'] == 'wallet' + assert inc['tag'] == 'deposit' + assert Millisatoshi(inc['credit']) == amount_msat + # The event should show up in the 'listaccountevents' however + events = l1.rpc.listaccountevents()['events'] + assert len(events) == 3 + external = [e for e in events if e['account'] == 'external'][0] + assert Millisatoshi(external['credit']) == Millisatoshi(amount // 2 * 1000) + + btc_balance = only_one(only_one(l1.rpc.listbalances()['accounts'])['balances']) + assert Millisatoshi(btc_balance['balance']) == amount_msat * 2 + + # Restart the node, issues a balance snapshot + # If we were counting these incorrectly, + # we'd have a new journal_entry + l1.restart() + + # the number of account + income events should be unchanged + incomes = l1.rpc.listincome()['income_events'] + assert len(find_tags(incomes, 'journal_entry')) == 0 + assert len(incomes) == 2 + events = l1.rpc.listaccountevents()['events'] + assert len(events) == 3 + assert len(find_tags(events, 'journal_entry')) == 0 + + # the wallet balance should be unchanged + btc_balance = only_one(only_one(l1.rpc.listbalances()['accounts'])['balances']) + assert Millisatoshi(btc_balance['balance']) == amount_msat * 2 + + # ok now we mine a block + bitcoind.generate_block(1) + sync_blockheight(bitcoind, [l1]) + + # expect the withdrawal to appear in the incomes + # and there should be an onchain fee + incomes = l1.rpc.listincome()['income_events'] + # 2 wallet deposits, 1 wallet withdrawal, 2 onchain_fees + assert len(incomes) == 5 + withdraw_amt = Millisatoshi(find_tags(incomes, 'withdrawal')[0]['debit']) + assert withdraw_amt == Millisatoshi(amount // 2 * 1000) + + fee_events = find_tags(incomes, 'onchain_fee') + fees = 0 + for e in fee_events: + # Millisatoshis cant be negative, so we reverse + # the credit/debit on the chain fee events and + # deal with them as ints instead of millisatoshis + fees -= int(e['credit'][:-4]) + fees += int(e['debit'][:-4]) + + # In sum, the fees are a net loss + assert fees > 0 + fees = Millisatoshi(fees) + + # wallet balance is decremented now + btc_balance = only_one(only_one(l1.rpc.listbalances()['accounts'])['balances']) + assert Millisatoshi(btc_balance['balance']) == amount_msat * 2 - withdraw_amt - fees + + +@unittest.skipIf(TEST_NETWORK != 'regtest', "External wallet support doesn't work with elements yet.") +@unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "Depends on sqlite3 database location") +def test_bookkeeping_external_withdraw_missing(node_factory, bitcoind): + """ Withdrawals to an external address turn up as + extremely large onchain_fees when they happen before + our accounting plugin is attached""" + l1 = node_factory.get_node() + + basedir = l1.daemon.opts.get("lightning-dir") + addr = l1.rpc.newaddr()['bech32'] + + amount = 1111111 + amount_msat = Millisatoshi(amount * 1000) + bitcoind.rpc.sendtoaddress(addr, amount / 10**8) + bitcoind.rpc.sendtoaddress(addr, amount / 10**8) + + bitcoind.generate_block(1) + wait_for(lambda: len(l1.rpc.listfunds()['outputs']) == 2) + + waddr = l1.bitcoin.rpc.getnewaddress() + + # 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 + assert len(l1.rpc.listincome()['income_events']) == 2 + # There are three account events: 2 wallet deposits, 1 external deposit + assert len(l1.rpc.listaccountevents()['events']) == 3 + + # 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 + assert len(l1.rpc.listincome()['income_events']) == 2 + # we're now missing the external deposit + events = l1.rpc.listaccountevents()['events'] + assert len(events) == 2 + assert len([e for e in events if e['account'] == 'external']) == 0 + assert len(find_tags(events, 'journal_entry')) == 0 + + # the wallet balance should be unchanged + btc_balance = only_one(only_one(l1.rpc.listbalances()['accounts'])['balances']) + assert Millisatoshi(btc_balance['balance']) == amount_msat * 2 + + # ok now we mine a block + bitcoind.generate_block(1) + sync_blockheight(bitcoind, [l1]) + + # expect the withdrawal to appear in the incomes + # and there should be an onchain fee + incomes = l1.rpc.listincome()['income_events'] + # 2 wallet deposits, 1 onchain_fee + assert len(incomes) == 3 + assert len(find_tags(incomes, 'withdrawal')) == 0 + + fee_events = find_tags(incomes, 'onchain_fee') + fees = 0 + for e in fee_events: + # Millisatoshis cant be negative, so we reverse + # the credit/debit on the chain fee events and + # deal with them as ints instead of millisatoshis + fees -= int(e['credit'][:-4]) + fees += int(e['debit'][:-4]) + + # In sum, the fees are a net loss + assert fees > 0 + fees = Millisatoshi(fees) + assert fees > Millisatoshi(amount // 2 * 1000) + + # wallet balance is decremented now + bal = only_one(only_one(l1.rpc.listbalances()['accounts'])['balances']) + assert Millisatoshi(bal['balance']) == amount_msat * 2 - fees + + +def test_bookkeeping_rbf_withdraw(node_factory, bitcoind): + """ If a withdraw to an external gets RBF'd, + it should *not* show up in our income ever. + (but it will show up in our account events) + """ + l1 = node_factory.get_node() + addr = l1.rpc.newaddr()['bech32'] + + amount = 1111111 + bitcoind.rpc.sendtoaddress(addr, amount / 10**8) + bitcoind.generate_block(1) + wait_for(lambda: len(l1.rpc.listfunds()['outputs']) == 1) + assert len(l1.rpc.listaccountevents()['events']) == 1 + assert len(l1.rpc.listincome()['income_events']) == 1 + + # Ok, now we send some funds to an external address + waddr = l1.bitcoin.rpc.getnewaddress() + out1 = l1.rpc.withdraw(waddr, amount // 2, feerate='253perkw') + mempool = bitcoind.rpc.getrawmempool(True) + assert len(list(mempool.keys())) == 1 + assert out1['txid'] in list(mempool.keys()) + + # another account event, still one income event + assert len(l1.rpc.listaccountevents()['events']) == 2 + assert len(l1.rpc.listincome()['income_events']) == 1 + + # unreserve the existing output + l1.rpc.unreserveinputs(out1['psbt'], 200) + + # resend the tx + out2 = l1.rpc.withdraw(waddr, amount // 2, feerate='1000perkw') + mempool = bitcoind.rpc.getrawmempool(True) + assert len(list(mempool.keys())) == 1 + assert out2['txid'] in list(mempool.keys()) + + # another account event, still one income event + assert len(l1.rpc.listaccountevents()['events']) == 3 + assert len(l1.rpc.listincome()['income_events']) == 1 + + # ok now we mine a block + bitcoind.generate_block(1) + sync_blockheight(bitcoind, [l1]) + + acct_evs = l1.rpc.listaccountevents()['events'] + externs = [e for e in acct_evs if e['account'] == 'external'] + assert len(externs) == 2 + assert externs[0]['outpoint'][:-2] == out1['txid'] + assert externs[0]['blockheight'] == 0 + assert externs[1]['outpoint'][:-2] == out2['txid'] + assert externs[1]['blockheight'] > 0 + + withdraws = find_tags(l1.rpc.listincome()['income_events'], 'withdrawal') + assert len(withdraws) == 1 + assert withdraws[0]['outpoint'][:-2] == out2['txid'] + + # make sure no onchain fees are counted for the replaced tx + fees = find_tags(acct_evs, 'onchain_fee') + assert len(fees) > 0 + for fee in fees: + assert fee['txid'] == out2['txid']