test: Fix CLI_MAX_ARG

maflcko commented at 9:39 am on August 23, 2025: member

CLI_MAX_ARG_SIZE has many edge case issues:

It seems to be lower on some systems, but it is unknown how to reproduce locally: #33079 (comment)
MAX_ARG_STRLEN is a limit per arg, but we probably want “The maximum length of [all of] the arguments”: See https://www.man7.org/linux/man-pages/man3/sysconf.3.html, section ARG_MAX - _SC_ARG_MAX.
It doesn’t account for the additional args added by the bitcoin command later on: https://github.com/bitcoin/bitcoin/blob/73220fc0f958f9b65f66cf0cf042af220b312fc6/src/bitcoin.cpp#L85-L92
It doesn’t account for unicode encoding a string to bytes before taking its length.

The issues are mostly harmless edge cases, but it would be good to fix them. So do that here, by:

Replacing max() by sum(), to correctly take into account all args, not just the largest one.
Reduce CLI_MAX_ARG_SIZE, to account for the bitcoin command additional args.

Also, there is a test. The test can be called with ulimit to hopefully limit the max args size to the hard-coded value in the test framework. For reference:

0$ ( ulimit -s 512 && python3 -c 'import os; print(os.sysconf("SC_ARG_MAX") )' ) 
1131072

On top of this pull it should pass, …

0bash -c 'ulimit -s 512 && BITCOIN_CMD="bitcoin -M" ./bld-cmake/test/functional/rpc_misc.py --usecli -l DEBUG'

… and with the test_framework changes reverted, it should fail:

0OSError: [Errno 7] Argument list too long: 'bitcoin'

Also, there is a commit to enable CI_LIMIT_STACK_SIZE=1 in the i686 task, because it should now be possible and no longer hit the hard-to-reproduce issue mentioned above.

DrahtBot commented at 9:39 am on August 23, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33243.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	cedwies, enirox001, mzumsande, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

maflcko renamed this:
~~2508 test fix cli cmd size~~
test: Fix CLI_MAX_ARG_SIZE issues
on Aug 23, 2025

DrahtBot renamed this:
~~test: Fix CLI_MAX_ARG_SIZE issues~~
test: Fix CLI_MAX_ARG_SIZE issues
on Aug 23, 2025

DrahtBot added the label Tests on Aug 23, 2025

in test/functional/rpc_misc.py:33 in fabe21499c outdated

29@@ -30,6 +30,11 @@ def run_test(self):
30             lambda: node.echo(arg9='trigger_internal_bug'),
31         )
32 
33+        self.log.info("test max arg size")

cedwies commented at 3:57 pm on August 25, 2025:

This test also passes on master (MacOS environment with ARG_MAX = 1048576). It does not fail with the test_framework changes reverted. However, the following test only passes on this PR, not on my master:

0self.log.info("test many small args (sum-size trigger)")
1tiny = "a" * 120000
2args = [tiny] * 10
3assert_equal(args, node.echo(*args))

maflcko commented at 11:01 am on August 26, 2025:

This test also passes on master (MacOS environment with ARG_MAX = 1048576).

Thanks for adding a macos data point for reference. This further supports that the hard-coded value may be different on different platforms.

However, the following test only passes on this PR, not on my master:

Nice, thanks for adapting the test and checking the bugfix on macos.

in test/functional/test_framework/test_node.py:940 in fa18c82500 outdated

931@@ -928,10 +932,10 @@ def send_cli(self, clicommand=None, *args, **kwargs):
932         if clicommand is not None:
933             p_args += [clicommand]
934         p_args += pos_args + named_args
935-        max_arg_size = max(len(arg) for arg in p_args)
936+        sum_arg_size = sum(len(arg) for arg in p_args)

enirox001 commented at 4:15 pm on August 25, 2025:

02025-08-25T15:54:00.131311Z TestFramework.bitcoincli (DEBUG): Cli: Command size 131140 too large, using stdin
12025-08-25T15:54:00.148742Z TestFramework.bitcoincli (DEBUG): Cli: Command size 262212 too large, using stdin
22025-08-25T15:54:00.179312Z TestFramework.bitcoincli (DEBUG): Cli: Command size 4194372 too large, using stdin
32025-08-25T15:54:00.418554Z TestFramework.bitcoincli (DEBUG): Cli: Command size 8388676 too large, using stdin

fa18c82 — The debug logs show byte-based sizes (131140 for 65K chars). Should we use len(a.encode('utf-8')) to ensure consistency between the check and the actual command size measurement?

maflcko commented at 11:01 am on August 26, 2025:

Yeah, encoding seems like another possible source for inconsistencies. I don’t think we want to deal with encoding here, because the value is small enough for it to not matter.

enirox001 commented at 4:52 pm on August 26, 2025:

That’s reasonable. Since the test uses ASCII ‘a’ and the sizes are large, encoding won’t change the outcome in practice.

in test/functional/test_framework/test_node.py:53 in fa18c82500 outdated

45@@ -46,7 +46,11 @@
46 # The size of the blocks xor key
47 # from InitBlocksdirXorKey::xor_key.size()
48 NUM_XOR_BYTES = 8
49-CLI_MAX_ARG_SIZE = 131071 # many systems have a 128kb limit per arg (MAX_ARG_STRLEN)
50+# Many systems have a 128kB limit for a command size. However, when using the
51+# 'bitcoin' command, it may internally insert more args, which must be
52+# accounted for. There is no need to pick the largest possible value here
53+# anyway and it should be fine to set it to 1kB.
54+CLI_MAX_ARG_SIZE = 1024

enirox001 commented at 4:17 pm on August 25, 2025:

fa18c82 — Does this test use the system ARG_MAX at runtime or intentionally use a conservative value? Could we add a short comment that 1024 is a test-only, conservative value to ensure the stdin fallback is exercised, and that production behavior still relies on the system ARG_MAX

maflcko commented at 11:01 am on August 26, 2025:

thx, reworded the comment a bit

cedwies commented at 4:18 pm on August 25, 2025: none

ACK fabe214

This PR fixes how the functional test framework estimates command-line argument size. Previously it checked only the longest argument, but the OS limit is on the total argv/env size, so it now sums all args. It also adds a functional test using very large echo args, which exercises the -stdin fallback. The test passes for me.

I appreciate lowering CLI_MAX_ARG_SIZE to 1024. It’s a very safe, conservative threshold that avoids platform-dependent ARG_MAX issues, and since larger payloads automatically go through -stdin anyway, I see no real downside. A nice fail-safe change IMO.

in test/functional/rpc_misc.py:34 in fa18c82500 outdated

29@@ -30,6 +30,11 @@ def run_test(self):
30             lambda: node.echo(arg9='trigger_internal_bug'),
31         )
32 
33+        self.log.info("test max arg size")
34+        for arg_sz in [0, 1, 100, 2**16 - 1, 2**17 - 1, 2**21 - 1, 2**22 - 1]:

enirox001 commented at 4:21 pm on August 25, 2025:

fa18c82 — Perhaps add a short note that these sizes target historical/modern ARG_MAX boundaries (64KiB, ~128KiB, ~2MiB, ~4MiB) and that -1 tests just-below the power-of-two to catch off-by-ones? Took me a while to understand the intent.

maflcko commented at 11:01 am on August 26, 2025:

It is mostly a regression test, checking that on master a single arg with size of CLI_MAX_ARG_SIZE = 131071 fails the test (with the reproducer shared in the pull description).

Force pushed to reduce to that (and added a comment).

enirox001 commented at 4:39 pm on August 25, 2025: contributor

Concept ACK fa18c82 — The fix is a welcome improvement: summing arg sizes better reflects kernel ARG_MAX behavior. Good use of >= to match inclusive boundary semantics. Left a few non-blocking clarifier nits inline.

maflcko force-pushed on Aug 26, 2025

DrahtBot added the label CI failed on Aug 26, 2025

test: Fix CLI_MAX_ARG_SIZE issues facfde2cdc

ci: Enable CI_LIMIT_STACK_SIZE=1 in i686_no_ipc task fa96a4afea

maflcko force-pushed on Aug 26, 2025

DrahtBot removed the label CI failed on Aug 26, 2025

cedwies commented at 1:09 pm on August 26, 2025: none

ACK fa96a4a

DrahtBot requested review from enirox001 on Aug 26, 2025

enirox001 commented at 4:58 pm on August 26, 2025: contributor

ACK fa96a4a — thanks for addressing the nits and clarifying the test; LGTM.

mzumsande commented at 8:53 pm on August 26, 2025: contributor

It claims to be a “limit per arg”, but it is a “The maximum length of the arguments”: See https://www.man7.org/linux/man-pages/man3/sysconf.3.html, section ARG_MAX - _SC_ARG_MAX.

the relevant bottleneck for me (tested on Ubuntu and Debian) was not ARG_MAX, but MAX_ARG_STRLEN, which accounts for the maximum per arg, not the sum. e.g. the folllowing passes

0arg1=$(printf "%0*d" $((131072 - 1)) 0)
1arg2=$(printf "%0*d" $((131072 - 1)) 0)
2$(which printf) "%s %s" "$arg1" "$arg2" >/dev/null

whereas

0arg3=$(printf "%0*d" $((131072)) 0)
1$(which printf) "%s" "$arg3" >/dev/null

fails with bash: /usr/bin/printf: Argument list too long

But seeing how much this apparently differs between systems, Concept ACK to using a smaller limit.

maflcko commented at 5:48 am on August 27, 2025: member

the relevant bottleneck for me (tested on Ubuntu and Debian) was not ARG_MAX, but MAX_ARG_STRLEN, which accounts for the maximum per arg, not the sum.

Thanks for clarifying that this was intentionally picked and that MAX_ARG_STRLEN is a limit per arg, separate from the command size limit SC_ARG_MAX. SC_ARG_MAX is larger on many modern systems, but not all. Also, it reduces to MAX_ARG_STRLEN, or even lower on some systems. I’ve edited the pull description, to better reflect this.

maflcko commented at 10:12 am on September 4, 2025: member

(+GHA ci)

maflcko closed this on Sep 4, 2025

maflcko reopened this on Sep 4, 2025

mzumsande commented at 3:39 pm on September 10, 2025: contributor

Code Review ACK fa96a4afea2a9bf90c843198e75a00acef02c32d

achow101 commented at 10:32 pm on September 11, 2025: member

ACK fa96a4afea2a9bf90c843198e75a00acef02c32d

achow101 merged this on Sep 11, 2025

achow101 closed this on Sep 11, 2025

fanquake referenced this in commit c7faf72ac6 on Sep 15, 2025

fanquake referenced this in commit 5dbb1bae38 on Sep 15, 2025

fanquake commented at 9:25 am on September 15, 2025: member

Backported to 30.x in #33356.

alexanderwiederin referenced this in commit 49e068b15b on Sep 16, 2025

alexanderwiederin referenced this in commit 4b0c2f2a8f on Sep 17, 2025

alexanderwiederin referenced this in commit 2edb618ffe on Sep 17, 2025

glozow referenced this in commit b7a722724d on Sep 17, 2025

stringintech referenced this in commit fb8510ba20 on Sep 17, 2025

maflcko deleted the branch on Sep 26, 2025

test: Fix CLI_MAX_ARG_SIZE issues #33243

Code Coverage & Benchmarks

Reviews