txoutsbyaddress index (take 2) by djpnewton · Pull Request #8660 · bitcoin/bitcoin

djpnewton · 2016-09-03T10:12:38Z

This is an attempt to revive PR #5048

I have moved the index to its own database and also added a gettxoutsbyaddress REST command

I still need to think about the layout of binary input/output for the REST command, or is bin encoding necessary?

Also still missing tests for the new REST stuff

dcousens · 2016-09-04T05:23:35Z

It's not overly complicated to maintain this index outside of bitcoind, IMHO, that is where it should be. Perhaps indexd or some other service.
~~Concept NACK~~

edit: See #8660 (comment)

luke-jr · 2016-09-04T05:24:44Z

NACK. Txouts are not associated with addresses at all.

paveljanik · 2016-09-04T06:47:42Z

@luke-jr They are, but this association is not useful for 99.9% bitcoind users. And it slows down the normal work used by 100% of bitcoind users. So NACK.

If people need this, please contribute some description/work using the external indexing solution into contrib/.

btcdrak · 2016-09-04T12:26:36Z

Concept ACK from me.
@paveljanik The feature is off by default so it shouldn't slow down anything for normal users.
OT: If indexes are really considered so bad then -txindex should be removed.

maflcko · 2016-09-04T21:29:21Z

qa/rpc-tests/txoutsbyaddress.sh

We don't use bash anymore in /rpc-tests

Yes, please look at the existing tests in qa/rpc-tests, use the Python framework. Also add the test to qa/pull-tester/rpc-tests.py so it gets executed by default.

djpnewton · 2016-09-04T22:25:05Z

The inspiration for this PR is to try and make it easier to build a fully validating wallet that does not use significantly more resources then a full node itself.

It seems that most (all?) current solutions for building a wallet either involve added trust (SPV or centralized API services) or added resource requirements (bitcore.io needs 8GB RAM and 200GB disk space)

Thats the rationale (which I probably should have stated at the outset) anyway. The last time I was trying to make a demo it was easier just to use the blockcypher API, or maybe there are projects that I missed that fill this gap?

luke-jr · 2016-09-05T01:30:33Z

If the RPC was by script (as the internals seem to be) instead of by address, it might make sense.

dcousens · 2016-09-05T01:59:49Z

If people need this, please contribute some description/work using the external indexing solution into contrib/.

OT: If indexes are really considered so bad then -txindex should be removed.

IMHO, that is where txindex should be, at least then it might easy to extend to things like this without adding 1000 SLOC.

djpnewton · 2016-09-05T03:28:11Z

@luke-jr the RPC accepts both hex encoded script or addresses

jonasschnelli · 2016-09-05T07:54:58Z

The inspiration for this PR is to try and make it easier to build a fully validating wallet that does not use significantly more resources then a full node itself.

It seems that most (all?) current solutions for building a wallet either involve added trust (SPV or centralized API services) or added resource requirements (bitcore.io needs 8GB RAM and 200GB disk space)

These are good point!

Some thoughts:

If we would add something like this, it should be more modular. Have a look at https://github.com/bitcoin/bitcoin/pull/8501/files, there you can see how just a handful of lines in init.cpp can add core features over a flexible modular approach. It also simplifies rebases.
Was it considered to add the index outside of bitcoin-core? I guess using ZMQ notifications together with REST (or RPC) would work. What would be the downsides or advantages of the "external" approach?

dcousens · 2016-09-05T08:22:10Z

Was it considered to add the index outside of bitcoin-core? I guess using ZMQ notifications together with REST (or RPC) would work. What would be the downsides or advantages of the "external" approach?

As someone who does this approach, the only difficulties is ensuring that you handle orphans and forks properly.
They do happen enough and cause issues, so you need to be prepared for the floor to fall out between RPC calls.
Simple enough code, but battle testing it can take a while and would probably be important to have examples for in contrib/ for beginners.
As far as the db requirements go, you just need to be able to rollback data for re-orgs.

btcdrak · 2016-09-05T10:42:03Z

Indexes are desperately needed, there are quite a few users of my addrindex fork https://github.com/btcdrak/bitcoin/releases that demonstrate this need and there is a steady flow of people finding out about it. This PR is a more lightweight option and I think it should be part of Bitcoin Core in an easily accessible way (i.e. just turn on with a switch+reindex). While I could say add it to my fork, it is does reduce the visibility of it.

sipa · 2016-09-05T11:00:47Z

-txindex was only added for backward compatibility with the pre-0.8 getrawtransaction RPC. I think people who need it should build an external solution. It's inefficient, requires updates all over the place, and doesn't even guarantee consistency when there are reorgs, and is not needed for any of the functions Bitcoin Core provides. The addrindex hack is even worse - it encourages people to build an external wallet in an easy, unscalable, and inadvisable way.

Indexing the UTXO set like this PR does is less bad - at least it doesn't prevent pruning - but what function would it provide apart from RPCs? The wallet can't actually use it (as it would miss historical transactions), and it won't be available in SPV mode.

jonasschnelli · 2016-09-05T11:24:16Z

Agree with @sipa.

A maybe more scalable way how "wallet" (or lets say something like the BWS [bitpay wallet service]) could be built on top of bitcoin core would be:

add multiwallet support (something similar like Multiwallet qt #2184)
add support for pubkey only wallets (should be relatively trivial, partially solved with the current support for "watchonly")
external wallet could use RPC in order to create "wallets", use fundrawtransaction in a pubkey-only multiwallet setup and
transactions can and should be signed in the "external wallet space"

"Wallets" would then be reduces to...

A set of addresses to keep track of (track/store transactions)
Coinselection and utxo per set

Restoring backups is the only feature I can think off that would require a addr-index (otherwise rescanning will be very slow).

djpnewton · 2016-09-05T20:34:32Z

The entire point of running a node is to provide validation for a wallet somewhere. That is what makes a node significant.

Is it actually simple to provide this service outside of bitcoin-core? Are there any projects that manage to do it without duplicating the entire blockchain into their own database? It seems a real shame if we go to the trouble of assembling the UTXO set and cant provide ways for external wallets to leverage it.

@sipa are you sure it would miss historical transactions? Any transactions adding funds to the wallet will show up in the UTXO set and any transactions spending funds from the wallet can only be created by the wallet itself (I guess they can be malleated).

@jonasschnelli I think I like this idea. It might not be as flexible in the kinds of external wallets it allows though.

dcousens · 2016-09-06T00:56:25Z

Are there any projects that manage to do it without duplicating the entire blockchain into their own database?

Of course, but this index is not small by any means.

laanwj · 2016-09-13T09:40:48Z

I think this functionality is very useful. E.g. to get the current balance for an arbitrary address/script.
Missed this a few times. I think external wallets such as coinjoin may find this useful as well.

Sure, you cannot use this with SPV but the primary reason for Bitcoin Core's existence is to be a full node. Storing the UTXO set comes with that. To be clear SPV mode is by no means the future of this project, at most it would be optional.
Unlike a full tx index a UTXO index can be used with pruning, and takes a lot less disk space.

Concept ACK.

The wallet can't actually use it (as it would miss historical transactions),

There may be use for a quick-rescan that ignores historical transactions, which can be used with pruning. (out of scope for this pull, but it has been brought up before - see #8497)

laanwj · 2016-09-13T09:45:10Z

is bin encoding necessary?

No it's not necessary - only if there is a straightforward binary representation like for transactions and blocks. You don't have to make up one.

dcousens · 2016-09-13T09:58:36Z

I wasn't aware this was just for UTXOs, not txouts at any point in history (aka, a full txout index, similar to the addrindex fork).

light concept ACK

dexX7 · 2016-09-13T19:57:54Z

Concept ACK.

Several projects I know of use the addrindex stuff from years ago to derive information about UTXO state, and this is much more elegant.

dexX7 · 2016-09-13T20:02:18Z

src/init.cpp

My personal preference would be something shorter like "txoutsindex", especially because it really indexes scripts and not addresses.

Sure I dont see a problem with that

Or just txoutindex (or utxoutindex or utxoindex)

argh, paradox of choice!

Ok, how about if nobody has any objection I will change the name to "txoutindex" (command line parameter, RPC, REST etc)?

djpnewton · 2016-09-15T10:49:34Z

Ok I converted the RPC tests to python and added some REST tests

maflcko · 2016-09-15T10:51:40Z

qa/rpc-tests/rest.py

Nit: You can put something like the following up a few lines, so the scope is not limited to setup_network.

self.extra_args = [["-txoutsbyaddressindex"]] * 3

maflcko · 2016-09-15T10:54:53Z

qa/rpc-tests/txoutsbyaddress.py

Nit: I think this is not required

maflcko · 2016-09-15T10:56:26Z

qa/rpc-tests/txoutsbyaddress.py

self.nodes = start_nodes(3, self.options.tmpdir, self.extra_args)

should work as well. (You can put self.extra_args = [['-txoutindex'], []. []] in __init__())

maflcko · 2016-09-15T10:58:13Z

qa/rpc-tests/txoutsbyaddress.py

Nit: you can move the connect_*s up into setup_network(). I think all other tests do this

maflcko · 2016-09-15T10:59:55Z

qa/rpc-tests/txoutsbyaddress.py

Why is this required?

node 2 is stopped so that later on it can be restarted and then orphan a block created by node 1 (to test txoutindex handles the reorg)

basically its stopped so I will not build on a block created by node 1

Should probably mention this in a comment.

djpnewton · 2016-09-17T05:05:01Z

addressed review nits by @MarcoFalke (thank you)

maflcko · 2016-09-26T14:02:22Z

qa/pull-tester/rpc-tests.py

Missing +x permission

btcdrak · 2016-12-16T11:05:14Z

needs rebase

gmaxwell · 2017-01-02T15:03:28Z

Concept ACK. Comment, can others provide feeback: I think the argument should be changed to unspentoutputindex or utxoindex or something like that, otherwise it sounds like it indexes everything. Similar for the rpc names.

droark · 2017-01-03T22:02:25Z

Hmmm. If @djpnewton doesn't pick this up again in the next few days, I'll seriously consider keeping it going.

- remove skeleton HEX/BIN format support from REST interface for gettxoutsbyaddress endpoint

ryanofsky · 2017-01-04T17:13:16Z

src/coinsbyscript.h

+    }
+};
+
+typedef std::map<uint160, CCoinsByScript> CCoinsMapByScript; // uint160 = hash of script


