test: Avoid empty errmsg in JSONRPCException

maflcko commented at 4:12 pm on February 12, 2026: member

It is unclear why the fallback should be an empty message, when it is better to include all rpc_error details that are available.

Also, include the http status.

This allows to revert commit 6354b4fd7fe819eb13274b212e426a7d10ca75d3, because it is no longer needed.

Can be tested by running this diff:

 0diff --git a/test/functional/wallet_disable.py b/test/functional/wallet_disable.py
 1index dbcccd4778..9717a2d248 100755
 2--- a/test/functional/wallet_disable.py
 3+++ b/test/functional/wallet_disable.py
 4@@ -18,9 +18,8 @@ class DisableWalletTest (BitcoinTestFramework):
 5         self.extra_args = [["-disablewallet"]]
 6         self.wallet_names = []
 7 
 8-    def run_test (self):
 9-        # Make sure wallet is really disabled
10-        assert_raises_rpc_error(-32601, 'Method not found', self.nodes[0].getwalletinfo)
11+    def run_test(self):
12+        self.nodes[0].getwalletinfo()
13         x = self.nodes[0].validateaddress('3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy')
14         assert x['isvalid'] == False
15         x = self.nodes[0].validateaddress('mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ')

DrahtBot added the label Tests on Feb 12, 2026

DrahtBot commented at 4:12 pm on February 12, 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	furszy, hodlinator, ismaelsadeeq, rkrux

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

fanquake added this to the milestone 31.0 on Feb 12, 2026

in test/functional/test_framework/test_framework.py:155 in fa2ac3ee57 outdated

146@@ -147,9 +147,6 @@ def main(self):
147         except subprocess.CalledProcessError as e:
148             self.log.exception(f"Called Process failed with stdout='{e.stdout}'; stderr='{e.stderr}';")
149             self.success = TestStatus.FAILED
150-        except JSONRPCException as e:
151-            self.log.exception(f"Failure during setup: error={e.error}, http_status={e.http_status}")
152-            self.success = TestStatus.FAILED
153         except BaseException:
154             self.log.exception("Unexpected exception")
155             self.success = TestStatus.FAILED

furszy commented at 4:18 pm on February 13, 2026:

Shouldn’t we log the exception here now? Like:

0except BaseException as e:
1   self.log.exception(f"Unexpected exception: {e}")
2   self.success = TestStatus.FAILED

maflcko commented at 4:32 pm on February 13, 2026:

No, that’d log it twice, because Python already does the right thing internally :)

Happy to add a comment, if it helps

furszy commented at 5:40 pm on February 13, 2026:

oh, yeah. It would be helpful to mention it for python neophytes like me, as it is something that could come up in the future again.

maflcko commented at 5:56 pm on February 13, 2026:

thx, done

test: Avoid empty errmsg in JSONRPCException

It is unclear why the fallback should be an empty message, when it is
better to include all rpc_error details that are available.

Also, include the http status.

This allows to revert commit 6354b4fd7fe819eb13274b212e426a7d10ca75d3,
because it is no longer needed.

211111b804

maflcko force-pushed on Feb 13, 2026

furszy commented at 6:32 pm on February 13, 2026: member

utACK 211111b8048dad959a510150d80277bd9b3d0e25

hodlinator approved

hodlinator commented at 9:26 pm on February 13, 2026: contributor

ACK 211111b8048dad959a510150d80277bd9b3d0e25

Modified wallet_disable.py as suggested and got the following results:

Output on master

0₿ ./build/test/functional/wallet_disable.py
12026-02-13T21:08:07.396695Z TestFramework (ERROR): Failure during setup: error={'code': -32601, 'message': 'Method not found'}, http_status=200
2...
3test_framework.authproxy.JSONRPCException: Method not found (-32601)

Output with PR

0₿ ./build/test/functional/wallet_disable.py
12026-02-13T21:01:42.321047Z TestFramework (ERROR): Unexpected exception:
2...
3test_framework.authproxy.JSONRPCException: {'code': -32601, 'message': 'Method not found'} [http_status=200]

Conclusions

“Failure during setup” message on master is incorrect, the example makes us throw an exception from run_test(), which is included in the try-block along with setup().

https://github.com/bitcoin/bitcoin/blob/b65ff0e5a1fd4ea2ae75e204729b8008c4ebb9ab/test/functional/test_framework/test_framework.py#L137-L142
I think JSON-RPC 2 is meant to always use HTTP status 200 regardless of error but cannot find it in the specification, I guess adding it is okay, it’s being sent in everywhere after all.
The old output on master

Method not found (-32601)

was nicer than the PR’s

{‘code’: -32601, ‘message’: ‘Method not found’} [http_status=200]

ismaelsadeeq commented at 3:16 pm on February 16, 2026: member

utACK 211111b8048dad959a510150d80277bd9b3d0e25

maflcko commented at 3:43 pm on February 16, 2026: member

I think JSON-RPC 2 is meant to always use HTTP status 200 regardless of error but cannot find it in the specification, I guess adding it is okay, it’s being sent in everywhere after all.

See https://github.com/bitcoin/bitcoin/blob/35e6444fdc4068adc79082648f9889ad593e623b/doc/JSON-RPC-interface.md#json-rpc-11-vs-20

The old output on master

Method not found (-32601)

was nicer than the PR’s

{‘code’: -32601, ‘message’: ‘Method not found’} [http_status=200]

Yeah, happy to review a follow-up improving the wording. One could use something like (untested):

0 class JSONRPCException(Exception):
1    def __init__(self, rpc_error, http_status=None):
2        message = rpc_error.pop("message", "INTERNAL ERROR: message field missing in rpc error dict")
3        code = rpc_error.pop("code", "INTERNAL_ERROR: code field missing in rpc error dict")
4        extra = f'{rpc_error}' if rpc_error else ''
5        super().__init__(f"{message} ({code}) {extra} [http_status={http_status}]")

rkrux approved

rkrux commented at 9:08 am on February 17, 2026: contributor

tACK 211111b8048dad959a510150d80277bd9b3d0e25

fanquake merged this on Feb 17, 2026

fanquake closed this on Feb 17, 2026

maflcko deleted the branch on Feb 17, 2026

hodlinator commented at 10:53 am on February 17, 2026: contributor

Thanks @maflcko regarding #34575 (comment), seems neat to use pop(). Might make a follow-up PR after I see them a few times in logs.

maflcko commented at 12:14 pm on February 17, 2026: member

A version without the magic strings could be (untested):

0 class JSONRPCException(Exception):
1    def __init__(self, rpc_error, http_status=None):
2        # thow KeyError if any required fields are missing
3        message = rpc_error.pop("message")
4        code = rpc_error.pop("code")
5        extra = f'{rpc_error}' if rpc_error else ''
6        super().__init__(f"{message} ({code}) {extra} [http_status={http_status}]")

This should also make it easier to debug, in the unlikely case that a required field is ever missing.

sedited referenced this in commit 925759d172 on Feb 23, 2026

sedited referenced this in commit 8640523843 on Feb 23, 2026

maflcko commented at 7:10 pm on March 3, 2026: member

Yeah, happy to review a follow-up improving the wording. One could use something like (untested):

Printing strings instead of json should also get rid of the escaped newlines inside the json:

 0test  2026-03-03T17:23:03.211345Z TestFramework (ERROR): Unexpected exception: 
 1                                   Traceback (most recent call last):
 2                                     File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/test_framework.py", line 142, in main
 3                                       self.run_test()
 4                                       ~~~~~~~~~~~~~^^
 5                                     File "/Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch/build-aarch64-apple-darwin24.6.0/test/functional/rpc_orphans.py", line 34, in run_test
 6                                       self.test_misc()
 7                                       ~~~~~~~~~~~~~~^^
 8                                     File "/Users/runner/work/bitcoin/bitcoin/repo_archive/ci/scratch/build-aarch64-apple-darwin24.6.0/test/functional/rpc_orphans.py", line 150, in test_misc
 9                                       help_output = node.help()
10                                     File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/coverage.py", line 50, in __call__
11                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
12                                     File "/Users/runner/work/bitcoin/bitcoin/repo_archive/test/functional/test_framework/authproxy.py", line 152, in __call__
13                                       raise JSONRPCException(response['error'], status)
14test_framework.authproxy.JSONRPCException: {'code': -1, 'message': 'Internal bug detected: Unreachable code reached (non-fatal)\nrpc/util.cpp:1025 (void RPCResult::ToSections(Sections &, const OuterType, const int) const)\nBitcoin Core v30.99.0-8d001d01b783\nPlease report this issue here: [https://github.com/bitcoin/bitcoin/issues\n](https://github.com/bitcoin/bitcoin/issues/n)'} [http_status=200]

test: Avoid empty errmsg in JSONRPCException #34575

Reviews

Output on master

Output with PR

Conclusions