bkpr: mark external deposits (withdraws) via blockheight when confirmed

We issue events for external deposits (withdrawals) before the tx is
confirmed in a block. To avoid double counting these, we don't count
them as confirmed/included until after they're confirmed. We do this
by keeping the blockheight as zero until the withdraw for the input for
them comes through.

Note that since we don't have any way to note when RBF'd withdraws
aren't eligible for block inclusion anymore, we don't really have a good
heuristic to trim them. Which is fine, they *will* show up in account
events however.

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) */

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,

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)

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,

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')
+    close = find_first_tag(evs, 'channel_close')
-    delayed_to = _find_first_tag(evs, 'delayed_to_us')
+    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')
+    close = find_first_tag(evs, 'channel_close')
-    deposit = _find_first_tag(evs, 'deposit')
+    deposit = find_first_tag(evs, 'deposit')
-    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
     # 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')
+    close = find_first_tag(evs, 'channel_close')
-    delayed_to = _find_first_tag(evs, 'delayed_to_us')
+    delayed_to = find_first_tag(evs, 'delayed_to_us')
-    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'])
 
     evs = l2.rpc.listaccountevents()['events']
-    close = _find_first_tag(evs, 'channel_close')
+    close = find_first_tag(evs, 'channel_close')
-    deposit = _find_first_tag(evs, 'deposit')
+    deposit = find_first_tag(evs, 'deposit')
-    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
     # 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']