bcli: convert `estimatefees` to synchronous execution
What changed, and why it matters
This change rewrites how Core Lightning asks Bitcoin for fee estimates, switching from an asynchronous (multi-step callback) style to a simpler synchronous style. It also adds a shared helper for reporting bad JSON responses from bitcoin-cli and fixes a typo in a log message ('32 bites' to '32 bits'). The rewrite does not obviously add or remove a vulnerability, but it changes timing and error-handling behavior in a security-sensitive area (fee estimation), so it warrants careful review.
Review the synchronous blocking behavior under slow bitcoind conditions and confirm that marking helpers `UNNEEDED` does not leave dead code that can be re-enabled by future patches. Verify that `command_err_badjson` does not expose sensitive backend output to RPC consumers. No immediate patch or incident response is indicated by the diff alone.
Security signals we found
Behavioral change in fee-estimation error path: previously a failure in one estimatesmartfee call could still allow partial results; now any failure returns an error/null response immediately, which may change fee fallback behavior.
New helper centralizes JSON parse/scan error messages and includes raw bitcoin-cli output; this could leak verbose backend output to RPC callers if output contains sensitive data, though no such data is evident.
Synchronous bitcoin-cli calls block the plugin thread; a slow or hung `estimatesmartfee`/`getmempoolinfo` could delay or deny fee responses, but this is a denial-of-service/availability concern rather than a code-execution vulnerability.
Old async functions marked `UNNEEDED` remain compiled; if still referenced or re-enabled accidentally they could create stale logic, but the diff shows no active references.
No input validation changes are visible; parameters still flow through existing `param()` and `tal_fmt()` paths.
Typo fix in log message ('32 bites' -> '32 bits') is benign.
Evidence from the diff
The patch converts estimatefees in plugins/bcli.c from an async chain (start_bitcoin_cli -> getminfees_done -> estimatefees_next -> estimatefees_done) to synchronous calls (run_bitcoin_cli) inside estimatefees(). It introduces get_feerate_floor() and get_feerate() helpers, adds command_err_badjson() for uniform JSON error responses, and stores args in struct bcli_result. Several old async helpers are marked UNNEEDED but left in the file. Error handling for getchaininfo and getutxout is refactored to use the same helper. The change is architectural/cleanup; no buffer overflow, injection, or cryptographic bug is directly visible in the diff.
Changed components
plugins/bcli.cestimatefees RPC handlerbitcoin-cli invocation layergetchaininfo error handlinggetutxout error handlingInspect captured patch +134 / −50
diff --git a/plugins/bcli.c b/plugins/bcli.c
index ce0d6c12..5174012b 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);
Why this scored 26/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.