Feat/refactor-bcli #8820

dovgopoly · 2026-01-06T08:19:49Z

Closes #8738.

This PR refactors the bcli plugin from async to sync execution. It introduces a new run_bitcoin_cli function that blocks until bitcoin-cli completes.

All RPC commands are converted to use this synchronous pattern.

The PR also adds a command_err helper for consistent error handling, improves waitpid error reporting, fixes fd leaks by properly closing file descriptors, and removes all the now-unused async infrastructure (pending queues, priority scheduling, retry callbacks).

Tests are fixed accordingly.

sangbida · 2026-01-07T09:57:33Z

plugins/bcli.c

+	res->output_len = strlen(res->output);
+
+	/* Wait for child to exit */
+	while (waitpid(child, &status, 0) < 0 && errno == EINTR);


Might be good to have something like this here:

while (waitpid(child, &status, 0) < 0) { if (errno == EINTR) continue; plugin_err(plugin, "waitpid(%s) failed: %s", res->args, strerror(errno)); }

Yeah, added this.

Also I think I forgot to close a from descriptor in wait_and_check_bitcoind and run_bitcoin_cliv. Added this as well.

sangbida · 2026-01-07T10:36:32Z

plugins/bcli.c

         * we can attempt to prevent a crash if the 'getchaininfo' function returns
         * a lower height than the one we already know, by waiting for a short period.
         * However, I currently don't have a better idea on how to handle this situation. */
 	u32 *height UNUSED;


I think it's fine to leave this as it is and not use the optional last_height parameter. We're planning on getting rid of retries altogether inside bcli and have lightningd solely deal with retrying. So it feels a bit clunky to me to add retrying based on the last_height and ALSO have it be blocking to the rest of the plugin. We were never using it before and I don't think anyone has complained about this (yet) so it's probably not fatal?

sangbida · 2026-01-07T11:12:21Z

plugins/bcli.c

+	/* As of at least v0.15.1.0, bitcoind returns "success" but an empty
+	   string on a spent txout. */


I couldn't see this documented in the bitcoin-cli documentation https://developer.bitcoin.org/reference/rpc/gettxout.html , it might be good to test it out on cli to see if it every comes up with something else that's not an empty string but something like the string null?

That should be alright. I've found an issue raising this.

sangbida · 2026-01-07T11:29:59Z

plugins/bcli.c

+static struct command_result *command_err_badjson(struct command *cmd,
+						  struct bcli_result *res,
+						  const char *errmsg)
+{
+	char *err = tal_fmt(cmd, "%s: bad JSON: %s (%.*s)",
+			    res->args, errmsg, (int)res->output_len, res->output);
+	return command_done_err(cmd, BCLI_ERROR, err, NULL);
+}
+


I think this function is actually pretty good to have! Despite the plugin now being synchronous, it doesn't have to die, which plugin_err would do.

sangbida · 2026-01-07T11:32:20Z

plugins/bcli.c

+	if (res->exitstatus != 0)
+		return estimatefees_null_response(cmd);
+
+	tokens = json_parse_simple(res->output, res->output, res->output_len);


I found this pattern quite absurd because res->output is not a ctx! But it's used all over bcli in other places so it's not your fault at all! Have asked @rustyrussell about this and if it is intentional.

Asked him about this, this works okay in these scenarios because the lifetime of the pointers are the same so once the parent pointer is cleaned up any children are also cleaned up at the same time.

Add `command_err_badjson` helper for sync error handling, mirroring the async `command_err_bcli_badjson`. Store args string in `bcli_result` for consistent error messages.

Also rename command_err_badjson to generic command_err helper, since error messages aren't always about bad JSON (e.g., "command failed" for non-zero exit).

Add check_bitcoin_error to detect errors from bcli plugin and call fatal with a clear message identifying the failing method.

dovgopoly · 2026-01-13T15:34:51Z

tests/test_closing.py

    # We block l3 from seeing close, so it will try to reestablish.
    def no_new_blocks(req):
-        return {"error": "go away"}
+        return {"error": {"code": -8, "message": "Block height out of range"}}


If we revert with another code, it kills lightningd instantly, not making retry anymore.
I think it's expected to return a null response to the lightningd here.

After dddf995 it doesn't break the tests and we can remove these changes, but I decided to leave it as it is.

Co-authored-by: ShahanaFarooqui <[email protected]>

Changelog-Changed: bcli plugin now uses synchronous execution, simplifying bitcoin backend communication and improving error handling reliability.

dovgopoly · 2026-01-14T08:46:34Z

plugins/bcli.c

-		if (res->exitstatus == 8)
-			return getrawblockbyheight_notfound(cmd);
-		return command_err(cmd, res, "command failed");
+		return getrawblockbyheight_notfound(cmd);


I think returning a null response should be alright. I don't see the case we need to return error here. We can return an empty block and try again later. Tests are waiting the same behavior.

sangbida · 2026-01-14T04:17:26Z

plugins/bcli.c

+			}
+		}
+
+		sleep(1);


I know that the sleep was there in the async implementation of the plugin but I would add a test that runs this under load. So try to getrawblockbyheight and also try to run bcli commands at the same time.

sangbida · 2026-01-16T00:42:01Z

lightningd/bitcoind.c

