From 007dd2ced860da2775d8ca56887ae8d89538656f Mon Sep 17 00:00:00 2001 From: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Date: Mon, 25 Mar 2024 21:42:50 +0100 Subject: [PATCH] fix: Do not call Remove during Walk - part 2 (#19851) --- CHANGELOG.md | 1 + x/gov/keeper/abci.go | 75 +++++++++++++++++++++-------------- x/staking/keeper/validator.go | 21 ++++++++-- 3 files changed, 64 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c960cfc29f..bd68c0a0b9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i * (server) [#18994](https://github.com/cosmos/cosmos-sdk/pull/18994) Update server context directly rather than a reference to a sub-object * (crypto) [#19691](https://github.com/cosmos/cosmos-sdk/pull/19691) Fix tx sign doesn't throw an error when incorrect Ledger is used. * [#19833](https://github.com/cosmos/cosmos-sdk/pull/19833) Fix some places in which we call Remove inside a Walk. +* [#19851](https://github.com/cosmos/cosmos-sdk/pull/19851) Fix some places in which we call Remove inside a Walk (x/staking and x/gov). ### API Breaking Changes diff --git a/x/gov/keeper/abci.go b/x/gov/keeper/abci.go index 9234377be88..4e4052102b3 100644 --- a/x/gov/keeper/abci.go +++ b/x/gov/keeper/abci.go @@ -27,35 +27,45 @@ func (k Keeper) EndBlocker(ctx context.Context) error { // delete dead proposals from store and returns theirs deposits. // A proposal is dead when it's inactive and didn't get enough deposit on time to get into voting phase. rng := collections.NewPrefixUntilPairRange[time.Time, uint64](k.environment.HeaderService.GetHeaderInfo(ctx).Time) - err := k.InactiveProposalsQueue.Walk(ctx, rng, func(key collections.Pair[time.Time, uint64], _ uint64) (bool, error) { - proposal, err := k.Proposals.Get(ctx, key.K2()) + iter, err := k.InactiveProposalsQueue.Iterate(ctx, rng) + if err != nil { + return err + } + + inactiveProps, err := iter.KeyValues() + if err != nil { + return err + } + + for _, prop := range inactiveProps { + proposal, err := k.Proposals.Get(ctx, prop.Key.K2()) if err != nil { // if the proposal has an encoding error, this means it cannot be processed by x/gov // this could be due to some types missing their registration // instead of returning an error (i.e, halting the chain), we fail the proposal if errors.Is(err, collections.ErrEncoding) { - proposal.Id = key.K2() + proposal.Id = prop.Key.K2() if err := failUnsupportedProposal(logger, ctx, k, proposal, err.Error(), false); err != nil { - return false, err + return err } if err = k.DeleteProposal(ctx, proposal.Id); err != nil { - return false, err + return err } - return false, nil + continue } - return false, err + return err } if err = k.DeleteProposal(ctx, proposal.Id); err != nil { - return false, err + return err } params, err := k.Params.Get(ctx) if err != nil { - return false, err + return err } if !params.BurnProposalDepositPrevote { err = k.RefundAndDeleteDeposits(ctx, proposal.Id) // refund deposit if proposal got removed without getting 100% of the proposal @@ -64,7 +74,7 @@ func (k Keeper) EndBlocker(ctx context.Context) error { } if err != nil { - return false, err + return err } // called when proposal become inactive @@ -91,42 +101,49 @@ func (k Keeper) EndBlocker(ctx context.Context) error { "min_deposit", sdk.NewCoins(proposal.GetMinDepositFromParams(params)...).String(), "total_deposit", sdk.NewCoins(proposal.TotalDeposit...).String(), ) + } - return false, nil - }) + // fetch active proposals whose voting periods have ended (are passed the block time) + rng = collections.NewPrefixUntilPairRange[time.Time, uint64](k.environment.HeaderService.GetHeaderInfo(ctx).Time) + + iter, err = k.ActiveProposalsQueue.Iterate(ctx, rng) if err != nil { return err } - // fetch active proposals whose voting periods have ended (are passed the block time) - rng = collections.NewPrefixUntilPairRange[time.Time, uint64](k.environment.HeaderService.GetHeaderInfo(ctx).Time) - err = k.ActiveProposalsQueue.Walk(ctx, rng, func(key collections.Pair[time.Time, uint64], _ uint64) (bool, error) { - proposal, err := k.Proposals.Get(ctx, key.K2()) + activeProps, err := iter.KeyValues() + if err != nil { + return err + } + + // err = k.ActiveProposalsQueue.Walk(ctx, rng, func(key collections.Pair[time.Time, uint64], _ uint64) (bool, error) { + for _, prop := range activeProps { + proposal, err := k.Proposals.Get(ctx, prop.Key.K2()) if err != nil { // if the proposal has an encoding error, this means it cannot be processed by x/gov // this could be due to some types missing their registration // instead of returning an error (i.e, halting the chain), we fail the proposal if errors.Is(err, collections.ErrEncoding) { - proposal.Id = key.K2() + proposal.Id = prop.Key.K2() if err := failUnsupportedProposal(logger, ctx, k, proposal, err.Error(), true); err != nil { - return false, err + return err } if err = k.ActiveProposalsQueue.Remove(ctx, collections.Join(*proposal.VotingEndTime, proposal.Id)); err != nil { - return false, err + return err } - return false, nil + continue } - return false, err + return err } var tagValue, logMsg string passes, burnDeposits, tallyResults, err := k.Tally(ctx, proposal) if err != nil { - return false, err + return err } // Deposits are always burned if tally said so, regardless of the proposal type. @@ -154,7 +171,7 @@ func (k Keeper) EndBlocker(ctx context.Context) error { } if err = k.ActiveProposalsQueue.Remove(ctx, collections.Join(*proposal.VotingEndTime, proposal.Id)); err != nil { - return false, err + return err } switch { @@ -210,14 +227,14 @@ func (k Keeper) EndBlocker(ctx context.Context) error { proposal.Expedited = false // can be removed as never read but kept for state coherence params, err := k.Params.Get(ctx) if err != nil { - return false, err + return err } endTime := proposal.VotingStartTime.Add(*params.VotingPeriod) proposal.VotingEndTime = &endTime err = k.ActiveProposalsQueue.Set(ctx, collections.Join(*proposal.VotingEndTime, proposal.Id), proposal.Id) if err != nil { - return false, err + return err } if proposal.ProposalType == v1.ProposalType_PROPOSAL_TYPE_EXPEDITED { @@ -237,7 +254,7 @@ func (k Keeper) EndBlocker(ctx context.Context) error { proposal.FinalTallyResult = &tallyResults if err = k.Proposals.Set(ctx, proposal.Id, proposal); err != nil { - return false, err + return err } // call hook when proposal become active @@ -264,10 +281,8 @@ func (k Keeper) EndBlocker(ctx context.Context) error { ); err != nil { logger.Error("failed to emit event", "error", err) } - - return false, nil - }) - return err + } + return nil } // executes route(msg) and recovers from panic. diff --git a/x/staking/keeper/validator.go b/x/staking/keeper/validator.go index b8c4472cccb..6e496f75676 100644 --- a/x/staking/keeper/validator.go +++ b/x/staking/keeper/validator.go @@ -494,9 +494,24 @@ func (k Keeper) UnbondAllMatureValidators(ctx context.Context) error { rng := new(collections.Range[collections.Triple[uint64, time.Time, uint64]]). EndInclusive(collections.Join3(uint64(29), blockTime, blockHeight)) - return k.ValidatorQueue.Walk(ctx, rng, func(key collections.Triple[uint64, time.Time, uint64], value types.ValAddresses) (stop bool, err error) { - return false, k.unbondMatureValidators(ctx, blockHeight, blockTime, key, value) - }) + // get all the values before performing any delete operations + iter, err := k.ValidatorQueue.Iterate(ctx, rng) + if err != nil { + return err + } + + kvs, err := iter.KeyValues() + if err != nil { + return err + } + + for _, kv := range kvs { + if err := k.unbondMatureValidators(ctx, blockHeight, blockTime, kv.Key, kv.Value); err != nil { + return err + } + } + + return nil } func (k Keeper) unbondMatureValidators(