prune: scan and unlink already pruned block files on startup

andrewtoth commented at 4:05 pm on November 18, 2022: contributor

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.

If we call FindFilesToPrune or FindFilesToPruneManual and crash before UnlinkPrunedFiles.
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 https://github.com/bitcoin/bitcoin/commit/ccd8ef65f93ed82a87cee634660bed3ac17d9eb5).

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

DrahtBot commented at 4:05 pm on November 18, 2022: 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
ACK	pablomartin4btc, furszy, theStack, achow101
Concept ACK	hernanmarino, MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27125 (refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan)
#27039 (blockstorage: do not flush block to disk if it is already there by pinheadmz)
#26664 (refactor: make some BlockManager members const by promag)

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.

andrewtoth force-pushed on Nov 18, 2022

hernanmarino commented at 6:25 pm on November 25, 2022: contributor

Concept ACK

pablomartin4btc commented at 7:42 pm on November 25, 2022: member

cr & tested ACK. I’ve also checked a bit of the history if your proposal in all these PRs #26308, #26316 (closed in favour of this current one), #17161 and #25232.

in src/node/blockstorage.cpp:383 in 3b0427f9df outdated

378+    }
379+
380+    std::set<int> block_files_to_prune;
381+    for (int file_number = 0; file_number < m_last_blockfile; file_number++) {
382+        if (m_blockfile_info[file_number].nSize == 0
383+            && !CAutoFile(OpenBlockFile(FlatFilePos(file_number, 0), true), SER_DISK, CLIENT_VERSION).IsNull()) {

luke-jr commented at 3:37 am on November 29, 2022:

This seems likely to be somewhat I/O-heavy.

Can’t we just call unlink and ignore any errors?

maflcko commented at 3:59 pm on December 6, 2022:

0            && !AutoFile{OpenBlockFile(FlatFilePos(file_number, 0), true)}.IsNull()) {

any reason to use the deprecated legacy CAutoFile, when AutoFile should compile as well?

andrewtoth commented at 5:31 pm on December 6, 2022:

Makes sense. Done.

in src/node/blockstorage.cpp:374 in 3b0427f9df outdated

370@@ -371,6 +371,28 @@ bool BlockManager::LoadBlockIndexDB(const Consensus::Params& consensus_params)
371     return true;
372 }
373 
374+void BlockManager::ScanAndUnlinkAlreadyPrunedFiles() {

maflcko commented at 3:58 pm on December 6, 2022:

0void BlockManager::ScanAndUnlinkAlreadyPrunedFiles()
1{

nit for new code

in src/test/blockmanager_tests.cpp:72 in 3b0427f9df outdated

67+    // Check that the new tip file has not been removed
68+    const CBlockIndex* new_tip{WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip())};
69+    BOOST_CHECK_NE(old_tip, new_tip);
70+    const int new_file_number{WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return new_tip->GetBlockPos().nFile)};
71+    const FlatFilePos new_pos(new_file_number, 0);
72+    BOOST_CHECK(!CAutoFile(OpenBlockFile(new_pos, true), SER_DISK, CLIENT_VERSION).IsNull());

maflcko commented at 4:00 pm on December 6, 2022:

Same

maflcko approved

maflcko commented at 4:06 pm on December 6, 2022: member

Nice. Concept ACK

andrewtoth force-pushed on Dec 6, 2022

andrewtoth commented at 5:31 pm on December 6, 2022: contributor

@MarcoFalke done.

andrewtoth force-pushed on Dec 6, 2022

in src/test/blockmanager_tests.cpp:47 in cd136f6e55 outdated

40@@ -39,4 +41,43 @@ BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
41     BOOST_CHECK_EQUAL(actual.nPos, BLOCK_SERIALIZATION_HEADER_SIZE + ::GetSerializeSize(params->GenesisBlock(), CLIENT_VERSION) + BLOCK_SERIALIZATION_HEADER_SIZE);
42 }
43 
44+BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain100Setup)
45+{
46+    // Cap last block file size, and mine new block in a new block file.
47+    const CBlockIndex* old_tip{WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain().Tip())};

maflcko commented at 10:19 am on December 7, 2022:

0    const auto& chainman{Assert(m_node.chainman)};
1    const CBlockIndex* old_tip{WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip())};

nit, if you retouch: Could use an alias to reduce verbosity

maflcko commented at 10:21 am on December 7, 2022: member

Feel free to ignore the nit

andrewtoth force-pushed on Dec 20, 2022

andrewtoth marked this as a draft on Dec 20, 2022

andrewtoth force-pushed on Dec 20, 2022

prune: scan and unlink already pruned block files on startup 77557dda4a

test: add unit test for ScanAndUnlinkAlreadyPrunedFiles e252909e56

