feat!: Staking module validator set epoching #9043
Conversation
… EpochDelegate, add panic for unrecognized message
…r BufferedValidators grpc query
…dator that's slashed or Jailed
…rresponding epoch number
…hValidatorUpdates for not using
github-actions
bot
added
the
T:Docs
|
will work on updating this tomorrow. |
|
Note: Add an event when the epoch number is incremented. |
There was a problem hiding this comment.
Here's an initial review. I'm still coming up to speed on the background here, so take everything with a grain of salt. I'm probably not aware of all the high-level invariants, so be aware of the depth of my review. One issue with the review is the clutter from all the coverage warnings makes certain sections painful to read. It might be worthwhile to add some trivial tests just so not to obscure important gaps in coverage or other warnings.
| app.mm.SetOrderEndBlockers(crisistypes.ModuleName, govtypes.ModuleName, stakingtypes.ModuleName) | ||
|
|
||
| // NOTE: slashing module endblocker should run before staking module since MsgUnjail epoch action should run before making | ||
| // validator set update on staking module |
There was a problem hiding this comment.
Cross-reference to ExecuteEpoch() below, for consistency.
x/epoching/keeper/keeper.go
Outdated
|
|
||
| // ActionStoreKey returns action store key from ID | ||
| func ActionStoreKey(epochNumber int64, actionID uint64) []byte { | ||
| return []byte(fmt.Sprintf("%s_%d_%d", EpochActionQueuePrefix, epochNumber, actionID)) |
There was a problem hiding this comment.
Consider zero-padding the numbers so that contiguous numbers are stored contiguously, vs the order 1, 10, 100, 11, 12, .., 2, 20, 21, .... Helps with locality and pre-fectching during traversals.
x/epoching/keeper/keeper.go
Outdated
| store.Set(NextEpochActionID, sdk.Uint64ToBigEndian(actionID)) | ||
| } | ||
|
|
||
| // GetNewActionID returns ID to be used for next epoch |
There was a problem hiding this comment.
Consider incrementing the stored NextEpochActionID here and dispensing with SetNewActionID() above, otherwise this doesn't actually guarantee that it's a new action ID. Note that the staking keeper's GetNewActionID() misses the increment and will keep returning the same ID - incrementing here will prevent this error.
x/epoching/keeper/keeper.go
Outdated
|
|
||
| actionID := k.GetNewActionID(ctx) | ||
| store.Set(ActionStoreKey(epochNumber, actionID), bz) | ||
| k.SetNewActionID(ctx, actionID+1) |
There was a problem hiding this comment.
See above suggestion to put the increment in GetNewActionID(), here and below.
| err := k.executeQueuedCreateValidatorMsg(cacheCtx, msg) | ||
| if err == nil { | ||
| writeCache() | ||
| } else if err = k.revertCreateValidatorMsg(ctx, msg); err != nil { |
There was a problem hiding this comment.
- Is it okay to not
writeCache()in the successful revert case? - Consider logging or telemetry for execution failure of the create validator message.
Same applies for other types below.
| @@ -539,3 +587,304 @@ func queryAllRedelegations(store sdk.KVStore, k Querier, req *types.QueryRedeleg | |||
|
|
|||
| return redels, res, err | |||
| } | |||
|
|
|||
| func (k Querier) QueuedMsgCreateValidators(c context.Context, req *types.QueryQueuedMsgCreateValidatorsRequest) (*types.QueryQueuedMsgCreateValidatorsResponse, error) { | |||
There was a problem hiding this comment.
Ugh, I'd be tempted to use reflection to consolidate similar cases rather than writing repeated boilerplate. On the other hand, reflection makes it easy to mess up, and the straightforward code is easier to analyze. Maybe I'm just reacting to all the code coverage warnings.
|
|
||
| testCases := []struct { | ||
| msg string | ||
| malleate func() |
There was a problem hiding this comment.
Please add a comment explaining the need to wrap what looks like a simple struct literal in a func.
|
@marbar3778 Could you fix the conflicts? I think we can merge this for 0.44 |
yup, will handle this and fix the issues in sims |
|
Visit https://dashboard.github.orijtech.com?pr=9043&remote=false&repo=cosmos%2Fcosmos-sdk to see benchmark details. |
tac0turtle
changed the title
|
closing this as I am pulling out chunks from it |
Description
closes #8455
Remaining tasks TODO:
super seeds #8829
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.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes