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

Allow programs to realloc their accounts within limits #19475

Merged
merged 2 commits into from
Sep 28, 2021

Conversation

jackcmay
Copy link
Contributor

@jackcmay jackcmay commented Aug 27, 2021

Problem

Programs can only allocate account data via SystemInstruction::Allocate which only supports new account allocations (account owned by system program and alloc from 0 to x bytes).

Summary of Changes

Allow programs to realloc the data size of accounts they own. They are still limited by the BPF serialization cap of 10k and the maximum account size of 10M.

  • This also allows programs to realloc by 10k multiple times to support >10k account sizes.
  • And allows programs to dealloc their account data to size 0 for the purpose of closing accounts safely.

This is a proof-of-concept meant to start a conversation as to the safety of allowing these types of operations so these changes don't contain any featurization or testing yet.

Thoughts?

Fixes #14694

@jackcmay jackcmay added the work in progress This isn't quite right yet label Aug 27, 2021
@jackcmay
Copy link
Contributor Author

@jstarry @mvines @t-nelson 👀

@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #19475 (80e599c) into master (6f08f9d) will decrease coverage by 0.0%.
The diff coverage is 71.6%.

@@            Coverage Diff            @@
##           master   #19475     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         487      487             
  Lines      135451   135528     +77     
=========================================
+ Hits       112237   112265     +28     
- Misses      23214    23263     +49     

Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

This will make a bunch of people very happy 🙂

program-runtime/src/instruction_processor.rs Outdated Show resolved Hide resolved
program-runtime/src/instruction_processor.rs Show resolved Hide resolved
program-runtime/src/instruction_processor.rs Outdated Show resolved Hide resolved
@jackcmay jackcmay force-pushed the deallocate-account-data branch from 9c3cdf0 to 329d660 Compare August 28, 2021 00:11
@mvines
Copy link
Member

mvines commented Aug 28, 2021

Dumb sleepy Saturday question: How does a BPF program take advantage of this again?

@jackcmay
Copy link
Contributor Author

Dumb sleepy Saturday question: How does a BPF program take advantage of this again?

Helpers and some tests added

@jackcmay jackcmay force-pushed the deallocate-account-data branch from 67a3fc6 to eab8eb8 Compare August 31, 2021 21:52
@jstarry
Copy link
Member

jstarry commented Sep 2, 2021

As discussed offline, are there any remaining blockers from allowing a program at depth 2+ to reallocate a non-empty account?

if !account_ref.data.is_empty() {
// Only support for `CreateAccount` at this time.
// Need a way to limit total realloc size across multiple CPI calls
ic_msg!(

Seems like the issue is that we would need to prevent sequentially invoked CPI's from each bumping the data size?

@jackcmay
Copy link
Contributor Author

jackcmay commented Sep 2, 2021 via email

@jackcmay jackcmay force-pushed the deallocate-account-data branch 2 times, most recently from 1f9c8ba to c1d60f4 Compare September 3, 2021 21:03
@jackcmay
Copy link
Contributor Author

jackcmay commented Sep 3, 2021

No featurization yet but lots of test cases, any other test scenarios folks can think of for account reallocs?

@stale
Copy link

stale bot commented Sep 11, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 11, 2021
@jackcmay jackcmay requested a review from jstarry September 14, 2021 17:36
@stale stale bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 14, 2021
@jackcmay jackcmay removed the work in progress This isn't quite right yet label Sep 16, 2021
@jackcmay jackcmay requested a review from Lichtso September 16, 2021 19:50
@jackcmay jackcmay force-pushed the deallocate-account-data branch from c1d60f4 to feab313 Compare September 16, 2021 20:01
@jackcmay jackcmay force-pushed the deallocate-account-data branch from feab313 to 89c4b8a Compare September 16, 2021 22:37
@jackcmay jackcmay force-pushed the deallocate-account-data branch from 89c4b8a to 675630f Compare September 16, 2021 23:51
@jackcmay jackcmay force-pushed the deallocate-account-data branch from 675630f to f6cf6d3 Compare September 17, 2021 16:57
@jackcmay jackcmay force-pushed the deallocate-account-data branch from f6cf6d3 to 7a63e4b Compare September 20, 2021 17:44
@jackcmay jackcmay requested a review from jstarry September 23, 2021 21:21
account.set_executable(account_ref.executable);
account.set_rent_epoch(account_ref.rent_epoch);
account.copy_into_owner_from_slice(caller_account.owner.as_ref());
caller_account.original_data_len = account.data().len();
Copy link
Member

Choose a reason for hiding this comment

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

This account.data could have already changed in an earlier CPI so we can't trust it as the original data length. The original data length has to come from the time of serializing account parameters.

@@ -114,6 +114,29 @@ impl<'a> AccountInfo<'a> {
.map_err(|_| ProgramError::AccountBorrowFailed)
}

pub fn realloc(&self, new_len: usize) -> Result<(), ProgramError> {
Copy link
Member

Choose a reason for hiding this comment

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

@jackcmay I still think we need to zero in this sdk method. If a program deallocs and then reallocs, it shouldn't be already filled with data.

@jackcmay
Copy link
Contributor Author

@jstarry I'd like to avoid calling memset here, for most it will be a waste of compute units. How about clearly documenting the behavior instead?

@jstarry
Copy link
Member

jstarry commented Sep 24, 2021

@jstarry I'd like to avoid calling memset here, for most it will be a waste of compute units. How about clearly documenting the behavior instead?

Ah sure, can we have an opt-in bool param to fn realloc called zero_freed_data so that devs have to think about whether they want it?

Copy link
Member

@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.

orig_data_lens change looks good!

rbpf-cli/src/main.rs Outdated Show resolved Hide resolved
@@ -114,6 +114,29 @@ impl<'a> AccountInfo<'a> {
.map_err(|_| ProgramError::AccountBorrowFailed)
}

pub fn realloc(&self, new_len: usize) -> Result<(), ProgramError> {
Copy link
Member

Choose a reason for hiding this comment

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

@jackcmay still working on adding a zero_freed_data param here?

@jackcmay
Copy link
Contributor Author

@jstarry

Yup, almost done, but question, what do you think of:

pub fn realloc(&self, new_len: usize, zero_init: bool) -> Result<(), ProgramError> {

vs

pub fn realloc(&self, new_len: usize) -> Result<(), ProgramError>
pub fn realloc_unset(&self, new_len: usize) -> Result<(), ProgramError>

@Lichtso
Copy link
Contributor

Lichtso commented Sep 24, 2021

Looking good so far.
This is only about BPF programs, not native_invoke, right?

pub fn realloc(&self, new_len: usize, zero_init: bool) -> Result<(), ProgramError>

I like the parameter flag better than two methods as you might decide to add even more parameters later on.

Btw, account realloc in ABI v2 is not designed yet and something I would like to discuss on Monday.

rbpf-cli/src/main.rs Outdated Show resolved Hide resolved
sdk/program/src/account_info.rs Outdated Show resolved Hide resolved
@jackcmay jackcmay merged commit 4e27543 into solana-labs:master Sep 28, 2021
@jackcmay jackcmay deleted the deallocate-account-data branch September 28, 2021 08:13
dankelleher pushed a commit to identity-com/solana that referenced this pull request Nov 24, 2021
frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BPF programs have a limited ability to realloc account data sizes
6 participants