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.
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 (review)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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 CAddrMan::Connected (which is only ever called during disconnection) and correct the documentation, as suggested in #20187 (review) and #20187#pullrequestreview-521189824 ?
How about adding a commit to rename CAddrMan::Connected (which is only ever called during disconnection) and correct the documentation
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
685 | @@ -676,7 +686,7 @@ friend class CAddrManTest; 686 | return vAddr; 687 | } 688 | 689 | - //! Mark an entry as currently-connected-to. 690 | + //! Outer function for Connected_()
this comment seems unnecessary- addrman uses this convention of external Function() & internal Function_() in several places.
(but ofc not a big deal if it stays)
I like all functions to have doxygen comments, even if they're short and seem unnecessary. I prefer this to say "outer function" than just be a duplicate of the comment for the inner function, which is what it was before.
257 | @@ -258,8 +258,18 @@ friend class CAddrManTest; 258 | //! Select several addresses at once. 259 | void GetAddr_(std::vector<CAddress> &vAddr, size_t max_addresses, size_t max_pct) EXCLUSIVE_LOCKS_REQUIRED(cs); 260 | 261 | - //! Mark an entry as currently-connected-to. 262 | - void Connected_(const CService &addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs); 263 | + /** We have successfully connected to this peer. Calling this function 264 | + * updates the CAddress's nTime, which is used in our IsTerrible()
might be worth adding a word or two that indicates these are our primary uses of nTime, but not the only ones. the comment got me curious so I dove in and looked around, but its not the easiest to sift through a grep for nTime 😅
* updates the CAddress's nTime, which is primarily used in our IsTerrible()
incase curious, some other times we read from nTime are when processing an ADDR message & deciding whether to relay & for debug printing in ConnectNode
some other times we read from nTime are when processing an ADDR message & deciding whether to relay
Ah. This is using the nTime from a received addr, so not quite the same as the nTime stored in a CAddress in addrman.
for debug printing in ConnectNode
Yes, this is true. The nTime is logged here.
I think I'm right in saying that the only functional uses of nTime in a CAddress in addrman are in IsTerrible() and when relaying the CAddress to a peer in response to a getaddr.
I think I'm right in saying that the only functional uses of nTime in a CAddress in addrman are in IsTerrible() and when relaying the CAddress to a peer in response to a getaddr.
yup, I agree. thats why my suggestion was just to clarify that difference (esp since grep doesn't immediately make this clear). not a big deal though, maybe to consider if you touch up the code for another reason.
Ah. This is using the nTime from a received addr, so not quite the same as the nTime stored in a CAddress in addrman.
hm well, looks like we read the addr into a CAddress object and then decide whether or not to add it to our addrman? ofc the functionality of Connected_ doesn't have the ability to impact the value at that time, but its still the same CAddress.nTime field thats being read?
code review ACK 0bfce9dc46. 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: https://github.com/bitcoin/bitcoin/blob/c772f4cb04aa2edc04d849039b07c5a454d3f666/src/addrman.h#L206-L208
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 https://github.com/bitcoin/bitcoin/commit/5fee401fe14aa6459428a26a82f764db70a6a0b9, so seems likely I'm misunderstanding the code.
one slightly tangential question- when looking for other touch points of nTime, I came across this comment:
// find an entry, creating it if necessary. // nTime and nServices of found node is updated, if necessary. CAddrInfo* Create(const CAddress &addr, const CNetAddr &addrSource, int *pnId = NULL);
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 Add_() function does), but it's inaccurate now.
ack 0bfce9dc46234b196a8b3679c21d6f8455962495
Code review ACK 0bfce9dc46234b196a8b3679c21d6f8455962495
851 | @@ -855,8 +852,9 @@ void PeerManager::FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) { 852 | if (state->fSyncStarted) 853 | nSyncStarted--; 854 | 855 | - if (misbehavior == 0 && state->fCurrentlyConnected && !node.IsBlockOnlyConn()) { 856 | - // Note: we avoid changing visible addrman state for block-relay-only peers 857 | + if (node.fSuccessfullyConnected && misbehavior == 0 && 858 | + !node.IsBlockOnlyConn() && !node.IsInboundConn()) { 859 | + // Only change visible addrman state for outbound, full-relay peers
Perhaps it's worth explicitly documenting that we do not call Connected() for FEELER connections? (This behavior emerges from feelers not having the fSuccessfullyConnected bool set, and I think it's desirable to maintain this behavior because otherwise long-lived block-relay-only connections might stick out as the ones which haven't been updated in a while.)