Skip to content

Giant tx fix #8639

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
wants to merge 3 commits into
base: master
Choose a base branch
from
+49 −16
Open

Giant tx fix #8639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f0a518
common: remove tal_check() call on libwally allocations.
rustyrussell Oct 28, 2025
54a187c
pytest: test for bcli crash with huge PSBTs.
rustyrussell Oct 28, 2025
1c84919
plugins/bcli: use -stdin to feed arguments, in case we have a giant tx.
rustyrussell Oct 31, 2025
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
1 change: 0 additions & 1 deletion common/setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
static void *cln_wally_tal(size_t size)
{
assert(wally_tal_ctx);
assert(tal_check(wally_tal_ctx, "cln_wally_tal ctx check"));
return tal_arr_label(wally_tal_ctx, u8, size, "cln_wally_tal");
}

Expand Down
47 changes: 33 additions & 14 deletions plugins/bcli.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ struct bitcoin_cli {
int *exitstatus;
pid_t pid;
const char **args;
const char **stdinargs;
struct timeabs start;
enum bitcoind_prio prio;
char *output;
Expand All @@ -95,7 +96,8 @@ static void add_arg(const char ***args, const char *arg TAKES)
tal_arr_expand(args, arg);
}

static const char **gather_argsv(const tal_t *ctx, const char *cmd, va_list ap)
/* If stdinargs is non-NULL, that is where we put additional args */
static const char **gather_argsv(const tal_t *ctx, const char ***stdinargs, const char *cmd, va_list ap)
{
const char **args = tal_arr(ctx, const char *, 1);
const char *arg;
Expand Down Expand Up @@ -128,23 +130,30 @@ static const char **gather_argsv(const tal_t *ctx, const char *cmd, va_list ap)
// `-rpcpassword` argument - secrets in arguments can leak when listing
// system processes.
add_arg(&args, "-stdinrpcpass");
/* To avoid giant command lines, we use -stdin (avail since bitcoin 0.13) */
if (stdinargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this check enough if stdinargs is allocated but empty?

add_arg(&args, "-stdin");

add_arg(&args, cmd);
while ((arg = va_arg(ap, char *)) != NULL)
add_arg(&args, arg);
while ((arg = va_arg(ap, char *)) != NULL) {
if (stdinargs)
add_arg(stdinargs, arg);
else
add_arg(&args, arg);
}
add_arg(&args, NULL);

return args;
}

static LAST_ARG_NULL const char **
gather_args(const tal_t *ctx, const char *cmd, ...)
gather_args(const tal_t *ctx, const char ***stdinargs, const char *cmd, ...)
{
va_list ap;
const char **ret;

va_start(ap, cmd);
ret = gather_argsv(ctx, cmd, ap);
ret = gather_argsv(ctx, stdinargs, cmd, ap);
va_end(ap);

return ret;
Expand All @@ -170,7 +179,7 @@ static struct io_plan *output_init(struct io_conn *conn, struct bitcoin_cli *bcl
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)
static char *args_string(const tal_t *ctx, const char **args, const char **stdinargs)
{
size_t i;
char *ret = tal_strdup(ctx, args[0]);
Expand All @@ -185,12 +194,16 @@ static char *args_string(const tal_t *ctx, const char **args)
ret = tal_strcat(ctx, take(ret), args[i]);
}
}
for (i = 0; i < tal_count(stdinargs); i++) {
ret = tal_strcat(ctx, take(ret), " ");
ret = tal_strcat(ctx, take(ret), stdinargs[i]);
}
return ret;
}

static char *bcli_args(const tal_t *ctx, struct bitcoin_cli *bcli)
{
return args_string(ctx, bcli->args);
return args_string(ctx, bcli->args, bcli->stdinargs);
}

/* Only set as destructor once bcli is in current. */
Expand Down Expand Up @@ -313,9 +326,14 @@ static void next_bcli(enum bitcoind_prio prio)
bcli->args[0], strerror(errno));


if (bitcoind->rpcpass)
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_now();
Expand Down Expand Up @@ -351,7 +369,8 @@ start_bitcoin_cliv(const tal_t *ctx,
else
bcli->exitstatus = NULL;

bcli->args = gather_argsv(bcli, method, ap);
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);
Expand Down Expand Up @@ -994,14 +1013,14 @@ static struct command_result *getutxout(struct command *cmd,

static void bitcoind_failure(struct plugin *p, const char *error_message)
{
const char **cmd = gather_args(bitcoind, "echo", NULL);
const char **cmd = gather_args(bitcoind, NULL, "echo", NULL);
plugin_err(p, "\n%s\n\n"
"Make sure you have bitcoind running and that bitcoin-cli"
" is able to connect to bitcoind.\n\n"
"You can verify that your Bitcoin Core installation is"
" ready for use by running:\n\n"
" $ %s 'hello world'\n", error_message,
args_string(cmd, cmd));
args_string(cmd, cmd, NULL));
}

/* Do some sanity checks on bitcoind based on the output of `getnetworkinfo`. */
Expand All @@ -1016,7 +1035,7 @@ static void parse_getnetworkinfo_result(struct plugin *p, const char *buf)
if (!result)
plugin_err(p, "Invalid response to '%s': '%s'. Can not "
"continue without proceeding to sanity checks.",
args_string(tmpctx, gather_args(bitcoind, "getnetworkinfo", NULL)),
args_string(tmpctx, gather_args(bitcoind, NULL, "getnetworkinfo", NULL), NULL),
buf);

/* Check that we have a fully-featured `estimatesmartfee`. */
Expand Down Expand Up @@ -1047,7 +1066,7 @@ static void wait_and_check_bitcoind(struct plugin *p)
int in, from, status;
pid_t child;
const char **cmd = gather_args(
bitcoind, "-rpcwait", "-rpcwaittimeout=30", "getnetworkinfo", NULL);
bitcoind, NULL, "-rpcwait", "-rpcwaittimeout=30", "getnetworkinfo", NULL);
char *output = NULL;

child = pipecmdarr(&in, &from, &from, cast_const2(char **, cmd));
Expand Down
2 changes: 1 addition & 1 deletion tests/test_misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2186,7 +2186,7 @@ def test_bitcoind_fail_first(node_factory, bitcoind):
# first.
timeout = 5 if 5 < TIMEOUT // 3 else TIMEOUT // 3
l1 = node_factory.get_node(start=False,
broken_log=r'plugin-bcli: .*(-stdinrpcpass getblockhash 100 exited 1 \(after [0-9]* other errors\)|we have been retrying command for)',
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})

Expand Down
15 changes: 15 additions & 0 deletions tests/test_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1875,6 +1875,21 @@ def test_bitcoin_backend(node_factory, bitcoind):
" bitcoind")


def test_bitcoin_backend_gianttx(node_factory, bitcoind):
"""Test that a giant tx doesn't crash bcli"""
l1 = node_factory.get_node(start=False)
# With memleak we spend far too much time gathering backtraces.
del l1.daemon.env["LIGHTNINGD_DEV_MEMLEAK"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this line is causing your test to fail. It seems like the LIGHTNINGD_DEV_MEMLEAK key does not exist under valgrind? You can use .pop() instead of del, that way the key will be removed only if it exists.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no idea why the liquid test fails like this though:

FAILED tests/test_plugin.py::test_bitcoin_backend_gianttx - pyln.client.lightning.RpcError: RPC call failed: method: withdraw, payload: {'destination': 'el1qqwawntx90mlylxle6q3vwjcm9r5fc7te6nf58gq469ejxym6fuv5sl6fhxnuat264hf86vmh6h0qupv99flgsy8hhudn3t32f', 'satoshi': 'all'}, error: {'code': -1, 'message': 'Could not parse destination address, destination should be a valid address'}

It seems like l1 is a liquid node, and we're trying to withdraw to a correct liquid address?

l1.start()
addrs = {addr: 0.00200000 for addr in [l1.rpc.newaddr('bech32')['bech32'] for _ in range(700)]}
bitcoind.rpc.sendmany("", addrs)
bitcoind.generate_block(1, wait_for_mempool=1)
sync_blockheight(bitcoind, [l1])

l1.rpc.withdraw(bitcoind.rpc.getnewaddress(), 'all')
bitcoind.generate_block(1, wait_for_mempool=1)


def test_bitcoin_bad_estimatefee(node_factory, bitcoind):
"""
This tests that we don't crash if bitcoind backend gives bad estimatefees.
Expand Down
Loading