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

1. 
   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 in BasicTestingSetup::BasicTestingSetup’s constructor and destructor, and in src/test/kernel/test_kernel.cpp’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 because the kernel test code’s is RAII, it is much easier to reason about, maybe in a followup or future change BasicTestingSetup could be improved.

   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.

2. 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.

   b822ba2488
3. test: functional: drop unused --keepcache argument

   At the time this was added in  #10197, building the test cache took 21
   seconds, this is no longer useful or necessary.
   
   bitcoin/bitcoin#10197: https://github.com/bitcoin/bitcoin/pull/10197

   aee3cd72c3
4. DrahtBot added the label Tests on Jan 29, 2026
5. 
   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:

   • #34418 (qa: Make wallet_multiwallet.py Windows-compatible by hodlinator)
   • #34372 (QA: wallet_migration: Test several more weird scenarios by luke-jr)
   • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

   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.

6. 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.

   4f836713e1
7. davidgumberg force-pushed on Jan 29, 2026
8. DrahtBot added the label CI failed on Jan 29, 2026
9. 
   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.

10. DrahtBot removed the label CI failed on Jan 29, 2026
11. 
    l0rinc commented at 10:38 am on January 29, 2026: contributor

    Sounds reasonable, light concept ACK
12. 
    w0xlt commented at 6:19 pm on January 29, 2026: contributor

    Approach ACK

Contributors
davidgumberg DrahtBot l0rinc w0xlt

Labels
Tests