-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 non-deterministic map iteration in fuzzer #2069
Conversation
are more than two txs in a block.
ff67698
to
be4c7de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left two minor remarks @ValarDragon 👍
keys[i] = key | ||
i++ | ||
} | ||
return keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont' we wanna do return strings.Sort(keys)
here?
for _, mVal := range validators { | ||
keys := getKeys(validators) | ||
|
||
for i := 0; i < len(keys); i++ { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just, for _, key := range getKeys(validators) { mVal := validators[key] }
?
@@ -166,12 +168,14 @@ func TestAppStateDeterminism(t *testing.T) { | |||
}, | |||
[]simulation.RandSetup{}, | |||
[]simulation.Invariant{noOpInvariant}, | |||
0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so (changed to 20, 20 in latest commit). Earlier values were with the intention that this tests the framework, but as chris pointed out this would be valuable to test gaia itself.
Codecov Report
@@ Coverage Diff @@
## release/v0.24.0 #2069 +/- ##
===================================================
+ Coverage 63.81% 63.97% +0.16%
===================================================
Files 113 113
Lines 6665 6665
===================================================
+ Hits 4253 4264 +11
+ Misses 2129 2118 -11
Partials 283 283 |
This should be good to merge now. The bug was a non-deterministic map iteration bug in the fuzzer, not the SDK itself. It has been fixed. Additionally the app determinism test now tests gaia itself, not just the randomized testing framework. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, thanks
For Admin Use: