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
+390 −705
Open

Feat/refactor-bcli #8820

Show file tree
Hide file tree
Changes from all commits
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
29 changes: 29 additions & 0 deletions lightningd/bitcoind.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,25 @@ static void bitcoin_plugin_error(struct bitcoind *bitcoind, const char *buf,
toks->end - toks->start, buf + toks->start);
}

/* Check if response is an error and fail with clear message if so */
static void check_bitcoin_error(struct bitcoind *bitcoind, const char *buf,
const jsmntok_t *toks, const char *method)
{
const jsmntok_t *err_tok, *msg_tok;

err_tok = json_get_member(buf, toks, "error");
if (!err_tok)
return;

msg_tok = json_get_member(buf, err_tok, "message");
if (msg_tok)
fatal("%s: %.*s", method,
json_tok_full_len(msg_tok), json_tok_full(buf, msg_tok));
else
fatal("%s: %.*s", method,
json_tok_full_len(err_tok), json_tok_full(buf, err_tok));
}

Comment on lines +114 to +132
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.

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

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

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

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

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

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

getrawblockbyheight - we have a retry on this one

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

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

/* Send a request to the Bitcoin plugin which registered that method,
* if it's still alive. */
static void bitcoin_plugin_send(struct bitcoind *bitcoind,
Expand Down Expand Up @@ -287,6 +306,8 @@ static void estimatefees_callback(const char *buf, const jsmntok_t *toks,
struct feerate_est *feerates;
u32 floor;

check_bitcoin_error(call->bitcoind, buf, toks, "estimatefees");

resulttok = json_get_member(buf, toks, "result");
if (!resulttok)
bitcoin_plugin_error(call->bitcoind, buf, toks,
Expand Down Expand Up @@ -390,6 +411,8 @@ static void sendrawtx_callback(const char *buf, const jsmntok_t *toks,
const char *errmsg = NULL;
bool success = false;

check_bitcoin_error(call->bitcoind, buf, toks, "sendrawtransaction");

err = json_scan(tmpctx, buf, toks, "{result:{success:%}}",
JSON_SCAN(json_to_bool, &success));
if (err) {
Expand Down Expand Up @@ -472,6 +495,8 @@ getrawblockbyheight_callback(const char *buf, const jsmntok_t *toks,
trace_span_resume(call);
trace_span_end(call);

check_bitcoin_error(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);
Expand Down Expand Up @@ -569,6 +594,8 @@ static void getchaininfo_callback(const char *buf, const jsmntok_t *toks,
u32 headers, blocks;
bool ibd;

check_bitcoin_error(call->bitcoind, buf, toks, "getchaininfo");

err = json_scan(tmpctx, buf, toks,
"{result:{chain:%,headercount:%,blockcount:%,ibd:%}}",
JSON_SCAN_TAL(tmpctx, json_strdup, &chain),
Expand Down Expand Up @@ -642,6 +669,8 @@ static void getutxout_callback(const char *buf, const jsmntok_t *toks,
/* Whatever happens, we want to free this. */
tal_steal(tmpctx, call);

check_bitcoin_error(call->bitcoind, buf, toks, "getutxout");

err = json_scan(tmpctx, buf, toks, "{result:{script:null}}");
if (!err) {
call->cb(call->bitcoind, NULL, call->cb_arg);
Expand Down
Loading
Loading