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

@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. note that the event names have been changed; we renamed them so that it's a lot easier to figure out what would prompt a notifier to send this. Suggested-By: @chrisguida Changelog-Added: PLUGINS: `bookkeeper` now listens for two custom events: `utxo_deposit` and `utxo_spent`. 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!

utxo_created -> utxo_deposit utxo_spent -> utxo_spend

vincenzopalazzo

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

plugins/bkpr/bookkeeper.c

@chrisguida

This is fixing a bug that @chrisguida reported, when we notify a plugin we also notify a plugin that it is in the middle of initialization. So imagine the following use case: - 1. Plugin A sends get manifest - 2. cln makes a check and generates a warning - 3. Plugin A is subscribed to the warning or * notification - 4. Core Lightning gets upset because it receives a warning message but it was waiting for a init. I think it is still a bug in how Core Lightning handles the init, but also I think we should communicate with the plugin only if it is initialized. ``` lightningd-1 2024-04-23T23:20:34.154Z DEBUG plugin-clnrest: Notification: {'warning': {'level': 'warn', 'time': '1713914433.818230531', 'timestamp': '2024-04-23T23:20:33.818Z', 'source': 'plugin-bookkeeper', 'log': \"topic 'utxo_deposit' is not a known notification topic\"}} lightningd-1 2024-04-23T23:20:34.154Z DEBUG hsmd: new_client: 0 lightningd-1 2024-04-23T23:20:34.154Z **BROKEN** plugin-test_libplugin: Did not receive 'init' yet, but got 'warning' instead lightningd-1 2024-04-23T23:20:34.155Z INFO plugin-test_libplugin: Killing plugin: exited before we sent init lightningd-1 2024-04-23T23:20:34.155Z **BROKEN** plugin-test_libplugin: Plugin marked as important, shutting down lightningd! lightningd-1 2024-04-23T23:20:34.155Z DEBUG lightningd: io_break: lightningd_exit lightningd-1 2024-04-23T23:20:34.155Z DEBUG lightningd: io_loop: connectd_init Time-out: can't find [re.compile('Server started with public key')] in logs {'github_repository': 'ElementsProject/lightning', 'github_sha': '014c1eb383b0a65394cf166b3aa0174cf2077896', 'github_ref': 'refs/pull/7258/merge', 'github_ref_name': 'HEAD', 'github_run_id': 8808124001, 'github_head_ref': 'cguida/onchain_notif', 'github_run_number': 9395, 'github_base_ref': 'master', 'github_run_attempt': '1', 'testname': 'test_self_disable', 'start_time': 1713914432, 'end_time': 1713914612, 'outcome': 'fail'} ----------------------------- Captured stderr call ----------------------------- Did not receive 'init' yet, but got 'warning' insteadlightningd: lightningd/connect_control.c:767: connectd_init: Assertion `ret == ld->connectd' failed. lightningd: FATAL SIGNAL 6 (version 014c1eb-modded) 0x555b1dc2d6f3 send_backtrace common/daemon.c:33 0x555b1dc2d8de crashdump common/daemon.c:75 0x7fda4584251f ??? ???:0 0x7fda458969fc ??? ???:0 0x7fda45842475 ??? ???:0 0x7fda458287f2 ??? ???:0 0x7fda4582871a ??? ???:0 0x7fda45839e95 ??? ???:0 0x555b1db98f8e connectd_init lightningd/connect_control.c:767 0x555b1dbb0307 main lightningd/lightningd.c:1249 0x7fda45829d8f ??? ???:0 0x7fda45829e3f ??? ???:0 0x555b1db6fd64 ??? ???:0 0xffffffffffffffff ??? ???:0 ``` Reported-by: @chrisguida Co-Developed-by: @chrisguida Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

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...

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)) {


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

niftynei and others added 4 commits April 23, 2024 16:16

make event tags consistent

82795df

utxo_created -> utxo_deposit utxo_spent -> utxo_spend

fix pep8 complaints

99f70f8

rebase on latest master, update deprecated type_to_string calls

109da8d

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

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

rustyrussell left a comment

rustyrussell Apr 29, 2024

rustyrussell Apr 29, 2024

rustyrussell Apr 29, 2024

rustyrussell Apr 29, 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

Are you sure you want to change the base?

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

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