Skip to content

Conversation

jnewbery
Copy link
Contributor

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.

@fanquake fanquake added the P2P label Jan 13, 2021
@jnewbery
Copy link
Contributor Author

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

@jnewbery jnewbery changed the title RFC: Move Peer and PeerManagerImpl to separate header RFC: Move Peer and PeerManagerImpl declarations to separate header Jan 13, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 13, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

Copy link
Contributor

@ajtowns ajtowns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jnewbery jnewbery force-pushed the 2021-01-peermanimplh branch from be52d7e to 1363e90 Compare January 14, 2021 11:25
@jnewbery jnewbery marked this pull request as ready for review January 14, 2021 11:25
@jnewbery jnewbery marked this pull request as draft January 14, 2021 13:04
@jnewbery
Copy link
Contributor Author

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

@jnewbery
Copy link
Contributor Author

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

@jnewbery jnewbery closed this Feb 15, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants