SIMD-0391: Stake Program Float to Fixed-Point #391
Conversation
grod220
force-pushed
the
stake-float-to-fixed
branch
from
f514ee4 to
b49c9f2
Compare
t-nelson
left a comment
t-nelson
left a comment
There was a problem hiding this comment.
firstly some basic stuff. i thought this was all documented in simd0001, but apparently that is not the case, so i'm going to elaborate here as a reference for foundation
simd's are intended to document protocol changes, not implementation changes. as such they should be written in a programming language and implementation agnostic way. imagine that you are sitting down to do a clean room implementation with only this document and knowledge of your intended target language. prefer prose. avoid code references where possible. where necessary, use pseudo code rather than a specific language
aside from the general corrections above and inline comments, a few things crossed my mind while reading this.
- do we want to depend on native u128 support landing in upstream ebpf or should we have a contingency for wide/arbitrary integer arithmetic? if the latter, that would make sense as its own simd
- i believe there are other instances of floating-point arithmetic in protocol, under consensus. would a generic "floating-point to fixed-point" migration simd be valuable to avoid needing to restate some of the details? we can just use that as a reference and stick to the specifics for each instance
|
Thanks for the review @t-nelson. Overhauled this SIMD to align with the the spirit of your feedback on being implementation agnostic.
This is currently in progress, but
I'm not sure if that would reduce redundancy as the motivation is specific to an on-chain program wanting to be aligned with the upstream compiler reqs. Not sure if agave's motivations would be the same. |
t-nelson
left a comment
t-nelson
left a comment
There was a problem hiding this comment.
we need to be clear of signededness of arithmetic throughout
grod220
left a comment
grod220
left a comment
There was a problem hiding this comment.
Also added clarification on the signedness of the integers throughout.
t-nelson
left a comment
t-nelson
left a comment
There was a problem hiding this comment.
getting close now by my eye. probably worth grabbing fd/blueshift reviewers
grod220
left a comment
grod220
left a comment
There was a problem hiding this comment.
Will chase down additional reviewers 👍
deanmlittle
left a comment
deanmlittle
left a comment
There was a problem hiding this comment.
thanks for addressing all of the feedback here. lgtm.
t-nelson
left a comment
t-nelson
left a comment
There was a problem hiding this comment.
lgtm now thanks!
cc/ @bw-solana for viz
|
Thanks, t-nelson!
|
| ### Rate representation (basis points) | ||
|
|
||
| The current network warmup/cooldown rate is 9%. This means that, in any given | ||
| epoch, at most 9% of the previous epoch's effective stake can be activated or | ||
| deactivated. | ||
|
|
||
| Currently, this figure is represented in floating-point: `0.09`. The new | ||
| representation is an integer of basis points: `900`. |
There was a problem hiding this comment.
Does this SIMD include any change to the value of warmup_cooldown_rate stored on-chain in the stake accounts? I don't think it does but could we clarify this here 🙏
There was a problem hiding this comment.
Originally I had a section about this, but was requested to remove it due to irrelevance as that field is deprecated and continues to be: #391 (comment)
| # All params are unsigned 64-bit integers | ||
| function rate_limited_stake_change(account_portion, cluster_portion, cluster_effective): | ||
| if account_portion == 0 or cluster_portion == 0 or cluster_effective == 0: | ||
| return 0 | ||
|
|
||
| # Cast all params to unsigned 128-bit integer | ||
| # All multiplications saturate | ||
| numerator = account_portion_128 * cluster_effective_128 * RATE_BPS_128 | ||
|
|
||
| denominator = cluster_portion_128 * BASIS_POINTS_PER_UNIT_128 | ||
|
|
||
| allowed_change_128 = numerator / denominator | ||
|
|
||
| # Never allow more than the account's own portion to change | ||
| if allowed_change_128 > account_portion_128: | ||
| allowed_change_128 = account_portion_128 | ||
|
|
||
| # Narrow back to unsigned 64-bit integer | ||
| allowed_change = allowed_change_128 as unsigned 64-bit integer | ||
|
|
||
| # Minimum progress clamp | ||
| if allowed_change == 0: | ||
| return 1 | ||
|
|
||
| return allowed_change | ||
| ``` |
There was a problem hiding this comment.
Can we use sat_mul_u128/sat_div_u128 functions in these pseudocode blocks? To make it clear that we are using saturating operations.
Would also be great to clarify the signedness and size of each of the variables, maybe declaring them with "types" such as u64 u128 etc.
There was a problem hiding this comment.
There was a problem hiding this comment.
imo we should be able to implement this function just from looking at the pseudocode. some form of types would make that a lot easier
There was a problem hiding this comment.
Let me know what you think about the most recent update. The ### Pseudocode conventions section establishes types / functions used later on to hopefully make things clearer.
| - **Stake Activation and Deactivation**: When querying a stake delegation's | ||
| status for a given epoch, the validator _computes how much of the | ||
| delegation's stake has completed warmup or cooldown_. This requires | ||
| walking through epochs from the delegation's activation or deactivation | ||
| point, computing the allowed stake change at each epoch boundary to | ||
| determine the portion that transitioned. The result categorizes the | ||
| delegation's lamports into effective, activating, and deactivating | ||
| buckets. | ||
| - **Epoch Boundary Stake History**: At each epoch boundary, the validator | ||
| iterates over all stake delegations and _computes their activation status_ | ||
| as of the concluding epoch. These per-delegation values are summed to | ||
| produce the cluster-wide totals (effective/activating/deactivating) that | ||
| form the new stake history entry. This entry is then used as input for | ||
| subsequent epoch calculations. | ||
| - **Stake Cache Updates**: The validator maintains a cache mapping vote | ||
| accounts to their delegated stake. When a stake account is | ||
| created/modified/closed, the cache entry for the associated vote account | ||
| MUST be updated. This requires _computing the delegation's effective stake_ | ||
| contribution before and after the change to correctly adjust the cached | ||
| totals. | ||
| - **Vote Account Stake Totals**: At epoch boundaries, the validator | ||
| refreshes the stake distribution across vote accounts for the upcoming | ||
| epoch. For each vote account, it _sums the effective stake_ of all | ||
| delegations pointing to that account. These totals determine leader | ||
| schedule weights and fork choice voting power. | ||
| - **Inflation Rewards**: Reward calculation iterates over each epoch in a | ||
| vote account's credit history. For each epoch, the validator _computes the | ||
| delegation's effective stake_ at that epoch, multiplies by the earned vote | ||
| credits to produce points and accumulates these across epochs. The final | ||
| reward is proportional to the delegation's share of total cluster points. | ||
| - Note: Only the effective stake computation (warmup/cooldown) is | ||
| affected by this SIMD. The downstream reward-to-lamport conversion | ||
| and sub-lamport deferral logic remain unchanged. |
There was a problem hiding this comment.
I had a look, and from what I can tell the point of divergence for all of these comes down to stake_and_activating.
Do you mind:
a) specifying whether this is true or not, and if not which other calculations need to change? I don't mean passing down in the call stack, but places where the actual calculation needs to be updated. I think it's just stake_and_activating but would be good to confirm.
b) outlining pseudocode for the fixed-point version of stake_and_activating (stake_and_activating_v2 in Agave) and how this differs from the current version of stake_and_activating
There was a problem hiding this comment.
the point of divergence for all of these comes down to stake_and_activating
It comes down to updating+feature gating two paths. We versioned the functions in the PR w/ v2:
- Activation path:
stake_and_activating_v2 - Deactivation path:
stake_activating_and_deactivating_v2
Down the call stack, the v2 versions eventually call rate_limited_stake_change, where the fixed point math updates are.
outlining pseudocode for the fixed-point version of stake_and_activating (stake_and_activating_v2 in Agave)
The SIMD already provides the pseudocode for rate_limited_stake_change, which is the actual mathematical formula that needs to change. That's the spec-level detail.
stake_and_activating / stake_and_activating_v2 are stake program/agave-specific functions that are responsible for quite a bit more (looping through epochs, checking history, etc). It's just their use specifically of calculating allowed stake change (internal or helper) that is relevant to the SIMD and needs feature gating.
6 participants
Proposal to migrate floats in stake-interface to fixed point