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

Add height in exported genesis #7089

Merged
merged 70 commits into from
Sep 3, 2020
Merged

Add height in exported genesis #7089

merged 70 commits into from
Sep 3, 2020

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Aug 18, 2020

Description

closes: #7018

Important changes

  • the initial_height field is correctly populated in the export command
  • a SetInitialVersion has been added to baseapp's CommitMultiStore, which sets the initial version to IAVL stores
  • the exported genesis is now marshalled using Amino instead of encoding/json

Notes

simd export works with the correct height.

TODO:

  • Add test

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Aug 18, 2020

Codecov Report

Merging #7089 into master will increase coverage by 0.03%.
The diff coverage is 74.54%.

@@            Coverage Diff             @@
##           master    #7089      +/-   ##
==========================================
+ Coverage   54.99%   55.02%   +0.03%     
==========================================
  Files         562      562              
  Lines       38703    38737      +34     
==========================================
+ Hits        21283    21315      +32     
- Misses      15671    15673       +2     
  Partials     1749     1749              

@amaury1093 amaury1093 marked this pull request as ready for review August 19, 2020 09:31
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.

Looks good apart from the question I have. Also, this doesn't actually seem to work, but I presume it's completely unrelated to this PR:

panic: UnmarshalJSON cannot decode empty bytes

goroutine 1 [running]:
github.com/cosmos/cosmos-sdk/x/params/types.Subspace.Get(0x2e8a7d18, 0xc001261580, 0xc000122170, 0x59130a0, 0xc001261650, 0x5913120, 0xc0012616b0, 0xc001009340, 0x4, 0x1a, ...)
        github.com/cosmos/cosmos-sdk/x/params/types/subspace.go:109 +0x30c
github.com/cosmos/cosmos-sdk/x/params/types.Subspace.GetParamSet(0x2e8a7d18, 0xc001261580, 0xc000122170, 0x59130a0, 0xc001261650, 0x5913120, 0xc0012616b0, 0xc001009340, 0x4, 0x1a, ...)
        github.com/cosmos/cosmos-sdk/x/params/types/subspace.go:222 +0x188
github.com/cosmos/cosmos-sdk/x/auth/keeper.AccountKeeper.GetParams(...)
        github.com/cosmos/cosmos-sdk/x/auth/keeper/params.go:15
github.com/cosmos/cosmos-sdk/x/auth.ExportGenesis(0x592e360, 0xc000126008, 0x5942200, 0xc000fe0f00, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        github.com/cosmos/cosmos-sdk/x/auth/genesis.go:32 +0x130
github.com/cosmos/cosmos-sdk/x/auth.AppModule.ExportGenesis(0x59130a0, 0xc0012615e0, 0x2e8a7d18, 0xc001261580, 0x2e8a7d18, 0xc001261580, 0xc000122170, 0x59130a0, 0xc001261650, 0x5913120, ...)
        github.com/cosmos/cosmos-sdk/x/auth/module.go:138 +0x98
github.com/cosmos/cosmos-sdk/types/module.(*Manager).ExportGenesis(0xc001156a10, 0x592e360, 0xc000126008, 0x5942200, 0xc000fe0f00, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        github.com/cosmos/cosmos-sdk/types/module/module.go:315 +0x11e
github.com/cosmos/cosmos-sdk/simapp.(*SimApp).ExportAppStateAndValidators(0xc000207100, 0xc001266200, 0x63f4548, 0x0, 0x0, 0x0, 0xc000122101, 0xc001269b00, 0x0, 0x0, ...)
        github.com/cosmos/cosmos-sdk/simapp/export.go:31 +0x170
github.com/cosmos/cosmos-sdk/simapp/simd/cmd.exportAppStateAndTMValidators(0x592e7e0, 0xc001266240, 0x5944400, 0xc000122038, 0x0, 0x0, 0xffffffffffffffff, 0x0, 0x63f4548, 0x0, ...)
        github.com/cosmos/cosmos-sdk/simapp/simd/cmd/root.go:210 +0x251
github.com/cosmos/cosmos-sdk/server.ExportCmd.func1(0xc00116cdc0, 0xc0011ee380, 0x0, 0x2, 0x0, 0x0)
        github.com/cosmos/cosmos-sdk/server/export.go:67 +0x351
github.com/spf13/cobra.(*Command).execute(0xc00116cdc0, 0xc0011ee360, 0x2, 0x2, 0xc00116cdc0, 0xc0011ee360)
        github.com/spf13/cobra@v1.0.0/command.go:842 +0x47c
github.com/spf13/cobra.(*Command).ExecuteC(0xc00026b340, 0x0, 0x0, 0xc00111bc80)
        github.com/spf13/cobra@v1.0.0/command.go:950 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
        github.com/spf13/cobra@v1.0.0/command.go:887
github.com/spf13/cobra.(*Command).ExecuteContext(...)
        github.com/spf13/cobra@v1.0.0/command.go:880
github.com/cosmos/cosmos-sdk/simapp/simd/cmd.Execute(0xc00026b340, 0x5931160, 0xc00111d3e0)
        github.com/cosmos/cosmos-sdk/simapp/simd/cmd/root.go:82 +0x158
main.main()
        github.com/cosmos/cosmos-sdk/simapp/simd/main.go:11 +0x2a

server/export.go Outdated Show resolved Hide resolved
@amaury1093
Copy link
Contributor Author

amaury1093 commented Aug 19, 2020

@alexanderbez How did you get the JSON error? (i already fixed one of those in this PR in ValidateGenesis)

here's how i tested:

// assuming keys are set up
./build/simd init amaury --chain-id my-chain
./build/simd add-genesis-account $(./build/simd keys show alice -a) 1000mytoken,100000000stake
./build/simd gentx alice --chain-id my-chain 
./build/simd collect-gentxs 
./build/simd start 
// wait a bit
./build/simd export 

@alexanderbez
Copy link
Contributor

@amaurymartiny

$ make clean localnet-start
$ BUILDDIR=./build/macos make build-simd
// wait for a few blocks...
$ ./build/macos/simd export

@amaury1093 amaury1093 marked this pull request as draft August 20, 2020 09:07
@clevinson clevinson linked an issue Aug 21, 2020 that may be closed by this pull request
4 tasks
// as if they could withdraw from the start of the next block
ctx := app.NewContext(true, tmproto.Header{Height: app.LastBlockHeight()})

// We export at last height + 1, because that's the height at which
// Tendermint will start InitChain.
height := app.LastBlockHeight() + 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexanderbez I finally still decided to export at N + 1.

Tendermint will then call InitChain with initial_height = N + 1 (or 1 if not set). So we InitChain(optionally also BeginBlock) at N+1, and the first commit on the new/upgraded chain will be at N+1.

Let me know if that makes sense to you.

@amaury1093 amaury1093 marked this pull request as ready for review September 1, 2020 17:43
@amaury1093
Copy link
Contributor Author

amaury1093 commented Sep 1, 2020

@alexanderbez thanks for the review & test, I addressed both points (added a test case for 1.)

@alexanderbez
Copy link
Contributor

@amaurymartiny did you test the flow on an actual chain? If so, feel free to merge 👍

@clevinson
Copy link
Contributor

  • Using --for-zero-height doesn't set the initial height to zero.
  • Actual export & restart doesn't work:
    2a. Start a network
    2b. Stop network
    2c. Export state to file (non-zero height)
    2d. Perform unsafe-reset-all on all nodes
    2e. Restart all nodes
    2f. Observe consensus failure

Just ran these tests on an actual chain. Validated that the --for-zero-height works fine, and that running the flow of export & restart works for me as well now.

However, I'm getting panics when running export with a manually set --height flag:

$ ./build/macos/simd export --height 5 --home build/node0/simd
panic: interface conversion: interface is nil, not types.KVStore

goroutine 1 [running]:
github.com/cosmos/cosmos-sdk/store/rootmulti.(*Store).GetKVStore(0xc000f29ea0, 0x5a18a00, 0xc000e615c0, 0x44005a2600, 0xffffffffffffffff)
	github.com/cosmos/cosmos-sdk/store/rootmulti/store.go:444 +0x86
github.com/cosmos/cosmos-sdk/types.Context.KVStore(0x5a36a80, 0xc0000a4008, 0x5a50740, 0xc000f29ea0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	github.com/cosmos/cosmos-sdk/types/context.go:229 +0x89
github.com/cosmos/cosmos-sdk/x/capability/keeper.(*Keeper).InitializeAndSeal(0xc0005fb590, 0x5a36a80, 0xc0000a4008, 0x5a50740, 0xc000f29ea0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	github.com/cosmos/cosmos-sdk/x/capability/keeper/keeper.go:95 +0x92
github.com/cosmos/cosmos-sdk/simapp.NewSimApp(0x5a36ec0, 0xc0010cbd60, 0x5a55540, 0xc0005a0198, 0x0, 0x0, 0xc0005a0100, 0xc000e9b950, 0x0, 0x0, ...)
	github.com/cosmos/cosmos-sdk/simapp/app.go:394 +0x651f
github.com/cosmos/cosmos-sdk/simapp/simd/cmd.exportAppStateAndTMValidators(0x5a36ec0, 0xc0010cbd60, 0x5a55540, 0xc0005a0198, 0x0, 0x0, 0x5, 0x0, 0x67a0558, 0x0, ...)
	github.com/cosmos/cosmos-sdk/simapp/simd/cmd/root.go:198 +0x1f4
github.com/cosmos/cosmos-sdk/server.ExportCmd.func1(0xc0010e82c0, 0xc0001d0d40, 0x0, 0x4, 0x0, 0x0)
	github.com/cosmos/cosmos-sdk/server/export.go:67 +0x333
github.com/spf13/cobra.(*Command).execute(0xc0010e82c0, 0xc0001d0d00, 0x4, 0x4, 0xc0010e82c0, 0xc0001d0d00)
	github.com/spf13/cobra@v1.0.0/command.go:842 +0x453
github.com/spf13/cobra.(*Command).ExecuteC(0xc001028840, 0x0, 0x0, 0xc0010978c0)
	github.com/spf13/cobra@v1.0.0/command.go:950 +0x349
github.com/spf13/cobra.(*Command).Execute(...)
	github.com/spf13/cobra@v1.0.0/command.go:887
github.com/spf13/cobra.(*Command).ExecuteContext(...)
	github.com/spf13/cobra@v1.0.0/command.go:880
github.com/cosmos/cosmos-sdk/simapp/simd/cmd.Execute(0xc001028840, 0x5a3a400, 0xc0010a2b90)
	github.com/cosmos/cosmos-sdk/simapp/simd/cmd/root.go:79 +0x155
main.main()
	github.com/cosmos/cosmos-sdk/simapp/simd/main.go:11 +0x27

@amaury1093
Copy link
Contributor Author

amaury1093 commented Sep 2, 2020

@clevinson thanks, reproed and fixed! I also added a test case.

@alexanderbez Yeah, I tested on an actual chain like how Cory did. Once #6839 is in, will try on the hub too.

@alexanderbez
Copy link
Contributor

Ok, feel free to merge whenever you're good.

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Sep 2, 2020
Copy link
Contributor

@sahith-narahari sahith-narahari left a comment

Choose a reason for hiding this comment

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

utACK

@mergify mergify bot merged commit 3b9b58c into master Sep 3, 2020
@mergify mergify bot deleted the am-7018-export-height branch September 3, 2020 10:11
erikgrinaker added a commit to cosmos/iavl that referenced this pull request Sep 25, 2020
Requested in cosmos/cosmos-sdk#7089 (comment).

I didn't add a `GetInitialVersion()`, since it's not clear whether this should return only the option or the _actual_ initial version that exists in a tree.

It might be cleaner to add `GetOptions()`, `SetOptions()`, and `UpdateOptions(func (o Options) Options)`, but we may not want all options to be mutable after construction.

CC @amaurymartiny
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* Add height in exported genesis

* +1

* Add test

* Refactor ctx in setupApp

* Use amino in export

* Use tmjson

* Add custom initialVersion (set to 0 for now)

* Add comment

* Add mount in initChainer

* app.LastBlockheight

* InitializeAndSeal in InitChain?

* Revert create store with initial version

* Update to latest iavl

* Check height in test

* Make it work

* Add more tests

* Rename interface

* Use struct isntead of 6 args

* Fix lint

* Remove stray fmt

* Revert go mod/sum

* Install iavl rc3

* Update comments

* Add fee in network

* Typo

* Fix logic in commit

* Fix tests

* Only set initial version on > 1

* Genesis block num = 1

* Fresh chain, genesis block = 0

* Add comments

* Revert Mutable/ImmutableTree

* Allow for zero height

* Fix restart

* Add comments

* Add comments, fix test

* Fix remaining one test

* Add panic test

* Update comment

* Add test for --height

* No cast

* Add check that genesis file exists

* Remove duplicate imports

* Fail early

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Jack Zampolin <jack.zampolin@gmail.com>
Co-authored-by: Cory <cjlevinson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support arbitrary initial height
8 participants