Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Finish removing support for legacy network protocol #8296

Merged
1 commit merged into from
Mar 11, 2021

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Mar 9, 2021

Close #5670

The so-called legacy network protocol was already half-non-functioning. We broke its features over time, such as performing block requests and sending transactions. The reason why it wasn't completely removed earlier is because older versions still open this substream for handshaking purposes, even though they don't make use of it once it's open.

This PR finishes removing it entirely.
The changes were very straight forward. It is purely code removal.

It seems that Rust does not always properly warn about unused code, as it didn't warn me about the upgrade::legacy module being unused even though it was. There might be more old code that can be removed, but I don't know where.

@tomaka tomaka added A0-please_review Pull request needs code review. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Mar 9, 2021
@tomaka tomaka requested a review from romanb March 9, 2021 09:14
Copy link
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

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

👍

@tomaka
Copy link
Contributor Author

tomaka commented Mar 9, 2021

Deploying the PR has brought up a new kind of disconnect reason:

image

(the red and violet peaks on the right aren't present on the left side)

However this is very minor and I suggest to not worry about these old nodes.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 11, 2021

Nothing unusual on the burnin node.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 11, 2021

Needs a second approval for merging.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 11, 2021

bot merge force

@ghost
Copy link

ghost commented Mar 11, 2021

Trying merge.

@ghost
Copy link

ghost commented Mar 11, 2021

Merge failed: "At least 2 approving reviews are required by reviewers with write access."

@tomaka
Copy link
Contributor Author

tomaka commented Mar 11, 2021

bot merge

@ghost
Copy link

ghost commented Mar 11, 2021

Checks failed; merge aborted.

@tomaka
Copy link
Contributor Author

tomaka commented Mar 11, 2021

bot merge force

@ghost
Copy link

ghost commented Mar 11, 2021

Trying merge.

@ghost ghost merged commit 0a61b0a into paritytech:master Mar 11, 2021
@tomaka tomaka deleted the rm-legacy branch March 11, 2021 13:57
jordy25519 pushed a commit to cennznet/substrate that referenced this pull request Sep 17, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove legacy single substream tracking issue
3 participants