RFC: Move Peer and PeerManagerImpl declarations to separate header #20925

pull jnewbery wants to merge 3 commits into bitcoin:master from jnewbery:2021-01-peermanimplh changing 6 files +245 −208
  1. jnewbery commented at 12:59 PM on January 13, 2021: member

    Not sure whether this is desirable, so marking as a proof of concept.

    This moves the Peer and PeerManagerImpl declarations to their own header file, peerman_impl.h, which can be included by net_processing.cpp and the test/bench/fuzz files.

    The benefits of this are:

    • PeerManagerImpl functions which are exposed through the PeerManager interface for testing, but would otherwise be private can be removed from PeerManager. That means that PeerManager truly is net_processing's minimal interface to expose to the rest of the program.
    • If a test needs to directly manipulate Peer objects, it can do so, since they're exposed in peerman_impl header.
  2. fanquake added the label P2P on Jan 13, 2021
  3. jnewbery commented at 1:00 PM on January 13, 2021: member

    This is a minimal implementation of what was first discussed here: #20758 (comment). Thoughts, @ajtowns?

  4. jnewbery renamed this:
    RFC: Move Peer and PeerManagerImpl to separate header
    RFC: Move Peer and PeerManagerImpl declarations to separate header
    on Jan 13, 2021
  5. DrahtBot commented at 6:49 PM on January 13, 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:

    • #21162 (Net Processing: Move RelayTransaction() into PeerManager by jnewbery)
    • #21160 (Net/Net processing: Move tx inventory into net_processing by jnewbery)
    • #21148 (Split orphan handling from net_processing into txorphanage by ajtowns)
    • #21061 ([p2p] Introduce node rebroadcast module by amitiuttarwar)
    • #20966 (banman: save the banlist in a JSON format on disk by vasild)
    • #20942 ([refactor] Move some net_processing globals into PeerManagerImpl by ajtowns)
    • #20729 (p2p: standardize outbound full/block relay connection type naming by jonatack)
    • #20721 (Net: Move ping data to net_processing by jnewbery)
    • #20295 (rpc: getblockfrompeer by Sjors)
    • #20228 (addrman: Make addrman a top-level component by jnewbery)
    • #18261 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

    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.

  6. jnewbery marked this as a draft on Jan 13, 2021
  7. DrahtBot cross-referenced this on Jan 13, 2021 from issue net-processing refactoring -- lose globals, move implementation details from .h to .cpp by ajtowns
  8. DrahtBot cross-referenced this on Jan 13, 2021 from issue p2p: standardize outbound full/block relay connection type naming by jonatack
  9. DrahtBot cross-referenced this on Jan 13, 2021 from issue Net: Move ping data to net_processing by jnewbery
  10. in src/Makefile.am:182 in be52d7e56d outdated 
     178 | @@ -179,6 +179,7 @@ BITCOIN_CORE_H = \
 179 |    noui.h \
 180 |    optional.h \
 181 |    outputtype.h \
 182 | +  peerman_imlp.h \
    ajtowns commented at 6:47 AM on January 14, 2021:

    "impl"

    jnewbery commented at 11:24 AM on January 14, 2021:

    gah

  11. ajtowns commented at 6:48 AM on January 14, 2021: contributor

    I'm not going to think seriously about this until having tried to pull orphan handling or similar into its own module, in the hopes that that alone will be enough to make testing work better.

  12. [net processing] Add peerman_impl.h 69a7251d05
  13. [tests] Use PeerManagerImpl directly in tests 687067ef41
  14. [tests] Move test-only functions out of net_processing.h 1363e906ad
  15. jnewbery force-pushed on Jan 14, 2021
  16. jnewbery marked this as ready for review on Jan 14, 2021
  17. jnewbery marked this as a draft on Jan 14, 2021
  18. jnewbery commented at 1:05 PM on January 14, 2021: member

    Fixed the typo. I'll fix the linter and fuzzer errors if this PR gets some concept ACKs.

  19. DrahtBot cross-referenced this on Jan 14, 2021 from issue addrman: Make addrman a top-level component by jnewbery
  20. DrahtBot cross-referenced this on Jan 15, 2021 from issue [refactor] Move some net_processing globals into PeerManagerImpl by ajtowns
  21. DrahtBot cross-referenced this on Jan 16, 2021 from issue rpc: getblockfrompeer by Sjors
  22. DrahtBot cross-referenced this on Jan 20, 2021 from issue banman: save the banlist in a JSON format on disk by vasild
  23. DrahtBot cross-referenced this on Jan 28, 2021 from issue tree-wide: De-globalize ChainstateManager by dongcarl
  24. DrahtBot cross-referenced this on Feb 2, 2021 from issue [p2p] Introduce node rebroadcast module by amitiuttarwar
  25. DrahtBot cross-referenced this on Feb 4, 2021 from issue p2p: Add DISABLETX message for negotiating block-relay-only connections by sdaftuar
  26. DrahtBot cross-referenced this on Feb 11, 2021 from issue Split orphan handling from net_processing into txorphanage by ajtowns
  27. DrahtBot cross-referenced this on Feb 12, 2021 from issue Net Processing: Move RelayTransaction() into PeerManager by jnewbery
  28. DrahtBot cross-referenced this on Feb 12, 2021 from issue net/net processing: Move tx inventory into net_processing by jnewbery
  29. DrahtBot cross-referenced this on Feb 14, 2021 from issue Erlay: bandwidth-efficient transaction relay protocol by naumenkogs
  30. jnewbery commented at 12:03 PM on February 15, 2021: member

    Closing this for now. Let's revisit once #21148 is merged.

  31. jnewbery closed this on Feb 15, 2021
  32. bitcoin locked this on Aug 16, 2022
Contributors
jnewberyDrahtBotajtowns
Labels
P2P
Linked (view graph)
#20758 net-processing refactoring -- lose globals, move implementation details from .h to .cpp
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