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. |
stickies-v
changed the title
|
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. |
| 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 |
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".