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: fix Group-TotalWeight invariant #13116

Merged
merged 4 commits into from
Sep 1, 2022
Merged
Show file tree
Hide file tree
Changes from 2 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
3 changes: 1 addition & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,9 @@ Ref: https://keepachangelog.com/en/1.0.0/

### CLI Breaking Changes

* /

### Bug Fixes

* [#13116](https://github.com/cosmos/cosmos-sdk/pull/13116) Fix a dead-lock in the `Group-TotalWeight` `x/group` invariant.
* [#13046](https://github.com/cosmos/cosmos-sdk/pull/13046) Fix missing return statement in BaseApp.Query.
* [#12548](https://github.com/cosmos/cosmos-sdk/pull/12548) Prevent signing from wrong key while using multisig.
* (genutil) [#12140](https://github.com/cosmos/cosmos-sdk/pull/12140) Fix staking's genesis JSON migrate in the `simd migrate v0.46` CLI command.
Expand Down
11 changes: 7 additions & 4 deletions store/cachekv/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ import (
"sort"
"sync"

"github.com/tendermint/tendermint/libs/math"
dbm "github.com/tendermint/tm-db"

"github.com/cosmos/cosmos-sdk/internal/conv"
"github.com/cosmos/cosmos-sdk/store/listenkv"
"github.com/cosmos/cosmos-sdk/store/tracekv"
"github.com/cosmos/cosmos-sdk/store/types"
"github.com/cosmos/cosmos-sdk/types/kv"
"github.com/tendermint/tendermint/libs/math"
)

// cValue represents a cached value.
Expand Down Expand Up @@ -378,11 +378,14 @@ func (store *Store) clearUnsortedCacheSubset(unsorted []*kv.Pair, sortState sort
if item.Value == nil {
// deleted element, tracked by store.deleted
// setting arbitrary value
store.sortedCache.Set(item.Key, []byte{})
if err := store.sortedCache.Set(item.Key, []byte{}); err != nil {
panic(err)
Copy link
Member

@kocubinski kocubinski Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the line that fixes the bug? the added panic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The fix is breaking up the nested for loops into 2 separate single for loops

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a patch while I was debugging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a consensus-breaking change?

}

continue
}
err := store.sortedCache.Set(item.Key, item.Value)
if err != nil {

if err := store.sortedCache.Set(item.Key, item.Value); err != nil {
panic(err)
}
}
Expand Down
18 changes: 13 additions & 5 deletions x/group/keeper/invariants.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,8 @@ func GroupTotalWeightInvariantHelper(ctx sdk.Context, key storetypes.StoreKey, g
}
defer groupIt.Close()

groups := make(map[uint64]group.GroupInfo)
for {
membersWeight, err := groupmath.NewNonNegativeDecFromString("0")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this line failing under some conditions? I'm not understanding how the behavior of parsing "0" into a Dec could be anything but invariant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has nothing to do with the fix/changes. The change was to not have nested iterators which a violation of how iterators should be used.

if err != nil {
msg += fmt.Sprintf("error while parsing positive dec zero for group member\n%v\n", err)
return msg, broken
}
var groupInfo group.GroupInfo
_, err = groupIt.LoadNext(&groupInfo)
if errors.ErrORMIteratorDone.Is(err) {
Expand All @@ -54,6 +50,16 @@ func GroupTotalWeightInvariantHelper(ctx sdk.Context, key storetypes.StoreKey, g
return msg, broken
}

groups[groupInfo.Id] = groupInfo
}

for _, groupInfo := range groups {
membersWeight, err := groupmath.NewNonNegativeDecFromString("0")
if err != nil {
msg += fmt.Sprintf("error while parsing positive dec zero for group member\n%v\n", err)
return msg, broken
}

memIt, err := groupMemberByGroupIndex.Get(ctx.KVStore(key), groupInfo.Id)
if err != nil {
msg += fmt.Sprintf("error while returning group member iterator for group with ID %d\n%v\n", groupInfo.Id, err)
Expand All @@ -77,6 +83,7 @@ func GroupTotalWeightInvariantHelper(ctx sdk.Context, key storetypes.StoreKey, g
msg += fmt.Sprintf("error while parsing non-nengative decimal for group member %s\n%v\n", groupMember.Member.Address, err)
return msg, broken
}

membersWeight, err = groupmath.Add(membersWeight, curMemWeight)
if err != nil {
msg += fmt.Sprintf("decimal addition error while adding group member voting weight to total voting weight\n%v\n", err)
Expand All @@ -96,5 +103,6 @@ func GroupTotalWeightInvariantHelper(ctx sdk.Context, key storetypes.StoreKey, g
break
}
}

return msg, broken
}