This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Remove backwards-compatibility networking hack#8068
Merged
3 commits merged intoparitytech:masterfrom Feb 8, 2021
Merged
Conversation
tomaka
added
A0-please_review
This was referenced Feb 8, 2021
romanb
approved these changes
Feb 8, 2021
Contributor
romanb
left a comment
romanb
left a comment
There was a problem hiding this comment.
Does the test_sync integration test rely on this hack being present or do you think the timeouts are unrelated?
Contributor
Author
|
It definitely shouldn't rely on the hack. |
Contributor
Author
|
On my machine, the nodes of the sync test can't even establish TCP connections between each other, which is not something that this PR could break. I'm investigating. |
Contributor
Author
|
We were missing the same fix as #7985 but for transactions. |
mxinden
reviewed
Feb 8, 2021
Contributor
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
For what my review is worth, the changes look good to me.
mxinden
approved these changes
Feb 8, 2021
Contributor
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
For what my review is worth, the changes look good to me.
Contributor
Author
|
bot merge |
|
Trying merge. |
ghost
merged commit
s3krit
added a commit
to paritytech/polkadot
that referenced
this pull request
Feb 8, 2021
bump substrate to include fix for paritytech/substrate#8068 (comment)
s3krit
added a commit
to paritytech/polkadot
that referenced
this pull request
Feb 9, 2021
* Bump substrate bump substrate to include fix for paritytech/substrate#8068 (comment) * bump version * bump impl_version for polkadot, kusama, westend * Blank commit to kick github
This pull request was closed.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fix #7852
After this PR, Polkadot 0.8.26 and below will be unable to open the transactions and GrandPa substreams with clients that have this PR in.
It is a noticable breakage, because so far even old versions of Polkadot (even maybe 0.8.0) were capable of connecting to recent versions. Only the other way around (recent connecting to older) was known to be broken.
Detailed explanations
In Polkadot 0.8.26 and below, we consider that either all substreams (block announces, transactions, and GrandPa) are open with a given peer, or none. As long as the block announces substream (i.e. the "main" one) is open, we silently ignore situations where the transactions and/or GrandPa has failed to open. If the rest of the client tries to send a transaction or a GrandPa message with a peer where it failed to open, the message is silently discarded.
In Polkadot 0.8.27 and later (since #7700), however, we properly separate substreams individually from each other. As part of #7700, nodes now open the block announces substream first, then, only if it is open, opens the transactions substreams and GrandPa.
Similarly, if a 0.8.27 node receives an open request for transactions without the block announces being already open, it will be refused. Later, this receiving node will then send back an open request on their own for a transactions substream.
Unfortunately, this causes some compatibility issue between 0.8.26- and 0.8.27+. Nodes on 0.8.26- try to open everything at once, and only the block announces will successfully open. Nodes on 0.8.26- then silently ignore the situation. Even though nodes on 0.8.27+, after refusing a transactions/grandpa substream, will later try to open an outgoing transactions/grandpa substream, this re-opening will be ignored by nodes on 0.8.26-.
In order to solve this problem, a backwards-compatibility hack was added by allowing a certain number of nodes to open transactions and GrandPa substreams despite having no block announces substream open.
This PR removes this hack.