[net] Consolidate logic around calling CAddrMan::Connected() #20291
Conversation
Currently, the logic around whether we called CAddrMan::Connected() for a peer is spread between verack processing (where we discard inbound peers) and FinalizeNode (where we discard misbehaving and block-relay-only peers). Consolidate that logic to a single place. Also remove the CNode.fCurrentlyConnected bool, which is now redundant. We can rely on CNode.fSuccessfullyConnected, since the two bools were only ever flipped to true in the same place.
|
I originally suggested this as a follow up to #20187: #20187 (comment) |
|
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. |
|
Concept ACK. How about adding a commit to rename |
I haven't changed the name, but I've updated the documentation, so at least it should be obvious to readers what the function is doing. I think we need to reconsider our whole approach to nTime/nLastSuccess, and potentially remove nTime for addr messages altogether (see https://github.com/bitcoin-core/bitcoin-devwiki/wiki/P2P-IRC-meetings#20-oct-2020). For now, I think updating the function comment is enough. |
|
Concept ACK |
amitiuttarwar
left a comment
There was a problem hiding this comment.
code review ACK 0bfce9d. nice tidy, and bonus that we get to remove an unnecessary call to cs_main
one slightly tangential question- when looking for other touch points of nTime, I came across this comment:
Lines 206 to 208 in c772f4c
but from reading the function definition, I don't understand how. can it really update the existing
nTime of a CAddress? looks like both the header comment and function were added in 5fee401, so seems likely I'm misunderstanding the code.
No, you're not misunderstanding. That comment is just plain wrong. It doesn't find the entry, it always creates it. And it doesn't update the nTime and nServices. I think it's probably vestigial from when the code was being written. Perhaps logic was moved around (what it's describing is closer to what the |
|
Code review ACK 0bfce9d |
…:Connected() 0bfce9d [addrman] Fix Connected() comment (John Newbery) eefe194 [net] Consolidate logic around calling CAddrMan::Connected() (John Newbery) Pull request description: Currently, the logic around whether we called CAddrMan::Connected() for a peer is spread between verack processing (where we discard inbound peers) and FinalizeNode (where we discard misbehaving and block-relay-only peers). Consolidate that logic to a single place. Also remove the CNode.fCurrentlyConnected bool, which is now redundant. We can rely on CNode.fSuccessfullyConnected, since the two bools were only ever flipped to true in the same place. ACKs for top commit: mzumsande: Code review ACK 0bfce9d amitiuttarwar: code review ACK 0bfce9d. nice tidy, and bonus that we get to remove an unnecessary call to `cs_main` Tree-SHA512: 1ab74dae3bc12a6846da57c853033e546bb4f91caa39f4c50bf0cf7eca59cb917bdb2ef795da55363e7e9f70949cb28bb3be004cb3afa4389f970d2fe097d932
Summary: Currently, the logic around whether we called `CAddrMan::Connected()` for a peer is spread between verack processing (where we discard inbound peers) and `FinalizeNode` (where we discard misbehaving and block-relay-only peers). Consolidate that logic to a single place. Also remove the `CNode.fCurrentlyConnected` bool, which is now redundant. We can rely on `CNode.fSuccessfullyConnected`, since the two bools were only ever flipped to true in the same place. This is a backport of [[bitcoin/bitcoin#20291 | core#20291]] [1/2] bitcoin/bitcoin@eefe194 Depends on D10697 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10698
Summary: This is a backport of [[bitcoin/bitcoin#20291 | core#20291]] [2/2] bitcoin/bitcoin@0bfce9d Depends on D10698 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10699
Currently, the logic around whether we called CAddrMan::Connected() for
a peer is spread between verack processing (where we discard inbound
peers) and FinalizeNode (where we discard misbehaving and
block-relay-only peers). Consolidate that logic to a single place.
Also remove the CNode.fCurrentlyConnected bool, which is now
redundant. We can rely on CNode.fSuccessfullyConnected, since the two
bools were only ever flipped to true in the same place.