test: allow all functional tests to be run or skipped with --usecli

mzumsande commented at 8:50 pm on April 16, 2025: contributor

I looked into all current failures listed in the issue, as well all tests that are already disabled for the cli with self.supports_cli = False. There are several reasons why existing tests fail with --usecli on many systems, the most important ones are:

Most common reason is that the test executes a RPC call with a large arg that exceeds MAX_ARG_STRLEN of the OS, which is usually 128kb on linux: This is fixed by using -stdin for these large calls (idea by 0xB10C)
they test specifically the rpc interface - nothing to do there except disabling.
Some functional test submit wrong types to params on purpose to test the error message (which is different when using the cli) - deactivated these specific subtests locally for the cli when there is just one or two of them, deactivated the entire tests when there are more spots
When python sets None for an arg, the cli converts this to ’null’ in arg_to_cli. This is fine e.g. for boolean args, but doesn’t work for strings where it’s interpreted as the string ’null’. Bypass this for named args by not including args in case the value is None for the cli is used (it’s effectively the same as leaving the optional arg out).
the height_or_hash param used in some RPC needs to be converted to a JSON (effectively adding full quotes).
Some tests were marked with self.supports_cli = False in the past but run fine on master today - enabled those.

In total, this PR fixes all tests that fail on master and reduces the number of tests that are deactivated (self.supports_cli = False) from 40 to 21. It also adds --usecli to one CI job (multiprocess, i686, DEBUG) to detect regressions.

DrahtBot commented at 8:50 pm on April 16, 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/32290.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	pinheadmz

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Apr 16, 2025

mzumsande marked this as a draft on Apr 16, 2025

mzumsande force-pushed on Apr 16, 2025

DrahtBot added the label CI failed on Apr 16, 2025

DrahtBot commented at 11:01 pm on April 16, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40687372455

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

mzumsande force-pushed on Apr 16, 2025

DrahtBot removed the label CI failed on Apr 17, 2025

in test/functional/test_framework/util.py:607 in 1c2ea56030 outdated

602@@ -603,3 +603,8 @@ def sync_txindex(test_framework, node):
603     sync_start = int(time.time())
604     test_framework.wait_until(lambda: node.getindexinfo("txindex")["txindex"]["synced"])
605     test_framework.log.debug(f"Synced in {time.time() - sync_start} seconds")
606+
607+def add_quotes_for_cli(cli, text):

maflcko commented at 8:12 am on April 17, 2025:

q in 1c2ea560300f57e6b5ed75dee2597ca55dd76c6f: Would it not be easier if this was auto-detected? That is, if the type is string, would it not be better to internally (before dispatching to the cli) add quotes to all strings?

This probably means that numbers must be passed as int, or Decimal on the python side, which seems fine.

mzumsande commented at 5:33 pm on April 17, 2025:

Hmm, this is only an issue because hash_or_height expects a JSON object, not a string (and a text without quotes is no valid JSON object), so regular string args don’t need this. I think the current naming is confusing, maybe renaming the function “convert_to_json_for_cli” and calling json.dumps(text) inside instead of adding quotes (in case the cli is used) would be better to convey why this is done? Or is there a better solution?

mzumsande commented at 9:53 pm on April 30, 2025:

renamed to convert_to_json_for_cli

fanquake commented at 9:02 am on April 17, 2025: member

If this ends up fixing all issues, should add --usecli to atleast one CI, so we catch regressions?

mzumsande commented at 9:57 pm on April 17, 2025: contributor

If this ends up fixing all issues, should add –usecli to atleast one CI, so we catch regressions?

Yes - it’d be a bit of a waste of a CI job if we end up disabling too many of the tests with self.supports_cli = False but depending on how many can be fixed (I’m currently still going through all the existing disabled tests) that would make sense to me.

mzumsande force-pushed on Apr 18, 2025

mzumsande marked this as ready for review on Apr 21, 2025

mzumsande force-pushed on Apr 24, 2025

DrahtBot added the label Needs rebase on Apr 25, 2025

mzumsande force-pushed on Apr 25, 2025

DrahtBot removed the label Needs rebase on Apr 25, 2025

mzumsande force-pushed on Apr 30, 2025

mzumsande commented at 9:55 pm on April 30, 2025: contributor

If this ends up fixing all issues, should add --usecli to atleast one CI, so we catch regressions?

I have added --usecli to one CI job now (multiprocess, i686, DEBUG)

mzumsande force-pushed on May 2, 2025

mzumsande commented at 7:42 pm on May 2, 2025: contributor

Also added a commmit to fix the too-large args (https://github.com/bitcoin/bitcoin/pull/32399#issuecomment-2846837427).

in test/functional/test_framework/util.py:626 in 959dc33283 outdated

609@@ -610,3 +610,8 @@ def sync_txindex(test_framework, node):
610     sync_start = int(time.time())
611     test_framework.wait_until(lambda: node.getindexinfo("txindex")["txindex"]["synced"])
612     test_framework.log.debug(f"Synced in {time.time() - sync_start} seconds")
613+
614+def convert_to_json_for_cli(cli, text):
615+    if cli:
616+        return json.dumps(text)
617+    return text

pinheadmz commented at 5:16 pm on May 13, 2025:

959dc33283be3c465476f604263f7a2de4007da0

Why not add this method to BitcoinTestFramework then you won’t need to pass self.options.usecli every call?

pinheadmz commented at 5:23 pm on May 13, 2025:

Is this only ever needed for passing a hash as a hash-or-height argument?

mzumsande commented at 6:29 pm on May 20, 2025:

This is only correct for hash_or_height, which because of its dual use is defined as a numeric, non-string parameter and is therefore converted from json. It would be incorrect for actual string args: You can see it directly using bitcoin-cli in the console: You can do bitcoin-cli -regtest getblockstats '"3520a1e79bd3df078c762693b5c113469e267176aefca0d3c1dac29b8db6dd33"' but bitcoin-cli -regtest getblockstats "3520a1e79bd3df078c762693b5c113469e267176aefca0d3c1dac29b8db6dd33" fails with Error parsing JSON.

For actual string args, it is the other way round: bitcoin-cli -regtest getblock "3520a1e79bd3df078c762693b5c113469e267176aefca0d3c1dac29b8db6dd33" succeeds while bitcoin-cli -regtest getblock '"3520a1e79bd3df078c762693b5c113469e267176aefca0d3c1dac29b8db6dd33"' fails.

Since BitcoinTestFramework doesn’t know that this is a special hash_or_height arg, I’ve done the conversion not there.

pinheadmz approved

pinheadmz commented at 5:40 pm on May 13, 2025: member

ACK 7c13240d7b9a03b77ad84460e80007063367e05a

Reviewed all commits and tested on macos/arm64. Compared test_runner --usecli on master to branch. Master failed 17 tests and skipped 58. This branch skipped 36 and failed none. Some of the skipped tests are due to my platform.

This PR touches test code only but also by adding CI coverage requires all future changes to the API to be bitcoin-cli compatible (or otherwise pass new tests with cli)

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 7c13240d7b9a03b77ad84460e80007063367e05a
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgjgv4ACgkQ5+KYS2KJ
 7yTrLzxAAlAVG0ALW7dHqa0NXSst4Rf2xMTAViSgPeIXVma69PHCGHk0y4J+ee9D1
 8fQkS2NNLAh57jYxXzHXLVyb4ic3m4mIgIkFwQvnZJ1tj5U7wwju+12Qy24CzOEHc
 9m0kXE8IbKui5TTbgVR1I+5+uIEE01Nd6/pyJnRMvJpcHYP3uQXVlPdq2oDPK4a1d
10JjsA8yfQfLh37Rm31wfX2Q0tDtxwXMmnpsDTHg/B+cwCAES4DWiFmpXrp5G9bW22
11IyLHIIAIDkhUP5d9wEx+ydahXPp6T9pbK84+2oNVk2EK+iQrdeM4iP88TCD8p4FP
12+wJJIOElXf6njtKIa8hAuq8e8uYmPsT7VwcK8GGrII2eYlInzLyFE6oPjVwdOlbc
13IaeurBZIlPho0Gb+iFOgM62p6ctj3eul+nDbS+eKkQ/FvaYpM+jQVjadvfVzNzp3
14Gyvx/JwyVJYp8uuwifWPMyFBMsvUqhZ0CFZrKUUT5Vt7apYkjWXxZUyWaBoDdYDH
1557o2VHwygtaJ1YygUGM+NH5jX4IZd2EPdCc7wWgVhN/hJRmc61mNQkESv1XDmlE4
1646FMvc378djlOZwyXouTJSaVG3iY0tXS51pNulPUIsIOTLUViDCNa3gkApAqc6zO
17lyokClFmwkfDDrSS/Zqfv8m+XE4Pu4eS0KE1IQZQ6LqpFGg9WIk=
18=nd5k
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on openpgp.org

DrahtBot added the label Needs rebase on May 17, 2025

mzumsande force-pushed on May 20, 2025

DrahtBot removed the label Needs rebase on May 20, 2025

mzumsande commented at 6:42 pm on May 20, 2025: contributor

7c13240 to 2bf3f90: rebased - as a side effect, the legacy wallet removal made wallet_labels.py --usecli work on master, so no longer mentioned here.

pinheadmz approved

pinheadmz commented at 1:59 pm on May 21, 2025: member

ACK 2bf3f907ff54efd4fa22930a81269d568e29274e

Trivial changes since last ack: rebase on master, adjust for removed legacy wallet

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 2bf3f907ff54efd4fa22930a81269d568e29274e
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgt3HwACgkQ5+KYS2KJ
 7yTpQkg/+MxpEt7N0HhnGVSIVABeCyUJjGsKAih1pbNjUlm/aqGdNMj8NWVu4xPeA
 8IpbzeT9NIMYCdprRcAOlotpGUl5vRpjnWGL5AACqHfP9i5qutjSFgdusYvu5inam
 9r4TEF2fxI26hjjrVL2VuOyHxMUEUCgI2GLtwVbl7hKK01efKqAw4cCf0KjxYNf/Y
10Ja6J8E3QbjeXgRjAY1nkln/ZW+UUGmdArtB8ysZqRO34ch8MznJExJsL13gMzQoZ
11QHloensLimqS8XDyVL/FmNGN/GKdQ/sHkUH8BPUUqSdAvRonhb5jA7yNdgzZRArW
122D6Qs0ba2ou5tSUDwC4HyqtaDVumKSGW4wx9P4aDUzY8baXiy7ypoYRzGmB9yT4o
138KW60Ptx2vnjAtMerqWSlR7decyMLQR0CYqh9e3wssUfNrXhhZbh8C3oG0LxhWKZ
14nHyora+lTRPLGXvp+q/i4VuvATt74Z/aHdihBhT6P+VOxRhvRZwNuhfjrkrklEs5
15BupeG1R3m3sG/ekpdY/Tk4zPRpbFlKksZEiFgvWS0hOHlDq0EQTGWXO83IVwO3sj
16RYEOKHevoeKeon7aLJd0hqtOq3Q0sWgHKUm1SmvMSK++HSM54IBfIYc6M1Qn6Fx5
17q8XEMSgzixDVF1yhxeD8JGzExVYpX9BJp85NHZ1gVbc0IcwOpwU=
18=y2Tc
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on openpgp.org

DrahtBot added the label Needs rebase on Jun 7, 2025

test: Enable various tests for usage with cli

These tests run successfully on current master without any changes.

3de66c6c3b

test: use -stdin for large rpc commands

Because of the MAX_ARG_STRLEN limit (128kb on most systems)
for args, these would usually fail. As a workaround, use
-stdin for these large calls. Idea by 0xB10C.

d8213a25ac

test: enable functional tests with large rpc args for cli

Also, the following tests (for which self.supports_cli = False was not
set) will now work with --usecli:

feature_fastprune.py
feature_fee_estimation.py
feature_reindex_readonly.py
feature_taproot.py
mempool_package_rbf.py
p2p_net_deadlock.py
p2p_tx_download.py
rpc_packages.py

969d4c9d0f

test: remove unnecessary .rpc

The tests will run with --usecli without it

33a5c8dd6f

test: make rpc_psbt.py usable with --usecli

The psbt string would include a "=" sign, which would
make the cli interpret this as a named argument.
Fix this by making it an actual named arg with the
correct name.

1bd23fdb59

test: convert tuple to json for cli

This makes it possible to run rpc_createmultisig.py with --usecli.

7c4a09a166

test: Don't send empty named args with cli

If python passed None for an optional (i.e. 'null' is
sent), this will lead to the arg being interpreted as not
provided by bitcoind - except for string args, for which the arg is
interpreted as as 'null' string. Bypass this by not sending
named args to bitcoin-cli - so that the default value will
actually be used.

6306451076

test: add function to convert to json for height_or_hash params

This is necessary for bitcoin-cli because a string without quotes
is not a valid json.

d6401bcb76

test: skip subtests that check for wrong types with cli

The error messages returned for wrong type differ betwen
using rpc or cli - skip these subtests where they happen.

4d331ce0c9

test: Disable several (sub)tests with cli

Reason for each test:
rpc_whitelist.py: Relies on direct RPC calls
wallet_encryption.py: Null characters cannot be passed to suprocess.Popen
wallet_fundrawtransaction.py: multiple checks for wrong types, which have different error messages with cli
wallet_send.py: multiple checks for wrong types

92d2843a73

ci: use --usecli in one of the CI jobs

This helps avoid regressions (changes to functional tests
that are not compatible with -usecli).

1f3a5cc918

mzumsande force-pushed on Jun 10, 2025

DrahtBot removed the label Needs rebase on Jun 10, 2025

DrahtBot added the label CI failed on Jun 10, 2025

mzumsande marked this as a draft on Jun 12, 2025

mzumsande commented at 6:24 pm on June 12, 2025: contributor

Converting to draft until I understand the CI failure (which I couldn’t reproduce locally so far).

fanquake commented at 9:23 am on June 13, 2025: member

the CI failure

The bitcoin call seems to be missing the binary argument:

0TestFramework (ERROR): Called Process failed with 'Error: Unknown option: -nonamed
1...
2subprocess.CalledProcessError: Command '['bitcoin', '-m', '-nonamed', '-datadir=/ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20250610_161648/feature_fee_estimation_266/node0', '-named', '-stdin']' returned non-zero exit status 1.

0./build/bin/bitcoin -m -nonamed        
1Error: Unknown option: -nonamed
2Try './build/bin/bitcoin --help' for more information.

cc @ryanofsky

maflcko commented at 9:32 am on June 13, 2025: member

The bitcoin call seems to be missing the binary argument:

Yes, but I fail to see why. It clearly picked the bitcoin_cmd, but then forgot to append [command], which is ["rpc"]:

https://github.com/bitcoin/bitcoin/blob/1f3a5cc918361edfb8e00b0856651a64abe667ed/test/functional/test_framework/test_framework.py#L108

ryanofsky commented at 12:32 pm on June 13, 2025: contributor

Converting to draft until I understand the CI failure (which I couldn’t reproduce locally so far).

Not sure about the error yet but I could reproduce locally by running BITCOIN_CMD=bitcoin build/test/functional/feature_fee_estimation.py --usecli

ryanofsky commented at 12:49 pm on June 13, 2025: contributor

Seems to fix the problem:

 0--- a/test/functional/test_framework/test_node.py
 1+++ b/test/functional/test_framework/test_node.py
 2@@ -922,6 +922,7 @@ class TestNodeCLI():
 3         p_args = self.binaries.rpc_argv() + [f"-datadir={self.datadir}"] + self.options
 4         if named_args:
 5             p_args += ["-named"]
 6+        base_args = len(p_args)
 7         if clicommand is not None:
 8             p_args += [clicommand]
 9         p_args += pos_args + named_args
10@@ -929,13 +930,12 @@ class TestNodeCLI():
11         stdin_data = self.input
12         if max_arg_size > CLI_MAX_ARG_SIZE:
13             self.log.debug(f"Cli: Command size {max_arg_size} too large, using stdin")
14-            rpc_args = "\n".join([arg for arg in p_args[1:] if not arg.startswith('-')])
15+            rpc_args = "\n".join([arg for arg in p_args[base_args:]])
16             if stdin_data is not None:
17                 stdin_data += "\n" + rpc_args
18             else:
19                 stdin_data = rpc_args
20-            base_args = [arg for arg in p_args if arg.startswith('-')]
21-            p_args =[p_args[0]] + base_args + ['-stdin']
22+            p_args = p_args[:base_args] + ['-stdin']
23 
24         self.log.debug("Running bitcoin-cli {}".format(p_args[2:]))
25         process = subprocess.Popen(p_args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)

mzumsande commented at 1:31 pm on June 13, 2025: contributor

Thanks - will try this fix soon. This has nothing to do with multiprocess by the way - it’s my bug, I just happened to pick that CI job to enable the -usecli run.

test: allow all functional tests to be run or skipped with –usecli #32290

Code Coverage & Benchmarks

Reviews

Conflicts