8000 v1.18: Fix - `FailedVerification` and `Closed` tombstones (backport of #419) by mergify[bot] · Pull Request #605 · anza-xyz/agave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

v1.18: Fix - FailedVerification and Closed tombstones (backport of #419) #605

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 11, 2024
Merged

v1.18: Fix - FailedVerification and Closed tombstones (backport of #419) #605

merged 1 commit into from
Apr 11, 2024

Conversation

mergify[bot]
Copy link
@mergify mergify bot commented Apr 5, 2024 •
edited by Lichtso
Loading

Problems

  1. TransactionBatchProcessor::load_program_with_pubkey() returns LoadedProgramType::FailedVerification even for accounts which never got to the verifier (because they are empty or buffers for example) which should be marked as LoadedProgramType::Closed instead. LoadedProgramType::FailedVerification should only be used if there is a chance a program gets valid again in the recompilation phase when the feature set and environment changes.

  2. In the tests we currently check that loading non existent accounts results in LoadedProgramType::Closed entries. This however can not happen in integration because we only attempt to load accounts which have a loader as a owner, thus these account must exist. The situation is similar to when we tested LoadedProgramType::FailedVerification existing in the global cache (which can't happen either) and it led to confusion and wasted efforts.

Summary of Changes

  • Replace ProgramAccountLoadResult::AccountNotFound by None
  • Have TransactionBatchProcessor::load_program_accounts() only return ProgramAccountLoadResult::InvalidAccountData in other error cases
  • Have TransactionBatchProcessor::load_program_with_pubkey() return LoadedProgramType::Closed when TransactionBatchProcessor::load_program_accounts() returned ProgramAccountLoadResult::InvalidAccountData
  • Panic in replenish_program_cache() when attempting to load a nonexistent account, and add a test for that

test_bpf_loader_upgradeable_deploy_with_max_len():

  • Replace mock_process_instruction() by a proper message send
  • Tests the invocation of an empty account before a program is deployed there
  • Tests the invocation of a buffer account before it is used to deploy a program
    This is an automatic backport of pull request Fix - FailedVerification and Closed tombstones #419 done by Mergify.

@mergify mergify bot added the conflicts label Apr 5, 2024
@mergify mergify bot assigned Lichtso Apr 5, 2024
@mergify Mergify
Copy link
Author
mergify bot commented Apr 5, 2024

Cherry-pick of 55c05c5 has failed:

On branch mergify/bp/v1.18/pr-419
Your branch is up to date with 'origin/v1.18'.

You are currently cherry-picking commit 55c05c5ea5.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	both modified:   ledger-tool/src/program.rs
	both modified:   program-runtime/src/loaded_programs.rs
	both modified:   runtime/src/bank.rs
	both modified:   runtime/src/bank/tests.rs
	deleted by us:   svm/src/transaction_processor.rs

no changes added to commit (use "git add" and/or "git commit -a")

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@Lichtso Lichtso removed the conflicts label Apr 5, 2024
@Lichtso Lichtso force-pushed the mergify/bp/v1.18/pr-419 branch 2 times, most recently from 4960a0f to 2fd67fd Compare April 8, 2024 22:24
@codecov-commenter
Copy link
codecov-commenter commented Apr 8, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 81.81818% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 81.5%. Comparing base (7b8ed3b) to head (37726cb).

Additional details and impacted files 
@@           Coverage Diff           @@
##            v1.18     #605   +/-   ##
=======================================
  Coverage    81.5%    81.5%           
=======================================
  Files         827      827           
  Lines      224833   224852   +19     
=======================================
+ Hits       183453   183474   +21     
+ Misses      41380    41378    -2

@Lichtso Lichtso requested review from alessandrod and pgarg66 April 8, 2024 23:58
@Lichtso Lichtso force-pushed the mergify/bp/v1.18/pr-419 branch from 2fd67fd to 8eb1fba Compare April 9, 2024 16:06
@Lichtso
Fix - FailedVerification and Closed tombstones (#419)
37726cb 
* Only the verifier can cause FailedVerification, everything else is Closed

* Removes the environments parameter from load_program_accounts().

* cargo fmt

* Simplify invocation of deployed program

* Attempt to invoke a program before it is deployed

* Attempt to invoke a buffer before it is used in a deployment

* Escalates Option return value of load_program_accounts() to load_program_with_pubkey().

* Review feedback

(cherry picked from commit 55c05c5)
@Lichtso Lichtso force-pushed the mergify/bp/v1.18/pr-419 branch from 8eb1fba to 37726cb Compare April 9, 2024 23:03
pgarg66
pgarg66 reviewed Apr 10, 2024
View reviewed changes
@Lichtso Lichtso merged commit b2d28f7 into v1.18 Apr 11, 2024
@Lichtso Lichtso deleted the mergify/bp/v1.18/pr-419 branch April 11, 2024 14:42
anwayde pushed a commit to firedancer-io/agave that referenced this pull request Jul 23, 2024
@mergify @Lichtso @yihau
v1.18: Fix - FailedVerification and Closed tombstones (backport of …
f2ea237 
…anza-xyz#419) (anza-xyz#605)

Fix - `FailedVerification` and `Closed` tombstones (anza-xyz#419)

* Only the verifier can cause FailedVerification, everything else is Closed

* Removes the environments parameter from load_program_accounts().

* cargo fmt

* Simplify invocation of deployed program

* Attempt to invoke a program before it is deployed

* Attempt to invoke a buffer before it is used in a deployment

* Escalates Option return value of load_program_accounts() to load_program_with_pubkey().

* Review feedback

(cherry picked from commit 55c05c5)

Co-authored-by: Alexander Meißner <AlexanderMeissner@gmx.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lichtso Lichtso Lichtso left review comments

@pgarg66 pgarg66 pgarg66 approved these changes

@alessandrod alessandrod Awaiting requested review from alessandrod

Assignees

@Lichtso Lichtso

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@codecov-commenter @Lichtso @pgarg66
0