andrewtoth force-pushed on Dec 20, 2022

andrewtoth marked this as ready for review on Dec 20, 2022

test: add functional test for ScanAndUnlinkAlreadyPrunedFiles 3141eab9c6

andrewtoth force-pushed on Jan 5, 2023

pablomartin4btc commented at 8:03 pm on January 6, 2023: member

re-ACK with added functional test 3141eab9c669488a2e7fef5f60d356ac92294922.

in src/node/blockstorage.cpp:583 in 77557dda4a outdated

581-        LogPrint(BCLog::BLOCKSTORE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it);
582+        const bool removed_blockfile{fs::remove(BlockFileSeq().FileName(pos), ec)};
583+        const bool removed_undofile{fs::remove(UndoFileSeq().FileName(pos), ec)};
584+        if (removed_blockfile || removed_undofile) {
585+            LogPrint(BCLog::BLOCKSTORE, "Prune: %s deleted blk/rev (%05u)\n", __func__, *it);
586+        }

furszy commented at 7:38 pm on February 23, 2023:

nit: could make use of the std::error_code: e.g.

0else {
1   LogPrint(BCLog::BLOCKSTORE, "Prune: failed to removed block/undo file, error %s", ec.message());
2}

furszy approved

furszy commented at 9:27 pm on February 23, 2023: member

Code review ACK 3141eab9 Left a non-blocking nit. No need to do it.

theStack commented at 10:32 pm on February 23, 2023: contributor

Concept ACK

Looks good to me, planning to do a more thorough review tomorrow. A (non-blocking) suggestion on how to write the functional test shorter. In particular, the sync_blocks call is basically a no-op if we only have one node:

 0diff --git a/test/functional/feature_remove_pruned_files_on_startup.py b/test/functional/feature_remove_pruned_files_on_startup.py
 1index ca0e5ace9..5df101863 100755
 2--- a/test/functional/feature_remove_pruned_files_on_startup.py
 3+++ b/test/functional/feature_remove_pruned_files_on_startup.py
 4@@ -13,11 +13,9 @@ class FeatureRemovePrunedFilesOnStartupTest(BitcoinTestFramework):
 5         self.extra_args = [["-fastprune", "-prune=1"]]
 6 
 7     def mine_batches(self, blocks):
 8-        n = blocks // 250
 9-        for _ in range(n):
10+        for _ in range(blocks // 250):
11             self.generate(self.nodes[0], 250)
12         self.generate(self.nodes[0], blocks % 250)
13-        self.sync_blocks()
14 
15     def run_test(self):
16         blk0 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'blk00000.dat')
17@@ -25,10 +23,8 @@ class FeatureRemovePrunedFilesOnStartupTest(BitcoinTestFramework):
18         blk1 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'blk00001.dat')
19         rev1 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'rev00001.dat')
20         self.mine_batches(800)
21-        fo1 = os.open(blk0, os.O_RDONLY)
22-        fo2 = os.open(rev1, os.O_RDONLY)
23-        fd1 = os.fdopen(fo1)
24-        fd2 = os.fdopen(fo2)
25+        fd1 = os.fdopen(os.open(blk0, os.O_RDONLY))
26+        fd2 = os.fdopen(os.open(rev1, os.O_RDONLY))
27         self.nodes[0].pruneblockchain(600)
28 
29         # Windows systems will not remove files with an open fd

theStack approved

theStack commented at 1:52 pm on February 26, 2023: contributor

Code-review ACK 3141eab9c669488a2e7fef5f60d356ac92294922

FWIW, was curious how calling fs::remove on thousands of non-existing files would affect the performance (on mainnet, there is currently >3400 .blk/.rev files). But as expected, it’s not a big deal:

 0$ cat remove_test.cpp                            
 1#include <filesystem>
 2#include <string>
 3int main() {
 4    for (int i = 0; i < 10000; ++i) {
 5        std::string filename = "/home/thestack/" + std::to_string(i) + ".dat";
 6        std::filesystem::remove(filename);
 7    }
 8}
 9$ c++ -std=c++17 -o remove_test remove_test.cpp  
10$ time ./remove_test                                                                                                        
11    0m00.08s real     0m00.01s user     0m00.07s system

achow101 commented at 2:33 pm on February 28, 2023: member

ACK 3141eab9c669488a2e7fef5f60d356ac92294922

achow101 merged this on Feb 28, 2023

achow101 closed this on Feb 28, 2023

sidhujag referenced this in commit a844acdd3f on Feb 28, 2023

andrewtoth deleted the branch on Aug 17, 2023

Fabcien referenced this in commit 7b4e342de0 on May 6, 2024

bitcoin locked this on Aug 16, 2024

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

Reviews

Conflicts