test: Fix broken --valgrind handling after bitcoin wrapper by maflcko · Pull Request #34608 · bitcoin/bitcoin

maflcko · 2026-02-17T20:38:21Z

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 · 2026-02-17T20:38:44Z

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  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 · 2026-02-18T10:14:49Z

cc @Sjors @ryanofsky

ryanofsky

Code review ACK faec648. 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 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.

test/functional/test_framework/test_framework.py

maflcko · 2026-02-18T14:06:00Z

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

Code review ACK fa46ff6 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

test/functional/test_framework/test_framework.py

theStack · 2026-02-18T20:13:55Z

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 · 2026-02-20T18:53:45Z

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 · 2026-02-20T19:54:08Z

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 · 2026-02-23T10:34:06Z

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

ACK fa4c950

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

ryanofsky

Code review ACK fa4c950, but the two commits fabd720 and fa4c950 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.

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.

The error was added in commit 1ea7e45, 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 ```

achow101 · 2026-02-24T22:05:52Z

ACK fa5d478

ryanofsky

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

fanquake · 2026-02-25T10:15:34Z

Backported to 30.x in #34459.

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. Github-Pull: bitcoin#34608 Rebased-From: fa03fbf

The error was added in commit 1ea7e45, 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 ``` Github-Pull: bitcoin#34608 Rebased-From: fa29fb7

Github-Pull: bitcoin#34608 Rebased-From: fa5d478

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. Github-Pull: bitcoin#34608 Rebased-From: fa03fbf

The error was added in commit 1ea7e45, 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 ``` Github-Pull: bitcoin#34608 Rebased-From: fa29fb7

Github-Pull: bitcoin#34608 Rebased-From: fa5d478

fdaf656 doc: update release notes for v30.x (fanquake) 0202ae3 doc: Update Guix install for Debian/Ubuntu (MarcoFalke) 2d035b1 guix: use a temporary file over sponge (fanquake) dca7700 test: valgrind --trace-children=yes for bitcoin wrapper (MarcoFalke) dd89249 test: Remove redundant warning about missing binaries (MarcoFalke) 6993b1b test: Fix broken --valgrind handling after bitcoin wrapper (MarcoFalke) d54d7df wallet: rpc: manpage: fix example missing `fee_rate` argument (SomberNight) 412725b net: reduce log level for PCP/NAT-PMP NOT_AUTHORIZED failures (ANAVHEOBA) 85f8d28 build: avoid exporting secp256k1 symbols (Cory Fields) b7a182c doc: fix broken bpftrace installation link (jayvaliya) 1a757af ci: Print verbose build error message in test-each-commit (MarcoFalke) f5d4dc9 ci: [refactor] Allow overwriting check option in run helper (MarcoFalke) 7317a0b ci: Always print low ccache hit rate notice (MarcoFalke) 0a768d4 fuzz: Use `__AFL_SHM_ID` for naming test directories (marcofleon) 48749cf miniscript: correct and_v() properties (Antoine Poinsot) 19b3e2e test: use ModuleNotFoundError in interface_ipc.py (fanquake) Pull request description: Backports: * #34409 * #34434 * #34445 * #34453 * #34461 * #34510 * #34549 * #34554 * #34561 * #34608 * #34627 * #34671 ACKs for top commit: willcl-ark: ACK fdaf656 marcofleon: lgtm ACK fdaf656 Tree-SHA512: dc0ca9a6298519e1dc69d37985f82d87765b6857b491285e3ff77eac5870bf72c662f18ab90b35d3df595dc75f9b0762299259e53588efb7f4994797fdd07213

DrahtBot added the Tests label Feb 17, 2026

ryanofsky approved these changes Feb 18, 2026

View reviewed changes

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

ryanofsky approved these changes Feb 18, 2026

View reviewed changes

test/functional/test_framework/test_framework.py Outdated Show resolved Hide resolved

This comment was marked as abuse.

Sign in to view

DrahtBot mentioned this pull request Feb 22, 2026

qa: Facilitate debugging bitcoind inside functional tests #31723

Open

1 task

theStack approved these changes Feb 23, 2026

View reviewed changes

DrahtBot requested a review from ryanofsky February 23, 2026 16:09

ryanofsky approved these changes Feb 23, 2026

View reviewed changes

MarcoFalke added 3 commits February 24, 2026 13:15

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

fa5d478

maflcko force-pushed the 2602-test-valgrind branch from fa4c950 to fa5d478 Compare February 24, 2026 12:19

fanquake added this to the 31.0 milestone Feb 24, 2026

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

DrahtBot added CI failed and removed CI failed labels Feb 24, 2026

DrahtBot requested review from ryanofsky and theStack February 24, 2026 22:05

ryanofsky approved these changes Feb 25, 2026

View reviewed changes

ryanofsky merged commit 4035231 into bitcoin:master Feb 25, 2026
51 of 52 checks passed

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

fanquake mentioned this pull request Feb 25, 2026

[30.x] Backports #34459

Merged

maflcko deleted the 2602-test-valgrind branch February 25, 2026 10:30

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 25, 2026

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

202233a

Github-Pull: bitcoin#34608 Rebased-From: fa5d478

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 25, 2026

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

dca7700

Github-Pull: bitcoin#34608 Rebased-From: fa5d478

kevkevinpal mentioned this pull request Mar 4, 2026

test: Fix broken --valgrind handling after bitcoin wrapper kevkevinpal/bitcoin#279

Open

Conversation

maflcko commented Feb 17, 2026

Uh oh!

DrahtBot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake commented Feb 18, 2026

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Feb 18, 2026

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as abuse.

Uh oh!

theStack commented Feb 18, 2026

Uh oh!

theStack commented Feb 20, 2026

Uh oh!

ryanofsky commented Feb 20, 2026

Uh oh!

maflcko commented Feb 23, 2026

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Feb 24, 2026

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Feb 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Feb 17, 2026 •

edited

Loading

ryanofsky left a comment •

edited

Loading