Repository navigation
feat: add svm crate - #21
Conversation
6908a8c to
bf5041b
Compare
a03f0fd to
d2ebd2c
Compare
81aff13 to
b426266
Compare
0386de8 to
ea8636a
Compare
aaa6060 to
7150fbd
Compare
ba48aa0 to
de2075a
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 59 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR adds a local Solana SVM workspace fork for Engine execution. It replaces batch-oriented loading and processing with callback-based single-transaction execution, simplifies public result and state types, adds account-access validation, and updates workspace dependencies and documentation. ChangesEngine SVM execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[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.