[Upgrades] v0.0.12 upgrade #1043

okdas · 2025-01-24T21:49:25Z

Summary

Adding an upgrade handler for v0.0.12

Type of change

Select one or more from the following:

New feature, functionality or library
Consensus breaking; add the consensus-breaking label if so. See [Infra] Automatically add the consensus-breaking label #791 for details
Bug fix
Code health or cleanup
Documentation
Other (specify)

Sanity Checklist

I have updated the GitHub Issue assignees, reviewers, labels, project, iteration and milestone
For docs, I have run make docusaurus_start
For code, I have run make go_develop_and_test and make test_e2e
For code, I have added the devnet-test-e2e label to run E2E tests in CI
For configurations, I have update the documentation
I added TODOs where applicable

app/upgrades/v0.0.12.go

red-0ne

LGTM! I feel we are starting to improve our migration plans 👏

app/upgrades/v0.0.12.go

red-0ne · 2025-01-31T02:24:49Z

app/upgrades/v0.0.12.go

+					// Force all services to have a 100% revshare to the supplier.
+					// Not something we would do on a real mainnet, but it's a quick way to resolve the issue.
+					// Currently, we don't break any existing suppliers (as all of them have a 100% revshare to the supplier).
+					service.RevShare = []*sharedtypes.ServiceRevenueShare{


Ideally we would be able to loop over the old revshare map by unmashalling to the old proto type then create the new Supplier proto with the new values.

But since we don't have proto versioning and we marked the old (float32) value as reserved, this is the next best option offered to us.

@red-0ne Really appreciate this feedback & input.

@okdas Can you #PUC what the "best" approach would be an "why" - essentially paragraphsing @red-0ne's comments.

In particular, explaining why we're not doing it would help. I'm thinking:

Requires multiple copies of the protobuf

Requiers multiple steps in the migration (overkill given that we're not in prod)

Etc...

Rephrased the whole block.

Olshansk

@okdas Looks good!

Overall, I do want us to take this as an opportunity to create a good reference + example.

keep it clean
do things right
iterate on process / template

Things aren't "on fire" and it's a "simple migration", so now is the time!

app/upgrades.go

Olshansk · 2025-01-31T15:19:29Z

app/upgrades.go

@@ -11,9 +11,7 @@ import (
 // The chain upgrade can be scheduled AFTER the new version (with upgrade strategy implemented) is released,


Isn't there a certain .md you're supposed to update (for documentation purposes) when we do an upgrade?

https://dev.poktroll.com/protocol/upgrades/upgrade_list

At the moment of release, we don't know when the upgrade will happen. My thinking, this should be a separate PR. But! We can prepare some changes in advance - that's a good idea.

But! We can prepare some changes in advance - that's a good idea.

Let's do it!

Wdyt of creating another PR (updating the docs that's a draft), and leave a TODO here.

var allUpgrades = []upgrades.Upgrade{ // v0.0.4 was the first upgrade we implemented and tested on network that is no longer used. // upgrades.Upgrade_0_0_4, // v0.0.10 was the first upgrade we implemented on Alpha TestNet. // upgrades.Upgrade_0_0_10, // v0.0.11 was the Alpha TestNet exclusive upgrade to bring it on par with Beta TestNet. // upgrades.Upgrade_0_0_11, // TODO(@okdas, #XX): Update the documentation once a block height has been selected. // Ref: https://dev.poktroll.com/protocol/upgrades/upgrade_list // v0.0.12 - the first upgrade going live on both Alpha and Beta TestNets. upgrades.Upgrade_0_0_12,

Nvm: I saw your changes in .md AFTER writing the message above. Leaving it as a reference.

Wdyt of adding the output of the CLI transaction to say "Please update docusaurus/docs/protocol/upgrades/upgrade_list.md` now that you've submitted the plan?

You know how they say "Go to where your users are", I'm trying to "Go where the dev's eyes are".

They're looking at the CLI when something says !!!!!!!!! :)

app/upgrades/types.go

app/upgrades/v0.0.12.go

Olshansk · 2025-01-31T16:25:13Z

app/upgrades/v0.0.12.go

+
+			err := applyNewParameters(ctx)
+			if err != nil {
+				logger.Error("Failed to apply new parameters", "error", err)


Thoughts on (here and elsewhere) adding the version upgrade a a (key,val) as well?

You can either have a local v0.0.12 const, or update the logger context, or both!

Olshansk · 2025-01-31T16:26:09Z

app/upgrades/v0.0.12.go

+				return vm, err
+			}
+
+			// Since we changed the type of RevSharePercent from float32 to uint64, we need to update all on-chain data.


Can we moved this into a local helper to scope / separate the changes from the "boilerplate"?

Olshansk · 2025-01-31T16:28:19Z

app/upgrades/v0.0.12.go

+				return vm, err
+			}
+
+			logger.Info("Successfully completed v0.0.12 upgrade handler")


Let's make v0.0.12 a local constant and do some string formatting here.

It'll avoid copy-pasta errors in the future.

Olshansk · 2025-01-31T16:29:11Z

app/upgrades/v0.0.12.go

+			logger.Info("Updating all suppliers to have a 100% revshare to the supplier", "num_suppliers", len(suppliers))
+			for _, supplier := range suppliers {
+				for _, service := range supplier.Services {
+					// Force all services to have a 100% revshare to the supplier.


Can you change this to read the prior state and use it for the new state/

I realize it's not a big deal here, but I want us to take this as an opportunity to do it right, and set a right example/reference in the future.

Can you please elaborate? GetAllSuppliers gives us the current state from what I understand.

I do want to add a warning if there's a supplier with more than 1 revshare, which would signal us if we broke somebody's setup.

Co-authored-by: Daniel Olshansky <[email protected]>

app/upgrades/v0.0.12.go

Olshansk

Looking great @okdas!

Just a handful of more comments & NITs but we should be g2g after the next set of changes.

Olshansk · 2025-02-03T20:13:43Z

app/upgrades.go

@@ -11,9 +11,7 @@ import (
 // The chain upgrade can be scheduled AFTER the new version (with upgrade strategy implemented) is released,


But! We can prepare some changes in advance - that's a good idea.

Let's do it!

Wdyt of creating another PR (updating the docs that's a draft), and leave a TODO here.

var allUpgrades = []upgrades.Upgrade{ // v0.0.4 was the first upgrade we implemented and tested on network that is no longer used. // upgrades.Upgrade_0_0_4, // v0.0.10 was the first upgrade we implemented on Alpha TestNet. // upgrades.Upgrade_0_0_10, // v0.0.11 was the Alpha TestNet exclusive upgrade to bring it on par with Beta TestNet. // upgrades.Upgrade_0_0_11, // TODO(@okdas, #XX): Update the documentation once a block height has been selected. // Ref: https://dev.poktroll.com/protocol/upgrades/upgrade_list // v0.0.12 - the first upgrade going live on both Alpha and Beta TestNets. upgrades.Upgrade_0_0_12,

app/upgrades/historical.go

Olshansk · 2025-02-03T20:35:11Z

localnet/poktrolld/config/relayminer_config_localnet_vscode.yaml

@@ -23,7 +23,7 @@ metrics:
  addr: :9070
 pocket_node:
  query_node_rpc_url: tcp://localhost:26657
-  query_node_grpc_url: tcp://localhost:36658


So confirming this should be left as is or no?

It's just a little confusing (without comments) why some ports are 26657 but the other is 9090

Olshansk · 2025-02-03T20:37:38Z

app/upgrades.go

@@ -11,9 +11,7 @@ import (
 // The chain upgrade can be scheduled AFTER the new version (with upgrade strategy implemented) is released,


Nvm: I saw your changes in .md AFTER writing the message above. Leaving it as a reference.

Wdyt of adding the output of the CLI transaction to say "Please update docusaurus/docs/protocol/upgrades/upgrade_list.md` now that you've submitted the plan?

You know how they say "Go to where your users are", I'm trying to "Go where the dev's eyes are".

They're looking at the CLI when something says !!!!!!!!! :)

app/upgrades/types.go

app/upgrades/v0.0.12.go

Olshansk · 2025-02-03T21:38:36Z

app/upgrades/v0.0.12.go

+				}
+				keepers.SupplierKeeper.SetSupplier(ctx, supplier)
+				logger.Info("Updated supplier",
+					"supplier", supplier.OperatorAddress)


Owner & operator please!

app/upgrades/v0.0.12.go

Olshansk · 2025-02-03T21:41:21Z

app/upgrades/v0.0.12.go

+				return vm, err
+			}
+
+			logger.Info("Running module migrations", "upgrade_plan_name", Upgrade_0_0_12_PlanName)


Can we add a log like this before every section / update?

Co-authored-by: Daniel Olshansky <[email protected]>

Olshansk

A few NITs.

Preliminary approval so we can start working on the upgrade while tending to the discussion / comments in parallel.

Feel free to merge at your pace.

app/upgrades/historical.go

Olshansk · 2025-02-04T20:33:37Z

app/upgrades/v0.0.12.go

+		// Parameter configurations aligned with repository config.yml specifications
+		// These values reflect the delta between v0.0.11 and current main branch
+		// Reference:
+		// - Comparison: https://github.com/pokt-network/poktroll/compare/v0.0.11..main


Moving conversation to discord to get this over the finish line.

https://discord.com/channels/824324475256438814/1332065342746525778/1336434350660784239

app/upgrades/v0.0.12.go

Co-authored-by: Daniel Olshansky <[email protected]>

okdas · 2025-02-04T23:48:28Z

Had a successful upgrade locally (using this guide).
Will create a release and plan an upgrade on Alpha.

okdas added the protocol General core protocol related changes label Jan 24, 2025

okdas added this to the Beta TestNet Iteration milestone Jan 24, 2025

okdas self-assigned this Jan 24, 2025

github-actions bot reviewed Jan 24, 2025

View reviewed changes

github-actions bot reviewed Jan 30, 2025

View reviewed changes

app/upgrades/v0.0.12.go Outdated Show resolved Hide resolved

v0.0.12

f187c35

okdas force-pushed the upgrade/prep-v0.0.12 branch from 5a3decc to f187c35 Compare January 30, 2025 23:49

set RevSharePercentage for all

adc3c4d

okdas changed the title ~~[Upgrades][WIP] prepare for v0.0.12~~ [Upgrades] v0.0.12 migration Jan 31, 2025

okdas changed the title ~~[Upgrades] v0.0.12 migration~~ [Upgrades] v0.0.12 upgrade Jan 31, 2025

okdas requested a review from Olshansk January 31, 2025 01:54

okdas marked this pull request as ready for review January 31, 2025 01:55

red-0ne approved these changes Jan 31, 2025

View reviewed changes

Olshansk requested changes Jan 31, 2025

View reviewed changes

Apply suggestions from code review

fb373c2

Co-authored-by: Daniel Olshansky <[email protected]>

github-actions bot reviewed Jan 31, 2025

View reviewed changes

app/upgrades/v0.0.12.go Outdated Show resolved Hide resolved

okdas added 2 commits January 31, 2025 17:08

requested changes

147949f

add new version to the table

dd6d1df

okdas requested a review from Olshansk February 1, 2025 01:27

Merge branch 'main' into upgrade/prep-v0.0.12

220eff6

Olshansk requested changes Feb 3, 2025

View reviewed changes

Olshansk requested a review from red-0ne February 3, 2025 21:42

okdas and others added 2 commits February 3, 2025 17:28

Apply suggestions from code review

f7a8636

Co-authored-by: Daniel Olshansky <[email protected]>

req changes

b8965af

okdas requested a review from Olshansk February 4, 2025 18:40

Olshansk approved these changes Feb 4, 2025

View reviewed changes

okdas and others added 2 commits February 4, 2025 14:48

Apply suggestions from code review

9c2ed24

Co-authored-by: Daniel Olshansky <[email protected]>

req changes

32163df

okdas merged commit 1617784 into main Feb 4, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Upgrades] v0.0.12 upgrade #1043

[Upgrades] v0.0.12 upgrade #1043

okdas commented Jan 24, 2025 •

edited

Loading

red-0ne left a comment

red-0ne Jan 31, 2025

Olshansk Feb 3, 2025

okdas Feb 4, 2025

Olshansk left a comment

Olshansk Jan 31, 2025

okdas Jan 31, 2025

Olshansk Feb 3, 2025

Olshansk Feb 3, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

Olshansk Jan 31, 2025

okdas Feb 1, 2025

Olshansk left a comment

Olshansk Feb 3, 2025

Olshansk Feb 3, 2025

Olshansk Feb 3, 2025

Olshansk Feb 3, 2025

Olshansk Feb 3, 2025

Olshansk left a comment

Olshansk Feb 4, 2025

okdas commented Feb 4, 2025

		@@ -11,9 +11,7 @@ import (
		// The chain upgrade can be scheduled AFTER the new version (with upgrade strategy implemented) is released,

[Upgrades] v0.0.12 upgrade #1043

[Upgrades] v0.0.12 upgrade #1043

Conversation

okdas commented Jan 24, 2025 • edited Loading

Summary

Type of change

Sanity Checklist

red-0ne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okdas commented Feb 4, 2025

okdas commented Jan 24, 2025 •

edited

Loading