Add a CChainState class to validation.cpp to take another step towards clarifying internal interfaces

TheBlueMatt commented at 1:49 am on April 26, 2017: member

CChainState should eventually, essentially, be our exposed “libconsensus”, but we’re probably a few releases away, so the real goal is to clarify our internal interfaces. The main split was a big step, but validation.cpp is still a somewhat ranomly-mixed bag of functions that are pure functions which validate inputs (which should probably either merge with their callers or move into another file in consensus/), read/write data from disk, manipulate our current chain state (which moves into CChainState), and do mempool transaction validation.

Obviously this is only a small step, but some effort is made to clean up what functions the functions in CChainState call, and obviously as things are added its easy to keep clear “CChainState::* cannot call anything except via callbacks through CValidationInterface, pure functions, or disk read/write things”. Right now there are some glaring violations in mempool callbacks, and general flushing logic needs cleaning up (FlushStateToDisk maybe shouldnt be called, and there should be an API towards setDirtyBlockIndex, but I’ll leave that for after @sipa’s current changesets land).

TheBlueMatt commented at 1:53 am on April 26, 2017: member

Obviously the end goal here is to take revenge for #771 :)

TheBlueMatt force-pushed on Apr 26, 2017

fanquake added the label Validation on Apr 26, 2017

in src/validation.cpp:3399 in 20aa3811df outdated

3537@@ -3511,9 +3538,9 @@ CBlockIndex * InsertBlockIndex(uint256 hash)
3538     return pindexNew;
3539 }
3540 
3541-bool static LoadBlockIndexDB(const CChainParams& chainparams)
3542+bool CChainState::Load(CBlockTreeDB& blocktree)

jtimon commented at 2:23 pm on April 27, 2017:

The CChainParams are still needed (or at least Consensus::Params) see https://github.com/bitcoin/bitcoin/pull/9176

TheBlueMatt commented at 2:46 pm on April 27, 2017:

Probably better to move the CheckProofOfWord check into CChainParams::InsertBlockIndex (or the lambda). As eventually CChainParams should be a local variable inside CChainState, any objections to leaving this for later?

jtimon commented at 3:46 pm on April 27, 2017:

What lambda? Anyway, if we plan not to do #9176 and moving the check instead, then there’s no point in passing Consensus::Params to CChainState::Load if it’s going to be only temporary.

TheBlueMatt commented at 4:02 pm on April 27, 2017:

eg the CheckProofOfWork call that is currently in LoadBlockIndexGuts can just go in the insertBlockIndex lambda function and the call to that should be a fully-filled CBlockIndex instead of that getting filled by LoadBlockIndexGuts.

in src/validation.cpp:3293 in 33aad08c6e outdated

3168+    if (!FindBlockPos(blockPos, nBlockSize+8, nHeight, block.GetBlockTime(), dbp != NULL)) {
3169+        error("AcceptBlock(): FindBlockPos failed");
3170+        return CDiskBlockPos();
3171+    }
3172+    if (dbp == NULL) {
3173+        if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart())) {

jtimon commented at 2:26 pm on April 27, 2017:

If this is the only use of chainparams, perhaps it is better to just pass chainparams.MessageStart() directly instead.

TheBlueMatt commented at 2:48 pm on April 27, 2017:

Seems somewhat strange for CChainState to care at all about the existance of MessageStart(), no?

jtimon commented at 3:41 pm on April 27, 2017:

Strange or not, it does care about ut. It seems more strange for this new function to take the whole CChainParams class only for one getter, no?

TheBlueMatt commented at 3:59 pm on April 27, 2017:

Well CChainState doesn’t care about MessageStart(), it only cares that the write function figures out what it needs to do, if that means using Messagestart() thats between the write function and chainparams, not CChainState’s business. Eventually the write function should be a virtual function inside an interface class and that class can hold the chainparams locally with CChainState only having a Consensus::Params.

jtimon commented at 4:22 pm on April 27, 2017:

CChainState::Load definitely cares about MessageStart(). But we disagree on next steps, I won’t insist further.

in src/validation.cpp:1325 in e32dc53af1 outdated

1445@@ -1446,8 +1446,12 @@ bool UndoWriteToDisk(const CBlockUndo& blockundo, CDiskBlockPos& pos, const uint
1446     return true;
1447 }
1448 
1449-bool UndoReadFromDisk(CBlockUndo& blockundo, const CDiskBlockPos& pos, const uint256& hashBlock)
1450+bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex *pindex)

jtimon commented at 2:40 pm on April 27, 2017:

I think UndoReadFromDisk can be static.

TheBlueMatt commented at 2:49 pm on April 27, 2017:

It is already in an anonymous namespace, which I believe is equivalent.

in src/validation.cpp:1977 in f2e9d9a5d8 outdated

1943-            pindex->nStatus |= BLOCK_HAVE_UNDO;
1944-        }
1945+    if (!WriteUndoDataForBlock(blockundo, state, pindex, chainparams))
1946+        return false;
1947 
1948+    if (!pindex->IsValid(BLOCK_VALID_SCRIPTS)) {

instagibbs commented at 2:40 pm on April 27, 2017:

just noting: previously the following two lines are hit even if this conditional isn’t true(and pindex->GetUndoPos().IsNull() is). No idea if this is a behavior change

TheBlueMatt commented at 3:37 pm on April 27, 2017:

Hmm, it may be, though not sure you could ever tickle it. I went ahead and duplicated the setDirtyBlockIndex.insert() call inside of WriteUndoDataForBlock.

in src/validation.cpp:1636 in f2e9d9a5d8 outdated

1629+    // Write undo information to disk
1630+    if (pindex->GetUndoPos().IsNull()) {
1631+        CDiskBlockPos _pos;
1632+        if (!FindUndoPos(state, pindex->nFile, _pos, ::GetSerializeSize(blockundo, SER_DISK, CLIENT_VERSION) + 40))
1633+            return error("ConnectBlock(): FindUndoPos failed");
1634+        if (!UndoWriteToDisk(blockundo, _pos, pindex->pprev->GetBlockHash(), chainparams.MessageStart()))

jtimon commented at 2:40 pm on April 27, 2017:

Again, If this is the only use of chainparams, perhaps it is better to just pass chainparams.MessageStart() directly instead.

in src/validation.cpp:1656 in f2e9d9a5d8 outdated

1651+    {
1652+        vPos.push_back(std::make_pair(tx->GetHash(), pos));
1653+        pos.nTxOffset += ::GetSerializeSize(*tx, SER_DISK, CLIENT_VERSION);
1654+    }
1655+
1656+    if (fTxIndex)

instagibbs commented at 2:45 pm on April 27, 2017:

Why do any of this if fTxIndex is false?

TheBlueMatt commented at 3:30 pm on April 27, 2017:

Because it used to be? Anyway, fixed in new commit.

in src/validation.cpp:76 in 20aa3811df outdated

72+
73+            // Identical blocks.
74+            return false;
75+        }
76+    };
77+};

jtimon commented at 2:46 pm on April 27, 2017:

There was a PR uniforming comments for ending namespaces. Also, according to our style, we don’t indent on namespaces: https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format#L32

TheBlueMatt commented at 2:50 pm on April 27, 2017:

Fixed.

TheBlueMatt force-pushed on Apr 27, 2017

in src/validation.cpp:3240 in 33aad08c6e outdated

3246-        if (dbp == NULL)
3247-            if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart()))
3248-                AbortNode(state, "Failed to write block");
3249+        CDiskBlockPos blockPos = WriteBlockToDisk(block, pindex->nHeight, chainparams, dbp);
3250+        if (blockPos.IsNull()) {
3251+            state.Error("AcceptBlock(): Failed to find position to write new block to disk");

jtimon commented at 2:56 pm on April 27, 2017:

strprintf("%s: Failed to find position to write new block to disk", __func__)

TheBlueMatt commented at 3:00 pm on April 27, 2017:

Fixed.

in src/validation.cpp:3169 in 33aad08c6e outdated

3164+    unsigned int nBlockSize = ::GetSerializeSize(block, SER_DISK, CLIENT_VERSION);
3165+    CDiskBlockPos blockPos;
3166+    if (dbp != NULL)
3167+        blockPos = *dbp;
3168+    if (!FindBlockPos(blockPos, nBlockSize+8, nHeight, block.GetBlockTime(), dbp != NULL)) {
3169+        error("AcceptBlock(): FindBlockPos failed");

jtimon commented at 2:56 pm on April 27, 2017:

strprintf("%s: FindBlockPos failed", __func__)

TheBlueMatt commented at 3:00 pm on April 27, 2017:

Fixed.

TheBlueMatt force-pushed on Apr 27, 2017

jtimon commented at 3:04 pm on April 27, 2017: contributor

utACK (beyond nits, most of them not very important):

Make DisconnectBlock unaware of where undo data resides on disk e32dc53af14aa28d936da9c9b5a80b5fe522ad18
Create initial CChainState to hold chain state information 20aa3811df65e39d60572876ab2850eea6194429
Move block writing out of AcceptBlock 33aad08c6e7b0a9e291163c39538bee00be7e953
Move some additional variables into CChainState private 0acb237f752c0cf7fd783e8c09c0cb55203a6a83

Still trying to understand: Make ConnectBlock unaware of txindex/undo data disk locations f2e9d9a5d8d57b277634c5f822134aab7fc05b7b

instagibbs approved

instagibbs commented at 3:21 pm on April 27, 2017: member

not-expert utACK aside from fTxIndex question

TheBlueMatt force-pushed on Apr 27, 2017

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

TheBlueMatt force-pushed on Apr 30, 2017

TheBlueMatt commented at 6:51 pm on April 30, 2017: member

Removed InsertBlockIndex from validation.h since it was moved into CChainState (there were no external callers already).

in src/validation.cpp:1329 in e32dc53af1 outdated

1445@@ -1446,8 +1446,12 @@ bool UndoWriteToDisk(const CBlockUndo& blockundo, CDiskBlockPos& pos, const uint
1446     return true;
1447 }
1448 
1449-bool UndoReadFromDisk(CBlockUndo& blockundo, const CDiskBlockPos& pos, const uint256& hashBlock)
1450+bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex *pindex)
1451 {
1452+    CDiskBlockPos pos = pindex->GetUndoPos();
1453+    if (pos.IsNull())
1454+        return error("%s: Undo data not available", __func__);

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

In commit “Make DisconnectBlock unaware of where undo data resides on disk”

Note slight change in behavior here. “no undo data available” is now “Undo data not available”

TheBlueMatt commented at 0:20 am on May 3, 2017:

Reverted the string change.

ryanofsky commented at 8:09 pm on June 8, 2017:

In commit “Make DisconnectBlock unaware of where undo data resides on disk”

String changed back again. Did you maybe rebase an older version of this PR? It seems like some of the changes you made between https://github.com/bitcoin/bitcoin/commits/c431484b612c68d4faf7f8d6af858729f2b9a56d and https://github.com/bitcoin/bitcoin/commits/120743a83f26923cd9ef4db068fd6c9b8b8ea4dd got reverted.

in src/validation.cpp:1629 in 41fef26554 outdated

1623@@ -1624,6 +1624,43 @@ void static FlushBlockFile(bool fFinalize = false)
1624 
1625 bool FindUndoPos(CValidationState &state, int nFile, CDiskBlockPos &pos, unsigned int nAddSize);
1626 
1627+static bool WriteUndoDataForBlock(const CBlockUndo& blockundo, CValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams)

ryanofsky commented at 5:38 pm on May 2, 2017:

In commit “Make ConnectBlock unaware of txindex/undo data disk locations”

MaybeWriteUndoDataForBlock might be a more descriptive name.

in src/validation.cpp:1657 in 41fef26554 outdated

1648+    CDiskTxPos pos(pindex->GetBlockPos(), GetSizeOfCompactSize(block.vtx.size()));
1649+    std::vector<std::pair<uint256, CDiskTxPos> > vPos;
1650+    vPos.reserve(block.vtx.size());
1651+    for (const CTransactionRef& tx : block.vtx)
1652+    {
1653+        vPos.push_back(std::make_pair(tx->GetHash(), pos));

ryanofsky commented at 5:46 pm on May 2, 2017:

In commit “Make ConnectBlock unaware of txindex/undo data disk locations”:

This is just moved code, but could get rid of make_pair using emplace_back here. Could also get rid of pos variable using vPos.back().

TheBlueMatt commented at 0:04 am on May 3, 2017:

Clarified in the commit message instead, thanks for pointing this out (the intent is to not do the actual reading from disk calls inside of CChainState). Will leave other cleanups for later PRs.

in src/validation.cpp:188 in 48e4cb5971 outdated

 97@@ -98,6 +98,7 @@ class CChainState {
 98     bool Load(CBlockTreeDB& blocktree);
 99 
100 private:
101+    /** Create a new block index entry for a given block hash */

ryanofsky commented at 6:13 pm on May 2, 2017:

Should squash the two “Create initial CChainState to hold chain state information” commits.

TheBlueMatt commented at 0:00 am on May 3, 2017:

Oh, no idea how that came about, thanks.

in src/validation.cpp:93 in b8f6d69812 outdated

79+/**
80+ * CChainState stores and provides an API to update our local copy of the chain
81+ *
82+ * It generally provides access to the current chain tree, as well as functions
83+ * to provide it new data, which it will appropriately validate and incorporate
84+ * in its state as neccessary.

ryanofsky commented at 6:15 pm on May 2, 2017:

In commit “Create initial CChainState to hold chain state information”

Spelling “necessary”

TheBlueMatt commented at 0:04 am on May 3, 2017:

Fixed.

in src/validation.cpp:89 in b8f6d69812 outdated

75+    }
76+};
77+};
78+
79+/**
80+ * CChainState stores and provides an API to update our local copy of the chain

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

In commit “Create initial CChainState to hold chain state information”

Stores what? May be some words missing.

TheBlueMatt commented at 0:00 am on May 3, 2017:

I’ll clarify. It pretty much means mapBlockIndex/chainActive.

in src/validation.cpp:97 in b8f6d69812 outdated

83+ * to provide it new data, which it will appropriately validate and incorporate
84+ * in its state as neccessary.
85+ *
86+ * Eventually, the API here is targeted at being exposed externally as a
87+ * consumable libconsensus library, so any functions added must only call
88+ * external functions via callbacks, or to read/write data from disk.

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

In commit “Create initial CChainState to hold chain state information”

What’s an external function? Would be helpful to say something about which functions are and aren’t external.

TheBlueMatt commented at 0:20 am on May 3, 2017:

Clarified a bunch.

ryanofsky commented at 8:14 pm on June 8, 2017:

In commit “Create initial CChainState to hold chain state information”

CChainState comment updates made previously seem to be lost, pindexBestInvalid initialization is gone, LoadBlockIndex() got changed back to Load(), and InsertBlockIndex() documentation is gone.

ryanofsky commented at 5:33 pm on July 25, 2017:

Thread #10279 (review)

Newer comment got reverted somehow. It was:

 0/**
 1 * CChainState stores and provides an API to update our local knowledge of the
 2 * current best chain and header tree.
 3 *
 4 * It generally provides access to the current block tree, as well as functions
 5 * to provide new data, which it will appropriately validate and incorporate in
 6 * its state as necessary.
 7 *
 8 * Eventually, the API here is targeted at being exposed externally as a
 9 * consumable libconsensus library, so any functions added must only call
10 * other class member functions, pure functions in other parts of the consensus
11 * library, callbacks, or read/write-to-disk functions (eventually this will
12 * also be via callbacks).
13 */

in src/validation.cpp:123 in b8f6d69812 outdated

91+public:
92+    CChain chainActive;
93+    BlockMap mapBlockIndex;
94+    std::multimap<CBlockIndex*, CBlockIndex*> mapBlocksUnlinked;
95+    std::set<CBlockIndex*, CBlockIndexWorkComparator> setBlockIndexCandidates;
96+    CBlockIndex *pindexBestInvalid;

ryanofsky commented at 6:22 pm on May 2, 2017:

In commit “Create initial CChainState to hold chain state information”

Should add = nullptr;

TheBlueMatt commented at 0:06 am on May 3, 2017:

Fixed.

in src/validation.cpp:3401 in b8f6d69812 outdated

3538@@ -3512,9 +3539,9 @@ CBlockIndex * InsertBlockIndex(uint256 hash)
3539     return pindexNew;
3540 }
3541 
3542-bool static LoadBlockIndexDB(const CChainParams& chainparams)
3543+bool CChainState::Load(CBlockTreeDB& blocktree)
3544 {
3545-    if (!pblocktree->LoadBlockIndexGuts(InsertBlockIndex))
3546+    if (!blocktree.LoadBlockIndexGuts([this](uint256 hash){ return this->InsertBlockIndex(hash); }))

ryanofsky commented at 6:26 pm on May 2, 2017:

In commit “Create initial CChainState to hold chain state information”

Probably should pass hash by const reference.

TheBlueMatt commented at 0:20 am on May 3, 2017:

Indeed, not actually 100% sure why that compiled…

in src/validation.cpp:126 in b8f6d69812 outdated

93+    BlockMap mapBlockIndex;
94+    std::multimap<CBlockIndex*, CBlockIndex*> mapBlocksUnlinked;
95+    std::set<CBlockIndex*, CBlockIndexWorkComparator> setBlockIndexCandidates;
96+    CBlockIndex *pindexBestInvalid;
97+
98+    bool Load(CBlockTreeDB& blocktree);

ryanofsky commented at 6:30 pm on May 2, 2017:

In commit “Create initial CChainState to hold chain state information”

Maybe Load and InsertBlockIndex should be static while they aren’t (yet?) accessing any class members.

TheBlueMatt commented at 0:20 am on May 3, 2017:

Load adds to mapBlocksUnlinked and InsertBlockIndex into mapBlockIndex. In the next PR mapBlockIndex and chainActive will be const when accessed via the global references, so would prefer to leave it.

in src/validation.cpp:3378 in a49f6add32 outdated

3323@@ -3278,6 +3324,8 @@ static bool AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CValidation
3324     if (fCheckForPruning)
3325         FlushStateToDisk(state, FLUSH_STATE_NONE); // we just allocated more disk space for block files
3326 
3327+    CheckBlockIndex(chainparams.GetConsensus());

ryanofsky commented at 6:55 pm on May 2, 2017:

In commit “Move a bunch of chainstate-manipulation functions into CChainState”

CheckBlockIndex call added accidentally here? Behavior change should probably go in a separate commit.

TheBlueMatt commented at 0:20 am on May 3, 2017:

This was to replace the one removed from ProcessNewBlock. Should be +/- equivalent to the old version, I believe?

in src/validation.cpp:3794 in a49f6add32 outdated

3941@@ -3881,6 +3942,17 @@ bool LoadBlockIndex(const CChainParams& chainparams)
3942     return true;
3943 }
3944 
3945+
3946+bool CChainState::ReceiveGenesisBlock(const CBlock &block, const CDiskBlockPos& blockPos, const Consensus::Params& consensusParams) {
3947+    CBlockIndex *pindex = AddToBlockIndex(block);
3948+    CValidationState state;
3949+    if (!ReceivedBlockTransactions(block, state, pindex, blockPos, consensusParams)) {
3950+        return error("LoadBlockIndex(): genesis block not accepted");

ryanofsky commented at 7:08 pm on May 2, 2017:

In commit “Move a bunch of chainstate-manipulation functions into CChainState”:

LoadBlockIndex should be ReceiveGenesisBlock or __func__ now.

TheBlueMatt commented at 0:20 am on May 3, 2017:

Fixed

in src/validation.cpp:181 in a49f6add32 outdated

128+
129+    void PruneBlockIndexCandidates();
130+
131+    bool ReceiveGenesisBlock(const CBlock &block, const CDiskBlockPos& blockPos, const Consensus::Params& consensusParams);
132+
133+    void UnloadBlockIndex();

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

In commit “Move a bunch of chainstate-manipulation functions into CChainState”

Maybe this method should just be called Unload to be consistent with the Load method (which was previously LoadBlockIndexDB)

TheBlueMatt commented at 0:20 am on May 3, 2017:

Changed Load to LoadBlockIndex instead.

ryanofsky commented at 7:24 pm on May 2, 2017: member

utACK c431484b612c68d4faf7f8d6af858729f2b9a56d.

No major comments, just a lot of suggestions, which you should feel free to ignore if not worth implementing.

I do think it would be nice if you added another commit which deleted the mapBlockIndex chainActive pindexBestInvalid mapBlocksUnlinked global references. It would make CChainState start to look more like a normal class.

TheBlueMatt force-pushed on May 3, 2017

TheBlueMatt commented at 0:21 am on May 3, 2017: member

As for removing the global references to member parts of CChainState…that’s probably a ways out. First step is to clarify the internal stuff within validation.cpp and make external accesses to it const, then I’ll expose the class itself (unless you feel particularly inclined to build the scripted diff before I get to it - need #10189 first, probably.

ryanofsky commented at 1:48 pm on May 3, 2017: member

utACK 120743a83f26923cd9ef4db068fd6c9b8b8ea4dd

TheBlueMatt commented at 1:34 am on June 6, 2017: member

Rebased.

TheBlueMatt force-pushed on Jun 6, 2017

ryanofsky commented at 8:18 pm on June 8, 2017: member

Started re-reviewing this, but it looks like there was a problem with the rebase and some changes were lost.

TheBlueMatt force-pushed on Jun 9, 2017

TheBlueMatt commented at 5:26 pm on June 9, 2017: member

Indeed, rebased the copy that was on my workstation and not my laptop, re-rebased.

TheBlueMatt force-pushed on Jun 9, 2017

sipa commented at 0:00 am on June 13, 2017: member

Nice, concept ACK

ryanofsky commented at 5:47 pm on June 13, 2017: member

utACK 5eb9eede5804b9c25b50429006a005a75a95da0f. Confirmed only changes since last review were fixing rebase conflicts, adding {} around some if statements, restoring a deleted comment.

TheBlueMatt force-pushed on Jun 28, 2017

ryanofsky changes_requested

ryanofsky commented at 5:41 pm on July 25, 2017: member

Started looking at this to re-ack, but it looks like the wrong version of the branch got rebased and pushed again. I think if you just rebase 5eb9eede5804b9c25b50429006a005a75a95da0f top of current master, that should fix it.

TheBlueMatt force-pushed on Aug 15, 2017

TheBlueMatt commented at 9:13 pm on August 15, 2017: member

Rebased properly this time, with the various 0.15 changes.

in src/validation.cpp:1978 in 8559637128 outdated

1867-        }
1868+    if (!WriteUndoDataForBlock(blockundo, state, pindex, chainparams))
1869+        return false;
1870 
1871+    if (!pindex->IsValid(BLOCK_VALID_SCRIPTS)) {
1872         pindex->RaiseValidity(BLOCK_VALID_SCRIPTS);

ryanofsky commented at 2:40 pm on August 16, 2017:

In commit “Move txindex/undo data disk location stuff out of ConnectBlock”

It seems RaiseValidity call used to happen in the case pindex->GetUndoPos().IsNull() && pindex->IsValid(BLOCK_VALID_SCRIPTS) but now doesn’t happen anymore.

Guessing this is not an issue but wanted to point out the change.

in src/validation.cpp:3288 in dc461681ff outdated

3102@@ -3103,6 +3103,25 @@ bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, CValidatio
3103 }
3104 
3105 /** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
3106+static CDiskBlockPos WriteBlockToDisk(const CBlock& block, int nHeight, const CChainParams& chainparams, const CDiskBlockPos* dbp) {
3107+    unsigned int nBlockSize = ::GetSerializeSize(block, SER_DISK, CLIENT_VERSION);
3108+    CDiskBlockPos blockPos;
3109+    if (dbp != nullptr)
3110+        blockPos = *dbp;
3111+    if (!FindBlockPos(blockPos, nBlockSize+8, nHeight, block.GetBlockTime(), dbp != nullptr)) {

ryanofsky commented at 3:11 pm on August 16, 2017:

In commit “Move block writing out of AcceptBlock”

You aren’t changing this, but a harder to screw up interface to FindBlockPos might take an optional<CDiskBlockPos>& argument, instead of CDiskBlockPos& and fKnown arguments.

TheBlueMatt commented at 11:07 pm on October 13, 2017:

I’ll leave interface changes for later - eventually I’d like to move the disk-writing/reading stuff out of validation.cpp, then it’ll be easy to play with interfaces that wont even be exposed (plus the functions will reside beside each other, so harder to mess up).

in src/validation.cpp:3106 in dc461681ff outdated

3102@@ -3103,6 +3103,25 @@ bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, CValidatio
3103 }
3104 
3105 /** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
3106+static CDiskBlockPos WriteBlockToDisk(const CBlock& block, int nHeight, const CChainParams& chainparams, const CDiskBlockPos* dbp) {

ryanofsky commented at 3:16 pm on August 16, 2017:

In commit “Move block writing out of AcceptBlock”

Might call this SaveBlockToDisk or since it doesn’t actually write to disk if it’s already written.

TheBlueMatt commented at 11:10 pm on October 13, 2017:

SaveBlockToDisk it is, that is much less confusing.

in src/validation.cpp:3366 in dc461681ff outdated

3189-        if (dbp == nullptr)
3190-            if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart()))
3191-                AbortNode(state, "Failed to write block");
3192+        CDiskBlockPos blockPos = WriteBlockToDisk(block, pindex->nHeight, chainparams, dbp);
3193+        if (blockPos.IsNull()) {
3194+            state.Error(strprintf("%s: Failed to find position to write new block to disk", __func__));

ryanofsky commented at 3:26 pm on August 16, 2017:

In commit “Move block writing out of AcceptBlock”:

This seems misleading since the error can also happen if the internal WriteBlockToDisk call fails. Maybe the new WriteBlockToDisk should just call state.Error itself, or return the error strings it already generates. Returning an error might also make sense because in both WriteBlockToDisk calls, the returned blockpos isn’t actually used for anything.

TheBlueMatt commented at 11:17 pm on October 13, 2017:

The goal here is that SaveBlockToDisk falls outside the scope of our consensus logic, and, thus, should have no idea about CValidationStates. The errors when writing to disk are entirely separate (and logged themselves, so its not like its going to result in materially different useability here).

in src/validation.cpp:103 in a6e978884e outdated

 98+ *
 99+ * Eventually, the API here is targeted at being exposed externally as a
100+ * consumable libconsensus library, so any functions added must only call
101+ * other class member functions, pure functions in other parts of the consensus
102+ * library, callbacks, or read/write-to-disk functions (eventually this will
103+ * also be via callbacks).

ryanofsky commented at 3:37 pm on August 16, 2017:

In commit “Create initial CChainState to hold chain state information”

Maybe mention:

Things that methods in this class should not be accessing. Presumably mempool & network?
Examples of where this class is currently doing something that it shouldn’t and will be cleaned up later.

TheBlueMatt commented at 11:13 pm on October 13, 2017:

I clarified that callbacks should all be via the validation interface. I think that is sufficiently restirctive? Anything not in those categories is clearly not kosher, not sure what specific examples should be given without trawling through the code there again.

in src/validation.cpp:153 in a6e978884e outdated

112+    std::set<CBlockIndex*, CBlockIndexWorkComparator> setBlockIndexCandidates;
113+public:
114+    CChain chainActive;
115+    BlockMap mapBlockIndex;
116+    std::multimap<CBlockIndex*, CBlockIndex*> mapBlocksUnlinked;
117+    CBlockIndex *pindexBestInvalid = nullptr;

ryanofsky commented at 3:44 pm on August 16, 2017:

In commit “Create initial CChainState to hold chain state information”

Since you’re creating backwards compatible aliases for these variables anyway, it seems like you could easily give them updated names that follow the style guide (e.g. m_best_invalid_block).

TheBlueMatt commented at 11:14 pm on October 13, 2017:

I wanted this PR to not conflict needlessly with every change to validation.cpp, so avoided doing so for now. In the future we should do that, agreed, but I dont want to rebase this PR every few days.

in src/validation.cpp:162 in a6e978884e outdated

157+    CBlockIndex* FindMostWorkChain();
158+    bool ReceivedBlockTransactions(const CBlock &block, CValidationState& state, CBlockIndex *pindexNew, const CDiskBlockPos& pos, const Consensus::Params& consensusParams);
159+
160+
161+    bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs, const CChainParams& params);
162+} chainstate;

ryanofsky commented at 3:46 pm on August 16, 2017:

In commit “Move a bunch of chainstate-manipulation functions into CChainState”

Maybe g_chainstate

TheBlueMatt commented at 11:27 pm on October 13, 2017:

Done.

in src/validation.cpp:4074 in a6e978884e outdated

3956@@ -3864,10 +3957,21 @@ bool RewindBlockIndex(const CChainParams& params)
3957         PruneBlockIndexCandidates();
3958 
3959         CheckBlockIndex(params.GetConsensus());
3960+    }
3961+
3962+    return true;
3963+}
3964+
3965+bool RewindBlockIndex(const CChainParams& params) {

ryanofsky commented at 4:05 pm on August 16, 2017:

In commit “Move a bunch of chainstate-manipulation functions into CChainState”

Two RewindBlockIndex functions with the same name & params but slightly different behavior seems not great.

Maybe should have RewindBlockIndexDB / CChainState::RewindBlockIndex, analogous to LoadBlockIndexDB / CChainState::LoadBlockIndex.

TheBlueMatt commented at 11:28 pm on October 13, 2017:

They dont (really) have different behavior, just the public wrapper version does a DB flush, but that isnt strictly required (I believe). But, really, CChainState/validation should have no concept of the DB backend in use/the disk, hence the reason for the flush outside, its just a thin wrapper functions to expose it publicly.

theuni commented at 7:31 pm on December 12, 2017:

Agree with @ryanofsky. Especially since the other wrappers just forward to g_chainstate. Why not just flush in init after the RewindBlockIndex call?

TheBlueMatt commented at 7:58 pm on December 12, 2017:

I’d be fine with a flush in init, but it seems weird given its kinda a validation-disk-storage-layer thing, and I hate adding more low-level logic in init.

in src/validation.cpp:2046 in 21ec3737e5 outdated

2079@@ -2080,10 +2080,8 @@ static void DoWarning(const std::string& strWarning)
2080     }
2081 }
2082 
2083-/** Update chainActive and related internal data structures. */
2084+/** Check warning conditions and do some notifications on new chain tip set. */
2085 void static UpdateTip(CBlockIndex *pindexNew, const CChainParams& chainParams) {
2086-    chainActive.SetTip(pindexNew);

ryanofsky commented at 4:08 pm on August 16, 2017:

In commit “Move some additional variables into CChainState private”

This seems like a strange change. Maybe assert chainActive tip is pindexNew here? pindexNew argument is otherwise no longer used in this function.

TheBlueMatt commented at 11:07 pm on October 13, 2017:

I think it feels strange mostly because UpdateTip is (now) poorly named - all it does is fire warnings, log messages, and a few notifications for mining longpolling. Eventually it should be a CValidationInterface listener (or its notifications should be routed through it. I went ahead and used pindexNew in place of chainActive.Tip() everywhere here.

ryanofsky commented at 4:53 pm on August 16, 2017: member

utACK 3b78b2d9ef1fef1fc838522f2f0bab319bf962a0. All my comments are minor, feel free to ignore.

ryanofsky commented at 5:36 pm on October 12, 2017: member

Should this be merged? It has utACKs from jtimon, instagibbs, and me, and a concept ACK from sipa.

TheBlueMatt force-pushed on Oct 13, 2017

TheBlueMatt commented at 11:29 pm on October 13, 2017: member

Addressed @ryanofsky’s nits and noted where I disagreed.

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

Make DisconnectBlock unaware of where undo data resides on disk 93a34cfeec

Move txindex/undo data disk location stuff out of ConnectBlock 50701ba5fc

Move block writing out of AcceptBlock e104f0fb7e

Create initial CChainState to hold chain state information fd4d80a2f8

Move some additional variables into CChainState private 2862aca40f

Avoid calling GetSerializeSize on each tx in a block if !fTxIndex 22fdddeabb

TheBlueMatt force-pushed on Dec 4, 2017

TheBlueMatt commented at 2:40 pm on December 4, 2017: member

Rebased.

laanwj commented at 1:36 pm on December 12, 2017: member

utACK 22fdddeabb17881af2004c45538f91514837d363

laanwj merged this on Dec 12, 2017

laanwj closed this on Dec 12, 2017

laanwj referenced this in commit 214046f69b on Dec 12, 2017

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

theuni commented at 7:45 pm on December 12, 2017: member

post-merge utACK 22fdddeabb17881af2004c45538f91514837d363. Very nice!

fanquake moved this from the "In progress" to the "Done" column in a project

PastaPastaPasta referenced this in commit 4695adf349 on Apr 2, 2020

PastaPastaPasta referenced this in commit a82abe2ad7 on Apr 2, 2020

PastaPastaPasta referenced this in commit 5ca8b1a405 on Apr 6, 2020

UdjinM6 referenced this in commit 93513d1296 on Apr 11, 2020

ckti referenced this in commit 5c031b0c4c on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021

Add a CChainState class to validation.cpp to take another step towards clarifying internal interfaces #10279