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

Ensure InitGenesis returns with non-empty validator set #8961

Closed
4 tasks
colin-axner opened this issue Mar 23, 2021 · 2 comments · Fixed by #9697
Closed
4 tasks

Ensure InitGenesis returns with non-empty validator set #8961

colin-axner opened this issue Mar 23, 2021 · 2 comments · Fixed by #9697
Assignees

Comments

@colin-axner
Copy link
Contributor

Summary

ref: #8909 #8908

Currently, the SDK allows InitGenesis to return with an empty validator set. In practice, the error for an empty validator set gets thrown in tendermint. This error is very common and is unhelpful in figuring out the root of the issue (usually an undelgated validator). The SDK should consider an empty validator set after InitGenesis to be invalid, as a chain cannot produce a block without a validator set and should then panic.

Problem Definition

Simapp relies on an empty validator set

Proposal

I would like to add this 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 simapp to use a single validator by default. I did a lot of the heavy lifting several months ago by adding SetupWithGenesisValSet function.

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})

	// 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 := 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 grpc query functions)
  • attempt to do their own initialize (not via Setup)

After fixing Setup, all other tests doing their on simapp setup should be switched to use the Setup function unless they have a very good reason for not using it

There are arguments to be made that we should keep simapp as simple as possible since most of these tests are for unit tests


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@colin-axner
Copy link
Contributor Author

colin-axner commented Mar 23, 2021

Tagging as a good first issue. This issue is not a good first issue for an unpaid contributor. I think this issue is a great issue for a new, paid engineer onboarding to the SDK. It is a lot of not super fun work, but I think it'd be very beneficial to the code base and isn't too difficult of an issue

@colin-axner colin-axner mentioned this issue Mar 23, 2021
9 tasks
@clevinson
Copy link
Contributor

Thanks for filing. I think this would be pretty valuable for chain devX. We have a few folks who joined the team recently, so we'll see if someone is able to pick this up next sprint.

@aleem1314 aleem1314 self-assigned this Jun 28, 2021
mergify bot pushed a commit that referenced this issue Aug 17, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

ref: #8961 

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->
Following up on [#9697](#9697 (review)), this PR is the first step for the #8961.

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
@github-actions github-actions bot added the stale label Aug 20, 2021
@aleem1314 aleem1314 removed the stale label Aug 25, 2021
@mergify mergify bot closed this as completed in #9697 Oct 5, 2021
mergify bot pushed a commit that referenced this issue Oct 5, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: #8961 

SDK allows InitGenesis to return with an empty validator set. In practice, the error for an empty validator set gets thrown in tendermint. To fix this,

* Add non-empty validator set check to the `mm.InitGenesis` function. This will break `simapp.Setup` because it relies on an empty validator set [#comment](#8909 (comment)).
* Update `simapp.Setup` to use a single validator.
* Fix failing tests (Most of them are keeper tests).

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this issue May 22, 2023
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

ref: cosmos#8961 

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->
Following up on [cosmos#9697](cosmos#9697 (review)), this PR is the first step for the cosmos#8961.

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this issue May 22, 2023
…s#9697)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: cosmos#8961 

SDK allows InitGenesis to return with an empty validator set. In practice, the error for an empty validator set gets thrown in tendermint. To fix this,

* Add non-empty validator set check to the `mm.InitGenesis` function. This will break `simapp.Setup` because it relies on an empty validator set [#comment](cosmos#8909 (comment)).
* Update `simapp.Setup` to use a single validator.
* Fix failing tests (Most of them are keeper tests).

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants