fix(market): clean up provider_sectors when empty

[rvagg]

We have this invariant complaining with nv22, no deal ids in sector X for the new ProviderSectors mapping: https://github.com/filecoin-project/go-state-types/blob/ad909a3d6ebef5ebc5688b4310e954e27da2203b/builtin/v13/market/invariants.go#L294

I don't think it's the migration that's the cause of this since it's checking the len==0 case: https://github.com/filecoin-project/go-state-types/blob/ad909a3d6ebef5ebc5688b4310e954e27da2203b/builtin/v13/migration/miner.go#L76-L78

We have 2 ways that we remove items from provider_sectors in actors, pop_sector_deal_ids and remove_sector_deal_ids. The former does cleanup when empty. But the latter just removes and saves. This PR proposes removing in both cases when we get to an empty DealID array.

Before I write any tests for this I want to check if this is correct. Also, if this is correct, should we propose a small cleanup migration for nv23 to get rid of the empty ones?

[Stebalien]

Before I write any tests for this I want to check if this is correct.

It looks correct, yes.

Also, if this is correct, should we propose a small cleanup migration for nv23 to get rid of the empty ones?

IIRc, we need to do another cleanup of dead deals. I'd do that all at the same time.

[anorth]

Thanks for fixing this

[rvagg]

Also, if this is correct, should we propose a small cleanup migration for nv23 to get rid of the empty ones?

IIRc, we need to do another cleanup of dead deals. I'd do that all at the same time.

This would become a FIP thing, wouldn't it? Should I open a discussion about it? And why do we have dead deals to clean up? Was there some old behaviour we're still living with?

filecoin-project/FIPs#956 exists, do we just add it to the list in there?

[Stebalien]

This would become a FIP thing, wouldn't it? Should I open a discussion about it? And why do we have dead deals to clean up? Was there some old behaviour we're still living with?

I'm talking about the change made in filecoin-project/FIPs#950.

But... that's a more complex migration than simply removing empty records. Honestly, I'd do nothing for now and add it to the todo list for the next time we touch this state.

[rvagg]

OK, this is cleaned up now and has test coverage. I didn't need to add any more tests, just make it much more strict about what it expected to be in ProvidersSectors in the various cases.

With the new test changes but with the old logic in place, these tests would fail as they hit the case where a removal ends up with a zero-length list:

• deal_is_correctly_processed_if_first_cron_after_expiry
• expired_deal_should_unlock_the_remaining_client_and_provider_locked_balance_after_payment_and_deal_should_be_deleted
• payment_for_a_deal_if_deal_is_already_expired_before_a_cron_tick
• regular_payments_till_deal_expires_and_then_locked_funds_are_unlocked