Skip to content

Bookkeeper migration #8410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 63 commits into
base: master
Choose a base branch
from
Open

Bookkeeper migration #8410

wants to merge 63 commits into from

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Jul 11, 2025 •
edited
Loading

This builds on #8445

We do all the things:

  1. We put the coin movement notifications into the db, in separate tables.
  2. We implement listchainmoves and listchannelmoves (and wait commands) on top of this.
  3. We implement sql plugin support for these tables, and optimize it.
  4. We migrate the internal tables & annotations that bookkeeper does from its local db into the datastore.
  5. We add new (internal, for now) interfaces to inject "foreign" utxo deposits and spends.
  6. We enhance libplugin to allow sync commands at any time, to avoid rewriting everything!
  7. We move bookkeeper off the notifications onto the list commands, so no more replays!
  8. We convert bookkeeper to use the sql plugin commands to query the db.
  9. We migrate bookkeeper's accounts.db into our internal db

It needs some more testing, and I'm sure it dosn't pass CI, but it's ready for review!

Closes: #8393

@rustyrussell rustyrussell added this to the v25.09 milestone Jul 11, 2025
@madelinevibes madelinevibes added Status::Ready for Review The work has been completed and is now awaiting evaluation or approval. PLEASE clear CI 🫠 labels Aug 6, 2025
@cdecker
Copy link
Member

cdecker commented Aug 11, 2025

Cache cleared, restarting CI.

@cdecker
Copy link
Member

cdecker commented Aug 11, 2025

Can't restart CI since it is too old, a rebase will start it again though.

@rustyrussell rustyrussell force-pushed the guilt/bookkeeper-migrate branch from be18b01 to f649a49 Compare August 12, 2025 05:49
@rustyrussell rustyrussell requested a review from cdecker as a code owner August 12, 2025 05:49
@rustyrussell rustyrussell requested a review from niftynei August 12, 2025 05:56
@rustyrussell rustyrussell changed the title Bookkeeper refactors and cleanups in preparation for migration Bookkeeper migration Aug 12, 2025
niftynei
niftynei reviewed Aug 15, 2025
View reviewed changes
Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkpoint submission #1

@rustyrussell
patch enhance-migration-test.patch
f129179
@rustyrussell
db: helper for binding/reading bitcoin_outpoint.
42ca0bc 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
wallet: chain_moves and channel_moves tables.
abe17c2 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
wallet: routines to append to chain_moves and channel_moves tables.
91cc3c9 
Note that bookkeeper de-duplicates chain_moves: we need to too!  So we add
an index to make this efficient.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
wallet: update chain_moves and channel_moves tables from channel_id -…
00f74ba 
…> nonchannel in db when we close it.

This avoids us keeping references into closed channels.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
wallet: record all coin movements into chain_moves or channel_moves t…
f4900b2 
…ables.

We change notify_chain_mvt to wallet_save_chain_mvt, and
notify_channel_mvt to wallet_save_channel_mvt, which save to the db
and call the notifier themselves.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
common: fix values of enum mvt_tag since they're now embedded in the db.
c9b4875 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: reorder JSON fields into more coherent order.
d6b0695 
This is mainly for the coming list commands, but it's just more
logical to have the common fields at the top.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
wallet: add accessors to read chain_moves & channel_moves tables.
95bd08f 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: speed mapping from coinmoves table to channel for listcoi…
3ec27c6 
…nmoves.

Iterating through every peer and channel every time can be very slow
for large nodes, when calling wallet_coinmoves_extract for listcoinmoves.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: add listchainmoves and listchannelmoves commands.
2905d16 
This is where all the previous work pays off: we can access the coinmoves
in the db.

Changelog-Added: JSON-RPC: `listchainmoves` and `listchannelmoves` commands to access the audit log of coin movements.
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
pytest: test listcoinmoves / listchannelmoves for unilateral closes.
0b8e38d 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
pytest: test accounting is correct for onchain fulfills.
0aee3ef 
Note that the channel account on l1 doesn't account for the
onchain-fulfilled HTLC:

```
>       assert sum(channel1[channel_id]) == -msats_sent_to_2
E       assert -50000000000 == -50100000000
E        +  where -50000000000 = sum([0, -50000000000])
```

Lisa points out that this is accounted for in the chain moves, instead.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
pytest: test coinmoves in the case of unexpected fulfill.
5a0f708 
This happens if l1 doesn't get a signature, so it doesn't consider
the fulfill complete, but l2 does (and thus credits l1).