+/* Check if response is an error and fail with clear message if so */
+static void check_bitcoin_error(struct bitcoind *bitcoind, const char *buf,
+				const jsmntok_t *toks, const char *method)
+{
+	const jsmntok_t *err_tok, *msg_tok;
+
+	err_tok = json_get_member(buf, toks, "error");
+	if (!err_tok)
+		return;
+
+	msg_tok = json_get_member(buf, err_tok, "message");
+	if (msg_tok)
+		fatal("%s: %.*s", method,
+		      json_tok_full_len(msg_tok), json_tok_full(buf, msg_tok));
+	else
+		fatal("%s: %.*s", method,
+		      json_tok_full_len(err_tok), json_tok_full(buf, err_tok));
+}
+


What was the motivation behind this function. This seems like it kills bitcoind if an error response or a timeout happens from bitcoind. It's not uncommon for bitcoind (the actual bitcoin core daemon) to go out for lunch and have a timeout.

@rustyrussell checked his node and these were top 5 bitcoind timeouts in msec:
21812
22053
22406
23600
30535
60324

Separate from this, commands like estimatefees and getblockhash can fail when they are syncing. In our meeting, Rusty also suggested that it would be good to change that function from a check_bitcoin_error to something like check/get_bitcoin_result? That way you can do a lot of common result handling inside that function and as a result of that tidy up some of the code.

If we're doing this we should probably be a bit more discerning with our retry behaviour:

sendrawtransaction - This one is probably okay without a retry because we have rebroadcasting mechanisms already in lightningd.

getchaininfo - Lightningd retry loops rely on getchaininfo succeeding I think, so I wonder if this needs more internal retries or if it's okay for this to just die on one missed rpc call?

getrawblockbyheight - we have a retry on this one

estimatefees - This does fail during sync but if it returns the estimatefees_null_response() that's probably okay since it's not an error?

getutxoout - I think this one is fine as well because we only crash on a broken json.

sangbida · 2026-01-16T00:45:32Z

plugins/bcli.c

+	}
+
+	strip_trailing_whitespace(res->output, res->output_len);
+	if (strlen(res->output) != 64)


nit: magic number

sangbida · 2026-01-16T00:45:52Z

plugins/bcli.c


-	return command_still_pending(cmd);
+		/* Try fetching from peers if bitcoind >= 23.0.0 */
+		if (bitcoind->version >= 230000) {


nit: magic number

sangbida · 2026-01-16T00:50:22Z

plugins/bcli.c

-		  const char *method,
-		  ...)
+static LAST_ARG_NULL struct bcli_result *
+run_bitcoin_cli(const tal_t *ctx,


I think you can use this function to refactor wait_and_check_bitcoind

sangbida reviewed Jan 7, 2026

View reviewed changes

dovgopoly force-pushed the feat/refactor-bcli branch 2 times, most recently from 2faf7ec to 8512a1d Compare January 9, 2026 22:31

dovgopoly requested a review from sangbida January 9, 2026 22:33

dovgopoly force-pushed the feat/refactor-bcli branch from 509f2f7 to 78c4f86 Compare January 9, 2026 22:47

dovgopoly marked this pull request as ready for review January 10, 2026 15:25

ShahanaFarooqui force-pushed the feat/refactor-bcli branch from 78c4f86 to a8079be Compare January 12, 2026 20:57

dovgopoly force-pushed the feat/refactor-bcli branch 4 times, most recently from bbc4e8d to b3632a5 Compare January 13, 2026 14:10

dovgopoly added 5 commits January 13, 2026 16:13

bcli: add synchronous run_bitcoin_cli for future refactor

598b984

bcli: convert getchaininfo to synchronous execution

5138815

bcli: convert getutxout to synchronous execution

5b48c8a

bcli: convert sendrawtransaction to synchronous execution

5c65ab6

bcli: convert estimatefees to synchronous execution

fa77e4b

Add `command_err_badjson` helper for sync error handling, mirroring the async `command_err_bcli_badjson`. Store args string in `bcli_result` for consistent error messages.

dovgopoly force-pushed the feat/refactor-bcli branch from b3632a5 to abf4d82 Compare January 13, 2026 14:13

dovgopoly added 2 commits January 13, 2026 17:30

bcli: convert getrawblockbyheight to synchronous execution

eb449e5

Also rename command_err_badjson to generic command_err helper, since error messages aren't always about bad JSON (e.g., "command failed" for non-zero exit).

lightningd: add error checking for bcli plugin responses

57d5946

Add check_bitcoin_error to detect errors from bcli plugin and call fatal with a clear message identifying the failing method.

dovgopoly force-pushed the feat/refactor-bcli branch from abf4d82 to ae11557 Compare January 13, 2026 15:31

dovgopoly commented Jan 13, 2026

View reviewed changes

dovgopoly and others added 3 commits January 13, 2026 23:07

pytest: fix bcli tests after sync refactor

8c0d9e5

Co-authored-by: ShahanaFarooqui <[email protected]>

bcli: remove unused async code after sync refactor

ef431c4

Changelog-Changed: bcli plugin now uses synchronous execution, simplifying bitcoin backend communication and improving error handling reliability.

bcli: return "not found" on any getblockhash exit status

dddf995

dovgopoly force-pushed the feat/refactor-bcli branch from ae11557 to dddf995 Compare January 13, 2026 21:07

dovgopoly commented Jan 14, 2026

View reviewed changes

sangbida reviewed Jan 16, 2026

View reviewed changes

		/* As of at least v0.15.1.0, bitcoind returns "success" but an empty
		string on a spent txout. */

Feat/refactor-bcli #8820

Are you sure you want to change the base?

Feat/refactor-bcli #8820

Conversation

dovgopoly commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dovgopoly Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dovgopoly Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dovgopoly commented Jan 6, 2026 •

edited

Loading

dovgopoly Jan 9, 2026 •

edited

Loading

dovgopoly Jan 14, 2026 •

edited

Loading