From ddaee9d0f34bbbb50ed7526bfd211a513a8a85ed Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Nov 2025 12:37:20 +1030 Subject: [PATCH 1/2] pytest: test for parallel bookkeeper queries. If both refresh new events, we will get an assertion: ``` ``` Signed-off-by: Rusty Russell --- tests/test_bookkeeper.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/test_bookkeeper.py b/tests/test_bookkeeper.py index 4186d917f990..10b194afa4be 100644 --- a/tests/test_bookkeeper.py +++ b/tests/test_bookkeeper.py @@ -1194,6 +1194,22 @@ def test_listincome_timebox(node_factory, bitcoind): incomes = l1.rpc.bkpr_listincome(end_time=first_one)['income_events'] assert [i for i in incomes if i['timestamp'] > first_one] == [] + +@pytest.mark.xfail(strict=True) +@unittest.skipIf(TEST_NETWORK != 'regtest', "Snapshots are bitcoin regtest.") +@unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "uses snapshots") +def test_bkpr_parallel(node_factory, bitcoind, executor): + """Bookkeeper could crash with parallel requests""" + bitcoind.generate_block(1) + l1 = node_factory.get_node(dbfile="l1-before-moves-in-db.sqlite3.xz", + options={'database-upgrade': True}) + + fut1 = executor.submit(l1.rpc.bkpr_listincome) + fut2 = executor.submit(l1.rpc.bkpr_listincome) + + fut1.result() + fut2.result() + # We save blockheights in storage, so make sure we restore them on restart! acctevents_before = l1.rpc.bkpr_listaccountevents() l1.restart() From 108f348aee70b8f7ceb83798053684fc7dabfab9 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 3 Nov 2025 12:37:22 +1030 Subject: [PATCH 2/2] bookkeeper: fix assert() which happens with parallel queries. ``` bookkeeper: plugins/bkpr/bookkeeper.c:1226: parse_and_log_chain_move: Assertion `e->db_id > bkpr->chainmoves_index' failed. bookkeeper: FATAL SIGNAL 6 (version v25.09-245-g901714b-modded) 0x5d7d8718b40f send_backtrace common/daemon.c:36 0x5d7d8718b4ab crashdump common/daemon.c:81 0x7a6086c4532f ??? ./signal/../sysdeps/unix/sysv/linux/x86_64/libc_sigaction.c:0 0x7a6086c9eb2c __pthread_kill_implementation ./nptl/pthread_kill.c:44 0x7a6086c9eb2c __pthread_kill_internal ./nptl/pthread_kill.c:78 0x7a6086c9eb2c __GI___pthread_kill ./nptl/pthread_kill.c:89 0x7a6086c4527d __GI_raise ../sysdeps/posix/raise.c:26 0x7a6086c288fe __GI_abort ./stdlib/abort.c:79 0x7a6086c2881a __assert_fail_base ./assert/assert.c:96 0x7a6086c3b516 __assert_fail ./assert/assert.c:105 0x5d7d8717505d parse_and_log_chain_move plugins/bkpr/bookkeeper.c:1226 0x5d7d871754f4 listchainmoves_done plugins/bkpr/bookkeeper.c:169 0x5d7d87182a4b handle_rpc_reply plugins/libplugin.c:1072 0x5d7d87182b5c rpc_conn_read_response plugins/libplugin.c:1361 0x5d7d871ba660 next_plan ccan/ccan/io/io.c:60 0x5d7d871bab31 do_plan ccan/ccan/io/io.c:422 0x5d7d871babee io_ready ccan/ccan/io/io.c:439 ``` Reported-by: @michael1011 Signed-off-by: Rusty Russell Changelog-Fixed: plugins: assertion crash in bookkeeper when fresh records arrive while multiple queries in progress. --- plugins/bkpr/bookkeeper.c | 11 +++++++---- tests/test_bookkeeper.py | 1 - 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/plugins/bkpr/bookkeeper.c b/plugins/bkpr/bookkeeper.c index 93f44e5c2296..e5e2e82f8e87 100644 --- a/plugins/bkpr/bookkeeper.c +++ b/plugins/bkpr/bookkeeper.c @@ -1222,8 +1222,10 @@ parse_and_log_chain_move(struct command *cmd, if (e->origin_acct) find_or_create_account(cmd, bkpr, e->origin_acct); - /* Make this visible for queries (we expect increasing!) */ - assert(e->db_id > bkpr->chainmoves_index); + /* Make this visible for queries (we expect increasing!). If we raced, this is not true. */ + if (e->db_id <= bkpr->chainmoves_index) + return; + bkpr->chainmoves_index = e->db_id; /* This event *might* have implications for account; @@ -1336,8 +1338,9 @@ parse_and_log_channel_move(struct command *cmd, " but no account exists %s", acct_name); - /* Make this visible for queries (we expect increasing!) */ - assert(e->db_id > bkpr->channelmoves_index); + /* Make this visible for queries (we expect increasing!). If we raced, this is not true. */ + if (e->db_id <= bkpr->channelmoves_index) + return; bkpr->channelmoves_index = e->db_id; /* Check for invoice desc data, necessary */ diff --git a/tests/test_bookkeeper.py b/tests/test_bookkeeper.py index 10b194afa4be..d22d924d3533 100644 --- a/tests/test_bookkeeper.py +++ b/tests/test_bookkeeper.py @@ -1195,7 +1195,6 @@ def test_listincome_timebox(node_factory, bitcoind): assert [i for i in incomes if i['timestamp'] > first_one] == [] -@pytest.mark.xfail(strict=True) @unittest.skipIf(TEST_NETWORK != 'regtest', "Snapshots are bitcoin regtest.") @unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "uses snapshots") def test_bkpr_parallel(node_factory, bitcoind, executor):