fix: verifyEncoding should revert changes in verifiable data #318
Conversation
| data[rebuiltIndex] = rebuiltShare | ||
| if rebuiltShare != nil && rebuiltIndex >= 0 { | ||
| data[rebuiltIndex] = rebuiltShare | ||
| defer func() { |
There was a problem hiding this comment.
Style: do we want to use defer here? AFAIK, no where else in rsmt2d uses defer, primarily because it's a Go-specific thing that doesn't translate well into a language like C (C being a lower common denominator for readability). I don't have strong opinions, so opening for discussion.
There was a problem hiding this comment.
I'm in favor of using defer because it ensures cleanup operations are closely tied to their corresponding setups. This not only makes the code more robust by preventing omission in multiple return paths but also aligns with good programming practices by keeping related actions together, improving both maintainability and readability.
An alternative would be to manually add a revert of the addition to the data slice in every return statement, which could be error-prone.
Another option is to adopt an immutable approach by creating a copy of the data that includes the extra share, ensuring the original data remains unmodified. However, this method would lead to extra allocations, which is why I haven't used it here.
There was a problem hiding this comment.
I see.
Hmmmm. Perhaps creating a copy wouldn't be so bad if it's only for a single row or column. Could you open an issue that someone can work on to investigate the actual performance impact?
Other that than, no further comments. Not blocking merging.
There was a problem hiding this comment.
Defer translates well into C or any other language by calling the deferred line after everything else, so we don't gain much by going with the alternative approach and only lose time (investigating performance impact through the issue) and readability(no defer based grouping). Overall, I don't think opening and spending time on the issue is worth it
There was a problem hiding this comment.
Ok. I'll defer (get it 😉) to you guys on this.
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.