feat: add program runtime crate - #20
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
526f178 to
6b99920
Compare
293b261 to
bdadc77
Compare
fea9f0b to
a7dda81
Compare
e1e6b84 to
d977fde
Compare
1a52f77 to
e478278
Compare
55478c4 to
c2aa052
Compare
|
@CodeRabbit review |
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning
|
| Layer / File(s) | Summary |
|---|---|
Workspace and program cache foundation Cargo.toml, solana/program-runtime/Cargo.toml, solana/program-runtime/README.md, solana/program-runtime/src/lib.rs, solana/program-runtime/src/loaded_programs.rs |
Adds the crate to the workspace, updates dependencies and metadata, documents the fork, removes obsolete modules, and replaces the fork-aware cache with synchronous entries. |
Execution context and budgets solana/program-runtime/src/invoke_context.rs, solana/program-runtime/src/execution_budget.rs |
Updates instruction preparation, executable dispatch, syscall contexts, mock execution, stack handling, and feature-dependent invocation costs. |
Direct memory and VM execution solana/program-runtime/src/serialization.rs, solana/program-runtime/src/memory.rs, solana/program-runtime/src/memory_context.rs, solana/program-runtime/src/mem_pool.rs, solana/program-runtime/src/vm.rs |
Uses direct account-data mappings, explicit memory regions, fixed stack sizing, updated serialization APIs, and revised VM access-violation handling. |
CPI translation and synchronization solana/program-runtime/src/cpi.rs |
Validates CPI pointers and metadata, applies feature-dependent limits and compute costs, maps account data directly, and synchronizes caller and callee regions. |
Deployment and sysvar state solana/program-runtime/src/deploy.rs, solana/program-runtime/src/sysvar_cache.rs |
Simplifies deployment environment and cache-entry handling and updates sysvar storage and access through InvokeContext. |
Estimated code review effort: 5 (Critical) | ~120 minutes
Possibly related issues
magicblock-labs/magicblock-engine#32: Imports and adapts theprogram-runtimecrate, including source, tests, fixtures, and manifest updates.
Possibly related PRs
- magicblock-labs/magicblock-engine#31: Refactors the same
solana-program-runtimeAPIs across CPI, invocation context, deployment, caching, serialization, VM, and execution budgets.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the primary change: adding the program runtime crate. |
| Description check | ✅ Passed | The description explains the customized runtime, direct account mapping, engine scope, and linked issue. |
| Linked Issues check | ✅ Passed | The changes fork and adapt the runtime for engine execution, direct account mapping, and removal of validator-oriented surfaces required by issue #9. |
| Out of Scope Changes check | ✅ Passed | The reviewed changes support the runtime fork and engine execution objectives without clear unrelated modifications. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
program-runtime
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
solana/program-runtime/src/deploy.rs (1)
87-112: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winUpdate
deploy_programto match the refactored deployment API.
deploy_program!still createsLoadedProgramMetrics, passes$loader_key,$account_size, and$deployment_slot, and callsload_program_metrics.submit_datapoint(...), but it only expands insidesolana/program-runtime/src/deploy.rsand the realdeploy::deploy_programnow takes no metrics. Call the refactored API directly so this path compiles.🐛 Proposed fix for the macro call
assert_eq!( $deployment_slot, $invoke_context.program_cache_for_tx_batch.slot() ); - #[cfg(feature = "metrics")] - let mut load_program_metrics = $crate::loaded_programs::LoadProgramMetrics::default(); $crate::deploy::deploy_program( $invoke_context.get_log_collector(), - #[cfg(feature = "metrics")] - &mut load_program_metrics, $invoke_context.program_cache_for_tx_batch, $invoke_context .get_program_runtime_environments_for_deployment() .get_env_for_deployment() .clone(), $program_id, - $loader_key, - $account_size, $programdata, - $deployment_slot, )?; - #[cfg(feature = "metrics")] - load_program_metrics.submit_datapoint(&mut $invoke_context_timings);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solana/program-runtime/src/deploy.rs` around lines 87 - 112, Update the deploy_program! macro to call the refactored deploy::deploy_program API without creating or passing LoadProgramMetrics, $loader_key, $account_size, or $deployment_slot arguments. Remove the associated submit_datapoint call and preserve the existing invocation context, environment, program ID, and programdata arguments required by the new signature.
🧹 Nitpick comments (5)
solana/program-runtime/src/cpi.rs (1)
237-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the reserved-address-space formula into one helper.
The same rule appears three times:
get_serialized_data(lines 237-242),update_caller_account_region(lines 1044-1048), andupdate_caller_account(lines 1092-1097). Each copy derivesis_caller_loader_deprecatedfrom!check_alignedand then addsMAX_PERMITTED_DATA_INCREASEfor non-deprecated loaders.serialization.rs::Serializer::write_accountencodes the same rule a fourth time. This value bounds account growth, so a divergence between copies becomes a realloc-limit defect.♻️ Proposed refactor
+/// Address space reserved for an account's data in the caller's VM memory. +fn address_space_reserved_for_account(check_aligned: bool, original_data_len: usize) -> usize { + let is_caller_loader_deprecated = !check_aligned; + if is_caller_loader_deprecated { + original_data_len + } else { + original_data_len.saturating_add(MAX_PERMITTED_DATA_INCREASE) + } +}) -> Result<&'a mut [u8], Error> { - let is_caller_loader_deprecated = !check_aligned; - let address_space_reserved_for_account = if is_caller_loader_deprecated { - original_data_len - } else { - original_data_len.saturating_add(MAX_PERMITTED_DATA_INCREASE) - }; - if len > address_space_reserved_for_account { + if len > address_space_reserved_for_account(check_aligned, original_data_len) { return Err(InstructionError::InvalidRealloc.into()); } Ok(&mut []) }As per path instructions: "Maintenance issues that materially increase long-term complexity, duplication, or unnecessary abstraction. Focus on clear DRY/YAGNI violations only."
Also applies to: 1044-1048, 1092-1097
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solana/program-runtime/src/cpi.rs` around lines 237 - 246, Extract the reserved address-space calculation into a shared helper and replace the duplicated logic in get_serialized_data, update_caller_account_region, and update_caller_account. Ensure the helper derives deprecated-loader status from check_aligned and adds MAX_PERMITTED_DATA_INCREASE only for non-deprecated loaders; update serialization.rs::Serializer::write_account to reuse the same rule.Source: Path instructions
solana/program-runtime/src/mem_pool.rs (1)
99-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
get_heapignoresheap_sizeoutside the debug assertion.The pool always returns a
MAX_HEAP_FRAME_BYTESbuffer. The caller invm.rsslices it down to the requested size, so behavior is correct, but the parameter name suggests an allocation size. Add a short comment that the pool intentionally uses uniform maximum-size buffers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solana/program-runtime/src/mem_pool.rs` around lines 99 - 104, Add a brief comment to get_heap documenting that the pool intentionally allocates and returns uniform MAX_HEAP_FRAME_BYTES buffers, while callers may slice them to heap_size. Keep the existing allocation behavior and parameter unchanged.solana/program-runtime/src/vm.rs (2)
240-246: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCall frames are now allocated on every program execution.
The previous implementation pooled call frames in
VmMemoryPool. This code builds a freshVecofmax_call_depthframes for each execution, including every CPI level. That adds one heap allocation and initialization per instruction on the execution hot path. Restore pooling for call frames, or reuse a thread-local buffer next toMEMORY_POOL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solana/program-runtime/src/vm.rs` around lines 240 - 246, Update the call-frame setup in the VM execution path to reuse pooled storage instead of collecting a fresh Vec for every execution and CPI level. Restore integration with VmMemoryPool, or use a thread-local buffer alongside MEMORY_POOL, while preserving max_call_depth sizing and the existing CallFrame initialization semantics.
48-75: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
accounts_metadatais stored twice per invocation.
create_vmclones the metadata intoSyscallContextand moves it intoMemoryContext. Both copies live for the same instruction frame. Consumers read fromget_syscall_context(), andMemoryContextholds the second copy. Keep one owner, or wrap the metadata inArcso the clone is cheap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solana/program-runtime/src/vm.rs` around lines 48 - 75, Update create_vm so accounts_metadata is not fully duplicated between SyscallContext and MemoryContext; use a shared Arc-backed ownership model if both contexts require access, and update their field types or constructors consistently. Preserve metadata availability through get_syscall_context() and MemoryContext while avoiding a deep clone per invocation.solana/program-runtime/src/invoke_context.rs (1)
1269-1290: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or track the commented-out configuration block.
The block is dead code with a conditional note about trace reordering. Delete it, or replace it with an issue reference so it does not remain indefinitely.
Do you want me to open an issue to track the trace reordering work?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@solana/program-runtime/src/invoke_context.rs` around lines 1269 - 1290, Remove the commented-out transaction_context.configure_instruction_at_index blocks and their trace-reordering note. If the trace reordering work must remain tracked, replace the block with a concise issue reference rather than retaining dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Around line 81-95: Remove the duplicate solana-system-interface,
solana-sysvar, and solana-sysvar-id entries from [workspace.dependencies] in
Cargo.toml. In solana/program-runtime/Cargo.toml, retain only one authors entry,
one repository entry, and one version entry, choosing consistently between {
workspace = true } and "4.1.1" for version.
In `@solana/program-runtime/README.md`:
- Around line 3-9: Update the program-runtime README description to remove or
correct the claim that workspace [patch.crates-io] entries force the dependency
graph to use this copy, since the root patch only targets agave-transaction-view
and does not select solana-program-runtime. Preserve the remaining scope and
architecture documentation.
- Around line 15-16: Update the compatibility-stub sentence in the README to
refer to the manifest’s agave-unstable-api feature instead of frozen-abi,
preserving the statement that it is a no-op and does not provide frozen ABI
metadata.
In `@solana/program-runtime/src/cpi.rs`:
- Line 747: Remove the cached raw `memory_mapping` pointer used across
`invoke_context.process_instruction(...)` in the CPI flow. Re-acquire
`MemoryMapping` through `invoke_context.memory_contexts.memory_mapping_mut()`
only after `process_instruction` returns, then use that fresh reference for the
synchronization logic currently dereferencing the mapping near the CPI-exit
handling.
- Line 457: Update the account-meta translation flow around
translate_slice::<AccountMeta> and translate_slice::<SolAccountMeta> to avoid
creating typed slices over VM-controlled bool fields. Read the raw bytes or
MaybeUninit representations first, validate is_signer and is_writable, then
initialize or convert them into AccountMeta values before exposing any
&[AccountMeta] reference.
In `@solana/program-runtime/src/invoke_context.rs`:
- Around line 956-1009: Make the final transaction_accounts.pop() conditional on
whether the helper created the synthetic loader account in the program_index
fallback branch. Preserve the existing epoch-schedule cleanup, and avoid
removing any caller-supplied account when program_index is Some.
In `@solana/program-runtime/src/loaded_programs.rs`:
- Around line 278-292: Update ProgramCache::get to use the index’s closure-based
read_sync API instead of get_sync, cloning the ProgramCacheEntry inside the read
closure so the bucket lock is released before returning. Leave assign_program
and merge unchanged.
In `@solana/program-runtime/src/serialization.rs`:
- Line 37: Update the doc comment for create_memory_region_of_account to
describe the returned value as a single MemoryRegion, using “region” terminology
consistent with modify_memory_region_of_account and the function’s actual return
type.
---
Outside diff comments:
In `@solana/program-runtime/src/deploy.rs`:
- Around line 87-112: Update the deploy_program! macro to call the refactored
deploy::deploy_program API without creating or passing LoadProgramMetrics,
$loader_key, $account_size, or $deployment_slot arguments. Remove the associated
submit_datapoint call and preserve the existing invocation context, environment,
program ID, and programdata arguments required by the new signature.
---
Nitpick comments:
In `@solana/program-runtime/src/cpi.rs`:
- Around line 237-246: Extract the reserved address-space calculation into a
shared helper and replace the duplicated logic in get_serialized_data,
update_caller_account_region, and update_caller_account. Ensure the helper
derives deprecated-loader status from check_aligned and adds
MAX_PERMITTED_DATA_INCREASE only for non-deprecated loaders; update
serialization.rs::Serializer::write_account to reuse the same rule.
In `@solana/program-runtime/src/invoke_context.rs`:
- Around line 1269-1290: Remove the commented-out
transaction_context.configure_instruction_at_index blocks and their
trace-reordering note. If the trace reordering work must remain tracked, replace
the block with a concise issue reference rather than retaining dead code.
In `@solana/program-runtime/src/mem_pool.rs`:
- Around line 99-104: Add a brief comment to get_heap documenting that the pool
intentionally allocates and returns uniform MAX_HEAP_FRAME_BYTES buffers, while
callers may slice them to heap_size. Keep the existing allocation behavior and
parameter unchanged.
In `@solana/program-runtime/src/vm.rs`:
- Around line 240-246: Update the call-frame setup in the VM execution path to
reuse pooled storage instead of collecting a fresh Vec for every execution and
CPI level. Restore integration with VmMemoryPool, or use a thread-local buffer
alongside MEMORY_POOL, while preserving max_call_depth sizing and the existing
CallFrame initialization semantics.
- Around line 48-75: Update create_vm so accounts_metadata is not fully
duplicated between SyscallContext and MemoryContext; use a shared Arc-backed
ownership model if both contexts require access, and update their field types or
constructors consistently. Preserve metadata availability through
get_syscall_context() and MemoryContext while avoiding a deep clone per
invocation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b516c40b-5b45-440c-8afc-7ec2cdccb333
⛔ Files ignored due to path filters (1)
solana/program-runtime/fixtures/noop_aligned.sois excluded by!**/*.so
📒 Files selected for processing (18)
Cargo.tomlsolana/program-runtime/Cargo.tomlsolana/program-runtime/README.mdsolana/program-runtime/src/cpi.rssolana/program-runtime/src/deploy.rssolana/program-runtime/src/execution_budget.rssolana/program-runtime/src/invoke_context.rssolana/program-runtime/src/lib.rssolana/program-runtime/src/loaded_programs.rssolana/program-runtime/src/loading_task.rssolana/program-runtime/src/mem_pool.rssolana/program-runtime/src/memory.rssolana/program-runtime/src/memory_context.rssolana/program-runtime/src/program_cache_entry.rssolana/program-runtime/src/program_metrics.rssolana/program-runtime/src/serialization.rssolana/program-runtime/src/sysvar_cache.rssolana/program-runtime/src/vm.rs
💤 Files with no reviewable changes (3)
- solana/program-runtime/src/loading_task.rs
- solana/program-runtime/src/program_cache_entry.rs
- solana/program-runtime/src/program_metrics.rs
| solana-sysvar = "3.1.1" | ||
| solana-sysvar-id = "3.1.0" | ||
| solana-system-interface = ">=3.0.0, <3.2.0" | ||
| solana-svm-callback = "4.0.0-rc.1" | ||
| solana-svm-feature-set = "4.0.0-rc.1" | ||
| solana-svm-log-collector = "4.0.0-rc.1" | ||
| solana-svm-measure = "4.0.0-rc.1" | ||
| solana-svm-timings = "4.0.0-rc.1" | ||
| solana-svm-transaction = "4.0.0-rc.1" | ||
| solana-svm-type-overrides = "4.0.0-rc.1" | ||
| solana-system-interface = ">=3.0.0, <3.2.0" | ||
| solana-system-program = "4.0.0-rc.0" | ||
| solana-system-transaction = "3.0.0" | ||
| solana-sysvar = "3.1.1" | ||
| solana-sysvar-id = "3.1.0" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Duplicate TOML keys in both manifests block the build. Both manifests declare the same key twice inside one table, which Cargo rejects with a "duplicate key" parse error. The shared root cause is the fork import merging two key lists without deduplication.
Cargo.toml#L81-L95: remove the second declaration ofsolana-system-interface,solana-sysvar, andsolana-sysvar-idfrom[workspace.dependencies].solana/program-runtime/Cargo.toml#L4-L13: remove the duplicateauthorsandrepositorykeys, and keep exactly oneversionkey after deciding between{ workspace = true }and"4.1.1".
📍 Affects 2 files
Cargo.toml#L81-L95(this comment)solana/program-runtime/Cargo.toml#L4-L13
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` around lines 81 - 95, Remove the duplicate
solana-system-interface, solana-sysvar, and solana-sysvar-id entries from
[workspace.dependencies] in Cargo.toml. In solana/program-runtime/Cargo.toml,
retain only one authors entry, one repository entry, and one version entry,
choosing consistently between { workspace = true } and "4.1.1" for version.
| This Agave fork implements invocation state, CPI translation, SBF VM setup, | ||
| sysvar access, logging, serialization, and program-cache primitives. Workspace | ||
| `[patch.crates-io]` entries force the dependency graph to use this copy. | ||
|
|
||
| Account loading and transaction-level policy belong to `solana-svm`. The | ||
| engine-specific direct account mapping, access-violation growth, and CPI | ||
| synchronization contracts are documented in [`../README.md`](../README.md). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the patch section and the referenced parent README content.
set -euo pipefail
# Test 1: Look for a patch table pointing at this crate.
rg -n -A6 '^\[patch' Cargo.toml || echo "no [patch] table in root Cargo.toml"
# Test 2: Confirm the parent README exists and mentions the documented contracts.
fd -a 'README.md' solana --max-depth 2
rg -n -i 'direct account mapping|access.violation|CPI synchronization' solana/README.md || echo "contracts not documented in solana/README.md"Repository: magicblock-labs/magicblock-engine
Length of output: 757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "===== root Cargo.toml relevant section ====="
sed -n '1,140p' Cargo.toml
echo "===== repository README candidates ====="
git ls-files '*README.md' | sed -n '1,80p'
echo "===== search for referenced contract phrases and variants ====="
rg -n -i 'direct account mapping|account mapping|access-violation|access violation|access .violat|growing|CPI synchronization|CPI sync|CPI synchronization contracts|CPI synchronization contract' \
-g '*.md' -g '*.rs' -g 'Cargo.toml' .Repository: magicblock-labs/magicblock-engine
Length of output: 4501
Fix the crate patch claim.
The root manifest has [patch.crates-io], but it only patches agave-transaction-view; nothing in the ranges under review or its package dependencies declares solana-program-runtime, so this README text does not match the dependency graph.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@solana/program-runtime/README.md` around lines 3 - 9, Update the
program-runtime README description to remove or correct the claim that workspace
[patch.crates-io] entries force the dependency graph to use this copy, since the
root patch only targets agave-transaction-view and does not select
solana-program-runtime. Preserve the remaining scope and architecture
documentation.
Source: Path instructions
| // poor options. We can tell what a relatively high retention score is, so we can make a | ||
| // few additional samples until we hit some other entry that isn't as highly scoring. | ||
| // | ||
| // Note that the "high enough" compilation time and use count numbers used here are | ||
| // relatively arbitrary. | ||
| const MAX_ADDITIONAL_SAMPLES: usize = 3; | ||
| let avoid_evicting_above_score = retention_score(now, 500 * EMA_SCALE, 500); | ||
| for _ in 0..num_to_unload { | ||
| let (mut index, mut score) = sample_entry(&candidates); | ||
| for _ in 0..MAX_ADDITIONAL_SAMPLES { | ||
| let (sample_index, sample_score) = sample_entry(&candidates); | ||
| if score > sample_score { | ||
| index = sample_index; | ||
| score = sample_score; | ||
| } | ||
| if score < avoid_evicting_above_score { | ||
| break; | ||
| } | ||
| } | ||
| let (id, last_modification_slot, entry) = candidates.swap_remove(index); | ||
| self.unload_program_entry(id, last_modification_slot, &entry); | ||
| } | ||
| } | ||
|
|
||
| /// Removes all the entries at the given keys, if they exist | ||
| pub fn remove_programs(&mut self, keys: impl Iterator<Item = Pubkey>) { | ||
| match &mut self.index { | ||
| IndexImplementation::V1 { entries, .. } => { | ||
| for k in keys { | ||
| entries.remove(&k); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// This function removes the given entry for the given program from the cache. | ||
| /// The function expects that the program and entry exists in the cache. Otherwise it'll panic. | ||
| fn unload_program_entry( | ||
| &mut self, | ||
| id: Pubkey, | ||
| _last_modification_slot: Slot, | ||
| remove_entry: &Arc<ProgramCacheEntry>, | ||
| ) { | ||
| match &mut self.index { | ||
| IndexImplementation::V1 { entries, .. } => { | ||
| let second_level = entries.get_mut(&id).expect("Cache lookup failed"); | ||
| let candidate = second_level | ||
| .iter_mut() | ||
| .find(|entry| entry == &remove_entry) | ||
| .expect("Program entry not found"); | ||
|
|
||
| // Certain entry types cannot be unloaded, such as tombstones, or already unloaded entries. | ||
| // For such entries, `to_unloaded()` will return None. | ||
| // These entry types do not occupy much memory. | ||
| if let Some(unloaded) = candidate.to_unloaded() { | ||
| if candidate.stats.uses.load(Ordering::Relaxed) == 1 { | ||
| self.stats.one_hit_wonders.fetch_add(1, Ordering::Relaxed); | ||
| } | ||
| self.stats | ||
| .evictions | ||
| .entry(id) | ||
| .and_modify(|c| *c = c.saturating_add(1)) | ||
| .or_insert(1); | ||
| *candidate = Arc::new(unloaded); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn remove_programs_with_no_entries(&mut self) { | ||
| match &mut self.index { | ||
| IndexImplementation::V1 { entries, .. } => { | ||
| let num_programs_before_removal = entries.len(); | ||
| entries.retain(|_key, second_level| !second_level.is_empty()); | ||
| if entries.len() < num_programs_before_removal { | ||
| self.stats.empty_entries.fetch_add( | ||
| num_programs_before_removal.saturating_sub(entries.len()) as u64, | ||
| Ordering::Relaxed, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "frozen-abi")] | ||
| impl solana_frozen_abi::abi_example::AbiExample for ProgramCacheEntry { | ||
| fn example() -> Self { | ||
| // ProgramCacheEntry isn't serializable by definition. | ||
| Self::default() | ||
| } | ||
| } | ||
|
|
||
| #[cfg(feature = "frozen-abi")] | ||
| impl<FG: ForkGraph> solana_frozen_abi::abi_example::AbiExample for ProgramCache<FG> { | ||
| fn example() -> Self { | ||
| // ProgramCache isn't serializable by definition. | ||
| Self::new(Slot::default()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the pinned scc version in the workspace manifests.
fd -t f 'Cargo.toml' | xargs rg -n 'scc'
fd -t f 'Cargo.lock' -x rg -n -A2 '^name = "scc"' {}Repository: magicblock-labs/magicblock-engine
Length of output: 235
🌐 Web query:
scc crate HashMap upsert_sync get_sync methods documentation
💡 Result:
In the scc crate, the HashMap struct provides both synchronous and asynchronous methods for managing key-value pairs [1][2]. Regarding your specific request about upsert and get operations: upsert_sync Method The upsert_sync method is a synchronous function that upserts a key-value pair into the HashMap [1][3]. Signature: pub fn upsert_sync(&self, key: K, val: V) -> Option [1][3] Behavior: It inserts the provided key-value pair into the map [3]. If the key is already present, it updates the value and returns the old value [1][3]. If the key is not present, it inserts the new pair and returns None [1][3]. get_sync Method It is important to note that the HashMap in the scc crate does not feature a method named get_sync. Instead, the crate provides read_sync and other related methods to access or manipulate data synchronously [2][4][5]. For example, to read a value associated with a key synchronously, you would typically use read_sync [2][4]: pub fn read_sync<Q, R, F>(&self, key: &Q, f: F) -> Option where K: Borrow, Q: Hash + Eq +?Sized, F: FnOnce(&K, &V) -> R, This method allows you to execute a closure f on the key and value if the key exists, returning the result of the closure, or None if the key is not found [4][6]. For more complex workflows, the HashMap provides an Entry API (entry_sync) that allows for operations such as or_insert [2][6].
Citations:
- 1: https://docs.rs/scc/latest/scc/hash_map/struct.HashMap.html
- 2: https://docs.rs/crate/scc/latest
- 3: https://docs.rs/scc/latest/src/scc/hash_map.rs.html
- 4: https://docs.rs/scc/latest/scc/index.html
- 5: https://github.com/wvwwvwwv/scalable-concurrent-containers/
- 6: https://docs.rs/scc
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- workspace scc version ---"
rg -n '^\[workspace\.dependencies\]|^\[workspace\.dependencies\.\]|^scc\s*=' solana/Cargo.toml -R or true
rg -n -A2 -B2 '^\[workspace\.dependencies\]' Cargo.toml solana*/Cargo.toml 2>/dev/null || true
echo "--- Cargo.lock scc section and all versions ---"
rg -n -A4 '^name = "scc"$|version = ' Cargo.lock | sed -n '1,80p'
echo "--- loaded_programs section ---"
wc -l solana/program-runtime/src/loaded_programs.rs
sed -n '250,305p' solana/program-runtime/src/loaded_programs.rs
echo "--- all scc HashMap synchronous API-like usages ---"
rg -n '\b(scc::HashMap|type .*HashMap|HashMap<|read_sync|upsert_sync|get_sync|assign_program|merge)\b' -S solana/program-runtime src 2>/dev/null || trueRepository: magicblock-labs/magicblock-engine
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- workspace scc version ---"
rg -n '^\[workspace\.dependencies\]|^\[workspace\.dependencies\.\]|^scc\s*=' solana/Cargo.toml Cargo.toml || true
echo "--- Cargo.lock scc section and all versions ---"
rg -n -A4 '^name = "scc"$|version = ' Cargo.lock | sed -n '1,100p'
echo "--- loaded_programs section ---"
wc -l solana/program-runtime/src/loaded_programs.rs
sed -n '250,305p' solana/program-runtime/src/loaded_programs.rs
echo "--- all scc HashMap synchronous API-like usages ---"
rg -n '\bscc::HashMap\b|type .*HashMap\b|HashMap<|read_sync|upsert_sync|get_sync|assign_program|merge' solana/program-runtime/src loaded_programs.rs 2>/dev/null || trueRepository: magicblock-labs/magicblock-engine
Length of output: 443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- root Cargo.toml work dep ---"
sed -n '1,80p' Cargo.toml | rg -n '^\[workspace\.dependencies\]|^scc\s*=' -C 2 || true
echo "--- load ProgramCache index type and assign/merge methods ---"
rg -n "type .*HashMap|ProgramCache|pub fn assign_program|pub fn get|pub fn merge|upsert_sync|get_sync|read_sync|entry_sync" -S . || true
echo "--- program-runtime files ---"
fd -t f Cargo.toml . | rg 'program-runtime|Cargo' || trueRepository: magicblock-labs/magicblock-engine
Length of output: 26320
🌐 Web query:
docs.rs scc 1 HashMap read_sync return value get_sync lock semantics
💡 Result:
In the scc crate, the HashMap methods read_sync and get_sync have distinct return values and locking semantics [1][2][3]. 1. read_sync: Return Value: It takes a closure and returns Option, where R is the return type of the provided closure [1][2]. Locking Semantics: It acquires a shared (read) lock on the bucket containing the target entry [3]. This lock ensures that the entry is not moved or structurally modified while the reader closure is executing, allowing safe access to the key and value [3]. 2. get_sync: Return Value: It returns an Option<OccupiedEntry<'_, K, V, H>> [1][2]. Locking Semantics: It provides exclusive access to the entry by returning an OccupiedEntry, which acts as a guard that holds a lock on the bucket [1][2]. Because it grants exclusive ownership of the entry to the caller for potential in-place manipulation, it effectively serializes access to that specific entry [1][2]. Key Differences and Warnings: - OccupiedEntry (from get_sync) exclusively owns the entry and prevents other threads from accessing it; the documentation explicitly suggests using read_sync instead if only read-only access is required [1][2]. - Both methods involve bucket-level locking [3]. As noted in the documentation, any Entry, OccupiedEntry, or VacantEntry held by the current thread while attempting to perform operations that acquire locks can lead to deadlocks [4][2]. - Blocking synchronous methods (including both get_sync and read_sync) are generally not recommended for use within asynchronous code blocks or futures, as they may cause deadlocks or performance degradation [1].
Citations:
- 1: https://docs.rs/scc/latest/src/scc/hash_map.rs.html
- 2: https://docs.rs/scc/latest/scc/hash_map/struct.HashMap.html
- 3: https://docs.rs/scc/latest/scc/
- 4: https://docs.rs/scc/latest/scc/hash_cache/struct.HashCache.html
🌐 Web query:
site:docs.rs/scc 1. HashMap get_sync return Option<OccupiedEntry> lock while holding value clone
💡 Result:
In the context of the Rust scc crate (a high-performance, concurrent hash map library), calling get_sync returns an Option<OccupiedEntry<'_, K, V, H>> [1][2]. The OccupiedEntry holds a reference to the bucket's lock [3][4]. When you obtain an OccupiedEntry via get_sync, the library acquires a write lock (or an exclusive lock) on the specific bucket containing that entry [3][1]. As long as the OccupiedEntry exists, it holds this lock, preventing other threads from accessing that specific entry (or bucket) [1][5]. If you perform a clone of the value while holding the OccupiedEntry, the lock remains active for the duration of the entry's lifetime [1]. Because the lock is held by the OccupiedEntry itself, cloning the value does not release the lock; the lock is only released when the OccupiedEntry is dropped [3][1]. Key technical points regarding this behavior: 1. Bucket-Level Locking: scc::HashMap and similar structures in the crate use bucket-level read-write locks, not container-level locks, to maximize concurrency [1][6]. 2. Exclusive Ownership: OccupiedEntry provides exclusive mutable access to an entry [1][2]. Because it exclusively owns the entry for the duration of its lifetime, it effectively blocks other threads from accessing that entry until the OccupiedEntry is dropped [1]. 3. Performance Implications: Since the lock is held for the entire lifetime of the OccupiedEntry, it is advised to use read_sync instead of get_sync if only read-only access is required, as read_sync does not hold the same exclusive lock [1][5]. 4. Deadlock Risk: Holding an OccupiedEntry while attempting to perform other operations that require the same lock (or performing operations that trigger further locking) can lead to deadlocks [1]. In summary, the OccupiedEntry does not "unlock" automatically upon cloning a value; it maintains its exclusive lock on the bucket until the entry object goes out of scope and is dropped [3][1].
Citations:
- 1: https://docs.rs/scc/latest/scc/hash_map/struct.HashMap.html
- 2: https://docs.rs/scc/latest/scc/hash_map/struct.Reserve.html
- 3: https://docs.rs/scc/latest/src/scc/hash_map.rs.html
- 4: https://docs.rs/scc/latest/src/scc/hash_cache.rs.html
- 5: https://docs.rs/scc/latest/scc/hash_index/struct.HashIndex.html
- 6: https://docs.rs/scc/latest/scc/index.html
Replace get_sync with read_sync for read-only lookups.
ProgramCache::get only clones the entry, but get_sync returns an exclusive OccupiedEntry guard that holds the bucket lock until the guard is dropped. This unnecessarily blocks concurrent upsert_sync/other map operations on the same bucket. Use a closure-based synchronous read API so the lock can be released before cloning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@solana/program-runtime/src/loaded_programs.rs` around lines 278 - 292, Update
ProgramCache::get to use the index’s closure-based read_sync API instead of
get_sync, cloning the ProgramCacheEntry inside the read closure so the bucket
lock is released before returning. Leave assign_program and merge unchanged.
1 participant
What changed
Customized the imported
solana-program-runtimebaseline for mapped accountregions and patched the crate into the workspace.
Why
The engine needs invocation state, CPI translation, and SBF VM setup that map
account data directly into the VM instead of threading it through a serialized
input buffer.
Closes #9.
Impact
logging, and the program cache needed by the engine.
MemoryRegions while preservingloader-selected ABI formats.
boundary.
Reviewer notes
Serialization and VM error remapping must stay aligned with the
transaction-context access-violation handler. The boundary is documented in
solana/README.md.Follow-up
svmdrives this customized runtime in the next PR.