feat(store): correct corrupted files on write #3859
Conversation
walldiss
requested review from
renaynay,
Wondertan,
vgonkivs and
cristaloleg
as code owners
walldiss
force-pushed
the
correct-errors-on-write
branch
from
225d396
to
7889e03
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3859 +/- ##
==========================================
+ Coverage 44.83% 45.07% +0.23%
==========================================
Files 265 314 +49
Lines 14620 21895 +7275
==========================================
+ Hits 6555 9869 +3314
- Misses 7313 10991 +3678
- Partials 752 1035 +283 ☔ View full report in Codecov by Sentry. |
store/store.go
Outdated
| // Validate the size of the file to ensure it's not corrupted | ||
| err = file.CheckODSQ4Size(pathODS, pathQ4, square) | ||
| if err != nil { | ||
| err = s.removeODSQ4(height, roots.Hash()) | ||
| if err != nil { | ||
| return false, fmt.Errorf("removing corrupted ODSQ4 file: %w", err) | ||
| } | ||
| err = file.CreateODSQ4(pathODS, pathQ4, roots, square) | ||
| if err != nil { | ||
| return false, fmt.Errorf("recreating ODSQ4 file: %w", err) | ||
| } |
There was a problem hiding this comment.
Readbility nit: extract into a validateAnRecoverX method
There was a problem hiding this comment.
I think it is much easier to understand what is going on here by not hiding details into smaller func. Function calls are self-explanatory here and validateAnRecoverX is not going to be reused anywhere
There was a problem hiding this comment.
The recovery details have layers of nesting involved with edcase logic unrelated to createX method. It would be more readable for a novice to read what create does only and jump on recovery logic only if needed.
There was a problem hiding this comment.
Essentially recovery logic is a separate operation thus needs a separate method
store/file/ods.go
Outdated
| } | ||
|
|
||
| shares := filledSharesAmount(eds) | ||
| expectedSize := ods.hdr.OffsetWithRoots() + shares*ods.hdr.ShareSize() |
There was a problem hiding this comment.
Shouldnt we verify header as well instead of trusting it? Whats if header also corrupted?
There was a problem hiding this comment.
It will fail to read the header in check and return the error (same results as corrupted)
There was a problem hiding this comment.
Corrupted doesn't necessarily mean malformed. It can be read, but it has a wrong value.
There was a problem hiding this comment.
If the header has corrupted values it is very unlikely it will match exactly expected file size, and will fail the check. Keep in mind this is only Size check, not a full content inspection. In case of full content integrity check it would be necessary to verify headers values as well as content of roots and data.
There was a problem hiding this comment.
If the header has corrupted values it is very unlikely it will match exactly expected file size
This is what concerns me, we don't know if it will match or not. I don't think trusting header is valid when we dont trust the file. We could simply get the file size from the eds, arent we?
There was a problem hiding this comment.
Share size is tricker than length, but I think I have a workaround. We can use the size of first share. So we don't have to trust header on disk at all. Will update the code
This PRs adds corruption detection to store. Corrupted files are detected by checking if their length is corresponding to expected file size. There is no content inspection for files verification yet, but it can be added via datahash compute. Added support for:
TODO: store level tests are blocked by #3847