p2p: implement new Transport interface#5791
Merged
mergify[bot] merged 60 commits intomasterfrom Dec 15, 2020
Merged
Conversation
Contributor
Author
|
Looks like a bunch of non-P2P tests rely on P2P testing infrastructure and internals, I'll do a pass over these on Monday. |
57 tasks
melekes
reviewed
Dec 14, 2020
Contributor
melekes
left a comment
There was a problem hiding this comment.
amazing work 🔥 looking forward to p2p 2.0
melekes
approved these changes
Dec 14, 2020
alexanderbez
approved these changes
Dec 14, 2020
Contributor
alexanderbez
left a comment
There was a problem hiding this comment.
Looks great! I'm still reviewing the MConnTransport implementation, but I'm giving a preliminary ACK.
Codecov Report
@@ Coverage Diff @@
## master #5791 +/- ##
==========================================
- Coverage 60.46% 59.96% -0.50%
==========================================
Files 261 262 +1
Lines 23627 23712 +85
==========================================
- Hits 14285 14218 -67
- Misses 7853 7981 +128
- Partials 1489 1513 +24
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This implements a new
Transportinterface and related types for the P2P refactor in #5670. Previously,conn.MConnectionwas very tightly coupled to thePeerimplementation -- in order to allow alternative non-multiplexed transports (e.g. QUIC), MConnection has now been moved below theTransportinterface, asMConnTransport, and decoupled from the peer. Since thep2ppackage is not covered by our Go API stability, this is not considered a breaking change, and not listed in the changelog.The initial approach was to implement the new interface in its final form (which also involved possible protocol changes, see tendermint/spec#227). However, it turned out that this would require a large amount of changes to existing P2P code because of the previous tight coupling between
PeerandMConnectionand the reliance on subtleties in the MConnection behavior. Instead, I have broadened theTransportinterface to expose much of the existing MConnection interface, preserved much of the existing MConnection logic and behavior in the transport implementation, and tried to make as few changes to the rest of the P2P stack as possible. We will instead reduce this interface gradually as we refactor other parts of the P2P stack.The low-level transport code and protocol (e.g. MConnection, SecretConnection and so on) has not been significantly changed, and refactoring this is not a priority until we come up with a plan for QUIC adoption, as we may end up discarding the MConnection code entirely.
There are no tests of the new
MConnTransport, as this code is likely to evolve as we proceed with the P2P refactor, but tests should be added before a final release. The E2E tests are sufficient for basic validation in the meanwhile.