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.

test/functional/feature_assumeutxo.py

@@ -10,7 +10,6 @@ The assumeutxo value generated and used here is committed to in
 `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 @@ class AssumeutxoTest(BitcoinTestFramework):
         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 @@ class AssumeutxoTest(BitcoinTestFramework):
         # 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 @@ class BlocksdirTest(BitcoinTestFramework):
         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 @@ class CoinStatsIndexTest(BitcoinTestFramework):
 
         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 @@ class InitTest(BitcoinTestFramework):
             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 @@ class ReindexInitTest(BitcoinTestFramework):
         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 @@ import argparse
 from datetime import datetime, timezone
 import logging
 import os
+from pathlib import Path
 import platform
 import pdb
 import random
@@ -331,7 +332,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             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 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             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)

test/functional/test_framework/test_node.py

@@ -11,7 +11,6 @@ from enum import Enum
 import json
 import logging
 import os
-import pathlib
 import platform
 import re
 import subprocess
@@ -20,7 +19,6 @@ import time
 import urllib.parse
 import collections
 import shlex
-import shutil
 import sys
 from collections.abc import Iterable
 from pathlib import Path
@@ -163,8 +161,8 @@ class TestNode():
                 self.args.append("-ipcbind=unix")
             else:
                 # Work around default CI path exceeding maximum socket path length.
-                self.ipc_tmp_dir = pathlib.Path(tempfile.mkdtemp(prefix="test-ipc-"))
-                self.ipc_socket_path = self.ipc_tmp_dir / "node.sock"
+                self.ipc_tmp_dir = tempfile.TemporaryDirectory(prefix="test-ipc-")
+                self.ipc_socket_path = Path(self.ipc_tmp_dir.name) / "node.sock"
                 self.args.append(f"-ipcbind=unix:{self.ipc_socket_path}")
 
         if self.version_is_at_least(190000):
@@ -248,9 +246,6 @@ class TestNode():
             # this destructor is called.
             print(self._node_msg("Cleaning up leftover process"), file=sys.stderr)
             self.process.kill()
-        if self.ipc_tmp_dir:
-            print(self._node_msg(f"Cleaning up ipc directory {str(self.ipc_tmp_dir)!r}"))
-            shutil.rmtree(self.ipc_tmp_dir)
 
     def __getattr__(self, name):
         """Dispatches any unrecognised messages to the RPC connection or a CLI instance."""

test/functional/test_runner.py

@@ -681,9 +681,6 @@ def run_tests(*, test_list, build_dir, tmpdir, jobs=1, enable_coverage=False, ar
 
     if coverage:
         coverage_passed = coverage.report_rpc_coverage()
-
-        logging.debug("Cleaning up coverage data")
-        coverage.cleanup()
     else:
         coverage_passed = True
 
@@ -898,7 +895,8 @@ class RPCCoverage():
 
     """
     def __init__(self):
-        self.dir = tempfile.mkdtemp(prefix="coverage")
+        self.temp_dir = tempfile.TemporaryDirectory(prefix="coverage")
+        self.dir = self.temp_dir.name
         self.flag = '--coveragedir=%s' % self.dir
 
     def report_rpc_coverage(self):
@@ -916,9 +914,6 @@ class RPCCoverage():
             print("All RPC commands covered.")
             return True
 
-    def cleanup(self):
-        return shutil.rmtree(self.dir)
-
     def _get_uncovered_rpc_commands(self):
         """
         Return a set of currently untested RPC commands.

test/functional/wallet_backwards_compatibility.py

@@ -303,7 +303,8 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
                     # Copy back to master
                     wallet.unloadwallet()
                     if n == node:
-                        shutil.rmtree(node_master.wallets_path / wallet_name)
+                        (node_master.wallets_path / wallet_name / "wallet.dat").unlink()
+                        (node_master.wallets_path / wallet_name).rmdir()
                         shutil.copytree(
                             n.wallets_path / wallet_name,
                             node_master.wallets_path / wallet_name,

test/functional/wallet_hd.py

@@ -70,8 +70,8 @@ class WalletHDTest(BitcoinTestFramework):
         self.stop_node(1)
         # we need to delete the complete chain directory
         # otherwise node1 would auto-recover all funds in flag the keypool keys as used
-        shutil.rmtree(self.nodes[1].blocks_path)
-        shutil.rmtree(self.nodes[1].chain_path / "chainstate")
+        self.cleanup_folder(self.nodes[1].blocks_path)
+        self.cleanup_folder(self.nodes[1].chain_path / "chainstate")
         shutil.copyfile(
             self.nodes[1].datadir_path / "hd.bak",
             self.nodes[1].wallets_path / self.default_wallet_name / self.wallet_data_filename
@@ -95,8 +95,8 @@ class WalletHDTest(BitcoinTestFramework):
 
         # Try a RPC based rescan
         self.stop_node(1)
-        shutil.rmtree(self.nodes[1].blocks_path)
-        shutil.rmtree(self.nodes[1].chain_path / "chainstate")
+        self.cleanup_folder(self.nodes[1].blocks_path)
+        self.cleanup_folder(self.nodes[1].chain_path / "chainstate")
         shutil.copyfile(
             self.nodes[1].datadir_path / "hd.bak",
             self.nodes[1].wallets_path / self.default_wallet_name / self.wallet_data_filename

test/functional/wallet_migration.py

@@ -3,6 +3,7 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test Migrating a wallet from legacy to descriptor."""
+from contextlib import suppress
 from pathlib import Path
 import os.path
 import random
@@ -664,8 +665,12 @@ class WalletMigrationTest(BitcoinTestFramework):
         # Test cleanup: Clear unnamed default wallet for subsequent tests
         (self.old_node.wallets_path / "wallet.dat").unlink()
         (self.master_node.wallets_path / "wallet.dat").unlink(missing_ok=True)
-        shutil.rmtree(self.master_node.wallets_path / "default_wallet_watchonly", ignore_errors=True)
-        shutil.rmtree(self.master_node.wallets_path / "default_wallet_solvables", ignore_errors=True)
+        with suppress(FileNotFoundError):
+            (self.master_node.wallets_path / "default_wallet_watchonly" / "wallet.dat").unlink()
+            (self.master_node.wallets_path / "default_wallet_watchonly").rmdir()
+
+            (self.master_node.wallets_path / "default_wallet_solvables" / "wallet.dat").unlink()
+            (self.master_node.wallets_path / "default_wallet_solvables").rmdir()
         backup_file.unlink()
 
     def test_default_wallet(self):

test/functional/wallet_multiwallet.py

@@ -140,12 +140,12 @@ class MultiWalletTest(BitcoinTestFramework):
         self.stop_nodes()
         empty_wallet = os.path.join(self.options.tmpdir, 'empty.dat')
         os.rename(self.wallet_file(node, "empty"), empty_wallet)
-        shutil.rmtree(wallet_dir(node, "empty"))
+        os.rmdir(wallet_dir(node, "empty"))
         empty_created_wallet = os.path.join(self.options.tmpdir, 'empty.created.dat')
         os.rename(wallet_dir(node, "created", self.wallet_data_filename), empty_created_wallet)
-        shutil.rmtree(wallet_dir(node, "created"))
+        os.rmdir(wallet_dir(node, "created"))
         os.rename(self.wallet_file(node, "plain"), wallet_dir(node, "w8"))
-        shutil.rmtree(wallet_dir(node, "plain"))
+        os.rmdir(wallet_dir(node, "plain"))
 
         # restart node with a mix of wallet names:
         #   w1, w2, w3 - to verify new wallets created when non-existing paths specified

test/functional/wallet_reorgsrestore.py

@@ -72,7 +72,7 @@ class ReorgsRestoreTest(BitcoinTestFramework):
         # Stop both nodes and replace node0 chain entirely for the node1 chain
         self.stop_nodes()
         for path in ["chainstate", "blocks"]:
-            shutil.rmtree(self.nodes[0].chain_path / path)
+            self.cleanup_folder(self.nodes[0].chain_path / path)
             shutil.copytree(self.nodes[1].chain_path / path, self.nodes[0].chain_path / path)
 
         # Start node0 and verify that now it has node1 chain and no info about its previous best block

test/functional/wallet_startup.py

@@ -36,7 +36,7 @@ class WalletStartupTest(BitcoinTestFramework):
         self.nodes[0].createwallet(wallet_name=wallet_name, **kwargs)
         self.nodes[0].unloadwallet(wallet_name)
         shutil.move(self.nodes[0].wallets_path / wallet_name / "wallet.dat", self.nodes[0].wallets_path / "wallet.dat")
-        shutil.rmtree(self.nodes[0].wallets_path / wallet_name)
+        (self.nodes[0].wallets_path / wallet_name).rmdir()
 
     def run_test(self):
         self.log.info('Should start without any wallets')

test/lint/test_runner/src/lint_py.rs

@@ -73,3 +73,28 @@ pub fn lint_py_lint() -> LintResult {
         Err(e) => Err(format!("Error running `{bin_name}`: {e}")),
     }
 }
+
+pub fn lint_rmtree() -> LintResult {
+    let found = git()
+        .args([
+            "grep",
+            "--line-number",
+            "rmtree",
+            "--",
+            "test/functional/",
+            ":(exclude)test/functional/test_framework/test_framework.py",
+        ])
+        .status()
+        .expect("command error")
+        .success();
+    if found {
+        Err(r#"
+Use of shutil.rmtree() is dangerous and should be avoided.
+            "#
+        .trim()
+        .to_string())
+    } else {
+        Ok(())
+    }
+}
+

test/lint/test_runner/src/main.rs

@@ -17,7 +17,7 @@ use lint_cpp::{
     lint_boost_assert, lint_includes_build_config, lint_remove_all, lint_rpc_assert, lint_std_filesystem,
 };
 use lint_docs::{lint_doc_args, lint_doc_release_note_snippets, lint_markdown};
-use lint_py::lint_py_lint;
+use lint_py::{lint_py_lint, lint_rmtree};
 use lint_repo_hygiene::{lint_scripted_diff, lint_subtree};
 use lint_text_format::{
     lint_commit_msg, lint_tabs_whitespace, lint_trailing_newline, lint_trailing_whitespace,
@@ -52,6 +52,11 @@ fn get_linter_list() -> Vec<&'static Linter> {
             name: "py_lint",
             lint_fn: lint_py_lint,
         },
+        &Linter {
+            description: "Check that shutil.rmtree is not used",
+            name: "rmtree",
+            lint_fn: lint_rmtree,
+        },
         &Linter {
             description: "Check that std::filesystem is not used directly",
             name: "std_filesystem",