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

Cleanup / DRY PD controller #2553

Closed
Tracked by #2529
cwgoes opened this issue Feb 7, 2024 · 3 comments · Fixed by #2575
Closed
Tracked by #2529

Cleanup / DRY PD controller #2553

cwgoes opened this issue Feb 7, 2024 · 3 comments · Fixed by #2575
Assignees
Labels
MASP PoS pre-mainnet Must happen before mainnet. security

Comments

@cwgoes
Copy link
Collaborator

cwgoes commented Feb 7, 2024

From a review of crates/core/src/ledger/inflation.rs:

  • We should be able to have a standard PDController which takes standard parameters and outputs the control input.
  • This PD controller should be wrapped by a small function for proof-of-stake which needs to calculate the locked ratio before running the controller.
  • We should double-check that we're using checked arithmetic (with the various numeric types and possible operator overloading, it's not clear to me).
  • We do not need to store the locked ratio in storage; we can simply recalculate it when needed (we do need to store the old locked ratio, just as we need to store the old target amount, but this should be done by the PD controller with no knowledge of proof-of-stake specific semantics).
  • This file should be renamed to pd-controller or something more indicative, a PD controller alone has nothing to do with inflation. It should also be moved to a standalone crate instead of being in core.
@brentstone
Copy link
Collaborator

brentstone commented Feb 9, 2024

Here's how I think this could be done: there are seven common input parameters between the PoS and Shielded controllers:

  • locked_tokens
  • total_native_tokens
  • max_reward_rate
  • last_inflation_amount
  • p_gain_nom
  • d_gain_nom
  • epochs_per_year

Then there are two different values between them that can be generalized as a target value metric_tar and a previous value metric_last. For PoS, this is a token ratio, and for MASP, this is a token amount.

We can have a struct PDController that is initialized with nine values - the seven above and metric_tar and metric_last. The inflation I is simply max[0, min(I_max, I_last + C)].

The computation of C is not super nicely generalized though - I think it would require two inputs to do something like PDController::compute_control(coefficient, metric_cur).

I'll come up with a proof-of-concept.

@brentstone
Copy link
Collaborator

We do not need to store the locked ratio in storage; we can simply recalculate it when needed (we do need to store the old locked ratio, just as we need to store the old target amount, but this should be done by the PD controller with no knowledge of proof-of-stake specific semantics).

We are already doing this - only the last locked ratio and last inflation amounts are stored.

@brentstone brentstone mentioned this issue Feb 9, 2024
2 tasks
@brentstone
Copy link
Collaborator

Started this in cb44f61 @cwgoes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MASP PoS pre-mainnet Must happen before mainnet. security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants