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

Fix - SVM account_loader tests #3448

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

Lichtso
Copy link

@Lichtso Lichtso commented Nov 3, 2024

Problem

Some tests in svm/src/account_loader.rs currently do test situations which are impossible to construct in production and do miss cases which are. The issue is a slightly incorrect setup of the program accounts involved. The tests assume that the program cache of the program cache is protocol relevant, which it is not, instead the SVM program loader is.

Otherwise these tests (introduced in #3045) are pretty good, so shout-out to Hana for writing them!

Summary of Changes

  • Fixes test_load_transaction_accounts_program_account_executable_bypass:
    Now constructs a loader-v2 tombstone, instead of an impossible program cache entry (a closed built-in) which would not be produced by the SVM program loader. This also changes the expected owner of the loaded program account from the native_loader to bpf_loader and thus inserts bpf_loader in the list of loaded accounts.

  • Fixes test_load_transaction_accounts_data_sizes:
    Now initializes the loader-v3 program with a valid UpgradeableLoaderState::Program pointing to a programdata account containing UpgradeableLoaderState::ProgramData and an ELF. Before the test simply forced in an impossible program cache entry (a closed built-in) which would not be produced by the SVM program loader. Also, uses the correctly initialized program cache for all tests now. Before it had some with an empty cache, which again can't happen in production because the cache is replenished before the account loader runs. Furthermore, adds two test cases for write lock demotion of loader-v3.

  • Removes test_load_transaction_accounts_program_account_not_found_but_loaded:
    The test constructs a scenario in which the program cache contains an entry even tough the accounts database has no account at the given key. This is not possible because the program cache only has entries for accounts owned by the loaders. A non existent account can not be owned by a loader.

Fixes #

@Lichtso Lichtso marked this pull request as ready for review November 3, 2024 20:40
@Lichtso Lichtso requested a review from 2501babe November 3, 2024 21:08
@Lichtso Lichtso added the v2.1 Backport to v2.1 branch label Nov 3, 2024
Copy link

mergify bot commented Nov 3, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

2501babe
2501babe previously approved these changes Nov 4, 2024
@Lichtso
Copy link
Author

Lichtso commented Nov 4, 2024

I think we are still missing coverage for the distinction in the size of valid program entries and tombstones in test_load_transaction_accounts_data_sizes. I will add that as well.

@Lichtso
Copy link
Author

Lichtso commented Nov 6, 2024

@2501babe can you take a look again? I changed the loader-v2 program to be closed so it counts as 0 and the loader-v3 program to be loaded correctly. Furthermore, I added test coverage for when loader-v3 experiences write lock demotion which affects if its programdata is counted.

program2_size + upgradeable_loader_size + fee_payer_size,
);

// programdata as instruction account double-counts it
// programdata as readonly instruction account double-counts it
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// programdata as readonly instruction account double-counts it
// programdata as instruction account double-counts it

these tests are in a loop that use both read-only and writable account metas

Copy link
Author

Choose a reason for hiding this comment

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

yep, but only one case is double counted. See factor below

@Lichtso Lichtso merged commit 7403549 into anza-xyz:master Nov 7, 2024
51 checks passed
@Lichtso Lichtso deleted the fix/svm_account_loader_tests branch November 7, 2024 16:21
mergify bot pushed a commit that referenced this pull request Nov 7, 2024
* Fixes test_load_transaction_accounts_program_account_executable_bypass.

* Fixes test_load_transaction_accounts_data_sizes().

* Removes test_load_transaction_accounts_program_account_not_found_but_loaded().

(cherry picked from commit 7403549)
Lichtso added a commit that referenced this pull request Nov 8, 2024
* Fixes test_load_transaction_accounts_program_account_executable_bypass.

* Fixes test_load_transaction_accounts_data_sizes().

* Removes test_load_transaction_accounts_program_account_not_found_but_loaded().

(cherry picked from commit 7403549)
Lichtso added a commit that referenced this pull request Nov 8, 2024
Fix - SVM account_loader tests (#3448)

* Fixes test_load_transaction_accounts_program_account_executable_bypass.

* Fixes test_load_transaction_accounts_data_sizes().

* Removes test_load_transaction_accounts_program_account_not_found_but_loaded().

(cherry picked from commit 7403549)

Co-authored-by: Alexander Meißner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.1 Backport to v2.1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants