Now we've found all the issues, the latency spike (4 seconds on my laptop)
for querying 2M elements remains.
Restore the limited sampling which we reverted, but make it 10,000 now.
This doesn't help our worst-case latency, because sql still asks for all 2M entries on
first access. We address that next.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This reverts commit 1dda0c0753 so we can test
what its like to be flooded with logs again.
This benefits from other improvements we've made this release, to handling
plugin input (i.e. converting to use common/jsonrpc_io), so this doesn't
make much difference.
tests/test_coinmoves.py::test_generate_coinmoves (100,000, sqlite3):
Time (from start to end of l2 node): 211 seconds
Worst latency: 108 seconds
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This reverts `bookkeeper: only read listchannelmoves 1000 entries at a time.` commit,
so we can properly fix the scalability in the coming patches.
tests/test_coinmoves.py::test_generate_coinmoves (100,000):
Time (from start to end of l2 node): 207 seconds
Worst latency: 106 seconds
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I noticed this in the logs:
```
listinvoices: description/bolt11/bolt12 not found (
{"jsonrpc":"2)
```
And we make the same formatting mistake in several places.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I added amount_msat_accumulate for the "a+=b" case, but I was struggling
with a name for the subtractive equivalent. After some prompting, ChatGPT
suggested deduct.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Since we're synchronous, these only reach lightningd after we're done:
in the case of 1.6M channelmoves, that can give it major heartburn.
In practice, this reduces the first bkpr command on a fresh upgrade
from 349 to 235 seconds (but this was before other improvements we did
this release).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Plugins: `bookkeeper` reduced logging for large imports to increase speed.
Each header should only include the other headers it needs to compile;
`devtools/reduce-includes.sh */*.h` does this. The C files then need
additional includes if they don't compile.
And remove the entirely useless wire/onion_wire.h, which only serves to include wire/onion_wiregen.h.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This means we don't have to manually choose what to link against,
which is much of the complexity of our Makefiles: the compiler will
automatically use any object files it needs to link.
We already do this for ccan as libccan.a, now we have libcommon.a.
We don't link against it for *everything*, as some tests require their own
versions.
Notes:
1. I get rid of the weird plugins/test/Makefile2 (accidental commit?)
2. Many tests change due to update-mocks.
3. In some places I added the missing dependency on the Makefile itself, though most are in the next
patch.
Before:
Total program size: 221366528
Total tests size: 364243856
After:
Total program size: 190733656
Total tests size: 337880888
Build time from make clean (RUST=0) (includes building external libs):
Before:
real 0m38.227000-44.245000(41.8222+/-1.6)s
user 3m2.105000-33.696000(23.1442+/-8.4)s
sys 0m35.054000-42.269000(39.7231+/-2)s
After:
real 0m38.944000-40.416000(40.1131+/-0.4)s
user 3m6.790000-17.159000(15.0571+/-2.8)s
sys 0m35.304000-37.336000(36.8942+/-0.57)s
Build time after touch config.vars (RUST=0):
Before:
real 0m18.928000-22.776000(21.5084+/-1.1)s
user 2m8.613000-36.567000(27.7281+/-7.7)s
sys 0m20.458000-23.436000(22.3963+/-0.77)s
After:
real 0m19.831000-21.862000(21.5528+/-0.58)s
user 2m15.361000-30.731000(28.4798+/-4.4)s
sys 0m21.056000-22.339000(22.0346+/-0.35)s
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rusty@rusty-Framework:~/devel/cvs/lightni
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 <rusty@rustcorp.com.au>
If we read all of them, we might get 1.6M at once (after initial
migration). Then we submit a few hundred thousand simultaneous
requests to lightningd, and it gets upset, queueing them all on the
xpay command hook and running out of memory.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: plugins: bookkeeper first invocation after migration from prior to 25.09 with very large databases will not crash.
Prior to 23.05, we used this tag to mark onchain to-self inputs we didn't
wait for (because they were too small). This fixes migration if that happened
(and we are debating whether we should re-introduce this!).
```
lightningd: FATAL SIGNAL 6 (version v25.09rc2)
0x100c8683 send_backtrace
common/daemon.c:33
0x100c876f crashdump
common/daemon.c:78
0x7fffb2080493 ???
???:0
0x7fffb1ab0cac ???
__pthread_kill_implementation+0x1bc:0
0x7fffb1a48a5b ???
__GI_raise+0x2b:0
0x7fffb1a2a3db ???
__GI_abort+0x153:0
0x100935b7 migrate_from_account_db
wallet/account_migration.c:424
0x10093ff7 db_migrate
wallet/db.c:1139
0x10096763 db_setup
wallet/db.c:1185
0x100a1bcb wallet_new
wallet/wallet.c:223
0x1004485f main
lightningd/lightningd.c:1311
0x7fffb1a2aba3 ???
__libc_start_call_main+0x93:0
0x7fffb1a2adeb ???
__libc_start_main_alias_1+0x1ab:0
0xffffffffffffffff ???
???:0
lightningd: Died with signal 6
```
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: https://github.com/ElementsProject/lightning/issues/8484
This requires us to turn "sql" calls into calls to a local db, which
means pulling in a lot of infrastructure. But it's possible.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
There will be no more missing events (and at initialization time, we will do
that as a migration).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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.
Since we no longer subscribe to the balance_snapshot event, we
need to create the wallet account at initialization, as callers
expect it to exist.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
This is not quite as efficient, perhaps, but in practice there are only
a handful of onchain fee records per account.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's account_get_chain_fees() without the ORDER BY clause, *and* it's
only used on the run-recorder.c test.
I also got ChatGPT to compare them, in case I missed something subtle.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Note that the test where we remove the database causes the bookkeeper
plugin to assert, since we have removed part (but not all!) of its data
by removing the datastore.
Once the transition to the datastore is complete, this can be restored.
Note that we destroy the request before receiving a response, which causes
a message in the trace span which was confusing our test.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When we don't have our own db, we're going to need to keep this information
in memory (and the datastore). As a first step, simply cache it in memory
and still write through to the db.
This introduces some changes:
1. The account structures are not temporary, but in the hash table (so don't steal them).
2. test_forward_pad_fees_and_cltv assumed ordering, which was a latent bug.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
In practice, it's always either find_account() or find_or_create_account().
This means account_add can be made internal: we rename it to
account_db_add.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