This is a trivial test, but will matter should we later correctly
account for channelmoves when onchain: in that case it will actually
hard to tell, in general, what HTLC(s) were fulfilled.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
pytest: test listcoinmoves / listchannelmoves during penalties.
8cadccb 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: add chainmoves and channelmoves to wait command.
910ebc6 
Only makes sense to wait on creation, since they neither are deleted
nor updated.

We also enhance the list commands to take the standard index options.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: JSON-RPC: `wait`: new subsystems `chainmoves` and `channelmoves`.
@rustyrussell
plugins/sql: add listchainmoves and listchannelmoves.
c8b7491 
And note the other commands in See Also section.

Note that this means handling the "outpoint" type.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: JSON-RPC: `sql` plugin now supports `chainmoves` and `channelmoves` tables.
@rustyrussell
sql: optimize listchainmoves and listchannelmoves accesses.
cb25faa 
We don't yet do the other list commands, as they are not append-only:
we would need to check deletes and updates.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
plugins/sql: use created_index as primary key, where available.
5d53d4e 
It's a unique integer, and very useful for querying changes.  Unlike
our generated rowid, it's *stable* across queries.

We still need an explicit rowid column for list commands which don't
(currently) have this.

Here's the documentation diff:

    @@ -85,69 +85,69 @@
     TABLES
     ------
     
    -Note that the first column of every table is a unique integer called `rowid`: this is used for related tables to refer to specific rows in their parent. sqlite3 usually has this as an implicit column, but we make it explicit as the implicit version is not allowed to be used as a foreign key.
    +Note that tables which have a `created_index` field use that as the primary key (and `rowid` is an alias to this), otherwise an explicit `rowid` integer primary key is generated, whose value changes on each refresh.  This field is used for related tables to refer to specific rows in their parent. (sqlite3 usually has this as an implicit column, but we make it explicit as the implicit version is not allowed to be used as a foreign key).
     
     The following tables are currently supported:
     - `bkpr_accountevents` (see lightning-bkpr-listaccountevents(7))
    @@ -119,14 +119,14 @@
       - `payment_id` (type `hex`, sqltype `BLOB`)
     
     - `chainmoves` indexed by `account_id` (see lightning-listchainmoves(7))
    -  - `created_index` (type `u64`, sqltype `INTEGER`)
    +  - `created_index` (type `u64`, sqltype `INTEGER PRIMARY KEY`)
       - `account_id` (type `string`, sqltype `TEXT`)
       - `credit_msat` (type `msat`, sqltype `INTEGER`)
       - `debit_msat` (type `msat`, sqltype `INTEGER`)
       - `timestamp` (type `u64`, sqltype `INTEGER`)
       - `primary_tag` (type `string`, sqltype `TEXT`)
       - related table `chainmoves_extra_tags`
    -    - `row` (reference to `chainmoves.rowid`, sqltype `INTEGER`)
    +    - `row` (reference to `chainmoves.created_index`, sqltype `INTEGER`)
         - `arrindex` (index within array, sqltype `INTEGER`)
         - `extra_tags` (type `string`, sqltype `TEXT`)
       - `peer_id` (type `pubkey`, sqltype `BLOB`)
    @@ -139,7 +139,7 @@
       - `blockheight` (type `u32`, sqltype `INTEGER`)
     
     - `channelmoves` indexed by `account_id` (see lightning-listchannelmoves(7))
    -  - `created_index` (type `u64`, sqltype `INTEGER`)
    +  - `created_index` (type `u64`, sqltype `INTEGER PRIMARY KEY`)
       - `account_id` (type `string`, sqltype `TEXT`)
       - `credit_msat` (type `msat`, sqltype `INTEGER`)
       - `debit_msat` (type `msat`, sqltype `INTEGER`)
    @@ -204,7 +204,7 @@
       - `last_stable_connection` (type `u64`, sqltype `INTEGER`)
     
     - `forwards` indexed by `in_channel` and `in_htlc_id` (see lightning-listforwards(7))
    -  - `created_index` (type `u64`, sqltype `INTEGER`)
    +  - `created_index` (type `u64`, sqltype `INTEGER PRIMARY KEY`)
       - `in_channel` (type `short_channel_id`, sqltype `TEXT`)
       - `in_htlc_id` (type `u64`, sqltype `INTEGER`)
       - `in_msat` (type `msat`, sqltype `INTEGER`)
    @@ -222,7 +222,7 @@
     
     - `htlcs` indexed by `short_channel_id` and `id` (see lightning-listhtlcs(7))
       - `short_channel_id` (type `short_channel_id`, sqltype `TEXT`)
    -  - `created_index` (type `u64`, sqltype `INTEGER`)
    +  - `created_index` (type `u64`, sqltype `INTEGER PRIMARY KEY`)
       - `updated_index` (type `u64`, sqltype `INTEGER`)
       - `id` (type `u64`, sqltype `INTEGER`)
       - `expiry` (type `u32`, sqltype `INTEGER`)
    @@ -242,7 +242,7 @@
       - `bolt12` (type `string`, sqltype `TEXT`)
       - `local_offer_id` (type `hash`, sqltype `BLOB`)
       - `invreq_payer_note` (type `string`, sqltype `TEXT`)
    -  - `created_index` (type `u64`, sqltype `INTEGER`)
    +  - `created_index` (type `u64`, sqltype `INTEGER PRIMARY KEY`)
       - `updated_index` (type `u64`, sqltype `INTEGER`)
       - `pay_index` (type `u64`, sqltype `INTEGER`)
       - `amount_received_msat` (type `msat`, sqltype `INTEGER`)
    @@ -408,7 +408,7 @@
       - `features` (type `hex`, sqltype `BLOB`)
     
     - `sendpays` indexed by `payment_hash` (see lightning-listsendpays(7))
    -  - `created_index` (type `u64`, sqltype `INTEGER`)
    +  - `created_index` (type `u64`, sqltype `INTEGER PRIMARY KEY`)
       - `id` (type `u64`, sqltype `INTEGER`)
       - `groupid` (type `u64`, sqltype `INTEGER`)
       - `partid` (type `u64`, sqltype `INTEGER`)

Changelog-Changed: Plugins: `sql` tables `forwards`, `htlcs`, `invoices`, `sendpays` all use `created_index` as their primary key (and `rowid` is now an alias to this).
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bookkeeper: infrastructure to keep descriptions (annotations) in mem …
47f6073 
…and datastore.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bkpr: use the in-memory event descriptions, not the database ones.
ead1cf3 
The new access APIs are more symmetrical:

1. edit_utxo_description -> add_utxo_description
2. add_payment_hash_desc -> add_payment_hash_description

And to read it, instead of accessing ->ev_desc (now removed) we use
chain_event_description() & channel_event_description(), threading bkpr though
as needed.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bkpr: infrastructure to keep rebalance pairs in mem & datastore.
e7b5e01 
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bookkeeper: use the in-memory rebalances.
64039f2 
Remove the rebalance field from channel_event, and use the
find_rebalance(bkpr, ev->db_id) to look it up instead.

chain_event's also had a `rebalance` field, but it was only ever set
(to false), never read.

Note: list_rebalances() was only used by tests, not a public API.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bkpr: pass bkpr struct into all functions in recorder.c.
a506f39 
We want to access it in stmt2chain_event, so plumb it through.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bkpr: add in-mem & datastore storage for external blockheights.
44b8904 
We won't be able to "UPDATE chain_events", so keep a separate record
of these blockheights, and lookup that when the blockheight is 0.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
pytest: use assert inside event testing.
9b94779 
Python's assert gives great analysis of what the differences are,
making debugging much easier.

So feed it dicts, not tuples, and simply do an assert.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: new internal-use commands to allow utxo spend / deposit i…
0f02db4 
…njection.

For the moment, we'll continue to use bookkeeper to monitor the
notifications to insert these (we don't have the internal infrastructure
for that, and actually these commands are probably better than using
notifications).

We hoist param_outpoint() into common code, since there are already
two uses.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
common: use "foreign" in extra_tags to indicate a chain movement is i…
089f57d 
…njected.

This allows the bookkeeper plugin to know it's not actually a channel account.

Remove the "ignored" tag from the schema too: we removed it previously.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bookkeeper: don't set origin account to "null" if that's specified as…
0c57437 
… transfer_from.

Before bkpr_listaccountevents() gave entries with origin like:

	{'account': "nifty's secret stash",
         'blockheight': 111,
         'credit_msat': 180000000,
         'currency': 'bcrt',
         'debit_msat': 0,
         'origin': 'null',
         'outpoint': 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa:0',
         'tag': 'deposit',
         'timestamp': 1679955976,
         'type': 'chain'},

Changelog-Changed: Plugins: "utxo_deposit" is allows to have missing `transfer_from`, and null is not considered an account name.
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bkpr: forward utxo_deposit / utxo_spend notifications to new injectut…
f6eeeb7 
…xodeposit / injectutxospend calls.

And thus we absorb them as normal when they come back as "foreign" entries.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
libplugin: allow sync interface to be used at all times.
8758613 
After much thought and mis-steps, I chose a simple solution: open another fd
for sync comms.  It's almost impossible to know what state the async one is in.

jsonrpc_request_sync() is enhanced to return a valid tal object, as the current
behaviour of returning a pointer to inside an array was surprising.

Changelog-Changed: libplugin: you can now call the synchronous API functions at any time (not just in the init callback).
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
plugins/bkpr/test/run-recorder: remove.
316f53f 
It's a great test, but it's very hard to simulate now we are going to be
going from the internal db.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bookkeeper: call refresh before processing any commands.
2dac9f0 
Rearrange all the JSON interfaces to call refresh_moves() (async)
before doing anything.

This does nothing for now, but it will be useful once we transition
from notifications to using the list commands.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bkpr: use list commands instead of subscribing to notifications.
558e48c 
This is reliable, meaning we should never get replayed events.

We have to reference count to make sure all commands are complete,
before we return.  In particular, annotating with descriptions can
involve several calls to list commands.  We need to give them the
results *after* this is all complete.

test_bookkeeping_descriptions() relied on log messages from
notifications, which now only happen when a command is called.  This
changes the test a bit.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
wallet: remove notification on duplicate chain entries.
2fdf0e5 
We don't need it now bookkeeper uses the list commands.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bkpr: helpers to query sql plugin for chainmoves and channelmoves.
3f6d55a 
We're going to be using this instead of our internal db.

I also made json_out_obj() take the str arg, as it didn't and I
expected it to.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bkpr: take, don't steal in new_channel_event.
53be0f7 
Cleaner (I'm about to hand it a sha256 on the stack).

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bkpr: convert all the local db sql queries into calls to sql plugin.
2909b1b 
With some help (and hinderance!) from ChatGPT: the field names
differ slightly from our internal db.

The particilar wrinkle is that we have to restrict all queries to
limit them to entries we've seen already.  Our code expects this (we
used to only enter it into the db when we processed it), and it would
otherwise be confusing if a sql query returned inconsistent results
because an event occurred while bookkeeper was processing.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bkpr: remove missing event handling.
c49d9f3 
There will be no more missing events (and at initialization time, we will do
that as a migration).

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
bookkeeper: remove database.
0ad83eb 
Changelog-Changed: Plugins: `bookkeeper` now uses the lightningd database, not "accounts.db".
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
pytest: restore bookkeeper to recover tests.
0e88b17 
Now handles when we remove the db.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
db: support mode where we don't want to create the db.
a835e67 
And gracefully fail for this case.

There's no such thing for Postgres, but that's because dbs need to be
set up by the admin.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
lightningd: migrate events from bookkeeper at startup.
863c5e2 
We take over the --bookkeeper-dir and --bookkeeper-db options, and
then if we can find the bookkeeper db we extract the records to
initialize our chain_moves and channel_moves tables.

Of course, bookkeeper now needs to not register those options.

When bookkeeper gets invoked the first time, it will reconstruct
everything from listchannelmoves and listcoinmoves.  It cannot
preserve manually-added descriptions, so we put those in the datastore
for it ready to go.

Note that the order of onchain_fee changes slightly from the original.
But this is fine.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
wallet: generate fixup chainmoves and channelmoves when first starting.
71babfc 
If we don't have an accountdb from bookkeeper:

1. Generate a deposit chain event for every confirmed UTXO.
2. Generate an open chain event for every open, confirmed channel.
3. Generate a push/lease event if necessary.
4. Generate a fixup "journal" entry if balance is different from initial.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
Copy link
Contributor Author

I've done some more work:

  1. Minor fixups from @niftynei review (mainly correcting tests and comments).
  2. Rebased onto master now part 1 is merged.
  3. Reworked final two commits (migrations) and implemented the full tests for them.

I appended the fixes needed, for later rebasing.

@rustyrussell rustyrussell force-pushed the guilt/bookkeeper-migrate branch 2 times, most recently from 41e21d6 to ef8cb22 Compare August 15, 2025 06:02
@rustyrussell
fixup!
5ab28b0 
These need to be rebased, but keeping separate for review.

1. uninitialized variables: thanks valgrind!
2. many header malorders.
@rustyrussell rustyrussell force-pushed the guilt/bookkeeper-migrate branch from ef8cb22 to 5ab28b0 Compare August 15, 2025 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niftynei niftynei niftynei left review comments

@cdecker cdecker Awaiting requested review from cdecker cdecker is a code owner

Assignees
No one assigned
Labels
Status::Ready for Review The work has been completed and is now awaiting evaluation or approval.
Projects
None yet
Milestone
v25.09
Development

Successfully merging this pull request may close these issues.

Move bookkeeper database into core
4 participants
@rustyrussell @cdecker @niftynei @madelinevibes