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
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.