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

0d1301b test: functional: drop rmtree usage and add lint check (David Gumberg)
8bfb422 test: functional: drop unused --keepcache argument (David Gumberg)
a7e4a59 qa: Remove all instances of `remove_all` except test cleanup (David Gumberg)

Pull request description:

  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/4ae00e9a7183cb15877c3014bf46f2e36470c304/src/test/kernel/test_kernel.cpp#L100-L112):

  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.

ACKs for top commit:
  achow101:
    ACK 0d1301b
  hodlinator:
    ACK 0d1301b
  sedited:
    ACK 0d1301b

Tree-SHA512: da8ca23846b73eff0eaff61a5f80ba1decf63db783dcd86b25f88f4862ae28816fc9e2e9ee71283ec800d73097b1cfae64e3c5ba0e991be69c200c6098f24d6e

Author: achow101

src/bench/wallet_create.cpp

@@ -44,16 +44,19 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
     bilingual_str error_string;
     std::vector<bilingual_str> warnings;
 
-    auto wallet_path = fs::PathToString(test_setup->m_path_root / "test_wallet");
+    const auto wallet_path = test_setup->m_path_root / "test_wallet";
+    const auto wallet_name = fs::PathToString(wallet_path);
+
     bench.run([&] {
-        auto wallet = CreateWallet(context, wallet_path, /*load_on_start=*/std::nullopt, options, status, error_string, warnings);
+        auto wallet = CreateWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error_string, warnings);
         assert(status == DatabaseStatus::SUCCESS);
         assert(wallet != nullptr);
 
         // Release wallet
         RemoveWallet(context, wallet, /*load_on_start=*/ std::nullopt);
         WaitForDeleteWallet(std::move(wallet));
-        fs::remove_all(wallet_path);
+        fs::remove(wallet_path / "wallet.dat");
+        fs::remove(wallet_path);
     });
 }
 

src/test/fuzz/i2p.cpp

@@ -61,7 +61,7 @@ FUZZ_TARGET(i2p, .init = initialize_i2p)
         }
     }
 
-    fs::remove_all(private_key_path);
+    fs::remove(private_key_path);
 
     CreateSock = CreateSockOrig;
 }

src/test/kernel/test_kernel.cpp

@@ -1175,8 +1175,8 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
     BOOST_CHECK_EQUAL(count, chain.CountEntries());
 
 
-    fs::remove_all(test_directory.m_directory / "blocks" / "blk00000.dat");
+    fs::remove(test_directory.m_directory / "blocks" / "blk00000.dat");
     BOOST_CHECK(!chainman->ReadBlock(tip_2).has_value());
-    fs::remove_all(test_directory.m_directory / "blocks" / "rev00000.dat");
+    fs::remove(test_directory.m_directory / "blocks" / "rev00000.dat");
     BOOST_CHECK_THROW(chainman->ReadBlockSpentOutputs(tip), std::runtime_error);
 }

src/test/util_tests.cpp

@@ -1138,7 +1138,8 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
 #endif
     // Clean up
     ReleaseDirectoryLocks();
-    fs::remove_all(dirname);
+    fs::remove(dirname / lockname);
+    fs::remove(dirname);
 }
 
 BOOST_AUTO_TEST_CASE(test_ToLower)

test/functional/feature_assumeutxo.py

@@ -10,7 +10,6 @@
 `CRegTestParams::m_assumeutxo_data` in `src/kernel/chainparams.cpp`.
 """
 import contextlib
-from shutil import rmtree
 
 from dataclasses import dataclass
 from test_framework.blocktools import (
@@ -192,7 +191,8 @@ def expected_error(log_msg="", error_msg=""):
         expected_error(log_msg=error_details, error_msg=expected_error_msg)
 
         # resurrect node again
-        rmtree(chainstate_snapshot_path)
+        (chainstate_snapshot_path / "base_blockhash").unlink()
+        chainstate_snapshot_path.rmdir()
         self.start_node(0)
 
     def test_invalid_mempool_state(self, dump_output_path):
@@ -304,7 +304,7 @@ def test_sync_from_assumeutxo_node(self, snapshot):
         # Start test fresh by cleaning up node directories
         for node in (snapshot_node, ibd_node):
             self.stop_node(node.index)
-            rmtree(node.chain_path)
+            self.cleanup_folder(node.chain_path)
             self.start_node(node.index, extra_args=self.extra_args[node.index])
 
         # Sync-up headers chain on snapshot_node to load snapshot

test/functional/feature_blocksdir.py

@@ -5,7 +5,6 @@
 """Test the blocksdir option.
 """
 
-import shutil
 from pathlib import Path
 
 from test_framework.test_framework import BitcoinTestFramework, initialize_datadir
@@ -20,7 +19,7 @@ def run_test(self):
         self.stop_node(0)
         assert self.nodes[0].blocks_path.is_dir()
         assert not (self.nodes[0].datadir_path / "blocks").is_dir()
-        shutil.rmtree(self.nodes[0].datadir_path)
+        self.cleanup_folder(self.nodes[0].datadir_path)
         initialize_datadir(self.options.tmpdir, 0, self.chain)
         self.log.info("Starting with nonexistent blocksdir ...")
         blocksdir_path = Path(self.options.tmpdir) / 'blocksdir'

test/functional/feature_coinstatsindex_compatibility.py

@@ -49,7 +49,7 @@ def _test_coin_stats_index_compatibility(self):
 
         self.log.info("Test that gettxoutsetinfo() output is consistent for the new index running on a datadir with the old version")
         self.stop_nodes()
-        shutil.rmtree(node.chain_path / "indexes" / "coinstatsindex")
+        self.cleanup_folder(node.chain_path / "indexes" / "coinstatsindex")
         shutil.copytree(legacy_node.chain_path / "indexes" / "coinstats", node.chain_path / "indexes" / "coinstats")
         old_version_path = node.chain_path / "indexes" / "coinstats"
         msg = f'[warning] Old version of coinstatsindex found at {old_version_path}. This folder can be safely deleted unless you plan to downgrade your node to version 29 or lower.'

test/functional/feature_init.py

@@ -241,7 +241,7 @@ def start_expecting_error(err_fragment, args):
             start_expecting_error(err_fragment, startup_args)
 
             for dir in dirs:
-                shutil.rmtree(node.chain_path / dir)
+                self.cleanup_folder(node.chain_path / dir)
                 shutil.move(node.chain_path / f"{dir}_bak", node.chain_path / dir)
 
     def init_pid_test(self):

test/functional/feature_reindex_init.py

@@ -7,7 +7,6 @@
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 import os
-import shutil
 
 
 class ReindexInitTest(BitcoinTestFramework):
@@ -19,7 +18,7 @@ def run_test(self):
         self.stop_nodes()
 
         self.log.info("Removing the block index leads to init error")
-        shutil.rmtree(node.blocks_path / "index")
+        self.cleanup_folder(node.blocks_path / "index")
         node.assert_start_raises_init_error(
             expected_msg=f"Error initializing block database.{os.linesep}"
             "Please restart with -reindex or -reindex-chainstate to recover.",

test/functional/test_framework/test_framework.py

@@ -10,6 +10,7 @@
 from datetime import datetime, timezone
 import logging
 import os
+from pathlib import Path
 import platform
 import pdb
 import random
@@ -331,7 +332,7 @@ def shutdown(self):
             h.flush()
             rpc_logger.removeHandler(h)
         if cleanup_tree_on_exit:
-            shutil.rmtree(self.options.tmpdir)
+            self.cleanup_folder(self.options.tmpdir)
 
         self.nodes.clear()
         return exit_code
@@ -1031,3 +1032,9 @@ def inspect_sqlite_db(self, path, fn, *args, **kwargs):
             return result
         except ImportError:
             self.log.warning("sqlite3 module not available, skipping tests that inspect the database")
+
+    def cleanup_folder(self, _path):
+        path = Path(_path)
+        if not path.is_relative_to(self.options.tmpdir):
+            raise AssertionError(f"Trying to delete #{path} outside of #{self.options.tmpdir}")
+        shutil.rmtree(path)