-
Notifications
You must be signed in to change notification settings - Fork 164
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
FIP-0076: Edit migration details to make it feasible to run #950
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -578,18 +578,15 @@ struct GetDealSectorReturn { | |
The built-in market actor's `ProviderSectors` mapping is initialised from the existing deal state | ||
and miner actor state per-sector deal IDs. | ||
|
||
- For each deal state object in the market actor state that has a terminated epoch set to `-1`: | ||
- find the corresponding deal proposal object and extract the provider's actor ID; | ||
- in the provider's miner state, find the ID of the sector with the corresponding deal ID in sector metadata; | ||
- if such a sector cannot be found, assert that the deal's end epoch has passed and use sector ID `0` [1]; | ||
- For each deal state object in the market actor state that has a slashed epoch set to `-1`: | ||
- find the corresponding deal proposal object; | ||
- if the deal has expired, set the sector number to `0`. | ||
- extract the provider's actor ID, look up the provider's miner state, find the ID of the sector with the corresponding deal ID in sector metadata; | ||
- set the new deal state object's sector number to the sector ID found; | ||
- add the deal ID to the `ProviderSectors` mapping for the provider's actor ID and sector number. | ||
- For each deal state object in the market actor state that has a terminated epoch set to any other value: | ||
- For each deal state object in the market actor state that has a slashed epoch set to any other value: | ||
- set the deal state object's sector number to `0`. | ||
|
||
[1] It may be impossible to find the sector for a deal that has completed successfully | ||
but not yet been cleaned up in market actor state, | ||
if the corresponding sector has since expired and been compacted out of state. | ||
- For each miner, for each unexpired sector (even if the sector has been terminated): | ||
- add the deal IDs in the sector to the `ProviderSectors` mapping for the provider's actor ID and sector number. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ProviderSectors map exists to support sector termination: it lists the deals to terminate when a sector does. The proposed change will add more entries to this than necessary (including already-expired deals).
These extra entries will impose a very minor gas cost on the SPs which might need to traverse a larger HAMT when activating deals in the future. I would hope that we can clean it up in a future migration. Entries corresponding to sectors that don't exist can be deleted (and exists -> doesn't-exist is monotonic, so easy to cache computation). |
||
|
||
The result includes a value in the `ProviderSectors` mapping for each activated and not yet terminated or expired deal. | ||
The built-in market actor's implementation of deal expiration clean-up must be robust to the provider sector mapping | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shoudnt this be null or -1 instead of 0? given 0 is a legit sector number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it can't be either, because sector numbers are unsigned integers (can't be null or -1). The original FIP specifies zero as the number to use, there's no change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm doesn’t this give incorrect info tho? I feel like we should still put the sector number it was originally in, and let cron clear it out later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to the authors of the original FIP here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it doesn't matter because the
SectorStartEpoch
will be -1. I.e., theSectorNumber
field is invalid unlessSectorStartEpoch >= 0
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think i mostly got confused by what was terminated epoch was referring to (which is not introduced in this PR but suggested edit to simplify it