Build txindex in parallel with validation #13033

jimpo · 2018-04-20T00:11:26Z

I'm re-opening #11857 as a new pull request because the last one stopped loading for people

This refactors the tx index code to be in it's own class and get built concurrently with validation code. The main benefit is decoupling and moving the txindex into a separate DB. The primary motivation is to lay the groundwork for other indexers that might be desired (such as the compact filters). The basic idea is that the TxIndex spins up its own thread, which first syncs the txindex to the current block index, then once in sync the BlockConnected ValidationInterface hook writes new blocks.

DB changes

At the suggestion of some other developers, the txindex has been split out into a separate database. A data migration runs at startup on any nodes with a legacy txindex. Currently the migration blocks node initialization until complete.

Open questions

Should the migration of txindex data from the old DB to the new DB block in init or should it happen in a background thread? The downside to backgrounding it is that getrawtransaction would return an error message saying the txindex is syncing while the migration is running.

Impact

In a sample size n=1 test where I synced nodes from scratch, the average time Index writing was 3.36ms in master and 1.72ms in this branch. The average time between UpdateTip log lines for sequential blocks between 400,000 and IBD end on mainnet was 0.297204s in master and 0.286134s in this branch. Most likely this is just variance in IBD times, but I can try with some more trials if people want.

Sjors · 2018-04-20T09:49:21Z

If anyone wants to read the old PR and has difficulty loading the page, it helps to use another browser session that's logged out.

Only comment of mine I'd like to export to this PR is the suggestion to backport "the migration code that removes txindex without requiring a reindex would be useful for folks who regret having set txindex=1 on a node with slow hardware."

Tested 3272b17637c5, including kill -9ing the migration from legacy to the new db.

jamesob · 2018-04-23T16:03:11Z

Reverse chron. summary of ACKs from #11857:

@TheBlueMatt: utACK 523dd76
@jamesob: Tested ACK ea8be45
@jtimon: Concept ACK
@ryanofsky: utACK 806b2f1
@Sjors: Tested (ACK, maybe?) 806b2f1
@jonasschnelli: Concept ACK
@sipa: Concept ACK
@laanwj: Concept ACK

promag

Tested ACK 3272b17 (round 1).

promag · 2018-04-24T14:24:20Z

doc/release-notes-pr11857.md

@@ -0,0 +1,11 @@
+Transaction index changes


Rename file? PR changed.

promag · 2018-04-24T14:25:31Z

src/index/txindex.cpp

@@ -0,0 +1,308 @@
+// Copyright (c) 2017 The Bitcoin Core developers


nit, 2018? (and 2 more files)

promag · 2018-04-24T14:46:24Z

src/rpc/rawtransaction.cpp

@@ -47,6 +48,8 @@ void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry)
    TxToUniv(tx, uint256(), entry, true, RPCSerializationFlags());

    if (!hashBlock.IsNull()) {
+        LOCK(cs_main);


In order to avoid locking and lookup here, how about adding the argument CBlockIndex* pindex to TxToJSON? (I know this is only called from getrawtransaction).

I'm pretty against adding more to this PR. Seems like a good follow-on cleanup item.

promag · 2018-04-24T15:07:01Z

src/index/txindex.h

+    /// @param[out]  block_hash  The hash of the block the transaction is found in.
+    /// @param[out]  tx  The raw transaction itself.
+    /// @return  true if transaction is found, false otherwise
+    bool FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRef& tx) const;


nit, LookupTransaction.

promag · 2018-04-24T15:07:15Z

src/index/txindex.h

+    /// up from far behind, this method does not block and immediately returns false.
+    bool BlockUntilSyncedToCurrentChain();
+
+    /// Look up a raw transaction by hash.


Drop raw?

Your wish is my command.

promag · 2018-04-24T15:07:29Z

src/index/txindex.h

+    ///
+    /// @param[in]   tx_hash  The hash of the transaction to be returned.
+    /// @param[out]  block_hash  The hash of the block the transaction is found in.
+    /// @param[out]  tx  The raw transaction itself.


promag · 2018-04-24T15:34:21Z

src/index/txindex.cpp

+    // Need to register this ValidationInterface before running Init(), so that
+    // callbacks are not missed if Init sets m_synced to true.
+    RegisterValidationInterface(this);
+    if (!Init()) {


I don't understand this case, should exit instead?

My thinking is that logging and running without the TxIndex is better than FatalError, but I could see it the other way too. Probably a good idea to add a log line at the very least.

I'd say running without the desired functionality should exit.

I agree with @promag. If the daemon can't provide the service that is requested, it should fail early. Otherwise you may just complicate debugging.

Changed to FatalError.

promag · 2018-04-24T15:43:40Z

src/rpc/rawtransaction.cpp

-            if (!coin.IsSpent()) {
-                pblockindex = chainActive[coin.nHeight];
-                break;
+        LOCK(cs_main);


Instead of nesting, repeat the lock:

if (!request.params[1].isNull()) { hashBlock = uint256S(request.params[1].get_str()); LOCK(cs_main); ... } else { LOCK(cs_main); ... }

sipa · 2018-04-24T22:06:06Z

src/index/txindex.cpp

+            }
+
+            if (last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) {
+                WriteBestBlock();


As this is called in the middle of ThreadSync(), shouldn't this be writing a locator for pindex rather than chainActive?

Yeah, wow, oops. Good catch.

sipa · 2018-04-24T23:40:56Z

src/index/txindex.cpp

+        int64_t last_locator_write_time = 0;
+        while (true) {
+            if (m_interrupt) {
+                WriteBestBlock();


This function takes an argument now.

The new TxIndexDB class will be used by a future commit in this change set.

The TxIndex will be responsible for building the transaction index concurrently with the main validation thread by implementing ValidationInterface. This does not process blocks concurrently yet.

TxIndex starts up a background thread to get in sync with the block index before blocks are processed through the ValidationInterface.

In order to preserve getrawtransaction RPC behavior, there needs to be a way for a thread to ensure the transaction index is in sync with the current state of the blockchain.

Now that the transaction index is updated asynchronously, in order to preserve the current behavior of public interfaces, the code blocks until the transaction index is caught up with the current state of the blockchain.

sipa · 2018-04-26T01:20:35Z

utACK 9b27047

Also compared with a re-merge of master with 806b2f1 (which had utACK from @ryanofsky and @Sjors), with a re-merge of master with 523dd76 (which had utACK from @TheBlueMatt), and with a re-merge of master with ea8be45 (which had Tested ACK from @jamesob) to find the only differences are:

Adding an abstracted-out function TxIndex::WriteBestBlock, which is invoked additionally after interrupting the background sync and periodically.
Checks in TxIndex::SetBestChain to see if callbacks are received after BlockConnected.
Push down cs_main locks in gettxoutproof.
Simplification of TxIndexDB::ReadBestBlock.
Changes in comments and logging

9b27047 [doc] Include txindex changes in the release notes. (Jim Posen) ed77dd6 [test] Simple unit test for TxIndex. (Jim Posen) 6d772a3 [rpc] Public interfaces to GetTransaction block until synced. (Jim Posen) a03f804 [index] Move disk IO logic from GetTransaction to TxIndex::FindTx. (Jim Posen) e0a3b80 [validation] Replace tx index code in validation code with TxIndex. (Jim Posen) 8181db8 [init] Initialize and start TxIndex in init code. (Jim Posen) f90c3a6 [index] TxIndex method to wait until caught up. (Jim Posen) 70d510d [index] Allow TxIndex sync thread to be interrupted. (Jim Posen) 94b4f8b [index] TxIndex initial sync thread. (Jim Posen) 34d68bf [index] Create new TxIndex class. (Jim Posen) c88bcec [db] Migration for txindex data to new, separate database. (Jim Posen) 0cb8303 [db] Create separate database for txindex. (Jim Posen) Pull request description: I'm re-opening #11857 as a new pull request because the last one stopped loading for people ------------------------------- This refactors the tx index code to be in it's own class and get built concurrently with validation code. The main benefit is decoupling and moving the txindex into a separate DB. The primary motivation is to lay the groundwork for other indexers that might be desired (such as the [compact filters](bitcoin/bips#636)). The basic idea is that the TxIndex spins up its own thread, which first syncs the txindex to the current block index, then once in sync the BlockConnected ValidationInterface hook writes new blocks. ### DB changes At the suggestion of some other developers, the txindex has been split out into a separate database. A data migration runs at startup on any nodes with a legacy txindex. Currently the migration blocks node initialization until complete. ### Open questions - Should the migration of txindex data from the old DB to the new DB block in init or should it happen in a background thread? The downside to backgrounding it is that `getrawtransaction` would return an error message saying the txindex is syncing while the migration is running. ### Impact In a sample size n=1 test where I synced nodes from scratch, the average time [Index writing](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L1903) was 3.36ms in master and 1.72ms in this branch. The average time between `UpdateTip` log lines for sequential blocks between 400,000 and IBD end on mainnet was 0.297204s in master and 0.286134s in this branch. Most likely this is just variance in IBD times, but I can try with some more trials if people want. Tree-SHA512: 451fd7d95df89dfafceaa723cdf0f7b137615b531cf5c5035cfb54e9ccc2026cec5ac85edbcf71b7f4e2f102e36e9202b8b3a667e1504a9e1a9976ab1f0079c4

jamesob · 2018-04-26T04:50:36Z

🎉 nice work, @jimpo.

9b27047 [doc] Include txindex changes in the release notes. (Jim Posen) ed77dd6 [test] Simple unit test for TxIndex. (Jim Posen) 6d772a3 [rpc] Public interfaces to GetTransaction block until synced. (Jim Posen) a03f804 [index] Move disk IO logic from GetTransaction to TxIndex::FindTx. (Jim Posen) e0a3b80 [validation] Replace tx index code in validation code with TxIndex. (Jim Posen) 8181db8 [init] Initialize and start TxIndex in init code. (Jim Posen) f90c3a6 [index] TxIndex method to wait until caught up. (Jim Posen) 70d510d [index] Allow TxIndex sync thread to be interrupted. (Jim Posen) 94b4f8b [index] TxIndex initial sync thread. (Jim Posen) 34d68bf [index] Create new TxIndex class. (Jim Posen) c88bcec [db] Migration for txindex data to new, separate database. (Jim Posen) 0cb8303 [db] Create separate database for txindex. (Jim Posen) Pull request description: I'm re-opening bitcoin#11857 as a new pull request because the last one stopped loading for people ------------------------------- This refactors the tx index code to be in it's own class and get built concurrently with validation code. The main benefit is decoupling and moving the txindex into a separate DB. The primary motivation is to lay the groundwork for other indexers that might be desired (such as the [compact filters](bitcoin/bips#636)). The basic idea is that the TxIndex spins up its own thread, which first syncs the txindex to the current block index, then once in sync the BlockConnected ValidationInterface hook writes new blocks. ### DB changes At the suggestion of some other developers, the txindex has been split out into a separate database. A data migration runs at startup on any nodes with a legacy txindex. Currently the migration blocks node initialization until complete. ### Open questions - Should the migration of txindex data from the old DB to the new DB block in init or should it happen in a background thread? The downside to backgrounding it is that `getrawtransaction` would return an error message saying the txindex is syncing while the migration is running. ### Impact In a sample size n=1 test where I synced nodes from scratch, the average time [Index writing](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L1903) was 3.36ms in master and 1.72ms in this branch. The average time between `UpdateTip` log lines for sequential blocks between 400,000 and IBD end on mainnet was 0.297204s in master and 0.286134s in this branch. Most likely this is just variance in IBD times, but I can try with some more trials if people want. Tree-SHA512: 451fd7d95df89dfafceaa723cdf0f7b137615b531cf5c5035cfb54e9ccc2026cec5ac85edbcf71b7f4e2f102e36e9202b8b3a667e1504a9e1a9976ab1f0079c4

Merge bitcoin#13033: Build txindex in parallel with validation

jimpo mentioned this pull request Apr 20, 2018

Build tx index in parallel with validation #11857

Closed

fanquake added the UTXO Db and Indexes label Apr 20, 2018

fanquake added this to Blockers in High-priority for review Apr 20, 2018

promag reviewed Apr 24, 2018

View reviewed changes

jimpo force-pushed the txindex-refactor-take2 branch from 3272b17 to 40eabf6 Compare April 24, 2018 18:20

sipa reviewed Apr 24, 2018

View reviewed changes

jimpo force-pushed the txindex-refactor-take2 branch from 40eabf6 to 8f1bfe1 Compare April 24, 2018 23:22

sipa reviewed Apr 24, 2018

View reviewed changes

jimpo force-pushed the txindex-refactor-take2 branch from 8f1bfe1 to 70d1881 Compare April 25, 2018 00:54

Sjors mentioned this pull request Apr 25, 2018

Interpret absense of prune= as prune=1 if there are pruned blocks #13029

Closed

jimpo force-pushed the txindex-refactor-take2 branch 3 times, most recently from ec79b24 to c6d4c88 Compare April 25, 2018 18:24

Jim Posen added 12 commits April 25, 2018 11:25

[db] Create separate database for txindex.

0cb8303

The new TxIndexDB class will be used by a future commit in this change set.

[db] Migration for txindex data to new, separate database.

c88bcec

[index] Create new TxIndex class.

34d68bf

The TxIndex will be responsible for building the transaction index concurrently with the main validation thread by implementing ValidationInterface. This does not process blocks concurrently yet.

[index] TxIndex initial sync thread.

94b4f8b

TxIndex starts up a background thread to get in sync with the block index before blocks are processed through the ValidationInterface.

[index] Allow TxIndex sync thread to be interrupted.

70d510d

[index] TxIndex method to wait until caught up.

f90c3a6

In order to preserve getrawtransaction RPC behavior, there needs to be a way for a thread to ensure the transaction index is in sync with the current state of the blockchain.

[init] Initialize and start TxIndex in init code.

8181db8

[validation] Replace tx index code in validation code with TxIndex.

e0a3b80

[index] Move disk IO logic from GetTransaction to TxIndex::FindTx.

a03f804

[rpc] Public interfaces to GetTransaction block until synced.

6d772a3

Now that the transaction index is updated asynchronously, in order to preserve the current behavior of public interfaces, the code blocks until the transaction index is caught up with the current state of the blockchain.

[test] Simple unit test for TxIndex.

ed77dd6

[doc] Include txindex changes in the release notes.

9b27047

jimpo force-pushed the txindex-refactor-take2 branch from c6d4c88 to 9b27047 Compare April 25, 2018 18:25

sipa merged commit 9b27047 into bitcoin:master Apr 26, 2018

This was referenced Apr 26, 2018

Refactor: separate wallet from node #10973

Merged

Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection #11640

Merged

Bushstar mentioned this pull request Apr 26, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#309

Merged

fanquake removed this from Blockers in High-priority for review Apr 26, 2018

jimpo mentioned this pull request May 1, 2018

RPC: Improve error messages on RPC endpoints that use GetTransaction #13144

Closed

jimpo deleted the txindex-refactor-take2 branch May 1, 2018 23:56

mgrychow mentioned this pull request Aug 11, 2018

refactor: Removal of circular dependency between index/txindex, validation and index/base #13942

Closed

sipa added the Needs release note label Aug 14, 2018

mgrychow mentioned this pull request Aug 23, 2018

Utxoscriptindex #14035

Closed

marcinja mentioned this pull request Aug 24, 2018

Add address-based index (attempt 4?) #14053

Closed

laanwj mentioned this pull request Aug 30, 2018

TODO for release notes 0.17.0 #12391

Closed

Sjors mentioned this pull request Sep 8, 2018

Allow txindex in prune mode #13014

Closed

2 tasks

promag mentioned this pull request Oct 28, 2018

Bus error (core dumped) crash on Fedora (txindex migration?) #14466

Closed

fanquake removed the Needs release note label Mar 20, 2019

UdjinM6 added a commit to dashpay/dash that referenced this pull request Jun 5, 2021

Merge pull request #4178 from UdjinM6/backports-0.17-pr30

bcc8b35

Merge bitcoin#13033: Build txindex in parallel with validation

jnewbery mentioned this pull request Aug 3, 2021

Remove txindex migration code #22615

Closed

Zero-1729 mentioned this pull request Aug 4, 2021

Remove txindex migration code #22626

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build txindex in parallel with validation #13033

Build txindex in parallel with validation #13033

jimpo commented Apr 20, 2018

Sjors commented Apr 20, 2018

jamesob commented Apr 23, 2018

promag left a comment

promag Apr 24, 2018

promag Apr 24, 2018

promag Apr 24, 2018

jimpo Apr 24, 2018

promag Apr 24, 2018

promag Apr 24, 2018

promag Apr 24, 2018

jimpo Apr 24, 2018

promag Apr 24, 2018

promag Apr 24, 2018

jimpo Apr 24, 2018

promag Apr 24, 2018

sipa Apr 24, 2018

jimpo Apr 24, 2018

promag Apr 24, 2018

sipa Apr 24, 2018

jimpo Apr 24, 2018

sipa Apr 24, 2018

sipa commented Apr 26, 2018 •

edited

jamesob commented Apr 26, 2018

		@@ -0,0 +1,308 @@
		// Copyright (c) 2017 The Bitcoin Core developers

Build txindex in parallel with validation #13033

Build txindex in parallel with validation #13033

Conversation

jimpo commented Apr 20, 2018

DB changes

Open questions

Impact

Sjors commented Apr 20, 2018

jamesob commented Apr 23, 2018

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa commented Apr 26, 2018 • edited

jamesob commented Apr 26, 2018

sipa commented Apr 26, 2018 •

edited