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

luke-jr · 2021-02-28T20:15:26Z

src/rpc/blockchain.cpp

0xB10C

Some notes and context for others looking at this PR:

The getblock <hash> has different levels of verbosity. A verbosity of 0 returns the block in hex, verbosity 1 in JSON, and further verbosity levels add more transaction details to the JSON.

When calling the getblock <hash> RPC with a verbosity of 0 we currently (before this PR) de-serialize the raw block stored on disk into a CBlock (happens in GetBlockChecked()) and then serialize it back into a hex string. This isn't ideal performance wise (block and tx are de-serialized and (I think) all transactions are hashed in CTransaction initialization).

This PR adds special handling to the verbosity 0 case to skip the CBlock de- and serialization. This works only if the block on disk is serialized the same way as the RPC serialization is configured. If you were to manually set -rpcserialversion=0 (currently version 1 (SegWit) by default), then special case wouldn't work.

Something very similar has been implemented in #13151 for P2P block serving. @MarcoFalke opened PR #17529 attempting to improve the performance (10%-25% on Intel CPUs) by leaving out the CTransaction hashing where not needed.

Comparison:

master:   disk -> deserialize -> hash -> serialize -> hex string
#17529:   disk -> deserialize -> ____ -> serialize -> hex string
this PR:  disk -> ___________ -> ____ -> _________ -> hex string

As @promag mentioned in #13151 (review) in regards to P2P block serving, skipping the de- and serialization here could potentially feed the RPC client corrupted blocks if the data on the disk is corrupted. Can't really judge how problematic that is for RPC.

src/rpc/blockchain.cpp

DrahtBot · 2021-07-28T21:39:53Z

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
Concept ACK	andrewtoth

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)
#25232 (rpc: Faster getblock API by AaronDewes)

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.

luke-jr · 2021-07-28T22:06:40Z

Closing due to lack of interest. Can reopen if people want to review.

andrewtoth · 2022-11-05T16:46:21Z

Concept ACK. This RPC is used by a lot of projects and optimizing it would be a big benefit.

I think the main problem though is serving potentially corrupted blocks. It was suggested in #13151 (comment) that CRC32Cs can be added to the on disk blocks, which I think would be sufficient. How to do that though I'm not sure of. Ideally they would be in the meta header of the on disk block. Adding them to every block on disk would require rewriting the entire blockchain on already synced nodes, which would take a very long time and not allow downgrading. We would also need to update code that assumes a fixed 8 byte header before the block. Maybe we could add a block file version and only do this for blocks added to the new version type.

Alternatively it could be added to the block index. That would take less time to upgrade but would require a migration that would not allow downgrading.

I tried adding CRCs as a new index type in #26415, but that requires storing all the 4 byte CRCs along with the 32 byte block hash so it's a lot more data on disk. It also adds the CRCs after the block is stored on the scheduler thread, so potentially a system that got notified of the new block and called getblock wouldn't have the CRC yet.

luke-jr · 2022-11-05T17:07:12Z

I don't think we need CRCs to make this change. The code already assumes no corruption in lots of other places.

Rebased

andrewtoth · 2022-11-05T17:25:01Z

The only other place using ReadRawBlockFromDisk is in net processing, where we assume the peer will validate correctly and disconnect. The same assumption can't be made for consumers of the RPC. All other block reads deserialize the block which will fail if the data is corrupt.

luke-jr · 2022-11-05T17:48:12Z

What happens if you corrupt the chainstate?

andrewtoth · 2022-11-06T14:37:29Z

Chainstate is separate from blocksdir. They can live on a different storage device. If an old block file gets corrupted it doesn't affect the chainstate.

luke-jr · 2022-11-06T17:55:18Z

Nevertheless, I don't think we guarantee anything in the case of disk corruption, nor should or can we.

andrewtoth · 2022-11-06T18:39:18Z

Well, I think currently we do guarantee we won't serve a corrupt block when we use ReadBlockFromDisk.

DrahtBot · 2022-12-08T09:58:10Z

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

maflcko · 2022-12-08T11:38:40Z

Well, I think currently we do guarantee we won't serve a corrupt block when we use ReadBlockFromDisk.

Do we?

The block header is checked, but not the transaction payload itself

andrewtoth · 2022-12-08T16:43:38Z

The block header is checked, but not the transaction payload itself

Indeed, I tested by modifying https://github.com/bitcoin/bitcoin/pull/26415/files#diff-186034bc1b6a0e662a59c81182b3d4a0318d44dc28810d2b837c92033e0d5630R142 in my PR to use ReadBlockFromDisk and it passes, but if I switch it to a byte inside the header (like 50), it fails. Looks like I was mistaken. The risk is just limited to the header then. Would it be worth it to modify ReadRawBlockFromDIsk to check the header hash at least then, or does that remove enough of the speed benefit here?

luke-jr · 2022-12-11T18:08:21Z

