Use output-then-input block validation before fork (with tests) #244
Conversation
|
Preliminary n=1 benchmark results: a 300 second bitcoind --reindex-chainstate run gets to progress=0.018081 with the previous commit, and to progress=0.017903 with commit e292694, a 1% reduction in performance. That slowdown very well could be OS caching effects (I did the e292694 test first) or me doing more webbrowsing during the second test. It also could be a real performance hit due to the position memos. In any case, performance without parallelization is not substantially worse with this commit, at least for early blocks. |
|
Is the impetus here that we could parallelize this in the future for faster syncing? |
|
@schancel Almost. Initial sync is already pretty well parallelized. However, after that, there's not much parallelization of verification for blocks yet. This commit would be the first step in achieving embarrassing parallelization for that. I have already got working code based off of this commit that will parallelize the insertion of outputs step under protection of locks, and have verified the correctness of this implementation. I have half-finished doing the same for the inputs step. Due to lock overhead and lock contention, this implementation is currently slower than the serial processing method, but that was expected. Edit: After making my locks finer-grained, this is no longer true. Parallel implementation of output insertion is now a couple percent faster overall than the serial method. The next step after the parallelization will be switching to an atomics-based concurrent hashtable or trie for UTXO storage. This will eliminate the locks, and should allow performance to far exceed what is possible serially. Incidentally, I also found a modest and simple optimization on the serial method last night. If we pull the coinbase transaction processing out of the main loops, that saves an unnecessary if statement in the inner loop and improves performance by about 9%. |
|
I just finished a moderate rewrite/rearrangement of ConnectBlock() that adds parallelization via OpenMP. It can be compiled with CXXFLAGS='-O2 -fopenmp' to enable parallelization, or compiled without -fopenmp for a serial version. The serial compilation of that branch is currently 2.5% faster than the master branch due to some mild refactoring. The parallel compilation is currently 6.3% to 53% slower (depending on thread count), at least during IBD, due to widespread locking and due to scheduler competition with the script validation threads. I'll submit it as a WIP PR in a bit, but for now that code can be found here: https://github.com/jtoomim/bitcoin-abc/commits/openmp_connectblock If there's interest, I can try to spruce up the serialized version of that code and submit it here for merging. I had another version that was 8% faster than master, so I could get it back to that level if needed. If there's not interest, then I'll close this PR and focus on the parallel version, and try to get rid of the locks by switching to lock-free hashtables. |
|
I've collected some performance data comparing the one-loop version to the two-loop non-OTI version (master HEAD) to my two-loop OTI version. The tl;dr is that the one-loop version is the fastest, my serialized two-loop OTI version is only very slightly slower (0.5%?), and the two-loop non-OTI version in ABC master is the slowest, and sometimes by a significant margin depending on the test (10%, maybe more). |
|
As CTOR has been activated, this PR is basically obsolete. Closing. |
Summary --- This fixes issue Bitcoin-ABC#244. The core of the problem encountered in the issue was that rows for block headers were missing from leveldb (likely due to some sort of corruption). The block index loading code assumed that all headers referenced by other headers (`hashPrevBlock`) would be present. If they are not, it would never detect this and just crash later (with an assertion failure) when building the skiplist. See issue Bitcoin-ABC#244. This MR adds a check after the block index is loaded to ensure that ALL of the headers in the map are not "0 initted" (a 0-initted header can only happen if a leveldb row was missing for that header). Note that an invariant in this app is that headers that are referenced as previous headers from *other* headers should never be missing from the DB, (even in a weird pruning situation) so this check is safe. This extra sanity check adds no more than a few milliseconds to the loader process for 1.5 million headers on my setup here, and it at least makes the app produce a sensible error message if the sanity check fails, rather than a crash as seen in Bitcoin-ABC#244. With this sanity check in place, in bitcoin-qt it actually leads to a dialog box asking you if you want to reindex! (Which is better than what we had before: a crash). Test Plan --- You are going to need to reproduce the situation encountered in issue Bitcoin-ABC#244. To reproduce the issue, try this against master: 1. Set up a datadir, ideally synched (but it doesn't have to be to reproduce the crash itself). 2. Copy the `index` folder from the tarball linked in issue Bitcoin-ABC#244, replacing your datadir/block/index folder completely with it (be sure to rename your index folder first or to delete it to get it out of the way!). 3. Start bitcoind or bitcoin-qt and observe it crash with an assertion failure. *Alternate method*: If the above is too complicated -- you can also reproduce the crash by randomly deleting a row in the leveldb blocks/index database. You will need to use an external tool to do this and you can pick a random blockindex row to delete (all rows starting with the byte `'b'` are block index rows). Google for leveldb-cli to find such a tool. Next, after having set up a corrupt block index using one of the above methods, and after having reproduced the crash seen in Bitcoin-ABC#244, try loading the app using this commit -- it shold now give you a more intelligent error message telling you what to do next to fix ths situation. **Also to test:** - Run a (non-corrupted) pruned node as well with this commit, ensure it loads ok. - Run a (non-corrupted) full node with this commit, ensuring it loads ok. - Start up a brand new blockchain synch with this commit, aborting it in various places, restarting the app and then starting it again, ensuring nothing broke. - Start up a new pruned node blockchain synch as above, killing it and restarting it and ensuring it still loads each time.
Block index: Add an additional sanity check when loading the block index Closes Bitcoin-ABC#244 See merge request bitcoin-cash-node/bitcoin-cash-node!1058
This will use the output-then-input validation order before the planned Nov 15 hard fork as well as after. Before the fork, this code will verify the topological sorting by making a memo of where in the block each output came from when doing the output inserts, and checking those memos when spending outputs from elsewhere within the block.
I'm not certain that the try-catch block is the best way to deal with the pre-BIP30 scenario. It seems to work okay, but it gives me the heebie jeebies that I might have overlooked something.
Successfully syncs past the BIP30 fork at height 91880. My node hasn't made it to the BCH fork yet, though. Edit: fully syncs without issues, and passes all unit tests.