-
Notifications
You must be signed in to change notification settings - Fork 253
SIMD-0357: Alpenglow VAT implementation #357
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
Conversation
| - If several validators have exactly the same amount of stake and including | ||
| all of them would exceed the 2,000 limit, then all of them are excluded | ||
|
|
||
| 5. Subtract 1.6 SOL from the corresponding vote identity account in the list of |
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 happens if the vote identity doesn't have 1.6 SOL? We should also probably restrict vote identities to being a system owned account so that we don't unintentionally subtract lamports from a locked stake account or something
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.
The proposal right now is to exclude those accounts without sufficient funds from staked validators in the new Epoch, so they can't vote or be leaders.
| - a BLS pubkey | ||
|
|
||
| - at least 1.6 SOL VAT fee plus the necessary storage rent amount for a new | ||
| epoch in its corresponding identity account |
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.
any reason we choose identity vs vote account?
Implementation wise it seems more complex as multiple vote accounts can specify the same identity
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.
No please, I'm in favor of the current design. All rewards go into the validator's identity and no sol goes into vote account (unless there's commissions). It'll be a pain to keep 1.6 sol/epoch in vote account.
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 already have a precedent that validator's identity account pays vote tx fees so I agree we should keep it as the identity. But @AshwinSekar makes a great point that we need to figure out what to do if multiple vote accounts specify the same identity account and the account doesn't have a high enough balance to pay for all of them.
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.
Yeah the identity account was mostly chosen because currently all rewards go there, so if someone has enough rewards to cover VAT they don't ever need to worry about transferring money into vote accounts.
Per the current algorithm, if multiple accounts use the same identity account, the fees will be deducted in descending stake order (then descending pubkey). If you have 3 vote accounts using the same identity but your fee can only cover 2, then the two higher staked accounts get in while the third one is out.
If people are okay with that behavior, I'll specify it in the SIMD.
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.
@jstarry I added the same comment below :) I'd fix 1-1 mapping at epoch boundary.
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.
Also, are you implying we should send rewards to vote account instead in Alpenglow?
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.
Wait, I think @tigarcia explained to me a lot of people don't actually have the private key to their vote accounts any more, because we didn't really have any operations requiring that.
Are we implying we need everyone to recreate vote accounts?
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.
Okay, scratch my false statement above, I think choosing vote account is fine. The only caveat is we need to move block rewards and prio fee destination from identity account to vote account, then people don't need to constantly top up their vote accounts for VAT fees.
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 need to move block rewards and prio fee destination from identity account to vote account
yes but keep that out of this simd. it's a separate optimization and doesn't need to muddy vat
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.
Okay, I changed this SIMD to deduct VAT from vote account, we can put rewards into vote account in another SIMD.
| - at least 1.6 SOL VAT fee plus the necessary storage rent amount for a new | ||
| epoch in its corresponding identity account | ||
|
|
||
| When the staked validators for a new epoch is calculated, the leader will |
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.
this affects all uses of staked validators - turbine, leader schedule etc correct? would be good to specify.
Also any behavior regarding staked validator overrides - if I have it set does it create a 2001 spot or replace a validator.
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.
Today this is not the case. The turbine tree is generated with a list sorted by (stake, identity), while the leader schedule is generated from the same list sorted by (stake, vote). So the lists are currently different. I'm of course highly in favor to unify, but that's prob a separated turbine-specific simd?
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.
Hmm do we have staked validator overrides for mainnet or testnet? I suppose staked validator overrides should be considered before we do the sorting, so it doesn't create the 2001th spot, it kicks one validator in the 2000 list out.
Err, I think currently Turbine gets node stake from the same list we are calculating here, so yes Turbine will be affected, unless we intentionally give Turbine a different list. Is there a reason we don't want unity here?
That said, theoretically we should never hit this 2000 limit, so we should be fine...
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.
The core issue is just that SIMD-0180 was done only for leader schedule and not for turbine tree. So now we seed the two randomizations with different lists. What I wanted to say is that I'm in favor of unifying, but maybe we should just do an independent simd to re-align turbine tree with the new list.
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.
For the record, we discussed this at the SLC dev meeting, I believe the current consensus is that it's okay to unify the list, since we should almost never hit the 2000 limit, this should be fine and unifying simplifies the reasoning.
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.
so just to clarify this affects all usages of validator stake / total stake in the protocol? Or could you specify exactly when we use this filtering vs the "normal" stake view?
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.
This will affect all usages of validator stake / total stake in the protocol.
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.
woops accidentally approved, meant to comment
|
|
||
| 3. The calculation goes through all vote accounts and filter the following: | ||
|
|
||
| - Contains at least 1.6 SOLs as balance in its corresponding identity account |
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.
Currently, the same identity could be used on multiple vote accounts. 😭
My strong preference is to enforce that, for every epoch, there's only 1 vote account per identity (if there's more, we can pick the vote account with higher stake). This simplifies a ton of other things.
Alternatively, we'd have to handle the case where the identity has enough sol for some vote accounts but not for all, and we'd have to agree on which ones we'd include and exclude.
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.
Would be ideal if we can enforce 1-1 mapping of vote account to identity account, less confusion. Let me chat with Foundation folks to see whether there is any legitimate use of multiple vote accounts for one identity account.
- Per [discussion](solana-foundation/solana-improvement-documents#357 (comment)) in VAT SIMD, we will enforce the restriction that each node identity should be 1:1 with vote accounts, no need to test other cases - Use standard gensis config creation to set epoch_stakes
Co-authored-by: Trent Nelson <[email protected]>
Co-authored-by: Trent Nelson <[email protected]>
Co-authored-by: Trent Nelson <[email protected]>
t-nelson
left a comment
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.
getting pretty close now. probably time to get another review from the firedancer team
|
|
||
| The validator admission ticket is a mechanism translating the current cost of | ||
| voting into a similar economic equilibrium for Alpenglow. By charging every | ||
| voting validator 1.6 SOL per epoch, it replaces the current voting fee at ~2 |
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.
nit: we repeat 1.6 SOL throughout the document. replace with simply VAT other than this first instance
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.
Sure, I kept the first one and the one in New Terminology, removed all other ones.
| 6. Record the VAT fee subtraction in the bank, move the lamports directly into | ||
| the incinerator account. | ||
|
|
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.
not a change for the simd, but wherever we're tracking the agave implementation, we need to emit an appropriate RewardInfo entry for each of these debits in the corresponding block. it is unacceptable to lose this accounting information
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.
Hmm, I did a quick search for RewardInfo in Agave, I don't see a case where the reward is negative. Would it break someone's tool if we suddenly have a negative number in RewardInfo? Should we add it to some other entries?
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.
the RewardInfo::lamports field was converted from u64 to i64 specifically to record rent debits (gh indexing of old labs repo is busted, so i can't find it). likely this has been removed since rent collection was disabled
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.
Okay, in that case I'll just add it to this SIMD so we don't forget to do it. Changed.
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.
implementation details do not belong in simds. this is not an agave todo list
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.
Okay, removed. Just for my education, which team is supposed to do this?
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.
the field is not under consensus. it's only metadata
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.
Sounds good, any more comments on this SIMD?
|
Thanks, lidatong!
|
bw-solana
left a comment
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.
Approving from Anza side
|
Thanks, bw-solana!
|
|
/merge |
|
iabdulin Only the PR author (wen-coding) can run the
|
|
/merge |
|
wen-coding
|
|
/merge Please work some Christmas magic! |
|
iabdulin Only the PR author (wen-coding) can run the ✅ Status: Ready to merge |
|
/merge |
|
✅ Merge successful! wen-coding's PR has been merged. |
No description provided.