FIP-0076: Edit migration details to make it feasible to run #950
Conversation
arajasek
requested review from
momack2,
jennijuju,
kaitlin-beegle,
anorth,
raulk,
jsoares and
TippyFlitsUK
as code owners
FIPS/fip-0076.md
Outdated
| - 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]; | ||
| - find the corresponding deal proposal object; | ||
| - if the deal has expired, skip processing this deal |
There was a problem hiding this comment.
The new deal state's sector number must be set to something. The schema has changed, so every one must be migrated. I think setting it to zero is ok, but specify that explicitly.
Why is zero ok?
- The prev spec asserted that if the sector cannot be found, the deal must have expired. So the set of expired deals is a superset of those with no sector found.
- The prev spec specified sector number zero for sector-not-found. The cron clean-up and deal settlement code is robust to a sector number that can't be later found in the ProviderSectors mapping.
- What about deals that have expired but their sector not yet terminated? The sector termination handler doesn't inspect this value, it just deletes the deal state object.
The observable effect of this change is that the exported GetDealSector method will report sector number zero for some deals that could otherwise have reported the correct sector number. But they are expired and will be cleaned up within 30 days, so this seems of minimal impact.
| 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: | ||
| - 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.
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).
- When a sector is forcefully terminated, this extra state will be cleaned up. The termination code ignores entries for which no deal proposal can be found.
- When a sector expires normally, the mappings for deals that had already expired will remain in state.
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).
FIPS/fip-0076.md
Outdated
| [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: |
There was a problem hiding this comment.
specifically: even if the sector has been terminated.
| - 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: | ||
| - set the deal state object's sector number to `0`. |
There was a problem hiding this comment.
shoudnt this be null or -1 instead of 0? given 0 is a legit sector number
There was a problem hiding this comment.
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.
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.
I'll defer to the authors of the original FIP here.
There was a problem hiding this comment.
In this case, it doesn't matter because the SectorStartEpoch will be -1. I.e., the SectorNumber field is invalid unless SectorStartEpoch >= 0.
There was a problem hiding this comment.
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
The Lotus team cannot implement the migration, as currently specified, cannot be safely implemented in what we currently consider satisfactory time (<=2 epochs). This change, which primarily decouples some of the conditions between the market state and miner(s) state, simplifies the migration to the point where it can be run faster.
The impact is to have more state than strictly necessary in the new
ProviderSectorsfield of the market actor.NOTE: The Core Devs should discuss what acceptable network downtime is. We currently require state migrations to run in ~1 epoch, which avoids null rounds, and is overall healthy for the network, but significantly slows down protocol development & adds risk to these operations. It is possible that we decide that we actually don't have a problem with 5-10 minutes of network downtime in order to accelerate the pace of development and de-risk state migrations by cutting out tricky optimizations.