[FIP-0034]Switch to Constant PCD, add CommD to PreCommit state #484

Kubuxu · 2022-06-20T15:25:05Z

Implements FIP-0034. Closes #421.

anorth

I realise we forgot to address the design for ProveReplicaUpdate in the FIP and you ran into it here. However I think we can do it better, and retain the design described in the FIP of dropping the deal weights from VerifyDealsForActivation and the PreCommitOnChainInfo.

Thinking about interactions with the market ideally, for the pre/prove commit flow I think we want:

A call from pre-commit that validates deals and computes CommD (but not weight)
A call from prove-commit that activates the deals and returns weight

The ideal for replica update would be a single call that does both of the above. However, simply making those same two calls in order would achieve the same. Batching the activation would be a further improvement.

The existing code in prove_replica_update is not very well factored, and makes those two calls in the opposite order. But the deal weight could still be moved to the activation call, as the FIP describes.

My sense is that you attempted to minimise the complexity of the change from the status quo. I suggest instead that we should strive to minimise the complexity of the end result code, which will be the subject of future changes.

On a style/structure note, the flows with SectorPreCommitInfoInner etc are quite complex. I think they emerge from attempting to get each of the entry point methods to converge on a single implementation, which then has to deal with tri-state CommD. An alternative would be to factor that inner implementation into smaller methods and then orchestrate those smaller methods from the entry points. This should move the handling for zero/unknown CommD into only the entry points where it's possible. I think having some code in those entry points would be fine if it's just straight line sequencing of smaller methods, plus the CommD handling specific to each one.

actors/market/src/lib.rs

actors/market/src/types.rs

actors/miner/src/lib.rs

anorth · 2022-06-28T01:42:48Z

actors/miner/src/types.rs

 /// Information stored on-chain for a pre-committed sector.
+#[derive(Debug, PartialEq, Clone, Serialize_tuple, Deserialize_tuple)]
+pub struct SectorPreCommitOnChainInfo2 {


We should be able to change the existing type rather than create a new one. I understand the old one should be un-used in the new code. We'll need both only in migration code.

I'm guessing you ran into the unfortunate re-use of SectorPreCommitInfo as both state type and the exported parameter type. We should fix this by redefining PreCommitSectorParams to be a copy of the state type rather than identified with it.

I left the old state affecting types as they are because I don't know what is the migration flow for FVM.

If we are sure that they are not needed, I can remove them and run a refactor.

Kubuxu · 2022-06-28T15:13:50Z

I've moved deal weights to activation, added some docs, and removed the usage of From/Into for the PreCommitParams wrapping.

Kubuxu · 2022-06-28T18:39:30Z

I also added the ReplicaUpdate changes.

Kubuxu · 2022-06-29T20:11:47Z

dropping out the v2 entry point
tristate for ReplicaUpdate
ComputeCommD delete

anorth

Looking good, much cleaner. I didn't review the tests in depth. After those few items we discussed and the tests passing, let's also ask Zen to review.

actors/market/src/lib.rs

actors/miner/src/lib.rs

anorth · 2022-06-29T23:49:11Z

actors/miner/src/lib.rs

+                        deal_ids: spci.deal_ids,
+                        expiration: spci.expiration,
+
+                        unsealed_cid: CommDState::Unknown,


Add explanatory comment.
"This entry point computes the unsealed CID from deals vial the market. A future one will accept it directly as a parameter."

actors/miner/src/lib.rs

actors/miner/src/policy.rs

actors/miner/src/lib.rs

anorth

Thanks, I gather this is now ready from your POV. I need to allocate a half hour for a final thorough review, but this is great.

Are we going to drop out the new v2 APIs from this PR so we can land it independently of the forward compatibility FIP process?

Let's now request @ZenGround0 review. In particular I would appreciate your focus on the tests, which I have not examined in as much depth as the main change.

test_vm/src/util.rs

ZenGround0 · 2022-07-13T20:25:12Z

Looking now, mostly at tests

actors/market/src/lib.rs

actors/miner/src/types.rs

ZenGround0

Got hung up on the code change, tests tomorrow

actors/miner/src/lib.rs

anorth

I've reviewed again, looks great. Minor things before we merge:

Resolve duplication of compute_data_commitment (prefer by deleting the method, but otherwise by extracting duplicate code)
Remove the FIP-0041 entry points for now. When we put them back in a new PR we need to test them too

actors/market/src/lib.rs

actors/market/tests/verify_deals_for_activation_test.rs

actors/miner/src/lib.rs

actors/miner/src/policy.rs

jennijuju · 2022-07-19T16:32:00Z

(note this FIP has been in discussion since Feb, 2022 and has moved to last call on July 13th (until July 27th), so far, there is no objection & concern about accepting this FIP. This FIP is also being considered to be finalized in nv17 actively. Therefore, if @filecoin-project/actors-maintainers approves this PR, it maybe optimistically merged into master. We will make changes if FIP status changes accordingly(i.e.: if rejected or won't be finalized in nv17, we will revert the PR).

ZenGround0

The only thing I haven't fully looked over since last review are miner lib.rs and compact commd. I will get to these tomorrow if you haven't merged yet. But assuming logic hasn't changed much and @anorth approves then this LGTM.

ZenGround0 · 2022-07-25T19:59:11Z

actors/market/tests/verify_deals_for_activation_test.rs

@@ -33,15 +38,21 @@ fn verify_deal_and_get_deal_weight_for_unverified_deal_proposal() {
        generate_and_publish_deal(&mut rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH);


nit: here and elsewhere "verify_and_activate_deal_and..."

runtime/src/test_utils.rs

ZenGround0 · 2022-07-25T20:05:01Z

test_vm/tests/replica_update_test.rs

@@ -165,15 +165,15 @@ fn prove_replica_update_multi_dline() {
    let store = &MemoryBlockstore::new();
    let policy = Policy::default();
    let mut v = VM::new_with_singletons(store);
-    let addrs = create_accounts(&v, 1, TokenAmount::from(100_000e18 as i128));
+    let addrs = create_accounts(&v, 1, TokenAmount::from(1_000_000e18 as i128));


It looks like these tests started running out of tokens, why would the changes in this PR introduce a different amount of tokens sent?

We initially create a Miner with a CC sector so it needs more money for a larger PCD.

ZenGround0 · 2022-07-26T05:51:35Z

actors/miner/tests/util.rs

-            if conf.sector_weights.len() > i {
-                sector_weights.push(conf.sector_weights[i].clone());
+            if conf.sector_deal_data.len() > i {
+                sector_weights.push(conf.sector_deal_data[i].clone());


Change the name to match the new return value

ZenGround0 · 2022-07-26T06:02:04Z

actors/miner/src/types.rs

+    pub expiration: ChainEpoch,
+    /// Whether to replace a "committed capacity" no-deal sector (requires non-empty DealIDs)
+    pub replace_capacity: bool,
+    /// The committed capacity sector to replace, and its deadline/partition location


nit: "// Deprecated: "

ZenGround0 · 2022-07-26T06:06:19Z

actors/miner/tests/miner_actor_test_commitment.rs

@@ -161,44 +133,6 @@ mod miner_actor_test_commitment {
        h.check_state(&rt);
    }

-    #[test]
-    fn deal_space_exceeds_sector_space() {


Potentially worth introducing this in proveCommitAggregate test. Or ConfirmSectorProofsValid though that seems a bit too annoying to be worth it.

ZenGround0 · 2022-07-26T06:24:14Z

actors/miner/tests/miner_actor_test_precommit_batch.rs


        assert_eq!(sector_nos[i], precommits[i].info.sector_number);
-        assert_eq!(sectors[i], precommits[i].info);
+        //assert_eq!(sectors[i], precommits[i].info);


I think these two types are now different and this comment should be removed

Signed-off-by: Jakub Sztandera <[email protected]>

Kubuxu force-pushed the feat/stable-pcd branch from b4366da to 302f46b Compare June 22, 2022 15:46

anorth requested changes Jun 28, 2022

View reviewed changes

Kubuxu closed this Jun 29, 2022

Kubuxu reopened this Jun 29, 2022

anorth reviewed Jun 30, 2022

View reviewed changes

anorth self-requested a review June 30, 2022 00:01

Kubuxu force-pushed the feat/stable-pcd branch 3 times, most recently from c212c3f to 807ca55 Compare July 6, 2022 18:45

Kubuxu marked this pull request as ready for review July 6, 2022 18:55

Kubuxu force-pushed the feat/stable-pcd branch from 91f224f to 6f76bae Compare July 6, 2022 19:49

anorth reviewed Jul 6, 2022

View reviewed changes

actors/miner/src/lib.rs Outdated Show resolved Hide resolved

Kubuxu requested a review from anorth July 11, 2022 12:52

Kubuxu changed the title ~~Initial work for Stable PCD FIP~~ Switch to Constant PCD, drop CommD out of PreCommit state, add future compatibility for CommD passing Jul 11, 2022

Kubuxu force-pushed the feat/stable-pcd branch from fab7eec to fe2785b Compare July 11, 2022 14:26

anorth reviewed Jul 12, 2022

View reviewed changes

test_vm/src/util.rs Outdated Show resolved Hide resolved

anorth requested review from anorth and ZenGround0 July 12, 2022 02:45

ZenGround0 reviewed Jul 13, 2022

View reviewed changes

actors/market/src/lib.rs Show resolved Hide resolved

actors/miner/src/types.rs Show resolved Hide resolved

ZenGround0 reviewed Jul 14, 2022

View reviewed changes

actors/miner/src/lib.rs Outdated Show resolved Hide resolved

actors/miner/src/lib.rs Show resolved Hide resolved

actors/miner/src/lib.rs Show resolved Hide resolved

anorth reviewed Jul 18, 2022

View reviewed changes

Kubuxu force-pushed the feat/stable-pcd branch from 7aa4018 to 2bfd393 Compare July 18, 2022 17:52

Kubuxu requested a review from anorth July 18, 2022 19:07

jennijuju changed the title ~~Switch to Constant PCD, drop CommD out of PreCommit state, add future compatibility for CommD passing~~ [FIP-0034]Switch to Constant PCD, drop CommD out of PreCommit state, add future compatibility for CommD passing Jul 19, 2022

ZenGround0 approved these changes Jul 26, 2022

View reviewed changes

Jakub Sztandera added 19 commits August 4, 2022 16:20

Improve zero_commd

a7ba177

Signed-off-by: Jakub Sztandera <[email protected]>

fmt and clippy

416654b

Signed-off-by: Jakub Sztandera <[email protected]>

Refactor Verify and ActivateDeals so deal weights come from Activate

f266985

Signed-off-by: Jakub Sztandera <[email protected]>

Add ProveReplicaUpdates2

6229912

Signed-off-by: Jakub Sztandera <[email protected]>

move commd verification

cd423a5

Signed-off-by: Jakub Sztandera <[email protected]>

Switch to CompactCommD

b80d8f5

Signed-off-by: Jakub Sztandera <[email protected]>

Fix VestSpec

c727cf1

Signed-off-by: Jakub Sztandera <[email protected]>

Refactor-out the 2

684a2d9

Signed-off-by: Jakub Sztandera <[email protected]>

cargo fmt -- **/*.rs

ab8a72c

Signed-off-by: Jakub Sztandera <[email protected]>

no-fail-fast

9edc77a

Signed-off-by: Jakub Sztandera <[email protected]>

Fix PRU param

94a738b

Signed-off-by: Jakub Sztandera <[email protected]>

Fix extend_sector_expiration_test

15153d1

Signed-off-by: Jakub Sztandera <[email protected]>

Fix test util deal detection

2e1874a

Signed-off-by: Jakub Sztandera <[email protected]>

Fix prove_sectors_from_batch_pre_commit

d8e1d8f

Signed-off-by: Jakub Sztandera <[email protected]>

Fix deal_included_in_multiple_sectors_failure

35f6779

Signed-off-by: Jakub Sztandera <[email protected]>

Address review

df200d5

Signed-off-by: Jakub Sztandera <[email protected]>

Further address review

590c754

Signed-off-by: Jakub Sztandera <[email protected]>

Disable new entry points

2fc1c60

Signed-off-by: Jakub Sztandera <[email protected]>

Address review feedback

ad348a3

Signed-off-by: Jakub Sztandera <[email protected]>

Kubuxu force-pushed the feat/stable-pcd branch from 7ac8314 to ad348a3 Compare August 4, 2022 14:20

Kubuxu enabled auto-merge (squash) August 4, 2022 14:21

Kubuxu changed the title ~~[FIP-0034]Switch to Constant PCD, drop CommD out of PreCommit state, add future compatibility for CommD passing~~ [FIP-0034]Switch to Constant PCD, add CommD to PreCommit state Aug 4, 2022

Kubuxu merged commit ac3d3e8 into master Aug 4, 2022

Kubuxu deleted the feat/stable-pcd branch August 4, 2022 14:47

Kubuxu restored the feat/stable-pcd branch August 4, 2022 16:37

anorth mentioned this pull request Aug 11, 2022

Remove old CC upgrade/replace params #13

Closed

jennijuju mentioned this pull request Sep 2, 2022

Integrate FIP-0034 filecoin-project/lotus#9251

Closed

arajasek mentioned this pull request Sep 5, 2022

feat: migration for FIP-0034 filecoin-project/go-state-types#48

Merged

LesnyRumcajs mentioned this pull request Sep 21, 2022

FIP 34 - Fix pre-commit deposit independent of sector content ChainSafe/forest#1903

Closed

4 tasks

diwufeiwen mentioned this pull request Oct 11, 2022

[FIP]评估/调研 filecoin-project/venus#5349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIP-0034]Switch to Constant PCD, add CommD to PreCommit state #484

[FIP-0034]Switch to Constant PCD, add CommD to PreCommit state #484

Kubuxu commented Jun 20, 2022 •

edited by anorth

Loading

anorth left a comment

anorth Jun 28, 2022

Kubuxu Jun 28, 2022

Kubuxu Jun 28, 2022

Kubuxu commented Jun 28, 2022

Kubuxu commented Jun 28, 2022

Kubuxu commented Jun 29, 2022 •

edited by anorth

Loading

anorth left a comment

anorth Jun 29, 2022

anorth left a comment

ZenGround0 commented Jul 13, 2022

ZenGround0 left a comment

anorth left a comment

jennijuju commented Jul 19, 2022

ZenGround0 left a comment

ZenGround0 Jul 25, 2022

ZenGround0 Jul 25, 2022

Kubuxu Jul 27, 2022 •

edited

Loading

ZenGround0 Jul 26, 2022

ZenGround0 Jul 26, 2022

ZenGround0 Jul 26, 2022

ZenGround0 Jul 26, 2022

		@@ -33,15 +38,21 @@ fn verify_deal_and_get_deal_weight_for_unverified_deal_proposal() {
		generate_and_publish_deal(&mut rt, CLIENT_ADDR, &MINER_ADDRESSES, START_EPOCH, END_EPOCH);

[FIP-0034]Switch to Constant PCD, add CommD to PreCommit state #484

[FIP-0034]Switch to Constant PCD, add CommD to PreCommit state #484

Conversation

Kubuxu commented Jun 20, 2022 • edited by anorth Loading

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Jun 28, 2022

Kubuxu commented Jun 28, 2022

Kubuxu commented Jun 29, 2022 • edited by anorth Loading

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

ZenGround0 commented Jul 13, 2022

ZenGround0 left a comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

jennijuju commented Jul 19, 2022

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Jun 20, 2022 •

edited by anorth

Loading

Kubuxu commented Jun 29, 2022 •

edited by anorth

Loading

Kubuxu Jul 27, 2022 •

edited

Loading