From 8f0a51868e1ec94617f65c09def499b858d7019b Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 28 Oct 2025 10:55:32 +1030 Subject: [PATCH 1/3] common: remove tal_check() call on libwally allocations. We call it once at the end, but calling on each allocation is excessive, and it shows when dealing with large PSBTS. Testing a 700-input PSBT was unusably slow without this: after this the entire test ran in 9 seconds. Changelog-Fixed: JSON-RPC: Dealing with giant PSBTs (700 inputs!) is now much faster. Signed-off-by: Rusty Russell --- common/setup.c | 1 - 1 file changed, 1 deletion(-) diff --git a/common/setup.c b/common/setup.c index 56ff6828473a..8db367eec9d6 100644 --- a/common/setup.c +++ b/common/setup.c @@ -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"); } From 54a187ce97f78fea4c018944fd8af990e0396761 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 28 Oct 2025 10:55:54 +1030 Subject: [PATCH 2/3] pytest: test for bcli crash with huge PSBTs. Signed-off-by: Rusty Russell --- tests/test_plugin.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index bc3e6c9178f5..4cded40f4027 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1875,6 +1875,22 @@ def test_bitcoin_backend(node_factory, bitcoind): " bitcoind") +@pytest.mark.xfail(strict=True) +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"] + 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. From 1c84919049797573c9c503548a843e11bb8e5b34 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 31 Oct 2025 14:21:59 +1030 Subject: [PATCH 3/3] plugins/bcli: use -stdin to feed arguments, in case we have a giant tx. ``` lightningd-1 2025-10-27T11:26:04.285Z **BROKEN** plugin-bcli: bitcoin-cli exec failed: Argument list too long ``` Use -stdin to bitcoin-cli: we can then handle arguments of arbitrary length. Fixes: https://github.com/ElementsProject/lightning/issues/8634 Changelog-Fixed: plugins: `bcli` would fail with "Argument list too long" when sending a giant tx. --- plugins/bcli.c | 47 +++++++++++++++++++++++++++++++------------- tests/test_misc.py | 2 +- tests/test_plugin.py | 1 - 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/plugins/bcli.c b/plugins/bcli.c index 5203022f18ea..7646b34801ab 100644 --- a/plugins/bcli.c +++ b/plugins/bcli.c @@ -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; @@ -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; @@ -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) + 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; @@ -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]); @@ -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. */ @@ -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(); @@ -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); @@ -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`. */ @@ -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`. */ @@ -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)); diff --git a/tests/test_misc.py b/tests/test_misc.py index b97ba10847c0..97e6ea73cacd 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -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}) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 4cded40f4027..3dc95f2ff60d 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1875,7 +1875,6 @@ def test_bitcoin_backend(node_factory, bitcoind): " bitcoind") -@pytest.mark.xfail(strict=True) def test_bitcoin_backend_gianttx(node_factory, bitcoind): """Test that a giant tx doesn't crash bcli""" l1 = node_factory.get_node(start=False)