Allow maintaining the blockfilterindex when using prune

jonasschnelli commented at 11:17 am on May 3, 2019: contributor

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:

Functional tests

jonasschnelli added the label UTXO Db and Indexes on May 3, 2019

DrahtBot commented at 1:40 pm on May 3, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20827 (During IBD, prune as much as possible until we get close to where we will eventually keep blocks by luke-jr)
#20664 (Add scanblocks RPC call by jonasschnelli)

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.

in src/index/base.cpp:74 in 0d8f7369cd outdated

71+        bool prune_violation = false;
72+        if (!m_best_block_index) {
73+            // it looks like this index is not built yet
74+            // make sure we have all data back to the genesis
75+            const CBlockIndex* block = chainActive.Tip();
76+            while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {

promag commented at 2:21 pm on May 6, 2019:

0d8f7369cd8b51dfb7b3b9b920e4e11f7962faa9

This doesn’t check status & BLOCK_HAVE_DATA on the tip, is that intentional?

MarcoFalke commented at 2:53 pm on May 6, 2019:

I don’t think the tip can ever not have data?

jonasschnelli commented at 6:06 pm on May 9, 2019:

I don’t think the tip can ever not have data?

Jup.

in src/index/base.cpp:72 in d2ab62fede outdated

69@@ -70,21 +70,21 @@ bool BaseIndex::Init()
70     if (!m_synced) {
71         bool prune_violation = false;
72         if (!m_best_block_index) {
73-            // it looks like this index is not built yet
74-            // make sure we have all data back to the genesis
75+            // index is not built yet
76+            // make sure we have all block data back to the genesis

promag commented at 2:26 pm on May 6, 2019:

d2ab62fede34b7980863617f3669c019ee42d029

These comment were added in the previous commit, could fixup these hunks?

in src/index/base.h:103 in 8bb1ca8a27 outdated

 99@@ -100,6 +100,8 @@ class BaseIndex : public CValidationInterface
100     /// not block and immediately returns false.
101     bool BlockUntilSyncedToCurrentChain();
102 
103+    bool IsSynced();

promag commented at 2:28 pm on May 6, 2019:

8bb1ca8a276f5f6fc5af3639c2d58c496b71bf8f

bool IsSyned() const;. Any reason to not write implementation here?

jonasschnelli commented at 6:06 pm on May 9, 2019:

Fixed.

in src/rpc/blockchain.cpp:1020 in bd11606a95 outdated

1003@@ -1004,6 +1004,13 @@ static UniValue pruneblockchain(const JSONRPCRequest& request)
1004 
1005     LOCK(cs_main);
1006 
1007+    // reject manual pruning in case if a blockfilterindex is syncing
1008+    ForEachBlockFilterIndex([](BlockFilterIndex& index) {

promag commented at 2:30 pm on May 6, 2019:

bd11606a95022478e6ef1ccaada1589cff13a327

Can be checked before locking cs_main?

jonasschnelli commented at 6:08 pm on May 9, 2019:

Fixed.

in src/rpc/blockchain.cpp:1022 in bd11606a95 outdated

1003@@ -1004,6 +1004,13 @@ static UniValue pruneblockchain(const JSONRPCRequest& request)
1004 
1005     LOCK(cs_main);
1006 
1007+    // reject manual pruning in case if a blockfilterindex is syncing
1008+    ForEachBlockFilterIndex([](BlockFilterIndex& index) {
1009+        if (!index.IsSynced()) {
1010+            throw JSONRPCError(RPC_MISC_ERROR, "Cannot prune blocks because blockfilterindex is syncing.");

promag commented at 2:31 pm on May 6, 2019:

bd11606a95022478e6ef1ccaada1589cff13a327

Do you see a way to test this?

jonasschnelli commented at 3:58 pm on May 9, 2019:

I’m currently working on making the block-/undo-filesize dynamic which would then allow testing this more reliable.

luke-jr commented at 4:22 pm on August 19, 2019:

Would be nicer to just check if the indexes are beyond the requested prune height…

in src/validation.cpp:2093 in bf744288bd outdated

2085@@ -2085,23 +2086,34 @@ bool static FlushStateToDisk(const CChainParams& chainparams, CValidationState &
2086     static int64_t nLastFlush = 0;
2087     std::set<int> setFilesToPrune;
2088     bool full_flush_completed = false;
2089+
2090+    // we delay pruning when one of the blockfilterindex is syncing
2091+    bool delay_pruning = false;
2092+    ForEachBlockFilterIndex([&delay_pruning](BlockFilterIndex& index) {
2093+        if (!index.IsSynced()) { delay_pruning = true; }

promag commented at 2:38 pm on May 6, 2019:

bf744288bd58718844841c088c498cd57a8f87d8

nit, no need for {}. Alternative:

0delay_pruning |= !index.IsSynced();

jonasschnelli commented at 6:08 pm on May 9, 2019:

Rather not do |= ! for code readability, but removed the extra unnecessary block

in test/lint/lint-circular-dependencies.sh:14 in bf744288bd outdated

10@@ -11,6 +11,7 @@ export LC_ALL=C
11 EXPECTED_CIRCULAR_DEPENDENCIES=(
12     "chainparamsbase -> util/system -> chainparamsbase"
13     "index/txindex -> validation -> index/txindex"
14+    "index/blockfilterindex -> validation -> index/blockfilterindex"

promag commented at 2:48 pm on May 6, 2019:

bf744288bd58718844841c088c498cd57a8f87d8

An alternative is to add a global g_delay_pruning in validation.h which gets updated every time an index m_synced is updated.

MarcoFalke commented at 2:54 pm on May 6, 2019:

Or move the “load block” function into a separate “storage” module.

jonasschnelli commented at 6:04 pm on May 9, 2019:

The global g_delay_pruning smells fragile. Better not do that.

I like the “storage” module approach which though would be something for another PR.

promag commented at 5:37 pm on May 6, 2019: member

Closes #15867?

DrahtBot added the label Needs rebase on May 7, 2019

jonasschnelli force-pushed on May 9, 2019

jonasschnelli commented at 6:10 pm on May 9, 2019: contributor

Rebased and fixed reported points.

jonasschnelli removed the label Needs rebase on May 9, 2019

jonasschnelli force-pushed on May 9, 2019

DrahtBot added the label Needs rebase on Jul 24, 2019

in src/index/base.cpp:26 in a205fc5273 outdated

22@@ -23,7 +23,7 @@ static void FatalError(const char* fmt, const Args&... args)
23     SetMiscWarning(strMessage);
24     LogPrintf("*** %s\n", strMessage);
25     uiInterface.ThreadSafeMessageBox(
26-        "Error: A fatal internal error occurred, see debug.log for details",

luke-jr commented at 4:17 pm on August 19, 2019:

Aren’t we intentionally keeping the message vague here to avoid confusing users?

in src/index/base.cpp:82 in a205fc5273 outdated

77+            }
78+            prune_violation = block != ::ChainActive().Genesis();
79+        }
80+        // in case the index has a best block set and is not fully synced
81+        // check if we have the required blocks to continue building the index
82+        else if (!(m_best_block_index.load()->nStatus & BLOCK_HAVE_DATA)) {

luke-jr commented at 4:20 pm on August 19, 2019:

IIRC, having the data for block N is not a guarantee we have it for block N+M, since blocks are stored out of order and pruned only as entire files.

luke-jr commented at 4:21 pm on August 19, 2019:

If m_best_block_index is not in the main chain, we may need its parent blocks to undo indexing. This should be checked here too.

in src/index/base.cpp:220 in a205fc5273 outdated

199@@ -178,6 +200,10 @@ bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_ti
200     assert(current_tip->GetAncestor(new_tip->nHeight) == new_tip);
201 
202     // In the case of a reorg, ensure persisted block locator is not stale.
203+    // Pruning has a minimum of 288 blocks-to-keep and getting the index
204+    // out of sync may be possible but a users fault.
205+    // In case we reorg beyond the pruned depth, ReadBlockFromDisk would
206+    // throw and led to a graceful shutdown

luke-jr commented at 4:21 pm on August 19, 2019:

lead*

in src/validation.cpp:2114 in a205fc5273 outdated

2105@@ -2105,23 +2106,34 @@ bool static FlushStateToDisk(const CChainParams& chainparams, CValidationState &
2106     static int64_t nLastFlush = 0;
2107     std::set<int> setFilesToPrune;
2108     bool full_flush_completed = false;
2109+
2110+    // we delay pruning when one of the blockfilterindex is syncing
2111+    bool delay_pruning = false;
2112+    ForEachBlockFilterIndex([&delay_pruning](BlockFilterIndex& index) {
2113+        if (!index.IsSynced()) delay_pruning = true;
2114+    });

luke-jr commented at 4:23 pm on August 19, 2019:

Won’t this effectively prevent all pruning until IBD completes? Seems important to prune as soon as all indexes are caught up to the given height…

laanwj added the label Feature on Sep 30, 2019

Roasbeef commented at 1:43 am on January 8, 2020: none

Any chance this will be revived? This is a blocker for lnd to support a pruned bitcoind node, as we want to be able to fetch filters from bitcoind, then manually fetch blocks ourselves (if bitcoind doesn’t have them) to scan them for rescans or just normal wallet sync.

prusnak commented at 3:54 pm on July 8, 2020: contributor

This feature increases the value of running a pruned node significantly. +1 for adding this!

luke-jr commented at 4:56 pm on July 8, 2020: member

I suspect #19463 could be useful for this

jonasschnelli force-pushed on Dec 10, 2020

jonasschnelli commented at 7:30 pm on December 10, 2020: contributor

Rewrote this PR and tried to fix the reported point by @promag, @MarcoFalke and @luke-jr. Added -fastprune debug parameter to test pruning more effectively (added a functional-test as well).

DrahtBot removed the label Needs rebase on Dec 10, 2020

jonasschnelli force-pushed on Dec 15, 2020

jonasschnelli added this to the milestone 22.0 on Dec 15, 2020

jonasschnelli force-pushed on Dec 15, 2020

jonasschnelli force-pushed on Dec 16, 2020

laanwj added this to the "Blockers" column in a project

in src/validation.cpp:2250 in 2541ca8df5 outdated

2245+        ForEachBlockFilterIndex([&index_lowest_blockheight, &has_blockfilter](BlockFilterIndex& index) {
2246+            int height = index.GetSummary().best_block_height;
2247+            if (height < index_lowest_blockheight) {
2248+                height = index_lowest_blockheight;
2249+            }
2250+            has_blockfilter = false;

fjahr commented at 5:08 pm on January 2, 2021:

has_blockfilter = true;?

jonasschnelli commented at 9:18 am on January 29, 2021:

Thanks. Fixed.

in src/validation.cpp:2241 in 2541ca8df5 outdated

2236@@ -2236,17 +2237,30 @@ bool CChainState::FlushStateToDisk(
2237     {
2238         bool fFlushForPrune = false;
2239         bool fDoFullFlush = false;
2240+
2241+        // make sure we don't prune below the blockfilterindexes bestblocks

fjahr commented at 5:22 pm on January 2, 2021:

I think this whole block of additions can be moved below line 2255 (if (fPruneMode...) because it’s only relevant for what is inside there.

jonasschnelli commented at 9:06 am on January 29, 2021:

Good point. Will change.

in src/validation.cpp:2248 in 2541ca8df5 outdated

2243+        int index_lowest_blockheight = m_chain.Height();
2244+        bool has_blockfilter = false;
2245+        ForEachBlockFilterIndex([&index_lowest_blockheight, &has_blockfilter](BlockFilterIndex& index) {
2246+            int height = index.GetSummary().best_block_height;
2247+            if (height < index_lowest_blockheight) {
2248+                height = index_lowest_blockheight;

fjahr commented at 5:27 pm on January 2, 2021:

should this be index_lowest_blockheight = height;?

jonasschnelli commented at 9:18 am on January 29, 2021:

Thanks. Fixed.

in src/validation.cpp:2259 in 2541ca8df5 outdated

2255         if (fPruneMode && (fCheckForPruning || nManualPruneHeight > 0) && !fReindex) {
2256             if (nManualPruneHeight > 0) {
2257                 LOG_TIME_MILLIS_WITH_CATEGORY("find files to prune (manual)", BCLog::BENCH);
2258 
2259-                m_blockman.FindFilesToPruneManual(setFilesToPrune, nManualPruneHeight, m_chain.Height());
2260+                m_blockman.FindFilesToPruneManual(setFilesToPrune, has_blockfilter ? std::max(nManualPruneHeight, index_lowest_blockheight) : nManualPruneHeight, m_chain.Height());

fjahr commented at 5:33 pm on January 2, 2021:

Shouldn’t this be std::min() because the lower height number is the one that let’s us prune less? (same below)

jonasschnelli commented at 9:18 am on January 29, 2021:

Absolutely. Fixed.

fjahr commented at 6:04 pm on January 2, 2021: member

Concept ACK although I am still thinking about how to keep the chances low that users shoot themselves in the foot and unnecessarily need to redownload the blockchain again.

It isn’t great that the changes in FlushStateToDisk() aren’t tested. I am thinking about how to make that possible.

jonasschnelli force-pushed on Jan 29, 2021

jonasschnelli commented at 9:19 am on January 29, 2021: contributor

Thanks @fjahr for the reviews. And I agree that there should be a test for the FlushStateToDisk() changes.

achow101 commented at 9:38 pm on February 8, 2021: member

Code Review ACK 0280274368e2df7c7832549cd581c710875a6be1

This is simpler than I expected it to be.

in src/validation.cpp:3215 in 0280274368 outdated

3216@@ -3203,7 +3217,7 @@ static bool FindBlockPos(FlatFilePos &pos, unsigned int nAddSize, unsigned int n
3217 
3218     bool finalize_undo = false;
3219     if (!fKnown) {
3220-        while (vinfoBlockFile[nFile].nSize + nAddSize >= MAX_BLOCKFILE_SIZE) {
3221+        while (vinfoBlockFile[nFile].nSize + nAddSize >= (gArgs.GetBoolArg("-fastprune", false) ? 0x10000 /* 64kb */ : MAX_BLOCKFILE_SIZE)) {

prusnak commented at 9:48 pm on February 8, 2021:

The numeric constant could be extracted into src/validation.h (on top of the file where MAX_BLOCKFILE_SIZE is defined).

jonasschnelli commented at 10:47 am on February 11, 2021:

I thought about it but since its a debug only parameter I rather keep that inline.

in src/validation.cpp:4007 in 0280274368 outdated

3999@@ -3986,7 +4000,7 @@ void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPr
4000 
4001 static FlatFileSeq BlockFileSeq()
4002 {
4003-    return FlatFileSeq(GetBlocksDir(), "blk", BLOCKFILE_CHUNK_SIZE);
4004+    return FlatFileSeq(GetBlocksDir(), "blk", gArgs.GetBoolArg("-fastprune", false) ? 0x4000 /* 16kb */ : BLOCKFILE_CHUNK_SIZE);

prusnak commented at 9:48 pm on February 8, 2021:

The numeric constant could be extracted to top of the file where BLOCKFILE_CHUNK_SIZE is defined.

prusnak changes_requested

in src/validation.cpp:2262 in bda1c9fc90 outdated

2259+                int height = index.GetSummary().best_block_height;
2260+                if (height < index_lowest_blockheight) {
2261+                    index_lowest_blockheight = height;
2262+                }
2263+                has_blockfilter = true;
2264+            });

ryanofsky commented at 1:08 am on February 10, 2021:

In commit “Avoid pruning below the blockfilterindex sync height” (bda1c9fc90a704d1962b16fe4242b5637c930b29)

Earlier commit can set best_block_height to 0, while FindFilesToPruneManual is asserting prune height > 0, which seems like it could lead to an unlucky crash.

Also, the has_blockfilter variable and conditionals below seem unnecessary, and the logic would work fine if it were just assumed always true.

Also, the code duplication in prune calls below also seems not great. I think this could all be more straightforward:

0int last_prune = m_chain.Height(); // last height we can prune
1ForEachBlockFilterIndex([&](BlockFilterIndex& index) {
2   last_prune = std::max(1, std::min(last_prune, index.GetSummary().block_block_height));
3});
4
5if (nManualPruneHeight > 0) {
6    FindFilesToPruneManual(setFilesToPrune, std::min(last_prune, nManualPruneHeight), m_chain.Height());
7} else {
8    FindFilesToPrune(setFilesToPrune, chainparams.PruneAfterHeight(), last_prune, m_chain.Height());
9}

jonasschnelli commented at 9:37 am on February 11, 2021:

Using last_prune instead of m_chain.Height() will lead to preserve 288 blocks below the artificial tip (last_prune) which IMO are not subject to reorg.

ryanofsky commented at 9:49 am on February 11, 2021:

Using last_prune instead of m_chain.Height() will lead to preserve 288 blocks below the artificial tip (last_prune) which IMO are not subject to reorg.

If you’re talking about the non-manual case, the you need to FindFilesToPrune a new parameter, not use the existing one, see #15946 (review). Not suggesting preserving below the tip.

jonasschnelli commented at 10:46 am on February 11, 2021:

Yes. You’re right. Lets just accept the 288blocks buffer for now (can be changed later if necessary).

in src/index/base.cpp:86 in 92b4a9cc3e outdated

81+        else {
82+            const CBlockIndex* block_to_test = m_best_block_index.load();
83+            if (!ChainActive().Contains(m_best_block_index.load())) {
84+                // if the bestblock is not part of the mainchain, find the fork
85+                // and make sure we have all data down to the fork
86+                block_to_test = ::ChainActive().FindFork(m_best_block_index.load());

ryanofsky commented at 1:18 am on February 10, 2021:

In commit “Allow blockfilter in conjunction with prune” (92b4a9cc3eca0897e455e00144a96e88b5c4ea1c)

On these two lines it’s confusing and appears racy for this code to be referring to the shared m_best_block_index.load() value when it could be just be using the private block_to_test value. Would suggest s/m_best_block_index.load()/block_to_test/

in src/index/base.cpp:92 in 92b4a9cc3e outdated

87+            }
88+            const CBlockIndex* block = ::ChainActive().Tip();
89+            prune_violation = true;
90+            // check backwards from the tip if we have all block data until we reach the indexes bestblock
91+            while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
92+                if (m_best_block_index.load() == block) {

ryanofsky commented at 1:20 am on February 10, 2021:

In commit “Allow blockfilter in conjunction with prune” (92b4a9cc3eca0897e455e00144a96e88b5c4ea1c)

This seems wrong in the fork case. Should be fixed with s/m_best_block_index.load()/block_to_test/ again

benthecarman approved

benthecarman commented at 1:28 am on February 10, 2021: contributor

ACK 0280274368e2df7c7832549cd581c710875a6be1

in src/validation.cpp:2254 in bda1c9fc90 outdated

2246@@ -2246,17 +2247,30 @@ bool CChainState::FlushStateToDisk(
2247     {
2248         bool fFlushForPrune = false;
2249         bool fDoFullFlush = false;
2250+
2251         CoinsCacheSizeState cache_state = GetCoinsCacheSizeState(&m_mempool);
2252         LOCK(cs_LastBlockFile);
2253         if (fPruneMode && (fCheckForPruning || nManualPruneHeight > 0) && !fReindex) {
2254+            // make sure we don't prune below the blockfilterindexes bestblocks

ryanofsky commented at 1:57 am on February 10, 2021:

In commit “Avoid pruning below the blockfilterindex sync height” (bda1c9fc90a704d1962b16fe4242b5637c930b29)

This comment and this commit title seem wrong, should s/below/above/

in src/validation.cpp:2273 in bda1c9fc90 outdated

2270+                m_blockman.FindFilesToPruneManual(setFilesToPrune, has_blockfilter ? std::min(nManualPruneHeight, index_lowest_blockheight) : nManualPruneHeight, m_chain.Height());
2271             } else {
2272                 LOG_TIME_MILLIS_WITH_CATEGORY("find files to prune", BCLog::BENCH);
2273 
2274-                m_blockman.FindFilesToPrune(setFilesToPrune, chainparams.PruneAfterHeight(), m_chain.Height(), IsInitialBlockDownload());
2275+                m_blockman.FindFilesToPrune(setFilesToPrune, has_blockfilter ? std::min(chainparams.PruneAfterHeight(), (uint64_t)index_lowest_blockheight) : chainparams.PruneAfterHeight(), m_chain.Height(), IsInitialBlockDownload());

ryanofsky commented at 2:09 am on February 10, 2021:

In commit “Avoid pruning below the blockfilterindex sync height” (bda1c9fc90a704d1962b16fe4242b5637c930b29)

I don’t think just passing a lower nPruneAfterHeight argument value is sufficient to avoid pruning here. I think the index_lowest_blockheight variable (last_prune variable in my previous suggestion) has to be passed as a new argument to FindFilesToPrune. If you look at the FindFilesToPrune implementation you can see it is not using nPruneAfterHeight value at all to compute nLastBlockWeCanPrune, which is the value we need to change there.

It’s also easy to see this code is wrong because it will have no effect at all if index_lowest_blockheight is bigger than chainparams.PruneAfterHeight, which is 100,000.

ryanofsky commented at 2:13 am on February 10, 2021: member

Looks close, but I think there are some bugs, see comments

92b4a9cc3eca0897e455e00144a96e88b5c4ea1c Allow blockfilter in conjunction with prune (1/6)
a779d94b709bbc063025ae3aa1b47b5a5f67eea0 Avoid accessing nullpointer in BaseIndex::GetSummary() (2/6)
bda1c9fc90a704d1962b16fe4242b5637c930b29 Avoid pruning below the blockfilterindex sync height (3/6)
e601442d541ad9e42496f511b328fae755a15555 Add debug startup parameter -fastprune for more effective pruning tests (4/6)
87ba09d0d980b6d3468df3661ecff02030255db7 Add functional test for blockfilterindex in prune-mode (5/6)
0280274368e2df7c7832549cd581c710875a6be1 Add “index/blockfilterindex -> validation -> index/blockfilterindex” to expected circular dependencies (6/6)

Allow blockfilter in conjunction with prune 6abe9f5b11

Avoid accessing nullpointer in BaseIndex::GetSummary() 00d57ff768

jonasschnelli commented at 10:44 am on February 11, 2021: contributor

Thanks a lot for testing @ryanofsky. Addressed your points (+rebased)(force pushed).

jonasschnelli force-pushed on Feb 11, 2021

in src/validation.cpp:2271 in 14c95089fe outdated

2268+                m_blockman.FindFilesToPruneManual(setFilesToPrune, std::min(last_prune, nManualPruneHeight), m_chain.Height());
2269             } else {
2270                 LOG_TIME_MILLIS_WITH_CATEGORY("find files to prune", BCLog::BENCH);
2271 
2272-                m_blockman.FindFilesToPrune(setFilesToPrune, chainparams.PruneAfterHeight(), m_chain.Height(), IsInitialBlockDownload());
2273+                m_blockman.FindFilesToPrune(setFilesToPrune, chainparams.PruneAfterHeight(), last_prune, IsInitialBlockDownload());

ryanofsky commented at 9:30 pm on February 12, 2021:

In commit “Avoid pruning below the blockfilterindex sync height” (14c95089fe9727794c1a6101d5cedaca54292bb7)

Pretending last_prune as the chain height may be safe, because the only side effect is to not prune blocks that could be pruned, but it is confusing, and also creates a difference in behavior between the manual prune case which uses the real height and this case which pretends the min index sync height is the height.

If this behavior is going to be kept, there should be an explanatory comment because it looks broken, and not obviously broken in a safe way instead of a dangerous way.

But better than adding a comment would be to make the code straightforward: Stop passing last_prune as chain_tip_height. Instead, continue passing m_chain.Height() as chain_tip_height, pass last_prune as prune_height, and assign nLastBlockWeCanPrune = min(prune_height, chain_tip_height - MIN_BLOCKS_TO_KEEP) inside FindFilesToPrune. This would be more consistent with the logic in FindFilesToPruneManual, as well as more efficient, and more obviously correct.

jonasschnelli commented at 9:31 am on February 16, 2021:

Yes. That makes sense. Implemented now as suggested by @ryanofsky.

ryanofsky approved

ryanofsky commented at 9:40 pm on February 12, 2021: member

Code review ACK e2251dfb05f79a7b52aff51773759d1929fd9ade. Only change is rebase and implementing some suggestions.

Please consider implementing the suggestion below. It is a small 2 line change which I think would make the non-manual prune code less confusing, more efficient, and more consistent with the manual prune code.

in test/functional/feature_blockfilterindex_prune.py:5 in b5c243cd52 outdated

0@@ -0,0 +1,45 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2020 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test RPC misc output."""

fjahr commented at 5:43 pm on February 14, 2021:

in b5c243cd5202ba17ed20e0ea7c14bb9e11ef2425:

Description needs an update.

jonasschnelli commented at 9:31 am on February 16, 2021:

Thanks for spotting. Fixed.

in test/functional/feature_blockfilterindex_prune.py:19 in b5c243cd52 outdated

14+        self.num_nodes = 2
15+        self.extra_args = [["-fastprune", "-prune=1"], ["-fastprune", "-prune=1", "-blockfilterindex=1"]]
16+
17+    def run_test(self):
18+        # test basic pruning compatibility & filter access of pruned blocks
19+        self.log.info("check if we can access a blockfilter when pruning is enabled but no actual pruned blocks")

fjahr commented at 5:50 pm on February 14, 2021:

in b5c243cd5202ba17ed20e0ea7c14bb9e11ef2425:

nit: “…but no blocks are actually pruned”

jonasschnelli commented at 9:31 am on February 16, 2021:

Fixed.

in test/functional/feature_blockfilterindex_prune.py:47 in b5c243cd52 outdated

38+        assert_greater_than(pruneheight_new, pruneheight)
39+        self.stop_node(1)
40+        self.log.info("make sure we get an init error when starting the node again with block filters")
41+        self.nodes[1].assert_start_raises_init_error(extra_args=self.extra_args[1])
42+        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)"])
43+

fjahr commented at 6:35 pm on February 14, 2021:

in b5c243c:

Suggested addition

0        self.log.info("make sure the node starts again with the -reindex arg")
1        reindex_args = self.extra_args[1]
2        reindex_args.append("-reindex")
3        self.start_node(1, extra_args=reindex_args)

jonasschnelli commented at 9:32 am on February 16, 2021:

Added that additional test.

fjahr commented at 6:37 pm on February 14, 2021: member

Code review ACK e2251df

I agree with @ryanofsky that his suggestion would be an improvement, at least a comment makes sense and the update to the test description noted below would be good. If you decide to make these changes I will do my best to re-review quickly.

Avoid pruning below the blockfilterindex sync height 5e112269c3

Add debug startup parameter -fastprune for more effective pruning tests c286a22f7b

Add functional test for blockfilterindex in prune-mode ab3a0a2fb9

Add "index/blockfilterindex -> validation -> index/blockfilterindex" to expected circular dependencies 84716b134e

jonasschnelli force-pushed on Feb 16, 2021

jonasschnelli commented at 9:37 am on February 16, 2021: contributor

Thanks @ryanofsky and @fjahr! Fixed the reported points. Thanks for a quick retest.

ryanofsky approved

ryanofsky commented at 9:46 pm on February 17, 2021: member

Code review ACK 84716b134e9afca2fba7731de4af1f22fa1b1518. Only changes since last review were suggested new FindFilesToPrune argument and test.

Change is neat and straightforward and I think close to ready for merge.

This should also get release notes at some point since it adds a new feature and a new error condition that wasn’t checked before.

benthecarman approved

benthecarman commented at 10:06 pm on February 17, 2021: contributor

tACK 84716b134e9afca2fba7731de4af1f22fa1b1518

fjahr commented at 10:23 pm on February 17, 2021: member

Code review ACK 84716b1

Checked that only changes since last review were small improvements on the functional test and the added parameter to FindFilesToPrune().

jonasschnelli added the label Needs release note on Feb 18, 2021

jonasschnelli merged this on Feb 18, 2021

jonasschnelli closed this on Feb 18, 2021

jonasschnelli removed this from the "Blockers" column in a project

sidhujag referenced this in commit 9dd8e3b2c5 on Feb 18, 2021

in src/validation.cpp:20 in 84716b134e

16@@ -17,6 +17,7 @@
17 #include <cuckoocache.h>
18 #include <flatfile.h>
19 #include <hash.h>
20+#include <index/blockfilterindex.h>

jnewbery commented at 10:31 am on February 23, 2021:

Is this making blockfilterindex part of validation code?

MarcoFalke referenced this in commit bf66e258a8 on Dec 13, 2021

sidhujag referenced this in commit 1fd54088ea on Dec 13, 2021

kittywhiskers referenced this in commit 089800dd47 on Feb 26, 2022

Fabcien referenced this in commit eacbda7591 on Apr 5, 2022

fanquake referenced this in commit 34ae04d775 on Apr 26, 2022

DrahtBot locked this on Aug 16, 2022

fanquake removed the label Needs release note on Sep 15, 2022

fanquake commented at 3:13 pm on September 15, 2022: member

This change was part of 22.0, removing “Needs release note”.

Allow maintaining the blockfilterindex when using prune #15946

Conflicts