SIMD-0148: MoveStake and MoveLamports instructions

[2501babe]

two new instructions for moving value between stake accounts without holding Withdrawer

[2501babe]

cc @joncinque for discussion, and if you could bring in the marinade guys

[joncinque]

Looks great to me! Mostly small things to address

[joncinque]

Although this is an edge case, the credits_observed on the destination account must be updated to the weighted average of the source and destination accounts, like during merge: https://github.com/anza-xyz/agave/blob/9403ca6f0451b670d41dee1b7592daa297727ed1/programs/stake/src/stake_state.rs#L1188, where you would do (source_credits_observed * amount + destination_credits_observed * destination_amount + (amount + destination_amount) - 1) / (amount + destination_amount)

[2501babe]

ill confess i dont understand the full implications of this and thought credits_observed was determined by, and advanced in lockstep with, epoch_credits on VoteState

i see how when two accounts are merged, the destination credits become a stake-weighted average of both accounts, but move is more complicated. checking my intuitions:

• partial move between active accounts. eg U1 and U2 have 100 sol each, U1 has C1 credits observed, U2 has C2 credits observed, and 50 sol moves U1 -> U2. this means that 33.3% of the new C2 value is provided by C1, because 1/3 of U2's stake have observed the credits seen by U1?
• full move between active accounts is identical to merge
• a move from active to inactive doesnt need to fiddle with credits observed because, much like delegation, it is a garbage value when inactive and assigned a fresh value when activated that doesnt depend on its prior value
• source credits observed never change because "credits observed" is more like an experience that a particular piece of stake has had rather than a quantity of something it carries with it

is this accurate?

[joncinque]

It's almost always advanced, except for a few edge cases, such as stakes that don't earn any rewards in an epoch.

And yep, you've correctly covered all of the situations and what should happen.

[ripatel-fd]

What error codes do these conditions produce? And what is the logger output?

[2501babe]

is this really necessary to include in specifications for core program instructions?

defining all this would drastically increase the burden of writing specs and of implementing those specs. you would have to worry about the exact order errors are checked in, structure programs such that the language runtime and vm can never trigger their own error conditions for situations that would be exceptional before they're checked explicitly, and possibly rewrite specs for any nontrivial refactor or any improvement to error or logging information

even simple things like nested match expressions become a potential headache because if error A has to happen before error B, and error A is returned by the wildcard pattern but error B is in a matched block, then error B will happen second but come first by line number, and you have to start worrying "is the order here right? is the order obvious enough to someone looking at this code for the first time? should i just unwrap_or_else everything instead of writing structured code?"

if we say "you have to define the exact error codes returned for each abort condition and the precise log outputs that would result from any operation" then we effectively incentivize maintainers to use as few error codes as humanly possible and log nothing

my original version of this reply continued the first line "these are only ever consumed outside of consensus" but upon further reflection i realize that if we dont go through with porting core programs to bpf, such that there are two native versions, then error codes need to agree between them for the sake of programs that call them via cpi. but i think we shouldnt add the burden of defining error codes in spec unless we decide there will be multiple core program impls, and shouldnt define log output in spec at all

[ripatel-fd]

We can only realistically provide an RPC replacement if we get the error codes and log messages right.

[2501babe]

sorry, i dont understand how this would affect rpc if theres one bpf implementation of the stake program. wouldnt you pass through or remap any errors or log messages using the same rules as the existing rpc, without concern for the underlying program?

[ripatel-fd]

sorry, i dont understand how this would affect rpc if theres one bpf implementation of the stake program.

@2501babe I assumed this SIMD changes the behavior of the stake native program. The SIMD doesn't say this change would only affect the core BPF version of the stake program. Even so, I suggest documenting error codes and log messages so API services / indexers / explorers know how to interpret the output.

[2501babe]

ok i updated to cover all of the comments ive marked resolved. accounts and instruction data are now defined precisely, i updated the spec to include the "full move" variant, and got rid of uses of "fully" but defined exactly what we mean by "active" and "inactive." the last thing to address in the spec is the exact logic for modifying credits_observed

also while i was implementing MoveStake i reread the Merge impl and noticed that lockups arent cleared on expiration, so i updated the language to indicate that only in-force lockups must match

[ripatel-fd]

@2501babe FYI I asked @lheeger-jump for a review. He will take a look soon.

[janlegner]

@2501babe thanks for this proposal, it looks awesome - and it will make things at Marinade much easier.

[lheeger-jump]

Okay it took like 3 read throughs over the course of the last month to really understand this proposal. Some questions I have:

• When does moved stake come active?
• How does this affect the future of slashing?
• Are there other problems this solves that the alternatives did not?
• There are a maximum amount of stake changes allowed per epoch, or am I misremembering that? Does that affect this proposal?
• Are there any drawbacks?
• If this change launches, what are the measurable effects it will have that we can look at to measure efficacy?

[lheeger-jump]

so not deactivating or activating, correct?

[2501babe]

correct. i defined active/inactive earlier in the simd since there was some quibbling over previous use of "fully active" (not being an official term), but the only allowed states are Initialized, Stake and fully active, or Stake and fully deactivated. Uninitialized, activating (active next epoch), deactivating (inactive next epoch), and in any warmup/cooldown state (partially active/inactive due to whole-cluster activation/deactivation) are all abort states

[lheeger-jump]

Does this move the two accounts in activating/deactivating?

[2501babe]

deactivating isnt allowed for either account, and activating is only allowed for the destination for MoveLamports. tbh i describe the allowed states from first principles for the sake of letting the simd stand alone, but a lot of these are just reusing logic from Merge, eg here destination is identical to a valid merge destination, and source is a valid merge source except it cant be activating because this would require calculating whether the minimum will be upheld next epoch (which is pointless because theres no usecase)

[lheeger-jump]

Thank you!

[lheeger-jump]

This proposal is getting into very good shape. All I have are questions.

[2501babe]

addressing the rest of @lheeger-jump comments:

• When does moved stake come active?

as described in code comment replies, moved stake never changes activation status

• How does this affect the future of slashing?

i dont believe it does, assuming slashing is a penalty applied to all the stake accounts of a vote account proportional to their active delegations. if there are other factors im missing please elaborate tho

• Are there other problems this solves that the alternatives did not?

the main reason for doing it this way is multistake is too complicated to move through the simd process and implement in a reasonable timeframe (and may not even cover the full usecase) and the alternative to adding new instructions is changing both Split and Merge to be more permissive. also on a philosophical level it would muddy the relatively clean concepts that both represent (division and unification) by making split more merge-like and merge more split-like

• There are a maximum amount of stake changes allowed per epoch, or am I misremembering that? Does that affect this proposal?

youre thinking of warmup cooldown, which is a system where if more than x% (presently 7, used to be 25) of stake on a whole cluster is activated or deactivated in one epoch, then in the next epoch, only that % of effective stake will change. the reason for the "this is required to be fully active, this is required to be fully active or inactive" etc rules for accounts with these instructions is so that none of that applies to us, we reject any accounts that are in transient states

• Are there any drawbacks?

uhhhh. well its more instructions to support. i cant think of any negative results tho, it doesnt allow any new behavior, it just lets you shuffle value around using Staker when you dont have Withdrawer, but only between accounts that have the same authorities

• If this change launches, what are the measurable effects it will have that we can look at to measure efficacy?

we should expect to see an appreciable reduction in the absolute number of stake accounts (not just a reduction in the rate of increase)

[CriesofCarrots]

Lgtm

[lheeger-jump]

Lets let it sit for a bit, and merge in a week. We should coordinate on implementation timelines separately.

[joncinque]

Merging since there haven't been any objections over the last week -- thanks everyone!