FIP-0106: Remove of the ProveReplicaUpdates method from the miner actor

[elmattic]

Changes:

• Removed the ProveReplicaUpdates method (number 27) from the miner actor
• Removed the unsealed_cid member from DataActivationOutput
• Ported 14 tests to use the ProveReplicaUpdates3 method

Important note: I wasn’t able to port deal_included_in_multiple_sectors_failure_test and get the corresponding assertions to work.

[BigLep]

@rvagg : FYI this has been marked ready for your review.

[rvagg]

@elmattic a couple of other things I'm seeing:

• ReplicaUpdateInner has a deals field that can be removed
• validate_replica_updates() can have its require_deals argument removed

[codecov-commenter]

Codecov Report

❌ Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.80%. Comparing base (d61ec1b) to head (ff4590e).

Files with missing lines	Patch %	Lines
actors/miner/src/lib.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1688      +/-   ##
==========================================
- Coverage   91.36%   90.80%   -0.57%     
==========================================
  Files         143      143              
  Lines       30410    30253     -157     
==========================================
- Hits        27784    27470     -314     
- Misses       2626     2783     +157     

Files with missing lines	Coverage Δ
actors/miner/src/types.rs	93.75% <ø> (ø)
integration_tests/src/expects.rs	97.36% <100.00%> (-0.02%)	⬇️
integration_tests/src/util/mod.rs	87.76% <100.00%> (-10.03%)	⬇️
actors/miner/src/lib.rs	83.63% <0.00%> (-2.61%)	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[elmattic]

@rvagg Do we want to remove deal_ids in DealsActivationInput as well? I think we can't do that because it can be constructed from a SectorPreCommitOnChainInfo value.

[ZenGround0]

@elmattic

@rvagg Do we want to remove deal_ids in DealsActivationInput as well? I think we can't do that because it can be constructed from a SectorPreCommitOnChainInfo value.

Given the fact that all precommits with deals will be dead in the water after the latest ProveCommitAggregate changes (see the explanation in #1695) it should be fine to drop all deal id values from a pre commit when constructing DealsActivationInput.

We actually can't do this because we are still relying on the hack method; internal_sector_setup_preseal to init test networks with lotus. Same goes for activate_sectors_deals and market actor BATCH_ACTIVATE_DEALS_METHOD.

Tracking these discoveries here

[ZenGround0]

I'll see if I can understand what's going on here. Just a reminder that we need to figure this out before merge.

[elmattic]

I noticed that it’s related to notifications. When I pass false to require_notification_success and expect OK, the first assertions pass, but then the last three fail.

    let params = ProveReplicaUpdates3Params {
        sector_updates: manifests.clone(),
        sector_proofs: proofs,
        aggregate_proof: RawBytes::default(),
        update_proofs_type: update_proof,
        aggregate_proof_type: None,
        require_activation_success: true,
        require_notification_success: false,
    };

    apply_code(
        v,
        &worker,
        &maddr,
        &TokenAmount::zero(),
        MinerMethod::ProveReplicaUpdates3 as u64,
        Some(params),
        ExitCode::OK,
    );

[rvagg]

This is a tricky one, we can't perform an identical test with PRU3 so we might just need to reframe the test.

require_notification_success: false means that it'll just go through and ignore the failure from f05, it'll set up deal_wight and verified_deal_weight as if f05 succeeded but it hasn't (i.e. those fields are basically meaningless because of this).

What we can test is that the deals are activated and are associated with the first one only, not the second. That kind of comes close.

possible diff (fix)

diff --git a/integration_tests/src/tests/replica_update_test.rs b/integration_tests/src/tests/replica_update_test.rs
index 6249b9fd..e184a809 100644
--- a/integration_tests/src/tests/replica_update_test.rs
+++ b/integration_tests/src/tests/replica_update_test.rs
@@ -13,7 +13,7 @@ use fvm_shared::sector::{RegisteredSealProof, SectorNumber};
 
 use export_macro::vm_test;
 use fil_actor_market::State as MarketState;
-use fil_actor_market::{Label, Method as MarketMethod};
+use fil_actor_market::{DealMetaArray, Label, Method as MarketMethod};
 use fil_actor_miner::{
     DisputeWindowedPoStParams, ExpirationExtension2, ExtendSectorExpiration2Params,
     Method as MinerMethod, PowerPair, ProveReplicaUpdates3Params, ProveReplicaUpdates3Return,
@@ -972,32 +972,43 @@ pub fn deal_included_in_multiple_sectors_failure_test(v: &dyn VM) {
         update_proofs_type: update_proof,
         aggregate_proof_type: None,
         require_activation_success: true,
-        require_notification_success: true,
+        require_notification_success: false,
     };
 
-    apply_code(
+    let ret: ProveReplicaUpdates3Return = apply_ok(
         v,
         &worker,
         &maddr,
         &TokenAmount::zero(),
         MinerMethod::ProveReplicaUpdates3 as u64,
         Some(params),
-        fil_actor_miner::ERR_NOTIFICATION_REJECTED,
-    );
+    )
+    .deserialize()
+    .unwrap();
 
-    // TODO: understand why those asserts don't hold anymore
-    // let new_sector_info_p1 = sector_info(v, &maddr, first_sector_number);
-    // let duration = new_sector_info_p1.expiration - new_sector_info_p1.power_base_epoch;
-    // let weights1 = get_deal_weights(v, deal_ids[0], duration);
-    // let weights2 = get_deal_weights(v, deal_ids[1], duration);
-    // assert_eq!(weights1.0 + weights2.0, new_sector_info_p1.deal_weight);
-    // assert_eq!(weights1.1 + weights2.1, new_sector_info_p1.verified_deal_weight);
-    // assert_eq!(new_sealed_cid1, new_sector_info_p1.sealed_cid);
+    assert_eq!(ret.activation_results.success_count, 2);
+
+    let new_sector_info_p1 = sector_info(v, &maddr, first_sector_number);
+    let duration = new_sector_info_p1.expiration - new_sector_info_p1.power_base_epoch;
+    let weights1 = get_deal_weights(v, deal_ids[0], duration);
+    let weights2 = get_deal_weights(v, deal_ids[1], duration);
+    assert_eq!(&weights1.0 + &weights2.0, new_sector_info_p1.deal_weight);
+    assert_eq!(&weights1.1 + &weights2.1, new_sector_info_p1.verified_deal_weight);
+    assert_eq!(new_sealed_cid1, new_sector_info_p1.sealed_cid);
 
     let new_sector_info_p2 = sector_info(v, &maddr, first_sector_number + 1);
-    assert!(new_sector_info_p2.deal_weight.is_zero());
-    assert!(new_sector_info_p2.verified_deal_weight.is_zero());
-    assert_ne!(new_sealed_cid2, new_sector_info_p2.sealed_cid);
+    assert_eq!(weights1.0 + weights2.0, new_sector_info_p2.deal_weight);
+    assert_eq!(weights1.1 + weights2.1, new_sector_info_p2.verified_deal_weight);
+    assert_eq!(new_sealed_cid2, new_sector_info_p2.sealed_cid);
+
+    let st: MarketState = get_state(v, &STORAGE_MARKET_ACTOR_ADDR).unwrap();
+    let store = DynBlockstore::wrap(v.blockstore());
+    let deal_states = DealMetaArray::load(&st.states, &store).unwrap();
+    for id in deal_ids.iter() {
+        // both deals are associated with the first sector
+        let state = deal_states.get(*id).unwrap();
+        assert_eq!(first_sector_number, state.unwrap().sector_number);
+    }
 
     assert_invariants(v, &Policy::default(), None)
 }

[elmattic]

Thanks! I think this makes more sense and probably the best we can do. I've applied your suggestion.

[Copilot]

Pull Request Overview

This PR implements FIP-0106 by removing the deprecated ProveReplicaUpdates method from the miner actor and migrating tests to use the newer ProveReplicaUpdates3 method instead.

• Removes the ProveReplicaUpdates method (number 27) from the miner actor
• Removes the unsealed_cid member from DataActivationOutput structure
• Migrates 14 integration tests from ProveReplicaUpdates to ProveReplicaUpdates3

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
integration_tests/src/util/mod.rs	Adds imports and utility function piece_change for creating piece changes
integration_tests/src/tests/replica_update_test.rs	Migrates all replica update tests from old to new API format
integration_tests/src/tests/replica_update3_test.rs	Removes duplicate piece_change function, now using the shared utility
integration_tests/src/tests/extend_sectors_test.rs	Updates sector extension test to use new replica update API
integration_tests/src/expects.rs	Updates test expectations from market_activate_deals to market_content_changed
actors/miner/src/lib.rs	Removes deprecated ProveReplicaUpdates method and related validation logic

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[akaladarshi]

@elmattic couple of suggestions:

• We can remove the ReplicaUpdate and ProveReplicaUpdatesParams

• We can move all the tests present in replica_update_test.rs to the replica_update_3_test.rs.