refactor, kernel: Decouple ArgsManager from blockstorage #27125

TheCharlatan · 2023-02-19T11:10:11Z

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: #26889 #25862 #25527 #25487

Related PR removing blockstorage globals: #25781

DrahtBot · 2023-02-19T11:10:14Z

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	ryanofsky, mzumsande
Concept ACK	brunoerg
Approach ACK	hebasto
Stale ACK	dergoegge, MarcoFalke

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:

#27596 (assumeutxo (2) by jamesob)
#27571 (ci: Run iwyu on all src files by MarcoFalke)
#26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
#26326 (net: don't lock cs_main while reading blocks in net processing by andrewtoth)
#26288 (Enable -Wstring-concatenation and -Wstring-conversion on clang builds by Empact)
#25193 (indexes: Read the locator's top block during init, allow interaction with reindex-chainstate by mzumsande)
#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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.

fanquake · 2023-02-19T14:10:29Z

validation.cpp:80:13: error: using decl 'ReadBlockFromDisk' is unused [misc-unused-using-decls,-warnings-as-errors]
using node::ReadBlockFromDisk;
            ^
/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/validation.cpp:80:13: note: remove the using
using node::ReadBlockFromDisk;

TheCharlatan · 2023-02-19T19:56:55Z

validation.cpp:80:13: error: using decl 'ReadBlockFromDisk' is unused [misc-unused-using-decls,-warnings-as-errors]
using node::ReadBlockFromDisk;
            ^
/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/validation.cpp:80:13: note: remove the using
using node::ReadBlockFromDisk;

Thank you, I'm improving my clang tidy workflow.

brunoerg

Concept ACK

src/net_processing.cpp

TheCharlatan · 2023-02-27T20:50:21Z

Updated e8b095f -> 899ba71 (removeBlockstorageArgs_0 -> removeBlockstorageArgs_1, compare) with suggested changes.
Rebased 899ba71 -> b640db3 to get recent PR's with kernel build improvements.

TheCharlatan · 2023-02-28T06:22:15Z

Updated b640db3 -> 3ded321 (removeBlockstorageArgs_2 -> removeBlockstorageArgs_3, compare) with suggested typo fix.

mzumsande

Concept ACK

Seeing all that effort to pass around the hacky fastprune arg (which doesn't make any sense outside of functional tests) seems a bit sad, so I wonder if it's really inevitable.

Did you consider whether it's possible to also move BlockFileSeq into blockmanager? I guess it doesn't work just because of one remaining call site for node::ReadBlockFromDisk from zmqpublishnotifier?

src/node/blockmanager_args.h

dergoegge · 2023-02-28T17:13:07Z

Concept ACK

TheCharlatan · 2023-02-28T21:50:21Z

Thanks for the reviews!

Seeing all that effort to pass around the hacky fastprune arg (which doesn't make any sense outside of functional tests) seems a bit sad, so I wonder if it's really inevitable.
Did you consider whether it's possible to also move BlockFileSeq into blockmanager? I guess it doesn't work just because of one remaining call site for node::ReadBlockFromDisk from zmqpublishnotifier?

Yes, totally echo this sentiment. Indeed BlockFileSeq and its dependants are not class members because of the call site in zmqpublishnotifier. I refrained from further refactoring because of this. If somebody has an idea on how to handle the zmq case I'll gladly rework this pull request.

Updated 3ded321 -> 5964043 (removeBlockstorageArgs_3 -> removeBlockstorageArgs_4, compare) removing unused declaration.
Rebased 5964043 -> 88e88a5 (removeBlockstorageArgs_4 -> removeBlockstorageArgs_5, compare) to resolve conflicts.

Ensures better memory safety for this global. This came up during discussion of the following commit, but is not strictly required for its implementation.

TheCharlatan · 2023-05-10T11:16:10Z

Rebased 8f94f05 -> 886a473 (removeBlockstorageArgs_23 -> removeBlockstorageArgs_24, compare)

Fixed conflicts with ci: Remove CI_EXEC bloat in test/06_script_b.sh #27573

maflcko · 2023-05-10T13:32:12Z

trivial re-ACK 886a473 🐗

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: trivial re-ACK 886a473fc48f2c7d67436b5d9ac5643cd007e27f 🐗
MiFgn+VZb98m014LmOnzmOv20QUq1tQSKEYOvhN6Tv3gta/Ii6BQq864nyHczaEFMZIfPXqKHHnPaRQ3SyVrAg==

ryanofsky · 2023-05-10T15:41:35Z

src/zmq/zmqnotificationinterface.cpp

 {
    std::map<std::string, CZMQNotifierFactory> factories;
    factories["pubhashblock"] = CZMQAbstractNotifier::Create<CZMQPublishHashBlockNotifier>;
    factories["pubhashtx"] = CZMQAbstractNotifier::Create<CZMQPublishHashTransactionNotifier>;
-    factories["pubrawblock"] = CZMQAbstractNotifier::Create<CZMQPublishRawBlockNotifier>;
+    factories["pubrawblock"] = [&get_block_by_index]() -> std::unique_ptr<CZMQAbstractNotifier> {


In commit "zmq: Pass lambda to zmq's ZMQPublishRawBlockNotifier" (6247b3c)

It looks like the lambda is taking a reference to the local get_block_by_index variable here, which doesn't seem right, since the variable will go out of scope before the lambda is called. So I think & should be dropped here.

Not totally sure though, since I would expect there to be problems in CI if there was a bug.

EDIT: I think moving instead of copying would also work:

factories["pubrawblock"] = [gb = std::move(get_block_by_index)]() -> std::unique_ptr<CZMQAbstractNotifier> { return std::make_unique<CZMQPublishRawBlockNotifier>(std::move(gb)); };

CZMQPublishRawBlockNotifier takes get_block_by_index by value (meaning instead of borrowing, copies its value), so I think that is why this works out.

re: #27125 (comment)

Yeah sorry, I saw the "factories" map and assumed the factories would be long-lived, not destroyed before the function returns. This code is pretty strange, because it creates a map and then never looks anything up in the map. Could be cleaned up later, though. Probably a more sane way to write it would be to drop the map, change the outer for loop into an add_notifiers lambda, and do

add_notifiers("pubhashblock", CZMQAbstractNotifier::Create<CZMQPublishHashBlockNotifier>); add_notifiers("pubhashtx", CZMQAbstractNotifier::Create<CZMQPublishHashTransactionNotifier>); ...

In any case, agree the current code is safe

TheCharlatan · 2023-05-10T16:51:46Z

Updated 886a473 -> e3311d6 (removeBlockstorageArgs_24 -> removeBlockstorageArgs_25, compare)

Addressed @ryanofsky's comment, moving the lambda instead of borrowing.

EDIT: Sorry, forgot the second part :P

Updated e3311d6 -> 87479b2 (removeBlockstorageArgs_25 -> removeBlockstorageArgs_26, compare)

ryanofsky

Code review ACK 886a473

re: #27125 (comment)

* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190097934), moving the lambda instead of borrowing.

Actually I think my suggestion won't work. There can be multiple notifiers, so the function needs to be copied into all of them not moved into the first one. ACKed previous version of the PR 886a473 though, so would suggest reverting.

src/zmq/zmqpublishnotifier.h

ci/test/06_script_b.sh

ryanofsky · 2023-05-10T16:51:23Z

src/zmq/zmqnotificationinterface.cpp

 {
    std::map<std::string, CZMQNotifierFactory> factories;
    factories["pubhashblock"] = CZMQAbstractNotifier::Create<CZMQPublishHashBlockNotifier>;
    factories["pubhashtx"] = CZMQAbstractNotifier::Create<CZMQPublishHashTransactionNotifier>;
-    factories["pubrawblock"] = CZMQAbstractNotifier::Create<CZMQPublishRawBlockNotifier>;
+    factories["pubrawblock"] = [&get_block_by_index]() -> std::unique_ptr<CZMQAbstractNotifier> {


re: #27125 (comment)

Yeah sorry, I saw the "factories" map and assumed the factories would be long-lived, not destroyed before the function returns. This code is pretty strange, because it creates a map and then never looks anything up in the map. Could be cleaned up later, though. Probably a more sane way to write it would be to drop the map, change the outer for loop into an add_notifiers lambda, and do

add_notifiers("pubhashblock", CZMQAbstractNotifier::Create<CZMQPublishHashBlockNotifier>); add_notifiers("pubhashtx", CZMQAbstractNotifier::Create<CZMQPublishHashTransactionNotifier>); ...

In any case, agree the current code is safe

The lambda captures a reference to the chainman unique_ptr to retrieve block data. An assert is added on the chainman to ensure that the lambda is not used while the chainman is uninitialized. This is done in preparation for the following commits where blockstorage functions are made BlockManager methods.

This is a commit in preparation for the next few commits. The functions are moved to methods to avoid their re-declaration for the purpose of passing in BlockManager options. The functions that were now moved into the BlockManager should no longer use the params as an argument, but instead use the member variable. In the moved ReadBlockFromDisk and UndoReadFromDisk, change the function signature to accept a reference to a CBlockIndex instead of a raw pointer. The pointer is expected to be non-null, so reflect that in the type. To allow for the move of functions to BlockManager methods all call sites require an instantiated BlockManager, or a callback to one.

Remove access to the global gArgs for the fastprune argument and replace it by adding a field to the existing BlockManager Options struct. When running `clang-tidy-diff` on this commit, there is a diagnostic error: `unknown type name 'uint64_t' [clang-diagnostic-error] uint64_t prune_target{0};`, which is fixed by including cstdint. This should eventually allow users of the BlockManager to not rely on the global gArgs and instead pass in their own options.

Add a blocks_dir field to the BlockManager options. Move functions relying on the global gArgs to get the blocks_dir into the BlockManager class. This should eventually allow users of the BlockManager to not rely on the global Args and instead pass in their own options.

Add a stop_after_block_import field to the BlockManager options. Use this field instead of the global gArgs. This should allow users of the BlockManager to not rely on the global Args.

fanquake · 2023-05-10T17:16:47Z

https://github.com/bitcoin/bitcoin/pull/27125/checks?check_run_id=13380672723

clang-tidy-16 -p=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/zmq/zmqnotificationinterface.cpp
zmq/zmqnotificationinterface.cpp:48:62: error: std::move of the const variable 'gb' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg,-warnings-as-errors]
        return std::make_unique<CZMQPublishRawBlockNotifier>(std::move(gb));
                                                             ^~~~~~~~~~  ~

TheCharlatan · 2023-05-10T17:24:51Z

Updated 87479b2 -> 5ff63a0 (removeBlockstorageArgs_26 -> removeBlockstorageArgs_27, compare)

Reverted previous change as suggested by @ryanofsky in comment.
Addressed @ryanofsky's comment, correcting the commit message.
Addressed @ryanofsky's comment, moving the lambda in the constructor.

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

mzumsande · 2023-05-10T20:12:07Z

src/node/transaction.h

@@ -53,11 +54,10 @@ static const CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};
 * @param[in]  block_index     The block to read from disk, or nullptr
 * @param[in]  mempool         If provided, check mempool for tx
 * @param[in]  hash            The txid
- * @param[in]  consensusParams The params
 * @param[out] hashBlock       The block hash, if the tx was found via -txindex or block_index


nit (if you repush): blockman should be added to the doc, also it would be nice to keep the out param at the end according to dev notes.

…kstorage

…base from kernel library db77f87 scripted-diff: move settings to common namespace (TheCharlatan) c27e4bd move-only: Move settings to the common library (TheCharlatan) c2dae5d kernel: Remove chainparams, chainparamsbase, args, settings from kernel library (TheCharlatan) 05870b1 refactor: Remove gArgs access from validation.cpp (TheCharlatan) 8789b11 refactor: Add path argument to FindSnapshotChainstateDir (TheCharlatan) ef95be3 refactor: Add stop_at_height option in ChainstateManager (TheCharlatan) Pull request description: This pull request is part of the `libbitcoinkernel` project #27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". --- This completes the removal of the node's chainparams, chainparamsbase, args and settings files and their respective classes from the kernel library. This is the last pull request in a long series working towards decoupling the `ArgsManager` and the `gArgs` global from kernel code. These prior pull requests are: #26177 #27125 #25527 #25487 #25290 ACKs for top commit: MarcoFalke: lgtm ACK db77f87 🍄 hebasto: ACK db77f87, I have reviewed the code and it looks OK. ryanofsky: Code review ACK db77f87. Looks great! Tree-SHA512: cbfbd705d056f2f10f16810d4f869eb152362fff2c5ddae5e1ac6785deae095588e52ad48b29d921962b085e51de1e0ecab6e50f46149ffe3c16250608a2c93a

TheCharlatan force-pushed the removeBlockstorageArgs branch from f736f9e to 0d99466 Compare February 19, 2023 11:37

DrahtBot mentioned this pull request Feb 19, 2023

reduce cs_main scope, guard block index 'nFile' under a local mutex #27006

Closed

TheCharlatan force-pushed the removeBlockstorageArgs branch from 0d99466 to 352d2f0 Compare February 19, 2023 17:13

DrahtBot added the Needs rebase label Feb 22, 2023

TheCharlatan force-pushed the removeBlockstorageArgs branch from 352d2f0 to e8b095f Compare February 22, 2023 21:19

DrahtBot removed the Needs rebase label Feb 22, 2023

brunoerg reviewed Feb 25, 2023

View reviewed changes

brunoerg reviewed Feb 27, 2023

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

TheCharlatan force-pushed the removeBlockstorageArgs branch from e8b095f to b640db3 Compare February 27, 2023 18:32

TheCharlatan force-pushed the removeBlockstorageArgs branch from b640db3 to 3ded321 Compare February 28, 2023 06:22

DrahtBot added the Needs rebase label Feb 28, 2023

mzumsande reviewed Feb 28, 2023

View reviewed changes

src/node/blockmanager_args.h Outdated Show resolved Hide resolved

TheCharlatan force-pushed the removeBlockstorageArgs branch from 3ded321 to 88e88a5 Compare February 28, 2023 21:50

DrahtBot removed the Needs rebase label Feb 28, 2023

TheCharlatan force-pushed the removeBlockstorageArgs branch from 88e88a5 to f87c393 Compare March 1, 2023 06:50

refactor: Declare g_zmq_notification_interface as unique_ptr

8ed4ff8

Ensures better memory safety for this global. This came up during discussion of the following commit, but is not strictly required for its implementation.

TheCharlatan force-pushed the removeBlockstorageArgs branch from 8f94f05 to 886a473 Compare May 10, 2023 11:15

ryanofsky reviewed May 10, 2023

View reviewed changes

DrahtBot requested a review from ryanofsky May 10, 2023 15:41

TheCharlatan force-pushed the removeBlockstorageArgs branch from 886a473 to e3311d6 Compare May 10, 2023 16:51

TheCharlatan force-pushed the removeBlockstorageArgs branch from e3311d6 to 87479b2 Compare May 10, 2023 16:57

ryanofsky approved these changes May 10, 2023

View reviewed changes

TheCharlatan added 6 commits May 10, 2023 19:06

refactor/iwyu: Complete includes for blockmanager_args

a498d69

refactor, blockstorage: Replace stopafterblockimport arg

5ff63a0

Add a stop_after_block_import field to the BlockManager options. Use this field instead of the global gArgs. This should allow users of the BlockManager to not rely on the global Args.

fanquake self-assigned this May 10, 2023

fanquake removed their assignment May 10, 2023

TheCharlatan force-pushed the removeBlockstorageArgs branch from 87479b2 to 5ff63a0 Compare May 10, 2023 17:20

DrahtBot added the CI failed label May 10, 2023

ryanofsky approved these changes May 10, 2023

View reviewed changes

DrahtBot requested a review from maflcko May 10, 2023 17:35

mzumsande reviewed May 10, 2023

View reviewed changes

DrahtBot removed the CI failed label May 10, 2023

fanquake merged commit c2f2abd into bitcoin:master May 11, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 11, 2023

Merge bitcoin#27125: refactor, kernel: Decouple ArgsManager from bloc…

43ccd09

…kstorage

bitcoin locked and limited conversation to collaborators May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor, kernel: Decouple ArgsManager from blockstorage #27125

refactor, kernel: Decouple ArgsManager from blockstorage #27125

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

refactor, kernel: Decouple ArgsManager from blockstorage #27125

refactor, kernel: Decouple ArgsManager from blockstorage #27125

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!