Remove almost all blockstorage globals #25781
Merged
Conversation
This file contains 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
maflcko
force-pushed
the
2208-block-globals-
branch
3 times, most recently
from
24a984a
to
a63ccbc
Compare
maflcko
changed the title
maflcko
force-pushed
the
2208-block-globals-
branch
from
def01e3
to
fa8ef4b
Compare
maflcko
force-pushed
the
2208-block-globals-
branch
from
fa8ef4b
to
fafe966
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
This was referenced Aug 5, 2022
Merged
incorrect blk file size calculation during reindex results in recoverable blk file corruption
#24858
Merged
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Aug 11, 2022
2e79fb6 validation tests: Use existing {Chainstate,Block}Man (Carl Dong) Pull request description: This is split up because it is needed for two changes: * bitcoin/bitcoin#25781 * bitcoin/bitcoin#25623 ACKs for top commit: adam2k: ACK tested 2e79fb6 aureleoules: ACK 2e79fb6. Tree-SHA512: 2cd6a2fec19545f8ffc77e37ccb793aa6cb5815bb1b5e560c0345af6e0f890fd500ae3297b044d3f6f613b8dd7fd4553f5fc2824013342b9e25af1fe2b624967
maflcko
force-pushed
the
2208-block-globals-
branch
from
fafe966
to
faaa139
Compare
This was referenced Aug 17, 2022
maflcko
force-pushed
the
2208-block-globals-
branch
from
faaa139
to
faf14c6
Compare
w0xlt
approved these changes
Aug 22, 2022
This was referenced Mar 15, 2023
p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode
#27050
Closed
Closed
maflcko
force-pushed
the
2208-block-globals-
branch
from
fa75bbd
to
fa9bd7b
Compare
|
I've added a new commit, because all of this was reworked anyway. |
|
Code review ACK fadf8b8 |
dergoegge
reviewed
Mar 15, 2023
| : m_prune_mode{opts.prune_target > 0}, | ||
| m_opts{std::move(opts)} {}; | ||
|
|
||
| std::atomic<bool> m_importing{false}; |
There was a problem hiding this comment.
nit: I think it would be nice if access to this was wrapped in a getter/setter, e.g. Importing(), {Start,Stop}Importing().
My thinking is that we might (in the future) want to mock the BlockManager to avoid expensive i/o in the fuzz tests for example. Having a clear interface defined would make that easier. A simple member like this is not a huge blocker for that so feel free to ignore.
There was a problem hiding this comment.
This shouldn't be exposed at all, so I think making it private and ImportingNow a friend or so might be better?
|
ACK fadf8b8 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Mar 16, 2023
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Mar 16, 2023
fanquake
added a commit
to bitcoin-core/gui
that referenced
this pull request
May 11, 2023
…rom blockstorage 5ff63a0 refactor, blockstorage: Replace stopafterblockimport arg (TheCharlatan) 18e5ba7 refactor, blockstorage: Replace blocksdir arg (TheCharlatan) 02a0899 refactor, BlockManager: Replace fastprune from arg with options (TheCharlatan) a498d69 refactor/iwyu: Complete includes for blockmanager_args (TheCharlatan) f0bb102 refactor: Move functions to BlockManager methods (TheCharlatan) cfbb212 zmq: Pass lambda to zmq's ZMQPublishRawBlockNotifier (TheCharlatan) 8ed4ff8 refactor: Declare g_zmq_notification_interface as unique_ptr (TheCharlatan) Pull request description: The libbitcoin_kernel library should not rely on the `ArgsManager`, but rather use option structs that can be passed to the various classes it uses. This PR removes reliance on the `ArgsManager` from the `blockstorage.*` files. Like similar prior work, it uses the options struct in the `BlockManager` that can be populated with `ArgsManager` values. Some related prior work: bitcoin/bitcoin#26889 bitcoin/bitcoin#25862 bitcoin/bitcoin#25527 bitcoin/bitcoin#25487 Related PR removing blockstorage globals: bitcoin/bitcoin#25781 ACKs for top commit: ryanofsky: Code review ACK 5ff63a0. Since last ACK just added std::move and fixed commit title. Sorry for the noise! mzumsande: Code Review ACK 5ff63a0 Tree-SHA512: 4bde8fd140a40b97eca923e9016d85dcea6fad6fd199731f158376294af59c3e8b163a0725aa47b4be3519b61828044e0a042deea005e0c28de21d8b6c3e1ea7
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
It seems preferable to assign globals to a class (in this case
BlockManager), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.