Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fip0081 gradual activation #967

Merged
merged 24 commits into from
Aug 15, 2024

Conversation

kkarrancsu
Copy link
Contributor

This pull request updates FIP0081 to include a ramped activation.

Pseudocode is provided for how a ramped activation could be realized. The pseudocode is written from the perspective of how to set the Gamma value (gamma_day) for each day of the network. This implies that there is a controller that is setting the Gamma value for each day and that controller would be active forever. It is possible to break out of the loop after Gamma reaches the target value of 0.7.

This pseudocode omits several realities that need to be taken into account when implementing in the network, including:

  • Absolute datetime or epoch numbers, due to the need for this to work in both testnet and mainnet
  • What entity/object is computing and setting the Gamma value
  • Whether Gamma changes every epoch or every day during the ramping period

FIPS/fip-0081.md Outdated Show resolved Hide resolved
Copy link
Member

@jsoares jsoares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something obvious but introducing a state machine here seems like unnecessary complexity and state, e.g.

  • Do we really need a pre-activation state given it's... erm... pre-activation?
  • Why not compute a gamma directly from activation, length, and date, thereby avoiding not just the state but the numerical considerations you mention? Something like $\gamma=0.7+\min(\max(0.3*\frac{(FIP \textunderscore activation \textunderscore date+activation \textunderscore len \textunderscore days) -datettime.today().days}{activation \textunderscore len \textunderscore days},0),0.3)$

Also, the while loop is underspecified as it could run with any frequency, but that's a bit pedantic esp. given your note on needing to choose a period. Regardless of the granularity, this should likely all be defined in epochs (i.e. activation epoch, activation_len_epochs, ...) to not be sensitive to clocks.

@kkarrancsu
Copy link
Contributor Author

I might be missing something obvious but introducing a state machine here seems like unnecessary complexity and state, e.g.

I don't disagree with anything you've stated, but this is just pseudocode. It likely has no correlation to how this would actually be implemented in Lotus, since things such as absolute dates are not available. I wrote it as a state machine because I thought it would be easy to understand.

FIPS/fip-0081.md Outdated
@@ -79,10 +79,28 @@ a reasonable lower bound on network collateral.

## Specification
Factor the per-sector initial consensus pledge into two parts: a 70% share which divides by the baseline function like today,
and a smaller 30% "simple" share which does not divide by the baseline function.
and a smaller 30% "simple" share which does not divide by the baseline function. Gradually activate the change from the current
to the target.
Copy link
Collaborator

@kaitlin-beegle kaitlin-beegle Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small ask, but can we also describe the activation period? It's helpful to have the qualitative description as well as the pseudocode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, added, but if unclear or could use further revision lmk and I can update!

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This is helpful in communicating the high level intention of a phase-in to humans. However the pseudocode as written can't function as a specification for implementers due to assuming a context for computation that doesn't match the chain's (which the PR description acknowledges). As such, I don't think we should merge this.

If we do want to merge something as a stepping stone to a full specification, I'd suggest avoiding pseudocode and just writing prose, with a TODO to actually specify how to implement that intent.

@kkarrancsu
Copy link
Contributor Author

I've updated the description and removed the pseudocode. Is it clearer now?

@@ -236,9 +281,5 @@ so the supply of storage is more than adequate to demand.

To be provided.

## TODO

- Implementation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still TODO. The description of intent is clear enough, but this currently lacks specification of how to achieve it.

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to view the attached PDF, but could only see 9 blank pages. Can someone else check this? @kkarrancsu do you want to try sending me the file by another channel too?

@kkarrancsu
Copy link
Contributor Author

I tried to view the attached PDF, but could only see 9 blank pages. Can someone else check this? @kkarrancsu do you want to try sending me the file by another channel too?

Hmm, not sure what happened there but I pushed a new pdf that is hopefully uncorrupted. Will also share directly with you.

@jsoares
Copy link
Member

jsoares commented May 8, 2024

I tried to view the attached PDF, but could only see 9 blank pages. Can someone else check this? @kkarrancsu do you want to try sending me the file by another channel too?

Hmm, not sure what happened there but I pushed a new pdf that is hopefully uncorrupted. Will also share directly with you.

The new one works for me.

@anorth
Copy link
Member

anorth commented May 9, 2024

New PDF 👍

@kkarrancsu
Copy link
Contributor Author

I've added a link to the draft implementation in the builtin-actors repository. Is it OK to move this FIP forward for consideration, while we continue to refine the draft implementation?

@kkarrancsu
Copy link
Contributor Author

I've added a link to the draft implementation in the builtin-actors repository. Is it OK to move this FIP forward for consideration, while we continue to refine the draft implementation?

Hi @jennijuju - gentle ping on this post Eth-CC!

@anorth
Copy link
Member

anorth commented Jul 18, 2024

I've added a link to the draft implementation in the builtin-actors repository. Is it OK to move this FIP forward for consideration, while we continue to refine the draft implementation?

What do you mean by move this FIP forward?

My original objection here still stands. The FIP does not specify how to achieve its outcome. From the draft implementation, I know that it has now been worked out how this can be achieved, but it's not represented in this document. This needs to be specified in the FIP. Linking to the code doesn't suffice for this.

This document must specify:

  • changes to state data structures
  • changes to actor APIs
  • briefly how those new state and APIs are used to get the right values to the right place to compute pledge amounts (prose is fine)
  • migration

I can lift my objection and allow this to merge as a draft, but the FIP will still not be ready for last call, so I don't want to mislead by doing that.

@jennijuju
Copy link
Member

@kkarrancsu - i agree with anorth, lmk if you want to do a pass tgt sometime this week!

@kkarrancsu
Copy link
Contributor Author

@kkarrancsu - i agree with anorth, lmk if you want to do a pass tgt sometime this week!

Hi @jennijuju - I've updated the Specification section to include a description of the necessary changes. Can you review and also help me understand how to think about Migration?

FIPS/fip-0081.md Outdated
The practical effect of this is that the sector initial pledge calculation will not tend to zero as the network baseline grows, but toward the `SectorInitialConsensusPledge` (30% of the pre-baseline-crossing total). When/if the baseline function exceeds network QAP, the per-sector initial pledge value will still decrease exponentially, but to this floor rather than toward zero.

### Built-in Actors Change Specification
In this section, we describe the changes required to the `builtin-actors` necessary to implement this update. The high level changes that need to occur is that the ramp configuration needs to be passed to the function which computes the consensus pledge.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: first sentence is redundant.

FIPS/fip-0081.md Outdated

### State Data Structures
**Market Actor**
1. `CurrentTotalPowerReturnParams` - Add ramp configuration parameters defined above to the `CurrentTotalPowerReturnParams` structure.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a copy of the parameter/return shape defined by the power actor. I think it's sufficient to define the change for the power actor only. Clearly the actors that call into it need to use the right schema. The existence of this copy of the structure is just a code artefact.

FIPS/fip-0081.md Show resolved Hide resolved
FIPS/fip-0081.md Outdated
but to this floor rather than toward zero.
**Miner Actor**
1. `NetworkPledgeInputs` - Add ramp configuration parameters defined above to the `NetworkPledgeInputs` structure.
2. `CurrentTotalPowerReturn` - Add ramp configuration parameters defined above to the `CurrentTotalPowerReturn` structure.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

FIPS/fip-0081.md Outdated

Under the updated formula for `SectorInitialConsensusPledge`, the network's current setting is `gamma=1.0`, which results in all of the `SectorInitialConsensusPledge` coming from `BaselineConsensusPledge`.

We recommend updating gamma from its current value of 1.0 to the target value of 0.7 over an period of 1Y. The technical reason for this recommendation is that gamma has a direct effect on pledge - changing this gradually will prevent pledge from sharply increasing, which will negatively affect SPs. There are additional economic considerations which support this recommendation and are discussed [here](../resources/fip-0081/gradual_activation_rationale.pdf).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No "we recommend" in a FIP. Just clearly define what the specification is. The fact that the FIP authors recommend it is implied.

@kkarrancsu
Copy link
Contributor Author

Hi @anorth @jennijuju - I think that the remaining changes are completed, can you take a look to see if it's ready to move to the next step?

FIPS/fip-0081.md Outdated Show resolved Hide resolved
@anorth anorth merged commit 3422557 into filecoin-project:master Aug 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants