(Alpenglow) Allow creating v4 account in genesis, upstream Consensus Pool.#8122
Conversation
|
The Firedancer team maintains a line-for-line reimplementation of the |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8122 +/- ##
=========================================
+ Coverage 83.0% 83.1% +0.1%
=========================================
Files 827 828 +1
Lines 362713 364668 +1955
=========================================
+ Hits 301254 303313 +2059
+ Misses 61459 61355 -104 🚀 New features to boost your workflow:
|
wen-coding
changed the title
| let credits = if let Ok(vote_state_v3) = VoteStateV3::deserialize(vote_account.data()) { | ||
| vote_state_v3.credits() | ||
| } else { | ||
| match VoteStateV4::deserialize(vote_account.data(), voter_pubkey) { |
There was a problem hiding this comment.
Just checking - is it possible for someone to create a VoteStateV4 on chain and then this fn has divergent behavior depending on software version?
Does this need to be part of a feature flag?
There was a problem hiding this comment.
That's a good point, I added stake_state::create_alpenglow_account which is only called from genesis when is_alpenglow is true. So when creating stake state on testnet/mainnet the stake program only recognizes VoteStateV3 accounts for now. We can remove it once we launched VoteStateV4 and converted all VoteStateV3 accounts.
|
Looks good from my end, please wait for approval from Jon/Joe for the program changes |
|
LGTM |
| let credits = if is_alpenglow { | ||
| let vote_state_v4 = VoteStateV4::deserialize(vote_account.data(), voter_pubkey).unwrap(); | ||
| vote_state_v4.epoch_credits.last().map_or(0, |(_, c, _)| *c) | ||
| } else { | ||
| let vote_state = VoteStateV3::deserialize(vote_account.data()).expect("vote_state"); | ||
| vote_state.credits() | ||
| }; |
There was a problem hiding this comment.
I'm currently adding the implementation of v4 to the Vote program, and I think this change will have to get captured in that PR, so that it coincides with the feature gate I'm adding. In fact, I believe I need to add this now to get it to pass CI.
Is it feasible to rebase this onto #8163?
There was a problem hiding this comment.
Just to make sure we are on the same page:
What we needed for Alpenglow upstreaming:
- Adding v4 account for tests:
create_v4_account_with_authorizedwas added in Add create_v4_account_with_authorized and actually populate bls_pubkey_to_rank_map in EpochStakes. #8103 as a temp workardound, but after vote-program: handler: init v4 support #8120 we should convert it tocreate_new_vote_state_v4_for_tests - We need to create v4 account only genesis config:
2.1 We are adding genesis util functions here
2.2 To make stake account creation work we are performing the operation here, you are saying once vote-program: outfit to fully support vote state v4 #8163 lands we should convert that to
let mut vote_state = VoteStateHandler::deserialize_and_convert(vote_account, VoteStateTargetVersion::V4)?;
Of course we can do that. Do you know when #8163 will land?
There was a problem hiding this comment.
Oh, create_v4_account_with_authorized uses create_new_vote_state_v4_for_tests, so I guess we are good there. Is the only difference on #8163 to change:
let vote_state_v4 = VoteStateV4::deserialize(vote_account.data(), voter_pubkey).unwrap();
to
let vote_state = VoteStateHandler::deserialize_and_convert(vote_account, VoteStateTargetVersion::V4)?;
?
There was a problem hiding this comment.
Yes, exactly, and that should be covered by my PR once it's fully updated, since I'll need to update any callsites that don't yet know about the v4 state feature gate.
There was a problem hiding this comment.
Cool, so sounds like it works both ways, if you manage to submit #8163 first, then I'll sync and change to the above call. If we submit this PR first, then it's just one-line change, so shouldn't be a big deal either.
In that case, is it okay if we ship this first to unblock other Alpenglow upstreaming?
Does rest of this PR look roughly correct to you?
There was a problem hiding this comment.
I can really only attest to the program stuff. I can't offer much of a review on the consensus side. But if you need the change immediately, sure, you can ship.
wen-coding
force-pushed
the
upstream_consensus_pool
branch
from
56a252b to
e9eae6b
Compare
|
Agreed with @buffalojoec, all of the logic outside of votor should hinge around vote state v4 being enabled, and not alpenglow. So #8163 should have landed first to make this change cleaner. Now all of the alpenglow-specific code in the non-votor files should be removed in #8163. Why did we need to land this so soon? |
@bw-solana Because we are now still developing in Alpenglow repo and syncing between Agave/Alpenglow is painful and we find breaking changes here and there, so pushing Alpenglow repo upstream even one week earlier helps the whole project a lot. We could of course wait for #8163 if it lands in a few days, but I don't want to rush Joe's work either, #8163 needs to be properly and carefully written so it handles all edge cases in VoteStateV4 transition. While after chatting with Joe, I think we figured
So it seems to be an okay compromise to me. Did I misunderstand any of the comments above? |
And yes, we are totally okay with replacing any functions outside votor with VoteStateV4 names if possible, including all the genesis functions. However, there is a difference between a VoteStateV4 account and an Alpenglow vote account, because an Alpenglow vote account needs to:
That said, if you think there is any function we should rename, I can totally send PR to fix it. |
|
The urgency is around getting out of sync/integration hell. We're trying to keep over 100k divergent LOC in sync between the repos. We absolutely need to maintain a high Agave quality bar, but the options as I see them:
|
|
VoteStateV4 is close. #8163 hasn't moved because I've been adding some more tests piecemeal like #8178 and #8191, and I also have to go through any other libs/tests that are going to break once the v4 feature is added. That's what I'm doing right now. It's easier for me if I don't have to sort through Alpenglow stuff when I update the stake stuff to bind to the v4 feature gate, but I also don't want to hold anyone up if it's critical. Regardless, I've earmarked this PR to evaluate against in #8163. |
Totally understood. Sorry for the additional trouble and thank you so much for being so flexible! Greatly appreciate it! |
|
@buffalojoec - you're a legend 🙏 Want to make sure everyone understands the urgency from our side, but also don't want you to feel pressured to take shortcuts, rush things, or feel bullied into any bad decisions. Lmk if we can help in any way |
|
Gotcha, if this is super urgent, then no problem! I just want to make sure we're not creating too much future work for ourselves. Every time we add |
|
Totally agreed. Unfortunately genesis is where I think we need to special case for now, because we need to create genesis with Alpenglow accounts before it's ready, we will be super careful in all other places. |
7 participants
create_genesis_config_with_alpenglow_vote_accountsto create Alpenglow vote accounts (VoteStateV4 account with bls_pubkey specified)is_alpenglowoption to the following:create_genesis_config_with_vote_accounts_and_cluster_typecreate_genesis_config_with_leader_excreate_genesis_config_with_leader_ex_no_featuresstake_state::create_alpenglow_accountso when creating stake accounts for genesis config whenis_alpenglowis true, it reads VoteStateV4 vote account data for credits. In all other cases it still reads VoteStateV3 vote account data