Make reusable base class for auxiliary indices #13243

jimpo · 2018-05-16T01:18:20Z

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

promag · 2018-05-16T15:02:00Z

src/txdb.h

Remove ReadBestBlock and WriteBestBlock from TxIndexDB definition.

Good catch, thanks.

promag · 2018-05-16T15:02:43Z

src/txdb.h

A comment explaining this base class would be nice.

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?

Right, ignore then.

promag · 2018-05-16T15:07:49Z

src/Makefile.am

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

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

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

Personally I think base.h is fine.

promag · 2018-05-16T15:09:06Z

src/init.cpp

Why is this being changed?

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

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

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.

jamesob

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.

jamesob · 2018-05-18T14:22:48Z

src/index/base.cpp

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?

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 · 2018-05-23T11:31:12Z

utACK da17ca6. Commit by commit refactors LGTM.

jamesob

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

jamesob · 2018-05-25T14:41:02Z

src/Makefile.am

Personally I think base.h is fine.

sipa · 2018-05-26T00:03:36Z

Concept ACK

laanwj · 2018-05-29T14:05:41Z

Concept ACK, nice

promag · 2018-06-04T22:43:04Z

Needs rebase.

laanwj · 2018-06-07T16:00:32Z

utACK ec3073a
verified move-onlyness of 2318aff

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.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/bitcoin/bitcoin/13243)  Tree-SHA512: 0857f04df2aa920178dab2eb8e57984d8eb4d5010deca9971190358479e05b6672ccca2a08af0a7ac9fe02afb947be84cf35a3693204d0667263c6add2959cbf

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

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.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/bitcoin/bitcoin/13243)  Tree-SHA512: 0857f04df2aa920178dab2eb8e57984d8eb4d5010deca9971190358479e05b6672ccca2a08af0a7ac9fe02afb947be84cf35a3693204d0667263c6add2959cbf

fanquake added Refactoring UTXO Db and Indexes labels May 16, 2018

jimpo force-pushed the index-abstraction branch from 2ed1ed2 to 0436c2a Compare May 16, 2018 01:24

promag reviewed May 16, 2018

View reviewed changes

jimpo force-pushed the index-abstraction branch 4 times, most recently from 9fbc583 to da17ca6 Compare May 17, 2018 18:36

jamesob reviewed May 18, 2018

View reviewed changes

jamesob approved these changes May 25, 2018

View reviewed changes

src/Makefile.am Outdated

Copy link

Contributor

jamesob May 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think base.h is fine.

jimpo added 7 commits June 4, 2018 19:22

db: Remove obsolete methods from CBlockTreeDB.

9b0ec1a

db: Make reusable base class for index databases.

e5af5fc

index: Extract logic from TxIndex into reusable base class.

61a1226

index: Generalize logged statements in BaseIndex.

f376a49

MOVEONLY: Move BaseIndex to its own file.

2318aff

index: Remove TxIndexDB from public interface of TxIndex.

89eddcd

index: Move index DBs into index/ directory.

ec3073a

jimpo force-pushed the index-abstraction branch from da17ca6 to ec3073a Compare June 5, 2018 02:22

laanwj merged commit ec3073a into bitcoin:master Jun 7, 2018

jimpo deleted the index-abstraction branch June 7, 2018 17:12

Bushstar mentioned this pull request Jun 12, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#343

Merged

mgrychow mentioned this pull request Aug 11, 2018

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

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Make reusable base class for auxiliary indices #13243

Make reusable base class for auxiliary indices #13243

Uh oh!

Conversation

jimpo commented May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented May 23, 2018

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Manual test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented May 26, 2018

Uh oh!

laanwj commented May 29, 2018

Uh oh!

promag commented Jun 4, 2018

Uh oh!

laanwj commented Jun 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jimpo commented May 16, 2018 •

edited

Loading

promag May 16, 2018 •

edited

Loading

promag May 16, 2018 •

edited

Loading