Refactor - LoadedPrograms::assign_program()
#35233
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #35233 +/- ##
=========================================
- Coverage 81.6% 81.6% -0.1%
=========================================
Files 834 834
Lines 224798 224829 +31
=========================================
+ Hits 183485 183499 +14
- Misses 41313 41330 +17 |
|
LGTM. @alessandrod, are you planning to review as well? |
|
Yep going to review today |
| (LoadedProgramType::Unloaded(_), LoadedProgramType::TestLoaded(_)) => {} | ||
| _ => { | ||
| // Something is wrong, I can feel it ... | ||
| debug_assert!(false); |
There was a problem hiding this comment.
nit: we should log::error! and print existing and entry here
There was a problem hiding this comment.
nit: pass a panic message to debug_assert and test for it in should_panic in tests
| existing.ix_usage_counter.load(Ordering::Relaxed), | ||
| Ordering::Relaxed, | ||
| ); | ||
| *existing = entry.clone(); |
There was a problem hiding this comment.
This should be Arc::clone(&entry)
| Ordering::Relaxed, | ||
| ); | ||
| *existing = entry.clone(); | ||
| self.stats.reloads.fetch_add(1, Ordering::Relaxed); | ||
| } | ||
| Err(index) => { | ||
| self.stats.insertions.fetch_add(1, Ordering::Relaxed); | ||
| slot_versions.insert(index, entry.clone()); |
There was a problem hiding this comment.
Arc::clone too.
And shouldn't we also assert which entry types can't get inserted here? Like DelayVisibility
| LoadedProgramType::Builtin(BuiltinProgram::new_mock()), | ||
| LoadedProgramType::Builtin(BuiltinProgram::new_mock()) | ||
| )] | ||
| fn test_assign_program_success(old: LoadedProgramType, new: LoadedProgramType) { |
There was a problem hiding this comment.
What about LegacyV*? We have some elf files in the monorepo can't we test those?
There was a problem hiding this comment.
If we do that I want to do it for the entire file in a separate PR and get rid of TestLoaded entirely.
Lichtso
force-pushed
the
refactor/loaded_programs_assign_program
branch
2 times, most recently
from
e178517 to
ab0509f
Compare
Lichtso
force-pushed
the
refactor/loaded_programs_assign_program
branch
from
ab0509f to
89b434d
Compare
There was a problem hiding this comment.
Approving, keeping in mind this needs to be done #35233 (comment)
This PR introduces the Core BPF implementation of Address Lookup Table. There are a few caveats with the implementation so far. You can find them in the source code by searching for comments prefixed with: ``` [Core BPF]: ``` The following caveats will be solved in the next Solana release, and a discussion can be had for backporting any of these changes: - `InstructionError::Immutable` has no `ProgramError` counterpart ([#35113](solana-labs/solana#35113)). - `InstructionError::IncorrectAuthority` has no `ProgramError` counterpart ([#35113](solana-labs/solana#35113)). - `solana-program-test` will not overwrite a builtin if the BPF program you've provided shares the same address as an existing builtin ([#35233](solana-labs/solana#35233)). The following caveat seems to be unavoidable: - The `build.rs` script and annotations in `lib.rs` are required for `solana-frozen-abi-macro` (See #3). Finally, I've implemented a cooldown period based on `Clock`, but this will likely not be sufficient. I think we should consider merging this initial implementation and leaving #1 unfinished until the proper research is completed.
Problem
Summary of Changes
test_assign_program_tombstones()test_assign_program_failure()andtest_assign_program_success().