refactor: consistently use ApplyArgsManOptions for PeerManager::Options #28148
Conversation
Refactor to consistently use ApplyArgsManOptions to set all PeerManager::Options, including ignore_incoming_txs.
Initialize PeerManager::Options early to avoid reading -blocksonly twice.
|
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. ConflictsNo conflicts as of last run. |
|
I'm not sure that I love using a subsystem's opts as a cache for init vars. But the consistency reasoning makes sense, so ~0 on that. |
8000
src/init.cpp
| const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)}; | ||
|
|
||
| PeerManager::Options peerman_opts{}; | ||
| ApplyArgsManOptions(args, peerman_opts); |
There was a problem hiding this comment.
unrelated in 8a31597: I think peermanager options have nothing to do with addrmanager options, so this could be moved down, closer to where it is used.
There was a problem hiding this comment.
Agreed, makes more sense. Will move down if I have to retouch.
|
lgtm ACK 8a31597 |
I felt iffy about it too (as per my initial suggestion), it's not the most elegant. I do think using a single var is safer and more clear than having a separate |
|
You could add back the |
| @@ -1216,7 +1218,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) | |||
| assert(!node.fee_estimator); | |||
| // Don't initialize fee estimation with old data if we don't relay transactions, | |||
| // as they would never get updated. | |||
| if (!ignores_incoming_txs) { | |||
| if (!peerman_opts.ignore_incoming_txs) { | |||
There was a problem hiding this comment.
No strong opinion, but it would be perfectly fine to just do
| if (!peerman_opts.ignore_incoming_txs) { | |
| if (!args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)) { |
here and then keep the peerman opts were they are right now.
There was a problem hiding this comment.
Yeah, that would be my approach if not the current one. But going to keep as is for now since everyone seems to be happy with the current approach.
Using a Looks like everyone's happy with the approach as is, so am going to refrain from further changes for now. |
|
ACK 8a31597 |
Summary: [net processing] Introduce PeerManager options bitcoin/bitcoin@8b87725 ----- [net processing] Use ignore_incoming_txs from m_opts bitcoin/bitcoin@4cfb7b9 ----- [net processing] Move -maxorphantx to PeerManager::Options bitcoin/bitcoin@567c4e0 ----- [net processing] Move -blockreconstructionextratxn to PeerManager::Options bitcoin/bitcoin@bd59bda ----- [net processing] Move -capturemessages to PeerManager::Options bitcoin/bitcoin@23c7b51 Note that we cannot yet remove `include <common/args.h>` The remaining calls to gArgs in net_processing.cpp are mostly related to avalanche options, which I will move in a separate diff. ----- refactor: set ignore_incoming_txs in ApplyArgsManOptions Refactor to consistently use ApplyArgsManOptions to set all PeerManager::Options, including ignore_incoming_txs. bitcoin/bitcoin@8a31597 Note we don't have the duplicated reading of -blocksonly, this is used by core for initializing some fee estimation machinery. ----- [net_processing] move -maxaddrtosend to PeerManager::Options Note that this option was added by us in D10823 ----- This is a backport of [[bitcoin/bitcoin#27499 | core#27499]] and [[bitcoin/bitcoin#28148 | core#28148]] It it missing the -txreconciliation related commit which is part of a feature that we have not yet backported. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D16455
Consistently use
ApplyArgsManOptionsforPeerManager::Options, and initializePeerManager::Optionsearly to avoid reading"-blocksonly"twice. Suggested in #27499 (comment) and also requested in #27499 (comment).No behaviour change, but the
TestingSetupis now also able to access"-blocksonly".