SIMD-0392 / SIMD-0123: Adjust stake delegations during rewards payout due to rent

[joncinque]

As part of rent reductions and increases, we need to change the stake delegation calculation at rewards payout.

[simd-bot]

Hello joncinque! Welcome to the SIMD process. By opening this PR you are affirming that your SIMD has been thoroughly discussed and vetted in the SIMD discussion section. The SIMD PR section should only be used to submit a final technical specification for review. If your design / idea still needs discussion, please close this PR and create a new discussion here.

This PR requires the following approvals before it can be merged:

• At least one approval from a Anza team member: t-nelson, steviez, or bw-solana
• At least one approval from an Firedancer team member: ptaffet-jump, topointon-jump, 0x0ece, lidatong, or ripatel-fd

Once all requirements are met, you can merge this PR by commenting /merge.

[joncinque]

@igor56D I can't add you as reviewer, but please take a look too

[t-nelson]

can we update the pr title? there's no stake acccount context there

[bw-solana]

LGTM (pending title update per Trent's request).

If we happen to activate features for SIMD-0438 and this one at the same time, would we expect this one to activate first? I.e. we would run this logic on the epoch boundary where it is activated to top off stake accounts to the increased minimum?

[simd-bot]

Thanks, bw-solana!

⚠️ Status: Cannot merge yet

• Missing approval from: Firedancer (ptaffet-jump, topointon-jump, 0x0ece, lidatong, or ripatel-fd)

[joncinque]

If we happen to activate features for SIMD-0438 and this one at the same time, would we expect this one to activate first? I.e. we would run this logic on the epoch boundary where it is activated to top off stake accounts to the increased minimum?

Good call: the rent changes MUST activate first, since the reward calculation need the updated rent values. We're already in this situation in Agave thankfully: https://github.com/anza-xyz/agave/blob/dced5bc552297587c6fbe98164c325fd4d9fe64f/runtime/src/bank.rs#L1737

I've specified this explicitly, let me know what you think

[simd-bot]

Thanks, bw-solana!

⚠️ Status: Cannot merge yet

• Missing approval from: Firedancer (ptaffet-jump, topointon-jump, 0x0ece, lidatong, or ripatel-fd)

[t-nelson]

as commented elsewhere i believe this should be an amendment to 438. they're both incomplete treatments to the issue on their own

#488 (comment)

[t-nelson]

Suggested change

	partitioned rewards payout system of
	epoch rewards payout system of

throughout

partitioning is an irrelevant implementation detail of an adjacent feature

[t-nelson]

Suggested change

	max(
	min(
	delegation + stake_rewards,
	lamports + stake_rewards - rent_exempt_reserve
	),
	0
	)
	post_delegation = max(
	min(
	pre_delegation + stake_rewards,
	lamports + stake_rewards - rent_exempt_reserve
	),
	0
	)

for clarity

[t-nelson]

Suggested change

	The `rent_exempt_reserve` calculation MUST use current `Rent` sysvar parameters,
	which MAY be updated immediately prior to reward calculation, during the epoch
	boundary.
	The `rent_exempt_reserve` calculation MUST use the cluster parameters stored in the `Rent` sysvar. Any updates to the `Rent` sysvar values MUST take place before epoch rewards calculation takes place.

i found the "MAY" a bit ambiguous. i think it was intended to suggest that the sysvar values can change, but the wording could have been misinterpreted as referring to the ordering of the update and rewards calculation

[t-nelson]

the offets here are relative to the start of the StakeState enum and dependent on the value of its variant, right?

[joncinque]

These are absolute offsets in the account, not counting the 4 bytes for the discriminant.

[t-nelson]

even more confusing! please document 😂

[t-nelson]

the arithmetic is all unsigned and saturating?

[joncinque]

I can specify that it's unsigned and saturating and remove the max part if that's clearer. That's how the implementation shook out in the end.

[t-nelson]

i think saturating math would be more in line with how a good implementation will look. like doing the wrong arithmetic and relying on max can introduce bugs on overflow

[2501babe]

we need to consider what to do with accounts where lamports + stake_rewards - rent_exempt_reserve is lte 0. consider the following series of events:

1. i have a grandfathered, staked 200*6960 + 1 stake account
2. rent is reduced to 696
3. i Withdraw 200*6264 lamports
4. rent is increased

unfortunately i just thought of this now and had been previously content with the "every account below 1sol has old rent, every account wth new rent has 1sol." Withdraw doesnt care about minimum delegation because it doesnt change activation status and doesnt change delegation (MoveLamports is similar)

im... inclined to leave the stake program as-is, i think instructions managing lamports shouldnt enforce delegation, these are separate concerns. if the post_delegation would be 0 we should just deactivate. the flow i detailed above would just be user trolling, so we get a little stake account spring cleaning if anyone actually does it

edit: and if id kept scrolling i would have seen the If the new delegation amount is 0 line, lgtm!

[joncinque]

I agree with your framing for the edge case on Withdraw -- let's keep minimum delegation out of it

[t-nelson]

also we should consider min delegation here. though i am just now realizing that not even the noop min delegation feature gate is activated on mb 🤔

[joncinque]

I thought about it, and went with the simpler approach of 0 -- I thought it would make more sense to treat the accounts as "below the minimum, but grandfathered in", similar to what'll happen when the minimum delegation is raised. But I can be convinced otherwise and force-deactivate if a delegation goes below the minimum.

Any thoughts @2501babe ?

[t-nelson]

oh we're grandfathering min delegation? that sounds... more complicated now

let's see what hana says

[2501babe]

my view was we would allow accounts to fall below the minimum delegation. it doesnt add complexity because unless we demonetize such accounts, there will be hundreds of thousands of accounts already below the minimum delegation anyway. its not really "the minimum amount any account will have as its delegation" so much as "the miniumum amount the stake program requires for certain state changes such as DelegateStake or Split

one side benefit i saw in increasing minimum delegation before rent decreases is new stakes will have plenty of buffer to dock delegation from. eg we will have lots of 200*6960lamp rent + 1lamp delegation accounts, but new 200*696lamp rent accounts will have 1sol delegation (with an important caveat in #488 (comment))

[t-nelson]

these are same behavior as today right?

[joncinque]

Yep, the partitioned epoch rewards system handles updating all of these through the stakes cache

[t-nelson]

in that case, if we think keeping these is useful, i think we should point out that there is no change being suggested here

[t-nelson]

same suggestion(s) as above. though i think this should technically be an amendment to there relevant 123 subtask simd

[joncinque]

Shall I move this part directly into the SIMD-123 document? I wasn't sure how often we should be modifying multiple SIMDs at once

[t-nelson]

this may be the first time it's come up (at least that i've been involved).

@jacobcreech wdyt? seems like in this case the 438/488 change is higher priority and likely to land first, so would make sense to adapt what we assume will change later. though we do probably need some guidance wrt simds with side-effects in other simds

[buffalojoec]

Can't we just amend SIMD-0123 to add a dependency to this one (SIMD-0488)? Then you can just specify it there and it will reference this one.

IMO the details for the block rewards version of the formula could go into SIMD-0123 instead if we added the dependency.

[joncinque]

No problem, I moved this part into SIMD-123, let me know what you think

[t-nelson]

s/Protocols/Dapps/? s/Protocols/Programs/? s/Protocols/Consumers/?

[joncinque]

I'll just enumerate them all

[igor56D]

Can we make this SIMD a pre-requisite for 438? I was planning on doing this myself but decided it made more sense to wait until this SIMD was posted, and putting it in the same PR probably makes sense.

[t-nelson]

i believe it makes most sense to merge them #488 (comment)

[joncinque]

I think making it a prerequisite makes the most sense. If we want to implement dynamic rent (meaning, with possibility to increase), we'll need this code is in place, regardless of 438's status.

[t-nelson]

we will not be changing rent in any way before it is safe to move it in both directions. it will not be changed with any combination of 438 and 488 that is not both active. all splitting these two simds is doing is extended the release schedule by creating two feature gates

[joncinque]

SIMD 438 increases rent back to the original value. If all goes well, it will never be enabled.

In my opinion, this SIMD, along with all of the on-chain program upgrades, should be enabled no matter what, since it's part of protecting the runtime in case rent needs to increase with a different proposal in the future.

[t-nelson]

sry meant 392, not 438. #488 (comment)

[brooksprumo]

This looks good to me. I'll echo Trent's comments w.r.t. the byte offsets in the stake account. I had to get rust analyzer to give me the field offsets in the stake types, but didn't factor in the enum discriminant 🫠. It would be useful to add a link/diagram for the stake fields layout (but not a blocker imo).

[2501babe]

this is true but not really the main concern, i would make this an "in addition..." after the main motivation, namely that we treat total_lamports - delegation - rent_exempt_reserve >= 0 as invariant. any onchain program that interacts with stake accounts in a meaningful way is adding and subtracting some subset of those variables in arbitrary ways

thus if a rent lamport can also be a delegation lamport, at best you will hit checked_sub() aborts for routine operations, at worst you might overestimate the value of an account and do horrible things like over-mint tokens

[joncinque]

Good point! I'll clarify that

[2501babe]

after more reflection, i want to emphasize this is correct and we must not allow delegation to be 0. what happens to StakesCache when we do this though? do we have a means to drop the entry?

[joncinque]

An adjusted delegation gets included in the set of "stake accounts that must be updated during distribution", and during distribution, we call bank.store_accounts at https://github.com/anza-xyz/agave/blob/7a2e721717e010c2a03c06c43aaaf9da81426d08/runtime/src/bank/partitioned_epoch_rewards/distribution.rs#L298, which triggers the StakesCache logic to set the delegation to correct amount: https://github.com/anza-xyz/agave/blob/7a2e721717e010c2a03c06c43aaaf9da81426d08/runtime/src/stakes.rs#L423

[joncinque]

In general, it doesn't seem like stakes with 0 delegation but rent-exempt lamports get dropped from the cache though

[brooksprumo]

it doesn't seem like stakes with 0 delegation but rent-exempt lamports get dropped from the cache

We should probably fix that, but it's an agave issue not a protocol one.

[buffalojoec]

Can't we just amend SIMD-0123 to add a dependency to this one (SIMD-0488)? Then you can just specify it there and it will reference this one.

IMO the details for the block rewards version of the formula could go into SIMD-0123 instead if we added the dependency.

[joncinque]

Comments have been addressed, so to my gracious reviewers, please give another look when you have a chance

[t-nelson]

as commented elsewhere i believe this should be an amendment to 438. they're both incomplete treatments to the issue on their own

sry mixed up the simds. i believe this should be and amendment to 392, not 438

[joncinque]

sry mixed up the simds. i believe this should be and amendment to 392, not 438

Ohhh that makes way more sense. I had to change quite a few things, but now they're combined, let me know what you think

[brooksprumo]

sry mixed up the simds. i believe this should be and amendment to 392, not 438

I believe this was my bad 😅

[brooksprumo]

lgtm overall, just a suggested wording nit

[brooksprumo]

As a wording nit, all of these things are denominated in lamports, but the first one lamports, is the account balance. We use the term "balance" above, and IMO would be clearer to use that here too. I'm guessing we're using lamports mainly because that's the name of the field in agave.

Could even say "0 lamports" at the end too, if you want.

Suggested change

	lamports - delegation - rent_exempt_reserve >= 0
	balance - delegation - rent_exempt_reserve >= 0 lamports

[joncinque]

Done for all them!

[brooksprumo]

Here too:

Suggested change

	lamports + stake_rewards - rent_exempt_reserve
	balance + stake_rewards - rent_exempt_reserve

[brooksprumo]

Suggested change

	- `lamports`: the account's pre-reward lamports
	- `balance`: the account's pre-reward balance, in lamports

[brooksprumo]

Can we do it here too?

Suggested change

	lamports + stake_rewards + block_rewards - rent_exempt_reserve
	balance + stake_rewards + block_rewards - rent_exempt_reserve

[brooksprumo]

Suggested change

	- `lamports`: the account's pre-reward lamports
	- `lamports`: the account's pre-reward balance, in lamports

[brooksprumo]

I missed this earlier. I think this file should have its references to SIMD-488 be updated to point to SIMD-392.

[brooksprumo]

Here though, we point to the PR not the blob/file. The file would need just one link, whereas linking to PR and getting the full SIMD-392 would require links to both PRs.

I don't have a strong opinion either way, just noting that we should probably fix.

[joncinque]

Probably better to link to the file, right?

[joncinque]

I updated the changed ones in one commit, and all the others in the following commit, so we can always drop the second one if people complain

[brooksprumo]

Would rent increases impact Alpenglow's VAT payment/calculations at all? Ideally that is specified in terms of Rent sysvar already, so should just work?

[joncinque]

Would rent increases impact Alpenglow's VAT payment/calculations at all? Ideally that is specified in terms of Rent sysvar already, so should just work?

From what I understand, validators will have to be sure to not overdraw their vote account. Here's the relevant part from that SIMD:

3. The calculation iterates all vote accounts and filters those that meet
the following criteria:

  - The account has a balance of at least VAT plus the necessary storage
  rent amount for a new epoch

That calculation MUST use the new rent amount. I imagine in most cases, a vote account will gain more than the VAT anyway, so this shouldn't be a problem unless there's a big jump in Rent.

[t-nelson]

@jacobcreech can we add a lint to prevent simds from linking to pull requests like this? we only want simds merged referencing merged simds. if there's valuable context in the pr comments, that should make it to the simd text, not be left to be dug out of the conversation

[t-nelson]

can we link to the appropriate section of 0392 instead of replicoding here?

[t-nelson]

Suggested change

	account
	account (see below)

[t-nelson]

do we need to consider updating block metadata to signal these occurrences since there will now be no onchain reference to the deactivation event nor easily accessible time of deactivation state snapshot?

[joncinque]

That's a very good question -- we could add a new bool field to Reward like instant_destake.

On the other hand, clients could figure it out based on the post_balance field and the rent parameters at the time of rewards payout. Is that clear enough?

Just to double-check, block metadata is out of protocol, correct?

[t-nelson]

That's a very good question -- we could add a new bool field to Reward like instant_destake.

On the other hand, clients could figure it out based on the post_balance field and the rent parameters at the time of rewards payout. Is that clear enough?

we don't have the rent parameters either tho. even if we did, we don't necessarily know the data size of the stake account at that time.

i was originally thinking add the min_balance field for each account, but we don't have enough info to make sense of that either.

i think easiest would be a new RewardType variant StakingInactive or similar. we could also emit it on requested deactivation final reward epoch.

Just to double-check, block metadata is out of protocol, correct?

technically, tho that's almost certainly a mistake. staking and validator rewards must be the most numerous taxable events in the protocol and there is no record of them without this metadata

[joncinque]

i think easiest would be a new RewardType variant StakingInactive or similar. we could also emit it on requested deactivation final reward epoch.

I didn't like the idea of adding a new status for just this edge case, but I'm down if we do it for all deactivations

[t-nelson]

way easier than changing layout 😅

[joncinque]

Ok, all comments should be addressed, I humbly request stamps from my gracious reviewers

[simd-bot]

Thanks, t-nelson!

⚠️ Status: Cannot merge yet

• Missing approval from: Firedancer (ptaffet-jump, topointon-jump, 0x0ece, lidatong, or ripatel-fd)

[brooksprumo]

@topointon-jump Bump to see if someone on the FD side can take a look, please. Thanks!