refactor: share blockmetadata with BlockManager

jamesob commented at 2:48 pm on June 12, 2019: member

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 commented at 3:10 pm on June 12, 2019: member

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.

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

DrahtBot added the label Refactoring on Jun 12, 2019

DrahtBot added the label RPC/REST/ZMQ on Jun 12, 2019

DrahtBot added the label Tests on Jun 12, 2019

DrahtBot added the label UTXO Db and Indexes on Jun 12, 2019

DrahtBot added the label Validation on Jun 12, 2019

DrahtBot added the label Wallet on Jun 12, 2019

DrahtBot commented at 3:19 pm on June 12, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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 commented at 3:47 pm on June 12, 2019: member

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

in src/validation.cpp:81 in 4368dea384 outdated

76@@ -77,7 +77,9 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex *pa, const CBlockIn
77     return false;
78 }
79 
80-CChainState g_chainstate;
81+BlockManager g_blockman;

promag commented at 3:48 pm on June 12, 2019:

7064f3045f41731dd8044cb6ef4277daedb06a2c

static or in a anonymous namespace?

in src/validation.h:624 in 4368dea384 outdated

629 
630+    /** Check whether we are doing an initial block download (synchronizing from disk or network) */
631+    bool IsInitialBlockDownload() const;
632+
633+    //! @returns the map of blocks that this chainstate is aware of.
634+    BlockMap& BlockIndex();

promag commented at 4:03 pm on June 12, 2019:

Can be const?

0BlockMap& BlockIndex() const;

fanquake removed the label RPC/REST/ZMQ on Jun 12, 2019

fanquake removed the label Tests on Jun 12, 2019

fanquake removed the label UTXO Db and Indexes on Jun 12, 2019

fanquake removed the label Wallet on Jun 12, 2019

jamesob force-pushed on Jun 14, 2019

jamesob commented at 5:44 pm on June 14, 2019: member

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

in src/validation.cpp:84 in 04fa434aab outdated

76@@ -77,7 +77,11 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex *pa, const CBlockIn
77     return false;
78 }
79 
80-CChainState g_chainstate;
81+namespace {
82+    BlockManager g_blockman;
83+} // anon namespace
84+
85+CChainState g_chainstate(g_blockman);

MarcoFalke commented at 9:42 pm on June 17, 2019:

Should be static

ryanofsky commented at 9:01 pm on June 18, 2019:

In commit “refactoring: move block metadata structures into BlockManager” (04fa434aabcc9d02bf1f0ceaa3ee4f099642a27a)

Should be static

Or equivalently, just move it into the anonymous namespace above.

in src/validation.cpp:131 in 04fa434aab outdated

134-
135-    /** All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions.
136-     * Pruned nodes may have entries where B is missing data.
137-     */
138-    std::multimap<CBlockIndex*, CBlockIndex*>& mapBlocksUnlinked = ::ChainstateActive().mapBlocksUnlinked;
139+    CBlockIndex* pindexBestInvalid = nullptr;

MarcoFalke commented at 9:46 pm on June 17, 2019:

Why is this changed?

jamesob commented at 3:27 pm on June 19, 2019:

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.

jamesob commented at 3:28 pm on June 19, 2019:

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

MarcoFalke commented at 5:37 pm on June 19, 2019:

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?

ryanofsky commented at 9:44 pm on June 19, 2019:

In commit “refactoring: move block metadata structures into BlockManager” (3a8f92e51f60287492d1a1bce5e5e58ae029bb67)

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

jamesob commented at 5:49 pm on June 24, 2019:

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.

ryanofsky commented at 8:16 pm on June 26, 2019:

re: #16194 (review)

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.

in src/validation.cpp:3530 in 04fa434aab outdated

3528-            // mapBlocksUnlinked or setBlockIndexCandidates.
3529-            std::pair<std::multimap<CBlockIndex*, CBlockIndex*>::iterator, std::multimap<CBlockIndex*, CBlockIndex*>::iterator> range = mapBlocksUnlinked.equal_range(pindex->pprev);
3530+            // m_blocks_unlinked or setBlockIndexCandidates.
3531+            std::pair<std::multimap<CBlockIndex*, CBlockIndex*>::iterator,
3532+                std::multimap<CBlockIndex*, CBlockIndex*>::iterator> range =
3533+                    g_blockman.m_blocks_unlinked.equal_range(pindex->pprev);

MarcoFalke commented at 9:49 pm on June 17, 2019:

Could use const auto instead of splitting this into three lines.

jamesob commented at 4:25 pm on June 18, 2019:

Happy to do that, but thought that the explicit type declaration might be helpful since the value is so complicated. If you think const auto is better I’ll change it.

in src/validation.cpp:4056 in 04fa434aab outdated

4050@@ -4049,10 +4051,12 @@ void CChainState::EraseBlockData(CBlockIndex* index)
4051     setDirtyBlockIndex.insert(index);
4052     // Update indexes
4053     setBlockIndexCandidates.erase(index);
4054-    std::pair<std::multimap<CBlockIndex*, CBlockIndex*>::iterator, std::multimap<CBlockIndex*, CBlockIndex*>::iterator> ret = mapBlocksUnlinked.equal_range(index->pprev);
4055+    std::pair<std::multimap<CBlockIndex*, CBlockIndex*>::iterator,
4056+        std::multimap<CBlockIndex*, CBlockIndex*>::iterator> ret =
4057+            m_blockman.m_blocks_unlinked.equal_range(index->pprev);

MarcoFalke commented at 9:50 pm on June 17, 2019:

Same

in src/validation.h:492 in 04fa434aab outdated

492+
493+        for (const BlockMap::value_type& entry : m_block_index) {
494+            delete entry.second;
495+        }
496+
497+        m_block_index.clear();

MarcoFalke commented at 9:51 pm on June 17, 2019:

Implementation should probably stay in the cpp file, no?

in src/validation.h:554 in 04fa434aab outdated

549@@ -509,15 +550,24 @@ class CChainState {
550      */
551     mutable std::atomic<bool> m_cached_finished_ibd{false};
552 
553+    //! Reference to a BlockManager instance which itself is shared across all
554+    //! CChainState instances.

MarcoFalke commented at 9:52 pm on June 17, 2019:

Could explain why this is useful to use instead of the g_blockman.

in src/validation.h:627 in 04fa434aab outdated

632      * By default this only executes fully when using the Regtest chain; see: fCheckBlockIndex.
633      */
634     void CheckBlockIndex(const Consensus::Params& consensusParams);
635 
636+    /** Check whether we are doing an initial block download (synchronizing from disk or network) */
637+    bool IsInitialBlockDownload() const;

MarcoFalke commented at 9:54 pm on June 17, 2019:

Why is this moved? Looks like this is public before and after.

jamesob commented at 3:14 pm on June 19, 2019:

Good catch, not sure how that got moved.

MarcoFalke commented at 9:57 pm on June 17, 2019: member

Some style questions

DrahtBot added the label Needs rebase on Jun 18, 2019

in src/validation.cpp:81 in 04fa434aab outdated

76@@ -77,7 +77,11 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex *pa, const CBlockIn
77     return false;
78 }
79 
80-CChainState g_chainstate;
81+namespace {
82+    BlockManager g_blockman;

ryanofsky commented at 9:00 pm on June 18, 2019:

In commit “refactoring: move block metadata structures into BlockManager” (04fa434aabcc9d02bf1f0ceaa3ee4f099642a27a)

Shouldn’t indent within a namespace (could use clang-format-diff) https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

in src/validation.cpp:102 in 04fa434aab outdated

 98@@ -95,7 +99,7 @@ CChain& ChainActive() { return g_chainstate.m_chain; }
 99  */
100 RecursiveMutex cs_main;
101 
102-BlockMap& mapBlockIndex = ::ChainstateActive().mapBlockIndex;
103+BlockMap& mapBlockIndex = g_blockman.m_block_index;

ryanofsky commented at 9:03 pm on June 18, 2019:

In commit “refactoring: move block metadata structures into BlockManager” (04fa434aabcc9d02bf1f0ceaa3ee4f099642a27a)

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

in src/validation.cpp:3314 in 04fa434aab outdated

3303@@ -3304,7 +3304,7 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState&
3304     if (ppindex)
3305         *ppindex = pindex;
3306 
3307-    CheckBlockIndex(chainparams.GetConsensus());
3308+    ::ChainstateActive().CheckBlockIndex(chainparams.GetConsensus());

ryanofsky commented at 9:20 pm on June 18, 2019:

In commit “refactoring: move block metadata structures into BlockManager” (3a8f92e51f60287492d1a1bce5e5e58ae029bb67)

I think ideally BlockManager wouldn’t depend on CChainState, and especially not on a particular hardcoded instance of it. Since nothing except this one line seems to do this, and this is at the end of a function, I think it’d be good to delete this line and just call CheckBlockIndex directly from the two callers.

jamesob commented at 3:07 pm on June 24, 2019:

Yep, agree. Willfix.

jamesob force-pushed on Jun 19, 2019

DrahtBot removed the label Needs rebase on Jun 19, 2019

jamesob commented at 3:56 pm on June 19, 2019: member

Rebased and incorporated @MarcoFalke’s feedback

in src/init.cpp:491 in 1e33f16dd4 outdated

487@@ -488,7 +488,7 @@ void SetupServerArgs()
488         "and level 4 tries to reconnect the blocks, "
489         "each level includes the checks of the previous levels "
490         "(0-4, default: %u)", DEFAULT_CHECKLEVEL), true, OptionsCategory::DEBUG_TEST);
491-    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);
492+    gArgs.AddArg("-checkblockindex", strprintf("Do a full consistency check for BlockManager.m_block_index, setBlockIndexCandidates, ::ChainActive() and mapBlocksUnlinked occasionally. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), true, OptionsCategory::DEBUG_TEST);

MarcoFalke commented at 5:51 pm on June 19, 2019:

Call it “block tree index map” or something to not leak implementation details?

in src/init.cpp:1745 in 1e33f16dd4 outdated

1741@@ -1741,7 +1742,7 @@ bool AppInitMain(InitInterfaces& interfaces)
1742     //// debug print
1743     {
1744         LOCK(cs_main);
1745-        LogPrintf("mapBlockIndex.size() = %u\n", mapBlockIndex.size());
1746+        LogPrintf("m_block_index.size() = %u\n", ::ChainstateActive().BlockIndex().size());

MarcoFalke commented at 5:53 pm on June 19, 2019:

Same

MarcoFalke commented at 6:00 pm on June 19, 2019: member

Just two dev-doc-nits

in src/validation.cpp:3737 in 3a8f92e51f outdated

3733@@ -3734,7 +3734,7 @@ bool CChainState::LoadBlockIndex(const Consensus::Params& consensus_params, CBlo
3734             setDirtyBlockIndex.insert(pindex);
3735         }
3736         if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS) && (pindex->HaveTxsDownloaded() || pindex->pprev == nullptr))
3737-            setBlockIndexCandidates.insert(pindex);
3738+            ::ChainstateActive().setBlockIndexCandidates.insert(pindex);

ryanofsky commented at 10:04 pm on June 19, 2019:

In commit “refactoring: move block metadata structures into BlockManager” (3a8f92e51f60287492d1a1bce5e5e58ae029bb67)

This BlockManager -> ChainstateActive() dependency seems like it will make BlockManager harder to test independently and have confusing side effects. Would suggest reverting this line and adding an explicit std::set<CBlockIndex*, CBlockIndexWorkComparator>& setBlockIndexCandidates output parameter to this function. It’s only called one place, and I think it’d make the call site clearer, because otherwise you might not know calling this has any effect on the active chain.

in src/validation.cpp:1054 in 1e33f16dd4 outdated

1050@@ -1045,6 +1051,11 @@ bool CChainState::IsInitialBlockDownload() const
1051 
1052 static CBlockIndex *pindexBestForkTip = nullptr, *pindexBestForkBase = nullptr;
1053 
1054+BlockMap& CChainState::BlockIndex() const

ryanofsky commented at 10:17 pm on June 19, 2019:

In commit “refactoring: remove mapBlockIndex global” (1e33f16dd47c42a9f6099f6172ebd5f8fbb32ec8)

I’m probably just bikeshedding, but I think having this method is counterproductive and only obfuscates what’s going on. In places where this is called in CChainState methods it seems clearer to replace BlockIndex() calls with direct m_blockman.m_block_index references. In places where this is called globally, it would seem clearer to able to just write ::BlockIndex() instead of ::ChainstateActive().BlockIndex() since the block index doesn’t depend on the active chain, and because it would be more consistent with the existing ::LookupBlockIndex calls.

jamesob commented at 3:07 pm on June 24, 2019:

Yep, I agree with this. Willfix.

ryanofsky approved

ryanofsky commented at 10:28 pm on June 19, 2019: member

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

jamesob force-pushed on Jun 24, 2019

jamesob commented at 2:58 pm on June 25, 2019: member

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

in src/validation.cpp:3309 in e89e4f62b8 outdated

3310@@ -3300,8 +3311,6 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState&
3311     if (ppindex)
3312         *ppindex = pindex;
3313 
3314-    CheckBlockIndex(chainparams.GetConsensus());

jamesob commented at 3:02 pm on June 25, 2019:

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

Sjors commented at 12:55 pm on September 30, 2019:

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

ryanofsky approved

ryanofsky commented at 8:38 pm on June 26, 2019: member

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

DrahtBot added the label Needs rebase on Jul 3, 2019

refactoring: move block metadata structures into BlockManager

Separate out the management of chain-agnostic block metadata from any given
CChainState instance. This allows us to avoid duplicating data like
`mapBlockIndex` unnecessarily for multiple chainstates.

This also adds a CChainState constructor that accepts and sets m_blockman.
Ultimately this reference will point to a BlockMan instance that
is shared across CChainStates.

This commit can be decomposed into smaller commits if necessary.

613c46fe9e

refactoring: add block_index_candidates arg to LoadBlockIndex

Prevents BlockManager from having to reference ChainstateActive()
within one of its methods which improves encapsulation and makes
testing easier.

4ed55dfcd7

refactoring: make pindexBestInvalid internal to validation.cpp

There's no need to have this member live on CChainState since it's only used
in validation.cpp.

55d525ab90

refactoring: remove mapBlockIndex global

in lieu of ::BlockIndex().

682a1d0f20

jamesob force-pushed on Jul 8, 2019

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

ryanofsky approved

ryanofsky commented at 6:16 pm on July 8, 2019: member

utACK 682a1d0f2004d808b87b3106d0dfae547005e638, only changes since last review were rebase and fixing conflict on a moved line

DrahtBot removed the label Needs rebase on Jul 8, 2019

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

in src/validation.cpp:84 in 613c46fe9e outdated

76@@ -77,7 +77,11 @@ bool CBlockIndexWorkComparator::operator()(const CBlockIndex *pa, const CBlockIn
77     return false;
78 }
79 
80-static CChainState g_chainstate;
81+namespace {
82+BlockManager g_blockman;
83+} // anon namespace
84+
85+static CChainState g_chainstate(g_blockman);

MarcoFalke commented at 3:17 pm on July 12, 2019:

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

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

in src/validation.cpp:3763 in 613c46fe9e outdated

3755@@ -3752,9 +3756,20 @@ bool CChainState::LoadBlockIndex(const Consensus::Params& consensus_params, CBlo
3756     return true;
3757 }
3758 
3759+void BlockManager::Unload() {
3760+    m_failed_blocks.clear();
3761+    m_blocks_unlinked.clear();
3762+
3763+    for (const BlockMap::value_type& entry : m_block_index) {

MarcoFalke commented at 3:20 pm on July 12, 2019:

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

const auto& entry does the same, less verbose

MarcoFalke commented at 3:45 pm on July 12, 2019: member

ACK 682a1d0f2004d808b87b3106d0dfae547005e638

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 682a1d0f2004d808b87b3106d0dfae547005e638
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhwngwAq5VhfgfOv6FJFWvZc8NpFrYVMPW/S6u8qLBlZs3q2KZMT7j12l0MQRmX
 8qwGyP+CRJKsuJvYIwShRGHV5el/0t5hkTD8HUMlGgXSSkQvuEJCCweD2t1Y1y8Pm
 9Ayi3QErIRcS1ErtnP7dxgBSjm8DOeZJdv+hKehG8kfwjAYePc+rEQhL84nT6wTgU
10qNkk7k3ri8eSce6ZEy/3aOHONmHoTw7iqdyqQaUpuqvBSzig5HZ09wx9BR7lic8Z
11yAyGmyaYBsfnyl6sl7d+u7EC8UjMQe13Tj9dW+rLYX9vCJz5HSLc/D4SioErNFe1
12gcHhk0E7bkLxjGvxO8vMOjRwDR/CCX0ubWYhXliAqNV8cExmr6/Z9aPHVa2oj7KL
13j7JBf1sH8qczMDlZqh+/qbN4i9/Ss7oiFVhALAUST07bMs3m9RdbyQ4Fuw6IqoI0
14daL3BadGheJmLdAeI1lKXFiaqQp8HNiMi02210gkhmVkHYI8zFSEXGSLt+xuySnw
15O0MY7j8+
16=6aAq
17-----END PGP SIGNATURE-----

Timestamp of file with hash 5b30b7a598d04185a0efdbef0b0f6e39648bf992b0c23151e41ad9445e741517 -

in src/init.cpp:495 in 682a1d0f20

491@@ -492,7 +492,7 @@ void SetupServerArgs()
492         "and level 4 tries to reconnect the blocks, "
493         "each level includes the checks of the previous levels "
494         "(0-4, default: %u)", DEFAULT_CHECKLEVEL), true, OptionsCategory::DEBUG_TEST);
495-    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);
496+    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);

MarcoFalke commented at 4:19 pm on July 12, 2019:

nit: mapBlocksUnlinked does no longer exist

in src/validation.h:473 in 613c46fe9e outdated

473+      * instead of putting things in this set.
474+      */
475+    std::set<CBlockIndex*> m_failed_blocks;
476+
477+    /**
478+     * All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions.

ariard commented at 5:47 pm on July 15, 2019:

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”

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

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

in src/validation.cpp:4500 in 613c46fe9e outdated

4498             assert(setBlockIndexCandidates.count(pindex) == 0);
4499         }
4500-        // Check whether this block is in mapBlocksUnlinked.
4501-        std::pair<std::multimap<CBlockIndex*,CBlockIndex*>::iterator,std::multimap<CBlockIndex*,CBlockIndex*>::iterator> rangeUnlinked = mapBlocksUnlinked.equal_range(pindex->pprev);
4502+        // Check whether this block is in m_blocks_unlinked.
4503+        std::pair<std::multimap<CBlockIndex*,CBlockIndex*>::iterator,std::multimap<CBlockIndex*,CBlockIndex*>::iterator> rangeUnlinked = m_blockman.m_blocks_unlinked.equal_range(pindex->pprev);

ariard commented at 6:47 pm on July 15, 2019:

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

in src/validation.h:485 in 613c46fe9e outdated

485+        CBlockTreeDB& blocktree) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
486+
487+    /** Clear all data members. */
488+    void Unload() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
489+
490+    CBlockIndex* AddToBlockIndex(const CBlockHeader& block) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

