test: fix feature_index_prune.py bug when using --usecli #34991

polespinasa commented at 7:44 PM on April 2, 2026: member

First commit

This commit fixes an error that made feature_index_prune.py fail if --usecli was used.

The test was failing because node.batch(data) was called with data being a dict. This worked in the normal scenario because AuthServiceProxy.batch() expects a list of petitions in the form of a dict. But TestNodeCLI.batch() expects callables and not a dict. When --usecli is used the test fails with an error TypeError: 'dict' object is not callable.

This is fixed by using get_request() which returns a lambda function if --usecli is used and returns a dict if not. The assert is also changed because before this PR, the requests, constructed by hand were not specifiying the json-rpc version. By default if no version is specified we use version 1.0 which always returns error: none if there is no error. However, in version 2.0, it does not include an error key if there is no error. By using get_request() the requests are done in version 2.0 so there's no key error.

Second commit

The current CI doesn't cover the case of running all tests with --extended if using --usecli. This led to a test failing (feature_index_prune.py) if run with --usecli and the CI not catching it. See #34991 for context.

This commit improves the CI test coverage by also running now all functional tests (--extended) with the flag --usecli.

<details> <summary>First "wrong" approach</summary> Fixes two bugs that make `feature_index_prune.py` fail if `--usecli` is used.

Makes TestNodeCLI.batch() response equivalent to what a JSON-RPC response would look like by adding error=None in the response. The lack of error in the response was giving a KeyError message.
Makes send_batch_request() compatible with --usecli. Before the PR it was passing dicts to node.batch(), but TestNodeCLI.batch() expects callables, not dicts.
</details>

DrahtBot added the label Tests on Apr 2, 2026

DrahtBot commented at 7:45 PM on April 2, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	w0xlt
Stale ACK	Bicaru20, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

polespinasa renamed this:
~~test: fix send_batch_request bug when using --usecli~~
test: fix feature_index_prune.py bug when using --usecli
on Apr 2, 2026

in test/functional/feature_index_prune.py:28 in 899661d6e2

Bicaru20 commented at 10:17 PM on April 2, 2026:

I have a question about this: Why are we building data differently? Why can't we always do it this way even if the cli is not used: data = [getattr(node, method).get_request(*p) for p in params] This way, we do the same to build the data list always and keep the code cleaner. Also at the same time we avoid modifying TestNodeCLI.batch() since the response never include an error key by default even when the cli it is not used. The error key would just appear if something had gone wrong (I modifyed the assert so it behaves accordingly).

I may be missing something or complicating thing too much, but this approach avoids modifying the TestNodeCLI and keeping the clone cleaner.

I tested the changes locally and they pass the test.

data = [getattr(node, method).get_request(*p) for p in params]

response = node.batch(data)
result = []
for item in response:
    assert "error" in item, item["error"]

polespinasa commented at 10:43 PM on April 2, 2026:

This cannot work:

assert "error" in item, item["error"]

This line asserts that error is a key in item and if it fails it executes the instruction after the coma. So if error does not exist it tried to access error. As you can see it does not make sense at all.

Bicaru20 commented at 10:46 PM on April 2, 2026:

Absolutely true, my bad. I meant to put not in

assert "error" not in item, item["error"]

polespinasa commented at 11:20 PM on April 2, 2026:

Yep you are right, that solution is cleaner. In the first time I though it was not a good option because the behavior was still different because of not adding the "error" key in the response. However the "error" key is only added in the json-rpc v1.0. By using get_request it uses 2.0 which doesn't add the "error" key.

The differences can be seen here:

$ curl --user admin:123 \
     --data-binary '{"jsonrpc": "1.0", "method": "getblockcount", "params": []}' \
     -H 'content-type: text/plain;' \
     http://127.0.0.1:18443/
{"result":221,"error":null}

$ curl --user admin:123      --data-binary '{"jsonrpc": "2.0", "id": 1, "method": "getblockcount", "params": []}'      -H 'content-type: text/plain;'      http://127.0.0.1:18443/
{"jsonrpc":"2.0","result":221,"id":1}

Good catch :)

w0xlt commented at 11:05 PM on April 2, 2026: contributor

Concept ACK

test: fix send_batch_request to pass callables when using --usecli

send_batch_request() was building raw dicts without a "jsonrpc" version, which made Core to treat them as version 1.0 requests.
This worked in normal mode, but failed with --usecli because TestNodeCLI.batch() expects callables, not dicts.

This commit fixes it by using get_request() which is defined in both AuthServiceProxy and TestNodeCLIAttr.
The assert is changed because by using get_reques() AuthServiceProxy treats it as "jsonrpc" version 2.0 requests, which don't return "error" keys.

5603ae0ffa

polespinasa force-pushed on Apr 2, 2026

in test/functional/feature_index_prune.py:25 in 5603ae0ffa outdated

  22 | +    data = [getattr(node, method).get_request(*p) for p in params]
  23 |      response = node.batch(data)
  24 |      result = []
  25 |      for item in response:
  26 | -        assert item["error"] is None, item["error"]
  27 | +        assert "error" not in item, item["error"]

polespinasa commented at 11:35 PM on April 2, 2026:

Note for reviewers. Key error in response does not exist anymore because by using get_request() we use json-rpc version 2.0 which does not include error unless there is an actual error. With version 1.0 it included error: null

fanquake commented at 1:44 AM on April 3, 2026: member

I thought the CI also run all the tests with --usecli option.

Yea. However feature_index_prune.py exists in EXTENDED_SCRIPTS (--extended), and it looks like there is no CI using --extended as well as --usecli.

polespinasa commented at 7:20 AM on April 3, 2026: member

I thought the CI also run all the tests with --usecli option.

Yea. However feature_index_prune.py exists in EXTENDED_SCRIPTS (--extended), and it looks like there is no CI using --extended as well as --usecli.

Do you think is worth to add?

fanquake commented at 7:46 AM on April 3, 2026: member

Yea, given that this has been silently broken.

Bicaru20 commented at 9:15 AM on April 3, 2026: none

Lgtm ACK 5603ae0ffa3f0be3d22e8a09008e46c5d48ae12f

sedited approved

sedited commented at 7:46 PM on April 23, 2026: contributor

ACK 5603ae0ffa3f0be3d22e8a09008e46c5d48ae12f

I think it would be better if the other PR were rolled into this one. It is minimally more to review and I'm not sure if going through the review motions in that PR again saves reviewers time.

polespinasa commented at 6:32 AM on April 24, 2026: member

I think it would be better if the other PR were rolled into this one. It is minimally more to review and I'm not sure if going through the review motions in that PR again saves reviewers time.

Makes sense. I've cherry-picked the other commit into this PR.

DrahtBot added the label CI failed on Apr 24, 2026

polespinasa commented at 10:11 AM on April 24, 2026: member

I am not sure at all why the CI is failing here tbh.

sedited commented at 10:30 AM on April 24, 2026: contributor

I am not sure at all why the CI is failing here tbh.

The CI runs the functional tests in the previous releases with the -coverage flag, which checks coverage of RPC calls. I guess this coverage does not apply to invoking the tests with the cli, so it fails.

polespinasa force-pushed on Apr 24, 2026

ci: add --extended when using --usecli

Add the flag --extended to a test (00_setup_env_i686_no_ipc.sh) with the --usecli flag to cover all tests with --usecli.

a49bc1e24e

polespinasa force-pushed on Apr 24, 2026

polespinasa commented at 10:55 AM on April 24, 2026: member

@sedited thanks that was it I think.

Force pushed to get back to the first approach from #34998. Added --extended to the CI that was already running with --usecli.

Friendly pinging @fanquake as you got the idea to do it the other way arround :)

$ git diff 1d45e51b5f329dc9908dc218219fe0c8a6e8591b..a49bc1e24e69ac43beecbdc9c39da3d02160034e
diff --git a/ci/test/00_setup_env_i686_no_ipc.sh b/ci/test/00_setup_env_i686_no_ipc.sh
index 3f0e5f87be..cc672a6f1f 100755
--- a/ci/test/00_setup_env_i686_no_ipc.sh
+++ b/ci/test/00_setup_env_i686_no_ipc.sh
@@ -14,7 +14,7 @@ export PACKAGES="llvm clang g++-multilib"
 export DEP_OPTS="DEBUG=1 NO_IPC=1"
 export GOAL="install"
 export CI_LIMIT_STACK_SIZE=1
-export TEST_RUNNER_EXTRA="--v2transport"
+export TEST_RUNNER_EXTRA="--v2transport --usecli --extended"
 export BITCOIN_CONFIG="\
  --preset=dev-mode \
  -DENABLE_IPC=OFF \
diff --git a/ci/test/00_setup_env_native_previous_releases.sh b/ci/test/00_setup_env_native_previous_releases.sh
index c933968e7d..d6af52c4bf 100755
--- a/ci/test/00_setup_env_native_previous_releases.sh
+++ b/ci/test/00_setup_env_native_previous_releases.sh
@@ -11,7 +11,7 @@ export CI_IMAGE_NAME_TAG="mirror.gcr.io/ubuntu:22.04"
 # Use minimum supported python3.10 and gcc-12, see doc/dependencies.md
 export PACKAGES="gcc-12 g++-12 python3-zmq"
 export DEP_OPTS="CC=gcc-12 CXX=g++-12"
-export TEST_RUNNER_EXTRA="--previous-releases --coverage --extended --usecli --exclude feature_dbcrash"  # Run extended tests so that coverage does not fail, but exclude the very slow dbcrash
+export TEST_RUNNER_EXTRA="--previous-releases --coverage --extended --exclude feature_dbcrash"  # Run extended tests so that coverage does not fail, but exclude the very slow dbcrash
 export GOAL="install"
 export CI_LIMIT_STACK_SIZE=1
 export DOWNLOAD_PREVIOUS_RELEASES="true"

</details>

DrahtBot removed the label CI failed on Apr 24, 2026

fanquake commented at 1:01 PM on April 24, 2026: member

I think my reasoning for combining it into an existing --extended job, was just to avoid slowing down another CI.

polespinasa commented at 1:06 PM on April 24, 2026: member

I think my reasoning for combining it into an existing --extended job, was just to avoid slowing down another CI.

Makes sense. Seems unavoidable... There are not more tests with --extended. Unless we get coverage for all rpc tests...