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

TheBlueMatt · 2017-04-26T01:49:59Z

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 · 2017-04-26T01:53:33Z

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

jtimon · 2017-04-27T14:23:18Z

src/validation.cpp

@@ -3511,9 +3538,9 @@ CBlockIndex * InsertBlockIndex(uint256 hash)
    return pindexNew;
 }

-bool static LoadBlockIndexDB(const CChainParams& chainparams)
+bool CChainState::Load(CBlockTreeDB& blocktree)


The CChainParams are still needed (or at least Consensus::Params) see #9176

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?

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.

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.

jtimon · 2017-04-27T14:26:16Z

src/validation.cpp

+        return CDiskBlockPos();
+    }
+    if (dbp == NULL) {
+        if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart())) {


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

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

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?

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.

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

jtimon · 2017-04-27T14:40:03Z

src/validation.cpp

@@ -1446,8 +1446,12 @@ bool UndoWriteToDisk(const CBlockUndo& blockundo, CDiskBlockPos& pos, const uint
    return true;
 }

-bool UndoReadFromDisk(CBlockUndo& blockundo, const CDiskBlockPos& pos, const uint256& hashBlock)
+bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex *pindex)


I think UndoReadFromDisk can be static.

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

jtimon · 2017-04-27T14:40:59Z

src/validation.cpp

+        CDiskBlockPos _pos;
+        if (!FindUndoPos(state, pindex->nFile, _pos, ::GetSerializeSize(blockundo, SER_DISK, CLIENT_VERSION) + 40))
+            return error("ConnectBlock(): FindUndoPos failed");
+        if (!UndoWriteToDisk(blockundo, _pos, pindex->pprev->GetBlockHash(), chainparams.MessageStart()))


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

jtimon · 2017-04-27T14:46:14Z

src/validation.cpp

+            return false;
+        }
+    };
+};


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

jtimon · 2017-04-27T14:56:18Z

src/validation.cpp

