Market state cleanup post-nv22

[rvagg]

Also see other state cleanup ideas in #956

Two state cleanup items need to be recorded as follow-up from network version 22, specifically from implementation of FIP-0076 and the new ProviderSectors data structure within the market actor.

1. ProviderSectors contains terminated sectors in order to make the migration practical

As per #950, the FIP was adjusted as the Lotus migration for nv22 was being written and it was discovered that being thorough with which sectors to include was going to be too resource intensive to make the migration efficient enough. A compromise was accepted into the FIP's migration procedure to allow the inclusion of terminated sectors.

As @anorth points out in that PR, ProvidersSectors exists for the market actor, so it knows which deals to terminate when a sector does. Having it contain already-terminated sectors means that the HAMT contains an unnecessary amount of unused information.

2. ProviderSectors contains some references to actors who have no active deals, and sectors that have no active deals

Due to an overlooked case in 0076 implementation, on deal termination (though not sector termination), the inner HAMT has been accumulating empty lists of deals for sectors that previously had deals in them rather than removing references to those sectors completely.

---

With both of these cases, the inner HAMT will be larger than it needs to be, potentially adding slightly to the gas costs paid for actors (that existed prior to nv22 for the first case, and everyone for the second) when mutating the data in market actor calls. The outer HAMT is also likely to be unnecessarily large as it may (eventually?) contain references to actors that no longer have any active sectors (first case) or active deals (second case) but still retain the list of pre-nv22 terminated sectors. This likely increases gas costs for all callers. It's unclear just how large the increased gas costs are in both of these cases, it may be small, and it will depend on the layer depth of both HAMTs.

A migration of the first case is complicated, for the same reasons it was skipped in the original nv22 migration. For each sector in ProviderSectors, the miner's Terminations bitfield needs to be checked and where the sector has been terminated it should be removed from ProviderSectors, and possibly the actor itself if it has no remaining sectors. Without the rest of the complicated nv22 migration, the cost of this is likely acceptable for a migration, but it needs to be tested.

A migration for the second case is simpler as it requires no cross-referencing, simply removing references to sectors that have an empty deals list, and afterward, removing actors that have no remaining sectors.

[anorth]

Thanks for the thorough write-up. This all sounds correct to me. Will the outer HAMT eventually be cleaned up when the sectors terminate, even if it failed to do so when the deals ended? If so, maybe it's not worth too much trouble to clean this one up (though it is the easy case).

I agree it's hard to estimate the cost savings. I expect they are large enough to become relevant if/when competition breaks out between different deal-making mechanisms in the future. But I am not rooting too hard for the built-in market if better alternatives emerge.

[rvagg]

Yes, I think it might be cleaned up on terminations. It hits the following logic in pop_sector_deal_ids:

        for sector_number in sector_numbers {
            let deals: Option<Vec<DealID>> = sector_deals
                .delete(&sector_number)
                .with_context(|| format!("provider {}", provider))?;
            if let Some(deals) = deals {
                popped_sector_deals.extend(deals.iter());
                flush = true;
            }
        }

The HAMT is going to have an empty Vec<DealID> in it which I hope would be returned by delete so we should have a Some but nothing gets added to popped_sector_deals. Then we end up at the is_empty clean-up logic because flush is true. Does that sound right?