Refactor - `LoadedProgramType::Loaded` and `LoadedProgramOwner` #606

Lichtso · 2024-04-05T13:57:51Z

Problems

The loaded program tests currently only test themselves (with LoadedProgramType::TestLoaded) instead of the production code.
The account owner / loader is monomorphized into LegacyV0, LegacyV1 and Typed, which has to be undone everywhere the carried executable is needed. It should be a separate field of LoadedProgram instead and that could also be used by other LoadedProgramTypes.

Summary of Changes

Unifies LoadedProgramTypes LegacyV0, LegacyV1, Typed and TestLoaded into Loaded.
Adds LoadedProgram::account_owner() and LoadedProgramOwner. Using the pubkey of the loader directly was also an option, but I didn't see any reason to waste storing and comparing 256 bits when there can be at most log_2(5) ~ 2.3219 bits of entropy.

dmakarov · 2024-04-09T14:41:52Z

The account owner / loader is monomorphized into LegacyV0, LegacyV1 and Typed, which has to be undone everywhere the carried executable is needed.

Could you, please, explain in plain English what this means?

Lichtso · 2024-04-09T15:28:41Z

What I meant is that instead of a generic LoadedProgramType::Loaded which would be paried with LoadedProgramOwner like so:

program_type: LoadedProgramType::Loaded(executable), account_owner: LoadedProgramOwner::LoaderV1
program_type: LoadedProgramType::Loaded(executable), account_owner: LoadedProgramOwner::LoaderV2
program_type: LoadedProgramType::Loaded(executable), account_owner: LoadedProgramOwner::LoaderV3
program_type: LoadedProgramType::Loaded(executable), account_owner: LoadedProgramOwner::LoaderV4

we instead have a combined one (basically a form of monomorphization):

program_type: LoadedProgramType::LegacyV1(executable)
program_type: LoadedProgramType::LegacyV1(executable)
program_type: LoadedProgramType::LegacyV2(executable)
program_type: LoadedProgramType::Typed(executable)

So whenever you want to get the contained executable, you have to explicitly match program_type to destructure them:

LoadedProgramType::LegacyV1(executable) => executable,
LoadedProgramType::LegacyV2(executable) => executable,
LoadedProgramType::Typed(executable) => executable,

codecov-commenter · 2024-04-09T17:15:29Z

Codecov Report

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

Project coverage is 81.9%. Comparing base (499d36e) to head (e4aaf05).
Report is 63 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master     #606     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         851      851             
  Lines      231209   231288     +79     
=========================================
+ Hits       189495   189536     +41     
- Misses      41714    41752     +38

dmakarov · 2024-04-09T17:44:17Z

program-runtime/src/loaded_programs.rs

-    #[cfg(test)]
-    TestLoaded(ProgramRuntimeEnvironment),
+    /// Verified and compiled program
+    Loaded(Executable<InvokeContext<'static>>),


maybe not to address in this PR, but I think the enum name is misnomer. A LoadedProgramType value can be 'Loaded', 'Unloaded', or something else. It doesn't make sense when a 'loaded program' is in fact 'unloaded'. It also doesn't make sense that for a LoadedProgramType you add an enum value 'Loaded'. If only 'Loaded' programs are loaded, then what are other 'LoadedProgramType' values?

Yes you are right, it evolved over time and we never adjusted the naming.
How about renaming (in a different PR):

LoadedProgram => ProgramCacheEntry

LoadedProgramType => ProgramCacheEntryType

Definitely, sounds better. I don't insist that these name changes should be in this PR. It's up to you.

dmakarov · 2024-04-09T17:53:48Z

What I meant is that instead of a generic LoadedProgramType::Loaded which would be paried with LoadedProgramOwner like so:

* `program_type: LoadedProgramType::Loaded(executable), account_owner: LoadedProgramOwner::LoaderV1`

* `program_type: LoadedProgramType::Loaded(executable), account_owner: LoadedProgramOwner::LoaderV2`

* `program_type: LoadedProgramType::Loaded(executable), account_owner: LoadedProgramOwner::LoaderV3`

* `program_type: LoadedProgramType::Loaded(executable), account_owner: LoadedProgramOwner::LoaderV4`

we instead have a combined one (basically a form of monomorphization):

* `program_type: LoadedProgramType::LegacyV1(executable)`

* `program_type: LoadedProgramType::LegacyV1(executable)`

* `program_type: LoadedProgramType::LegacyV2(executable)`

* `program_type: LoadedProgramType::Typed(executable)`

So whenever you want to get the contained executable, you have to explicitly match program_type to destructure them:

* `LoadedProgramType::LegacyV1(executable) => executable,`

* `LoadedProgramType::LegacyV2(executable) => executable,`

* `LoadedProgramType::Typed(executable) => executable,`

Thanks for the explanation. I'm not convinced that monomorphization is applicable nomenclature in this situation. As far as I understand monomorphization is performed by compiler if a trait can be resolved to a concrete type at compile time, when static dispatch is used.

Lichtso · 2024-04-09T17:59:10Z

It might be a generous view of what "monomorphization" includes, but IMO it means instantiating generics by giving them a distinct code path each. And because the enum items have individual code paths it made sense to me.

Merges LoadedProgramTypes LegacyV0, LegacyV1, Typed and TestLoaded into Loaded.

alessandrod

Sorry it took so long to review!

I love this, we're testing more, code looks better and logic is simpler. Thanks a lot for doing it.

I only left a few nits, otherwise looks great.

alessandrod · 2024-04-16T23:34:33Z

program-runtime/src/loaded_programs.rs

-            LoadedProgramType::LegacyV0(executable)
-        } else if bpf_loader::check_id(loader_key) || bpf_loader_upgradeable::check_id(loader_key) {
-            LoadedProgramType::LegacyV1(executable)
+        let account_owner = if bpf_loader_deprecated::check_id(loader_key) {


nit: this is LoadedProgramOwner::try_from(loader_key)

alessandrod · 2024-04-16T23:35:27Z

program-runtime/src/loaded_programs.rs

@@ -393,6 +391,7 @@ impl LoadedProgram {
        }
        Some(Self {
            program: LoadedProgramType::Unloaded(self.program.get_environment()?.clone()),
+            account_owner: self.account_owner.clone(),


nit: the owner type should probably be Copy?

alessandrod · 2024-04-16T23:40:08Z

program-runtime/src/loaded_programs.rs

@@ -464,6 +469,16 @@ impl LoadedProgram {
        let decaying_for = std::cmp::min(63, now.saturating_sub(last_access));
        self.tx_usage_counter.load(Ordering::Relaxed) >> decaying_for
    }
+
+    pub fn account_owner(&self) -> Pubkey {
+        match self.account_owner {


nit: this is Into for LoadedProgramOwner

alessandrod · 2024-04-16T23:43:19Z

program-runtime/src/loaded_programs.rs

-                            Some((*id, program.clone()))
+                        LoadedProgramType::Loaded(_) => {
+                            let include =
+                                if let LoadedProgramOwner::LoaderV4 = program.account_owner {


nit: if you implement Eq for LoadedProgramOwner then I think this can become one
if with LoaderV4 && include_program_runtime_v2 { .. } else if
include_program_runtime_v1 { ... }

…-xyz#606) * Adds LoadedProgram::account_owner() and LoadedProgramOwner. Merges LoadedProgramTypes LegacyV0, LegacyV1, Typed and TestLoaded into Loaded. * Review feedback.

Lichtso force-pushed the refactor/remove_unloaded branch from 4514047 to 9aa7c05 Compare April 8, 2024 23:59

Lichtso requested review from alessandrod and pgarg66 April 9, 2024 00:11

Lichtso force-pushed the refactor/remove_unloaded branch 2 times, most recently from 4199d18 to 26e2adf Compare April 9, 2024 12:35

Lichtso force-pushed the refactor/remove_unloaded branch from 26e2adf to 8261960 Compare April 9, 2024 15:39

dmakarov reviewed Apr 9, 2024

View reviewed changes

Lichtso force-pushed the refactor/remove_unloaded branch 2 times, most recently from 3339803 to cf8f6ba Compare April 12, 2024 08:53

Adds LoadedProgram::account_owner() and LoadedProgramOwner.

1e59c45

Merges LoadedProgramTypes LegacyV0, LegacyV1, Typed and TestLoaded into Loaded.

Lichtso force-pushed the refactor/remove_unloaded branch from cf8f6ba to 1e59c45 Compare April 12, 2024 13:05

buffalojoec mentioned this pull request Apr 15, 2024

Refactor - Remove program_accounts_map from account_loader #768

Merged

pgarg66 previously approved these changes Apr 16, 2024

View reviewed changes

alessandrod previously approved these changes Apr 16, 2024

View reviewed changes

Lichtso dismissed stale reviews from alessandrod and pgarg66 via ff96903 April 17, 2024 07:21

Lichtso force-pushed the refactor/remove_unloaded branch from ff96903 to 235c54b Compare April 17, 2024 07:35

Review feedback.

e4aaf05

Lichtso force-pushed the refactor/remove_unloaded branch from 235c54b to e4aaf05 Compare April 17, 2024 08:32

alessandrod approved these changes Apr 17, 2024

View reviewed changes

Lichtso merged commit ac36dbb into anza-xyz:master Apr 17, 2024
38 checks passed

Lichtso deleted the refactor/remove_unloaded branch April 17, 2024 15:48

Lichtso mentioned this pull request Apr 18, 2024

Refactor - Rename LoadedProgram to ProgramCacheEntry #880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor - `LoadedProgramType::Loaded` and `LoadedProgramOwner` #606

Refactor - `LoadedProgramType::Loaded` and `LoadedProgramOwner` #606

Lichtso commented Apr 5, 2024

dmakarov commented Apr 9, 2024

Lichtso commented Apr 9, 2024

codecov-commenter commented Apr 9, 2024 •

edited

Loading

dmakarov Apr 9, 2024

Lichtso Apr 9, 2024

dmakarov Apr 9, 2024

dmakarov commented Apr 9, 2024

Lichtso commented Apr 9, 2024

alessandrod left a comment

alessandrod Apr 16, 2024

alessandrod Apr 16, 2024

alessandrod Apr 16, 2024

alessandrod Apr 16, 2024

Refactor - LoadedProgramType::Loaded and LoadedProgramOwner #606

Refactor - LoadedProgramType::Loaded and LoadedProgramOwner #606

Conversation

Lichtso commented Apr 5, 2024

Problems

Summary of Changes

dmakarov commented Apr 9, 2024

Lichtso commented Apr 9, 2024

codecov-commenter commented Apr 9, 2024 • edited Loading

Codecov Report

dmakarov Apr 9, 2024

Choose a reason for hiding this comment

Lichtso Apr 9, 2024

Choose a reason for hiding this comment

dmakarov Apr 9, 2024

Choose a reason for hiding this comment

dmakarov commented Apr 9, 2024

Lichtso commented Apr 9, 2024

alessandrod left a comment

Choose a reason for hiding this comment

alessandrod Apr 16, 2024

Choose a reason for hiding this comment

alessandrod Apr 16, 2024

Choose a reason for hiding this comment

alessandrod Apr 16, 2024

Choose a reason for hiding this comment

alessandrod Apr 16, 2024

Choose a reason for hiding this comment

Refactor - `LoadedProgramType::Loaded` and `LoadedProgramOwner` #606

Refactor - `LoadedProgramType::Loaded` and `LoadedProgramOwner` #606

codecov-commenter commented Apr 9, 2024 •

edited

Loading