bookkeeper: fix assert() which happens with parallel queries.
What the developer wrote
Authored by Rusty Russell
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 <rusty@rustcorp.com.au>
Changelog-Fixed: plugins: assertion crash in bookkeeper when fresh records arrive while multiple queries in progress.
What changed, and why it matters
This commit fixes a crash in Core Lightning's bookkeeper plugin. Under parallel queries, a bookkeeping record could arrive out of order, triggering an internal 'assertion' that immediately killed the plugin. The fix replaces the fatal assertion with a safe early return when an out-of-order race occurs, and removes the 'expected failure' marker from the related test so it now passes.
Apply the patch. The change is low-risk and defensive. Operators running bookkeeper with concurrent RPC workloads should upgrade to avoid the assertion crash. No immediate incident response is required unless the plugin crash is observed in logs.
Security signals we found
Denial-of-service condition: plugin aborts on assertion failure
Race condition between database writes and asynchronous RPC query replies
Fix removes fatal assertion in favor of defensive early return
Test previously marked as expected failure now enabled
Evidence from the diff
In plugins/bkpr/bookkeeper.c, parse_and_log_chain_move() and parse_and_log_channel_move() previously asserted that newly processed event db_ids were strictly greater than the tracked chainmoves_index/channelmoves_index. With concurrent queries, fresh records can be inserted into the database while multiple listchainmoves/listchannelmoves RPC replies are in flight, causing an event with db_id <= current index to be processed and aborting the daemon via assert(). The patch changes both assertions to conditional returns, treating the race as a no-op. The test_bkpr_parallel test’s @pytest.mark.xfail(strict=True) decorator is removed, confirming the race is now handled correctly.
Changed components
plugins/bkpr/bookkeeper.cparse_and_log_chain_move()parse_and_log_channel_move()tests/test_bookkeeper.py::test_bkpr_parallelInspect captured patch +7 / −5
diff --git a/plugins/bkpr/bookkeeper.c b/plugins/bkpr/bookkeeper.c
index 93f44e5c..e5e2e82f 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 10b194af..d22d924d 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):
Why this scored 43/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.