Merge bitcoin#15946: Allow maintaining the blockfilterindex when using prune

84716b1 Add "index/blockfilterindex -> validation -> index/blockfilterindex" to expected circular dependencies (Jonas Schnelli)
ab3a0a2 Add functional test for blockfilterindex in prune-mode (Jonas Schnelli)
c286a22 Add debug startup parameter -fastprune for more effective pruning tests (Jonas Schnelli)
5e11226 Avoid pruning below the blockfilterindex sync height (Jonas Schnelli)
00d57ff Avoid accessing nullpointer in BaseIndex::GetSummary() (Jonas Schnelli)
6abe9f5 Allow blockfilter in conjunction with prune (Jonas Schnelli)

Pull request description:

  Maintaining the blockfilterindexes in prune mode is possible and may lead to efficient p2p based rescans of wallets (restore backups, import/sweep keys) beyond the prune height (rescans not part of that PR).

  This PR allows running the blockfilterindex(es) in conjunction with pruning.
  * Bitcoind/Qt will shutdown during startup when missing block data has been detected ([re]enable `-blockfilterindex` when we already have pruned)
  * manual block pruning is disabled during blockfilterindex sync
  * auto-pruning is delayed during blockfilterindex sync

  ToDos:
  * [x] Functional tests

ACKs for top commit:
  fjahr:
    Code review ACK 84716b1
  ryanofsky:
    Code review ACK 84716b1. Only changes since last review were suggested new FindFilesToPrune argument and test.
  benthecarman:
    tACK 84716b1

Tree-SHA512: 91d832c6c562c463f7ec7655c08956385413a99a896640b9737bda0183607fac530435d03d87c3c0e70c61ccdfe73fe8f3639bc7d26d33ca7e60925ebb97d77a

Author: jonasschnelli

src/chainparams.cpp

@@ -412,7 +412,7 @@ class CRegTestParams : public CChainParams {
         pchMessageStart[2] = 0xb5;
         pchMessageStart[3] = 0xda;
         nDefaultPort = 18444;
-        nPruneAfterHeight = 1000;
+        nPruneAfterHeight = gArgs.GetBoolArg("-fastprune", false) ? 100 : 1000;
         m_assumed_blockchain_size = 0;
         m_assumed_chain_state_size = 0;
 

src/index/base.cpp

@@ -65,6 +65,43 @@ bool BaseIndex::Init()
         m_best_block_index = g_chainman.m_blockman.FindForkInGlobalIndex(::ChainActive(), locator);
     }
     m_synced = m_best_block_index.load() == ::ChainActive().Tip();
+    if (!m_synced) {
+        bool prune_violation = false;
+        if (!m_best_block_index) {
+            // index is not built yet
+            // make sure we have all block data back to the genesis
+            const CBlockIndex* block = ::ChainActive().Tip();
+            while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
+                block = block->pprev;
+            }
+            prune_violation = block != ::ChainActive().Genesis();
+        }
+        // in case the index has a best block set and is not fully synced
+        // check if we have the required blocks to continue building the index
+        else {
+            const CBlockIndex* block_to_test = m_best_block_index.load();
+            if (!ChainActive().Contains(block_to_test)) {
+                // if the bestblock is not part of the mainchain, find the fork
+                // and make sure we have all data down to the fork
+                block_to_test = ::ChainActive().FindFork(block_to_test);
+            }
+            const CBlockIndex* block = ::ChainActive().Tip();
+            prune_violation = true;
+            // check backwards from the tip if we have all block data until we reach the indexes bestblock
+            while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
+                if (block_to_test == block) {
+                    prune_violation = false;
+                    break;
+                }
+                block = block->pprev;
+            }
+        }
+        if (prune_violation) {
+            // throw error and graceful shutdown if we can't build the index
+            FatalError("%s: %s best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)", __func__, GetName());
+            return false;
+        }
+    }
     return true;
 }
 
@@ -177,6 +214,10 @@ bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_ti
     assert(current_tip->GetAncestor(new_tip->nHeight) == new_tip);
 
     // In the case of a reorg, ensure persisted block locator is not stale.
+    // Pruning has a minimum of 288 blocks-to-keep and getting the index
+    // out of sync may be possible but a users fault.
+    // In case we reorg beyond the pruned depth, ReadBlockFromDisk would
+    // throw and lead to a graceful shutdown
     m_best_block_index = new_tip;
     if (!Commit()) {
         // If commit fails, revert the best block index to avoid corruption.
@@ -325,6 +366,6 @@ IndexSummary BaseIndex::GetSummary() const
     IndexSummary summary{};
     summary.name = GetName();
     summary.synced = m_synced;
-    summary.best_block_height = m_best_block_index.load()->nHeight;
+    summary.best_block_height = m_best_block_index ? m_best_block_index.load()->nHeight : 0;
     return summary;
 }

src/init.cpp

@@ -386,6 +386,7 @@ void SetupServerArgs(NodeContext& node)
 #endif
     argsman.AddArg("-assumevalid=<hex>", strprintf("If this block is in the chain assume that it and its ancestors are valid and potentially skip their script verification (0 to verify all, default: %s, testnet: %s, signet: %s)", defaultChainParams->GetConsensus().defaultAssumeValid.GetHex(), testnetChainParams->GetConsensus().defaultAssumeValid.GetHex(), signetChainParams->GetConsensus().defaultAssumeValid.GetHex()), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-blocksdir=<dir>", "Specify directory to hold blocks subdirectory for *.dat files (default: <datadir>)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-fastprune", "Use smaller block files and lower minimum prune height for testing purposes", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 #if HAVE_SYSTEM
     argsman.AddArg("-blocknotify=<cmd>", "Execute command when the best block changes (%s in cmd is replaced by block hash)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 #endif
@@ -1028,9 +1029,6 @@ bool AppInitParameterInteraction(const ArgsManager& args)
     if (args.GetArg("-prune", 0)) {
         if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX))
             return InitError(_("Prune mode is incompatible with -txindex."));
-        if (!g_enabled_filter_types.empty()) {
-            return InitError(_("Prune mode is incompatible with -blockfilterindex."));
-        }
     }
 
     // -bind and -whitebind can't be set when not listening

src/validation.cpp

@@ -17,6 +17,7 @@
 #include <cuckoocache.h>
 #include <flatfile.h>
 #include <hash.h>
+#include <index/blockfilterindex.h>
 #include <index/txindex.h>
 #include <logging.h>
 #include <logging/timer.h>
@@ -2240,17 +2241,25 @@ bool CChainState::FlushStateToDisk(
     {
         bool fFlushForPrune = false;
         bool fDoFullFlush = false;
+
         CoinsCacheSizeState cache_state = GetCoinsCacheSizeState(&m_mempool);
         LOCK(cs_LastBlockFile);
         if (fPruneMode && (fCheckForPruning || nManualPruneHeight > 0) && !fReindex) {
+            // make sure we don't prune above the blockfilterindexes bestblocks
+            // pruning is height-based
+            int last_prune = m_chain.Height(); // last height we can prune
+            ForEachBlockFilterIndex([&](BlockFilterIndex& index) {
+               last_prune = std::max(1, std::min(last_prune, index.GetSummary().best_block_height));
+            });
+
             if (nManualPruneHeight > 0) {
                 LOG_TIME_MILLIS_WITH_CATEGORY("find files to prune (manual)", BCLog::BENCH);
 
-                m_blockman.FindFilesToPruneManual(setFilesToPrune, nManualPruneHeight, m_chain.Height());
+                m_blockman.FindFilesToPruneManual(setFilesToPrune, std::min(last_prune, nManualPruneHeight), m_chain.Height());
             } else {
                 LOG_TIME_MILLIS_WITH_CATEGORY("find files to prune", BCLog::BENCH);
 
-                m_blockman.FindFilesToPrune(setFilesToPrune, chainparams.PruneAfterHeight(), m_chain.Height(), IsInitialBlockDownload());
+                m_blockman.FindFilesToPrune(setFilesToPrune, chainparams.PruneAfterHeight(), m_chain.Height(), last_prune, IsInitialBlockDownload());
                 fCheckForPruning = false;
             }
             if (!setFilesToPrune.empty()) {
@@ -3194,7 +3203,7 @@ static bool FindBlockPos(FlatFilePos &pos, unsigned int nAddSize, unsigned int n
 
     bool finalize_undo = false;
     if (!fKnown) {
-        while (vinfoBlockFile[nFile].nSize + nAddSize >= MAX_BLOCKFILE_SIZE) {
+        while (vinfoBlockFile[nFile].nSize + nAddSize >= (gArgs.GetBoolArg("-fastprune", false) ? 0x10000 /* 64kb */ : MAX_BLOCKFILE_SIZE)) {
             // when the undo file is keeping up with the block file, we want to flush it explicitly
             // when it is lagging behind (more blocks arrive than are being connected), we let the
             // undo block write case handle it
@@ -3925,7 +3934,7 @@ void PruneBlockFilesManual(int nManualPruneHeight)
     }
 }
 
-void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPruneAfterHeight, int chain_tip_height, bool is_ibd)
+void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPruneAfterHeight, int chain_tip_height, int prune_height, bool is_ibd)
 {
     LOCK2(cs_main, cs_LastBlockFile);
     if (chain_tip_height < 0 || nPruneTarget == 0) {
@@ -3935,7 +3944,7 @@ void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPr
         return;
     }
 
-    unsigned int nLastBlockWeCanPrune = chain_tip_height - MIN_BLOCKS_TO_KEEP;
+    unsigned int nLastBlockWeCanPrune = std::min(prune_height, chain_tip_height - static_cast<int>(MIN_BLOCKS_TO_KEEP));
     uint64_t nCurrentUsage = CalculateCurrentUsage();
     // We don't check to prune until after we've allocated new space for files
     // So we should leave a buffer under our target to account for another allocation
@@ -3986,7 +3995,7 @@ void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPr
 
 static FlatFileSeq BlockFileSeq()
 {
-    return FlatFileSeq(GetBlocksDir(), "blk", BLOCKFILE_CHUNK_SIZE);
+    return FlatFileSeq(GetBlocksDir(), "blk", gArgs.GetBoolArg("-fastprune", false) ? 0x4000 /* 16kb */ : BLOCKFILE_CHUNK_SIZE);
 }
 
 static FlatFileSeq UndoFileSeq()

src/validation.h

@@ -399,7 +399,7 @@ class BlockManager
      *
      * @param[out]   setFilesToPrune   The set of file indices that can be unlinked will be returned
      */
-    void FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPruneAfterHeight, int chain_tip_height, bool is_ibd);
+    void FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPruneAfterHeight, int chain_tip_height, int prune_height, bool is_ibd);
 
 public:
     BlockMap m_block_index GUARDED_BY(cs_main);

test/functional/feature_blockfilterindex_prune.py

@@ -0,0 +1,49 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 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 blockfilterindex in conjunction with prune."""
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_raises_rpc_error,
+    assert_greater_than,
+)
+
+class FeatureBlockfilterindexPruneTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 2
+        self.extra_args = [["-fastprune", "-prune=1"], ["-fastprune", "-prune=1", "-blockfilterindex=1"]]
+
+    def run_test(self):
+        # test basic pruning compatibility & filter access of pruned blocks
+        self.log.info("check if we can access a blockfilter when pruning is enabled but no blocks are actually pruned")
+        assert(len(self.nodes[1].getblockfilter(self.nodes[1].getbestblockhash())['filter']) > 0)
+        self.nodes[1].generate(500)
+        self.sync_all()
+        self.log.info("prune some blocks")
+        pruneheight = self.nodes[1].pruneblockchain(400)
+        assert(pruneheight != 0)
+        self.log.info("check if we can access the tips blockfilter when we have pruned some blocks")
+        assert(len(self.nodes[1].getblockfilter(self.nodes[1].getbestblockhash())['filter']) > 0)
+        self.log.info("check if we can access the blockfilter of a pruned block")
+        assert(len(self.nodes[1].getblockfilter(self.nodes[1].getblockhash(2))['filter']) > 0)
+        self.log.info("start node without blockfilterindex")
+        self.stop_node(1)
+        self.start_node(1, extra_args=self.extra_args[0])
+        self.log.info("make sure accessing the blockfilters throws an error")
+        assert_raises_rpc_error(-1,"Index is not enabled for filtertype basic", self.nodes[1].getblockfilter, self.nodes[1].getblockhash(2))
+        self.nodes[1].generate(1000)
+        self.log.info("prune below the blockfilterindexes best block while blockfilters are disabled")
+        pruneheight_new = self.nodes[1].pruneblockchain(1000)
+        assert_greater_than(pruneheight_new, pruneheight)
+        self.stop_node(1)
+        self.log.info("make sure we get an init error when starting the node again with block filters")
+        self.nodes[1].assert_start_raises_init_error(extra_args=self.extra_args[1])
+        self.nodes[1].assert_debug_log(["basic block filter index best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)"])
+        self.log.info("make sure the node starts again with the -reindex arg")
+        reindex_args = self.extra_args[1]
+        reindex_args.append("-reindex")
+        self.start_node(1, extra_args=reindex_args)
+
+if __name__ == '__main__':
+    FeatureBlockfilterindexPruneTest().main()

test/functional/test_runner.py

@@ -289,6 +289,7 @@
     'feature_help.py',
     'feature_shutdown.py',
     'p2p_ibd_txrelay.py',
+    'feature_blockfilterindex_prune.py'
     # Don't append tests at the end to avoid merge conflicts
     # Put them in a random line within the section that fits their approximate run-time
 ]

test/lint/lint-circular-dependencies.sh

@@ -11,6 +11,7 @@ export LC_ALL=C
 EXPECTED_CIRCULAR_DEPENDENCIES=(
     "chainparamsbase -> util/system -> chainparamsbase"
     "index/txindex -> validation -> index/txindex"
+    "index/blockfilterindex -> validation -> index/blockfilterindex"
     "policy/fees -> txmempool -> policy/fees"
     "qt/addresstablemodel -> qt/walletmodel -> qt/addresstablemodel"
     "qt/bitcoingui -> qt/walletframe -> qt/bitcoingui"