Switch chainstate db and cache to per-txout model

sipa commented at 12:37 pm on April 12, 2017: member

Since Bitcoin Core v0.8, we’ve used a per-tx model for the chainstate database and its cache. This means that the database is effectively a map from txids to the list of unspent outputs for that transaction. This PR changes that to a map from outpoints ((txid,index) pairs) to just the individual unspent output for that outpoint.

The original reason for aggregating the outputs per transaction was to save space: this way, we could avoid duplicating the txid and transaction meta across the multiple outputs. However, LevelDB internally uses an encoding that omits repeated prefix bytes in keys, and because of that, duplicating the txids is not very significant.

There are many advantages for using a per-txout model:

Simpler code.
Avoiding the CPU overhead of deserializing and serializing the unused outputs.
More predictable memory usage.
More easily adaptable to various cache flushing strategies.

Downsides:

Slightly larger on-disk representation, and sometimes larger in-memory representation (when there are multiple outputs for the same txid in the cache, which becomes optional).

This PR includes:

The main commit from #10248. We’ll be dropping tx nVersion from the undo data later, so reserializing data from disk won’t roundtrip anymore. This change allows us to compute the checksum based on the written data rather than the reserialized data.
#10249, as we’ll be using some c++11 methods of std::unordered_map which aren’t available in all versions of boost::unordered_map.
#10250, as the new tests trigger the problem fixed there.
A forward-compatible undo data format change (new versions can use old undo data, not the other way around).
Switch to a new per-txout coinsview model with related database changes.
Upgrade code to convert the old chainstate to a new chainstate (which is interruptible).
Adapted unit and functional tests (thanks to @ryanofsky for adapting the unit tests).

Tests:

All existing unit and functional tests pass.
Manually verified that continuing a reindex started with old code succeeds (even when recovering from a crash in the middle of flushing while upgrading).

Commit links:

Preparation:

e66dbde6d14cb5f253b3bf8850a98f4fda2d9f49: Add SizeEstimate to CDBBatch
f54580e7e4f225bb615204daef32f72ab8688418: error() in disconnect for disk corruption, not inconsistency
e484652fc36ef7135cf08ad380ea7142b6cbadc0: Introduce CHashVerifier to hash read data
7e0032290669fae5f52c256856c53038511c7db4: Add specialization of SipHash for 256 + 32 bit data
d342424301013ec47dc146a4beb49d5c9319d80a: Remove/ignore tx version in utxo and undo
c3aa0c11947dfd82702df276d39bb7f748dd83a1: Report on-disk size in gettxoutsetinfo
7d991b55dbf0b0f6e21c0680ee3ebd09df09012f: Store/allow tx metadata in all undo records

Switch to COutPoint/Coin instead of uint256/CCoins:

422634e2f5ac1ff74cd358144cecbac63007adc4: Introduce Coin, a single unspent output
cb2c7fdac2dc74368ed24ae4717ed72178956b92: Replace CTxInUndo with Coin
bd83111a0fcfdb97204a0180bcf861d3b53bb6c2: Optimization: Coin&& to ApplyTxInUndo
000391132608343c66488d62625c714814959bc9: Introduce new per-txout CCoinsViewCache functions
f68cdfe92b37f5a75be612b7de3c1a03245696d0: Switch from per-tx to per-txout CCoinsViewCache methods in some places
c87b957a32e03c09d410abadf661f87eb813bcdb: Only pass things committed to by tx’s witness hash to CScriptCheck
8b3868c1b4bf89c41b26ecb3a4b7c3a2557e3868: Switch CScriptCheck to use Coin instead of CCoins
961e4839793f8b4ad37d29672faf1695ff6ec03a: Switch tests from ModifyCoins to AddCoin/SpendCoin
05293f3cb75ad08ca23cba8e795e27d4d5e4d690: Remove ModifyCoins/ModifyNewCoins
13870b56fcd0bfacedce3ae42a3de3d5e9dc7bc1: Replace CCoins-based CTxMemPool::pruneSpent with isSpent
4ec0d9e794e3f338e1ebb8b644ae890d2c2da2ee: Refactor GetUTXOStats in preparation for per-COutPoint iteration
50830796889ecaa458871f1db878c255dd2554cb: Switch CCoinsView and chainstate db from per-txid to per-txout

Cleanup:

ce23efaa5c2e8e50744a896424b01052db34a3d6: Extend coins_tests
97072d6685564dd50aab4c145b1758ccc10863b3: Remove unused CCoins methods
41aa5b79a3d79f8afe4c556b4f14fb1dc0cc3f9f: Pack Coin more tightly
b2af357f39c7d17ab6ddb2938531155bf90126ec: Reduce reserved memory space for flushing
8b25d2c0ce64d7f8577a0c2e601e505c9f1140bf: Upgrade from per-tx database to per-txout
580b023092a28f444068b53792eb542f9d5e6892: [MOVEONLY] Move old CCoins class to txdb.cpp
119e552f7ccd49c0137a3c6b4f94018a84d69620: Merge CCoinsViewCache’s GetOutputFor and AccessCoin
73de2c1ff345ac38c098d7b1bef03176f3ea1f16: Rename CCoinsCacheEntry::coins to coin
a5e02bc7f8a1af27fcafd892d8da651e8f1ab156: Increase travis unit test timeout
589827975f9f241e2f23eb674a7383592bff1cad: scripted-diff: various renames for per-utxo consistency

jonasschnelli added the label UTXO Db and Indexes on Apr 12, 2017

sipa added the label Resource usage on Apr 12, 2017

sipa force-pushed on Apr 12, 2017

gmaxwell commented at 6:03 pm on April 12, 2017: contributor

This is awesome.

sipa force-pushed on Apr 13, 2017

sipa force-pushed on Apr 16, 2017

sipa commented at 5:43 pm on April 16, 2017: member

Rebased on top of new #10148.

sipa force-pushed on Apr 19, 2017

sipa commented at 3:35 pm on April 19, 2017: member

Rebased without #10148 at popular request.

paveljanik commented at 4:01 pm on April 19, 2017: contributor

Few overrides needed probably:

 0+./coins.h:200:10: warning: 'BatchWrite' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
 1+    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
 2+         ^
 3+./coins.h:178:18: note: overridden virtual function is here
 4+    virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock);
 5+                 ^
 6+./coins.h:201:23: warning: 'Cursor' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
 7+    CCoinsViewCursor *Cursor() const;
 8+                      ^
 9+./coins.h:181:31: note: overridden virtual function is here
10+    virtual CCoinsViewCursor *Cursor() const;
11+                              ^
12+2 warnings generated.

And few initializations:

0+./undo.h:70:23: note: initialize the variable 'count' to silence this warning
1+        uint64_t count;
2+                      ^
3+                       = 0
4...
5+./coins.h:81:22: note: initialize the variable 'code' to silence this warning
6+        uint32_t code;
7+                     ^
8+                      = 0

sipa force-pushed on Apr 19, 2017

in test/functional/blockchain.py:55 in 1aa8b8b8ba outdated

51@@ -52,7 +52,6 @@ def _test_gettxoutsetinfo(self):
52         assert_equal(res['txouts'], 200)
53         assert_equal(res['bytes_serialized'], 13924),
54         assert_equal(len(res['bestblock']), 64)
55-        assert_equal(len(res['hash_serialized']), 64)

TheBlueMatt commented at 4:12 pm on April 20, 2017:

Why not just switch to hash_serialized_2?

in src/txdb.cpp:338 in 2f28c1c1ec outdated

336+    CDBBatch batch(db);
337+    while (pcursor->Valid()) {
338+        boost::this_thread::interruption_point();
339+        COldCoins oldcoins;
340+        std::pair<unsigned char, uint256> key;
341+        if (pcursor->GetValue(oldcoins) && pcursor->GetKey(key) && key.first == DB_COINS_OLD) {

TheBlueMatt commented at 4:31 pm on April 20, 2017:

Shouldnt the first two be an error and not a “done upgrading” result?

TheBlueMatt commented at 4:37 pm on April 20, 2017: member

Concept ACK. Briefly looked over most of the commits except the big one, which I only briefly skimmed, but am left wondering if there’s an easier way to get that +650-881 commit in. At least the first 6 commits could be merged separately (I know the first two are already in another PR), but I’m not sure thats sufficient. Anyway, will circle back around to doing a full review as other things get merged.

sipa force-pushed on Apr 21, 2017

sipa force-pushed on Apr 22, 2017

sipa commented at 11:43 am on April 22, 2017: member

I’m splitting the commits up more. I’ve pushed one update already, but I’m working splitting the big commit further.

sipa force-pushed on Apr 22, 2017

sipa force-pushed on Apr 23, 2017

sipa force-pushed on Apr 24, 2017

sipa commented at 9:25 pm on April 24, 2017: member

Done. There is still one commit that does many things at once (“Switch CCoinsView and chainstate db to per-txout”), but splitting it is nontrivial. If requested, I can try splitting the database format change into a second commit, but that would require adding a bunch of conversion logic in the first commit that just gets removed in the second one. Thoughts?

sipa force-pushed on Apr 24, 2017

sipa force-pushed on Apr 25, 2017

in src/undo.h:70 in 8da478ebd7 outdated

92-    inline void SerializationOp(Stream& s, Operation ser_action) {
93-        READWRITE(vprevout);
94+    template <typename Stream>
95+    void Unserialize(Stream& s) {
96+        // TODO: avoid reimplementing vector deserializer
97+        uint64_t count;

paveljanik commented at 8:08 am on April 25, 2017:

init to 0 here please.

sipa force-pushed on Apr 25, 2017

sipa commented at 6:31 pm on April 25, 2017: member

Rebased and reset the author timestamps (so that GitHub shows them in logical order, $@#!*).

sipa force-pushed on Apr 26, 2017

sipa force-pushed on Apr 27, 2017

laanwj added this to the "Blockers" column in a project

in src/undo.h:72 in aad2cf520b outdated

94+    template <typename Stream>
95+    void Unserialize(Stream& s) {
96+        // TODO: avoid reimplementing vector deserializer
97+        uint64_t count;
98+        ::Unserialize(s, COMPACTSIZE(count));
99+        if (count > 111111) { // TODO: avoid hardcoding max txouts per tx

TheBlueMatt commented at 7:53 pm on April 28, 2017:

nit: would be nice to at least use MAX_BLOCK_BASE_SIZE/9, as you indicated in your TODO

sipa commented at 0:41 am on May 13, 2017:

Fixed.

in src/undo.h:47 in aad2cf520b outdated

63+            ::Unserialize(s, VARINT(nVersionDummy));
64+        }
65+        ::Unserialize(s, REF(CTxOutCompressor(REF(txout->out))));
66     }
67+
68+    CTxInUndoSerializer(const CCoin* coins) : txout(const_cast<CCoin*>(coins)) {}

TheBlueMatt commented at 7:55 pm on April 28, 2017:

ugh, would be nice to not add more const_casts. Its an extra ~10 LOC to split CTxInUndoSerializer into CTxInUndoDeserializer and CTxInUndoSerializer. Not 100% sure I love that either, but maybe better than more const_casts…thoughts?

sipa commented at 0:40 am on May 13, 2017:

Fixed.

in src/validation.cpp:1279 in 45425d1227 outdated

1275@@ -1276,11 +1276,8 @@ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txund
1276             if (nPos >= coins->vout.size() || coins->vout[nPos].IsNull())
1277                 assert(false);
1278             // mark an outpoint spent, and construct undo information
1279-            txundo.vprevout.push_back(CTxInUndo(coins->vout[nPos]));
1280+            txundo.vprevout.push_back(CCoin(std::move(coins->vout[nPos]), coins->nHeight, coins->fCoinBase));

TheBlueMatt commented at 8:05 pm on April 28, 2017:

As russ pointed out: I think this just maybe happens to work. If I were to add a CTxOut(CTxOut&&) constructor which SetNull() afterwards the CCoins::Spend call on the next line wouldn’t get to Cleanup(). While even that shouldnt be a bug, I could envision CCoinsModifyer being tweaked to track whether there was actually a modification, which it would think there was not.

In any case, looks like the std::move here doesn’t make it to the end of the PR, so might as well just drop it.

ryanofsky commented at 5:57 pm on May 3, 2017:

In commit “Replace CTxInUndo with CCoin”

Maybe emplace_back

ryanofsky commented at 6:03 pm on May 3, 2017:

In commit “Replace CTxInUndo with CCoin”

I’m not sure this std::move is ok. If vout[nPos] is cleared out then the Spend call below will fail early and never call CCoins::Cleanup.

sipa commented at 11:49 pm on May 12, 2017:

Agree, but I’m not going to rewrite history to fix that now. I can remember it when squashing.

sipa commented at 7:52 pm on May 13, 2017:

Actually, that std::move doesn’t exist anywhere in the current PR. I think you were commenting on an old commit.

in src/validation.cpp:1574 in 6d6a5ffaa1 outdated

1570                 return error("DisconnectBlock(): transaction and undo data inconsistent");
1571             for (unsigned int j = tx.vin.size(); j-- > 0;) {
1572                 const COutPoint &out = tx.vin[j].prevout;
1573-                const CCoin &undo = txundo.vprevout[j];
1574-                if (!ApplyTxInUndo(undo, view, out))
1575+                if (!ApplyTxInUndo(std::move(txundo.vprevout[j]), view, out))

TheBlueMatt commented at 8:09 pm on April 28, 2017:

super nit: would be nice to add a comment to the end of the loop or so that notes that blockUndo should, by that point, be considered to have been tirely std::move’d, since pretty much all of its internal state has been std::moved during the loop.

sipa commented at 0:41 am on May 13, 2017:

Fixed.

in src/coins.cpp:73 in 778f00e920 outdated

161+    }
162+    if (!possible_overwrite) {
163+        if (ret.first->second.coins.IsAvailable(outpoint.n)) {
164+            throw std::logic_error("Adding new coin that replaces non-pruned entry");
165+        }
166+        fresh = !(ret.first->second.flags & CCoinsCacheEntry::DIRTY);

TheBlueMatt commented at 8:45 pm on April 28, 2017:

This is technically insufficient for FRESH (though is in context - that this is only called by AddCoins). I interpret the possible_overwrite to apply only to the given CCoin, not all CCoins for that tx, which thus means you also need to know that the full CCoins (not just this CCoin) is fully-pruned in the parent cache. Of course this bug doesn’t stick around to the end of the PR, so maybe just always set fresh to false in the intermediary commits and leave it at that?

sipa commented at 0:01 am on May 13, 2017:

Agree, but again I’d rather not rewrite history. I can fix it when squashing, but just always setting it to false would not work (it would break the tests, which are very useful in determining that the correct behaviour remains during the whole changeset).

in src/coins.h:284 in aad2cf520b outdated

292      * By making the copy constructor private, we prevent accidentally using it when one intends to create a cache on top of a base cache.
293      */
294     CCoinsViewCache(const CCoinsViewCache &);
295 };
296 
297+//! Utility function to add all of a transaction's outputs to a cache.

TheBlueMatt commented at 9:36 pm on April 28, 2017:

You should document that this function assumes the only coin possible_overwrites in are coinbase transactions, ie that it is equivalent to ModifyNewCoins (and maybe note in a TODO to pass in a boolean as to whether even that is possible - ie if we are or are not yet past BIP 34, because then we can avoid writes to coinbases spent in the same cache, though admittedly the 100-height limit makes that much less useful).

sipa commented at 0:42 am on May 13, 2017:

Done.

in src/coins.cpp:193 in 778f00e920 outdated

188+void CCoinsViewCache::SpendCoin(const COutPoint &outpoint, CCoin* moveout) {
189+    CCoinsMap::iterator it = FetchCoins(outpoint.hash);
190+    if (it == cacheCoins.end()) return;
191+    cachedCoinsUsage -= it->second.coins.DynamicMemoryUsage();
192+    if (moveout && it->second.coins.IsAvailable(outpoint.n)) {
193+        *moveout = CCoin(std::move(it->second.coins.vout[outpoint.n]), it->second.coins.nHeight, it->second.coins.fCoinBase);

TheBlueMatt commented at 9:41 pm on April 28, 2017:

Same comment as before, the std::move here is only barely right, but because its gone by the end of the PR its mostly ok, probably worth just not having since it doesnt change anything at the end.

sipa commented at 7:53 pm on May 13, 2017:

Going to fix this in a squashed version.

in src/coins.cpp:96 in aad2cf520b outdated

189 
190-const CCoins* CCoinsViewCache::AccessCoins(const uint256 &txid) const {
191-    CCoinsMap::const_iterator it = FetchCoins(txid);
192+void CCoinsViewCache::SpendCoin(const COutPoint &outpoint, CCoin* moveout) {
193+    CCoinsMap::iterator it = FetchCoins(outpoint);
194+    if (it == cacheCoins.end()) return;

TheBlueMatt commented at 11:22 pm on April 28, 2017:

Would be nice to return a bool here to keep the previous assert semantics in UpdateCoins, though not a huge deal.

sipa commented at 0:03 am on May 13, 2017:

Meh.

sipa force-pushed on May 3, 2017

sipa commented at 3:34 am on May 3, 2017: member

Rebased.

sipa force-pushed on May 3, 2017

in src/validation.cpp:1507 in f42c47b59d outdated

1503@@ -1504,18 +1504,15 @@ bool ApplyTxInUndo(const CTxInUndo& undo, CCoinsViewCache& view, const COutPoint
1504     CCoinsModifier coins = view.ModifyCoins(out.hash);
1505     if (undo.nHeight != 0) {
1506         // undo data contains height: this is the last output of the prevout tx being spent
1507-        if (!coins->IsPruned())
1508-            fClean = fClean && error("%s: undo data overwriting existing transaction", __func__);
1509+        if (!coins->IsPruned()) fClean = false;

ryanofsky commented at 2:16 pm on May 3, 2017:

In commit “error() in disconnect for disk corruption, not inconsistency”

Given that you’re already changing places where fClean is set, might be good also to change return type of ApplyTxInUndo from bool to DisconnectResult, which I think would make the code more consistent and easier to understand.

in src/validation.cpp:1276 in f42c47b59d outdated

1515-        if (coins->IsPruned())
1516-            fClean = fClean && error("%s: undo data adding output to missing transaction", __func__);
1517+        if (coins->IsPruned()) fClean = false;
1518     }
1519-    if (coins->IsAvailable(out.n))
1520-        fClean = fClean && error("%s: undo data overwriting existing output", __func__);

ryanofsky commented at 2:19 pm on May 3, 2017:

In commit “error() in disconnect for disk corruption, not inconsistency”

Maybe the old error strings should become code comments, since they seem useful for understanding the different cases.

in src/hash.h:185 in 2f0f26b347 outdated

180+    {
181+        char data[1024];
182+        while (nSize > 0) {
183+            size_t now = std::min<size_t>(nSize, 1024);
184+            source->read(data, now);
185+            this->write(data, now);

ryanofsky commented at 2:37 pm on May 3, 2017:

In commit “Introduce CHashVerifier to hash read data”

Could replace these two lines with just read(data, now);

in src/rpc/blockchain.cpp:796 in 43fa7b7b11 outdated

792@@ -793,12 +793,14 @@ static bool GetUTXOStats(CCoinsView *view, CCoinsStats &stats)
793         if (pcursor->GetKey(key) && pcursor->GetValue(coins)) {
794             stats.nTransactions++;
795             ss << key;
796+            ss << VARINT(coins.nHeight * 2 + coins.fCoinBase);

ryanofsky commented at 3:00 pm on May 3, 2017:

In commit “Remove/ignore tx version in utxo and undo”

Maybe mention this change in the commit message. It seems like a good change (I guess it makes the hash more robust), but not really related to the version stuff?

in src/rpc/blockchain.cpp:995 in 43fa7b7b11 outdated

978@@ -977,7 +979,6 @@ UniValue gettxout(const JSONRPCRequest& request)
979     UniValue o(UniValue::VOBJ);
980     ScriptPubKeyToUniv(coins.vout[n].scriptPubKey, o, true);
981     ret.push_back(Pair("scriptPubKey", o));
982-    ret.push_back(Pair("version", coins.nVersion));

ryanofsky commented at 3:04 pm on May 3, 2017:

In commit “Remove/ignore tx version in utxo and undo”

Maybe update doc/release-notes.md noting the various changes in RPC output here.

in src/validation.cpp:1510 in 880868b0a7 outdated

1508+        }
1509+        // restore height/coinbase tx metadata from undo data
1510         coins->fCoinBase = undo.fCoinBase;
1511         coins->nHeight = undo.nHeight;
1512     } else {
1513+        // undo data does not contain height/coinbase: it cannot be the last spent output of a tx

ryanofsky commented at 3:38 pm on May 3, 2017:

In commit “Store/allow tx metadata in all undo records”

Maybe expand this comment, I think new code would be confusing to read without knowing the history. Suggest maybe:

0// Undo data does not contain height/coinbase. This should never happen
1// currently. Previously, this data was only saved for pruned transactions,
2// so check coins->IsPruned().

in src/coins.h:56 in 0810a11afb outdated

51+
52+    //! empty constructor
53+    CCoin() : fCoinBase(false), nHeight(0) { }
54+
55+    //! equality test
56+    friend bool operator==(const CCoin &a, const CCoin &b) {

ryanofsky commented at 3:59 pm on May 3, 2017:

In commit “Introduce CCoin, a single unspent output”

This equality operator doesn’t seem like something you would want to encourage use of. Maybe just move to coins_test where it’s needed.

in src/coins.h:46 in 0810a11afb outdated

41+
42+    //! construct a CCoin from a CTransaction, at a given height
43+    CCoin(CTxOut&& outIn, int nHeightIn, bool fCoinBaseIn) : fCoinBase(fCoinBaseIn), out(std::move(outIn)), nHeight(nHeightIn) {}
44+    CCoin(const CTxOut& outIn, int nHeightIn, bool fCoinBaseIn) : fCoinBase(fCoinBaseIn), out(outIn), nHeight(nHeightIn) {}
45+
46+    void Clear() {

ryanofsky commented at 4:01 pm on May 3, 2017:

In commit “Introduce CCoin, a single unspent output”

Maybe Clear() / Pruned() should be named consistently? Prune/Pruned or Clear/IsCleared

sipa commented at 1:55 am on May 4, 2017:

Not going to rename things all over the place now. Maybe I can add a commit at the end that fixes it up later.

sipa force-pushed on May 3, 2017

in src/undo.h:18 in f9d26448f3 outdated

11@@ -12,56 +12,72 @@
12 
13 /** Undo information for a CTxIn
14  *
15- *  Contains the prevout's CTxOut being spent, and if this was the
16- *  last output of the affected transaction, its metadata as well
17- *  (coinbase or not, height, transaction version)
18+ *  Contains the prevout's CTxOut being spent, and its metadata as well
19+ *  (coinbase or not, height). Earlier versions also stored the transaction
20+ *  version.

ryanofsky commented at 5:28 pm on May 3, 2017:

In commit “Replace CTxInUndo with CCoin”

Guess this comment should have been updated in the earlier commits, but good to see it updated.

in src/validation.cpp:1283 in 389181ab57 outdated

1283-                undo.nHeight = coins->nHeight;
1284-                undo.fCoinBase = coins->fCoinBase;
1285-            }
1286+            CTxInUndo& undo = txundo.vprevout.back();
1287+            undo.nHeight = coins->nHeight;
1288+            undo.fCoinBase = coins->fCoinBase;

ryanofsky commented at 5:36 pm on May 3, 2017:

In commit “Store/allow tx metadata in all undo records”

What does it mean in the commit message that “undo dat written with this patch won’t be readable by older versions anymore.”? If this change is just writing actual data where there used to be 0’s before, why would older versions have a problem reading it?

sipa commented at 1:54 am on May 4, 2017:

Older code requires that there are 0s for everything but the last spend of an output, in the disconnect code. That’s what the “allow” refers to in the commit title: it allows nonzero metadata in any undo record.

ryanofsky commented at 11:13 am on May 4, 2017:

In commit “Store/allow tx metadata in all undo records”

Older code requires that there are 0s for everything but the last spend of an output, in the disconnect code. That’s what the “allow” refers to in the commit title: it allows nonzero metadata in any undo record.

Could you clarify in the commit message what “won’t be readable by older versions anymore” means? Will older software just log errors and keep working, or require re-indexing, or will it not work at all?

I understand that older code has a check for this condition here: https://github.com/bitcoin/bitcoin/blob/431a548faaf51c7a5fc89b6e479187a1c0e29805/src/validation.cpp#L1508. But I also thought this was one of the cases that DisconnectBlock would “graciously deal with” (according to earlier commit message).

sipa commented at 8:20 pm on May 4, 2017:

Will older software just log errors and keep working, or require re-indexing, or will it not work at all?

Older code will fail during the start-up consistency check (the rollback test will notice entries with undo data present where it isn’t expected), and fail to start.

But I also thought this was one of the cases that DisconnectBlock would “graciously deal with”

Well, it does deal with it correctly (it’s DISCONNECT_UNCLEAN case, not a DISCONNECT_FAILED one), but the code calling DisconnectBlock requires a DISCONNECT_OK.

In #10148, DisconnectBlock is called to rewind blocks for replay at startup, and there DISCONNECT_UNCLEAN is acceptable.

in src/undo.h:84 in f9d26448f3 outdated

90-    template <typename Stream, typename Operation>
91-    inline void SerializationOp(Stream& s, Operation ser_action) {
92-        READWRITE(vprevout);
93+    template <typename Stream>
94+    void Unserialize(Stream& s) {
95+        // TODO: avoid reimplementing vector deserializer

ryanofsky commented at 5:52 pm on May 3, 2017:

In commit “Replace CTxInUndo with CCoin”

This isn’t done in the current PR. What’s the future plan?

sipa commented at 1:57 am on May 4, 2017:

I have a plan for doing this cleanly, but it requires some refactoring in serialize.h which I’d like to avoid in this PR.

in src/undo.h:72 in f9d26448f3 outdated

93+    template <typename Stream>
94+    void Unserialize(Stream& s) {
95+        // TODO: avoid reimplementing vector deserializer
96+        uint64_t count;
97+        ::Unserialize(s, COMPACTSIZE(count));
98+        if (count > 111111) { // TODO: avoid hardcoding max txouts per tx

ryanofsky commented at 5:53 pm on May 3, 2017:

In commit “Replace CTxInUndo with CCoin”

What’s the alternative?

sipa commented at 1:58 am on May 4, 2017:

Depending on consensus.h and deriving the constant directly. Alternatively, we may not need this protection if we’d use the std::vector deserialization code (see TODO above).

sdaftuar commented at 1:34 pm on May 12, 2017:

FYI, I get this warning when I compile:

 0In file included from test/test_bitcoin_fuzzy.cpp:19:
 1./undo.h:72:13: warning: variable 'count' is uninitialized when used here [-Wuninitialized]
 2        if (count > 111111) { // TODO: avoid hardcoding max txouts per tx
 3            ^~~~~
 4./serialize.h:544:7: note: in instantiation of function template specialization 'CTxUndo::Unserialize<CDataStream>' requested here
 5    a.Unserialize(is);
 6      ^
 7./streams.h:407:11: note: in instantiation of function template specialization 'Unserialize<CDataStream, CTxUndo>' requested here
 8        ::Unserialize(*this, obj);
 9          ^
10test/test_bitcoin_fuzzy.cpp:155:20: note: in instantiation of function template specialization 'CDataStream::operator>><CTxUndo>' requested here
11                ds >> tu;
12                   ^
13./undo.h:70:23: note: initialize the variable 'count' to silence this warning
14        uint64_t count;
15                      ^
16                       = 0
171 warning generated.

in src/undo.h:63 in f9d26448f3 outdated

80+    void Serialize(Stream& s) const {
81+        // TODO: avoid reimplementing vector serializer
82+        uint64_t count = vprevout.size();
83+        ::Serialize(s, COMPACTSIZE(REF(count)));
84+        uint64_t n = 0;
85+        while (n < count) {

ryanofsky commented at 5:55 pm on May 3, 2017:

In commit “Replace CTxInUndo with CCoin”

This seems like it should be a for loop or range-for loop.

in src/undo.h:78 in f9d26448f3 outdated

 98+        if (count > 111111) { // TODO: avoid hardcoding max txouts per tx
 99+            throw std::ios_base::failure("Too many input undo records");
100+        }
101+        vprevout.resize(count);
102+        uint64_t n = 0;
103+        while (n < count) {

ryanofsky commented at 5:56 pm on May 3, 2017:

In commit “Replace CTxInUndo with CCoin”

for (auto& prevout : vprevout)?

in src/validation.cpp:1280 in f9d26448f3 outdated

1275@@ -1276,11 +1276,8 @@ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txund
1276             if (nPos >= coins->vout.size() || coins->vout[nPos].IsNull())
1277                 assert(false);
1278             // mark an outpoint spent, and construct undo information
1279-            txundo.vprevout.push_back(CTxInUndo(coins->vout[nPos]));
1280+            txundo.vprevout.push_back(CCoin(std::move(coins->vout[nPos]), coins->nHeight, coins->fCoinBase));
1281             coins->Spend(nPos);

ryanofsky commented at 6:04 pm on May 3, 2017:

In commit “Replace CTxInUndo with CCoin”

Might be good to check the return value from Spend. Or have Spend raise errors instead of returning bool.

in src/coins.cpp:96 in 93733c0c97 outdated

185+    }
186+}
187+
188+void CCoinsViewCache::SpendCoin(const COutPoint &outpoint, CCoin* moveout) {
189+    CCoinsMap::iterator it = FetchCoins(outpoint.hash);
190+    if (it == cacheCoins.end()) return;

ryanofsky commented at 6:53 pm on May 3, 2017:

In commit “Introduce new per-txout CCoinsViewCache functions”

Should it be an error to spend a coin that can’t be found? Can you add a comment here explaining when this case is expected to happen if it is not an error condition?

in src/coins.cpp:192 in 93733c0c97 outdated

187+
188+void CCoinsViewCache::SpendCoin(const COutPoint &outpoint, CCoin* moveout) {
189+    CCoinsMap::iterator it = FetchCoins(outpoint.hash);
190+    if (it == cacheCoins.end()) return;
191+    cachedCoinsUsage -= it->second.coins.DynamicMemoryUsage();
192+    if (moveout && it->second.coins.IsAvailable(outpoint.n)) {

ryanofsky commented at 6:54 pm on May 3, 2017:

In commit “Introduce new per-txout CCoinsViewCache functions”

Again should it be an error if IsAvailable is not true?

in src/coins.h:226 in 93733c0c97 outdated

481@@ -480,6 +482,13 @@ class CCoinsViewCache : public CCoinsViewBacked
482     const CCoins* AccessCoins(const uint256 &txid) const;
483 
484     /**
485+     * Return a reference to CCoin in the cache, or a pruned one if not found. This is
486+     * more efficient than GetCoins. Modifications to other cache entries are
487+     * allowed while accessing the returned pointer.

ryanofsky commented at 6:59 pm on May 3, 2017:

In commit “Introduce new per-txout CCoinsViewCache functions”

Just noting at this point the comment is aspirational. It becomes true in a later commit.

in src/rpc/rawtransaction.cpp:635 in 40982486bb outdated

631@@ -632,9 +632,7 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
632         view.SetBackend(viewMempool); // temporarily switch cache backend to db+mempool view
633 
634         BOOST_FOREACH(const CTxIn& txin, mergedTx.vin) {
635-            const uint256& prevHash = txin.prevout.hash;
636-            CCoins coins;
637-            view.AccessCoins(prevHash); // this is certainly allowed to fail
638+            view.AccessCoin(txin.prevout); // this is certainly allowed to fail

ryanofsky commented at 7:23 pm on May 3, 2017:

In commit “Switch from per-tx to per-txout CCoinsViewCache methods in some places”

Could you add a comment saying what this is for? This would only seem to affect the internal state of the view. Realize your change probably doesn’t affect whatever it’s supposed to be doing,

in src/validation.cpp:1020 in 40982486bb outdated

1015@@ -1016,11 +1016,14 @@ bool GetTransaction(const uint256 &hash, CTransactionRef &txOut, const Consensus
1016 
1017     if (fAllowSlow) { // use coin database to locate block that contains transaction, and scan it
1018         int nHeight = -1;
1019-        {
1020+        size_t out = 0;
1021+        while (nHeight == -1 && out < 1024) { // Only try first 1024 outputs

ryanofsky commented at 7:47 pm on May 3, 2017:

In commit “Switch from per-tx to per-txout CCoinsViewCache methods in some places”

Seems like there should be a more direct way to do this using leveldb Iterator::Seek. Unfortunately the cache map is unordered so harder to search by txid.

ryanofsky commented at 7:48 pm on May 3, 2017:

In commit “Switch from per-tx to per-txout CCoinsViewCache methods in some places”

Seems like should be a for loop, not while.

sipa commented at 1:54 am on May 4, 2017:

This is very hard, unfortunately.

in src/validation.cpp:1501 in 40982486bb outdated

1501+        // Missing undo metadata (height and coinbase). Older versions included this
1502+        // information only in undo records for the last spend of a transactions'
1503+        // outputs. This implies that it must be present for some other output of the same tx.
1504+        COutPoint iter(out.hash, 0);
1505+        bool found = false;
1506+        while (iter.n * ::GetSerializeSize(CTxOut(), SER_NETWORK, PROTOCOL_VERSION) < MAX_BLOCK_BASE_SIZE) {

ryanofsky commented at 8:01 pm on May 3, 2017:

In commit “Switch from per-tx to per-txout CCoinsViewCache methods in some places”

This commit is a lot to take in at once. It consists of a lot of straightforward replacements, plus 2 more gnarly workarounds (the “try first 1024 outputs” code above, and this undo height/coinbase reconstruction). Would recommend splitting the two workarounds out into separate commits so they don’t get in the way of the easier parts of the review.

in src/validation.cpp:1502 in 40982486bb outdated

1502+        // information only in undo records for the last spend of a transactions'
1503+        // outputs. This implies that it must be present for some other output of the same tx.
1504+        COutPoint iter(out.hash, 0);
1505+        bool found = false;
1506+        while (iter.n * ::GetSerializeSize(CTxOut(), SER_NETWORK, PROTOCOL_VERSION) < MAX_BLOCK_BASE_SIZE) {
1507+            const auto alternate = view.AccessCoin(iter);

ryanofsky commented at 8:03 pm on May 3, 2017:

In commit “Switch from per-tx to per-txout CCoinsViewCache methods in some places”

Meant to be const auto&?

in src/test/coins_tests.cpp:726 in b3c81cf91a outdated

728      * entry in the cache after the modification. Verify behavior with the
729      * with the ModifyNewCoin coinbase argument set to false, and to true.
730      *
731-     *                  Cache   Write   Result  Cache        Result     Coinbase
732-     *                  Value   Value   Value   Flags        Flags
733+     *           Cache   Write   Result  Cache        Result     Coinbase

ryanofsky commented at 8:21 pm on May 3, 2017:

In commit “Switch tests from ModifyCoins to AddCoin/SpendCoin”

Consider replacing “coinbase” everywhere in this test with “potential_overwrite” to reflect the new name of the argument.

in src/rpc/blockchain.cpp:795 in 19eb485cc4 outdated

772@@ -773,6 +773,21 @@ struct CCoinsStats
773     CCoinsStats() : nHeight(0), nTransactions(0), nTransactionOutputs(0), nSerializedSize(0), nTotalAmount(0) {}
774 };
775 
776+static void ApplyStats(CCoinsStats &stats, CHashWriter& ss, const uint256& hash, const std::map<uint32_t, CCoin>& outputs)
777+{
778+    ss << hash;
779+    ss << VARINT(outputs.begin()->second.nHeight * 2 + outputs.begin()->second.fCoinBase);

ryanofsky commented at 8:29 pm on May 3, 2017:

In commit “Refactor GetUTXOStats in preparation for per-COutPoint iteration”

Should check or assert outputs.size() is not 0.

in src/coins.h:106 in 25b95d6b17 outdated

336 };
337 
338 struct CCoinsCacheEntry
339 {
340-    CCoins coins; // The actual cached data.
341+    CCoin coins; // The actual cached data.

ryanofsky commented at 8:34 pm on May 3, 2017:

In commit “Switch CCoinsView and chainstate db from per-txid to per-txout”

Should this be called coin instead of coins? Also should CCoinsCacheEntry class be renamed? (maybe in a separate commit if it would make this diff bigger.)

sipa commented at 1:56 am on May 4, 2017:

It actually used to be called coin in an earlier version of the patch, but just s/coins/coin/ changes were such a substantial fraction of the commit that I backed out. Can be done later, I guess.

in src/coins.h:350 in 25b95d6b17 outdated

345@@ -346,21 +346,22 @@ struct CCoinsCacheEntry
346          */
347     };
348 
349-    CCoinsCacheEntry() : coins(), flags(0) {}
350-    CCoinsCacheEntry(nullptr_t) : coins(), flags(0) {}
351+    CCoinsCacheEntry() : flags(0) {}
352+    explicit CCoinsCacheEntry(std::nullptr_t) : coins(), flags(0) {}

ryanofsky commented at 8:36 pm on May 3, 2017:

In commit “Switch CCoinsView and chainstate db from per-txid to per-txout”

This seems identical to the default constructor. Maybe remove and call that instead, or remove the default constructor.

in src/net_processing.cpp:915 in 25b95d6b17 outdated

913@@ -914,8 +914,7 @@ bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
914             // requesting or processing some txs which have already been included in a block
915             return recentRejects->contains(inv.hash) ||
916                    mempool.exists(inv.hash) ||
917-                   mapOrphanTransactions.count(inv.hash) ||
918-                   pcoinsTip->HaveCoinsInCache(inv.hash);
919+                   mapOrphanTransactions.count(inv.hash);

ryanofsky commented at 8:42 pm on May 3, 2017:

In commit “Switch CCoinsView and chainstate db from per-txid to per-txout”

Comment above on line 913 needs to be updated.

Also maybe worth noting this change of behavior in the commit message.

sipa commented at 11:42 pm on May 9, 2017:

Fixed.

TheBlueMatt commented at 9:30 pm on May 12, 2017:

Can we not just leave it and use HaveCoinsInCache(COutPoint(hash, 0))? Something in the latest block is most likely to still have all its outputs available. Do we have any stats on how often this check gets used (especially for nodes which get blocks early compared to random p2p nodes?)

in src/rpc/blockchain.cpp:802 in 19eb485cc4 outdated

781+    for (const auto output : outputs) {
782+        ss << VARINT(output.first + 1);
783+        ss << *(const CScriptBase*)(&output.second.out.scriptPubKey);
784+        ss << VARINT(output.second.out.nValue);
785+        stats.nTransactionOutputs++;
786+        stats.nTotalAmount += output.second.out.nValue;

ryanofsky commented at 8:50 pm on May 3, 2017:

In commit “Refactor GetUTXOStats in preparation for per-COutPoint iteration”:

This stats.ntotalamount value seems to get overwritten in line 815. Probably that line should be removed.

in src/rpc/rawtransaction.cpp:224 in 25b95d6b17 outdated

218@@ -219,9 +219,13 @@ UniValue gettxoutproof(const JSONRPCRequest& request)
219             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
220         pblockindex = mapBlockIndex[hashBlock];
221     } else {
222-        CCoins coins;
223-        if (pcoinsTip->GetCoins(oneTxid, coins) && coins.nHeight > 0 && coins.nHeight <= chainActive.Height())
224-            pblockindex = chainActive[coins.nHeight];
225+        CCoin coins;
226+        size_t o = 0;
227+        while (pblockindex == nullptr && o < 1024) {

ryanofsky commented at 8:57 pm on May 3, 2017:

In commit “Switch CCoinsView and chainstate db from per-txid to per-txout”

Oh no… not another one of these 1024-output-while-loop-that-should-be-a-for-loop things!

I’d suggest adding a GetTxHeight(CCoinsView&, const& uint256) or similar helper function implementing the lookup 1024 times logic in a single place that could potentially be improved later.

sipa commented at 11:42 pm on May 9, 2017:

Fixed by introducing a helper function AccessByTxid.

in src/txmempool.cpp:891 in 25b95d6b17 outdated

887@@ -889,19 +888,23 @@ bool CTxMemPool::HasNoInputsOf(const CTransaction &tx) const
888 
889 CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { }
890 
891-bool CCoinsViewMemPool::GetCoins(const uint256 &txid, CCoins &coins) const {
892+bool CCoinsViewMemPool::GetCoins(const COutPoint &txid, CCoin &coins) const {

ryanofsky commented at 9:03 pm on May 3, 2017:

In commit “Switch CCoinsView and chainstate db from per-txid to per-txout”

Maybe rename txid to outpoint in various places.

ryanofsky commented at 9:30 pm on May 3, 2017: member

utACK 493f03f02ab71376374bb851fe82241e5ee0a15f. This is a really nice change, and seems complete and ready to go as far as I could tell. I left a lot of comments and possible suggestions in my review, but they’re all very minor, so feel free ignore anything not of interest.

sipa force-pushed on May 4, 2017

sipa commented at 1:51 am on May 4, 2017: member

Updated and addressed many of @ryanofsky’s review comments. I’ll respond only individually to the ones I’m not addressing.

sipa force-pushed on May 4, 2017

sipa commented at 5:26 pm on May 5, 2017: member

Here is a graph of the speed and memory usage of a -reindex-chainstate until the default assumevalid point, with infinity -dbcache, on a 2.2GHz dual-cpu 28-core 56-threads machine with 256GiB RAM. Note that the x axis is progress (so proportional to the number of transactions).

reindex

in src/coins.h:33 in aaf39b924b outdated

80+class CCoin
81 {
82 public:
83-    //! whether transaction is a coinbase
84-    bool fCoinBase;
85+    //! unspent transaction outputs; spent outputs are .IsNull()

jnewbery commented at 8:15 pm on May 5, 2017:

I think this comment is now wrong. Should just say unspent transaction output

sipa commented at 11:41 pm on May 9, 2017:

Fixed.

in src/coins.h:36 in aaf39b924b outdated

85+    //! unspent transaction outputs; spent outputs are .IsNull()
86+    CTxOut out;
87 
88-    //! unspent transaction outputs; spent outputs are .IsNull(); spent outputs at the end of the array are dropped
89-    std::vector<CTxOut> vout;
90+    //! whether transaction is a coinbase

jnewbery commented at 8:16 pm on May 5, 2017:

whether containing transaction was a coinbase

sipa commented at 11:41 pm on May 9, 2017:

Fixed.

in src/coins.h:39 in aaf39b924b outdated

88-    //! unspent transaction outputs; spent outputs are .IsNull(); spent outputs at the end of the array are dropped
89-    std::vector<CTxOut> vout;
90+    //! whether transaction is a coinbase
91+    unsigned int fCoinBase : 1;
92 
93     //! at which height this transaction was included in the active block chain

jnewbery commented at 8:16 pm on May 5, 2017:

at which height the containing transaction was included in the active block chain

sipa commented at 11:41 pm on May 9, 2017:

Fixed.

in src/coins.h:42 in aaf39b924b outdated

108 
109-    //! construct a CCoins from a CTransaction, at a given height
110-    CCoins(const CTransaction &tx, int nHeightIn) {
111-        FromTx(tx, nHeightIn);
112-    }
113+    //! construct a CCoin from a CTransaction, at a given height

jnewbery commented at 8:16 pm on May 5, 2017:

construct a CCoin from a CTxOut, at a given height

sipa commented at 11:41 pm on May 9, 2017:

Fixed.

in src/undo.h:47 in aaf39b924b outdated

63+            ::Unserialize(s, VARINT(nVersionDummy));
64+        }
65+        ::Unserialize(s, REF(CTxOutCompressor(REF(txout->out))));
66     }
67+
68+    CTxInUndoSerializer(const CCoin* coins) : txout(const_cast<CCoin*>(coins)) {}

jnewbery commented at 8:39 pm on May 5, 2017:

misleading name for this variable. Can you name it coin or txout?

sipa commented at 11:41 pm on May 9, 2017:

Fixed.

in src/coins.h:27 in aaf39b924b outdated

71- *  - vout[16]: bbd123008c988f1a4a4de2161e0f50aac7f17e7f9555caa4
72- *              * bbd123: compact amount representation for 110397 (0.001 BTC)
73- *              * 00: special txout type pay-to-pubkey-hash
74- *              * 8c988f1a4a4de2161e0f50aac7f17e7f9555caa4: address uint160
75- *  - height = 120891
76+ * - VARINT(coinbase + height * 2)

jnewbery commented at 9:11 pm on May 5, 2017:

I found this quite confusing until I worked out you were packing bits into a single VARINT (partly because I didn’t understand that coinbase was supposed to be 1 or 0). I don’t know if this would be clearer as: VARINT(height|coinbase) ie height bits concatenated with coinbase bit.

sipa commented at 11:41 pm on May 9, 2017:

Rewritten in a different way.

in src/coins.h:188 in aaf39b924b outdated

188+    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) override;
189+    CCoinsViewCursor *Cursor() const override;
190 };
191 
192 
193 class CCoinsViewCache;

jnewbery commented at 9:39 pm on May 5, 2017:

now that you’ve removed CCoinsModifier, I believe you can remove this forward declaration of CCoinsViewCache.

sipa commented at 11:41 pm on May 9, 2017:

Fixed.

jnewbery commented at 10:01 pm on May 5, 2017: member

I’ve reviewed up to 4f09ae53d8cc3b570b04ea72b8f9844f93543823 “Introduce new per-txout CCoinsViewCache functions” and have a few nits. I’ll finish reviewing next week.

sipa commented at 1:58 am on May 6, 2017: member

Same machine, but flushing at 1300 MiB: reindex-1300

in src/coins.cpp:158 in 4f09ae53d8 outdated

152@@ -152,6 +153,55 @@ CCoinsModifier CCoinsViewCache::ModifyNewCoins(const uint256 &txid, bool coinbas
153     return CCoinsModifier(*this, ret.first, 0);
154 }
155 
156+void CCoinsViewCache::AddCoin(const COutPoint &outpoint, CCoin&& coin, bool possible_overwrite) {
157+    assert(!coin.IsPruned());
158+    auto ret = cacheCoins.emplace(std::piecewise_construct, std::forward_as_tuple(outpoint.hash), std::tuple<>());

theuni commented at 8:33 pm on May 9, 2017:

std::tie would make this a little easier to read:

0CCoinsMap::iterator it;
1bool inserted;
2std::tie(it, inserted) = cacheCoins.emplace(...

sipa commented at 11:42 pm on May 9, 2017:

Fixed.

sipa force-pushed on May 9, 2017

in src/validation.cpp:1507 in ac6910b5f6 outdated

1504  * @param out The out point that corresponds to the tx input.
1505- * @return True on success.
1506+ * @return True when the UTXO state was consistent with the undo data.
1507  */
1508-bool ApplyTxInUndo(const CTxInUndo& undo, CCoinsViewCache& view, const COutPoint& out)
1509+DisconnectResult ApplyTxInUndo(const CTxInUndo& undo, CCoinsViewCache& view, const COutPoint& out)

TheBlueMatt commented at 3:53 pm on May 10, 2017:

You need to update the second declaration in src/test/coins_tests.cpp as well, I believe.

sipa commented at 11:19 pm on May 12, 2017:

Ugh, that means exposing DisconnectResult :(

sipa commented at 0:41 am on May 13, 2017:

Fixed by changing the type to int :(

sipa force-pushed on May 10, 2017

sipa commented at 7:44 pm on May 10, 2017: member

I’ve pushed a few more changes (rewriting the commits, but not a rebase), renaming some of the classes and variables to be more consistent.

For any further changes I’ll create fixup commits in order to not hurt review.

ryanofsky commented at 8:32 pm on May 10, 2017: member

utACK 37b273524984eb034f68277a635fa7265ece638e. Thanks for implementing so many review suggestions. Removing coins->Clear() from ApplyTxInUndo in an earlier commit, renaming CCoin to Coin, changing stats.nSerializedSize were the only significant changes since my last review that I don’t remember seeing suggested, and these all look good.

in src/coins.cpp:241 in 37b2735249 outdated

254-        cache.cacheCoins.erase(it);
255-    } else {
256-        // If the coin still exists after the modification, add the new usage
257-        cache.cachedCoinsUsage += it->second.coins.DynamicMemoryUsage();
258+    COutPoint iter(txid, 0);
259+    while (iter.n * ::GetSerializeSize(CTxOut(), SER_NETWORK, PROTOCOL_VERSION) < MAX_BLOCK_BASE_SIZE) {

TheBlueMatt commented at 11:07 pm on May 10, 2017:

Hmm, can the GetSerializeSize() be moved out of the loop?

sipa commented at 0:42 am on May 13, 2017:

Done.

in src/bench/ccoins_caching.cpp:38 in 37b2735249 outdated

34@@ -35,14 +35,14 @@ SetupDummyInputs(CBasicKeyStore& keystoreRet, CCoinsViewCache& coinsRet)
35     dummyTransactions[0].vout[0].scriptPubKey << ToByteVector(key[0].GetPubKey()) << OP_CHECKSIG;
36     dummyTransactions[0].vout[1].nValue = 50 * CENT;
37     dummyTransactions[0].vout[1].scriptPubKey << ToByteVector(key[1].GetPubKey()) << OP_CHECKSIG;
38-    coinsRet.ModifyCoins(dummyTransactions[0].GetHash())->FromTx(dummyTransactions[0], 0);
39+    AddCoins(coinsRet, dummyTransactions[0], 0);

TheBlueMatt commented at 11:17 pm on May 10, 2017:

Just a note for reviewers/benchmarkers - this is a behavior change as AddCoins is equivalent to ModifyNewCoins, not ModifyCoins.

sipa commented at 0:43 am on May 13, 2017:

Right, there is no equivalent of ModifyCoins anymore.

in src/rpc/rawtransaction.cpp:844 in 37b2735249 outdated

837@@ -839,9 +838,12 @@ UniValue sendrawtransaction(const JSONRPCRequest& request)
838         nMaxRawTxFee = 0;
839 
840     CCoinsViewCache &view = *pcoinsTip;
841-    const CCoins* existingCoins = view.AccessCoins(hashTx);
842+    bool fHaveChain = false;
843+    for (size_t o = 0; !fHaveChain && o < tx->vout.size(); o++) {
844+        const Coin& existingCoin = view.AccessCoin(COutPoint(hashTx, o));
845+        fHaveChain = !existingCoin.IsPruned() && existingCoin.nHeight < 1000000000;

TheBlueMatt commented at 11:30 pm on May 10, 2017:

Can we drop the nHeight check now? It looks like it used to be a shitty version of HaveCoins(), but we now do an availability check before returning and otherwise return null anyway, so it seems useless.

sipa commented at 0:43 am on May 13, 2017:

Done.

in src/validation.h:453 in 546681621b outdated

449@@ -450,8 +450,8 @@ class CScriptCheck
450 
451 public:
452     CScriptCheck(): amount(0), ptxTo(0), nIn(0), nFlags(0), cacheStore(false), error(SCRIPT_ERR_UNKNOWN_ERROR) {}
453-    CScriptCheck(const CCoins& txFromIn, const CTransaction& txToIn, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn) :
454-        scriptPubKey(txFromIn.vout[txToIn.vin[nInIn].prevout.n].scriptPubKey), amount(txFromIn.vout[txToIn.vin[nInIn].prevout.n].nValue),
455+    CScriptCheck(const Coin& txFromIn, const CTransaction& txToIn, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn) :

TheBlueMatt commented at 11:55 pm on May 10, 2017:

Shameless ask: Can you swap “Switch CScriptCheck to use CCoin instead of CCoins” out for https://github.com/bitcoin/bitcoin/pull/10192/commits/a7ae7f9560720fe4245cdaa4eabc528d8b216f4d which should be nearly as good here but wont make things needlessly conflict?

sipa commented at 0:13 am on May 13, 2017:

Sounds good. Will do when squashing.

in src/coins.cpp:90 in 777f4c6b63 outdated

181+void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight) {
182+    bool fCoinbase = tx.IsCoinBase();
183+    const uint256& txid = tx.GetHash();
184+    for (size_t i = 0; i < tx.vout.size(); ++i) {
185+        if (!tx.vout[i].scriptPubKey.IsUnspendable()) {
186+            cache.AddCoin(COutPoint(txid, i), Coin(tx.vout[i], nHeight, fCoinbase), fCoinbase);

sdaftuar commented at 5:13 pm on May 12, 2017:

nit: Add comment somewhere referencing BIP30 and the historical coinbase transactions which overwrote existing entries, as explanation for the last argument here.

sipa commented at 11:14 pm on May 12, 2017:

fixed

in src/txmempool.h:601 in 21791e4fbc outdated

600@@ -587,7 +601,7 @@ class CTxMemPool
601       *  pvNoSpendsRemaining, if set, will be populated with the list of transactions

sdaftuar commented at 6:23 pm on May 12, 2017:

nit: should be “list of transaction outputs”

sipa commented at 11:14 pm on May 12, 2017:

fixed

in src/rpc/blockchain.cpp:781 in 21791e4fbc outdated

777@@ -778,12 +778,15 @@ static void ApplyStats(CCoinsStats &stats, CHashWriter& ss, const uint256& hash,
778     assert(!outputs.empty());
779     ss << hash;
780     ss << VARINT(outputs.begin()->second.nHeight * 2 + outputs.begin()->second.fCoinBase);
781+    stats.nSerializedSize += 30 + 4 * outputs.size(); // Account for 1/16 chance of txid duplication in LevelDB.

sdaftuar commented at 7:43 pm on May 12, 2017:

Could you explain this formula a bit more? I would have thought nSerializedSize would refer to the size of the data that we hashed, and hence it’d just be exactly the size of what we’re passing to CHashWriter, but instead it seems we’re trying to estimate the on-disk size?

I’m also not familiar with the leveldb internals, any chance you have a reference for understanding this duplication?

sipa commented at 11:16 pm on May 12, 2017:

I tried to fix this by writing a big comment about the data format and the rationale for the formula, only to discover it was wrong. Then I went on to fix it, and take even more cases into account, but the result still was pretty far off from the actual disk size. Instead, I removed the field entirely and replaced it with a disk_size element that is computed by LevelDB directly (but is not guaranteed to be deterministic).

in src/rpc/rawtransaction.cpp:223 in 21791e4fbc outdated

218@@ -219,9 +219,8 @@ UniValue gettxoutproof(const JSONRPCRequest& request)
219             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
220         pblockindex = mapBlockIndex[hashBlock];
221     } else {
222-        CCoins coins;
223-        if (pcoinsTip->GetCoins(oneTxid, coins) && coins.nHeight > 0 && coins.nHeight <= chainActive.Height())
224-            pblockindex = chainActive[coins.nHeight];
225+        const Coin& coin = AccessByTxid(*pcoinsTip, oneTxid);
226+        if (!coin.IsPruned()) pblockindex = chainActive[coin.nHeight];

sdaftuar commented at 8:05 pm on May 12, 2017:

It looks like the old code had sanity checking on the height, to prevent out-of-bounds access into chainActive. I assume we should put that protection back in?

sipa commented at 11:16 pm on May 12, 2017:

Fixed.

in src/txmempool.cpp:1054 in 21791e4fbc outdated

1050@@ -1048,7 +1051,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector<uint256>* pvNoSpendsRe
1051                         continue;
1052                     auto iter = mapNextTx.lower_bound(COutPoint(txin.prevout.hash, 0));
1053                     if (iter == mapNextTx.end() || iter->first->hash != txin.prevout.hash)
1054-                        pvNoSpendsRemaining->push_back(txin.prevout.hash);
1055+                        pvNoSpendsRemaining->push_back(txin.prevout);

sdaftuar commented at 8:22 pm on May 12, 2017:

Shouldn’t this be including all outpoints that are no longer spent, rather than just outpoints from transactions where none of the outpoints are spent?

sipa commented at 11:16 pm on May 12, 2017:

Nice catch, fixed.

sdaftuar commented at 7:53 pm on May 15, 2017:

The if (!mapNextTx.count(txin.prevout)) guard isn’t necessary here, right?

sdaftuar commented at 8:36 pm on May 12, 2017: member

I’ve spent a bunch of time code reviewing today; got up to 21791e4fbc0bf385283ed82ff736961d45b0d6d6.

Posting my current set of comments now; will pick review up again on Monday.

in src/coins.h:228 in 21791e4fbc outdated

451      * more efficient than GetCoins. Modifications to other cache entries are
452      * allowed while accessing the returned pointer.
453-     * TODO: return a reference to a Coin after changing CCoinsViewCache storage.
454      */
455-    const Coin AccessCoin(const COutPoint &output) const;
456+    const Coin& AccessCoin(const COutPoint &output) const;

TheBlueMatt commented at 9:17 pm on May 12, 2017:

I’m super not a big fan of removing the old hasModifyer sanity checks here. Returning a reference to an object in our map without enforcing some semantics on the caller seems like a footgun. I’m curious if you have any numbers on performance degredation of not doing the Modfiyer approach? Also, the comment is somewhat ambiguous - you’re allowed to modify (and even delete) other elements, but you are not allowed to insert new elements while holding the returned reference from AccessCoins (also, you cannot modify the returned value if it IsPruned()).

sipa commented at 11:22 pm on May 12, 2017:

As explained on IRC, references to std::unordered_map entries always remain valid, even under concurrent modification/insertion (including when a rehash is triggered), except when the entry itself is deleted. Iterators do get invalidated by rehashing, but CCoinsViewCache does not allow iterators to escape anymore (they used to be, inside CCoinsModifier).

in src/init.cpp:149 in 37b2735249 outdated

145@@ -146,9 +146,9 @@ class CCoinsViewErrorCatcher : public CCoinsViewBacked
146 {
147 public:
148     CCoinsViewErrorCatcher(CCoinsView* view) : CCoinsViewBacked(view) {}
149-    bool GetCoins(const uint256 &txid, CCoins &coins) const {
150+    bool GetCoins(const COutPoint &outpoint, Coin &coin) const {

TheBlueMatt commented at 9:24 pm on May 12, 2017:

That reminds me, should we add a similar wrapper here for HaveCoins?

TheBlueMatt commented at 9:27 pm on May 12, 2017:

Also, nit: can you override here?

sipa commented at 0:43 am on May 13, 2017:

Added a bunch of overrides.

in src/rpc/blockchain.cpp:781 in 37b2735249 outdated

772@@ -773,6 +773,25 @@ struct CCoinsStats
773     CCoinsStats() : nHeight(0), nTransactions(0), nTransactionOutputs(0), nSerializedSize(0), nTotalAmount(0) {}
774 };
775 
776+static void ApplyStats(CCoinsStats &stats, CHashWriter& ss, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
777+{
778+    assert(!outputs.empty());
779+    ss << hash;
780+    ss << VARINT(outputs.begin()->second.nHeight * 2 + outputs.begin()->second.fCoinBase);
781+    stats.nSerializedSize += 30 + 4 * outputs.size(); // Account for 1/16 chance of txid duplication in LevelDB.

TheBlueMatt commented at 9:48 pm on May 12, 2017:

Can you elaborate a bit on the comment here? Also, shouldnt this consider that the output indecies are VARINTs?

sipa commented at 11:24 pm on May 12, 2017:

Deleted this whole thing (see my response to @sdaftuar’s comment on the same line).

in src/txdb.h:76 in 37b2735249 outdated

72@@ -75,11 +73,13 @@ class CCoinsViewDB : public CCoinsView
73 public:
74     CCoinsViewDB(size_t nCacheSize, bool fMemory = false, bool fWipe = false);
75 
76-    bool GetCoins(const uint256 &txid, CCoins &coins) const;
77-    bool HaveCoins(const uint256 &txid) const;
78+    bool GetCoins(const COutPoint &outpoint, Coin &coin) const;

TheBlueMatt commented at 10:06 pm on May 12, 2017:

nit: while you’re changing these lines, it’d be nice to add overrides where appropriate.

sipa commented at 0:44 am on May 13, 2017:

Done.

TheBlueMatt commented at 10:36 pm on May 12, 2017: member

Got through a big chunk, but still a bunch to review.

sipa force-pushed on May 13, 2017

sipa commented at 10:18 pm on May 13, 2017: member

Squashed history with fixups. Original history:

83367a95f922f6a5aec8b755e55c748e2edcbbc6 Add SizeEstimate to CDBBatch
ac6910b5f6fd7fcfc2b002afe4a69f9ebca499fe error() in disconnect for disk corruption, not inconsistency
f7beab2b69b4e239e6c6d92062dd7a5533f7fd9c Introduce CHashVerifier to hash read data
e3d9ac37e1f7b74df263f9fbff2a482169284afe Add specialization of SipHash for 256 + 32 bit data
5be8c484780f1e66ee6d2e089d40a4d702e8a2f1 Remove/ignore tx version in utxo and undo
ec8c2615d00cefbdec4f9af6c68545f4a0814fe2 Store/allow tx metadata in all undo records
ac70d1fc96737eb49d6ca6ba8adf8e24d30a7f7d Introduce Coin, a single unspent output
50a8df05a1849cf7fe85e14226725a3e91390b0b Replace CTxInUndo with Coin
c3ce6c92f7502d075118ed013f482fbd55ec102a Optimization: Coin&& to ApplyTxInUndo
777f4c6b63bbb4ce8e3afe51c02696893f4b165a Introduce new per-txout CCoinsViewCache functions
8df47a3ae67f1ce270b7612a61c5a136bf75297e Switch from per-tx to per-txout CCoinsViewCache methods in some places
546681621bbe8086fd38b568d0fb95cab224087e Switch CScriptCheck to use Coin instead of CCoins
ce86045f34eb87cf32d50cd2e73a267cf2073d73 Switch tests from ModifyCoins to AddCoin/SpendCoin
b5144262ebdacee7fe0cb77bf19186a7a1bde497 Remove ModifyCoins/ModifyNewCoins
593a6324fbaf6bf1188c15ad23d670213e9cb41c Replace CCoins-based CTxMemPool::pruneSpent with isSpent
deb42bb6c84e93d3f5e53c9007d58a7abdc45001 Refactor GetUTXOStats in preparation for per-COutPoint iteration
21791e4fbc0bf385283ed82ff736961d45b0d6d6 Switch CCoinsView and chainstate db from per-txid to per-txout
6fd519e2d1fad48fbe5bf5b335c6fb0606d906e5 Remove unused CCoins methods
575ffb39d6563c73b057690ab3171aa96097c27e Pack Coin more tightly
f2cd835a18d3f784fe45fa0421120d8fb96a6dce Reduce reserved memory space for flushing
b951db2885c97c3bc71b58aa00a3a897b75a8340 Upgrade from per-tx database to per-txout
229b89c36f104a7f26a2d40b604e631cc1c56814 [MOVEONLY] Move old CCoins class to txdb.cpp
fdab87c96cbd4e92aec40fc10ba83dd2ef2ddbe8 Merge CCoinsViewCache’s GetOutputFor and AccessCoin
37b273524984eb034f68277a635fa7265ece638e Rename CCoinsCacheEntry::coins to coin
e581f28c0f7c7bba36f28b1c2a69c7a584e54edb f Replace CTxInUndo with Coin: initialize count variale
69f14c4c7be6fd233a420953255a21119e11096f f Introduce new per-txout CCoinsViewCache functions: add comment explaining fCoinbase
71ccd9d1bd50a523087ef74140af2032907f7656 f Switch CCoinsView and chainstate db from per-txid to per-txout: fix txmempool comment
fdfafc54503aca8645c9bd8816f590833debf363 f Remove/ignore tx version in utxo and undo: remove ill-defined bytes_serialized
9da167ec95b3bbf44ca545adcc0855d8d0c25642 Report on-disk size in gettxoutsetinfo
3e04e32a10df48965981e86c559cf4b8691e7699 f Switch CCoinsView and chainstate db from per-txid to per-txout: re-add sanity check in gettxoutproof
c8985414f376e73ae301d282b6e38e8accf493f9 f Switch CCoinsView and chainstate db from per-txid to per-txout: CTxMempool::TrimToSize should not use per-txid logic for pvNoRemainingSpends
b69f3bc3400e6693aa04b00c054f5018b6ffa954 f Replace CTxInUndo with Coin: split TxInUndoSerializer off from deserializer
544c34b202725c932cb9edbd8be655bc7764fa85 f Replace CTxInUndo with Coin: don’t hardcode max outputs per block
bf4dadbd98d07a07b87fea577ef9d38328d8607d f error() in disconnect for disk corruption, not inconsistency: correct type in extern ApplyTxInUndo in coins_tests.cpp
6ac7ea345d18d13bac4bc00e513eb3065a36c713 f Optimization: Coin&& to ApplyTxInUndo: Comment that all of txundo.prevout was moved
369afe4208ede1741cbde553364a1fcca7d44781 f Introduce new per-txout CCoinsViewCache functions: do not compute CTxOut() size in loop
dc2bb20a396317e47e17bb0533ca0e42d3f81fb1 f Introduce new per-txout CCoinsViewCache functions: Add comment explaining AddCoins potential_overwrite assumptions
5611b76c963d02bd267127122222db85a5e3761a f Switch from per-tx to per-txout CCoinsViewCache methods in some places: remove redundant nHeight < 1000000000 logic
51c7c4235f12de8271ff2ed0447571b6a3e71a23 f Switch CCoinsView and chainstate db from per-txid to per-txout: add overrides
b5621d6dbd41dfde331afd45ab6e0c4f8c8c2b82 f Switch CCoinsView and chainstate db from per-txid to per-txout: still try COutPoint(hash, 0) in AlreadyHave
ea2a5b937a625f738c97314de48e117be9e76abb Only pass things committed to by tx’s witness hash to CScriptCheck

in src/rpc/blockchain.cpp:907 in a86f1c959c outdated

890@@ -878,8 +891,8 @@ UniValue gettxoutsetinfo(const JSONRPCRequest& request)
891             "  \"bestblock\": \"hex\",   (string) the best block hash hex\n"
892             "  \"transactions\": n,      (numeric) The number of transactions\n"
893             "  \"txouts\": n,            (numeric) The number of output transactions\n"
894-            "  \"bytes_serialized\": n,  (numeric) The serialized size\n"
895             "  \"hash_serialized\": \"hash\",   (string) The serialized hash\n"

laanwj commented at 5:36 am on May 15, 2017:

doc: hash_serialized_2

jnewbery commented at 1:24 pm on May 30, 2017:

the name should be updated to hash_serialized_2. The description should also be updated to say that hash_serialized_2 will not be compatible with hash_serialized from clients <0.15.0.

laanwj commented at 12:52 pm on May 15, 2017: member

I’ve been testing this on a slow system: on an ARM (i.MX6), USB2 external harddisk, the database upgrade took about 50 minutes:

02017-05-15 10:27:45 Upgrading database...
12017-05-15 11:20:25 LoadBlockIndexDB: last block file = 869

Not necessarily in this pull, but some kind of progress feedback like for rescans / reindexes would be indispensable. Users might assume that the client has crashed after no log messages for such a long time.

sdaftuar commented at 8:03 pm on May 15, 2017: member

Code review ACK apart from the tests, which I’m still reviewing/testing.

The database upgrade took about 5 minutes on my machine (chainstate stored on SSD).

sipa commented at 10:19 pm on May 15, 2017: member

Not necessarily in this pull, but some kind of progress feedback like for rescans / reindexes would be indispensable. Users might assume that the client has crashed after no log messages for such a long time.

Yes, fully agree. There would need to be some UI interaction in the GUI, and at least some progress reported in debug.log during the process. I’d prefer to do that after merging, though.

We also need some calls to leveldb’s CompactRange during the conversion, as the old entries don’t seem to be cleaned up quickly automatically.

in test/functional/blockchain.py:54 in 1b4202cca1 outdated

48@@ -49,6 +49,9 @@ def _test_gettxoutsetinfo(self):
49         assert_equal(res['transactions'], 200)
50         assert_equal(res['height'], 200)
51         assert_equal(res['txouts'], 200)
52+        size = res['disk_size']
53+        assert size > 6400

ryanofsky commented at 2:11 pm on May 17, 2017:

In commit “Report on-disk size in gettxoutsetinfo”

Could assert 6400 < size < 64000

ryanofsky commented at 2:54 pm on May 17, 2017: member

utACK a86f1c959c4beaf2b2667b000983f22de6acadf2

(Changes since last review: added fixes 37b273524984eb034f68277a635fa7265ece638e..ea2a5b937a625f738c97314de48e117be9e76abb, squashed ea2a5b937a625f738c97314de48e117be9e76abb -> 02aa1bec3e4e2b15caa3bd233c72ce1449133a2f, and merged new master 94e52273f30fc9f3f1a7b58778ed21781bb2a744.)

in src/coins.cpp:223 in a86f1c959c outdated

227         return 0;
228 
229     CAmount nResult = 0;
230     for (unsigned int i = 0; i < tx.vin.size(); i++)
231-        nResult += GetOutputFor(tx.vin[i]).nValue;
232+        nResult += AccessCoin(tx.vin[i].prevout).out.nValue;

sdaftuar commented at 7:40 pm on May 17, 2017:

I was wondering what happens in this function if an input doesn’t exist – in the old code, it appears we’d assert() in GetOutputFor(), whereas in the new code we’ll just silently return an incorrect value (because nValue is -1 in that case). Do you think it’s worth asserting here that AccessCoin is returning something valid?

sipa commented at 8:12 pm on May 17, 2017:

I can add an assert if you’re concerned, but given that this code hasn’t asserted probably ever, it should be fine. I really want to get rid of this method, and instead make the validation code build a vector of CCoin& references for all its inputs, and use that everywhere, as opposed to looking things up several times inline.

sdaftuar commented at 9:39 pm on May 18, 2017:

Your plan sounds good!

sdaftuar commented at 10:26 pm on May 18, 2017: member

ACK a86f1c959c4beaf2b2667b000983f22de6acadf2, this is awesome.

Though it looks like this needs rebase now. (Also @sipa I’m confused about why you included the merge commit in this PR?)

I did some benchmarking of validation speed in my simulation environment, comparing master to this PR. This makes better use of pcoinsTip, which means we flush less often and thus expect a big performance win from that effect alone (which I did observe). Moreover, even if I compare master against this PR using a -dbcache big enough that neither one flushes to disk during the simulation period, this PR still performs substantially better – 33% improvement in the “Connect transactions” time in ConnectBlock, and roughly 8% reduction overall in “Connect block” time.

sipa force-pushed on May 18, 2017

sipa commented at 11:15 pm on May 18, 2017: member

@sdaftuar Thanks, rebased. The merge commit at the end was to avoid a CI error that was fixed in master, but existed at the time the code branched off.

sdaftuar commented at 4:57 pm on May 19, 2017: member

re-ACK 71837d800cdacd92ddb5bf8536c0e16338a6a889

in src/coins.cpp:82 in 71837d800c outdated

143+        fresh = !(it->second.flags & CCoinsCacheEntry::DIRTY);
144+    }
145+    it->second.coin = std::move(coin);
146+    it->second.flags |= CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0);
147+    cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
148+}

gmaxwell commented at 10:07 pm on May 19, 2017:

Having trouble with the logic around !inserted here. If an entry already existed, we remove its size. (potentially making cachedCoinsUsage underflow). Then if we were not willing to overwrite, we throw leaving the accounting in an odd state.

Why is the logic here not just having an if(inserted) on the last line and elimiating the !inserted block?

sipa commented at 10:19 pm on May 19, 2017:

If an entry already existed, we remove its size. (potentially making cachedCoinsUsage underflow).

Underflow is not possible if the cachedCoinsUsage value was consistent with the database.

Then if we were not willing to overwrite, we throw leaving the accounting in an odd state.

That throw should be regarded as an assertion failure, except for the fact that tests can catch it.

Why is the logic here not just having an if(inserted) on the last line and elimiating the !inserted block?

It’s possible the entry was overwritten with a different value, needing correction.

in src/coins.h:287 in 71837d800c outdated

287 };
288 
289+//! Utility function to add all of a transaction's outputs to a cache.
290+// It assumes that overwrites are only possible for coinbase transactions,
291+// TODO: pass in a boolean to limit these possible overwrites to pre-BIP34
292+// coinbases.

gmaxwell commented at 10:16 pm on May 19, 2017:

preferably it should limit it to just the two overwrites that re actually needed.

in src/net_processing.cpp:917 in 71837d800c outdated

915-            // requesting or processing some txs which have already been included in a block
916             return recentRejects->contains(inv.hash) ||
917                    mempool.exists(inv.hash) ||
918                    mapOrphanTransactions.count(inv.hash) ||
919-                   pcoinsTip->HaveCoinsInCache(inv.hash);
920+                   pcoinsTip->HaveCoinsInCache(COutPoint(inv.hash, 0)); // Best effort: only try output 0

gmaxwell commented at 10:21 pm on May 19, 2017:

It may be advisable to also check 1. The reason is that recently confirmed transactions will get requested again, and we’re currently protected by this check. But if the transaction chained off output 0 it won’t be in the cache anymore. Also testing 1 would be a lot less often that we would erroneously double request a transaction we just confirmed.

in src/txdb.cpp:92 in 71837d800c outdated

87@@ -63,8 +88,14 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) {
88     if (!hashBlock.IsNull())
89         batch.Write(DB_BEST_BLOCK, hashBlock);
90 
91-    LogPrint(BCLog::COINDB, "Committing %u changed transactions (out of %u) to coin database...\n", (unsigned int)changed, (unsigned int)count);
92-    return db.WriteBatch(batch);
93+    bool ret = db.WriteBatch(batch);
94+    LogPrint(BCLog::COINDB, "Committed %u changed transactions (out of %u) to coin database...\n", (unsigned int)changed, (unsigned int)count);

gmaxwell commented at 10:28 pm on May 19, 2017:

changed txouts?

gmaxwell commented at 0:38 am on May 20, 2017: contributor

I tested the tests by manually injecting faults (e.g. stepping through the code line by line and inserting a bug at each opportunity then rerunning the tests to make sure they fail) in coins.cpp, here are my findings:

CCoinsViewCache::FetchCoins commenting out cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage(); still passes coins_tests.

CCoinsViewCache::AddCoin inverting condition in if (!inserted) passes coins_tests.

CCoinsViewCache::AddCoin commenting out cachedCoinsUsage += it-> passes coins_tests.

AddCoins Adding 1 || to the !tx.[…].IsUnspendtable() condition passes coins_tests.

CCoinsViewCache::SpendCoin commenting out cachedCoinsUsage -= it->seco passes coins_tests

CCoinsViewCache::HaveCoins removing && !it->second.coin.IsPruned() passes coins_tests

CCoinsViewCache::BatchWrite commenting out cachedCoinsUsage += entry.coin.DynamicMemoryUsag passes coins_tests (anywhere in the function)

CCoinsViewCache::BatchWrite commenting out hashBlock = hashBlockIn passes coins_tests

CCoinsViewCache::Flush commenting out cachedCoinsUsage = 0; passes coins_tests

CCoinsViewCache::Uncache commenting out cachedCoinsUsage += entry.coin.DynamicMemoryUsag passes coins_tests

CCoinsViewCache::Uncache adding return to top of function passes coins_test

CCoinsViewCache::GetCacheSize changing function to return 0 passes coins_test

CCoinsViewCache::GetValueIn changing function to return 0 passes coins_test

CCoinsViewCache::HaveInputs changing function to return true; passes coins test

AccessByTxid changing function to return coinEmpty; passes coins test

sipa commented at 7:35 pm on May 20, 2017: member

@gmaxwell I’ve added a commit that extends the coins_test to (hopefully) address all the cases you found, except for HaveInputs and GetValueIn (which I plan to remove in a follow-up PR).

CCoinsViewCache::BatchWrite commenting out hashBlock = hashBlockIn passes coins_tests

That will be caught outside of coins_tests already.

sipa force-pushed on May 20, 2017

gmaxwell commented at 10:31 pm on May 20, 2017: contributor

AddCoins Adding 1 || to the !tx.[…].IsUnspendable() condition passes coins_tests.

^ that isn’t fixed yet.

sipa force-pushed on May 20, 2017

sipa commented at 11:39 pm on May 20, 2017: member

@gmaxwell Fixed.

in src/undo.h:83 in dde1fcbdf3 outdated

104+    template <typename Stream>
105+    void Unserialize(Stream& s) {
106+        // TODO: avoid reimplementing vector deserializer
107+        uint64_t count = 0;
108+        ::Unserialize(s, COMPACTSIZE(count));
109+        if (count > MAX_OUTPUTS_PER_BLOCK) {

gmaxwell commented at 8:02 am on May 21, 2017:

Isn’t this the maximum here the number of INPUTS consumed by a block? (e.g. base size divided by 40?) I suppose the output number is higher so this at least won’t cause failures, but I think it’s wrong.

sipa commented at 5:44 pm on May 21, 2017:

Indeed! Fixed.

sipa force-pushed on May 21, 2017

gmaxwell commented at 9:37 am on May 22, 2017: contributor

sync testnet. restart.

invalidate block 5.

{ “height”: 4, “bestblock”: “000000008b5d0af9ffb1741e38b17b193bd12d7683401cecd2fd94f548b6e5dd”, “transactions”: 4, “txouts”: 4, “hash_serialized_2”: “9ea56b9590b2ce5582a0013e2bfd08222334247ec2bdbe4c71dabd7210040b71”, “disk_size”: 808430596, “total_amount”: 200.00000000 }

restart

{ “height”: 4, “bestblock”: “000000008b5d0af9ffb1741e38b17b193bd12d7683401cecd2fd94f548b6e5dd”, “transactions”: 4, “txouts”: 4, “hash_serialized_2”: “9ea56b9590b2ce5582a0013e2bfd08222334247ec2bdbe4c71dabd7210040b71”, “disk_size”: 1199640169, “total_amount”: 200.00000000 }

after another restart “disk_size”: 561,

leveldb be crazy, yo. I doubt think it’s wrong, but we should perhaps expect confused questions from users.

And gettxoutsetinfo crashes with an assert on slice validity in leveldb if called when the tip is height 0 (either having never synced any blocks, or after invalidating block 1) – pretty sure this is not desired. :)

#4 0x00005555558e0eb3 in leveldb::(anonymous namespace)::DBIter::key (this=) at leveldb/db/db_iter.cc:67 #5 0x00005555556dd0f0 in CDBIterator::GetKey<(anonymous namespace)::CoinsEntry> (this=, key=) at dbwrapper.h:148 #6 CCoinsViewDB::Cursor (this=) at txdb.cpp:133 #7 0x00005555556722da in GetUTXOStats (stats=…, view=0x555556aa6890) at rpc/blockchain.cpp:810 #8 gettxoutsetinfo (request=…) at rpc/blockchain.cpp:920 #9 0x00005555556b6fcb in CRPCTable::execute (this=, request=…) at rpc/server.cpp:491 #10 0x000055555577cfd1 in HTTPReq_JSONRPC (req=0x7fff88001630) at httprpc.cpp:193

gmaxwell commented at 11:05 pm on May 22, 2017: contributor

@sipa it isn’t a new bug introduced by this PR, but my ACK is waiting on a fix for the gettxoutsetinfo crash on an empty chainstate.

sdaftuar commented at 2:36 pm on May 23, 2017: member

@sipa it isn’t a new bug introduced by this PR, but my ACK is waiting on a fix for the gettxoutsetinfo crash on an empty chainstate.

This seems to fix the crash @gmaxwell discovered (which is easy to reproduce in regtest), though I’m not sure if this is exactly the right fix:

 0diff --git a/src/txdb.cpp b/src/txdb.cpp
 1index 01108beae..d8d754617 100644
 2--- a/src/txdb.cpp
 3+++ b/src/txdb.cpp
 4@@ -128,10 +128,12 @@ CCoinsViewCursor *CCoinsViewDB::Cursor() const
 5        only need read operations on it, use a const-cast to get around
 6        that restriction.  */
 7     i->pcursor->Seek(DB_COIN);
 8-    // Cache key of first record
 9-    CoinsEntry entry(&i->keyTmp.second);
10-    i->pcursor->GetKey(entry);
11-    i->keyTmp.first = entry.key;
12+    if (i->pcursor->Valid()) {
13+        // Cache key of first record
14+        CoinsEntry entry(&i->keyTmp.second);
15+        i->pcursor->GetKey(entry);
16+        i->keyTmp.first = entry.key;
17+    }
18     return i;
19 }

in src/test/coins_tests.cpp:160 in 8ec6ed0bc9 outdated

161+            if (insecure_rand() % 5 == 0 || coin.IsPruned()) {
162+                newcoin.out.scriptPubKey.assign(insecure_rand() & 0x3F, 0); // Random sizes so we can test memory usage accounting
163+                newcoin.out.nValue = insecure_rand();
164+                newcoin.nHeight = 1;
165+            }
166+            if (newcoin.IsPruned()) {

ryanofsky commented at 3:47 pm on May 23, 2017:

In commit “Extend coins_tests”

I think it would be clearer to combine this if/else block with the previous one, so the different cases perform steps in a uniform order, and there are fewer conditions that need to be checked. The following seems to do the same work and drops around 10 lines of code:

 0if (insecure_rand() % 5 == 0 || coin.IsPruned()) {
 1    newcoin.out.nValue = insecure_rand();
 2    newcoin.nHeight = 1;
 3    if (insecure_rand() % 16 == 0 && coin.IsPruned()) {
 4        newcoin.out.scriptPubKey.assign(1 + (insecure_rand() & 0x3F), OP_RETURN);
 5        BOOST_CHECK(newcoin.out.scriptPubKey.IsUnspendable());
 6        added_an_unspendable_entry = true;
 7    } else {
 8        newcoin.out.scriptPubKey.assign(insecure_rand() & 0x3F, 0); // Random sizes so we can test memory usage accounting
 9        (coin.IsPruned() ? added_an_entry : updated_an_entry) = true;
10        coin = newcoin;
11    }
12    stack.back()->AddCoin(COutPoint(txid, 0), std::move(newcoin), !coin.IsPruned() || insecure_rand() & 1);
13} else {
14    removed_an_entry = true;
15    coin.Clear();
16    stack.back()->SpendCoin(COutPoint(txid, 0));
17}

in src/test/coins_tests.cpp:151 in 8ec6ed0bc9 outdated

152-            BOOST_CHECK(coins == *entry);
153-            if (insecure_rand() % 5 == 0 || coins.IsPruned()) {
154-                if (coins.IsPruned()) {
155-                    added_an_entry = true;
156+            Coin& coin = result[COutPoint(txid, 0)];
157+            Coin newcoin;

ryanofsky commented at 3:48 pm on May 23, 2017:

“In commit “Extend coins_tests”

Maybe declare newcoin below closer to its first use.

ryanofsky commented at 4:00 pm on May 23, 2017: member

Obviously there are other issues still being worked out, but posting an updated utACK for 8ec6ed0bc964a3beb0ba6b8945b3219d6d8189b2. Changes since my previous review were a rebase that resolved conflicts with #8329, and 5 additional commits.

sipa force-pushed on May 23, 2017

sipa commented at 5:07 pm on May 23, 2017: member

Modified the last few commits to include @sdaftuar and @ryanofsky’s suggestions.

gmaxwell approved

gmaxwell commented at 0:17 am on May 24, 2017: contributor

ACK. Great work

dougstrong77 commented at 0:32 am on May 24, 2017: none

Need helpwith all this

sdaftuar commented at 3:40 pm on May 24, 2017: member

re-ACK 99eecb9

Add SizeEstimate to CDBBatch

This allows estimating the in-memory size of a LevelDB batch.

e66dbde6d1

error() in disconnect for disk corruption, not inconsistency

The error() function unconditionally reports an error. It should only
be used for actually exception situations, and not for the type of
inconsistencies that ApplyTxInUndo/DisconnectBlock can graciously deal
with.

This also makes a subtle semantics change: in ApplyTxInUndo, when a
record with metadata is encountered (indicating it is the last spend
from a tx), don't wipe the CCoins record if it wasn't empty at that
point. This makes sure that UTXO operations never affect any other
UTXOs (including those from the same tx).

f54580e7e4

Introduce CHashVerifier to hash read data

This is necessary later, when we drop the nVersion field from the undo
data. At that point deserializing and reserializing the data won't
roundtrip anymore, and thus that approach can't be used to verify
checksums anymore.

With this CHashVerifier approach, we can deserialize while hashing the
exact serialized form that was used. This is both more efficient and
more correct in that case.

e484652fc3

Add specialization of SipHash for 256 + 32 bit data

We'll need a version of SipHash for tuples of 256 bits and 32 bits
data, when CCoinsViewCache switches from using txids to COutPoints as
keys.

7e00322906

Remove/ignore tx version in utxo and undo

This makes the following changes:
* In undo data and the chainstate database, the transaction nVersion
  field is removed from the data structures, always written as 0, and
  ignored when reading.
* The definition of hash_serialized in gettxoutsetinfo is changed to no
  longer incude the nVersion field. It is renamed to hash_serialized_2
  to avoid confusion. The new definition also includes transaction
  height and coinbase information, as this information was missing
  before.

This depends on having a CHashVerifier-based undo data checksum
verifier.

Apart from changing the definition of serialized_hash, downgrading
after using this patch is supported, as no release ever used the value
of nVersion field in UTXO entries.

d342424301

Report on-disk size in gettxoutsetinfo c3aa0c1194

Store/allow tx metadata in all undo records

Previously, transaction metadata (height, coinbase or not, and before
the previous commit also nVersion) was only stored for undo records
that correspond to the last output of a transaction being spent.

This only saves 2 bytes per undo record. Change this to storing this
information for every undo record, and stop complaining for having it
in non-last output spends. This means that undo dat written with
this patch won't be readable by older versions anymore.

7d991b55db

Introduce Coin, a single unspent output 422634e2f5

sipa force-pushed on May 26, 2017

sipa commented at 9:49 pm on May 26, 2017: member

Rebased on top of #10445, and squashed fixes into appropriate commits. No other changes were made.

I think this is ready.

gmaxwell commented at 7:44 am on May 30, 2017: contributor

re-ACK

in src/undo.h:17 in 1927df1a23 outdated

15  *
16- *  Contains the prevout's CTxOut being spent, and if this was the
17- *  last output of the affected transaction, its metadata as well
18- *  (coinbase or not, height, transaction version)
19+ *  Contains the prevout's CTxOut being spent, and its metadata as well
20+ *  (coinbase or not, height). Earlier versions also stored the transaction

jnewbery commented at 1:45 pm on May 30, 2017:

I think what you’re trying to say with "Earlier versions also stored the transaction version." would be better captured as "Also stores a dummy value of zero. This is to be compatible with older versions which stored the transaction version".

sipa commented at 1:25 am on May 31, 2017:

Done.

in src/undo.h:29 in 1927df1a23 outdated

40-        if (nHeight > 0)
41-            ::Serialize(s, VARINT(this->nVersion));
42-        ::Serialize(s, CTxOutCompressor(REF(txout)));
43+        ::Serialize(s, VARINT(txout->nHeight * 2 + (txout->fCoinBase ? 1 : 0)));
44+        if (txout->nHeight > 0) {
45+            // Store a dummy for the transaction version.

jnewbery commented at 1:57 pm on May 30, 2017:

perhaps add Required to maintain compatibility with undo files written by pre-v0.15 clients

sipa commented at 1:26 am on May 31, 2017:

Fixed.

in test/functional/blockchain.py:54 in 1927df1a23 outdated

48@@ -49,10 +49,12 @@ def _test_gettxoutsetinfo(self):
49         assert_equal(res['transactions'], 200)
50         assert_equal(res['height'], 200)
51         assert_equal(res['txouts'], 200)
52-        assert_equal(res['bytes_serialized'], 13924),
53         assert_equal(res['bestblock'], node.getblockhash(200))
54+        size = res['disk_size']
55+        assert size > 6400

jnewbery commented at 2:13 pm on May 30, 2017:

nit: prefer using the util function assert_greater_than_or_equal() for this and the next line (assert_greater_than_or_equal() will print the value of size if the assert fails). Also consider implementing an assert_in_range() function to combine the two lines.

You could also add the size asserts to the tests on res2 and res3 below.

sipa commented at 1:27 am on May 31, 2017:

Will do, but this should go in #10396.

in src/undo.h:29 in 1927df1a23 outdated

39-        ::Serialize(s, VARINT(nHeight*2+(fCoinBase ? 1 : 0)));
40-        if (nHeight > 0)
41-            ::Serialize(s, VARINT(this->nVersion));
42-        ::Serialize(s, CTxOutCompressor(REF(txout)));
43+        ::Serialize(s, VARINT(txout->nHeight * 2 + (txout->fCoinBase ? 1 : 0)));
44+        if (txout->nHeight > 0) {

jnewbery commented at 2:22 pm on May 30, 2017:

Why is this test needed? Won’t the prevout’s height always be greater than zero?

sipa commented at 1:25 am on May 31, 2017:

Added a comment to explain.

in src/coins.h:71 in 1927df1a23 outdated

235-        // coinbase height
236-        ::Unserialize(s, VARINT(nHeight));
237-        Cleanup();
238+        uint32_t code = 0;
239+        ::Unserialize(s, VARINT(code));
240+        nHeight = code >> 1;

jnewbery commented at 2:31 pm on May 30, 2017:

Is there a particular reason you’re using a bitshift here, but multiplication/division elsewhere (in Serialize() and in TxInUndoSerializer::Serialize()/Deserialize())?

sipa commented at 1:27 am on May 31, 2017:

Not really… just inconsistently applying the best practices of using the faster operation (and not relying on the compiler optimizer for doing so).

in src/coins.cpp:394 in 7c1efc60f1 outdated

387@@ -318,3 +388,16 @@ CCoinsModifier::~CCoinsModifier()
388 CCoinsViewCursor::~CCoinsViewCursor()
389 {
390 }
391+
392+static const size_t MAX_OUTPUTS_PER_BLOCK = MAX_BLOCK_BASE_SIZE /  ::GetSerializeSize(CTxOut(), SER_NETWORK, PROTOCOL_VERSION); // TODO: merge with similar definition in undo.h.
393+
394+const Coin AccessByTxid(const CCoinsViewCache& view, const uint256& txid)

morcos commented at 8:12 pm on May 30, 2017:

Should we add some sort of performance warning here, so people don’t think they should be able to use this function without thinking about what it might end up doing?

sdaftuar commented at 8:40 pm on May 30, 2017: member

re-ACK

in src/rpc/blockchain.cpp:988 in 1927df1a23 outdated

985+    Coin coin;
986     if (fMempool) {
987         LOCK(mempool.cs);
988         CCoinsViewMemPool view(pcoinsTip, mempool);
989-        if (!view.GetCoins(hash, coins))
990+        if (!view.GetCoins(out, coin) || mempool.isSpent(out)) // TODO: this should be done by the CCoinsViewMemPool

jnewbery commented at 8:56 pm on May 30, 2017:

nit: should use new style of braces for an if clause on a new line (and remove the outdated comment)

sipa commented at 1:24 am on May 31, 2017:

Fixed. The comment is not outdated.

jnewbery commented at 3:06 pm on May 31, 2017:

In that case, I don’t understand the comment. I thought it meant that the CCoinsViewMemPool class should be responsible for pruning the spent funds, instead of only when called through the rpc or rest.

in src/coins.h:150 in 1927df1a23 outdated

145@@ -306,17 +146,17 @@ class CCoinsViewCursor
146 class CCoinsView
147 {
148 public:
149-    //! Retrieve the CCoins (unspent transaction outputs) for a given txid
150-    virtual bool GetCoins(const uint256 &txid, CCoins &coins) const;
151+    //! Retrieve the Coin (unspent transaction output) for a given outpoint.
152+    virtual bool GetCoins(const COutPoint &outpoint, Coin &coin) const;

jnewbery commented at 9:15 pm on May 30, 2017:

Rename to GetCoin()?

sipa commented at 1:28 am on May 31, 2017:

Fixed using scripted-diff.

in src/coins.h:154 in 1927df1a23 outdated

154-    //! Just check whether we have data for a given txid.
155-    //! This may (but cannot always) return true for fully spent transactions
156-    virtual bool HaveCoins(const uint256 &txid) const;
157+    //! Just check whether we have data for a given outpoint.
158+    //! This may (but cannot always) return true for spent outputs.
159+    virtual bool HaveCoins(const COutPoint &outpoint) const;

jnewbery commented at 9:16 pm on May 30, 2017:

rename to HaveCoin() (or HasCoin())?

in src/coins.h:276 in 1927df1a23 outdated

276-
277-    friend class CCoinsModifier;
278-
279 private:
280-    CCoinsMap::const_iterator FetchCoins(const uint256 &txid) const;
281+    CCoinsMap::iterator FetchCoins(const COutPoint &outpoint) const;

jnewbery commented at 9:16 pm on May 30, 2017:

rename to FetchCoin()?

sipa commented at 1:28 am on May 31, 2017:

Fixed using scripted-diff.

in src/txdb.cpp:30 in 1927df1a23 outdated

24@@ -24,17 +25,40 @@ static const char DB_FLAG = 'F';
25 static const char DB_REINDEX_FLAG = 'R';
26 static const char DB_LAST_BLOCK = 'l';
27 
28+namespace {
29+
30+struct CoinsEntry {

jnewbery commented at 9:24 pm on May 30, 2017:

Rename to CoinEntry?

sipa commented at 1:29 am on May 31, 2017:

Fixed using scripted-diff.

in src/coins.h:221 in 1927df1a23 outdated

253+     * Check if we have the given utxo already loaded in this cache.
254      * The semantics are the same as HaveCoins(), but no calls to
255      * the backing CCoinsView are made.
256      */
257-    bool HaveCoinsInCache(const uint256 &txid) const;
258+    bool HaveCoinsInCache(const COutPoint &outpoint) const;

jnewbery commented at 9:32 pm on May 30, 2017:

Rename to HaveCoinInCache() (or HasCoinInCache())?

sipa commented at 1:28 am on May 31, 2017:

Fixed using scripted-diff.

in src/validation.cpp:453 in 1927df1a23 outdated

458-        // and only helps with filling in pfMissingInputs (to determine missing vs spent).
459         BOOST_FOREACH(const CTxIn txin, tx.vin) {
460-            if (!pcoinsTip->HaveCoinsInCache(txin.prevout.hash))
461-                vHashTxnToUncache.push_back(txin.prevout.hash);
462-            if (!view.HaveCoins(txin.prevout.hash)) {
463+            if (!pcoinsTip->HaveCoinsInCache(txin.prevout))

jnewbery commented at 9:45 pm on May 30, 2017:

braces

sipa commented at 1:30 am on May 31, 2017:

Fixed.

in src/validation.cpp:442 in 1927df1a23 outdated

442-            if (!fHadTxInCache)
443-                vHashTxnToUncache.push_back(hash);
444-            return state.Invalid(false, REJECT_ALREADY_KNOWN, "txn-already-known");
445+        for (size_t out = 0; out < tx.vout.size(); out++) {
446+            COutPoint outpoint(hash, out);
447+            bool fHadTxInCache = pcoinsTip->HaveCoinsInCache(outpoint);

jnewbery commented at 9:49 pm on May 30, 2017:

rename to fHadCoinInCache?

sipa commented at 1:30 am on May 31, 2017:

Fixed using scripted-diff.

in src/validation.cpp:347 in 1927df1a23 outdated

343@@ -344,7 +344,7 @@ static bool IsCurrentForFeeEstimation()
344 
345 bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const CTransactionRef& ptx, bool fLimitFree,
346                               bool* pfMissingInputs, int64_t nAcceptTime, std::list<CTransactionRef>* plTxnReplaced,
347-                              bool fOverrideMempoolLimit, const CAmount& nAbsurdFee, std::vector<uint256>& vHashTxnToUncache)
348+                              bool fOverrideMempoolLimit, const CAmount& nAbsurdFee, std::vector<COutPoint>& vHashTxnToUncache)

jnewbery commented at 9:50 pm on May 30, 2017:

rename final argument to vCoinsToUncache?

sipa commented at 1:30 am on May 31, 2017:

Fixed using scripted-diff.

in src/validation.cpp:764 in 1927df1a23 outdated

760@@ -763,10 +761,10 @@ bool AcceptToMemoryPoolWithTime(CTxMemPool& pool, CValidationState &state, const
761                         bool* pfMissingInputs, int64_t nAcceptTime, std::list<CTransactionRef>* plTxnReplaced,
762                         bool fOverrideMempoolLimit, const CAmount nAbsurdFee)
763 {
764-    std::vector<uint256> vHashTxToUncache;
765+    std::vector<COutPoint> vHashTxToUncache;

jnewbery commented at 9:51 pm on May 30, 2017:

rename to vCoinToUncache?

sipa commented at 1:30 am on May 31, 2017:

Fixed using scripted-diff.

in src/test/test_bitcoin_fuzzy.cpp:171 in 1927df1a23 outdated

167@@ -168,7 +168,7 @@ int do_fuzz()
168         {
169             try
170             {
171-                CCoins block;
172+                Coin block;

jnewbery commented at 9:53 pm on May 30, 2017:

rename variable to coin?

sipa commented at 1:31 am on May 31, 2017:

Fixed.

in src/coins.h:76 in 1927df1a23 outdated

250-        return (nPos < vout.size() && !vout[nPos].IsNull());
251-    }
252-
253-    //! check whether the entire CCoins is spent
254-    //! note that only !IsPruned() CCoins can be serialized
255     bool IsPruned() const {

jnewbery commented at 10:00 pm on May 30, 2017:

to me, IsPruned() makes less sense in the context of a single coin. For CCoins, IsPruned() meant that all the coins were spent and so the containing object could be pruned. For a single Coin class, I think IsSpent() makes more sense.

sipa commented at 1:28 am on May 31, 2017:

Fixed using scripted-diff.

in src/test/coins_tests.cpp:284 in 1927df1a23 outdated

284     // Track the txids we've used in various sets
285-    std::set<uint256> coinbaseids;
286-    std::set<uint256> disconnectedids;
287-    std::set<uint256> duplicateids;
288-    std::set<uint256> utxoset;
289+    std::set<COutPoint> coinbaseids;

jnewbery commented at 10:02 pm on May 30, 2017:

rename to coinbasecoins? Same for next two variables.

sipa commented at 1:31 am on May 31, 2017:

Fixed using scripted-diff.

in src/test/coins_tests.cpp:300 in 1927df1a23 outdated

294@@ -264,138 +295,144 @@ BOOST_AUTO_TEST_CASE(updatecoins_simulation_test)
295             tx.vin.resize(1);
296             tx.vout.resize(1);
297             tx.vout[0].nValue = i; //Keep txs unique unless intended to duplicate
298+            tx.vout[0].scriptPubKey.assign(insecure_rand() & 0x3F, 0); // Random sizes so we can test memory usage accounting
299             unsigned int height = insecure_rand();
300-            CCoins oldcoins;
301+            Coin oldcoins;

jnewbery commented at 10:03 pm on May 30, 2017:

rename to oldcoin?

sipa commented at 1:31 am on May 31, 2017:

Fixed using scripted-diff.

in src/test/coins_tests.cpp:360 in 1927df1a23 outdated

378+                utxoset.erase(prevout);
379 
380                 // The test is designed to ensure spending a duplicate coinbase will work properly
381                 // if that ever happens and not resurrect the previously overwritten coinbase
382-                if (duplicateids.count(prevouthash))
383+                if (duplicateids.count(prevout))

jnewbery commented at 10:07 pm on May 30, 2017:

braces, same line, or ternary operator

sipa commented at 1:32 am on May 31, 2017:

Fixed.

in src/test/coins_tests.cpp:381 in 1927df1a23 outdated

402-            alltxs.insert(std::make_pair(tx.GetHash(),std::make_tuple(tx,undo,oldcoins)));
403+            utxoData.emplace(outpoint, std::make_tuple(tx,undo,oldcoins));
404         }
405 
406         //1/20 times undo a previous transaction
407         else if (utxoset.size()) {

jnewbery commented at 10:07 pm on May 30, 2017:

braces or same line

sipa commented at 1:33 am on May 31, 2017:

Fixed.

in src/test/coins_tests.cpp:386 in 1927df1a23 outdated

413+            auto utxod = FindRandomFrom(utxoset);
414 
415-            uint256 undohash = tx.GetHash();
416+            CTransaction &tx = std::get<0>(utxod->second);
417+            CTxUndo &undo = std::get<1>(utxod->second);
418+            Coin &origcoins = std::get<2>(utxod->second);

jnewbery commented at 10:08 pm on May 30, 2017:

rename to origcoin?

sipa commented at 1:33 am on May 31, 2017:

Fixed using scripted-diff.

in src/rpc/blockchain.cpp:991 in 1927df1a23 outdated

990+        if (!view.GetCoins(out, coin) || mempool.isSpent(out)) // TODO: this should be done by the CCoinsViewMemPool
991             return NullUniValue;
992-        mempool.pruneSpent(hash, coins); // TODO: this should be done by the CCoinsViewMemPool
993     } else {
994-        if (!pcoinsTip->GetCoins(hash, coins))
995+        if (!pcoinsTip->GetCoins(out, coin))

jnewbery commented at 10:18 pm on May 30, 2017:

braces or same line

sipa commented at 1:25 am on May 31, 2017:

Fixed.

in src/rpc/blockchain.cpp:998 in 1927df1a23 outdated

1000 
1001     BlockMap::iterator it = mapBlockIndex.find(pcoinsTip->GetBestBlock());
1002     CBlockIndex *pindex = it->second;
1003     ret.push_back(Pair("bestblock", pindex->GetBlockHash().GetHex()));
1004-    if ((unsigned int)coins.nHeight == MEMPOOL_HEIGHT)
1005+    if ((unsigned int)coin.nHeight == MEMPOOL_HEIGHT)

jnewbery commented at 10:18 pm on May 30, 2017:

braces

sipa commented at 1:25 am on May 31, 2017:

Fixed.

in src/txmempool.h:34 in 1927df1a23 outdated

29@@ -30,7 +30,7 @@
30 class CAutoFile;
31 class CBlockIndex;
32 
33-/** Fake height value used in CCoins to signify they are only in the memory pool (since 0.8) */
34+/** Fake height value used in Coin to signify they are only in the memory pool (since 0.8) */
35 static const unsigned int MEMPOOL_HEIGHT = 0x7FFFFFFF;

jnewbery commented at 10:21 pm on May 30, 2017:

Why not make this a uint32_t to match Coin.nHeight?

sipa commented at 1:33 am on May 31, 2017:

Fixed.

jnewbery commented at 10:27 pm on May 30, 2017: member

I’ve got as far as 0dcdbbd0b36298299d59f3960cd8b0be48dd3b80

Switch CCoinsView and chainstate db from per-txid to per-txout

All of my comments are nits, mostly around naming where the names are still ___coins, when in fact the variable or function is now referring to single coin object. You can take them or leave them (or we can address in a future PR). They certainly shouldn’t hold up merging this PR.

in src/init.cpp:1460 in 1bc281f36e outdated

1454@@ -1455,6 +1455,11 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1455                     //If we're reindexing in prune mode, wipe away unusable block files and all undo data files
1456                     if (fPruneMode)
1457                         CleanupBlockRevFiles();
1458+                } else {
1459+                    if (!pcoinsdbview->Upgrade()) {

jnewbery commented at 2:36 pm on May 31, 2017:

I’d like to see some comments around this call (and the Upgrade() function definition). I imagine that at some point in a future version we might want to remove the Upgrade code and tear out the CCoins class entirely. That will break the ability to upgrade directly from pre 0.15 to that version without a reindex. A small comment at this call saying that this Upgrade is for a chainstate db build on pre 0.15 would make make that easier.

sipa commented at 4:13 pm on May 31, 2017:

Added some comments. The details about upgrading are in the cpp file, because I expect that the Upgrade method will remain, regardless of what upgrades it supports.

in src/txdb.cpp:259 in 1bc281f36e outdated

251@@ -215,3 +252,121 @@ bool CBlockTreeDB::LoadBlockIndexGuts(std::function<CBlockIndex*(const uint256&)
252 
253     return true;
254 }
255+
256+namespace {
257+
258+class CCoins

jnewbery commented at 2:38 pm on May 31, 2017:

Please add a comment saying this is a legacy class, only retained for the ability to upgrade chainstate DBs from pre 0.15 to post 0.15 without a reindex.

sipa commented at 4:13 pm on May 31, 2017:

Done.

in src/txdb.cpp:347 in 1bc281f36e outdated

342+    CDBBatch batch(db);
343+    while (pcursor->Valid()) {
344+        boost::this_thread::interruption_point();
345+        std::pair<unsigned char, uint256> key;
346+        if (pcursor->GetKey(key) && key.first == DB_COINS) {
347+            CCoins old_coin;

jnewbery commented at 2:54 pm on May 31, 2017:

this was caught up in your scripted diff. For correctness should really be called old_coins, but I don’t think it really matters.

sipa commented at 4:14 pm on May 31, 2017:

Reverted.

in src/txdb.cpp:278 in 1bc281f36e outdated

273+    /**
274+     * calculate number of bytes for the bitmask, and its number of non-zero bytes
275+     * each bit in the bitmask represents the availability of one output, but the
276+     * availabilities of the first two outputs are encoded separately
277+     */
278+    void CalcMaskSize(unsigned int &nBytes, unsigned int &nNonzeroBytes) const {

jnewbery commented at 2:57 pm on May 31, 2017:

I think this can be removed? The only CCoins functions that are used in Upgrade() are the constructor and the deserializer.

sipa commented at 4:13 pm on May 31, 2017:

Great catch, done!

in src/rpc/blockchain.cpp:1000 in 1bc281f36e outdated

1002 
1003     BlockMap::iterator it = mapBlockIndex.find(pcoinsTip->GetBestBlock());
1004     CBlockIndex *pindex = it->second;
1005     ret.push_back(Pair("bestblock", pindex->GetBlockHash().GetHex()));
1006-    if ((unsigned int)coins.nHeight == MEMPOOL_HEIGHT)
1007+    if ((unsigned int)coin.nHeight == MEMPOOL_HEIGHT) {

jnewbery commented at 3:09 pm on May 31, 2017:

cast not required. coin.nHeight and MEMPOOL_HEIGHT are both uint32_t

jnewbery commented at 3:20 pm on May 31, 2017: member

Finished reviewing. Just a few more nits.

I’m not familiar enough with the code to give a full ACK for functionality and non-regression, but I’ve reviewed as best I can for correctness and style and it looks good to me.

I’ve also done some limited test locally and haven’t hit any problems.

sipa force-pushed on May 31, 2017

jnewbery commented at 5:17 pm on May 31, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/10195/commits/dfdf1d5fef732d24602e693f917e41ed357d4937

laanwj commented at 11:38 am on June 1, 2017: member

Tested ACK dfdf1d5

I think this is ready.

I agree. Are you going to do a final squash before merge (“Address some nits” and “Address more nits” seem like squashme’s)?

harding commented at 12:21 pm on June 1, 2017: contributor

How does this change affect UTXO set bloat attacks?

In the current model where UTXO entries are indexed by txid, I think the most efficient attack would be stuffing a block full of max-sized (10,000 byte?) scriptPubKeys. That makes the worst-case UTXO byte-size increase per block slightly smaller than the maximum base block size.

But if UTXO entries are indexed by outpoint, it seems to me an attacker could create a single large transaction with many small outputs (minimum size 9 bytes?) which would each add a 33 to 36 byte compressed outpoint to the UTXO DB. That makes the worst-case byte-size increase per block a multiple of the maximum base block size.

I see @sipa’s comment in the OP about, “LevelDB internally uses an encoding that omits repeated prefix bytes in keys, and because of that, duplicating the txids is not very significant.” I was just wondering if the affect of this change on UTXO bloat attacks was considered (or was even worth considering)?

morcos commented at 3:25 pm on June 1, 2017: member

This is a fantastic change. I’m sorry it’s taking me so long to work through it. I can’t promise I will finish today, but I have no objection to merge based on other’s review and I’ll keep working my way through either way. @harding raises an interesting question, but I feel its more of an academic one, there is nothing preventing us from changing the implementation again in the future if that becomes an issue (or heaven forbid reverting)

gmaxwell commented at 4:27 pm on June 1, 2017: contributor

@harding As you note that sipa notes: the database deduplicates the IDs; this largely eliminates the concern your present. The old structure also has quadratic access costs for transactions with many outputs which this change eliminates.

sipa commented at 5:06 pm on June 1, 2017: member

@laanwj I’ll squash soon. @harding It’s an interesting question what the worst cast UTXO bloat attack becomes after this change. I believe you’re right that many small outputs will have a larger impact on disk size, though not as dramatically as you predict - most of it will be eaten by prefix compression.

in src/validation.cpp:1312 in b1216c967a outdated

1308+        for (size_t o = 0; o < tx.vout.size(); o++) {
1309+            if (!tx.vout[o].scriptPubKey.IsUnspendable()) {
1310+                COutPoint out(hash, o);
1311+                Coin coin;
1312+                view.SpendCoin(out, &coin);
1313+                if (tx.vout[o] != coin.out) {

morcos commented at 5:48 pm on June 1, 2017:

Do we want to check that coin.nHeight == pindex->nHeight ? Not sure if it matters, but that used to get checked…

sipa commented at 6:36 pm on June 1, 2017: member

Pre-squash history:

e66dbde6d14cb5f253b3bf8850a98f4fda2d9f49: Add SizeEstimate to CDBBatch
f54580e7e4f225bb615204daef32f72ab8688418: error() in disconnect for disk corruption, not inconsistency
e484652fc36ef7135cf08ad380ea7142b6cbadc0: Introduce CHashVerifier to hash read data
7e0032290669fae5f52c256856c53038511c7db4: Add specialization of SipHash for 256 + 32 bit data
d342424301013ec47dc146a4beb49d5c9319d80a: Remove/ignore tx version in utxo and undo
c3aa0c11947dfd82702df276d39bb7f748dd83a1: Report on-disk size in gettxoutsetinfo
7d991b55dbf0b0f6e21c0680ee3ebd09df09012f: Store/allow tx metadata in all undo records
422634e2f5ac1ff74cd358144cecbac63007adc4: Introduce Coin, a single unspent output
16a2789d33ab830787da8e716f4a65e14aecb159: Replace CTxInUndo with Coin
4d1246060c3587c1605f85735a50515929a8deff: Optimization: Coin&& to ApplyTxInUndo
7c1efc60f122ae18f501babdcae9047507267be6: Introduce new per-txout CCoinsViewCache functions
b1216c967a9f12a18bcce9a9a2e39be30825c367: Switch from per-tx to per-txout CCoinsViewCache methods in some places
2cc9c5495c8c5f9b394017e163a21e4e3f9b2c84: Only pass things committed to by tx’s witness hash to CScriptCheck
f2d42ab5d45163aa2a6574884635ba3c2bb82984: Switch CScriptCheck to use Coin instead of CCoins
875b4123134c02a61486060996d3573116be776b: Switch tests from ModifyCoins to AddCoin/SpendCoin
96b992072a8cda6509b4185118aadb3097452ee2: Remove ModifyCoins/ModifyNewCoins
f1b70c320f31d4a0cced7928757297cbb3475cff: Replace CCoins-based CTxMemPool::pruneSpent with isSpent
105377227ce041b943d149c59458d48319e56730: Refactor GetUTXOStats in preparation for per-COutPoint iteration
0dcdbbd0b36298299d59f3960cd8b0be48dd3b80: Switch CCoinsView and chainstate db from per-txid to per-txout
b3cf86e59245d3715ab4e32186e0e30931f63aa8: Extend coins_tests
d475d03c4ca1dedaba311add2e2c38279c79c6a0: Remove unused CCoins methods
363843fc4c0f1eadef589be20af34738e67ba835: Pack Coin more tightly
c5faf390dfee5c969b20e8bee305542ef74fe8cb: Reduce reserved memory space for flushing
72eed2650fa9f3d31205712a965b9f99a5c8ee99: Upgrade from per-tx database to per-txout
7acbc2b7c688729c7c50802182300b7859f77349: [MOVEONLY] Move old CCoins class to txdb.cpp
854dad479c53960ee7fbae2bb7f544c10473b023: Merge CCoinsViewCache’s GetOutputFor and AccessCoin
ea0a7b9224d20962aa6bb78e5bd678b8d5492832: Rename CCoinsCacheEntry::coins to coin
1927df1a2316004d5cac22b65a561b578b37ec47: Increase travis unit test timeout
0328074fc823e0e9cc8461140df68fcdf70d43fd: scripted-diff: various renames for per-utxo consistency
1bc281f36e8ec88c66dad651c405b381cb9a39a0: Address some nits
dfdf1d5fef732d24602e693f917e41ed357d4937: Address more nits

harding commented at 6:51 pm on June 1, 2017: contributor

@morcos @gmaxwell @sipa Thanks for your responses (and, with the others, your work on this change!). I agree the ability to adapt the underlying implementation to prevailing conditions makes the question academic, as hopefully does the increasing ratio of fees-to-subsidy which increases the lost-income cost to miners for maximizing bloat.

Please consider my comment non-blocking, and thank you all again for your work.

Replace CTxInUndo with Coin

The earlier CTxInUndo class now holds the same information as the Coin
class. Instead of duplicating functionality, replace CTxInUndo with a
serialization adapter for Coin.

cb2c7fdac2

Optimization: Coin&& to ApplyTxInUndo

This avoids a prevector copy in ApplyTxInUndo.

bd83111a0f

Introduce new per-txout CCoinsViewCache functions

The new functions are:
* CCoinsViewCache::AddCoin: Add a single COutPoint/Coin pair.
* CCoinsViewCache::SpendCoin: Remove a single COutPoint.
* AddCoins: utility function that invokes CCoinsViewCache::AddCoin for
  each output in a CTransaction.
* AccessByTxid: utility function that searches for any output with
  a given txid.
* CCoinsViewCache::AccessCoin: retrieve the Coin for a COutPoint.
* CCoinsViewCache::HaveCoins: check whether a non-empty Coin exists
  for a given COutPoint.

The AddCoin and SpendCoin methods will eventually replace ModifyCoins
and ModifyNewCoins, AddCoins will replace CCoins::FromTx, and the new
AccessCoins and HaveCoins functions will replace their per-txid
counterparts.

Note that AccessCoin for now returns a copy of the Coin object. In a
later commit it will be change to returning a const reference (which
keeps working in all call sites).

0003911326

Switch from per-tx to per-txout CCoinsViewCache methods in some places f68cdfe92b

Only pass things committed to by tx's witness hash to CScriptCheck

This clarifies a bit more the ways in which the new script execution
cache could break consensus in the future if additional data from
the CCoins object were to be used as a part of script execution.

After this change, any such consensus breaks should be very visible
to reviewers, hopefully ensuring no such changes can be made.

c87b957a32

Switch CScriptCheck to use Coin instead of CCoins 8b3868c1b4

Switch tests from ModifyCoins to AddCoin/SpendCoin 961e483979

Remove ModifyCoins/ModifyNewCoins 05293f3cb7

Replace CCoins-based CTxMemPool::pruneSpent with isSpent 13870b56fc

Refactor GetUTXOStats in preparation for per-COutPoint iteration 4ec0d9e794

Switch CCoinsView and chainstate db from per-txid to per-txout

This patch makes several related changes:
* Changes the CCoinsView virtual methods (GetCoins, HaveCoins, ...)
  to be COutPoint/Coin-based rather than txid/CCoins-based.
* Changes the chainstate db to a new incompatible format that is also
  COutPoint/Coin based.
* Implements reconstruction code for hash_serialized_2.
* Adapts the coins_tests unit tests (thanks to Russell Yanofsky).

A side effect of the new CCoinsView model is that we can no longer
use the (unreliable) test for transaction outputs in the UTXO set
to determine whether we already have a particular transaction.

5083079688

Extend coins_tests ce23efaa5c

Remove unused CCoins methods 97072d6685

Pack Coin more tightly 41aa5b79a3

Reduce reserved memory space for flushing

As the maximum amount of data that can be pulled into the cache due to
a block validation is much lower now (at most one CCoin entry per input
and per output), reduce the conservative estimate used to determine
flushing time.

b2af357f39

Upgrade from per-tx database to per-txout 8b25d2c0ce

[MOVEONLY] Move old CCoins class to txdb.cpp

It's only used for upgrading from the old database anymore.

580b023092

Merge CCoinsViewCache's GetOutputFor and AccessCoin

They're doing the same thing now.

119e552f7c

Rename CCoinsCacheEntry::coins to coin 73de2c1ff3

Increase travis unit test timeout a5e02bc7f8

scripted-diff: various renames for per-utxo consistency

Thanks to John Newberry for pointing these out.

-BEGIN VERIFY SCRIPT-
sed -i 's/\<GetCoins\>/GetCoin/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<HaveCoins\>/HaveCoin/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<HaveCoinsInCache\>/HaveCoinInCache/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<IsPruned\>/IsSpent/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<FetchCoins\>/FetchCoin/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<CoinsEntry\>/CoinEntry/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<vHashTxnToUncache\>/coins_to_uncache/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<vHashTxToUncache\>/coins_to_uncache/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<fHadTxInCache\>/had_coin_in_cache/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<coinbaseids\>/coinbase_coins/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<disconnectedids\>/disconnected_coins/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<duplicateids\>/duplicate_coins/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
sed -i 's/\<oldcoins\>/old_coin/g' src/test/coins_tests.cpp
sed -i 's/\<origcoins\>/orig_coin/g' src/*.cpp src/*.h src/*/*.cpp src/*/*.h
-END VERIFY SCRIPT-

589827975f

in src/coins.h:153 in 0dcdbbd0b3 outdated

370 
371-    //! Just check whether we have data for a given txid.
372-    //! This may (but cannot always) return true for fully spent transactions
373-    virtual bool HaveCoins(const uint256 &txid) const;
374+    //! Just check whether we have data for a given outpoint.
375+    //! This may (but cannot always) return true for spent outputs.

morcos commented at 6:52 pm on June 1, 2017:

This comment is incorrect I believe? It will never return true (nor should it) for a spent output

sipa force-pushed on Jun 1, 2017

sipa commented at 8:20 pm on June 1, 2017: member

Squashed all fixes without rebasing (so the resulting tree is identical to dfdf1d5).

sipa merged this on Jun 1, 2017

sipa closed this on Jun 1, 2017

sipa referenced this in commit 1088b02f0c on Jun 1, 2017

fanquake removed this from the "Blockers" column in a project

in src/validation.cpp:443 in 5083079688 outdated

443-                vHashTxnToUncache.push_back(hash);
444-            return state.Invalid(false, REJECT_ALREADY_KNOWN, "txn-already-known");
445+        for (size_t out = 0; out < tx.vout.size(); out++) {
446+            COutPoint outpoint(hash, out);
447+            bool fHadTxInCache = pcoinsTip->HaveCoinsInCache(outpoint);
448+            if (view.HaveCoins(outpoint)) {

morcos commented at 4:58 pm on June 7, 2017:

This is a DOS vector for too many disk lookups for a tx with lots of outputs, maybe the best bet is to check for all outputs only in cache iff there is a missing input

in src/txdb.h:25 in b2af357f39 outdated

21@@ -22,9 +22,7 @@ class uint256;
22 //! Compensate for extra memory peak (x1.5-x1.9) at flush time.
23 static constexpr int DB_PEAK_USAGE_FACTOR = 2;
24 //! No need to periodic flush if at least this much space still available.
25-static constexpr int MAX_BLOCK_COINSDB_USAGE = 200 * DB_PEAK_USAGE_FACTOR;
26-//! Always periodic flush if less than this much space still available.
27-static constexpr int MIN_BLOCK_COINSDB_USAGE = 50 * DB_PEAK_USAGE_FACTOR;
28+static constexpr int MAX_BLOCK_COINSDB_USAGE = 10 * DB_PEAK_USAGE_FACTOR;

morcos commented at 7:53 pm on June 7, 2017:

maybe this should be 30? sdaftuar said he saw 17 in only a week of testing

sdaftuar commented at 8:36 pm on June 7, 2017:

17 was incorrect; I mistakenly reported the cache delta between blocks (which includes the effect of transactions on the cache) as the per-block cache delta.

morcos commented at 8:04 pm on June 7, 2017: member

posthumous utACK (very untested)

nice work. i left my couple of nits anyway. I can open a PR to address the output lookups in ATMP and for any of the others that are worth addressing.

sipa deleted the branch on Jun 23, 2017

MarcoFalke referenced this in commit 96a63a3e0c on Aug 11, 2017

in src/rest.cpp:57 in 589827975f

54     template <typename Stream, typename Operation>
55     inline void SerializationOp(Stream& s, Operation ser_action)
56     {
57-        READWRITE(nTxVer);
58+        uint32_t nTxVerDummy = 0;
59+        READWRITE(nTxVerDummy);

Earlz commented at 4:48 am on September 11, 2017:

What is the point of this? Just to keep the database backwards compatible?

codablock referenced this in commit 7a6024e877 on Sep 27, 2017

codablock referenced this in commit fe479c3c84 on Oct 12, 2017

codablock referenced this in commit 1c75e080c9 on Oct 26, 2017

codablock referenced this in commit 36bd23ead6 on Oct 26, 2017

codablock referenced this in commit 32cc06647c on Oct 30, 2017

codablock referenced this in commit 8407b3e7de on Oct 31, 2017

codablock referenced this in commit 54ffb133a3 on Oct 31, 2017

codablock referenced this in commit c81394b975 on Oct 31, 2017

UdjinM6 referenced this in commit 95bc4cf30c on Nov 8, 2017

PastaPastaPasta referenced this in commit 50a10a6769 on Sep 8, 2019

PastaPastaPasta referenced this in commit 836db0ec07 on Sep 8, 2019

PastaPastaPasta referenced this in commit d2f1259c8a on Sep 8, 2019

barrystyle referenced this in commit b77fef2d5a on Jan 22, 2020

laanwj referenced this in commit 9e8e813df5 on Apr 22, 2020

furszy referenced this in commit 90ffc6683b on Aug 19, 2020

random-zebra referenced this in commit 30d353edab on Sep 5, 2020

jasonbcox referenced this in commit 6a02152a2b on Nov 4, 2020

PastaPastaPasta referenced this in commit ab0ab96a3b on Sep 17, 2021

PastaPastaPasta referenced this in commit 436b52eb99 on Sep 17, 2021

PastaPastaPasta referenced this in commit d0b0f08432 on Sep 18, 2021

thelazier referenced this in commit bc1e2ebbb5 on Sep 25, 2021

DrahtBot locked this on Feb 15, 2022

Switch chainstate db and cache to per-txout model #10195