v1.18: Fix - FailedVerification and Closed tombstones (backport of #419)#605
Merged
v1.18: Fix - FailedVerification and Closed tombstones (backport of #419)#605
FailedVerification and Closed tombstones (backport of #419)#605Conversation
Author
|
Cherry-pick of 55c05c5 has failed: 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 |
4960a0f to
2fd67fd
Compare
Codecov ReportAttention: Patch coverage is
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 |
2fd67fd to
8eb1fba
Compare
* 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)
8eb1fba to
37726cb
Compare
pgarg66
reviewed
Apr 10, 2024
pgarg66
approved these changes
Apr 11, 2024
anwayde
pushed a commit
to firedancer-io/agave
that referenced
this pull request
Jul 23, 2024
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problems
TransactionBatchProcessor::load_program_with_pubkey()returnsLoadedProgramType::FailedVerificationeven for accounts which never got to the verifier (because they are empty or buffers for example) which should be marked asLoadedProgramType::Closedinstead.LoadedProgramType::FailedVerificationshould only be used if there is a chance a program gets valid again in the recompilation phase when the feature set and environment changes.In the tests we currently check that loading non existent accounts results in
LoadedProgramType::Closedentries. 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 testedLoadedProgramType::FailedVerificationexisting in the global cache (which can't happen either) and it led to confusion and wasted efforts.Summary of Changes
ProgramAccountLoadResult::AccountNotFoundbyNoneTransactionBatchProcessor::load_program_accounts()only returnProgramAccountLoadResult::InvalidAccountDatain other error casesTransactionBatchProcessor::load_program_with_pubkey()returnLoadedProgramType::ClosedwhenTransactionBatchProcessor::load_program_accounts()returnedProgramAccountLoadResult::InvalidAccountDatareplenish_program_cache()when attempting to load a nonexistent account, and add a test for thattest_bpf_loader_upgradeable_deploy_with_max_len():mock_process_instruction()by a proper message sendThis is an automatic backport of pull request Fix -
FailedVerificationandClosedtombstones #419 done by Mergify.