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.

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

Pass full version in Handshake #15679

Merged
merged 5 commits into from
Mar 27, 2024
Merged

Pass full version in Handshake #15679

merged 5 commits into from
Mar 27, 2024

Conversation

felixbrucker
Copy link
Contributor

@felixbrucker felixbrucker commented Jul 3, 2023 •
edited
Loading

Purpose:

Currently the Handshake contains a cut down version of the package version (__version__). As it is desired to get the exact version of peers for a future PR related to farming/pooling and there is no downside in having more/exact info on the version of peers this PR changes this to now include the full __version__. The version of a WSChiaConnection is currently only used by the seeder via get_version, which was refactored to return the desired cut down version. Additionally legacy code was removed in favor of using packaging.version.Version, same as we already do in the WSChiaConnection for parsing the Handshake version.

Current Behavior:

A Handshake contains a cut down version, for example: __version__ is 1.8.2rc3.dev116, the handshake contains 1.8.2rc3 instead. Another example: __version__ is 2.0.0b2.dev0+og-1.4.0, the handshake contains 2.0.0b2.

New Behavior:

The Handshake now contains __version__ as is, so from the examples above: 1.8.2rc3.dev116 or 2.0.0b2.dev0+og-1.4.0 respectively.
The get_chia_version_str() method on a WSChiaConnection returns the cut down version, same as before.

Testing Notes:

I added tests for the three "new" functions which are responsible for retrieving the "chia version" (cut down version) of the package or a passed Version or str instance, these basically replace the chia_full_version_str from init_funcs.py.

@felixbrucker felixbrucker requested a review from a team as a code owner July 3, 2023 08:30
@hoffmang9
Copy link
Member

This probably has unintended consequences for the crawler/seeder as it will significantly change all downstream version reporting on the dashboards. This is may drive inconsistency between versions inside the windows installer as the windows installer has to be int.int.int.

@felixbrucker
Copy link
Contributor Author

felixbrucker commented Jul 3, 2023 •
edited
Loading

This probably has unintended consequences for the crawler/seeder as it will significantly change all downstream version reporting on the dashboards.

The version used by the seeder/crawler remains unchanged (cut down version), or are you referring to something else? If you are referring to older seeder/crawler version collection then yes, this is a breaking change, and thus why i'd put this change into 2.0.0, a breaking release 👍

This is may drive inconsistency between versions inside the windows installer as the windows installer has to be int.int.int.

The chia_full_version_str() usages remained unchanged (using the cut down version)

The only place which actually changed is the version in the handshake message

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 10, 2023
@github-actions
Copy link
Contributor

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

@felixbrucker felixbrucker mentioned this pull request Jul 11, 2023
Merged
@felixbrucker felixbrucker force-pushed the pass-full-version-in-handshake branch from 77f0d15 to 3e6e300 Compare July 11, 2023 06:38
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 11, 2023
@github-actions
Copy link
Contributor

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

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 13, 2023
@github-actions
Copy link
Contributor

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

@felixbrucker felixbrucker force-pushed the pass-full-version-in-handshake branch from 3e6e300 to 3a7242a Compare July 13, 2023 20:46
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 13, 2023
@github-actions
Copy link
Contributor

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

@jack60612 jack60612 added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jul 22, 2023
@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 added the merge_conflict Branch has conflicts that prevent merge to main label Aug 29, 2023
@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 Oct 14, 2023
@felixbrucker
Copy link
Contributor Author

Waiting for CNI re versioning schema decisions

@github-actions github-actions bot removed the stale-pr Flagged as stale and in need of manual review label Oct 15, 2023
@felixbrucker felixbrucker force-pushed the pass-full-version-in-handshake branch from 3a7242a to d721ef9 Compare October 27, 2023 18:27
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Oct 27, 2023
@github-actions
Copy link
Contributor

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

@felixbrucker
Copy link
Contributor Author

Since i opened this PR about 3 months ago the version on the connection is now a str again, i have updated the code accordingly.

With this being said i don't actually think we need a versioning discussion for this PR anymore, as the field in the protocol is a string and as indicated by others is already being used with values other than x.y.z. As such the result of get_version on the connection is not guaranteed to yield a x.y.z version string, it can contain any string value instead.

This PR will try to transform the version string from the handshake into the x.y.z format, but fallback to the string value (as was returned before) for its currently only use: the seeder.

I don't think the fact that new clients now send a more detailed/exact version in case they indeed are different from the official releases is a breaking change, instead it is how other custom nodes already behave.

@felixbrucker felixbrucker marked this pull request as draft October 28, 2023 10:59
@felixbrucker felixbrucker marked this pull request as ready for review October 28, 2023 10:59
@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 Dec 12, 2023
@ChiaMineJP ChiaMineJP removed the stale-pr Flagged as stale and in need of manual review label Mar 4, 2024
@github-actions 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 added the merge_conflict Branch has conflicts that prevent merge to main label Mar 14, 2024
@felixbrucker
Merge branch 'upstream-main' into pass-full-version-in-handshake
fdec511 
# Conflicts:
#	chia/_tests/core/server/test_server.py
#	chia/_tests/util/test_chia_version.py
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Mar 15, 2024
@github-actions GitHub Actions
Copy link
Contributor

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

emlowe
emlowe requested changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@emlowe emlowe left a comment

Choose a reason for hiding this comment

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

Suggested a few changes

@emlowe
Copy link
Contributor

emlowe commented Mar 22, 2024

After taking some time to study the code, I think some minor adjustments to the proposed code changes and this should be ok. The Handshake version string isn't really used anywhere other than the crawler

Note, this does require the crawler to need an update such that it does some post-processing on the version in the Handshake into the form it is expecting. But this is a reasonable lift.

An alternative approach would be to change the code such that only the harvester uses a modified version string, which would have modestly less implications, at the cost of some muckity muck.

@emlowe
Copy link
Contributor

emlowe commented Mar 22, 2024

I'm going to go back a bit - at least specifically for 3rd party harvesters

For 3rd Party Harvesters, this isn't needed at all. That harvester can put whatever they want into the Handshake version string. and the farmer can use it from the connection get_version() call

@felixbrucker felixbrucker requested a review from emlowe March 23, 2024 00:36
@felixbrucker
Copy link
Contributor Author

From discord:

While that is true, it's not true for otherwise unchanged chia code, where a cut down version is sent
Sending the full version is the right thing to do
A future pr will send the harvester peer version as a separate header, similarly to your example
It's just that having a potentially incorrect version is not desired

emlowe
emlowe reviewed Mar 26, 2024
View reviewed changes
Copy link
Contributor

@emlowe emlowe left a comment

Choose a reason for hiding this comment

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

Approved

emlowe
emlowe previously approved these changes Mar 26, 2024
View reviewed changes
@felixbrucker
Copy link
Contributor Author

I have made one more commit to ensure we extract major.minor.patch from versions which are invalid pep440 but contain a major.minor.patch version. This is now the same behavior as chia_version_number previously did with manually splitting the string etc.

@felixbrucker felixbrucker mentioned this pull request Mar 27, 2024
Closed
@emlowe
Copy link
Contributor

emlowe commented Mar 27, 2024

coverage exemption for a couple of lines not previously covered

@pmaslana pmaslana merged commit d09b61b into Chia-Network:main Mar 27, 2024
303 of 304 checks passed
@felixbrucker felixbrucker deleted the pass-full-version-in-handshake branch March 27, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emlowe emlowe emlowe approved these changes

Assignees
No one assigned
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@felixbrucker @hoffmang9 @emlowe @jack60612 @pmaslana @ChiaMineJP