refactor: replace CConnman pointers by references in net_processing.cpp #19174

1. 
   theStack commented at 11:29 am on June 5, 2020: member

   This is a follow-up to the recently merged PR #19053, replacing two more types of one more type of pointer (CConnman) by references to increase the code quality – pointers should either check for nullptr or be replaced by references, and the latter strategy seems to be more reasonable.

   Again, to keep the review burden managable, the changes are kept simple,

   • only tackling CConnman* and BanMan* pointers
   • only within the net_processing module, i.e. no changes that would need adaption in other modules
   • keeping the names of the variables as they are
2. fanquake added the label Refactoring on Jun 5, 2020
3. 
   MarcoFalke commented at 11:36 am on June 5, 2020: member

   Concept ACK, but this conflicts with some ongoing work, so let’s get those in first.
4. 
   DrahtBot commented at 1:26 pm on June 5, 2020: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #19503 (Add parameter feature to serialization and use it for CAddress by sipa)
   • #19498 (Tidy up ProcessOrphanTx by jnewbery)
   • #19184 (Overhaul transaction request logic by sipa)
   • #18991 (Cache responses to GETADDR to prevent topology leaks by naumenkogs)

   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.

5. 
   promag commented at 0:24 am on June 6, 2020: member

   Concept ACK, no rush though.
6. DrahtBot added the label Needs rebase on Jun 6, 2020
7. theStack force-pushed on Jun 10, 2020
8. 
   theStack commented at 1:41 pm on June 10, 2020: member

   Rebased.
9. DrahtBot removed the label Needs rebase on Jun 10, 2020
10. DrahtBot added the label Needs rebase on Jun 19, 2020
11. 
    jnewbery commented at 2:59 pm on July 13, 2020: member

    concept ACK. Needs rebase
12. theStack force-pushed on Jul 13, 2020
13. theStack force-pushed on Jul 13, 2020
14. DrahtBot removed the label Needs rebase on Jul 13, 2020
15. 
    theStack commented at 5:15 pm on July 13, 2020: member

    Rebased.
16. 
    jnewbery commented at 8:58 pm on July 13, 2020: member

    I’m modifying my concept ACK to a partial concept ACK. @theuni has pointed out to me that for maximum flexibility, we should potentially be able to run the software with different components disabled, eg it should be possible to run with connman or banman disabled. It’s not possible to run net_processing without a connman, but it should be possible to run without a banman, and that possibility was part of banman’s Original Vision. Therefore, I think this PR should be modified to:

    • change connman pointers to references within net_processing
    • wrap all banman derefernces in net_processing inside if (banman) conditionals
    • add comments inside the PeerValidationLogic definition in net_processing.h explaining that.
17. 
    theStack commented at 11:53 am on July 14, 2020: member

    @jnewbery: I see your PR #19514 already covers points 2 and 3 of your list. Will remove the BanMan pointer by references replacements and rebase on master as soon as #19514 is merged.
18. DrahtBot added the label Needs rebase on Jul 14, 2020
19. 
    jnewbery commented at 1:53 pm on July 14, 2020: member

    #19514 is merged. I’ll review this as soon as it’s rebased.
20. refactor: replace CConnman pointers by references in net_processing.cpp 0c8461a88e
21. theStack force-pushed on Jul 14, 2020
22. theStack renamed this:
    refactor: replace CConnman/BanMan pointers by references in net_processing.cpp
    refactor: replace CConnman pointers by references in net_processing.cpp
    on Jul 14, 2020
23. 
    theStack commented at 2:11 pm on July 14, 2020: member

    Removed BanMan pointer by reference replacements (only leaving CConnman replacements), rebased on master, adapted PR title and PR description accordingly.
24. DrahtBot removed the label Needs rebase on Jul 14, 2020
25. 
    theuni commented at 8:51 pm on July 14, 2020: member

    Concept NACK. CConnman is a pointer here because it’s intended to be (eventually) optional, as a way to run bitcoind without p2p (for only local rpc, wallet, etc.)

    Making it a reference is a step backwards, imo.

