-
Notifications
You must be signed in to change notification settings - Fork 81
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
test: add randomized tests for errByzantine #312
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #312 +/- ##
==========================================
+ Coverage 80.89% 84.03% +3.14%
==========================================
Files 8 7 -1
Lines 869 614 -255
==========================================
- Hits 703 516 -187
+ Misses 119 59 -60
+ Partials 47 39 -8 ☔ View full report in Codecov by Sentry. |
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.
LGTM
@@ -2,6 +2,7 @@ package rsmt2d | |||
|
|||
import ( | |||
"bytes" | |||
crand "crypto/rand" | |||
"errors" | |||
"fmt" | |||
"math/rand" |
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.
[no change needed] in a follow up PR we may consider replacing usage of math/rand
with crypto/rand
and then deleting the import alias for crypto/rand
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.
Unfortunately, crypto/rand
lacks handy IntN funciton, only
func Int(rand io.Reader, max *big.Int) (n *big.Int, err error)
While math/rand
lacks ability to generate random []byte slice. I wish one day there will be single package with both
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.
There is rand v2
func TestErrRandByzantine(t *testing.T) { | ||
codec := NewLeoRSCodec() | ||
original, corrupted, idx := randCorruptedEDS(t, codec, 8) | ||
require.False(t, original.Equals(corrupted), "corrupted eds is equal to original eds") |
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.
[question] why .False
instead of:
require.NotEqual(t, original, corrupted)
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.
NotEqual uses ==
slice compare under the hood, while Equals
method does more precise comparison.
…ed from orthogonal (#313) If axis is fully recomputed from orthogonal axises, we need to verify correct encoding on top of verification of roots. Test coverage should happen by celestiaorg/rsmt2d#312
ErrByzantine encompasses numerous edge cases. To address this, I implemented randomized fuzzing tests, which identified instances where shares encoding was not properly verified.
While I am neutral about maintaining the pretty printing code (as it looks kinda off), I retained it because it proved extremely useful for understanding the reasons behind test failures during extensive debugging. Should these tests fail in the future for any reason, having a visual representation of the issues will expedite debugging for future developers.
Allowed to discover #313