fix: miner: move partitions with expired/terminated sectors #1455
Conversation
|
I think we want to know for sure that in a |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1455 +/- ##
==========================================
- Coverage 91.13% 91.08% -0.05%
==========================================
Files 146 146
Lines 27951 27951
==========================================
- Hits 25473 25460 -13
- Misses 2478 2491 +13
|
It seems not, since there's a comment for So it seems to me that The implicit relation seems to be that |
actors/miner/src/deadline_state.rs
Outdated
|
|
||
| // start updating orig/dest `Deadline` here | ||
|
|
||
| orig_deadline.total_sectors -= sector_count; | ||
| orig_deadline.live_sectors -= sector_count; | ||
| orig_deadline.live_sectors -= live_sector_count; | ||
|
|
||
| dest_deadline.total_sectors += sector_count; | ||
| dest_deadline.live_sectors += sector_count; |
There was a problem hiding this comment.
This would need to be `+= live_sector_count
arajasek
force-pushed
the
asr/fix-move-partitions
branch
from
c5520fc
to
27d8d70
Compare
|
I added a testcase for this bug here: #1448 It's currently failing with the error: After merging this fix, it should pass. |
(removed the tests due to conflicts)
…1458) (removed the tests due to conflicts) Co-authored-by: Aayush Rajasekaran <[email protected]>
I think the comment
sector_count is both total sector count and total live sector count, since no sector is faulty here.was wrong. You can have "dead" sectors (terminated or expired) ones. @TippyFlitsUK hit this case when testing moving partitions.The question is what's the right thing to do. I think it's fine to just move partitions with terminated / expired sectors, and the text of the FIP doesn't disallow this. If so, I think this PR fixes this, and we can land it with a test.
Otherwise, we need to explicitly disallow moving partitions with expired sectors, which the code doesn't currently do.