feat: add svm crate - #21
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. |
6908a8c to
bf5041b
Compare
a03f0fd to
d2ebd2c
Compare
81aff13 to
b426266
Compare
63967b5 to
8fad9d3
Compare
0386de8 to
ea8636a
Compare
aaa6060 to
7150fbd
Compare
ba48aa0 to
de2075a
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
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 crate baseline Cargo.toml, solana/README.md, solana/svm/Cargo.toml, solana/svm/README.md, solana/program-runtime/Cargo.toml |
Adds the local SVM workspace member, updates Solana dependencies, reduces SVM features and dependencies, and documents Engine runtime boundaries. |
Callback account and program loading solana/svm/src/account_loader.rs, solana/svm/src/program_loader.rs, solana/svm/src/lib.rs |
Replaces stateful account loading with callback access, records program indices and loaded sizes, simplifies program loading, and removes obsolete override and rollback exports. |
Single-transaction execution path solana/svm/src/transaction_processor.rs, solana/svm/src/message_processor.rs, solana/svm/src/access_permissions.rs |
Replaces batch processing with single-transaction execution, passes program indices through instruction processing, validates account mutations, checks lamport conservation, and updates CPI grouping. |
Transaction state and result contracts solana/svm/src/rent_calculator.rs, solana/svm/src/transaction_account_state_info.rs, solana/svm/src/transaction_balances.rs, solana/svm/src/transaction_execution_result.rs, solana/svm/src/transaction_processing_result.rs, solana/svm/src/transaction_processing_callback.rs |
Uses optional rent states and direct keyed-account balances, removes token and rollback result structures, and stores boxed executed transactions with simplified execution details. |
Example crate metadata alignment solana/svm/tests/example-programs/*/Cargo.toml, solana/transaction-context/Cargo.toml |
Updates explicit package versions and metadata ordering for example programs and transaction-context. |
Estimated code review effort: 5 (Critical) | ~120 minutes
Possibly related issues
- magicblock-labs/magicblock-engine#32 — The PR adds and integrates the Solana execution crates described by this issue.
Possibly related PRs
- magicblock-labs/magicblock-engine#31 — The PR modifies the same Solana manifests and core SVM modules introduced by that PR.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title identifies the primary change: adding the customized SVM crate to the workspace. |
| Description check | ✅ Passed | The description directly explains the customized solana-svm fork, engine execution model, and removed validator-oriented behavior. |
| Linked Issues check | ✅ Passed | For [#8], the PR forks solana-svm, adapts account loading and execution to callbacks, and removes validator-oriented surfaces. |
| Out of Scope Changes check | ✅ Passed | The documented runtime differences, API changes, dependency updates, and removed validator surfaces support the linked issue scope. |
| 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
svm
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
solana/svm/src/account_loader.rs (1)
212-225: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse the already loaded program account for the owner check.
The loop at Line 207 loads every account key through the callback and stores the result in
loaded_transaction_accounts.accounts. The loop at Line 212 callsget_account_shared_dataagain for each instruction program id. This repeats one callback lookup per instruction, and the owner check can read a different snapshot than the account that is later executed.Index the already collected account by
instruction.program_id_indexinstead.♻️ Proposed refactor
- for (program_id, instruction) in message.program_instructions_iter() { - let Some(program_account) = account_loader.get_account_shared_data(program_id) else { - return Err(TransactionError::ProgramAccountNotFound); - }; - - let owner_id = program_account.0.owner(); - if !PROGRAM_OWNERS.contains(owner_id) { - return Err(TransactionError::InvalidProgramForExecution); - } - - loaded_transaction_accounts - .program_indices - .push(instruction.program_id_index as IndexOfAccount); - } + for (_program_id, instruction) in message.program_instructions_iter() { + let index = instruction.program_id_index as IndexOfAccount; + let Some((_, program_account)) = loaded_transaction_accounts + .accounts + .get(index as usize) + else { + return Err(TransactionError::ProgramAccountNotFound); + }; + + if !PROGRAM_OWNERS.contains(program_account.owner()) { + return Err(TransactionError::InvalidProgramForExecution); + } + + loaded_transaction_accounts.program_indices.push(index); + }Note: this changes the error for a missing program account. A missing account is materialized as a default account at Line 246, so the owner check reports
InvalidProgramForExecutioninstead ofProgramAccountNotFound. Several tests assertProgramAccountNotFound. Keep the current behavior if that error mapping is required.🤖 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/svm/src/account_loader.rs` around lines 212 - 225, Update the program-instruction loop to retrieve the program account from loaded_transaction_accounts.accounts using instruction.program_id_index instead of calling account_loader.get_account_shared_data. Perform the existing owner validation against that already loaded account and preserve the current missing-account error mapping if required by existing tests.solana/svm/src/access_permissions.rs (1)
227-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the privileged fee-payer case.
The comment states that a dirty immutable fee payer is rejected "unless privileged". Every row sets
privilegedtofalse, so the exemption is never exercised. Add one row with a privileged transaction, or remove the clause from the comment.💚 Proposed addition
let cases = [ // (payer, privileged, accepted) (account(AccountMode::ReadOnly), false, false), (account(AccountMode::Transient), false, false), (account(AccountMode::Delegated), false, true), + (account(AccountMode::ReadOnly), true, true), ];🤖 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/svm/src/access_permissions.rs` around lines 227 - 235, Add a case to the `fee_payer_guard` test table where the fee payer is immutable, `privileged` is true, and the expected result is accepted, so the test covers the documented privileged exemption while preserving the existing cases.
🤖 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 `@solana/svm/src/access_permissions.rs`:
- Around line 81-86: Update the rustdoc comment for the account function to
describe that it builds a dirtied account in the requested AccountMode, rather
than referring specifically to Delegated mode. Keep the implementation
unchanged.
In `@solana/svm/src/program_loader.rs`:
- Around line 13-26: Update load_program so executable accounts owned by
bpf_loader_upgradeable::id() are not passed to ProgramCacheEntry::new or
inserted into the program cache; filter that owner out of PROGRAM_OWNERS or
bypass caching for it, while preserving caching for ELF-backed owners.
In `@solana/svm/src/rent_calculator.rs`:
- Around line 71-81: Update the rustdoc for get_account_rent_state to state that
ephemeral accounts are classified as RentExempt regardless of their lamport
balance, alongside the lamports and data-size inputs. Keep the implementation
unchanged and identify this as the fork-specific exemption affecting rent-state
transitions.
In `@solana/svm/src/transaction_processor.rs`:
- Around line 154-173: Update the rustdoc for new_uninitialized and new to
reflect that the caller-provided ProgramCache is preserved and may already
contain programs, and remove the outdated runtime-environment statement. Since
new and new_uninitialized are identical, consolidate them into a single
constructor if the API permits, updating references and documentation
accordingly.
- Around line 247-266: Update replenish_program_cache so failed verification
results from load_program are not stored in self.program_cache; cache and reuse
entries only when verification succeeds, while still replenishing
program_cache_for_tx_batch with the current result. Ensure failed entries can be
retried after runtime-environment changes or program redeployments.
- Around line 230-238: Ensure balance_collector.collect_post_balances runs
regardless of the result of executed_tx.access_is_valid(tx), so native_pre and
native_post remain aligned when the transaction is returned successfully. Keep
the program-cache drain and merge restricted to the valid-access branch, while
preserving the existing Ok(Box::new(executed_tx)) return.
---
Nitpick comments:
In `@solana/svm/src/access_permissions.rs`:
- Around line 227-235: Add a case to the `fee_payer_guard` test table where the
fee payer is immutable, `privileged` is true, and the expected result is
accepted, so the test covers the documented privileged exemption while
preserving the existing cases.
In `@solana/svm/src/account_loader.rs`:
- Around line 212-225: Update the program-instruction loop to retrieve the
program account from loaded_transaction_accounts.accounts using
instruction.program_id_index instead of calling
account_loader.get_account_shared_data. Perform the existing owner validation
against that already loaded account and preserve the current missing-account
error mapping if required by existing tests.
🪄 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: 37df8f36-abd6-4935-b6a8-52f4e4d843ae
⛔ Files ignored due to path filters (6)
solana/svm/doc/diagrams/context.svgis excluded by!**/*.svgsolana/svm/tests/example-programs/clock-sysvar/clock_sysvar_program.sois excluded by!**/*.sosolana/svm/tests/example-programs/hello-solana/hello_solana_program.sois excluded by!**/*.sosolana/svm/tests/example-programs/simple-transfer/simple_transfer_program.sois excluded by!**/*.sosolana/svm/tests/example-programs/transfer-from-account/transfer_from_account_program.sois excluded by!**/*.sosolana/svm/tests/example-programs/write-to-account/write_to_account_program.sois excluded by!**/*.so
📒 Files selected for processing (33)
Cargo.tomlsolana/README.mdsolana/program-runtime/Cargo.tomlsolana/svm/Cargo.tomlsolana/svm/README.mdsolana/svm/doc/diagrams/context.texsolana/svm/doc/spec.mdsolana/svm/src/access_permissions.rssolana/svm/src/account_loader.rssolana/svm/src/account_overrides.rssolana/svm/src/lib.rssolana/svm/src/message_processor.rssolana/svm/src/nonce_info.rssolana/svm/src/program_loader.rssolana/svm/src/rent_calculator.rssolana/svm/src/rollback_accounts.rssolana/svm/src/transaction_account_state_info.rssolana/svm/src/transaction_balances.rssolana/svm/src/transaction_commit_result.rssolana/svm/src/transaction_error_metrics.rssolana/svm/src/transaction_execution_result.rssolana/svm/src/transaction_processing_callback.rssolana/svm/src/transaction_processing_result.rssolana/svm/src/transaction_processor.rssolana/svm/tests/concurrent_tests.rssolana/svm/tests/example-programs/clock-sysvar/Cargo.tomlsolana/svm/tests/example-programs/hello-solana/Cargo.tomlsolana/svm/tests/example-programs/simple-transfer/Cargo.tomlsolana/svm/tests/example-programs/transfer-from-account/Cargo.tomlsolana/svm/tests/example-programs/write-to-account/Cargo.tomlsolana/svm/tests/integration_test.rssolana/svm/tests/mock_bank.rssolana/transaction-context/Cargo.toml
💤 Files with no reviewable changes (9)
- solana/svm/src/transaction_commit_result.rs
- solana/svm/doc/spec.md
- solana/svm/src/transaction_error_metrics.rs
- solana/svm/src/nonce_info.rs
- solana/svm/tests/concurrent_tests.rs
- solana/svm/doc/diagrams/context.tex
- solana/svm/src/rollback_accounts.rs
- solana/svm/tests/mock_bank.rs
- solana/svm/src/account_overrides.rs
|
Regarding the |
1 participant
What changed
Customized the imported
solana-svmbaseline into the engine's caller-ownedtransaction loader and executor, and patched it into the workspace.
Why
Persistence and commit policy must remain above transaction execution rather
than inside validator-owned bank state.
Closes #8.
Impact
transaction_processing_callbackand returns the mutated account set after execution.
and
PROGRAM_OWNERSownership.surfaces that are outside this engine boundary.
Reviewer notes
Rent-state and lamport-balance checks still surround execution, but this runtime
owns no validator state. The fork constraints are documented in
solana/README.md.Follow-up
processorschedules transactions through this SVM later in the stack.