Skip to content
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.

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

Separate protocol versions #17791

Merged
merged 3 commits into from
Mar 29, 2024
Merged

Separate protocol versions #17791

merged 3 commits into from
Mar 29, 2024

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Mar 27, 2024

Purpose:

As we introduced CHIP-22, supporting 3rd party harvesters, the Harvester protocol version should only be bumped if there are changes to it. Right now, all our protocol version numbers always increase in lock-step. Disconnecting them allows for more precise warnings in case the two ends of a connection uses different versions.

Current Behavior:

The protocol version for FullNode, Wallet, Farmer and Harvester are all defined by a single constant, and always update in lock-step.

New Behavior:

The versions of the different protocols can be bumped independently, depending on which protocol is updated.

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Mar 27, 2024
@arvidn arvidn requested a review from Rigidity March 27, 2024 15:00
@arvidn arvidn marked this pull request as ready for review March 27, 2024 15:26
@arvidn arvidn requested a review from a team as a code owner March 27, 2024 15:26
chia/server/ws_connection.py Outdated Show resolved Hide resolved

This comment was marked as outdated.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Mar 27, 2024
chia/server/ws_connection.py Outdated Show resolved Hide resolved
chia/server/ws_connection.py Outdated Show resolved Hide resolved
@arvidn arvidn force-pushed the separate-protocol-versions branch from 69a6126 to 70866e6 Compare March 27, 2024 23:42

This comment was marked as outdated.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Mar 27, 2024

This comment was marked as outdated.

Rigidity
Rigidity previously approved these changes Mar 28, 2024
Copy link
Contributor

@Rigidity Rigidity left a comment

Choose a reason for hiding this comment

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

I think this is a very nice improvement

@arvidn arvidn force-pushed the separate-protocol-versions branch from 967b362 to e373852 Compare March 28, 2024 16:37
@arvidn arvidn requested a review from Rigidity March 28, 2024 16:59
@arvidn arvidn marked this pull request as ready for review March 28, 2024 16:59
@arvidn
Copy link
Contributor Author

arvidn commented Mar 28, 2024

I think it's fine to not have coverage for the warning

Rigidity
Rigidity previously approved these changes Mar 28, 2024
@arvidn arvidn requested a review from wjblanke March 28, 2024 18:28
Copy link
Contributor

File Coverage Missing Lines
chia/server/ws_connection.py 77.8% lines 225, 262
Total Missing Coverage
19 lines 2 lines 89%

@arvidn arvidn added ready_to_merge Submitter and reviewers think this is ready and removed coverage-diff labels Mar 28, 2024
@pmaslana pmaslana merged commit f200fcf into main Mar 29, 2024
309 of 311 checks passed
@pmaslana pmaslana deleted the separate-protocol-versions branch March 29, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants