Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make reusable base class for auxiliary indices #13243

Merged
merged 7 commits into from Jun 7, 2018

Conversation

@jimpo
Copy link
Contributor

jimpo commented May 16, 2018

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 Reviewable

@jimpo jimpo force-pushed the jimpo:index-abstraction branch May 16, 2018
src/txdb.h Outdated
bool f_memory = false, bool f_wipe = false, bool f_obfuscate = false);

/// Read block locator of the chain that the txindex is in sync with.
bool ReadBestBlock(CBlockLocator& locator) const;

This comment has been minimized.

Copy link
@promag

promag May 16, 2018

Member

Remove ReadBestBlock and WriteBestBlock from TxIndexDB definition.

This comment has been minimized.

Copy link
@jimpo

jimpo May 16, 2018

Author Contributor

Good catch, thanks.

src/txdb.h Outdated
@@ -125,6 +125,19 @@ class CBlockTreeDB : public CDBWrapper
bool LoadBlockIndexGuts(const Consensus::Params& consensusParams, std::function<CBlockIndex*(const uint256&)> insertBlockIndex);
};

class BaseIndexDB : public CDBWrapper

This comment has been minimized.

Copy link
@promag

promag May 16, 2018

Member

A comment explaining this base class would be nice.

This comment has been minimized.

Copy link
@jimpo

jimpo May 16, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@promag

promag May 16, 2018

Member

Right, ignore then.

@@ -103,6 +103,7 @@ BITCOIN_CORE_H = \
fs.h \
httprpc.h \
httpserver.h \
index/base.h \

This comment has been minimized.

Copy link
@promag

promag May 16, 2018

Member

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

This comment has been minimized.

Copy link
@jimpo

jimpo May 16, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@promag

promag May 16, 2018

Member

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

This comment has been minimized.

Copy link
@jamesob

jamesob May 25, 2018

Member

Personally I think base.h is fine.

@@ -1587,8 +1587,7 @@ bool AppInitMain()

// ********************************************************* Step 8: start indexers
if (gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
auto txindex_db = MakeUnique<TxIndexDB>(nTxIndexCache, false, fReindex);
g_txindex = MakeUnique<TxIndex>(std::move(txindex_db));
g_txindex = MakeUnique<TxIndex>(nTxIndexCache, false, fReindex);

This comment has been minimized.

Copy link
@promag

promag May 16, 2018

Member

Why is this being changed?

This comment has been minimized.

Copy link
@jimpo

jimpo May 16, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@promag

promag May 16, 2018

Member

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

This comment has been minimized.

Copy link
@jimpo

jimpo May 16, 2018

Author Contributor

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 jimpo force-pushed the jimpo:index-abstraction branch 4 times, most recently May 16, 2018
@laanwj laanwj added this to Blockers in High-priority for review May 17, 2018
Copy link
Member

jamesob left a comment

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.

{
AssertLockNotHeld(cs_main);

if (!m_synced) {

This comment has been minimized.

Copy link
@jamesob

jamesob May 18, 2018

Member

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?

This comment has been minimized.

Copy link
@jimpo

jimpo May 18, 2018

Author Contributor

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).

@promag

This comment has been minimized.

Copy link
Member

promag commented May 23, 2018

utACK da17ca6. Commit by commit refactors LGTM.

Copy link
Member

jamesob left a comment

Tested ACK da17ca6

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
    
@@ -103,6 +103,7 @@ BITCOIN_CORE_H = \
fs.h \
httprpc.h \
httpserver.h \
index/base.h \

This comment has been minimized.

Copy link
@jamesob

jamesob May 25, 2018

Member

Personally I think base.h is fine.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 26, 2018

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 29, 2018

Concept ACK, nice

@promag

This comment has been minimized.

Copy link
Member

promag commented Jun 4, 2018

Needs rebase.

@jimpo jimpo force-pushed the jimpo:index-abstraction branch to ec3073a Jun 5, 2018
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jun 7, 2018

utACK ec3073a
verified move-onlyness of 2318aff

@laanwj laanwj merged commit ec3073a into bitcoin:master Jun 7, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jun 7, 2018
ec3073a index: Move index DBs into index/ directory. (Jim Posen)
89eddcd index: Remove TxIndexDB from public interface of TxIndex. (Jim Posen)
2318aff MOVEONLY: Move BaseIndex to its own file. (Jim Posen)
f376a49 index: Generalize logged statements in BaseIndex. (Jim Posen)
61a1226 index: Extract logic from TxIndex into reusable base class. (Jim Posen)
e5af5fc db: Make reusable base class for index databases. (Jim Posen)
9b0ec1a db: Remove obsolete methods from CBlockTreeDB. (Jim Posen)

Pull request description:

  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.

  <!-- Reviewable:start -->
  ---
  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/bitcoin/bitcoin/13243)
  <!-- Reviewable:end -->

Tree-SHA512: 0857f04df2aa920178dab2eb8e57984d8eb4d5010deca9971190358479e05b6672ccca2a08af0a7ac9fe02afb947be84cf35a3693204d0667263c6add2959cbf
@laanwj laanwj removed this from Blockers in High-priority for review Jun 7, 2018
@jimpo jimpo deleted the jimpo:index-abstraction branch Jun 7, 2018
domob1812 added a commit to domob1812/namecore that referenced this pull request Jun 11, 2018
Updated the code modified through bitcoin/bitcoin#13243
for Namecoin's changes to the validation interface (BlockConnected, in
particular).

Conflicts:
	contrib/init/README.md
	doc/init.md
	doc/release-process.md
	src/Makefile.qt.include
	src/Makefile.qttest.include
	src/Makefile.test.include
	src/index/txindex.cpp
	src/index/txindex.h
	src/test/test_bitcoin.h
	src/txdb.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.