Skip to content

RPC: Improve performance of `getblockstats` & `getblock`

Calin Culianu requested to merge cculianu/bitcoin-cash-node:speedup_getblockstats into master

Co-Authored-By: Axel Gembe derago@gmail.com Co-Authored-By: MarcoFalke falke.marco@gmail.com

Summary

It was observed during the recent round of ScaleNet tests by @ago that the getblockstats call is very slow. This call is used by block explorers and as such the extant block explorers were timing out when issuing getblockstats on large ScaleNet blocks.

getblockstats has a bunch of problems:

  • It keeps cs_main held the entire time, which is not necessary.
  • This is particularly problematic because it must iterate over all the inputs in a block, reading their ancestor tx's from disk. This operation can take tens of seconds to minutes on very large 300k input ScaleNet blocks.
  • During this time cs_main is held and the entire node freezes.
  • What's more, it would potentially read the same tx multiple times from disk as it iterated over the inputs.
  • It also had a bunch of un-optimized UniValue API usage (this is minor).

What's changed:

  • Release cs_main as soon as we don't need it anymore. It's only needed to look-up the block. Once we have the CBlockIndex, we don't need it anymore. All of the rest of the code was reviewed carefully and is lock-free.
  • Don't call GetTransaction() because it grabs cs_main and also is very slow and redundant. It is not needed since if you examine GetTransaction() with the args we were calling it with, it ends up just using g_txindex always. So we use the lock-free g_txindex ourselves directly to find tx's. We instead backported part of Core#14802, and we use the undo (.rev) file to get previous inputs in order to calculate fees.
  • Optimized the UniValue usage to our new (faster, safer) API.
  • Leverage NRVO/RVO in C++ by returning only 1 thing in 1 branch (this is a minor optimizaton compared to all the others).

In addition, we modified getblock to release the lock earlier (was MR !850 (closed)):

  • We refactored GetBlockChecked (called in only 2 places, getblock and getblockstats) out into 2 functions: ThrowIfPrunedBlock (requires cs_main) and the slower ReadBlockChecked (does not require locks). We also added the new ReadUndoChecked for reading the inputs from the undo file.

Sorry for having 2 commits in 1 MR but the refactor was necessary, since to do it right we needed to touch getblock.

Test Plan

  • Basic: ninja check check-functional
  • Advanced: We want to illustrate the speedup here, so you will need ScaleNet for best results.
    1. Synch up a ScaleNet node, be sure to enable RPC on it in the conf file or via the right CLI arg magic. Also enable txindex=1 (you will need this only for testing the version of the client without this MR)
    2. Create a shell script similar to the below, call it cmd.sh:
#!/bin/bash

blocks="16029 16026 16037 16130 16179 15813 16124"

for ht in $blocks; do
    src/bitcoin-cli -conf=$HOME/bitcoin/bchn/scalenet.conf getblockstats $ht &
done

wait
  1. Note: Be sure to replace the -conf arg in the shell script above with the conf file you created for your scalenet node.
  2. Next, build bitcoind with this commit and start the node, wait for it to synch fully and settle. Note that the txindex takes maybe a minute to catch up if this was a full IBD you just did.
  3. Execute the following: time ./cmd.sh (if you get an error about txindex, this is normal and unrelated to this commit. Wait a bit for it to catch up and try again).
  4. You should see it spit out a bunch of JSON results and a time.
  5. Do 5-6 above again but this time with a bitcoind built against master (without this commit). Note how the time is multiple times longer, or more, depending on your machine.

This commit does not change behavior does in fact enhance the node to no longer need txindex for getblockstats, otherwise correctness of the getblockstats call is identical, and now more efficient.

Special thanks to Axel Gembe <derago@gmail.com> for his hard work in troubleshooting and investigating this. This code is largely inspired by his test code here: https://gitlab.com/ago/bitcoin-cash-node/-/tree/optimize_getblockstats

Also special thanks to Mark Lundeberg <5505128-markblundeberg@users.noreply.gitlab.com> for his tip on using the latest core changes that use the undo file rather than the (slower) txindex lookups per input.

Merge instructions: Preferably, do not squash if at all possible, maybe use this merge request message in the merge-commit message.

Edited by freetrader

Merge request reports