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

[Upgrade] Alpha TestNet v0.0.10 upgrade #894

Merged
merged 11 commits into from
Nov 15, 2024
Merged

Conversation

okdas
Copy link
Member

@okdas okdas commented Oct 25, 2024

Summary

Prep for v0.0.10 release and upgrade.

Type of change

Select one or more from the following:

Testing

Went through upgrade testing locally.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@okdas okdas added the code health Cleans up some code label Oct 25, 2024
@okdas okdas added this to the Shannon Beta TestNet Launch milestone Oct 25, 2024
@okdas okdas self-assigned this Oct 25, 2024
@okdas
Copy link
Member Author

okdas commented Oct 28, 2024

Merged in 78a7445, seems like there are no changes that would affect the release so gtg.

@okdas
Copy link
Member Author

okdas commented Oct 31, 2024

Merged in ff76430 and began testing the upgrade locally.

The upgrade failed initially. Here's a gist with a panic: https://gist.github.com/okdas/535bc3c0282483fef27454c70d889aa6

I believe the reason is we've changed the data type of the protobuf field here. Meaning the new version is unable to map the existing uint64 in kvstore into a Coin that it now expects.

After I manually created a new prooftypes.Params the upgrade went through.

Screenshot 2024-10-30 at 6 26 29 PM

We probably need to treat protobuf files as they used are production - meaning no type/field changes, any change that needs to be done should go through adding a new field, ensuring backwards-compatibility.

Copy link

gitguardian bot commented Nov 6, 2024

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@Olshansk
Copy link
Member

Olshansk commented Nov 6, 2024

We probably need to treat protobuf files as they used are production - meaning no type/field changes, any change that needs to be done should go through adding a new field, ensuring backwards-compatibility.

Noted. We'll start doing this in Beta. We'll have one last cleanup opportunity prior to mainnet.

cc @red-0ne

@okdas
Copy link
Member Author

okdas commented Nov 13, 2024

I'm going to create a release using f0d0e7a as this is the commit that has been tested using this branch.

Looking at new changes in main branch, there are more state changes required in the upgrade. As discussed in the meeting yesterday, we will add them in another upgrade shortly after.

Let's practice upgrades!

@okdas okdas marked this pull request as ready for review November 13, 2024 23:54
@okdas okdas requested a review from Olshansk November 13, 2024 23:55
@okdas
Copy link
Member Author

okdas commented Nov 13, 2024

The release has been created: https://github.com/pokt-network/poktroll/releases/tag/v0.0.10

I did not use main branch specifically as there are more commits now that would require a bit more testing. We'll issue the next upgrade with the new changes. Probably next week.

@Olshansk let's get this in main to preserve the upgrade code! :)

app/upgrades/types.go Outdated Show resolved Hide resolved
app/upgrades/types.go Outdated Show resolved Hide resolved
app/upgrades/types.go Outdated Show resolved Hide resolved
"plan": {
"name": "v0.0.10",
"height": "20",
"info": "NOT NEEDED FOR LOCAL TESTING"
Copy link
Member

Choose a reason for hiding this comment

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

#PUC why we have this file then

app/upgrades/v0.0.10.go Outdated Show resolved Hide resolved
app/upgrades/v0.0.10.go Outdated Show resolved Hide resolved
app/upgrades/v0.0.10.go Show resolved Hide resolved
app/upgrades/v0.0.10.go Outdated Show resolved Hide resolved
app/upgrades/v0.0.10.go Outdated Show resolved Hide resolved
app/upgrades/v0.0.10.go Outdated Show resolved Hide resolved
@okdas
Copy link
Member Author

okdas commented Nov 14, 2024

Thanks for the review @Olshansk.

At this point, with many changes here, I want to delete the v0.0.10 version we currently have a release for, remove the merges from the main branch (so the code only contains the commits we wrote an upgrade for), and test the upgrade again.

Will re-create release and apply an upgrade on the network after that.

@okdas okdas force-pushed the dk-alpha-testnet-upgrade-0.0.10 branch from 05aeeb2 to 10d45b7 Compare November 15, 2024 00:10
@okdas
Copy link
Member Author

okdas commented Nov 15, 2024

I've verified all new params and grants locally - we're good to upgrade!
I'll create a new release and issue an upgrade on alpha network now.

@okdas okdas added the push-image CI related - pushes images to ghcr.io label Nov 15, 2024
Copy link

The image is going to be pushed after the next commit.

You can use make trigger_ci to push an empty commit.

If you also want to run E2E tests, please add devnet-test-e2e label.

@okdas okdas requested a review from Olshansk November 15, 2024 18:26
@okdas
Copy link
Member Author

okdas commented Nov 15, 2024

@Olshansk the upgrade has been applied on Alpha TestNet. We don't really have to merge this, but I still would to have an example for future upgrades.

app/upgrades/types.go Outdated Show resolved Hide resolved
// Should we use the same address/wallet for DAO or find a way to detect the network the upgrade is being applied to,
// to pick different addresses depending on the name of the network? (e.g chain-id)

const (
Copy link
Member

Choose a reason for hiding this comment

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

Ty for the comments

Co-authored-by: Daniel Olshansky <[email protected]>
@okdas okdas merged commit e0aa1ff into main Nov 15, 2024
9 checks passed
bryanchriswhite added a commit that referenced this pull request Nov 18, 2024
* todo_beta/upnext:
  [C&P] Populate NumEstimatedComputeUnits and ClaimedUpokt of Claim and Proof events (#927)
  [Tokenomics] feat: add `dao_reward_address` param to tokenomics module (#922)
  chore: review feedback improvements
  chore: review feedback improvements
  [Service] Return the default RelayMinerDifficuly for services that have none. (#926)
  [Upgrade] Alpha TestNet v0.0.10 upgrade (#894)
  Update reviewdog.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Cleans up some code push-image CI related - pushes images to ghcr.io
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants