Make reusable base class for auxiliary indices #13243

jimpo commented at 1:18 AM on May 16, 2018: contributor

This refactors most of the logic in TxIndex into a reusable base class for other indices. There are two commits moving code between files, which may be be more easily reviewed using git diff --color-moved (https://blog.github.com/2018-04-05-git-217-released/).

The motivation for this is to support BIP 157 by indexing block filters.

This change is <img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>

fanquake added the label Refactoring on May 16, 2018

fanquake added the label UTXO Db and Indexes on May 16, 2018

jimpo force-pushed on May 16, 2018

in src/txdb.h:135 in 053844b23c outdated

 130 | +public:
 131 | +    BaseIndexDB(const fs::path& path, size_t n_cache_size,
 132 | +                bool f_memory = false, bool f_wipe = false, bool f_obfuscate = false);
 133 | +
 134 | +    /// Read block locator of the chain that the txindex is in sync with.
 135 | +    bool ReadBestBlock(CBlockLocator& locator) const;

promag commented at 3:02 PM on May 16, 2018:

Remove ReadBestBlock and WriteBestBlock from TxIndexDB definition.

jimpo commented at 6:31 PM on May 16, 2018:

Good catch, thanks.

in src/txdb.h:128 in 053844b23c outdated

 124 | @@ -125,6 +125,19 @@ class CBlockTreeDB : public CDBWrapper
 125 |      bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function<CBlockIndex*(const uint256&)> insertBlockIndex);
 126 |  };
 127 |  
 128 | +class BaseIndexDB : public CDBWrapper

promag commented at 3:02 PM on May 16, 2018:

A comment explaining this base class would be nice.

jimpo commented at 6:27 PM on May 16, 2018:

Given that it gets moved to BaseIndex for better context in "index: Move index DBs into index/ directory.", would you still find the comment helpful?

promag commented at 7:17 PM on May 16, 2018:

Right, ignore then.

in src/Makefile.am:110 in a488978b33 outdated

 102 | @@ -103,6 +103,7 @@ BITCOIN_CORE_H = \
 103 |    fs.h \
 104 |    httprpc.h \
 105 |    httpserver.h \
 106 | +  index/base.h \

promag commented at 3:07 PM on May 16, 2018:

How about index/baseindex.h? (base.h a little vague even in folder index)

jimpo commented at 6:25 PM on May 16, 2018:

I tend to dislike filenames that are redundant with their directories, but I'll change if people prefer.

promag commented at 7:14 PM on May 16, 2018:

Well I just think base is too vague. Also, there is index/txindex.*.

jamesob commented at 2:41 PM on May 25, 2018:

Personally I think base.h is fine.

in src/init.cpp:1609 in 58e7705a63 outdated

1586 | @@ -1587,8 +1587,7 @@ bool AppInitMain()
1587 |  
1588 |      // ********************************************************* Step 8: start indexers
1589 |      if (gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
1590 | -        auto txindex_db = MakeUnique<TxIndexDB>(nTxIndexCache, false, fReindex);
1591 | -        g_txindex = MakeUnique<TxIndex>(std::move(txindex_db));
1592 | +        g_txindex = MakeUnique<TxIndex>(nTxIndexCache, false, fReindex);

promag commented at 3:09 PM on May 16, 2018:

Why is this being changed?

jimpo commented at 6:26 PM on May 16, 2018:

I think it makes sense for the DB to be encapsulated in the index interface, even if it is implemented separately internally.

promag commented at 7:16 PM on May 16, 2018:

Agree with being encapsulated. It's just a unrelated change AFAIU, but a nice to include.

jimpo commented at 9:21 PM on May 16, 2018:

Oh, sorry. The reason is that the TxIndex::DB is protected now (at the end of the commit series), so can't be independently instantiated in init.cpp.

jimpo force-pushed on May 16, 2018

jimpo force-pushed on May 17, 2018

laanwj added this to the "Blockers" column in a project

in src/index/base.cpp:232 in da17ca6cd6 outdated

 227 | +
 228 | +bool BaseIndex::BlockUntilSyncedToCurrentChain()
 229 | +{
 230 | +    AssertLockNotHeld(cs_main);
 231 | +
 232 | +    if (!m_synced) {

jamesob commented at 2:22 PM on May 18, 2018:

I find this name a bit confusing ("if we're not synced and this method is called BlockUntilSynced..., why are we returning?"). Would a rename to m_initial_sync_complete or something along those lines make sense or does that seem too wordy?

jimpo commented at 5:52 PM on May 18, 2018:

Yeah, I see your point. I'm fine changing the variable name to be more descriptive m_initial_sync_complete or m_sync_complete both make sense to me (though the latter may not alleviate the confusion).

jamesob commented at 2:57 PM on May 18, 2018: member

Changes look really nice so far. Excited for how straightforward this will make adding more optional indexes. I'll start manual testing in the next few days.

promag commented at 11:31 AM on May 23, 2018: member

utACK da17ca6. Commit by commit refactors LGTM.

jamesob approved

jamesob commented at 6:17 PM on May 25, 2018: member

Tested ACK https://github.com/bitcoin/bitcoin/pull/13243/commits/da17ca6cd6539a6f8dc8ef0d76bb670e20feac67

Did a manual test of txindex behavior. Take or leave my previous comment about member attribute naming. Great change.

Manual test plan

run -reindex (testnet)

set txindex=0, ensure that getrawtransaction doesn't work (testnet)

 $ which trpc
 trpc: aliased to ~/src/bitcoin/src/bitcoin-cli -testnet -rpcuser=foo -rpcpassword=bar

 $ trpc getrawtransaction $(trpc getblock `trpc getblockhash 500123` 1 | jq -r ".tx[3]")

 error code: -5
 error message:
 No such mempool transaction. Use -txindex to enable blockchain transaction queries. Use gettransaction for wallet transactions.

set txindex=1, ensure that getrawtransaction works again without a full reindex (testnet)

[debug.log]
2018-05-25T17:55:00Z Syncing txindex with block chain from height 153579
2018-05-25T17:55:31Z Syncing txindex with block chain from height 208298
2018-05-25T17:55:00Z Syncing txindex with block chain from height 153579
2018-05-25T17:55:31Z Syncing txindex with block chain from height 208298
2018-05-25T17:56:02Z Syncing txindex with block chain from height 276717
2018-05-25T17:56:33Z Syncing txindex with block chain from height 506327
2018-05-25T17:57:04Z Syncing txindex with block chain from height 628106
2018-05-25T17:57:35Z Syncing txindex with block chain from height 894194
2018-05-25T17:58:06Z Syncing txindex with block chain from height 1060431
2018-05-25T17:58:37Z Syncing txindex with block chain from height 1162648
2018-05-25T17:59:08Z Syncing txindex with block chain from height 1226430
2018-05-25T17:59:39Z Syncing txindex with block chain from height 1285766
2018-05-25T17:59:57Z txindex is enabled at height 1315371
2018-05-25T17:59:57Z txindex thread exit

$ trpc getrawtransaction $(trpc getblock `trpc getblockhash 500123` 1 | jq -r ".tx[3]")

0100000001272bba5a7cb395ed0aad4360f8ecd2f3f6aa0089e695a40dc44f0d3d13d209e9010000006b483045022100c88339fe420b4d83a1a42bf8dbaaccbae4d1c4c2ded37deaa0c7b5b88151bec2022007a5f91f742933be69f6bcc7efb820c435db6c50f611e54e09b8bf97d3e77ff4012103fe138317b7f505764062a2a752e2339afb1ba5435c1de2bf42719dce2c35a3e9ffffffff0240420f00000000001976a9146d0ef49ca42b59112be638bdc751360b1be4995d88ac606a3901000000001976a9141987e0b5410e42dce91cdcea8804cae6b36b7bf588ac00000000

$ trpc getrawtransaction $(trpc getblock `trpc getbestblockhash` 1 | jq -r ".tx[10]")

020000000143e4a966e927267b74e11aba867bc4356fc1ed0ed98d41633db958ec4b3d7ed6010000006a4730440220671a21fc391ca7d00a3ed01809ffb121c70993670f4c72203514623ad1b985f3022006b7d7a895e6808c6f8165b1944699475628f6dae72edd67bdf9b55da92199fe012103e183b4824996f294db0a04c80fbeacc539b719d09f2fd50d4c8a2e8192db6b74feffffff027ab5cabf3e0000001976a914d361e252335820fe0e1a7991b1a03a57994fa08e88ac0d74cf05000000001976a914210dbd566c705e77802925c6b416aeca13e726b888ac2a121400

sipa commented at 12:03 AM on May 26, 2018: member

Concept ACK

laanwj commented at 2:05 PM on May 29, 2018: member

Concept ACK, nice

promag commented at 10:43 PM on June 4, 2018: member

Needs rebase.

db: Remove obsolete methods from CBlockTreeDB. 9b0ec1a7f9

db: Make reusable base class for index databases. e5af5fc6fb

index: Extract logic from TxIndex into reusable base class. 61a1226d87

index: Generalize logged statements in BaseIndex. f376a49241

MOVEONLY: Move BaseIndex to its own file. 2318affd27

index: Remove TxIndexDB from public interface of TxIndex. 89eddcd365

index: Move index DBs into index/ directory. ec3073a274

jimpo force-pushed on Jun 5, 2018

laanwj commented at 4:00 PM on June 7, 2018: member

utACK ec3073a274bf7affe1b8c87a10f75d126f5ac027 verified move-onlyness of 2318affd27de436ddf9d866a4b82eed8ea2e738b

laanwj merged this on Jun 7, 2018

laanwj closed this on Jun 7, 2018

laanwj referenced this in commit ea263e1eb0 on Jun 7, 2018

laanwj removed this from the "Blockers" column in a project

jimpo deleted the branch on Jun 7, 2018

domob1812 referenced this in commit f6f8cf5aa6 on Jun 11, 2018

UdjinM6 referenced this in commit 83e544daef on Jun 19, 2021

UdjinM6 referenced this in commit 09fbfc0231 on Jun 20, 2021

UdjinM6 referenced this in commit 710b0ed309 on Jun 24, 2021

UdjinM6 referenced this in commit 45b85feb95 on Jun 26, 2021

UdjinM6 referenced this in commit 1b22d8deec on Jun 29, 2021

UdjinM6 referenced this in commit 7714a3456c on Jun 29, 2021

UdjinM6 referenced this in commit 22e4729496 on Jul 1, 2021

MarcoFalke locked this on Sep 8, 2021