test: pass -datadir to bitcoin-cli -ipcconnect check #35431

pull mjdietzx wants to merge 1 commits into bitcoin:master from mjdietzx:minor_fix_interface_bitcoin_cli_test changing 1 files +2 −1

mjdietzx commented at 1:17 PM on June 1, 2026: contributor

This case invokes bitcoin-cli via raw subprocess.run() without -datadir, so it reads the default datadir's bitcoin.conf (e.g. ~/.bitcoin) and fails whenever that real config is unusable. Pass the node's -datadir so the check reads the test's own bitcoin.conf and depends only on the build's IPC support, not the host environment.
DrahtBot added the label Tests on Jun 1, 2026
DrahtBot commented at 1:17 PM on June 1, 2026: 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/35431.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK maflcko, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
fanquake commented at 1:18 PM on June 1, 2026: member

cc @ryanofsky @Sjors

Type	Reviewers
ACK	maflcko, sedited

in test/functional/interface_bitcoin_cli.py:463 in a3d8efa213

 457 | @@ -458,7 +458,9 @@ def run_test(self):
 458 |              # This tests behavior when ENABLE_IPC is off. When it is on,
 459 |              # behavior is checked by the interface_ipc_cli.py test.
 460 |              self.log.info("Test bitcoin-cli -ipcconnect triggers error if not built with IPC support")
 461 | -            args = [self.binary_paths.bitcoincli, "-ipcconnect=unix", "-getinfo"]
 462 | +            # Pass the node's -datadir so bitcoin-cli reads the test's bitcoin.conf rather than a
 463 | +            # real one in the default datadir (e.g. ~/.bitcoin), keeping this test self-contained.
 464 | +            args = [self.binary_paths.bitcoincli, f"-datadir={self.nodes[0].datadir_path}", "-ipcconnect=unix", "-getinfo"]

maflcko commented at 10:47 AM on June 2, 2026:

Why is this not using valgrind?

$ git grep -A1 Binaries::node_argv test/
test/functional/tool_bitcoin.py:        # Manually construct the `bitcoin node` command, similar to Binaries::node_argv()
test/functional/tool_bitcoin.py-        bitcoin_cmd = node.binaries.valgrind_cmd + [node.binaries.paths.bitcoin_bin]

Also, the options (datadir, etc ) would better come from self.nodes[0].cli.options?

mjdietzx commented at 2:38 PM on June 2, 2026:

Thanks @maflcko I added the valgrind wrapper. I tried node.cli.options based on your review, but this includes -rpcconnect which bitcoin-cli rejects together with -ipcconnect. So I left as-is and added comment

maflcko commented at 3:07 PM on June 2, 2026:

Ah, I see. Of course rpcconnect and ipcconnect are mutually exclusive. I wonder if it could make sense to remove --usecli and instead add --usecli-rpc and --usecli-ipc, which then sets either -rpcconnect or -ipconnect in a later pull request?

mjdietzx force-pushed on Jun 2, 2026

test: run bitcoin-cli -ipcconnect check under valgrind with -datadir

This case invoked bitcoin-cli via raw subprocess.run() without the
valgrind wrapper (bypassing valgrind) and without -datadir (so it read
the default datadir's bitcoin.conf, e.g. ~/.bitcoin, and failed whenever
that real config was unusable). Build the command like the rest of the
framework: prepend binaries.valgrind_cmd and pass the node's -datadir so
the check runs under valgrind and depends only on the build's IPC
support, not the host environment.

7735c13488

mjdietzx force-pushed on Jun 2, 2026
maflcko commented at 3:05 PM on June 2, 2026: member

lgtm ACK 7735c134887c9ab25d4ec3dc1981b5e4ae4b3ece
sedited approved
sedited commented at 8:58 AM on June 4, 2026: contributor

ACK 7735c134887c9ab25d4ec3dc1981b5e4ae4b3ece
sedited merged this on Jun 4, 2026
sedited closed this on Jun 4, 2026

Contributors

Labels

test: pass -datadir to bitcoin-cli -ipcconnect check #35431

Code Coverage & Benchmarks

Reviews