Stop loading program accounts if program exists in cache #30703
Conversation
pgarg66
force-pushed
the
cache-try
branch
2 times, most recently
from
9cd3eab
to
538acbe
Compare
Codecov Report
@@ Coverage Diff @@
## master #30703 +/- ##
=========================================
- Coverage 81.5% 81.5% -0.1%
=========================================
Files 727 727
Lines 205150 205178 +28
=========================================
+ Hits 167273 167280 +7
- Misses 37877 37898 +21 |
pgarg66
force-pushed
the
cache-try
branch
2 times, most recently
from
3308e3c
to
f3b5c26
Compare
| sanitized_txs: &[SanitizedTransaction], | ||
| check_results: &mut [TransactionCheckResult], | ||
| ) -> ( | ||
| HashMap<Pubkey, &'a Pubkey>, | ||
| HashMap<Pubkey, Arc<LoadedProgram>>, | ||
| ) { | ||
| let mut filter_programs_time = Measure::start("filter_programs_accounts"); | ||
| let program_accounts_map = self.rc.accounts.filter_executable_program_accounts( |
There was a problem hiding this comment.
Should be factored out to call site, so that we don't need to return program_accounts_map, but that can be done in a separate PR.
| @@ -180,9 +180,6 @@ pub fn load_program_from_account( | |||
| return Err(InstructionError::IncorrectProgramId); | |||
| } | |||
|
|
|||
| let (programdata_offset, deployment_slot) = | |||
| get_programdata_offset_and_depoyment_offset(&log_collector, program, programdata)?; | |||
|
|
|||
| if let Some(ref tx_executor_cache) = tx_executor_cache { | |||
| if let Some(loaded_program) = tx_executor_cache.get(program.get_key()) { | |||
| if loaded_program.is_tombstone() { | |||
There was a problem hiding this comment.
We can have tombstone in the TX batch cache now, even if delay_visibility_of_program_deployment is not enabled, right?
There was a problem hiding this comment.
Yes, in case the program fails to compile/verify.
There was a problem hiding this comment.
I was referring to the comment below which is incorrect now.
| @@ -180,9 +180,6 @@ pub fn load_program_from_account( | |||
| return Err(InstructionError::IncorrectProgramId); | |||
| } | |||
|
|
|||
| let (programdata_offset, deployment_slot) = | |||
There was a problem hiding this comment.
Is this code movement necessary?
Bit concerned about error priorities here.
There was a problem hiding this comment.
Yes, since we don't load the actual program account, the get_programdata_offset_and_depoyment_offset returns an error, since state() is not valid.
There was a problem hiding this comment.
Looking through the code, get_programdata_offset_and_depoyment_offset() returns InvalidAccountData in all error scenarios. So, moving the code after cache lookup should not change the error codes (as the cache lookup failure returns the same error code).
| .get_number_of_program_accounts() | ||
| .saturating_sub(2), | ||
| ) | ||
| .ok() |
There was a problem hiding this comment.
As this is only an Option type this could lead to a confusion between the upgradeable loader vs the normal loader and whether a program was loaded in a transaction or not (e.g. used for an instruction or has the is_writable flag in the message).
There was a problem hiding this comment.
load_program_from_account() call preceding this code block is always going to return the program entry from the cache. This code block will be deleted in the follow up PR, and replaced with the cache lookup.
Would you prefer if the code deletion/refactor was done in this PR itself. It was causing some changes to tests in other parts of the code which are unrelated to this PR.
runtime/src/accounts.rs
Outdated
| ) -> Result<AccountSharedData> { | ||
| // Check for tombstone | ||
| if program.is_tombstone() { | ||
| return Err(TransactionError::InvalidProgramForExecution); |
There was a problem hiding this comment.
I think this changes the error code for attempting to execute a closed program.
There was a problem hiding this comment.
I'll check and fix it.
There was a problem hiding this comment.
We probably need to store the reason for tombstone in the cache. Otherwise we don't have enough information to return the correct error code.
pgarg66
force-pushed
the
cache-try
branch
2 times, most recently
from
db79d29
to
412079b
Compare
| @@ -180,9 +180,6 @@ pub fn load_program_from_account( | |||
| return Err(InstructionError::IncorrectProgramId); | |||
| } | |||
|
|
|||
| let (programdata_offset, deployment_slot) = | |||
| get_programdata_offset_and_depoyment_offset(&log_collector, program, programdata)?; | |||
|
|
|||
| if let Some(ref tx_executor_cache) = tx_executor_cache { | |||
| if let Some(loaded_program) = tx_executor_cache.get(program.get_key()) { | |||
| if loaded_program.is_tombstone() { | |||
There was a problem hiding this comment.
I was referring to the comment below which is incorrect now.
|
Tested by cherry-picking #30928 on top of this PR. The results match for all the modified tests. |
Problem
Program/ProgramData accounts get loaded even though the compiled program exists in the cache.
Summary of Changes
Check if the compiled program is already cached. If it is, do not load its program and program data accounts.
Fixes #