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

refactor: load transaction accounts #2442

Merged
merged 4 commits into from
Aug 12, 2024

Conversation

jstarry
Copy link

@jstarry jstarry commented Aug 5, 2024

Problem

load_transaction_accounts does an unnecessary clone for the fee payer account and has an unnecessary additional level of nesting for the instructions sysvar.

Summary of Changes

  • Refactor load_transaction_accounts to first handle the fee payer account without cloning
  • Fold instruction sysvar handling into the same logic as all other accounts

Fixes #

@jstarry jstarry force-pushed the refactor/load-tx-accounts branch from 748ddc0 to 829368a Compare August 6, 2024 09:14

account
};
accumulate_and_check_loaded_account_data_size(
Copy link

Choose a reason for hiding this comment

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

This part looks a bit dangerous to me and could be consensus breaking if there is a slight difference in which accounts count towards the loaded_accounts_bytes limit.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed that it's a sensitive part of the code that needs extra eyes. This PR allows the instructions sysvar to hit this function call now but I have set the loaded size for that account to 0 so there should be no consensus change here.

svm/src/account_loader.rs Outdated Show resolved Hide resolved
svm/src/account_loader.rs Show resolved Hide resolved
svm/src/account_loader.rs Show resolved Hide resolved
@apfitzge
Copy link

apfitzge commented Aug 8, 2024

@2501babe this changes some things in account loading, which we'll want to take into account for the SIMD83 changes.

@jstarry
Copy link
Author

jstarry commented Aug 12, 2024

Any other feedback or concerns here? This refactor is not too important so I don't mind abandoning it but I think it's safe and makes things a bit easier to read.

@jstarry jstarry merged commit cb62b6a into anza-xyz:master Aug 12, 2024
40 checks passed
@jstarry jstarry deleted the refactor/load-tx-accounts branch August 12, 2024 23:47
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
* refactor: load transaction accounts

* feedback

* fix account found

* fix clippy
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.

4 participants