I don't think there's a benefit to checking the header, or trying to plan for corrupt data in general (aside from startup verification of high-risk/recent data).

maflcko

lgtm

maflcko · 2022-12-12T08:21:25Z

src/rpc/blockchain.cpp

+
+        if (verbosity <= 0 && !RPCSerializationFlags()) {
+            // This one case doesn't need to parse the block at all
+            if (!node::ReadRawBlockFromDisk(raw_block, pblockindex->GetBlockPos(), Params().MessageStart())) {


nit: Maybe move this into a GetRawBlockChecked to avoid the duplicate pruned check when GetBlockChecked below is called?

fanquake · 2023-02-06T15:57:36Z

Still needs a rebase. Maybe someone wants to take this over?

andrewtoth · 2023-02-06T16:09:11Z

I can add this functionality to #26415.

fanquake · 2023-02-06T16:28:10Z

I can add this functionality to #26415.

@andrewtoth ok. Will close this for now.

1c4b9cb bench: add readblock benchmark (Andrew Toth) Pull request description: Requested in bitcoin/bitcoin#13151 (comment). See bitcoin/bitcoin#26415 and bitcoin/bitcoin#21319. Benchmarking shows a >50x increase in speed on both nvme and spinning disk. Benchmark results: | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 5,377,375.00 | 185.96 | 0.2% | 60,125,513.00 | 11,633,676.00 | 5.168 | 3,588,800.00 | 0.4% | 0.09 | `ReadBlockFromDiskTest` | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 89,945.58 | 11,117.83 | 0.7% | 12,743.90 | 64,530.33 | 0.197 | 2,595.20 | 0.2% | 0.01 | `ReadRawBlockFromDiskTest` ACKs for top commit: maflcko: lgtm ACK 1c4b9cb achow101: ACK 1c4b9cb TheCharlatan: ACK 1c4b9cb Tree-SHA512: 71dbcd6c7e2be97eb3001e35d0a95ef8e0c9b10dc9193025c7f8e11a09017fa2fbf89489b686353cd88fb409fb729fe2c4a25c567d2988f64c9c164ab09fba9f

DrahtBot added the RPC/REST/ZMQ label Feb 28, 2021

luke-jr force-pushed the getblock_optimise branch from d30f7d9 to 33cead5 Compare February 28, 2021 20:30

0xB10C suggested changes Mar 3, 2021

View reviewed changes

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

luke-jr force-pushed the getblock_optimise branch from 33cead5 to 7b23e32 Compare March 3, 2021 23:37

0xB10C reviewed Mar 4, 2021

View reviewed changes

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

DrahtBot mentioned this pull request Jul 28, 2021

refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager #22564

Merged

luke-jr closed this Jul 28, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

andrewtoth mentioned this pull request Oct 30, 2022

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

Merged

bitcoin unlocked this conversation Oct 30, 2022

luke-jr reopened this Nov 5, 2022

luke-jr force-pushed the getblock_optimise branch from 7b23e32 to 43882cf Compare November 5, 2022 15:55

DrahtBot added the Needs rebase label Nov 5, 2022

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

6822667

luke-jr force-pushed the getblock_optimise branch from 43882cf to 6822667 Compare November 5, 2022 17:06

DrahtBot removed the Needs rebase label Nov 5, 2022

This was referenced Nov 7, 2022

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

Merged

rpc: Faster getblock API #25232

Closed

DrahtBot added the Needs rebase label Dec 8, 2022

andrewtoth mentioned this pull request Dec 11, 2022

bench: add readblock benchmark #26684

Merged

maflcko reviewed Dec 12, 2022

View reviewed changes

maflcko added the Up for grabs label Feb 6, 2023

fanquake closed this Feb 6, 2023

fanquake removed the Up for grabs label Feb 6, 2023

bitcoin locked and limited conversation to collaborators Feb 6, 2024

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

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

Uh oh!

Conversation

luke-jr commented Feb 28, 2021

Uh oh!

Uh oh!

0xB10C left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

luke-jr commented Jul 28, 2021

Uh oh!

andrewtoth commented Nov 5, 2022

Uh oh!

luke-jr commented Nov 5, 2022

Uh oh!

andrewtoth commented Nov 5, 2022

Uh oh!

luke-jr commented Nov 5, 2022

Uh oh!

andrewtoth commented Nov 6, 2022

Uh oh!

luke-jr commented Nov 6, 2022

Uh oh!

andrewtoth commented Nov 6, 2022

Uh oh!

DrahtBot commented Dec 8, 2022

Uh oh!

maflcko commented Dec 8, 2022

Uh oh!

andrewtoth commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Dec 11, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

fanquake commented Feb 6, 2023

Uh oh!

andrewtoth commented Feb 6, 2023

Uh oh!

fanquake commented Feb 6, 2023

Uh oh!

Uh oh!

DrahtBot commented Jul 28, 2021 •

edited

Loading

andrewtoth commented Dec 8, 2022 •

edited

Loading