From a7649a6b2a307bfbbd6b60677b4a9a0dd1eebc82 Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Mon, 5 Jan 2026 12:17:29 +0200 Subject: [PATCH 01/13] bcli: add synchronous `run_bitcoin_cli` for future refactor --- plugins/bcli.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/plugins/bcli.c b/plugins/bcli.c index cb99763e724a..b774d3d0b469 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -91,6 +91,13 @@ struct bitcoin_cli { void *stash; }; +/* Result of a synchronous bitcoin-cli call */ +struct bcli_result { + char *output; + size_t output_len; + int exitstatus; +}; + /* Add the n'th arg to *args, incrementing n and keeping args of size n+1 */ static void add_arg(const char ***args, const char *arg TAKES) { @@ -204,6 +211,71 @@ static char *args_string(const tal_t *ctx, const char **args, const char **stdin return ret; } +/* Synchronous execution of bitcoin-cli. + * Returns result with output and exit status. */ +static UNNEEDED struct bcli_result * +run_bitcoin_cliv(const tal_t *ctx, + struct plugin *plugin, + const char *method, + va_list ap) +{ + int in, from, status; + pid_t child; + const char **stdinargs; + const char **cmd; + struct bcli_result *res; + + stdinargs = tal_arr(ctx, const char *, 0); + cmd = gather_argsv(ctx, &stdinargs, method, ap); + + child = pipecmdarr(&in, &from, &from, cast_const2(char **, cmd)); + if (child < 0) + plugin_err(plugin, "%s exec failed: %s", cmd[0], strerror(errno)); + + /* Send rpcpass via stdin if configured */ + if (bitcoind->rpcpass) { + write_all(in, bitcoind->rpcpass, strlen(bitcoind->rpcpass)); + write_all(in, "\n", 1); + } + /* Send any additional stdin args */ + for (size_t i = 0; i < tal_count(stdinargs); i++) { + write_all(in, stdinargs[i], strlen(stdinargs[i])); + write_all(in, "\n", 1); + } + close(in); + + /* Read all output until EOF */ + res = tal(ctx, struct bcli_result); + res->output = grab_fd_str(res, from); + res->output_len = strlen(res->output); + + /* Wait for child to exit */ + while (waitpid(child, &status, 0) < 0 && errno == EINTR); + + if (!WIFEXITED(status)) + plugin_err(plugin, "%s died with signal %i", + args_string(tmpctx, cmd, stdinargs), WTERMSIG(status)); + + res->exitstatus = WEXITSTATUS(status); + + return res; +} + +static UNNEEDED LAST_ARG_NULL struct bcli_result * +run_bitcoin_cli(const tal_t *ctx, + struct plugin *plugin, + const char *method, ...) +{ + va_list ap; + struct bcli_result *res; + + va_start(ap, method); + res = run_bitcoin_cliv(ctx, plugin, method, ap); + va_end(ap); + + return res; +} + static char *bcli_args(const tal_t *ctx, struct bitcoin_cli *bcli) { return args_string(ctx, bcli->args, bcli->stdinargs); From 27b0d8c57ed37a1e0c16155c3cabd21395efd8fe Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Mon, 5 Jan 2026 15:14:14 +0200 Subject: [PATCH 02/13] bcli: convert `getchaininfo` to synchronous execution --- plugins/bcli.c | 54 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index b774d3d0b469..85140be46c43 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -213,7 +213,7 @@ static char *args_string(const tal_t *ctx, const char **args, const char **stdin /* Synchronous execution of bitcoin-cli. * Returns result with output and exit status. */ -static UNNEEDED struct bcli_result * +static struct bcli_result * run_bitcoin_cliv(const tal_t *ctx, struct plugin *plugin, const char *method, @@ -261,7 +261,7 @@ run_bitcoin_cliv(const tal_t *ctx, return res; } -static UNNEEDED LAST_ARG_NULL struct bcli_result * +static LAST_ARG_NULL struct bcli_result * run_bitcoin_cli(const tal_t *ctx, struct plugin *plugin, const char *method, ...) @@ -535,7 +535,7 @@ static struct command_result *process_getutxout(struct bitcoin_cli *bcli) return command_finished(bcli->cmd, response); } -static struct command_result *process_getblockchaininfo(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result *process_getblockchaininfo(struct bitcoin_cli *bcli) { const jsmntok_t *tokens; struct json_stream *response; @@ -904,16 +904,56 @@ static struct command_result *getchaininfo(struct command *cmd, * 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; + struct bcli_result *res; + const jsmntok_t *tokens; + struct json_stream *response; + bool ibd; + u32 headers, blocks; + const char *chain, *err; + if (!param(cmd, buf, toks, p_opt("last_height", param_number, &height), NULL)) return command_param_failed(); - start_bitcoin_cli(NULL, cmd, process_getblockchaininfo, false, - BITCOIND_HIGH_PRIO, NULL, - "getblockchaininfo", NULL); + res = run_bitcoin_cli(cmd, cmd->plugin, "getblockchaininfo", NULL); + if (res->exitstatus != 0) { + return command_done_err(cmd, BCLI_ERROR, + tal_fmt(cmd, "getblockchaininfo: %s", res->output), + NULL); + } - return command_still_pending(cmd); + tokens = json_parse_simple(cmd, res->output, res->output_len); + if (!tokens) { + return command_done_err(cmd, BCLI_ERROR, + tal_fmt(cmd, "getblockchaininfo: bad JSON: %s", + res->output), + NULL); + } + + err = json_scan(tmpctx, res->output, tokens, + "{chain:%,headers:%,blocks:%,initialblockdownload:%}", + JSON_SCAN_TAL(tmpctx, json_strdup, &chain), + JSON_SCAN(json_to_number, &headers), + JSON_SCAN(json_to_number, &blocks), + JSON_SCAN(json_to_bool, &ibd)); + if (err) { + return command_done_err(cmd, BCLI_ERROR, + tal_fmt(cmd, "getblockchaininfo: bad JSON: %s (%s)", + err, res->output), + NULL); + } + + if (bitcoind->dev_ignore_ibd) + ibd = false; + + response = jsonrpc_stream_success(cmd); + json_add_string(response, "chain", chain); + json_add_u32(response, "headercount", headers); + json_add_u32(response, "blockcount", blocks); + json_add_bool(response, "ibd", ibd); + + return command_finished(cmd, response); } /* Mutual recursion. */ From fc411b92c7aec8acfc8918cf91a08d52d70813c9 Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Mon, 5 Jan 2026 16:17:42 +0200 Subject: [PATCH 03/13] bcli: convert `getutxout` to synchronous execution --- plugins/bcli.c | 49 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index 85140be46c43..899cd1a845e5 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -496,7 +496,7 @@ static void json_add_null(struct json_stream *stream, const char *fieldname) json_add_primitive(stream, fieldname, "null"); } -static struct command_result *process_getutxout(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result *process_getutxout(struct bitcoin_cli *bcli) { const jsmntok_t *tokens; struct json_stream *response; @@ -926,7 +926,7 @@ static struct command_result *getchaininfo(struct command *cmd, tokens = json_parse_simple(cmd, res->output, res->output_len); if (!tokens) { return command_done_err(cmd, BCLI_ERROR, - tal_fmt(cmd, "getblockchaininfo: bad JSON: %s", + tal_fmt(cmd, "getblockchaininfo: bad JSON: cannot parse (%s)", res->output), NULL); } @@ -1120,6 +1120,11 @@ static struct command_result *getutxout(struct command *cmd, const jsmntok_t *toks) { const char *txid, *vout; + struct bcli_result *res; + const jsmntok_t *tokens; + struct json_stream *response; + struct bitcoin_tx_output output; + const char *err; /* bitcoin-cli wants strings. */ if (!param(cmd, buf, toks, @@ -1128,11 +1133,43 @@ static struct command_result *getutxout(struct command *cmd, NULL)) return command_param_failed(); - start_bitcoin_cli(NULL, cmd, process_getutxout, true, - BITCOIND_HIGH_PRIO, NULL, - "gettxout", txid, vout, NULL); + res = run_bitcoin_cli(cmd, cmd->plugin, "gettxout", txid, vout, NULL); - return command_still_pending(cmd); + /* As of at least v0.15.1.0, bitcoind returns "success" but an empty + string on a spent txout. */ + if (res->exitstatus != 0 || res->output_len == 0) { + response = jsonrpc_stream_success(cmd); + json_add_null(response, "amount"); + json_add_null(response, "script"); + return command_finished(cmd, response); + } + + tokens = json_parse_simple(cmd, res->output, res->output_len); + if (!tokens) { + return command_done_err(cmd, BCLI_ERROR, + tal_fmt(cmd, "gettxout: bad JSON: cannot parse (%s)", + res->output), + NULL); + } + + err = json_scan(tmpctx, res->output, tokens, + "{value:%,scriptPubKey:{hex:%}}", + JSON_SCAN(json_to_bitcoin_amount, + &output.amount.satoshis), /* Raw: bitcoind */ + JSON_SCAN_TAL(cmd, json_tok_bin_from_hex, + &output.script)); + if (err) { + return command_done_err(cmd, BCLI_ERROR, + tal_fmt(cmd, "gettxout: bad JSON: %s (%s)", + err, res->output), + NULL); + } + + response = jsonrpc_stream_success(cmd); + json_add_sats(response, "amount", output.amount); + json_add_string(response, "script", tal_hex(response, output.script)); + + return command_finished(cmd, response); } static void bitcoind_failure(struct plugin *p, const char *error_message) From 140387ee5f6445c32a516d05da9511fe347de30c Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Mon, 5 Jan 2026 16:26:58 +0200 Subject: [PATCH 04/13] bcli: convert `sendrawtransaction` to synchronous execution --- plugins/bcli.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index 899cd1a845e5..ce0d6c123bda 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -632,7 +632,7 @@ estimatefees_parse_feerate(struct bitcoin_cli *bcli, u64 *feerate) return NULL; } -static struct command_result *process_sendrawtransaction(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result *process_sendrawtransaction(struct bitcoin_cli *bcli) { struct json_stream *response; @@ -1094,6 +1094,8 @@ static struct command_result *sendrawtransaction(struct command *cmd, { const char *tx, *highfeesarg; bool *allowhighfees; + struct bcli_result *res; + struct json_stream *response; /* bitcoin-cli wants strings. */ if (!param(cmd, buf, toks, @@ -1107,12 +1109,26 @@ static struct command_result *sendrawtransaction(struct command *cmd, } else highfeesarg = NULL; - start_bitcoin_cli(NULL, cmd, process_sendrawtransaction, true, - BITCOIND_HIGH_PRIO, NULL, - "sendrawtransaction", - tx, highfeesarg, NULL); + res = run_bitcoin_cli(cmd, cmd->plugin, + "sendrawtransaction", tx, highfeesarg, NULL); - return command_still_pending(cmd); + /* This is useful for functional tests. */ + if (res->exitstatus) + plugin_log(cmd->plugin, LOG_DBG, + "sendrawtx exit %i (%s)", + res->exitstatus, + res->output); + + response = jsonrpc_stream_success(cmd); + json_add_bool(response, "success", + res->exitstatus == 0 || + res->exitstatus == RPC_TRANSACTION_ALREADY_IN_CHAIN); + json_add_string(response, "errmsg", + res->exitstatus ? + tal_strndup(cmd, res->output, res->output_len) + : ""); + + return command_finished(cmd, response); } static struct command_result *getutxout(struct command *cmd, From 62a7d558340545bd89b75d89fe50a0d2c9b5e9af Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Tue, 6 Jan 2026 10:16:12 +0200 Subject: [PATCH 05/13] bcli: convert `estimatefees` to synchronous execution 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. --- plugins/bcli.c | 184 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 134 insertions(+), 50 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index ce0d6c123bda..5174012bef58 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -96,6 +96,8 @@ struct bcli_result { char *output; size_t output_len; int exitstatus; + /* Command args string for error messages */ + const char *args; }; /* Add the n'th arg to *args, incrementing n and keeping args of size n+1 */ @@ -248,13 +250,14 @@ run_bitcoin_cliv(const tal_t *ctx, res = tal(ctx, struct bcli_result); res->output = grab_fd_str(res, from); res->output_len = strlen(res->output); + res->args = args_string(res, cmd, stdinargs); /* Wait for child to exit */ while (waitpid(child, &status, 0) < 0 && errno == EINTR); if (!WIFEXITED(status)) plugin_err(plugin, "%s died with signal %i", - args_string(tmpctx, cmd, stdinargs), WTERMSIG(status)); + res->args, WTERMSIG(status)); res->exitstatus = WEXITSTATUS(status); @@ -490,6 +493,15 @@ static struct command_result *command_err_bcli_badjson(struct bitcoin_cli *bcli, return command_done_err(bcli->cmd, BCLI_ERROR, err, NULL); } +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); +} + /* Don't use this in general: it's better to omit fields. */ static void json_add_null(struct json_stream *stream, const char *fieldname) { @@ -591,19 +603,19 @@ struct estimatefees_stash { }; static struct command_result * -estimatefees_null_response(struct bitcoin_cli *bcli) +estimatefees_null_response(struct command *cmd) { - struct json_stream *response = jsonrpc_stream_success(bcli->cmd); + struct json_stream *response = jsonrpc_stream_success(cmd); /* We give a floor, which is the standard minimum */ json_array_start(response, "feerates"); json_array_end(response); json_add_u32(response, "feerate_floor", 1000); - return command_finished(bcli->cmd, response); + return command_finished(cmd, response); } -static struct command_result * +static UNNEEDED struct command_result * estimatefees_parse_feerate(struct bitcoin_cli *bcli, u64 *feerate) { const jsmntok_t *tokens; @@ -626,7 +638,7 @@ estimatefees_parse_feerate(struct bitcoin_cli *bcli, u64 *feerate) } /* We return null if estimation failed, and bitcoin-cli will * exit with 0 but no feerate field on failure. */ - return estimatefees_null_response(bcli); + return estimatefees_null_response(bcli->cmd); } return NULL; @@ -923,13 +935,9 @@ static struct command_result *getchaininfo(struct command *cmd, NULL); } - tokens = json_parse_simple(cmd, res->output, res->output_len); - if (!tokens) { - return command_done_err(cmd, BCLI_ERROR, - tal_fmt(cmd, "getblockchaininfo: bad JSON: cannot parse (%s)", - res->output), - NULL); - } + tokens = json_parse_simple(res->output, res->output, res->output_len); + if (!tokens) + return command_err_badjson(cmd, res, "cannot parse"); err = json_scan(tmpctx, res->output, tokens, "{chain:%,headers:%,blocks:%,initialblockdownload:%}", @@ -937,12 +945,8 @@ static struct command_result *getchaininfo(struct command *cmd, JSON_SCAN(json_to_number, &headers), JSON_SCAN(json_to_number, &blocks), JSON_SCAN(json_to_bool, &ibd)); - if (err) { - return command_done_err(cmd, BCLI_ERROR, - tal_fmt(cmd, "getblockchaininfo: bad JSON: %s (%s)", - err, res->output), - NULL); - } + if (err) + return command_err_badjson(cmd, res, err); if (bitcoind->dev_ignore_ibd) ibd = false; @@ -962,30 +966,30 @@ static struct command_result *estimatefees_done(struct bitcoin_cli *bcli); /* Add a feerate, but don't publish one that bitcoind won't accept. */ static void json_add_feerate(struct json_stream *result, const char *fieldname, struct command *cmd, - const struct estimatefees_stash *stash, - uint64_t value) + u64 perkb_floor, + u64 value) { /* Anthony Towns reported signet had a 900kbtc fee block, and then * CLN got upset scanning feerate. It expects a u32. */ if (value > 0xFFFFFFFF) { plugin_log(cmd->plugin, LOG_UNUSUAL, - "Feerate %"PRIu64" is ridiculous: trimming to 32 bites", + "Feerate %"PRIu64" is ridiculous: trimming to 32 bits", value); value = 0xFFFFFFFF; } /* 0 is special, it means "unknown" */ - if (value && value < stash->perkb_floor) { + if (value && value < perkb_floor) { plugin_log(cmd->plugin, LOG_DBG, "Feerate %s raised from %"PRIu64 " perkb to floor of %"PRIu64, - fieldname, value, stash->perkb_floor); - json_add_u64(result, fieldname, stash->perkb_floor); + fieldname, value, perkb_floor); + json_add_u64(result, fieldname, perkb_floor); } else { json_add_u64(result, fieldname, value); } } -static struct command_result *estimatefees_next(struct command *cmd, +static UNNEEDED struct command_result *estimatefees_next(struct command *cmd, struct estimatefees_stash *stash) { struct json_stream *response; @@ -1010,7 +1014,7 @@ static struct command_result *estimatefees_next(struct command *cmd, continue; json_object_start(response, NULL); json_add_u32(response, "blocks", estimatefee_params[i].blocks); - json_add_feerate(response, "feerate", cmd, stash, stash->perkb[i]); + json_add_feerate(response, "feerate", cmd, stash->perkb_floor, stash->perkb[i]); json_object_end(response); } json_array_end(response); @@ -1018,7 +1022,7 @@ static struct command_result *estimatefees_next(struct command *cmd, return command_finished(cmd, response); } -static struct command_result *getminfees_done(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result *getminfees_done(struct bitcoin_cli *bcli) { const jsmntok_t *tokens; const char *err; @@ -1026,7 +1030,7 @@ static struct command_result *getminfees_done(struct bitcoin_cli *bcli) struct estimatefees_stash *stash = bcli->stash; if (*bcli->exitstatus != 0) - return estimatefees_null_response(bcli); + return estimatefees_null_response(bcli->cmd); tokens = json_parse_simple(bcli->output, bcli->output, bcli->output_bytes); @@ -1048,6 +1052,72 @@ static struct command_result *getminfees_done(struct bitcoin_cli *bcli) return estimatefees_next(bcli->cmd, stash); } +/* Get the feerate floor from getmempoolinfo. + * Returns NULL on success (floor stored in *perkb_floor), or error response. */ +static struct command_result *get_feerate_floor(struct command *cmd, + u64 *perkb_floor) +{ + struct bcli_result *res; + const jsmntok_t *tokens; + const char *err; + u64 mempoolfee, relayfee; + + res = run_bitcoin_cli(cmd, cmd->plugin, "getmempoolinfo", NULL); + if (res->exitstatus != 0) + return estimatefees_null_response(cmd); + + tokens = json_parse_simple(res->output, res->output, res->output_len); + if (!tokens) + return command_err_badjson(cmd, res, "cannot parse"); + + err = json_scan(tmpctx, res->output, tokens, + "{mempoolminfee:%,minrelaytxfee:%}", + JSON_SCAN(json_to_bitcoin_amount, &mempoolfee), + JSON_SCAN(json_to_bitcoin_amount, &relayfee)); + if (err) + return command_err_badjson(cmd, res, err); + + *perkb_floor = max_u64(mempoolfee, relayfee); + return NULL; +} + +/* Get a single feerate from estimatesmartfee. + * Returns NULL on success (feerate stored in *perkb), or error response. */ +static struct command_result *get_feerate(struct command *cmd, + u32 blocks, + const char *style, + u64 *perkb) +{ + struct bcli_result *res; + const jsmntok_t *tokens; + + res = run_bitcoin_cli(cmd, cmd->plugin, "estimatesmartfee", + tal_fmt(tmpctx, "%u", blocks), style, NULL); + + if (res->exitstatus != 0) + return estimatefees_null_response(cmd); + + tokens = json_parse_simple(res->output, res->output, res->output_len); + if (!tokens) + return command_err_badjson(cmd, res, "cannot parse"); + + if (json_scan(tmpctx, res->output, tokens, "{feerate:%}", + JSON_SCAN(json_to_bitcoin_amount, perkb)) != NULL) { + /* Paranoia: if it had a feerate, but was malformed: */ + if (json_get_member(res->output, tokens, "feerate")) + return command_err_badjson(cmd, res, "cannot scan"); + /* Regtest fee estimation is generally awful: Fake it at min. */ + if (bitcoind->fake_fees) + *perkb = 1000; + else + /* We return null if estimation failed, and bitcoin-cli will + * exit with 0 but no feerate field on failure. */ + return estimatefees_null_response(cmd); + } + + return NULL; +} + /* Get the current feerates. We use an urgent feerate for unilateral_close and max, * a slightly less urgent feerate for htlc_resolution and penalty transactions, * a slow feerate for min, and a normal one for all others. @@ -1056,26 +1126,48 @@ static struct command_result *estimatefees(struct command *cmd, const char *buf UNUSED, const jsmntok_t *toks UNUSED) { - struct estimatefees_stash *stash = tal(cmd, struct estimatefees_stash); + struct command_result *err; + u64 perkb_floor; + u64 perkb[ARRAY_SIZE(estimatefee_params)]; + struct json_stream *response; if (!param(cmd, buf, toks, NULL)) return command_param_failed(); - start_bitcoin_cli(NULL, cmd, getminfees_done, true, - BITCOIND_LOW_PRIO, stash, - "getmempoolinfo", - NULL); - return command_still_pending(cmd); + err = get_feerate_floor(cmd, &perkb_floor); + if (err) + return err; + + for (size_t i = 0; i < ARRAY_SIZE(estimatefee_params); i++) { + err = get_feerate(cmd, estimatefee_params[i].blocks, + estimatefee_params[i].style, &perkb[i]); + if (err) + return err; + } + + response = jsonrpc_stream_success(cmd); + json_array_start(response, "feerates"); + for (size_t i = 0; i < ARRAY_SIZE(perkb); i++) { + if (!perkb[i]) + continue; + json_object_start(response, NULL); + json_add_u32(response, "blocks", estimatefee_params[i].blocks); + json_add_feerate(response, "feerate", cmd, perkb_floor, perkb[i]); + json_object_end(response); + } + json_array_end(response); + json_add_u64(response, "feerate_floor", perkb_floor); + return command_finished(cmd, response); } -static struct command_result *estimatefees_done(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result *estimatefees_done(struct bitcoin_cli *bcli) { struct command_result *err; struct estimatefees_stash *stash = bcli->stash; /* If we cannot estimate fees, no need to continue bothering bitcoind. */ if (*bcli->exitstatus != 0) - return estimatefees_null_response(bcli); + return estimatefees_null_response(bcli->cmd); err = estimatefees_parse_feerate(bcli, &stash->perkb[stash->cursor]); if (err) @@ -1160,13 +1252,9 @@ static struct command_result *getutxout(struct command *cmd, return command_finished(cmd, response); } - tokens = json_parse_simple(cmd, res->output, res->output_len); - if (!tokens) { - return command_done_err(cmd, BCLI_ERROR, - tal_fmt(cmd, "gettxout: bad JSON: cannot parse (%s)", - res->output), - NULL); - } + tokens = json_parse_simple(res->output, res->output, res->output_len); + if (!tokens) + return command_err_badjson(cmd, res, "cannot parse"); err = json_scan(tmpctx, res->output, tokens, "{value:%,scriptPubKey:{hex:%}}", @@ -1174,12 +1262,8 @@ static struct command_result *getutxout(struct command *cmd, &output.amount.satoshis), /* Raw: bitcoind */ JSON_SCAN_TAL(cmd, json_tok_bin_from_hex, &output.script)); - if (err) { - return command_done_err(cmd, BCLI_ERROR, - tal_fmt(cmd, "gettxout: bad JSON: %s (%s)", - err, res->output), - NULL); - } + if (err) + return command_err_badjson(cmd, res, err); response = jsonrpc_stream_success(cmd); json_add_sats(response, "amount", output.amount); From eb22d08278da4a2b2a5c502be10c099ce094f2d2 Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Thu, 8 Jan 2026 21:04:52 +0200 Subject: [PATCH 06/13] bcli: convert `getrawblockbyheight` to synchronous execution 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). --- plugins/bcli.c | 199 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 160 insertions(+), 39 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index 5174012bef58..2510ebd9574b 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -493,11 +493,11 @@ static struct command_result *command_err_bcli_badjson(struct bitcoin_cli *bcli, return command_done_err(bcli->cmd, BCLI_ERROR, err, NULL); } -static struct command_result *command_err_badjson(struct command *cmd, - struct bcli_result *res, - const char *errmsg) +static struct command_result *command_err(struct command *cmd, + struct bcli_result *res, + const char *errmsg) { - char *err = tal_fmt(cmd, "%s: bad JSON: %s (%.*s)", + char *err = tal_fmt(cmd, "%s: %s (%.*s)", res->args, errmsg, (int)res->output_len, res->output); return command_done_err(cmd, BCLI_ERROR, err, NULL); } @@ -679,9 +679,9 @@ struct getrawblock_stash { }; /* Mutual recursion. */ -static struct command_result *getrawblock(struct bitcoin_cli *bcli); +static UNNEEDED struct command_result *getrawblock(struct bitcoin_cli *bcli); -static struct command_result *process_rawblock(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result *process_rawblock(struct bitcoin_cli *bcli) { struct json_stream *response; struct getrawblock_stash *stash = bcli->stash; @@ -696,7 +696,7 @@ static struct command_result *process_rawblock(struct bitcoin_cli *bcli) return command_finished(bcli->cmd, response); } -static struct command_result *process_getblockfrompeer(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result *process_getblockfrompeer(struct bitcoin_cli *bcli) { /* Remove the peer that we tried to get the block from and move along, * we may also check on errors here */ @@ -723,7 +723,7 @@ static struct command_result *process_getblockfrompeer(struct bitcoin_cli *bcli) return getrawblock(bcli); } -static struct command_result *process_getpeerinfo(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result *process_getpeerinfo(struct bitcoin_cli *bcli) { const jsmntok_t *t, *toks; struct getrawblock_stash *stash = bcli->stash; @@ -773,7 +773,7 @@ static struct command_result *process_getpeerinfo(struct bitcoin_cli *bcli) return command_still_pending(bcli->cmd); } -static struct command_result *process_getrawblock(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result *process_getrawblock(struct bitcoin_cli *bcli) { /* We failed to get the raw block. */ if (bcli->exitstatus && *bcli->exitstatus != 0) { @@ -825,8 +825,8 @@ static struct command_result *process_getrawblock(struct bitcoin_cli *bcli) return process_rawblock(bcli); } -static struct command_result * -getrawblockbyheight_notfound(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result * +getrawblockbyheight_notfound_bcli(struct bitcoin_cli *bcli) { struct json_stream *response; @@ -837,7 +837,19 @@ getrawblockbyheight_notfound(struct bitcoin_cli *bcli) return command_finished(bcli->cmd, response); } -static struct command_result *getrawblock(struct bitcoin_cli *bcli) +static struct command_result * +getrawblockbyheight_notfound(struct command *cmd) +{ + struct json_stream *response; + + response = jsonrpc_stream_success(cmd); + json_add_null(response, "blockhash"); + json_add_null(response, "block"); + + return command_finished(cmd, response); +} + +static UNNEEDED struct command_result *getrawblock(struct bitcoin_cli *bcli) { struct getrawblock_stash *stash = bcli->stash; @@ -850,7 +862,7 @@ static struct command_result *getrawblock(struct bitcoin_cli *bcli) return command_still_pending(bcli->cmd); } -static struct command_result *process_getblockhash(struct bitcoin_cli *bcli) +static UNNEEDED struct command_result *process_getblockhash(struct bitcoin_cli *bcli) { struct getrawblock_stash *stash = bcli->stash; @@ -859,7 +871,7 @@ static struct command_result *process_getblockhash(struct bitcoin_cli *bcli) /* Other error means we have to retry. */ if (*bcli->exitstatus != 8) return NULL; - return getrawblockbyheight_notfound(bcli); + return getrawblockbyheight_notfound_bcli(bcli); } strip_trailing_whitespace(bcli->output, bcli->output_bytes); @@ -871,6 +883,44 @@ static struct command_result *process_getblockhash(struct bitcoin_cli *bcli) return getrawblock(bcli); } +/* Get peers that support NODE_NETWORK (full nodes). + * Returns array of peer ids, or empty array if none found. */ +static int *get_fullnode_peers(const tal_t *ctx, struct command *cmd) +{ + struct bcli_result *res; + const jsmntok_t *t, *toks; + int *peers = tal_arr(ctx, int, 0); + size_t i; + + res = run_bitcoin_cli(cmd, cmd->plugin, "getpeerinfo", NULL); + if (res->exitstatus != 0) + return peers; + + toks = json_parse_simple(res->output, res->output, res->output_len); + if (!toks) + return peers; + + json_for_each_arr(i, t, toks) { + int id; + u8 *services; + + if (json_scan(tmpctx, res->output, t, "{id:%,services:%}", + JSON_SCAN(json_to_int, &id), + JSON_SCAN_TAL(tmpctx, json_tok_bin_from_hex, &services)) == NULL) { + /* From bitcoin source: + * NODE_NETWORK means that the node is capable of serving the complete block chain. + * It is currently set by all Bitcoin Core non pruned nodes, and is unset by SPV + * clients or other light clients. + * NODE_NETWORK = (1 << 0) + */ + if (tal_count(services) > 0 && (services[tal_count(services)-1] & (1 << 0))) + tal_arr_expand(&peers, id); + } + } + + return peers; +} + /* Get a raw block given its height. * Calls `getblockhash` then `getblock` to retrieve it from bitcoin_cli. * Will return early with null fields if block isn't known (yet). @@ -879,27 +929,101 @@ static struct command_result *getrawblockbyheight(struct command *cmd, const char *buf, const jsmntok_t *toks) { - struct getrawblock_stash *stash; + struct bcli_result *res; + struct json_stream *response; + const char *block_hash; u32 *height; + struct timemono first_error_time; + bool first_error = true; + int *peers = NULL; - /* bitcoin-cli wants a string. */ if (!param(cmd, buf, toks, p_req("height", param_number, &height), NULL)) return command_param_failed(); - stash = tal(cmd, struct getrawblock_stash); - stash->block_height = *height; - stash->peers = NULL; - tal_free(height); + res = run_bitcoin_cli(cmd, cmd->plugin, "getblockhash", + tal_fmt(tmpctx, "%u", *height), NULL); + + if (res->exitstatus != 0) { + /* Exit code 8 means block height doesn't exist (empty response) */ + if (res->exitstatus == 8) + return getrawblockbyheight_notfound(cmd); + return command_err(cmd, res, "command failed"); + } + + strip_trailing_whitespace(res->output, res->output_len); + if (strlen(res->output) != 64) + return command_err(cmd, res, "bad JSON: bad blockhash"); + + block_hash = tal_strdup(cmd, res->output); + + for (;;) { + res = run_bitcoin_cli(cmd, cmd->plugin, "getblock", + block_hash, "0", NULL); + + if (res->exitstatus == 0) { + strip_trailing_whitespace(res->output, res->output_len); + response = jsonrpc_stream_success(cmd); + json_add_string(response, "blockhash", block_hash); + json_add_string(response, "block", res->output); + return command_finished(cmd, response); + } + + plugin_log(cmd->plugin, LOG_DBG, + "failed to fetch block %s from the bitcoin backend (maybe pruned).", + block_hash); + + if (first_error) { + first_error_time = time_mono(); + first_error = false; + } + + struct timerel elapsed = timemono_between(time_mono(), first_error_time); + if (time_greater(elapsed, time_from_sec(bitcoind->retry_timeout))) { + return command_done_err(cmd, BCLI_ERROR, + tal_fmt(cmd, "getblock %s timed out after %"PRIu64" seconds", + block_hash, bitcoind->retry_timeout), NULL); + } - start_bitcoin_cli(NULL, cmd, process_getblockhash, true, - BITCOIND_LOW_PRIO, stash, - "getblockhash", - take(tal_fmt(NULL, "%u", stash->block_height)), - NULL); + /* Try fetching from peers if bitcoind >= 23.0.0 */ + if (bitcoind->version >= 230000) { + if (!peers) + peers = get_fullnode_peers(cmd, cmd); + + if (tal_count(peers) > 0) { + int peer = peers[tal_count(peers) - 1]; + tal_resize(&peers, tal_count(peers) - 1); + + res = run_bitcoin_cli(cmd, cmd->plugin, + "getblockfrompeer", + block_hash, + tal_fmt(tmpctx, "%i", peer), + NULL); + + if (res->exitstatus != 0) { + /* We still continue with the execution if we cannot fetch the + * block from peer */ + plugin_log(cmd->plugin, LOG_DBG, + "failed to fetch block %s from peer %i, skip.", + block_hash, peer); + } else { + plugin_log(cmd->plugin, LOG_DBG, + "try to fetch block %s from peer %i.", + block_hash, peer); + } + } - return command_still_pending(cmd); + if (tal_count(peers) == 0) { + plugin_log(cmd->plugin, LOG_DBG, + "asked all known peers about block %s, retry", + block_hash); + peers = tal_free(peers); + } + } + + sleep(1); + } } /* Get infos about the block chain. @@ -929,15 +1053,12 @@ static struct command_result *getchaininfo(struct command *cmd, return command_param_failed(); res = run_bitcoin_cli(cmd, cmd->plugin, "getblockchaininfo", NULL); - if (res->exitstatus != 0) { - return command_done_err(cmd, BCLI_ERROR, - tal_fmt(cmd, "getblockchaininfo: %s", res->output), - NULL); - } + if (res->exitstatus != 0) + return command_err(cmd, res, "command failed"); tokens = json_parse_simple(res->output, res->output, res->output_len); if (!tokens) - return command_err_badjson(cmd, res, "cannot parse"); + return command_err(cmd, res, "bad JSON: cannot parse"); err = json_scan(tmpctx, res->output, tokens, "{chain:%,headers:%,blocks:%,initialblockdownload:%}", @@ -946,7 +1067,7 @@ static struct command_result *getchaininfo(struct command *cmd, JSON_SCAN(json_to_number, &blocks), JSON_SCAN(json_to_bool, &ibd)); if (err) - return command_err_badjson(cmd, res, err); + return command_err(cmd, res, tal_fmt(tmpctx, "bad JSON: %s", err)); if (bitcoind->dev_ignore_ibd) ibd = false; @@ -1068,14 +1189,14 @@ static struct command_result *get_feerate_floor(struct command *cmd, tokens = json_parse_simple(res->output, res->output, res->output_len); if (!tokens) - return command_err_badjson(cmd, res, "cannot parse"); + return command_err(cmd, res, "bad JSON: cannot parse"); err = json_scan(tmpctx, res->output, tokens, "{mempoolminfee:%,minrelaytxfee:%}", JSON_SCAN(json_to_bitcoin_amount, &mempoolfee), JSON_SCAN(json_to_bitcoin_amount, &relayfee)); if (err) - return command_err_badjson(cmd, res, err); + return command_err(cmd, res, tal_fmt(tmpctx, "bad JSON: %s", err)); *perkb_floor = max_u64(mempoolfee, relayfee); return NULL; @@ -1099,13 +1220,13 @@ static struct command_result *get_feerate(struct command *cmd, tokens = json_parse_simple(res->output, res->output, res->output_len); if (!tokens) - return command_err_badjson(cmd, res, "cannot parse"); + return command_err(cmd, res, "bad JSON: cannot parse"); if (json_scan(tmpctx, res->output, tokens, "{feerate:%}", JSON_SCAN(json_to_bitcoin_amount, perkb)) != NULL) { /* Paranoia: if it had a feerate, but was malformed: */ if (json_get_member(res->output, tokens, "feerate")) - return command_err_badjson(cmd, res, "cannot scan"); + return command_err(cmd, res, "bad JSON: cannot scan"); /* Regtest fee estimation is generally awful: Fake it at min. */ if (bitcoind->fake_fees) *perkb = 1000; @@ -1254,7 +1375,7 @@ static struct command_result *getutxout(struct command *cmd, tokens = json_parse_simple(res->output, res->output, res->output_len); if (!tokens) - return command_err_badjson(cmd, res, "cannot parse"); + return command_err(cmd, res, "bad JSON: cannot parse"); err = json_scan(tmpctx, res->output, tokens, "{value:%,scriptPubKey:{hex:%}}", @@ -1263,7 +1384,7 @@ static struct command_result *getutxout(struct command *cmd, JSON_SCAN_TAL(cmd, json_tok_bin_from_hex, &output.script)); if (err) - return command_err_badjson(cmd, res, err); + return command_err(cmd, res, tal_fmt(tmpctx, "bad JSON: %s", err)); response = jsonrpc_stream_success(cmd); json_add_sats(response, "amount", output.amount); From 3e1bca8ce2e7f910102cbb26936e9ad5cf82717d Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Fri, 9 Jan 2026 17:16:44 +0200 Subject: [PATCH 07/13] lightningd: add `get_bitcoin_result` for bcli response handling Add `get_bitcoin_result` function that checks bcli plugin responses for errors and returns the result token. Previously, callbacks only detected errors when result parsing failed, ignoring the explicit error field from the plugin. Now we extract the actual error message from bcli, providing clearer reasoning when the plugin returns an error response. --- lightningd/bitcoind.c | 75 ++++++++++++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 19 deletions(-) diff --git a/lightningd/bitcoind.c b/lightningd/bitcoind.c index a7e9b876d77b..abf5171b0ac7 100644 --- a/lightningd/bitcoind.c +++ b/lightningd/bitcoind.c @@ -111,6 +111,35 @@ static void bitcoin_plugin_error(struct bitcoind *bitcoind, const char *buf, toks->end - toks->start, buf + toks->start); } +/* Check response for error (fatal if found) and return result token (fatal if not found). */ +static const jsmntok_t *get_bitcoin_result(struct bitcoind *bitcoind, + const char *buf, + const jsmntok_t *toks, + const char *method) +{ + const jsmntok_t *err_tok, *msg_tok, *result_tok; + + err_tok = json_get_member(buf, toks, "error"); + if (err_tok) { + msg_tok = json_get_member(buf, err_tok, "message"); + if (msg_tok) + bitcoin_plugin_error(bitcoind, buf, toks, method, + "%.*s", json_tok_full_len(msg_tok), + json_tok_full(buf, msg_tok)); + else + bitcoin_plugin_error(bitcoind, buf, toks, method, + "%.*s", json_tok_full_len(err_tok), + json_tok_full(buf, err_tok)); + } + + result_tok = json_get_member(buf, toks, "result"); + if (!result_tok) + bitcoin_plugin_error(bitcoind, buf, toks, method, + "missing 'result' field"); + + return result_tok; +} + /* Send a request to the Bitcoin plugin which registered that method, * if it's still alive. */ static void bitcoin_plugin_send(struct bitcoind *bitcoind, @@ -287,11 +316,7 @@ static void estimatefees_callback(const char *buf, const jsmntok_t *toks, struct feerate_est *feerates; u32 floor; - resulttok = json_get_member(buf, toks, "result"); - if (!resulttok) - bitcoin_plugin_error(call->bitcoind, buf, toks, - "estimatefees", - "bad 'result' field"); + resulttok = get_bitcoin_result(call->bitcoind, buf, toks, "estimatefees"); /* Modern style has floor. */ floortok = json_get_member(buf, resulttok, "feerate_floor"); @@ -386,21 +411,24 @@ static void sendrawtx_callback(const char *buf, const jsmntok_t *toks, const jsmntok_t *idtok, struct sendrawtx_call *call) { + const jsmntok_t *resulttok; const char *err; const char *errmsg = NULL; bool success = false; - err = json_scan(tmpctx, buf, toks, "{result:{success:%}}", + resulttok = get_bitcoin_result(call->bitcoind, buf, toks, "sendrawtransaction"); + + err = json_scan(tmpctx, buf, resulttok, "{success:%}", JSON_SCAN(json_to_bool, &success)); if (err) { - bitcoin_plugin_error(call->bitcoind, buf, toks, + bitcoin_plugin_error(call->bitcoind, buf, resulttok, "sendrawtransaction", "bad 'result' field: %s", err); } else if (!success) { - err = json_scan(tmpctx, buf, toks, "{result:{errmsg:%}}", + err = json_scan(tmpctx, buf, resulttok, "{errmsg:%}", JSON_SCAN_TAL(tmpctx, json_strdup, &errmsg)); if (err) - bitcoin_plugin_error(call->bitcoind, buf, toks, + bitcoin_plugin_error(call->bitcoind, buf, resulttok, "sendrawtransaction", "bad 'errmsg' field: %s", err); @@ -465,6 +493,7 @@ getrawblockbyheight_callback(const char *buf, const jsmntok_t *toks, const jsmntok_t *idtok, struct getrawblockbyheight_call *call) { + const jsmntok_t *resulttok; const char *block_str, *err; struct bitcoin_blkid blkid; struct bitcoin_block *blk; @@ -472,6 +501,8 @@ getrawblockbyheight_callback(const char *buf, const jsmntok_t *toks, trace_span_resume(call); trace_span_end(call); + resulttok = get_bitcoin_result(call->bitcoind, buf, toks, "getrawblockbyheight"); + /* Callback may free parent of call, so steal onto context to * free if it doesn't */ ctx = tal(NULL, char); @@ -479,24 +510,24 @@ getrawblockbyheight_callback(const char *buf, const jsmntok_t *toks, /* If block hash is `null`, this means not found! Call the callback * with NULL values. */ - err = json_scan(tmpctx, buf, toks, "{result:{blockhash:null}}"); + err = json_scan(tmpctx, buf, resulttok, "{blockhash:null}"); if (!err) { call->cb(call->bitcoind, call->height, NULL, NULL, call->cb_arg); goto clean; } - err = json_scan(tmpctx, buf, toks, "{result:{blockhash:%,block:%}}", + err = json_scan(tmpctx, buf, resulttok, "{blockhash:%,block:%}", JSON_SCAN(json_to_sha256, &blkid.shad.sha), JSON_SCAN_TAL(tmpctx, json_strdup, &block_str)); if (err) - bitcoin_plugin_error(call->bitcoind, buf, toks, + bitcoin_plugin_error(call->bitcoind, buf, resulttok, "getrawblockbyheight", "bad 'result' field: %s", err); blk = bitcoin_block_from_hex(tmpctx, chainparams, block_str, strlen(block_str)); if (!blk) - bitcoin_plugin_error(call->bitcoind, buf, toks, + bitcoin_plugin_error(call->bitcoind, buf, resulttok, "getrawblockbyheight", "bad block"); @@ -565,18 +596,21 @@ static void getchaininfo_callback(const char *buf, const jsmntok_t *toks, const jsmntok_t *idtok, struct getchaininfo_call *call) { + const jsmntok_t *resulttok; const char *err, *chain; u32 headers, blocks; bool ibd; - err = json_scan(tmpctx, buf, toks, - "{result:{chain:%,headercount:%,blockcount:%,ibd:%}}", + resulttok = get_bitcoin_result(call->bitcoind, buf, toks, "getchaininfo"); + + err = json_scan(tmpctx, buf, resulttok, + "{chain:%,headercount:%,blockcount:%,ibd:%}", JSON_SCAN_TAL(tmpctx, json_strdup, &chain), JSON_SCAN(json_to_number, &headers), JSON_SCAN(json_to_number, &blocks), JSON_SCAN(json_to_bool, &ibd)); if (err) - bitcoin_plugin_error(call->bitcoind, buf, toks, "getchaininfo", + bitcoin_plugin_error(call->bitcoind, buf, resulttok, "getchaininfo", "bad 'result' field: %s", err); call->cb(call->bitcoind, chain, headers, blocks, ibd, @@ -636,24 +670,27 @@ static void getutxout_callback(const char *buf, const jsmntok_t *toks, const jsmntok_t *idtok, struct getutxout_call *call) { + const jsmntok_t *resulttok; const char *err; struct bitcoin_tx_output txout; /* Whatever happens, we want to free this. */ tal_steal(tmpctx, call); - err = json_scan(tmpctx, buf, toks, "{result:{script:null}}"); + resulttok = get_bitcoin_result(call->bitcoind, buf, toks, "getutxout"); + + err = json_scan(tmpctx, buf, resulttok, "{script:null}"); if (!err) { call->cb(call->bitcoind, NULL, call->cb_arg); return; } - err = json_scan(tmpctx, buf, toks, "{result:{script:%,amount:%}}", + err = json_scan(tmpctx, buf, resulttok, "{script:%,amount:%}", JSON_SCAN_TAL(tmpctx, json_tok_bin_from_hex, &txout.script), JSON_SCAN(json_to_sat, &txout.amount)); if (err) - bitcoin_plugin_error(call->bitcoind, buf, toks, "getutxout", + bitcoin_plugin_error(call->bitcoind, buf, resulttok, "getutxout", "bad 'result' field: %s", err); call->cb(call->bitcoind, &txout, call->cb_arg); From 62cb94111a45f6c63090bc95fb99b72a930c3501 Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Fri, 9 Jan 2026 17:17:24 +0200 Subject: [PATCH 08/13] pytest: fix bcli tests after sync refactor Rewrite `test_bitcoin_failure` to reflect synchronous bcli behavior: the node now crashes on invalid bitcoind responses rather than retrying. Add `may_fail` and `broken_log` to handle expected crash. Update `test_bitcoind_fail_first` stderr check to match the new error message format from `get_bitcoin_result`. Update test mocks to use proper error format for "block not found". Co-authored-by: ShahanaFarooqui --- tests/test_closing.py | 4 ++-- tests/test_misc.py | 55 +++++++++++++------------------------------ 2 files changed, 19 insertions(+), 40 deletions(-) diff --git a/tests/test_closing.py b/tests/test_closing.py index 47cc28d8077c..3879934a07a7 100644 --- a/tests/test_closing.py +++ b/tests/test_closing.py @@ -4277,7 +4277,7 @@ def test_onchain_reestablish_reply(node_factory, bitcoind, executor): # 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"}} l3.daemon.rpcproxy.mock_rpc('getblockhash', no_new_blocks) l2.rpc.disconnect(l3.info['id'], force=True) @@ -4360,7 +4360,7 @@ def test_reestablish_closed_channels(node_factory, bitcoind): # We block l2 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"}} l2.daemon.rpcproxy.mock_rpc('getblockhash', no_new_blocks) # Make a payment, make sure it's entirely finished before we close. diff --git a/tests/test_misc.py b/tests/test_misc.py index b1aa0379611d..72fff873193f 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -77,35 +77,28 @@ def test_db_upgrade(node_factory): def test_bitcoin_failure(node_factory, bitcoind): - l1 = node_factory.get_node() + # The node will crash when bitcoind fails, so we need `may_fail` and `broken_log`. + l1 = node_factory.get_node(may_fail=True, broken_log=r'getrawblockbyheight|FATAL SIGNAL|backtrace') # Make sure we're not failing it between getblockhash and getblock. sync_blockheight(bitcoind, [l1]) def crash_bitcoincli(r): - return {'error': 'go away'} + return {'id': r['id'], 'result': 'not_a_valid_blockhash', 'error': None} - # This is not a JSON-RPC response by purpose - l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', crash_bitcoincli) + # This is not a JSON-RPC response by purpose. l1.daemon.rpcproxy.mock_rpc('getblockhash', crash_bitcoincli) - # This should cause both estimatefee and getblockhash fail - l1.daemon.wait_for_logs(['Unable to estimate any fees', - 'getblockhash .* exited with status 1']) - - # And they should retry! - l1.daemon.wait_for_logs(['Unable to estimate any fees', - 'getblockhash .* exited with status 1']) - - # Restore, then it should recover and get blockheight. - l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', None) - l1.daemon.rpcproxy.mock_rpc('getblockhash', None) + # Generate a block to trigger the topology update which calls getblockhash. + bitcoind.generate_block(1) - bitcoind.generate_block(5) - sync_blockheight(bitcoind, [l1]) + # lightningd should crash with the error + # `fatal()` calls `abort()` when crashlog is set (during operation), so exit code is -6 (SIGABRT). + l1.daemon.wait_for_log(r'bad response to getrawblockbyheight') + assert l1.daemon.wait() != 0 # We refuse to start if bitcoind is in `blocksonly` - l1.stop() + # l1 already crashed, so we just need to restart bitcoind. bitcoind.stop() bitcoind.cmd_line += ["-blocksonly"] bitcoind.start() @@ -2216,31 +2209,17 @@ def test_bitcoind_fail_first(node_factory, bitcoind): """ # Do not start the lightning node since we need to instrument bitcoind # first. - timeout = 5 if 5 < TIMEOUT // 3 else TIMEOUT // 3 - l1 = node_factory.get_node(start=False, - broken_log=r'plugin-bcli: .*(-stdinrpcpass -stdin getblockhash 100 exited 1 \(after [0-9]* other errors\)|we have been retrying command for)', - may_fail=True, - options={'bitcoin-retry-timeout': timeout}) + l1 = node_factory.get_node(start=False, may_fail=True) # Instrument bitcoind to fail some queries first. - def mock_fail(*args): - raise ValueError() - - # If any of these succeed, they reset fail timeout. - l1.daemon.rpcproxy.mock_rpc('getblockhash', mock_fail) - l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', mock_fail) - l1.daemon.rpcproxy.mock_rpc('getmempoolinfo', mock_fail) + def crash_bitcoincli(r): + return {'id': r['id'], 'result': 'not_a_valid_blockhash', 'error': None} + l1.daemon.rpcproxy.mock_rpc('getblockhash', crash_bitcoincli) l1.daemon.start(wait_for_initialized=False, stderr_redir=True) - l1.daemon.wait_for_logs([r'getblockhash [a-z0-9]* exited with status 1', - r'Unable to estimate any fees', - r'BROKEN.*we have been retrying command for --bitcoin-retry-timeout={} seconds'.format(timeout)]) - # Will exit with failure code. - assert l1.daemon.wait() == 1 - # Now unset the mock, so calls go through again - l1.daemon.rpcproxy.mock_rpc('getblockhash', None) - l1.daemon.rpcproxy.mock_rpc('estimatesmartfee', None) + assert l1.daemon.wait() == 1 + assert l1.daemon.is_in_stderr('bad response to getrawblockbyheight') @unittest.skipIf(TEST_NETWORK == 'liquid-regtest', "Fees on elements are different") From 11d8786eebb611257311a029b6ebdde11a014f3d Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Sat, 10 Jan 2026 00:02:25 +0200 Subject: [PATCH 09/13] bcli: remove unused async code after sync refactor Remove the asynchronous execution infrastructure no longer needed after converting all bcli commands to synchronous execution. This includes removing the async callbacks, the pending request queue, etc. Fix missing `close(from)` file descriptor leak in `run_bitcoin_cliv`. Changelog-Changed: bcli plugin now uses synchronous execution, simplifying bitcoin backend communication and improving error handling reliability. --- plugins/bcli.c | 720 ++----------------------------------------------- 1 file changed, 15 insertions(+), 705 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index 2510ebd9574b..4616c58b55aa 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -1,7 +1,6 @@ #include "config.h" #include #include -#include #include #include #include @@ -12,21 +11,10 @@ #include #include #include +#include -/* Bitcoind's web server has a default of 4 threads, with queue depth 16. - * It will *fail* rather than queue beyond that, so we must not stress it! - * - * This is how many request for each priority level we have. - */ -#define BITCOIND_MAX_PARALLEL 4 #define RPC_TRANSACTION_ALREADY_IN_CHAIN -27 -enum bitcoind_prio { - BITCOIND_LOW_PRIO, - BITCOIND_HIGH_PRIO -}; -#define BITCOIND_NUM_PRIO (BITCOIND_HIGH_PRIO+1) - struct bitcoind { /* eg. "bitcoin-cli" */ char *cli; @@ -37,22 +25,6 @@ struct bitcoind { /* bitcoind's version, used for compatibility checks. */ u32 version; - /* Is bitcoind synced? If not, we retry. */ - bool synced; - - /* How many high/low prio requests are we running (it's ratelimited) */ - size_t num_requests[BITCOIND_NUM_PRIO]; - - /* Pending requests (high and low prio). */ - struct list_head pending[BITCOIND_NUM_PRIO]; - - /* In flight requests (in a list for memleak detection) */ - struct list_head current; - - /* If non-zero, time we first hit a bitcoind error. */ - unsigned int error_count; - struct timemono first_error_time; - /* How long to keep trying to contact bitcoind * before fatally exiting. */ u64 retry_timeout; @@ -73,24 +45,6 @@ struct bitcoind { static struct bitcoind *bitcoind; -struct bitcoin_cli { - struct list_node list; - int fd; - int *exitstatus; - pid_t pid; - const char **args; - const char **stdinargs; - struct timemono start; - enum bitcoind_prio prio; - char *output; - size_t output_bytes; - size_t new_output; - struct command_result *(*process)(struct bitcoin_cli *); - struct command *cmd; - /* Used to stash content between multiple calls */ - void *stash; -}; - /* Result of a synchronous bitcoin-cli call */ struct bcli_result { char *output; @@ -171,25 +125,6 @@ gather_args(const tal_t *ctx, const char ***stdinargs, const char *cmd, ...) return ret; } -static struct io_plan *read_more(struct io_conn *conn, struct bitcoin_cli *bcli) -{ - bcli->output_bytes += bcli->new_output; - if (bcli->output_bytes == tal_count(bcli->output)) - tal_resize(&bcli->output, bcli->output_bytes * 2); - return io_read_partial(conn, bcli->output + bcli->output_bytes, - tal_count(bcli->output) - bcli->output_bytes, - &bcli->new_output, read_more, bcli); -} - -static struct io_plan *output_init(struct io_conn *conn, struct bitcoin_cli *bcli) -{ - bcli->output_bytes = bcli->new_output = 0; - bcli->output = tal_arr(bcli, char, 100); - return read_more(conn, bcli); -} - -static void next_bcli(enum bitcoind_prio prio); - /* For printing: simple string of args (no secrets!) */ static char *args_string(const tal_t *ctx, const char **args, const char **stdinargs) { @@ -251,9 +186,15 @@ run_bitcoin_cliv(const tal_t *ctx, res->output = grab_fd_str(res, from); res->output_len = strlen(res->output); res->args = args_string(res, cmd, stdinargs); + close(from); /* Wait for child to exit */ - while (waitpid(child, &status, 0) < 0 && errno == EINTR); + while (waitpid(child, &status, 0) < 0) { + if (errno == EINTR) + continue; + plugin_err(plugin, "waitpid(%s) failed: %s", + res->args, strerror(errno)); + } if (!WIFEXITED(status)) plugin_err(plugin, "%s died with signal %i", @@ -279,202 +220,6 @@ run_bitcoin_cli(const tal_t *ctx, return res; } -static char *bcli_args(const tal_t *ctx, struct bitcoin_cli *bcli) -{ - return args_string(ctx, bcli->args, bcli->stdinargs); -} - -/* Only set as destructor once bcli is in current. */ -static void destroy_bcli(struct bitcoin_cli *bcli) -{ - list_del_from(&bitcoind->current, &bcli->list); -} - -static struct command_result *retry_bcli(struct command *cmd, - struct bitcoin_cli *bcli) -{ - list_del_from(&bitcoind->current, &bcli->list); - tal_del_destructor(bcli, destroy_bcli); - - list_add_tail(&bitcoind->pending[bcli->prio], &bcli->list); - tal_free(bcli->output); - next_bcli(bcli->prio); - return timer_complete(cmd); -} - -/* We allow 60 seconds of spurious errors, eg. reorg. */ -static void bcli_failure(struct bitcoin_cli *bcli, - int exitstatus) -{ - struct timerel t; - - if (!bitcoind->error_count) - bitcoind->first_error_time = time_mono(); - - t = timemono_between(time_mono(), bitcoind->first_error_time); - if (time_greater(t, time_from_sec(bitcoind->retry_timeout))) - plugin_err(bcli->cmd->plugin, - "%s exited %u (after %u other errors) '%.*s'; " - "we have been retrying command for " - "--bitcoin-retry-timeout=%"PRIu64" seconds; " - "bitcoind setup or our --bitcoin-* configs broken?", - bcli_args(tmpctx, bcli), - exitstatus, - bitcoind->error_count, - (int)bcli->output_bytes, - bcli->output, - bitcoind->retry_timeout); - - plugin_log(bcli->cmd->plugin, LOG_UNUSUAL, "%s exited with status %u", - bcli_args(tmpctx, bcli), exitstatus); - bitcoind->error_count++; - - /* Retry in 1 second */ - command_timer(bcli->cmd, time_from_sec(1), retry_bcli, bcli); -} - -static void bcli_finished(struct io_conn *conn UNUSED, struct bitcoin_cli *bcli) -{ - int ret, status; - struct command_result *res; - enum bitcoind_prio prio = bcli->prio; - u64 msec = time_to_msec(timemono_between(time_mono(), bcli->start)); - - /* If it took over 10 seconds, that's rather strange. */ - if (msec > 10000) - plugin_log(bcli->cmd->plugin, LOG_UNUSUAL, - "bitcoin-cli: finished %s (%"PRIu64" ms)", - bcli_args(tmpctx, bcli), msec); - - assert(bitcoind->num_requests[prio] > 0); - - /* FIXME: If we waited for SIGCHILD, this could never hang! */ - while ((ret = waitpid(bcli->pid, &status, 0)) < 0 && errno == EINTR); - if (ret != bcli->pid) - plugin_err(bcli->cmd->plugin, "%s %s", bcli_args(tmpctx, bcli), - ret == 0 ? "not exited?" : strerror(errno)); - - if (!WIFEXITED(status)) - plugin_err(bcli->cmd->plugin, "%s died with signal %i", - bcli_args(tmpctx, bcli), - WTERMSIG(status)); - - /* Implicit nonzero_exit_ok == false */ - if (!bcli->exitstatus) { - if (WEXITSTATUS(status) != 0) { - bcli_failure(bcli, WEXITSTATUS(status)); - bitcoind->num_requests[prio]--; - goto done; - } - } else - *bcli->exitstatus = WEXITSTATUS(status); - - if (WEXITSTATUS(status) == 0) - bitcoind->error_count = 0; - - bitcoind->num_requests[bcli->prio]--; - - res = bcli->process(bcli); - if (!res) - bcli_failure(bcli, WEXITSTATUS(status)); - else - tal_free(bcli); - -done: - next_bcli(prio); -} - -static void next_bcli(enum bitcoind_prio prio) -{ - struct bitcoin_cli *bcli; - struct io_conn *conn; - int in; - - if (bitcoind->num_requests[prio] >= BITCOIND_MAX_PARALLEL) - return; - - bcli = list_pop(&bitcoind->pending[prio], struct bitcoin_cli, list); - if (!bcli) - return; - - bcli->pid = pipecmdarr(&in, &bcli->fd, &bcli->fd, - cast_const2(char **, bcli->args)); - if (bcli->pid < 0) - plugin_err(bcli->cmd->plugin, "%s exec failed: %s", - bcli->args[0], strerror(errno)); - - - if (bitcoind->rpcpass) { - write_all(in, bitcoind->rpcpass, strlen(bitcoind->rpcpass)); - write_all(in, "\n", strlen("\n")); - } - for (size_t i = 0; i < tal_count(bcli->stdinargs); i++) { - write_all(in, bcli->stdinargs[i], strlen(bcli->stdinargs[i])); - write_all(in, "\n", strlen("\n")); - } - close(in); - - bcli->start = time_mono(); - - bitcoind->num_requests[prio]++; - - /* We don't keep a pointer to this, but it's not a leak */ - conn = notleak(io_new_conn(bcli, bcli->fd, output_init, bcli)); - io_set_finish(conn, bcli_finished, bcli); - - list_add_tail(&bitcoind->current, &bcli->list); - tal_add_destructor(bcli, destroy_bcli); -} - -static void -start_bitcoin_cliv(const tal_t *ctx, - struct command *cmd, - struct command_result *(*process)(struct bitcoin_cli *), - bool nonzero_exit_ok, - enum bitcoind_prio prio, - void *stash, - const char *method, - va_list ap) -{ - struct bitcoin_cli *bcli = tal(bitcoind, struct bitcoin_cli); - - bcli->process = process; - bcli->cmd = cmd; - bcli->prio = prio; - - if (nonzero_exit_ok) - bcli->exitstatus = tal(bcli, int); - else - bcli->exitstatus = NULL; - - bcli->stdinargs = tal_arr(bcli, const char *, 0); - bcli->args = gather_argsv(bcli, &bcli->stdinargs, method, ap); - bcli->stash = stash; - - list_add_tail(&bitcoind->pending[bcli->prio], &bcli->list); - next_bcli(bcli->prio); -} - -/* If ctx is non-NULL, and is freed before we return, we don't call process(). - * process returns false() if it's a spurious error, and we should retry. */ -static void LAST_ARG_NULL -start_bitcoin_cli(const tal_t *ctx, - struct command *cmd, - struct command_result *(*process)(struct bitcoin_cli *), - bool nonzero_exit_ok, - enum bitcoind_prio prio, - void *stash, - const char *method, - ...) -{ - va_list ap; - - va_start(ap, method); - start_bitcoin_cliv(ctx, cmd, process, nonzero_exit_ok, prio, stash, method, - ap); - va_end(ap); -} - static void strip_trailing_whitespace(char *str, size_t len) { size_t stripped_len = len; @@ -484,15 +229,6 @@ static void strip_trailing_whitespace(char *str, size_t len) str[stripped_len] = 0x00; } -static struct command_result *command_err_bcli_badjson(struct bitcoin_cli *bcli, - const char *errmsg) -{ - char *err = tal_fmt(bcli, "%s: bad JSON: %s (%.*s)", - bcli_args(tmpctx, bcli), errmsg, - (int)bcli->output_bytes, bcli->output); - return command_done_err(bcli->cmd, BCLI_ERROR, err, NULL); -} - static struct command_result *command_err(struct command *cmd, struct bcli_result *res, const char *errmsg) @@ -508,80 +244,6 @@ static void json_add_null(struct json_stream *stream, const char *fieldname) json_add_primitive(stream, fieldname, "null"); } -static UNNEEDED struct command_result *process_getutxout(struct bitcoin_cli *bcli) -{ - const jsmntok_t *tokens; - struct json_stream *response; - struct bitcoin_tx_output output; - const char *err; - - /* As of at least v0.15.1.0, bitcoind returns "success" but an empty - string on a spent txout. */ - if (*bcli->exitstatus != 0 || bcli->output_bytes == 0) { - response = jsonrpc_stream_success(bcli->cmd); - json_add_null(response, "amount"); - json_add_null(response, "script"); - - return command_finished(bcli->cmd, response); - } - - tokens = json_parse_simple(bcli->output, bcli->output, - bcli->output_bytes); - if (!tokens) { - return command_err_bcli_badjson(bcli, "cannot parse"); - } - - err = json_scan(tmpctx, bcli->output, tokens, - "{value:%,scriptPubKey:{hex:%}}", - JSON_SCAN(json_to_bitcoin_amount, - &output.amount.satoshis), /* Raw: bitcoind */ - JSON_SCAN_TAL(bcli, json_tok_bin_from_hex, - &output.script)); - if (err) - return command_err_bcli_badjson(bcli, err); - - response = jsonrpc_stream_success(bcli->cmd); - json_add_sats(response, "amount", output.amount); - json_add_string(response, "script", tal_hex(response, output.script)); - - return command_finished(bcli->cmd, response); -} - -static UNNEEDED struct command_result *process_getblockchaininfo(struct bitcoin_cli *bcli) -{ - const jsmntok_t *tokens; - struct json_stream *response; - bool ibd; - u32 headers, blocks; - const char *chain, *err; - - tokens = json_parse_simple(bcli->output, - bcli->output, bcli->output_bytes); - if (!tokens) { - return command_err_bcli_badjson(bcli, "cannot parse"); - } - - err = json_scan(tmpctx, bcli->output, tokens, - "{chain:%,headers:%,blocks:%,initialblockdownload:%}", - JSON_SCAN_TAL(tmpctx, json_strdup, &chain), - JSON_SCAN(json_to_number, &headers), - JSON_SCAN(json_to_number, &blocks), - JSON_SCAN(json_to_bool, &ibd)); - if (err) - return command_err_bcli_badjson(bcli, err); - - if (bitcoind->dev_ignore_ibd) - ibd = false; - - response = jsonrpc_stream_success(bcli->cmd); - json_add_string(response, "chain", chain); - json_add_u32(response, "headercount", headers); - json_add_u32(response, "blockcount", blocks); - json_add_bool(response, "ibd", ibd); - - return command_finished(bcli->cmd, response); -} - struct estimatefee_params { u32 blocks; const char *style; @@ -594,14 +256,6 @@ static const struct estimatefee_params estimatefee_params[] = { { 100, "ECONOMICAL" }, }; -struct estimatefees_stash { - /* This is max(mempoolminfee,minrelaytxfee) */ - u64 perkb_floor; - u32 cursor; - /* FIXME: We use u64 but lightningd will store them as u32. */ - u64 perkb[ARRAY_SIZE(estimatefee_params)]; -}; - static struct command_result * estimatefees_null_response(struct command *cmd) { @@ -615,228 +269,6 @@ estimatefees_null_response(struct command *cmd) return command_finished(cmd, response); } -static UNNEEDED struct command_result * -estimatefees_parse_feerate(struct bitcoin_cli *bcli, u64 *feerate) -{ - const jsmntok_t *tokens; - - tokens = json_parse_simple(bcli->output, - bcli->output, bcli->output_bytes); - if (!tokens) { - return command_err_bcli_badjson(bcli, "cannot parse"); - } - - if (json_scan(tmpctx, bcli->output, tokens, "{feerate:%}", - JSON_SCAN(json_to_bitcoin_amount, feerate)) != NULL) { - /* Paranoia: if it had a feerate, but was malformed: */ - if (json_get_member(bcli->output, tokens, "feerate")) - return command_err_bcli_badjson(bcli, "cannot scan"); - /* Regtest fee estimation is generally awful: Fake it at min. */ - if (bitcoind->fake_fees) { - *feerate = 1000; - return NULL; - } - /* We return null if estimation failed, and bitcoin-cli will - * exit with 0 but no feerate field on failure. */ - return estimatefees_null_response(bcli->cmd); - } - - return NULL; -} - -static UNNEEDED struct command_result *process_sendrawtransaction(struct bitcoin_cli *bcli) -{ - struct json_stream *response; - - /* This is useful for functional tests. */ - if (bcli->exitstatus) - plugin_log(bcli->cmd->plugin, LOG_DBG, - "sendrawtx exit %i (%s) %.*s", - *bcli->exitstatus, bcli_args(tmpctx, bcli), - *bcli->exitstatus ? - (u32)bcli->output_bytes-1 : 0, - bcli->output); - - response = jsonrpc_stream_success(bcli->cmd); - json_add_bool(response, "success", - *bcli->exitstatus == 0 || - *bcli->exitstatus == - RPC_TRANSACTION_ALREADY_IN_CHAIN); - json_add_string(response, "errmsg", - *bcli->exitstatus ? - tal_strndup(bcli->cmd, - bcli->output, bcli->output_bytes-1) - : ""); - - return command_finished(bcli->cmd, response); -} - -struct getrawblock_stash { - const char *block_hash; - u32 block_height; - const char *block_hex; - int *peers; -}; - -/* Mutual recursion. */ -static UNNEEDED struct command_result *getrawblock(struct bitcoin_cli *bcli); - -static UNNEEDED struct command_result *process_rawblock(struct bitcoin_cli *bcli) -{ - struct json_stream *response; - struct getrawblock_stash *stash = bcli->stash; - - strip_trailing_whitespace(bcli->output, bcli->output_bytes); - stash->block_hex = tal_steal(stash, bcli->output); - - response = jsonrpc_stream_success(bcli->cmd); - json_add_string(response, "blockhash", stash->block_hash); - json_add_string(response, "block", stash->block_hex); - - return command_finished(bcli->cmd, response); -} - -static UNNEEDED struct command_result *process_getblockfrompeer(struct bitcoin_cli *bcli) -{ - /* Remove the peer that we tried to get the block from and move along, - * we may also check on errors here */ - struct getrawblock_stash *stash = bcli->stash; - - if (bcli->exitstatus && *bcli->exitstatus != 0) { - /* We still continue with the execution if we can not fetch the - * block from peer */ - plugin_log(bcli->cmd->plugin, LOG_DBG, - "failed to fetch block %s from peer %i, skip.", - stash->block_hash, stash->peers[tal_count(stash->peers) - 1]); - } else { - plugin_log(bcli->cmd->plugin, LOG_DBG, - "try to fetch block %s from peer %i.", - stash->block_hash, stash->peers[tal_count(stash->peers) - 1]); - } - tal_resize(&stash->peers, tal_count(stash->peers) - 1); - - /* `getblockfrompeer` is an async call. sleep for a second to allow the - * block to be delivered by the peer. fixme: We could also sleep for - * double the last ping here (with sanity limit)*/ - sleep(1); - - return getrawblock(bcli); -} - -static UNNEEDED struct command_result *process_getpeerinfo(struct bitcoin_cli *bcli) -{ - const jsmntok_t *t, *toks; - struct getrawblock_stash *stash = bcli->stash; - size_t i; - - toks = - json_parse_simple(bcli->output, bcli->output, bcli->output_bytes); - - if (!toks) { - return command_err_bcli_badjson(bcli, "cannot parse"); - } - - stash->peers = tal_arr(bcli->stash, int, 0); - - json_for_each_arr(i, t, toks) - { - int id; - u8 *services; - - if (json_scan(tmpctx, bcli->output, t, "{id:%,services:%}", - JSON_SCAN(json_to_int, &id), - JSON_SCAN_TAL(tmpctx, json_tok_bin_from_hex, &services)) == NULL) { - /* From bitcoin source: - * // NODE_NETWORK means that the node is capable of serving the complete block chain. It is currently - * // set by all Bitcoin Core non pruned nodes, and is unset by SPV clients or other light clients. - * NODE_NETWORK = (1 << 0) - */ - if (tal_count(services) > 0 && (services[tal_count(services)-1] & (1<<0))) { - // fixme: future optimization: sort by last ping - tal_arr_expand(&stash->peers, id); - } - } - } - - if (tal_count(stash->peers) <= 0) { - /* We don't have peers yet, retry from `getrawblock` */ - plugin_log(bcli->cmd->plugin, LOG_DBG, - "got an empty peer list."); - return getrawblock(bcli); - } - - start_bitcoin_cli(NULL, bcli->cmd, process_getblockfrompeer, true, - BITCOIND_HIGH_PRIO, stash, "getblockfrompeer", - stash->block_hash, - take(tal_fmt(NULL, "%i", stash->peers[tal_count(stash->peers) - 1])), NULL); - - return command_still_pending(bcli->cmd); -} - -static UNNEEDED struct command_result *process_getrawblock(struct bitcoin_cli *bcli) -{ - /* We failed to get the raw block. */ - if (bcli->exitstatus && *bcli->exitstatus != 0) { - struct getrawblock_stash *stash = bcli->stash; - - plugin_log(bcli->cmd->plugin, LOG_DBG, - "failed to fetch block %s from the bitcoin backend (maybe pruned).", - stash->block_hash); - - if (bitcoind->version >= 230000) { - /* `getblockformpeer` was introduced in v23.0.0 */ - - if (!stash->peers) { - /* We don't have peers to fetch blocks from, get - * some! */ - start_bitcoin_cli(NULL, bcli->cmd, - process_getpeerinfo, true, - BITCOIND_HIGH_PRIO, stash, - "getpeerinfo", NULL); - - return command_still_pending(bcli->cmd); - } - - if (tal_count(stash->peers) > 0) { - /* We have peers left that we can ask for the - * block */ - start_bitcoin_cli( - NULL, bcli->cmd, process_getblockfrompeer, - true, BITCOIND_HIGH_PRIO, stash, - "getblockfrompeer", stash->block_hash, - take(tal_fmt(NULL, "%i", stash->peers[tal_count(stash->peers) - 1])), - NULL); - - return command_still_pending(bcli->cmd); - } - - /* We failed to fetch the block from from any peer we - * got. */ - plugin_log( - bcli->cmd->plugin, LOG_DBG, - "asked all known peers about block %s, retry", - stash->block_hash); - stash->peers = tal_free(stash->peers); - } - - return NULL; - } - - return process_rawblock(bcli); -} - -static UNNEEDED struct command_result * -getrawblockbyheight_notfound_bcli(struct bitcoin_cli *bcli) -{ - struct json_stream *response; - - response = jsonrpc_stream_success(bcli->cmd); - json_add_null(response, "blockhash"); - json_add_null(response, "block"); - - return command_finished(bcli->cmd, response); -} - static struct command_result * getrawblockbyheight_notfound(struct command *cmd) { @@ -849,40 +281,6 @@ getrawblockbyheight_notfound(struct command *cmd) return command_finished(cmd, response); } -static UNNEEDED struct command_result *getrawblock(struct bitcoin_cli *bcli) -{ - struct getrawblock_stash *stash = bcli->stash; - - start_bitcoin_cli(NULL, bcli->cmd, process_getrawblock, true, - BITCOIND_HIGH_PRIO, stash, "getblock", - stash->block_hash, - /* Non-verbose: raw block. */ - "0", NULL); - - return command_still_pending(bcli->cmd); -} - -static UNNEEDED struct command_result *process_getblockhash(struct bitcoin_cli *bcli) -{ - struct getrawblock_stash *stash = bcli->stash; - - /* If it failed with error 8, give an empty response. */ - if (bcli->exitstatus && *bcli->exitstatus != 0) { - /* Other error means we have to retry. */ - if (*bcli->exitstatus != 8) - return NULL; - return getrawblockbyheight_notfound_bcli(bcli); - } - - strip_trailing_whitespace(bcli->output, bcli->output_bytes); - stash->block_hash = tal_strdup(stash, bcli->output); - if (!stash->block_hash || strlen(stash->block_hash) != 64) { - return command_err_bcli_badjson(bcli, "bad blockhash"); - } - - return getrawblock(bcli); -} - /* Get peers that support NODE_NETWORK (full nodes). * Returns array of peer ids, or empty array if none found. */ static int *get_fullnode_peers(const tal_t *ctx, struct command *cmd) @@ -1081,9 +479,6 @@ static struct command_result *getchaininfo(struct command *cmd, return command_finished(cmd, response); } -/* Mutual recursion. */ -static struct command_result *estimatefees_done(struct bitcoin_cli *bcli); - /* Add a feerate, but don't publish one that bitcoind won't accept. */ static void json_add_feerate(struct json_stream *result, const char *fieldname, struct command *cmd, @@ -1110,69 +505,6 @@ static void json_add_feerate(struct json_stream *result, const char *fieldname, } } -static UNNEEDED struct command_result *estimatefees_next(struct command *cmd, - struct estimatefees_stash *stash) -{ - struct json_stream *response; - - if (stash->cursor < ARRAY_SIZE(stash->perkb)) { - start_bitcoin_cli(NULL, cmd, estimatefees_done, true, - BITCOIND_LOW_PRIO, stash, - "estimatesmartfee", - take(tal_fmt(NULL, "%u", - estimatefee_params[stash->cursor].blocks)), - estimatefee_params[stash->cursor].style, - NULL); - - return command_still_pending(cmd); - } - - response = jsonrpc_stream_success(cmd); - /* Present an ordered array of block deadlines, and a floor. */ - json_array_start(response, "feerates"); - for (size_t i = 0; i < ARRAY_SIZE(stash->perkb); i++) { - if (!stash->perkb[i]) - continue; - json_object_start(response, NULL); - json_add_u32(response, "blocks", estimatefee_params[i].blocks); - json_add_feerate(response, "feerate", cmd, stash->perkb_floor, stash->perkb[i]); - json_object_end(response); - } - json_array_end(response); - json_add_u64(response, "feerate_floor", stash->perkb_floor); - return command_finished(cmd, response); -} - -static UNNEEDED struct command_result *getminfees_done(struct bitcoin_cli *bcli) -{ - const jsmntok_t *tokens; - const char *err; - u64 mempoolfee, relayfee; - struct estimatefees_stash *stash = bcli->stash; - - if (*bcli->exitstatus != 0) - return estimatefees_null_response(bcli->cmd); - - tokens = json_parse_simple(bcli->output, - bcli->output, bcli->output_bytes); - if (!tokens) - return command_err_bcli_badjson(bcli, - "cannot parse getmempoolinfo"); - - /* Look at minrelaytxfee they configured, and current min fee to get - * into mempool. */ - err = json_scan(tmpctx, bcli->output, tokens, - "{mempoolminfee:%,minrelaytxfee:%}", - JSON_SCAN(json_to_bitcoin_amount, &mempoolfee), - JSON_SCAN(json_to_bitcoin_amount, &relayfee)); - if (err) - return command_err_bcli_badjson(bcli, err); - - stash->perkb_floor = max_u64(mempoolfee, relayfee); - stash->cursor = 0; - return estimatefees_next(bcli->cmd, stash); -} - /* Get the feerate floor from getmempoolinfo. * Returns NULL on success (floor stored in *perkb_floor), or error response. */ static struct command_result *get_feerate_floor(struct command *cmd, @@ -1248,7 +580,7 @@ static struct command_result *estimatefees(struct command *cmd, const jsmntok_t *toks UNUSED) { struct command_result *err; - u64 perkb_floor; + u64 perkb_floor = 0; u64 perkb[ARRAY_SIZE(estimatefee_params)]; struct json_stream *response; @@ -1281,23 +613,6 @@ static struct command_result *estimatefees(struct command *cmd, return command_finished(cmd, response); } -static UNNEEDED struct command_result *estimatefees_done(struct bitcoin_cli *bcli) -{ - struct command_result *err; - struct estimatefees_stash *stash = bcli->stash; - - /* If we cannot estimate fees, no need to continue bothering bitcoind. */ - if (*bcli->exitstatus != 0) - return estimatefees_null_response(bcli->cmd); - - err = estimatefees_parse_feerate(bcli, &stash->perkb[stash->cursor]); - if (err) - return err; - - stash->cursor++; - return estimatefees_next(bcli->cmd, stash); -} - /* Send a transaction to the Bitcoin network. * Calls `sendrawtransaction` using the first parameter as the raw tx. */ @@ -1326,11 +641,11 @@ static struct command_result *sendrawtransaction(struct command *cmd, "sendrawtransaction", tx, highfeesarg, NULL); /* This is useful for functional tests. */ - if (res->exitstatus) - plugin_log(cmd->plugin, LOG_DBG, - "sendrawtx exit %i (%s)", - res->exitstatus, - res->output); + plugin_log(cmd->plugin, LOG_DBG, + "sendrawtx exit %i (%s) %.*s", + res->exitstatus, res->args, + res->exitstatus ? (int)res->output_len : 0, + res->output); response = jsonrpc_stream_success(cmd); json_add_bool(response, "success", @@ -1468,6 +783,7 @@ static void wait_and_check_bitcoind(struct plugin *p) } output = grab_fd_str(cmd, from); + close(from); waitpid(child, &status, 0); @@ -1543,12 +859,6 @@ static struct bitcoind *new_bitcoind(const tal_t *ctx) bitcoind->cli = NULL; bitcoind->datadir = NULL; - for (size_t i = 0; i < BITCOIND_NUM_PRIO; i++) { - bitcoind->num_requests[i] = 0; - list_head_init(&bitcoind->pending[i]); - } - list_head_init(&bitcoind->current); - bitcoind->error_count = 0; bitcoind->retry_timeout = 60; bitcoind->rpcuser = NULL; bitcoind->rpcpass = NULL; From 3a5e2d3c1d498adbf4b67e2adb05274004fa3706 Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Tue, 13 Jan 2026 23:02:52 +0200 Subject: [PATCH 10/13] bcli: return "not found" on any `getblockhash` exit status Return "not found" on any `getblockhash` exit status. Previously, only exit code 8 (block height doesn't exist) returned "not found", while other exit codes returned an error. Now any non-zero exit status returns "not found" since any failure means the block is unavailable. --- plugins/bcli.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index 4616c58b55aa..b914714d867c 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -344,10 +344,7 @@ static struct command_result *getrawblockbyheight(struct command *cmd, tal_fmt(tmpctx, "%u", *height), NULL); if (res->exitstatus != 0) { - /* Exit code 8 means block height doesn't exist (empty response) */ - if (res->exitstatus == 8) - return getrawblockbyheight_notfound(cmd); - return command_err(cmd, res, "command failed"); + return getrawblockbyheight_notfound(cmd); } strip_trailing_whitespace(res->output, res->output_len); From a40612ac85804af47180c9d78e5d7c34a7a62b5e Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Sat, 17 Jan 2026 20:54:33 +0200 Subject: [PATCH 11/13] bcli: refactor `wait_and_check_bitcoind` and `run_bitcoin_cli` to use shared execution Extract `execute_bitcoin_cli` as shared function used by both `run_bitcoin_cli` and `wait_and_check_bitcoind`. --- plugins/bcli.c | 93 +++++++++++++++++++++++--------------------------- 1 file changed, 42 insertions(+), 51 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index b914714d867c..c9ca1ec8525c 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -148,23 +148,18 @@ static char *args_string(const tal_t *ctx, const char **args, const char **stdin return ret; } -/* Synchronous execution of bitcoin-cli. +/* Execute bitcoin-cli with pre-built command and optional stdin args. * Returns result with output and exit status. */ static struct bcli_result * -run_bitcoin_cliv(const tal_t *ctx, - struct plugin *plugin, - const char *method, - va_list ap) +execute_bitcoin_cli(const tal_t *ctx, + struct plugin *plugin, + const char **cmd, + const char **stdinargs) { int in, from, status; pid_t child; - const char **stdinargs; - const char **cmd; struct bcli_result *res; - stdinargs = tal_arr(ctx, const char *, 0); - cmd = gather_argsv(ctx, &stdinargs, method, ap); - child = pipecmdarr(&in, &from, &from, cast_const2(char **, cmd)); if (child < 0) plugin_err(plugin, "%s exec failed: %s", cmd[0], strerror(errno)); @@ -175,9 +170,11 @@ run_bitcoin_cliv(const tal_t *ctx, write_all(in, "\n", 1); } /* Send any additional stdin args */ - for (size_t i = 0; i < tal_count(stdinargs); i++) { - write_all(in, stdinargs[i], strlen(stdinargs[i])); - write_all(in, "\n", 1); + if (stdinargs) { + for (size_t i = 0; i < tal_count(stdinargs); i++) { + write_all(in, stdinargs[i], strlen(stdinargs[i])); + write_all(in, "\n", 1); + } } close(in); @@ -205,6 +202,23 @@ run_bitcoin_cliv(const tal_t *ctx, return res; } +/* Synchronous execution of bitcoin-cli. + * Returns result with output and exit status. */ +static struct bcli_result * +run_bitcoin_cliv(const tal_t *ctx, + struct plugin *plugin, + const char *method, + va_list ap) +{ + const char **stdinargs; + const char **cmd; + + stdinargs = tal_arr(ctx, const char *, 0); + cmd = gather_argsv(ctx, &stdinargs, method, ap); + + return execute_bitcoin_cli(ctx, plugin, cmd, stdinargs); +} + static LAST_ARG_NULL struct bcli_result * run_bitcoin_cli(const tal_t *ctx, struct plugin *plugin, @@ -757,51 +771,28 @@ static void parse_getnetworkinfo_result(struct plugin *p, const char *buf) static void wait_and_check_bitcoind(struct plugin *p) { - int in, from, status; - pid_t child; - const char **cmd = gather_args( - bitcoind, NULL, "-rpcwait", "-rpcwaittimeout=30", "getnetworkinfo", NULL); - char *output = NULL; - - child = pipecmdarr(&in, &from, &from, cast_const2(char **, cmd)); - - if (bitcoind->rpcpass) - write_all(in, bitcoind->rpcpass, strlen(bitcoind->rpcpass)); - - close(in); - - if (child < 0) { - if (errno == ENOENT) - bitcoind_failure( - p, - "bitcoin-cli not found. Is bitcoin-cli " - "(part of Bitcoin Core) available in your PATH?"); - plugin_err(p, "%s exec failed: %s", cmd[0], strerror(errno)); - } - - output = grab_fd_str(cmd, from); - close(from); + struct bcli_result *res; + const char **cmd; - waitpid(child, &status, 0); + /* Special case: -rpcwait flags go on command line, not stdin */ + cmd = gather_args(bitcoind, NULL, "-rpcwait", "-rpcwaittimeout=30", + "getnetworkinfo", NULL); + res = execute_bitcoin_cli(bitcoind, p, cmd, NULL); - if (!WIFEXITED(status)) - bitcoind_failure(p, tal_fmt(bitcoind, "Death of %s: signal %i", - cmd[0], WTERMSIG(status))); - - if (WEXITSTATUS(status) != 0) { - if (WEXITSTATUS(status) == 1) - bitcoind_failure(p, - "RPC connection timed out. Could " - "not connect to bitcoind using " - "bitcoin-cli. Is bitcoind running?"); + if (res->exitstatus == 1) + bitcoind_failure(p, + "RPC connection timed out. Could " + "not connect to bitcoind using " + "bitcoin-cli. Is bitcoind running?"); + if (res->exitstatus != 0) bitcoind_failure(p, tal_fmt(bitcoind, "%s exited with code %i: %s", - cmd[0], WEXITSTATUS(status), output)); - } + res->args, res->exitstatus, res->output)); - parse_getnetworkinfo_result(p, output); + parse_getnetworkinfo_result(p, res->output); tal_free(cmd); + tal_free(res); } static void memleak_mark_bitcoind(struct plugin *p, struct htable *memtable) From 9bdfb42bf8b2ffc44afea798c9c49ef1007a33ff Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Sat, 17 Jan 2026 21:07:50 +0200 Subject: [PATCH 12/13] bcli: replace magic numbers with constants --- plugins/bcli.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index c9ca1ec8525c..67271bb0e9db 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -13,8 +13,15 @@ #include #include +/* Bitcoin Core RPC error code for duplicate transaction */ #define RPC_TRANSACTION_ALREADY_IN_CHAIN -27 +/* Hex-encoded SHA256 block hash length (32 bytes = 64 hex chars) */ +#define BLOCK_HASH_HEX_LEN 64 + +/* Bitcoin Core version 23.0.0 introduced getblockfrompeer RPC */ +#define BITCOIND_VERSION_GETBLOCKFROMPEER 230000 + struct bitcoind { /* eg. "bitcoin-cli" */ char *cli; @@ -362,7 +369,7 @@ static struct command_result *getrawblockbyheight(struct command *cmd, } strip_trailing_whitespace(res->output, res->output_len); - if (strlen(res->output) != 64) + if (strlen(res->output) != BLOCK_HASH_HEX_LEN) return command_err(cmd, res, "bad JSON: bad blockhash"); block_hash = tal_strdup(cmd, res->output); @@ -396,7 +403,7 @@ static struct command_result *getrawblockbyheight(struct command *cmd, } /* Try fetching from peers if bitcoind >= 23.0.0 */ - if (bitcoind->version >= 230000) { + if (bitcoind->version >= BITCOIND_VERSION_GETBLOCKFROMPEER) { if (!peers) peers = get_fullnode_peers(cmd, cmd); From 468a74fa4ef34c5f31e62ede18d426fd74e16754 Mon Sep 17 00:00:00 2001 From: dovgopoly Date: Sun, 18 Jan 2026 12:15:07 +0200 Subject: [PATCH 13/13] pytest: add tests for bcli `getblockfrompeer` retry path Add `test_bcli_concurrent` to verify bcli handles concurrent requests while the `getblockfrompeer` retry path is active, simulating a pruned node scenario where `getblock` initially fails. Add `test_bcli_retry_timeout` to verify lightningd crashes with a clear error message when we run out of `getblock` retries. --- tests/test_plugin.py | 106 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 6e2a94ff1e58..727b1463969e 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1,3 +1,4 @@ +from bitcoin.rpc import RawProxy from collections import OrderedDict from datetime import datetime from fixtures import * # noqa: F401,F403 @@ -1991,6 +1992,111 @@ def test_bcli(node_factory, bitcoind, chainparams): assert not resp["success"] and "decode failed" in resp["errmsg"] +def test_bcli_concurrent(node_factory, bitcoind, executor): + """Test bcli handles concurrent requests while `getblockfrompeer` retry is active. + + Simulates a pruned node scenario where getblock initially fails. The bcli + plugin should use getblockfrompeer to fetch the block from peers, then + retry `getblock` successfully. Meanwhile, other concurrent requests + (`getchaininfo`, `estimatefees`) should complete normally. + """ + retry_count = 5 + getblockfrompeer_count = 0 + + def mock_getblock(r): + if getblockfrompeer_count >= retry_count: + conf_file = os.path.join(bitcoind.bitcoin_dir, "bitcoin.conf") + brpc = RawProxy(btc_conf_file=conf_file) + return { + "result": brpc._call(r["method"], *r["params"]), + "error": None, + "id": r["id"] + } + return { + "id": r["id"], + "result": None, + "error": {"code": -1, "message": "Block not available (pruned data)"} + } + + def mock_getpeerinfo(r): + return {"id": r["id"], "result": [{"id": 1, "services": "000000000000040d"}]} + + def mock_getblockfrompeer(r): + nonlocal getblockfrompeer_count + getblockfrompeer_count += 1 + return {"id": r["id"], "result": {}} + + l1 = node_factory.get_node(start=False) + l1.daemon.rpcproxy.mock_rpc("getblock", mock_getblock) + l1.daemon.rpcproxy.mock_rpc("getpeerinfo", mock_getpeerinfo) + l1.daemon.rpcproxy.mock_rpc("getblockfrompeer", mock_getblockfrompeer) + l1.start(wait_for_bitcoind_sync=False) + + # Submit concurrent bcli requests, `getrawblockbyheight` hits a retry path. + block_future = executor.submit(l1.rpc.call, "getrawblockbyheight", {"height": 1}) + chaininfo_futures = [] + fees_futures = [] + for _ in range(5): + chaininfo_futures.append(executor.submit(l1.rpc.call, "getchaininfo", {"last_height": 0})) + fees_futures.append(executor.submit(l1.rpc.call, "estimatefees")) + + block_result = block_future.result(TIMEOUT) + assert "blockhash" in block_result + assert "block" in block_result + + for fut in chaininfo_futures: + result = fut.result(TIMEOUT) + assert "chain" in result + assert "blockcount" in result + + for fut in fees_futures: + result = fut.result(TIMEOUT) + assert "feerates" in result + assert "feerate_floor" in result + + assert getblockfrompeer_count == retry_count + + +def test_bcli_retry_timeout(node_factory, bitcoind): + """Test that lightningd crashes when getblock retries are exhausted. + + Currently, when bcli returns an error after retry timeout, lightningd's + get_bitcoin_result() calls fatal(). This test documents that behavior. + """ + getblockfrompeer_count = 0 + + def mock_getblock(r): + return { + "id": r["id"], + "result": None, + "error": {"code": -1, "message": "Block not available (pruned data)"} + } + + def mock_getpeerinfo(r): + return {"id": r["id"], "result": [{"id": 1, "services": "000000000000040d"}]} + + def mock_getblockfrompeer(r): + nonlocal getblockfrompeer_count + getblockfrompeer_count += 1 + return {"id": r["id"], "result": {}} + + l1 = node_factory.get_node(may_fail=True, + broken_log=r'getrawblockbyheight|FATAL SIGNAL|backtrace', + options={"bitcoin-retry-timeout": 3}) + sync_blockheight(bitcoind, [l1]) + + l1.daemon.rpcproxy.mock_rpc("getblock", mock_getblock) + l1.daemon.rpcproxy.mock_rpc("getpeerinfo", mock_getpeerinfo) + l1.daemon.rpcproxy.mock_rpc("getblockfrompeer", mock_getblockfrompeer) + + # Mine a new block - lightningd will try to fetch it and crash. + bitcoind.generate_block(1) + + l1.daemon.wait_for_log(r"timed out after 3 seconds") + assert l1.daemon.wait() != 0 + assert getblockfrompeer_count > 0 + + @unittest.skipIf(TEST_NETWORK != 'regtest', 'p2tr addresses not supported by elementsd') def test_hook_crash(node_factory, executor, bitcoind): """Verify that we fail over if a plugin crashes while handling a hook.