26. 
    MarcoFalke commented at 6:59 am on July 15, 2020: member

    This is only changing stuff inside the call path of PeerLogicValidation::ProcessMessages, a function which is impossible to call when connman is nullptr. I don’t see how this makes it harder to run without p2p.
27. 
    jnewbery commented at 8:30 am on July 15, 2020: member

    I tend to agree with @MarcoFalke here. I think if we ran bitcoind without p2p, then both ConnMan and PeerLogicValidation would be disabled. PeerLogicValidation can’t do anything without a ConnMan. Contrast that with BanMan, which PeerLogicValidation can very easily run without.

    The fact that connman is dereferenced everywhere without being checked first shows that there’s an expectation that net_processing always has a ConnMan. The alternative of adding nullptr checks to all of those dereferences seems like it’d make the code much less readable. @theuni - am I missing something important?

    utACK 0c8461a88ed66a1f70559fc96646708949b17e4b

28. 
    MarcoFalke commented at 6:07 am on July 16, 2020: member

    ACK 0c8461a88ed66a1f70559fc96646708949b17e4b 🕧

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 0c8461a88ed66a1f70559fc96646708949b17e4b 🕧
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUht4Qv/SineQytgBJXUxI6I4QLHmS7miAD8FwB1cYzr9D4kXhIXNifLt5d/c1tR
     8RnkkKmimrCeiy6IRXKkDSYt4wq9OKGBFdbbs+xiGNRz3CVBtMDwXEynkPKQc00V3
     92n01vRPfMKwsE3h/DK2GezxCily+u2vEMQGm07lmxThie00YtUPkWObmxFFKxTcZ
    10De6GhMZmzsVmR105lU4UWik75n6YeWShmY4qnJJrSWyDQyc0FnRCUHC2JIcrLhMG
    11Hkurn8+NUo9t4hVNwsdpaBjQu7yEDA4Lpg2scC6ohHmrrrK8OkQoyz3ED5GKkSh3
    12ETkVZXWM58NHhLGU4saMwVe0/hcYTiVAMbtBlLcc4EmBeEuwHZQZj2X0YC2kPrhS
    13ijguxYEthzTtsbFa7zQ7Q60DbEFV4xtIa6Q9GpuivpFsK1D1Wul32HIM7qcGgCLp
    14N70AvShj8+EZRYFbwK8dmVObQshIrGar+X1JkhjR2sp8gRSowb+dveBt/t6J68sm
    15P5ahRdA8
    16=MB6Y
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 5a8ecf4a91cf13cdd18a3b4e4f73b5e1b8bece873c6f28a95508698de9f4917e -

29. MarcoFalke merged this on Jul 16, 2020
30. MarcoFalke closed this on Jul 16, 2020

    ---

31. 
    JeremyRubin commented at 4:21 pm on July 16, 2020: contributor

    Process wise I don’t think that this should have been merged over @theuni’s concept nack, especially not without a reasonable amount of time to follow up.
32. 
    fanquake commented at 6:27 am on July 17, 2020: member

    I agree. I would be good to have at-least heard @theuni’s thoughts here.
33. 
    laanwj commented at 9:50 am on July 17, 2020: member

    I’m going to revert this. I don’t think there was enough agreement to make this change. See #19542.
34. laanwj referenced this in commit 1aff21b364 on Jul 17, 2020
35. 
    theuni commented at 8:32 pm on July 17, 2020: member

    For posterity, concept NACK withdrawn. Agree with @MarcoFalke and @jnewbery above.

    ACK 0c8461a88ed66a1f70559fc96646708949b17e4b.

36. sidhujag referenced this in commit 647a40216e on Jul 18, 2020
37. jasonbcox referenced this in commit 7ce32e64a0 on Nov 21, 2020
38. theStack deleted the branch on Dec 1, 2020
39. DrahtBot locked this on Feb 15, 2022

Contributors
theStack MarcoFalke DrahtBot promag jnewbery theuni JeremyRubin fanquake laanwj

Labels
Refactoring