Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SIMD-0148:
MoveStake
andMoveLamports
instructions #148SIMD-0148:
MoveStake
andMoveLamports
instructions #148Changes from 1 commit
4e5aadf
23e811e
8c24903
af14e2c
59e33b2
2aa4f4b
5049f87
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error codes do these conditions produce? And what is the logger output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 justunwrap_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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can only realistically provide an RPC replacement if we get the error codes and log messages right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ill confess i dont understand the full implications of this and thought
credits_observed
was determined by, and advanced in lockstep with,epoch_credits
onVoteState
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:
is this accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this move the two accounts in activating/deactivating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fromMerge
, 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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!