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

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

   • #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. 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
8. 
   ajtowns commented at 6:48 am on January 14, 2021: member

   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.
9. [net processing] Add peerman_impl.h 69a7251d05
10. [tests] Use PeerManagerImpl directly in tests 687067ef41
11. [tests] Move test-only functions out of net_processing.h 1363e906ad
12. jnewbery force-pushed on Jan 14, 2021
13. jnewbery marked this as ready for review on Jan 14, 2021
14. jnewbery marked this as a draft on Jan 14, 2021
15. 
    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.
16. 
    jnewbery commented at 12:03 pm on February 15, 2021: member

    Closing this for now. Let’s revisit once #21148 is merged.
17. jnewbery closed this on Feb 15, 2021

    ---

18. DrahtBot locked this on Aug 16, 2022

Contributors
jnewbery DrahtBot ajtowns

Labels
P2P