FIP-0084: Drop ProveCommitSectors message after ProveCommitSectors3 inclusion #820

jennijuju · 2023-09-04T20:28:29Z

I wanna start this FIP early so to suggest the implementation clients to move away from ProveCommitSectors sooner rather than later - so that we can resolve the imbalanced sector activation charges and move more jobs outside of the network cron.

This will addressed the bug where aggregation is more expensive than single prove commit post FVM and FIP-45 - this is a bug requested to be fixed by a handful SPs #689

This FIP is dependent on the ProveCommitSectors2 thats being introduced in the new direct data onboarding pipeline.

jennijuju · 2023-09-04T21:01:16Z

Will open for review when DDO fip is merged

jennijuju · 2023-11-23T19:09:26Z

@filecoin-project/fips-editors this is ready for review.

@Stebalien @magik6k @rjan90 @snadrus @zhiqiangxu - will appreciate your peer review as well.

rjan90

Added comments and suggestions primarily to enhance readability, correct spelling errors, and remove unnecessary standard comments.

FIPS/fip-drop-provecommit1.md

kaitlin-beegle · 2023-11-28T20:02:02Z

FIPS/fip-drop-provecommit1.md

+
+Test cases for an implementation are mandatory for FIPs that are affecting consensus changes. Other FIPs can choose to include links to test cases if applicable.
+
+Calling `ProveCommitSector` will fail with ***USR_UNHANDLED_MESSAGE.***


Small nit, but should this be moved to the specification section?

Ties in to https://github.com/filecoin-project/FIPs/pull/820/files#r1409927089

kaitlin-beegle

Editorial Review - looks good. Clearly written and concisely scoped.

anorth · 2023-11-29T22:00:30Z

FIPS/fip-drop-provecommit1.md

+fip: "<to be assigned>" <!--keep the qoutes around the fip number, i.e: `fip: "0001"`-->
+title:  Deprecate  `ProveCommitSectors`  to Avoid Single PoRep Proof Verification in Cron 
+author: Jennifer Wang (@jennijuju)
+discussions-to: [<URL>](https://github.com/filecoin-project/FIPs/discussions/689)


I'm not sure this is an appropriate discussion topic to use here. It's a good discussion about a problem, but not particularly oriented around this solution. The context of FIP-0076 wasn't available when that discussion was made. Could you please open a new one and summarise this proposal there?

FIPS/fip-drop-provecommit1.md

anorth · 2023-11-29T22:18:03Z

FIPS/fip-drop-provecommit1.md

+
+## Incentive Considerations
+
+This FIP will enforce storage activation fees to be paid by the callers rather than having an imbalanced network subsidy. This means storage providers who only onboard sectors via `ProveCommitSector` will start to pay for sector proof validation and sector/deal activation, which is a gas cost increase from ~71M to ~196M based on the current implementation. However, the activation cost is expected to be lower with the direct data onboarding flow via the `ProveCommitSectors2` method, and storage providers and their data clients may choose to opt-out of f05 deal activation, which would result in significantly less gas cost.


I think more could be said here (or in motivation) about how the subsidy incentivises inefficient use of the resources of chain validation by discouraging aggregation of proofs.

(included in the motivation section

FIPS/fip-drop-provecommit1.md

jsoares · 2023-11-30T07:39:31Z

FIPS/fip-drop-provecommit1.md

+
+Currently, `ProveCommitSector` accepts a single PoRep proof for an individual sector, validates the proof, and activates the sector along with the deals inside it via a cron call to the power actor. In contrast, when submitting an aggregated PoRep with `ProveCommitAggregate`, all proof validation and sector activation are executed synchronously and the costs are billed to the caller. This creates an imbalance in the sector activation gas subsidy available to different onboarding methods, and also adds unnecessary work to unpaid network cron jobs.
+
+To address this issue, we propose to deprecate the `ProveCommitSector` method after the network upgrade that includes the `ProveCommitSectors2` method, which is planned as part of [FIP-0076 - Direct Data Onboarding](https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0076.md) and is expected to be in scope for upcoming network upgrades.


I agree that the planning of upgrades isn't within scope, but I'd still like us to clearly state that this comes strictly after FIP-0076, i.e. not at the same time. This is important as you yourself point out in the discussion and as explicitly stated further down. Change here could be as simple as:

FIP-0076 provides an alternative, synchronous activation path for non-aggregated proofs. This proposal subsequently removes the ProveCommitSector method and thus the imbalance.

jsoares · 2023-11-30T07:41:30Z

FIPS/fip-drop-provecommit1.md

@@ -0,0 +1,79 @@
+---
+


Suggested change

FIPS/fip-drop-provecommit1.md

jsoares · 2023-11-30T08:25:37Z

FIPS/fip-drop-provecommit1.md

+
+Test cases for an implementation are mandatory for FIPs that are affecting consensus changes. Other FIPs can choose to include links to test cases if applicable.
+
+Calling `ProveCommitSector` will fail with ***USR_UNHANDLED_MESSAGE.***


Ties in to https://github.com/filecoin-project/FIPs/pull/820/files#r1409927089

FIPS/fip-drop-provecommit1.md

Stebalien · 2023-12-07T06:35:06Z

FIPS/fip-drop-provecommit1.md

+Thus, we would like to resolve the imbalance in sector activation and make proof aggregation financially sensible again.
+
+This change also removes unnecessary yet expensive sector and deal activation work from the network cron, which prevents the network from potential cron blowup in the long term.
+


I'd also like to note that this will let us get rid of the FVM syscall that lets actors explicitly charge gas. We currently use this to charge for the deferred proof verification.

which sys call is this?

kernel GasOps?

anorth

Please take FIP-0084 and update the README to link here. I will approve this after that, and nits.

anorth · 2024-01-04T20:59:36Z

FIPS/fip-drop-provecommit1.md

+
+## Simple Summary
+
+Remove *`ProveCommitSector` (method 7)* in favor of `ProveCommitSectors2` and `ProveCommitAggregate`. This change ensures that single PoRep validations and single sector activations are executed synchronously and billed to the caller, instead of being executed asynchronously via a cron call to the actor.


The new method name changed to ProveCommitSectors3 in 886. Perhaps include the method numbers (#892).

FIPS/fip-drop-provecommit1.md

…inclusion

Co-authored-by: Phi-rjan <[email protected]>

Co-authored-by: Jorge Soares <[email protected]>

Co-authored-by: Alex North <[email protected]> Co-authored-by: Jorge Soares <[email protected]>

Co-authored-by: Alex North <[email protected]>

jennijuju requested review from momack2, arajasek, kaitlin-beegle, anorth, raulk and jsoares as code owners September 4, 2023 20:28

jennijuju marked this pull request as draft September 4, 2023 21:00

jennijuju force-pushed the jen/droppc branch from da0e57a to 84de187 Compare November 23, 2023 18:59

jennijuju marked this pull request as ready for review November 23, 2023 19:00

rjan90 reviewed Nov 24, 2023

View reviewed changes

kaitlin-beegle reviewed Nov 28, 2023

View reviewed changes

kaitlin-beegle approved these changes Nov 28, 2023

View reviewed changes

anorth reviewed Nov 29, 2023

View reviewed changes

jsoares reviewed Nov 30, 2023

View reviewed changes

Stebalien reviewed Dec 7, 2023

View reviewed changes

Stebalien mentioned this pull request Dec 14, 2023

refactor: remove GasOps from the Kernel filecoin-project/ref-fvm#1951

Merged

jennijuju mentioned this pull request Dec 26, 2023

FIP-0083: Add built-in Actor events in the Verified Registry, Miner and Market Actors #872

Merged

jennijuju force-pushed the jen/droppc branch from d1f0287 to a24f0ed Compare January 3, 2024 11:37

jennijuju changed the title ~~Deprecating provecommitsectors message after ProveCommitSectors2 inclusion~~ Drop provecommitsectors message after ProveCommitSectors2 inclusion Jan 3, 2024

anorth reviewed Jan 4, 2024

View reviewed changes

jennijuju and others added 8 commits January 5, 2024 05:16

initial draft for deprecating provecommitsectors message after DDO's …

0a006f7

…inclusion

minor tweaks

84b9366

Apply suggestions from @rjan90

9146412

Co-authored-by: Phi-rjan <[email protected]>

Update FIPS/fip-drop-provecommit1.md

37299ba

Co-authored-by: Jorge Soares <[email protected]>

accept editorial reviews

3a2a063

Co-authored-by: Alex North <[email protected]> Co-authored-by: Jorge Soares <[email protected]>

address peer and editor feedback

140bd00

attach a dedicated fip disucssion

f22e39c

Apply suggestions from code review

b2defd0

Co-authored-by: Alex North <[email protected]>

jennijuju added 2 commits January 5, 2024 05:16

address feedback and add fip number

c0d366c

add fip to the readme

c7eab4b

jennijuju force-pushed the jen/droppc branch from 3cef90a to c7eab4b Compare January 4, 2024 21:18

anorth approved these changes Jan 4, 2024

View reviewed changes

anorth changed the title ~~Drop provecommitsectors message after ProveCommitSectors2 inclusion~~ FIP-0084: Drop ProveCommitSectors message after ProveCommitSectors2 inclusion Jan 4, 2024

anorth merged commit e22c254 into master Jan 4, 2024
1 check passed

anorth deleted the jen/droppc branch January 4, 2024 21:43

jennijuju changed the title ~~FIP-0084: Drop ProveCommitSectors message after ProveCommitSectors2 inclusion~~ FIP-0084: Drop ProveCommitSectors message after ProveCommitSectors3 inclusion Jan 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIP-0084: Drop ProveCommitSectors message after ProveCommitSectors3 inclusion #820

FIP-0084: Drop ProveCommitSectors message after ProveCommitSectors3 inclusion #820

jennijuju commented Sep 4, 2023 •

edited

Loading

jennijuju commented Sep 4, 2023

jennijuju commented Nov 23, 2023

rjan90 left a comment

kaitlin-beegle Nov 28, 2023

jsoares Nov 30, 2023

kaitlin-beegle left a comment

anorth Nov 29, 2023

jennijuju Jan 3, 2024

anorth Nov 29, 2023

jennijuju Jan 3, 2024

jsoares Nov 30, 2023

jsoares Nov 30, 2023

jsoares Nov 30, 2023

Stebalien Dec 7, 2023

jennijuju Jan 3, 2024

jennijuju Jan 3, 2024

anorth left a comment

anorth Jan 4, 2024


		Test cases for an implementation are mandatory for FIPs that are affecting consensus changes. Other FIPs can choose to include links to test cases if applicable.

		Calling `ProveCommitSector` will fail with *USR_UNHANDLED_MESSAGE.*


		## Incentive Considerations

		This FIP will enforce storage activation fees to be paid by the callers rather than having an imbalanced network subsidy. This means storage providers who only onboard sectors via `ProveCommitSector` will start to pay for sector proof validation and sector/deal activation, which is a gas cost increase from ~71M to ~196M based on the current implementation. However, the activation cost is expected to be lower with the direct data onboarding flow via the `ProveCommitSectors2` method, and storage providers and their data clients may choose to opt-out of f05 deal activation, which would result in significantly less gas cost.


		Currently, `ProveCommitSector` accepts a single PoRep proof for an individual sector, validates the proof, and activates the sector along with the deals inside it via a cron call to the power actor. In contrast, when submitting an aggregated PoRep with `ProveCommitAggregate`, all proof validation and sector activation are executed synchronously and the costs are billed to the caller. This creates an imbalance in the sector activation gas subsidy available to different onboarding methods, and also adds unnecessary work to unpaid network cron jobs.

		To address this issue, we propose to deprecate the `ProveCommitSector` method after the network upgrade that includes the `ProveCommitSectors2` method, which is planned as part of [FIP-0076 - Direct Data Onboarding](https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0076.md) and is expected to be in scope for upcoming network upgrades.

		Thus, we would like to resolve the imbalance in sector activation and make proof aggregation financially sensible again.

		This change also removes unnecessary yet expensive sector and deal activation work from the network cron, which prevents the network from potential cron blowup in the long term.


		## Simple Summary

		Remove `ProveCommitSector` (method 7) in favor of `ProveCommitSectors2` and `ProveCommitAggregate`. This change ensures that single PoRep validations and single sector activations are executed synchronously and billed to the caller, instead of being executed asynchronously via a cron call to the actor.

FIP-0084: Drop ProveCommitSectors message after ProveCommitSectors3 inclusion #820

FIP-0084: Drop ProveCommitSectors message after ProveCommitSectors3 inclusion #820

Conversation

jennijuju commented Sep 4, 2023 • edited Loading

jennijuju commented Sep 4, 2023

jennijuju commented Nov 23, 2023

rjan90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaitlin-beegle 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

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

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

Choose a reason for hiding this comment

jennijuju commented Sep 4, 2023 •

edited

Loading