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

feat: add endblocker with valsetupdate type #15829

Merged
merged 10 commits into from
Apr 14, 2023
Merged

Conversation

tac0turtle
Copy link
Member

@tac0turtle tac0turtle commented Apr 13, 2023

Description

This pr adds valset updating to the module manager in the shape of a endblocker interface


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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • 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 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)

runtime/app.go Fixed Show fixed Hide fixed
x/staking/keeper/abci.go Fixed Show fixed Hide fixed
x/staking/keeper/abci.go Fixed Show fixed Hide fixed
@@ -191,13 +189,15 @@
// BeginBlock returns the begin blocker for the staking module.
func (am AppModule) BeginBlock(ctx context.Context) error {
c := sdk.UnwrapSDKContext(ctx)
BeginBlocker(c, am.keeper)

am.keeper.BeginBlocker(c)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call
x/staking/keeper/abci.go Fixed Show fixed Hide fixed
baseapp/abci.go Outdated
Comment on lines 252 to 258
}
}

// set the validator set for the next block
res.ValidatorUpdates = app.updatedValidators

return res
Copy link
Contributor

Choose a reason for hiding this comment

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

Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).EndBlock (baseapp/abci.go:229)

@tac0turtle tac0turtle marked this pull request as ready for review April 13, 2023 11:56
@tac0turtle tac0turtle requested a review from a team as a code owner April 13, 2023 11:56
@github-actions

This comment has been minimized.

@github-prbot github-prbot requested review from a team, aaronc and samricotta and removed request for a team April 13, 2023 11:57
x/staking/keeper/abci.go Fixed Show fixed Hide fixed
x/staking/module.go Fixed Show fixed Hide fixed
runtime/app.go Outdated
@@ -52,6 +51,8 @@ type App struct {
// initChainer is the init chainer function defined by the app config.
// this is only required if the chain wants to add special InitChainer logic.
initChainer sdk.InitChainer

ValSetUpdate []abci.ValidatorUpdate
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed?

baseapp/abci.go Outdated
@@ -252,6 +252,9 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc
}
}

// set the validator set for the next block
res.ValidatorUpdates = app.updatedValidators
Copy link
Member

Choose a reason for hiding this comment

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

The challenge is that this overwrites updates in the case where the app is not using the update service. So we need to have some bool flag on the update service to indicate that updates have been set I think

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.

This level of abstraction doesn't make sense to me personally.

baseapp/abci.go Outdated
@@ -252,6 +252,9 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc
}
}

// set the validator set for the next block
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

QQ: When is res, err = app.endBlocker(app.deliverState.ctx, req) going to become err = app.endBlocker(app.deliverState.ctx, req)? i.e. when are we removing the ResponseEndBlock types from core's EndBlock API?

abci "github.com/cometbft/cometbft/abci/types"
)

// ValidatorUpdateService is the service that runtime will provide to the module that sets validator updates.
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought BaseApp would just have a reference to the service, which is defined in runtime? Why is this being defined in BaseApp?

return sh.Ctx
}

// TurnBlockTimeDiff calls EndBlocker and updates the block time by adding the
// duration to the current block time
func (sh *Helper) TurnBlockTimeDiff(diff time.Duration) sdk.Context {
sh.Ctx = sh.Ctx.WithBlockTime(sh.Ctx.BlockHeader().Time.Add(diff))
staking.EndBlocker(sh.Ctx, sh.k)
sh.k.EndBlocker(sh.Ctx)

Check warning

Code scanning / gosec

Errors unhandled.

Errors unhandled.
@@ -127,15 +126,15 @@
// TurnBlock calls EndBlocker and updates the block time
func (sh *Helper) TurnBlock(newTime time.Time) sdk.Context {
sh.Ctx = sh.Ctx.WithBlockTime(newTime)
staking.EndBlocker(sh.Ctx, sh.k)
sh.k.EndBlocker(sh.Ctx)

Check warning

Code scanning / gosec

Errors unhandled.

Errors unhandled.
Comment on lines 181 to 186
message ValidatorUpdate {
option (gogoproto.stringer) = true;

tendermint.crypto.PublicKey pub_key = 1 [(gogoproto.nullable) = false];
int64 power = 2;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

we only need this type to make it work, should we just depend on comet for it? @aaronc @alexanderbez

Copy link
Contributor

Choose a reason for hiding this comment

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

The pubkey? I think that's fine

Copy link
Member Author

@tac0turtle tac0turtle Apr 13, 2023

Choose a reason for hiding this comment

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

The validator update type

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're defining parts of ABCI ourselves, I would say let's define the entire type, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

since this endblock is tied to comet i may leave it for now.

@tac0turtle tac0turtle changed the title feat: add validatorset update in runtime feat: add endblocker with valsetupdate type Apr 13, 2023
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker)

return k.BlockValidatorUpdates(ctx)
return k.BlockValidatorUpdates(sdk.UnwrapSDKContext(ctx)), nil

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call
func (am AppModule) EndBlock(ctx sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate {
return EndBlocker(ctx, am.keeper)
func (am AppModule) EndBlock(ctx context.Context) ([]sdk.ValidatorUpdate, error) {
return am.keeper.EndBlocker(ctx)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call
@@ -695,6 +701,22 @@
if err != nil {
return abci.ResponseEndBlock{}, err
}
} else if module, ok := m.Modules[moduleName].(HasABCIEndblock); ok {
moduleValUpdates, err := module.EndBlock(ctx)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call path flow from Begin/EndBlock to a panic call
Comment on lines 181 to 186
message ValidatorUpdate {
option (gogoproto.stringer) = true;

tendermint.crypto.PublicKey pub_key = 1 [(gogoproto.nullable) = false];
int64 power = 2;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The pubkey? I think that's fine

@tac0turtle tac0turtle enabled auto-merge (squash) April 14, 2023 09:20
@tac0turtle tac0turtle merged commit 8d6c23f into main Apr 14, 2023
@tac0turtle tac0turtle deleted the marko/valsetupdate branch April 14, 2023 09:41
@@ -212,6 +213,11 @@ type EndBlockAppModule interface {
EndBlock(sdk.Context, abci.RequestEndBlock) []abci.ValidatorUpdate
}

type HasABCIEndblock interface {
Copy link
Member

@julienrbrt julienrbrt Apr 14, 2023

Choose a reason for hiding this comment

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

Do we have somewhere something that explains the difference between:

HasABCIEndblock, EndBlockAppModule and HasEndBlocker?

Those 3 extension interfaces doing seemingly the same thing (I know they don't but still) could get confusing.

Small nit on that new extension interface, I think it should have had a capital B for block for consistency with the other 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

the one above gets removed in another pr. Left a todo for when we do docs on core api

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

Successfully merging this pull request may close these issues.

None yet

4 participants