Fix for #5557

contrib/pyln-testing/pyln/testing/utils.py

@@ -1435,8 +1435,8 @@ def get_nodes(self, num_nodes, opts=None):
         return [j.result() for j in jobs]
 
     def get_node(self, node_id=None, options=None, dbfile=None,
-                 feerates=(15000, 11000, 7500, 3750), start=True,
-                 wait_for_bitcoind_sync=True, may_fail=False,
+                 bkpr_dbfile=None, feerates=(15000, 11000, 7500, 3750),
+                 start=True, wait_for_bitcoind_sync=True, may_fail=False,
                  expect_fail=False, cleandir=True, **kwargs):
         self.throttler.wait()
         node_id = self.get_node_id() if not node_id else node_id
@@ -1470,6 +1470,12 @@ def get_node(self, node_id=None, options=None, dbfile=None,
             with lzma.open(os.path.join('tests/data', dbfile), 'rb') as f:
                 out.write(f.read())
 
+        if bkpr_dbfile:
+            out = open(os.path.join(node.daemon.lightning_dir, TEST_NETWORK,
+                                    'accounts.sqlite3'), 'xb')
+            with lzma.open(os.path.join('tests/data', bkpr_dbfile), 'rb') as f:
+                out.write(f.read())
+
         if start:
             try:
                 node.start(wait_for_bitcoind_sync)

lightningd/channel.h

@@ -466,6 +466,13 @@ static inline bool channel_unsaved(const struct channel *channel)
 		&& channel->dbid == 0;
 }
 
+static inline bool channel_pre_open(const struct channel *channel)
+{
+	return channel->state == CHANNELD_AWAITING_LOCKIN
+		|| channel->state == DUALOPEND_OPEN_INIT
+		|| channel->state == DUALOPEND_AWAITING_LOCKIN;
+}
+
 static inline bool channel_active(const struct channel *channel)
 {
 	return channel->state != FUNDING_SPEND_SEEN

lightningd/coin_mvts.c

@@ -81,6 +81,13 @@ struct channel_coin_mvt *new_channel_mvt_routed_hout(const tal_t *ctx,
 				    hout->fees);
 }
 
+static bool report_chan_balance(const struct channel *chan)
+{
+	return (channel_active(chan)
+	       || chan->state == AWAITING_UNILATERAL)
+	       && !channel_pre_open(chan);
+}
+
 void send_account_balance_snapshot(struct lightningd *ld, u32 blockheight)
 {
 	struct balance_snapshot *snap = tal(NULL, struct balance_snapshot);
@@ -121,8 +128,7 @@ void send_account_balance_snapshot(struct lightningd *ld, u32 blockheight)
 	/* Add channel balances */
 	list_for_each(&ld->peers, p, list) {
 		list_for_each(&p->channels, chan, list) {
-			if (channel_active(chan)
-			    || chan->state == AWAITING_UNILATERAL) {
+			if (report_chan_balance(chan)) {
 				bal = tal(snap, struct account_balance);
 				bal->bip173_name = chainparams->lightning_hrp;
 				bal->acct_id = type_to_string(bal,

plugins/bkpr/db.c

@@ -15,6 +15,8 @@ struct migration {
 
 static struct plugin *plugin;
 
+static void migration_remove_dupe_lease_fees(struct plugin *p, struct db *db);
+
 /* Do not reorder or remove elements from this array.
  * It is used to migrate existing databases from a prevoius state, based on
  * string indicies */
@@ -99,6 +101,7 @@ static struct migration db_migrations[] = {
 	{SQL("ALTER TABLE chain_events ADD ev_desc TEXT DEFAULT NULL;"), NULL},
 	{SQL("ALTER TABLE channel_events ADD ev_desc TEXT DEFAULT NULL;"), NULL},
 	{SQL("ALTER TABLE channel_events ADD rebalance_id BIGINT DEFAULT NULL;"), NULL},
+	{NULL, migration_remove_dupe_lease_fees}
 };
 
 static bool db_migrate(struct plugin *p, struct db *db, bool *created)
@@ -141,6 +144,49 @@ static bool db_migrate(struct plugin *p, struct db *db, bool *created)
 	return current != orig;
 }
 
+static void migration_remove_dupe_lease_fees(struct plugin *p, struct db *db)
+{
+	struct db_stmt *stmt, *del_stmt;
+	u64 *last_acct_id;
+
+	stmt = db_prepare_v2(db, SQL("SELECT"
+				     "  id"
+				     ", account_id"
+				     " FROM channel_events"
+				     " WHERE tag = 'lease_fee'"
+				     " ORDER BY account_id"));
+	db_query_prepared(stmt);
+	last_acct_id = NULL;
+	while (db_step(stmt)) {
+		u64 id, acct_id;
+		id = db_col_u64(stmt, "id");
+		acct_id = db_col_u64(stmt, "account_id");
+
+		if (!last_acct_id) {
+			last_acct_id = tal(stmt, u64);
+			*last_acct_id = acct_id;
+			continue;
+		}
+
+		if (*last_acct_id != acct_id) {
+			*last_acct_id = acct_id;
+			continue;
+		}
+
+		plugin_log(plugin, LOG_INFORM,
+			   "Duplicate 'lease_fee' found for"
+			   " account %"PRIu64", deleting dupe",
+			   id);
+
+		/* same acct as last, we found a duplicate */
+		del_stmt = db_prepare_v2(db, SQL("DELETE FROM channel_events"
+						 " WHERE id=?"));
+		db_bind_u64(del_stmt, 0, id);
+		db_exec_prepared_v2(take(del_stmt));
+	}
+	tal_free(stmt);
+}
+
 /* Implement db_fatal, as a wrapper around fatal.
  * We use a ifndef block so that it can get be
  * implemented in a test file first, if necessary */

tests/test_bookkeeper.py

@@ -1,6 +1,7 @@
 from fixtures import *  # noqa: F401,F403
 from decimal import Decimal
 from pyln.client import Millisatoshi
+from db import Sqlite3Db
 from fixtures import TEST_NETWORK
 from utils import (
     sync_blockheight, wait_for, only_one, first_channel_id, TIMEOUT
@@ -45,7 +46,7 @@ def test_bookkeeping_closing_trimmed_htlcs(node_factory, bitcoind, executor):
     bitcoind.generate_block(5)
     sync_blockheight(bitcoind, [l1])
     l1.daemon.wait_for_log('Broadcasting OUR_DELAYED_RETURN_TO_WALLET')
-    bitcoind.generate_block(20)
+    bitcoind.generate_block(20, wait_for_mempool=1)
     sync_blockheight(bitcoind, [l1])
     l1.daemon.wait_for_log(r'All outputs resolved.*')
 
@@ -708,3 +709,17 @@ def test_rebalance_tracking(node_factory, bitcoind):
     assert outbound_ev['debit_msat'] == Millisatoshi(1001)
     assert outbound_ev['credit_msat'] == Millisatoshi(0)
     assert outbound_ev['payment_id'] == pay_hash
+
+
+@unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "This test is based on a sqlite3 snapshot")
+def test_bookkeeper_lease_fee_dupe_migration(node_factory):
+    """ Check that if there's duplicate lease_fees, we remove them"""
+
+    l1 = node_factory.get_node(bkpr_dbfile='dupe_lease_fee.sqlite3.xz')
+
+    wait_for(lambda: l1.daemon.is_in_log('Duplicate \'lease_fee\' found for account'))
+
+    accts_db_path = os.path.join(l1.lightning_dir, TEST_NETWORK, 'accounts.sqlite3')
+    accts_db = Sqlite3Db(accts_db_path)
+
+    assert accts_db.query('SELECT tag from channel_events where tag = \'lease_fee\';') == [{'tag': 'lease_fee'}]

tests/test_opening.py

@@ -1513,6 +1513,124 @@ def test_buy_liquidity_ad_no_v2(node_factory, bitcoind):
                            compact_lease='029a002d000000004b2003e8')
 
 
+@pytest.mark.openchannel('v2')
+def test_v2_replay_bookkeeping(node_factory, bitcoind):
+    """ Test that your bookkeeping for a liquidity ad is good
+        even if we replay the opening and locking tx!
+    """
+
+    opts = [{'funder-policy': 'match', 'funder-policy-mod': 100,
+             'lease-fee-base-sat': '100sat', 'lease-fee-basis': 100,
+             'rescan': 10, 'funding-confirms': 6, 'may_reconnect': True},
+            {'funder-policy': 'match', 'funder-policy-mod': 100,
+             'lease-fee-base-sat': '100sat', 'lease-fee-basis': 100,
+             'may_reconnect': True}]
+    l1, l2, = node_factory.get_nodes(2, opts=opts)
+    amount = 500000
+    feerate = 2000
+
+    l1.fundwallet(amount * 100)
+    l2.fundwallet(amount * 100)
+
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+    rates = l1.rpc.dev_queryrates(l2.info['id'], amount, amount)
+    wait_for(lambda: len(l1.rpc.listpeers(l2.info['id'])['peers']) == 0)
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+
+    # l1 leases a channel from l2
+    l1.rpc.fundchannel(l2.info['id'], amount, request_amt=amount,
+                       feerate='{}perkw'.format(feerate),
+                       compact_lease=rates['compact_lease'])
+
+    # add the funding transaction
+    bitcoind.generate_block(4, wait_for_mempool=1)
+
+    l1.restart()
+
+    bitcoind.generate_block(2)
+    l1.daemon.wait_for_log('to CHANNELD_NORMAL')
+
+    chan_id = first_channel_id(l1, l2)
+    ev_tags = [e['tag'] for e in l1.rpc.bkpr_listaccountevents(chan_id)['events']]
+    assert 'lease_fee' in ev_tags
+
+    # This should work ok
+    l1.rpc.bkpr_listbalances()
+
+    bitcoind.generate_block(2)
+    sync_blockheight(bitcoind, [l1])
+
+    l1.restart()
+
+    chan_id = first_channel_id(l1, l2)
+    ev_tags = [e['tag'] for e in l1.rpc.bkpr_listaccountevents(chan_id)['events']]
+    assert 'lease_fee' in ev_tags
+
+    l1.rpc.close(l2.info['id'], 1)
+    bitcoind.generate_block(6, wait_for_mempool=1)
+
+    l1.daemon.wait_for_log(' to ONCHAIN')
+    l2.daemon.wait_for_log(' to ONCHAIN')
+
+    # This should not crash
+    l1.rpc.bkpr_listbalances()
+
+
+@pytest.mark.openchannel('v2')
+def test_buy_liquidity_ad_check_bookkeeping(node_factory, bitcoind):
+    """ Test that your bookkeeping for a liquidity ad is good."""
+
+    opts = [{'funder-policy': 'match', 'funder-policy-mod': 100,
+             'lease-fee-base-sat': '100sat', 'lease-fee-basis': 100,
+             'rescan': 10, 'disable-plugin': 'bookkeeper',
+             'funding-confirms': 6, 'may_reconnect': True},
+            {'funder-policy': 'match', 'funder-policy-mod': 100,
+             'lease-fee-base-sat': '100sat', 'lease-fee-basis': 100,
+             'may_reconnect': True}]
+    l1, l2, = node_factory.get_nodes(2, opts=opts)
+    amount = 500000
+    feerate = 2000
+
+    l1.fundwallet(amount * 100)
+    l2.fundwallet(amount * 100)
+
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+    rates = l1.rpc.dev_queryrates(l2.info['id'], amount, amount)
+    wait_for(lambda: len(l1.rpc.listpeers(l2.info['id'])['peers']) == 0)
+    l1.rpc.connect(l2.info['id'], 'localhost', l2.port)
+
+    # l1 leases a channel from l2
+    l1.rpc.fundchannel(l2.info['id'], amount, request_amt=amount,
+                       feerate='{}perkw'.format(feerate),
+                       compact_lease=rates['compact_lease'])
+
+    # add the funding transaction
+    bitcoind.generate_block(4, wait_for_mempool=1)
+
+    l1.stop()
+    del l1.daemon.opts['disable-plugin']
+    l1.start()
+
+    bitcoind.generate_block(2)
+    l1.daemon.wait_for_log('to CHANNELD_NORMAL')
+
+    chan_id = first_channel_id(l1, l2)
+    ev_tags = [e['tag'] for e in l1.rpc.bkpr_listaccountevents(chan_id)['events']]
+    assert 'lease_fee' in ev_tags
+
+    # This should work ok
+    l1.rpc.bkpr_listbalances()
+
+    l1.rpc.close(l2.info['id'], 1)
+    bitcoind.generate_block(6, wait_for_mempool=1)
+
+    l1.daemon.wait_for_log(' to ONCHAIN')
+    l2.daemon.wait_for_log(' to ONCHAIN')
+
+    # This should not crash
+    l1.rpc.bkpr_listbalances()
+
+
 def test_scid_alias_private(node_factory, bitcoind):
     """Test that we don't allow use of real scid for scid_alias-type channels"""
     l1, l2, l3 = node_factory.line_graph(3, fundchannel=False, opts=[{}, {},