Skip to content
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

Check EpochRewards::active within the Stake Program #617

Merged

Conversation

CriesofCarrots
Copy link

@CriesofCarrots CriesofCarrots commented Apr 5, 2024

Problem

As per SIMD-0118, credits to stake accounts should be allowed during the rewards interval. The current implementation Errors all transactions that load a stake account as writable, overly restrictive.

Summary of Changes

  • Check whether EpochRewards::active and return new StakeError for all instructions except GetMinimumDelegation
    Note: this implements the specification in the SIMD, but I think this could be relaxed on a handful of instructions, like Authorize and SetLockup. It's possible that even Initialize, DelegateStake, and Deactivate could be supported during the rewards interval as well, depending on cache state for recalculation (boot from snapshot). At any rate, unless either of you feel strongly about doing that now, I'll make any of those changes in a separate PR.
  • (edit) The following moved to separate PR, which I will link.
    Remove existing check_account_access() call in SVM
    I did leave this method in the TransactionProcessingCallback trait, however. It's a generic-enough name that it could be used for something other than rewards-period restriction.

Towards #426

@CriesofCarrots
Copy link
Author

CriesofCarrots commented Apr 5, 2024

Oh also: I know we need some comprehensive tests of this logic in solana_stake_program now. If you would like that added to this PR, as opposed to done separately, let me know.

(edit) Test added

@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (01460ef) to head (df3e81a).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #617    +/-   ##
========================================
  Coverage    81.8%    81.9%            
========================================
  Files         851      851            
  Lines      230238   230394   +156     
========================================
+ Hits       188511   188729   +218     
+ Misses      41727    41665    -62     

Copy link

@jstarry jstarry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind splitting this into a change that removes the old logic and a change that introduces the new logic? As for tests, I would like to see at least one more that checks that get min delegation still works during epoch rewards

programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved
@CriesofCarrots CriesofCarrots force-pushed the simd-118-stake-program branch from 3ed9bca to caf1b0e Compare April 6, 2024 21:37
@CriesofCarrots
Copy link
Author

Do you mind splitting this into a change that removes the old logic and a change that introduces the new logic? As for tests, I would like to see at least one more that checks that get min delegation still works during epoch rewards

Removed the old-logic-removal commits from this PR.
Added test that shows that GetMinimumDelegation works (and all other ix fail).

jstarry
jstarry previously approved these changes Apr 7, 2024
programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Comment on lines +67 to +74
// The EpochRewards sysvar only exists after the
// enable_partitioned_epoch_reward feature is activated. If it exists, check
// the `active` field
let epoch_rewards_active = invoke_context
.get_sysvar_cache()
.get_epoch_rewards()
.map(|epoch_rewards| epoch_rewards.active)
.unwrap_or(false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @2501babe on this change, since the bpf version of the stake program will need to fetch the epoch rewards using the new syscall

@CriesofCarrots CriesofCarrots merged commit 2470b45 into anza-xyz:master Apr 9, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants