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

Limit number of accounts that a transaction can lock #22201

Merged
merged 1 commit into from
Jan 4, 2022

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Dec 31, 2021

Problem

No limit to the number of accounts that a transaction can lock

Summary of Changes

  • Moved account key validation to SanitizedTransaction::get_account_locks()
  • Added validation check to limit the number of locked accounts per transaction to 64
  • Added feature switch to activate the new validation check

Fixes #21748
Feature Gate Issue #24046

@codecov
Copy link

codecov bot commented Dec 31, 2021

Codecov Report

Merging #22201 (5cecb4c) into master (736f974) will increase coverage by 10.9%.
The diff coverage is 98.0%.

@@             Coverage Diff             @@
##           master   #22201       +/-   ##
===========================================
+ Coverage    70.1%    81.0%    +10.9%     
===========================================
  Files          35      523      +488     
  Lines        2076   146549   +144473     
  Branches      296        0      -296     
===========================================
+ Hits         1456   118844   +117388     
- Misses        519    27705    +27186     
+ Partials      101        0      -101     

@jackcmay
Copy link
Contributor

Why move the account validation out of the sanitization process?

@jstarry
Copy link
Member Author

jstarry commented Dec 31, 2021

Why move the account validation out of the sanitization process?

In my opinion, sanitization is more for things like bounds checking. Validation of requested account locks for a transaction is something different governed by the runtime and makes more sense to handle at the point of account locking.

@jstarry jstarry merged commit 2b5e00d into solana-labs:master Jan 4, 2022
@jstarry jstarry deleted the max-tx-locked-accounts branch January 4, 2022 06:25
mergify bot pushed a commit that referenced this pull request Jan 4, 2022
(cherry picked from commit 2b5e00d)

# Conflicts:
#	accountsdb-plugin-postgres/src/postgres_client/postgres_client_transaction.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	sdk/src/feature_set.rs
#	sdk/src/transaction/error.rs
#	storage-proto/proto/transaction_by_addr.proto
#	storage-proto/src/convert.rs
mergify bot added a commit that referenced this pull request Jan 4, 2022
#22263)

* Limit number of accounts that a transaction can lock (#22201)

(cherry picked from commit 2b5e00d)

# Conflicts:
#	accountsdb-plugin-postgres/src/postgres_client/postgres_client_transaction.rs
#	runtime/src/accounts.rs
#	runtime/src/bank.rs
#	sdk/src/feature_set.rs
#	sdk/src/transaction/error.rs
#	storage-proto/proto/transaction_by_addr.proto
#	storage-proto/src/convert.rs

* resolve conflicts

Co-authored-by: Justin Starry <justin@solana.com>
@brooksprumo brooksprumo mentioned this pull request Jan 5, 2022
@jstarry jstarry added the feature-gate Pull Request adds or modifies a runtime feature gate label Apr 1, 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.

No cap on the number of accounts loaded in a transaction
2 participants