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

Migrate genesis cmd #4471

Merged
merged 72 commits into from
Jul 3, 2019
Merged

Migrate genesis cmd #4471

merged 72 commits into from
Jul 3, 2019

Conversation

sabau
Copy link
Contributor

@sabau sabau commented Jun 3, 2019

Closes #4409

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

sabau added 2 commits June 3, 2019 15:20
Signed-off-by: Karoly Albert Szabo <[email protected]>
@codecov
Copy link

codecov bot commented Jun 3, 2019

Codecov Report

Merging #4471 into master will increase coverage by 0.07%.
The diff coverage is 80.76%.

@@            Coverage Diff             @@
##           master    #4471      +/-   ##
==========================================
+ Coverage   54.81%   54.89%   +0.07%     
==========================================
  Files         269      271       +2     
  Lines       16923    16975      +52     
==========================================
+ Hits         9276     9318      +42     
- Misses       6965     6972       +7     
- Partials      682      685       +3

@alexanderbez
Copy link
Contributor

@sabau this shouldn't exist in the SDK :)

@sabau
Copy link
Contributor Author

sabau commented Jun 4, 2019

So should we move also the other script in Gaia?

@sabau
Copy link
Contributor Author

sabau commented Jun 4, 2019

I think those migration helpers belong to here or to the docs.
Let's imagine a dapp developer that uses the SDK upgrade to a new version of the SDK, the old exported genesis won't work until he fixes it, probably the fixes will be a superset of the changes we will provide here, but would be helpful to provide a starting point for him with all the changes we've done.

@alexanderbez
Copy link
Contributor

@sabau I guess if there isn't anything specific pertaining to gaia, then it's fine. But also, the SDK may not contain all upgrades that are possible (e.g. we're skipping v0.35.0). So maybe it should live in Gaia?

Also, @rigelrozanski expressed interest in the new script being written in Go.

i.e. under contrib/export, we have a new directory: v0_34_0_to_v0_36_0. The contains an export script along with the old type definitions and new types definitions and performs the various genesis conversions.

@sabau
Copy link
Contributor Author

sabau commented Jun 4, 2019

But this means translating/moving also the previous script?
contrib/export/v0.33.x-to-v0.34.0.py

I don't have strong preferences between having it in Bash, Go or Python I would just prefer to have all those migrations tools written in the same language.

@fedekunze
Copy link
Collaborator

Also, @rigelrozanski expressed interest in the new script being written in Go.

i.e. under contrib/export, we have a new directory: v0_34_0_to_v0_36_0. The contains an export script along with the old type definitions and new types definitions and performs the various genesis conversions.

@alexanderbez I strongly disagree with this, specially because we'll need to create structs specifically for the changes made (see this comment #4023 (review)).

I actually started the last genesis script in Go (see first commits on #4023) but then we all agreed on using Python because it's untyped and way easier to make the changes required. cc: @alessio

@sabau sabau added the S:blocked Status: Blocked label Jun 4, 2019
Signed-off-by: Karoly Albert Szabo <[email protected]>
@alessio
Copy link
Contributor

alessio commented Jun 4, 2019

Yes, that exactly was the rationale behind the choice of the language

@alexanderbez
Copy link
Contributor

But this means translating/moving also the previous script?
contrib/export/v0.33.x-to-v0.34.0.py

No, we should leave this unchanged.

@alexanderbez I strongly disagree with this, specially because we'll need to create structs specifically for the changes made (see this comment #4023 (review)).

Correct. This is mostly a point of contention with @rigelrozanski.

@sabau
Copy link
Contributor Author

sabau commented Jun 5, 2019

So we keep Python for this release and I've opened an issue to build the proper tool for long term upgrading, sounds good?

#4481

But this means translating/moving also the previous script?
contrib/export/v0.33.x-to-v0.34.0.py

No, we should leave this unchanged.

Wouldn't we want to cover situations where someone updates from 0.33 to 0.36? Or we say that only from a certain point on this will happen (after the proper migration strategy is implemented)

Can we apply a logic similar to typical database migrations? So from now on (next release on) every PR that breaks genesis will need also to add a script that does the conversion

@alexanderbez
Copy link
Contributor

alexanderbez commented Jun 5, 2019

@sabau

Wouldn't we want to cover situations where someone updates from 0.33 to 0.36? Or we say that only from a certain point on this will happen (after the proper migration strategy is implemented)

No, we wouldn't. We don't need to take on that technical burden and overhead. If someone really wants to migrate genesis from cosmoshub-1 to cosmoshub-3, they can first migrate to cosmoshub-2.

Can we apply a logic similar to typical database migrations? So from now on (next release on) every PR that breaks genesis will need also to add a script that does the conversion

As discussed in person, PRs that break genesis state must include the accompanying migration logic in contrib/export/v{current-version}_to_v{new-version}, where the types are defined and the migration logic exists -- in Go.

@fedekunze
Copy link
Collaborator

fedekunze commented Jun 5, 2019

Maybe it's a good idea to also create a genesis-breaking label for issues/PRs ? cc @jackzampolin

@alessio
Copy link
Contributor

alessio commented Jun 5, 2019

Recreating structs and types that had been abandoned in favor of new ones comes with maintenance burden - incidentally it would be the only reason to write the migration script/logic in Go.

So I see 2 options here:

  1. Ship each and every breaking version of the SDK with some mechanism (possibly in app.InitChainer) to automatically migrate the genesis state.
  2. Ship SDK breaking releases with migration scripts.

If we go with 1. we need to implement such logic in Go, integrate it in the codebase and maintain it.
If we go with 2. implementing such JSON manipulation logic with a scripting language would be the sanest choice.

@fedekunze
Copy link
Collaborator

@alessio I think in the long run we'll want to automatically migrate the genesis state. Ideally any PR that breaks the genesis should include a port file with functions that port the specific genesis state that's broken to the new version.

It'd be cool to have something like what we have with the clog tool but for this purpose. So at every release after exporting the genesis and before calling app.InitGenesis it iterates over all these changes, resulting in a valid genesis for the new release. This genesis is then used as the param for the app.InitGenesis.

@alexanderbez
Copy link
Contributor

alexanderbez commented Jun 5, 2019

Recreating structs and types that had been abandoned in favor of new ones comes with maintenance burden - incidentally it would be the only reason to write the migration script/logic in Go.

There is zero maintenance burden. You define them once. You don't have to maintain anything.

We're drastically overthinking here as this is outside the scope of live upgrades. This is simply a means to migrate genesis from the current version to the next/new version. It should be pretty trivial to implement.

We only implement a migration from one version to another -- we don't maintain or support anything else. Any PR that changes the genesis is responsible for including any necessary updates to the current and only migration (script). So I agree with @fedekunze here that we should use a GH tag for this.

Example:

// contrib/export/v0_34_x_to_v0_36_0
// NOTE: We skip v0.35.0
package main

type (
        // THESE TYPES NEVER CHANGE ONCE REVIEWED/MERGED
	CurrentProposal struct {
		// ...
	}
	
	NewProposal struct {
		// ...
	}
)

func migrate(appStateBz []byte) NewGenesisState {
         var (
                appState OldGenesisState
                newAppState NewGenesisState
         )

	// use old types to load into appState
        // use new types to set into newAppState

    return newAppState
}

func main() {
	appStateBz := read(args[0])
	newGenState := migrate(appStateBz)
	
	err := write(newGenState, output)
	// handle error
}

@sabau
Copy link
Contributor Author

sabau commented Jun 6, 2019

In the short them I would prefer Python, but if can be propedeutic to for the proper store upgrade let's do it in Go

@sabau sabau removed the S:blocked Status: Blocked label Jun 7, 2019
Signed-off-by: Karoly Albert Szabo <[email protected]>
@sabau sabau changed the title Update genesis python script Update genesis script Jun 11, 2019
Once done need to experiment with go plugins to switch types as make parameters

Signed-off-by: Karoly Albert Szabo <[email protected]>
Signed-off-by: Karoly Albert Szabo <[email protected]>
Signed-off-by: Karoly Albert Szabo <[email protected]>
@sabau sabau marked this pull request as ready for review June 12, 2019 15:22
@sabau sabau requested a review from alessio as a code owner June 12, 2019 15:22
@sabau sabau requested a review from alexanderbez June 28, 2019 16:53
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK

@sabau
Copy link
Contributor Author

sabau commented Jul 1, 2019

@fedekunze @rigelrozanski last changes should fix your concerns, would you like to take a look?
On top of that we now will have to merge master and update the types with Supply 🎉

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

utACK. One minor nit

x/genutil/client/cli/migrate.go Outdated Show resolved Hide resolved
x/genutil/legacy/v036/migrate_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

One more tiny nit. Also, we need to account for and test two further changes/updates:

  1. Supply module (any pools). Sync with @rigelrozanski and @fedekunze on this.
  2. Slash event periods are now required to be included in the ValidatorSlashEvent object. Sync with @rigelrozanski on this.

x/genutil/client/cli/migrate.go Outdated Show resolved Hide resolved
@sabau
Copy link
Contributor Author

sabau commented Jul 1, 2019

I think will make more sense to merge this as it is and add the supply changes in a second PR and the validator slash changes in a third (that will be really little), or we end up having another big PR.

@sabau sabau requested a review from alexanderbez July 1, 2019 13:30
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

ACK in addressing further changes in separate PRs.

@aaronc aaronc mentioned this pull request Jul 2, 2019
6 tasks
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

uACK

@alexanderbez alexanderbez merged commit 00f753d into master Jul 3, 2019
@alexanderbez alexanderbez deleted the sabau/4409-migrate-genesis branch July 3, 2019 16:23
mircea-c pushed a commit that referenced this pull request Jul 3, 2019
@alexanderbez alexanderbez mentioned this pull request Jul 5, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:genesis relating to chain genesis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Genesis port script for 0.36
7 participants