Previously, cs_main had to be locked during GetBlockChecked in the getblock rpc call. This made parallel getblock calls slower, for example in Fulcrum.
By moving the pruned check out of GetBlockChecked into a separate function, cs_main is no longer locked during ReadBlockFromDisk.
It still locks cs_main if pruning is enabled to prevent a race condition (#13903), so the performance improvement will only be noticeable without pruning.
There is some way to ensure that this is a speed improvement that justify this refactoring?
Yes, I’m running a benchmark right now and will share the results. A good way to test is also trying to sync Fulcrum, an Electrum server for a more real-world scenario where getblock is used a lot.
LookupBlockIndex) happens with cs_main locked. So if it’s not written before the lock is released, the reading won’t happen.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | aureleoules |
| Concept ACK | laanwj |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
564@@ -565,14 +565,18 @@ static RPCHelpMan getblockheader()
565 };
566 }
567
568-static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
569+static void EnsureNotPruned(BlockManager& blockman, const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
nit: i think it’s clearer
0static void EnsureBlockNotPruned(BlockManager& blockman, const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
ACK c46d05144d1440889251cb01e6ae8361ce4381a8.
I’m not sure how to benchmark this either so I used the poor’s man benchmark tool time:
time seq 10000 | xargs -i -P 15 ./src/bitcoin-cli -testnet getblock 000000000000001b3a9aeab5659c3e13dbc32d771a8b4f86aeac529d51a12039
This executes 10,000 getblock requests in parallel batches of 15 on testnet.
46.48s user 21.84s system 395% cpu 17.277 total 46.44s user 21.75s system 419% cpu 16.254 total
45.73s user 21.27s system 425% cpu 15.740 total 45.74s user 21.40s system 422% cpu 15.894 total
The results are slightly faster than master but they’re consistent.
What if pruning occurs for the first time, after the lock is released, and before it is read?
IMO a better approach would be to use a temporary prune lock. This would extend the benefit to pruned nodes, and could possibly (in another PR) be used to safely re-fetch a pruned block from another peer.
Sorry I wasn’t aware of this PR until drahtbot alerted me to conflicts. You can bench with ApacheBench. Run
0ab -p data.json -n 10000 -c 16 -A user:password "http://127.0.0.1:8332/"
where data.json is
0{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e"]}
and the -c arg is the number of rpcthread you have in your bitcoin.conf. You should set rpcthread to the number of CPU threads on your bench machine. Also try with just -c 1 to make sure there’s no regression on single thread.
What if pruning occurs for the first time, after the lock is released, and before it is read?
If the file has not yet been opened, it will fail and throw an RPC error similar to if it is pruned. If the file has already been opened, then it should succeed and remove the file once the file is closed on linux. On Windows it could cause the files to not be pruned https://en.cppreference.com/w/cpp/io/c/remove. See #26316.
726@@ -722,7 +727,15 @@ static RPCHelpMan getblock()
727 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
728 }
729
730- block = GetBlockChecked(chainman.m_blockman, pblockindex);
731+ havePruned = chainman.m_blockman.m_have_pruned;
732+ if(havePruned) {
733+ EnsureBlockNotPruned(chainman.m_blockman, pblockindex);
734+ block = GetBlockChecked(pblockindex);
Is there any value in keeping the cs_main lock here. In the very rare case where the block could be deleted after EnsureBlockNotPruned but before GetBlockChecked without the lock, it seems fine to just error out as well. The only difference would be the error message.
This is done in https://github.com/bitcoin/bitcoin/pull/26308
🐙 This pull request conflicts with the target branch and needs rebase.