-                AbortNode(state, "Failed to write block");
+        CDiskBlockPos blockPos = WriteBlockToDisk(block, pindex->nHeight, chainparams, dbp);
+        if (blockPos.IsNull()) {
+            state.Error("AcceptBlock(): Failed to find position to write new block to disk");


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

jtimon · 2017-04-27T14:56:51Z

src/validation.cpp

+    if (dbp != NULL)
+        blockPos = *dbp;
+    if (!FindBlockPos(blockPos, nBlockSize+8, nHeight, block.GetBlockTime(), dbp != NULL)) {
+        error("AcceptBlock(): FindBlockPos failed");


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

jtimon · 2017-04-27T15:04:14Z

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

not-expert utACK aside from fTxIndex question

instagibbs · 2017-04-27T14:40:47Z

src/validation.cpp


+    if (!pindex->IsValid(BLOCK_VALID_SCRIPTS)) {


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

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

instagibbs · 2017-04-27T14:45:57Z

src/validation.cpp

+        pos.nTxOffset += ::GetSerializeSize(*tx, SER_DISK, CLIENT_VERSION);
+    }
+
+    if (fTxIndex)


Why do any of this if fTxIndex is false?

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

TheBlueMatt · 2017-04-30T18:51:11Z

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

ryanofsky

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.

ryanofsky · 2017-05-02T17:28:48Z

src/validation.cpp

 {
+    CDiskBlockPos pos = pindex->GetUndoPos();
+    if (pos.IsNull())
+        return error("%s: Undo data not available", __func__);


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"

Reverted the string change.

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.

ryanofsky · 2017-05-02T17:38:25Z

src/validation.cpp

@@ -1624,6 +1624,43 @@ void static FlushBlockFile(bool fFinalize = false)

 bool FindUndoPos(CValidationState &state, int nFile, CDiskBlockPos &pos, unsigned int nAddSize);

+static bool WriteUndoDataForBlock(const CBlockUndo& blockundo, CValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams)


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

MaybeWriteUndoDataForBlock might be a more descriptive name.

ryanofsky · 2017-05-02T17:46:56Z

src/validation.cpp

+    vPos.reserve(block.vtx.size());
+    for (const CTransactionRef& tx : block.vtx)
+    {
+        vPos.push_back(std::make_pair(tx->GetHash(), pos));


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().

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.

ryanofsky · 2017-05-02T18:13:10Z

src/validation.cpp

@@ -98,6 +98,7 @@ class CChainState {
    bool Load(CBlockTreeDB& blocktree);

 private:
+    /** Create a new block index entry for a given block hash */


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

Oh, no idea how that came about, thanks.

ryanofsky · 2017-05-02T18:15:13Z

src/validation.cpp

+ *
+ * It generally provides access to the current chain tree, as well as functions
+ * to provide it new data, which it will appropriately validate and incorporate
+ * in its state as neccessary.


In commit "Create initial CChainState to hold chain state information"

Spelling "necessary"

ryanofsky · 2017-05-02T18:26:48Z

src/validation.cpp

 {
-    if (!pblocktree->LoadBlockIndexGuts(InsertBlockIndex))
+    if (!blocktree.LoadBlockIndexGuts([this](uint256 hash){ return this->InsertBlockIndex(hash); }))


In commit "Create initial CChainState to hold chain state information"

Probably should pass hash by const reference.

Indeed, not actually 100% sure why that compiled...

ryanofsky · 2017-05-02T18:30:38Z

src/validation.cpp

+    std::set<CBlockIndex*, CBlockIndexWorkComparator> setBlockIndexCandidates;
+    CBlockIndex *pindexBestInvalid;
+
+    bool Load(CBlockTreeDB& blocktree);


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.

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.

ryanofsky · 2017-05-02T18:55:41Z

src/validation.cpp

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

+    CheckBlockIndex(chainparams.GetConsensus());


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.

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

ryanofsky · 2017-05-02T19:08:09Z

src/validation.cpp

+    CBlockIndex *pindex = AddToBlockIndex(block);
+    CValidationState state;
+    if (!ReceivedBlockTransactions(block, state, pindex, blockPos, consensusParams)) {
+        return error("LoadBlockIndex(): genesis block not accepted");


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

LoadBlockIndex should be ReceiveGenesisBlock or __func__ now.

ryanofsky · 2017-05-02T19:16:36Z

src/validation.cpp

+
+    bool ReceiveGenesisBlock(const CBlock &block, const CDiskBlockPos& blockPos, const Consensus::Params& consensusParams);
+
+    void UnloadBlockIndex();


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)

Changed Load to LoadBlockIndex instead.

TheBlueMatt · 2017-05-03T00:21:58Z

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 · 2017-05-03T13:48:17Z

utACK 120743a83f26923cd9ef4db068fd6c9b8b8ea4dd

TheBlueMatt · 2017-06-06T01:34:45Z

Rebased.

ryanofsky

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

ryanofsky · 2017-06-08T20:09:40Z

src/validation.cpp

 {
+    CDiskBlockPos pos = pindex->GetUndoPos();
+    if (pos.IsNull())
+        return error("%s: Undo data not available", __func__);


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.

ryanofsky · 2017-06-08T20:14:15Z

src/validation.cpp

+ *
+ * Eventually, the API here is targeted at being exposed externally as a
+ * consumable libconsensus library, so any functions added must only call
+ * external functions via callbacks, or to read/write data from disk.


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.

TheBlueMatt · 2017-06-09T17:26:07Z

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

sipa · 2017-06-13T00:00:22Z

Nice, concept ACK

ryanofsky

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

ryanofsky

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.

ryanofsky · 2017-07-25T17:33:28Z

src/validation.cpp

+ *
+ * Eventually, the API here is targeted at being exposed externally as a
+ * consumable libconsensus library, so any functions added must only call
+ * external functions via callbacks, or to read/write data from disk.


Thread #10279 (comment)

Newer comment got reverted somehow. It was:

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

TheBlueMatt · 2017-08-15T21:13:15Z

Rebased properly this time, with the various 0.15 changes.

ryanofsky

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

ryanofsky · 2017-08-16T14:40:23Z

src/validation.cpp


+    if (!pindex->IsValid(BLOCK_VALID_SCRIPTS)) {
        pindex->RaiseValidity(BLOCK_VALID_SCRIPTS);


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.

ryanofsky · 2017-08-16T15:11:39Z

src/validation.cpp

+    CDiskBlockPos blockPos;
+    if (dbp != nullptr)
+        blockPos = *dbp;
+    if (!FindBlockPos(blockPos, nBlockSize+8, nHeight, block.GetBlockTime(), dbp != nullptr)) {


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.

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).

ryanofsky · 2017-08-16T15:16:15Z

src/validation.cpp

@@ -3103,6 +3103,25 @@ bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, CValidatio
 }

 /** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
+static CDiskBlockPos WriteBlockToDisk(const CBlock& block, int nHeight, const CChainParams& chainparams, const CDiskBlockPos* dbp) {


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.

SaveBlockToDisk it is, that is much less confusing.

ryanofsky · 2017-08-16T15:26:08Z

src/validation.cpp

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


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.

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).

ryanofsky · 2017-08-16T15:37:28Z

src/validation.cpp

+ * consumable libconsensus library, so any functions added must only call
+ * other class member functions, pure functions in other parts of the consensus
+ * library, callbacks, or read/write-to-disk functions (eventually this will
+ * also be via callbacks).


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.

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.

ryanofsky · 2017-08-16T15:44:27Z

src/validation.cpp

+    CChain chainActive;
+    BlockMap mapBlockIndex;
+    std::multimap<CBlockIndex*, CBlockIndex*> mapBlocksUnlinked;
+    CBlockIndex *pindexBestInvalid = nullptr;


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).

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.

ryanofsky · 2017-08-16T15:46:43Z

src/validation.cpp

+
+
+    bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs, const CChainParams& params);
+} chainstate;


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

Maybe g_chainstate

ryanofsky · 2017-08-16T16:05:16Z

src/validation.cpp

+    return true;
+}
+
+bool RewindBlockIndex(const CChainParams& params) {


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.

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.

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

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.

ryanofsky · 2017-08-16T16:08:55Z

src/validation.cpp

 void static UpdateTip(CBlockIndex *pindexNew, const CChainParams& chainParams) {
-    chainActive.SetTip(pindexNew);


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.

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 · 2017-10-12T17:36:09Z

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

TheBlueMatt · 2017-10-13T23:29:10Z

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

TheBlueMatt · 2017-12-04T14:40:05Z

Rebased.

laanwj · 2017-12-12T13:36:42Z

utACK 22fddde

@sipa

…er step towards clarifying internal interfaces 22fddde Avoid calling GetSerializeSize on each tx in a block if !fTxIndex (Matt Corallo) 2862aca Move some additional variables into CChainState private (Matt Corallo) fd4d80a Create initial CChainState to hold chain state information (Matt Corallo) e104f0f Move block writing out of AcceptBlock (Matt Corallo) 50701ba Move txindex/undo data disk location stuff out of ConnectBlock (Matt Corallo) 93a34cf Make DisconnectBlock unaware of where undo data resides on disk (Matt Corallo) Pull request description: 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). Tree-SHA512: 69b8ec191b36b19c9492b4dee74c8057621fb6ec98ad219e8da0b2ed5c3ad711b10b5af9ff1117e8807ccf88918eeeab573be8448baecc9a59f099c53095985b

theuni · 2017-12-12T19:45:34Z

post-merge utACK 22fddde. Very nice!

@sipa

…e another step towards clarifying internal interfaces 22fddde Avoid calling GetSerializeSize on each tx in a block if !fTxIndex (Matt Corallo) 2862aca Move some additional variables into CChainState private (Matt Corallo) fd4d80a Create initial CChainState to hold chain state information (Matt Corallo) e104f0f Move block writing out of AcceptBlock (Matt Corallo) 50701ba Move txindex/undo data disk location stuff out of ConnectBlock (Matt Corallo) 93a34cf Make DisconnectBlock unaware of where undo data resides on disk (Matt Corallo) Pull request description: 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). Tree-SHA512: 69b8ec191b36b19c9492b4dee74c8057621fb6ec98ad219e8da0b2ed5c3ad711b10b5af9ff1117e8807ccf88918eeeab573be8448baecc9a59f099c53095985b

@sipa

…e another step towards clarifying internal interfaces 22fddde Avoid calling GetSerializeSize on each tx in a block if !fTxIndex (Matt Corallo) 2862aca Move some additional variables into CChainState private (Matt Corallo) fd4d80a Create initial CChainState to hold chain state information (Matt Corallo) e104f0f Move block writing out of AcceptBlock (Matt Corallo) 50701ba Move txindex/undo data disk location stuff out of ConnectBlock (Matt Corallo) 93a34cf Make DisconnectBlock unaware of where undo data resides on disk (Matt Corallo) Pull request description: 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). Tree-SHA512: 69b8ec191b36b19c9492b4dee74c8057621fb6ec98ad219e8da0b2ed5c3ad711b10b5af9ff1117e8807ccf88918eeeab573be8448baecc9a59f099c53095985b

@sipa

…e another step towards clarifying internal interfaces 22fddde Avoid calling GetSerializeSize on each tx in a block if !fTxIndex (Matt Corallo) 2862aca Move some additional variables into CChainState private (Matt Corallo) fd4d80a Create initial CChainState to hold chain state information (Matt Corallo) e104f0f Move block writing out of AcceptBlock (Matt Corallo) 50701ba Move txindex/undo data disk location stuff out of ConnectBlock (Matt Corallo) 93a34cf Make DisconnectBlock unaware of where undo data resides on disk (Matt Corallo) Pull request description: 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). Tree-SHA512: 69b8ec191b36b19c9492b4dee74c8057621fb6ec98ad219e8da0b2ed5c3ad711b10b5af9ff1117e8807ccf88918eeeab573be8448baecc9a59f099c53095985b

@sipa

* Merge bitcoin#10279: Add a CChainState class to validation.cpp to take another step towards clarifying internal interfaces 22fddde Avoid calling GetSerializeSize on each tx in a block if !fTxIndex (Matt Corallo) 2862aca Move some additional variables into CChainState private (Matt Corallo) fd4d80a Create initial CChainState to hold chain state information (Matt Corallo) e104f0f Move block writing out of AcceptBlock (Matt Corallo) 50701ba Move txindex/undo data disk location stuff out of ConnectBlock (Matt Corallo) 93a34cf Make DisconnectBlock unaware of where undo data resides on disk (Matt Corallo) Pull request description: 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). Tree-SHA512: 69b8ec191b36b19c9492b4dee74c8057621fb6ec98ad219e8da0b2ed5c3ad711b10b5af9ff1117e8807ccf88918eeeab573be8448baecc9a59f099c53095985b * fix Signed-off-by: Pasta <pasta@dashboost.org> * fix Signed-off-by: Pasta <pasta@dashboost.org> * More of 10279 Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com> Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 8db57b0 to 0acb237 Compare April 26, 2017 02:19

fanquake added the Validation label Apr 26, 2017

jtimon reviewed Apr 27, 2017

View reviewed changes

TheBlueMatt mentioned this pull request Apr 27, 2017

Cache full script execution results in addition to signatures #10192

Merged

jtimon reviewed Apr 27, 2017

View reviewed changes

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 0acb237 to 9c7732d Compare April 27, 2017 14:50

jtimon reviewed Apr 27, 2017

View reviewed changes

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 9c7732d to 6ef97ed Compare April 27, 2017 15:00

instagibbs approved these changes Apr 27, 2017

View reviewed changes

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 6ef97ed to 6cd32b4 Compare April 27, 2017 15:37

jtimon mentioned this pull request Apr 28, 2017

pass Consensus::Params& to functions in validation.cpp and make them static #10201

Merged

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 6cd32b4 to c431484 Compare April 30, 2017 18:50

ryanofsky reviewed May 2, 2017

View reviewed changes

TheBlueMatt force-pushed the 2016-12-cconsensus branch 2 times, most recently from df00f69 to 120743a Compare May 3, 2017 00:20

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 120743a to 895c0bd Compare June 6, 2017 01:34

ryanofsky reviewed Jun 8, 2017

View reviewed changes

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 895c0bd to 5f2135f Compare June 9, 2017 17:25

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 5f2135f to 5eb9eed Compare June 9, 2017 17:28

ryanofsky reviewed Jun 13, 2017

View reviewed changes

TheBlueMatt force-pushed the 2016-12-cconsensus branch 2 times, most recently from 11b06ee to 1e0bef6 Compare June 28, 2017 15:41

TheBlueMatt mentioned this pull request Jun 28, 2017

Make mapBlockIndex and chainActive and all CBlockIndex*es const outside of validation/CChainState #10692

Closed

ryanofsky requested changes Jul 25, 2017

View reviewed changes

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 1e0bef6 to 3b78b2d Compare August 15, 2017 21:12

ryanofsky reviewed Aug 16, 2017

View reviewed changes

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 3b78b2d to 6a2cce2 Compare October 13, 2017 23:29

TheBlueMatt mentioned this pull request Nov 7, 2017

Avoid lock: Call FlushStateToDisk(...) regardless of fCheckForPruning #11617

Merged

TheBlueMatt added 6 commits December 4, 2017 09:33

Make DisconnectBlock unaware of where undo data resides on disk

93a34cf

Move txindex/undo data disk location stuff out of ConnectBlock

50701ba

Move block writing out of AcceptBlock

e104f0f

Create initial CChainState to hold chain state information

fd4d80a

Move some additional variables into CChainState private

2862aca

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

22fddde

TheBlueMatt force-pushed the 2016-12-cconsensus branch from 6a2cce2 to 22fddde Compare December 4, 2017 14:39

laanwj merged commit 22fddde into bitcoin:master Dec 12, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -1624,6 +1624,43 @@ void static FlushBlockFile(bool fFinalize = false)

		bool FindUndoPos(CValidationState &state, int nFile, CDiskBlockPos &pos, unsigned int nAddSize);

		static bool WriteUndoDataForBlock(const CBlockUndo& blockundo, CValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams)


		bool ReceiveGenesisBlock(const CBlock &block, const CDiskBlockPos& blockPos, const Consensus::Params& consensusParams);

		void UnloadBlockIndex();


		if (!pindex->IsValid(BLOCK_VALID_SCRIPTS)) {
		pindex->RaiseValidity(BLOCK_VALID_SCRIPTS);



		bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs, const CChainParams& params);
		} chainstate;

		void static UpdateTip(CBlockIndex *pindexNew, const CChainParams& chainParams) {
		chainActive.SetTip(pindexNew);

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

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

Uh oh!

Conversation

TheBlueMatt commented Apr 26, 2017

Uh oh!

TheBlueMatt commented Apr 26, 2017

Uh oh!

jtimon Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Apr 27, 2017

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Apr 30, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon Apr 27, 2017 •

edited

Loading