-
Notifications
You must be signed in to change notification settings - Fork 37.7k
refactor: share blockmetadata with BlockManager #16194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Concept ACK
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK another man to the gang. Overall changes look good to me but will look closely.
ee815b1
to
66b2a2c
Compare
Thanks for the look, guys. I've made @promag's suggested changes and squashed the second commit into the first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some style questions
* Pruned nodes may have entries where B is missing data. | ||
*/ | ||
std::multimap<CBlockIndex*, CBlockIndex*>& mapBlocksUnlinked = ::ChainstateActive().mapBlocksUnlinked; | ||
CBlockIndex* pindexBestInvalid = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth pointing out that we need to touch this piece of data because it is used by both CChainState
methods and BlockManager
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cf9af76
to
1e33f16
Compare
Rebased and incorporated @MarcoFalke's feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two dev-doc-nits
* Pruned nodes may have entries where B is missing data. | ||
*/ | ||
std::multimap<CBlockIndex*, CBlockIndex*>& mapBlocksUnlinked = ::ChainstateActive().mapBlocksUnlinked; | ||
CBlockIndex* pindexBestInvalid = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 1e33f16. Code changes all look correct. Left some suggestions and feedback, but feel free to ignore it.
src/validation.cpp
Outdated
@@ -95,7 +99,7 @@ CChain& ChainActive() { return g_chainstate.m_chain; } | |||
*/ | |||
RecursiveMutex cs_main; | |||
|
|||
BlockMap& mapBlockIndex = ::ChainstateActive().mapBlockIndex; | |||
BlockMap& mapBlockIndex = g_blockman.m_block_index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "refactoring: move block metadata structures into BlockManager" (04fa434)
Note to help review: this is temporary, removed in the next commit
* Pruned nodes may have entries where B is missing data. | ||
*/ | ||
std::multimap<CBlockIndex*, CBlockIndex*>& mapBlocksUnlinked = ::ChainstateActive().mapBlocksUnlinked; | ||
CBlockIndex* pindexBestInvalid = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
1e33f16
to
e89e4f6
Compare
Thanks for the good feedback, @MarcoFalke @ryanofsky. I've incorporated your changes. |
@@ -3300,8 +3311,6 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& | |||
if (ppindex) | |||
*ppindex = pindex; | |||
|
|||
CheckBlockIndex(chainparams.GetConsensus()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewers: this is moved to the two callsites below to avoid making reference to a specific CChainState in BlockManager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @sdaftuar pointed out in #16444 (comment) AcceptBlockHeader
can return early, so we're now calling CheckBlockIndex(()
more often than before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* Pruned nodes may have entries where B is missing data. | ||
*/ | ||
std::multimap<CBlockIndex*, CBlockIndex*>& mapBlocksUnlinked = ::ChainstateActive().mapBlocksUnlinked; | ||
CBlockIndex* pindexBestInvalid = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ACK 682a1d0 Show signature and timestampSignature:
Timestamp of file with hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
std::set<CBlockIndex*> m_failed_blocks; | ||
|
||
/** | ||
* All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :).
// 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may use auto
there, compiler can do type inference?
/** Clear all data members. */ | ||
void Unload() EXCLUSIVE_LOCKS_REQUIRED(cs_main); | ||
|
||
CBlockIndex* AddToBlockIndex(const CBlockHeader& block) EXCLUSIVE_LOCKS_REQUIRED(cs_main); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea - if I have to rebase this PR for some other reason I'll add it.
/** | ||
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -147,6 +146,13 @@ namespace { | |||
std::set<int> setDirtyFileInfo; | |||
} // anon namespace | |||
|
|||
CBlockIndex* LookupBlockIndex(const uint256& hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
BlockManager g_blockman; | ||
} // anon namespace | ||
|
||
static CChainState g_chainstate(g_blockman); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style-nit in commit 613c46f refactoring: move block metadata structures into BlockManager:
For consistency should make both static or put both into a namespace.
m_failed_blocks.clear(); | ||
m_blocks_unlinked.clear(); | ||
|
||
for (const BlockMap::value_type& entry : m_block_index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style-nit: in commit 613c46f refactoring: move block metadata structures into BlockManager:
const auto& entry
does the same, less verbose
@@ -492,7 +492,7 @@ void SetupServerArgs() | |||
"and level 4 tries to reconnect the blocks, " | |||
"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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: mapBlocksUnlinked
does no longer exist
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: bitcoin#15606 Issue: bitcoin#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
…ckManager Summary: 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. --- Partial backport of Core [[bitcoin/bitcoin#16194 | PR16194]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6959
…lockIndex Summary: Prevents BlockManager from having to reference ChainstateActive() within one of its methods which improves encapsulation and makes testing easier. bitcoin/bitcoin@4ed55df --- Depends on D6959 Partial backport of Core [[bitcoin/bitcoin#16194 | PR16194]] Test Plan: ninja check && ninja check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6967
…dation.cpp Summary: There's no need to have this member live on CChainState since it's only used in validation.cpp. bitcoin/bitcoin@55d525a --- Depends on D6967 Partial backport of Core [[bitcoin/bitcoin#16194 | PR16194]] Test Plan: ninja check && ninja check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6968
Summary: in lieu of ::BlockIndex(). bitcoin/bitcoin@682a1d0 --- Depends on D6968 Concludes backport of Core [[bitcoin/bitcoin#16194 | PR16194]] Notes: This backport originally changes LookupBlockIndex from being defined inline in validation.h to declared there and defined in validation.cpp. Since we have that function defined in chain.h, I have chosen to defer to the change in the backport to stick closer to Core. That has the unfortunate side effect of bringing back the checkpoints -> validation -> checkpoints circular dependency, that I plan on resolving by backporting Core [[bitcoin/bitcoin#15655 | PR15655]], if this diff is landed as is. Test Plan: ninja check-all --extended Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6970
@@ -3319,7 +3328,10 @@ bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& headers, CValidatio | |||
LOCK(cs_main); | |||
for (const CBlockHeader& header : headers) { | |||
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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this necessary when accepted is false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3362,7 +3374,10 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, CVali | |||
CBlockIndex *pindexDummy = nullptr; | |||
CBlockIndex *&pindex = ppindex ? *ppindex : pindexDummy; | |||
|
|||
if (!AcceptBlockHeader(block, state, chainparams, &pindex)) | |||
bool accepted_header = m_blockman.AcceptBlockHeader(block, state, chainparams, &pindex); | |||
CheckBlockIndex(chainparams.GetConsensus()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this necessary when accepted_header is false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
merge bitcoin#15948, bitcoin#15976, bitcoin#16194...: assumeutxo project backports (part 1)
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
This is part of the assumeutxo project:
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 withgit 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.