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

fix(x/authz)!: limit expired authz grant pruning to 200 per block #19315

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion x/authz/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,14 +374,21 @@ func (k Keeper) removeFromGrantQueue(ctx sdk.Context, grantKey []byte, granter,
return nil
}

// NOTE: backported from v50
// DequeueAndDeleteExpiredGrants deletes expired grants from the state and grant queue.
func (k Keeper) DequeueAndDeleteExpiredGrants(ctx sdk.Context) error {
func (k Keeper) DequeueAndDeleteExpiredGrants(ctx sdk.Context, limit int) error {
store := ctx.KVStore(k.storeKey)

iterator := store.Iterator(GrantQueuePrefix, sdk.InclusiveEndBytes(GrantQueueTimePrefix(ctx.BlockTime())))
defer iterator.Close()

count := 0
for ; iterator.Valid(); iterator.Next() {
// limit the amount of iterations to avoid taking too much time
if count >= limit {
return nil
}

var queueItem authz.GrantQueueItem
if err := k.cdc.Unmarshal(iterator.Value(), &queueItem); err != nil {
return err
Expand All @@ -397,6 +404,8 @@ func (k Keeper) DequeueAndDeleteExpiredGrants(ctx sdk.Context) error {
for _, typeURL := range queueItem.MsgTypeUrls {
store.Delete(grantStoreKey(grantee, granter, typeURL))
}

count++
}

return nil
Expand Down
71 changes: 70 additions & 1 deletion x/authz/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,8 @@ func (s *TestSuite) TestDequeueAllGrantsQueue() {
require.NoError(err)

newCtx := s.ctx.WithBlockTime(exp.AddDate(1, 0, 0))
err = s.authzKeeper.DequeueAndDeleteExpiredGrants(newCtx)
// setting a high limit so all grants are dequeued
err = s.authzKeeper.DequeueAndDeleteExpiredGrants(newCtx, 200)
mpoke marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(err)

s.T().Log("verify expired grants are pruned from the state")
Expand All @@ -391,6 +392,74 @@ func (s *TestSuite) TestDequeueAllGrantsQueue() {
require.Len(authzs, 1)
}

func (s *TestSuite) TestDequeueGrantsQueueEdgecases() {
require := s.Require()
addrs := s.addrs
granter := addrs[0]
grantee := addrs[1]
grantee1 := addrs[2]
exp := s.ctx.BlockTime().AddDate(0, 0, 1)
a := banktypes.SendAuthorization{SpendLimit: coins100}

// create few authorizations
err := s.authzKeeper.SaveGrant(s.ctx, grantee, granter, &a, &exp)
require.NoError(err)

err = s.authzKeeper.SaveGrant(s.ctx, grantee1, granter, &a, &exp)
require.NoError(err)

exp2 := exp.AddDate(0, 1, 0)
err = s.authzKeeper.SaveGrant(s.ctx, granter, grantee1, &a, &exp2)
require.NoError(err)

exp2 = exp.AddDate(2, 0, 0)
err = s.authzKeeper.SaveGrant(s.ctx, granter, grantee, &a, &exp2)
require.NoError(err)

newCtx := s.ctx.WithBlockTime(exp.AddDate(1, 0, 0))
err = s.authzKeeper.DequeueAndDeleteExpiredGrants(newCtx, 0)
require.NoError(err)

s.T().Log("verify no pruning happens when limit is 0")
authzs, err := s.authzKeeper.GetAuthorizations(newCtx, grantee, granter)
require.NoError(err)
require.Len(authzs, 1)

authzs, err = s.authzKeeper.GetAuthorizations(newCtx, granter, grantee1)
require.NoError(err)
require.Len(authzs, 1)

authzs, err = s.authzKeeper.GetAuthorizations(newCtx, grantee1, granter)
require.NoError(err)
require.Len(authzs, 1)

authzs, err = s.authzKeeper.GetAuthorizations(newCtx, granter, grantee)
require.NoError(err)
require.Len(authzs, 1)

// expecting to prune 1 record when limit is 1
newCtx = s.ctx.WithBlockTime(exp.AddDate(1, 0, 0))
err = s.authzKeeper.DequeueAndDeleteExpiredGrants(newCtx, 1)
require.NoError(err)

s.T().Log("verify 1 record is prunded when limit is 1")
authzs, err = s.authzKeeper.GetAuthorizations(newCtx, grantee, granter)
require.NoError(err)
require.Len(authzs, 0)

authzs, err = s.authzKeeper.GetAuthorizations(newCtx, granter, grantee1)
require.NoError(err)
require.Len(authzs, 1)

authzs, err = s.authzKeeper.GetAuthorizations(newCtx, grantee1, granter)
require.NoError(err)
require.Len(authzs, 1)

authzs, err = s.authzKeeper.GetAuthorizations(newCtx, granter, grantee)
require.NoError(err)
require.Len(authzs, 1)
}

func (s *TestSuite) TestGetAuthorization() {
addr1 := s.addrs[3]
addr2 := s.addrs[4]
Expand Down
2 changes: 1 addition & 1 deletion x/authz/module/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// BeginBlocker is called at the beginning of every block
func BeginBlocker(ctx sdk.Context, keeper keeper.Keeper) {
// delete all the mature grants
if err := keeper.DequeueAndDeleteExpiredGrants(ctx); err != nil {
if err := keeper.DequeueAndDeleteExpiredGrants(ctx, 200); err != nil {

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

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
panic(err)
}
}
Loading