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.

Sign up for GitHub

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

Jump to bottom

FIP-0076: Edit migration details to make it feasible to run #950

Merged
merged 4 commits into from
Mar 5, 2024
Merged

FIP-0076: Edit migration details to make it feasible to run #950

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b23e4bd
FIP-0076: Edit migration details to make it feasible to run
arajasek Feb 22, 2024
61eb027
address review
arajasek Feb 22, 2024
34e483f
Update FIPS/fip-0076.md
jennijuju Mar 5, 2024
362f732
Update FIPS/fip-0076.md
jennijuju Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions FIPS/fip-0076.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -579,17 +579,14 @@ The built-in market actor's `ProviderSectors` mapping is initialised from the ex
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`:
jennijuju marked this conversation as resolved.
Show resolved Hide resolved
- 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];
- find the corresponding deal proposal object;
- if the deal has expired, skip processing this deal
Copy link
Member

Choose a reason for hiding this comment

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

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.

- 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:
jennijuju marked this conversation as resolved.
Show resolved Hide resolved
- set the deal state object's sector number to `0`.
Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

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., the SectorNumber field is invalid unless SectorStartEpoch >= 0.

Copy link
Member

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


[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:
Copy link
Member

Choose a reason for hiding this comment

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

specifically: 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.
Copy link
Member

Choose a reason for hiding this comment

The 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).

  • 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).


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
Expand Down
Loading