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

make executable, vote and stake account rent exempt #6017

Merged
merged 17 commits into from Oct 3, 2019

Conversation

@ParthDesai
Copy link
Contributor

ParthDesai commented Sep 23, 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

Requiring executable, stake and vote account to be rent exempt at the time of initializing them.

Fixes #5883
Fixes #5882
Fixes #5818

@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 23, 2019

Codecov Report

Merging #6017 into master will decrease coverage by 0.3%.
The diff coverage is 64.6%.

@@           Coverage Diff            @@
##           master   #6017     +/-   ##
========================================
- Coverage    64.4%     64%   -0.4%     
========================================
  Files         244     244             
  Lines       48046   48455    +409     
========================================
+ Hits        30953   31024     +71     
- Misses      17093   17431    +338
@ParthDesai ParthDesai marked this pull request as ready for review Sep 23, 2019
Copy link
Member

mvines left a comment

Thanks Parth, this looks way better to me.

Have you tried creating a vote/stake account using the cli/ with this change? I suspect we're going to run into a minor usability issue where the user doesn't really know how many lamports to assign to a vote/stake account to make it rent exempt. Same for a user loading a program from solana-web3.js (or the cli/). We may want to add a JSON RPC API that allows the user to query for how many lamports are required to be exempt

@@ -0,0 +1,17 @@
use crate::account::KeyedAccount;

This comment has been minimized.

Copy link
@mvines

mvines Sep 23, 2019

Member

Can this fn just live in rent.rs?

This comment has been minimized.

Copy link
@ParthDesai

ParthDesai Sep 23, 2019

Author Contributor

you mean rent_calculator.rs right? I didn't put it there, because rent_calculator.rs as per name should only be dealing with the RentCalculator object IMHO.

One other way to do this, is to rename rent_calculator.rs to rent.rs and put RentCalculator as well as this fn there. Wdyt?

This comment has been minimized.

Copy link
@mvines

mvines Sep 23, 2019

Member

I was thinking rent.rs, because the inputs to verify_rent_exemption() do require rent.rs

This comment has been minimized.

Copy link
@ParthDesai

ParthDesai Sep 23, 2019

Author Contributor

That's sysvar/rent.rs.

This comment has been minimized.

Copy link
@mvines

mvines Sep 23, 2019

Member

yeah, same file that from_keyed_account was added to

This comment has been minimized.

Copy link
@mvines

mvines Sep 23, 2019

Member

(basically creating a "util" file is asking for somebody else to come along and dump other random stuff into it, then we end up with a mess. I'm trying to preempt that)

This comment has been minimized.

Copy link
@ParthDesai

ParthDesai Sep 23, 2019

Author Contributor

done

runtime/src/bank.rs Show resolved Hide resolved
@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

ParthDesai commented Sep 23, 2019

Thanks Parth, this looks way better to me.

Have you tried creating a vote/stake account using the cli/ with this change? I suspect we're going to run into a minor usability issue where the user doesn't really know how many lamports to assign to a vote/stake account to make it rent exempt. Same for a user loading a program from solana-web3.js (or the cli/). We may want to add a JSON RPC API that allows the user to query for how many lamports are required to be exempt

hmm. Ok. I will modify cli in a way that, if user tries to create vote/stake account with too low lamports, than can include amount of lamports required in error message. Plus will add JSON RPC API to query how many lamports required for particular data length.

@mvines

This comment has been minimized.

Copy link
Member

mvines commented Sep 23, 2019

Sounds good. For a vote and program account we could just automatically set the lamports. I can’t imagine why anybody would want to create either with more lamports than the minimum required

@rob-solana rob-solana requested review from rob-solana and jackcmay Sep 23, 2019
@ParthDesai ParthDesai force-pushed the ParthDesai:exempt-accounts branch from 956ced1 to ceb1988 Sep 25, 2019
@@ -306,6 +306,35 @@ impl RpcClient {
self.get_account(pubkey).map(|account| account.data)
}

pub fn get_minimum_balance_for_rent_exemption(&self, data_len: usize) -> io::Result<u64> {

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 25, 2019

Contributor

instead of a new RPC, how about using get_account(sys::rent::id) and running the calculation locally?

This comment has been minimized.

Copy link
@ParthDesai

ParthDesai Sep 26, 2019

Author Contributor

hmm. Less clutter, I like it.

@@ -559,6 +560,12 @@ impl Bank {
}
}

pub fn get_minimum_balance_for_rent_exemption(&self, data_len: usize) -> u64 {

This comment has been minimized.

Copy link
@rob-solana

rob-solana Sep 25, 2019

Contributor

I don't think we need this on bank if you remove the new RPC

@mvines

This comment has been minimized.

Copy link
Member

mvines commented Sep 26, 2019

@ParthDesai this PR is getting a little big. Can you try breaking it up into perhaps at least 3 smaller PRs. This way we can get parts of your work landed as quickly as possible, and you don't need to carry big patches and it makes your reviewer's job much easer too.

PR 1: The RPC API changes
PR 2: The cli changes to use the new RPC API
PR 3: The actual changes to make vote/stake/program accounts rent exempt (which we've basically already reviewed)

@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

ParthDesai commented Sep 26, 2019

@ParthDesai this PR is getting a little big. Can you try breaking it up into perhaps at least 3 smaller PRs. This way we can get parts of your work landed as quickly as possible, and you don't need to carry big patches and it makes your reviewer's job much easer too.

PR 1: The RPC API changes
PR 2: The cli changes to use the new RPC API
PR 3: The actual changes to make vote/stake/program accounts rent exempt (which we've basically already reviewed)

@mvines ok. So, let's make this PR 3. Will open PR1 and PR2 shortly.
Also, I think we may not need to add new RPC API, and can just query sysvar account, and compute locally.

@mvines

This comment has been minimized.

Copy link
Member

mvines commented Sep 26, 2019

Also, I think we may not need to add new RPC API, and can just query sysvar account, and compute locally.

Yeah that's true. I feel that's a much larger burden on all clients in this case though, and could be annoying to update all the clients if the sysvar format ever changes before mainnet.

@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

ParthDesai commented Sep 26, 2019

Also, I think we may not need to add new RPC API, and can just query sysvar account, and compute locally.

Yeah that's true. I feel that's a much larger burden on all clients in this case though, and could be annoying to update all the clients if the sysvar format ever changes before mainnet.

Ok. Let me think on this bit more. Meanwhile I will close this PR and open another one with only vote/stake/program account exempt. Let's land that PR.

@mvines

This comment has been minimized.

Copy link
Member

mvines commented Sep 26, 2019

Oh keep this PR open, just use it for "PR3"

@ParthDesai ParthDesai force-pushed the ParthDesai:exempt-accounts branch from caf02bd to ceb1988 Sep 26, 2019
@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

ParthDesai commented Sep 26, 2019

Oh keep this PR open, just use it for "PR3"

@mvines done. Let's merge this.

@rob-solana

This comment has been minimized.

Copy link
Contributor

rob-solana commented Sep 30, 2019

what happens with merge commits in a PR?

@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

ParthDesai commented Sep 30, 2019

what happens with merge commits in a PR?

There was a conflict. It is easier for me to do merge and resolve, then rebase and resolve (Since in rebase conflict can arrive per commit).
Also, since we are squashing, this doesn't matter anyway.

@mvines

This comment has been minimized.

Copy link
Member

mvines commented Sep 30, 2019

I mostly don't want to look at any PR with a merge commit in it personally, it looks ugly and even a little scary. I'm also not 100% sure that squashing a merge commit will do the right thing.

I always use git pull --rebase to resync my local branches to the tip of upstream, haven't used merge commits in years and don't miss them at all.

@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

ParthDesai commented Oct 1, 2019

I mostly don't want to look at any PR with a merge commit in it personally, it looks ugly and even a little scary. I'm also not 100% sure that squashing a merge commit will do the right thing.

I always use git pull --rebase to resync my local branches to the tip of upstream, haven't used merge commits in years and don't miss them at all.

Squashing a PR , essentially produce one commit with combined changes from n number of commits. So, it doesn't matter what individual commits look like.

See this:
https://help.github.com/assets/images/help/pull_requests/commit-squashing-diagram.png

@ParthDesai ParthDesai force-pushed the ParthDesai:exempt-accounts branch from 83c0149 to a45b68b Oct 2, 2019
@ParthDesai

This comment has been minimized.

Copy link
Contributor Author

ParthDesai commented Oct 2, 2019

I mostly don't want to look at any PR with a merge commit in it personally, it looks ugly and even a little scary. I'm also not 100% sure that squashing a merge commit will do the right thing.
I always use git pull --rebase to resync my local branches to the tip of upstream, haven't used merge commits in years and don't miss them at all.

Squashing a PR , essentially produce one commit with combined changes from n number of commits. So, it doesn't matter what individual commits look like.

See this:
https://help.github.com/assets/images/help/pull_requests/commit-squashing-diagram.png

@mvines Anyways, Now this is merge commit free. I just rebased the commits. We can merge now.

@@ -15,6 +16,7 @@ pub enum LoaderInstruction {
/// bit of the Account
///
/// * key[0] - the account to prepare for execution
/// * key[1] - rent sysvar account

This comment has been minimized.

Copy link
@mvines

mvines Oct 2, 2019

Member

@ParthDesai - how's your Javascript? We'll need to update https://github.com/solana-labs/solana-web3.js/blob/b9841a302c577880f325a2a8bec6d31334d1f6a6/src/loader.js#L126 to include rent sysvar before/when this PR lands to keep the examples working. It looks like we could update web3.js before and it would be no harm.

This comment has been minimized.

Copy link
@ParthDesai

ParthDesai Oct 2, 2019

Author Contributor

@mvines I am familiar with server side JS. This shouldn't be much different. Will do it.

Copy link
Member

mvines left a comment

Looking good to me! Just:

  • need to deal with solana-web3.js to keep everything working after this lands
  • Another quick pass by @rob-solana would be nice too
Copy link
Contributor

rob-solana left a comment

2 comments outstanding, otherwise lgtm

@mergify mergify bot dismissed mvines’s stale review Oct 3, 2019

Pull request has been modified.

@ParthDesai ParthDesai merged commit 92ea11f into solana-labs:master Oct 3, 2019
11 checks passed
11 checks passed
Summary 1 rule matches and 7 potential rules
Details
buildkite/solana Build #12578 passed (31 minutes, 53 seconds)
Details
buildkite/solana/bench Passed (15 minutes, 55 seconds)
Details
buildkite/solana/checks Passed (1 minute, 34 seconds)
Details
buildkite/solana/coverage Passed (25 minutes, 37 seconds)
Details
buildkite/solana/local-cluster Passed (14 minutes, 44 seconds)
Details
buildkite/solana/pipeline-upload Passed (3 seconds)
Details
buildkite/solana/shellcheck Passed (30 seconds)
Details
buildkite/solana/stable Passed (14 minutes, 56 seconds)
Details
buildkite/solana/stable-perf Passed (10 minutes, 12 seconds)
Details
ci-gate Pull Request accepted for CI pipeline
@ParthDesai ParthDesai deleted the ParthDesai:exempt-accounts branch Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.