fix: fix Group-TotalWeight invariant #13116
Conversation
Codecov Report
@@ Coverage Diff @@
## main #13116 +/- ##
==========================================
- Coverage 55.87% 53.15% -2.73%
==========================================
Files 646 641 -5
Lines 54895 54874 -21
==========================================
- Hits 30675 29167 -1508
- Misses 21762 23383 +1621
+ Partials 2458 2324 -134
|
| for { | ||
| membersWeight, err := groupmath.NewNonNegativeDecFromString("0") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
is this the line that fixes the bug? the added panic
There was a problem hiding this comment.
No. The fix is breaking up the nested for loops into 2 separate single for loops
There was a problem hiding this comment.
This is just a patch while I was debugging.
There was a problem hiding this comment.
is this a consensus-breaking change?
There was a problem hiding this comment.
Thanks @alexanderbez !
What's still unclear to me is why this error appeared only in your CommunitySpend PR.
| @@ -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) | |||
There was a problem hiding this comment.
is this a consensus-breaking change?
Description
Closes: #13088
The
Group-TotalWeightwas poorly implemented as it violates the correct usage of KVStore iterators. This resulted in deadlocks.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change