refactor: share blockmetadata with BlockManager #16194

jamesob · 2019-06-12T14:48:36Z

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

Under an assumeutxo model, we have multiple CChainState instances in use at once in order to support background validation. Currently, each CChainState instance has its own mapBlockIndex, a collection of linked block headers, in addition to a few other data structures that are related to maintenance of the block tree but not necessarily to any given chainstate.

In order to avoid duplicating this data across chainstates, this change moves chainstate-agnostic block metadata (and related behavior) into a class, BlockManager. Chainstates are parameterized with a reference to a blockmanager instance and in practice they share the same instance.

Most of this change is conceptually move-only, though the diff is somewhat muddled. The first commit can be reviewed slightly more easily with --color-moved=dimmed_zebra. Admittedly, that commit is pretty unwieldy; I tried to split it up after the fact with git add --patch, but that was difficult because of git's inability to split hunks past a certain point. Some of the moves also ended up being obscured when done over separate commits.

ryanofsky · 2019-06-12T15:10:55Z

Concept ACK

that was difficult because of git's inability to split hunks past a certain point

First commit seems reasonable to me, and I wouldn't see a need to split it up further. But just for reference I'd recommend trying git add --patch's split and edit commands if you haven't used them before. The split command is good enough for splitting up changes most of the time, and when it isn't, the edit command is fallback that works really well.

s - split the current hunk into smaller hunks
e - manually edit the current hunk

DrahtBot · 2019-06-12T15:19:14Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16362 (gui: Bilingual translation by hebasto)
#16324 (Get cs_main out of the critical path in ProcessMessages by TheBlueMatt)
#16323 (Call ProcessNewBlock() asynchronously by TheBlueMatt)
#16224 (gui: Bilingual GUI error messages by hebasto)
#15191 (validation: Add missing cs_{LastBlockFile,nBlockSequenceId} locks in PruneAndFlush() and UnloadBlockIndex(). Add missing locking annotations. by practicalswift)

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.

promag

Concept ACK another man to the gang. Overall changes look good to me but will look closely.

src/validation.cpp

src/validation.h

jamesob · 2019-06-14T17:44:44Z

Thanks for the look, guys. I've made @promag's suggested changes and squashed the second commit into the first.

maflcko

Some style questions

src/validation.cpp

maflcko · 2019-06-17T21:46:08Z

src/validation.cpp

-     * Pruned nodes may have entries where B is missing data.
-     */
-    std::multimap<CBlockIndex*, CBlockIndex*>& mapBlocksUnlinked = ::ChainstateActive().mapBlocksUnlinked;
+    CBlockIndex* pindexBestInvalid = nullptr;


Why is this changed?

Ah sorry, this was a little confusing. Because pindexBestInvalid is only used in validation.cpp, I removed it as a member of CChainState (though in this revision I forgot to actually remove the declaration - willfix) since it doesn't need to be a part of that interface. There is a change in name only, because ::ChainstateActive().pindexBestInvalid is nullptr at the time of this declaration.

Also worth pointing out that we need to touch this piece of data because it is used by both CChainState methods and BlockManager methods.

Ok, I see that you now removed the CChainstate::pindexBestInvalid, but it could make sense to split this change into a separate commit the next time you have to touch this pull?

In commit "refactoring: move block metadata structures into BlockManager" (3a8f92e)

pindexBestInvalid just seems like it should be a BlockManager member. Or am I missing something?

pindexBestInvalid just seems like it should be a BlockManager member. Or am I missing something?

In theory we could, the only problem here is that (i) g_blockman is retired in later commits and (ii) pindexBestInvalid is used in non-method functions like CheckForkWarningConditions, so essentially there'd be no way to reference m_blockman.pindexBestInvalid in functions like that without moving them onto CChainState. So instead of creating extra diff, I just figured keeping pindexBestInvalid private to validation.cpp made sense. Let me know if you think it'd be better as a member of BlockManager.

re: #16194 (comment)

Thanks for explaining. It would be nice to clean up and document these free floating global variables (pindexBestInvalid, pindexBestForkTip, pindexBestForkBase), giving them g_ prefixes or moving them to appropriate classes so they don't look like local variables. It's beyond the scope of this PR, though.

FWIW, I don't see later commits interfering here. It looks like there still is a single blockman instance, just called g_chainman.m_blockman instead of g_blockman.

src/validation.cpp

src/validation.h

jamesob · 2019-06-19T15:56:35Z

Rebased and incorporated @MarcoFalke's feedback

maflcko

Just two dev-doc-nits

maflcko · 2019-06-19T17:37:25Z

src/validation.cpp

-     * Pruned nodes may have entries where B is missing data.
-     */
-    std::multimap<CBlockIndex*, CBlockIndex*>& mapBlocksUnlinked = ::ChainstateActive().mapBlocksUnlinked;
+    CBlockIndex* pindexBestInvalid = nullptr;


Ok, I see that you now removed the CChainstate::pindexBestInvalid, but it could make sense to split this change into a separate commit the next time you have to touch this pull?

src/init.cpp

ryanofsky

utACK 1e33f16. Code changes all look correct. Left some suggestions and feedback, but feel free to ignore it.

src/validation.cpp

ryanofsky · 2019-06-18T21:03:16Z

src/validation.cpp

 RecursiveMutex cs_main;

-BlockMap& mapBlockIndex = ::ChainstateActive().mapBlockIndex;
+BlockMap& mapBlockIndex = g_blockman.m_block_index;


In commit "refactoring: move block metadata structures into BlockManager" (04fa434)

Note to help review: this is temporary, removed in the next commit

src/validation.cpp

ryanofsky · 2019-06-19T21:44:04Z

src/validation.cpp

-     * Pruned nodes may have entries where B is missing data.
-     */
-    std::multimap<CBlockIndex*, CBlockIndex*>& mapBlocksUnlinked = ::ChainstateActive().mapBlocksUnlinked;
+    CBlockIndex* pindexBestInvalid = nullptr;


In commit "refactoring: move block metadata structures into BlockManager" (3a8f92e)

pindexBestInvalid just seems like it should be a BlockManager member. Or am I missing something?

src/validation.cpp

jamesob · 2019-06-25T14:58:37Z

Thanks for the good feedback, @MarcoFalke @ryanofsky. I've incorporated your changes.

jamesob · 2019-06-25T15:02:06Z

src/validation.cpp

    if (ppindex)
        *ppindex = pindex;

-    CheckBlockIndex(chainparams.GetConsensus());


Note to reviewers: this is moved to the two callsites below to avoid making reference to a specific CChainState in BlockManager.

As @sdaftuar pointed out in #16444 (comment) AcceptBlockHeader can return early, so we're now calling CheckBlockIndex(() more often than before.

ryanofsky

utACK e89e4f6. One changes since last review were various review suggestions discussed above, and splitting some small parts of the first commit into separate commits.

ryanofsky · 2019-06-26T20:16:23Z

src/validation.cpp

-     * Pruned nodes may have entries where B is missing data.
-     */
-    std::multimap<CBlockIndex*, CBlockIndex*>& mapBlocksUnlinked = ::ChainstateActive().mapBlocksUnlinked;
+    CBlockIndex* pindexBestInvalid = nullptr;


re: #16194 (comment)

Thanks for explaining. It would be nice to clean up and document these free floating global variables (pindexBestInvalid, pindexBestForkTip, pindexBestForkBase), giving them g_ prefixes or moving them to appropriate classes so they don't look like local variables. It's beyond the scope of this PR, though.

FWIW, I don't see later commits interfering here. It looks like there still is a single blockman instance, just called g_chainman.m_blockman instead of g_blockman.

maflcko · 2019-07-12T15:45:00Z

ACK 682a1d0

Show signature and timestamp

Signature:

Timestamp of file with hash 5b30b7a598d04185a0efdbef0b0f6e39648bf992b0c23151e41ad9445e741517 -

ariard

utACK 682a1d0. Most of the changes are move-only, with main problem being to avoid creating circular dependencies between BlockManager and CChainState. Tested, comments are mostly nits, feel free to ignore them

ariard · 2019-07-15T17:47:21Z

src/validation.h

