Merge bitcoin#26533: prune: scan and unlink already pruned block files on startup

3141eab test: add functional test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
e252909 test: add unit test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
77557dd prune: scan and unlink already pruned block files on startup (Andrew Toth)

Pull request description:

  There are a few cases where we can mark a block and undo file as pruned in our block index, but not actually remove the files from disk.
  1. If we call `FindFilesToPrune` or `FindFilesToPruneManual` and crash before `UnlinkPrunedFiles`.
  2. If on Windows there is an open file handle to the file somewhere else when calling `fs::remove` in `UnlinkPrunedFiles` (https://en.cppreference.com/w/cpp/filesystem/remove, https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-deletefilew#remarks). This could be from another process, or if we are calling `ReadBlockFromDisk`/`ReadRawBlockFromDisk` without having a lock on `cs_main` (which has been allowed since bitcoin@ccd8ef6).

  This PR mitigates this by scanning all pruned block files on startup after `LoadBlockIndexDB` and unlinking them again.

ACKs for top commit:
  achow101:
    ACK 3141eab
  pablomartin4btc:
    re-ACK with added functional test 3141eab.
  furszy:
    Code review ACK 3141eab
  theStack:
    Code-review ACK 3141eab

Tree-SHA512: 6c73bc57838ad1b7e5d441af3c4d6bf4c61c4382e2b86485e57fbb74a61240710c0ceeceb8b4834e610ecfa3175c6955c81ea4b2285fee11ca6383f472979d8d

Author: achow101

src/node/blockstorage.cpp

@@ -400,6 +400,23 @@ bool BlockManager::LoadBlockIndexDB()
     return true;
 }
 
+void BlockManager::ScanAndUnlinkAlreadyPrunedFiles()
+{
+    AssertLockHeld(::cs_main);
+    if (!m_have_pruned) {
+        return;
+    }
+
+    std::set<int> block_files_to_prune;
+    for (int file_number = 0; file_number < m_last_blockfile; file_number++) {
+        if (m_blockfile_info[file_number].nSize == 0) {
+            block_files_to_prune.insert(file_number);
+        }
+    }
+
+    UnlinkPrunedFiles(block_files_to_prune);
+}
+
 const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data)
 {
     const MapCheckpoints& checkpoints = data.mapCheckpoints;
@@ -575,11 +592,14 @@ uint64_t BlockManager::CalculateCurrentUsage()
 
 void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune)
 {
+    std::error_code ec;
     for (std::set<int>::iterator it = setFilesToPrune.begin(); it != setFilesToPrune.end(); ++it) {
         FlatFilePos pos(*it, 0);
-        fs::remove(BlockFileSeq().FileName(pos));
-        fs::remove(UndoFileSeq().FileName(pos));
-        LogPrint(BCLog::BLOCKSTORE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it);
+        const bool removed_blockfile{fs::remove(BlockFileSeq().FileName(pos), ec)};
+        const bool removed_undofile{fs::remove(UndoFileSeq().FileName(pos), ec)};
+        if (removed_blockfile || removed_undofile) {
+            LogPrint(BCLog::BLOCKSTORE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it);
+        }
     }
 }
 

src/node/blockstorage.h

@@ -169,6 +169,13 @@ class BlockManager
     bool WriteBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
     bool LoadBlockIndexDB() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
+    /**
+     * Remove any pruned block & undo files that are still on disk.
+     * This could happen on some systems if the file was still being read while unlinked,
+     * or if we crash before unlinking.
+     */
+    void ScanAndUnlinkAlreadyPrunedFiles() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
     CBlockIndex* AddToBlockIndex(const CBlockHeader& block, const uint256& hash, CBlockIndex*& best_header,
                                  enum BlockStatus nStatus = BLOCK_VALID_TREE)
         EXCLUSIVE_LOCKS_REQUIRED(cs_main);

src/test/blockmanager_tests.cpp

@@ -12,6 +12,8 @@
 
 using node::BlockManager;
 using node::BLOCK_SERIALIZATION_HEADER_SIZE;
+using node::MAX_BLOCKFILE_SIZE;
+using node::OpenBlockFile;
 
 // use BasicTestingSetup here for the data directory configuration, setup, and cleanup
 BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup)
@@ -42,4 +44,45 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
     BOOST_CHECK_EQUAL(actual.nPos, BLOCK_SERIALIZATION_HEADER_SIZE + ::GetSerializeSize(params->GenesisBlock(), CLIENT_VERSION) + BLOCK_SERIALIZATION_HEADER_SIZE);
 }
 
+BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain100Setup)
+{
+    // Cap last block file size, and mine new block in a new block file.
+    const auto& chainman = Assert(m_node.chainman);
+    auto& blockman = chainman->m_blockman;
+    const CBlockIndex* old_tip{WITH_LOCK(chainman->GetMutex(), return chainman->ActiveChain().Tip())};
+    WITH_LOCK(chainman->GetMutex(), blockman.GetBlockFileInfo(old_tip->GetBlockPos().nFile)->nSize = MAX_BLOCKFILE_SIZE);
+    CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
+
+    // Prune the older block file, but don't unlink it
+    int file_number;
+    {
+        LOCK(chainman->GetMutex());
+        file_number = old_tip->GetBlockPos().nFile;
+        blockman.PruneOneBlockFile(file_number);
+    }
+
+    const FlatFilePos pos(file_number, 0);
+
+    // Check that the file is not unlinked after ScanAndUnlinkAlreadyPrunedFiles
+    // if m_have_pruned is not yet set
+    WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
+    BOOST_CHECK(!AutoFile(OpenBlockFile(pos, true)).IsNull());
+
+    // Check that the file is unlinked after ScanAndUnlinkAlreadyPrunedFiles
+    // once m_have_pruned is set
+    blockman.m_have_pruned = true;
+    WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
+    BOOST_CHECK(AutoFile(OpenBlockFile(pos, true)).IsNull());
+
+    // Check that calling with already pruned files doesn't cause an error
+    WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
+
+    // Check that the new tip file has not been removed
+    const CBlockIndex* new_tip{WITH_LOCK(chainman->GetMutex(), return chainman->ActiveChain().Tip())};
+    BOOST_CHECK_NE(old_tip, new_tip);
+    const int new_file_number{WITH_LOCK(chainman->GetMutex(), return new_tip->GetBlockPos().nFile)};
+    const FlatFilePos new_pos(new_file_number, 0);
+    BOOST_CHECK(!AutoFile(OpenBlockFile(new_pos, true)).IsNull());
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/validation.cpp

@@ -4693,6 +4693,8 @@ bool ChainstateManager::LoadBlockIndex()
         bool ret{m_blockman.LoadBlockIndexDB()};
         if (!ret) return false;
 
+        m_blockman.ScanAndUnlinkAlreadyPrunedFiles();
+
         std::vector<CBlockIndex*> vSortedByHeight{m_blockman.GetAllBlockIndices()};
         std::sort(vSortedByHeight.begin(), vSortedByHeight.end(),
                   CBlockIndexHeightOnlyComparator());

test/functional/feature_remove_pruned_files_on_startup.py

@@ -0,0 +1,54 @@
+#!/usr/bin/env python3
+# Copyright (c) 2022 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test removing undeleted pruned blk files on startup."""
+
+import os
+from test_framework.test_framework import BitcoinTestFramework
+
+class FeatureRemovePrunedFilesOnStartupTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+        self.extra_args = [["-fastprune", "-prune=1"]]
+
+    def mine_batches(self, blocks):
+        n = blocks // 250
+        for _ in range(n):
+            self.generate(self.nodes[0], 250)
+        self.generate(self.nodes[0], blocks % 250)
+        self.sync_blocks()
+
+    def run_test(self):
+        blk0 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'blk00000.dat')
+        rev0 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'rev00000.dat')
+        blk1 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'blk00001.dat')
+        rev1 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'rev00001.dat')
+        self.mine_batches(800)
+        fo1 = os.open(blk0, os.O_RDONLY)
+        fo2 = os.open(rev1, os.O_RDONLY)
+        fd1 = os.fdopen(fo1)
+        fd2 = os.fdopen(fo2)
+        self.nodes[0].pruneblockchain(600)
+
+        # Windows systems will not remove files with an open fd
+        if os.name != 'nt':
+            assert not os.path.exists(blk0)
+            assert not os.path.exists(rev0)
+            assert not os.path.exists(blk1)
+            assert not os.path.exists(rev1)
+        else:
+            assert os.path.exists(blk0)
+            assert not os.path.exists(rev0)
+            assert not os.path.exists(blk1)
+            assert os.path.exists(rev1)
+
+        # Check that the files are removed on restart once the fds are closed
+        fd1.close()
+        fd2.close()
+        self.restart_node(0)
+        assert not os.path.exists(blk0)
+        assert not os.path.exists(rev1)
+
+if __name__ == '__main__':
+    FeatureRemovePrunedFilesOnStartupTest().main()

test/functional/test_runner.py

@@ -402,6 +402,7 @@
     'p2p_permissions.py',
     'feature_blocksdir.py',
     'wallet_startup.py',
+    'feature_remove_pruned_files_on_startup.py',
     'p2p_i2p_ports.py',
     'p2p_i2p_sessions.py',
     'feature_config_args.py',