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
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 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/.
-txindex should be removed.
0@@ -0,0 +1,163 @@
1+#!/usr/bin/env bash
qa/rpc-tests, use the Python framework. Also add the test to qa/pull-tester/rpc-tests.py so it gets executed by default.
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?
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.
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?
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.
-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.
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:
fundrawtransaction in a pubkey-only multiwallet setup and“Wallets” would then be reduces to…
Restoring backups is the only feature I can think off that would require a addr-index (otherwise rescanning will be very slow).
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.
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.
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.
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)
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.
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
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.
344@@ -340,6 +345,7 @@ std::string HelpMessage(HelpMessageMode mode)
345 strUsage += HelpMessageOpt("-sysperms", _("Create new files with system default permissions, instead of umask 077 (only effective with disabled wallet functionality)"));
346 #endif
347 strUsage += HelpMessageOpt("-txindex", strprintf(_("Maintain a full transaction index, used by the getrawtransaction rpc call (default: %u)"), DEFAULT_TXINDEX));
348+ strUsage += HelpMessageOpt("-txoutsbyaddressindex", strprintf(_("Maintain an address to unspent outputs index (rpc: gettxoutsbyaddress). The index is built on first use. (default: %u)"), 0));
"txoutsindex", especially because it really indexes scripts and not addresses.
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)?
52@@ -53,7 +53,8 @@ def __init__(self):
53 self.num_nodes = 3
54
55 def setup_network(self, split=False):
56- self.nodes = start_nodes(self.num_nodes, self.options.tmpdir)
57+ args = ["-txoutsbyaddressindex"]
Nit: You can put something like the following up a few lines, so the scope is not limited to setup_network.
0self.extra_args = [["-txoutsbyaddressindex"]] * 3
20+ self.nodes = []
21+ self.nodes.append(start_node(0, self.options.tmpdir, ["-txoutsbyaddressindex"]))
22+ self.nodes.append(start_node(1, self.options.tmpdir))
23+ self.nodes.append(start_node(2, self.options.tmpdir))
24+ self.is_network_split = False
25+ self.sync_all()
15+ self.setup_clean_chain = True
16+ self.num_nodes = 3
17+
18+ def setup_network(self, split=False):
19+ print("Setup network...")
20+ self.nodes = []
0self.nodes = start_nodes(3, self.options.tmpdir, self.extra_args)
should work as well. (You can put self.extra_args = [['-txoutindex'], []. []] in __init__())
30+ # Check that there's no UTXO on any of the nodes
31+ for node in self.nodes:
32+ assert_equal(len(node.listunspent()), 0)
33+
34+ # mining
35+ connect_nodes(self.nodes[1], 0)
connect_*s up into setup_network(). I think all other tests do this
49+ txouts = self.nodes[0].gettxoutsbyaddress(1, (address,))
50+ txid = txouts[0]["txid"]
51+ assert_is_hash_string(txid)
52+ assert_equal(txid, txid1)
53+
54+ # stop node 2
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
142@@ -143,6 +143,7 @@
143 'importprunedfunds.py',
144 'signmessages.py',
145 'p2p-compactblocks.py',
146+ 'txoutsbyaddress.py',
+x permission
- remove skeleton HEX/BIN format support from REST interface for gettxoutsbyaddress endpoint
35+ inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
36+ READWRITE(setCoins);
37+ }
38+};
39+
40+typedef std::map<uint160, CCoinsByScript> CCoinsMapByScript; // uint160 = hash of script
52+ bool GetCoinsByScript(const CScript &script, CCoinsByScript &coins);
53+
54+ // Return a modifiable reference to a CCoinsByScript.
55+ CCoinsByScript &GetCoinsByScript(const CScript &script, bool fRequireExisting = true);
56+
57+ static uint160 getKey(const CScript &script); // we use the hash of the script as key in the database
18+ uint64_t nAddressesOutputs; // equal nTransactionOutputs (if addressindex is enabled)
19+ uint64_t nSerializedSize;
20+ uint256 hashSerialized;
21+ CAmount nTotalAmount;
22+
23+ CCoinsStats() : nHeight(0), nTransactions(0), nTransactionOutputs(0), nAddresses(0), nAddressesOutputs(0), nSerializedSize(0), nTotalAmount(0) {}
32+ if (!base->GetCoinsByHashOfScript(key, tmp))
33+ {
34+ if (fRequireExisting)
35+ return cacheCoinsByScript.end();
36+ }
37+ 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,
ret variable here, could just return insert().second.391+{
392+ LOCK(cs);
393+ CCoinsMapByScript::const_iterator it = mapCoinsByScript.find(CCoinsViewByScript::getKey(script));
394+ if (it != mapCoinsByScript.end())
395+ {
396+ BOOST_FOREACH(const COutPoint &outpoint, it->second.setCoins)
could avoid the loop with
coinsByScript.setCoins.insert(it->second.setCoins.begin(), it->second.setCoins.end());
147+ boost::this_thread::interruption_point();
148+ try {
149+ i++;
150+ pcursor->Next();
151+ } catch (std::exception &e) {
152+ return 0;
115@@ -116,7 +116,7 @@ void CheckSort(CTxMemPool &pool, std::vector<std::string> &sortedOrder)
116
117 BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
118 {
119- CTxMemPool pool(CFeeRate(0));
120+ CTxMemPool pool(CFeeRate(0), false);
41+ txid1 = self.nodes[0].sendtoaddress(address, 10)
42+ self.nodes[0].generate(101) # node will collect its own fee
43+ self.sync_all()
44+ assert_equal(self.nodes[0].getbalance(), 5090)
45+ assert_equal(self.nodes[1].getbalance(), 10)
46+ txouts = self.nodes[0].gettxoutsbyaddress(1, (address,))
94+ pcoinsViewByScriptIn->cacheCoinsByScript.clear();
95+
96+ if (!hashBlock.IsNull())
97+ batch.Write(DB_BEST_BLOCK, hashBlock);
98+
99+ LogPrint("coindb", "Committing %u coin address indexes to coin database...\n", (unsigned int)count);
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”
I am thinking about unifying it around “-utxoindex”
Sounds good.
@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.
