refactor: replace CConnman pointers by references in net_processing.cpp #19174
Conversation
|
Concept ACK, but this conflicts with some ongoing work, so let's get those in first. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
022ad4e to
bb725d8
Compare
|
Rebased. |
|
concept ACK. Needs rebase |
033a6e3 to
11e8464
Compare
|
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:
|
|
#19514 is merged. I'll review this as soon as it's rebased. |
11e8464 to
0c8461a
Compare
|
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 |
|
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 utACK 0c8461a |
|
ACK 0c8461a 🕧 Show signature and timestampSignature: Timestamp of file with hash |
|
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. |
…essing.cpp" This reverts commit 0c8461a. In discussion in bitcoin#19174 there was a concept NACK by the original author of the code. It was merged after this without any further discussion.
|
For posterity, concept NACK withdrawn. Agree with @MarcoFalke and @jnewbery above. ACK 0c8461a. |
…s in net_processing.cpp 0c8461a refactor: replace CConnman pointers by references in net_processing.cpp (Sebastian Falbesoner) Pull request description: This is a follow-up to the recently merged PR bitcoin#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 ACKs for top commit: jnewbery: utACK 0c8461a MarcoFalke: ACK 0c8461a 🕧 Tree-SHA512: 79dc05144bcfb5e0bbc62180285aadcc6199f044fa3016c0f54f7b7f45037415260970037bd63b18fafefb8aef448549dae14b780bafb540fa2373f493a17f71
Summary:
```
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
```
Backport of core [[bitcoin/bitcoin#19174 | PR19174]].
Depends on D8479.
Test Plan:
ninja all check-all
Reviewers: #bitcoin_abc, deadalnix
Reviewed By: #bitcoin_abc, deadalnix
Differential Revision: https://reviews.bitcoinabc.org/D8480
This is a follow-up to the recently merged PR #19053, replacing
two more types ofone more type of pointer (CConnman) by references to increase the code quality -- pointers should either check fornullptror 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,
CConnman*andpointersBanMan*