fix: verify encoding of shares in case when axis is fully reconstructed from orthogonal #313
Conversation
| vectorData := eds.row(uint(r)) | ||
| for c := 0; c < int(eds.width); c++ { | ||
| shares[c] = vectorData[c] | ||
| } | ||
| copy(shares, eds.row(uint(r))) |
There was a problem hiding this comment.
unrelated, just small simplification change to use builtin func instead of loop
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #313 +/- ##
==========================================
+ Coverage 80.89% 81.15% +0.25%
==========================================
Files 8 7 -1
Lines 869 626 -243
==========================================
- Hits 703 508 -195
+ Misses 119 72 -47
+ Partials 47 46 -1 ☔ View full report in Codecov by Sentry. |
|
Do you know why codecov failed? 🤔 |
|
This seems like an important case we missed before and needs rsmt2d OGs to review it |
|
I think this makes sense, nice catch. @adlerjohn should also have a look. |
codecov/patch is failing because not enough lines are covered. IMO we can ignore that CI check if this fix is verified via the tests in #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
| @@ -177,6 +174,10 @@ func (eds *ExtendedDataSquare) solveCrosswordRow( | |||
| } | |||
| return false, false, err | |||
| } | |||
|
|
|||
| if eds.verifyEncoding(col, r, rebuiltShares[c]) != nil { | |||
There was a problem hiding this comment.
Something worth consideration is the the precedence of these returned errors i.e., 1) root mismatch (due to the call to verifyAgainstColRoots) and 2) the encoding error (caused by verifyEncoding). I think if the encoding is already wrong, perhaps it does not matter whether the roots match or not, so returning the encoding verification error would be more accurate and informative. Depending on this decision, we may want to swap the order of the returned errors. This can be tackled in a separate issue though if further discussion is needed.
The VerifyEncoding function introduced in PR #313 might not revert changes to the original row or column after successful verification. This could lead to a scenario where one of the fields, either squareRow or squareCol, retains a rebuilt share, resulting in only partial repair. This oversight would leave some repaired data unavailable when it should be accessible. Existing tests did not cover this scenario. This PR addresses the issue and includes randomized repair tests to ensure the fix is effective.
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 celestiaorg/rsmt2d#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 #312