Review fixes

actors/market/src/lib.rs

@@ -587,7 +587,7 @@ impl Actor {
 
                 sector_deals.push((
                     p.sector_number,
-                    SectorDealIDs { sector_expiration: p.sector_expiry, deals: p.deal_ids.clone() },
+                    SectorDealIDs { deals: p.deal_ids.clone() },
                 ));
 
                 // Update deal states

actors/market/src/state.rs

@@ -88,7 +88,6 @@ pub struct State {
 /// IDs of deals associated with a single sector.
 #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)]
 pub struct SectorDealIDs {
-    pub sector_expiration: ChainEpoch,
     pub deals: Vec<DealID>,
 }
 
@@ -127,7 +126,6 @@ impl State {
                 "failed to create empty pending deal allocation map",
             )?;
 
-        // XXX Tune the width parameter
         let empty_sector_deals_hamt = make_empty_map::<_, Cid>(store, HAMT_BIT_WIDTH)
             .flush()
             .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to create empty sector deals map")?;
@@ -628,8 +626,7 @@ impl State {
     ////////////////////////////////////////////////////////////////////////////////
 
     // Stores deal IDs associated with sectors for a provider.
-    // Deal IDs are added to any already stored for the provider and sector, while the
-    // sector expiration overwrites any expiration stored for the sector.
+    // Deal IDs are added to any already stored for the provider and sector.
     // Returns the root cid of the sector deals map.
     pub fn put_sector_deal_ids<BS>(
         &mut self,
@@ -651,8 +648,9 @@ impl State {
                 .context_code(ExitCode::USR_ILLEGAL_STATE, "failed to read sector deals")?;
             if let Some(existing_deal_ids) = existing_deal_ids {
                 new_deals.deals.extend(existing_deal_ids.deals.iter());
-                new_deals.deals.sort();
             }
+            new_deals.deals.sort();
+            new_deals.deals.dedup();
             sector_deals
                 .set(key, new_deals)
                 .with_context_code(ExitCode::USR_ILLEGAL_STATE, || {
@@ -743,7 +741,6 @@ impl State {
                         .set(
                             key,
                             SectorDealIDs {
-                                sector_expiration: existing_deal_ids.sector_expiration,
                                 deals: new_deals,
                             },
                         )

actors/market/tests/batch_activate_deals.rs

@@ -21,6 +21,44 @@ const MINER_ADDRESSES: MinerAddresses = MinerAddresses {
     control: vec![],
 };
 
+#[test]
+fn activate_deals_one_sector() {
+    let rt = setup();
+    let epoch = rt.set_epoch(START_EPOCH);
+    let deals = [
+        create_deal(&rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH, false),
+        create_deal(&rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH + 1, false),
+        create_deal(&rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH + 2, true),
+    ];
+    let next_allocation_id = 1;
+    let datacap_required = TokenAmount::from_whole(deals[2].piece_size.0);
+    rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, WORKER_ADDR);
+    let deal_ids =
+        publish_deals(&rt, &MINER_ADDRESSES, &deals, datacap_required, next_allocation_id);
+
+    // Reverse deal IDs to check they are stored sorted in state.
+    let mut deal_ids_reversed = deal_ids.clone();
+    deal_ids_reversed.reverse();
+    let sectors = [(1, END_EPOCH + 10, deal_ids_reversed)];
+    let res = batch_activate_deals(&rt, PROVIDER_ADDR, &sectors);
+    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_ADDR, 1));
+
+    for id in deal_ids.iter() {
+        let state = get_deal_state(&rt, *id);
+        assert_eq!(1, state.sector_number);
+        assert_eq!(epoch, state.sector_start_epoch);
+        if *id == deal_ids[2] {
+            assert_eq!(state.verified_claim, next_allocation_id);
+        } else {
+            assert_eq!(state.verified_claim, NO_ALLOCATION_ID);
+        }
+    }
+    check_state(&rt);
+}
+
 #[test]
 fn activate_deals_across_multiple_sectors() {
     let rt = setup();
@@ -291,38 +329,35 @@ fn fails_to_activate_sectors_containing_duplicate_deals() {
 }
 
 #[test]
-fn activate_deals_in_same_sector_separately() {
+fn activate_new_deals_in_existing_sector() {
+    // At time of writing, the miner actor won't do this.
+    // But future re-snap could allow it.
     let rt = setup();
-    let deal1 = create_deal(&rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH, false);
-    let deal2 = create_deal(&rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH + 1, false);
-    let deal3 = create_deal(&rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH + 2, false);
+    let deals = vec![
+        create_deal(&rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH, false),
+        create_deal(&rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH + 1, false),
+        create_deal(&rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH + 2, false),
+    ];
 
     rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, WORKER_ADDR);
-    let deal_ids =
-        publish_deals(&rt, &MINER_ADDRESSES, &[deal1, deal2, deal3], TokenAmount::zero(), 0);
+    let deal_ids = publish_deals(&rt, &MINER_ADDRESSES, &deals, TokenAmount::zero(), 0);
     assert_eq!(3, deal_ids.len());
 
-    let deal1_id = deal_ids[0];
-    let deal2_id = deal_ids[1];
-    let deal3_id = deal_ids[2];
-
+    // Activate deals separately, and out of order.
     let sector_number = 1;
     batch_activate_deals(
         &rt,
         PROVIDER_ADDR,
-        &[(sector_number, END_EPOCH + 10, vec![deal1_id, deal2_id])],
+        &[(sector_number, END_EPOCH + 10, vec![deal_ids[0], deal_ids[2]])],
     );
-    // Another deal in the same sector.
-    batch_activate_deals(&rt, PROVIDER_ADDR, &[(sector_number, END_EPOCH + 10, vec![deal3_id])]);
+    batch_activate_deals(&rt, PROVIDER_ADDR, &[(sector_number, END_EPOCH + 10, vec![deal_ids[1]])]);
 
     // all deals are activated
-    assert_eq!(0, get_deal_state(&rt, deal1_id).sector_start_epoch);
-    assert_eq!(0, get_deal_state(&rt, deal3_id).sector_start_epoch);
-    assert_eq!(0, get_deal_state(&rt, deal2_id).sector_start_epoch);
+    assert_eq!(0, get_deal_state(&rt, deal_ids[0]).sector_start_epoch);
+    assert_eq!(0, get_deal_state(&rt, deal_ids[1]).sector_start_epoch);
+    assert_eq!(0, get_deal_state(&rt, deal_ids[2]).sector_start_epoch);
 
-    assert_eq!(
-        vec![deal1_id, deal2_id, deal3_id],
-        get_sector_deal_ids(&rt, &PROVIDER_ADDR, sector_number)
-    );
+    // All deals stored under the sector, in order.
+    assert_eq!(deal_ids, get_sector_deal_ids(&rt, &PROVIDER_ADDR, sector_number));
     check_state(&rt);
 }

actors/market/tests/market_actor_test.rs

@@ -53,7 +53,6 @@ fn test_remove_all_error() {
     SetMultimap::new(&rt.store()).remove_all(42).expect("expected no error");
 }
 
-// TODO add array stuff
 #[test]
 fn simple_construction() {
     let rt = MockRuntime {

actors/market/tests/on_miner_sectors_terminate.rs

@@ -19,7 +19,48 @@ mod harness;
 
 use harness::*;
 
-// TODO: test deals in different sector with same provider are not terminated.
+#[test]
+fn terminate_multiple_deals_from_single_provider() {
+let start_epoch = 10;
+    let end_epoch = start_epoch + 200 * EPOCHS_IN_DAY;
+    let sector_expiry = end_epoch + 100;
+    let current_epoch = 5;
+
+    let rt = setup();
+    rt.set_epoch(current_epoch);
+
+    // IDs are both deal and sector IDs.
+    let [id1, id2, id3]: [DealID; 3] = (end_epoch..end_epoch + 3)
+        .map(|epoch| {
+            let id = generate_and_publish_deal(
+                &rt,
+                CLIENT_ADDR,
+                &MinerAddresses::default(),
+                start_epoch,
+                epoch,
+            );
+            let ret = activate_deals(
+                &rt,
+                sector_expiry,
+                PROVIDER_ADDR,
+                current_epoch,
+                id, // use deal ID as unique sector number
+                &[id],
+            );
+            assert!(ret.activation_results.all_ok());
+            id
+        })
+        .collect::<Vec<DealID>>()
+        .try_into()
+        .unwrap();
+
+    terminate_deals(&rt, PROVIDER_ADDR, &[id1]);
+    assert_deals_terminated(&rt, current_epoch, &[id1]);
+    assert_deals_not_terminated(&rt, &[id2, id3]);
+
+    terminate_deals(&rt, PROVIDER_ADDR, &[id2, id3]);
+    assert_deals_terminated(&rt, current_epoch, &[id1, id2, id3]);
+}
 
 #[test]
 fn terminate_multiple_deals_from_multiple_providers() {
@@ -47,19 +88,21 @@ fn terminate_multiple_deals_from_multiple_providers() {
         .try_into()
         .unwrap();
     let sector_number = 7; // Both providers used the same sector number
-    activate_deals(
+    let ret = activate_deals(
         &rt,
         sector_expiry,
         PROVIDER_ADDR,
         current_epoch,
         sector_number,
         &[deal1, deal2, deal3],
     );
+    assert!(ret.activation_results.all_ok());
 
     let addrs = MinerAddresses { provider: provider2, ..MinerAddresses::default() };
     let deal4 = generate_and_publish_deal(&rt, CLIENT_ADDR, &addrs, start_epoch, end_epoch);
     let deal5 = generate_and_publish_deal(&rt, CLIENT_ADDR, &addrs, start_epoch, end_epoch + 1);
-    activate_deals(&rt, sector_expiry, provider2, current_epoch, sector_number, &[deal4, deal5]);
+    let ret = activate_deals(&rt, sector_expiry, provider2, current_epoch, sector_number, &[deal4, deal5]);
+    assert!(ret.activation_results.all_ok());
 
     terminate_deals(&rt, PROVIDER_ADDR, &[sector_number]);
     assert_deals_terminated(&rt, current_epoch, &[deal1, deal2, deal3]);
@@ -91,7 +134,8 @@ fn ignore_sector_that_does_not_exist() {
         start_epoch,
         end_epoch,
     );
-    activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &[deal1]);
+    let ret = activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &[deal1]);
+    assert!(ret.activation_results.all_ok());
     terminate_deals(&rt, PROVIDER_ADDR, &[sector_number + 1]);
 
     let s = get_deal_state(&rt, deal1);
@@ -132,14 +176,15 @@ fn terminate_valid_deals_along_with_just_expired_deal() {
         end_epoch - 1,
     );
     let sector_number = 7;
-    activate_deals(
+    let ret = activate_deals(
         &rt,
         sector_expiry,
         PROVIDER_ADDR,
         current_epoch,
         sector_number,
         &[deal1, deal2, deal3],
     );
+    assert!(ret.activation_results.all_ok());
 
     let new_epoch = end_epoch - 1;
     rt.set_epoch(new_epoch);
@@ -187,7 +232,9 @@ fn terminate_valid_deals_along_with_expired_and_cleaned_up_deal() {
     );
     assert_eq!(2, deal_ids.len());
     let sector_number = 7;
-    activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &deal_ids);
+    let ret = activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &deal_ids);
+    assert!(ret.activation_results.all_ok());
+
 
     let new_epoch = end_epoch - 1;
     rt.set_epoch(new_epoch);
@@ -217,7 +264,8 @@ fn terminating_a_deal_the_second_time_does_not_change_its_slash_epoch() {
         end_epoch,
     );
     let sector_number = 7;
-    activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &[deal1]);
+    let ret = activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &[deal1]);
+    assert!(ret.activation_results.all_ok());
 
     // terminating the deal so slash epoch is the current epoch
     terminate_deals(&rt, PROVIDER_ADDR, &[sector_number]);
@@ -256,12 +304,14 @@ fn terminating_new_deals_and_an_already_terminated_deal_only_terminates_the_new_
     let [deal1, deal2, deal3]: [DealID; 3] = deals.as_slice().try_into().unwrap();
     // Activate 1 deal
     let sector_number = 7;
-    activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &deals[0..1]);
+    let ret = activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &deals[0..1]);
+    assert!(ret.activation_results.all_ok());
     // Terminate them
     terminate_deals(&rt, PROVIDER_ADDR, &[sector_number]);
 
     // Activate other deals in the same sector
-    activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &deals[1..3]);
+    let ret = activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &deals[1..3]);
+    assert!(ret.activation_results.all_ok());
     // set a new epoch and terminate again
     let new_epoch = current_epoch + 1;
     rt.set_epoch(new_epoch);
@@ -298,7 +348,8 @@ fn do_not_terminate_deal_if_end_epoch_is_equal_to_or_less_than_current_epoch() {
         end_epoch,
     );
     let sector_number = 7;
-    activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &[deal1]);
+    let ret = activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &[deal1]);
+    assert!(ret.activation_results.all_ok());
     rt.set_epoch(end_epoch);
     terminate_deals(&rt, PROVIDER_ADDR, &[sector_number]);
     assert_deals_not_terminated(&rt, &[deal1]);
@@ -313,7 +364,8 @@ fn do_not_terminate_deal_if_end_epoch_is_equal_to_or_less_than_current_epoch() {
         end_epoch,
     );
     let sector_number = sector_number + 1;
-    activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &[deal2]);
+    let ret = activate_deals(&rt, sector_expiry, PROVIDER_ADDR, current_epoch, sector_number, &[deal2]);
+    assert!(ret.activation_results.all_ok());
     rt.set_epoch(end_epoch + 1);
     terminate_deals(&rt, PROVIDER_ADDR, &[sector_number]);
     assert_deals_not_terminated(&rt, &[deal2]);
@@ -329,7 +381,6 @@ fn fail_when_caller_is_not_a_storage_miner_actor() {
     let params =
         OnMinerSectorsTerminateParams { epoch: *rt.epoch.borrow(), sectors: BitField::new() };
 
-    // XXX: Which exit code is correct: SYS_FORBIDDEN(8) or USR_FORBIDDEN(18)?
     assert_eq!(
         ExitCode::USR_FORBIDDEN,
         rt.call::<MarketActor>(

actors/miner/src/lib.rs

@@ -3960,10 +3960,6 @@ fn process_early_terminations(
 
             let mut sectors_terminated: Vec<BitField> = Vec::new();
             let mut total_initial_pledge = TokenAmount::zero();
-            // let mut deals_to_terminate =
-            //     Vec::<ext::market::OnMinerSectorsTerminateParams>::with_capacity(
-            //         result.sectors.len(),
-            //     );
             let mut penalty = TokenAmount::zero();
 
             for (epoch, sector_numbers) in result.iter() {
@@ -3986,9 +3982,6 @@ fn process_early_terminations(
                     deal_ids.extend(sector.deal_ids);
                     total_initial_pledge += sector.initial_pledge;
                 }
-
-                // let params = ext::market::OnMinerSectorsTerminateParams { epoch, deal_ids };
-                // deals_to_terminate.push(params);
             }
 
             // Pay penalty
@@ -4282,9 +4275,9 @@ fn request_terminate_deals(
     sectors: &BitField,
 ) -> Result<(), ActorError> {
     if !sectors.is_empty() {
-        // XXX REVIEW: should we chunk this call?
-        // The sector bitfield representation is now much more efficient.
-        // But possibly still large, or large amount of work in the market.
+        // The sectors bitfield could be large, but will fit into a single parameters block.
+        // The FVM max block size of 1MiB supports 130K 8-byte integers, but the policy parameter
+        // ADDRESSED_SECTORS_MAX (currently 25k) will avoid reaching that.
         let res = extract_send_result(rt.send_simple(
             &STORAGE_MARKET_ACTOR_ADDR,
             ext::market::ON_MINER_SECTORS_TERMINATE_METHOD,

runtime/src/runtime/policy.rs

@@ -73,7 +73,10 @@ pub struct Policy {
     /// Maximum number of unique "declarations" in batch operations.
     pub declarations_max: u64,
 
-    /// The maximum number of sector infos that may be required to be loaded in a single invocation.
+    /// The maximum number of sector numbers addressable in a single invocation
+    /// (which implies also the max infos that may be loaded at once).
+    /// One upper bound on this is the max size of a storage block: 1MiB supports 130k at 8 bytes each,
+    /// though bitfields can compress this.
     pub addressed_sectors_max: u64,
 
     pub max_pre_commit_randomness_lookback: ChainEpoch,
@@ -299,7 +302,10 @@ pub mod policy_constants {
     /// Maximum number of unique "declarations" in batch operations.
     pub const DECLARATIONS_MAX: u64 = ADDRESSED_PARTITIONS_MAX;
 
-    /// The maximum number of sector infos that may be required to be loaded in a single invocation.
+    /// The maximum number of sector numbers addressable in a single invocation
+    /// (which implies also the max infos that may be loaded at once).
+    /// One upper bound on this is the max size of a storage block: 1MiB supports 130k at 8 bytes each,
+    /// though bitfields can compress this.
     pub const ADDRESSED_SECTORS_MAX: u64 = 25_000;
 
     pub const MAX_PRE_COMMIT_RANDOMNESS_LOOKBACK: ChainEpoch = EPOCHS_IN_DAY + CHAIN_FINALITY;

runtime/src/test_utils.rs

@@ -1143,7 +1143,7 @@ impl<BS: Blockstore> Runtime for MockRuntime<BS> {
 
         let expected_msg = self.expectations.borrow_mut().expect_sends.pop_front().unwrap();
 
-        assert_eq!(expected_msg.to, *to, "expected message to {}, was {}", expected_msg.to, to,);
+        assert_eq!(expected_msg.to, *to, "expected message to {}, was {}", expected_msg.to, to);
         assert_eq!(
             expected_msg.method, method,
             "expected method {}, was {}",