Skip to content

patch capabilities for old clients #14105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

patch capabilities for old clients #14105

wants to merge 16 commits into from

Conversation

almogdepaz
Copy link
Contributor

@almogdepaz almogdepaz commented Dec 11, 2022 •
edited
Loading

make sure we dont send new capabilities for clients under 1.6.2
no breaking changes, fixes the issue where old clients would disconnect new one but only for the older clients outbound connections

example:
a 1.5 peer initiates a connection to a 1.6.2 peer, the 1.6.2 knows what 1.5 supports and only sends those to avoid the disconnect

this cant be done the other way because of who sends the first handshake message with the version and supported capabilties

@almogdepaz almogdepaz requested a review from altendky December 11, 2022 13:53
@almogdepaz almogdepaz added bug Something isn't working Known Issue Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog labels Dec 11, 2022
altendky
altendky requested changes Dec 11, 2022
View reviewed changes
Copy link
Contributor

@altendky altendky left a comment

Choose a reason for hiding this comment

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

Maybe parametrize the tests both against the edges right below and above the boundary as well as further away including even future versions.

@almogdepaz
Copy link
Contributor Author

Maybe parametrize the tests both against the edges right below and above the boundary as well as further away including even future versions.

i changed the tests to be more close to the check case, i dont think there's use in adding more cases

@almogdepaz almogdepaz marked this pull request as ready for review December 12, 2022 09:59
@almogdepaz almogdepaz requested a review from a team as a code owner December 12, 2022 09:59
@emlowe
Copy link
Contributor

emlowe commented Dec 12, 2022

Almog, clarification.
This works if the old peer initiates the connection, in which case this code makes sure to not send a new capability

This does not work if the new peer initiates the connection, as it will send the new capability that will be rejected by the old node.

Is my understanding correct here?

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Dec 14, 2022
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Dec 15, 2022
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@almogdepaz almogdepaz force-pushed the patch_capabilties_for_old_clients branch from d45216c to 7e5d629 Compare December 15, 2022 10:35
arvidn
arvidn reviewed Dec 15, 2022
View reviewed changes
@almogdepaz almogdepaz requested a review from arvidn December 15, 2022 12:37
@almogdepaz
Copy link
Contributor Author

almogdepaz commented Dec 15, 2022 •
edited
Loading

@altendky suggested using https://packaging.pypa.io/en/stable/version.html for parsing the version, im up for that if we are ok with moving towards using that everywhere for versions, otherwise i feel this will only cause more confusion and prefer to leave as is.
@emlowe any thoughts on this?

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

@github-actions github-actions bot added the stale-pr Flagged as stale and in need of manual review label Feb 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Feb 4, 2023
@github-actions github-actions bot removed the stale-pr Flagged as stale and in need of manual review label Oct 13, 2023
@github-actions GitHub Actions
Copy link
Contributor

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

@github-actions github-actions bot added the stale-pr Flagged as stale and in need of manual review label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aqk aqk Awaiting requested review from aqk

@altendky altendky Awaiting requested review from altendky

@emlowe emlowe Awaiting requested review from emlowe

@arvidn arvidn Awaiting requested review from arvidn

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
bug Something isn't working Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog Known Issue merge_conflict Branch has conflicts that prevent merge to main stale-pr Flagged as stale and in need of manual review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@almogdepaz @emlowe @altendky @arvidn