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

theStack · 2020-06-05T11:29:52Z

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

maflcko · 2020-06-05T11:36:33Z

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

DrahtBot · 2020-06-05T13:26:11Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Add parameter feature to serialization and use it for CAddress #19503 (Add parameter feature to serialization and use it for CAddress by sipa)
Tidy up ProcessOrphanTx #19498 (Tidy up ProcessOrphanTx by jnewbery)
Overhaul transaction request logic #19184 (Overhaul transaction request logic by sipa)
Cache responses to GETADDR to prevent topology leaks #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.

promag

Concept ACK, no rush though.

theStack · 2020-06-10T13:41:21Z

Rebased.

jnewbery · 2020-07-13T14:59:21Z

concept ACK. Needs rebase

theStack · 2020-07-13T17:15:34Z

Rebased.

jnewbery · 2020-07-13T20:58:59Z

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.

theStack · 2020-07-14T11:53:50Z

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

jnewbery · 2020-07-14T13:53:04Z

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

theStack · 2020-07-14T14:11:57Z

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

theuni · 2020-07-14T20:51:58Z

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.

maflcko · 2020-07-15T06:59:07Z

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.

jnewbery · 2020-07-15T08:30:34Z

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 0c8461a

maflcko · 2020-07-16T06:07:02Z

ACK 0c8461a 🕧

Show signature and timestamp

Signature:

Timestamp of file with hash 5a8ecf4a91cf13cdd18a3b4e4f73b5e1b8bece873c6f28a95508698de9f4917e -

JeremyRubin · 2020-07-16T16:21:02Z

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.

fanquake · 2020-07-17T06:27:01Z

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

laanwj · 2020-07-17T09:50:03Z

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.

theuni · 2020-07-17T20:32:48Z

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

fanquake added the Refactoring label Jun 5, 2020

promag reviewed Jun 6, 2020

View reviewed changes

DrahtBot mentioned this pull request Jun 6, 2020

Overhaul transaction request logic #19184

Closed

DrahtBot added the Needs rebase label Jun 6, 2020

theStack force-pushed the 20200602-refactor-use-cconnman-references-within-net_processing branch from 022ad4e to bb725d8 Compare June 10, 2020 13:40

DrahtBot removed the Needs rebase label Jun 10, 2020

This was referenced Jun 17, 2020

[net] Cleanup logic around connection types #19316

Merged

[tests] Allow outbound & block-relay-only connections in functional tests. #19315

Merged

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

DrahtBot added the Needs rebase label Jun 19, 2020

theStack force-pushed the 20200602-refactor-use-cconnman-references-within-net_processing branch 2 times, most recently from 033a6e3 to 11e8464 Compare July 13, 2020 16:33

DrahtBot removed the Needs rebase label Jul 13, 2020

DrahtBot mentioned this pull request Jul 13, 2020

Tidy up ProcessOrphanTx #19498

Merged

DrahtBot mentioned this pull request Jul 14, 2020

Add parameter feature to serialization and use it for CAddress #19503

Closed

DrahtBot added the Needs rebase label Jul 14, 2020

refactor: replace CConnman pointers by references in net_processing.cpp

0c8461a

theStack force-pushed the 20200602-refactor-use-cconnman-references-within-net_processing branch from 11e8464 to 0c8461a Compare July 14, 2020 14:08

theStack changed the title ~~refactor: replace CConnman/BanMan pointers by references in net_processing.cpp~~ refactor: replace CConnman pointers by references in net_processing.cpp Jul 14, 2020

DrahtBot removed the Needs rebase label Jul 14, 2020

maflcko merged commit affed84 into bitcoin:master Jul 16, 2020

laanwj mentioned this pull request Jul 17, 2020

Revert "refactor: replace CConnman pointers by references in net_processing.cpp" #19542

Closed

hebasto mentioned this pull request Jul 21, 2020

p2p: Try to preserve outbound block-relay-only connections during restart #17428

Merged

jnewbery mentioned this pull request Aug 12, 2020

Net processing: move ProcessMessage() to PeerLogicValidation #19704

Merged

theStack deleted the 20200602-refactor-use-cconnman-references-within-net_processing branch December 1, 2020 09:58

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

theStack commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 5, 2020

Uh oh!

DrahtBot commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Jun 10, 2020

Uh oh!

jnewbery commented Jul 13, 2020

Uh oh!

theStack commented Jul 13, 2020

Uh oh!

jnewbery commented Jul 13, 2020

Uh oh!

theStack commented Jul 14, 2020

Uh oh!

jnewbery commented Jul 14, 2020

Uh oh!

theStack commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Jul 14, 2020

Uh oh!

maflcko commented Jul 15, 2020

Uh oh!

jnewbery commented Jul 15, 2020

Uh oh!

maflcko commented Jul 16, 2020

Uh oh!

JeremyRubin commented Jul 16, 2020

Uh oh!

fanquake commented Jul 17, 2020

Uh oh!

laanwj commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Jul 17, 2020

Uh oh!

Uh oh!

theStack commented Jun 5, 2020 •

edited

Loading

DrahtBot commented Jun 5, 2020 •

edited

Loading

theStack commented Jul 14, 2020 •

edited

Loading

laanwj commented Jul 17, 2020 •

edited

Loading