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.

Sign up for GitHub

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

Jump to bottom

fix: fix Group-TotalWeight invariant #13116

Merged
merged 4 commits into from
Sep 1, 2022
Merged

fix: fix Group-TotalWeight invariant #13116

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9ebb53b
updates
alexanderbez Aug 31, 2022
87a88b1
updates
alexanderbez Aug 31, 2022
eb7f701
Merge branch 'main' into bez/13088-fix-group-inv
alexanderbez Sep 1, 2022
ec5268f
Merge branch 'main' into bez/13088-fix-group-inv
julienrbrt Sep 1, 2022
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
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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
}