test: Fix broken --valgrind handling after bitcoin wrapper

maflcko commented at 8:38 pm on February 17, 2026: member

Currently, tool_bitcoin.py is failing under --valgrind:

 0$ ./bld-cmake/test/functional/tool_bitcoin.py --valgrind
 1TestFramework (ERROR): Unexpected exception
 2Traceback (most recent call last):
 3  File "./test/functional/test_framework/test_framework.py", line 138, in main
 4    self.setup()
 5    ~~~~~~~~~~^^
 6  File "./test/functional/test_framework/test_framework.py", line 269, in setup
 7    self.setup_network()
 8    ~~~~~~~~~~~~~~~~~~^^
 9  File "./test/functional/tool_bitcoin.py", line 38, in setup_network
10    assert all(node.args[:len(node_argv)] == node_argv for node in self.nodes)
11           ~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12AssertionError

Fix this issue by running bitcoin under valgrind.

Prior to this commit, tool_bitcoin.py was failing:

```sh
$ ./bld-cmake/test/functional/tool_bitcoin.py --valgrind
TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
  File "./test/functional/test_framework/test_framework.py", line 138, in main
    self.setup()
    ~~~~~~~~~~^^
  File "./test/functional/test_framework/test_framework.py", line 269, in setup
    self.setup_network()
    ~~~~~~~~~~~~~~~~~~^^
  File "./test/functional/tool_bitcoin.py", line 38, in setup_network
    assert all(node.args[:len(node_argv)] == node_argv for node in self.nodes)
           ~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
```

This commit fixes this issue by running `bitcoin` under valgrind. Also,
it comes with other improvements:

* Drop the outdated valgrind 3.14 requirement, because there is no
  distro that ships a version that old anymore.
* Drop the VALGRIND_SUPPRESSIONS_FILE env var handling, because it was
  presumably never used since it was introduced. Also, the use-case
  seems limited.

fabd7200f1

DrahtBot added the label Tests on Feb 17, 2026

DrahtBot commented at 8:38 pm on February 17, 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	ryanofsky
Concept ACK	theStack

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31723 (qa: Facilitate debugging bitcoind inside functional tests by hodlinator)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

test: Remove redundant warning about missing binaries

The error was added in commit 1ea7e45a1f445d32a2b690d52befb2e63418653b,
because there was an additional confusing `AssertionError: [node 0]
Error: no RPC connection` instead of just a single `FileNotFoundError:
[Errno 2] No such file or directory`.

This is no longer needed on current master.

Also, the test is incomplete, because it was just checking bitcoind and
bitcoin-cli, not any other missing binaries.

Also, after the previous commit, it would not work in combination with
--valgrind.

Instead of trying to make it complete, and work in all combinations,
just remove it, because the already existing error will be clear in any
case.

This can be tested via:

```sh
 ./test/get_previous_releases.py

 mv releases releases_backup
 # Confirm the test is skipped due to missing releases
 ./bld-cmake/test/functional/wallet_migration.py
 # Confirm the test fails due to missing releases
 ./bld-cmake/test/functional/wallet_migration.py --previous-releases
 mv releases_backup releases

 mv ./releases/v28.2 ./releases/v28.2_backup
 # Confirm the test fails with a single FileNotFoundError
 ./bld-cmake/test/functional/wallet_migration.py
 mv ./releases/v28.2_backup ./releases/v28.2
 # Confirm the test runs and passes
 ./bld-cmake/test/functional/wallet_migration.py

 rm ./bld-cmake/bin/bitcoind
 # Confirm the test fails with a single "No such file or directory",
 # testing with and without --valgrind
 ./bld-cmake/test/functional/wallet_migration.py
 ./bld-cmake/test/functional/wallet_migration.py --valgrind
```

faec648b5b

fanquake commented at 10:14 am on February 18, 2026: member

cc @Sjors @ryanofsky

in test/functional/test_framework/test_framework.py:195 in fabd7200f1

191@@ -192,7 +192,7 @@ def parse_args(self, test_file):
192         parser.add_argument("--perf", dest="perf", default=False, action="store_true",
193                             help="profile running nodes with perf for the duration of the test")
194         parser.add_argument("--valgrind", dest="valgrind", default=False, action="store_true",
195-                            help="run nodes under the valgrind memory error detector: expect at least a ~10x slowdown. valgrind 3.14 or later required. Does not apply to previous release binaries.")
196+                            help="Run binaries under the valgrind memory error detector: Expect at least a ~10x slowdown. Does not apply to previous release binaries or binaries called from the bitcoin wrapper executable.")

ryanofsky commented at 1:10 pm on February 18, 2026:

In commit “test: Fix broken –valgrind handling after bitcoin wrapper” (fabd7200f11e3f16e336eda1f411dd1af5898c1c)

Is the new text “Does not apply to […] binaries called from the bitcoin wrapper executable” true? It looks like valgrind args are prepended to bitcoin_cmd, not omitted. And the bitcoin command executable internally just calls exec and doesn’t even start a separate process so I would think it would not disable valgrind. Maybe this is not the case though? It would be good to clarify help text to say what happens if BITCOIN_CMD is set or IPC tests are running. Like if the wrapper executable itself runs under valgrind but whatever binary it executes isn’t. We might want to add valgrind support to the wrapper if that is the case, although it would be beyond the scope of this PR.

maflcko commented at 2:16 pm on February 18, 2026:

Checked by triggering the #34597 UB:

0$  echo 'pipe' | valgrind --quiet ./bld-cmake/bin/bitcoin rpc -stdinrpcpass uptime
1error: timeout on transient error: Could not connect to the server 127.0.0.1:18443
2
3Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
4Use "bitcoin-cli -help" for more info.

 0$  echo 'pipe' | valgrind --quiet --trace-children=yes ./bld-cmake/bin/bitcoin rpc -stdinrpcpass uptime
 1==841070== Syscall param ioctl(TCSET{S,SW,SF}) points to uninitialised byte(s)
 2==841070==    at 0x4E65871: tcsetattr (in /usr/lib64/libc.so.6)
 3==841070==    by 0x40678A7: SetStdinEcho (src/compat/stdin.cpp:39)
 4==841070==    by 0x40678A7: NoechoInst::NoechoInst() (src/compat/stdin.cpp:67)
 5==841070==    by 0x403E557: CommandLineRPC(int, char**) (src/bitcoin-cli.cpp:1223)
 6==841070==    by 0x403D8DC: main (src/bitcoin-cli.cpp:1354)
 7==841070==  Address 0x1ffefff490 is on thread 1's stack
 8==841070==  in frame [#0](/bitcoin-bitcoin/0/), created by tcsetattr (???:)
 9==841070== 
10==841070== Syscall param ioctl(TCSET{S,SW,SF}) points to uninitialised byte(s)
11==841070==    at 0x4E65871: tcsetattr (in /usr/lib64/libc.so.6)
12==841070==    by 0x4067907: SetStdinEcho (src/compat/stdin.cpp:39)
13==841070==    by 0x4067907: NoechoInst::~NoechoInst() (src/compat/stdin.cpp:68)
14==841070==    by 0x403E65F: CommandLineRPC(int, char**) (src/bitcoin-cli.cpp:1235)
15==841070==    by 0x403D8DC: main (src/bitcoin-cli.cpp:1354)
16==841070==  Address 0x1ffefff490 is on thread 1's stack
17==841070==  in frame [#0](/bitcoin-bitcoin/0/), created by tcsetattr (???:)
18==841070== 
19error: timeout on transient error: Could not connect to the server 127.0.0.1:18443
20
21Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
22Use "bitcoin-cli -help" for more info.

maflcko commented at 2:20 pm on February 18, 2026:

thx, pushed a commit to fix this

ryanofsky commented at 3:26 pm on February 18, 2026:

re: #34608 (review)

New commit getting valgrind to work seems good.

I was confused by output posted above because I wouldn’t expect valgrind without --trace-children=yes to result in a “timeout on transient error” from bitcoin-cli. But maybe that error is unrelated and just happens because bitcoind is not started.

I’d also expect –trace-children=yes not to be needed the bitcoin wrapper shouldn’t create a child process, but maybe option name is a little misleading and it is needed when there are exec calls even without fork.

theStack commented at 5:54 pm on February 20, 2026:

I’d also expect –trace-children=yes not to be needed the bitcoin wrapper shouldn’t create a child process, but maybe option name is a little misleading and it is needed when there are exec calls even without fork.

Was also wondering about that and found that the option is indeed named in a confusing way, as also admitted in the manpage:

0--trace-children=<yes|no> [default: no]
1    When enabled, Valgrind will trace into sub-processes initiated via the exec system call. 
2    This is necessary for multi-process programs.
3
4    Note that Valgrind does trace into the child of a fork (it would be difficult not to,
5    since fork makes an identical copy of a process), so this option is arguably badly named.
6    However, most children of fork calls immediately call exec anyway.

ryanofsky approved

ryanofsky commented at 1:24 pm on February 18, 2026: contributor

Code review ACK faec648b5b391c6bf173b13b48e3de92018a0c17. Nice cleanups and fix. It seems good to keep valgrind args separate and add them right before invoking commands instead of baking them into bitcoind args.

The second commit removing the missing binaries checks added in #31462 and #32921 is probably also ok in most cases but would seem worse in the case where “only a subset of the necessary previous release binaries are available” which #31462 was intended to improve. This is not a case I normally deal with, while I do deal with the case where I have done a partial build and the check warns about a missing binary which is not actually needed. So I am ok with getting rid of the check, but maybe @theStack would be less happy.

You could try scaling back the second commit faec648b5b391c6bf173b13b48e3de92018a0c17 to keep the missing binaries check but only raise the error if v is not None, dropping the “did you compile” error but keeping the previous releases error.

maflcko commented at 2:06 pm on February 18, 2026: member

You could try scaling back the second commit faec648 to keep the missing binaries check but only raise the error if v is not None, dropping the “did you compile” error but keeping the previous releases error.

Yeah, I guess so, but it seems an edge case. Personally, I think the single FileNotFoundError with the path to the releases folder, and the version number in the path should be enough for a dev to figure out that the specific version executable was not found, and that it may need to be fetched. The prior confusing AssertionError is long fixed on current master.

But no strong opinion. Happy to push your suggestion, if another reviewer likes it.

while I do deal with the case where I have done a partial build and the check warns about a missing binary which is not actually needed. So I am ok with getting rid of the check,

Ah, good point that the check has not only false-negatives, but also false-positives.

test: valgrind --trace-children=yes for bitcoin wrapper fa46ff661f

ryanofsky approved

ryanofsky commented at 3:30 pm on February 18, 2026: contributor

Code review ACK fa46ff661f3d5a845c85ae3f782243dd8eaa0cb6 just adding new commit with –trace-children=yes option, which seems good, although I don’t look into what full effects of this option are

r9tt8cf86k-code commented at 3:40 pm on February 18, 2026: none

How

theStack commented at 8:13 pm on February 18, 2026: contributor

Concept ACK

The second commit removing the missing binaries checks added in #31462 and #32921 is probably also ok in most cases but would seem worse in the case where “only a subset of the necessary previous release binaries are available” which #31462 was intended to improve. This is not a case I normally deal with, while I do deal with the case where I have done a partial build and the check warns about a missing binary which is not actually needed. So I am ok with getting rid of the check, but maybe @theStack would be less happy.

Thanks for pinging. It’s fine for me to remove the missing binaries checks, considering that the confusing error message at the time #31462 was merged has been improved on master since #31620 (i.e. the FileNotFoundError now shown directly at the bottom, without follow-up exceptions). I think getting the direct instructions on how get the previous releases would still have some merit (and some devs would probably still save a few seconds by it ;p), but no strong objections only for that.

theStack commented at 6:53 pm on February 20, 2026: contributor

It seems that passing --valgrind to the tool_bitcoin.py test now removes the AssertionError, but valgrind is still not involved:

0$ strace -e trace=execve --follow ./build/test/functional/tool_bitcoin.py --valgrind 2>&1 | grep valgrind
1[ matches only the `--valgrind` parameters of the python test, but not any valgrind binary ]

In comparison, running the same command with any other functional tests shows the valgrind binary, e.g.

0$ strace -e trace=execve --follow ./build/test/functional/feature_dersig.py --valgrind 2>&1 | grep valgrind
1...
2[pid 91053] execve("/usr/bin/valgrind.bin"..... )
3...

Didn’t look closer yet, but I guess the set_cmd_args method has to be rewritten in some way to take over the valgrind options from the Binaries class?

ryanofsky commented at 7:54 pm on February 20, 2026: contributor

Didn’t look closer yet, but I guess the set_cmd_args method has to be rewritten in some way to take over the valgrind options from the Binaries class?

Following might be a quick fix:

 0--- a/test/functional/tool_bitcoin.py
 1+++ b/test/functional/tool_bitcoin.py
 2@@ -39,7 +39,7 @@ class ToolBitcoinTest(BitcoinTestFramework):
 3 
 4     def set_cmd_args(self, node, args):
 5         """Set up node so it will be started through bitcoin wrapper command with specified arguments."""
 6-        node.args = [self.binary_paths.bitcoin_bin] + args + ["node"] + self.node_options[node.index]
 7+        node.args = self.get_binaries().valgrind_cmd + [self.binary_paths.bitcoin_bin] + args + ["node"] + self.node_options[node.index]
 8 
 9     def test_args(self, cmd_args, node_args, expect_exe=None, expect_error=None):
10         node = self.nodes[0]

It would probably be good to do a little refactoring so tool_bitcoin.py could get arguments from the binaries class more easily, but it wouldn’t necessarily be required here

test: Fix broken –valgrind handling after bitcoin wrapper #34608

Reviews

Conflicts