Remove almost all blockstorage globals

maflcko commented at 3:02 pm on August 4, 2022: member

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.

maflcko force-pushed on Aug 4, 2022

maflcko renamed this:
~~Remove all blockstorage globals~~
Remove almost all blockstorage globals
on Aug 4, 2022

maflcko force-pushed on Aug 4, 2022

maflcko marked this as ready for review on Aug 4, 2022

maflcko force-pushed on Aug 4, 2022

DrahtBot commented at 4:13 am on August 5, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, dergoegge, achow101
Concept ACK	fanquake
Stale ACK	w0xlt

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27254 (refactor: Extract util/fs from util/system by TheCharlatan)
#27125 (refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan)
#27050 (p2p, validation: Don’t download witnesses for assumed-valid blocks when running in prune mode by dergoegge)
#25386 (refactor: Extract MIB_BYTES constant for init.cpp by Empact)
#25193 (indexes: Read the locator’s top block during init, allow interaction with reindex-chainstate by mzumsande)
#15606 (assumeutxo by jamesob)

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.

maflcko referenced this in commit e5d8b65423 on Aug 11, 2022

maflcko force-pushed on Aug 11, 2022

DrahtBot added the label Needs rebase on Aug 22, 2022

maflcko force-pushed on Aug 22, 2022

DrahtBot removed the label Needs rebase on Aug 22, 2022

w0xlt approved

w0xlt commented at 4:54 pm on August 22, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/25781/commits/faf14c6305dcde11337cb1ed4f035f2e6b3bfc04

DrahtBot added the label Needs rebase on Aug 25, 2022

TheCharlatan approved

TheCharlatan commented at 1:47 pm on September 18, 2022: contributor

Light code review ACK faf14c6305dcde11337cb1ed4f035f2e6b3bfc04

I focused on the removal of the global params and for unwanted side effects during initialization. Delaying the pruning related errors a bit, but still reporting them before loading the chain data, seems acceptable to me.

dergoegge commented at 5:20 pm on October 19, 2022: member

Concept ACK

maflcko force-pushed on Dec 7, 2022

fanquake commented at 2:29 pm on December 7, 2022: member

Concept ACK

maflcko force-pushed on Dec 7, 2022

DrahtBot removed the label Needs rebase on Dec 7, 2022

maflcko force-pushed on Dec 7, 2022

maflcko force-pushed on Jan 3, 2023

in src/node/blockstorage.h:152 in fa20626495 outdated

147+    std::atomic<bool> m_importing{false};
148+    /** Pruning-related variables and constants */
149+    /** True if we're running in -prune mode. */
150+    const bool m_prune_mode;
151+    /** Number of bytes of block files that we're trying to stay below. */
152+    const uint64_t m_prune_target;

dergoegge commented at 10:10 am on January 4, 2023:

I would prefer if these were private and only accessed through methods on BlockManager.

maflcko commented at 9:28 am on January 30, 2023:

done, I think

in src/node/blockstorage.h:188 in fa20626495 outdated

184@@ -177,6 +185,11 @@ class BlockManager
185     /** Store block on disk. If dbp is not nullptr, then it provides the known position of the block within a block file on disk. */
186     FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp);
187 
188+    [[nodiscard]] bool LoadingBlocks()

dergoegge commented at 10:10 am on January 4, 2023:

0    [[nodiscard]] bool LoadingBlocks() const

fanquake commented at 11:50 am on January 4, 2023:

See also #26664.

maflcko commented at 9:28 am on January 30, 2023:

already done in master

dergoegge changes_requested

DrahtBot added the label Needs rebase on Jan 16, 2023

maflcko force-pushed on Jan 17, 2023

DrahtBot removed the label Needs rebase on Jan 17, 2023

maflcko referenced this in commit 9a288430df on Jan 27, 2023

maflcko force-pushed on Jan 27, 2023

TheCharlatan approved

TheCharlatan commented at 9:16 am on January 30, 2023: contributor

re-code review ACK fac85d8a93be71e919e01511f2bff7e684932b6d

in src/validation.cpp:5309 in fa710f1aa3 outdated

5305@@ -5306,7 +5306,7 @@ static ChainstateManager::Options&& Flatten(ChainstateManager::Options&& opts)
5306     return std::move(opts);
5307 }
5308 
5309-ChainstateManager::ChainstateManager(Options options) : m_options{Flatten(std::move(options))} {}
5310+ChainstateManager::ChainstateManager(Options options) : m_options{Flatten(std::move(options))}, m_blockman{options.blockman_opts} {}

maflcko commented at 9:30 am on January 30, 2023:

in fa710f1aa3280532279ddaa510826e6bd2d3d1c8: I wonder if this is a use-after-move and why clang-tidy does not see it

dergoegge commented at 11:11 am on January 30, 2023:

From https://en.cppreference.com/w/cpp/language/constructor#Initialization_order:

non-static data member are initialized in order of declaration in the class definition.

So according to that I’d say yes, this is a use after move (as m_options is declared before m_blockman).

(also: https://github.com/llvm/llvm-project/issues/56195)

TheCharlatan commented at 4:00 pm on February 15, 2023:

In this case why not decouple the two options and pass and construct them independently? I’ve been using this branch for further work on getting the gArgs out of blockstorage.cpp and got the impression like there is little benefit in nesting the options.

maflcko commented at 5:52 pm on March 14, 2023:

there is little benefit …

Thanks, done.

maflcko commented at 9:30 am on March 23, 2023:

(also: https://github.com/llvm/llvm-project/issues/56195)

Thanks, fixed in https://github.com/llvm/llvm-project/commit/8c10256734cd47274671fcabe94f24f15ecd6209

fanquake requested review from john-moffett on Feb 6, 2023

DrahtBot added the label Needs rebase on Feb 7, 2023

DrahtBot requested review from w0xlt on Feb 15, 2023

maflcko force-pushed on Mar 14, 2023

DrahtBot removed the label Needs rebase on Mar 14, 2023

fanquake removed review request from w0xlt on Mar 14, 2023

fanquake requested review from TheCharlatan on Mar 14, 2023

in src/node/blockmanager_args.cpp:14 in ffffd70359 outdated

 9+
10+namespace node {
11+std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Options& opts)
12+{
13+    // block pruning; get the amount of disk space (in MiB) to allot for block & undo files
14+    int64_t nPruneArg = args.GetIntArg("-prune", 0);

TheCharlatan commented at 7:35 pm on March 14, 2023:

I’m taking a look at this with some fresh knowledge now. If I understand this correctly, this always overrides what is passed in the opts with 0 if the prune arg is not set. The way I understand the current patterns elsewhere in the code for reading the arguments, the passed in opts are preserved if no argument is found, for example as done in chainstatemanager_args / chainstatemanager_opts. I’m not sure which pattern is preferable to be honest, but I think it should be consistent.

This also similarly applies to fPruneMode.

maflcko commented at 2:48 pm on March 15, 2023:

Yes, makes sense for consistency. Thanks, done.

This also similarly applies to fPruneMode.

I don’t think it does. I’ve reworked the second commit.

TheCharlatan commented at 4:15 pm on March 15, 2023:

Right on, can be marked as resolved.

Move ::nPruneTarget into BlockManager fa721f1cab

Move ::fPruneMode into BlockManager fa177d7b6b

Pass fImporting to ImportingNow helper class fa442b1377

Move ::fImporting to BlockManager fa9bd7be47

maflcko force-pushed on Mar 15, 2023

refactor: Add and use PRUNE_TARGET_MANUAL constexpr fadf8b8182

maflcko commented at 3:04 pm on March 15, 2023: member

I’ve added a new commit, because all of this was reworked anyway.

TheCharlatan commented at 4:51 pm on March 15, 2023: contributor

Code review ACK fadf8b818226dc60adf88e927160f9c9680473a4

DrahtBot requested review from w0xlt on Mar 15, 2023

in src/node/blockstorage.h:150 in fadf8b8182

145+
146+    explicit BlockManager(Options opts)
147+        : m_prune_mode{opts.prune_target > 0},
148+          m_opts{std::move(opts)} {};
149+
150+    std::atomic<bool> m_importing{false};

dergoegge commented at 5:04 pm on March 15, 2023:

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.

maflcko commented at 8:31 am on March 16, 2023:

This shouldn’t be exposed at all, so I think making it private and ImportingNow a friend or so might be better?

dergoegge commented at 5:05 pm on March 15, 2023: member

Code review ACK fadf8b818226dc60adf88e927160f9c9680473a4

DrahtBot removed review request from w0xlt on Mar 15, 2023

DrahtBot requested review from w0xlt on Mar 15, 2023

DrahtBot removed review request from w0xlt on Mar 15, 2023

DrahtBot requested review from w0xlt on Mar 15, 2023

achow101 commented at 10:38 pm on March 15, 2023: member

ACK fadf8b818226dc60adf88e927160f9c9680473a4

DrahtBot removed review request from w0xlt on Mar 15, 2023

DrahtBot requested review from w0xlt on Mar 15, 2023

achow101 merged this on Mar 15, 2023

achow101 closed this on Mar 15, 2023

sidhujag referenced this in commit 1eb1360d5e on Mar 16, 2023

sidhujag referenced this in commit 5ed0b8bb8c on Mar 16, 2023

maflcko deleted the branch on Mar 16, 2023

fanquake referenced this in commit c2f2abd0a4 on May 11, 2023

bitcoin locked this on Mar 22, 2024

Remove almost all blockstorage globals #25781

Reviews

Conflicts