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 an ability to update ConsensusParams #6197

Closed
4 tasks
whylee259 opened this issue May 12, 2020 · 9 comments · Fixed by #7107
Closed
4 tasks

Add an ability to update ConsensusParams #6197

whylee259 opened this issue May 12, 2020 · 9 comments · Fixed by #7107
Assignees
Labels

Comments

@whylee259
Copy link
Contributor

whylee259 commented May 12, 2020

Summary

Adding an ability to update ConsensusParams by collecting ConsensusParamUpdates from Module.EndBlock.

Problem Definition

Updating ConsensusParams can be done by passing changes to Tendermint through ResponseEndBlock.

We can decide whether to change using the governance module.

However, ModuleManager does not collect ConsensusParamUpdates nor passes them to Tendermint because the interface of AppModule.EndBlock returns only ValidatorUpdate.

func (m *Manager) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock {
ctx = ctx.WithEventManager(sdk.NewEventManager())
validatorUpdates := []abci.ValidatorUpdate{}
for _, moduleName := range m.OrderEndBlockers {
moduleValUpdates := m.Modules[moduleName].EndBlock(ctx, req)
// use these validator updates if provided, the module manager assumes
// only one module will update the validator set
if len(moduleValUpdates) > 0 {
if len(validatorUpdates) > 0 {
panic("validator EndBlock updates already set by a previous module")
}
validatorUpdates = moduleValUpdates
}
}
return abci.ResponseEndBlock{
ValidatorUpdates: validatorUpdates,
Events: ctx.EventManager().ABCIEvents(),
}
}

Proposal

  • AppModule.EndBlock returns ConsensusParamUpdates also.
  • passes it to Tendermint

For Admin Use

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

The SDK now manages consensus parameters via the x/params module and are changed via governance proposals. That being said, we should probably figure out if we need to send these to Tendermint at all.

Do we need to @marbar3778 @melekes ?

@tac0turtle
Copy link
Member

tac0turtle commented May 12, 2020

yes they should be sent to Tendermint. If not then while the application changes the params the Tendermint logic will adhere to the genesis consensus params and this may cause problems

@whylee259
Copy link
Contributor Author

Any schedule to handle this issue?

@tac0turtle
Copy link
Member

Add this to #6365 as it should be done prioir to the next major release

@alexanderbez
Copy link
Contributor

What do you mean @marbar3778 -- this was already done. #5952

@tac0turtle
Copy link
Member

The SDK now manages consensus parameters via the x/params module and are changed via governance proposals. That being said, we should probably figure out if we need to send these to Tendermint at all.

ah based off this comment I thought it wasn't

@whylee259
Copy link
Contributor Author

whylee259 commented Jul 22, 2020

The SDK now manages consensus parameters via the x/params module and are changed via governance proposals. That being said, we should probably figure out if we need to send these to Tendermint at all.

ah based off this comment I thought it wasn't

As @marbar3778 said, this issue is not done. I agree with him.
PTAL @alexanderbez

@alexanderbez
Copy link
Contributor

What do you mean it's not done? I pointed to the PR that completes the work => #5952

@tac0turtle
Copy link
Member

tac0turtle commented Aug 19, 2020

ConsensusParams should be updated in Endblock via:

`ConsensusParamUpdates (ConsensusParams)`: Changes to consensus-critical time, size, and other parameters.

This is not implemented yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants