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

chore: bump app version 9 -> 10 #5073

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Conversation

conorsch
Copy link
Contributor

@conorsch conorsch commented Feb 7, 2025

Describe your changes

Bumps the protocol version to denote the addition of LQT functionality. Also bumps the crate versions from 1.0.x -> 2.0.x, as an alpha. No tag has been created, to avoid triggering the formal release workflows, since we're still testing.

Issue ticket number and link

#5010.

Testing and review

I'm submitting this PR ahead of a planned testnet upgrade, so we'll shortly learn how effective the overall upgrade process is. However, special attention should be given to the proposed versions strings. In particular, the transition from 0.81.x to 1.0.x maintained compatibility with APP_VERSION 9, and that may present difficulties during the upgrade for nodes on one or the other version.

Checklist before requesting a review

  • I have added guiding text to explain how a reviewer should test these changes.

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    explicitly denotes consensus-breaking changes, and therefore targets a protocol feature branch, rather than main

@conorsch conorsch added the consensus-breaking breaking change to execution of on-chain data label Feb 7, 2025
@cronokirby cronokirby self-requested a review February 7, 2025 16:31
COMPATIBILITY.md Outdated
@@ -10,3 +10,5 @@
| 7 (Mainnet) | v0.79.x | v0.37.x | v1 |
| 8 (Mainnet) | v0.80.x | v0.37.x | v1 |
| 9 (Mainnet) | v0.81.x | v0.37.x | v1 |
| 9 (Mainnet) | v1.0.x | v0.37.x | v1 |
| 10 (Mainnet) | v1.1.x | v0.37.x | v1 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, if 10 is breaking compared to 9, shouldn't it actually be version 2.0.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should be referring to the LQT changes at v2. I've updated the PR accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To recap prior discussion, however, we're bumping the crate versions because of breaking changes in the Rust APIs, not because of the protocol version change, which is tracked separately. The compatibility doc was created to disambiguate here, but we should still write down a full explanation of versioning scheme and stick those in the dev docs.

Bumps the protocol version to denote the addition of LQT functionality.
Also bumps the crate versions from 1.0.x -> 2.0.x, as an alpha.
No tag has been created, to avoid triggering the formal release
workflows, since we're still testing.
@@ -16,7 +16,8 @@ fn version_to_software_version(version: u64) -> &'static str {
6 => "v0.77.x",
7 => "v0.79.x",
8 => "v0.80.x",
9 => "v0.81.x",
9 => "v1.0.x",
10 => "v2.0.x",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the transition from 0.81.x to 1.0.x maintained compatibility with APP_VERSION 9, and that may present difficulties during the upgrade for nodes on one or the other version.

The clobbering of the 9 match is what I expect will introduce problems if used as-is on mainnet, where different versions are surely running. Must we extend the matching logic to accept multiple-choice? I don't see a way around that, unless we mandate upgrading 0.81.x -> 1.0.x -> 2.0.x.

Copy link
Member

Choose a reason for hiding this comment

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

I thought UIP-6 only checked the APP_VERSION, can take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On closer inspection, I agree: the crate version string is only ever used in user-facing messaging, so we should be good to go here.

@conorsch conorsch merged commit f60a07f into protocol/lqt_branch Feb 7, 2025
10 checks passed
@conorsch conorsch deleted the lqt-app-version branch February 7, 2025 17:02
conorsch added a commit that referenced this pull request Feb 7, 2025
Follow-up to #5073, which should have included a no-op migration.
conorsch added a commit that referenced this pull request Feb 7, 2025
Follow-up to #5073, which should have included a no-op migration.
conorsch added a commit that referenced this pull request Feb 7, 2025
## Describe your changes

Follow-up to #5073, which should have included a no-op migration.

## Issue ticket number and link

Towards #5010. 

## Testing and review

This work will be used to perform a chain upgrade on the PL testnet.
Once that's done, we should expect CI to pass fully on this PR, in
particular the "testnet-integration" suite.

## Checklist before requesting a review

- [x] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > provides migration logic for proposed protocol-breaking changes
conorsch added a commit that referenced this pull request Feb 14, 2025
Bumps the protocol version to denote the addition of LQT functionality.
Also bumps the crate versions from 1.0.x -> 2.0.x, as an alpha. No tag
has been created, to avoid triggering the formal release workflows,
since we're still testing.

I'm submitting this PR ahead of a planned testnet upgrade, so we'll
shortly learn how effective the overall upgrade process is. However,
special attention should be given to the proposed versions strings. In
particular, the transition from `0.81.x` to `1.0.x` maintained
compatibility with APP_VERSION 9, and that may present difficulties
during the upgrade for nodes on one or the other version.

- [x] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

> explicitly denotes consensus-breaking changes, and therefore targets a
protocol feature branch, rather than main
conorsch added a commit that referenced this pull request Feb 14, 2025
## Describe your changes

Follow-up to #5073, which should have included a no-op migration.

## Issue ticket number and link

Towards #5010. 

## Testing and review

This work will be used to perform a chain upgrade on the PL testnet.
Once that's done, we should expect CI to pass fully on this PR, in
particular the "testnet-integration" suite.

## Checklist before requesting a review

- [x] I have added guiding text to explain how a reviewer should test
these changes.

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > provides migration logic for proposed protocol-breaking changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking breaking change to execution of on-chain data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants