DDO types, nv22 edition

[magik6k]

For filecoin-project/lotus#11226
Resurrects #209 for nv22
On top of #234

[arajasek]

Some notes.

[arajasek]

I agree with all of this, but why not just deal with this case too? You know what sectorBefore and sectorAfter's deals are, so you can:

• set the mapping for sectorAfters deals easily
• look up the entries for sectorBefore's deals
  • if they map to sectorNo, delete them -- if they need to get updated to something else, that will happen later
  • if they map to something other than sectorNo leave them alone, they've presumably already been cleaned up

[arajasek]

or alternatively, add sectorBefores deals to removedDealToSector?

[magik6k]

Do we even use the cache through reorgs? If not this really can't happen, and if we do use the cache through reorgs I imagine the market migrator makes a lot of invalid assumptions too.

(also this reminds me that removedDealToSector is actually no longer used)

[arajasek]

Yes, we use the cache through reorgs, and should be fully reorg-resistant. There is no assumption that the state it ran on is an ancestor of the current state tree. Instead, the assumption is just that if we cached that X -> Y, we can use that information to calculate X' -> Y', where X and X' probably have some commonalities.

[arajasek]

What are some of the invalid assumptions you're concerned about?

We could tweak how Lotus uses the cache to wipe it if we know its base state was re-orged out, but I'd rather not do that.

[arajasek]

I don't think so, but I'm not sure.

[anorth]

Isn't this the case annotated "// FIP: if such a sector cannot be found, assert that the deal's end epoch has passed and use sector ID 0" in the FromScratch case (in which case yes)?

Adding the FIP spec as comments in this version too would be helpful. Though this code isn't setting the deal state sector number.

[magik6k]

This function is called inside of a oldState.SlashEpoch != -1 so I believe that this isn't this case, there the sector number will correctly be set to zero

		case amt.Add: // both in case of chain going forward and chain reorg this is just new deals
			var oldState market12.DealState
			if err := oldState.UnmarshalCBOR(bytes.NewReader(change.After.Raw)); err != nil {
				return cid.Cid{}, cid.Cid{}, xerrors.Errorf("failed to unmarshal old state: %w", err)
			}

			var newState market13.DealState
			newState.SlashEpoch = oldState.SlashEpoch
			newState.LastUpdatedEpoch = oldState.LastUpdatedEpoch
			newState.SectorStartEpoch = oldState.SectorStartEpoch
			newState.SectorNumber = 0 // terminated / not found (?)

			if oldState.SlashEpoch == -1 {
				si, err := addProviderSectorEntry(deal)

[anorth]

The case annotated "// todo is this normal and possible??" seems to be returning an error where it shouldn't.

The FromScratch version looks good but is lacking an assertion that the FIP specifies.

The complexity of the Diff version makes it much harder to verify. I didn't see anything wrong from a FIP specification point of view, but I don't understand the diff mechanics in depth.

[anorth]

This isn't making the assertion that the FIP specifies. If there's a good reason for that, can it be written in the comment? Should the FIP be changed?

[magik6k]

I believe the assert is caught by this invariant

acc.Require((deal.SectorNumber == sectorDeal.SectorNumber) || (deal.SectorNumber == 0 && deal.SlashEpoch != -1),
			"deal sector number %d does not match sector %d for miner %v (ds: %#v; ss %#v)",

Will add a comment here

[anorth]

Isn't this the case annotated "// FIP: if such a sector cannot be found, assert that the deal's end epoch has passed and use sector ID 0" in the FromScratch case (in which case yes)?

Adding the FIP spec as comments in this version too would be helpful. Though this code isn't setting the deal state sector number.

[arajasek]

Closing in favour of #246