-
Notifications
You must be signed in to change notification settings - Fork 959
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(ipld): fix reconstruction by re-enabling re-importing of rsmt2d.ExtendedDataSquare #804
Conversation
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 would really, really prefer a testcase for this but I trust your hours of debugging this.
…xtendedDataSquare
55adc76
to
4aa8148
Compare
Force push with a cleaner commit |
Codecov Report
@@ Coverage Diff @@
## main #804 +/- ##
==========================================
+ Coverage 53.15% 53.19% +0.03%
==========================================
Files 117 117
Lines 6750 6760 +10
==========================================
+ Hits 3588 3596 +8
- Misses 2798 2801 +3
+ Partials 364 363 -1
Continue to review full report at Codecov.
|
@liamsi, I added a test anyway. You can try removing the fix commit and reproduce it yourself. Sometimes it fails with other rsmt2d errors besides byzantine. The test ensures that at least 2 more quadrants are fetching besides the initial one, and all of those written into the shared square, and somewhere in between, the reconstruction fails. |
As always on CI something goes not like locally, aaaaa |
bd5daa5
to
10a215a
Compare
Force push with an updated test that should not fail on CI, lets see... |
10a215a
to
0dc7d7a
Compare
Ok, the test is fixed on CI |
An update of rsmt2d happened in #737 during Mamaki launch preparation. This update changed our code not to reimport the square on each reconstruction attempt which silently sneaked in the problem, which is not reproducible with current tests. Reconstruction tests(#702) reproduce it, but they are not yet merged.
I was able to change the code to reproduce it reliably. Mainly I had to fetch only from columns and force the code to download only [2:4) quadrants. Unfortunately, it is hard to extract the reproducible case in a separate test, so we can either rely on reconstruction tests that are soon to be merged, or I can still try to make a test.
The reason for the bug is explained in #787, as well as a cleaner way to fix it.
Closes #787
P.S. Was pretty hard to debug this case. I went on the wrong paths like 3 times. Though collected some minor findings that I am going to submit further.