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

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

This PR 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.

Maybe to check for a followup PR I saw that feature_index_prune.py was failing locally if using the --usecli option while testing another PR. I am not really familiar with the CI but it didn’t detect this error. I thought the CI also run all the tests with --usecli option.

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.

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
ACK	Bicaru20
Concept ACK	w0xlt

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> 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.

0data = [getattr(node, method).get_request(*p) for p in params]
1
2response = node.batch(data)
3result = []
4for item in response:
5    assert "error" in item, item["error"]

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

This cannot work:

0assert "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

0assert "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:

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

0$ 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/
1{"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

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

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

Reviews