Improve Indices on pruned nodes via prune blockers #21726

fjahr · 2021-04-18T23:56:49Z

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.

DrahtBot · 2021-04-19T03:52:24Z

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.

src/validation.cpp

laanwj · 2021-05-17T18:27:31Z

Concept ACK, I think this is an elegant approach.

achow101

ACK 11f01ef

src/validation.cpp

promag

Concept ACK.

src/node/blockstorage.cpp

src/validation.h

src/validation.cpp

src/index/base.cpp

fjahr · 2021-05-26T23:41:04Z

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.

promag

How about keeping some blocks before the deepest prune blocker?

fjahr · 2021-06-01T23:17:54Z

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.

ryanofsky

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

292d508 refactor: Introduce GetLastPruneBlock helper function (1/7)
9f8d6a9 Add prune blockers to BlockManager (2/7)
c07a567 Index: Use prune blockers for blockfilterindex (3/7)
395757f Index: Allow coinstatsindex with pruning enabled (4/7)
6aad867 test: Update test for indices on pruned nodes (5/7)
596f738 move-only: Rename index + pruning functional test (6/7)
e365e87 Index: Skip pruning checks when node is not pruning (7/7)

src/rpc/blockchain.cpp

src/validation.h

src/validation.cpp

fjahr · 2022-04-25T22:46:27Z

Another rebase, removed a line that slipped in during the last rebase and added a small comment.

ryanofsky

Code review ACK 71c3f03. Changes since last review: just tweaking comments and asserts, and rebasing

mzumsande · 2022-04-26T15:38:56Z

Code review ACK 71c3f03

w0xlt

tACK 71c3f03 on signet.

maflcko

some nits and questions

maflcko · 2022-04-27T06:53:31Z

src/node/blockstorage.cpp

@@ -397,6 +404,16 @@ bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex)
    return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
 }

+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block) {
+    AssertLockHeld(::cs_main);
+    assert(start_block);


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

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.

maflcko · 2022-04-27T06:54:23Z

src/node/blockstorage.cpp

+    while (last_block->pprev && (last_block->pprev->nStatus & BLOCK_HAVE_DATA)) {
+        last_block = last_block->pprev;
+    }
+    return last_block;


Would be nice if there was a way to annotate the returned value is never nullptr

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

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

interesting

maflcko · 2022-04-27T06:56:04Z

src/node/blockstorage.h

    ~BlockManager()
    {
        Unload();
    }
 };

+//! Find the first block that is not pruned
+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);


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.

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.

maflcko · 2022-04-27T06:57:01Z

src/node/blockstorage.cpp

@@ -397,6 +404,16 @@ bool BlockManager::IsBlockPruned(const CBlockIndex* pblockindex)
    return (m_have_pruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0);
 }

+const CBlockIndex* GetFirstStoredBlock(const CBlockIndex* start_block) {


nit: clang-format

Picked up these clang-formatting nits inside 8507260 in #24150 and #25016.

maflcko · 2022-04-27T06:57:10Z

src/node/blockstorage.cpp

@@ -230,6 +232,11 @@ void BlockManager::FindFilesToPrune(std::set<int>& setFilesToPrune, uint64_t nPr
           nLastBlockWeCanPrune, count);
 }

+void BlockManager::UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) {


maflcko · 2022-04-27T06:57:22Z

src/index/base.cpp

@@ -381,3 +377,14 @@ IndexSummary BaseIndex::GetSummary() const
    summary.best_block_height = m_best_block_index ? m_best_block_index.load()->nHeight : 0;
    return summary;
 }
+
+void BaseIndex::SetBestBlockIndex(const CBlockIndex* block) {


maflcko · 2022-04-27T06:58:20Z

src/index/base.cpp

@@ -138,7 +134,7 @@ void BaseIndex::ThreadSync()
        int64_t last_locator_write_time = 0;
        while (true) {
            if (m_interrupt) {
-                m_best_block_index = pindex;


Haven't tried this, but what happens if m_best_block_index is private?

I'm not sure I can follow: m_best_block_index is already private?

…ollow-ups e2b954e rpc: use GetBlockTime() for getblockchaininfo#time (Jon Atack) 86ce844 blockstorage, refactor: pass GetFirstStoredBlock() start_block by reference (Jon Atack) ed12c0a blockstorage, refactor: make GetFirstStoredBlock() a member of BlockManager (Jon Atack) Pull request description: Picks up the remaining review feedback in #21726 and #24956. - make the global function `GetFirstStoredBlock()` a member of the `BlockManager` class - pass the `start_block` param of `GetFirstStoredBlock()` by reference instead of a pointer - use `GetBlockTime()` for RPC getblockchaininfo#time ACKs for top commit: MarcoFalke: ACK e2b954e Tree-SHA512: 546e3c2e18245996b5b286829a605ae919eff3510963ec71b7c9ede521b1f501697e5b2f9d35d7a0606a74cbc8907201c58acf1e2cf7daaa86eefe2e3a8e296b

…ninfo follow-ups e2b954e rpc: use GetBlockTime() for getblockchaininfo#time (Jon Atack) 86ce844 blockstorage, refactor: pass GetFirstStoredBlock() start_block by reference (Jon Atack) ed12c0a blockstorage, refactor: make GetFirstStoredBlock() a member of BlockManager (Jon Atack) Pull request description: Picks up the remaining review feedback in bitcoin#21726 and bitcoin#24956. - make the global function `GetFirstStoredBlock()` a member of the `BlockManager` class - pass the `start_block` param of `GetFirstStoredBlock()` by reference instead of a pointer - use `GetBlockTime()` for RPC getblockchaininfo#time ACKs for top commit: MarcoFalke: ACK e2b954e Tree-SHA512: 546e3c2e18245996b5b286829a605ae919eff3510963ec71b7c9ede521b1f501697e5b2f9d35d7a0606a74cbc8907201c58acf1e2cf7daaa86eefe2e3a8e296b

Since commit f08c9fb from PR bitcoin#21726, the `BlockUntilSyncedToCurrentChain` behavior has been less reliable and also introduced a race condition in the coinstatsindex_initial_sync unit test. It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable. Also since commit f08c9fb, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in bitcoin#25365 (comment) This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, bitcoin#26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors bitcoin#25365. There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: MacroFake <falke.marco@gmail.com>

Since commit f08c9fb from PR bitcoin#21726, index `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test. It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable. Also since commit f08c9fb, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in bitcoin#25365 (comment) This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, bitcoin#26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors bitcoin#25365. There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: MacroFake <falke.marco@gmail.com>

…edToCurrentChain reliability 8891949 index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Ryan Ofsky) Pull request description: Since commit f08c9fb from PR bitcoin/bitcoin#21726, index `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test. It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable. Also since commit f08c9fb, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in bitcoin/bitcoin#25365 (comment) This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, bitcoin/bitcoin#26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors bitcoin/bitcoin#25365. There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down. Co-authored-by: vasild Co-authored-by: MarcoFalke ACKs for top commit: mzumsande: re-ACK 8891949 Tree-SHA512: 52e29e3772a0c92873c54e5ffb31dd66a909b68a2031b7585713cd1d976811289c98bd9bb41679a8689062f03be4f97bb8368696e789caa4607c2fd8b1fe289b

…entChain reliability 8891949 index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Ryan Ofsky) Pull request description: Since commit f08c9fb from PR bitcoin#21726, index `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test. It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable. Also since commit f08c9fb, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in bitcoin#25365 (comment) This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, bitcoin#26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors bitcoin#25365. There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down. Co-authored-by: vasild Co-authored-by: MarcoFalke ACKs for top commit: mzumsande: re-ACK 8891949 Tree-SHA512: 52e29e3772a0c92873c54e5ffb31dd66a909b68a2031b7585713cd1d976811289c98bd9bb41679a8689062f03be4f97bb8368696e789caa4607c2fd8b1fe289b

Since commit f08c9fb from PR bitcoin#21726, index `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test. It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable. Also since commit f08c9fb, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in bitcoin#25365 (comment) This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, bitcoin#26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors bitcoin#25365. There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: MacroFake <falke.marco@gmail.com> Github-Pull: bitcoin#26215 Rebased-From: 8891949

Since commit f08c9fb from PR bitcoin#21726, index `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test. It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable. Also since commit f08c9fb, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in bitcoin#25365 (comment) This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, bitcoin#26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors bitcoin#25365. There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: MacroFake <falke.marco@gmail.com>

Since commit f08c9fb0c6a799e3cb75ca5f763a746471625beb from PR bitcoin/bitcoin#21726, index `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test. It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable. Also since commit f08c9fb0c6a799e3cb75ca5f763a746471625beb, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in bitcoin/bitcoin#25365 (comment) This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, bitcoin/bitcoin#26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors bitcoin/bitcoin#25365. There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: MacroFake <falke.marco@gmail.com>

* doc: add info about status code 404 for some rest endpoints * refactor: remove duplicate code from BlockAssembler * build: Fix `capnp` package build for Android * build: Specify native binaries explicitly when building `capnp` package From `configure --help`: --with-external-capnp use the system capnp binary (or the one specified with $CAPNP) instead of compiling a new one (useful for cross-compiling) * net: remove useless call to IsReachable() from CConnman::Bind() `CConnman::Bind()` is called without `BF_EXPLICIT` only when passed either `0.0.0.0` or `::`. For those addresses `IsReachable()` is always true (regardless of the `-onlynet=` setting!), meaning that the `if` condition never evaluates to true. `IsReachable()` is always true for the "any" IPv4 and IPv6 addresses because `CNetAddr::GetNetwork()` returns `NET_UNROUTABLE` instead of `NET_IPV4` or `NET_IPV6` and the network `NET_UNROUTABLE` is always considered reachable. It follows that `BF_EXPLICIT` is unnecessary, remove it too. * sync: simplify MaybeCheckNotHeld() definitions by using a template Reduce 4 of the `MaybeCheckNotHeld()` definitions to 2 by using a template. This also makes the function usable for other [BasicLockable](https://en.cppreference.com/w/cpp/named_req/BasicLockable) types. * sync: remove unused template parameter from ::UniqueLock The template parameter `typename Base = typename Mutex::UniqueLock` is not used, so remove it. Use internally defined type `Base` to avoid repetitions of `Mutex::UniqueLock`. * scripted-diff: Rename time symbols -BEGIN VERIFY SCRIPT- ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ':(exclude)src/versionbits.cpp') ; } ren nStart time_start ren nTimeStart time_start ren nTimeReadFromDiskTotal time_read_from_disk_total ren nTimeConnectTotal time_connect_total ren nTimeFlush time_flush ren nTimeChainState time_chainstate ren nTimePostConnect time_post_connect ren nTimeCheck time_check ren nTimeForks time_forks ren nTimeConnect time_connect ren nTimeVerify time_verify ren nTimeUndo time_undo ren nTimeIndex time_index ren nTimeTotal time_total ren nTime1 time_1 ren nTime2 time_2 ren nTime3 time_3 ren nTime4 time_4 ren nTime5 time_5 ren nTime6 time_6 ren nBlocksTotal num_blocks_total # Newline after semicolon perl -0777 -pi -e 's/; time_connect_total/;\n time_connect_total/g' src/validation.cpp perl -0777 -pi -e 's/; time_/;\n time_/g' src/validation.cpp -END VERIFY SCRIPT- * Use steady clock for bench logging * doc: add historical 0.20.2 release notes * doc: add historical 0.21.2 release notes * build: split ARM crc & crypto extension checks We currently perform the same check twice, to put the same set of flags in two different variables. Split the checks so we test for crc and crypto extensions independently. If we don't want to split, we should just delete the second AX_CHECK_COMPILE_FLAG check, and set ARM_CRC_CXXFLAGS & ARM_CRC_CXXFLAGS at the same time. * refactor: Do not discard `try_lock()` return value Microsoft's C++ Standard Library uses the `[[nodiscard]]` attribute for `try_lock()`. See: https://github.com/microsoft/STL/blob/main/stl/inc/mutex This change allows to drop the current suppression for the warning C4838 and helps to prevent the upcoming warning C4858. See: microsoft/STL@539c26c * signet/miner: reduce default interblock interval limit to 30min Also allow the operator to change it, if desired, without having to edit the code. * Squashed 'src/leveldb/' changes from 22f1e4a02f..e2f10b4e47 e2f10b4e47 Merge bitcoin-core/leveldb-subtree#34: win32: fix -Wmissing-field-initializers warnings 12c52b392d win32: fix -Wmissing-field-initializers warnings git-subtree-dir: src/leveldb git-subtree-split: e2f10b4e47bc950a81bc96d1c6db3a8048216642 * ci: Bump vcpkg to the latest version `2022.09.27` Dependency changes (2022.06.16.1 - 2022.09.27): - boost 1.79.0#0 -> 1.80.0#0 - sqlite3 3.37.2#1 -> 3.39.2#0 - zeromq 4.3.4#5 -> 4.3.4#6 * contrib: Fix capture_output in getcoins.py Our required Python version 3.6.12 does not support `capture_output` as a subprocess.run argument; this was added in python 3.7. We can emulate it by setting stdout and stderr to subprocess.PIPE * fuzz: Limit outpoints.size in txorphan target to avoid OOM * refactor: move Boost datetime usage to wallet This means we don't need datetime in a --disable-wallet build, and it isn't included in the kernel. * wallet: Use correct effective value when checking target * test: Check external coin effective value is used in CoinSelection * test: Use proper Boost macros instead of assertions * ci: Run `bench_bitcoin.exe --sanity-check` in "Win64 native" task Also a better name used for the script as it follows GNU's `make check`. * doc: bump bips.md up-to-date version to v24.0 This is a trivial follow-up to bitcoin#26124. * refactor: Drop `owns_lock()` call Co-authored-by: Vasil Dimov <vd@FreeBSD.org> * refactor: move DEFAULT_TXINDEX from validation to txindex * refactor: move DEFAULT_COINSTATSINDEX from validation to coinstatsindex * refactor: move DEFAULT_BLOCKFILTERINDEX from val to blockfilterindex * kernel: remove util/bytevectorhash.cpp * ci: Move `git config` commands into script where they are used * ci: Use same `merge_script` implementation for Windows as for all * ci: Remove unused package Address feedback from https://github.com/bitcoin/bitcoin/pull/24561/files#r985719812 * ci: Allow PIP_PACKAGES on centos This was added in 7fc5e86 but I can't see a reason why this should be forbidden. * test: Remove unused fCheckpointsEnabled from miner_tests The earliest checkpoint is at height 11111, so this can't possibly have any impact on this test. * build, msvc: Enable C4834 warning https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/c4834 * kernel: move RunCommandParseJSON to its own file Because libbitcoinkernel does not include this new object, this has the side-effect of eliminating the unnecessary boost::process dependency. * ci: Workaround Windows filesystem executable bit loss * fuzz: add util/mempool/h.cpp Moving the mempool code (Boost) out of util.h, results in a ~10% speedup (for me) when compiling the fuzz tests. * fuzz: pass max fee into ConsumeTxMemPoolEntry * refactor: Make 64-bit shift explicit Also this change enables MSVC warning C4334 for all codebase. https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334 * refactor: move run_command from util to common Quoting ryanofsky: "util can be the library for things included in the kernel which the kernel can depend on, and common can be the library for other code that needs to be shared internally, but should not be part of the kernel or shared externally." * Remove clang-format from lint task clang-format could be used in scripted diffs, but remained largely unused. * test: Pass mempool reference to AssemblerForTest * test: Use dedicated mempool in TestPrioritisedMining No need for a shared mempool. Also remove unused chainparams parameter. * test: Use dedicated mempool in TestPackageSelection No need for a shared mempool. Also remove unused chainparams parameter. * test: Use dedicated mempool in TestBasicMining No need for a shared mempool. Also remove unused chainparams parameter. Can be reviewed with --ignore-all-space * refactor: mempool: add MemPoolLimits::NoLimits() There are quite a few places in the codebase that require us to construct a CTxMemPool without limits on ancestors and descendants. This helper function allows us to get rid of all that duplication. * refactor: mempool: use CTxMempool::Limits Simplifies function signatures by removing repetition of all the ancestor/descendant limits, and increases readability by being more verbose by naming the limits, while still reducing the LoC. * test: use NoLimits() in MempoolIndexingTest The (100, 1000000, 1000, 1000000) limits are arbitrarily high and don't restrict anything, they are just meant to calculate ancestors properly. Using NoLimits() makes this intent more clear and simplifies the code. * docs: improve docs where MemPoolLimits is used * Remove unused CDataStream::rdbuf method It is unused and seems unlikely to be ever used. * index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability Since commit f08c9fb from PR bitcoin#21726, index `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test. It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable. Also since commit f08c9fb, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in bitcoin#25365 (comment) This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, bitcoin#26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors bitcoin#25365. There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down. Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: MacroFake <falke.marco@gmail.com> * test: Prevent UB in `minisketch_tests.cpp` * test: Remove confusing DUMMY_P2WPKH_SCRIPT * docs: fix m_children to be a member of CTxMemPoolEntry * wallet: have prune error take precedence over assumedvalid From Russ Yanofsky: "Agree with all of Marco's points here and think this should be updated If havePrune and hasAssumedValidChain are both true, better to show havePrune error message. Assumed-valid error message is vague and not very actionable. Would suggest "Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}" Co-authored-by: MacroFake <MarcoFalke@gmail.com> Co-authored-by: Russ Yanofsky <russ@yanofsky.org> * Fix comment typos in scriptpubkeyman.cpp, wallet.cpp, wallet.h Fix comment typos: sigature -> signature ponter -> pointer it's key -> its key * sync: avoid confusing name overlap (Mutex) Use `MutexType` instead of `Mutex` for the template parameter of `UniqueLock` because there is already a class named `Mutex` and the naming overlap is confusing. `MutexType` is used elsewhere in `sync.h`. * sync: remove DebugLock alias template Use `UniqueLock` directly. Type deduction works just fine from the first argument to the constructor of `UniqueLock`, so there is no need to repeat ```cpp UniqueLock<typename std::remove_reference<typename std::remove_pointer<decltype(cs)>::type>::type> ``` five times in the `LOCK` macros. Just `UniqueLock` suffices. * sync: rename AnnotatedMixin::UniqueLock to AnnotatedMixin::unique_lock This avoids confusion with the global `UniqueLock` and the snake case is consistent with `UniqueLock::reverse_lock. * iwyu: Add zmq source files * Update .cirrus.yml Co-authored-by: brunoerg <brunoely.gc@gmail.com> Co-authored-by: James O'Beirne <james.obeirne@pm.me> Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Co-authored-by: Vasil Dimov <vd@FreeBSD.org> Co-authored-by: MacroFake <falke.marco@gmail.com> Co-authored-by: fanquake <fanquake@gmail.com> Co-authored-by: Anthony Towns <aj@erisian.com.au> Co-authored-by: willcl-ark <will8clark@gmail.com> Co-authored-by: Aurèle Oulès <aurele@oules.com> Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com> Co-authored-by: glozow <gloriajzhao@gmail.com> Co-authored-by: Cory Fields <cory-nospam-@coryfields.com> Co-authored-by: stickies-v <stickies-v@protonmail.com> Co-authored-by: Ryan Ofsky <ryan@ofsky.org> Co-authored-by: MacroFake <MarcoFalke@gmail.com> Co-authored-by: Russ Yanofsky <russ@yanofsky.org> Co-authored-by: Dimitris Tsapakidis <dimitris@tsapakidis.com> Co-authored-by: alex v <alex@encrypt-s.com>

Summary: This is a partial backport of [[bitcoin/bitcoin#21726 | core#21726]] bitcoin/bitcoin@231fc7b Depends on D13096 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D13097

fjahr mentioned this pull request Apr 19, 2021

Prune locks #19463

Draft

fjahr force-pushed the prune_improve branch from b3e9c13 to 23fb6fe Compare April 19, 2021 00:03

DrahtBot added RPC/REST/ZMQ UTXO Db and Indexes Validation labels Apr 19, 2021

maflcko reviewed Apr 19, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

maflcko mentioned this pull request Apr 19, 2021

refactor: Create blockstorage module #21575

Merged

This was referenced Apr 19, 2021

Add fee_est tool for debugging fee estimation code #10443

Closed

Coinstats Index #19521

Merged

[Bundle 6/n] Prune g_chainman usage in auxiliary modules #21767

Merged

fjahr force-pushed the prune_improve branch from 23fb6fe to c3acc71 Compare May 13, 2021 18:54

fjahr marked this pull request as ready for review May 13, 2021 19:04

fjahr force-pushed the prune_improve branch from c3acc71 to 7cb32c3 Compare May 13, 2021 19:04

fjahr changed the title ~~Add prune blockers to BlockManager~~ Improve Indices on pruned nodes via prune blockers May 13, 2021

fjahr force-pushed the prune_improve branch from 7cb32c3 to 11f01ef Compare May 13, 2021 20:27

achow101 reviewed May 24, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

fjahr force-pushed the prune_improve branch from 11f01ef to b586242 Compare May 24, 2021 22:13

promag reviewed May 24, 2021

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/index/base.cpp Outdated Show resolved Hide resolved

fjahr force-pushed the prune_improve branch from b586242 to 50cd985 Compare May 26, 2021 23:36

promag reviewed May 26, 2021

View reviewed changes

DrahtBot mentioned this pull request May 28, 2021

assumeutxo #15606

Closed

18 tasks

DrahtBot added the Needs rebase label Jun 1, 2021

fjahr force-pushed the prune_improve branch from 50cd985 to e365e87 Compare June 1, 2021 23:18

DrahtBot removed the Needs rebase label Jun 2, 2021

ryanofsky reviewed Jun 2, 2021

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

ryanofsky reviewed Jun 2, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

fjahr force-pushed the prune_improve branch from b5712ec to 71c3f03 Compare April 25, 2022 21:49

DrahtBot removed the Needs rebase label Apr 25, 2022

ryanofsky approved these changes Apr 26, 2022

View reviewed changes

fanquake requested review from mzumsande, achow101 and jamesob April 26, 2022 09:07

w0xlt approved these changes Apr 26, 2022

View reviewed changes

fanquake merged commit 34ae04d into bitcoin:master Apr 26, 2022

maflcko reviewed Apr 27, 2022

View reviewed changes

jonatack mentioned this pull request Apr 28, 2022

refactor: GetFirstStoredBlock() and getblockchaininfo follow-ups #25016

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 29, 2022

Merge bitcoin#21726: Improve Indices on pruned nodes via prune blockers

ac7c16c

dongcarl mentioned this pull request May 3, 2022

[kernel 2a/n] Split hashing/index GetUTXOStats codepaths, decouple from coinstatsindex #24410

Merged

1 task

ryanofsky mentioned this pull request May 12, 2022

Intermittent win64 CI failure in feature_index_prune.py #25031

Closed

ryanofsky mentioned this pull request Sep 29, 2022

test: silence TSAN false positive in coinstatsindex_initial_sync #26188

Merged

ryanofsky mentioned this pull request Sep 30, 2022

index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability #26215

Merged

Sjors mentioned this pull request Jan 20, 2023

Add NODE_P2P_V2 to filters sipa/bitcoin-seeder#102

Merged

bitcoin locked and limited conversation to collaborators Aug 13, 2023

Improve Indices on pruned nodes via prune blockers #21726

Improve Indices on pruned nodes via prune blockers #21726

Uh oh!

Conversation

fjahr commented Apr 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Background

Alternative approach

Uh oh!

DrahtBot commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

laanwj commented May 17, 2021

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjahr commented May 26, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jun 1, 2021

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjahr commented Apr 25, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Apr 26, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

fjahr commented Apr 18, 2021 •

edited

Loading

DrahtBot commented Apr 19, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading

jonatack Apr 28, 2022 •

edited

Loading

jonatack Apr 28, 2022 •

edited

Loading

jonatack Apr 28, 2022 •

edited

Loading