DeactivateDelinquent for Closed Vote Accounts [SIMD-0608] - #532
Conversation
9fc637b to
759f64b
Compare
| /// Deserialize a Vote Program-owned account as [`VoteStateV4`]. | ||
| #[cfg(feature = "bincode")] | ||
| pub fn get_vote_state(vote_account_info: &AccountInfo) -> Result<Box<VoteStateV4>, ProgramError> { | ||
| if *vote_account_info.owner != solana_vote_interface::program::id() { | ||
| return Err(ProgramError::IncorrectProgramId); | ||
| } | ||
|
|
||
| let mut vote_state = Box::new(MaybeUninit::uninit()); | ||
| VoteStateV4::deserialize_into_uninit( | ||
| &vote_account_info.try_borrow_data()?, | ||
| vote_state.as_mut(), | ||
| vote_account_info.key, | ||
| ) | ||
| .map_err(|_| ProgramError::InvalidAccountData)?; | ||
| let vote_state = unsafe { Box::from_raw(Box::into_raw(vote_state).cast::<VoteStateV4>()) }; | ||
|
|
||
| Ok(vote_state) | ||
| } | ||
|
|
There was a problem hiding this comment.
Moved from program/src/processor.rs so helpers here can use it
| if let StakeStateV2::Stake(meta, mut stake, stake_flags) = | ||
| let StakeStateV2::Stake(meta, mut stake, stake_flags) = | ||
| get_stake_state(stake_account_info)? | ||
| { | ||
| if stake.delegation.voter_pubkey != *delinquent_vote_account_info.key { | ||
| return Err(StakeError::VoteAddressMismatch.into()); | ||
| } | ||
| else { | ||
| return Err(ProgramError::InvalidAccountData); | ||
| }; | ||
|
|
||
| // Deactivate the stake account if its delegated vote account has never voted or | ||
| // has not voted in the last | ||
| // `MINIMUM_DELINQUENT_EPOCHS_FOR_DEACTIVATION` | ||
| if eligible_for_deactivate_delinquent(&delinquent_vote_state.epoch_credits, clock.epoch) | ||
| { | ||
| stake.deactivate(clock.epoch)?; | ||
| if stake.delegation.voter_pubkey != *delinquent_vote_account_info.key { | ||
| return Err(StakeError::VoteAddressMismatch.into()); | ||
| } | ||
|
|
||
| set_stake_state( | ||
| stake_account_info, | ||
| &StakeStateV2::Stake(meta, stake, stake_flags), | ||
| ) | ||
| } else { | ||
| Err(StakeError::MinimumDelinquentEpochsForDeactivationNotMet.into()) | ||
| } | ||
| } else { | ||
| Err(ProgramError::InvalidAccountData) | ||
| }?; |
There was a problem hiding this comment.
Inverted some of this control flow for easier reading
759f64b to
522e6bc
Compare
| &Pubkey::new_unique(), | ||
| ), | ||
| Err(ProgramError::IncorrectProgramId), | ||
| ); | ||
| process_instruction_as_one_arg( | ||
| &mollusk, | ||
| &instruction::deactivate_delinquent_stake( | ||
| &Pubkey::new_unique(), | ||
| &invalid_vote_state_pubkey(), | ||
| &Pubkey::new_unique(), | ||
| &invalid_vote_state_pubkey(), |
There was a problem hiding this comment.
The error order changes because the reference vote account is now checked before the delinquent account in the processor.
joncinque
left a comment
There was a problem hiding this comment.
Looks great overall! Just a few small questions
| vote_account_info.key, | ||
| ) | ||
| .map_err(|_| ProgramError::InvalidAccountData)?; | ||
| let vote_state = unsafe { Box::from_raw(Box::into_raw(vote_state).cast::<VoteStateV4>()) }; |
There was a problem hiding this comment.
What's with this change from the original function?
| solana_cpi::{get_return_data, invoke_unchecked}, | ||
| solana_program_error::ProgramError, | ||
| solana_vote_interface::state::VoteStateV4, | ||
| std::mem::MaybeUninit, |
There was a problem hiding this comment.
nit: although the crate isn't no-std yet, let's be sure to avoid std when it's easy
| std::mem::MaybeUninit, | |
| core::mem::MaybeUninit, |
| /// Check if the provided `epoch_credits` demonstrate delinquency over the previous | ||
| /// [`MINIMUM_DELINQUENT_EPOCHS_FOR_DEACTIVATION`]. | ||
| #[deprecated( | ||
| since = "4.5.0", |
There was a problem hiding this comment.
Sorry, this version will have to change after my breaking publish 🙏
| return Ok(true); | ||
| } | ||
|
|
||
| let vote_state = get_vote_state(vote_account_info)?; |
There was a problem hiding this comment.
I think this makes sense, but just to point it out: if get_vote_state fails, then it won't be possible to use deactivate_delinquent on that account.
get_vote_state would most likely fail on a very old or very new vote state version, and we would want that to fail loudly.
|
|
||
| #[test_case(system_program::id(), vec![], 0; "removed_account")] | ||
| #[test_case(Pubkey::new_unique(), vec![255], 1; "other_owner_with_invalid_data")] | ||
| fn test_non_vote_owner(owner: Pubkey, data: Vec<u8>, lamports: u64) { |
There was a problem hiding this comment.
nit: the lamports field is a bit misleading since it doesn't make a difference in the implementation -- maybe just hardcode it in the test?
| solana-pubkey = { version = "4.3.0", default-features = false } | ||
| solana-stake-history = "1.0.0" | ||
| solana-system-interface = "3.3.0" | ||
| solana-vote-interface = { version = "5.0.0", features = ["bincode"], optional = true } |
There was a problem hiding this comment.
Also my fault, this'll need to be bumped to 7.0.0
| solana-svm-log-collector = "3.0.0" | ||
| solana-system-interface = { version = "3.3.0", features = ["bincode"] } | ||
| solana-transaction = "3.0.2" | ||
| solana-vote-interface = { version = "5.0.0", features = ["bincode"] } |
There was a problem hiding this comment.
Same with this, will need to be 7.0.0
| #[test_case(solana_sdk_ids::vote::id(), vec![0; VoteStateV4::size_of()], 0; "closed_vote_state")] | ||
| #[test_case(solana_sdk_ids::vote::id(), vec![0; VoteStateV4::size_of()], 1; "refunded_vote_state")] | ||
| #[test_case(solana_sdk_ids::vote::id(), vec![0; 10 * 1024 * 1024], 1; "maximum_size_shell")] | ||
| fn test_deactivate_delinquent_closed_vote_account(owner: Pubkey, data: Vec<u8>, lamports: u64) { |
There was a problem hiding this comment.
nit: same with this one, the lamports field shouldn't be doing anything, so we can hardcode it to be the same for all cases
| }); | ||
| let (mut mollusk, instruction_accounts, transaction_accounts) = | ||
| setup_deactivate_delinquent_test_with_vote_account(vote_account); | ||
| mollusk.compute_budget.compute_unit_limit = 200_000; |
There was a problem hiding this comment.
Just for my own information, why is this needed?
Implements SIMD-0608. The
DeactivateDelinquentinstruction now also accepts stake delegated to a closed vote account, restoring a cleanup path for delegations orphaned in this edge case.New Behavior
The delinquency condition is satisfied when the delegated vote account:
All other Vote owned data goes through the existing vote-state decoding and five-epoch credit checks.