From 5e23284d716d7396e7330fb08d0a44f97d5862d8 Mon Sep 17 00:00:00 2001 From: MSalopek Date: Thu, 1 Feb 2024 11:35:04 +0100 Subject: [PATCH 1/2] fix(x/authz)!: limit expired authz grant pruning to 200 per block (#19315) --- x/authz/keeper/keeper.go | 11 +++++- x/authz/keeper/keeper_test.go | 71 ++++++++++++++++++++++++++++++++++- x/authz/module/abci.go | 2 +- 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/x/authz/keeper/keeper.go b/x/authz/keeper/keeper.go index 788b87418e23..0056eb9ffd00 100644 --- a/x/authz/keeper/keeper.go +++ b/x/authz/keeper/keeper.go @@ -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 @@ -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 diff --git a/x/authz/keeper/keeper_test.go b/x/authz/keeper/keeper_test.go index c9d3189ca055..2995cfd46cf4 100644 --- a/x/authz/keeper/keeper_test.go +++ b/x/authz/keeper/keeper_test.go @@ -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) require.NoError(err) s.T().Log("verify expired grants are pruned from the state") @@ -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] diff --git a/x/authz/module/abci.go b/x/authz/module/abci.go index c1015abae841..36efb4fadd4d 100644 --- a/x/authz/module/abci.go +++ b/x/authz/module/abci.go @@ -8,7 +8,7 @@ import ( // 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 { panic(err) } } From 42b82202c5b769d80befae4ade1ac69f98d58aba Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Fri, 8 Mar 2024 15:38:21 -0700 Subject: [PATCH 2/2] changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b712292c0f7..31b00c0cc098 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (store) [#525](https://github.com/osmosis-labs/cosmos-sdk/pull/525) CacheKV speedups * (slashing) [#548](https://github.com/osmosis-labs/cosmos-sdk/pull/548) Implement v0.50 slashing bitmap logic * (slashing) [#543](https://github.com/osmosis-labs/cosmos-sdk/pull/543) Make slashing not write sign info every block +* (authz) [#513](https://github.com/osmosis-labs/cosmos-sdk/pull/513) Limit expired authz grant pruning to 200 per block ## IAVL v23 v1 Releases