diff --git a/actors/market/src/lib.rs b/actors/market/src/lib.rs index 5837cddad..d93902a9d 100644 --- a/actors/market/src/lib.rs +++ b/actors/market/src/lib.rs @@ -545,7 +545,7 @@ impl Actor { let (activations, batch_ret) = rt.transaction(|st: &mut State, rt| { let mut deal_states: Vec<(DealID, DealState)> = vec![]; let mut batch_gen = BatchReturnGen::new(params.sectors.len()); - let mut activations: Vec = vec![]; + let mut activations: Vec = vec![]; let mut activated_deals: HashSet = HashSet::new(); for p in params.sectors { @@ -651,7 +651,7 @@ impl Actor { match update_result { Ok(_) => { - activations.push(DealActivation { + activations.push(SectorDealActivation { nonverified_deal_space: deal_spaces.deal_space, verified_infos, }); diff --git a/actors/market/src/types.rs b/actors/market/src/types.rs index 5e73e82c3..b9099ec15 100644 --- a/actors/market/src/types.rs +++ b/actors/market/src/types.rs @@ -99,6 +99,8 @@ pub struct SectorDealData { #[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, } @@ -113,7 +115,7 @@ pub struct VerifiedDealInfo { // Information about a sector-grouping of deals that have been activated. #[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone, Eq, PartialEq)] -pub struct DealActivation { +pub struct SectorDealActivation { /// The total size of the non-verified deals activated. #[serde(with = "bigint_ser")] pub nonverified_deal_space: BigInt, @@ -126,7 +128,7 @@ pub struct BatchActivateDealsResult { /// Status of each sector grouping of deals. pub activation_results: BatchReturn, /// Activation information for the sector groups that were activated. - pub activations: Vec, + pub activations: Vec, } #[derive(Serialize_tuple, Deserialize_tuple, Debug, Clone, Eq, PartialEq)] diff --git a/actors/miner/src/ext.rs b/actors/miner/src/ext.rs index 2ee1ac30d..04d6f5815 100644 --- a/actors/miner/src/ext.rs +++ b/actors/miner/src/ext.rs @@ -202,31 +202,36 @@ pub mod verifreg { } #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] - pub struct SectorAllocationClaim { + pub struct SectorAllocationClaims { + pub sector: SectorNumber, + pub expiry: ChainEpoch, + pub claims: Vec, + } + + #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] + pub struct AllocationClaim { pub client: ActorID, pub allocation_id: AllocationID, pub data: Cid, pub size: PaddedPieceSize, - pub sector: SectorNumber, - pub sector_expiry: ChainEpoch, } #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] pub struct ClaimAllocationsParams { - pub allocations: Vec, + pub sectors: Vec, pub all_or_nothing: bool, } #[derive(Clone, Debug, PartialEq, Eq, Default, Serialize_tuple, Deserialize_tuple)] #[serde(transparent)] - pub struct SectorAllocationClaimResult { + pub struct SectorClaimSummary { #[serde(with = "bigint_ser")] pub claimed_space: BigInt, } #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] pub struct ClaimAllocationsReturn { - pub claim_results: BatchReturn, - pub claims: Vec, + pub sector_results: BatchReturn, + pub sector_claims: Vec, } } diff --git a/actors/miner/src/lib.rs b/actors/miner/src/lib.rs index d52a9c387..24518ced8 100644 --- a/actors/miner/src/lib.rs +++ b/actors/miner/src/lib.rs @@ -5010,32 +5010,39 @@ fn batch_activate_deals_and_claim_allocations( for (activation_info, activate_res) in successful_activation_infos.iter().zip(&batch_activation_res.activations) { - let mut sector_claims = activate_res + let sector_claims = activate_res .verified_infos .iter() - .map(|info| ext::verifreg::SectorAllocationClaim { + .map(|info| ext::verifreg::AllocationClaim { client: info.client, allocation_id: info.allocation_id, data: info.data, size: info.size, - sector: activation_info.sector_number, - sector_expiry: activation_info.sector_expiry, }) .collect(); - verified_claims.append(&mut sector_claims); + verified_claims.push(ext::verifreg::SectorAllocationClaims { + sector: activation_info.sector_number, + expiry: activation_info.sector_expiry, + claims: sector_claims, + }); } - let claim_res = match verified_claims.is_empty() { + let claim_res = match verified_claims.iter().all(|sector| sector.claims.is_empty()) { + // Short-circuit the call if there are no claims, + // but otherwise send a group for each sector (even if empty) to ease association of results. true => ext::verifreg::ClaimAllocationsReturn { - claim_results: BatchReturn::empty(), - claims: Vec::default(), + sector_results: BatchReturn::ok(verified_claims.len() as u32), + sector_claims: vec![ + ext::verifreg::SectorClaimSummary { claimed_space: BigInt::zero() }; + verified_claims.len() + ], }, false => { let claim_raw = extract_send_result(rt.send_simple( &VERIFIED_REGISTRY_ACTOR_ADDR, ext::verifreg::CLAIM_ALLOCATIONS_METHOD, IpldBlock::serialize_cbor(&ext::verifreg::ClaimAllocationsParams { - allocations: verified_claims, + sectors: verified_claims, all_or_nothing: true, })?, TokenAmount::zero(), @@ -5048,29 +5055,19 @@ fn batch_activate_deals_and_claim_allocations( }; assert!( - claim_res.claim_results.all_ok() || claim_res.claim_results.success_count == 0, + claim_res.sector_results.all_ok() || claim_res.sector_results.success_count == 0, "batch return of claim allocations partially succeeded but request was all_or_nothing {:?}", claim_res ); - let mut claims = claim_res.claims.into_iter(); // reassociate the verified claims with corresponding DealActivation information - let activation_and_claim_results: Vec = batch_activation_res + let activation_and_claim_results = batch_activation_res .activations .iter() - .map(|deal_activation| { - // each activation contributed as many claims as verified_info entries it had - let number_of_claims = deal_activation.verified_infos.len(); - // we consume these claims from the iterator, then combine the claims into one - // value per DealActivation - let verified_deal_space = claims - .by_ref() - .take(number_of_claims) - .fold(BigInt::zero(), |acc, claim| acc + claim.claimed_space); - ext::market::DealSpaces { - verified_deal_space, - deal_space: deal_activation.nonverified_deal_space.clone(), - } + .zip(claim_res.sector_claims) + .map(|(sector_deals, sector_claim)| ext::market::DealSpaces { + verified_deal_space: sector_claim.claimed_space, + deal_space: sector_deals.nonverified_deal_space.clone(), }) .collect(); diff --git a/actors/miner/tests/util.rs b/actors/miner/tests/util.rs index 5482d5dae..f43a7896e 100644 --- a/actors/miner/tests/util.rs +++ b/actors/miner/tests/util.rs @@ -2,19 +2,16 @@ use fil_actor_account::Method as AccountMethod; use fil_actor_market::{ - BatchActivateDealsParams, BatchActivateDealsResult, DealActivation, DealSpaces, - Method as MarketMethod, OnMinerSectorsTerminateParams, SectorDealData, SectorDeals, + BatchActivateDealsParams, BatchActivateDealsResult, DealSpaces, Method as MarketMethod, + OnMinerSectorsTerminateParams, SectorDealActivation, SectorDealData, SectorDeals, VerifiedDealInfo, VerifyDealsForActivationParams, VerifyDealsForActivationReturn, }; use fil_actor_miner::ext::market::ON_MINER_SECTORS_TERMINATE_METHOD; use fil_actor_miner::ext::power::{UPDATE_CLAIMED_POWER_METHOD, UPDATE_PLEDGE_TOTAL_METHOD}; -use fil_actor_miner::ext::verifreg::{ - ClaimAllocationsParams, ClaimAllocationsReturn, SectorAllocationClaim, - SectorAllocationClaimResult, CLAIM_ALLOCATIONS_METHOD, -}; +use fil_actor_miner::ext::verifreg::CLAIM_ALLOCATIONS_METHOD; use fil_actor_miner::{ aggregate_pre_commit_network_fee, aggregate_prove_commit_network_fee, consensus_fault_penalty, - initial_pledge_for_power, locked_reward_from_reward, max_prove_commit_duration, + ext, initial_pledge_for_power, locked_reward_from_reward, max_prove_commit_duration, new_deadline_info_from_offset_and_epoch, pledge_penalty_for_continued_fault, power_for_sectors, qa_power_for_sector, qa_power_for_weight, reward_for_consensus_slash_report, ActiveBeneficiary, Actor, ApplyRewardParams, BeneficiaryTerm, BitFieldQueue, ChangeBeneficiaryParams, @@ -1056,11 +1053,11 @@ impl ActorHarness { let mut valid_pcs = Vec::new(); // claim FIL+ allocations - let mut sectors_claims: Vec = Vec::new(); + let mut sectors_claims: Vec = Vec::new(); // build expectations per sector let mut sector_activation_params: Vec = Vec::new(); - let mut sector_activations: Vec = Vec::new(); + let mut sector_activations: Vec = Vec::new(); let mut sector_activation_results = BatchReturnGen::new(pcs.len()); for pc in pcs { @@ -1073,7 +1070,7 @@ impl ActorHarness { }; sector_activation_params.push(activate_params); - let ret = DealActivation { + let ret = SectorDealActivation { nonverified_deal_space: deal_spaces.deal_space, verified_infos: cfg .verified_deal_infos @@ -1094,25 +1091,23 @@ impl ActorHarness { } } - if ret.verified_infos.is_empty() { - if activate_deals_exit == ExitCode::OK { - valid_pcs.push(pc); - } - } else { - let mut sector_claims: Vec = ret + if activate_deals_exit == ExitCode::OK { + valid_pcs.push(pc); + let sector_claims = ret .verified_infos .iter() - .map(|info| SectorAllocationClaim { + .map(|info| ext::verifreg::AllocationClaim { client: info.client, allocation_id: info.allocation_id, data: info.data, size: info.size, - sector: pc.info.sector_number, - sector_expiry: pc.info.expiration, }) .collect(); - sectors_claims.append(&mut sector_claims); - valid_pcs.push(pc); + sectors_claims.push(ext::verifreg::SectorAllocationClaims { + sector: pc.info.sector_number, + expiry: pc.info.expiration, + claims: sector_claims, + }); } } else { // empty deal ids @@ -1121,11 +1116,16 @@ impl ActorHarness { sector_expiry: pc.info.expiration, sector_type: RegisteredSealProof::StackedDRG8MiBV1, }); - sector_activations.push(DealActivation { + sector_activations.push(SectorDealActivation { nonverified_deal_space: BigInt::zero(), verified_infos: vec![], }); sector_activation_results.add_success(); + sectors_claims.push(ext::verifreg::SectorAllocationClaims { + sector: pc.info.sector_number, + expiry: pc.info.expiration, + claims: vec![], + }); valid_pcs.push(pc); } } @@ -1148,20 +1148,24 @@ impl ActorHarness { ); } - if !sectors_claims.is_empty() { - let claim_allocation_params = ClaimAllocationsParams { - allocations: sectors_claims.clone(), + if !sectors_claims.iter().all(|c| c.claims.is_empty()) { + let claim_allocation_params = ext::verifreg::ClaimAllocationsParams { + sectors: sectors_claims.clone(), all_or_nothing: true, }; // TODO handle failures of claim allocations // use exit code map for claim allocations in config - let claim_allocs_ret = ClaimAllocationsReturn { - claims: sectors_claims + let claim_allocs_ret = ext::verifreg::ClaimAllocationsReturn { + sector_results: BatchReturn::ok(sectors_claims.len() as u32), + sector_claims: sectors_claims .iter() - .map(|claim| SectorAllocationClaimResult { claimed_space: claim.size.0.into() }) + .map(|sector| ext::verifreg::SectorClaimSummary { + claimed_space: BigInt::from( + sector.claims.iter().map(|c| c.size.0).sum::(), + ), + }) .collect(), - claim_results: BatchReturn::ok(sectors_claims.len() as u32), }; rt.expect_send_simple( VERIFIED_REGISTRY_ACTOR_ADDR, diff --git a/actors/verifreg/src/lib.rs b/actors/verifreg/src/lib.rs index c79c96f17..e86299e02 100644 --- a/actors/verifreg/src/lib.rs +++ b/actors/verifreg/src/lib.rs @@ -366,108 +366,109 @@ impl Actor { /// Called by storage provider actor to claim allocations for data provably committed to storage. /// For each allocation claim, the registry checks that the provided piece CID /// and size match that of the allocation. - /// Returns an indicator of success for each claim, and the size of each successful claim. - /// When `all_or_nothing` is enabled, any failure to claim results in the entire - /// method returning an error. + /// Claims are processed in groups by sector. A failed claim will cause the + /// others in its group to fail too, unless `all_or_nothing` is enabled, in which case + /// the method will abort. + /// Returns an indicator of success for each sector group, and the size of claimed space. pub fn claim_allocations( rt: &impl Runtime, params: ClaimAllocationsParams, ) -> Result { rt.validate_immediate_caller_type(std::iter::once(&Type::Miner))?; let provider = rt.message().caller().id().unwrap(); - if params.allocations.is_empty() { + if params.sectors.is_empty() { return Err(actor_error!(illegal_argument, "claim allocations called with no claims")); } - let mut total_datacap_claimed = DataCap::zero(); - let mut sector_claims = Vec::new(); - let mut ret_gen = BatchReturnGen::new(params.allocations.len()); + let mut batch_gen = BatchReturnGen::new(params.sectors.len()); + let mut sector_results: Vec = vec![]; + let mut total_claimed_space = DataCap::zero(); + rt.transaction(|st: &mut State, rt| { let mut claims = st.load_claims(rt.store())?; let mut allocs = st.load_allocs(rt.store())?; - for claim_alloc in params.allocations { - let maybe_alloc = state::get_allocation( - &mut allocs, - claim_alloc.client, - claim_alloc.allocation_id, - )?; - let alloc: &Allocation = match maybe_alloc { - None => { - ret_gen.add_fail(ExitCode::USR_NOT_FOUND); - info!( - "no allocation {} for client {}", - claim_alloc.allocation_id, claim_alloc.client, - ); - continue; + // Note: this doesn't prevent being called with the same sector number twice. + 'sectors: for sector in params.sectors { + // Load and validate all allocations for the sector group before + // making any state changes. + // Errors cause the sector to be skipped, unless all-or-nothing is requested. + let mut sector_new_claims: Vec<(ClaimID, Claim)> = vec![]; + for claim in sector.claims { + let maybe_alloc = + state::get_allocation(&mut allocs, claim.client, claim.allocation_id)?; + if let Some(alloc) = maybe_alloc { + if !can_claim_alloc(&claim, provider, alloc, rt.curr_epoch(), sector.expiry) + { + info!( + "failed to claim allocation {} in sector {} expiry {}", + claim.allocation_id, sector.sector, sector.expiry + ); + batch_gen.add_fail(ExitCode::USR_FORBIDDEN); + continue 'sectors; + } + sector_new_claims.push(( + claim.allocation_id, + Claim { + provider, + client: alloc.client, + data: alloc.data, + size: alloc.size, + term_min: alloc.term_min, + term_max: alloc.term_max, + term_start: rt.curr_epoch(), + sector: sector.sector, + }, + )); + } else { + info!("no allocation {} for client {}", claim.allocation_id, claim.client); + batch_gen.add_fail(ExitCode::USR_NOT_FOUND); + continue 'sectors; } - Some(a) => a, - }; - - if !can_claim_alloc(&claim_alloc, provider, alloc, rt.curr_epoch()) { - ret_gen.add_fail(ExitCode::USR_FORBIDDEN); - info!( - "invalid sector {:?} for allocation {}", - claim_alloc.sector, claim_alloc.allocation_id, - ); - continue; } - let new_claim = Claim { - provider, - client: alloc.client, - data: alloc.data, - size: alloc.size, - term_min: alloc.term_min, - term_max: alloc.term_max, - term_start: rt.curr_epoch(), - sector: claim_alloc.sector, - }; - - let inserted = claims - .put_if_absent(provider, claim_alloc.allocation_id, new_claim) - .context_code( + // Update state. + // Errors from here on are unexpected, so abort. + let mut sector_claimed_space = DataCap::zero(); + for (id, new_claim) in sector_new_claims { + let inserted = + claims.put_if_absent(provider, id, new_claim.clone()).context_code( + ExitCode::USR_ILLEGAL_STATE, + format!("failed to write claim {}", id), + )?; + if !inserted { + return Err(actor_error!(illegal_argument, "claim {} already exists", id)); + } + allocs.remove(new_claim.client, id).context_code( ExitCode::USR_ILLEGAL_STATE, - format!("failed to write claim {}", claim_alloc.allocation_id), + format!("failed to remove allocation {}", id), )?; - if !inserted { - ret_gen.add_fail(ExitCode::USR_ILLEGAL_STATE); - // should be unreachable since claim and alloc can't exist at once - info!( - "claim for allocation {} could not be inserted as it already exists", - claim_alloc.allocation_id, - ); - continue; + sector_claimed_space += DataCap::from(new_claim.size.0); } - - allocs.remove(claim_alloc.client, claim_alloc.allocation_id).context_code( - ExitCode::USR_ILLEGAL_STATE, - format!("failed to remove allocation {}", claim_alloc.allocation_id), - )?; - - total_datacap_claimed += DataCap::from(claim_alloc.size.0); - sector_claims - .push(SectorAllocationClaimResult { claimed_space: claim_alloc.size.0.into() }); - ret_gen.add_success(); + total_claimed_space += §or_claimed_space; + sector_results.push(SectorClaimSummary { claimed_space: sector_claimed_space }); + batch_gen.add_success(); } st.save_allocs(&mut allocs)?; st.save_claims(&mut claims)?; Ok(()) }) .context("state transaction failed")?; - let batch_info = ret_gen.gen(); + + let batch_info = batch_gen.gen(); if params.all_or_nothing && !batch_info.all_ok() { - return Err(actor_error!( - illegal_argument, - "all or nothing call contained failures: {}", - batch_info.to_string() + return Err(ActorError::checked( + // Returning the first actual error code from the batch might be better, but + // would change behaviour from the original implementation. + ExitCode::USR_ILLEGAL_ARGUMENT, + format!("claim failed with all-or-nothing: {}", batch_info), + None, )); } // Burn the datacap tokens from verified registry's own balance. - burn(rt, &total_datacap_claimed)?; - - Ok(ClaimAllocationsReturn { claim_results: batch_info, claims: sector_claims }) + burn(rt, &total_claimed_space)?; + Ok(ClaimAllocationsReturn { sector_results: batch_info, sector_claims: sector_results }) } // get claims for a provider @@ -1060,13 +1061,13 @@ fn check_miner_id(rt: &impl Runtime, id: ActorID) -> Result<(), ActorError> { } fn can_claim_alloc( - claim_alloc: &SectorAllocationClaim, + claim_alloc: &AllocationClaim, provider: ActorID, alloc: &Allocation, curr_epoch: ChainEpoch, + sector_expiry: ChainEpoch, ) -> bool { - let sector_lifetime = claim_alloc.sector_expiry - curr_epoch; - + let sector_lifetime = sector_expiry - curr_epoch; provider == alloc.provider && claim_alloc.client == alloc.client && claim_alloc.data == alloc.data diff --git a/actors/verifreg/src/types.rs b/actors/verifreg/src/types.rs index 9ccf71c3c..425258ceb 100644 --- a/actors/verifreg/src/types.rs +++ b/actors/verifreg/src/types.rs @@ -121,35 +121,43 @@ pub struct RemoveExpiredAllocationsReturn { } #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] -pub struct SectorAllocationClaim { +pub struct SectorAllocationClaims { + pub sector: SectorNumber, + pub expiry: ChainEpoch, + pub claims: Vec, +} + +#[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] +pub struct AllocationClaim { pub client: ActorID, pub allocation_id: AllocationID, pub data: Cid, pub size: PaddedPieceSize, - pub sector: SectorNumber, - pub sector_expiry: ChainEpoch, } #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] pub struct ClaimAllocationsParams { - pub allocations: Vec, - // Whether to abort entirely if any claim fails. + /// Allocations to claim, grouped by sector. + pub sectors: Vec, + /// Whether to abort entirely if any claim fails. + /// If false, a failed claim will cause other claims in the same sector group to also fail, + /// but allow other sectors to proceed. pub all_or_nothing: bool, } #[derive(Clone, Debug, PartialEq, Eq, Default, Serialize_tuple, Deserialize_tuple)] #[serde(transparent)] -pub struct SectorAllocationClaimResult { +pub struct SectorClaimSummary { #[serde(with = "bigint_ser")] pub claimed_space: BigInt, } #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] pub struct ClaimAllocationsReturn { - // Success/failure indication for each claim. - pub claim_results: BatchReturn, - // The claimed space for each successful claim, in order. - pub claims: Vec, + /// Status of each sector grouping of claims. + pub sector_results: BatchReturn, + /// The claimed space for each successful sector group. + pub sector_claims: Vec, } #[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] diff --git a/actors/verifreg/tests/harness/mod.rs b/actors/verifreg/tests/harness/mod.rs index 61b49e902..fa97f1baa 100644 --- a/actors/verifreg/tests/harness/mod.rs +++ b/actors/verifreg/tests/harness/mod.rs @@ -1,11 +1,11 @@ use fil_actor_verifreg::testing::check_state_invariants; use fil_actor_verifreg::{ ext, Actor as VerifregActor, AddVerifiedClientParams, AddVerifierParams, Allocation, - AllocationID, AllocationRequest, AllocationRequests, AllocationsResponse, Claim, - ClaimAllocationsParams, ClaimAllocationsReturn, ClaimExtensionRequest, ClaimID, DataCap, + AllocationClaim, AllocationID, AllocationRequest, AllocationRequests, AllocationsResponse, + Claim, ClaimAllocationsParams, ClaimAllocationsReturn, ClaimExtensionRequest, ClaimID, DataCap, ExtendClaimTermsParams, ExtendClaimTermsReturn, GetClaimsParams, GetClaimsReturn, Method, RemoveExpiredAllocationsParams, RemoveExpiredAllocationsReturn, RemoveExpiredClaimsParams, - RemoveExpiredClaimsReturn, SectorAllocationClaim, State, + RemoveExpiredClaimsReturn, SectorAllocationClaims, State, }; use fil_actors_runtime::cbor::serialize; use fil_actors_runtime::runtime::builtins::Type; @@ -257,7 +257,7 @@ impl Harness { &self, rt: &MockRuntime, provider: ActorID, - claim_allocs: Vec, + claim_allocs: Vec, datacap_burnt: u64, all_or_nothing: bool, ) -> Result { @@ -278,7 +278,7 @@ impl Harness { ); } - let params = ClaimAllocationsParams { allocations: claim_allocs, all_or_nothing }; + let params = ClaimAllocationsParams { sectors: claim_allocs, all_or_nothing }; let ret = rt .call::( Method::ClaimAllocations as MethodNum, @@ -500,19 +500,23 @@ pub fn alloc_from_req(client: ActorID, req: &AllocationRequest) -> Allocation { } } -pub fn make_claim_req( - id: AllocationID, - alloc: &Allocation, - sector_id: SectorNumber, - sector_expiry: ChainEpoch, -) -> SectorAllocationClaim { - SectorAllocationClaim { - client: alloc.client, - allocation_id: id, - data: alloc.data, - size: alloc.size, - sector: sector_id, - sector_expiry, +pub fn make_claim_reqs( + sector: SectorNumber, + expiry: ChainEpoch, + allocs: &[(AllocationID, &Allocation)], +) -> SectorAllocationClaims { + SectorAllocationClaims { + sector, + expiry, + claims: allocs + .iter() + .map(|(id, alloc)| AllocationClaim { + client: alloc.client, + allocation_id: *id, + data: alloc.data, + size: alloc.size, + }) + .collect(), } } @@ -610,6 +614,8 @@ pub fn assert_alloc_claimed( // Claim is present let expected_claim = claim_from_alloc(alloc, epoch, sector); + assert_eq!(client, expected_claim.client); // Check the caller provided sensible arguments. + assert_eq!(provider, expected_claim.provider); let mut claims = st.load_claims(store).unwrap(); assert_eq!(&expected_claim, claims.get(provider, id).unwrap().unwrap()); expected_claim diff --git a/actors/verifreg/tests/verifreg_actor_test.rs b/actors/verifreg/tests/verifreg_actor_test.rs index c2e8df8ef..087f6d8f1 100644 --- a/actors/verifreg/tests/verifreg_actor_test.rs +++ b/actors/verifreg/tests/verifreg_actor_test.rs @@ -15,10 +15,8 @@ lazy_static! { } mod util { - use fil_actor_verifreg::ClaimAllocationsReturn; use fil_actors_runtime::test_utils::MockRuntime; - use fvm_shared::{bigint::BigInt, sector::StoragePower}; - use num_traits::Zero; + use fvm_shared::sector::StoragePower; pub fn verifier_allowance(rt: &MockRuntime) -> StoragePower { rt.policy.minimum_verified_allocation_size.clone() + 42 @@ -27,17 +25,10 @@ mod util { pub fn client_allowance(rt: &MockRuntime) -> StoragePower { verifier_allowance(rt) - 1 } - - // Gets the total claimed_power_across sectors for a claim_allocation - pub fn total_claimed_space(claim_allocations_ret: &ClaimAllocationsReturn) -> BigInt { - claim_allocations_ret - .claims - .iter() - .fold(BigInt::zero(), |acc, claim_result| acc + claim_result.claimed_space.clone()) - } } mod construction { + use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::address::{Address, BLS_PUB_LEN}; use fvm_shared::error::ExitCode; use fvm_shared::MethodNum; @@ -45,7 +36,6 @@ mod construction { use fil_actor_verifreg::{Actor as VerifregActor, Method}; use fil_actors_runtime::test_utils::*; use fil_actors_runtime::SYSTEM_ACTOR_ADDR; - use fvm_ipld_encoding::ipld_block::IpldBlock; use harness::*; use crate::*; @@ -86,15 +76,16 @@ mod construction { } mod verifiers { + use std::ops::Deref; + + use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::address::{Address, BLS_PUB_LEN}; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; use fvm_shared::{MethodNum, METHOD_SEND}; - use std::ops::Deref; use fil_actor_verifreg::{Actor as VerifregActor, AddVerifierParams, DataCap, Method}; use fil_actors_runtime::test_utils::*; - use fvm_ipld_encoding::ipld_block::IpldBlock; use harness::*; use util::*; @@ -246,10 +237,12 @@ mod verifiers { } mod clients { + use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::address::{Address, BLS_PUB_LEN}; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; use fvm_shared::{MethodNum, METHOD_SEND}; + use num_traits::ToPrimitive; use num_traits::Zero; use fil_actor_verifreg::{ @@ -257,10 +250,7 @@ mod clients { }; use fil_actors_runtime::test_utils::*; use fil_actors_runtime::{DATACAP_TOKEN_ACTOR_ADDR, STORAGE_MARKET_ACTOR_ADDR}; - - use fvm_ipld_encoding::ipld_block::IpldBlock; use harness::*; - use num_traits::ToPrimitive; use util::*; use crate::*; @@ -501,6 +491,8 @@ mod clients { } mod allocs_claims { + use std::str::FromStr; + use cid::Cid; use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::bigint::BigInt; @@ -508,7 +500,6 @@ mod allocs_claims { use fvm_shared::piece::PaddedPieceSize; use fvm_shared::{ActorID, MethodNum}; use num_traits::Zero; - use std::str::FromStr; use fil_actor_verifreg::{ Actor, AllocationID, ClaimTerm, DataCap, ExtendClaimTermsParams, GetClaimsParams, @@ -520,12 +511,11 @@ mod allocs_claims { MINIMUM_VERIFIED_ALLOCATION_TERM, }; use fil_actors_runtime::test_utils::{ - expect_abort_contains_message, ACCOUNT_ACTOR_CODE_ID, EVM_ACTOR_CODE_ID, + expect_abort, expect_abort_contains_message, ACCOUNT_ACTOR_CODE_ID, EVM_ACTOR_CODE_ID, }; use fil_actors_runtime::FailCode; use harness::*; - use crate::util::total_claimed_space; use crate::*; const CLIENT1: ActorID = 101; @@ -635,11 +625,13 @@ mod allocs_claims { let size = MINIMUM_VERIFIED_ALLOCATION_SIZE as u64; let alloc1 = make_alloc("1", CLIENT1, PROVIDER1, size); let alloc2 = make_alloc("2", CLIENT2, PROVIDER1, size); // Distinct client - let alloc3 = make_alloc("3", CLIENT1, PROVIDER2, size); // Distinct provider + let alloc3 = make_alloc("3", CLIENT1, PROVIDER1, size); + let alloc4 = make_alloc("4", CLIENT1, PROVIDER2, size); // Distinct provider - h.create_alloc(&rt, &alloc1).unwrap(); - h.create_alloc(&rt, &alloc2).unwrap(); - h.create_alloc(&rt, &alloc3).unwrap(); + let id1 = h.create_alloc(&rt, &alloc1).unwrap(); + let id2 = h.create_alloc(&rt, &alloc2).unwrap(); + let id3 = h.create_alloc(&rt, &alloc3).unwrap(); + let id4 = h.create_alloc(&rt, &alloc4).unwrap(); h.check_state(&rt); let sector = 1000; @@ -647,31 +639,29 @@ mod allocs_claims { let prior_state: State = rt.get_state(); { - // Claim two for PROVIDER1 - let reqs = vec![ - make_claim_req(1, &alloc1, sector, expiry), - make_claim_req(2, &alloc2, sector, expiry), - ]; + // Claim two for PROVIDER1 in one sector + let reqs = vec![make_claim_reqs(sector, expiry, &[(id1, &alloc1), (id2, &alloc2)])]; let ret = h.claim_allocations(&rt, PROVIDER1, reqs, size * 2, false).unwrap(); - assert_eq!(ret.claim_results.codes(), vec![ExitCode::OK, ExitCode::OK]); - assert_eq!(total_claimed_space(&ret), BigInt::from(2 * size)); - assert_alloc_claimed(&rt, CLIENT1, PROVIDER1, 1, &alloc1, 0, sector); - assert_alloc_claimed(&rt, CLIENT2, PROVIDER1, 2, &alloc2, 0, sector); + assert_eq!(ret.sector_results.codes(), vec![ExitCode::OK]); + assert_eq!(ret.sector_claims[0].claimed_space, BigInt::from(2 * size)); + assert_alloc_claimed(&rt, CLIENT1, PROVIDER1, id1, &alloc1, 0, sector); + assert_alloc_claimed(&rt, CLIENT2, PROVIDER1, id2, &alloc2, 0, sector); h.check_state(&rt); } { - // Can't find claim for wrong client + // Can't find claim for wrong client. + // Claim in another sector succeeds regardless. rt.replace_state(&prior_state); let mut reqs = vec![ - make_claim_req(1, &alloc1, sector, expiry), - make_claim_req(2, &alloc2, sector, expiry), + make_claim_reqs(sector, expiry, &[(id1, &alloc1)]), + make_claim_reqs(sector, expiry, &[(id2, &alloc2)]), ]; - reqs[1].client = CLIENT1; + reqs[1].claims[0].client = CLIENT1; let ret = h.claim_allocations(&rt, PROVIDER1, reqs, size, false).unwrap(); - assert_eq!(ret.claim_results.codes(), vec![ExitCode::OK, ExitCode::USR_NOT_FOUND]); - assert_eq!(total_claimed_space(&ret), BigInt::from(size)); - assert_alloc_claimed(&rt, CLIENT1, PROVIDER1, 1, &alloc1, 0, sector); + assert_eq!(ret.sector_results.codes(), vec![ExitCode::OK, ExitCode::USR_NOT_FOUND]); + assert_eq!(ret.sector_claims[0].claimed_space, BigInt::from(size)); + assert_alloc_claimed(&rt, CLIENT1, PROVIDER1, id1, &alloc1, 0, sector); assert_allocation(&rt, CLIENT2, 2, &alloc2); h.check_state(&rt); } @@ -679,58 +669,125 @@ mod allocs_claims { // Can't claim for other provider rt.replace_state(&prior_state); let reqs = vec![ - make_claim_req(2, &alloc2, sector, expiry), - make_claim_req(3, &alloc3, sector, expiry), // Different provider + make_claim_reqs(sector, expiry, &[(id4, &alloc4)]), // Wrong provider ]; - let ret = h.claim_allocations(&rt, PROVIDER1, reqs, size, false).unwrap(); - assert_eq!(ret.claim_results.codes(), vec![ExitCode::OK, ExitCode::USR_FORBIDDEN]); - assert_eq!(total_claimed_space(&ret), BigInt::from(size)); - assert_alloc_claimed(&rt, CLIENT1, PROVIDER1, 2, &alloc2, 0, sector); - assert_allocation(&rt, CLIENT1, 3, &alloc3); + let ret = h.claim_allocations(&rt, PROVIDER1, reqs, 0, false).unwrap(); + assert_eq!(ret.sector_results.codes(), vec![ExitCode::USR_FORBIDDEN]); + assert_eq!(ret.sector_claims.len(), 0); + assert_allocation(&rt, CLIENT1, id4, &alloc4); h.check_state(&rt); } + { + // Can't claim same alloc twice in one sector. + rt.replace_state(&prior_state); + let reqs = vec![make_claim_reqs(sector, expiry, &[(id1, &alloc1), (id1, &alloc1)])]; + expect_abort( + ExitCode::USR_ILLEGAL_ARGUMENT, + h.claim_allocations(&rt, PROVIDER1, reqs, size, false), + ); + rt.reset(); + + // Can only claim alloc once across multiple sectors. + let reqs = vec![ + make_claim_reqs(sector, expiry, &[(id1, &alloc1)]), + make_claim_reqs(sector, expiry, &[(id1, &alloc1)]), + ]; + let ret = h.claim_allocations(&rt, PROVIDER1, reqs, size, false).unwrap(); + assert_eq!(ret.sector_results.codes(), vec![ExitCode::OK, ExitCode::USR_NOT_FOUND]); + assert_eq!(ret.sector_claims[0].claimed_space, BigInt::from(size)); + assert_alloc_claimed(&rt, CLIENT1, PROVIDER1, id1, &alloc1, 0, sector); + rt.reset(); + } { // Mismatched data / size rt.replace_state(&prior_state); let mut reqs = vec![ - make_claim_req(1, &alloc1, sector, expiry), - make_claim_req(2, &alloc2, sector, expiry), + make_claim_reqs(sector, expiry, &[(id1, &alloc1)]), + make_claim_reqs(sector, expiry, &[(id2, &alloc2)]), ]; - reqs[0].data = + reqs[0].claims[0].data = Cid::from_str("bafyreibjo4xmgaevkgud7mbifn3dzp4v4lyaui4yvqp3f2bqwtxcjrdqg4") .unwrap(); - reqs[1].size = PaddedPieceSize(size + 1); + reqs[1].claims[0].size = PaddedPieceSize(size + 1); let ret = h.claim_allocations(&rt, PROVIDER1, reqs, 0, false).unwrap(); assert_eq!( - ret.claim_results.codes(), + ret.sector_results.codes(), vec![ExitCode::USR_FORBIDDEN, ExitCode::USR_FORBIDDEN] ); - assert_eq!(total_claimed_space(&ret), BigInt::zero()); + assert_eq!(ret.sector_claims.len(), 0); h.check_state(&rt); } { // Expired allocation rt.replace_state(&prior_state); - let reqs = vec![make_claim_req(1, &alloc1, sector, expiry)]; + let reqs = vec![make_claim_reqs(sector, expiry, &[(id1, &alloc1)])]; rt.set_epoch(alloc1.expiration + 1); let ret = h.claim_allocations(&rt, PROVIDER1, reqs, 0, false).unwrap(); - assert_eq!(ret.claim_results.codes(), vec![ExitCode::USR_FORBIDDEN]); - assert_eq!(total_claimed_space(&ret), BigInt::zero()); + assert_eq!(ret.sector_results.codes(), vec![ExitCode::USR_FORBIDDEN]); + assert_eq!(ret.sector_claims.len(), 0); h.check_state(&rt); + rt.set_epoch(0); } { // Sector expiration too soon rt.replace_state(&prior_state); - let reqs = vec![make_claim_req(1, &alloc1, sector, alloc1.term_min - 1)]; + let reqs = vec![make_claim_reqs(sector, alloc1.term_min - 1, &[(id1, &alloc1)])]; let ret = h.claim_allocations(&rt, PROVIDER1, reqs, 0, false).unwrap(); - assert_eq!(ret.claim_results.codes(), vec![ExitCode::USR_FORBIDDEN]); + assert_eq!(ret.sector_results.codes(), vec![ExitCode::USR_FORBIDDEN]); + assert_eq!(ret.sector_claims.len(), 0); + // Sector expiration too late - let reqs = vec![make_claim_req(1, &alloc1, sector, alloc1.term_max + 1)]; + let reqs = vec![make_claim_reqs(sector, alloc1.term_max + 1, &[(id1, &alloc1)])]; let ret = h.claim_allocations(&rt, PROVIDER1, reqs, 0, false).unwrap(); - assert_eq!(ret.claim_results.codes(), vec![ExitCode::USR_FORBIDDEN]); - assert_eq!(total_claimed_space(&ret), BigInt::zero()); + assert_eq!(ret.sector_results.codes(), vec![ExitCode::USR_FORBIDDEN]); + assert_eq!(ret.sector_claims.len(), 0); h.check_state(&rt); } + { + // Without all-or-nothing, a failure aborts the sector but not other sectors + rt.replace_state(&prior_state); + let mut reqs = vec![ + make_claim_reqs(sector, expiry, &[(id1, &alloc1), (id2, &alloc2)]), + make_claim_reqs(sector, expiry, &[(id3, &alloc3)]), + ]; + reqs[0].claims[1].size = PaddedPieceSize(0); + let ret = h.claim_allocations(&rt, PROVIDER1, reqs, size, false).unwrap(); + assert_eq!(ret.sector_results.codes(), vec![ExitCode::USR_FORBIDDEN, ExitCode::OK]); + assert_eq!(ret.sector_claims[0].claimed_space, BigInt::from(size)); + assert_allocation(&rt, CLIENT1, id1, &alloc1); + assert_allocation(&rt, CLIENT2, id2, &alloc2); + assert_alloc_claimed(&rt, CLIENT1, PROVIDER1, id3, &alloc3, 0, sector); + } + { + // Without all-or-nothing, every sector can fail but the method succeeds. + rt.replace_state(&prior_state); + let mut reqs = vec![ + make_claim_reqs(sector, expiry, &[(id1, &alloc1), (id2, &alloc2)]), + make_claim_reqs(sector, expiry, &[(id3, &alloc3)]), + ]; + reqs[0].claims[1].size = PaddedPieceSize(0); + reqs[1].claims[0].size = PaddedPieceSize(0); + let ret = h.claim_allocations(&rt, PROVIDER1, reqs, 0, false).unwrap(); + assert_eq!( + ret.sector_results.codes(), + vec![ExitCode::USR_FORBIDDEN, ExitCode::USR_FORBIDDEN] + ); + assert_eq!(ret.sector_claims.len(), 0); + } + { + // With all-or-nothing, a failure aborts everything + rt.replace_state(&prior_state); + let mut reqs = vec![ + make_claim_reqs(sector, expiry, &[(id1, &alloc1), (id2, &alloc2)]), + make_claim_reqs(sector, expiry, &[(id3, &alloc3)]), + ]; + reqs[0].claims[1].size = PaddedPieceSize(0); + expect_abort( + ExitCode::USR_ILLEGAL_ARGUMENT, + h.claim_allocations(&rt, PROVIDER1, reqs, 0, true), + ); + rt.reset(); + } } #[test] @@ -1060,6 +1117,7 @@ mod allocs_claims { mod datacap { use frc46_token::receiver::FRC46_TOKEN_TYPE; use fvm_actor_utils::receiver::UniversalReceiverParams; + use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::address::Address; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; @@ -1075,7 +1133,6 @@ mod datacap { use fil_actors_runtime::{ BatchReturn, DATACAP_TOKEN_ACTOR_ADDR, EPOCHS_IN_YEAR, STORAGE_MARKET_ACTOR_ADDR, }; - use fvm_ipld_encoding::ipld_block::IpldBlock; use harness::*; use crate::*; @@ -1118,6 +1175,18 @@ mod datacap { let st: State = rt.get_state(); assert_eq!(4, st.next_allocation_id); } + { + // Allocations can be identical and will receive distinct IDs. + let reqs = + vec![make_alloc_req(&rt, PROVIDER1, SIZE), make_alloc_req(&rt, PROVIDER1, SIZE)]; + assert_eq!(reqs[0], reqs[1]); + let payload = make_receiver_hook_token_payload(CLIENT1, reqs.clone(), vec![], SIZE * 2); + h.receive_tokens(&rt, payload, BatchReturn::ok(2), BATCH_EMPTY, vec![4, 5], 0).unwrap(); + + // Verify allocations in state. + assert_allocation(&rt, CLIENT1, 4, &alloc_from_req(CLIENT1, &reqs[0])); + assert_allocation(&rt, CLIENT1, 5, &alloc_from_req(CLIENT1, &reqs[1])); + } h.check_state(&rt); }