Skip to content
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

sealing: Check piece CIDs after AddPiece #7185

Merged
merged 1 commit into from
Aug 27, 2021
Merged

sealing: Check piece CIDs after AddPiece #7185

merged 1 commit into from
Aug 27, 2021

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Aug 26, 2021

Non-worker-api-breaking alternative to #7175

The difference here is that after adding a bad piece ta a sector, it will be market as 'allocated' in the sector file, but that's actually fine as we allow overwriting allocated parts of sectors - the only thing we need to do to recover from this is to not add the bad piece to the sector.Pieces array, and the subsequent AddPiece calls will overwrite the bad data with good data

@magik6k magik6k requested a review from a team as a code owner August 26, 2021 14:28
@codecov
Copy link

codecov bot commented Aug 26, 2021

Codecov Report

Merging #7185 (b42171d) into master (c664f9e) will increase coverage by 5.22%.
The diff coverage is 8.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7185      +/-   ##
==========================================
+ Coverage   33.80%   39.02%   +5.22%     
==========================================
  Files         601      607       +6     
  Lines       64277    64577     +300     
==========================================
+ Hits        21728    25204    +3476     
+ Misses      38464    34985    -3479     
- Partials     4085     4388     +303     
Impacted Files Coverage Δ
extern/storage-sealing/input.go 56.45% <0.00%> (-1.94%) ⬇️
extern/storage-sealing/states_sealing.go 37.87% <33.33%> (-0.03%) ⬇️
storage/addresses.go 55.95% <0.00%> (-4.77%) ⬇️
storage/wdpost_sched.go 77.22% <0.00%> (-3.97%) ⬇️
extern/sector-storage/sched.go 83.05% <0.00%> (-1.66%) ⬇️
node/impl/storminer.go 22.46% <0.00%> (-0.40%) ⬇️
extern/sector-storage/stores/local.go 59.25% <0.00%> (ø)
chain/actors/builtin/paych/mock/mock.go 64.28% <0.00%> (ø)
markets/dagstore/mocks/mock_lotus_accessor.go 80.48% <0.00%> (ø)
chain/vectors/gen/main.go 2.36% <0.00%> (ø)
... and 134 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c664f9e...b42171d. Read the comment docs.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both solution seem to work fine, although the first one is arguably safer.

@Stebalien
Copy link
Member

Coverage should hopefully be fixed by #7189.
Broken integration test is disabled in #7187.

@Stebalien
Copy link
Member

Ok, integration tests disabled entirely.

@magik6k
Copy link
Contributor Author

magik6k commented Aug 27, 2021

although the first one is arguably safer

It's not really safer, we only really use the info about which bits of the unsealed sector file are allocated to check if we need to Unseal the sector before reading unsealed data.
Since here we wrote bad data into the sector, we won't be unsealing that piece anyways, and it will just be overwritten with real data. The fact that this miswritten piece is marked as allocated doesn't really impact anything at the sealing stage

@magik6k magik6k merged commit 525c584 into master Aug 27, 2021
@magik6k magik6k deleted the feat/ap-check-cid branch August 27, 2021 09:14
@magik6k magik6k mentioned this pull request Aug 27, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants