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>
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>
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.
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>
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>
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>
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>
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>
This will let us cache them gradually in account.c, and eventually
remove the database table.
We also change find_close_account() to find_close_account_name(),
and maybe_mark_account_onchain() into account_onchain_closeheight()
and account_update_closeheight();
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Rather than forcing them to wrap their parameters in a "payload"
sub-object, copy in params directly. We include the "origin" field
one level up, if they care.
The next patch restores compatibility for the one place we currently use
them, which is the pay plugin.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Deprecated: pyln-client: plugin custom notifications origins and payload (use parameters directly)
We still output the fields, they're just always the currency of the node.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: Plugins: `bookkeeper` now explicitly assumes every transaction is in the same currency as the node (true unless you added manually)
We're going to get rid of this concept, but the main change is that the
account_get_balance API can be drastically simplified:
account_get_credit_debit() accesses the raw fields, never fails, but
returns the a flag which tells us if the account doesn't actually have
any events.
The one place we care about the balance, calculate by hand. Then
account_get_balance() (and struct account_balance) can simply be
moved to th test.
Subtly, without the "GROUP BY" clause, you always get one row, even if
there are no rows (but the SUM are null).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Given an {outpoint}, sets the description on the matching outpoint (if exists).
Note that if no outpoint exists in bookkeeper, will return an empty list
Changleog-Added: PLUGINS: bookkeeper has a new RPC `bkrp-editdescriptionbyoutpoint` which will set/update a description for an outpoint creation event.
This takes an {payment_id} and {description}.
It looks for all chain + channel events that match
that {payment_id} and updates the description for those events.
We return all the updated events. If no events are updated, an empty
list is returned.
Changelog-Added: PLUGINS: bookkeeper has a new RPC `bkpr-editdescriptionbypaymentid` which will update the description for any event with matching payment_id
Without knowing what method was called, we can't have useful general logging
methods, so go through the pain of adding "const char *method" everywhere,
and add:
1. ignore_and_complete - we're done when jsonrpc returned
2. log_broken_and_complete - we're done, but emit BROKEN log.
3. plugin_broken_cb - if this happens, fail the plugin.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
When we used to allow cmd to be NULL, we had to hand the plugin
everywhere. We no longer do.
1. Various jsonrpc_ functions no longer need the plugin arg.
2. send_outreq no longer needs a plugin arg.
3. The init function takes a command, not a plugin.
4. Remove command_deprecated_in_nocmd_ok.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Rather than making the callers do this, make the invoice decoder perform
the various sanity checks.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
I used `amount_msat_eq(x, AMOUNT_MSAT(0))` because I forgot this
function existed. I probably missed it because the name is surprising,
so add "is" in there to make it clear it's a boolean function.
You'll note almost all the places which did use it are Eduardo's and
Lisa's code, so maybe it's just me.
Fix up a few places which I could use it, too.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
