From 4797a078761e8d1b4e0d8411c73653e66a7644d5 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 21 Oct 2025 14:58:51 +1030 Subject: [PATCH 1/3] pytest: test (failing) for bkpr-listincome filtering times on onchain events. Signed-off-by: Rusty Russell --- tests/test_bookkeeper.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/test_bookkeeper.py b/tests/test_bookkeeper.py index c7ba58b212f5..c9ddf9e6afe0 100644 --- a/tests/test_bookkeeper.py +++ b/tests/test_bookkeeper.py @@ -1162,3 +1162,34 @@ def test_migration_no_bkpr(node_factory, bitcoind): 'is_rebalance': False, 'tag': 'journal_entry', 'type': 'channel'}] + + +@pytest.mark.xfail(strict=True) +def test_listincome_timebox(node_factory, bitcoind): + 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_mempool=1) + wait_for(lambda: len(l1.rpc.listfunds()['outputs']) == 1) + + waddr = bitcoind.rpc.getnewaddress() + + # Ok, now we send some funds to an external address, get change. + l1.rpc.withdraw(waddr, amount // 2) + bitcoind.generate_block(1, wait_for_mempool=1) + wait_for(lambda: len(l1.rpc.listfunds(spent=True)['outputs']) == 2) + + first_one = int(time.time()) + time.sleep(2) + + # Do another one, make sure we don't see it if we filter by timestamp. + bitcoind.rpc.sendtoaddress(addr, amount / 10**8) + + bitcoind.generate_block(1, wait_for_mempool=1) + wait_for(lambda: len(l1.rpc.listfunds()['outputs']) == 2) + + incomes = l1.rpc.bkpr_listincome(end_time=first_one)['income_events'] + assert [i for i in incomes if i['timestamp'] > first_one] == [] From 12e684a66ec8af7c94b5f7dc29662ce1613b8b28 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 21 Oct 2025 14:58:57 +1030 Subject: [PATCH 2/3] bookkeeper: save last timestamp to avoid another query in find_consolidated_fees. If the fees are not *all* of the fees (as we do in next patch), the query would be wrong. Plus, as the FIXME suggests, we should just save it as we're getting the fee_sums, not do a whole new query! Signed-off-by: Rusty Russell --- plugins/bkpr/incomestmt.c | 6 +----- plugins/bkpr/onchain_fee.c | 21 +++------------------ plugins/bkpr/onchain_fee.h | 5 ----- plugins/bkpr/recorder.h | 1 + 4 files changed, 5 insertions(+), 28 deletions(-) diff --git a/plugins/bkpr/incomestmt.c b/plugins/bkpr/incomestmt.c index fd3a7e452c08..faf1355292f9 100644 --- a/plugins/bkpr/incomestmt.c +++ b/plugins/bkpr/incomestmt.c @@ -290,11 +290,7 @@ static struct onchain_fee **find_consolidated_fees(const tal_t *ctx, fee->debit = AMOUNT_MSAT(0); fee->acct_name = tal_steal(fee, sums[i]->acct_name); fee->txid = *sums[i]->txid; - - fee->timestamp = - onchain_fee_last_timestamp(bkpr, sums[i]->acct_name, - sums[i]->txid); - + fee->timestamp = sums[i]->last_timestamp; tal_arr_expand(&fee_sums, fee); } diff --git a/plugins/bkpr/onchain_fee.c b/plugins/bkpr/onchain_fee.c index 4af85e259e0c..0bec506b13f7 100644 --- a/plugins/bkpr/onchain_fee.c +++ b/plugins/bkpr/onchain_fee.c @@ -366,10 +366,13 @@ static struct fee_sum **fee_sums_by_txid_and_account(const tal_t *ctx, sum->txid = tal_dup(sum, struct bitcoin_txid, &ofs[i]->txid); credit = debit = AMOUNT_MSAT(0); + sum->last_timestamp = 0; } ok = amount_msat_accumulate(&credit, ofs[i]->credit); assert(ok); ok = amount_msat_accumulate(&debit, ofs[i]->debit); + if (ofs[i]->timestamp > sum->last_timestamp) + sum->last_timestamp = ofs[i]->timestamp; } /* Final, if any */ @@ -679,24 +682,6 @@ struct fee_sum **find_account_onchain_fees(const tal_t *ctx, return fee_sums_by_txid_and_account(ctx, ofs); } -/* FIXME: Put this value into fee_sums! */ -u64 onchain_fee_last_timestamp(const struct bkpr *bkpr, - const char *acct_name, - const struct bitcoin_txid *txid) -{ - struct onchain_fee **ofs; - u64 timestamp = 0; - - ofs = account_get_chain_fees(tmpctx, bkpr, acct_name); - for (size_t i = 0; i < tal_count(ofs); i++) { - if (!bitcoin_txid_eq(&ofs[i]->txid, txid)) - continue; - if (ofs[i]->timestamp > timestamp) - timestamp = ofs[i]->timestamp; - } - return timestamp; -} - /* If we're freeing the entire hash table, remove destructors from * individual entries! */ static void ofees_hash_destroy(struct ofees_hash *ofees_hash) diff --git a/plugins/bkpr/onchain_fee.h b/plugins/bkpr/onchain_fee.h index 8b954b092194..5e3be5e5d6b8 100644 --- a/plugins/bkpr/onchain_fee.h +++ b/plugins/bkpr/onchain_fee.h @@ -67,11 +67,6 @@ struct fee_sum **find_account_onchain_fees(const tal_t *ctx, struct fee_sum **calculate_onchain_fee_sums(const tal_t *ctx, const struct bkpr *bkpr); -/* Find the last timestamp for the onchain fees for this txid + account */ -u64 onchain_fee_last_timestamp(const struct bkpr *bkpr, - const char *acct_name, - const struct bitcoin_txid *txid); - /* Update our onchain fees now? */ char *maybe_update_onchain_fees(const tal_t *ctx, struct command *cmd, diff --git a/plugins/bkpr/recorder.h b/plugins/bkpr/recorder.h index 3c807a7e14c1..c13c0f21458a 100644 --- a/plugins/bkpr/recorder.h +++ b/plugins/bkpr/recorder.h @@ -18,6 +18,7 @@ struct fee_sum { const char *acct_name; struct bitcoin_txid *txid; struct amount_msat fees_paid; + u64 last_timestamp; }; struct txo_pair { From 89a56bc64313f34c6c73557ad03a2bcc30712fc8 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 21 Oct 2025 16:25:28 +1030 Subject: [PATCH 3/3] bookkeeper: honor start and ent times when consolidating onchain fees. Fixes: https://github.com/ElementsProject/lightning/issues/8318 Reported-by: Shahaha Changelog-Fixed: Plugins: `bkpr_listincome` now honors `start_time` and `end_time` parameters for onchain fees. --- plugins/bkpr/incomestmt.c | 2 +- plugins/bkpr/onchain_fee.c | 6 ++++-- plugins/bkpr/onchain_fee.h | 4 +++- tests/test_bookkeeper.py | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/plugins/bkpr/incomestmt.c b/plugins/bkpr/incomestmt.c index faf1355292f9..d9041f56ba71 100644 --- a/plugins/bkpr/incomestmt.c +++ b/plugins/bkpr/incomestmt.c @@ -276,7 +276,7 @@ static struct onchain_fee **find_consolidated_fees(const tal_t *ctx, struct onchain_fee **fee_sums = tal_arr(ctx, struct onchain_fee *, 0); - sums = calculate_onchain_fee_sums(ctx, bkpr); + sums = calculate_onchain_fee_sums(ctx, bkpr, start_time, end_time); for (size_t i = 0; i < tal_count(sums); i++) { /* Find the last matching feerate's data */ diff --git a/plugins/bkpr/onchain_fee.c b/plugins/bkpr/onchain_fee.c index 0bec506b13f7..6cdce78ab3ca 100644 --- a/plugins/bkpr/onchain_fee.c +++ b/plugins/bkpr/onchain_fee.c @@ -383,11 +383,13 @@ static struct fee_sum **fee_sums_by_txid_and_account(const tal_t *ctx, } struct fee_sum **calculate_onchain_fee_sums(const tal_t *ctx, - const struct bkpr *bkpr) + const struct bkpr *bkpr, + u64 start_time, + u64 end_time) { struct onchain_fee **ofs; - ofs = list_chain_fees(tmpctx, bkpr); + ofs = list_chain_fees_timebox(tmpctx, bkpr, start_time, end_time); return fee_sums_by_txid_and_account(ctx, ofs); } diff --git a/plugins/bkpr/onchain_fee.h b/plugins/bkpr/onchain_fee.h index 5e3be5e5d6b8..3dbbbd2df15c 100644 --- a/plugins/bkpr/onchain_fee.h +++ b/plugins/bkpr/onchain_fee.h @@ -65,7 +65,9 @@ struct fee_sum **find_account_onchain_fees(const tal_t *ctx, /* Final all the onchain fees */ struct fee_sum **calculate_onchain_fee_sums(const tal_t *ctx, - const struct bkpr *bkpr); + const struct bkpr *bkpr, + u64 start_time, + u64 end_time); /* Update our onchain fees now? */ char *maybe_update_onchain_fees(const tal_t *ctx, diff --git a/tests/test_bookkeeper.py b/tests/test_bookkeeper.py index c9ddf9e6afe0..2beceb8462f6 100644 --- a/tests/test_bookkeeper.py +++ b/tests/test_bookkeeper.py @@ -1164,7 +1164,7 @@ def test_migration_no_bkpr(node_factory, bitcoind): 'type': 'channel'}] -@pytest.mark.xfail(strict=True) +@unittest.skipIf(TEST_NETWORK != 'regtest', "External wallet support doesn't work with elements yet.") def test_listincome_timebox(node_factory, bitcoind): l1 = node_factory.get_node() addr = l1.rpc.newaddr()['bech32']