+    std::set<CBlockIndex*> m_failed_blocks;
+
+    /**
+     * All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions.


nit: comment has just been moved but maybe you could take opportunity to explain better what m_blocks_unlinked is useful for, something like "a child block B may receive its transactions before ancestor A which means B nChainTx is going to be inaccurate until we received txn for every one of its ancestors. When we accept A, update B nChainTx to accurate number of transactions"

Agree, but don't want to invalidate the move-only nature for this change. Would be a great follow-up if you're on the hunt for PR ideas :).

ariard · 2019-07-15T18:47:02Z

src/validation.cpp

-        // Check whether this block is in mapBlocksUnlinked.
-        std::pair<std::multimap<CBlockIndex*,CBlockIndex*>::iterator,std::multimap<CBlockIndex*,CBlockIndex*>::iterator> rangeUnlinked = mapBlocksUnlinked.equal_range(pindex->pprev);
+        // Check whether this block is in m_blocks_unlinked.
+        std::pair<std::multimap<CBlockIndex*,CBlockIndex*>::iterator,std::multimap<CBlockIndex*,CBlockIndex*>::iterator> rangeUnlinked = m_blockman.m_blocks_unlinked.equal_range(pindex->pprev);


You may use auto there, compiler can do type inference?

ariard · 2019-07-15T18:59:57Z

src/validation.h

+    /** Clear all data members. */
+    void Unload() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+
+    CBlockIndex* AddToBlockIndex(const CBlockHeader& block) EXCLUSIVE_LOCKS_REQUIRED(cs_main);


nit: now than AddToBlockIndex is a public method, it could be commented like "Create an index entry for given header with minimal metadatas. If header shows most-work it becomes best-tracked-header"

Good idea - if I have to rebase this PR for some other reason I'll add it.

ariard · 2019-07-15T19:13:26Z

src/validation.h

+    /**
+     * Load the blocktree off disk and into memory. Populate certain metadata
+     * per index entry (nStatus, nChainWork, nTimeMax, etc.) as well as peripheral
+     * collections like setDirtyBlockIndex.


hmmm maybe setDirtyBlockIndex could be part of BlockManager in future commits given this set of block index is also shared between chainstates? And would let implement batching managed by BlockManager

ariard · 2019-07-15T19:41:16Z

src/validation.cpp

    std::set<int> setDirtyFileInfo;
 } // anon namespace

+CBlockIndex* LookupBlockIndex(const uint256& hash)


It's unclear reading commits messages what's the long term destination of g_blockman is. Do other parts of the code calling LookupBlockIndex will always rely on a global BlockManager or should they access to it through their instance of CChainstate after future refactoring ?

Eventually, a single BlockManager object will be managed and shared to all created CChainState objects by something called ChainstateManager, to be introduced later (df81dc4). The single global block index will be accessed through the chainstate manager.

682a1d0 refactoring: remove mapBlockIndex global (James O'Beirne) 55d525a refactoring: make pindexBestInvalid internal to validation.cpp (James O'Beirne) 4ed55df refactoring: add block_index_candidates arg to LoadBlockIndex (James O'Beirne) 613c46f refactoring: move block metadata structures into BlockManager (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11): Parent PR: #15606 Issue: #15605 Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal --- Under an assumeutxo model, we have multiple CChainState instances in use at once in order to support background validation. Currently, each CChainState instance has its own mapBlockIndex, a collection of linked block headers, in addition to a few other data structures that are related to maintenance of the block tree but not necessarily to any given chainstate. In order to avoid duplicating this data across chainstates, this change moves chainstate-agnostic block metadata (and related behavior) into a class, `BlockManager`. Chainstates are parameterized with a reference to a blockmanager instance and in practice they share the same instance. Most of this change is conceptually move-only, though the diff is somewhat muddled. The first commit can be reviewed slightly more easily with `--color-moved=dimmed_zebra`. Admittedly, that commit is pretty unwieldy; I tried to split it up after the fact with `git add --patch`, but that was difficult because of git's inability to split hunks past a certain point. Some of the moves also ended up being obscured when done over separate commits. ACKs for top commit: MarcoFalke: ACK 682a1d0 ryanofsky: utACK 682a1d0, only changes since last review were rebase and fixing conflict on a moved line ariard: utACK 682a1d0. Most of the changes are move-only, with main problem being to avoid creating circular dependencies between `BlockManager` and `CChainState`. Tested, comments are mostly nits, feel free to ignore them Tree-SHA512: 738d8d06539ba53acf4bd2d48ae000473e645bbc4e63d798d55d247a4d5a4f781b73538ed590f6407be9ab402ea9d395570ea20bff0a4b9ce747bcc1600c5108

maflcko · 2019-07-12T15:17:38Z

src/validation.cpp

+BlockManager g_blockman;
+} // anon namespace
+
+static CChainState g_chainstate(g_blockman);


style-nit in commit 613c46f refactoring: move block metadata structures into BlockManager:

For consistency should make both static or put both into a namespace.

maflcko · 2019-07-12T15:20:30Z

src/validation.cpp

+    m_failed_blocks.clear();
+    m_blocks_unlinked.clear();
+
+    for (const BlockMap::value_type& entry : m_block_index) {


style-nit: in commit 613c46f refactoring: move block metadata structures into BlockManager:

const auto& entry does the same, less verbose

maflcko · 2019-07-12T16:19:16Z

src/init.cpp

        "each level includes the checks of the previous levels "
        "(0-4, default: %u)", DEFAULT_CHECKLEVEL), true, OptionsCategory::DEBUG_TEST);
-    gArgs.AddArg("-checkblockindex", strprintf("Do a full consistency check for mapBlockIndex, setBlockIndexCandidates, ::ChainActive() and mapBlocksUnlinked occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), true, OptionsCategory::DEBUG_TEST);
+    gArgs.AddArg("-checkblockindex", strprintf("Do a full consistency check for the block tree, setBlockIndexCandidates, ::ChainActive() and mapBlocksUnlinked occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), true, OptionsCategory::DEBUG_TEST);


nit: mapBlocksUnlinked does no longer exist

rebroad · 2021-04-27T14:11:21Z

src/validation.cpp

            CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast
-            if (!::ChainstateActive().AcceptBlockHeader(header, state, chainparams, &pindex)) {
+            bool accepted = g_blockman.AcceptBlockHeader(header, state, chainparams, &pindex);
+            ::ChainstateActive().CheckBlockIndex(chainparams.GetConsensus());


is this necessary when accepted is false?

rebroad · 2021-04-27T14:11:44Z

src/validation.cpp


-    if (!AcceptBlockHeader(block, state, chainparams, &pindex))
+    bool accepted_header = m_blockman.AcceptBlockHeader(block, state, chainparams, &pindex);
+    CheckBlockIndex(chainparams.GetConsensus());


is this necessary when accepted_header is false?

DrahtBot added Refactoring RPC/REST/ZMQ Tests UTXO Db and Indexes Validation Wallet labels Jun 12, 2019

promag reviewed Jun 12, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

promag reviewed Jun 12, 2019

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

fanquake removed RPC/REST/ZMQ Tests UTXO Db and Indexes Wallet labels Jun 12, 2019

jamesob force-pushed the 2019-06-au-blockman branch 2 times, most recently from ee815b1 to 66b2a2c Compare June 14, 2019 17:07

maflcko reviewed Jun 17, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jun 18, 2019

jamesob force-pushed the 2019-06-au-blockman branch 2 times, most recently from cf9af76 to 1e33f16 Compare June 19, 2019 15:48

DrahtBot removed the Needs rebase label Jun 19, 2019

maflcko reviewed Jun 19, 2019

View reviewed changes

ryanofsky approved these changes Jun 19, 2019

View reviewed changes

jamesob force-pushed the 2019-06-au-blockman branch from 1e33f16 to e89e4f6 Compare June 24, 2019 19:58

jamesob commented Jun 25, 2019

View reviewed changes

ryanofsky approved these changes Jun 26, 2019

View reviewed changes

DrahtBot removed the Needs rebase label Jul 8, 2019

ariard approved these changes Jul 15, 2019

View reviewed changes

laanwj merged commit 682a1d0 into bitcoin:master Jul 16, 2019

maflcko reviewed Jul 16, 2019

View reviewed changes

jamesob mentioned this pull request Jul 24, 2019

validation: Run CheckBlockIndex only on success for now #16453

Closed

sdaftuar mentioned this pull request Jul 24, 2019

Assertion `setBlockIndexCandidates.count(pindex)' failed #16444

Closed

maflcko mentioned this pull request Sep 8, 2019

[WIP] [FEATURE] RPC: Add command for pruning past ChainTip #16827

Closed

rebroad reviewed Apr 27, 2021

View reviewed changes

michaelfolkson mentioned this pull request Jun 3, 2021

doc: Various validation doc fixups #22121

Merged

jamesob mentioned this pull request Aug 5, 2021

assumeutxo #15606

Closed

18 tasks

kwvg mentioned this pull request Oct 9, 2021

merge bitcoin#15948, #15976, #16194...: assumeutxo project backports (part 1) dashpay/dash#4498

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

refactor: share blockmetadata with BlockManager #16194

refactor: share blockmetadata with BlockManager #16194

Uh oh!

Conversation

jamesob commented Jun 12, 2019

Uh oh!

ryanofsky commented Jun 12, 2019

Uh oh!

DrahtBot commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jamesob commented Jun 14, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jamesob commented Jun 19, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jamesob commented Jun 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 12, 2019

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DrahtBot commented Jun 12, 2019 •

edited

Loading