refactor: have CCoins* data managed under CChainState

jamesob commented at 7:27 pm on July 23, 2019: member

Parent PR: #15606 Issue: #15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal

This change encapsulates UTXO set data within CChainState instances, removing global data pcoinsTip and pcoinsviewdb. This is necessary if we want to maintain multiple chainstates with their own rendering of the UTXO set.

We introduce a class CoinsViews which consolidates the construction of a CCoins* hierarchy.

This commit could be broken into smaller pieces, but it would require more ephemeral diffs to, e.g., temporarily change CCoinsViewDB’s constructor invocations.

fanquake added the label Refactoring on Jul 23, 2019

in src/validation.h:23 in e56af43d8d outdated

18@@ -19,6 +19,8 @@
19 #include <script/script_error.h>
20 #include <sync.h>
21 #include <txmempool.h> // For CTxMemPool::cs
22+#include <txdb.h>
23+#include <util/system.h>

MarcoFalke commented at 7:48 pm on July 23, 2019:

I’d prefer to make the call to GetDataDir in the cpp file.

in src/validation.h:665 in e56af43d8d outdated

717@@ -659,11 +718,7 @@ CChain& ChainActive();
718 /** @returns the global block index map. */
719 BlockMap& BlockIndex();
720 
721-/** Global variable that points to the coins database (protected by cs_main) */
722-extern std::unique_ptr<CCoinsViewDB> pcoinsdbview;
723-
724-/** Global variable that points to the active CCoinsView (protected by cs_main) */

MarcoFalke commented at 7:48 pm on July 23, 2019:

Why are you removing the comment that this data is protected by cs_main?

ryanofsky commented at 6:14 pm on July 24, 2019:

In commit “refactor: pcoinsTip -> CChainState::CoinsTip()” (9edf6537f13b61952f77ab62c2221c2b18d9561a)

Why are you removing the comment that this data is protected by cs_main?

Seems resolved now (comment is just moved).

jamesob commented at 8:08 pm on July 24, 2019:

Good point - since it was easy to do, I’ve tacked on a commit that adds lock annotations to the equivalent CoinsViews members.

in src/validation.cpp:3876 in e56af43d8d outdated

3872@@ -3853,12 +3873,13 @@ bool static LoadBlockIndexDB(const CChainParams& chainparams) EXCLUSIVE_LOCKS_RE
3873 bool LoadChainTip(const CChainParams& chainparams)
3874 {
3875     AssertLockHeld(cs_main);
3876-    assert(!pcoinsTip->GetBestBlock().IsNull()); // Never called when the coins view is empty
3877+    CCoinsViewCache* coins_cache = &::ChainstateActive().CoinsTip();

MarcoFalke commented at 7:50 pm on July 23, 2019:

0    const CCoinsViewCache& coins_cache = ::ChainstateActive().CoinsTip();

in src/validation.cpp:88 in e56af43d8d outdated

85+std::unique_ptr<CChainState> g_chainstate;
86 
87-CChainState& ChainstateActive() { return g_chainstate; }
88+CChainState& ChainstateActive() {
89+    assert(g_chainstate);
90+    return *g_chainstate.get();

MarcoFalke commented at 7:52 pm on July 23, 2019:

0    return *g_chainstate;

in src/net_processing.cpp:1294 in e56af43d8d outdated

1290@@ -1291,11 +1291,12 @@ bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1291                 LOCK(g_cs_orphans);
1292                 if (mapOrphanTransactions.count(inv.hash)) return true;
1293             }
1294+            CCoinsViewCache& coins_cache = ::ChainstateActive().CoinsTip();

MarcoFalke commented at 7:54 pm on July 23, 2019:

0            const CCoinsViewCache& coins_cache = ::ChainstateActive().CoinsTip();

MarcoFalke commented at 7:59 pm on July 23, 2019: member

Would it make sense to split this into dumb refactoring and then hard-to-review changes, like the on in init.cpp?

An early commit (scripted-diff or not) could add ChainstateActive::CoinsTip, a dumb alias to return pcoinsTip; and replace the usages. The later commit could then make the other changes in this pull.

DrahtBot commented at 10:30 pm on July 23, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16411 (Signet support by kallewoof)
#16400 ([refactor] Rewrite AcceptToMemoryPoolWorker() using smaller parts by sdaftuar)
#16324 (Get cs_main out of the critical path in ProcessMessages by TheBlueMatt)
#16323 (Call ProcessNewBlock() asynchronously by TheBlueMatt)
#15997 (refactor: GuessVerificationProgress requires cs_main lock by promag)
#13868 (Remove unused fScriptChecks parameter from CheckInputs by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake added this to the "In progress" column in a project

jamesob force-pushed on Jul 24, 2019

in src/validation.cpp:522 in 9edf6537f1 outdated

512@@ -513,12 +513,13 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
513         CCoinsViewCache view(&dummy);
514 
515         LockPoints lp;
516-        CCoinsViewMemPool viewMemPool(pcoinsTip.get(), pool);
517+        CCoinsViewCache* coins_cache = &::ChainstateActive().CoinsTip();

ryanofsky commented at 6:11 pm on July 24, 2019:

In commit “refactor: pcoinsTip -> CChainState::CoinsTip()” (9edf6537f13b61952f77ab62c2221c2b18d9561a)

Here and in rpc/blockchain.cpp, if you’re going to introduce a new variable, it seems better to make it a reference than a raw pointer. (IMO at least it is best to reserve c++ pointers for nullable cases to refer to objects that may or may not exist, and c++ references to refer to objects that are assumed to exist).

DrahtBot added the label Needs rebase on Jul 24, 2019

in src/validation.cpp:1053 in e5d83bf15b outdated

1042+    std::string ldb_name,
1043+    size_t cache_size_bytes,
1044+    bool in_memory,
1045+    bool should_wipe)
1046+{
1047+    m_dbview.reset(new CCoinsViewDB(

ryanofsky commented at 6:21 pm on July 24, 2019:

In commit “refactor: add CoinsViews class” (e5d83bf15bbf1f9359dcb6146dc5f12ae61d8720)

This doesn’t seem to compile. CCoinsViewDB at this point takes 3 arguments not 4. Also could avoid raw pointer management and new call with member initialization and MakeUnique:

0: m_dbview(MakeUnique<CCoinsViewDB>(cache_size_bytes, in_memory, should_wipe))

Member initialization is better because it ensure objects are destroyed in reverse order they are constructed, and raw pointers and news are just generally suspicious and should be avoided when you can make ownership explicit.

jamesob commented at 8:15 pm on July 24, 2019:

Ah, oops - you’re right. Shouldn’t have tried to split those commits out.

in src/rpc/blockchain.cpp:1066 in ceca2c92b7 outdated

1061@@ -1062,6 +1062,8 @@ static UniValue gettxoutsetinfo(const JSONRPCRequest& request)
1062 
1063     CCoinsStats stats;
1064     ::ChainstateActive().ForceFlushStateToDisk();
1065+
1066+    LOCK(::cs_main);

ryanofsky commented at 7:06 pm on July 24, 2019:

In commit “Cover UTXO set access with lock annotations” (ceca2c92b79a8a2c5ae37d03e99de80ec55895c3)

Commit message is a bit misleading imo, because this is adding a new lock, not just new annotations.

But more importantly, GetUTXOStats is acquiring and releasing cs_main internally, so this change seems like it is complicating locking and also locking too much. Maybe a change like the following would make sense?

0-    LOCK(::cs_main);
1-    if (GetUTXOStats(&::ChainstateActive().CoinsDB(), stats)) {
2+    CCoinsView* coins_view = WITH_LOCK(cs_main, return &ChainstateActive().CoinsDB());
3+    if (GetUTXOStats(coins_view, stats)) {

jamesob commented at 8:25 pm on July 24, 2019:

Ah that’s a good point - I forgot that GetUTXOStats only briefly acquires cs_main to get the height for a certain blockhash; it (counterintuitively) doesn’t need to hold cs_main to iterate through the coins db because leveldb’s implicit use of snapshots with cursors.

I think the WITH_LOCK usage above obeys the letter of the law but not the spirit since technically the coinsdb could be modified in between that line and the GetUTXOStats call, but in this context I don’t think it matters. Thanks for the suggestion!

in src/validation.h:750 in eed4e62ad6 outdated

713@@ -686,8 +714,7 @@ CChain& ChainActive();
714 /** @returns the global block index map. */
715 BlockMap& BlockIndex();
716 
717-/** Global variable that points to the coins database (protected by cs_main) */
718-extern std::unique_ptr<CCoinsViewDB> pcoinsdbview;
719+extern std::unique_ptr<CChainState> g_chainstate;

ryanofsky commented at 7:30 pm on July 24, 2019: