Skip to content

Feat/refactor-bcli #8820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dovgopoly wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Feat/refactor-bcli #8820

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
598b984
bcli: add synchronous `run_bitcoin_cli` for future refactor
dovgopoly Jan 5, 2026
5138815
bcli: convert `getchaininfo` to synchronous execution
dovgopoly Jan 5, 2026
5b48c8a
bcli: convert `getutxout` to synchronous execution
dovgopoly Jan 5, 2026
5c65ab6
bcli: convert `sendrawtransaction` to synchronous execution
dovgopoly Jan 5, 2026
fa77e4b
bcli: convert `estimatefees` to synchronous execution
dovgopoly Jan 6, 2026
eb449e5
bcli: convert `getrawblockbyheight` to synchronous execution
dovgopoly Jan 8, 2026
57d5946
lightningd: add error checking for bcli plugin responses
dovgopoly Jan 9, 2026
8c0d9e5
pytest: fix bcli tests after sync refactor
dovgopoly Jan 9, 2026
ef431c4
bcli: remove unused async code after sync refactor
dovgopoly Jan 9, 2026
dddf995
bcli: return "not found" on any `getblockhash` exit status
dovgopoly Jan 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 160 additions & 39 deletions plugins/bcli.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
Expand All @@ -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 */
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;

Expand All @@ -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;

Expand All @@ -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;

Expand All @@ -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);
Expand All @@ -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).
Expand All @@ -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)
Copy link
Collaborator

@sangbida sangbida Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: magic number

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) {
Copy link
Collaborator

@sangbida sangbida Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: magic number

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);
Copy link
Collaborator

@sangbida sangbida Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

/* Get infos about the block chain.
Expand Down Expand Up @@ -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:%}",
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Copy link
Collaborator

@sangbida sangbida Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@sangbida sangbida Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
Expand All @@ -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;
Expand Down Expand Up @@ -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:%}}",
Expand All @@ -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);
Expand Down