qa: Drop recursive deletes from test code, add lint checks.

davidgumberg commented at 3:59 am on January 29, 2026: contributor

Both fs::remove_all and shutil::rmtree() are a bit dangerous, and most of their uses are not necessary, this PR removes most instances of both.

remove_all() is still used in in src/test/util/setup_common.cpp as part of BasicTestingSetup::BasicTestingSetup’s constructor and destructor, and it is used in the kernel test code’s TestDirectory:

https://github.com/bitcoin/bitcoin/blob/734899a4c404f24379d0597744465c147d9ad024/src/test/kernel/test_kernel.cpp#L100-L112

In both cases, remove_all is likely necessary, but the kernel’s test code is RAII, ideally BasicTestingSetup could be made similar in a follow-up or in this PR if reviewers think it is important.

Similarly in the python code, most usage was unnecessary, but there are a few places where rmtree() was necessary, I have added sanity checks to make sure these are inside of the tmpdir before doing recursive delete there.

DrahtBot added the label Tests on Jan 29, 2026

DrahtBot commented at 4:00 am on January 29, 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
Concept ACK	l0rinc
Approach ACK	w0xlt

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:

#34603 (wallet: Fix detection of symlinks on Windows by achow101)
#34418 (qa: Make wallet_multiwallet.py Windows crossbuild-compatible by hodlinator)
#34372 (QA: wallet_migration: Test several more weird scenarios by luke-jr)

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.

davidgumberg force-pushed on Jan 29, 2026

DrahtBot added the label CI failed on Jan 29, 2026

DrahtBot commented at 4:05 am on January 29, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21465095038/job/61825339241 LLM reason (✨ experimental): Lint check failed due to Ruff error: unused import shutil in test/functional/test_framework/test_node.py.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Jan 29, 2026

l0rinc commented at 10:38 am on January 29, 2026: contributor

Sounds reasonable, light concept ACK

w0xlt commented at 6:19 pm on January 29, 2026: contributor

Approach ACK

sedited approved

sedited commented at 10:47 am on March 8, 2026: contributor

lgtm

The changes to the cache directory logic in the last two commits were not that straight forward to understand for me. Can you elaborate on them a bit in the commit messages?

DrahtBot added the label Needs rebase on Mar 9, 2026

davidgumberg force-pushed on Mar 10, 2026

davidgumberg commented at 11:33 pm on March 10, 2026: contributor

The changes to the cache directory logic in the last two commits were not that straight forward to understand for me. Can you elaborate on them a bit in the commit messages?

Thanks for the feedback, I’ve (hopefully) improved the commit message for the one where I drop the --keepcache argument, and I’ve moved changes that I accidentally included in the last commit in the series back to their true home in that commit.

DrahtBot removed the label Needs rebase on Mar 10, 2026

DrahtBot added the label CI failed on Mar 11, 2026

qa: Remove all instances of `remove_all` except test cleanup

Adds a lint check for `remove_all()`

`fs::remove_all()`/`std::filesystem::remove_all()` is extremely
dangerous, all user-facing instances of it have been removed, and it
also deserves to be removed from the places in our test code where it is
being used unnecessarily.

63d5aef8b4

test: functional: drop unused --keepcache argument

At the time this was added in  #10197, building the test cache took 21
seconds, as described in that PR, but this is no longer true, as
demonstrated by running the functional test framework with and without
the --keepcache arguments on master prior to this commit:

```
hyperfine   --warmup 1  --export-markdown results.md --runs 3 \
    -n 'without --keepcache'    './build/test/functional/test_runner.py -j $(nproc)' \
    -n 'with --keepcache'       './build/test/functional/test_runner.py -j $(nproc) --keepcache'
```

| Command               | Mean [s]       | Min [s] | Max [s] | Relative |
|:----------------------|---------------:|--------:|---:|---:|
| `without --keepcache` | 76.373 ± 3.058 | 74.083  | 79.846  | 1.00 |
| `with --keepcache`    | 77.384 ± 1.836 | 75.952  | 79.454  | 1.01 ± 0.05 |

As a consequence, this argument can be removed from the test runner and
this also has the benefit of being able to use an RAII-like
`tempfile.TemporaryDirectory` instead of having to clean up the cache
manually at the end of test runs.

bitcoin/bitcoin#10197: https://github.com/bitcoin/bitcoin/pull/10197

b0b53abe19

test: functional: drop rmtree usage and add lint check

`shutil.rmtree` is dangerous because it recursively deletes. There are
not likely to be any issues with it's current uses, but it is possible
that some of the assumptions being made now won't always be true, e.g.
about what some of the variables being passed to `rmtree` represent.

For some remaining uses of rmtree that can't be avoided for now, use
`cleanup_dir` which asserts that the recursively deleted folder is a
child of the the `tmpdir` of the test run. Otherwise,
`tempfile.TemporaryDirectory` should be used which does it's own
deleting on being garbage collected, or old fashioned unlinking and
rmdir in the case of directories with known contents.

548dc8137a

davidgumberg force-pushed on Mar 12, 2026

DrahtBot removed the label CI failed on Mar 12, 2026

DrahtBot added the label Needs rebase on Mar 13, 2026

DrahtBot commented at 11:45 pm on March 13, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

qa: Drop recursive deletes from test code, add lint checks. #34439

Reviews

Conflicts