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

Feature: Early verification of account modifications in BorrowedAccount #25899

Merged
merged 14 commits into from
Jul 15, 2022

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Jun 10, 2022

Problem

This is a continuation of #25380.

Currently, for BPF programs the verification cost is:

  • One data copy from AccountSharedData to create or update the PreAccount
  • One data copy from the BPF program into the AccountSharedData so we can compare it against PreAccount
  • The actual comparison in PreAccount (this is skipped if the data is allowed to be modified)

Assuming that copies and comparison incur a similar cost (as they both need to access two different pointers, polluting the data cache), we could reduce this cost by 50% up to 66% for every account in every ABIv0 and ABIv1 BPF program executed. Built-in programs would also profit, reducing their verification cost by almost 100%. In ABIv2 BPF programs will be like built-in programs in this regard.

Summary of Changes

This PR introduces the feature enable_early_verification_of_account_modifications which controls the switch from the current behavior to the following:

  1. Account modification errors are thrown earlier (hence the name of the feature). Specifically, they occur the moment they are attempted, not at the begin / end of instructions and CPI. The primary motivation is to get rid of PreAccount and all the redundant data copies and data comparisons that come with it. As a side effect, this should make debugging easier because we can get the call stack where the error occurs.

  2. Similarly to the deprecation of PreAccount, AccountsDataMeter will also be deprecated and its functionality replicated inside TransactionContext. This was necessary as the AccountsDataMeter is currently updated and verified from withing the PreAccounts.

  3. As a consequence, first corrupting (meta)data and then later correcting it into a valid state again won't fly anymore. Every change will have to be valid on its own. This however, does only apply to the runtime and built-in programs, not to BPF programs of ABIv0 and ABIv1 (all currently deployed programs) as they will continue to have their checks at the deserialization edge.

  4. Failed attempted account modifications used to be counted as modifications, now they are not recorded as "touched" anymore. For BPF programs, all accounts which are modifiable however, will be counted as "touched" even if their data did not change.

  5. All the explicit / dedicated account modification tests inside InvokeContext, PreAccount and AccountsDataMeter are disabled / become unreachable.

  6. The loader and built-in tests on the other hand, become stricter as they now have to adhere to all account modification rules.

  7. Two tests to trigger InstructionError::UnbalancedInstruction (which was not covered before) were added.


Feature Gate Issue: #26589

sdk/src/transaction_context.rs Outdated Show resolved Hide resolved
sdk/src/transaction_context.rs Outdated Show resolved Hide resolved
sdk/src/transaction_context.rs Show resolved Hide resolved
sdk/src/transaction_context.rs Show resolved Hide resolved
sdk/src/transaction_context.rs Outdated Show resolved Hide resolved
programs/bpf_loader/src/serialization.rs Show resolved Hide resolved
programs/bpf_loader/src/serialization.rs Show resolved Hide resolved
sdk/src/transaction_context.rs Show resolved Hide resolved
@Lichtso Lichtso added the work in progress This isn't quite right yet label Jun 10, 2022
@Lichtso Lichtso requested review from jackcmay, ryoqun and jstarry June 10, 2022 11:16
@Lichtso Lichtso force-pushed the feature/early_access_violation branch from cb0f075 to a263e52 Compare June 10, 2022 16:07
@jackcmay
Copy link
Contributor

I'm not too worried about rent_epoch anymore since accounts must be rent-exempt. We will either have to continue setting it for ABIv1, or maybe feature gate a deprecation of it which sets it to some acceptable value that is least likely to break existing programs.

@Lichtso Lichtso force-pushed the feature/early_access_violation branch from a263e52 to c6c391b Compare June 14, 2022 16:19
@Lichtso Lichtso changed the title Feature: Early access violation in BorrowedAccount Feature: Early verification of account modifications in BorrowedAccount Jun 14, 2022
@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #25899 (3fbf0c7) into master (1165a7f) will increase coverage by 0.0%.
The diff coverage is 77.5%.

❗ Current head 3fbf0c7 differs from pull request most recent head 61a171d. Consider uploading reports for the commit 61a171d to get more accurate results

@@            Coverage Diff            @@
##           master   #25899     +/-   ##
=========================================
  Coverage    81.9%    81.9%             
=========================================
  Files         631      633      +2     
  Lines      174252   175374   +1122     
=========================================
+ Hits       142728   143798   +1070     
- Misses      31524    31576     +52     

@Lichtso Lichtso force-pushed the feature/early_access_violation branch 8 times, most recently from 2dda9ab to 2e9d321 Compare June 21, 2022 17:05
@Lichtso Lichtso force-pushed the feature/early_access_violation branch 6 times, most recently from b6ed6c6 to 29a7ffb Compare June 27, 2022 16:57
@Lichtso Lichtso force-pushed the feature/early_access_violation branch from 29a7ffb to 213376f Compare June 29, 2022 17:32
@Lichtso Lichtso force-pushed the feature/early_access_violation branch from f9e4d01 to 5307666 Compare July 14, 2022 16:56
@Lichtso Lichtso force-pushed the feature/early_access_violation branch from 5307666 to 61a171d Compare July 14, 2022 17:05
@Lichtso
Copy link
Contributor Author

Lichtso commented Jul 14, 2022

I think I addressed all review comments, CI is passing, no merge conflicts ...
@jstarry @ryoqun @brooksprumo Can you once more approve so we can finally merge this?

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

The accounts resize delta changes look good to me.

Copy link
Member

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

lgtm. master piece pr. :)

@Lichtso Lichtso merged commit 038da82 into solana-labs:master Jul 15, 2022
@Lichtso Lichtso deleted the feature/early_access_violation branch July 15, 2022 07:31
Lcchy pushed a commit to Bonfida/solana that referenced this pull request Jul 22, 2022
…unt` (solana-labs#25899)

* Adjusts test cases for stricter requirements.

* Removes account reset in deserialization test.

* Removes verify related test cases.

* Replicates account modification verification logic of PreAccount in BorrowedAccount.

* Adds TransactionContext::account_touched_flags.

* Adds account modification verification to the BPF ABIv0 and ABIv1 deserialization, CPI syscall and program-test.

* Replicates the total sum of all lamports verification of PreAccounts in InstructionContext

* Check that the callers instruction balance is maintained during a call / push.

* Replicates PreAccount statistics in TransactionContext.

* Disable verify() and verify_and_update() if the feature enable_early_verification_of_account_modifications is enabled.

* Moves Option<Rent> of enable_early_verification_of_account_modifications into TransactionContext::new().

* Relaxes AccountDataMeter related test cases.

* Don't touch the account if nothing changes.

* Adds two tests to trigger InstructionError::UnbalancedInstruction.

Co-authored-by: Justin Starry <justin@solana.com>
@Lichtso Lichtso added the feature-gate Pull Request adds or modifies a runtime feature gate label Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants