Consistently use ApplyArgsManOptions for PeerManager::Options, and initialize PeerManager::Options early to avoid reading "-blocksonly" twice. Suggested in #27499 (review) and also requested in #27499 (review).
No behaviour change, but the TestingSetup is now also able to access "-blocksonly".
Refactor to consistently use ApplyArgsManOptions to set all PeerManager::Options,
including ignore_incoming_txs.
Initialize PeerManager::Options early to avoid reading -blocksonly twice.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, dergoegge, MarcoFalke, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da
utACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da
Thanks for the follow-up!
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.
1167 | @@ -1168,7 +1168,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) 1168 | 1169 | fListen = args.GetBoolArg("-listen", DEFAULT_LISTEN); 1170 | fDiscover = args.GetBoolArg("-discover", true); 1171 | - const bool ignores_incoming_txs{args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)}; 1172 | + 1173 | + PeerManager::Options peerman_opts{}; 1174 | + ApplyArgsManOptions(args, peerman_opts);
unrelated in 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da: I think peermanager options have nothing to do with addrmanager options, so this could be moved down, closer to where it is used.
Agreed, makes more sense. Will move down if I have to retouch.
lgtm ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da
I'm not sure that I love using a subsystem's opts as a cache for init vars.
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 ignores_incoming_txs var, but I'm happy to drop the second commit if that's what people prefer.
You could add back the const bool ignores_incoming_txs{peerman_opts.ignore_incoming_txs}; alias? But I don't see what is iffy about the code. Are you saying it would be cleaner for node.fee_estimator to be constructed after the peerman, and using the peerman property instead of the peerman options property?
1217 | @@ -1216,7 +1218,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) 1218 | assert(!node.fee_estimator); 1219 | // Don't initialize fee estimation with old data if we don't relay transactions, 1220 | // as they would never get updated. 1221 | - if (!ignores_incoming_txs) { 1222 | + if (!peerman_opts.ignore_incoming_txs) {
No strong opinion, but it would be perfectly fine to just do
if (!args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)) {
here and then keep the peerman opts were they are right now.
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.
But I don't see what is iffy about the code.
Using a PeerManager::Options object to initialize fee estimation is what irks me a bit. I suppose using peerman instead of the options object would be a bit more elegant indeed, but not worth the code rearrangement atm.
Looks like everyone's happy with the approach as is, so am going to refrain from further changes for now.
ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da