Make Prove{CommitSectors, ReplicaUpdates}3Params aggregate proof type…

… optional (#1511)

* Make Prove{CommitSectors, ReplicaUpdates}3Params aggregate proof type optional.

* Enforce no agg proof type when not used

actors/miner/src/lib.rs

@@ -13,6 +13,7 @@ use cid::multihash::Code::Blake2b256;
 use cid::Cid;
 use fvm_ipld_bitfield::{BitField, Validate};
 use fvm_ipld_blockstore::Blockstore;
+use fvm_ipld_encoding::ipld_block::IpldBlock;
 use fvm_ipld_encoding::{from_slice, BytesDe, CborStore, RawBytes};
 use fvm_shared::address::{Address, Payload, Protocol};
 use fvm_shared::bigint::{BigInt, Integer};
@@ -31,7 +32,6 @@ use log::{error, info, warn};
 use num_derive::FromPrimitive;
 use num_traits::{Signed, Zero};
 
-use crate::notifications::{notify_data_consumers, ActivationNotifications};
 pub use beneficiary::*;
 pub use bitfield_queue::*;
 pub use commd::*;
@@ -49,7 +49,6 @@ use fil_actors_runtime::{
     BURNT_FUNDS_ACTOR_ADDR, INIT_ACTOR_ADDR, REWARD_ACTOR_ADDR, STORAGE_MARKET_ACTOR_ADDR,
     STORAGE_POWER_ACTOR_ADDR, SYSTEM_ACTOR_ADDR, VERIFIED_REGISTRY_ACTOR_ADDR,
 };
-use fvm_ipld_encoding::ipld_block::IpldBlock;
 
 use crate::ext::market::NO_ALLOCATION_ID;
 pub use monies::*;
@@ -62,6 +61,8 @@ pub use termination::*;
 pub use types::*;
 pub use vesting_state::*;
 
+use crate::notifications::{notify_data_consumers, ActivationNotifications};
+
 // The following errors are particular cases of illegal state.
 // They're not expected to ever happen, but if they do, distinguished codes can help us
 // diagnose the problem.
@@ -1014,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,
@@ -1725,9 +1753,6 @@ impl Actor {
         rt.validate_immediate_caller_is(
             info.control_addresses.iter().chain(&[info.worker, info.owner]),
         )?;
-        if params.aggregate_proof_type != RegisteredAggregateProof::SnarkPackV2 {
-            return Err(actor_error!(illegal_argument, "aggregate proof type must be SnarkPackV2"));
-        }
 
         // Load pre-commits, failing if any don't exist.
         let sector_numbers = params.sector_activations.iter().map(|sa| sa.sector_number);
@@ -1746,6 +1771,12 @@ impl Actor {
 
         if !params.sector_proofs.is_empty() {
             // Batched proofs, one per sector
+            if params.aggregate_proof_type.is_some() {
+                return Err(actor_error!(
+                    illegal_argument,
+                    "aggregate proof type must be null with batched proofs"
+                ));
+            }
             if params.sector_activations.len() != params.sector_proofs.len() {
                 return Err(actor_error!(
                     illegal_argument,
@@ -1756,6 +1787,12 @@ impl Actor {
             }
             validate_seal_proofs(precommits[0].info.seal_proof, &params.sector_proofs)?;
         } else {
+            if params.aggregate_proof_type != Some(RegisteredAggregateProof::SnarkPackV2) {
+                return Err(actor_error!(
+                    illegal_argument,
+                    "aggregate proof type must be SnarkPackV2"
+                ));
+            }
             validate_seal_aggregate_proof(
                 &params.aggregate_proof,
                 params.sector_activations.len() as u64,
@@ -1824,7 +1861,7 @@ impl Actor {
                 &proof_inputs,
                 miner_id,
                 precommits[0].info.seal_proof,
-                params.aggregate_proof_type,
+                params.aggregate_proof_type.unwrap(),
                 &params.aggregate_proof,
             )?;
 

actors/miner/src/types.rs

@@ -145,8 +145,8 @@ pub struct ProveCommitSectors3Params {
     // Aggregate proof for all sectors.
     // Exactly one of sector_proofs or aggregate_proof must be non-empty.
     pub aggregate_proof: RawBytes,
-    // The proof type for the aggregate proof (ignored if no aggregate proof).
-    pub aggregate_proof_type: RegisteredAggregateProof,
+    // The proof type for the aggregate proof (must be None if no aggregate proof).
+    pub aggregate_proof_type: Option<RegisteredAggregateProof>,
     // Whether to abort if any sector activation fails.
     pub require_activation_success: bool,
     // Whether to abort if any notification returns a non-zero exit code.
@@ -490,8 +490,8 @@ pub struct ProveReplicaUpdates3Params {
     pub aggregate_proof: RawBytes,
     // The proof type for all sector update proofs, individually or before aggregation.
     pub update_proofs_type: RegisteredUpdateProof,
-    // The proof type for the aggregate proof (ignored if no aggregate proof).
-    pub aggregate_proof_type: RegisteredAggregateProof,
+    // The proof type for the aggregate proof (must be None if no aggregate proof).
+    pub aggregate_proof_type: Option<RegisteredAggregateProof>,
     // Whether to abort if any sector update activation fails.
     pub require_activation_success: bool,
     // Whether to abort if any notification returns a non-zero exit code.

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,14 +1,13 @@
 #![allow(clippy::all)]
 
-use anyhow::anyhow;
 use std::collections::{BTreeMap, BTreeSet, HashMap};
 use std::convert::TryInto;
 use std::iter;
 use std::ops::Neg;
 
+use anyhow::anyhow;
 use cid::multihash::MultihashDigest;
 use cid::Cid;
-use fil_actors_runtime::test_blockstores::MemoryBlockstore;
 use fvm_ipld_amt::Amt;
 use fvm_ipld_bitfield::iter::Ranges;
 use fvm_ipld_bitfield::{BitField, UnvalidatedBitField, Validate};
@@ -91,6 +90,7 @@ use fil_actor_power::{
 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_blockstores::MemoryBlockstore;
 use fil_actors_runtime::{test_utils::*, BatchReturn, BatchReturnGen, EventBuilder};
 use fil_actors_runtime::{
     ActorDowncast, ActorError, Array, DealWeight, MessageAccumulator, BURNT_FUNDS_ACTOR_ADDR,
@@ -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: 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);
@@ -1408,7 +1418,7 @@ impl ActorHarness {
             sector_proofs: sector_updates.iter().map(|su| make_proof(su.sector as u8)).collect(),
             aggregate_proof: RawBytes::default(),
             update_proofs_type: self.seal_proof_type.registered_update_proof().unwrap(),
-            aggregate_proof_type: RegisteredAggregateProof::Invalid(0),
+            aggregate_proof_type: None,
             require_activation_success,
             require_notification_success,
         };

integration_tests/src/tests/prove_commit2_test.rs

@@ -7,9 +7,7 @@ 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 fil_actor_market::Method as MarketMethod;
@@ -211,7 +209,7 @@ pub fn prove_commit_sectors2_test(v: &dyn VM) {
         sector_activations: manifests.clone(),
         sector_proofs: proofs,
         aggregate_proof: RawBytes::default(),
-        aggregate_proof_type: RegisteredAggregateProof::SnarkPackV2,
+        aggregate_proof_type: None,
         require_activation_success: true,
         require_notification_success: true,
     };

integration_tests/src/tests/replica_update2_test.rs

@@ -1,17 +1,15 @@
 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 +97,7 @@ pub fn prove_replica_update2_test(v: &dyn VM) {
         sector_activations: activations,
         sector_proofs: proofs,
         aggregate_proof: RawBytes::default(),
-        aggregate_proof_type: RegisteredAggregateProof::SnarkPackV2,
+        aggregate_proof_type: None,
         require_activation_success: true,
         require_notification_success: true,
     };
@@ -260,7 +258,7 @@ pub fn prove_replica_update2_test(v: &dyn VM) {
         sector_proofs: proofs,
         aggregate_proof: RawBytes::default(),
         update_proofs_type: update_proof,
-        aggregate_proof_type: RegisteredAggregateProof::SnarkPackV2,
+        aggregate_proof_type: None,
         require_activation_success: true,
         require_notification_success: true,
     };