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

* fix(market): clean up provider_sectors when empty

* fix(market): extend tests to ensure no empty sector/deal map

actors/market/src/state.rs

@@ -626,16 +626,7 @@ impl State {
 
         // Flush if any of the requested sectors were found.
         if flush {
-            if sector_deals.is_empty() {
-                // Remove from top-level map
-                provider_sectors
-                    .delete(&provider)
-                    .with_context_code(ExitCode::USR_ILLEGAL_STATE, || {
-                        format!("failed to delete sector deals for {}", provider)
-                    })?;
-            } else {
-                save_provider_sector_deals(&mut provider_sectors, provider, &mut sector_deals)?;
-            }
+            save_provider_sector_deals(&mut provider_sectors, provider, &mut sector_deals)?;
             self.save_provider_sectors(&mut provider_sectors)?;
         }
 
@@ -651,6 +642,7 @@ impl State {
     ) -> Result<(), ActorError> {
         let mut provider_sectors = self.load_provider_sectors(store)?;
         for (provider, sector_deal_ids) in provider_sector_deal_ids {
+            let mut flush = false;
             let mut sector_deals = load_provider_sector_deals(store, &provider_sectors, *provider)?;
             for (sector_number, deals_to_remove) in sector_deal_ids {
                 let existing_deal_ids = sector_deals
@@ -662,20 +654,39 @@ impl State {
                     // pretty fast.
                     // Loading into a HashSet could be an improvement for large collections of deals
                     // in a single sector being removed at one time.
-                    let new_deals = existing_deal_ids
+                    let new_deals: Vec<_> = existing_deal_ids
                         .iter()
                         .filter(|deal_id| !deals_to_remove.contains(*deal_id))
                         .cloned()
                         .collect();
-
-                    sector_deals
-                        .set(sector_number, new_deals)
-                        .with_context_code(ExitCode::USR_ILLEGAL_STATE, || {
-                            format!("failed to set sector deals for {} {}", provider, sector_number)
-                        })?;
+                    flush = true;
+
+                    if new_deals.is_empty() {
+                        sector_deals.delete(sector_number).with_context_code(
+                            ExitCode::USR_ILLEGAL_STATE,
+                            || {
+                                format!(
+                                    "failed to delete sector deals for {} {}",
+                                    provider, sector_number
+                                )
+                            },
+                        )?;
+                    } else {
+                        sector_deals.set(sector_number, new_deals).with_context_code(
+                            ExitCode::USR_ILLEGAL_STATE,
+                            || {
+                                format!(
+                                    "failed to set sector deals for {} {}",
+                                    provider, sector_number
+                                )
+                            },
+                        )?;
+                    }
                 }
             }
-            save_provider_sector_deals(&mut provider_sectors, *provider, &mut sector_deals)?;
+            if flush {
+                save_provider_sector_deals(&mut provider_sectors, *provider, &mut sector_deals)?;
+            }
         }
         self.save_provider_sectors(&mut provider_sectors)?;
         Ok(())
@@ -1273,7 +1284,15 @@ fn save_provider_sector_deals<BS>(
 where
     BS: Blockstore,
 {
-    let sectors_root = sector_deals.flush()?;
-    provider_sectors.set(&provider, sectors_root)?;
+    if sector_deals.is_empty() {
+        provider_sectors
+            .delete(&provider)
+            .with_context_code(ExitCode::USR_ILLEGAL_STATE, || {
+                format!("failed to delete sector deals for {}", provider)
+            })?;
+    } else {
+        let sectors_root = sector_deals.flush()?;
+        provider_sectors.set(&provider, sectors_root)?;
+    }
     Ok(())
 }

actors/market/tests/activate_deal_failures.rs

@@ -107,7 +107,7 @@ fn reject_proposal_expired() {
         ExitCode::OK,
     );
     cron_tick(&rt);
-    assert_deal_deleted(&rt, deal_id, &deal, 0);
+    assert_deal_deleted(&rt, deal_id, &deal, 0, true);
 
     assert_activation_failure(&rt, deal_id, &deal, 1, sector_expiry, EX_DEAL_EXPIRED);
 }

actors/market/tests/batch_activate_deals.rs

@@ -46,7 +46,7 @@ fn activate_deals_one_sector() {
     assert!(res.activation_results.all_ok());
 
     // Deal IDs are stored under the sector, in correct order.
-    assert_eq!(deal_ids, get_sector_deal_ids(&rt, PROVIDER_ID, &[1]));
+    assert_eq!(deal_ids, get_sector_deal_ids(&rt, PROVIDER_ID, &[1]).unwrap());
 
     for id in deal_ids.iter() {
         let state = get_deal_state(&rt, *id);
@@ -392,7 +392,7 @@ fn activate_new_deals_in_existing_sector() {
     assert_eq!(0, get_deal_state(&rt, deal_ids[2]).sector_start_epoch);
 
     // All deals stored under the sector, in order.
-    assert_eq!(deal_ids, get_sector_deal_ids(&rt, PROVIDER_ID, &[sector_number]));
+    assert_eq!(deal_ids, get_sector_deal_ids(&rt, PROVIDER_ID, &[sector_number]).unwrap());
     check_state(&rt);
 }
 

actors/market/tests/cron_tick_deal_expiry.rs

@@ -49,7 +49,7 @@ fn deal_is_correctly_processed_if_first_cron_after_expiry() {
     assert!(slashed.is_zero());
 
     // deal should be deleted as it should have expired
-    assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number);
+    assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number, true);
     check_state(&rt);
 }
 
@@ -124,7 +124,7 @@ fn regular_payments_till_deal_expires_and_then_locked_funds_are_unlocked() {
     assert!(slashed.is_zero());
 
     // deal should be deleted as it should have expired
-    assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number);
+    assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number, true);
     check_state(&rt);
 }
 
@@ -154,7 +154,7 @@ fn payment_for_a_deal_if_deal_is_already_expired_before_a_cron_tick() {
     assert_eq!((end - start) * &deal_proposal.storage_price_per_epoch, pay);
     assert!(slashed.is_zero());
 
-    assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number);
+    assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number, true);
 
     // running cron tick again doesn't do anything
     cron_tick_no_change(&rt, CLIENT_ADDR, PROVIDER_ADDR);
@@ -203,7 +203,7 @@ fn expired_deal_should_unlock_the_remaining_client_and_provider_locked_balance_a
     assert!(provider_acct.locked.is_zero());
 
     // deal should be deleted
-    assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number);
+    assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number, true);
     check_state(&rt);
 }
 

actors/market/tests/cron_tick_deal_slashing.rs

@@ -102,7 +102,7 @@ fn deal_is_slashed() {
         rt.set_epoch(cron_tick_epoch);
         cron_tick(&rt);
 
-        assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number);
+        assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number, true);
 
         check_state(&rt);
     }
@@ -142,7 +142,7 @@ fn deal_is_slashed_at_the_end_epoch_should_not_be_slashed_and_should_be_consider
     assert!(slashed.is_zero());
 
     // deal should be deleted as it should have expired
-    assert_deal_deleted(&rt, deal_id, &deal_proposal, SECTOR_NUMBER);
+    assert_deal_deleted(&rt, deal_id, &deal_proposal, SECTOR_NUMBER, true);
 
     check_state(&rt);
 }
@@ -192,7 +192,7 @@ fn deal_payment_and_slashing_correctly_processed_in_same_crontick() {
     cron_tick(&rt);
 
     // deal should be deleted as it should have expired
-    assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number);
+    assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number, true);
     check_state(&rt);
 }
 
@@ -249,9 +249,9 @@ fn slash_multiple_deals_in_the_same_epoch() {
     rt.set_epoch(epoch + 1);
     cron_tick(&rt);
 
-    assert_deal_deleted(&rt, deal_id1, &deal_proposal1, SECTOR_NUMBER);
-    assert_deal_deleted(&rt, deal_id2, &deal_proposal2, SECTOR_NUMBER);
-    assert_deal_deleted(&rt, deal_id3, &deal_proposal3, SECTOR_NUMBER);
+    assert_deal_deleted(&rt, deal_id1, &deal_proposal1, SECTOR_NUMBER, true);
+    assert_deal_deleted(&rt, deal_id2, &deal_proposal2, SECTOR_NUMBER, true);
+    assert_deal_deleted(&rt, deal_id3, &deal_proposal3, SECTOR_NUMBER, true);
     check_state(&rt);
 }
 
@@ -319,7 +319,7 @@ fn regular_payments_till_deal_is_slashed_and_then_slashing_is_processed() {
     cron_tick(&rt);
 
     // deal should be deleted as it should have expired
-    assert_deal_deleted(&rt, deal_id, &deal_proposal, SECTOR_NUMBER);
+    assert_deal_deleted(&rt, deal_id, &deal_proposal, SECTOR_NUMBER, true);
     check_state(&rt);
 }
 
