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

pull mzumsande wants to merge 11 commits into bitcoin:master from mzumsande:202505_fix_usecli changing 42 files +110 −98
  1. mzumsande commented at 8:50 PM on April 16, 2025: contributor

    Fixes #32264

    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.

  2. DrahtBot commented at 8:50 PM on April 16, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, pinheadmz

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. DrahtBot added the label Tests on Apr 16, 2025
  4. mzumsande marked this as a draft on Apr 16, 2025
  5. mzumsande force-pushed on Apr 16, 2025
  6. mzumsande force-pushed on Apr 16, 2025
  7. DrahtBot added the label CI failed on Apr 16, 2025
  8. DrahtBot commented at 11:01 PM on April 16, 2025: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <details><summary>Hints</summary>

    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.

    </details>

  9. mzumsande force-pushed on Apr 16, 2025
  10. DrahtBot removed the label CI failed on Apr 17, 2025
  11. 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

  12. 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?

  13. 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.

  14. mzumsande force-pushed on Apr 18, 2025
  15. mzumsande marked this as ready for review on Apr 21, 2025
  16. mzumsande force-pushed on Apr 24, 2025
  17. DrahtBot added the label Needs rebase on Apr 25, 2025
  18. mzumsande force-pushed on Apr 25, 2025
  19. DrahtBot removed the label Needs rebase on Apr 25, 2025
  20. mzumsande force-pushed on Apr 30, 2025
  21. 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)

  22. mzumsande force-pushed on May 2, 2025
  23. 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).

  24. 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.

  25. pinheadmz approved
  26. 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)

    <details><summary>Show Signature</summary>

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA256
    
    ACK 7c13240d7b9a03b77ad84460e80007063367e05a
    -----BEGIN PGP SIGNATURE-----
    
    iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgjgv4ACgkQ5+KYS2KJ
    yTrLzxAAlAVG0ALW7dHqa0NXSst4Rf2xMTAViSgPeIXVma69PHCGHk0y4J+ee9D1
    fQkS2NNLAh57jYxXzHXLVyb4ic3m4mIgIkFwQvnZJ1tj5U7wwju+12Qy24CzOEHc
    m0kXE8IbKui5TTbgVR1I+5+uIEE01Nd6/pyJnRMvJpcHYP3uQXVlPdq2oDPK4a1d
    JjsA8yfQfLh37Rm31wfX2Q0tDtxwXMmnpsDTHg/B+cwCAES4DWiFmpXrp5G9bW22
    IyLHIIAIDkhUP5d9wEx+ydahXPp6T9pbK84+2oNVk2EK+iQrdeM4iP88TCD8p4FP
    +wJJIOElXf6njtKIa8hAuq8e8uYmPsT7VwcK8GGrII2eYlInzLyFE6oPjVwdOlbc
    IaeurBZIlPho0Gb+iFOgM62p6ctj3eul+nDbS+eKkQ/FvaYpM+jQVjadvfVzNzp3
    Gyvx/JwyVJYp8uuwifWPMyFBMsvUqhZ0CFZrKUUT5Vt7apYkjWXxZUyWaBoDdYDH
    57o2VHwygtaJ1YygUGM+NH5jX4IZd2EPdCc7wWgVhN/hJRmc61mNQkESv1XDmlE4
    46FMvc378djlOZwyXouTJSaVG3iY0tXS51pNulPUIsIOTLUViDCNa3gkApAqc6zO
    lyokClFmwkfDDrSS/Zqfv8m+XE4Pu4eS0KE1IQZQ6LqpFGg9WIk=
    =nd5k
    -----END PGP SIGNATURE-----
    

    pinheadmz's public key is on openpgp.org

    </details>

  27. DrahtBot added the label Needs rebase on May 17, 2025
  28. mzumsande force-pushed on May 20, 2025
  29. DrahtBot removed the label Needs rebase on May 20, 2025
  30. 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.

  31. pinheadmz approved
  32. 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

    <details><summary>Show Signature</summary>

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA256
    
    ACK 2bf3f907ff54efd4fa22930a81269d568e29274e
    -----BEGIN PGP SIGNATURE-----
    
    iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgt3HwACgkQ5+KYS2KJ
    yTpQkg/+MxpEt7N0HhnGVSIVABeCyUJjGsKAih1pbNjUlm/aqGdNMj8NWVu4xPeA
    IpbzeT9NIMYCdprRcAOlotpGUl5vRpjnWGL5AACqHfP9i5qutjSFgdusYvu5inam
    r4TEF2fxI26hjjrVL2VuOyHxMUEUCgI2GLtwVbl7hKK01efKqAw4cCf0KjxYNf/Y
    Ja6J8E3QbjeXgRjAY1nkln/ZW+UUGmdArtB8ysZqRO34ch8MznJExJsL13gMzQoZ
    QHloensLimqS8XDyVL/FmNGN/GKdQ/sHkUH8BPUUqSdAvRonhb5jA7yNdgzZRArW
    2D6Qs0ba2ou5tSUDwC4HyqtaDVumKSGW4wx9P4aDUzY8baXiy7ypoYRzGmB9yT4o
    8KW60Ptx2vnjAtMerqWSlR7decyMLQR0CYqh9e3wssUfNrXhhZbh8C3oG0LxhWKZ
    nHyora+lTRPLGXvp+q/i4VuvATt74Z/aHdihBhT6P+VOxRhvRZwNuhfjrkrklEs5
    BupeG1R3m3sG/ekpdY/Tk4zPRpbFlKksZEiFgvWS0hOHlDq0EQTGWXO83IVwO3sj
    RYEOKHevoeKeon7aLJd0hqtOq3Q0sWgHKUm1SmvMSK++HSM54IBfIYc6M1Qn6Fx5
    q8XEMSgzixDVF1yhxeD8JGzExVYpX9BJp85NHZ1gVbc0IcwOpwU=
    =y2Tc
    -----END PGP SIGNATURE-----
    

    pinheadmz's public key is on openpgp.org

    </details>

  33. DrahtBot added the label Needs rebase on Jun 7, 2025
  34. mzumsande force-pushed on Jun 10, 2025
  35. DrahtBot removed the label Needs rebase on Jun 10, 2025
  36. DrahtBot added the label CI failed on Jun 10, 2025
  37. mzumsande marked this as a draft on Jun 12, 2025
  38. 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).

  39. fanquake commented at 9:23 AM on June 13, 2025: member

    the CI failure

    The bitcoin call seems to be missing the binary argument:

    TestFramework (ERROR): Called Process failed with 'Error: Unknown option: -nonamed
    ...
    subprocess.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.
    
    ./build/bin/bitcoin -m -nonamed        
    Error: Unknown option: -nonamed
    Try './build/bin/bitcoin --help' for more information.
    

    cc @ryanofsky

  40. 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

  41. 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

  42. ryanofsky commented at 12:49 PM on June 13, 2025: contributor

    Seems to fix the problem:

    --- a/test/functional/test_framework/test_node.py
    +++ b/test/functional/test_framework/test_node.py
    @@ -922,6 +922,7 @@ class TestNodeCLI():
             p_args = self.binaries.rpc_argv() + [f"-datadir={self.datadir}"] + self.options
             if named_args:
                 p_args += ["-named"]
    +        base_args = len(p_args)
             if clicommand is not None:
                 p_args += [clicommand]
             p_args += pos_args + named_args
    @@ -929,13 +930,12 @@ class TestNodeCLI():
             stdin_data = self.input
             if max_arg_size > CLI_MAX_ARG_SIZE:
                 self.log.debug(f"Cli: Command size {max_arg_size} too large, using stdin")
    -            rpc_args = "\n".join([arg for arg in p_args[1:] if not arg.startswith('-')])
    +            rpc_args = "\n".join([arg for arg in p_args[base_args:]])
                 if stdin_data is not None:
                     stdin_data += "\n" + rpc_args
                 else:
                     stdin_data = rpc_args
    -            base_args = [arg for arg in p_args if arg.startswith('-')]
    -            p_args =[p_args[0]] + base_args + ['-stdin']
    +            p_args = p_args[:base_args] + ['-stdin']
     
             self.log.debug("Running bitcoin-cli {}".format(p_args[2:]))
             process = subprocess.Popen(p_args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)
    
  43. 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.

  44. mzumsande force-pushed on Jun 18, 2025
  45. mzumsande commented at 6:29 PM on June 18, 2025: contributor

    I took @ryanofsky's fix and fixed another silent conflict - I had no idea what was going on (#31375 got merged and conflicted with my previous changes to send_cli), so I guess it's good that I randomly chose to enable --usecli in the multiprocess CI job.

  46. mzumsande marked this as ready for review on Jun 18, 2025
  47. mzumsande force-pushed on Jun 18, 2025
  48. DrahtBot removed the label CI failed on Jun 18, 2025
  49. in test/functional/rpc_deprecated.py:35 in 8e110f1a6c outdated
      31 | @@ -32,7 +32,7 @@ def run_test(self):
      32 |              self.log.info("Tests for deprecated wallet-related RPC methods (if any)")
      33 |              self.log.info("Test settxfee RPC deprecation")
      34 |              self.nodes[0].createwallet("settxfeerpc")
      35 | -            assert_raises_rpc_error(-32, 'settxfee is deprecated and will be fully removed in v31.0.', self.nodes[0].rpc.settxfee, 0.01)
      36 | +            assert_raises_rpc_error(-32, 'settxfee is deprecated and will be fully removed in v31.0.', self.nodes[0].settxfee, 0.01)
    


    maflcko commented at 10:24 AM on June 19, 2025:

    nit in 8e110f1a6c38c9df07a1df51fd43cc77b3e85325:

    Looks like this is an easy mistake/misunderstanding.

    It would be good to avoid it in the future by making the field "private". Python doesn't have "private", but it can be achieved by naming it _rpc or _rpc_internal. This makes it easier for reviewers (or review tools) to spot the mistake instead of having to rely on runtime checks via --usecli.


    maflcko commented at 12:40 PM on June 19, 2025:

    This will also unlock new findings:

    diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py
    index 864eed5a56..43001fb3f3 100755
    --- a/test/functional/wallet_backwards_compatibility.py
    +++ b/test/functional/wallet_backwards_compatibility.py
    @@ -327,7 +327,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
             for node in descriptors_nodes:
                 self.log.info(f"- {node.version}")
                 wallet_name = f"up_{node.version}"
    -            node.rpc.createwallet(wallet_name=wallet_name, descriptors=True)
    +            node.createwallet(wallet_name=wallet_name, descriptors=True)
                 wallet_prev = node.get_wallet_rpc(wallet_name)
                 address = wallet_prev.getnewaddress('', "bech32")
                 addr_info = wallet_prev.getaddressinfo(address)
    @@ -395,9 +395,9 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
                 self.log.info(f"- {node.version}")
                 wallet_name = f"legacy_up_{node.version}"
                 if self.major_version_at_least(node, 21):
    -                node.rpc.createwallet(wallet_name=wallet_name, descriptors=False)
    +                node.createwallet(wallet_name=wallet_name, descriptors=False)
                 else:
    -                node.rpc.createwallet(wallet_name=wallet_name)
    +                node.createwallet(wallet_name=wallet_name)
                 wallet_prev = node.get_wallet_rpc(wallet_name)
                 address = wallet_prev.getnewaddress('', "bech32")
                 addr_info = wallet_prev.getaddressinfo(address)
    

    mzumsande commented at 9:44 PM on June 23, 2025:

    Makes sense - I renamed it to _rpc, added a comment and fixed the additional spots. There is a legit use for rpc in setting ensure_ascii.

  50. in test/functional/feature_assumeutxo.py:485 in de8b806ad5 outdated
     481 | @@ -481,7 +482,7 @@ def check_dump_output(output):
     482 |  
     483 |          # Use a hash instead of a height
     484 |          prev_snap_hash = n0.getblockhash(prev_snap_height)
     485 | -        dump_output5 = n0.dumptxoutset('utxos5.dat', rollback=prev_snap_hash)
     486 | +        dump_output5 = n0.dumptxoutset('utxos5.dat', rollback=convert_to_json_for_cli(self.options.usecli, prev_snap_hash))
    


    maflcko commented at 11:58 AM on June 19, 2025:

    nit in de8b806ad577a43391d54470081579a4fe8de146: Maybe self.convert_to_json_for_cli(prev_shap_hash) for a less-verbose alternative?


    mzumsande commented at 9:45 PM on June 23, 2025:

    Done.

  51. maflcko commented at 12:39 PM on June 19, 2025: member

    review ACK 60106f32852524498d5d6e958a3bcabdfb06ee1a 🕍

    <details><summary>Show signature</summary>

    Signature:

    untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    trusted comment: review ACK 60106f32852524498d5d6e958a3bcabdfb06ee1a 🕍
    q9TcJblhIp5uAW9SpZJvKitcJo0CClUV8fGnmsGoBotY+vf6/F036IIVR9RFmv4HxhGGbteaWo7q+MLE4QZICA==
    

    </details>

  52. DrahtBot requested review from pinheadmz on Jun 19, 2025
  53. in test/functional/test_framework/test_node.py:921 in 62b82fcb41 outdated
     917 | @@ -918,7 +918,7 @@ def batch(self, requests):
     918 |      def send_cli(self, clicommand=None, *args, **kwargs):
     919 |          """Run bitcoin-cli command. Deserializes returned string as python object."""
     920 |          pos_args = [arg_to_cli(arg) for arg in args]
     921 | -        named_args = [str(key) + "=" + arg_to_cli(value) for (key, value) in kwargs.items()]
     922 | +        named_args = [str(key) + "=" + arg_to_cli(value) for (key, value) in kwargs.items() if value is not None]
    


    maflcko commented at 12:42 PM on June 19, 2025:

    completely unrelated, but converting a value to string is confusing when it:

    • must be of type string and can't be any other type
    • wouldn't make sense to be of non-string type

    .

    It could make sense to drop that:

    diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
    index 9449d3808b..c49bfcffcd 100755
    --- a/test/functional/test_framework/test_node.py
    +++ b/test/functional/test_framework/test_node.py
    @@ -918,7 +918,7 @@ class TestNodeCLI():
         def send_cli(self, clicommand=None, *args, **kwargs):
             """Run bitcoin-cli command. Deserializes returned string as python object."""
             pos_args = [arg_to_cli(arg) for arg in args]
    -        named_args = [str(key) + "=" + arg_to_cli(value) for (key, value) in kwargs.items() if value is not None]
    +        named_args = [key + "=" + arg_to_cli(value) for (key, value) in kwargs.items() if value is not None]
             p_args = self.binaries.rpc_argv() + [f"-datadir={self.datadir}"] + self.options
             if named_args:
                 p_args += ["-named"]
    

    mzumsande commented at 9:46 PM on June 23, 2025:

    yes - I removed this in commit "test: allow all functional tests to be run or skipped with --usecli" which touches this line anyway.

  54. in test/functional/rpc_psbt.py:1212 in 72f8f4f4af outdated
    1207 | @@ -1209,7 +1208,8 @@ def test_psbt_input_keys(psbt_input, keys):
    1208 |          self.log.info("Test descriptorprocesspsbt raises if an invalid sighashtype is passed")
    1209 |          assert_raises_rpc_error(-8, "'all' is not a valid sighash parameter.", self.nodes[2].descriptorprocesspsbt, psbt, [descriptor], sighashtype="all")
    1210 |  
    1211 | -        self.test_sighash_mismatch()
    1212 | +        if not self.options.usecli:
    1213 | +            self.test_sighash_mismatch()
    


    maflcko commented at 12:46 PM on June 19, 2025:

    72f8f4f4afbe975887f77d89ff81d4b63cb4ccf4: Could make sense to use kwargs after commit 62b82fcb410000272340cbdae877ccbbe5da1935 and enable this test again for cli, but no strong opinion.


    mzumsande commented at 9:41 PM on June 23, 2025:

    That's what I did in an earlier version, but with the last push there was a conflict with the recently added test_sighash_mismatch subtest (unrelated to https://github.com/bitcoin/bitcoin/commit/62b82fcb410000272340cbdae877ccbbe5da1935) so I disabled only that subtest. I wonder if there is a better solution for mismatched type checks instead of just disabling them for the cli - but that would probably be something for a follow-up.

  55. test: Enable various tests for usage with cli
    These tests run successfully on current master without any changes.
    6c364e0c10
  56. 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.
    
    Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
    7d5352ac73
  57. 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
    5b08885986
  58. test: rename .rpc to ._rpc and remove unnecessary uses
    It is usually not necessary, and makes it impossible to use --usecli
    8f8ce9e174
  59. 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.
    af34e98086
  60. test: convert tuple to json for cli
    This makes it possible to run rpc_createmultisig.py with --usecli.
    cca422060e
  61. 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.
    
    Also drops an unnecessary str() conversion, kwargs keys
    are always strings.
    54d28722ba
  62. 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.
    6530d0015b
  63. 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.
    f420b6356b
  64. 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
    7ea248a020
  65. ci: use --usecli in one of the CI jobs
    This helps avoid regressions (changes to functional tests
    that are not compatible with -usecli).
    666016e56b
  66. mzumsande force-pushed on Jun 23, 2025
  67. mzumsande commented at 10:08 PM on June 23, 2025: contributor

    60106f3 to 666016e: Addressed feedback by @maflcko and rebased.

  68. maflcko commented at 8:51 AM on June 24, 2025: member

    only change is some nits

    re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae 🔀

    <details><summary>Show signature</summary>

    Signature:

    untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    trusted comment: re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae 🔀
    HWd4XUY5PezE7oN9IN6P2/iqv4G3zJhbGT19Z6z1RjeD4avNt9t1BOKK+S+9nMnUEO9mN9rC35GsAuq2N2e1BA==
    

    </details>

  69. pinheadmz approved
  70. pinheadmz commented at 5:39 PM on July 2, 2025: member

    re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae

    Built and ran all functional tests with --usecli on macos/arm64. Reviewed all code changes again since it's been a while since last review. Coverage is really great.

    <details><summary>Show Signature</summary>

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA256
    
    ACK 666016e56b28b77f798dc85c767b95c1ca0abfae
    -----BEGIN PGP SIGNATURE-----
    
    iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmhlbmsACgkQ5+KYS2KJ
    yTp9HxAAmFHGtNyp2QfIz2EERL9n8BI/sooD3u/8gs6m56qb5Y42qztjq9Qfjk9k
    V4fRpNqSpHEByw1UwxLiZyWMz019zI4PzpNXEHjoXalcgzjyKWmm7U9iivBuGOJF
    R0tLmtzPxYlNtX8TBGPqcKpOD28VxIvriptHrG5fB3qTQU8NyP0cFdfzVuLj+ASu
    JSRUoi7MZWD/yZ+gaF4jDrayVovt5yPMdca6QrVFGL4+pHlEKlwod+WKOjIH+QpN
    rcw1FYchw5PHjsppWUKpGymAF44Au/eytzVx9IVf3u7dRrpjyjF9wtJm0yuNnRfm
    a2YlV248qjHBa4dQJUuZPEUAiDAXuqe7smNhQAsSkWMVgWYVNlMlsYQNuGlfu4vs
    7+waNsqjCc6jkMgNRAhB5WwPgu6KQvaEL2RCKCZxiKNdG+TMPkq789Nv6wbv5hhe
    I3Ubg6QrnWCfsamj/DehLcliK15Imt1TPOwZo84ftLOPGAcMlqtmUoz/EOm4235V
    k1VtnU3/HQZCkENj1b8YF2LKmRf/XzpwrHXMLS4pwNXV0rbWDD/TA+BtgX73UtZp
    Ict2MSNQgjun0wP9w/NxjomOrEfs2R6sNHePXKSlkGh8dyqujoukjmWi435fM8tu
    4G9Q7roRbLeaigjH8OnEUBUfjA/tq6/qla+OkloXXWJIpc5AGIQ=
    =FOfY
    -----END PGP SIGNATURE-----
    

    pinheadmz's public key is on openpgp.org

    </details>

  71. fanquake merged this on Jul 3, 2025
  72. fanquake closed this on Jul 3, 2025

  73. mzumsande deleted the branch on Jul 9, 2025

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me