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

require stake, vote and executable accounts to be rent exempt #5928

Merged
merged 23 commits into from Sep 20, 2019

Conversation

@ParthDesai
Copy link
Contributor

commented Sep 17, 2019

Problem

Right now, stake and vote accounts are not required to be rent exempt, this leads to scenario where ill-configured accounts are unable to load after their balance is exhausted.

Summary of Changes

Require stake and vote accounts to be exempt in order to be initialized.

Fixes #5883 #5882 #5818

@mvines

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

I don't see where this PR fixes #5818?

@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

I don't see where this PR fixes #5818?

Work in progress

@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

@rob-solana

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

I don't think this approach is gonna work (also I think you want to split executable and stake/vote/etc into separate PRs)

Using this approach can strand the lamports in an account, in fact it may actually burn lamports to create the account (part of the rent design is for the system program to initialize rent_epoch), then fail to initialize.

Two other approaches that might work better:

  1. have the system account take a parameter that means "don't create the account unless it's rent exempt", and stake, vote, etc. can use that instruction
  2. expose rent over RPC, so that vote and stake aren't exposed, but wallet's looking out for us

I think I'd prefer the former.

@ParthDesai ParthDesai changed the title require stake, vote and executable account rent exempt require stake and account rent exempt Sep 17, 2019
@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

I don't see where this PR fixes #5818?

Work in progress

Nvm. Will be separate PR.

loaders,
accounts,
credits,
&self.rent_collector.rent_calculator,

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 18, 2019

Contributor

this looks really, really messy to me...

// If account is converted to executable in this instruction, it must need to be rent exempt
if pre.executable != post.executable
&& !pre.executable
&& !rent_calculator.is_exempt(post.lamports, post.data.len())

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 18, 2019

Contributor

yeah, I get it, but this is too rigid. the behavior we want is to optionally enforce. also this doesn't really help address the stake and vote programs' needs

Copy link
Contributor

left a comment

nice hack, but not what we want, I fear

@ParthDesai ParthDesai force-pushed the ParthDesai:exempt-accounts branch 2 times, most recently from b6bcec2 to 9ff44d8 Sep 19, 2019

if should_rent_exempt {
let rent_calculator = rent::from_keyed_account(&keyed_accounts[RENT_SYSVAR_ACCOUNT_INDEX])
.unwrap()

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 19, 2019

Contributor

you can't unwrap here, you need to throw the error...

.unwrap()
.rent_calculator;
if !rent_calculator.is_exempt(lamports, space as usize) {
Err(SystemError::InvalidInstructionData)?;

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 19, 2019

Contributor

insufficient balance?

should_rent_exempt,
} => {
if should_rent_exempt && keyed_accounts.len() < (RENT_SYSVAR_ACCOUNT_INDEX + 1) {
Err(InstructionError::InvalidInstructionData)?;

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 19, 2019

Contributor

again, I think there's a better error for this...

@@ -11,6 +12,7 @@ pub enum SystemError {
SourceNotSystemAccount,
InvalidProgramId,
InvalidAccountId,
InvalidInstructionData,

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 19, 2019

Contributor

there are good InstructionErrors that cover the cases you need, use those

CreateAccount {
lamports: u64,
space: u64,
program_id: Pubkey,
should_rent_exempt: bool,

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 19, 2019

Contributor

instead of "should", how about "verify" or "check" or "require"?

) -> Transaction {
let from_pubkey = from_keypair.pubkey();
let create_instruction =
system_instruction::create_account(&from_pubkey, to, lamports, space, program_id);
let create_instruction = if should_rent_exempt {

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 19, 2019

Contributor

at this point, I'd make the system_instruction::generate_XXX function pub(crate)...

Copy link
Contributor

left a comment

this looks really good overall, what do you think of this approach, now that you've implemented it?

@ParthDesai ParthDesai force-pushed the ParthDesai:exempt-accounts branch from d756838 to 578cc9d Sep 19, 2019
)
}

pub fn create_rent_exempted_account(

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 19, 2019

Contributor

is anyone actually using this function? no need to expose this. "completeness' sake" isn't a thing for us.

This comment has been minimized.

Copy link
@ParthDesai

ParthDesai Sep 20, 2019

Author Contributor

This is being used by wallet, while creating an executable.

Copy link
Contributor

left a comment

if you can remove the changes to system_transaction.rs, we're good to go

@mergify mergify bot dismissed rob-solana’s stale review Sep 20, 2019

Pull request has been modified.

@ParthDesai ParthDesai changed the title require stake and account rent exempt require stake, vote and executable accounts to be rent exempt Sep 20, 2019
@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

this looks really good overall, what do you think of this approach, now that you've implemented it?

yeah, looks more elegant than I thought. I was thinking implementing new system instruction might be too excessive.

@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

if you can remove the changes to system_transaction.rs, we're good to go

No, need. Used by cli/wallet.rs

@ParthDesai ParthDesai marked this pull request as ready for review Sep 20, 2019
@codecov

This comment has been minimized.

Copy link

commented Sep 20, 2019

Codecov Report

Merging #5928 into master will increase coverage by 2.2%.
The diff coverage is 99%.

@@           Coverage Diff            @@
##           master   #5928     +/-   ##
========================================
+ Coverage      72%   74.2%   +2.2%     
========================================
  Files         236     236             
  Lines       43088   41906   -1182     
========================================
+ Hits        31032   31129     +97     
+ Misses      12056   10777   -1279
ParthDesai added 2 commits Sep 20, 2019
@ParthDesai ParthDesai merged commit 11e6197 into solana-labs:master Sep 20, 2019
11 checks passed
11 checks passed
Summary 1 rule matches and 7 potential rules
Details
buildkite/solana Build #11971 passed (32 minutes, 52 seconds)
Details
buildkite/solana/bench Passed (19 minutes, 25 seconds)
Details
buildkite/solana/checks Passed (1 minute, 28 seconds)
Details
buildkite/solana/coverage Passed (30 minutes, 57 seconds)
Details
buildkite/solana/local-cluster Passed (27 minutes, 28 seconds)
Details
buildkite/solana/pipeline-upload Passed (8 seconds)
Details
buildkite/solana/shellcheck Passed (22 seconds)
Details
buildkite/solana/stable Passed (15 minutes, 23 seconds)
Details
buildkite/solana/stable-perf Passed (18 minutes, 35 seconds)
Details
ci-gate Pull Request accepted for CI pipeline
mvines added a commit that referenced this pull request Sep 20, 2019
mvines added a commit that referenced this pull request Sep 20, 2019
…#5928)" (#6005)

This reverts commit 11e6197.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.