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

Remove overly restrictive check_account_access for partitioned epoch rewards #631

Conversation

CriesofCarrots
Copy link

@CriesofCarrots CriesofCarrots commented Apr 6, 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

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.

The test changes in this PR need rebasing on #617 to pick up the StakeError; I've included a temporary WIP commit to allow this to pass CI (and still demonstrate the change in logic here) in the meantime. Done

Towards #426

@CriesofCarrots CriesofCarrots force-pushed the simd-118-remove-writable-stake-restriction branch from b4ea440 to 2cf1d93 Compare April 6, 2024 21:58
@CriesofCarrots
Copy link
Author

Screenshot 2024-04-06 at 3 59 35 PM

Sidenote: my commit message just says #617. It's weird that github is linking it to solana-labs/solana instead of the repo that this PR is in.

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (c0be86d) to head (1b404d5).
Report is 17 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master     #631    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         851      851            
  Lines      230475   230724   +249     
========================================
+ Hits       188785   188995   +210     
- Misses      41690    41729    +39     

jstarry
jstarry previously approved these changes Apr 7, 2024
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.

lgtm, sorry didn't realize splitting up the change would make the test migration a pain

joncinque
joncinque previously approved these changes Apr 8, 2024
@CriesofCarrots CriesofCarrots dismissed stale reviews from joncinque and jstarry via 1b404d5 April 9, 2024 23:54
@CriesofCarrots CriesofCarrots force-pushed the simd-118-remove-writable-stake-restriction branch from 2cf1d93 to 1b404d5 Compare April 9, 2024 23:54
@CriesofCarrots
Copy link
Author

Thanks! Force-pushed to get rid of the wip commit, so unfortunately need one of you to re- ✅

@CriesofCarrots CriesofCarrots merged commit dd8e1f4 into anza-xyz:master Apr 10, 2024
38 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