Enforce no agg proof type when not used

actors/miner/src/lib.rs

@@ -1015,6 +1015,33 @@ impl Actor {
         rt.validate_immediate_caller_is(
             info.control_addresses.iter().chain(&[info.worker, info.owner]),
         )?;
+        if !params.sector_proofs.is_empty() {
+            if !params.aggregate_proof.is_empty() {
+                return Err(actor_error!(
+                    illegal_argument,
+                    "exactly one of sector proofs or aggregate proof must be non-empty"
+                ));
+            }
+            if params.aggregate_proof_type.is_some() {
+                return Err(actor_error!(
+                    illegal_argument,
+                    "aggregate proof type must be empty when sector proofs are specified"
+                ));
+            }
+        } else {
+            if params.aggregate_proof.is_empty() {
+                return Err(actor_error!(
+                    illegal_argument,
+                    "exactly one of sector proofs or aggregate proof must be non-empty"
+                ));
+            }
+            if params.aggregate_proof_type.is_none() {
+                return Err(actor_error!(
+                    illegal_argument,
+                    "aggregate proof type must be specified when aggregate proof is specified"
+                ));
+            }
+        }
         if params.sector_proofs.is_empty() == params.aggregate_proof.is_empty() {
             return Err(actor_error!(
                 illegal_argument,

actors/miner/tests/prove_replica_failures_test.rs

@@ -3,6 +3,7 @@ use fvm_shared::address::Address;
 use fvm_shared::clock::ChainEpoch;
 use fvm_shared::deal::DealID;
 use fvm_shared::error::ExitCode;
+use fvm_shared::sector::RegisteredAggregateProof::SnarkPackV2;
 use fvm_shared::sector::SectorNumber;
 use fvm_shared::ActorID;
 
@@ -98,7 +99,8 @@ fn reject_aggregate_proof() {
     let cfg = ProveReplicaUpdatesConfig {
         param_twiddle: Some(Box::new(|p: &mut ProveReplicaUpdates3Params| {
             p.sector_proofs = vec![];
-            p.aggregate_proof = RawBytes::new(vec![1, 2, 3, 4])
+            p.aggregate_proof = RawBytes::new(vec![1, 2, 3, 4]);
+            p.aggregate_proof_type = Some(SnarkPackV2);
         })),
         ..Default::default()
     };

actors/miner/tests/util.rs

@@ -1,27 +1,27 @@
 #![allow(clippy::all)]
 
-use anyhow::anyhow;
 use std::collections::{BTreeMap, BTreeSet, HashMap};
 use std::convert::TryInto;
 use std::iter;
 use std::ops::Neg;
 
-use cid::multihash::MultihashDigest;
+use anyhow::anyhow;
 use cid::Cid;
-use fil_actors_runtime::test_blockstores::MemoryBlockstore;
+use cid::multihash::MultihashDigest;
 use fvm_ipld_amt::Amt;
-use fvm_ipld_bitfield::iter::Ranges;
 use fvm_ipld_bitfield::{BitField, UnvalidatedBitField, Validate};
+use fvm_ipld_bitfield::iter::Ranges;
 use fvm_ipld_blockstore::Blockstore;
+use fvm_ipld_encoding::{BytesDe, CborStore, RawBytes};
 use fvm_ipld_encoding::de::Deserialize;
 use fvm_ipld_encoding::ipld_block::IpldBlock;
 use fvm_ipld_encoding::ser::Serialize;
-use fvm_ipld_encoding::{BytesDe, CborStore, RawBytes};
+use fvm_shared::{ActorID, HAMT_BIT_WIDTH, METHOD_SEND};
 use fvm_shared::address::Address;
 use fvm_shared::bigint::BigInt;
 use fvm_shared::bigint::Zero;
-use fvm_shared::clock::QuantSpec;
 use fvm_shared::clock::{ChainEpoch, NO_QUANTIZATION};
+use fvm_shared::clock::QuantSpec;
 use fvm_shared::commcid::{FIL_COMMITMENT_SEALED, FIL_COMMITMENT_UNSEALED};
 use fvm_shared::consensus::ConsensusFault;
 use fvm_shared::crypto::hash::SupportedHashes;
@@ -37,7 +37,6 @@ use fvm_shared::sector::{
     SectorSize, StoragePower, WindowPoStVerifyInfo,
 };
 use fvm_shared::smooth::FilterEstimate;
-use fvm_shared::{ActorID, HAMT_BIT_WIDTH, METHOD_SEND};
 use itertools::Itertools;
 use lazy_static::lazy_static;
 use multihash::derive::Multihash;
@@ -46,57 +45,58 @@ use num_traits::Signed;
 use fil_actor_account::Method as AccountMethod;
 use fil_actor_market::{
     ActivatedDeal, BatchActivateDealsParams, BatchActivateDealsResult, Method as MarketMethod,
-    OnMinerSectorsTerminateParams, SectorDealActivation, SectorDeals,
-    VerifyDealsForActivationParams, VerifyDealsForActivationReturn, NO_ALLOCATION_ID,
+    NO_ALLOCATION_ID, OnMinerSectorsTerminateParams, SectorDealActivation,
+    SectorDeals, VerifyDealsForActivationParams, VerifyDealsForActivationReturn,
 };
 use fil_actor_miner::{
-    aggregate_pre_commit_network_fee, aggregate_prove_commit_network_fee, consensus_fault_penalty,
-    ext,
-    ext::market::ON_MINER_SECTORS_TERMINATE_METHOD,
-    ext::power::UPDATE_CLAIMED_POWER_METHOD,
-    ext::verifreg::{
-        AllocationClaim, AllocationID, ClaimAllocationsParams, ClaimAllocationsReturn,
-        SectorAllocationClaims, SectorClaimSummary, CLAIM_ALLOCATIONS_METHOD,
-    },
-    ext::verifreg::{Claim as FILPlusClaim, ClaimID, GetClaimsParams, GetClaimsReturn},
-    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,
-    testing::{check_deadline_state_invariants, check_state_invariants, DeadlineStateSummary},
-    ActiveBeneficiary, Actor, ApplyRewardParams, BeneficiaryTerm, BitFieldQueue,
-    ChangeBeneficiaryParams, ChangeMultiaddrsParams, ChangePeerIDParams, ChangeWorkerAddressParams,
-    CheckSectorProvenParams, CompactCommD, CompactPartitionsParams, CompactSectorNumbersParams,
-    ConfirmSectorProofsParams, CronEventPayload, DataActivationNotification, Deadline,
-    DeadlineInfo, Deadlines, DeclareFaultsParams, DeclareFaultsRecoveredParams,
-    DeferredCronEventParams, DisputeWindowedPoStParams, ExpirationQueue, ExpirationSet,
-    ExtendSectorExpiration2Params, ExtendSectorExpirationParams, FaultDeclaration,
-    GetAvailableBalanceReturn, GetBeneficiaryReturn, GetControlAddressesReturn,
-    GetMultiaddrsReturn, GetPeerIDReturn, Method, Method as MinerMethod,
-    MinerConstructorParams as ConstructorParams, MinerInfo, Partition, PendingBeneficiaryChange,
-    PieceActivationManifest, PieceChange, PieceReturn, PoStPartition, PowerPair,
-    PreCommitSectorBatchParams, PreCommitSectorBatchParams2, PreCommitSectorParams,
+    ActiveBeneficiary, Actor, aggregate_pre_commit_network_fee,
+    aggregate_prove_commit_network_fee,
+    ApplyRewardParams,
+    BeneficiaryTerm,
+    BitFieldQueue,
+    ChangeBeneficiaryParams,
+    ChangeMultiaddrsParams, ChangePeerIDParams, ChangeWorkerAddressParams,
+    CheckSectorProvenParams, CompactCommD, CompactPartitionsParams,
+    CompactSectorNumbersParams, ConfirmSectorProofsParams, consensus_fault_penalty,
+    CRON_EVENT_PROVING_DEADLINE,
+    CronEventPayload, DataActivationNotification, Deadline, DeadlineInfo, Deadlines,
+    DeclareFaultsParams, DeclareFaultsRecoveredParams, DeferredCronEventParams, DisputeWindowedPoStParams,
+    ExpirationQueue, ExpirationSet, ext, ext::market::ON_MINER_SECTORS_TERMINATE_METHOD,
+    ext::power::UPDATE_CLAIMED_POWER_METHOD, ext::verifreg::{
+        AllocationClaim, AllocationID, CLAIM_ALLOCATIONS_METHOD, ClaimAllocationsParams,
+        ClaimAllocationsReturn, SectorAllocationClaims, SectorClaimSummary,
+    }, ext::verifreg::{Claim as FILPlusClaim, ClaimID, GetClaimsParams, GetClaimsReturn}, ExtendSectorExpiration2Params,
+    ExtendSectorExpirationParams, FaultDeclaration, GetAvailableBalanceReturn, GetBeneficiaryReturn,
+    GetControlAddressesReturn, GetMultiaddrsReturn, GetPeerIDReturn, initial_pledge_for_power,
+    locked_reward_from_reward, max_prove_commit_duration, Method,
+    Method as MinerMethod, MinerConstructorParams as ConstructorParams, MinerInfo,
+    new_deadline_info_from_offset_and_epoch, Partition, PendingBeneficiaryChange, PieceActivationManifest,
+    PieceChange, PieceReturn, pledge_penalty_for_continued_fault, PoStPartition,
+    power_for_sectors, PowerPair, PreCommitSectorBatchParams, PreCommitSectorBatchParams2, PreCommitSectorParams,
     ProveCommitAggregateParams, ProveCommitSectorParams, ProveCommitSectors3Params,
-    ProveCommitSectors3Return, RecoveryDeclaration, ReportConsensusFaultParams,
-    SectorActivationManifest, SectorChanges, SectorContentChangedParams,
-    SectorContentChangedReturn, SectorOnChainInfo, SectorPreCommitInfo, SectorPreCommitOnChainInfo,
-    SectorReturn, SectorUpdateManifest, Sectors, State, SubmitWindowedPoStParams,
-    TerminateSectorsParams, TerminationDeclaration, VerifiedAllocationKey, VestingFunds,
-    WindowedPoSt, WithdrawBalanceParams, WithdrawBalanceReturn, CRON_EVENT_PROVING_DEADLINE,
-    REWARD_VESTING_SPEC, SECTORS_AMT_BITWIDTH, SECTOR_CONTENT_CHANGED,
+    ProveCommitSectors3Return, qa_power_for_sector, qa_power_for_weight,
+    RecoveryDeclaration, ReportConsensusFaultParams, reward_for_consensus_slash_report,
+    REWARD_VESTING_SPEC, SECTOR_CONTENT_CHANGED, SectorActivationManifest,
+    SectorChanges, SectorContentChangedParams, SectorContentChangedReturn, SectorOnChainInfo,
+    SectorPreCommitInfo, SectorPreCommitOnChainInfo, SectorReturn, Sectors, SECTORS_AMT_BITWIDTH,
+    SectorUpdateManifest, State, SubmitWindowedPoStParams, TerminateSectorsParams,
+    TerminationDeclaration, testing::{check_deadline_state_invariants, check_state_invariants, DeadlineStateSummary}, VerifiedAllocationKey, VestingFunds,
+    WindowedPoSt, WithdrawBalanceParams, WithdrawBalanceReturn,
 };
 use fil_actor_miner::{ProveReplicaUpdates3Params, ProveReplicaUpdates3Return};
 use fil_actor_power::{
     CurrentTotalPowerReturn, EnrollCronEventParams, Method as PowerMethod, UpdateClaimedPowerParams,
 };
 use fil_actor_reward::{Method as RewardMethod, ThisEpochRewardReturn};
-use fil_actors_runtime::cbor::serialize;
-use fil_actors_runtime::runtime::{DomainSeparationTag, Runtime, RuntimePolicy};
-use fil_actors_runtime::{test_utils::*, BatchReturn, BatchReturnGen, EventBuilder};
+use fil_actors_runtime::{BatchReturn, BatchReturnGen, EventBuilder, test_utils::*};
 use fil_actors_runtime::{
-    ActorDowncast, ActorError, Array, DealWeight, MessageAccumulator, BURNT_FUNDS_ACTOR_ADDR,
-    INIT_ACTOR_ADDR, REWARD_ACTOR_ADDR, STORAGE_MARKET_ACTOR_ADDR, STORAGE_POWER_ACTOR_ADDR,
+    ActorDowncast, ActorError, Array, BURNT_FUNDS_ACTOR_ADDR, DealWeight, INIT_ACTOR_ADDR,
+    MessageAccumulator, REWARD_ACTOR_ADDR, STORAGE_MARKET_ACTOR_ADDR, STORAGE_POWER_ACTOR_ADDR,
     VERIFIED_REGISTRY_ACTOR_ADDR,
 };
+use fil_actors_runtime::cbor::serialize;
+use fil_actors_runtime::runtime::{DomainSeparationTag, Runtime, RuntimePolicy};
+use fil_actors_runtime::test_blockstores::MemoryBlockstore;
 
 const RECEIVER_ID: u64 = 1000;
 
@@ -1139,17 +1139,27 @@ impl ActorHarness {
         rt.set_caller(*ACCOUNT_ACTOR_CODE_ID, cfg.caller.unwrap_or(self.worker));
         rt.expect_validate_caller_addr(self.caller_addrs());
 
-        let mut params = ProveCommitSectors3Params {
-            sector_activations: sector_activations.into(),
-            aggregate_proof: if aggregate { make_proof(0) } else { RawBytes::default() },
-            sector_proofs: if !aggregate {
-                sector_activations.iter().map(|sa| make_proof(sa.sector_number as u8)).collect()
-            } else {
-                vec![]
-            },
-            aggregate_proof_type: Some(RegisteredAggregateProof::SnarkPackV2),
-            require_activation_success,
-            require_notification_success,
+        let mut params = if aggregate {
+            ProveCommitSectors3Params {
+                sector_activations: sector_activations.into(),
+                aggregate_proof: make_proof(0),
+                sector_proofs: vec![],
+                aggregate_proof_type: Some(RegisteredAggregateProof::SnarkPackV2),
+                require_activation_success,
+                require_notification_success,
+            }
+        } else {
+            ProveCommitSectors3Params {
+                sector_activations: sector_activations.into(),
+                aggregate_proof: RawBytes::default(),
+                sector_proofs: sector_activations
+                    .iter()
+                    .map(|sa| make_proof(sa.sector_number as u8))
+                    .collect(),
+                aggregate_proof_type: None,
+                require_activation_success,
+                require_notification_success,
+            }
         };
         if let Some(param_twiddle) = cfg.param_twiddle {
             param_twiddle(&mut params);

integration_tests/src/tests/replica_update2_test.rs

@@ -1,17 +1,16 @@
 use cid::Cid;
-use export_macro::vm_test;
 use fvm_ipld_encoding::ipld_block::IpldBlock;
 use fvm_ipld_encoding::RawBytes;
 use fvm_shared::bigint::BigInt;
 use fvm_shared::clock::ChainEpoch;
 use fvm_shared::deal::DealID;
 use fvm_shared::econ::TokenAmount;
 use fvm_shared::piece::{PaddedPieceSize, PieceInfo};
-use fvm_shared::sector::{
-    RegisteredAggregateProof, RegisteredSealProof, SectorNumber, StoragePower,
+use fvm_shared::sector::{RegisteredSealProof, SectorNumber, StoragePower,
 };
 use num_traits::Zero;
 
+use export_macro::vm_test;
 use fil_actor_market::Method as MarketMethod;
 use fil_actor_miner::{
     max_prove_commit_duration, CompactCommD, DataActivationNotification, PieceActivationManifest,
@@ -99,7 +98,7 @@ pub fn prove_replica_update2_test(v: &dyn VM) {
         sector_activations: activations,
         sector_proofs: proofs,
         aggregate_proof: RawBytes::default(),
-        aggregate_proof_type: Some(RegisteredAggregateProof::SnarkPackV2),
+        aggregate_proof_type: None,
         require_activation_success: true,
         require_notification_success: true,
     };