search

filecoin-project / builtin-actors

The Filecoin built-in actors
Other
78 stars 74 forks source link

fix(market): clean up provider_sectors when empty #1539

Closed rvagg closed 1 month ago

rvagg commented 1 month ago

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 commented 1 month ago

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.

rvagg commented 1 month ago

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?

https://github.com/filecoin-project/FIPs/discussions/956 exists, do we just add it to the list in there?

Stebalien commented 1 month ago

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 https://github.com/filecoin-project/FIPs/pull/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 commented 1 month ago

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: