Adds CommD as option in return value from ActivateDeals (#1361)

* Optionally return CommD from ActivateDeals, avoid redundant call in ProveReplicaUpdates

* Fix integration tests

actors/market/src/lib.rs

@@ -496,7 +496,7 @@ impl Actor {
         let st: State = rt.state()?;
         let proposal_array = st.get_proposal_array(rt.store())?;
 
-        let mut sectors_data = Vec::with_capacity(params.sectors.len());
+        let mut unsealed_cids = Vec::with_capacity(params.sectors.len());
         for sector in params.sectors.iter() {
             let sector_proposals = get_proposals(&proposal_array, &sector.deal_ids, st.next_id)?;
             let sector_size = sector
@@ -518,10 +518,10 @@ impl Actor {
                 Some(compute_data_commitment(rt, &sector_proposals, sector.sector_type)?)
             };
 
-            sectors_data.push(SectorDealData { commd });
+            unsealed_cids.push(commd);
         }
 
-        Ok(VerifyDealsForActivationReturn { sectors: sectors_data })
+        Ok(VerifyDealsForActivationReturn { unsealed_cids })
     }
 
     /// Activate a set of deals grouped by sector, returning the size and
@@ -584,9 +584,9 @@ impl Actor {
                 // This construction could be replaced with a single "update deal state"
                 // state method, possibly batched over all deal ids at once.
                 let update_result: Result<(), ActorError> =
-                    proposals.into_iter().try_for_each(|(deal_id, proposal)| {
+                    proposals.iter().try_for_each(|(deal_id, proposal)| {
                         let s = st
-                            .find_deal_state(rt.store(), deal_id)
+                            .find_deal_state(rt.store(), *deal_id)
                             .context(format!("error looking up deal state for {}", deal_id))?;
 
                         if s.is_some() {
@@ -597,7 +597,7 @@ impl Actor {
                             ));
                         }
 
-                        let propc = rt_deal_cid(rt, &proposal)?;
+                        let propc = rt_deal_cid(rt, proposal)?;
 
                         // Confirm the deal is in the pending proposals queue.
                         // It will be removed from this queue later, during cron.
@@ -613,7 +613,7 @@ impl Actor {
 
                         // Extract and remove any verified allocation ID for the pending deal.
                         let allocation = st
-                            .remove_pending_deal_allocation_id(rt.store(), deal_id)
+                            .remove_pending_deal_allocation_id(rt.store(), *deal_id)
                             .context(format!(
                                 "failed to remove pending deal allocation id {}",
                                 deal_id
@@ -630,23 +630,30 @@ impl Actor {
                         }
 
                         deal_states.push((
-                            deal_id,
+                            *deal_id,
                             DealState {
                                 sector_start_epoch: curr_epoch,
                                 last_updated_epoch: EPOCH_UNDEFINED,
                                 slash_epoch: EPOCH_UNDEFINED,
                                 verified_claim: allocation,
                             },
                         ));
-                        activated_deals.insert(deal_id);
+                        activated_deals.insert(*deal_id);
                         Ok(())
                     });
 
+                let data_commitment = if params.compute_cid && !p.deal_ids.is_empty() {
+                    Some(compute_data_commitment(rt, &proposals, p.sector_type)?)
+                } else {
+                    None
+                };
+
                 match update_result {
                     Ok(_) => {
                         activations.push(SectorDealActivation {
                             nonverified_deal_space: deal_spaces.deal_space,
                             verified_infos,
+                            unsealed_cid: data_commitment,
                         });
                         batch_gen.add_success();
                     }

actors/market/src/types.rs

@@ -86,22 +86,19 @@ pub struct SectorDeals {
 
 #[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone, Eq, PartialEq)]
 pub struct VerifyDealsForActivationReturn {
-    pub sectors: Vec<SectorDealData>,
-}
-
-#[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone, Eq, PartialEq, Default)]
-pub struct SectorDealData {
-    /// Option::None signifies commitment to empty sector, meaning no deals.
-    pub commd: Option<Cid>,
+    // The unsealed CID computed from the deals specified for each sector.
+    // A None indicates no deals were specified.
+    pub unsealed_cids: Vec<Option<Cid>>,
 }
 
 #[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone, Eq, PartialEq)]
-#[serde(transparent)]
 pub struct BatchActivateDealsParams {
     /// Deals to activate, grouped by sector.
     /// A failed deal activation will cause other deals in the same sector group to also fail,
     /// but allow other sectors to proceed.
     pub sectors: Vec<SectorDeals>,
+    /// Requests computation of an unsealed CID for each sector from the provided deals.
+    pub compute_cid: bool,
 }
 
 // Information about a verified deal that has been activated.
@@ -121,6 +118,9 @@ pub struct SectorDealActivation {
     pub nonverified_deal_space: BigInt,
     /// Information about each verified deal activated.
     pub verified_infos: Vec<VerifiedDealInfo>,
+    /// Unsealed CID computed from the deals specified for the sector.
+    /// A None indicates no deals were specified, or the computation was not requested.
+    pub unsealed_cid: Option<Cid>,
 }
 
 #[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone, Eq, PartialEq)]

actors/market/tests/activate_deal_failures.rs

@@ -37,6 +37,7 @@ fn fail_when_caller_is_not_the_provider_of_the_deal() {
             sector_type: RegisteredSealProof::StackedDRG8MiBV1,
             deal_ids: vec![deal_id],
         }],
+        false,
     )
     .unwrap();
     let res: BatchActivateDealsResult =
@@ -59,7 +60,7 @@ fn fail_when_caller_is_not_a_storage_miner_actor() {
         sector_expiry: 0,
         sector_type: RegisteredSealProof::StackedDRG8MiBV1,
     };
-    let params = BatchActivateDealsParams { sectors: vec![sector_activation] };
+    let params = BatchActivateDealsParams { sectors: vec![sector_activation], compute_cid: false };
 
     expect_abort(
         ExitCode::USR_FORBIDDEN,
@@ -85,6 +86,7 @@ fn fail_when_deal_has_not_been_published_before() {
             sector_expiry: EPOCHS_IN_DAY,
             deal_ids: vec![DealID::from(42u32)],
         }],
+        false,
     )
     .unwrap();
     let res: BatchActivateDealsResult =
@@ -120,6 +122,7 @@ fn fail_when_deal_has_already_been_activated() {
             sector_expiry,
             deal_ids: vec![deal_id],
         }],
+        false,
     )
     .unwrap();
     let res: BatchActivateDealsResult =

actors/market/tests/batch_activate_deals.rs

@@ -55,7 +55,7 @@ fn activate_deals_across_multiple_sectors() {
         (END_EPOCH + 2, vec![unverified_deal_2_id]), // contains unverified deal only
     ];
 
-    let res = batch_activate_deals(&rt, PROVIDER_ADDR, &sectors);
+    let res = batch_activate_deals(&rt, PROVIDER_ADDR, &sectors, false);
 
     // three sectors activated successfully
     assert!(res.activation_results.all_ok());
@@ -121,7 +121,7 @@ fn sectors_fail_and_succeed_independently_during_batch_activation() {
         SectorDeals { deal_ids: vec![id_4], sector_type, sector_expiry: END_EPOCH + 2 }, // sector succeeds
     ];
 
-    let res = batch_activate_deals_raw(&rt, PROVIDER_ADDR, sectors_deals).unwrap();
+    let res = batch_activate_deals_raw(&rt, PROVIDER_ADDR, sectors_deals, false).unwrap();
     let res: BatchActivateDealsResult =
         res.unwrap().deserialize().expect("VerifyDealsForActivation failed!");
 
@@ -172,7 +172,7 @@ fn handles_sectors_empty_of_deals_gracefully() {
         SectorDeals { deal_ids: vec![], sector_type, sector_expiry: END_EPOCH + 2 }, // empty sector
     ];
 
-    let res = batch_activate_deals_raw(&rt, PROVIDER_ADDR, sectors_deals).unwrap();
+    let res = batch_activate_deals_raw(&rt, PROVIDER_ADDR, sectors_deals, false).unwrap();
     let res: BatchActivateDealsResult =
         res.unwrap().deserialize().expect("VerifyDealsForActivation failed!");
 
@@ -221,7 +221,7 @@ fn fails_to_activate_sectors_containing_duplicate_deals() {
         SectorDeals { deal_ids: vec![id_3], sector_type, sector_expiry: END_EPOCH },
     ];
 
-    let res = batch_activate_deals_raw(&rt, PROVIDER_ADDR, sectors_deals).unwrap();
+    let res = batch_activate_deals_raw(&rt, PROVIDER_ADDR, sectors_deals, false).unwrap();
     let res: BatchActivateDealsResult =
         res.unwrap().deserialize().expect("VerifyDealsForActivation failed!");
 

actors/market/tests/harness.rs

@@ -320,7 +320,7 @@ pub fn activate_deals(
     deal_ids: &[DealID],
 ) -> BatchActivateDealsResult {
     rt.set_epoch(current_epoch);
-
+    let compute_cid = false;
     let ret = batch_activate_deals_raw(
         rt,
         provider,
@@ -329,6 +329,7 @@ pub fn activate_deals(
             sector_expiry,
             sector_type: RegisteredSealProof::StackedDRG8MiBV1,
         }],
+        compute_cid,
     )
     .unwrap();
 
@@ -352,6 +353,7 @@ pub fn batch_activate_deals(
     rt: &MockRuntime,
     provider: Address,
     sectors: &[(ChainEpoch, Vec<DealID>)],
+    compute_cid: bool,
 ) -> BatchActivateDealsResult {
     let sectors_deals: Vec<SectorDeals> = sectors
         .iter()
@@ -361,7 +363,7 @@ pub fn batch_activate_deals(
             sector_type: RegisteredSealProof::StackedDRG8MiBV1,
         })
         .collect();
-    let ret = batch_activate_deals_raw(rt, provider, sectors_deals).unwrap();
+    let ret = batch_activate_deals_raw(rt, provider, sectors_deals, compute_cid).unwrap();
 
     let ret: BatchActivateDealsResult =
         ret.unwrap().deserialize().expect("VerifyDealsForActivation failed!");
@@ -376,11 +378,12 @@ pub fn batch_activate_deals_raw(
     rt: &MockRuntime,
     provider: Address,
     sectors_deals: Vec<SectorDeals>,
+    compute_cid: bool,
 ) -> Result<Option<IpldBlock>, ActorError> {
     rt.set_caller(*MINER_ACTOR_CODE_ID, provider);
     rt.expect_validate_caller_type(vec![Type::Miner]);
 
-    let params = BatchActivateDealsParams { sectors: sectors_deals };
+    let params = BatchActivateDealsParams { sectors: sectors_deals, compute_cid };
 
     let ret = rt.call::<MarketActor>(
         Method::BatchActivateDeals as u64,

actors/market/tests/market_actor_test.rs

@@ -1699,6 +1699,7 @@ fn fail_when_current_epoch_greater_than_start_epoch_of_deal() {
             sector_type: RegisteredSealProof::StackedDRG8MiBV1,
             deal_ids: vec![deal_id],
         }],
+        false,
     )
     .unwrap();
 
@@ -1733,6 +1734,7 @@ fn fail_when_end_epoch_of_deal_greater_than_sector_expiry() {
             sector_type: RegisteredSealProof::StackedDRG8MiBV1,
             deal_ids: vec![deal_id],
         }],
+        false,
     )
     .unwrap();
 
@@ -1760,7 +1762,7 @@ fn fail_to_activate_all_deals_if_one_deal_fails() {
         start_epoch,
         end_epoch,
     );
-    batch_activate_deals(&rt, PROVIDER_ADDR, &[(sector_expiry, vec![deal_id1])]);
+    batch_activate_deals(&rt, PROVIDER_ADDR, &[(sector_expiry, vec![deal_id1])], false);
 
     let deal_id2 = generate_and_publish_deal(
         &rt,
@@ -1778,6 +1780,7 @@ fn fail_to_activate_all_deals_if_one_deal_fails() {
             sector_type: RegisteredSealProof::StackedDRG8MiBV1,
             deal_ids: vec![deal_id1, deal_id2],
         }],
+        false,
     )
     .unwrap();
     let res: BatchActivateDealsResult =

actors/market/tests/verify_deals_for_activation_test.rs

@@ -51,8 +51,8 @@ fn verify_deal_and_activate_to_get_deal_space_for_unverified_deal_proposal() {
     );
     let a_response = activate_deals(&rt, SECTOR_EXPIRY, PROVIDER_ADDR, CURR_EPOCH, &[deal_id]);
     let s_response = a_response.activations.get(0).unwrap();
-    assert_eq!(1, v_response.sectors.len());
-    assert_eq!(Some(make_piece_cid("1".as_bytes())), v_response.sectors[0].commd);
+    assert_eq!(1, v_response.unsealed_cids.len());
+    assert_eq!(Some(make_piece_cid("1".as_bytes())), v_response.unsealed_cids[0]);
     assert!(s_response.verified_infos.is_empty());
     assert_eq!(BigInt::from(deal_proposal.piece_size.0), s_response.nonverified_deal_space);
 
@@ -87,8 +87,8 @@ fn verify_deal_and_activate_to_get_deal_space_for_verified_deal_proposal() {
     let a_response = activate_deals(&rt, SECTOR_EXPIRY, PROVIDER_ADDR, CURR_EPOCH, &[deal_id]);
     let s_response = a_response.activations.get(0).unwrap();
 
-    assert_eq!(1, response.sectors.len());
-    assert_eq!(Some(make_piece_cid("1".as_bytes())), response.sectors[0].commd);
+    assert_eq!(1, response.unsealed_cids.len());
+    assert_eq!(Some(make_piece_cid("1".as_bytes())), response.unsealed_cids[0]);
     assert_eq!(1, s_response.verified_infos.len());
     assert_eq!(deal_proposal.piece_size, s_response.verified_infos[0].size);
     assert_eq!(deal_proposal.client.id().unwrap(), s_response.verified_infos[0].client);
@@ -148,7 +148,7 @@ fn verification_and_weights_for_verified_and_unverified_deals() {
     let a_response = activate_deals(&rt, SECTOR_EXPIRY, PROVIDER_ADDR, CURR_EPOCH, &deal_ids);
     let s_response = a_response.activations.get(0).unwrap();
 
-    assert_eq!(1, response.sectors.len());
+    assert_eq!(1, response.unsealed_cids.len());
     let returned_verified_space: BigInt =
         s_response.verified_infos.iter().map(|info| BigInt::from(info.size.0)).sum();
     assert_eq!(verified_space, returned_verified_space);

actors/miner/src/ext.rs

@@ -1,5 +1,4 @@
 use cid::Cid;
-use fil_actors_runtime::BatchReturn;
 use fvm_ipld_encoding::tuple::*;
 use fvm_ipld_encoding::RawBytes;
 use fvm_shared::bigint::{bigint_ser, BigInt};
@@ -12,12 +11,13 @@ use fvm_shared::sector::{RegisteredSealProof, StoragePower};
 use fvm_shared::smooth::FilterEstimate;
 use fvm_shared::ActorID;
 
+use fil_actors_runtime::BatchReturn;
+
 pub mod account {
     pub const PUBKEY_ADDRESS_METHOD: u64 = 2;
 }
 
 pub mod market {
-
     use super::*;
 
     pub const VERIFY_DEALS_FOR_ACTIVATION_METHOD: u64 = 5;
@@ -32,9 +32,9 @@ pub mod market {
     }
 
     #[derive(Serialize_tuple, Deserialize_tuple)]
-    #[serde(transparent)]
     pub struct BatchActivateDealsParams {
         pub sectors: Vec<SectorDeals>,
+        pub compute_cid: bool,
     }
 
     #[derive(Serialize_tuple, Deserialize_tuple, Clone)]
@@ -57,24 +57,17 @@ pub mod market {
     }
 
     #[derive(Serialize_tuple, Deserialize_tuple, Clone)]
-    pub struct DealActivation {
+    pub struct SectorDealActivation {
         #[serde(with = "bigint_ser")]
         pub nonverified_deal_space: BigInt,
         pub verified_infos: Vec<VerifiedDealInfo>,
+        pub unsealed_cid: Option<Cid>,
     }
 
     #[derive(Serialize_tuple, Deserialize_tuple, Clone)]
     pub struct BatchActivateDealsResult {
         pub activation_results: BatchReturn,
-        pub activations: Vec<DealActivation>,
-    }
-
-    #[derive(Serialize_tuple, Deserialize_tuple, Clone, Default)]
-    pub struct DealSpaces {
-        #[serde(with = "bigint_ser")]
-        pub deal_space: BigInt,
-        #[serde(with = "bigint_ser")]
-        pub verified_deal_space: BigInt,
+        pub activations: Vec<SectorDealActivation>,
     }
 
     #[derive(Serialize_tuple, Deserialize_tuple)]
@@ -100,15 +93,9 @@ pub mod market {
         pub sectors: &'a [SectorDeals],
     }
 
-    #[derive(Serialize_tuple, Deserialize_tuple, Default, Clone)]
-    pub struct SectorDealData {
-        /// Option::None signifies commitment to empty sector, meaning no deals.
-        pub commd: Option<Cid>,
-    }
-
     #[derive(Serialize_tuple, Deserialize_tuple, Default, Clone)]
     pub struct VerifyDealsForActivationReturn {
-        pub sectors: Vec<SectorDealData>,
+        pub unsealed_cids: Vec<Option<Cid>>,
     }
 }