Improve Indices on pruned nodes via prune blockers #21726

pull fjahr wants to merge 6 commits into bitcoin:master from fjahr:prune_improve changing 15 files +265 −116
  1. fjahr commented at 11:56 pm on April 18, 2021: contributor

    Motivation

    The main motivation of this change and only behavior change noticeable by user is to allow running coinstatsindex on pruned nodes as has been requested here for example.

    Background

    coinstatsindex on pruned nodes can be enabled in a much simpler than it is done here but it comes with downside. The ability to run blockfilterindexon pruned nodes was added in #15946 but it also added the blockfilterindex as a dependency to validation and it introduced two new circular dependencies. Enabling coinstatsindex on pruned nodes in a similar way would add it as a dependency as well and introduce another circular dependency.

    Instead, this PR introduces a m_prune_blockers map to BlockManager as a flexible approach to block pruning. Entities like blockfilterindex, for example, can add a key and a height to block pruning over that height. These entities need to update that value to allow more pruning when they are ready.

    Alternative approach

    Upon completing the first draft of this PR I found #19463 as an alternative that follows the same but follows a very different approach. I am listing the main differences here as I see them:

    • Usage of globals
    • Blocks pruning with a start and a stop height
    • Can persist blockers across restarts
    • Blockers can be set/unset via RPCs

    Personally, I don’t think any of these are necessary to be added here but if the general approach or specific features are more appealing to reviewers I am happy to change to a solution based on that PR or port over specific parts of it here.

  2. fjahr force-pushed on Apr 19, 2021
  3. DrahtBot added the label RPC/REST/ZMQ on Apr 19, 2021
  4. DrahtBot added the label UTXO Db and Indexes on Apr 19, 2021
  5. DrahtBot added the label Validation on Apr 19, 2021
  6. DrahtBot commented at 3:52 am on April 19, 2021: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24956 (Call CHECK_NONFATAL only once where needed by MarcoFalke)
    • #24865 (rpc: Enable wallet import on pruned nodes and add test by aureleoules)
    • #24595 (deploymentstatus: move g_versionbitscache global to ChainstateManager by ajtowns)
    • #24539 (Add a “tx output spender” index by sstone)
    • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
    • #24150 (refactor: move index class members from protected to private by jonatack)
    • #22564 (refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager by dongcarl)

    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.

  7. in src/validation.cpp:1339 in 23fb6feec6 outdated
    1333@@ -1335,6 +1334,14 @@ int BlockManager::GetSpendHeight(const CCoinsViewCache& inputs)
    1334     return pindexPrev->nHeight + 1;
    1335 }
    1336 
    1337+const CBlockIndex* BlockManager::GetLastPruneBlock(const CBlockIndex* start_block) {
    1338+    const CBlockIndex* block = start_block;
    1339+    CHECK_NONFATAL(block);
    


    MarcoFalke commented at 5:49 am on April 19, 2021:
    This function doesn’t use any member of BlockManager, so could be stand-alone? Also, I think you can avoid this check by having the caller pass a const CBlockIndex& start_block.

    fjahr commented at 6:55 pm on May 13, 2021:
    Thanks! I made the changes as suggested.
  8. fjahr force-pushed on May 13, 2021
  9. laanwj added this to the "Blockers" column in a project

  10. fjahr marked this as ready for review on May 13, 2021
  11. fjahr force-pushed on May 13, 2021
  12. fjahr renamed this:
    Add prune blockers to BlockManager
    Improve Indices on pruned nodes via prune blockers
    on May 13, 2021
  13. fjahr force-pushed on May 13, 2021
  14. laanwj commented at 6:27 pm on May 17, 2021: member
    Concept ACK, I think this is an elegant approach.
  15. in src/validation.cpp:3672 in 5d23ddf5ff outdated
    3672@@ -3663,6 +3673,10 @@ void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPr
    3673            nLastBlockWeCanPrune, count);
    3674 }
    3675 
    3676+void BlockManager::UpdatePruneBlocker(std::string name, const CBlockIndex* block) {
    


    achow101 commented at 7:42 pm on May 24, 2021:

    In 5d23ddf5ff3bd1e5d4f6b85440d9ad0c59bb9994 “Add prune blockers to BlockManager”

    Since this has EXCLUSIVE_LOCKS_REQUIRED, I think it should also have AssertLockHeld in here too.


    fjahr commented at 10:13 pm on May 24, 2021:
    Right, fixed!
  16. achow101 commented at 7:50 pm on May 24, 2021: member
    ACK 11f01efadf8e93f042ebd05d12f1f3bc5bce8178
  17. fjahr force-pushed on May 24, 2021
  18. in src/node/blockstorage.cpp:54 in bfe9719e29 outdated
    50@@ -51,6 +51,14 @@ bool IsBlockPruned(const CBlockIndex* pblockindex)
    51     return (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
    52 }
    53 
    54+const CBlockIndex* GetLastPruneBlock(const CBlockIndex& start_block) {
    


    promag commented at 11:11 pm on May 24, 2021:

    bfe9719e29d7947e2ffa9b510cd078e8a09b5d03

    nit, GetLastPrunedBlock?


    fjahr commented at 11:36 pm on May 26, 2021:
    done
  19. in src/validation.h:455 in b5a239390f outdated
    449@@ -450,6 +450,10 @@ class BlockManager
    450      */
    451     int GetSpendHeight(const CCoinsViewCache& inputs) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    452 
    453+    std::unordered_map<std::string, const CBlockIndex*> m_prune_blockers;
    454+
    455+    void UpdatePruneBlocker(std::string name, const CBlockIndex* block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
    


    promag commented at 11:13 pm on May 24, 2021:

    b5a239390fd1b31c7e6107a2e6c35658a2b4697c

    nit, const std::string&.


    fjahr commented at 11:36 pm on May 26, 2021:
    done
  20. in src/validation.cpp:2045 in b5a239390f outdated
    2040@@ -2041,6 +2041,16 @@ bool CChainState::FlushStateToDisk(
    2041                last_prune = std::max(1, std::min(last_prune, index.GetSummary().best_block_height));
    2042             });
    2043 
    2044+            for (auto const& blocker : m_blockman.m_prune_blockers) {
    2045+                if (blocker.second) {
    


    promag commented at 11:15 pm on May 24, 2021:

    b5a239390fd1b31c7e6107a2e6c35658a2b4697c

    How can it be nullptr?


    fjahr commented at 11:36 pm on May 26, 2021:
    I guess it should never be nullptr. Dropped that if statement.
  21. in src/validation.cpp:3903 in b5a239390f outdated
    3672@@ -3663,6 +3673,11 @@ void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPr
    3673            nLastBlockWeCanPrune, count);
    3674 }
    3675 
    3676+void BlockManager::UpdatePruneBlocker(std::string name, const CBlockIndex* block) {
    3677+    AssertLockHeld(::cs_main);
    3678+    m_prune_blockers[name] = block;
    


    promag commented at 11:16 pm on May 24, 2021:

    b5a239390fd1b31c7e6107a2e6c35658a2b4697c

    Should check/assert it only goes forward when updating?


    fjahr commented at 11:36 pm on May 26, 2021:

    I am not sure. The way I use it right now it can also go down with reorgs. I am undecided what the right course is. Only going up avoids a potential footgun, with going down it could react to (highly unlikely) deep reorg. If I add this I think I would do it so that setting a lower number is simply a no op:

    0if (m_prune_blockers[name] < block) m_prune_blockers[name] = block;
    

    promag commented at 11:41 pm on May 26, 2021:
    But what should happen if the block is already pruned?

    fjahr commented at 8:46 pm on June 1, 2021:
    Hm, that would mean the node would have to restart with -reindex anyway, right?

    ryanofsky commented at 5:23 pm on June 2, 2021:

    In commit “Add prune blockers to BlockManager” (9f8d6a9a8f7d113ae12306c72a773830b5725e37):

    re: #21726 (review)

    Hm, that would mean the node would have to restart with -reindex anyway, right?

    It would be nice to have a test for this case if there isn’t one. does seem like like if there was reorg and a block needed by the index was pruned, the index should be responsible for handling the error when the rewind method is called. I guess the index could just disable itself or stop the node at that point.

    But for this UpdatePruneBlocker method, it would seem too strict to never allow a blocker to move backwards, because moving an existing blocker backwards seems no worse of a problem than adding new blocker at a lower minimum height. Maybe it could be useful to have a sanity check here which asserts that whenever a new minimum height is set in the map, that no blocks in the (new_min_height, prev_min_height] range have been pruned. But this doesn’t seem strictly necessary either if we are confident that indexes compatible with pruning are able to handle pruned block errors safely when they are syncing or handling reorgs


    fjahr commented at 11:37 pm on September 5, 2021:

    Yes, I added functional tests for the -reindex case.

    I am looking into the sanity check assert idea don’t have a clear opinion yet.

  22. in src/index/base.cpp:373 in d18677006d outdated
    368+void BaseIndex::SetBestBlockIndex(const CBlockIndex* block) {
    369+    m_best_block_index = block;
    370+    if (AllowPrune()) {
    371+        LOCK(::cs_main);
    372+        if (!block) {
    373+            g_chainman.m_blockman.UpdatePruneBlocker(GetName(), ::ChainActive().Genesis());
    


    promag commented at 11:18 pm on May 24, 2021:

    d18677006d3d48ca7654039ea6e267164a2690e0

    nit, use ternary? block ? block : ::ChainActive().Genesis()


    fjahr commented at 11:36 pm on May 26, 2021:
    done
  23. promag commented at 11:22 pm on May 24, 2021: member
    Concept ACK.
  24. fjahr force-pushed on May 26, 2021
  25. fjahr commented at 11:41 pm on May 26, 2021: contributor

    Addressed comments by @promag , thank you!

    I also fixed a comment I had overlooked and added a new commit at the end because I realized that we can skip the pruning stuff in index/base if the node is not actually pruning. With the new behavior especially this prevents locking cs_main. Happy to squash this but I am still checking that this is 100% safe and want to make sure it is looked at separately because it was not included in the existing concept acks.

  26. promag commented at 11:48 pm on May 26, 2021: member
    How about keeping some blocks before the deepest prune blocker?
  27. DrahtBot added the label Needs rebase on Jun 1, 2021
  28. fjahr commented at 11:17 pm on June 1, 2021: contributor

    How about keeping some blocks before the deepest prune blocker?

    Interesting. That should help make it a bit more robust. I added this but set it pretty low for now (10), looking for feedback if it should be higher. I also encapsulated the logic for getting the deepest prune height into it’s own function to not have more logic in FlushStateToDisk.

    Also rebased.

  29. fjahr force-pushed on Jun 1, 2021
  30. DrahtBot removed the label Needs rebase on Jun 2, 2021
  31. in src/rpc/blockchain.cpp:1075 in 292d508e95 outdated
    1073-    CHECK_NONFATAL(block);
    1074-    while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
    1075-        block = block->pprev;
    1076-    }
    1077-    return uint64_t(block->nHeight);
    1078+    const CBlockIndex* last_block = GetLastPrunedBlock(*active_chain.Tip());
    


    ryanofsky commented at 3:46 pm on June 2, 2021:

    In commit “refactor: Introduce GetLastPruneBlock helper function” (292d508e956bfe5b05e6ee685415063c2a0f04af)

    Note: Here and in the other location below there’s no more CHECK_NONFATAL so there would be segfault instead of an exception if the tip was null. Doesn’t seem like a problem, but if were for some reason, GetLastPrunedBlock argument could be a pointer instead of a reference and the check could be added there.


    fjahr commented at 11:30 pm on September 5, 2021:
    True, I changed the function to take a pointer and added a CHECK_NONFATAL to be more consistent with the previous code.
  32. ryanofsky commented at 3:49 pm on June 2, 2021: contributor

    Approach ACK and started review (will update list below with progress).

    • 292d508e956bfe5b05e6ee685415063c2a0f04af refactor: Introduce GetLastPruneBlock helper function (1/7)
    • 9f8d6a9a8f7d113ae12306c72a773830b5725e37 Add prune blockers to BlockManager (2/7)
    • c07a567835ca3232fa30d4e5306592e23d55dacd Index: Use prune blockers for blockfilterindex (3/7)
    • 395757f3080b96baec65e42cb5e56051a17f2241 Index: Allow coinstatsindex with pruning enabled (4/7)
    • 6aad8676952f2a854e9f8bf8650bb7768d3e1cd1 test: Update test for indices on pruned nodes (5/7)
    • 596f738ec33efb40e6e2eee27356e493e85bf8ed move-only: Rename index + pruning functional test (6/7)
    • e365e87a72cc49e175b31256a11a6c5b1621d1f2 Index: Skip pruning checks when node is not pruning (7/7)
  33. in src/validation.h:499 in 9f8d6a9a8f outdated
    496@@ -494,6 +497,12 @@ class BlockManager
    497      */
    498     int GetSpendHeight(const CCoinsViewCache& inputs) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    499 
    500+    std::unordered_map<std::string, const CBlockIndex*> m_prune_blockers;
    


    ryanofsky commented at 4:04 pm on June 2, 2021:

    In commit “Add prune blockers to BlockManager” (9f8d6a9a8f7d113ae12306c72a773830b5725e37)

    Would be good to have a descriptive comment like:

    0//! Map from external index name to most recent block the index can tolerate being pruned.
    1//!
    2//! [@note](/bitcoin-bitcoin/contributor/note/) Internally, only blocks at height (block->nHeight + PRUNE_BLOCKER_BUFFER) and
    3//! below will be pruned, but callers should avoid assuming any particular buffer size.
    

    fjahr commented at 11:01 pm on September 5, 2021:
    Added!
  34. in src/validation.cpp:3802 in 9f8d6a9a8f outdated
    3793@@ -3792,6 +3794,21 @@ void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPr
    3794            nLastBlockWeCanPrune, count);
    3795 }
    3796 
    3797+void BlockManager::UpdatePruneBlocker(const std::string& name, const CBlockIndex* block) {
    3798+    AssertLockHeld(::cs_main);
    3799+    m_prune_blockers[name] = block;
    3800+}
    3801+
    3802+void BlockManager::GetLastPruneBlockerHeight(int& last_prune) {
    


    ryanofsky commented at 4:12 pm on June 2, 2021:

    In commit “Add prune blockers to BlockManager” (9f8d6a9a8f7d113ae12306c72a773830b5725e37)

    Just a style suggestion, so feel free to ignore, but usage of this function seems awkward to understand with last_prune being an input and an output parameter. I think FlushStateToDisk would be easier to read with the code in this function just inlined there (which is the only place where it is called).


    fjahr commented at 11:01 pm on September 5, 2021:
    Done
  35. in src/validation.cpp:3807 in 9f8d6a9a8f outdated
    3802+void BlockManager::GetLastPruneBlockerHeight(int& last_prune) {
    3803+    for (auto const& blocker : m_prune_blockers) {
    3804+        const int blocker_height = blocker.second->nHeight + PRUNE_BLOCKER_BUFFER;
    3805+        last_prune = std::max(1, std::min(last_prune, blocker_height));
    3806+        if (last_prune == blocker_height) {
    3807+            LogPrintf("%s limited pruning to height %d\n", blocker.first, blocker_height);
    


    ryanofsky commented at 6:06 pm on June 2, 2021:

    In commit “Add prune blockers to BlockManager” (9f8d6a9a8f7d113ae12306c72a773830b5725e37):

    Would it make sense to use a log category here LogPrint(BCLog::PRUNE, ...) instead of printing unconditionally? Also in the future it could be nice to unify this print with the prints in the FindFilesToPrune functions and print the full pruning status in a consistent way on just one line.


    fjahr commented at 11:01 pm on September 5, 2021:
    Yeah, I agree, I added a log category. I am leaving unifying for a potential follow-up to not complicate this PR further.
  36. in src/validation.cpp:3800 in e365e87a72 outdated
    3795+    m_prune_blockers[name] = block;
    3796+}
    3797+
    3798+void BlockManager::GetLastPruneBlockerHeight(int& last_prune) {
    3799+    for (auto const& blocker : m_prune_blockers) {
    3800+        const int blocker_height = blocker.second->nHeight + PRUNE_BLOCKER_BUFFER;
    


    dergoegge commented at 10:15 am on June 19, 2021:
    0        const int blocker_height = blocker.second->nHeight - PRUNE_BLOCKER_BUFFER;
    

    Shouldn’t we subtract here if we want to keep PRUNE_BLOCKER_BUFFER blocks before the deepest blocker?

    from the logs of a test run:

    0node1 2021-06-19T12:10:06.022496Z [init] [validation.cpp:3803] [GetLastPruneBlockerHeight] coinstatsindex limited pruning to height 10
    1...
    2node1 2021-06-19T12:10:06.044329Z [coinstatsindex] [index/base.cpp:159] [ThreadSync] Syncing coinstatsindex with block chain from height 0
    

    node1 limited pruning to height 10 before it started syncing from height 0.


    fjahr commented at 10:58 pm on September 5, 2021:
    right 🙄 thanks, fixed!
  37. in test/functional/feature_index_prune.py:69 in e365e87a72 outdated
    57+        for node in stats_nodes:
    58+            assert(node.gettxoutsetinfo(hash_type="muhash", hash_or_height=tip)['muhash'])
    59+
    60+        # Mine two batches of blocks to avoid hitting NODE_NETWORK_LIMITED_MIN_BLOCKS disconnection
    61+        self.mine_batches(2)
    62+        self.sync_index(height=700)
    


    dergoegge commented at 12:22 pm on June 19, 2021:

    Wouldn’t it make more sense to wait for the indexes to sync after pruning? Otherwise it is pretty much guaranteed that the index data at the tip (700) is available after pruning.

    In general i think it would be good for the test to check for expected prune blocker heights somehow. (not sure if that is possible, i dont know that much about the functional tests)


    fjahr commented at 11:05 pm on September 5, 2021:

    Wouldn’t it make more sense to wait for the indexes to sync after pruning? Otherwise it is pretty much guaranteed that the index data at the tip (700) is available after pruning.

    Yes, that would be good, but unless I am misunderstanding your comment, what we would be testing then is a race condition which would make the test flaky.

    In general i think it would be good for the test to check for expected prune blocker heights somehow. (not sure if that is possible, i dont know that much about the functional tests)

    yes, I added two sanity checks for the exact prune blocker height by checking the logs

  38. dergoegge commented at 12:31 pm on June 19, 2021: member
    Concept ACK
  39. in src/index/base.cpp:374 in e365e87a72 outdated
    365@@ -370,3 +366,11 @@ IndexSummary BaseIndex::GetSummary() const
    366     summary.best_block_height = m_best_block_index ? m_best_block_index.load()->nHeight : 0;
    367     return summary;
    368 }
    369+
    370+void BaseIndex::SetBestBlockIndex(const CBlockIndex* block) {
    371+    m_best_block_index = block;
    372+    if (AllowPrune() && (fPruneMode || fHavePruned) && !fReindex) {
    373+        LOCK(::cs_main);
    374+        g_chainman.m_blockman.UpdatePruneBlocker(GetName(), block ? block : ::ChainActive().Genesis());
    


    jonatack commented at 1:50 pm on June 28, 2021:

    Error building this branch at e365e87a72cc4 rebased to current master at 8cdf91735f2b, looks like it needs updating:

    0index/base.cpp:373:9: error: use of undeclared identifier 'g_chainman'
    1        g_chainman.m_blockman.UpdatePruneBlocker(GetName(), block ? block : ::ChainActive().Genesis());
    2        ^
    3index/base.cpp:373:79: error: no member named 'ChainActive' in the global namespace
    4        g_chainman.m_blockman.UpdatePruneBlocker(GetName(), block ? block : ::ChainActive().Genesis());
    5                                                                            ~~^
    62 errors generated.
    

    fjahr commented at 11:02 pm on September 5, 2021:
    Thanks, fixed.
  40. jonatack commented at 10:15 am on July 20, 2021: contributor
    Nudge to update.
  41. MarcoFalke removed this from the "Blockers" column in a project

  42. MarcoFalke commented at 2:38 pm on July 20, 2021: member
    Removed from high-prio for now due to inactivity, but can be added back
  43. DrahtBot added the label Needs rebase on Jul 28, 2021
  44. fjahr force-pushed on Sep 5, 2021
  45. fjahr force-pushed on Sep 5, 2021
  46. fjahr commented at 11:38 pm on September 5, 2021: contributor
    Sorry for the long wait, I should have addressed all the feedback for now and also optimized the test a bit further.
  47. DrahtBot removed the label Needs rebase on Sep 5, 2021
  48. fjahr force-pushed on Sep 6, 2021
  49. ryanofsky commented at 2:46 pm on September 7, 2021: contributor

    Sorry for the long wait, I should have addressed all the feedback for now and also optimized the test a bit further.

    Thanks for updating this! I want to review it more soon, and it would be great to have feedback also from @jamesob because I think there is or could be some interaction between this and #15606#pullrequestreview-692965905:

    • More straightforward approach to pruning. Instead of having to write special case pruning logic, this could be integrated with [Improve Indices on pruned nodes via prune blockers #21726](https://github.com/bitcoin/bitcoin/pull/21726), and each chainstate could just add to a list of prune blockers (ranges of blocks not allowed to be pruned) and pruning code would not need to be aware of assumeutxo states.
  50. jamesob commented at 3:09 pm on September 7, 2021: member

    it would be great to have feedback also from @jamesob

    Yep, will plan to review this in the next few days.

  51. DrahtBot added the label Needs rebase on Sep 9, 2021
  52. in src/index/base.cpp:75 in 2c815adc88 outdated
    71@@ -72,11 +72,7 @@ bool BaseIndex::Init()
    72         if (!m_best_block_index) {
    73             // index is not built yet
    74             // make sure we have all block data back to the genesis
    75-            const CBlockIndex* block = active_chain.Tip();
    76-            while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
    77-                block = block->pprev;
    78-            }
    79-            prune_violation = block != active_chain.Genesis();
    80+            prune_violation = GetLastPrunedBlock(active_chain.Tip()) != active_chain.Genesis();
    


    jamesob commented at 1:53 pm on September 9, 2021:

    https://github.com/bitcoin/bitcoin/pull/21726/commits/2c815adc881fa90f14887e2ec33ccb11ef964387

    Note: ensured active_chain.Tip() will not ever be nullptr here because g_txindex et al are started in init.cpp after genesis block load.

  53. in src/validation.h:499 in 9c70676ade outdated
    494+     * Map from external index name to most recent block the index can tolerate being pruned.
    495+     *
    496+     * @note Internally, only blocks at height (block->nHeight - PRUNE_BLOCKER_BUFFER) and
    497+     * below will be pruned, but callers should avoid assuming any particular buffer size.
    498+     */
    499+    std::unordered_map<std::string, const CBlockIndex*> m_prune_blockers;
    


    jamesob commented at 2:13 pm on September 9, 2021:

    https://github.com/bitcoin/bitcoin/pull/21726/commits/9c70676aded6e2c9c4719fd5039f27a2c16a54fb

    I’m curious why the method below is marked as requiring cs_main, but this data structure isn’t marked as guarded by cs_main - is that intentional?


    fjahr commented at 9:59 pm on September 22, 2021:
    Not intentional, just an oversight. Fixed!
  54. jamesob approved
  55. jamesob commented at 3:37 pm on September 9, 2021: member

    ACK 983a3552f22678d4466e0faa8e784f25b145a76b (jamesob/ackr/21726.1.fjahr.improve_indices_on_prune)

    This change has two immediate benefits: (i) it removes an existing cicular depdendency between indexing code and validation, and (ii) allows use of coinstatsindex alongside pruning configuration. This inteface is also a nice way to decouple pruning itself from various consumers of blockdata that limit it.

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 983a3552f22678d4466e0faa8e784f25b145a76b ([`jamesob/ackr/21726.1.fjahr.improve_indices_on_prune`](https://github.com/jamesob/bitcoin/tree/ackr/21726.1.fjahr.improve_indices_on_prune))
     4
     5This change has two immediate benefits: (i) it removes an existing cicular depdendency between indexing code and validation, and (ii) allows use of coinstatsindex alongside pruning configuration. This inteface is also a nice way to decouple pruning itself from various consumers of blockdata that limit it.
     6-----BEGIN PGP SIGNATURE-----
     7
     8iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmE6KhgACgkQepNdrbLE
     9TwWm+A//e7YHR1ZGAp+nsrIF2ZtO0a/TSlR1YEVIwzcYXXmA6BonfRfS8OCXuKfa
    10cdKsbxQudd8vjPoQ+ec4jCKqep+1DxrZzes56Z1sinMYqk8W1IosMJYXFH+QZt4h
    118ujmZ5bvtjBBStmpWW7jkUljGQlE2dMhH3h+jZ5F7P5nFDBgMeZoutUntgTkSWpk
    12G8wv5OMGgm5jNFXBfgyA7wi0SCMw/QNBhu/dKDbcCYg9CVDPh6sPXeuYAJPk2Xbd
    133W5sLv+9cRFRAeQLeBc5112UnixqF4VCUJZIeLTcwgb1F786kAWj9SBurM0o0KKW
    145AjXL2dmbmni3qxqLh3xAPjIXLJse+ZIVZ3S4LW8ds36jpY2oDtt4hzQb1Lf/bm3
    15bfvjuMz6SF25oZWnE+XUeg/kREb0JK4wxlr7hK3qIjvp8LxEf9vSmozEsIbNZZlB
    16q4o/+aFUuRy0mJPrJb7D5RqdC+/LIRXbkPSyeJiibYxnhOMAUK8d2i4atpfjpzN6
    17PF+8Es7GaUYbZnJxO6hxbRrEWimewnvRHS9N4jwDcFB0FRuFVWJmRfkcBwY8NTm3
    185O22Abo7sPAWztZjOlM6yq6Vn1yuwVvFPSCzpE/eWZtAhhf86b7Dwnnd/NXiKa5s
    19ZzuhCuHQLGZJnX3sM38h/E9g4nmClP0Drf0uxDJNqyXw8nQIonQ=
    20=UW7v
    21-----END PGP SIGNATURE-----
    
    0Tested on Linux-4.19.0-17-amd64-x86_64-with-glibc2.28
    1
    2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare --enable-wallet --with-daemon CXX=/usr/local/bin/clang++ CC=/usr/local/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
    3
    4Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -fdebug-prefix-map=$(abs_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4 -msha -msse4.1 -msse4.2  i
    5
    6Compiler version: Debian clang version 11.1.0-++20210428103820+1fdec59bffc1-1~exp1~20210428204437.162
    
  56. in test/functional/feature_index_prune.py:109 in 983a3552f2 outdated
    104+        self.mine_batches(4)
    105+
    106+        self.log.info("prune further than the indices best blocks while the indices are disabled")
    107+        for i in range(3):
    108+            pruneheight_new = self.nodes[i].pruneblockchain(1000)
    109+            assert_greater_than(pruneheight_new, pruneheight)
    


    dergoegge commented at 12:50 pm on September 11, 2021:

    nit: make the code match the log statement (pruneheight is here still set to 248 while the indices best block is 700)

    0            assert_greater_than(pruneheight_new, 700)
    

    fjahr commented at 9:59 pm on September 22, 2021:
    Makes sense, fixed!
  57. in test/functional/feature_index_prune.py:70 in 983a3552f2 outdated
    65+
    66+        # Mine two batches of blocks to avoid hitting NODE_NETWORK_LIMITED_MIN_BLOCKS disconnection
    67+        self.mine_batches(2)
    68+        self.sync_index(height=700)
    69+
    70+        self.expected_prune_height(690)
    


    dergoegge commented at 3:04 pm on September 11, 2021:

    I am able to put any value here besides 690 and the test still passes.

    This is because assert_debug_log is a generator and meant to be used in combination with with:

    0with assert_debug_log(...):
    1  # some other code that prints the (un)expected logs
    

    “At the point where the generator yields, the block nested in the with statement is executed. The generator is then resumed after the block is exited. […]” from https://docs.python.org/3/library/contextlib.html#contextlib.contextmanager

    Secondly i think that checking the logs to test the prune blocker height won’t work here as the expected message is only printed when pruning is attempted (https://github.com/bitcoin/bitcoin/blob/983a3552f22678d4466e0faa8e784f25b145a76b/src/validation.cpp#L2027)

    the following would work:

    0 self.log.info("prune some blocks")
    1 for node in self.nodes[:2]:
    2     with node.assert_debug_log(['limited pruning to height 690']):
    3         pruneheight = node.pruneblockchain(400)
    4         assert_equal(pruneheight, 248)
    

    fjahr commented at 10:09 pm on September 22, 2021:
    Thanks a lot for the detailed feedback! I fixed this using your suggested approach in the two locations where I wanted to do the check.
  58. dergoegge commented at 3:13 pm on September 11, 2021: member

    It’s a bit sad that there isn’t a way to pause/resume an index for testing purposes. With that the test here could do something like:

    1. mine some blocks and let the index sync to tip
    2. pause the index
    3. mine a bunch of blocks
    4. prune and check that the last pruned block (returned from pruneblockchain) matches the prune blocker.
    5. resume the index and sync to tip
    6. prune again and check that blocks after the previous blocker were pruned

    might be something for a follow-up.

  59. in src/node/blockstorage.h:52 in 2c815adc88 outdated
    47@@ -48,6 +48,9 @@ extern uint64_t nPruneTarget;
    48 //! Check whether the block associated with this index entry is pruned or not.
    49 bool IsBlockPruned(const CBlockIndex* pblockindex);
    50 
    51+//! Find the last block that is pruned
    52+const CBlockIndex* GetLastPrunedBlock(const CBlockIndex* start_block);
    


    ryanofsky commented at 5:28 pm on September 20, 2021:

    In commit “refactor: Introduce GetLastPruneBlock helper function” (2c815adc881fa90f14887e2ec33ccb11ef964387)

    This doesn’t seem to actually return the last block that is pruned. It seems to return the next block that has data. So maybe should be called GetFirstNonPrunedBlock? It also seems like pruneblockchain RPC documentation describes this incorrectly while getblockchaininfo describes it correctly.


    fjahr commented at 10:02 pm on September 22, 2021:
    True, I fixed this and went with with the name GetFirstStoredBlock inspired by the docs in getblockchaininfo. I also added an extra commit to fix the docs in pruneblockchain. I think fixing the docs is the right approach rather than change the behavior but I kept it in the separate commit in case there are different opinions on that.
  60. fjahr force-pushed on Sep 22, 2021
  61. DrahtBot removed the label Needs rebase on Sep 22, 2021
  62. fjahr force-pushed on Sep 22, 2021
  63. in src/node/blockstorage.h:183 in 4ebe3a51da outdated
    47@@ -48,6 +48,9 @@ extern uint64_t nPruneTarget;
    48 //! Check whether the block associated with this index entry is pruned or not.
    49 bool IsBlockPruned(const CBlockIndex* pblockindex);
    50 
    51+//! Find the first block that is not pruned
    52+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block);
    


    ryanofsky commented at 2:11 am on September 23, 2021:

    In commit “refactor: Introduce GetFirstStoredlock helper function” (4ebe3a51da88589f23e243045a9e392701dfbbad)

    Function name is spelled wrong in commit message


    fjahr commented at 11:18 pm on September 29, 2021:
    fixed
  64. in src/index/base.cpp:381 in 70b9e4f6ab outdated
    364@@ -369,3 +365,12 @@ IndexSummary BaseIndex::GetSummary() const
    365     summary.best_block_height = m_best_block_index ? m_best_block_index.load()->nHeight : 0;
    366     return summary;
    367 }
    368+
    369+void BaseIndex::SetBestBlockIndex(const CBlockIndex* block) {
    


    ryanofsky commented at 5:52 pm on September 23, 2021:

    In commit “Index: Use prune blockers for blockfilterindex” (7db7f26e97665c048b3f614c092899addd658f4f)

    Can we add an assert here:

    0assert(!fPruneMode || AllowPrune());
    

    I know there is startup code that prevents the indexes which don’t support AllowPrune from being enabled at the same time as the prune option, but you could imagine someone adding a new index where AllowPrune is false, and someone forgets to add the startup check. Or if the startup check just happens to be buggy, it would be good to have this check to detect the problem.


    fjahr commented at 11:18 pm on September 29, 2021:
    Added
  65. in src/validation.cpp:2345 in 7db7f26e97 outdated
    1939@@ -1941,12 +1940,9 @@ bool CChainState::FlushStateToDisk(
    1940         CoinsCacheSizeState cache_state = GetCoinsCacheSizeState();
    1941         LOCK(cs_LastBlockFile);
    1942         if (fPruneMode && (fCheckForPruning || nManualPruneHeight > 0) && !fReindex) {
    1943-            // make sure we don't prune above the blockfilterindexes bestblocks
    1944+            // make sure we don't prune above any of the prune blockers bestblocks
    1945             // pruning is height-based
    1946             int last_prune = m_chain.Height(); // last height we can prune
    1947-            ForEachBlockFilterIndex([&](BlockFilterIndex& index) {
    1948-               last_prune = std::max(1, std::min(last_prune, index.GetSummary().best_block_height));
    


    ryanofsky commented at 6:33 pm on September 23, 2021:

    In commit “Index: Use prune blockers for blockfilterindex” (7db7f26e97665c048b3f614c092899addd658f4f)

    I think it would be good if the commit message noted the minor change in behavior here: Previously pruning code would keep the blockfilter index best block and later blocks, and now an extra 10 previous blocks(PRUNE_BLOCKER_BUFFER) will be kept.


    fjahr commented at 11:18 pm on September 29, 2021:
    Added a note in the commit message
  66. in src/index/base.cpp:70 in 70b9e4f6ab outdated
    66@@ -67,7 +67,7 @@ bool BaseIndex::Init()
    67         SetBestBlockIndex(m_chainstate->m_blockman.FindForkInGlobalIndex(active_chain, locator));
    68     }
    69     m_synced = m_best_block_index.load() == active_chain.Tip();
    70-    if (!m_synced) {
    71+    if (!m_synced && (fPruneMode || fHavePruned) && !fReindex) {
    


    ryanofsky commented at 6:40 pm on September 23, 2021:

    In commit “Index: Skip pruning checks when node is not pruning” (70b9e4f6abda5d6dd38504d9149ddc79b33961ce)

    What is the motivation for this commit? It just seems to be disabling error checking and making the code more complicated and interdependent. Is there a real performance benefit or some other reason to do this?


    fjahr commented at 11:19 pm on September 29, 2021:
    I did not do performance tests on this change, it will probably not be huge but if we can get it, why not? But my main motivation is that I think that these checks are not necessary unless the conditions hold. I think it makes code easier to understand if sections that are not relevant are skipped explicitly. But I can see how this might be missing a comment for clarity, happy to add that if you agree.

    ryanofsky commented at 4:51 pm on September 30, 2021:

    I think it makes code easier to understand if sections that are not relevant are skipped explicitly.

    I would love to visit the planet where if (!m_synced && (fPruneMode || fHavePruned) && !fReindex) is easier to understand than if (!m_synced). In general I don’t think code is easier to read with special cases, and all else equal I wouldn’t think that skipping checks would be a great idea even if did make code simpler. But this isn’t very important. Would suggest dropping this commit, but it seems fine to keep it or let others weigh in. Thanks at least for splitting this change up nicely so it is easy to understand in sequence.


    fjahr commented at 7:25 pm on October 3, 2021:
    I agree with you that the line is harder to understand after the change but in exchange for that, it makes explicit that the code inside the block is only relevant and needs to run if this evaluates to true. Let’s wait for more feedback as a tie-breaker, of course, I am happy to remove it if others agree with you :)

    dergoegge commented at 7:53 pm on October 5, 2021:

    it makes explicit that the code inside the block is only relevant and needs to run if this evaluates to true

    I don’t have a strong opinion but i agree with this statement and think the change should stay.


    jonatack commented at 3:43 pm on October 6, 2021:
    86121153164a05a5d not sure, but it seems there should be a demonstrated clear improvement and documentation of the intention in both places.
  67. in src/validation.cpp:1954 in 40ec224da2 outdated
    1947@@ -1948,6 +1948,15 @@ bool CChainState::FlushStateToDisk(
    1948                last_prune = std::max(1, std::min(last_prune, index.GetSummary().best_block_height));
    1949             });
    1950 
    1951+            for (auto const& blocker : m_blockman.m_prune_blockers) {
    1952+                const int blocker_height = blocker.second->nHeight - PRUNE_BLOCKER_BUFFER;
    


    ryanofsky commented at 7:31 pm on September 23, 2021:

    In commit “Index: Use prune blockers for blockfilterindex” (7db7f26e97665c048b3f614c092899addd658f4f)

    I’m seeing a segfault this line starting in the “Index: Use prune blockers” commit and continuing up to the ““Index: Skip pruning checks” commit in the feature_blockfilterindex_prune.py test because blocker.second is null. It would probably be good to not insert null prune blockers or just ignore them.

     0[#0](/bitcoin-bitcoin/0/)  0x000055555583d8a6 in CChainState::FlushStateToDisk (this=0x555555e46f10, state=..., mode=FlushStateMode::IF_NEEDED, nManualPruneHeight=0) at validation.cpp:1948
     1[#1](/bitcoin-bitcoin/1/)  0x0000555555849e51 in CChainState::ConnectTip (this=0x555555e46f10, state=..., pindexNew=0x7fffac001000, pblock=..., connectTrace=..., disconnectpool=...) at validation.cpp:2288
     2[#2](/bitcoin-bitcoin/2/)  0x000055555584f7f2 in CChainState::ActivateBestChainStep (this=0x555555e46f10, state=..., pindexMostWork=0x7fffac001000, pblock=std::shared_ptr<const CBlock> (empty) = {...}, fInvalidFound=@0x7fffa7ffe32f: false, connectTrace=...)
     3    at validation.cpp:2432
     4[#3](/bitcoin-bitcoin/3/)  0x000055555584fff8 in CChainState::ActivateBestChain (this=<optimized out>, state=..., pblock=std::shared_ptr<const CBlock> (empty) = {...}) at validation.cpp:2557
     5[#4](/bitcoin-bitcoin/4/)  0x00005555556c77fd in ThreadImport (chainman=..., vImportFiles=std::vector of length 0, capacity 0, args=...) at node/blockstorage.cpp:555
     6[#5](/bitcoin-bitcoin/5/)  0x0000555555633850 in operator() (__closure=0x7fffac000b60) at init.cpp:1656
     7[#6](/bitcoin-bitcoin/6/)  std::__invoke_impl<void, AppInitMain(NodeContext&, interfaces::BlockAndHeaderTipInfo*)::<lambda()>&> (__f=...) at /nix/store/5qjycalzb9sqzvqg65kf5zimqwjabm9g-gcc-10.3.0/include/c++/10.3.0/bits/invoke.h:60
     8[#7](/bitcoin-bitcoin/7/)  std::__invoke_r<void, AppInitMain(NodeContext&, interfaces::BlockAndHeaderTipInfo*)::<lambda()>&> (__fn=...) at /nix/store/5qjycalzb9sqzvqg65kf5zimqwjabm9g-gcc-10.3.0/include/c++/10.3.0/bits/invoke.h:110
     9[#8](/bitcoin-bitcoin/8/)  std::_Function_handler<void(), AppInitMain(NodeContext&, interfaces::BlockAndHeaderTipInfo*)::<lambda()> >::_M_invoke(const std::_Any_data &) (__functor=...)
    10    at /nix/store/5qjycalzb9sqzvqg65kf5zimqwjabm9g-gcc-10.3.0/include/c++/10.3.0/bits/std_function.h:291
    11[#9](/bitcoin-bitcoin/9/)  0x0000555555b0ef5f in std::function<void ()>::operator()() const (this=0x7fffa7ffeb70) at /nix/store/5qjycalzb9sqzvqg65kf5zimqwjabm9g-gcc-10.3.0/include/c++/10.3.0/bits/std_function.h:622
    12[#10](/bitcoin-bitcoin/10/) util::TraceThread(char const*, std::function<void ()>) (thread_name=<optimized out>, thread_func=...) at util/thread.cpp:18
    13[#11](/bitcoin-bitcoin/11/) 0x000055555563376b in std::__invoke_impl<void, void (*)(char const*, std::function<void()>), char const*, AppInitMain(NodeContext&, interfaces::BlockAndHeaderTipInfo*)::<lambda()> > (
    14    __f=@0x555555e7f818: 0x555555b0edf0 <util::TraceThread(char const*, std::function<void ()>)>) at /nix/store/5qjycalzb9sqzvqg65kf5zimqwjabm9g-gcc-10.3.0/include/c++/10.3.0/bits/invoke.h:60
    15[#12](/bitcoin-bitcoin/12/) std::__invoke<void (*)(char const*, std::function<void()>), char const*, AppInitMain(NodeContext&, interfaces::BlockAndHeaderTipInfo*)::<lambda()> > (__fn=@0x555555e7f818: 0x555555b0edf0 <util::TraceThread(char const*, std::function<void ()>)>)
    16    at /nix/store/5qjycalzb9sqzvqg65kf5zimqwjabm9g-gcc-10.3.0/include/c++/10.3.0/bits/invoke.h:95
    17[#13](/bitcoin-bitcoin/13/) std::thread::_Invoker<std::tuple<void (*)(char const*, std::function<void()>), char const*, AppInitMain(NodeContext&, interfaces::BlockAndHeaderTipInfo*)::<lambda()> > >::_M_invoke<0, 1, 2> (this=0x555555e7f7e8)
    18    at /nix/store/5qjycalzb9sqzvqg65kf5zimqwjabm9g-gcc-10.3.0/include/c++/10.3.0/thread:264
    19[#14](/bitcoin-bitcoin/14/) std::thread::_Invoker<std::tuple<void (*)(char const*, std::function<void()>), char const*, AppInitMain(NodeContext&, interfaces::BlockAndHeaderTipInfo*)::<lambda()> > >::operator() (this=0x555555e7f7e8)
    20    at /nix/store/5qjycalzb9sqzvqg65kf5zimqwjabm9g-gcc-10.3.0/include/c++/10.3.0/thread:271
    21[#15](/bitcoin-bitcoin/15/) std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(char const*, std::function<void()>), char const*, AppInitMain(NodeContext&, interfaces::BlockAndHeaderTipInfo*)::<lambda()> > > >::_M_run(void) (this=0x555555e7f7e0)
    22    at /nix/store/5qjycalzb9sqzvqg65kf5zimqwjabm9g-gcc-10.3.0/include/c++/10.3.0/thread:215
    23[#16](/bitcoin-bitcoin/16/) 0x00007ffff7eaaca0 in ?? () from /nix/store/dzaadhn5y56da24635icafv1nrawxm4n-gcc-10.3.0-lib/lib/../lib64/libstdc++.so.6
    24[#17](/bitcoin-bitcoin/17/) 0x00007ffff7fb1e9e in start_thread () from /nix/store/gk42f59363p82rg2wv2mfy71jn5w4q4c-glibc-2.32-48/lib/libpthread.so.0
    25[#18](/bitcoin-bitcoin/18/) 0x00007ffff774949f in clone () from /nix/store/gk42f59363p82rg2wv2mfy71jn5w4q4c-glibc-2.32-48/lib/libc.so.6
    

    fjahr commented at 11:22 pm on September 29, 2021:
    Oh, TIL that Genesis() can return a nullptr and does so initially when reindexing. I never really checked and assumed this couldn’t happen since the Genesis block is hardcoded. Well, good to know and this should be fixed now.

    jonatack commented at 2:35 pm on October 6, 2021:

    18e768f style nits and missing space

    0-            for (auto const& blocker : m_blockman.m_prune_blockers) {
    1-                const int blocker_height = blocker.second->nHeight - PRUNE_BLOCKER_BUFFER;
    2+            for (const auto& blocker : m_blockman.m_prune_blockers) {
    3+                const int blocker_height{blocker.second->nHeight - PRUNE_BLOCKER_BUFFER};
    4                 if (last_prune == blocker_height) {
    5-                    LogPrint(BCLog::PRUNE,"%s limited pruning to height %d\n", blocker.first, blocker_height);
    6+                    LogPrint(BCLog::PRUNE, "%s limited pruning to height %d\n", blocker.first, blocker_height);
    
  68. ryanofsky commented at 7:39 pm on September 23, 2021: contributor
    Code review almost-ACK 70b9e4f6abda5d6dd38504d9149ddc79b33961ce. I reviewed most of this pretty thoroughly but I didn’t really look at the python test yet. I think it would be good to fix the segfault in intermedate commits by not inserting null blockers, and to drop the “skip pruning checks” commit.
  69. DrahtBot added the label Needs rebase on Sep 27, 2021
  70. fjahr force-pushed on Sep 29, 2021
  71. fjahr commented at 11:23 pm on September 29, 2021: contributor
    Rebased and addressed @ryanofsky ’s comments. Thanks for reviewing!
  72. DrahtBot removed the label Needs rebase on Sep 29, 2021
  73. ryanofsky approved
  74. ryanofsky commented at 5:07 pm on September 30, 2021: contributor

    Code review ~ACK f6766d436b22ac595f6e13d365d07af37dbc0ebd ignoring windows test failure. Changes since last review: rebasing and fixing GetIntArg conflicts, adding assert, fixing segfault in intermediate commits, and adding a description in one of the commit messages.

    Windows test failure https://cirrus-ci.com/task/6252130924232704?logs=functional_tests#L2528 does seem like it is related. feature_index_prune.py is failing with “AssertionError: Block sync timed out after 480s.” Unclear if that is just a timeout or if it is a real sync error.

  75. DrahtBot added the label Needs rebase on Sep 30, 2021
  76. fjahr force-pushed on Oct 3, 2021
  77. fjahr force-pushed on Oct 3, 2021
  78. fjahr commented at 8:04 pm on October 3, 2021: contributor

    Rebased and beefed up the wait time one last block sync that caused a CI failure previously. I think it was a normal timeout because I could not reproduce it locally and there are a lot of blocks being produced. Comparable tests in terms of number of blocks also use longer sync times.

    I kindly ask other reviewers to weigh in on @ryanofsky ’s feedback here if you have a preference: #21726 (review)

  79. DrahtBot removed the label Needs rebase on Oct 3, 2021
  80. jonatack commented at 7:59 pm on October 5, 2021: contributor
    Concept ACK, will have a look.
  81. in src/rpc/blockchain.cpp:1034 in 8612115316 outdated
    1030@@ -1031,7 +1031,7 @@ static RPCHelpMan pruneblockchain()
    1031             "                  to prune blocks whose block time is at least 2 hours older than the provided timestamp."},
    1032                 },
    1033                 RPCResult{
    1034-                    RPCResult::Type::NUM, "", "Height of the last block pruned"},
    1035+                    RPCResult::Type::NUM, "", "Height of the first block that is still stored after pruning"},
    


    jonatack commented at 2:14 pm on October 6, 2021:

    19c360e

    0                    RPCResult::Type::NUM, "", "The height of the first block that is still stored after pruning."},
    

    fjahr commented at 1:04 am on December 5, 2021:
    Done
  82. in src/validation.h:91 in 8612115316 outdated
    87@@ -88,6 +88,8 @@ static const unsigned int DEFAULT_CHECKLEVEL = 3;
    88 // one 128MB block file + added 15% undo data = 147MB greater for a total of 545MB
    89 // Setting the target to >= 550 MiB will make it likely we can respect the target.
    90 static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
    91+/** Blocks to keep below deepest prune blocker */
    


    jonatack commented at 2:16 pm on October 6, 2021:

    18e768f

    0/** The number of blocks to keep below the deepest prune blocker. */
    

    fjahr commented at 1:05 am on December 5, 2021:
    Done
  83. in src/validation.cpp:3903 in 8612115316 outdated
    3598@@ -3594,6 +3599,11 @@ void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPr
    3599            nLastBlockWeCanPrune, count);
    3600 }
    3601 
    3602+void BlockManager::UpdatePruneBlocker(const std::string& name, const CBlockIndex* block) {
    3603+    AssertLockHeld(::cs_main);
    3604+    m_prune_blockers[name] = block;
    


    jonatack commented at 2:21 pm on October 6, 2021:
    18e768f Perhaps just inline this one-line function.

    fjahr commented at 4:52 pm on December 5, 2021:
    Hm, I would like to keep this setter, it feels more clean to me.
  84. in src/validation.h:467 in 8612115316 outdated
    486+     * Map from external index name to most recent block the index can tolerate being pruned.
    487+     *
    488+     * @note Internally, only blocks at height (block->nHeight - PRUNE_BLOCKER_BUFFER) and
    489+     * below will be pruned, but callers should avoid assuming any particular buffer size.
    490+     */
    491+    std::unordered_map<std::string, const CBlockIndex*> m_prune_blockers GUARDED_BY(::cs_main);
    


    jonatack commented at 2:33 pm on October 6, 2021:

    18e768f missing headers for unordered_map and std::min/max in this commit

     0--- a/src/validation.cpp
     1+++ b/src/validation.cpp
     2@@ -54,9 +54,11 @@
     3 #include <warnings.h>
     4 
     5+#include <algorithm>
     6 #include <numeric>
     7 #include <optional>
     8 #include <string>
     9+#include <unordered_map>
    10
    11--- a/src/validation.h
    12+++ b/src/validation.h
    13@@ -34,6 +34,7 @@
    14 #include <thread>
    15+#include <unordered_map>
    16 #include <utility>
    

    fjahr commented at 1:04 am on December 5, 2021:
    Done
  85. in test/functional/feature_index_prune.py:46 in 8612115316 outdated
    41+        self.connect_nodes(0,2)
    42+        self.connect_nodes(0,3)
    43+
    44+    def mine_batches(self, n):
    45+        for _ in range(n):
    46+            self.nodes[0].generate(250)
    


    jonatack commented at 3:08 pm on October 6, 2021:

    1b51e212 see #22741

    0            self.generate(self.nodes[0], 250)
    

    MarcoFalke commented at 12:10 pm on November 9, 2021:
    still not fixed?

    fjahr commented at 1:04 am on December 5, 2021:
    Done
  86. jonatack commented at 3:50 pm on October 6, 2021: contributor

    Approach ACK 10eb269e04c3306ba3749e2d178253747319a565 with some suggestions. Rebased to latest master, reviewed, debug built and ran unit tests and relevant functional tests at each commit.

    I had to run test/functional/feature_index_prune.py --timeout-factor=0, otherwise that updated test always times out for me locally even in turbo mode with the cpu maxed out.

     0$ test/functional/feature_index_prune.py 
     12021-10-06T15:47:31.133000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_289uzlfx
     22021-10-06T15:47:32.445000Z TestFramework (INFO): check if we can access blockfilters and coinstats when pruning is enabled but no blocks are actually pruned
     32021-10-06T15:48:32.497000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
     4        self.wait_until(lambda: self.nodes[1].getindexinfo() == expected_stats)
     5'''
     62021-10-06T15:48:32.498000Z TestFramework (ERROR): Assertion failed
     7Traceback (most recent call last):
     8  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
     9    self.run_test()
    10  File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_index_prune.py", line 54, in run_test
    11    self.sync_index(height=200)
    12  File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_index_prune.py", line 34, in sync_index
    13    self.wait_until(lambda: self.nodes[1].getindexinfo() == expected_stats)
    14  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 689, in wait_until
    15    return wait_until_helper(test_function, timeout=timeout, timeout_factor=self.options.timeout_factor)
    16  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 257, in wait_until_helper
    17    raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
    18AssertionError: Predicate ''''
    19        self.wait_until(lambda: self.nodes[1].getindexinfo() == expected_stats)
    20''' not true after 60.0 seconds
    212021-10-06T15:48:32.501000Z TestFramework (INFO): Stopping nodes
    
  87. in test/functional/test_runner.py:325 in 8612115316 outdated
    309@@ -310,7 +310,7 @@
    310     'feature_help.py',
    311     'feature_shutdown.py',
    312     'p2p_ibd_txrelay.py',
    313-    'feature_blockfilterindex_prune.py'
    314+    'feature_index_prune.py'
    


    jonatack commented at 3:59 pm on October 6, 2021:

    This test is now time-consuming enough (16 minutes for me locally) that it might be best to place it in the extended scripts. Or at least, at the top (rather than the bottom) of the base scripts. Even better, find a way to speed it up :)

    0EXTENDED_SCRIPTS = [
    1    # These tests are not run by default.
    2    # Longest test should go first, to favor running tests in parallel
    3    'feature_pruning.py',
    4    'feature_dbcrash.py',
    5]
    6
    7BASE_SCRIPTS = [
    8    # Scripts that are run by default.
    9    # Longest test should go first, to favor running tests in parallel
    
     0$ test/functional/feature_index_prune.py --timeout-factor=0
     12021-10-06T15:50:22.006000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_arh_064q
     22021-10-06T15:50:23.815000Z TestFramework (INFO): check if we can access blockfilters and coinstats when pruning is enabled but no blocks are actually pruned
     32021-10-06T15:54:06.653000Z TestFramework (INFO): prune some blocks
     42021-10-06T15:54:06.656000Z TestFramework (INFO): check if we can access the tips blockfilter and coinstats when we have pruned some blocks
     52021-10-06T15:54:06.732000Z TestFramework (INFO): check if we can access the blockfilter and coinstats of a pruned block
     62021-10-06T15:54:06.767000Z TestFramework (INFO): restart nodes without indices
     72021-10-06T15:54:09.197000Z TestFramework (INFO): make sure trying to access the indices throws errors
     82021-10-06T15:55:15.675000Z TestFramework (INFO): prune further than the indices best blocks while the indices are disabled
     92021-10-06T15:55:16.536000Z TestFramework (INFO): make sure we get an init error when starting the nodes again with the indices
    102021-10-06T15:55:18.413000Z TestFramework (INFO): make sure the nodes start again with the indices and an additional -reindex arg
    112021-10-06T16:06:34.500000Z TestFramework (INFO): Stopping nodes
    122021-10-06T16:06:35.271000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_arh_064q on exit
    132021-10-06T16:06:35.271000Z TestFramework (INFO): Tests successful
    

    fjahr commented at 1:03 am on December 5, 2021:

    Hmm, 16 minutes? There must be something off. The test takes ~40 seconds on my (5+ year old) laptop. Can you check again if there wasn’t another cause and maybe share some more info?

     0$ test/functional/feature_index_prune.py
     12021-12-04T23:40:35.661000Z TestFramework (INFO): Initializing test directory /var/folders/9z/n7rz_6cj3bq__11k5kcrsvvm0000gn/T/bitcoin_func_test_t25asmxj
     22021-12-04T23:40:36.639000Z TestFramework (INFO): check if we can access blockfilters and coinstats when pruning is enabled but no blocks are actually pruned
     32021-12-04T23:40:43.562000Z TestFramework (INFO): prune some blocks
     42021-12-04T23:40:43.566000Z TestFramework (INFO): check if we can access the tips blockfilter and coinstats when we have pruned some blocks
     52021-12-04T23:40:43.595000Z TestFramework (INFO): check if we can access the blockfilter and coinstats of a pruned block
     62021-12-04T23:40:43.615000Z TestFramework (INFO): restart nodes without indices
     72021-12-04T23:40:44.986000Z TestFramework (INFO): make sure trying to access the indices throws errors
     82021-12-04T23:40:53.843000Z TestFramework (INFO): prune further than the indices best blocks while the indices are disabled
     92021-12-04T23:40:54.330000Z TestFramework (INFO): make sure we get an init error when starting the nodes again with the indices
    102021-12-04T23:40:55.357000Z TestFramework (INFO): make sure the nodes start again with the indices and an additional -reindex arg
    112021-12-04T23:41:14.996000Z TestFramework (INFO): Stopping nodes
    122021-12-04T23:41:15.223000Z TestFramework (INFO): Cleaning up /var/folders/9z/n7rz_6cj3bq__11k5kcrsvvm0000gn/T/bitcoin_func_test_t25asmxj on exit
    132021-12-04T23:41:15.223000Z TestFramework (INFO): Tests successful
    

    jonatack commented at 11:51 am on April 6, 2022:
    Re-ran time test/functional/feature_index_prune.py --timeout-factor=0 a few times, best times are 12 to 13 minutes with CPUs at the max setting (3100 MHz for 2500 MHz core x 2), and ~ a half hour at more leisurely CPU settings.
  88. ryanofsky approved
  89. ryanofsky commented at 8:05 pm on November 8, 2021: contributor
    Code review ACK 86121153164a05a5da9812010f0bfdab434bec1f. Only changes are fixing AddArg rebase conflict and adding sync_blocks python test timeout
  90. MarcoFalke commented at 5:26 pm on November 10, 2021: member
    Test failue: #21726 (review)
  91. DrahtBot added the label Needs rebase on Nov 16, 2021
  92. fjahr force-pushed on Dec 5, 2021
  93. fjahr force-pushed on Dec 5, 2021
  94. DrahtBot removed the label Needs rebase on Dec 5, 2021
  95. fjahr commented at 4:58 pm on December 5, 2021: contributor

    Rebased and addressed all of @jonatack ’s comments (I still can’t figure out why GitHub lets me comment on and close some comments and not others).

    I also decided to drop the last commit that @ryanofsky wasn’t very happy with in order to make the rest of the review process a bit easier hopefully. In general, I think the whole topic “are we pruning? do we have to do something about that or not?” is a bit messy and I will tackle that more holistically in a future PR.

  96. dergoegge commented at 10:51 pm on December 8, 2021: member
    ACK deaa5f537bf21c33c8eb8ca0c276172a6a225f67
  97. DrahtBot added the label Needs rebase on Dec 13, 2021
  98. laanwj added this to the "Blockers" column in a project

  99. fjahr force-pushed on Dec 16, 2021
  100. fjahr force-pushed on Dec 16, 2021
  101. fjahr commented at 0:07 am on December 17, 2021: contributor

    Another two rebases later…

    Unfortunately there were some significant changes merged to the functional test, so there is a bit more to re-review this time 😢 Also these changes have more than doubled the runtime of the test for me. I have tried some simple optimizations but wasn’t successful it getting a meaningful speedup so far. I will revisit speeding up the tests in a follow-up because I don’t want to block review here further.

    That is, unless the test is now too slow to run on some machines now. Please test it again on your toaster, @jonatack ;)

  102. DrahtBot removed the label Needs rebase on Dec 17, 2021
  103. in src/validation.cpp:2230 in e9d434e03e outdated
    2229-            ForEachBlockFilterIndex([&](BlockFilterIndex& index) {
    2230-               last_prune = std::max(1, std::min(last_prune, index.GetSummary().best_block_height));
    2231-            });
    2232+
    2233+            for (const auto& blocker : m_blockman.m_prune_blockers) {
    2234+                const int blocker_height{blocker.second->nHeight - PRUNE_BLOCKER_BUFFER};
    


    luke-jr commented at 6:08 pm on December 19, 2021:
    This logic fails if a reorg happens removing deeper than the blocker’s block.

    luke-jr commented at 3:33 am on December 21, 2021:
    Can be fixed with something like 55f04cf1dd0f36981e3d09c180da52e11c974575

    fjahr commented at 6:34 pm on January 16, 2022:
    Conceptually i makes sense that the blockers can move backwards in reorg scenarios, so I have added this, though to be honest I have a hard time thinking of a realistic scenario where this prevents an issue on mainnet. I have also add a test for this feature.
  104. in src/validation.h:469 in e9d434e03e outdated
    464+     * @note Internally, only blocks at height (block->nHeight - PRUNE_BLOCKER_BUFFER) and
    465+     * below will be pruned, but callers should avoid assuming any particular buffer size.
    466+     */
    467+    std::unordered_map<std::string, const CBlockIndex*> m_prune_blockers GUARDED_BY(::cs_main);
    468+
    469+    void UpdatePruneBlocker(const std::string& name, const CBlockIndex* block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
    


    luke-jr commented at 6:49 pm on December 19, 2021:
    Prefer to use “lock” over “block”, since the latter is too easily confused with blockchain blocks.

    fjahr commented at 6:34 pm on January 16, 2022:
    I slightly prefer blocker because lock makes me think of mutex locks, which is even present on the same line. So I have kept the term blockers.

    luke-jr commented at 7:16 pm on January 16, 2022:
    Mutex locks and prune locks are fundamentally the same thing…?

    fjahr commented at 7:59 pm on January 16, 2022:
    Yes, to me that doesn’t make it less confusing

    luke-jr commented at 9:18 pm on January 16, 2022:

    Using the same word (lock) for the same thing makes sense.

    Using the same word (block) for completely different things, is confusing.


    fjahr commented at 11:30 pm on January 23, 2022:
    Renamed to locks.
  105. in src/validation.h:467 in e9d434e03e outdated
    462+     * Map from external index name to most recent block the index can tolerate being pruned.
    463+     *
    464+     * @note Internally, only blocks at height (block->nHeight - PRUNE_BLOCKER_BUFFER) and
    465+     * below will be pruned, but callers should avoid assuming any particular buffer size.
    466+     */
    467+    std::unordered_map<std::string, const CBlockIndex*> m_prune_blockers GUARDED_BY(::cs_main);
    


    luke-jr commented at 3:34 am on December 21, 2021:
    Better to just use a height here, I think. See 598b0d8dbf5793c60c63dc2a181a49291fe8011b

    fjahr commented at 6:34 pm on January 16, 2022:
    Added this, though simplified it a bit
  106. luke-jr changes_requested
  107. luke-jr commented at 3:36 am on December 21, 2021: member

    Looked into using this to rebase #19463. Seems mostly fine, with one bug and a few nits.

    The RPC interface for prune locks (not in this PR presently) could be useful for testing, so it might be a good idea to add it. If you want, you can just cherry-pick 9c9e0c0b8c6a5a332f45896261aca5bdb3746447 + e83ed21172049ef593702c52bb840868728f47d1 + da86f7bdcfda0f5b27091b3934bc9f3e8de03157 for that.

  108. dongcarl commented at 0:32 am on December 23, 2021: contributor

    Concept ACK, was just working on my own version of this when luke-jr alerted me to this PR. Thanks for doing the work so I can just cherry-pick haha :-)

    One thing I’m wondering: have you considered having CChainState “pull” the blocker_height from blockers when necessary instead of requiring blockers to update CChainState every time their blocker_height changes?

  109. luke-jr commented at 0:38 am on December 23, 2021: member

    One thing I’m wondering: have you considered having CChainState “pull” the blocker_height from blockers when necessary instead of requiring blockers to update CChainState every time their blocker_height changes?

    That won’t work for external or offline blockers. In #19463, my primary goal is to support tools like Armory or Lightning nodes that require parsing the block data, as well as ensuring pruning doesn’t invalidate wallet backups (even of offline wallets).

  110. dongcarl commented at 0:52 am on December 23, 2021: contributor

    One thing I’m wondering: have you considered having CChainState “pull” the blocker_height from blockers when necessary instead of requiring blockers to update CChainState every time their blocker_height changes?

    That won’t work for external or offline blockers. In #19463, my primary goal is to support tools like Armory or Lightning nodes that require parsing the block data, as well as ensuring pruning doesn’t invalidate wallet backups (even of offline wallets).

    The way I think this would work is: if CChainState asks WalletBlocker about the lowest prunable block, and the wallet is offline, WalletBlocker would just reply with the last one it knows about. I see in your original PR you planned on persisting blockers, so in my mind upon initialization, WalletBlocker would just load up the persisted file.

    Lmk if that makes sense. In my mind, all else being equal, a “pull” configuration is more likely to be correct since blockers won’t have to worry about forgetting to update their blocker_height, and avoids the need to constantly notify CChainState.

  111. luke-jr commented at 0:59 am on December 23, 2021: member

    The way I think this would work is: if CChainState asks WalletBlocker about the lowest prunable block, and the wallet is offline, WalletBlocker would just reply with the last one it knows about.

    No reason it would be limited to wallets, though. It ends up back to BlockManager keeping track of them all - not sure what the issue is with this? It seems perfectly in line with its role?

    In my mind, all else being equal, a “pull” configuration is more likely to be correct since blockers won’t have to worry about forgetting to update their blocker_height, and avoids the need to constantly notify CChainState.

    For each block, indexes will move their lock one height later - pull or push doesn’t matter in this scenario. But other locks might not move right away, so keeping track in the BlockManager avoids unnecessary interaction between them. It also allows BlockManager to shift backward any locks that will be impacted by a reorganization, preventing pruning of those blocks before the clients even know they need an older height.

  112. DrahtBot added the label Needs rebase on Jan 4, 2022
  113. jonatack commented at 7:22 pm on January 13, 2022: contributor

    That is, unless the test is now too slow to run on some machines now. Please test it again on your toaster, @jonatack ;)

    Happy to :) … needs rebase (again).

  114. fjahr force-pushed on Jan 16, 2022
  115. fjahr force-pushed on Jan 16, 2022
  116. fjahr commented at 8:01 pm on January 16, 2022: contributor
    Rebased and addressed @luke-jr comments (now co-author of the main commit introducing prune blockers).
  117. DrahtBot removed the label Needs rebase on Jan 16, 2022
  118. fjahr commented at 8:16 pm on January 16, 2022: contributor

    The RPC interface for prune locks (not in this PR presently) could be useful for testing, so it might be a good idea to add it. If you want, you can just cherry-pick 9c9e0c0 + e83ed21 + da86f7b for that.

    I have not done this yet because I don’t want to expand the scope of this PR too much. But I am happy to open a new PR with these commits as a follow-up when this one here has been merged.

  119. in src/node/blockstorage.cpp:411 in 8612115316 outdated
    50@@ -51,6 +51,15 @@ bool IsBlockPruned(const CBlockIndex* pblockindex)
    51     return (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
    52 }
    53 
    54+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block) {
    55+    CHECK_NONFATAL(start_block);
    56+    const CBlockIndex* last_block = start_block;
    57+    while (last_block->pprev && (last_block->pprev->nStatus & BLOCK_HAVE_DATA)) {
    


    jonatack commented at 6:22 pm on January 19, 2022:

    79a080f It may be prudent to add lock annotations to GetFirstStoredBlock().

    CBLockIndex::nStatus will be guarded by a mutex if #22932 is merged.

     0diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
     1index a36cb421e3..bc37f9eaff 100644
     2--- a/src/node/blockstorage.cpp
     3+++ b/src/node/blockstorage.cpp
     4@@ -438,6 +438,7 @@ bool IsBlockPruned(const CBlockIndex* pblockindex)
     5 }
     6 
     7 const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block) {
     8+    AssertLockHeld(::cs_main);
     9     CHECK_NONFATAL(start_block);
    10     const CBlockIndex* last_block = start_block;
    11     while (last_block->pprev && (last_block->pprev->nStatus & BLOCK_HAVE_DATA)) {
    12diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
    13index 749edae161..acae4bc897 100644
    14--- a/src/node/blockstorage.h
    15+++ b/src/node/blockstorage.h
    16@@ -180,7 +180,8 @@ public:
    17 bool IsBlockPruned(const CBlockIndex* pblockindex);
    18 
    19 //! Find the first block that is not pruned
    20-const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block);
    21+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block)
    22+    EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
    

    fjahr commented at 11:26 pm on January 23, 2022:
    Done
  120. in src/rpc/blockchain.cpp:1118 in 522661b92e outdated
    1114@@ -1115,7 +1115,7 @@ static RPCHelpMan pruneblockchain()
    1115             "                  to prune blocks whose block time is at least 2 hours older than the provided timestamp."},
    1116                 },
    1117                 RPCResult{
    1118-                    RPCResult::Type::NUM, "", "Height of the last block pruned"},
    1119+                    RPCResult::Type::NUM, "", "The height of the first block that is still stored after pruning"},
    


    jonatack commented at 6:57 pm on January 19, 2022:

    2ac873b7 for your consideration, maybe disambiguate “first” and add a comma

    0                    RPCResult::Type::NUM, "", "The height of the first (lowest) block that is still stored, after pruning"},
    

    fjahr commented at 11:25 pm on January 23, 2022:
    Done
  121. in src/validation.h:38 in 522661b92e outdated
    34@@ -35,6 +35,7 @@
    35 #include <stdint.h>
    36 #include <string>
    37 #include <thread>
    38+#include <unordered_map>
    


    jonatack commented at 7:02 pm on January 19, 2022:
    b44fea2c it looks like these two includes should be moved to node/blockstorage.{h,cpp} – or only the header file if you inline UpdatePruneBlocker() in the h file – instead of validation.{h,cpp}

    fjahr commented at 11:25 pm on January 23, 2022:
    Done
  122. in src/validation.cpp:2483 in 522661b92e outdated
    2478+    {
    2479+        // Prune blockers that began at or after the tip should get moved backward so they get a chance to reorg
    2480+        const int max_height_first = pindexDelete->nHeight - 1;
    2481+        for (auto& prune_blocker : m_blockman.m_prune_blockers) {
    2482+            if (prune_blocker.second.m_height_first <= max_height_first) continue;
    2483+            // NOTE: Don't need to write to db here, since it will get synced when the rest of the chainstate
    


    jonatack commented at 7:13 pm on January 19, 2022:
    b44fea2c This comment seems to be unfinished or missing context (“when”…what?), and not sure if “when” should be “with”

    fjahr commented at 11:25 pm on January 23, 2022:
    Done
  123. in src/index/base.cpp:384 in 522661b92e outdated
    379+    m_best_block_index = block;
    380+    if (AllowPrune() && block) {
    381+        node::PruneBlockerInfo prune_blocker;
    382+        prune_blocker.m_height_first = block->nHeight;
    383+        LOCK(::cs_main);
    384+        m_chainstate->m_blockman.UpdatePruneBlocker(GetName(), prune_blocker);
    


    jonatack commented at 7:19 pm on January 19, 2022:

    a879711

    0-        LOCK(::cs_main);
    1-        m_chainstate->m_blockman.UpdatePruneBlocker(GetName(), prune_blocker);
    2+        WITH_LOCK(::cs_main, m_chainstate->m_blockman.UpdatePruneBlocker(GetName(), prune_blocker));
    

    fjahr commented at 11:25 pm on January 23, 2022:
    Done
  124. in src/index/base.h:106 in 522661b92e outdated
    102@@ -103,6 +103,8 @@ class BaseIndex : public CValidationInterface
    103     /// Get the name of the index for display in logs.
    104     virtual const char* GetName() const = 0;
    105 
    106+    virtual bool AllowPrune() const = 0;
    


    jonatack commented at 7:28 pm on January 19, 2022:
    a879711 There doesn’t seem to be any need for the four declarations of AllowPrune() to be protected; can they just be private?

    fjahr commented at 11:25 pm on January 23, 2022:
    Done
  125. in src/init.cpp:420 in 522661b92e outdated
    416@@ -417,7 +417,7 @@ void SetupServerArgs(ArgsManager& argsman)
    417         -GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    418     argsman.AddArg("-persistmempool", strprintf("Whether to save the mempool on shutdown and load on restart (default: %u)", DEFAULT_PERSIST_MEMPOOL), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    419     argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    420-    argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex and -coinstatsindex. "
    421+    argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex. "
    


    jonatack commented at 7:30 pm on January 19, 2022:

    16f6718

    0    argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex. "
    

    fjahr commented at 11:25 pm on January 23, 2022:
    Done
  126. jonatack commented at 7:42 pm on January 19, 2022: contributor

    Re-reviewing from scratch. Haven’t yet re-reviewed the tests.

    I still need to run test/functional/feature_index_prune.py --timeout-factor=0 to avoid timeouts and the test still takes 15-16 minutes to run in performance mode with my cores at max frequency, so no change from #21726 (review) and the suggestion to place the test in the extended scripts of the test runner (or at the top of the base scripts, or speed it up).

    Edit: The slowest test is make sure the nodes start again with the indices and an additional -reindex arg, taking 8-9 minutes out of 16 for me, or a bit more than half of the total test run time.

  127. fjahr force-pushed on Jan 23, 2022
  128. fjahr commented at 11:25 pm on January 23, 2022: contributor

    I still need to run test/functional/feature_index_prune.py --timeout-factor=0 to avoid timeouts and the test still takes 15-16 minutes to run in performance mode with my cores at max frequency, so no change from #21726 (comment) and the suggestion to place the test in the extended scripts of the test runner (or at the top of the base scripts, or speed it up).

    That is actually an improvement relatively speaking because for me the test became about 2.5x slower since the rebase here. For me the other extended tests are still a lot slower than this one but it’s not too far off anymore so I have added the test to the list now.

  129. fjahr commented at 11:29 pm on January 23, 2022: contributor
    Addressed comments by @jonatack and renamed from blockers to locks since I am not super passionate about naming stuff and I hope getting rid of controversies gets this closer to a merge faster.
  130. in src/validation.h:108 in 447f8c500d outdated
     99@@ -100,6 +100,8 @@ static const unsigned int DEFAULT_CHECKLEVEL = 3;
    100 // one 128MB block file + added 15% undo data = 147MB greater for a total of 545MB
    101 // Setting the target to >= 550 MiB will make it likely we can respect the target.
    102 static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
    103+/** The number of blocks to keep below the deepest prune lock. */
    104+static constexpr int PRUNE_LOCK_BUFFER{10};
    


    jonatack commented at 6:44 pm on January 26, 2022:

    e530e5e Is the choice of buffer value described somewhere (i.e. why 10 blocks)? Perhaps document the reasoning here and/or in the commit message.

    The commit description "This change also introduces an aditional buffer of 10 blocks (PRUNE_LOCK_BUFFER) that will not be pruned before the best block." might be in the wrong commit; it is in “Index: Use prune locks for blockfilterindex” but this constant is introduced in the preceding commit, “blockstorage: Add prune locks to BlockManager”.


    fjahr commented at 11:01 pm on February 20, 2022:

    The number is kind of arbitrary: A bit higher than what we expect to see on mainnet, like at this level we might actually have bigger problems than the indices on pruned nodes. On the other hand low enough to not have too much impact on the pruning mechanism. I have added a bit more concise version of this to the comment.

    Good catch on the commit message, fixed that as well.


    jonatack commented at 11:10 am on April 6, 2022:
    https://github.com/bitcoin/bitcoin/commit/527ef4463b23ab8c80b8502cd833d64245c5cfc4 PRUNE_LOCK_BUFFER is only used in validation.cpp, so it could perhaps be moved there.
  131. jonatack commented at 10:06 pm on January 26, 2022: contributor
    Updates LGTM. I need to go through the functional tests to finish the review. One WIP comment.
  132. in src/index/base.cpp:388 in 55e1a8109d outdated
    378+
    379+    m_best_block_index = block;
    380+    if (AllowPrune() && block) {
    381+        node::PruneLockInfo prune_lock;
    382+        prune_lock.m_height_first = block->nHeight;
    383+        WITH_LOCK(::cs_main, m_chainstate->m_blockman.UpdatePruneLock(GetName(), prune_lock));
    


    mzumsande commented at 0:04 am on January 28, 2022:
    This updates the prune lock for coinstatsindex and blockfilterindex regularly, even if we don’t have pruning enabled. Is that necessary?

    fjahr commented at 11:01 pm on February 20, 2022:

    I was feeling the same and previously had a commit included here but removed it due to feedback that it wasn’t needed: https://github.com/bitcoin/bitcoin/commit/e365e87a72cc49e175b31256a11a6c5b1621d1f2

    I guess I could open a follow-up PR but for now I am trying to keep this PR as simple as possible to move it forward.

  133. in src/validation.cpp:2605 in e530e5e76a outdated
    2472@@ -2468,6 +2473,18 @@ bool CChainState::DisconnectTip(BlockValidationState& state, DisconnectedBlockTr
    2473         assert(flushed);
    2474     }
    2475     LogPrint(BCLog::BENCH, "- Disconnect block: %.2fms\n", (GetTimeMicros() - nStart) * MILLI);
    2476+
    2477+    {
    2478+        // Prune locks that began at or after the tip should get moved backward so they get a chance to reorg
    2479+        const int max_height_first = pindexDelete->nHeight - 1;
    2480+        for (auto& prune_lock : m_blockman.m_prune_locks) {
    


    mzumsande commented at 0:16 am on January 28, 2022:
    Couldn’t this also be skipped when we are not pruning?

    fjahr commented at 11:01 pm on February 20, 2022:
    (see my other comment, same reasoning)
  134. mzumsande commented at 0:20 am on January 28, 2022: contributor
    Concept ACK. The code looks good to me on first pass, my only question is why bookkeeping for prune locks is done when not pruning (see below).
  135. luke-jr commented at 0:48 am on February 4, 2022: member
    We don’t really have a “never prune” option right now, just a “haven’t pruned yet, no plans to do so” which could be changed at any time by the user…
  136. mzumsande commented at 1:56 am on February 4, 2022: contributor

    We don’t really have a “never prune” option right now, just a “haven’t pruned yet, no plans to do so” which could be changed at any time by the user…

    Right, but we do have a fPruneMode flag that is only set in Init, so we can’t switch to pruning without a restart afaik. And since the spot where the prune locks are read is conditional on fPruneMode (code) I was wondering whether it would make sense to do the same with the spots where the locks are written. The init code must ensure anyway that nothing is pruned at startup before the indexes are initialized and the prune blockers are set.

  137. luke-jr commented at 3:13 am on February 4, 2022: member
    I guess this PR doesn’t persist prune locks at all, but that is planned for afterward, so I’m not sure it makes sense to special-case it here (which would also be a layering violation IMO).
  138. in src/node/blockstorage.h:165 in 447f8c500d outdated
    160@@ -156,6 +161,16 @@ class BlockManager
    161     //! Returns last CBlockIndex* that is a checkpoint
    162     CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    163 
    164+    /**
    165+     * Map from external index name to most recent block the index can tolerate being pruned.
    


    luke-jr commented at 5:49 pm on February 4, 2022:
    “oldest block that must not be pruned”

    fjahr commented at 11:05 pm on February 20, 2022:
    done
  139. in src/validation.cpp:2356 in 447f8c500d outdated
    2234+
    2235+            for (const auto& prune_lock : m_blockman.m_prune_locks) {
    2236+                if (prune_lock.second.m_height_first == std::numeric_limits<int>::max()) continue;
    2237+                const int lock_height{prune_lock.second.m_height_first - PRUNE_LOCK_BUFFER};
    2238+                last_prune = std::max(1, std::min(last_prune, lock_height));
    2239+                if (last_prune == lock_height) {
    


    luke-jr commented at 5:57 pm on February 4, 2022:
    Not sure this logic makes sense. Depending on iteration order, it might or might not log prune locks with a higher start height. If we only want to log the limiting lock, I think we just need to keep track of which it is, then log after the loop is done. Otherwise, maybe we should just log all locks that impact the pruning?

    fjahr commented at 11:05 pm on February 20, 2022:
    Yeah, makes sense, changed the logging part.
  140. in src/validation.cpp:2482 in 447f8c500d outdated
    2477+    {
    2478+        // Prune locks that began at or after the tip should get moved backward so they get a chance to reorg
    2479+        const int max_height_first = pindexDelete->nHeight - 1;
    2480+        for (auto& prune_lock : m_blockman.m_prune_locks) {
    2481+            if (prune_lock.second.m_height_first <= max_height_first) continue;
    2482+            // NOTE: Don't need to write to db here, since it will get synced with the rest of the chainstate
    


    luke-jr commented at 6:02 pm on February 4, 2022:
    Comment is premature since this PR never writes prune locks :)

    fjahr commented at 11:05 pm on February 20, 2022:
    Removed
  141. in src/validation.cpp:2344 in 447f8c500d outdated
    2223@@ -2225,12 +2224,18 @@ bool CChainState::FlushStateToDisk(
    2224         CoinsCacheSizeState cache_state = GetCoinsCacheSizeState();
    2225         LOCK(m_blockman.cs_LastBlockFile);
    2226         if (fPruneMode && (m_blockman.m_check_for_pruning || nManualPruneHeight > 0) && !fReindex) {
    2227-            // make sure we don't prune above the blockfilterindexes bestblocks
    2228+            // make sure we don't prune above any of the prune locks bestblocks
    2229             // pruning is height-based
    2230             int last_prune = m_chain.Height(); // last height we can prune
    2231-            ForEachBlockFilterIndex([&](BlockFilterIndex& index) {
    


    luke-jr commented at 6:05 pm on February 4, 2022:
    This is deleted one commit too early (part of “blockstorage: Add prune locks to BlockManager” rather than “Index: Use prune locks for blockfilterindex”)

    fjahr commented at 11:05 pm on February 20, 2022:
    fixed
  142. in src/init.cpp:864 in 447f8c500d outdated
    872     if (args.GetIntArg("-prune", 0)) {
    873         if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX))
    874             return InitError(_("Prune mode is incompatible with -txindex."));
    875-        if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX))
    876-            return InitError(_("Prune mode is incompatible with -coinstatsindex."));
    877     }
    


    luke-jr commented at 6:19 pm on February 4, 2022:
    IMO this should be a separate PR so we don’t have to review the whole coinstatsindex logic for prune-safety in this PR.

    fjahr commented at 11:04 pm on February 20, 2022:
    Hmm, this part was, to me, the main motivation for this PR. Given the review that has already gone into it up until now with this included I think it’s ok to leave it in. But will reconsider if others agree with you.
  143. luke-jr changes_requested
  144. fjahr force-pushed on Feb 20, 2022
  145. fjahr commented at 11:06 pm on February 20, 2022: contributor
    I hope I addressed all the comments, thanks for reviewing!
  146. in src/node/blockstorage.h:60 in d249636878 outdated
    55@@ -55,6 +56,10 @@ struct CBlockIndexWorkComparator {
    56     bool operator()(const CBlockIndex* pa, const CBlockIndex* pb) const;
    57 };
    58 
    59+struct PruneLockInfo {
    60+    int m_height_first{std::numeric_limits<int>::max()};
    


    ryanofsky commented at 3:46 am on March 3, 2022:

    In commit “blockstorage: Add prune locks to BlockManager” (d249636878b39aa34222d335cd85f0e2561bd50a)

    Minor: It would be good say what this variable means in the place where it’s declared. Would suggest adding “//! Height of earliest block that should be kept and not pruned” on line above.

    Minor: Would be nice to drop m_ prefix, because this a structure definition for public state, not a class definition for hidden state, where the m_ prefix would make internal methods accessing the state more readable.


    fjahr commented at 5:46 pm on March 8, 2022:
    Makes sense, done (both)
  147. in src/node/blockstorage.h:167 in d249636878 outdated
    160@@ -156,6 +161,16 @@ class BlockManager
    161     //! Returns last CBlockIndex* that is a checkpoint
    162     CBlockIndex* GetLastCheckpoint(const CCheckpointData& data) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    163 
    164+    /**
    165+     * Map from external index name to oldest block that must not be pruned.
    166+     *
    167+     * @note Internally, only blocks at height (block->nHeight - PRUNE_LOCK_BUFFER) and
    


    ryanofsky commented at 3:48 am on March 3, 2022:

    In commit “blockstorage: Add prune locks to BlockManager” (d249636878b39aa34222d335cd85f0e2561bd50a)

    Comment’s slightly out of date: block->nHeight here should be replaced by height_first


    fjahr commented at 5:46 pm on March 8, 2022:
    done
  148. in src/validation.cpp:2238 in d249636878 outdated
    2235+                last_prune = std::max(1, std::min(last_prune, index.GetSummary().best_block_height));
    2236             });
    2237 
    2238+            for (const auto& prune_lock : m_blockman.m_prune_locks) {
    2239+                if (prune_lock.second.m_height_first == std::numeric_limits<int>::max()) continue;
    2240+                const int lock_height{prune_lock.second.m_height_first - PRUNE_LOCK_BUFFER};
    


    ryanofsky commented at 4:03 am on March 3, 2022:

    In commit “blockstorage: Add prune locks to BlockManager” (d249636878b39aa34222d335cd85f0e2561bd50a)

    I think there is a minor off-by-one error here and this should be lock_height = height_first - PRUNE_LOCK_BUFFER - 1.

    If height_first is 100 (“oldest block that must not be pruned”), and PRUNE_LOCK_BUFFER is 1 (“number of blocks to keep below the deepest prune lock”), then lock_height (“last height we can prune”) should be 98 not 99. Pruning up to 99 when 100 is the first block that can’t be pruned would be leaving a buffer of size 0 not size 1.


    fjahr commented at 5:46 pm on March 8, 2022:
    Yeah, makes sense, I also added a comment here to make it clearer to future readers what the additional - 1 is there for.
  149. in src/validation.cpp:2529 in d249636878 outdated
    2481@@ -2468,6 +2482,18 @@ bool CChainState::DisconnectTip(BlockValidationState& state, DisconnectedBlockTr
    2482         assert(flushed);
    2483     }
    2484     LogPrint(BCLog::BENCH, "- Disconnect block: %.2fms\n", (GetTimeMicros() - nStart) * MILLI);
    2485+
    2486+    {
    2487+        // Prune locks that began at or after the tip should get moved backward so they get a chance to reorg
    


    ryanofsky commented at 4:33 am on March 3, 2022:

    In commit “blockstorage: Add prune locks to BlockManager” (d249636878b39aa34222d335cd85f0e2561bd50a)

    Note just for understanding this: Interesting to see this logic added, since it wasn’t in original version of the PR. It looks like the result of discussion with Luke in #21726 (review) and is it checked by a python test looking for the “lock moved back to” log message below.

    IMO, this change adds a little bit of internal complexity but improves the external design and reliability of prune locks. If you think about indexes running asynchronously behind the main chain, it’s a nice thing if prune locks are updated automatically when there’s a reorg, and indexes are free to catch up to block connected and disconnected notifications at any time later. They don’t need to suddenly hurry and update their locks because this code does it for them.

  150. ryanofsky approved
  151. ryanofsky commented at 4:51 am on March 3, 2022: contributor
    Code review ACK eccb1b4b0b3bf26850e73af1c33ad65980730918. Main change since last review is PR has been Lukified with two nice changes: introducing the PruneLockInfo struct which should make locks more extensible, and automatically updating locks on reorgs so indexes (and wallets in the future) can be safe after reorgs if they are running more asynchronously. There were also a lot of rebase fixes and other smaller changes.
  152. fjahr force-pushed on Mar 8, 2022
  153. fjahr commented at 5:48 pm on March 8, 2022: contributor
    Addressed comments from @ryanofsky for some nice improvements , thanks!
  154. DrahtBot added the label Needs rebase on Mar 17, 2022
  155. dongcarl added this to the "WIP PRs" column in a project

  156. dongcarl moved this from the "WIP PRs" to the "Relevant External PRs" column in a project

  157. fjahr force-pushed on Mar 19, 2022
  158. fjahr commented at 9:27 pm on March 19, 2022: contributor
    Rebased, also changed tests slightly to account for the fix of the off-by-one error added earlier
  159. fjahr force-pushed on Mar 19, 2022
  160. DrahtBot removed the label Needs rebase on Mar 19, 2022
  161. luke-jr referenced this in commit ea47c0d91f on Mar 21, 2022
  162. luke-jr referenced this in commit dae2843755 on Mar 21, 2022
  163. luke-jr referenced this in commit a1c1b83ab3 on Mar 21, 2022
  164. luke-jr referenced this in commit d8398860b3 on Mar 21, 2022
  165. luke-jr referenced this in commit e5450af95b on Mar 21, 2022
  166. in src/rpc/blockchain.cpp:747 in ce081ba2b1 outdated
    743@@ -744,7 +744,7 @@ static RPCHelpMan pruneblockchain()
    744             "                  to prune blocks whose block time is at least 2 hours older than the provided timestamp."},
    745                 },
    746                 RPCResult{
    747-                    RPCResult::Type::NUM, "", "Height of the last block pruned"},
    748+                    RPCResult::Type::NUM, "", "The height of the first (lowest) block that is still stored, after pruning"},
    


    luke-jr commented at 6:42 pm on March 21, 2022:

    This is actually a bug. From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last block pruned was returned, subject to a bug if there were blocks left unpruned due to sharing files with later blocks.

    In #15991, this was “fixed” to the current implementation, introducing the new bug you’re documenting here.

    Since the user provides the parameter as a block to include in pruning, I have a slight preference for fixing the behaviour to match the documentation. See #24629


    ryanofsky commented at 9:57 pm on March 21, 2022:

    In commit “RPC: Fix pruneblockchain return documentation” (f2d397c68e5042dee5827402d27741f1a89ff583)

    I’d agree with Luke. #24629 seems like a better fix, restoring behavior to what it used to be and straightforwardly returning the last block that was pruned. Even if there is a problem with #24629, and we want to take this approach instead, it could be done in different PR, so I think it would be good to drop this commit here to avoid confusing the issue more.


    fjahr commented at 9:12 pm on April 3, 2022:
    Ok, makes sense. Dropped the commit and will take a look at #24629.
  167. ryanofsky approved
  168. ryanofsky commented at 10:01 pm on March 21, 2022: contributor

    Code review ACK ce081ba2b1a2c4811df9a59e752d00bfcfce35fe. Only changes since last review were rebasing and making suggested cleanups, including fixing off-by-one buffer value.

    I do think it would be good to drop commit “RPC: Fix pruneblockchain return documentation” (f2d397c68e5042dee5827402d27741f1a89ff583), though, for reasons Luke suggested

  169. DrahtBot added the label Needs rebase on Mar 24, 2022
  170. in src/node/blockstorage.cpp:402 in 1961099959 outdated
    396@@ -397,6 +397,16 @@ bool IsBlockPruned(const CBlockIndex* pblockindex)
    397     return (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
    398 }
    399 
    400+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block) {
    401+    AssertLockHeld(::cs_main);
    402+    CHECK_NONFATAL(start_block);
    


    mzumsande commented at 2:33 pm on March 24, 2022:
    should we have CHECK_NONFATAL in src/node? It was designed for RPC use cases, but this function is now being used by non-RPC code as well. An alternative would be to leave the checks in the rpc code.

    fjahr commented at 9:12 pm on April 3, 2022:
    I agree, changed.
  171. in src/validation.cpp:2352 in 422d08345b outdated
    2287-               last_prune = std::max(1, std::min(last_prune, index.GetSummary().best_block_height));
    2288+                last_prune = std::max(1, std::min(last_prune, index.GetSummary().best_block_height));
    2289             });
    2290 
    2291+            for (const auto& prune_lock : m_blockman.m_prune_locks) {
    2292+                if (prune_lock.second.height_first == std::numeric_limits<int>::max()) continue;
    


    mzumsande commented at 5:36 pm on March 24, 2022:
    Can this condition ever become true? I would imagine not, because we wouldn’t ever have a default-initialized PruneLockInfo object in m_prune_locks.

    fjahr commented at 9:11 pm on April 3, 2022:
    Yeah, at the current state of the code I don’t see how this should happen. But I still would like to keep something there as belts & suspenders. But since this would be a developer error and not a usual case, I changed this line to an assert because it seems more appropriate here.
  172. mzumsande commented at 5:47 pm on March 24, 2022: contributor

    Code review ACK ce081ba2b1a2c4811df9a59e752d00bfcfce35fe

    This looks good to me, just two small comments below - happy to reACK after rebase / possibly removing the pruneblockchain doc change commit.

  173. fjahr force-pushed on Apr 3, 2022
  174. fjahr commented at 9:12 pm on April 3, 2022: contributor
    Rebased and addressed latest comments
  175. DrahtBot removed the label Needs rebase on Apr 3, 2022
  176. in src/node/blockstorage.cpp:411 in 8323cce2aa outdated
    396@@ -397,6 +397,15 @@ bool IsBlockPruned(const CBlockIndex* pblockindex)
    397     return (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
    398 }
    399 
    400+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block) {
    401+    AssertLockHeld(::cs_main);
    402+    const CBlockIndex* last_block = start_block;
    403+    while (last_block->pprev && (last_block->pprev->nStatus & BLOCK_HAVE_DATA)) {
    


    ryanofsky commented at 6:19 pm on April 4, 2022:

    In commit “refactor: Introduce GetFirstStoredBlock helper function” (8323cce2aa76123bc385c7927701d42282b0bd26)

    Minor: previous version of this function in 19610999595ec557fc602c8cefb930c2dfc1ad67 had a CHECK_NONFATAL(start_block) line, which I think was good because it prevented UB if a null start block was passed, which seems like an easy mistake to make. IMO it would be nice to restore this, or add assert(start_block) since callers are now using CHECK_NONFATAL.


    fjahr commented at 7:58 pm on April 4, 2022:
    I think the assert makes sense here now, added.
  177. in src/validation.cpp:2284 in a473db7e6a outdated
    2281+                last_prune = std::max(1, std::min(last_prune, index.GetSummary().best_block_height));
    2282             });
    2283 
    2284+            for (const auto& prune_lock : m_blockman.m_prune_locks) {
    2285+                assert(prune_lock.second.height_first < std::numeric_limits<int>::max());
    2286+                // if (prune_lock.second.height_first == std::numeric_limits<int>::max()) continue;
    


    ryanofsky commented at 6:29 pm on April 4, 2022:

    In commit “blockstorage: Add prune locks to BlockManager” (a473db7e6a97289f981c3ff4b560e07a680efed5)

    Minor: Could drop the commented line


    fjahr commented at 7:58 pm on April 4, 2022:
    🤦‍♂️ done
  178. ryanofsky approved
  179. ryanofsky commented at 6:34 pm on April 4, 2022: contributor
    Code review ACK 735e16c8e2e6e7023c58570cfc55b8f031136ea9. Main change since last review is dropping commit f2d397c68e5042dee5827402d27741f1a89ff583 “RPC: Fix pruneblockchain return documentation”. Also rebased and tweaked “limited pruning to height” log message and GetFirstStoredBlock null block asserts
  180. fjahr force-pushed on Apr 4, 2022
  181. fjahr commented at 8:00 pm on April 4, 2022: contributor
    Addressed latest comments by @ryanofsky , thanks everyone for your reviews!
  182. ryanofsky approved
  183. ryanofsky commented at 8:04 pm on April 4, 2022: contributor
    Code review ACK 2faa5b51799e690e20e25e660d7b18a18ae4dc62. Just suggested comment and assert tweaks since last review
  184. mzumsande approved
  185. mzumsande commented at 1:52 pm on April 5, 2022: contributor
    Code review ACK 2faa5b51799e690e20e25e660d7b18a18ae4dc62
  186. jonatack commented at 2:42 pm on April 5, 2022: contributor
    Catching up with the feedback and changes since my last review per git range-diff ee9af95 10eb269 2faa5b5, going over the full changes shortly.
  187. in src/index/base.cpp:78 in 9126b1eb5f outdated
    74@@ -75,11 +75,7 @@ bool BaseIndex::Init()
    75         if (!m_best_block_index) {
    76             // index is not built yet
    77             // make sure we have all block data back to the genesis
    78-            const CBlockIndex* block = active_chain.Tip();
    79-            while (block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA)) {
    80-                block = block->pprev;
    81-            }
    82-            prune_violation = block != active_chain.Genesis();
    83+            prune_violation = node::GetFirstStoredBlock(active_chain.Tip()) != active_chain.Genesis();
    


    mzumsande commented at 3:00 pm on April 5, 2022:
    Just noticing that the reason this cannot not trigger the added assert in GetFirstStoredBlock() is that if active_chain.Tip() is nullptr (which happens on first startup), then m_synced will be determined to be true, and we don’t get to this point (#22485 documents this). I think that this is a bit fragile, but doesn’t need to be addressed in this PR.
  188. DrahtBot added the label Needs rebase on Apr 6, 2022
  189. in src/node/blockstorage.h:185 in 2faa5b5179 outdated
    180+     * Map from external index name to oldest block that must not be pruned.
    181+     *
    182+     * @note Internally, only blocks at height (height_first - PRUNE_LOCK_BUFFER) and
    183+     * below will be pruned, but callers should avoid assuming any particular buffer size.
    184+     */
    185+    std::unordered_map<std::string, PruneLockInfo> m_prune_locks GUARDED_BY(::cs_main);
    


    jonatack commented at 11:12 am on April 6, 2022:
    527ef44 It looks like m_prune_locks can be private instead of public.

    fjahr commented at 6:27 pm on April 10, 2022:
    Right, done.
  190. jonatack commented at 11:12 am on April 6, 2022: contributor

    Re-reviewing. Each commit builds cleanly. Needs rebase.

    Edit: utACK modulo comments and didn’t yet review the tests (will do after rebase).

     0--- a/src/validation.cpp
     1+++ b/src/validation.cpp
     2@@ -2271,7 +2277,7 @@ bool CChainState::FlushStateToDisk(
     3         if (fPruneMode && (m_blockman.m_check_for_pruning || nManualPruneHeight > 0) && !fReindex) {
     4             // make sure we don't prune above any of the prune locks bestblocks
     5             // pruning is height-based
     6-            int last_prune = m_chain.Height(); // last height we can prune
     7+            int last_prune{m_chain.Height()}; // last height we can prune
     8             std::optional<std::string> limiting_lock; // prune lock that actually was the limiting factor, only used for logging
     9 
    10             for (const auto& prune_lock : m_blockman.m_prune_locks) {
    11@@ -2526,8 +2532,8 @@ bool CChainState::DisconnectTip(BlockValidationState& state, DisconnectedBlockTr
    12     LogPrint(BCLog::BENCH, "- Disconnect block: %.2fms\n", (GetTimeMicros() - nStart) * MILLI);
    13 
    14     {
    15-        // Prune locks that began at or after the tip should get moved backward so they get a chance to reorg
    16-        const int max_height_first = pindexDelete->nHeight - 1;
    17+        // Prune locks that began at or after the tip should be moved backward so they have a chance to reorg
    18+        const int max_height_first{pindexDelete->nHeight - 1};
    19         for (auto& prune_lock : m_blockman.m_prune_locks) {
    20             if (prune_lock.second.height_first <= max_height_first) continue;
    
  191. in src/index/base.h:132 in 2faa5b5179 outdated
    127@@ -125,6 +128,8 @@ class BaseIndex : public CValidationInterface
    128 
    129     /// Get a summary of the index and its state.
    130     IndexSummary GetSummary() const;
    131+
    132+    void SetBestBlockIndex(const CBlockIndex* block);
    


    jonatack commented at 11:36 am on April 6, 2022:
    3b8b898 it looks like SetBestBlockIndex can be private

    fjahr commented at 6:27 pm on April 10, 2022:
    Right, done.

    jonatack commented at 2:20 pm on April 28, 2022:

    https://github.com/bitcoin/bitcoin/commit/3b8b898d96f570489238a4aa21cf4fe27a4a7e73 it looks like SetBestBlockIndex can be private

    Right, done.

    Seems this was reverted in a later push or rebase. Fixed in #24150 (review welcome).

  192. fjahr force-pushed on Apr 10, 2022
  193. fjahr commented at 6:31 pm on April 10, 2022: contributor

    Rebased and addressed comments from @jonatack, thanks for reviewing!

    Also @mzumsande is so kind to host a PR Review Club on this PR this week, in case anyone is interested in joining :)

  194. DrahtBot removed the label Needs rebase on Apr 10, 2022
  195. otech47 commented at 12:59 pm on April 13, 2022: none
    concept ACK
  196. DrahtBot added the label Needs rebase on Apr 20, 2022
  197. in src/node/blockstorage.h:131 in 49dac436cd outdated
    124@@ -120,6 +125,14 @@ class BlockManager
    125     /** Dirty block file entries. */
    126     std::set<int> m_dirty_fileinfo;
    127 
    128+    /**
    129+     * Map from external index name to oldest block that must not be pruned.
    130+     *
    131+     * @note Internally, only blocks at height (height_first - PRUNE_LOCK_BUFFER) and
    


    ryanofsky commented at 7:52 pm on April 20, 2022:

    In commit “blockstorage: Add prune locks to BlockManager” (49dac436cd570a20ad0d9a5ea11f65f0be83a15c)

    Would be more correct to say (height_first - PRUNE_LOCK_BUFFER - 1) I think


    fjahr commented at 11:34 pm on April 24, 2022:
    Yepp, fixed.
  198. ryanofsky approved
  199. ryanofsky commented at 8:05 pm on April 20, 2022: contributor
    Code review ACK f37f2020eb3bd782206de7532b428b29e963718f. Changes since last review: cleanup to comments and initialization, test updates
  200. in src/validation.cpp:2353 in f37f2020eb outdated
    2353-            });
    2354+            int last_prune{m_chain.Height()}; // last height we can prune
    2355+            std::optional<std::string> limiting_lock; // prune lock that actually was the limiting factor, only used for logging
    2356+
    2357+            for (const auto& prune_lock : m_blockman.m_prune_locks) {
    2358+                assert(prune_lock.second.height_first < std::numeric_limits<int>::max());
    


    luke-jr commented at 8:07 pm on April 21, 2022:
    I don’t think this should assert. Seems like begging for a DoS vector.

    fjahr commented at 11:36 pm on April 24, 2022:
    It seems unlikely to me but for the sake of safety I changed it back to just continue.
  201. fjahr force-pushed on Apr 24, 2022
  202. DrahtBot removed the label Needs rebase on Apr 25, 2022
  203. in src/node/blockstorage.h:200 in b5712ecb50 outdated
    195         Unload();
    196     }
    197 };
    198 
    199+//! Check whether the block associated with this index entry is pruned or not.
    200+bool IsBlockPruned(const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
    


    luke-jr commented at 1:02 am on April 25, 2022:
    Bad rebase?

    fjahr commented at 9:49 pm on April 25, 2022:
    Yepp
  204. DrahtBot added the label Needs rebase on Apr 25, 2022
  205. refactor: Introduce GetFirstStoredBlock helper function 231fc7b035
  206. blockstorage: Add prune locks to BlockManager
    This change also introduces an aditional buffer of 10 blocks (PRUNE_LOCK_BUFFER) that will not be pruned before the best block.
    
    Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
    2561823531
  207. Index: Use prune locks for blockfilterindex
    Prior to this change blocks could be pruned up to the last block before the blockfilterindex current best block.
    f08c9fb0c6
  208. Index: Allow coinstatsindex with pruning enabled 825d19839b
  209. test: Update test for indices on pruned nodes de08932efa
  210. move-only: Rename index + pruning functional test 71c3f0356c
  211. fjahr force-pushed on Apr 25, 2022
  212. fjahr commented at 10:46 pm on April 25, 2022: contributor
    Another rebase, removed a line that slipped in during the last rebase and added a small comment.
  213. DrahtBot removed the label Needs rebase on Apr 25, 2022
  214. ryanofsky approved
  215. ryanofsky commented at 2:03 am on April 26, 2022: contributor
    Code review ACK 71c3f0356c01521a95c64fba1e7375aea6286bb0. Changes since last review: just tweaking comments and asserts, and rebasing
  216. fanquake requested review from mzumsande on Apr 26, 2022
  217. fanquake requested review from achow101 on Apr 26, 2022
  218. fanquake requested review from jamesob on Apr 26, 2022
  219. mzumsande commented at 3:38 pm on April 26, 2022: contributor
    Code review ACK 71c3f0356c01521a95c64fba1e7375aea6286bb0
  220. w0xlt approved
  221. w0xlt commented at 6:07 pm on April 26, 2022: contributor
  222. fanquake removed this from the "Blockers" column in a project

  223. fanquake merged this on Apr 26, 2022
  224. fanquake closed this on Apr 26, 2022

  225. fanquake moved this from the "Relevant External PRs" to the "Done or Closed or Rethinking" column in a project

  226. in src/node/blockstorage.cpp:409 in 71c3f0356c
    403@@ -397,6 +404,16 @@ bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex)
    404     return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
    405 }
    406 
    407+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block) {
    408+    AssertLockHeld(::cs_main);
    409+    assert(start_block);
    


    MarcoFalke commented at 6:53 am on April 27, 2022:
    Why is this function accepting a nullptr, but then aborting on it. Wouldn’t it be better to not accept a nullptr (aka reference)?

    jonatack commented at 2:28 pm on April 28, 2022:

    Why is this function accepting a nullptr, but then aborting on it. Wouldn’t it be better to not accept a nullptr (aka reference)?

    Proposed in #25016.

  227. in src/node/blockstorage.cpp:414 in 71c3f0356c
    409+    assert(start_block);
    410+    const CBlockIndex* last_block = start_block;
    411+    while (last_block->pprev && (last_block->pprev->nStatus & BLOCK_HAVE_DATA)) {
    412+        last_block = last_block->pprev;
    413+    }
    414+    return last_block;
    


    MarcoFalke commented at 6:54 am on April 27, 2022:
    Would be nice if there was a way to annotate the returned value is never nullptr

    MarcoFalke commented at 7:06 pm on April 28, 2022:
    I guess this can be achieved by deleting the move/copy constructor of CBlockIndex and then returning a reference instead of pointer.

    jonatack commented at 7:28 pm on April 28, 2022:

    I guess this can be achieved by deleting the move/copy constructor of CBlockIndex and then returning a reference instead of pointer.

    interesting

  228. in src/node/blockstorage.h:201 in 71c3f0356c
    196         Unload();
    197     }
    198 };
    199 
    200+//! Find the first block that is not pruned
    201+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
    


    MarcoFalke commented at 6:56 am on April 27, 2022:
    Why is this a global function, when it could be a member of BlockManager? I think that’d make more sense, if we longer term want to remove the cs_main lock on storage stuff and give BlockManager its own mutex to take.

    jonatack commented at 2:28 pm on April 28, 2022:

    Why is this a global function, when it could be a member of BlockManager? I think that’d make more sense, if we longer term want to remove the cs_main lock on storage stuff and give BlockManager its own mutex to take.

    Done in #25016.

  229. in src/node/blockstorage.cpp:407 in 71c3f0356c
    403@@ -397,6 +404,16 @@ bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex)
    404     return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
    405 }
    406 
    407+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block) {
    


    MarcoFalke commented at 6:57 am on April 27, 2022:
    nit: clang-format

    jonatack commented at 2:56 pm on April 28, 2022:
    Picked up these clang-formatting nits inside 8507260fd32914fa63 in #24150 and #25016.
  230. in src/node/blockstorage.cpp:235 in 71c3f0356c
    231@@ -230,6 +232,11 @@ void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPr
    232            nLastBlockWeCanPrune, count);
    233 }
    234 
    235+void BlockManager::UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) {
    


    MarcoFalke commented at 6:57 am on April 27, 2022:
    same nit
  231. in src/index/base.cpp:381 in 71c3f0356c
    376@@ -381,3 +377,14 @@ IndexSummary BaseIndex::GetSummary() const
    377     summary.best_block_height = m_best_block_index ? m_best_block_index.load()->nHeight : 0;
    378     return summary;
    379 }
    380+
    381+void BaseIndex::SetBestBlockIndex(const CBlockIndex* block) {
    


    MarcoFalke commented at 6:57 am on April 27, 2022:
    same nit
  232. in src/index/base.cpp:141 in 71c3f0356c
    133@@ -138,7 +134,7 @@ void BaseIndex::ThreadSync()
    134         int64_t last_locator_write_time = 0;
    135         while (true) {
    136             if (m_interrupt) {
    137-                m_best_block_index = pindex;
    


    MarcoFalke commented at 6:58 am on April 27, 2022:
    Haven’t tried this, but what happens if m_best_block_index is private?

    fjahr commented at 10:56 pm on April 28, 2022:
    I’m not sure I can follow: m_best_block_index is already private?
  233. MarcoFalke commented at 6:58 am on April 27, 2022: member
    some nits and questions
  234. fanquake referenced this in commit 194b414697 on Apr 29, 2022
  235. sidhujag referenced this in commit ac7c16ce90 on Apr 29, 2022
  236. sidhujag referenced this in commit cd0f3ec4a9 on Apr 29, 2022
  237. ryanofsky referenced this in commit dd2ef55a86 on Sep 30, 2022
  238. ryanofsky referenced this in commit 8891949bdc on Oct 5, 2022
  239. fanquake referenced this in commit 4175c332b9 on Oct 10, 2022
  240. sidhujag referenced this in commit f4572f9ae4 on Oct 10, 2022
  241. fanquake referenced this in commit 5ad82a09b4 on Oct 11, 2022
  242. adam2k referenced this in commit c8fca8a784 on Oct 19, 2022
  243. janus referenced this in commit f7bf2d1a4a on Jan 20, 2023
  244. aguycalled referenced this in commit 2f61f6b4bb on Jan 25, 2023
  245. Fabcien referenced this in commit 093c9f2192 on Feb 3, 2023
  246. bitcoin locked this on Aug 13, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me