Skip to content

Commit

Permalink
fix: Do not call Remove during Walk - part 2
Browse files Browse the repository at this point in the history
  • Loading branch information
facundomedica committed Mar 25, 2024
1 parent adc3432 commit d971f58
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 33 deletions.
75 changes: 45 additions & 30 deletions x/gov/keeper/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

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
if err != nil {
return err
}

inactiveProps, err := iter.KeyValues()

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
if err != nil {
return err
}

for _, prop := range inactiveProps {
proposal, err := k.Proposals.Get(ctx, prop.Key.K2())

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
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
Expand All @@ -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
Expand All @@ -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)

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
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()

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
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())

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
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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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.
Expand Down
21 changes: 18 additions & 3 deletions x/staking/keeper/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit d971f58

Please sign in to comment.