rpc: Faster getblock API #25232

AaronDewes · 2022-05-28T10:30:44Z

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.

vincenzopalazzo

There is some way to ensure that this is a speed improvement that justify this refactoring?

AaronDewes · 2022-05-28T18:41:01Z

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.

AaronDewes · 2022-05-29T08:33:29Z

I haven't found a good way to benchmark it except trying to sync Fulcrum and viewing the logs. Without this patch, it often exceeded the RPC work queue depth and was very slow. With this patch, it was much faster and didn't do that.

laanwj · 2022-05-30T09:20:19Z

Concept ACK on doing the block read from disk without locking, when pruning is not enabled. This seems quite low-hanging fruit for optimization. Assuming, there can't be a race condition between the block being written to disk and reading it?

AaronDewes · 2022-05-31T05:44:29Z

I don't think there is, because the check if the block exists (LookupBlockIndex) happens with cs_main locked. So if it's not written before the lock is released, the reading won't happen.

DrahtBot · 2022-06-21T01:44:22Z

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

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26415 (rpc,rest: faster getblock and rest_block by reading raw block by andrewtoth)
#21319 (RPC/Blockchain: Optimise getblock for simple disk->hex case by luke-jr)

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.

aureleoules

ACK c46d051.

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.

Master

46.48s user 21.84s system 395% cpu 17.277 total
46.44s user 21.75s system 419% cpu 16.254 total

PR

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.

aureleoules · 2022-09-23T12:31:49Z

src/rpc/blockchain.cpp

@@ -565,14 +565,18 @@ static RPCHelpMan getblockheader()
    };
 }

-static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
+static void EnsureNotPruned(BlockManager& blockman, const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)


nit: i think it's clearer

Suggested change

static void EnsureNotPruned(BlockManager& blockman, const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)

static void EnsureBlockNotPruned(BlockManager& blockman, const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)

aureleoules · 2022-09-23T14:59:59Z

You'll need to squash the new commit https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

aureleoules

ACK 599f260

luke-jr · 2022-09-24T19:28:35Z

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.

andrewtoth · 2022-10-17T01:51:17Z

Sorry I wasn't aware of this PR until drahtbot alerted me to conflicts. You can bench with ApacheBench. Run

ab -p data.json -n 10000 -c 16 -A user:password "http://127.0.0.1:8332/"

where data.json is

{"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.

maflcko · 2022-12-07T13:43:05Z

src/rpc/blockchain.cpp

+        havePruned = chainman.m_blockman.m_have_pruned;
+        if(havePruned) {
+            EnsureBlockNotPruned(chainman.m_blockman, pblockindex);
+            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 #26308

DrahtBot · 2022-12-08T09:51:09Z

🐙 This pull request conflicts with the target branch and needs rebase.

andrewtoth · 2022-12-08T14:53:06Z

@AaronDewes I think #26308 accomplishes this. I syncing Fulcrum on master faster now?

AaronDewes · 2022-12-09T14:51:09Z

Yes, it seems a lot faster, thank you!

AaronDewes changed the title ~~Faster getblock API~~ rpc: Faster getblock API May 28, 2022

DrahtBot added the RPC/REST/ZMQ label May 28, 2022

vincenzopalazzo reviewed May 28, 2022

View reviewed changes

AaronDewes force-pushed the perf/faster-getblock branch 2 times, most recently from dc29e19 to c46d051 Compare May 28, 2022 19:34

DrahtBot mentioned this pull request Jul 26, 2022

rpc: Faster getblock using PureBlock #17529

Closed

aureleoules reviewed Sep 23, 2022

View reviewed changes

Faster getblock API

599f260

AaronDewes force-pushed the perf/faster-getblock branch from fbb889a to 599f260 Compare September 23, 2022 16:01

aureleoules approved these changes Sep 23, 2022

View reviewed changes

DrahtBot mentioned this pull request Oct 17, 2022

rpc/rest/zmq: reduce LOCK(cs_main) scope: ~6 times as many requests per second #26308

Merged

andrewtoth mentioned this pull request Oct 17, 2022

use shared mutex to guard against block files being removed before read #26316

Closed

DrahtBot mentioned this pull request Oct 28, 2022

rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block #26415

Merged

DrahtBot mentioned this pull request Nov 7, 2022

RPC/Blockchain: Optimise getblock for simple disk->hex case #21319

Closed

maflcko reviewed Dec 7, 2022

View reviewed changes

DrahtBot added the Needs rebase label Dec 8, 2022

AaronDewes closed this Dec 9, 2022

bitcoin locked and limited conversation to collaborators Dec 9, 2023

	static void EnsureNotPruned(BlockManager& blockman, const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
	static void EnsureBlockNotPruned(BlockManager& blockman, const CBlockIndex* pblockindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)

rpc: Faster getblock API #25232

rpc: Faster getblock API #25232

Uh oh!

Conversation

AaronDewes commented May 28, 2022

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

AaronDewes commented May 28, 2022

Uh oh!

AaronDewes commented May 29, 2022

Uh oh!

laanwj commented May 30, 2022

Uh oh!

AaronDewes commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Master

PR

Uh oh!

aureleoules Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

aureleoules commented Sep 23, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Sep 24, 2022

Uh oh!

andrewtoth commented Oct 17, 2022

Uh oh!

maflcko Dec 7, 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!

DrahtBot commented Dec 8, 2022

Uh oh!

andrewtoth commented Dec 8, 2022

Uh oh!

AaronDewes commented Dec 9, 2022

Uh oh!

Uh oh!

AaronDewes commented May 31, 2022 •

edited

Loading

DrahtBot commented Jun 21, 2022 •

edited

Loading

maflcko Dec 7, 2022 •

edited

Loading