Maybe replace uint160 with CScriptID to make the map type more obvious.

ryanofsky

Reviewed most of this PR, but not the whole thing. Mostly minor comments, except for a possible bug in CCoinsViewByScript::FetchCoinsByScript.

ryanofsky · 2017-01-04T17:15:10Z

src/coinsbyscript.h

+    // Return a modifiable reference to a CCoinsByScript.
+    CCoinsByScript &GetCoinsByScript(const CScript &script, bool fRequireExisting = true);
+
+    static uint160 getKey(const CScript &script); // we use the hash of the script as key in the database


Could remove if using CScriptID as the map key type.

ryanofsky · 2017-01-04T17:22:31Z

src/coinstats.h

+    uint256 hashSerialized;
+    CAmount nTotalAmount;
+
+    CCoinsStats() : nHeight(0), nTransactions(0), nTransactionOutputs(0), nAddresses(0), nAddressesOutputs(0), nSerializedSize(0), nTotalAmount(0) {}


Could assign these values inline above with c++11.

ryanofsky · 2017-01-04T17:54:23Z

src/coinsbyscript.cpp

+        if (fRequireExisting)
+            return cacheCoinsByScript.end();
+    }
+    CCoinsMapByScript::iterator ret = cacheCoinsByScript.insert(it, std::make_pair(key, CCoinsByScript()));


Shouldn't this be inserting tmp into the map, not an empty CCoinsByScript() set?

Also,

It would be nice if this code had some basic c++ unit tests similar to coins_tests.cpp, even if they aren't as comprehensive.

Don't really need the other temporary ret variable here, could just return insert().second.

Could replace insert with emplace or emplace_hint here to avoid needing to call make_pair.

ryanofsky · 2017-01-04T18:27:15Z

src/txmempool.cpp

+    CCoinsMapByScript::const_iterator it = mapCoinsByScript.find(CCoinsViewByScript::getKey(script));
+    if (it != mapCoinsByScript.end())
+    {
+        BOOST_FOREACH(const COutPoint &outpoint, it->second.setCoins)


could avoid the loop with

coinsByScript.setCoins.insert(it->second.setCoins.begin(), it->second.setCoins.end());

ryanofsky · 2017-01-04T18:43:43Z

src/txdb.cpp

+            i++;
+            pcursor->Next();
+        } catch (std::exception &e) {
+            return 0;


Why is this exception being caught instead of passed on to the caller? I'd think it'd be better to remove the catch and have the error propogate up, or add a comment here which explains the cases where std::exception is expected and why they should return 0.

ryanofsky · 2017-01-04T18:57:35Z

src/test/mempool_tests.cpp

 BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
 {
-    CTxMemPool pool(CFeeRate(0));
+    CTxMemPool pool(CFeeRate(0), false);


The CTxMemPool constructor saves a const reference to the second argument, so instead of passing literal false, you should pass a variable with a lifetime at least as long as CTxMemPool. Might be better to have the CTxMemPool class just store a bool instead of a reference to a bool to avoid the need to do this.

ryanofsky · 2017-01-04T19:28:50Z

qa/rpc-tests/txoutsbyaddress.py

Should probably mention this in a comment.

ryanofsky · 2017-01-04T19:34:35Z

qa/rpc-tests/txoutsbyaddress.py

+        self.sync_all()
+        assert_equal(self.nodes[0].getbalance(), 5090)
+        assert_equal(self.nodes[1].getbalance(), 10)
+        txouts = self.nodes[0].gettxoutsbyaddress(1, (address,))


Maybe check other txouts fields here as well.

ryanofsky · 2017-01-04T19:46:01Z

src/coinsbyscript.cpp

+    if (!hashBlock.IsNull())
+        batch.Write(DB_BEST_BLOCK, hashBlock);
+
+    LogPrint("coindb", "Committing %u coin address indexes to coin database...\n", (unsigned int)count);


Could use %z instead of (unsigned int) cast, I think.

paveljanik · 2017-01-05T19:50:31Z

Needs rebase.

djpnewton · 2017-01-05T22:55:31Z

rebased

I will start addressing review by @ryanofsky (cheers mate)

Agree with @gmaxwell that naming is confusing, I am thinking about unifying it around "-utxoindex"

maflcko · 2017-01-05T23:36:21Z

I am thinking about unifying it around "-utxoindex"

Sounds good.

btcdrak · 2017-01-18T12:19:42Z

needs rebase again.

ryanofsky · 2017-02-13T18:54:20Z

@djpnewton or @droark have you done more work on this? I'd be happy to help continue the effort on this feature.

droark · 2017-02-19T01:54:46Z

@ryanofsky - I hadn't been working on this but I'm happy to run with the ball since the OP seems to have disappeared again. I'll post a new PR once it's up and running.

Thanks.

fanquake · 2017-02-19T02:14:58Z

Closing this for now then. @Droak or @ryanofsky can open a new PR once they've restarted the work.

fanquake added the UTXO Db and Indexes label Sep 3, 2016

djpnewton force-pushed the cozz8 branch from 085c09c to 673efd7 Compare September 3, 2016 11:08

maflcko reviewed Sep 4, 2016
View reviewed changes

dexX7 reviewed Sep 13, 2016
View reviewed changes

maflcko reviewed Sep 15, 2016

View reviewed changes

qa/rpc-tests/txoutsbyaddress.py Outdated

Copy link

Member

maflcko Sep 15, 2016

Choose a reason for hiding this comment

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

Nit: I think this is not required

maflcko reviewed Sep 15, 2016

View reviewed changes

maflcko reviewed Sep 26, 2016

View reviewed changes

djpnewton force-pushed the cozz8 branch from 8dc81e4 to 0caeaab Compare October 1, 2016 06:22

cozz and others added 10 commits January 4, 2017 13:46

txoutsbyaddress index

f277cf5

move txoutsbyaddress index to its own database

8033706

add txoutsbyaddress querying via rest interface

7458f18

C++11: s/boost::scoped_ptr/std::unique_ptr/

227cfe9

convert txoutsbyaddress test to python

ad28b1b

- add txoutsbyaddress REST interface tests

8e6154a

- remove skeleton HEX/BIN format support from REST interface for gettxoutsbyaddress endpoint

address test review nits

83277c8

rename -txoutsbyaddress to -txoutindex

dd18e44

set executable bit

eed6139

add doc for gettxoutsbyaddress REST endpoint

5c2fb0d

ryanofsky reviewed Jan 4, 2017

View reviewed changes

fix up a change missed by the rebase

8804522

djpnewton force-pushed the cozz8 branch from 0caeaab to 8804522 Compare January 5, 2017 22:47

fanquake closed this Feb 19, 2017

droark mentioned this pull request Feb 20, 2017

txoutsbyaddress index (take 3) #9806

Closed

TheBlueMatt mentioned this pull request May 9, 2017

[pull request idea] addressindex, spentindex, timestampindex (Bitcore patches) #10370

Closed

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

Conversation

djpnewton commented Sep 3, 2016

Uh oh!

dcousens commented Sep 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Sep 4, 2016

Uh oh!

paveljanik commented Sep 4, 2016

Uh oh!

btcdrak commented Sep 4, 2016 • 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!

djpnewton commented Sep 4, 2016

Uh oh!

luke-jr commented Sep 5, 2016

Uh oh!

dcousens commented Sep 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djpnewton commented Sep 5, 2016

Uh oh!

jonasschnelli commented Sep 5, 2016

Uh oh!

dcousens commented Sep 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

btcdrak commented Sep 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Sep 5, 2016

Uh oh!

jonasschnelli commented Sep 5, 2016

Uh oh!

djpnewton commented Sep 5, 2016

Uh oh!

dcousens commented Sep 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcousens commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dexX7 commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 15, 2016 • 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!

djpnewton commented Sep 15, 2016

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!

maflcko Sep 15, 2016 • 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!

dcousens commented Sep 4, 2016 •

edited

Loading

btcdrak commented Sep 4, 2016 •

edited

Loading

dcousens commented Sep 5, 2016 •

edited

Loading

dcousens commented Sep 5, 2016 •

edited

Loading

btcdrak commented Sep 5, 2016 •

edited

Loading

dcousens commented Sep 6, 2016 •

edited

Loading

laanwj commented Sep 13, 2016 •

edited

Loading

laanwj commented Sep 13, 2016 •

edited

Loading

dcousens commented Sep 13, 2016 •

edited

Loading

maflcko Sep 15, 2016 •

edited

Loading

maflcko Sep 15, 2016 •

edited

Loading