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 all commits
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
17 changes: 7 additions & 10 deletions FIPS/fip-0076.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
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 (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