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

revert 8909 #8960

Merged
merged 6 commits into from
Apr 6, 2021
Merged

revert 8909 #8960

merged 6 commits into from
Apr 6, 2021

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Mar 23, 2021

Description

reverts: #8909

I would like to do the fix in types/module/module.go:

// InitGenesis performs init genesis functionality for modules
+// InitGenesis performs init genesis functionality for modules. Exactly one
+// module must return a non-empty validator set update to correctly initialize
+// the chain.
 func (m *Manager) InitGenesis(ctx sdk.Context, cdc codec.JSONMarshaler, genesisData map[string]json.RawMessage) abci.ResponseInitChain {
        var validatorUpdates []abci.ValidatorUpdate
        for _, moduleName := range m.OrderInitGenesis {
@@ -315,6 +318,11 @@ func (m *Manager) InitGenesis(ctx sdk.Context, cdc codec.JSONMarshaler, genesisD
                }
        }
 
+       // a chain must initialize with a non-empty validator set
+       if len(validatorUpdates) == 0 {
+               panic(fmt.Sprintf("validator set is empty after InitGenesis, please ensure at least one validator is initialized with a delegation greater than or equal to the PowerReduction (%d)", sdk.PowerReduction))
+       }
+
        return abci.ResponseInitChain{
                Validators: validatorUpdates,
        }

But this breaks simapp. Specifically the default simapp runs without a validator set and it uses InitGenesis. I'd like either someone from the SDK to take over this issue or to give me an ACK on switching default simapp setup to initialize with a single validator. I ran into this issue several months ago when creating the ibc testing package, so I already have done most of the heavy lifting.

We would need to add this into the Setup function of simapp:

// generate validator private/public key
	privVal := mock.NewPV()
	pubKey, err := privVal.GetPubKey()
	require.NoError(t, err)

	// create validator set with single validator
	validator := tmtypes.NewValidator(pubKey, 1)
	valSet := tmtypes.NewValidatorSet([]*tmtypes.Validator{validator})
	signers := []tmtypes.PrivValidator{privVal}

	// generate genesis account
	senderPrivKey := secp256k1.GenPrivKey()
	acc := authtypes.NewBaseAccount(senderPrivKey.PubKey().Address().Bytes(), senderPrivKey.PubKey(), 0, 0)
	balance := banktypes.Balance{
		Address: acc.GetAddress().String(),
		Coins:   sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100000000000000))),
	}

	app := simapp.SetupWithGenesisValSet(t, valSet, []authtypes.GenesisAccount{acc}, balance)

NOTE: mock is a package in ibc-go which mocks a simple tendermint validator. It is very straightforward and we could port it over to the SDK

I did a quick check locally to see how troublesome this change is, and several tests either:

  • rely on empty validator set (looks like query functions?)
  • attempt to do their own initialize (not via Setup)

Anyone have any thoughts? We can merge the revert for now and move all this into an issue

closes: #XXXX


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

@colin-axner
Copy link
Contributor Author

opened #8961

@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #8960 (9577865) into master (2ebf79f) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head 9577865 differs from pull request most recent head 81c3ef0. Consider uploading reports for the commit 81c3ef0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8960      +/-   ##
==========================================
- Coverage   58.99%   58.87%   -0.12%     
==========================================
  Files         575      571       -4     
  Lines       32198    32108      -90     
==========================================
- Hits        18996    18905      -91     
+ Misses      10985    10982       -3     
- Partials     2217     2221       +4     
Impacted Files Coverage Δ
x/staking/types/msg.go 54.92% <ø> (-0.63%) ⬇️
x/staking/simulation/operations.go 73.46% <100.00%> (+0.29%) ⬆️
x/auth/tx/sigs.go 53.75% <0.00%> (-23.75%) ⬇️
store/mem/store.go 60.00% <0.00%> (-21.82%) ⬇️
codec/json.go 50.00% <0.00%> (-10.00%) ⬇️
store/dbadapter/store.go 91.66% <0.00%> (-8.34%) ⬇️
client/keys/utils.go 38.88% <0.00%> (-3.97%) ⬇️
x/bank/keeper/keeper.go 72.91% <0.00%> (-3.81%) ⬇️
x/auth/vesting/types/vesting_account.go 85.36% <0.00%> (-3.47%) ⬇️
types/module/configurator.go 15.38% <0.00%> (-3.14%) ⬇️
... and 59 more

@orijbot
Copy link

orijbot commented Mar 25, 2021

@colin-axner colin-axner requested a review from sunnya97 March 25, 2021 10:59
@sunnya97 sunnya97 added this to the v0.43 milestone Apr 2, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

@colin-axner could you fix the conflicts? Okay to automerge right after

@colin-axner colin-axner added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 6, 2021
@mergify mergify bot merged commit 5247a55 into master Apr 6, 2021
@mergify mergify bot deleted the colin/8909-revertandfix branch April 6, 2021 11:40
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. C:x/staking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants