TheCharlatan
commented at 11:10 am on February 19, 2023:
contributor
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.
#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.
TheCharlatan force-pushed
on Feb 19, 2023
fanquake
commented at 2:10 pm on February 19, 2023:
member
0validation.cpp:80:13: error: using decl 'ReadBlockFromDisk' is unused [misc-unused-using-decls,-warnings-as-errors]1using node::ReadBlockFromDisk;
2 ^
3/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/validation.cpp:80:13: note: remove the using
4using node::ReadBlockFromDisk;
TheCharlatan force-pushed
on Feb 19, 2023
TheCharlatan
commented at 7:56 pm on February 19, 2023:
contributor
0validation.cpp:80:13: error: using decl 'ReadBlockFromDisk' is unused [misc-unused-using-decls,-warnings-as-errors]1using node::ReadBlockFromDisk;
2 ^
3/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/validation.cpp:80:13: note: remove the using
4using node::ReadBlockFromDisk;
Thank you, I’m improving my clang tidy workflow.
DrahtBot added the label
Needs rebase
on Feb 22, 2023
TheCharlatan force-pushed
on Feb 22, 2023
DrahtBot removed the label
Needs rebase
on Feb 22, 2023
brunoerg
commented at 1:11 pm on February 25, 2023:
contributor
Concept ACK
in
src/net_processing.cpp:3869
in
810f815cb6outdated
mzumsande
commented at 3:58 pm on February 28, 2023:
bilingual_str seems unused
mzumsande
commented at 5:05 pm on February 28, 2023:
contributor
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?
dergoegge
commented at 5:13 pm on February 28, 2023:
member
Concept ACK
TheCharlatan force-pushed
on Feb 28, 2023
TheCharlatan
commented at 9:50 pm on February 28, 2023:
contributor
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.
TheCharlatan
commented at 5:15 pm on March 9, 2023:
contributor
Updated f87c39399823e22c553b797cc66fa4063462a32b -> 6d9826f182d9122d4464d35d6682dc6fb4b1116e (removeBlockstorageArgs_6 -> removeBlockstorageArgs_7, compare) addressing #27125#pullrequestreview-1317942691 by instantiating a BlockManager in the zmqpublishnotifier. As commented previously #27125#pullrequestreview-1317942691 by @mzumsande other functions like BlockFileSeqare now moved into the BlockManager. This resolves the double declaration of functions that was previously required in the previous patchset.
fanquake referenced this in commit
b175bdb9b2
on Mar 14, 2023
DrahtBot added the label
Needs rebase
on Mar 15, 2023
TheCharlatan force-pushed
on Mar 16, 2023
TheCharlatan
commented at 9:28 am on March 16, 2023:
contributor
unrelated: If you want, you can add this file to ci iwyu in ./ci/test/06_…
DrahtBot removed the label
Needs rebase
on Mar 16, 2023
TheCharlatan force-pushed
on Mar 17, 2023
TheCharlatan
commented at 3:53 pm on March 17, 2023:
contributor
Updated 593a9f36609a74f47175e681a3921f3975272766 -> 54d8644dce180197fa657e9b68d6de63cf4c8072 (removeBlockstorageArgs_8 -> removeBlockstorageArgs_9, compare) to improve commit messages and add a commit adding the blockmanager_args.cpp file to iwyu as suggested by @MarcoFalke .
in
src/zmq/zmqpublishnotifier.cpp:258
in
9ba7ab344foutdated
in 9ba7ab344fb39c1145283b02e6b4ff4753c6511b: Can you explain why it is safe to construct two blockman in the same process? This seems fragile, considering that the objects may hold state?
TheCharlatan
commented at 5:03 pm on March 17, 2023:
I may have missed some nuance here. To me it seemed acceptable, because functions are protected by cs_main. For const functions likeReadBlockFromDisk this seems safe to me (though I’m still new to the locking model and might be missing something here). However doing block write and pruning operations with two blockmanagers is fragile, since their internal state might no longer reflect the state on disk.
I’m not sure how to proceed here. I could mark the BlockManager here as const, that would at least ensure that it’s state cannot change.
I may have missed some nuance here. To me it seemed acceptable, because functions are protected by cs_main. For const functions likeReadBlockFromDisk this seems safe to me
Note that according to the PR description in #27006 there is currently at least one cs_main related bug in ReadBlockFromDisk.
TheCharlatan
commented at 11:16 am on March 20, 2023:
Hmm, that sounds like you are making it harder to remove cs_main.
I don’t think this change complicates a refactor of the global locks. zmqpublishnotifier relies on cs_main being in global scope before and after this change. If the lock is moved into some narrower scope, this would have to be handled for the zmqpublishnotifier too - with our without this patch. If I read #27006 correctly, no change is required to make it work with the patch here.
my brain enjoys the thought of having only to think about a single blockman object at a time.
I feel this though, I’ll try out the first and second option you laid out.
TheCharlatan
commented at 2:45 pm on March 20, 2023:
I feel this though, I’ll try out the first and second option you laid out.
When touching this, it might be better to not pass the params, but access them from a member?
Also, I don’t like that several unrelated “refactoring” is bundled with potentially behavior changing stuff. For example, it would be good to do the function moving in a separate commit from the change that constructs two blockman in the same process.
TheCharlatan
commented at 5:03 pm on March 17, 2023:
TheCharlatan
commented at 1:03 pm on March 19, 2023:
contributor
Updated 54d8644dce180197fa657e9b68d6de63cf4c8072 -> 9f5600742c65a2421c9fe5a2a2670e86a25ef696 (removeBlockstorageArgs_9 -> removeBlockstorageArgs_10, compare) to split a commit moving methods and instantiating a block manager in zmq, and replacing the fastprune arg. I also added a short inline comment explaining the zmq’s blockmanager const declaration and threading requirements.
TheCharlatan force-pushed
on Mar 23, 2023
TheCharlatan
commented at 4:38 pm on March 23, 2023:
contributor
Thank you for the discussion and suggestions.
Updated 9f5600742c65a2421c9fe5a2a2670e86a25ef696 -> 97bf70119e5b8567bcdc553d59b18a09023fd05a (removeBlockstorageArgs_10 -> removeBlockstorageArgs_11, compare) to add @dergoegge’s patch as suggested in his comment. With this latest push, there is no additional BlockManager being instantiated anymore.
TheCharlatan
commented at 8:51 pm on March 23, 2023:
contributor
Not sure why the functional tests are failing. The test failing on the Win64 job is passing on the 32-bit job and vice-versa. Both are passing locally.
ryanofsky
commented at 9:08 pm on March 23, 2023:
contributor
Win64 wallet_create_tx.py failure is probably fixed by #27318
TheCharlatan force-pushed
on Mar 23, 2023
TheCharlatan force-pushed
on Mar 23, 2023
fanquake referenced this in commit
369d4c03b7
on Apr 3, 2023
This moves the creation of the ZMQ interface into a for loop, which I think can be executed twice under some special conditions (-reindex is activated by a GUI user).
However, g_zmq_notification_interface is just a simple pointer, not a std::unique_ptr, so I think that there could be a memory leak with the first object never being deleted if we call CZMQNotificationInterface::Create a second time.
in
src/zmq/zmqpublishnotifier.cpp:20
in
97bf70119eoutdated
Addressed @mzumsande’s comment by making the g_zmq_notification_interface a std::unique_ptr and checking that it is not already the owner of a CZMQNotificationInterface object before assigning to it.
Pass a reference of the node context to the CZMQNotificationInterface to ensure that we access the currently initialized BlockManager within the zmq code. In the previous revision, the reference to the BlockManager might have been pointing to a destructed object.
TheCharlatan force-pushed
on Apr 18, 2023
TheCharlatan
commented at 3:12 pm on April 18, 2023:
contributor
nit: Seems unnecessary to pass the entire NodeContext when you only need the blockman.
Feel free to ignore, but would be interested in why you chose to do it this way.
TheCharlatan
commented at 3:40 pm on April 21, 2023:
The blockman and chainstate instances may be deleted during the lifetime of the zmq instance similarly as described in mzsumsande’s comment when iterating the for loop. If we pass a reference to the NodeContext instead, we have better guarantees that its blockman reference points to an actual object.
Or are you just saying that having a reference to the node context has better lifetime guarantees because basically nothing outlives the node context?
TheCharlatan
commented at 4:16 pm on April 21, 2023:
Is that actually possible?
Likely that I am missing something in the logic. The way I see it each iteration of the for loop creates and destroys the chainman by calling std::make_unique again, while the Shutdown code is not called during each iteration.
dergoegge
commented at 3:09 pm on April 21, 2023:
member
lgtm once the last gArgs access is removed.
TheCharlatan force-pushed
on Apr 21, 2023
TheCharlatan force-pushed
on Apr 21, 2023
TheCharlatan
commented at 3:49 pm on April 21, 2023:
contributor
Just talked about this offline, but if possible to make ZMQ just take a pointer to BlockManager instead of NodeContext, that would be good. This way ZMQ would only have access to the state it actually needs
DrahtBot added the label
Needs rebase
on May 2, 2023
TheCharlatan force-pushed
on May 2, 2023
TheCharlatan
commented at 1:02 pm on May 2, 2023:
contributor
Reverted change made for passing a NodeContext to ZmqNotificationInterface back to passing a reference of a BlockManager
Introduced a change declaring the BlockManager outside of the ChainstateManager and making the NodeContext its new owner. References to this BlockManager are now passed to declare ChainstateManager and ZmqNotificationInterface. This could also allow the kernel more control over how blocks are stored.
Unrelated in 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91: Is there a reason you are sometimes using gArgs and then m_args? It looks like m_args ignores extra_args and gArgs/m_node.args doesn’t?
So my preference would be to figure out whether to use m_node.args or m_args, and then use it consistently?
87999089e1b7794cd595ca19893a383149546087: Can you explain why it is necessary and safe to move this out of the for loop?
TheCharlatan
commented at 12:37 pm on May 3, 2023:
In the context of this commit it is necessary to move the assignment out of the for loop, so we don’t destruct the node.blockman on each iteration and end up with a dangling reference to the original object in zmq. What I did not think about though is that we are also no longer resetting the internal state of the BlockManagerwhen the ChainstateManager is destructed. So no, I’m no longer sure that this is safe.
I wonder if the for loop can be removed, and replaced by the shutdown workflow from #26674 (comment)? See also 5921b863e39e5c3997895ffee1c87159e37a5d6f
TheCharlatan
commented at 12:58 pm on May 3, 2023:
Thank you for the context, I see that I am conceptually going backwards here.
I wonder if the for loop can be removed, and replaced by the shutdown workflow from #26674 (comment)
That does sound more sane than continuously re-indexing forever, or waiting for the user to interrupt.
The change I pushed will be reverted. How about instead of moving the previously static functions of blockstorage.cpp into its BlockManager class, moving them to a stateless sub-class of the BlockManager? Then we can have the node context be the owner of this class, and pass it through as a const reference to the ChainstateManager and its BlockManager, as well as the zmq code, which can now use this class without taking ownership of the entire BlockManager?
I very much like them to be stateful, see also #27125 (review) . This is also needed for some other, unrelated, stuff that someone should be working on (cc @josibake )
I was unclear with the word “stateless”. What I meant was, “does not retain memory of previous interactions”, e.g. we set all the options (including the consensus params) on initialization.
nit in 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91: What is the point of adding 5 LOC that will be removed in the next commit anyway? If you want to keep that, it would be good to also remove the now unused include as well, which you forgot?
in
src/zmq/zmqnotificationinterface.cpp:26
in
3b9d6b0640outdated
unrelated in 512592cf41ae8fb307bd6eb651e3319e5053851e: Might be good to store a reference to the chain params in m_opts in a previous commit and use it inside the function. Then, while touching all lines here, the now unused params argument could be dropped.
This cleanup applies to other functions as well, so I am happy to have this split up (for those other functions), to avoid this pull from growing too large?
nit in 1def580e80a1892e473324016db9a0f2ffec0c27: (For this method and others like UndoReadFromDisk …) It is UB to pass a nullptr pindex, so it would be good, while touching the function signature in this pull request to switch the pointer to a reference.
6b5eb9376c97ac7edd096754708aba6d454cef76:Shouldn’t this happen inside ApplyArgsManOptions? If not, shouldn’t blocks_dir be const to avoid an empty path?
I copied this behavior from ChainstateManagerOpts. We expect the default constructed Options to contain defaults. If we don’t have defaults, we assign on object construction. So, yes, in both instances a const qualifier (and maybe in others using the same pattern) should be added.
Reverted a previous change moving BlockManager initialization out of the ChainstateManager as discssed in #27125 (review)
Added a commit a614ac59cd753c7ef83899602e0176654c5fff87 introducing an interface function for accessing the BlockManager::ReadBlockFromDisk. This was introduced since existing functions were more complex and less efficient than just a direct call. If this interface should not grow, I can drop this commit again and use a different method, like the previously discussed FindBlock in the following commit.
Added a commit 6b9ba7b54097bb32d3c1596b7fdcec4428c27a3a capturing this introduced interface function in a lambda and using it in the zmq code. Capturing the node.chain should be safe for the purpose of zmq notifying about block tip changes, even with the for loop re-declaring the ChainstateManager.
Addressed @MarcoFalke’s comment, reordering the commits. Functions are now moved to BlockManager methods before removing ArgsManager usage.
Changes in test/util/common.cpp do not seem required in the dbd739d54386dcd5dd1d945423bc82362263d4e8“refactor, BlockManager: Replace fastprune from arg with options” commit. Maybe combine them into the following commit?
TheCharlatan force-pushed
on May 4, 2023
TheCharlatan
commented at 1:43 pm on May 4, 2023:
contributor
Passing a pointer from one process to another one is UB. I wonder if there should be a compile failure if someone adds a pointer type to the multiprocess interface? cc ryan
In general it’s ok to pass pointers and references. These are both really useful for output parameters, so the IPC framework does support them. But the type needs to be serializable, and it isn’t really possible to write serialization code for the CBlockIndex type because of the pprev pointers it contains. It compile error in #10102 to try to pass types that aren’t serializable.
For this PR would suggest not adding a new chain method, and instead doing:
Thanks for taking a look. It’s also not used by an actual client of the interface, so I think I’ll just drop the commit then. No need to needlessly pollute the interface.
Changed the lambda to capture a reference to the node’s chainman and call ReadBlockFromDisk from its m_blockman. I think this is equivalent to going through the chain interface’s findBlock, which also accesses m_node.chainman.
DrahtBot added the label
CI failed
on May 5, 2023
DrahtBot added the label
Needs rebase
on May 5, 2023
TheCharlatan force-pushed
on May 5, 2023
TheCharlatan
commented at 8:23 pm on May 5, 2023:
contributor
Also, it would be good to include the diff in src/zmq/zmqpublishnotifier.cpp from the next commit. This makes review easier, because reviewers can see that the function-to-lambda is a simple 1-1 replacement that compiles and passes all tests on its own.
in
src/node/transaction.cpp:143
in
0400fb7b55outdated
In commit “refactor: Use BlockManager options for params” (0400fb7b55415867d9ff0aa88898920c60b4b2df)
Seem like you could squash this commit into the last commit and decrease review burden because the changes are mechanical and basically every line that changed here changed last commit. Not important though.
ryanofsky approved
ryanofsky
commented at 1:58 pm on May 8, 2023:
contributor
Code review ACK3b34ac7465919b968795063995f6610a73aa2d29. Left some comments but this looks good as is and would not change unless necessary
This is a slightly different approach to -fastprune and others, which are set in ApplyArgsManOptions().
So I wonder what is the criterion for an arg to be set in ApplyArgsManOptions(), as opposed to being initialized directly? And, closely connected, isn’t the current approach of not calling ApplyArgsManOptions() in some spots (setup_common, some unit tests) a bit brittle? (considering that users could specify extra bitcoind args while running unit tests, which then would be ignored)
This is a slightly different approach to -fastprune and others, which are set in ApplyArgsManOptions().
So I wonder what is the criterion for an arg to be set in ApplyArgsManOptions(), as opposed to being initialized directly?
Initializing in the list is done for option members that don’t have sane default or empty values. This also has precedent in the ChainstateManager::Options.
And, closely connected, isn’t the current approach of not calling ApplyArgsManOptions() in some spots (setup_common, some unit tests) a bit brittle?
Yes, the current situation does not seem to be ideal. Sometimes these extra_args that the user can supply are currently parsed into the Options in the particular tests, and sometimes, like for the ChainstateManager, they are ignored. This is down to the mixed usage of both the m_args, which creates a fresh, default ArgsManager for each test, and the m_node.args, which parses all the passed in arguments.
This was brought up previously in this PR in #27125 (review), where based on the related discussion in #25055 and #21244 (review) I decided to keep the current behavior. If I understand the current problem correctly, it is not ideal that the m_node.args points to the global mutable gArgs instead of re-creating the arguments cleanly on each test run, while m_args is recreated on each run, but does not read the extra arguments.
Based on this, I am not sure how to proceed with this on this PR, since I do think that the current approach is broken. I could add a commit making the argument handling consistent for all the Options, at least that would make it probably less broken than it is now. What do you think?
Addressed @MarcoFalke’s comment and comment making CBlockIndex a reference in ReadBlockFromDisk and UndoReadFromDisk as well as in the new zmq lambda function.
Addressed @ryanofsky’s comment by squashing commits changing the function signature of the moved functions. Also did this for the CBlockIndex argument change.
Addressed @MarcoFalke’s comment removing unused consensus params in src/node/transaction.cpp
Addressed @MarcoFalke’s comment making sure the lambda is defined in one, atomic commit.
maflcko
commented at 7:31 am on May 10, 2023:
member
trivial rebase re-ACK8f94f059b3af5ecaf175a95389ba5e73b724203b 🗝
Signature:
0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: trivial rebase re-ACK8f94f059b3af5ecaf175a95389ba5e73b724203b 🗝
3IJvbWV8BH4qE1BxY+hGbsdP4EvakcWtP3Ryi2ezkt+UfLS/RJhQc4QJ6u9zxvmShvLdpsFomlooRZAMl7D3aCQ==
DrahtBot requested review from ryanofsky
on May 10, 2023
fanquake referenced this in commit
883766fa45
on May 10, 2023
refactor: Declare g_zmq_notification_interface as unique_ptr
Ensures better memory safety for this global. This came up during
discussion of the following commit, but is not strictly required for its
implementation.
8ed4ff8e05
TheCharlatan force-pushed
on May 10, 2023
TheCharlatan
commented at 11:16 am on May 10, 2023:
contributor
In commit “zmq: Pass lambda to zmq’s ZMQPublishRawBlockNotifier” (6247b3c5e3c9e70097243a85b3d883b279a473cc)
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:
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
In commit “zmq: Pass lambda to zmq’s ZMQPublishRawBlockNotifier” (6247b3c5e3c9e70097243a85b3d883b279a473cc)
Would probably be a little more efficient to make the get_block_by_index non-const and use m_get_block_by_index{std::move(get_block_by_index)} to move the function instead of copying it.
in
ci/test/06_script_b.sh:55
in
c065e093b1outdated
* Addressed [@ryanofsky](/bitcoin-bitcoin/contributor/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 886a473fc48f2c7d67436b5d9ac5643cd007e27f though, so would suggest reverting.
zmq: Pass lambda to zmq's ZMQPublishRawBlockNotifier
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.
cfbb212493
refactor: Move functions to 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.
f0bb1021f0
refactor/iwyu: Complete includes for blockmanager_argsa498d699e3
refactor, BlockManager: Replace fastprune from arg with options
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.
02a0899527
refactor, blockstorage: Replace blocksdir arg
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.
5ff63a09a9
fanquake assigned fanquake
on May 10, 2023
fanquake
commented at 5:16 pm on May 10, 2023:
member
0clang-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
1zmq/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]2return std::make_unique<CZMQPublishRawBlockNotifier>(std::move(gb));
3 ^~~~~~~~~~ ~
fanquake unassigned fanquake
on May 10, 2023
TheCharlatan force-pushed
on May 10, 2023
DrahtBot added the label
CI failed
on May 10, 2023
TheCharlatan
commented at 5:24 pm on May 10, 2023:
contributor
Addressed @ryanofsky’s comment, moving the lambda in the constructor.
ryanofsky approved
ryanofsky
commented at 5:35 pm on May 10, 2023:
contributor
Code review ACK5ff63a09a9edd1204b2cc56cf6f48a44adab7bb3. Since last ACK just added std::move and fixed commit title. Sorry for the noise!
DrahtBot requested review from maflcko
on May 10, 2023
in
src/node/transaction.h:57
in
f0bb1021f0outdated
53@@ -53,11 +54,10 @@ static const CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};
54 * @param[in] block_index The block to read from disk, or nullptr
55 * @param[in] mempool If provided, check mempool for tx
56 * @param[in] hash The txid
57- * @param[in] consensusParams The params
58 * @param[out] hashBlock The block hash, if the tx was found via -txindex or block_index
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-06-27 15:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
