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

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

Concept ACK, but this conflicts with some ongoing work, so let's get those in first.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

Concept ACK, no rush though.

Rebased.

concept ACK. Needs rebase

Rebased.

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.

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

#19514 is merged. I'll review this as soon as it's rebased.

Removed BanMan pointer by reference replacements (only leaving CConnman replacements), rebased on master, adapted PR title and PR description accordingly.

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.

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.

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

ACK 0c8461a88ed66a1f70559fc96646708949b17e4b 🕧

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 0c8461a88ed66a1f70559fc96646708949b17e4b 🕧
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUht4Qv/SineQytgBJXUxI6I4QLHmS7miAD8FwB1cYzr9D4kXhIXNifLt5d/c1tR
RnkkKmimrCeiy6IRXKkDSYt4wq9OKGBFdbbs+xiGNRz3CVBtMDwXEynkPKQc00V3
2n01vRPfMKwsE3h/DK2GezxCily+u2vEMQGm07lmxThie00YtUPkWObmxFFKxTcZ
De6GhMZmzsVmR105lU4UWik75n6YeWShmY4qnJJrSWyDQyc0FnRCUHC2JIcrLhMG
Hkurn8+NUo9t4hVNwsdpaBjQu7yEDA4Lpg2scC6ohHmrrrK8OkQoyz3ED5GKkSh3
ETkVZXWM58NHhLGU4saMwVe0/hcYTiVAMbtBlLcc4EmBeEuwHZQZj2X0YC2kPrhS
ijguxYEthzTtsbFa7zQ7Q60DbEFV4xtIa6Q9GpuivpFsK1D1Wul32HIM7qcGgCLp
N70AvShj8+EZRYFbwK8dmVObQshIrGar+X1JkhjR2sp8gRSowb+dveBt/t6J68sm
P5ahRdA8
=MB6Y
-----END PGP SIGNATURE-----

Timestamp of file with hash 5a8ecf4a91cf13cdd18a3b4e4f73b5e1b8bece873c6f28a95508698de9f4917e -

</details>

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.

I agree. I would be good to have at-least heard @theuni's thoughts here.

I'm going to revert this. I don't think there was enough agreement to make this change. See #19542.

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

ACK 0c8461a88ed66a1f70559fc96646708949b17e4b.