feat: FIP-0076: add new invariants #245
Conversation
builtin/v13/check.go
Outdated
| continue | ||
| } | ||
|
|
||
| sector := minerSummary.SectorsWithDeals[sectorID.Number] |
There was a problem hiding this comment.
This is going to break soon after the upgrade as new sectors are added (which won't record deal IDs). This also won't catch deals we failed to migrate.
IMO, we'd be better off if we:
- Iterated over the
ProviderSectorsto create a reverse deal -> sector ID index, checking for duplicates as we do that. - Then, iterate over all miners, then all
SectorsWithDealsfor each miner, making sure that the deal got migrated into theProviderSectorstable.
There was a problem hiding this comment.
I guess there's a TODO below, but we might as well make this "correct" to start with (unless I'm missing something). I.e., the invariant is: every deal in a sector is also listed in the market. (is that correct? even when they expire?)
If we do want to catch "unknown" deals, we could, e.g., count the found deals and make sure the lengths match. (then we can remove that additional check when we're all done with the migration, or maybe have a way to print a warning?).
There was a problem hiding this comment.
@Stebalien Unfortunately, I don't think your proposed invariant works -- deals might have left the market (both its Proposals and ProviderSectors fields), while still being in the miner sector data. So really all I can think to do here is:
- ensure no duplicates
- ensure that for the immediately-after-migration state anything in
ProviderSectorsis in the corresponding miner's sector data
After that, I think this entire invariant kinda goes out the window due to the decoupling. There will be deals in sector data that aren't in either the market's proposals or ProviderSectors fields. And there will be deals in the proposals and ProviderSectors fields that are NOT in miner sector data.
There was a problem hiding this comment.
@anorth If you could chime in here, that would be great.
There was a problem hiding this comment.
Regarding making it "correct" to start with, I do somewhat strongly want to keep as much migration-specific invariants here, it'll make testing the migration significantly easier, and that is very important IMO.
There was a problem hiding this comment.
Hm. Yeah, you're right. However, I still think going from miner -> market is better than going from market -> miner because it'll help us identify deals that haven't been migrated.
There was a problem hiding this comment.
But IIUC we don't want every deal found in miner data to be in ProviderSectors, only the specific subset as detailed here. I could implement that very specific check, but at that point the "invariant" is just rewriting the migration, I think.
There was a problem hiding this comment.
Ah... you're right. We don't have a 1:1 mapping anyways.
These are new invariants that cover FIP-0076 changes.
Partly extracted from #236, with additional invariants based on filecoin-project/builtin-actors#1520.