refactor: consistently use ApplyArgsManOptions for PeerManager::Options #28148

1. 
   stickies-v commented at 1:42 pm on July 25, 2023: contributor

   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".

2. refactor: set ignore_incoming_txs in ApplyArgsManOptions

   Refactor to consistently use ApplyArgsManOptions to set all PeerManager::Options,
   including ignore_incoming_txs.

   5f41afcc46
3. refactor: deduplicate ignores_incoming_txs

   Initialize PeerManager::Options early to avoid reading -blocksonly twice.

   8a3159728a
4. 
   DrahtBot commented at 1:42 pm on July 25, 2023: contributor

   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	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.

   Conflicts

   No conflicts as of last run.

5. DrahtBot added the label Refactoring on Jul 25, 2023
6. stickies-v renamed this:
   Refactor: consistently use ApplyArgsManOptions for PeerManager::Options
   refactor: consistently use ApplyArgsManOptions for PeerManager::Options
   on Jul 25, 2023
7. TheCharlatan approved
8. 
   TheCharlatan commented at 7:15 pm on July 25, 2023: contributor

   ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da
9. dergoegge approved
10. 
    dergoegge commented at 8:04 pm on July 25, 2023: member

    utACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da

    Thanks for the follow-up!

11. 
    theuni commented at 9:22 pm on July 25, 2023: member

    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.
12. in src/init.cpp:1173 in 8a3159728a

    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);
    

    ---

    ---

    

    MarcoFalke commented at 5:52 am on July 26, 2023:

    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.

    ---

    

    stickies-v commented at 10:15 am on July 26, 2023:

    Agreed, makes more sense. Will move down if I have to retouch.
13. 
    MarcoFalke commented at 5:58 am on July 26, 2023: member

    lgtm ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da
14. 
    stickies-v commented at 10:23 am on July 26, 2023: contributor

    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.

15. 
    MarcoFalke commented at 10:43 am on July 26, 2023: member

    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?
16. in src/init.cpp:1221 in 8a3159728a

    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) {
    

    ---

    ---

    

    dergoegge commented at 12:12 pm on July 26, 2023:

    No strong opinion, but it would be perfectly fine to just do

    0    if (!args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)) {
    

    here and then keep the peerman opts were they are right now.

    ---

    

    stickies-v commented at 2:19 pm on July 26, 2023:

    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.
17. 
    stickies-v commented at 2:22 pm on July 26, 2023: contributor

    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.

18. 
    achow101 commented at 3:22 pm on July 27, 2023: member

    ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da
19. achow101 merged this on Jul 27, 2023
20. achow101 closed this on Jul 27, 2023

    ---

21. stickies-v deleted the branch on Jul 27, 2023

Contributors
stickies-v DrahtBot TheCharlatan dergoegge theuni MarcoFalke achow101

Labels
Refactoring