refactor: Move m_num_unconnecting_headers_msgs out of cs_main guard #25555

1. 
   maflcko commented at 3:01 pm on July 6, 2022: member

   The variable does not need to be guarded by cs_main, so move it out of the cs_main scope. It is not used for validation-specific state, but only for headers-processing internal state and logging.
2. fanquake added the label Refactoring on Jul 6, 2022
3. in src/net_processing.cpp:2405 in fa34e723d1 outdated

   2221@@ -2221,28 +2222,26 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
   2222  *
   2223  * We'll send a getheaders message in response to try to connect the chain.
   2224  *
   2225- * The peer can send up to MAX_UNCONNECTING_HEADERS in a row that
   2226+ * The peer can send up to MAX_UNCONNECTING_HEADERS_MSGS in a row that
   

   ---

   ---

   

   sdaftuar commented at 6:13 pm on July 6, 2022:

   Care to address this comment while you’re touching this line?

   ---

   

   maflcko commented at 6:57 pm on July 6, 2022:

   Thanks, done.
4. in src/net_processing.cpp:254 in fa34e723d1 outdated

   249@@ -250,6 +250,9 @@ struct Peer {
   250       * to the peer. */
   251     std::chrono::microseconds m_next_send_feefilter{0};
   252 
   253+    //! Length of current-streak of unconnecting headers announcements
   254+    int num_unconnecting_headers_msgs{0};
   

   ---

   ---

   

   sdaftuar commented at 6:14 pm on July 6, 2022:

   Should this be named m_num_unconnecting_headers_msgs for consistency with the style guide?

   ---

   

   maflcko commented at 6:57 pm on July 6, 2022:

   Peer is a struct, so it shouldn’t use m_ prefixes. To quote the style guide:

   Class member variables have a m_ prefix.

   For rationale, see for example #19759

   (So in theory #25557 could drop the m_ prefix)

   Though, I don’t care about the style guide too much, so I am happy to change this to make reviewers happy and I am also not going to comment on #25557 to suggest a change.

   ---

   

   sdaftuar commented at 7:34 pm on July 6, 2022:

   Thanks for pointing me to that (and now I’m kicking myself for all those m_’s!)
5. maflcko force-pushed on Jul 6, 2022
6. 
   DrahtBot commented at 3:13 am on July 7, 2022: contributor

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #26140 (refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer by dergoegge)
   • #24571 (p2p: Prevent block index fingerprinting by sending additional getheaders messages by dergoegge)

   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.

7. DrahtBot added the label Needs rebase on Jul 19, 2022
8. maflcko renamed this:
   refactor: Move num_unconnecting_headers_msgs out of cs_main guard
   refactor: Move m_num_unconnecting_headers_msgs out of cs_main guard
   on Jul 19, 2022
9. maflcko force-pushed on Jul 19, 2022
10. DrahtBot removed the label Needs rebase on Jul 19, 2022
11. DrahtBot added the label Needs rebase on Aug 30, 2022
12. 
    achow101 commented at 7:42 pm on October 12, 2022: member

    Are you still working on this?
13. refactor: Move m_num_unconnecting_headers_msgs out of cs_main guard fad5d825a7
14. maflcko force-pushed on Oct 24, 2022
15. maflcko force-pushed on Oct 24, 2022
16. DrahtBot removed the label Needs rebase on Oct 24, 2022
17. 
    maflcko commented at 1:15 pm on October 24, 2022: member

    Rebased, but closing for now to pick it up later
18. maflcko closed this on Oct 24, 2022

    ---

19. maflcko deleted the branch on Oct 24, 2022
20. bitcoin locked this on Oct 24, 2023

Contributors
maflcko sdaftuar DrahtBot achow101

Labels
Refactoring