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

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

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

$ ./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

Fix this issue by running bitcoin under valgrind.

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	achow101, ryanofsky
Stale ACK	theStack

If your review is incorrectly listed, please copy-paste <code></code> 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.

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:

$  echo 'pipe' | valgrind --quiet ./bld-cmake/bin/bitcoin rpc -stdinrpcpass uptime
error: timeout on transient error: Could not connect to the server 127.0.0.1:18443

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
Use "bitcoin-cli -help" for more info.

$  echo 'pipe' | valgrind --quiet --trace-children=yes ./bld-cmake/bin/bitcoin rpc -stdinrpcpass uptime
==841070== Syscall param ioctl(TCSET{S,SW,SF}) points to uninitialised byte(s)
==841070==    at 0x4E65871: tcsetattr (in /usr/lib64/libc.so.6)
==841070==    by 0x40678A7: SetStdinEcho (src/compat/stdin.cpp:39)
==841070==    by 0x40678A7: NoechoInst::NoechoInst() (src/compat/stdin.cpp:67)
==841070==    by 0x403E557: CommandLineRPC(int, char**) (src/bitcoin-cli.cpp:1223)
==841070==    by 0x403D8DC: main (src/bitcoin-cli.cpp:1354)
==841070==  Address 0x1ffefff490 is on thread 1's stack
==841070==  in frame [#0](/bitcoin-bitcoin/0/), created by tcsetattr (???:)
==841070== 
==841070== Syscall param ioctl(TCSET{S,SW,SF}) points to uninitialised byte(s)
==841070==    at 0x4E65871: tcsetattr (in /usr/lib64/libc.so.6)
==841070==    by 0x4067907: SetStdinEcho (src/compat/stdin.cpp:39)
==841070==    by 0x4067907: NoechoInst::~NoechoInst() (src/compat/stdin.cpp:68)
==841070==    by 0x403E65F: CommandLineRPC(int, char**) (src/bitcoin-cli.cpp:1235)
==841070==    by 0x403D8DC: main (src/bitcoin-cli.cpp:1354)
==841070==  Address 0x1ffefff490 is on thread 1's stack
==841070==  in frame [#0](/bitcoin-bitcoin/0/), created by tcsetattr (???:)
==841070== 
error: timeout on transient error: Could not connect to the server 127.0.0.1:18443

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
Use "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:

--trace-children=<yes|no> [default: no]
    When enabled, Valgrind will trace into sub-processes initiated via the exec system call. 
    This is necessary for multi-process programs.

    Note that Valgrind does trace into the child of a fork (it would be difficult not to,
    since fork makes an identical copy of a process), so this option is arguably badly named.
    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.

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:

$ strace -e trace=execve --follow ./build/test/functional/tool_bitcoin.py --valgrind 2>&1 | grep valgrind
[ 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.

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

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:

--- a/test/functional/tool_bitcoin.py
+++ b/test/functional/tool_bitcoin.py
@@ -39,7 +39,7 @@ class ToolBitcoinTest(BitcoinTestFramework):
 
     def set_cmd_args(self, node, args):
         """Set up node so it will be started through bitcoin wrapper command with specified arguments."""
-        node.args = [self.binary_paths.bitcoin_bin] + args + ["node"] + self.node_options[node.index]
+        node.args = self.get_binaries().valgrind_cmd + [self.binary_paths.bitcoin_bin] + args + ["node"] + self.node_options[node.index]
 
     def test_args(self, cmd_args, node_args, expect_exe=None, expect_error=None):
         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

maflcko commented at 10:34 AM on February 23, 2026: member

Following might be a quick fix:

Thx. I took something like this. I think it is sufficient for now, and any refactoring can be done in a follow-up. I am also happy to push any commit or diff here, if someone sends one.

theStack approved

theStack commented at 4:09 PM on February 23, 2026: contributor

ACK fa4c9506db954c865ca3f4de1bf62d95c19994e2

I agree that improving the last commit can be done in a follow-up PR.

DrahtBot requested review from ryanofsky on Feb 23, 2026

ryanofsky approved

ryanofsky commented at 5:49 PM on February 23, 2026: contributor

Code review ACK fa4c9506db954c865ca3f4de1bf62d95c19994e2, but the two commits fabd7200f11e3f16e336eda1f411dd1af5898c1c and fa4c9506db954c865ca3f4de1bf62d95c19994e2 should be squashed. It doesn't seem to make sense to have one commit trying use valgrind with the bitcoin wrapper followed by another commit immediately fixing a case where it isn't used.

test: Fix broken --valgrind handling after bitcoin wrapper

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.

Review note:

The set_cmd_args was ignoring the --valgrind test option.

In theory, this could be fixed by refactoring Binaries::node_argv() to
be used here. However, for now, just re-implement the node_argv logic in
set_cmd_args to prepend the valgrind cmd.

fa03fbf7e3

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
```

fa29fb72cb

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

maflcko force-pushed on Feb 24, 2026

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

fanquake added the label Needs backport (30.x) on Feb 24, 2026

DrahtBot added the label CI failed on Feb 24, 2026

DrahtBot removed the label CI failed on Feb 24, 2026

achow101 commented at 10:05 PM on February 24, 2026: member

ACK fa5d478853a88ce7d7095b6abfe8112390298b65

DrahtBot requested review from theStack on Feb 24, 2026

DrahtBot requested review from ryanofsky on Feb 24, 2026

ryanofsky approved

ryanofsky commented at 9:27 AM on February 25, 2026: contributor

Code review ACK fa5d478853a88ce7d7095b6abfe8112390298b65 just squashing commits since last review (Thanks!)

ryanofsky merged this on Feb 25, 2026

ryanofsky closed this on Feb 25, 2026

fanquake removed the label Needs backport (30.x) on Feb 25, 2026

fanquake commented at 10:15 AM on February 25, 2026: member

Backported to 30.x in #34459.

maflcko deleted the branch on Feb 25, 2026

fanquake referenced this in commit b294285a3f on Feb 25, 2026

fanquake referenced this in commit 5075ae80ed on Feb 25, 2026

fanquake referenced this in commit 202233a610 on Feb 25, 2026

fanquake referenced this in commit 6993b1be78 on Feb 25, 2026

fanquake referenced this in commit dd8924909a on Feb 25, 2026

fanquake referenced this in commit dca7700269 on Feb 25, 2026