net: Move SocketSendData lock annotation to header #20864

1. 
   MarcoFalke commented at 7:44 am on January 6, 2021: member

   Lock annotations must be in the header, otherwise the will have limited or no effect
2. MarcoFalke added the label Refactoring on Jan 6, 2021
3. MarcoFalke added the label P2P on Jan 6, 2021
4. 
   ajtowns commented at 8:33 am on January 6, 2021: member

   Concept ACK.

   If you’re moving things around in net.h to add lock annotations, might be worth moving NetEventsInterface to after the CNode definition, and making it virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(pnode->cs_sendProcessing) = 0;.

   Moving IsPeerAddrLocalGood and AdvertiseLocal later as well would allow removing the class CNode; forward declaration entirely.

   It would be nice if there were some easy way to review pointer-becomes-reference changes.

5. MarcoFalke force-pushed on Jan 6, 2021
6. 
   MarcoFalke commented at 8:46 am on January 6, 2021: member

   It would be nice if there were some easy way to review pointer-becomes-reference changes.

   Apart from --word-diff-regex=.?

   SendMessages …

   Thanks, done

7. 
   DrahtBot commented at 12:03 pm on January 6, 2021: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #20811 (refactor: move net_processing implementation details out of header by ajtowns)
   • #20786 (net: [refactor] Prefer integral types in CNodeStats by MarcoFalke)
   • #20729 (p2p: standardize on outbound-{full, block}-relay connection type naming by jonatack)
   • #20724 (Cleanup of -debug=net log messages by ajtowns)
   • #20646 (doc: refer to BIPs 339/155 in feature negotiation by jonatack)
   • #20364 (Follow-ups to 19107 by troygiorshev)
   • #20234 (net: don’t extra bind for Tor if binds are restricted by vasild)
   • #20228 (addrman: Make addrman a top-level component by jnewbery)
   • #20196 (net: fix GetListenPort() to derive the proper port by vasild)
   • #19843 (Refactoring and minor improvement for self-advertisements by naumenkogs)
   • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
   • #19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)
   • #18819 (net: Replace cs_feeFilter with simple std::atomic by MarcoFalke)
   • #18077 (net: Add NAT-PMP port forwarding support by hebasto)
   • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

   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.

8. in src/net.h:1216 in fad2e1f267 outdated

   2080 
   2081-    /** Whether this peer is an inbound onion, e.g. connected via our Tor onion service. */
   2082-    bool IsInboundOnion() const { return m_inbound_onion; }
   2083+    friend struct CConnmanTest;
   2084+    friend struct ConnmanTestMsg;
   2085 };
   

   ---

   ---

   

   jnewbery commented at 1:40 pm on January 6, 2021:

   If you touch this again, add a blank line after the class declaration.

   ---

   

   MarcoFalke commented at 8:42 am on January 7, 2021:

   thanks, done
9. 
   jnewbery commented at 1:43 pm on January 6, 2021: member

   ACK fad2e1f267d60afe9799e431233f54f02d14e8e0

   Nice tidy-up.

10. theStack approved
11. 
    theStack commented at 6:34 pm on January 6, 2021: member

    ACK fad2e1f267d60afe9799e431233f54f02d14e8e0 With the suggestions in the commit messages on how to best show the diffs, the review was quite straight-forward and painless. 👌
12. 
    ajtowns commented at 1:07 am on January 7, 2021: member

    ACK fad2e1f267d60afe9799e431233f54f02d14e8e0
13. DrahtBot added the label Needs rebase on Jan 7, 2021
14. MarcoFalke force-pushed on Jan 7, 2021
15. 
    MarcoFalke commented at 8:39 am on January 7, 2021: member

    Rebased, should be trivial to re-ACK with git range-diff or from scratch
16. net: Move CConnman/NetEventsInterface after CNode in header file

    Can be reviewed with --color-moved=dimmed-zebra --patience

    fa0a71781a
17. net: Move SocketSendData lock annotation to header

    Also, add lock annotation to SendMessages
    
    Can be reviewed with "--word-diff-regex=."

    fa210689e2
18. MarcoFalke force-pushed on Jan 7, 2021
19. DrahtBot removed the label Needs rebase on Jan 7, 2021
20. 
    jnewbery commented at 9:20 am on January 7, 2021: member

    utACK fa210689e2
21. MarcoFalke merged this on Jan 7, 2021
22. MarcoFalke closed this on Jan 7, 2021

    ---

23. MarcoFalke deleted the branch on Jan 7, 2021
24. sidhujag referenced this in commit 52a356407c on Jan 7, 2021
25. Fabcien referenced this in commit 018d4cef0c on Jan 25, 2022
26. Fabcien referenced this in commit 13db675708 on Jan 25, 2022
27. DrahtBot locked this on Aug 16, 2022

Contributors
MarcoFalke ajtowns DrahtBot jnewbery theStack

Labels
Refactoring P2P