net: Move SocketSendData lock annotation to header #20864

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2101-netLock changing 2 files +892 −893
  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: contributor

    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: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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. DrahtBot cross-referenced this on Jan 6, 2021 from issue net: [refactor] Prefer integral types in CNodeStats by MarcoFalke
  9. DrahtBot cross-referenced this on Jan 6, 2021 from issue p2p: standardize outbound full/block relay connection type naming by jonatack
  10. DrahtBot cross-referenced this on Jan 6, 2021 from issue Cleanup of -debug=net log messages by ajtowns
  11. DrahtBot cross-referenced this on Jan 6, 2021 from issue doc: refer to BIPs 339/155 in feature negotiation by jonatack
  12. DrahtBot cross-referenced this on Jan 6, 2021 from issue Declare de facto const reference variables/member functions as const by practicalswift
  13. DrahtBot cross-referenced this on Jan 6, 2021 from issue Follow-ups to 19107 by troygiorshev
  14. 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

  15. jnewbery commented at 1:43 PM on January 6, 2021: member

    ACK fad2e1f267d60afe9799e431233f54f02d14e8e0

    Nice tidy-up.

  16. DrahtBot cross-referenced this on Jan 6, 2021 from issue net: don't bind on 0.0.0.0 if binds are restricted to Tor by vasild
  17. DrahtBot cross-referenced this on Jan 6, 2021 from issue addrman: Make addrman a top-level component by jnewbery
  18. DrahtBot cross-referenced this on Jan 6, 2021 from issue net: fix GetListenPort() to derive the proper port by vasild
  19. DrahtBot cross-referenced this on Jan 6, 2021 from issue Refactoring and minor improvement for self-advertisements by naumenkogs
  20. DrahtBot cross-referenced this on Jan 6, 2021 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky
  21. DrahtBot cross-referenced this on Jan 6, 2021 from issue [tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar
  22. DrahtBot cross-referenced this on Jan 6, 2021 from issue net: Replace cs_feeFilter with simple std::atomic by MarcoFalke
  23. DrahtBot cross-referenced this on Jan 6, 2021 from issue net: Add NAT-PMP port forwarding support by hebasto
  24. DrahtBot cross-referenced this on Jan 6, 2021 from issue Multiprocess bitcoin by ryanofsky
  25. theStack approved
  26. theStack commented at 6:34 PM on January 6, 2021: contributor

    ACK fad2e1f267d60afe9799e431233f54f02d14e8e0 With the suggestions in the commit messages on how to best show the diffs, the review was quite straight-forward and painless. 👌

  27. ajtowns commented at 1:07 AM on January 7, 2021: contributor

    ACK fad2e1f267d60afe9799e431233f54f02d14e8e0

  28. ajtowns cross-referenced this on Jan 7, 2021 from issue refactor: move net_processing implementation details out of header by ajtowns
  29. DrahtBot added the label Needs rebase on Jan 7, 2021
  30. MarcoFalke force-pushed on Jan 7, 2021
  31. 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

  32. net: Move CConnman/NetEventsInterface after CNode in header file
    Can be reviewed with --color-moved=dimmed-zebra --patience
    fa0a71781a
  33. net: Move SocketSendData lock annotation to header
    Also, add lock annotation to SendMessages

Can be reviewed with "--word-diff-regex=."
    fa210689e2
  34. MarcoFalke force-pushed on Jan 7, 2021
  35. DrahtBot removed the label Needs rebase on Jan 7, 2021
  36. jnewbery commented at 9:20 AM on January 7, 2021: member

    utACK fa210689e2

  37. MarcoFalke merged this on Jan 7, 2021
  38. MarcoFalke closed this on Jan 7, 2021
  39. MarcoFalke deleted the branch on Jan 7, 2021
  40. sidhujag referenced this in commit 52a356407c on Jan 7, 2021
  41. Fabcien referenced this in commit 018d4cef0c on Jan 25, 2022
  42. Fabcien referenced this in commit 13db675708 on Jan 25, 2022
  43. bitcoin locked this on Aug 16, 2022
Contributors
MarcoFalkeajtownsDrahtBotjnewberytheStack
Labels
RefactoringP2P
Linked (view graph)
#20811 refactor: move net_processing implementation details out of header
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:53 UTC