Remove deprecated precommit methods (#1357)

- PreCommitSector and PreCommitBatch deprecated - PreCommitBatchV2 now requires specifying (compact) CommD - Move all testing to use PreCommitBatchV2 Co-authored-by: ZenGround0 <[email protected]> Co-authored-by: zenground0 <[email protected]>
filecoin-project · Dec 11, 2023 · d5cd05b · d5cd05b
1 parent cbf3890
commit d5cd05b
Show file tree

Hide file tree

Showing 15 changed files with 409 additions and 562 deletions.
diff --git a/actors/miner/src/lib.rs b/actors/miner/src/lib.rs
@@ -100,7 +100,7 @@ pub enum Method {
     ChangeWorkerAddress = 3,
     ChangePeerID = 4,
     SubmitWindowedPoSt = 5,
-    PreCommitSector = 6,
+    //PreCommitSector = 6, // Deprecated
     ProveCommitSector = 7,
     ExtendSectorExpiration = 8,
     TerminateSectors = 9,
@@ -119,7 +119,7 @@ pub enum Method {
     RepayDebt = 22,
     ChangeOwnerAddress = 23,
     DisputeWindowedPoSt = 24,
-    PreCommitSectorBatch = 25,
+    //PreCommitSectorBatch = 25, // Deprecated
     ProveCommitAggregate = 26,
     ProveReplicaUpdates = 27,
     PreCommitSectorBatch2 = 28,
@@ -1401,54 +1401,6 @@ impl Actor {
         Ok(())
     }
 
-    /// Pledges to seal and commit a single sector.
-    /// See PreCommitSectorBatch for details.
-    fn pre_commit_sector(
-        rt: &impl Runtime,
-        params: PreCommitSectorParams,
-    ) -> Result<(), ActorError> {
-        Self::pre_commit_sector_batch(rt, PreCommitSectorBatchParams { sectors: vec![params] })
-    }
-
-    /// Pledges the miner to seal and commit some new sectors.
-    /// The caller specifies sector numbers, sealed sector data CIDs, seal randomness epoch, expiration, and the IDs
-    /// of any storage deals contained in the sector data. The storage deal proposals must be already submitted
-    /// to the storage market actor.
-    /// A pre-commitment may specify an existing committed-capacity sector that the committed sector will replace
-    /// when proven.
-    /// This method calculates the sector's power, locks a pre-commit deposit for the sector, stores information about the
-    /// sector in state and waits for it to be proven or expire.
-    fn pre_commit_sector_batch(
-        rt: &impl Runtime,
-        params: PreCommitSectorBatchParams,
-    ) -> Result<(), ActorError> {
-        let sectors = params
-            .sectors
-            .into_iter()
-            .map(|spci| {
-                if spci.replace_capacity {
-                    Err(actor_error!(
-                        forbidden,
-                        "cc upgrade through precommit discontinued, use ProveReplicaUpdate"
-                    ))
-                } else {
-                    Ok(SectorPreCommitInfoInner {
-                        seal_proof: spci.seal_proof,
-                        sector_number: spci.sector_number,
-                        sealed_cid: spci.sealed_cid,
-                        seal_rand_epoch: spci.seal_rand_epoch,
-                        deal_ids: spci.deal_ids,
-                        expiration: spci.expiration,
-                        // This entry point computes the unsealed CID from deals via the market.
-                        // A future one will accept it directly as a parameter.
-                        unsealed_cid: None,
-                    })
-                }
-            })
-            .collect::<Result<_, _>>()?;
-        Self::pre_commit_sector_batch_inner(rt, sectors)
-    }
-
     /// Pledges the miner to seal and commit some new sectors.
     /// The caller specifies sector numbers, sealed sector CIDs, unsealed sector CID, seal randomness epoch, expiration, and the IDs
     /// of any storage deals contained in the sector data. The storage deal proposals must be already submitted
@@ -1549,10 +1501,20 @@ impl Actor {
                 ));
             }
 
-            if let Some(ref commd) = precommit.unsealed_cid.as_ref().and_then(|c| c.0) {
-                if !is_unsealed_sector(commd) {
-                    return Err(actor_error!(illegal_argument, "unsealed CID had wrong prefix"));
+            if let Some(compact_commd) = &precommit.unsealed_cid {
+                if let Some(commd) = compact_commd.0 {
+                    if !is_unsealed_sector(&commd) {
+                        return Err(actor_error!(
+                            illegal_argument,
+                            "unsealed CID had wrong prefix"
+                        ));
+                    }
                 }
+            } else {
+                return Err(actor_error!(
+                    illegal_argument,
+                    "unspecified CompactCommD not allowed past nv21, need explicit None value for CC or CommD"
+                ));
             }
 
             // Require sector lifetime meets minimum by assuming activation happens at last epoch permitted for seal proof.
@@ -1658,11 +1620,7 @@ impl Actor {
                 // 1. verify that precommit.unsealed_cid is correct
                 // 2. create a new on_chain_precommit
 
-                let commd = match precommit.unsealed_cid {
-                    // if the CommD is unknown, use CommD computed by the market
-                    None => CompactCommD::new(*computed_cid),
-                    Some(x) => x,
-                };
+                let commd = precommit.unsealed_cid.unwrap();
                 if commd.0 != *computed_cid {
                     return Err(actor_error!(illegal_argument, "computed {:?} and passed {:?} CommDs not equal",
                             computed_cid, commd));
@@ -5352,7 +5310,6 @@ impl ActorCode for Actor {
         ChangeWorkerAddress|ChangeWorkerAddressExported => change_worker_address,
         ChangePeerID|ChangePeerIDExported => change_peer_id,
         SubmitWindowedPoSt => submit_windowed_post,
-        PreCommitSector => pre_commit_sector,
         ProveCommitSector => prove_commit_sector,
         ExtendSectorExpiration => extend_sector_expiration,
         TerminateSectors => terminate_sectors,
@@ -5372,7 +5329,6 @@ impl ActorCode for Actor {
         RepayDebt|RepayDebtExported => repay_debt,
         ChangeOwnerAddress|ChangeOwnerAddressExported => change_owner_address,
         DisputeWindowedPoSt => dispute_windowed_post,
-        PreCommitSectorBatch => pre_commit_sector_batch,
         ProveCommitAggregate => prove_commit_aggregate,
         ProveReplicaUpdates => prove_replica_updates,
         PreCommitSectorBatch2 => pre_commit_sector_batch2,

diff --git a/actors/miner/tests/miner_actor_test_precommit_batch.rs b/actors/miner/tests/miner_actor_test_precommit_batch.rs
@@ -34,7 +34,6 @@ struct DealSpec {
 }
 
 fn assert_simple_batch(
-    v2: bool,
     batch_size: usize,
     balance_surplus: TokenAmount,
     base_fee: TokenAmount,
@@ -44,10 +43,7 @@ fn assert_simple_batch(
 ) {
     let period_offset = ChainEpoch::from(100);
 
-    let h = ActorHarness::new_with_options(HarnessOptions {
-        use_v2_pre_commit_and_replica_update: v2,
-        proving_period_offset: period_offset,
-    });
+    let h = ActorHarness::new_with_options(HarnessOptions { proving_period_offset: period_offset });
     let rt = h.new_runtime();
 
     let precommit_epoch = period_offset + 1;
@@ -156,47 +152,25 @@ mod miner_actor_precommit_batch {
     };
     use fil_actors_runtime::{STORAGE_MARKET_ACTOR_ADDR, STORAGE_POWER_ACTOR_ADDR};
     use fvm_ipld_encoding::ipld_block::IpldBlock;
-    use test_case::test_case;
 
-    #[test_case(false; "v1")]
-    #[test_case(true; "v2")]
-    fn one_sector(v2: bool) {
-        assert_simple_batch(v2, 1, TokenAmount::zero(), TokenAmount::zero(), &[], ExitCode::OK, "");
+    #[test]
+    fn one_sector() {
+        assert_simple_batch(1, TokenAmount::zero(), TokenAmount::zero(), &[], ExitCode::OK, "");
     }
 
-    #[test_case(false; "v1")]
-    #[test_case(true; "v2")]
-    fn thirty_two_sectors(v2: bool) {
-        assert_simple_batch(
-            v2,
-            32,
-            TokenAmount::zero(),
-            TokenAmount::zero(),
-            &[],
-            ExitCode::OK,
-            "",
-        );
+    #[test]
+    fn thirty_two_sectors() {
+        assert_simple_batch(32, TokenAmount::zero(), TokenAmount::zero(), &[], ExitCode::OK, "");
     }
 
-    #[test_case(false; "v1")]
-    #[test_case(true; "v2")]
-    fn max_sectors(v2: bool) {
-        assert_simple_batch(
-            v2,
-            256,
-            TokenAmount::zero(),
-            TokenAmount::zero(),
-            &[],
-            ExitCode::OK,
-            "",
-        );
+    #[test]
+    fn max_sectors() {
+        assert_simple_batch(256, TokenAmount::zero(), TokenAmount::zero(), &[], ExitCode::OK, "");
     }
 
-    #[test_case(false; "v1")]
-    #[test_case(true; "v2")]
-    fn one_deal(v2: bool) {
+    #[test]
+    fn one_deal() {
         assert_simple_batch(
-            v2,
             3,
             TokenAmount::zero(),
             TokenAmount::zero(),
@@ -205,12 +179,9 @@ mod miner_actor_precommit_batch {
             "",
         );
     }
-
-    #[test_case(false; "v1")]
-    #[test_case(true; "v2")]
-    fn many_deals(v2: bool) {
+    #[test]
+    fn many_deals() {
         assert_simple_batch(
-            v2,
             3,
             TokenAmount::zero(),
             TokenAmount::zero(),
@@ -224,11 +195,9 @@ mod miner_actor_precommit_batch {
         );
     }
 
-    #[test_case(false; "v1")]
-    #[test_case(true; "v2")]
-    fn empty_batch(v2: bool) {
+    #[test]
+    fn empty_batch() {
         assert_simple_batch(
-            v2,
             0,
             TokenAmount::zero(),
             TokenAmount::zero(),
@@ -238,11 +207,9 @@ mod miner_actor_precommit_batch {
         );
     }
 
-    #[test_case(false; "v1")]
-    #[test_case(true; "v2")]
-    fn too_many_sectors(v2: bool) {
+    #[test]
+    fn too_many_sectors() {
         assert_simple_batch(
-            v2,
             Policy::default().pre_commit_sector_batch_max_size + 1,
             TokenAmount::zero(),
             TokenAmount::zero(),
@@ -251,12 +218,9 @@ mod miner_actor_precommit_batch {
             "batch of 257 too large",
         );
     }
-
-    #[test_case(false; "v1")]
-    #[test_case(true; "v2")]
-    fn insufficient_balance(v2: bool) {
+    #[test]
+    fn insufficient_balance() {
         assert_simple_batch(
-            v2,
             10,
             TokenAmount::from_atto(-1),
             TokenAmount::zero(),
@@ -266,19 +230,16 @@ mod miner_actor_precommit_batch {
         );
     }
 
-    #[test_case(false; "v1")]
-    #[test_case(true; "v2")]
-    fn one_bad_apple_ruins_batch(v2: bool) {
+    #[test]
+    fn one_bad_apple_ruins_batch() {
         // This test does not enumerate all the individual conditions that could cause a single precommit
         // to be rejected. Those are covered in the PreCommitSector tests, and we know that that
         // method is implemented in terms of a batch of one.
 
         let period_offset = ChainEpoch::from(100);
 
-        let h = ActorHarness::new_with_options(HarnessOptions {
-            use_v2_pre_commit_and_replica_update: v2,
-            proving_period_offset: period_offset,
-        });
+        let h =
+            ActorHarness::new_with_options(HarnessOptions { proving_period_offset: period_offset });
 
         let rt = h.new_runtime();
 
@@ -311,15 +272,12 @@ mod miner_actor_precommit_batch {
         rt.reset();
     }
 
-    #[test_case(false; "v1")]
-    #[test_case(true; "v2")]
-    fn duplicate_sector_rejects_batch(v2: bool) {
+    #[test]
+    fn duplicate_sector_rejects_batch() {
         let period_offset = ChainEpoch::from(100);
 
-        let h = ActorHarness::new_with_options(HarnessOptions {
-            use_v2_pre_commit_and_replica_update: v2,
-            proving_period_offset: period_offset,
-        });
+        let h =
+            ActorHarness::new_with_options(HarnessOptions { proving_period_offset: period_offset });
         let rt = h.new_runtime();
 
         rt.set_balance(BIG_BALANCE.clone());
@@ -355,10 +313,8 @@ mod miner_actor_precommit_batch {
     fn mismatch_of_commd() {
         let period_offset = ChainEpoch::from(100);
 
-        let h = ActorHarness::new_with_options(HarnessOptions {
-            use_v2_pre_commit_and_replica_update: true,
-            proving_period_offset: period_offset,
-        });
+        let h =
+            ActorHarness::new_with_options(HarnessOptions { proving_period_offset: period_offset });
         let rt = h.new_runtime();
 
         rt.set_balance(BIG_BALANCE.clone());