@@ -372,6 +372,6 @@ fn regular_payments_till_deal_expires_and_then_we_attempt_to_slash_it_but_it_wil
     assert!(slashed.is_zero());
 
     // deal should be deleted as it should have expired
-    assert_deal_deleted(&rt, deal_id, &deal_proposal, SECTOR_NUMBER);
+    assert_deal_deleted(&rt, deal_id, &deal_proposal, SECTOR_NUMBER, true);
     check_state(&rt);
 }

actors/market/tests/cron_tick_timedout_deals.rs

@@ -55,7 +55,7 @@ fn timed_out_deal_is_slashed_and_deleted() {
     assert_eq!(c_escrow, client_acct.balance);
     assert!(client_acct.locked.is_zero());
     assert_account_zero(&rt, PROVIDER_ADDR);
-    assert_deal_deleted(&rt, deal_id, &deal_proposal, 0);
+    assert_deal_deleted(&rt, deal_id, &deal_proposal, 0, true);
     check_state(&rt);
 }
 
@@ -126,7 +126,7 @@ fn publishing_timed_out_deal_again_should_work_after_cron_tick_as_it_should_no_l
         ExitCode::OK,
     );
     cron_tick(&rt);
-    assert_deal_deleted(&rt, deal_id, &deal_proposal, 0);
+    assert_deal_deleted(&rt, deal_id, &deal_proposal, 0, true);
 
     // now publishing should work
     generate_and_publish_deal(&rt, CLIENT_ADDR, &MinerAddresses::default(), START_EPOCH, END_EPOCH);
@@ -192,8 +192,8 @@ fn timed_out_and_verified_deals_are_slashed_deleted() {
     cron_tick_no_change(&rt, CLIENT_ADDR, PROVIDER_ADDR);
 
     assert_account_zero(&rt, PROVIDER_ADDR);
-    assert_deal_deleted(&rt, deal_ids[0], &deal1, 0);
-    assert_deal_deleted(&rt, deal_ids[1], &deal2, 0);
-    assert_deal_deleted(&rt, deal_ids[2], &deal3, 0);
+    assert_deal_deleted(&rt, deal_ids[0], &deal1, 0, true);
+    assert_deal_deleted(&rt, deal_ids[1], &deal2, 0, true);
+    assert_deal_deleted(&rt, deal_ids[2], &deal3, 0, true);
     check_state(&rt);
 }

actors/market/tests/deal_termination.rs

@@ -106,7 +106,7 @@ fn deal_is_terminated() {
         assert_eq!(tc.termination_payment, pay);
         assert_eq!(deal_proposal.provider_collateral, slashed);
 
-        assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number);
+        assert_deal_deleted(&rt, deal_id, &deal_proposal, sector_number, true);
 
         // assert that trying to settle is always a no-op after termination
 
@@ -197,7 +197,7 @@ fn settle_payments_then_terminate_deal_in_the_same_epoch() {
         &[sector_number],
         &[deal_id],
     );
-    assert_deal_deleted(&rt, deal_id, &proposal, sector_number);
+    assert_deal_deleted(&rt, deal_id, &proposal, sector_number, true);
 
     // end state should be equivalent to only calling termination
     let client_after = get_balance(&rt, &CLIENT_ADDR);
@@ -245,7 +245,7 @@ fn terminate_a_deal_then_settle_it_in_the_same_epoch() {
     );
     let ret = settle_deal_payments(&rt, PROVIDER_ADDR, &[deal_id], &[], &[]);
     assert_eq!(ret.results.codes(), vec![EX_DEAL_EXPIRED]);
-    assert_deal_deleted(&rt, deal_id, &proposal, sector_number);
+    assert_deal_deleted(&rt, deal_id, &proposal, sector_number, true);
 
     check_state(&rt);
 }

actors/market/tests/harness.rs

@@ -543,7 +543,7 @@ pub fn get_sector_deal_ids(
     rt: &MockRuntime,
     provider: ActorID,
     sector_numbers: &[SectorNumber],
-) -> Vec<DealID> {
+) -> Option<Vec<DealID>> {
     let st: State = rt.get_state();
     let provider_sectors = ProviderSectorsMap::load(
         &rt.store,
@@ -553,22 +553,31 @@ pub fn get_sector_deal_ids(
     )
     .unwrap();
     let sectors_root: Option<&Cid> = provider_sectors.get(&provider).unwrap();
-    if let Some(sectors_root) = sectors_root {
-        let sector_deals =
-            SectorDealsMap::load(&rt.store, sectors_root, SECTOR_DEALS_CONFIG, "sector deals")
-                .unwrap();
-        sector_numbers
-            .iter()
-            .flat_map(|sector_number| {
-                let deals: Option<&Vec<DealID>> = sector_deals.get(sector_number).unwrap();
-                match deals {
-                    Some(deals) => deals.clone(),
-                    None => vec![],
-                }
-            })
-            .collect()
-    } else {
-        vec![]
+    match sectors_root {
+        Some(sectors_root) => {
+            let sector_deals =
+                SectorDealsMap::load(&rt.store, sectors_root, SECTOR_DEALS_CONFIG, "sector deals")
+                    .unwrap();
+            let deal_ids: Vec<_> = sector_numbers
+                .iter()
+                .flat_map(|sector_number| {
+                    let deals: Option<&Vec<DealID>> = sector_deals.get(sector_number).unwrap();
+                    match deals {
+                        Some(deals) => {
+                            assert!(!deals.is_empty());
+                            deals.clone()
+                        }
+                        None => vec![],
+                    }
+                })
+                .collect();
+            if deal_ids.is_empty() {
+                None
+            } else {
+                Some(deal_ids)
+            }
+        }
+        None => None,
     }
 }
 
@@ -1194,6 +1203,7 @@ pub fn assert_deal_deleted(
     deal_id: DealID,
     p: &DealProposal,
     sector_number: SectorNumber,
+    empty_sector_deals: bool,
 ) {
     use cid::multihash::Code;
     use cid::multihash::MultihashDigest;
@@ -1224,7 +1234,12 @@ pub fn assert_deal_deleted(
 
     // Check deal is no longer associated with sector
     let sector_deals = get_sector_deal_ids(rt, p.provider.id().unwrap(), &[sector_number]);
-    assert!(!sector_deals.contains(&deal_id));
+    if empty_sector_deals {
+        assert!(sector_deals.is_none());
+    } else {
+        // expect some other deals, but not the one we deleted
+        assert!(!sector_deals.unwrap().contains(&deal_id));
+    }
 }
 
 pub fn assert_deal_failure<F>(add_funds: bool, post_setup: F, exit_code: ExitCode, sig_valid: bool)
@@ -1506,8 +1521,10 @@ pub fn terminate_deals_and_assert_balances(
 ) -> (/*total_paid*/ TokenAmount, /*total_slashed*/ TokenAmount) {
     // Accumulate deal IDs for all sectors
     let deal_ids = get_sector_deal_ids(rt, provider_addr.id().unwrap(), sectors);
+    assert!(deal_ids.is_some());
     // get deal state  before the are cleaned up in terminate deals
     let deal_infos: Vec<(DealState, DealProposal)> = deal_ids
+        .unwrap()
         .iter()
         .filter_map(|id| {
             let proposal = find_deal_proposal(rt, *id);
@@ -1599,11 +1616,13 @@ pub fn terminate_deals(
     // calculate the expected amount to be slashed for the provider that it is burnt
     let curr_epoch = *rt.epoch.borrow();
     let mut total_slashed = TokenAmount::zero();
-    for deal_id in deal_ids {
-        let d = find_deal_proposal(rt, deal_id);
-        if let Some(d) = d {
-            if curr_epoch < d.end_epoch {
-                total_slashed += d.provider_collateral.clone();
+    if let Some(deal_ids) = deal_ids {
+        for deal_id in deal_ids {
+            let d = find_deal_proposal(rt, deal_id);
+            if let Some(d) = d {
+                if curr_epoch < d.end_epoch {
+                    total_slashed += d.provider_collateral.clone();
+                }
             }
         }
     }

actors/market/tests/market_actor_legacy_tests.rs

@@ -59,7 +59,7 @@ fn slash_a_deal_and_make_payment_for_another_deal_in_the_same_epoch() {
     terminate_deals(&rt, PROVIDER_ADDR, &[sector_1], &[deal_id1]);
     cron_tick(&rt);
 
-    assert_deal_deleted(&rt, deal_id1, &d1, sector_1);
+    assert_deal_deleted(&rt, deal_id1, &d1, sector_1, true);
     let s2 = get_deal_state(&rt, deal_id2);
     assert_eq!(slash_epoch, s2.last_updated_epoch);
     check_state(&rt);