Remove allocation id from market deal state. Clean up and improve sta…

…te checks. (#1403)

actors/market/src/deal.rs

@@ -1,7 +1,6 @@
 // Copyright 2019-2022 ChainSafe Systems
 // SPDX-License-Identifier: Apache-2.0, MIT
 
-use crate::ext::verifreg::AllocationID;
 use cid::{Cid, Version};
 use fvm_ipld_encoding::tuple::*;
 use fvm_ipld_encoding::BytesSer;
@@ -145,6 +144,4 @@ pub struct DealState {
     pub last_updated_epoch: ChainEpoch,
     // -1 if deal never slashed
     pub slash_epoch: ChainEpoch,
-    // ID of the verified registry allocation/claim for this deal's data (0 if none).
-    pub verified_claim: AllocationID,
 }

actors/market/src/lib.rs

@@ -619,7 +619,6 @@ impl Actor {
                             sector_start_epoch: curr_epoch,
                             last_updated_epoch: EPOCH_UNDEFINED,
                             slash_epoch: EPOCH_UNDEFINED,
-                            verified_claim: alloc_id,
                         },
                     ));
                 }
@@ -734,9 +733,10 @@ impl Actor {
                     // No continue below here, to ensure state changes are consistent.
                     activated_deals.insert(deal_id);
 
-                    // Extract and remove any verified allocation ID for the pending deal.
-                    let alloc_id =
-                        pending_deal_allocation_ids.delete(&deal_id)?.unwrap_or(NO_ALLOCATION_ID);
+                    // Remove any verified allocation ID for the pending deal.
+                    // The allocation ID is only used in BatchActivateDeals
+                    // (where it's returned to the caller).
+                    pending_deal_allocation_ids.delete(&deal_id)?.unwrap_or(NO_ALLOCATION_ID);
 
                     deal_states.push((
                         deal_id,
@@ -745,15 +745,6 @@ impl Actor {
                             sector_start_epoch: curr_epoch,
                             last_updated_epoch: EPOCH_UNDEFINED,
                             slash_epoch: EPOCH_UNDEFINED,
-                            // This allocation ID may exist if allocation was created by this
-                            // built-in market actor, but in this method, that doesn't mean the
-                            // SP necessarily claimed the allocation before activating this deal.
-                            // The allocation ID is only in market state to support the
-                            // old ActivateDeals flow where this is the way that the miner actor
-                            // learns the allocation ID.
-                            // In the new flow, the SP provides the allocation ID explicitly
-                            // and claims it _before_ notifying the market of deal activation.
-                            verified_claim: alloc_id,
                         },
                     ));
                     sector_deal_ids.push(deal_id);

actors/market/src/testing.rs

@@ -22,11 +22,11 @@ use fil_actors_runtime::{
     make_map_with_root_and_bitwidth, parse_uint_key, ActorError, MessageAccumulator, SetMultimap,
 };
 
+use crate::ext::verifreg::AllocationID;
 use crate::{
     balance_table::BalanceTable, DealArray, DealMetaArray, DealProposal, State,
     PROPOSALS_AMT_BITWIDTH,
 };
-use crate::{ext::verifreg::AllocationID, NO_ALLOCATION_ID};
 
 #[derive(Clone)]
 pub struct DealSummary {
@@ -55,7 +55,6 @@ impl Default for DealSummary {
 #[derive(Default, Clone)]
 pub struct StateSummary {
     pub deals: BTreeMap<DealID, DealSummary>,
-    pub claim_id_to_deal_id: BTreeMap<u64, DealID>,
     pub alloc_id_to_deal_id: BTreeMap<u64, DealID>,
     pub pending_proposal_count: u64,
     pub deal_state_count: u64,
@@ -174,7 +173,6 @@ pub fn check_state_invariants<BS: Blockstore>(
 
     // deal states
     let mut deal_state_count = 0;
-    let mut claim_id_to_deal_id = BTreeMap::<u64, DealID>::new();
     match DealMetaArray::load(&state.states, store) {
         Ok(deal_states) => {
             let ret = deal_states.for_each(|deal_id, deal_state| {
@@ -210,11 +208,6 @@ pub fn check_state_invariants<BS: Blockstore>(
                 acc.require(!pending_allocations.contains_key(&deal_id), format!("deal {deal_id} has pending allocation"));
 
                 deal_state_count += 1;
-
-                if deal_state.verified_claim != NO_ALLOCATION_ID {
-                    claim_id_to_deal_id.insert(deal_state.verified_claim, deal_id);
-                }
-
                 Ok(())
             });
             acc.require_no_error(ret, "error iterating deal states");
@@ -224,7 +217,6 @@ pub fn check_state_invariants<BS: Blockstore>(
 
     // pending proposals
     let mut pending_proposal_count = 0;
-    // XXX this is invalid, proposals are AMT not HAMT
     match make_map_with_root_and_bitwidth::<_, ()>(
         &state.pending_proposals,
         store,
@@ -330,7 +322,6 @@ pub fn check_state_invariants<BS: Blockstore>(
             lock_table_count,
             deal_op_epoch_count,
             deal_op_count,
-            claim_id_to_deal_id,
             alloc_id_to_deal_id,
         },
         acc,

actors/market/tests/batch_activate_deals.rs

@@ -1,6 +1,5 @@
 use fil_actor_market::{
     BatchActivateDealsParams, BatchActivateDealsResult, DealMetaArray, Method, SectorDeals, State,
-    NO_ALLOCATION_ID,
 };
 use fil_actors_runtime::runtime::builtins::Type;
 use fil_actors_runtime::test_utils::{expect_abort, ACCOUNT_ACTOR_CODE_ID};
@@ -53,11 +52,6 @@ fn activate_deals_one_sector() {
         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);
 }
@@ -108,12 +102,6 @@ fn activate_deals_across_multiple_sectors() {
     let verified_deal_2 = get_deal_state(&rt, verified_deal_2_id);
     let unverified_deal_2 = get_deal_state(&rt, unverified_deal_2_id);
 
-    // allocations were claimed successfully
-    assert_eq!(next_allocation_id, verified_deal_1.verified_claim);
-    assert_eq!(next_allocation_id + 1, verified_deal_2.verified_claim);
-    assert_eq!(NO_ALLOCATION_ID, unverified_deal_1.verified_claim);
-    assert_eq!(NO_ALLOCATION_ID, unverified_deal_2.verified_claim);
-
     // all activated during same epoch
     assert_eq!(0, verified_deal_1.sector_start_epoch);
     assert_eq!(0, verified_deal_2.sector_start_epoch);

actors/market/tests/market_actor_test.rs

@@ -7,8 +7,8 @@ use fil_actor_market::{
     ext, next_update_epoch, Actor as MarketActor, BatchActivateDealsResult, ClientDealProposal,
     DealArray, DealMetaArray, Label, MarketNotifyDealParams, Method, PendingDealAllocationsMap,
     PublishStorageDealsParams, PublishStorageDealsReturn, SectorDeals, State,
-    WithdrawBalanceParams, EX_DEAL_EXPIRED, MARKET_NOTIFY_DEAL_METHOD, NO_ALLOCATION_ID,
-    PENDING_ALLOCATIONS_CONFIG, PROPOSALS_AMT_BITWIDTH, STATES_AMT_BITWIDTH,
+    WithdrawBalanceParams, EX_DEAL_EXPIRED, MARKET_NOTIFY_DEAL_METHOD, PENDING_ALLOCATIONS_CONFIG,
+    PROPOSALS_AMT_BITWIDTH, STATES_AMT_BITWIDTH,
 };
 use fil_actors_runtime::cbor::{deserialize, serialize};
 use fil_actors_runtime::network::EPOCHS_IN_DAY;
@@ -704,11 +704,9 @@ fn simple_deal() {
     activate_deals(&rt, end_epoch + 1, PROVIDER_ADDR, publish_epoch, 1, &[deal1_id, deal2_id]);
     let deal1st = get_deal_state(&rt, deal1_id);
     assert_eq!(publish_epoch, deal1st.sector_start_epoch);
-    assert_eq!(NO_ALLOCATION_ID, deal1st.verified_claim);
 
     let deal2st = get_deal_state(&rt, deal2_id);
     assert_eq!(publish_epoch, deal2st.sector_start_epoch);
-    assert_eq!(next_allocation_id, deal2st.verified_claim);
 
     check_state(&rt);
 }

actors/market/tests/sector_content_changed.rs

@@ -86,11 +86,6 @@ fn simple_one_sector() {
         let state = get_deal_state(&rt, *id);
         assert_eq!(sno, 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);
 }

actors/miner/src/testing.rs

@@ -1,7 +1,4 @@
-use crate::{
-    power_for_sectors, BitFieldQueue, Deadline, ExpirationQueue, MinerInfo, Partition, PowerPair,
-    SectorOnChainInfo, SectorPreCommitOnChainInfo, Sectors, State,
-};
+use crate::{power_for_sectors, BitFieldQueue, Deadline, ExpirationQueue, MinerInfo, Partition, PowerPair, SectorOnChainInfo, SectorPreCommitOnChainInfo, Sectors, State, SectorOnChainInfoFlags};
 use fil_actors_runtime::runtime::Policy;
 use fil_actors_runtime::{
     parse_uint_key, DealWeight, Map, MessageAccumulator, DEFAULT_HAMT_CONFIG,
@@ -79,11 +76,12 @@ pub fn check_state_invariants<BS: Blockstore>(
                 if !sector.deal_weight.is_zero() || !sector.verified_deal_weight.is_zero() {
                     miner_summary.live_data_sectors.insert(
                         sector_number,
-                        DealSummary {
+                        DataSummary {
                             sector_start: sector.activation,
                             sector_expiration: sector.expiration,
                             deal_weight: sector.deal_weight.clone(),
                             verified_deal_weight: sector.verified_deal_weight.clone(),
+                            legacy_qap: !sector.flags.contains(SectorOnChainInfoFlags::SIMPLE_QA_POWER),
                         },
                     );
                 }
@@ -142,11 +140,12 @@ pub fn check_state_invariants<BS: Blockstore>(
     (miner_summary, acc)
 }
 
-pub struct DealSummary {
+pub struct DataSummary {
     pub sector_start: ChainEpoch,
     pub sector_expiration: ChainEpoch,
     pub deal_weight: DealWeight,
     pub verified_deal_weight: DealWeight,
+    pub legacy_qap: bool,
 }
 
 pub struct StateSummary {
@@ -156,7 +155,7 @@ pub struct StateSummary {
     pub window_post_proof_type: RegisteredPoStProof,
     pub deadline_cron_active: bool,
     // sectors with non zero (verified) deal weight that may carry deals
-    pub live_data_sectors: BTreeMap<SectorNumber, DealSummary>,
+    pub live_data_sectors: BTreeMap<SectorNumber, DataSummary>,
 }
 
 impl Default for StateSummary {

actors/miner/src/types.rs

@@ -193,6 +193,7 @@ pub struct DataActivationNotification {
 }
 
 #[derive(Clone, Debug, Eq, PartialEq, Serialize_tuple, Deserialize_tuple)]
+#[serde(transparent)]
 pub struct ProveCommitSectors2Return {
     pub activation_results: BatchReturn,
 }

integration_tests/src/tests/batch_onboarding_deals_test.rs

@@ -7,7 +7,6 @@ use fil_actor_miner::{Method as MinerMethod, ProveCommitAggregateParams};
 use fil_actors_runtime::runtime::policy::policy_constants::PRE_COMMIT_CHALLENGE_DELAY;
 use fil_actors_runtime::runtime::Policy;
 use fil_actors_runtime::test_utils::make_piece_cid;
-use fil_actors_runtime::STORAGE_MARKET_ACTOR_ADDR;
 use fvm_shared::address::Address;
 use fvm_shared::bigint::BigInt;
 use fvm_shared::clock::ChainEpoch;
@@ -25,9 +24,9 @@ use vm_api::VM;
 use crate::deals::{DealBatcher, DealOptions};
 use crate::util::{
     advance_to_proving_deadline, bf_all, create_accounts, create_miner, get_network_stats,
-    make_bitfield, market_add_balance, miner_balance, precommit_meta_data_from_deals,
-    precommit_sectors_v2, precommit_sectors_v2_expect_code, submit_windowed_post,
-    verifreg_add_client, verifreg_add_verifier, PrecommitMetadata,
+    make_bitfield, market_add_balance, market_pending_deal_allocations, miner_balance,
+    precommit_meta_data_from_deals, precommit_sectors_v2, precommit_sectors_v2_expect_code,
+    submit_windowed_post, verifreg_add_client, verifreg_add_verifier, PrecommitMetadata,
 };
 
 const BATCH_SIZE: usize = 8;
@@ -137,12 +136,8 @@ pub fn batch_onboarding_deals_test(v: &dyn VM) {
     assert_eq!(BATCH_SIZE, deals.len());
 
     // Verify datacap allocations.
-    let mut market_state: fil_actor_market::State =
-        get_state(v, &STORAGE_MARKET_ACTOR_ADDR).unwrap();
     let deal_keys: Vec<DealID> = deals.iter().map(|(id, _)| *id).collect();
-    let alloc_ids = market_state
-        .get_pending_deal_allocation_ids(&DynBlockstore::wrap(v.blockstore()), &deal_keys)
-        .unwrap();
+    let alloc_ids = market_pending_deal_allocations(v, &deal_keys);
     assert_eq!(BATCH_SIZE, alloc_ids.len());
 
     // Associate deals with sectors.

integration_tests/src/tests/extend_sectors_test.rs

@@ -7,7 +7,6 @@ use fvm_shared::piece::PaddedPieceSize;
 use fvm_shared::sector::{RegisteredSealProof, SectorNumber, StoragePower};
 
 use export_macro::vm_test;
-use fil_actor_market::{DealMetaArray, State as MarketState};
 use fil_actor_miner::{
     max_prove_commit_duration, power_for_sector, ExpirationExtension, ExpirationExtension2,
     ExtendSectorExpiration2Params, ExtendSectorExpirationParams, Method as MinerMethod, PowerPair,
@@ -17,9 +16,7 @@ use fil_actor_miner::{
 use fil_actor_verifreg::Method as VerifregMethod;
 use fil_actors_runtime::runtime::Policy;
 use fil_actors_runtime::test_utils::make_sealed_cid;
-use fil_actors_runtime::{
-    DealWeight, EPOCHS_IN_DAY, STORAGE_MARKET_ACTOR_ADDR, VERIFIED_REGISTRY_ACTOR_ADDR,
-};
+use fil_actors_runtime::{DealWeight, EPOCHS_IN_DAY, VERIFIED_REGISTRY_ACTOR_ADDR};
 use vm_api::trace::ExpectInvocation;
 use vm_api::util::{apply_ok, get_state, mutate_state, DynBlockstore};
 use vm_api::VM;
@@ -29,9 +26,9 @@ use crate::util::{
     advance_by_deadline_to_epoch, advance_by_deadline_to_epoch_while_proving,
     advance_by_deadline_to_index, advance_to_proving_deadline, bf_all, create_accounts,
     create_miner, cron_tick, expect_invariants, invariant_failure_patterns, market_add_balance,
-    market_publish_deal, miner_precommit_one_sector_v2, miner_prove_sector,
-    precommit_meta_data_from_deals, sector_deadline, submit_windowed_post, verifreg_add_client,
-    verifreg_add_verifier, PrecommitMetadata,
+    market_pending_deal_allocations, market_publish_deal, miner_precommit_one_sector_v2,
+    miner_prove_sector, precommit_meta_data_from_deals, sector_deadline, submit_windowed_post,
+    verifreg_add_client, verifreg_add_verifier, PrecommitMetadata,
 };
 
 #[allow(clippy::too_many_arguments)]
@@ -626,6 +623,8 @@ pub fn extend_updated_sector_with_claims_test(v: &dyn VM) {
     )
     .ids;
 
+    let claim_id = market_pending_deal_allocations(v, &deal_ids)[0];
+
     // replica update
     let new_sealed_cid = make_sealed_cid(b"replica1");
 
@@ -660,7 +659,7 @@ pub fn extend_updated_sector_with_claims_test(v: &dyn VM) {
         subinvocs: Some(vec![
             Expect::market_activate_deals(
                 miner_id,
-                deal_ids.clone(),
+                deal_ids,
                 sector_number,
                 initial_sector_info.expiration,
                 initial_sector_info.seal_proof,
@@ -718,12 +717,6 @@ pub fn extend_updated_sector_with_claims_test(v: &dyn VM) {
     let curr_epoch = v.epoch();
     v.set_epoch(curr_epoch + 1);
 
-    let market_state: MarketState = get_state(v, &STORAGE_MARKET_ACTOR_ADDR).unwrap();
-    let blockstore = DynBlockstore::wrap(v.blockstore());
-    let deal_states = DealMetaArray::load(&market_state.states, &blockstore).unwrap();
-    let deal_state = deal_states.get(deal_ids[0]).unwrap().unwrap();
-    let claim_id = deal_state.verified_claim;
-
     let extension_params = ExtendSectorExpiration2Params {
         extensions: vec![ExpirationExtension2 {
             deadline: d_idx,

integration_tests/src/tests/replica_update_test.rs

@@ -282,12 +282,12 @@ pub fn prove_replica_update_multi_dline_test(v: &dyn VM) {
     let new_sector_info_p1 = sector_info(v, &maddr, first_sector_number_p1);
     let duration = new_sector_info_p1.expiration - new_sector_info_p1.power_base_epoch;
     let deal_weights1 = get_deal_weights(v, deal_ids[0], duration);
-    let deal_weights2 = get_deal_weights(v, deal_ids[1], duration);
     assert_eq!(deal_weights1.0, new_sector_info_p1.deal_weight);
     assert_eq!(deal_weights1.1, new_sector_info_p1.verified_deal_weight);
     assert_eq!(old_sector_commr_p1, new_sector_info_p1.sector_key_cid.unwrap());
     assert_eq!(new_sealed_cid1, new_sector_info_p1.sealed_cid);
     let new_sector_info_p2 = sector_info(v, &maddr, first_sector_number_p2);
+    let deal_weights2 = get_deal_weights(v, deal_ids[1], duration);
     assert_eq!(deal_weights2.0, new_sector_info_p2.deal_weight);
     assert_eq!(deal_weights2.1, new_sector_info_p2.verified_deal_weight);
     assert_eq!(old_sector_commr_p2, new_sector_info_p2.sector_key_cid.unwrap());

integration_tests/src/tests/verified_claim_test.rs

@@ -34,10 +34,11 @@ use crate::util::{
     advance_by_deadline_to_epoch, advance_by_deadline_to_epoch_while_proving,
     advance_by_deadline_to_index, advance_to_proving_deadline, assert_invariants, create_accounts,
     create_miner, cron_tick, datacap_extend_claim, datacap_get_balance, expect_invariants,
-    invariant_failure_patterns, market_add_balance, market_publish_deal,
-    miner_extend_sector_expiration2, miner_precommit_one_sector_v2, miner_prove_sector,
-    precommit_meta_data_from_deals, sector_deadline, submit_windowed_post, verifreg_add_client,
-    verifreg_add_verifier, verifreg_extend_claim_terms, verifreg_remove_expired_allocations,
+    invariant_failure_patterns, market_add_balance, market_pending_deal_allocations,
+    market_publish_deal, miner_extend_sector_expiration2, miner_precommit_one_sector_v2,
+    miner_prove_sector, precommit_meta_data_from_deals, sector_deadline, submit_windowed_post,
+    verifreg_add_client, verifreg_add_verifier, verifreg_extend_claim_terms,
+    verifreg_remove_expired_allocations,
 };
 
 /// Tests a scenario involving a verified deal from the built-in market, with associated
@@ -91,6 +92,8 @@ pub fn verified_claim_scenario_test(v: &dyn VM) {
     )
     .ids;
 
+    let claim_id = market_pending_deal_allocations(v, &deals)[0];
+
     // Precommit and prove the sector for the max term allowed by the deal.
     let sector_term = deal_term_min + MARKET_DEFAULT_ALLOCATION_TERM_BUFFER;
     let _precommit = miner_precommit_one_sector_v2(
@@ -99,7 +102,7 @@ pub fn verified_claim_scenario_test(v: &dyn VM) {
         &miner_id,
         seal_proof,
         sector_number,
-        precommit_meta_data_from_deals(v, deals.clone(), seal_proof),
+        precommit_meta_data_from_deals(v, deals, seal_proof),
         true,
         deal_start + sector_term,
     );
@@ -122,14 +125,6 @@ pub fn verified_claim_scenario_test(v: &dyn VM) {
     let verified_weight = DealWeight::from(sector_term as u64 * deal_size);
     assert_eq!(verified_weight, sector_info.verified_deal_weight);
 
-    // Verify deal state.
-    let market_state: MarketState = get_state(v, &STORAGE_MARKET_ACTOR_ADDR).unwrap();
-    let store = DynBlockstore::wrap(v.blockstore());
-    let deal_states = DealMetaArray::load(&market_state.states, &store).unwrap();
-    let deal_state = deal_states.get(deals[0]).unwrap().unwrap();
-    let claim_id = deal_state.verified_claim;
-    assert_ne!(0, claim_id);
-
     // Verify datacap state
     let datacap_state: DatacapState = get_state(v, &DATACAP_TOKEN_ACTOR_ADDR).unwrap();
     assert_eq!(