bkpr: add two custom notifications that we listen for #7258

chrisguida · 2024-04-23T21:33:36Z

This PR adds two new event types that bookkeeper listens for in order to track on-chain movements in external wallets.

This API is currently implemented by the plugin Smaug, which depends on this PR being merged before a release can be cut.

tests/plugins/bookkeeper_custom_coins.py

vincenzopalazzo

Looked inside the CI failure and I think I found the problem

plugins/bkpr/bookkeeper.c

vincenzopalazzo

Concept ACK

I left a comment

vincenzopalazzo · 2024-04-26T15:56:30Z

plugins/bkpr/README.md

+
+See the doc/PLUGINS.md#coin_movement section on the message that CLN emits for us to process.
+
+// FIXME: add more detailed documenation for how bookkeeper works.


Suggested change

// FIXME: add more detailed documenation for how bookkeeper works.

// FIXME: add more detailed documenation for how bookkeeper works.

This can be removed?

I think it should stay until there's more detailed documentation, currently the README only describes how the 2 new event types work. But up to @niftynei really

plugins/bkpr/bookkeeper.c

vincenzopalazzo · 2024-04-26T16:01:54Z

plugins/bkpr/recorder.c

@@ -1935,11 +1935,12 @@ char *maybe_update_onchain_fees(const tal_t *ctx, struct db *db,
 }

 void maybe_closeout_external_deposits(struct db *db,
-			              struct chain_event *ev)
+			              struct bitcoin_txid *txid,


Suggested change

struct bitcoin_txid *txid,

const struct bitcoin_txid *txid,

usually we use const here

vincenzopalazzo · 2024-04-26T16:02:06Z

plugins/bkpr/recorder.h

@@ -208,7 +208,9 @@ void add_payment_hash_desc(struct db *db,
 *
 * This method updates the blockheight on these events to the
 * height an input was spent into */
-void maybe_closeout_external_deposits(struct db *db, struct chain_event *ev);
+void maybe_closeout_external_deposits(struct db *db,
+				      struct bitcoin_txid *txid,


rustyrussell

Minor change, I think the first commit is overzealous, and I'd like the fixup folded so we don't have an intermediary commit using type_to_strings which doesn't compile.

rustyrussell · 2024-04-29T16:47:24Z

plugins/bkpr/recorder.c

 		     const struct account *acct,
 		     struct chain_event *e)
 {
 	struct db_stmt *stmt;

 	/* We're responsible for de-duping chain events! */
-	if (find_chain_event(e, db, acct,
+	if (find_chain_event(ctx, db, acct,


This should actually be tmpctx!

And ctx arg is unnecessary...

wow yeah, lots of gymnastics on this one. fixing..

rustyrussell · 2024-04-29T16:47:43Z

plugins/bkpr/recorder.c

@@ -2040,7 +2041,7 @@ bool log_chain_event(struct db *db,
 	db_exec_prepared_v2(stmt);
 	e->db_id = db_last_insert_id_v2(stmt);
 	e->acct_db_id = acct->db_id;
-	e->acct_name = tal_strdup(e, acct->name);
+	e->acct_name = tal_strdup(ctx, acct->name);


This change looks wrong?

rustyrussell · 2024-04-29T16:56:09Z

lightningd/plugin.c

@@ -2415,7 +2415,7 @@ bool plugin_single_notify(struct plugin *p,
 			  const struct jsonrpc_notification *n TAKES)
 {
 	bool interested;
-	if (plugin_subscriptions_contains(p, n->method)) {
+	if (p->plugin_state == INIT_COMPLETE && plugin_subscriptions_contains(p, n->method)) {


@chrisguida

It might be nice to let the bookkeeper keep track of external accounts as well as the internal onchain wallet? To this end, we add some new custom notifications, which the bookkeeper will ingest and add to its ledger. Suggested-By: @chrisguida Changelog-Added: PLUGINS: `bookkeeper` now listens for two custom events: `utxo_deposit` and `utxo_spend`. This allows for 3rd party plugins to send onchain coin events to the `bookkeeper`. See the new plugins/bkpr/README.md for details on how these work!

niftynei

Comments have been addressed and fixed up as requested!

tests/plugins/bookkeeper_custom_coins.py

niftynei · 2024-05-20T23:18:05Z

plugins/bkpr/bookkeeper.c

@@ -1724,10 +1724,10 @@ static struct command_result *json_utxo_deposit(struct command *cmd, const char

 	plugin_log(cmd->plugin, LOG_DBG, "%s (%s|%s) %s -%s %"PRIu64" %d %s",
 		   move_tag, ev.tag, ev.acct_name,
-		   type_to_string(tmpctx, struct amount_msat, &ev.credit),
-		   type_to_string(tmpctx, struct amount_msat, &ev.debit),
+		   fmt_amount_msat(tmpctx, ev.credit),


same as prior, this should be squashed into the original commit that makes these changes.

niftynei · 2024-07-01T18:33:03Z

plugins/bkpr/recorder.c

 		     const struct account *acct,
 		     struct chain_event *e)
 {
 	struct db_stmt *stmt;

 	/* We're responsible for de-duping chain events! */
-	if (find_chain_event(e, db, acct,
+	if (find_chain_event(ctx, db, acct,


wow yeah, lots of gymnastics on this one. fixing..

niftynei · 2024-07-01T18:33:08Z

plugins/bkpr/recorder.c

@@ -2040,7 +2041,7 @@ bool log_chain_event(struct db *db,
 	db_exec_prepared_v2(stmt);
 	e->db_id = db_last_insert_id_v2(stmt);
 	e->acct_db_id = acct->db_id;
-	e->acct_name = tal_strdup(e, acct->name);
+	e->acct_name = tal_strdup(ctx, acct->name);


niftynei · 2024-07-01T18:33:13Z

plugins/bkpr/recorder.h

@@ -208,7 +208,9 @@ void add_payment_hash_desc(struct db *db,
 *
 * This method updates the blockheight on these events to the
 * height an input was spent into */
-void maybe_closeout_external_deposits(struct db *db, struct chain_event *ev);
+void maybe_closeout_external_deposits(struct db *db,
+				      struct bitcoin_txid *txid,


chrisguida · 2024-07-02T19:55:24Z

Thanks for the fixups @niftynei!

@rustyrussell I think this one should be ready to go now :)

ElementsProject/lightning#7258 finally merged, so we need to switch to the new more consistent tag names and use the elementsproject fork instead of niftynei There are also some changes required for the new startup_regtest script.

vincenzopalazzo requested changes Apr 23, 2024

View reviewed changes

tests/plugins/bookkeeper_custom_coins.py Outdated Show resolved Hide resolved

tests/plugins/bookkeeper_custom_coins.py Show resolved Hide resolved

tests/plugins/bookkeeper_custom_coins.py Outdated Show resolved Hide resolved

chrisguida force-pushed the cguida/onchain_notif branch from 044807c to 109da8d Compare April 23, 2024 22:36

vincenzopalazzo reviewed Apr 24, 2024

View reviewed changes

plugins/bkpr/bookkeeper.c Show resolved Hide resolved

vincenzopalazzo approved these changes Apr 26, 2024

View reviewed changes

endothermicdev added this to the v24.05 milestone Apr 29, 2024

rustyrussell requested changes Apr 29, 2024

View reviewed changes

rustyrussell modified the milestones: v24.05, v24.08 Jun 9, 2024

niftynei force-pushed the cguida/onchain_notif branch from e5c2d2d to d48ea39 Compare July 1, 2024 18:36

niftynei reviewed Jul 1, 2024

View reviewed changes

rustyrussell merged commit 0e99f2e into ElementsProject:master Aug 9, 2024
39 checks passed

chrisguida mentioned this pull request Sep 26, 2024

update to new upstream CLN chrisguida/smaug#62

Merged

chrisguida mentioned this pull request Sep 28, 2024

merge upstream PRs chrisguida/smaug#60

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bkpr: add two custom notifications that we listen for #7258

bkpr: add two custom notifications that we listen for #7258

chrisguida commented Apr 23, 2024

vincenzopalazzo left a comment

vincenzopalazzo left a comment

vincenzopalazzo Apr 26, 2024

chrisguida Apr 26, 2024

vincenzopalazzo Apr 26, 2024

vincenzopalazzo Apr 26, 2024

niftynei Jul 1, 2024

rustyrussell left a comment

rustyrussell Apr 29, 2024

rustyrussell Apr 29, 2024

niftynei Jul 1, 2024

rustyrussell Apr 29, 2024

niftynei Jul 1, 2024

rustyrussell Apr 29, 2024

niftynei left a comment

niftynei May 20, 2024

niftynei Jul 1, 2024

niftynei Jul 1, 2024

niftynei Jul 1, 2024

chrisguida commented Jul 2, 2024


		See the doc/PLUGINS.md#coin_movement section on the message that CLN emits for us to process.

		// FIXME: add more detailed documenation for how bookkeeper works.

bkpr: add two custom notifications that we listen for #7258

bkpr: add two custom notifications that we listen for #7258

Conversation

chrisguida commented Apr 23, 2024

vincenzopalazzo left a comment

Choose a reason for hiding this comment

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyrussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niftynei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisguida commented Jul 2, 2024