ariard commented at 6:59 pm on July 15, 2019:

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”

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

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

in src/validation.h:481 in 4ed55dfcd7 outdated

474@@ -475,9 +475,19 @@ class BlockManager {
475      */
476     std::multimap<CBlockIndex*, CBlockIndex*> m_blocks_unlinked;
477 
478+    /**
479+     * Load the blocktree off disk and into memory. Populate certain metadata
480+     * per index entry (nStatus, nChainWork, nTimeMax, etc.) as well as peripheral
481+     * collections like setDirtyBlockIndex.

ariard commented at 7:13 pm on July 15, 2019:

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

in src/validation.cpp:149 in 682a1d0f20

145@@ -147,6 +146,13 @@ namespace {
146     std::set<int> setDirtyFileInfo;
147 } // anon namespace
148 
149+CBlockIndex* LookupBlockIndex(const uint256& hash)

ariard commented at 7:41 pm on July 15, 2019:

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 ?

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

Eventually, a single BlockManager object will be managed and shared to all created CChainState objects by something called ChainstateManager, to be introduced later (https://github.com/bitcoin/bitcoin/pull/15606/commits/df81dc462e72047ed6857d927946cb18e0e9492b). The single global block index will be accessed through the chainstate manager.

ariard approved

ariard commented at 7:52 pm on July 15, 2019: member

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

laanwj merged this on Jul 16, 2019

laanwj closed this on Jul 16, 2019

laanwj referenced this in commit 8f604361eb on Jul 16, 2019

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

sidhujag referenced this in commit 17e7b271dd on Jul 29, 2019

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

deadalnix referenced this in commit 06bba9f0e8 on Sep 22, 2020

jasonbcox referenced this in commit 3e68843ee4 on Sep 22, 2020

jasonbcox referenced this in commit 3f95b1f4ba on Sep 22, 2020

jasonbcox referenced this in commit 54155de050 on Sep 22, 2020

in src/validation.cpp:3332 in 682a1d0f20

3327@@ -3319,7 +3328,10 @@ bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, CValidatio
3328         LOCK(cs_main);
3329         for (const CBlockHeader& header : headers) {
3330             CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast
3331-            if (!::ChainstateActive().AcceptBlockHeader(header, state, chainparams, &pindex)) {
3332+            bool accepted = g_blockman.AcceptBlockHeader(header, state, chainparams, &pindex);
3333+            ::ChainstateActive().CheckBlockIndex(chainparams.GetConsensus());

rebroad commented at 2:11 pm on April 27, 2021:

is this necessary when accepted is false?

MarcoFalke commented at 4:56 pm on April 27, 2021:

#16453

in src/validation.cpp:3378 in 682a1d0f20

3373@@ -3362,7 +3374,10 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CVali
3374     CBlockIndex *pindexDummy = nullptr;
3375     CBlockIndex *&pindex = ppindex ? *ppindex : pindexDummy;
3376 
3377-    if (!AcceptBlockHeader(block, state, chainparams, &pindex))
3378+    bool accepted_header = m_blockman.AcceptBlockHeader(block, state, chainparams, &pindex);
3379+    CheckBlockIndex(chainparams.GetConsensus());

rebroad commented at 2:11 pm on April 27, 2021:

is this necessary when accepted_header is false?

MarcoFalke commented at 4:56 pm on April 27, 2021:

#16453

kittywhiskers referenced this in commit 0f3f4cb1ee on Oct 9, 2021

kittywhiskers referenced this in commit 710af7cbe2 on Oct 9, 2021

kittywhiskers referenced this in commit a7b002057f on Oct 10, 2021

kittywhiskers referenced this in commit 8d86fbd5c4 on Oct 16, 2021

kittywhiskers referenced this in commit f6215ab285 on Oct 16, 2021

kittywhiskers referenced this in commit 4858b3554c on Oct 16, 2021

kittywhiskers referenced this in commit 67016babae on Oct 21, 2021

kittywhiskers referenced this in commit a620d941d3 on Oct 22, 2021

PastaPastaPasta referenced this in commit 29dbe98ee2 on Oct 23, 2021

pravblockc referenced this in commit fce6ab1221 on Nov 18, 2021

DrahtBot locked this on Aug 18, 2022

refactor: share blockmetadata with BlockManager #16194

Conflicts