txoutsbyaddress index (take 3)

droark commented at 1:38 am on February 20, 2017: contributor

This is an attempt to revive PR #8660 (and, by extension, PR #5048). For now, this PR simply compiles without fresh warnings or errors. Once it is confirmed that no more conflicts exist, the remaining comments/requests from #8660 will be fully addressed.

droark force-pushed on Feb 20, 2017

fanquake added the label UTXO Db and Indexes on Feb 20, 2017

paveljanik commented at 8:53 am on February 20, 2017: contributor

Wshadow statistics: 1 coinsbyscript.cpp:180:2200: warning: declaration shadows a local variable [-Wshadow]

droark force-pushed on Feb 20, 2017

droark commented at 9:33 pm on February 20, 2017: contributor

@paveljanik - Thanks! Fixed that up. Looks like that’s the only shadow warning.

droark force-pushed on Feb 20, 2017

laanwj commented at 7:58 am on February 22, 2017: member

Thanks for reviving this. IMO this is important.

laanwj added this to the milestone 0.15.0 on Feb 22, 2017

sidhujag commented at 6:27 pm on March 14, 2017: none

Concept ACK, are there any statistics on how much extra burden this places on an ordinary node with the index off?

instagibbs commented at 2:23 am on March 16, 2017: member

needs rebase

droark force-pushed on Mar 17, 2017

droark commented at 10:13 pm on March 17, 2017: contributor

@instagibbs - Rebased. Thanks for the heads up. (Side note: Is there any way to have GH tell you when a conflict occurs? That would be really handy.) @sidhujag - Good question. I don’t know offhand. I’m happy to do some benchmarking. I may need some help with that. If anybody would like to make some suggestions, I’m all ears.

All - Will get more movement on this. Life intervened for awhile and has finally slowed down enough to where I can dedicate more time to this.

droark force-pushed on Mar 17, 2017

sidhujag commented at 2:24 am on March 18, 2017: none

Thanks droark great feature btw.. this makes walletless spending very easy

droark force-pushed on Mar 18, 2017

droark commented at 9:53 pm on March 18, 2017: contributor

Am placing a to-do list here to remind myself of what I need to do, and solicit feedback on anything people might think is missing.

Remove all Boost code from the PR.
Address feedback from #8660 (primarily @ryanofsky). (Mostly done but I want to double check a couple of things before proceeding.)
Add some C++ tests for CCoinsView classes.
Probably switch names of this & that per the suggestion of @gmaxwell. (“utxoindex” is my current choice but I’m not attached to it.)
Add a default constructor value for CTxMemPool. (A recently merged PR, combined with this PR, inadvertently nukes the default constructor.)
Check in once a day to see if a rebase is needed. (Ongoing task.)

btcdrak commented at 0:49 am on March 22, 2017: contributor

needs rebase.

droark force-pushed on Mar 22, 2017

droark commented at 1:40 am on March 22, 2017: contributor

@btcdrak - Thanks. Rebased.

droark force-pushed on Mar 22, 2017

weex commented at 3:03 am on March 23, 2017: none

Running this on a .bitcoin/ folder that was last used with 0.13 and was run pruned to 2000. It seems 1GB of ram is too low for this enabled as I got an out of memory error. Now on restarting I get the following after/during the “Rescanning…” step:

~/.bitcoin# bitcoind: coinsbyscript.cpp:53: CCoinsByScript& CCoinsViewByScript::GetCoinsByScript(const CScript&, bool): Assertion `it != cacheCoinsByScript.end()' failed.

droark force-pushed on Mar 23, 2017

droark force-pushed on Mar 24, 2017

droark force-pushed on Mar 25, 2017

droark force-pushed on Mar 27, 2017

droark force-pushed on Mar 29, 2017

droark force-pushed on Mar 30, 2017

droark force-pushed on Mar 31, 2017

droark force-pushed on Apr 4, 2017

droark force-pushed on Apr 7, 2017

droark force-pushed on Apr 10, 2017

droark force-pushed on Apr 14, 2017

in src/coinsbyscript.cpp:1 in 5227e68c66 outdated

0@@ -0,0 +1,280 @@
1+// Copyright (c) 2014-2016 The Bitcoin developers

luke-jr commented at 11:19 am on April 17, 2017:

Standard copyright line says “The Bitcoin Core developers”.

Might as well start this one off with the end year 2017.

in src/coinsbyscript.cpp:14 in 5227e68c66 outdated

 9+
10+#include <assert.h>
11+
12+#include <boost/thread.hpp>
13+
14+using namespace std;

luke-jr commented at 11:19 am on April 17, 2017:

Don’t do this.

in src/init.cpp:1497 in 5227e68c66 outdated

1491@@ -1484,6 +1492,58 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1492                     }
1493                 }
1494 
1495+                // Check -txoutindex
1496+                pcoinsByScriptDB->ReadFlag("txoutindex", fTxOutIndex);

luke-jr commented at 11:23 am on April 17, 2017:

I don’t think we really need a dedicated flag in the db for this. Either it exists and has a “bestblock” defined, or it doesn’t…

in src/coinsbyscript.cpp:18 in 5227e68c66 outdated

13+
14+using namespace std;
15+
16+static const char DB_COINS_BYSCRIPT = 'd';
17+static const char DB_FLAG = 'F';
18+static const char DB_BEST_BLOCK = 'B';

luke-jr commented at 11:25 am on April 17, 2017:

IMO it would be better to not have overlapping chars between databases, such that they could be combined cleanly if desired. Therefore, I suggest using 'D' for DB_BEST_BLOCK (and eliminating DB_FLAG here entirely).

in src/coinsbyscript.cpp:23 in 5227e68c66 outdated

18+static const char DB_BEST_BLOCK = 'B';
19+
20+CCoinsViewByScript::CCoinsViewByScript(CCoinsViewByScriptDB* viewIn) : base(viewIn) { }
21+
22+bool CCoinsViewByScript::GetCoinsByScript(const CScript &script, CCoinsByScript &coins) {
23+    const CScriptID key = CScriptID(script);

luke-jr commented at 11:31 am on April 17, 2017:

This isn’t the purpose of CScriptID, which is specific to P2SH addresses. Suggest having a static ScriptIndexHash function that returns a (possibly typedef’d) uint160.

in src/coinsbyscript.cpp:35 in 5227e68c66 outdated

30+        return true;
31+    }
32+    return false;
33+}
34+
35+CCoinsMapByScript::iterator CCoinsViewByScript::FetchCoinsByScript(const CScript &script, bool fRequireExisting) {

luke-jr commented at 11:33 am on April 17, 2017:

The name sucks. :/

in src/coinsbyscript.cpp:53 in 5227e68c66 outdated

48+    return cacheCoinsByScript.emplace_hint(it, key, tmp);
49+}
50+
51+CCoinsByScript &CCoinsViewByScript::GetCoinsByScript(const CScript &script, bool fRequireExisting) {
52+    CCoinsMapByScript::iterator it = FetchCoinsByScript(script, fRequireExisting);
53+    assert(it != cacheCoinsByScript.end());

luke-jr commented at 11:36 am on April 17, 2017:

Rather throw an exception here.

in src/coinsbyscript.cpp:51 in 5227e68c66 outdated

46+    }
47+
48+    return cacheCoinsByScript.emplace_hint(it, key, tmp);
49+}
50+
51+CCoinsByScript &CCoinsViewByScript::GetCoinsByScript(const CScript &script, bool fRequireExisting) {

luke-jr commented at 11:36 am on April 17, 2017:

Name also sucks, and overlaps with a fairly different usage-case.

in src/coinsbyscript.cpp:87 in 5227e68c66 outdated

82+        if (it->second.IsEmpty())
83+            batch.Erase(make_pair(DB_COINS_BYSCRIPT, it->first));
84+        else
85+            batch.Write(make_pair(DB_COINS_BYSCRIPT, it->first), it->second);
86+        CCoinsMapByScript::iterator itOld = it++;
87+        pcoinsViewByScriptIn->cacheCoinsByScript.erase(itOld);

luke-jr commented at 11:40 am on April 17, 2017:

I don’t see why this is necessary: we clear the entire cache when complete.

Eliminating this erase allows simplifying the entire loop to a normal C++11 for-each.

in src/coinsbyscript.cpp:172 in 5227e68c66 outdated

167+        try {
168+            CScriptID hash;
169+            if (!pcursor->GetKey(hash))
170+                break;
171+            v.push_back(hash);
172+            if (v.size() >= 10000)

luke-jr commented at 11:43 am on April 17, 2017:

This is lacking comments explaining why.

in src/coinsbyscript.cpp:204 in 5227e68c66 outdated

199+        db.WriteBatch(batch);
200+    }
201+    if (i > 0)
202+        LogPrintf("Address index with %d addresses successfully deleted.\n", i);
203+
204+    return true;

luke-jr commented at 11:44 am on April 17, 2017:

Fails to delete DB_BEST_BLOCK

in src/coinsbyscript.cpp:229 in 5227e68c66 outdated

224+            uint256 txhash;
225+            CCoins coins;
226+            if (!pcursor->GetKey(txhash) || !pcursor->GetValue(coins))
227+                break;
228+
229+            for (unsigned int j = 0; j < coins.vout.size(); j++)

luke-jr commented at 11:45 am on April 17, 2017:

Use size_t and ++j. It may also be better to do this backward:

0for (size_t j = coins.vout.size(); j--; ) {

(note j-- in this case because we want to look at the pre-decrement value)

in src/coinsbyscript.cpp:245 in 5227e68c66 outdated

240+                }
241+                mapCoinsByScript[key].setCoins.insert(COutPoint(txhash, (uint32_t)j));
242+                i++;
243+            }
244+
245+            if (mapCoinsByScript.size() >= 10000)

luke-jr commented at 11:49 am on April 17, 2017:

Since we’re doing partial writes, we should ensure DB_BEST_BLOCK is cleared before we begin.

in src/coinsbyscript.cpp:254 in 5227e68c66 outdated

249+                    if (it->second.IsEmpty())
250+                        batch.Erase(make_pair(DB_COINS_BYSCRIPT, it->first));
251+                    else
252+                        batch.Write(make_pair(DB_COINS_BYSCRIPT, it->first), it->second);
253+                    CCoinsMapByScript::iterator itOld = it++;
254+                    mapCoinsByScript.erase(itOld);

luke-jr commented at 11:50 am on April 17, 2017:

Needless erase

in src/coinsbyscript.cpp:274 in 5227e68c66 outdated

269+           if (it->second.IsEmpty())
270+               batch.Erase(make_pair(DB_COINS_BYSCRIPT, it->first));
271+           else
272+               batch.Write(make_pair(DB_COINS_BYSCRIPT, it->first), it->second);
273+           CCoinsMapByScript::iterator itOld = it++;
274+           mapCoinsByScript.erase(itOld);

luke-jr commented at 11:50 am on April 17, 2017:

Needless erase

in src/coinsbyscript.cpp:279 in 5227e68c66 outdated

274+           mapCoinsByScript.erase(itOld);
275+       }
276+       db.WriteBatch(batch);
277+    }
278+    LogPrintf("Address index with %d outputs successfully built.\n", i);
279+    return true;

luke-jr commented at 11:51 am on April 17, 2017:

DB_BEST_BLOCK is never written here.

in src/coinsbyscript.h:1 in 5227e68c66 outdated

0@@ -0,0 +1,116 @@
1+// Copyright (c) 2014-2016 The Bitcoin developers

luke-jr commented at 11:52 am on April 17, 2017:

(see copyright line comments earlier)

in src/coinsbyscript.h:28 in 5227e68c66 outdated

23+    std::set<COutPoint> setCoins;
24+
25+    // empty constructor
26+    CCoinsByScript() { }
27+
28+    bool IsEmpty() const {

luke-jr commented at 11:53 am on April 17, 2017:

Perhaps this ought to be named empty like C++ STL?

in src/coinsbyscript.h:23 in 5227e68c66 outdated

18+
19+class CCoinsByScript
20+{
21+public:
22+    // unspent transaction outputs
23+    std::set<COutPoint> setCoins;

luke-jr commented at 11:53 am on April 17, 2017:

Why not just subclass std::set<COutPoint>?

in src/coinsbyscript.h:51 in 5227e68c66 outdated

46+class CCoinsViewByScript
47+{
48+private:
49+    CCoinsViewByScriptDB *base;
50+
51+    mutable uint256 hashBlock;

luke-jr commented at 11:54 am on April 17, 2017:

Why mutable?

in src/coinstats.cpp:1 in 5227e68c66 outdated

0@@ -0,0 +1,72 @@
1+// Copyright (c) 2014-2016 The Bitcoin Core developers

luke-jr commented at 11:55 am on April 17, 2017:

(see copyright line comments)

in src/coinstats.cpp:12 in 5227e68c66 outdated

 7+
 8+#include <stdint.h>
 9+
10+#include <boost/thread/thread.hpp> // boost::thread::interrupt
11+
12+using namespace std;

luke-jr commented at 11:55 am on April 17, 2017:

Don’t.

in src/coinstats.cpp:15 in 5227e68c66 outdated

10+#include <boost/thread/thread.hpp> // boost::thread::interrupt
11+
12+using namespace std;
13+
14+//! Calculate statistics about the unspent transaction output set
15+bool GetUTXOStats(CCoinsView *view, CCoinsViewByScriptDB *viewbyscriptdb, CCoinsStats &stats)

luke-jr commented at 11:56 am on April 17, 2017:

(didn’t review)

in src/init.cpp:377 in 5227e68c66 outdated

372@@ -368,6 +373,7 @@ std::string HelpMessage(HelpMessageMode mode)
373     strUsage += HelpMessageOpt("-sysperms", _("Create new files with system default permissions, instead of umask 077 (only effective with disabled wallet functionality)"));
374 #endif
375     strUsage += HelpMessageOpt("-txindex", strprintf(_("Maintain a full transaction index, used by the getrawtransaction rpc call (default: %u)"), DEFAULT_TXINDEX));
376+    strUsage += HelpMessageOpt("-txoutindex", strprintf(_("Maintain an address to unspent outputs index (rpc: getutxoindex). The index is built on first use. (default: %u)"), 0));

luke-jr commented at 11:58 am on April 17, 2017:

Should probably be renamed to -utxoscriptindex.

-txoutindex suggests it indexes all txouts (by what?).

in src/init.cpp:1442 in 5227e68c66 outdated

1437@@ -1432,11 +1438,13 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1438                 UnloadBlockIndex();
1439                 delete pcoinsTip;
1440                 delete pcoinsdbview;
1441+                delete pcoinsByScriptDB;

luke-jr commented at 11:59 am on April 17, 2017:

Not really clear why we have anything to delete here, but don’t we need to delete pcoinsByScript also?

in src/init.cpp:1503 in 5227e68c66 outdated

1497+                if (IsArgSet("-txoutindex"))
1498+                {
1499+                    if (GetBoolArg("-txoutindex", false))
1500+                    {
1501+                        // build index
1502+                        if (!fTxOutIndex)

luke-jr commented at 12:03 pm on April 17, 2017:

We also need to build the index if it’s outdated. For example, if the user ran an older version to sync. (This is fixed automatically if you replace the flag with a check that the index’s best block matches that of the chainstate.)

in src/rest.cpp:604 in 5227e68c66 outdated

600@@ -598,6 +601,109 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
601     return true; // continue to process further HTTP reqs on this cxn
602 }
603 
604+static bool rest_getutxoindex(HTTPRequest* req, const std::string& strURIPart)

luke-jr commented at 12:03 pm on April 17, 2017:

(did not review)

in src/rpc/blockchain.cpp:831 in 5227e68c66 outdated

827@@ -876,6 +828,8 @@ UniValue gettxoutsetinfo(const JSONRPCRequest& request)
828             "  \"bestblock\": \"hex\",   (string) the best block hash hex\n"
829             "  \"transactions\": n,      (numeric) The number of transactions\n"
830             "  \"txouts\": n,            (numeric) The number of output transactions\n"
831+            "  \"addresses\": n,         (numeric) The number of addresses and scripts. Only if -txoutindex=1\n"

luke-jr commented at 12:04 pm on April 17, 2017:

Call it "uniquescripts"?

in src/rpc/blockchain.cpp:832 in 5227e68c66 outdated

827@@ -876,6 +828,8 @@ UniValue gettxoutsetinfo(const JSONRPCRequest& request)
828             "  \"bestblock\": \"hex\",   (string) the best block hash hex\n"
829             "  \"transactions\": n,      (numeric) The number of transactions\n"
830             "  \"txouts\": n,            (numeric) The number of output transactions\n"
831+            "  \"addresses\": n,         (numeric) The number of addresses and scripts. Only if -txoutindex=1\n"
832+            "  \"utxoindex\": n,   (numeric) The number of output transactions. Only if -txoutindex=1\n"

luke-jr commented at 12:05 pm on April 17, 2017:

Duplicates "txouts". Remove and simply add an integrity check.

in src/rpc/blockchain.cpp:955 in 5227e68c66 outdated

950+            "\nTo use this function, you must start bitcoin with the -txoutindex parameter.\n"
951+            "\nArguments:\n"
952+            "1. minconf          (numeric) Minimum confirmations\n"
953+            "2. \"addresses\"    (string) A json array of bitcoin addresses (or scripts)\n"
954+            "    [\n"
955+            "      \"address\"   (string) bitcoin address (or script)\n"

luke-jr commented at 12:06 pm on April 17, 2017:

The address abstraction ends before/when coins hit the UTXO set. UTXOs do not have and are not associated to addresses.

Rename this to "scripts", and don’t support specifying as addresses.

in src/rpc/blockchain.cpp:994 in 5227e68c66 outdated

989+            + HelpExampleCli("getutxoindex", "6 \"[\\\"1PGFqEzfmQch1gKD3ra4k18PNj3tTUUSqg\\\",\\\"1LtvqCaApEdUGFkpKMM4MstjcaL4dKg8SP\\\"]\"")
990+            + "\nAs a json rpc call\n"
991+            + HelpExampleRpc("getutxoindex", "6, \"[\\\"1PGFqEzfmQch1gKD3ra4k18PNj3tTUUSqg\\\",\\\"1LtvqCaApEdUGFkpKMM4MstjcaL4dKg8SP\\\"]\"")
992+        );
993+
994+    if (!fTxOutIndex)

luke-jr commented at 12:07 pm on April 17, 2017:

Add braces

in src/rpc/blockchain.cpp:1004 in 5227e68c66 outdated

 999+    UniValue vObjects(UniValue::VARR);
1000+    std::vector<std::pair<int, unsigned int> > vSort;
1001+    int nMinDepth = request.params[0].get_int();
1002+    UniValue inputs = request.params[1].get_array();
1003+
1004+    int nCount = 999999999;

luke-jr commented at 12:08 pm on April 17, 2017:

Max [guaranteed] value of int is 32767, so this could overflow.

in src/rpc/blockchain.cpp:1035 in 5227e68c66 outdated

1030+
1031+        CCoinsByScript coinsByScript;
1032+        pcoinsByScript->GetCoinsByScript(script, coinsByScript);
1033+
1034+        if (nMinDepth == 0)
1035+            mempool.GetCoinsByScript(script, coinsByScript);

luke-jr commented at 12:10 pm on April 17, 2017:

Need to document that mempool.GetCoinsByScript must only add, not replace.

in src/rpc/blockchain.cpp:1041 in 5227e68c66 outdated

1036+
1037+        CoinsByScriptToJSON(coinsByScript, nMinDepth, vObjects, vSort, true); 
1038+    }
1039+
1040+    UniValue results(UniValue::VARR);
1041+    sort(vSort.begin(), vSort.end());

luke-jr commented at 12:11 pm on April 17, 2017:

Why sort? Let the caller sort if they need it sorted somehow.

in src/rpc/blockchain.cpp:1042 in 5227e68c66 outdated

1037+        CoinsByScriptToJSON(coinsByScript, nMinDepth, vObjects, vSort, true); 
1038+    }
1039+
1040+    UniValue results(UniValue::VARR);
1041+    sort(vSort.begin(), vSort.end());
1042+    for (unsigned int i = (unsigned int)nFrom; i < vSort.size(); i++)

luke-jr commented at 12:11 pm on April 17, 2017:

size_t

in src/rpc/rawtransaction.cpp:127 in 5227e68c66 outdated

121@@ -122,6 +122,63 @@ void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry)
122     }
123 }
124 
125+void CoinsByScriptToJSON(const CCoinsByScript& coinsByScript, int nMinDepth, UniValue& vObjects, std::vector<std::pair<int, unsigned int>>& vSort, bool fIncludeHex)
126+{
127+    BOOST_FOREACH(const COutPoint &outpoint, coinsByScript.setCoins)

luke-jr commented at 12:12 pm on April 17, 2017:

Use C++11 for-each.

in src/rpc/blockchain.cpp:949 in 5227e68c66 outdated

944+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 4)
945+        throw std::runtime_error(
946+            "getutxoindex ( minconf [\"address\",...] count from )\n"
947+            "\nReturns a list of unspent transaction outputs by address (or script).\n"
948+            "The list is ordered by confirmations in descending order.\n"
949+            "Note that passing minconf=0 will include the mempool.\n"

luke-jr commented at 12:14 pm on April 17, 2017:

Consider the case of a current UTXO that has been spent in the mempool. Under what conditions is the mempool ignored and that UTXO returned? Would you ever want to see it while also searching the mempool?

in src/rpc/rawtransaction.cpp:139 in 5227e68c66 outdated

134+            if (!view.GetCoins(outpoint.hash, coins))
135+                continue;
136+            mempool.pruneSpent(outpoint.hash, coins); // TODO: this should be done by the CCoinsViewMemPool
137+        }
138+        else if (!pcoinsTip->GetCoins(outpoint.hash, coins))
139+            continue;

luke-jr commented at 12:15 pm on April 17, 2017:

Inconsistent behaviour for UTXOs created in mined transactions vs in the mempool. We should probably consider mempool spends the same for both mempool-created UTXOs and confirmed-tx-created UTXOs.

in src/rpc/rawtransaction.cpp:166 in 5227e68c66 outdated

161+            o.push_back(Pair("txid", outpoint.hash.GetHex()));
162+            o.push_back(Pair("vout", (int)outpoint.n));
163+            o.push_back(Pair("value", ValueFromAmount(coins.vout[outpoint.n].nValue)));
164+            o.push_back(Pair("scriptPubKey", oScriptPubKey));
165+            o.push_back(Pair("version", coins.nVersion));
166+            o.push_back(Pair("coinbase", coins.fCoinBase));

luke-jr commented at 12:17 pm on April 17, 2017:

Rather call this "generated"

in src/test/blockencodings_tests.cpp:60 in 5227e68c66 outdated

56@@ -57,7 +57,7 @@ static CBlock BuildBlockTestCase() {
57 
58 BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
59 {
60-    CTxMemPool pool;
61+    CTxMemPool pool(false);

luke-jr commented at 12:17 pm on April 17, 2017:

false is the default. No need to change all this.

in src/txdb.cpp:102 in 5227e68c66 outdated

 98@@ -98,7 +99,8 @@ CCoinsViewCursor *CCoinsViewDB::Cursor() const
 99        that restriction.  */
100     i->pcursor->Seek(DB_COINS);
101     // Cache key of first record
102-    i->pcursor->GetKey(i->keyTmp);
103+    if (!i->pcursor->Valid() || !i->pcursor->GetKey(i->keyTmp))

luke-jr commented at 12:17 pm on April 17, 2017:

Braces

in src/txmempool.cpp:365 in 5227e68c66 outdated

370@@ -370,6 +371,16 @@ unsigned int CTxMemPool::GetTransactionsUpdated() const
371     return nTransactionsUpdated;
372 }
373 
374+void CTxMemPool::GetCoinsByScript(const CScript& script, CCoinsByScript& coinsByScript) const
375+{

luke-jr commented at 12:19 pm on April 17, 2017:

Throw an exception if CTxMemPool was created without it?

in src/txmempool.cpp:436 in 5227e68c66 outdated

442@@ -432,6 +443,11 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
443     vTxHashes.emplace_back(tx.GetWitnessHash(), newit);
444     newit->vTxHashesIdx = vTxHashes.size() - 1;
445 
446+    if (fTxOutIndex)

luke-jr commented at 12:19 pm on April 17, 2017:

Braces…

in src/validation.cpp:2061 in 5227e68c66 outdated

2054@@ -2051,6 +2055,62 @@ void PruneAndFlush() {
2055     FlushStateToDisk(state, FLUSH_STATE_NONE);
2056 }
2057 
2058+void static UpdateAddressIndex(const CTxOut& txout, const COutPoint& outpoint, bool fInsert)

luke-jr commented at 12:22 pm on April 17, 2017:

This should be moved to coinsbyscript.cpp and renamed.

in src/validation.cpp:2073 in 5227e68c66 outdated

2065+        else
2066+            coinsByScript.setCoins.erase(outpoint);
2067+    }
2068+}
2069+
2070+void static UpdateAddressIndex(const CBlock& block, CBlockUndo& blockundo, bool fConnect)

luke-jr commented at 12:22 pm on April 17, 2017:

This should be moved to coinsbyscript.cpp and renamed.

in test/functional/rest.py:50 in 5227e68c66 outdated

46@@ -47,14 +47,14 @@ def __init__(self):
47         super().__init__()
48         self.setup_clean_chain = True
49         self.num_nodes = 3
50+        self.extra_args = [["-txoutindex"]] * 3

luke-jr commented at 12:25 pm on April 17, 2017:

(didn’t review tests)

in test/functional/utxoindex.py:1 in 5227e68c66 outdated

0@@ -0,0 +1,95 @@
1+#!/usr/bin/env python3

luke-jr commented at 12:25 pm on April 17, 2017:

(didn’t review tests)

luke-jr changes_requested

droark force-pushed on Apr 22, 2017

droark commented at 1:19 am on April 22, 2017: contributor

@luke-jr - Thanks for the detailed feedback. I’ll look over it this weekend.

droark force-pushed on Apr 25, 2017

txoutsbyaddress index c889147288

move txoutsbyaddress index to its own database 536eb705b7

add txoutsbyaddress querying via rest interface 3ae8aa098d

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

convert txoutsbyaddress test to python a2873c261c

- add txoutsbyaddress REST interface tests

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

ddddc03455

address test review nits 113402255c

rename -txoutsbyaddress to -txoutindex efa276b55d

set executable bit 30460d5847

add doc for gettxoutsbyaddress REST endpoint 6caf075e7e

fix up a change missed by the rebase cb43d8a924

Fixup to remove errors and warnings 80bf01465a

Replace PR’s remaining Boost calls with C++11 82a9ea7c5b

Fixup - Move file missed by a rebase bd5c62976d

Code feedback cleanup

- Switched getutxosbyaddress to getutxoindex, and altered the files as needed to reflect this change.
- Switched CCoinsMapByScript key from uint160 to CScriptID.
- Various C++11-related changes.
- Removed a try/catch case from CCoinsViewDB::CountCoins().
- Changed a const ref in CTxMemPool to a const copy to prevent a possible subtle error.
- Added a default value (false) for UTXO indexing under CTxMemPool.

77aa7e091d

droark force-pushed on Apr 27, 2017

dexX7 commented at 9:01 pm on May 25, 2017: contributor

Thanks for preparing this @droark. Is this PR still alive?

droark commented at 8:06 am on May 30, 2017: contributor

@dexX7 - Thanks for checking in. Some family-related issues came up recently that I had to deal with for awhile. I’m almost done rebasing the PR and will catch up with the remaining feedback ASAP, along with some test harness changes that probably need to be folded in. The path forward looks pretty clear to me, IMO. I just need to wrap up the work.

laanwj removed this from the milestone 0.15.0 on Jul 6, 2017

jtimon commented at 3:34 am on July 7, 2017: contributor

I still need to read more, but…

Regarding searching by address , by scriptPubKey, or by COutPoint (tx_id, output_pos), I’m not sure whether I want them all or a subset of them. The later doesn’t require any further index. Searching by scriptPubKey only requires an additional scriptPubKey -> COutPoint index but not much logic. Searching by address on the other hand…there’s many types of addresses and people still propose new better ones…I wonder if perhaps the translation address -> scriptPubKey (which shouldn’t require an extra index on top of the scriptPubKey one) belongs to an upper layer like the wallet and/or ui that the users of this feature implement. I’m like supporting it here, but maybe script/standard needs to be extended for the address -> scriptPubKey translator (sorry to reiterate, but I don’t think we want an address index, just a one way translation function [the other way is for wallets and relay policies, or higher level protocols ala coinjoin and whatnot]).

This all assumes you create a new scriptPubKey -> COutPoint index.

Regarding utxo vs stxo vs txo… You can easily serve both utxo and stxo (that is, the whole txo) by just calling GetTransaction() (in validation.o) like getrawtransaction does, without needing any consideration on whether the the output is spent or not while getting the better performance if the COutPoint happens to be in the current utxo, which is very nice. That will just complain if you search when you are pruning or not using -txindex and thus you cannot serve certain outputs unless you do a rescan which is completely out of the question. You probably want a couple of options -utxoscriptindex and -txoscriptindex or something of the sort. Having -utxoscriptindex but not -txoscriptindex maybe should force the error when searching for spent txo’s. Having both is just reduntant. Having -txoscriptindex implies having the full historic scriptPubKey -> COutPoint index. You don’t even need to remove the spent entries from the index if you only have -utxoscriptindex if you make sure the outputs fail when -utxoscriptindex is set but -txoscriptindex is not. In that case, garbage collecting or “is spent reference counting” or “worrying about reorgs” or “adding a spent bool to the index” would probably be nice additions to have since that selection could prune them and otherwise they will accumulate over running time, even if they’re just entries on an index and not the outputs themselves. That seems like it could be an important distraction in the short term.

So although I’m not against gettxoutsbyaddress and I celebrate concept ACK it, but I think it would be nicer to get gettxoutsbyoutpoint and gettxoutsbyscript reviewed and merged in that order first. But I think some other people don’t like chained PRs all that much, so just take it as “if we’re offering gettxoutsbyaddress, we should be able to offer gettxoutsbyoutpoint and gettxoutsbyscript ‘almost for free’(tm) too”.

It may seem contradictory: I’m asking you to do more with respect to utxo/stxo, but I’m suggesting to do less with respect to addresses and properly removing things for the users that want this feature but don’t want to be full archives, which is something we can optimize further later after leveraging the easy important one, which is just looking in the utxo first. The way I see it, the utxo is just the subset of the txo that can’t be pruned, we happen to cache very efficiently, so it is nice to always search there first just in case and well, people usually care less about stxo because it is already spent…but is simple to also serve it or just complain when you can’t or you are not expected to, sorry to reiterate.

Anyway, feel free to note the parts of my long post you like in whatever order you like best and ignore the rest. I plan to have a deeper look either way beyond this concept ack.

All the best but needs rebase.

jtimon commented at 4:48 pm on July 7, 2017: contributor

Sorry, no, using GetTransaction doesn’t leverage the utxo index we maintain…One would need to search in the utxo (with CCoinsViewCache::AccessCoin) first and then try with GetTransaction only if we want to also serve stxo, which should perhaps be left out of scope for this PR as a later improvement.

jtimon commented at 8:28 am on July 14, 2017: contributor

Sorry for the long post, not for being long but for being incorrect.

There’s not need for a gettxobyoutpoint or similar because it already exists and is named gettxout! That means everything I said about “optimize later, it shouldn’t be hard” turns into “it’s already done”.

Only the slower parts were missing, which I plan to serve in #10822 which is just a draft that needs lots of testing.

To reiterate, the important parts of my previous fedback:

I’m more than happy to extend whatever you do for utxo for txo too, ideallly almost transparently
Don’t do an address index, do a more generic script (that means binary or lower level, hex strings on rpc) to outpoint index, and once you have the outpoint you have the whole txo (unless you don’t tcindex or you prune).
Create a function to translate “an address” into a script so that it can be easily searched for in the outpoint -> script index. There will always be new address types your index doesn’t explicitly support yet, make them support your index instead.

The PR that should help extend whatever is done for utxo to txo if you do the index: outpoint -> script; is the following:

#10822

As said it needs testing, but there’s no need to wait, let’s expose utxo if we can first and then txo “almost for free”(tm).

Sorry again for invading the pr in a distractive way, but this needs rebase.

braydonf commented at 1:28 am on April 17, 2018: none

What’s the status of this PR, needs more review and testing? General concept ok?

I think it would be useful to have an index on the UTXO set for the purposes of supporting external and hardware wallets.

jnewbery commented at 10:01 pm on April 23, 2018: member

Closing as ‘up for grabs’. PR author hasn’t checked in for about a year. @droark - if you still want this, feel free to re-open and rebase!

jnewbery closed this on Apr 23, 2018

jnewbery added the label Up for grabs on Apr 23, 2018

MarcoFalke removed the label Up for grabs on Mar 5, 2019

MarcoFalke commented at 7:33 pm on March 5, 2019: member

Picked up in " Add address-based index (attempt 4?) #14053 "

laanwj commented at 2:37 pm on August 20, 2020: member

Picked up in " Add address-based index (attempt 4?) #14053 "

I think those are different, right? this an indeex over the UTXO set, while that is an index over all addresses in the block chain? Or do I get this wrong.

MarcoFalke commented at 3:29 pm on August 20, 2020: member

Indeed looks like it. I wonder why it is called take 4 then.

Though, scanning the utxo set can already be done with scantxoutset, no?

laanwj commented at 4:12 pm on August 20, 2020: member

Though, scanning the utxo set can already be done with scantxoutset, no?

Absolutely, that does the same thing. Though it can be pretty slow, which can be awkward if it’s part of some user flow, so UTXO indexing is not completely out of the picture probably.

DrahtBot locked this on Feb 15, 2022

txoutsbyaddress index (take 3) #9806