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)?;
         }
 
@@ -694,19 +685,7 @@ impl State {
                 }
             }
             if flush {
-                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 {
-                    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)?;
@@ -1305,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,13 @@ 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 {
+        println!("sector deals: {:?}", 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 +1522,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_none());
     // 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 +1617,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);

actors/market/tests/market_actor_test.rs

@@ -344,7 +344,7 @@ fn worker_balance_after_withdrawal_must_account_for_slashed_funds() {
     // terminate the deal
     rt.set_epoch(publish_epoch + 1);
     terminate_deals(&rt, PROVIDER_ADDR, &[sector_number], &[deal_id]);
-    assert_deal_deleted(&rt, deal_id, &proposal, sector_number);
+    assert_deal_deleted(&rt, deal_id, &proposal, sector_number, true);
 
     // provider cannot withdraw any funds since it's been terminated
     let withdraw_amount = TokenAmount::from_atto(1);
@@ -1369,7 +1369,7 @@ fn terminating_a_deal_removes_proposal_synchronously() {
 
     // terminating the deal deletes proposal, state and pending_proposal but leaves deal op in queue
     terminate_deals(&rt, addrs.provider, &[sector_number], &[deal_id]);
-    assert_deal_deleted(&rt, deal_id, &proposal, sector_number);
+    assert_deal_deleted(&rt, deal_id, &proposal, sector_number, true);
     check_state(&rt);
 
     // the next cron_tick will remove the dangling deal op entry