refactor: Move stuff to ChainstateManager #23785
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 4 commits
December 15, 2021 17:45
The helper was moved in commit b026e31, which also mentioned that it could be moved to CChainState. So do that, as the functionality is not block-storage related. This also allows to drop one function argument.
This is needed for the next commit.
The member is unrelated to block storage (BlockManager). It is related to validation. Fix the confusion by moving it. Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
A private member is better than a global.
Member
Author
|
This is required for and split off of #23581 |
naumenkogs
reviewed
Dec 16, 2021
| * ahead and mark descendants of invalid blocks as FAILED_CHILD at that time, | ||
| * instead of putting things in this set. | ||
| */ | ||
| std::set<CBlockIndex*> m_failed_blocks; |
Contributor
There was a problem hiding this comment.
probably irrelevant, but would it make sense to make it unordered_set?
Member
Author
There was a problem hiding this comment.
Maybe as a separate pull? I want to keep this move-only refactoring and not change the behaviour/performance.
Contributor
|
ACK fab6d6b |
1 similar comment
Member
|
ACK fab6d6b |
shaavan
approved these changes
Dec 16, 2021
| //! by the background validation chainstate. | ||
| bool m_snapshot_validated{false}; | ||
|
|
||
| CBlockIndex* m_best_invalid; |
Contributor
There was a problem hiding this comment.
I was just curious, though. How about naming this variable to:
Suggested change
| CBlockIndex* m_best_invalid; | |
| CBlockIndex* m_best_invalid_index; |
or to:
Suggested change
| CBlockIndex* m_best_invalid; | |
| CBlockIndex* m_index_best_invalid; |
Because:
- This would help express the purpose of this variable more clearly.
- Since this replaces
pindexBestInvalid, it makes it more logical to name it this way.
Member
There was a problem hiding this comment.
Alternatively m_best_invalid_block_index, though that's a bit long
Member
Author
There was a problem hiding this comment.
Personally I am not a fan of putting the type of the symbol in the name of the symbol.
For example, I think we prefer std::string name over std::string strName or std::string name_string.
Also, leaving as-is for now to not invalidate reviews.
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 16, 2021
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 16, 2022
Summary: > The helper was moved in commit b026e318c39f59a06e29f1b25c7f577e01b25ccb, > which also mentioned that it could be moved to CChainState. So do that, > as the functionality is not block-storage related. > > This also allows to drop one function argument. This is a backport of [[bitcoin/bitcoin#23785 | core#23785]] bitcoin/bitcoin@fa3d62c Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12484
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 16, 2022
Summary: This is needed for the next commit. Note that `m_chainman` was made `const` by us in D12227 when backporting "validation: add chainman ref to CChainState" ([[bitcoin/bitcoin#21526 | core#21526]]), because we could. In this commit is need to be made non-const so `m_chainman.AcceptBlockHeader` can be called. This member function cannot be made const. This is a backport of [[bitcoin/bitcoin#23785 | core#23785]] bitcoin/bitcoin@fa47b5c Depends on D12484 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12485
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 16, 2022
Summary: The member is unrelated to block storage (BlockManager). It is related to validation. Fix the confusion by moving it. Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space This is a partial backport of [[bitcoin/bitcoin#23785 | core#23785]] bitcoin/bitcoin@facd213 Depends on D12485 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12486
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 16, 2022
Summary: A private member is better than a global. This concludes backport of [[bitcoin/bitcoin#23785 | core#23785]] bitcoin/bitcoin@fab6d6b Depends on D12486 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12487
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move globals or members of the wrong class to the right class.