chore: import upstream Solana runtime crates - #31
Open
bmuddha wants to merge 1 commit into
Open
Conversation
This was referenced Jul 27, 2026
Collaborator
Author
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 ignored due to path filters (6)
📒 Files selected for processing (78)
📝 WalkthroughWalkthroughThis PR imports upstream Solana account, transaction-context, program-runtime, SVM, and transaction-view crates. It adds manifests, runtime structures, VM and transaction-processing logic, zero-copy parsing, tests, fixtures, benchmarks, and SVM documentation. ChangesUpstream Solana execution crates
Estimated code review effort: 5 (Critical) | ~180 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 12
🤖 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/account/src/state_traits.rs`:
- Line 10: Correct the typo in the documentation comment for the convenience
trait by changing “covert” to “convert”; do not alter the trait or its behavior.
In `@solana/program-runtime/src/cpi.rs`:
- Around line 1226-1229: Correct the duplicated article in the safety
documentation comment near the MemoryMapping usage, changing “The the account
data” to “The account data” without altering the documented safety requirement.
In `@solana/program-runtime/src/loaded_programs.rs`:
- Around line 978-986: Fix the fixture setup used by new_loaded_entry so it can
load noop_aligned.so in a tracked checkout: either add the missing test ELF at
the expected location with the required parent directory, or update the file
path to an existing tracked fixture. Ensure tests invoking
new_test_entry_with_usage no longer panic during fixture initialization.
In `@solana/svm/doc/spec.md`:
- Line 243: Correct the spelling of “interwined” to “intertwined” in the
sentence describing the relationship with the SVMInstruction struct.
- Line 74: Update the context diagram reference in the documentation to use the
repository’s correct relative location, and ensure the referenced rendered
context.svg is added alongside the documentation; otherwise remove the image
reference and retain only valid diagram sources.
In `@solana/svm/src/program_loader.rs`:
- Around line 242-255: Update loader_v4_get_state to avoid the undocumented
slice transmute by using a checked, alignment-safe deserialization approach such
as from_le_bytes or bytemuck. If retaining the transmute, validate pointer
alignment before casting and add a precise // SAFETY: comment documenting the
alignment, size, layout, and lifetime invariants.
In `@solana/svm/src/rent_calculator.rs`:
- Around line 75-79: Update the rustdoc for the free function
get_account_rent_state to describe that it receives a Rent reference directly
and remove the claims about a default implementation and nonexistent get_rent
method. Also revise the corresponding carried-over documentation at the other
noted sections so it accurately reflects the current free-function API.
In `@solana/svm/src/transaction_processor.rs`:
- Around line 377-385: Update the rustdoc for
program_runtime_environment_for_epoch to remove the inaccurate statement that it
returns None on potential deadlock. Keep the description consistent with its
actual ProgramRuntimeEnvironment return value and fallback behavior.
In `@solana/svm/tests/concurrent_tests.rs`:
- Around line 104-107: Update the concurrency test comments near the
probabilistic and exhaustive scheduler descriptions to remove the stale
hardcoded iteration counts and reference MAX_ITERATIONS instead. Keep the
existing preemption and thread-count details accurate, and ensure both comments
match the actual iteration count used by their tests.
In `@solana/svm/tests/integration_test.rs`:
- Around line 1405-1452: Append the constructed batch 3 `test_entry` to
`test_entries` before leaving the surrounding `simd83_intrabatch_account_reuse`
setup, matching the other batches’ `test_entries.push(test_entry)` calls so this
scenario executes.
In `@solana/transaction-context/src/instruction_accounts.rs`:
- Line 90: Correct the spelling in the documentation comment from “Assignes” to
“Assigns,” leaving the surrounding comment and behavior unchanged.
In `@solana/transaction-context/src/instruction.rs`:
- Around line 39-41: Update the comment above instruction_accounts and
instruction_data to describe the actual default pointer value of 0, removing the
incorrect reference to u64::MAX while preserving the existing VmSlice
initialization.
🪄 Autofix (Beta)
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: 60b2ebde-c865-4aab-a122-d504aeddfcc9
⛔ 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 (60)
solana/account/Cargo.tomlsolana/account/src/lib.rssolana/account/src/state_traits.rssolana/program-runtime/Cargo.tomlsolana/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/stable_log.rssolana/program-runtime/src/sysvar_cache.rssolana/program-runtime/src/vm.rssolana/svm/Cargo.tomlsolana/svm/doc/diagrams/context.texsolana/svm/doc/spec.mdsolana/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/clock-sysvar/src/lib.rssolana/svm/tests/example-programs/hello-solana/Cargo.tomlsolana/svm/tests/example-programs/hello-solana/src/lib.rssolana/svm/tests/example-programs/simple-transfer/Cargo.tomlsolana/svm/tests/example-programs/simple-transfer/src/lib.rssolana/svm/tests/example-programs/transfer-from-account/Cargo.tomlsolana/svm/tests/example-programs/transfer-from-account/src/lib.rssolana/svm/tests/example-programs/write-to-account/Cargo.tomlsolana/svm/tests/example-programs/write-to-account/src/lib.rssolana/svm/tests/integration_test.rssolana/svm/tests/mock_bank.rssolana/transaction-context/Cargo.tomlsolana/transaction-context/src/instruction.rssolana/transaction-context/src/instruction_accounts.rssolana/transaction-context/src/lib.rssolana/transaction-context/src/transaction.rssolana/transaction-context/src/transaction_accounts.rssolana/transaction-context/src/vm_addresses.rssolana/transaction-context/src/vm_slice.rs
| std::cell::Ref, | ||
| }; | ||
|
|
||
| /// Convenience trait to covert bincode errors to instruction errors. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo: "covert" → "convert".
✏️ Proposed fix
-/// Convenience trait to covert bincode errors to instruction errors.
+/// Convenience trait to convert bincode errors to instruction errors.As per path instructions, "Typos in identifiers, comments, or user-facing strings."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Convenience trait to covert bincode errors to instruction errors. | |
| /// Convenience trait to convert bincode errors to instruction errors. |
🤖 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/account/src/state_traits.rs` at line 10, Correct the typo in the
documentation comment for the convenience trait by changing “covert” to
“convert”; do not alter the trait or its behavior.
Source: Path instructions
Comment on lines
+978
to
+986
| fn new_loaded_entry(env: ProgramRuntimeEnvironment) -> ProgramCacheEntryType { | ||
| let mut elf = Vec::new(); | ||
| File::open("../programs/bpf_loader/test_elfs/out/noop_aligned.so") | ||
| .unwrap() | ||
| .read_to_end(&mut elf) | ||
| .unwrap(); | ||
| let executable = Executable::load(&elf, Arc::clone(&*env)).unwrap(); | ||
| ProgramCacheEntryType::Loaded(executable) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -HI 'noop_aligned.so'
fd -HI -t d 'test_elfs'Repository: magicblock-labs/magicblock-engine
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching noop_aligned.so =="
git ls-files | grep -E '(^|/)[^/]*noop_aligned\.so$' || true
echo "== repo files matching test_elfs directories =="
git ls-files | grep -E '(^|/)test_elfs($|/)' || true
echo "== candidate loaded_programs file =="
fd -HI '^loaded_programs\.rs$' .
echo "== occurrences of new_test_entry/new_loaded_entry/test_elfs =="
rg -n "new_test_entry|new_loaded_entry|test_elfs|noop_aligned" . || true
echo "== git diff stat/names if available =="
git diff --stat || true
git diff --name-only || trueRepository: magicblock-labs/magicblock-engine
Length of output: 6521
Add the missing test ELF fixture or update the path.
new_loaded_entry needs ../programs/bpf_loader/test_elfs/out/noop_aligned.so, but this tracked checkout does not include that file or the parent directory. Tests that call new_test_entry_with_usage in loaded_programs.rs will panic on open during fixture setup.
🤖 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 978 - 986, Fix
the fixture setup used by new_loaded_entry so it can load noop_aligned.so in a
tracked checkout: either add the missing test ELF at the expected location with
the required parent directory, or update the file path to an existing tracked
fixture. Ensure tests invoking new_test_entry_with_usage no longer panic during
fixture initialization.
| bank. It creates an SVM, submits transactions for execution and | ||
| receives results of transaction execution from SVM. | ||
|
|
||
|  |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Diagram link is broken in this repo layout.
The path is repo-root-absolute (/svm/doc/...) but the file lives under solana/svm/doc/, and only context.tex was imported — no rendered context.svg exists. Use a relative path and add the rendered SVG (or drop the image).
📝 Proposed fix
-
+🤖 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/doc/spec.md` at line 74, Update the context diagram reference in
the documentation to use the repository’s correct relative location, and ensure
the referenced rendered context.svg is added alongside the documentation;
otherwise remove the image reference and retain only valid diagram sources.
Comment on lines
+377
to
+385
| /// Returns the current environments depending on the given epoch | ||
| /// Returns None if the call could result in a deadlock | ||
| pub fn program_runtime_environment_for_epoch(&self, epoch: Epoch) -> ProgramRuntimeEnvironment { | ||
| self.epoch_boundary_preparation | ||
| .read() | ||
| .unwrap() | ||
| .get_upcoming_environment_for_epoch(epoch) | ||
| .unwrap_or_else(|| ProgramRuntimeEnvironment::clone(&self.program_runtime_environment)) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale doc comment: this function cannot return None.
The signature returns ProgramRuntimeEnvironment and always falls back to the current environment, so the "Returns None if the call could result in a deadlock" line is inaccurate.
As per path instructions: "Check docs and rustdoc for factual consistency with the code."
📝 Proposed doc fix
/// Returns the current environments depending on the given epoch
- /// Returns None if the call could result in a deadlock
+ /// Falls back to the current epoch's environment if no upcoming
+ /// environment is prepared for the given epoch.
pub fn program_runtime_environment_for_epoch(&self, epoch: Epoch) -> ProgramRuntimeEnvironment {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Returns the current environments depending on the given epoch | |
| /// Returns None if the call could result in a deadlock | |
| pub fn program_runtime_environment_for_epoch(&self, epoch: Epoch) -> ProgramRuntimeEnvironment { | |
| self.epoch_boundary_preparation | |
| .read() | |
| .unwrap() | |
| .get_upcoming_environment_for_epoch(epoch) | |
| .unwrap_or_else(|| ProgramRuntimeEnvironment::clone(&self.program_runtime_environment)) | |
| } | |
| /// Returns the current environments depending on the given epoch | |
| /// Falls back to the current epoch's environment if no upcoming | |
| /// environment is prepared for the given epoch. | |
| pub fn program_runtime_environment_for_epoch(&self, epoch: Epoch) -> ProgramRuntimeEnvironment { | |
| self.epoch_boundary_preparation | |
| .read() | |
| .unwrap() | |
| .get_upcoming_environment_for_epoch(epoch) | |
| .unwrap_or_else(|| ProgramRuntimeEnvironment::clone(&self.program_runtime_environment)) | |
| } |
🤖 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/transaction_processor.rs` around lines 377 - 385, Update the
rustdoc for program_runtime_environment_for_epoch to remove the inaccurate
statement that it returns None on potential deadlock. Keep the description
consistent with its actual ProgramRuntimeEnvironment return value and fallback
behavior.
Source: Path instructions
Comment on lines
+1405
to
+1452
| // batch 3: | ||
| // * non-processable transfer due to underfunded fee-payer (two signatures) | ||
| // * successful transfer with the same fee-payer (one signature) | ||
| { | ||
| let mut test_entry = SvmTestEntry::default(); | ||
|
|
||
| let feepayer_keypair = Keypair::new(); | ||
| let feepayer = feepayer_keypair.pubkey(); | ||
| let separate_source_keypair = Keypair::new(); | ||
| let separate_source = separate_source_keypair.pubkey(); | ||
| let destination = Pubkey::new_unique(); | ||
|
|
||
| let mut feepayer_data = AccountSharedData::default(); | ||
| let mut separate_source_data = AccountSharedData::default(); | ||
| let mut destination_data = AccountSharedData::default(); | ||
|
|
||
| feepayer_data.set_lamports(1 + LAMPORTS_PER_SIGNATURE + wallet_rent); | ||
| test_entry.add_initial_account(feepayer, &feepayer_data); | ||
|
|
||
| separate_source_data.set_lamports(LAMPORTS_PER_SOL * 10); | ||
| test_entry.add_initial_account(separate_source, &separate_source_data); | ||
|
|
||
| test_entry.push_transaction_with_status( | ||
| Transaction::new_signed_with_payer( | ||
| &[system_instruction::transfer( | ||
| &separate_source, | ||
| &destination, | ||
| 1, | ||
| )], | ||
| Some(&feepayer), | ||
| &[&feepayer_keypair, &separate_source_keypair], | ||
| Hash::default(), | ||
| ), | ||
| ExecutionStatus::Discarded, | ||
| ); | ||
|
|
||
| test_entry.push_transaction(system_transaction::transfer( | ||
| &feepayer_keypair, | ||
| &destination, | ||
| 1, | ||
| Hash::default(), | ||
| )); | ||
|
|
||
| destination_data.checked_add_lamports(1).unwrap(); | ||
| test_entry.create_expected_account(destination, &destination_data); | ||
|
|
||
| test_entry.decrease_expected_lamports(&feepayer, 1 + LAMPORTS_PER_SIGNATURE); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Batch 3 is built but never added to test_entries, so it never runs.
Every other batch in simd83_intrabatch_account_reuse ends with test_entries.push(test_entry) (lines 1303, 1346, 1402, 1500). Batch 3 does not, so the "underfunded fee-payer discarded, then same fee-payer succeeds" scenario is silently dropped from the suite.
🐛 Proposed fix
test_entry.decrease_expected_lamports(&feepayer, 1 + LAMPORTS_PER_SIGNATURE);
+
+ test_entries.push(test_entry);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // batch 3: | |
| // * non-processable transfer due to underfunded fee-payer (two signatures) | |
| // * successful transfer with the same fee-payer (one signature) | |
| { | |
| let mut test_entry = SvmTestEntry::default(); | |
| let feepayer_keypair = Keypair::new(); | |
| let feepayer = feepayer_keypair.pubkey(); | |
| let separate_source_keypair = Keypair::new(); | |
| let separate_source = separate_source_keypair.pubkey(); | |
| let destination = Pubkey::new_unique(); | |
| let mut feepayer_data = AccountSharedData::default(); | |
| let mut separate_source_data = AccountSharedData::default(); | |
| let mut destination_data = AccountSharedData::default(); | |
| feepayer_data.set_lamports(1 + LAMPORTS_PER_SIGNATURE + wallet_rent); | |
| test_entry.add_initial_account(feepayer, &feepayer_data); | |
| separate_source_data.set_lamports(LAMPORTS_PER_SOL * 10); | |
| test_entry.add_initial_account(separate_source, &separate_source_data); | |
| test_entry.push_transaction_with_status( | |
| Transaction::new_signed_with_payer( | |
| &[system_instruction::transfer( | |
| &separate_source, | |
| &destination, | |
| 1, | |
| )], | |
| Some(&feepayer), | |
| &[&feepayer_keypair, &separate_source_keypair], | |
| Hash::default(), | |
| ), | |
| ExecutionStatus::Discarded, | |
| ); | |
| test_entry.push_transaction(system_transaction::transfer( | |
| &feepayer_keypair, | |
| &destination, | |
| 1, | |
| Hash::default(), | |
| )); | |
| destination_data.checked_add_lamports(1).unwrap(); | |
| test_entry.create_expected_account(destination, &destination_data); | |
| test_entry.decrease_expected_lamports(&feepayer, 1 + LAMPORTS_PER_SIGNATURE); | |
| } | |
| // batch 3: | |
| // * non-processable transfer due to underfunded fee-payer (two signatures) | |
| // * successful transfer with the same fee-payer (one signature) | |
| { | |
| let mut test_entry = SvmTestEntry::default(); | |
| let feepayer_keypair = Keypair::new(); | |
| let feepayer = feepayer_keypair.pubkey(); | |
| let separate_source_keypair = Keypair::new(); | |
| let separate_source = separate_source_keypair.pubkey(); | |
| let destination = Pubkey::new_unique(); | |
| let mut feepayer_data = AccountSharedData::default(); | |
| let mut separate_source_data = AccountSharedData::default(); | |
| let mut destination_data = AccountSharedData::default(); | |
| feepayer_data.set_lamports(1 + LAMPORTS_PER_SIGNATURE + wallet_rent); | |
| test_entry.add_initial_account(feepayer, &feepayer_data); | |
| separate_source_data.set_lamports(LAMPORTS_PER_SOL * 10); | |
| test_entry.add_initial_account(separate_source, &separate_source_data); | |
| test_entry.push_transaction_with_status( | |
| Transaction::new_signed_with_payer( | |
| &[system_instruction::transfer( | |
| &separate_source, | |
| &destination, | |
| 1, | |
| )], | |
| Some(&feepayer), | |
| &[&feepayer_keypair, &separate_source_keypair], | |
| Hash::default(), | |
| ), | |
| ExecutionStatus::Discarded, | |
| ); | |
| test_entry.push_transaction(system_transaction::transfer( | |
| &feepayer_keypair, | |
| &destination, | |
| 1, | |
| Hash::default(), | |
| )); | |
| destination_data.checked_add_lamports(1).unwrap(); | |
| test_entry.create_expected_account(destination, &destination_data); | |
| test_entry.decrease_expected_lamports(&feepayer, 1 + LAMPORTS_PER_SIGNATURE); | |
| test_entries.push(test_entry); | |
| } |
🤖 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/tests/integration_test.rs` around lines 1405 - 1452, Append the
constructed batch 3 `test_entry` to `test_entries` before leaving the
surrounding `simd83_intrabatch_account_reuse` setup, matching the other batches’
`test_entries.push(test_entry)` calls so this scenario executes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
solana/svm/doc/spec.md (1)
249-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent naming for the
load_transaction_accountsreturn type.Three different names are used for what appears to be the same return value:
LoadedTransaction(line 249), thenTransactionLoadedResultandLoadTransaction(line 253). This is confusing for readers trying to map the spec to actual types. Pick one consistent name (matching the real struct name insolana/svm/src/account_loader.rs) throughout this step. Markdownlint also flags line 253's indentation as inconsistent with its sibling list items — worth aligning while fixing the naming.📝 Proposed fix
- - Returns `TransactionLoadedResult`, containing the `LoadTransaction` we obtained from `loaded_transaction_accounts` + - Returns a `TransactionLoadedResult`, containing the `LoadedTransaction` we obtained from `load_transaction_accounts`🤖 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/doc/spec.md` around lines 249 - 253, Standardize the return-type naming in the load_transaction_accounts specification to the actual struct name defined in account_loader.rs, replacing the inconsistent LoadedTransaction, TransactionLoadedResult, and LoadTransaction references. While updating that step, align the indentation of the Returns list item with its sibling entries.Source: Linters/SAST tools
🤖 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/program-runtime/src/loaded_programs.rs`:
- Around line 1938-1944: Update the stale effective-slot references in the
loaded-programs test comments for program4 to use slot 18, matching 15 +
DELAY_VISIBILITY_SLOT_OFFSET and the existing slot-18 assertion. Change comments
only; preserve the tombstone assertions and test behavior.
---
Nitpick comments:
In `@solana/svm/doc/spec.md`:
- Around line 249-253: Standardize the return-type naming in the
load_transaction_accounts specification to the actual struct name defined in
account_loader.rs, replacing the inconsistent LoadedTransaction,
TransactionLoadedResult, and LoadTransaction references. While updating that
step, align the indentation of the Returns list item with its sibling entries.
🪄 Autofix (Beta)
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: e28c8379-2749-4b68-8f81-30f52b4fa17b
⛔ 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 (60)
solana/account/Cargo.tomlsolana/account/src/lib.rssolana/account/src/state_traits.rssolana/program-runtime/Cargo.tomlsolana/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/stable_log.rssolana/program-runtime/src/sysvar_cache.rssolana/program-runtime/src/vm.rssolana/svm/Cargo.tomlsolana/svm/doc/diagrams/context.texsolana/svm/doc/spec.mdsolana/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/clock-sysvar/src/lib.rssolana/svm/tests/example-programs/hello-solana/Cargo.tomlsolana/svm/tests/example-programs/hello-solana/src/lib.rssolana/svm/tests/example-programs/simple-transfer/Cargo.tomlsolana/svm/tests/example-programs/simple-transfer/src/lib.rssolana/svm/tests/example-programs/transfer-from-account/Cargo.tomlsolana/svm/tests/example-programs/transfer-from-account/src/lib.rssolana/svm/tests/example-programs/write-to-account/Cargo.tomlsolana/svm/tests/example-programs/write-to-account/src/lib.rssolana/svm/tests/integration_test.rssolana/svm/tests/mock_bank.rssolana/transaction-context/Cargo.tomlsolana/transaction-context/src/instruction.rssolana/transaction-context/src/instruction_accounts.rssolana/transaction-context/src/lib.rssolana/transaction-context/src/transaction.rssolana/transaction-context/src/transaction_accounts.rssolana/transaction-context/src/vm_addresses.rssolana/transaction-context/src/vm_slice.rs
🚧 Files skipped from review as they are similar to previous changes (57)
- solana/svm/src/transaction_processing_callback.rs
- solana/svm/tests/example-programs/clock-sysvar/Cargo.toml
- solana/svm/tests/example-programs/hello-solana/src/lib.rs
- solana/svm/tests/example-programs/clock-sysvar/src/lib.rs
- solana/svm/tests/example-programs/simple-transfer/Cargo.toml
- solana/svm/tests/example-programs/transfer-from-account/Cargo.toml
- solana/svm/src/lib.rs
- solana/account/Cargo.toml
- solana/svm/tests/example-programs/simple-transfer/src/lib.rs
- solana/svm/tests/example-programs/hello-solana/Cargo.toml
- solana/svm/tests/example-programs/transfer-from-account/src/lib.rs
- solana/transaction-context/src/vm_addresses.rs
- solana/svm/src/transaction_error_metrics.rs
- solana/program-runtime/src/lib.rs
- solana/transaction-context/Cargo.toml
- solana/svm/src/transaction_commit_result.rs
- solana/svm/src/nonce_info.rs
- solana/program-runtime/Cargo.toml
- solana/transaction-context/src/vm_slice.rs
- solana/svm/src/transaction_execution_result.rs
- solana/svm/tests/example-programs/write-to-account/src/lib.rs
- solana/svm/doc/diagrams/context.tex
- solana/svm/src/rent_calculator.rs
- solana/program-runtime/src/loading_task.rs
- solana/svm/tests/example-programs/write-to-account/Cargo.toml
- solana/program-runtime/src/memory_context.rs
- solana/svm/src/account_overrides.rs
- solana/program-runtime/src/program_metrics.rs
- solana/svm/src/rollback_accounts.rs
- solana/transaction-context/src/instruction_accounts.rs
- solana/program-runtime/src/stable_log.rs
- solana/program-runtime/src/deploy.rs
- solana/account/src/state_traits.rs
- solana/svm/src/transaction_account_state_info.rs
- solana/program-runtime/src/memory.rs
- solana/svm/src/message_processor.rs
- solana/program-runtime/src/program_cache_entry.rs
- solana/svm/tests/concurrent_tests.rs
- solana/svm/src/transaction_balances.rs
- solana/program-runtime/src/sysvar_cache.rs
- solana/transaction-context/src/lib.rs
- solana/svm/tests/mock_bank.rs
- solana/program-runtime/src/execution_budget.rs
- solana/svm/src/transaction_processing_result.rs
- solana/program-runtime/src/vm.rs
- solana/svm/src/account_loader.rs
- solana/program-runtime/src/serialization.rs
- solana/svm/src/program_loader.rs
- solana/program-runtime/src/mem_pool.rs
- solana/svm/src/transaction_processor.rs
- solana/transaction-context/src/instruction.rs
- solana/program-runtime/src/cpi.rs
- solana/transaction-context/src/transaction.rs
- solana/program-runtime/src/invoke_context.rs
- solana/svm/tests/integration_test.rs
- solana/transaction-context/src/transaction_accounts.rs
- solana/svm/Cargo.toml
Comment on lines
+1938
to
+1944
| // The effective slot of program4 deployed in slot 15 is 19. So it should not be usable in slot 16. | ||
| // A delay visibility tombstone should be returned here. | ||
| let tombstone = extracted | ||
| .find(&program4) | ||
| .expect("Failed to find the tombstone"); | ||
| assert_matches!(tombstone.program, ProgramCacheEntryType::DelayVisibility); | ||
| assert_eq!(tombstone.deployment_slot, 15); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale effective-slot values in the test comments.
program4 is deployed at slot 15 with 15 + DELAY_VISIBILITY_SLOT_OFFSET, and the assertion at Line 1955 (slot 18) plus the comment at Line 1954 establish the effective slot as 18, not 19. The comments at Line 1938 and Line 1965 say 19.
📝 Proposed comment fix
- // The effective slot of program4 deployed in slot 15 is 19. So it should not be usable in slot 16.
+ // The effective slot of program4 deployed in slot 15 is 18. So it should not be usable in slot 15.- // The effective slot of program4 deployed in slot 15 is 19. So it should be usable in slot 23.
+ // The effective slot of program4 deployed in slot 15 is 18. So it should be usable in slot 23.As per path instructions, "Check docs and rustdoc for factual consistency with the code."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The effective slot of program4 deployed in slot 15 is 19. So it should not be usable in slot 16. | |
| // A delay visibility tombstone should be returned here. | |
| let tombstone = extracted | |
| .find(&program4) | |
| .expect("Failed to find the tombstone"); | |
| assert_matches!(tombstone.program, ProgramCacheEntryType::DelayVisibility); | |
| assert_eq!(tombstone.deployment_slot, 15); | |
| // The effective slot of program4 deployed in slot 15 is 18. So it should not be usable in slot 15. | |
| // A delay visibility tombstone should be returned here. | |
| let tombstone = extracted | |
| .find(&program4) | |
| .expect("Failed to find the tombstone"); | |
| assert_matches!(tombstone.program, ProgramCacheEntryType::DelayVisibility); | |
| assert_eq!(tombstone.deployment_slot, 15); |
🤖 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 1938 - 1944,
Update the stale effective-slot references in the loaded-programs test comments
for program4 to use slot 18, matching 15 + DELAY_VISIBILITY_SLOT_OFFSET and the
existing slot-18 assertion. Change comments only; preserve the tombstone
assertions and test behavior.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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/account/src/lib.rs`:
- Around line 639-646: Update the rustdoc for
create_account_shared_data_with_fields to state that it creates an
AccountSharedData from a Sysvar, matching the function’s return type and
preserving the existing implementation.
In `@solana/program-runtime/Cargo.toml`:
- Around line 24-29: Update the frozen-abi feature wiring in the program-runtime
manifest so enabling frozen-abi also activates the optional solana-frozen-abi
dependency required by the cfg-gated AbiExample implementation for SysvarCache.
Preserve the existing feature name and ensure builds with --features frozen-abi
resolve all referenced crates.
In `@solana/program-runtime/src/invoke_context.rs`:
- Around line 350-351: Update the comment above transaction_callee_map to
accurately state that account indices are u16 and describe the capacity using
MAX_ACCOUNTS_PER_TRANSACTION, removing the outdated u8 and 256-account
rationale. Keep the Vec<u16> initialization unchanged.
In `@solana/program-runtime/src/loading_task.rs`:
- Line 17: Correct the spelling of “cooprative” to “cooperative” in the doc
comment describing the loading-task suspension behavior.
In `@solana/program-runtime/src/program_metrics.rs`:
- Line 33: Correct the duplicated “the” in the documentation comment describing
Self::compilation_time_ema, leaving the identifier and surrounding documentation
unchanged.
In `@solana/transaction-context/src/transaction_accounts.rs`:
- Around line 109-110: Remove the explicit pub(crate) visibility from
set_data_from_slice and try_borrow_mut when they are annotated with
cfg_attr(..., qualifiers(pub)), allowing the attribute to provide visibility
without generating duplicate qualifiers.
In `@solana/transaction-context/src/transaction.rs`:
- Around line 259-262: Update the documentation for get_next_instruction_context
to remove the nonexistent prepare_next_instruction and
prepare_next_top_level_instruction references, and instead describe
configuration through configure_instruction_at_index, mentioning the *_for_tests
helpers only if needed.
In `@solana/transaction-view/Cargo.toml`:
- Line 3: Update the package description in Cargo.toml from “Agave
TranactionView” to “Agave TransactionView”, correcting the misspelled
user-facing crate name.
In `@solana/transaction-view/src/bytes.rs`:
- Line 150: Correct the typo in the public rustdoc comment by changing “point t
the byte” to “point to the byte” while leaving the surrounding documentation and
implementation unchanged.
In `@solana/transaction-view/src/sanitize.rs`:
- Around line 25-33: Update the rustdoc above sanitize_transaction_size to
remove the inaccurate “size <= 4096 bytes” constraint and describe the
version-specific limits represented by PACKET_DATA_SIZE for Legacy/V0 and
v1::MAX_TRANSACTION_SIZE for V1.
In `@solana/transaction-view/src/transaction_frame.rs`:
- Around line 44-50: Update TransactionFrame::try_new to reject any input whose
length is not strictly below u16::MAX before calling is_legacy_or_v0 or either
frame parser, returning the existing appropriate error type. Ensure all parsing
and offset casts occur only after this validation; keep the existing legacy/v0
and v1 dispatch unchanged.
🪄 Autofix (Beta)
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: e8392034-3a93-4373-a1dc-579eaf9f03c3
⛔ 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 (78)
solana/account/Cargo.tomlsolana/account/src/lib.rssolana/account/src/state_traits.rssolana/program-runtime/Cargo.tomlsolana/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/stable_log.rssolana/program-runtime/src/sysvar_cache.rssolana/program-runtime/src/vm.rssolana/svm/Cargo.tomlsolana/svm/doc/diagrams/context.texsolana/svm/doc/spec.mdsolana/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/clock-sysvar/src/lib.rssolana/svm/tests/example-programs/hello-solana/Cargo.tomlsolana/svm/tests/example-programs/hello-solana/src/lib.rssolana/svm/tests/example-programs/simple-transfer/Cargo.tomlsolana/svm/tests/example-programs/simple-transfer/src/lib.rssolana/svm/tests/example-programs/transfer-from-account/Cargo.tomlsolana/svm/tests/example-programs/transfer-from-account/src/lib.rssolana/svm/tests/example-programs/write-to-account/Cargo.tomlsolana/svm/tests/example-programs/write-to-account/src/lib.rssolana/svm/tests/integration_test.rssolana/svm/tests/mock_bank.rssolana/transaction-context/Cargo.tomlsolana/transaction-context/src/instruction.rssolana/transaction-context/src/instruction_accounts.rssolana/transaction-context/src/lib.rssolana/transaction-context/src/transaction.rssolana/transaction-context/src/transaction_accounts.rssolana/transaction-context/src/vm_addresses.rssolana/transaction-context/src/vm_slice.rssolana/transaction-view/Cargo.tomlsolana/transaction-view/benches/bytes.rssolana/transaction-view/benches/transaction_view.rssolana/transaction-view/src/address_table_lookup_frame.rssolana/transaction-view/src/bytes.rssolana/transaction-view/src/instructions_frame.rssolana/transaction-view/src/lib.rssolana/transaction-view/src/message_header_frame.rssolana/transaction-view/src/resolved_transaction_view.rssolana/transaction-view/src/result.rssolana/transaction-view/src/sanitize.rssolana/transaction-view/src/signature_frame.rssolana/transaction-view/src/static_account_keys_frame.rssolana/transaction-view/src/transaction_config_frame.rssolana/transaction-view/src/transaction_data.rssolana/transaction-view/src/transaction_frame.rssolana/transaction-view/src/transaction_version.rssolana/transaction-view/src/transaction_view.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- solana/svm/tests/example-programs/simple-transfer/Cargo.toml
- solana/svm/tests/example-programs/clock-sysvar/Cargo.toml
- solana/svm/src/transaction_processing_callback.rs
- solana/svm/tests/example-programs/transfer-from-account/Cargo.toml
Comment on lines
+639
to
+646
| #[cfg(feature = "bincode")] | ||
| /// Create an `Account` from a `Sysvar`. | ||
| pub fn create_account_shared_data_with_fields<S: SysvarSerialize>( | ||
| sysvar: &S, | ||
| fields: InheritableAccountFields, | ||
| ) -> AccountSharedData { | ||
| AccountSharedData::from(create_account_with_fields(sysvar, fields)) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc says Account, function returns AccountSharedData.
✏️ Proposed fix
#[cfg(feature = "bincode")]
-/// Create an `Account` from a `Sysvar`.
+/// Create an `AccountSharedData` from a `Sysvar`.
pub fn create_account_shared_data_with_fields<S: SysvarSerialize>(As per path instructions, "Check docs and rustdoc for factual consistency with the code."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[cfg(feature = "bincode")] | |
| /// Create an `Account` from a `Sysvar`. | |
| pub fn create_account_shared_data_with_fields<S: SysvarSerialize>( | |
| sysvar: &S, | |
| fields: InheritableAccountFields, | |
| ) -> AccountSharedData { | |
| AccountSharedData::from(create_account_with_fields(sysvar, fields)) | |
| } | |
| #[cfg(feature = "bincode")] | |
| /// Create an `AccountSharedData` from a `Sysvar`. | |
| pub fn create_account_shared_data_with_fields<S: SysvarSerialize>( | |
| sysvar: &S, | |
| fields: InheritableAccountFields, | |
| ) -> AccountSharedData { | |
| AccountSharedData::from(create_account_with_fields(sysvar, fields)) | |
| } |
🤖 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/account/src/lib.rs` around lines 639 - 646, Update the rustdoc for
create_account_shared_data_with_fields to state that it creates an
AccountSharedData from a Sysvar, matching the function’s return type and
preserving the existing implementation.
Source: Path instructions
Comment on lines
+350
to
+351
| // We reference accounts by an u8 index, so we have a total of 256 accounts. | ||
| let mut transaction_callee_map: Vec<u16> = vec![u16::MAX; MAX_ACCOUNTS_PER_TRANSACTION]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale comment: indices are u16, not u8.
The map is Vec<u16> sized MAX_ACCOUNTS_PER_TRANSACTION, and InstructionAccount indices are u16, so the "u8 index / total of 256 accounts" rationale no longer matches the code.
📝 Proposed comment fix
- // We reference accounts by an u8 index, so we have a total of 256 accounts.
+ // Accounts are referenced by a u16 index, bounded by MAX_ACCOUNTS_PER_TRANSACTION.
let mut transaction_callee_map: Vec<u16> = vec![u16::MAX; MAX_ACCOUNTS_PER_TRANSACTION];As per path instructions, "Check docs and rustdoc for factual consistency with the code."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // We reference accounts by an u8 index, so we have a total of 256 accounts. | |
| let mut transaction_callee_map: Vec<u16> = vec![u16::MAX; MAX_ACCOUNTS_PER_TRANSACTION]; | |
| // Accounts are referenced by a u16 index, bounded by MAX_ACCOUNTS_PER_TRANSACTION. | |
| let mut transaction_callee_map: Vec<u16> = vec![u16::MAX; MAX_ACCOUNTS_PER_TRANSACTION]; |
🤖 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 350 - 351, Update
the comment above transaction_callee_map to accurately state that account
indices are u16 and describe the capacity using MAX_ACCOUNTS_PER_TRANSACTION,
removing the outdated u8 and 256-account rationale. Keep the Vec<u16>
initialization unchanged.
Source: Path instructions
| } | ||
| } | ||
|
|
||
| /// Suspends the thread in case no cooprative loading task was assigned |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo in doc comment: "cooprative" → "cooperative".
📝 Proposed fix
-/// Suspends the thread in case no cooprative loading task was assigned
+/// Suspends the thread in case no cooperative loading task was assignedAs per path instructions: "Typos in identifiers, comments, or user-facing strings."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Suspends the thread in case no cooprative loading task was assigned | |
| /// Suspends the thread in case no cooperative loading task was assigned |
🤖 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/loading_task.rs` at line 17, Correct the spelling
of “cooprative” to “cooperative” in the doc comment describing the loading-task
suspension behavior.
Source: Path instructions
Comment on lines
+259
to
+262
| /// Returns a view on the next instruction. This function assumes it has already been | ||
| /// configured with the correct values in `prepare_next_instruction` or | ||
| /// `prepare_next_top_level_instruction` | ||
| pub fn get_next_instruction_context( |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale doc: referenced functions don't exist in this crate.
prepare_next_instruction / prepare_next_top_level_instruction aren't defined here; configuration goes through configure_instruction_at_index (or the *_for_tests helpers).
✏️ Proposed fix
- /// Returns a view on the next instruction. This function assumes it has already been
- /// configured with the correct values in `prepare_next_instruction` or
- /// `prepare_next_top_level_instruction`
+ /// Returns a view on the next instruction. This function assumes the instruction has
+ /// already been configured via `configure_instruction_at_index`.As per path instructions, "Flag only real mismatches, broken examples, stale comments, or important omissions."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Returns a view on the next instruction. This function assumes it has already been | |
| /// configured with the correct values in `prepare_next_instruction` or | |
| /// `prepare_next_top_level_instruction` | |
| pub fn get_next_instruction_context( | |
| /// Returns a view on the next instruction. This function assumes the instruction has | |
| /// already been configured via `configure_instruction_at_index`. | |
| pub fn get_next_instruction_context( |
🤖 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/transaction-context/src/transaction.rs` around lines 259 - 262, Update
the documentation for get_next_instruction_context to remove the nonexistent
prepare_next_instruction and prepare_next_top_level_instruction references, and
instead describe configuration through configure_instruction_at_index,
mentioning the *_for_tests helpers only if needed.
Source: Path instructions
| @@ -0,0 +1,49 @@ | |||
| [package] | |||
| name = "agave-transaction-view" | |||
| description = "Agave TranactionView" | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo in crate description.
"Agave TranactionView" is missing an "s" — should be "Agave TransactionView". This is published to crates.io/docs.rs, making it user-facing.
✏️ Fix
-description = "Agave TranactionView"
+description = "Agave TransactionView"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| description = "Agave TranactionView" | |
| description = "Agave TransactionView" |
🤖 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/transaction-view/Cargo.toml` at line 3, Update the package description
in Cargo.toml from “Agave TranactionView” to “Agave TransactionView”, correcting
the misspelled user-facing crate name.
| Ok(()) | ||
| } | ||
|
|
||
| /// Update the `offset` to point t the byte after the `T`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo in public rustdoc.
"point t the byte" should read "point to the byte". This is on a pub fn, so it appears in generated docs.
✏️ Fix
-/// Update the `offset` to point t the byte after the `T`.
+/// Update the `offset` to point to the byte after the `T`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Update the `offset` to point t the byte after the `T`. | |
| /// Update the `offset` to point to the byte after the `T`. |
🤖 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/transaction-view/src/bytes.rs` at line 150, Correct the typo in the
public rustdoc comment by changing “point t the byte” to “point to the byte”
while leaving the surrounding documentation and implementation unchanged.
Comment on lines
+25
to
+33
| /// Transaction constraints: | ||
| /// * size <= 4096 bytes | ||
| fn sanitize_transaction_size( | ||
| view: &UnsanitizedTransactionView<impl TransactionData>, | ||
| ) -> Result<()> { | ||
| let max_transaction_size = match view.version() { | ||
| TransactionVersion::Legacy | TransactionVersion::V0 => solana_packet::PACKET_DATA_SIZE, | ||
| TransactionVersion::V1 => solana_message::v1::MAX_TRANSACTION_SIZE, | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale doc comment: size limit isn't 4096.
The code uses PACKET_DATA_SIZE (1232) for legacy/v0 and v1::MAX_TRANSACTION_SIZE for v1; the size <= 4096 bytes doc matches neither.
📝 Proposed doc fix
/// Transaction constraints:
-/// * size <= 4096 bytes
+/// * legacy/v0: size <= `PACKET_DATA_SIZE`
+/// * v1: size <= `v1::MAX_TRANSACTION_SIZE`As per path instructions, "Check docs and rustdoc for factual consistency with the code. Flag only real mismatches, broken examples, stale comments".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Transaction constraints: | |
| /// * size <= 4096 bytes | |
| fn sanitize_transaction_size( | |
| view: &UnsanitizedTransactionView<impl TransactionData>, | |
| ) -> Result<()> { | |
| let max_transaction_size = match view.version() { | |
| TransactionVersion::Legacy | TransactionVersion::V0 => solana_packet::PACKET_DATA_SIZE, | |
| TransactionVersion::V1 => solana_message::v1::MAX_TRANSACTION_SIZE, | |
| }; | |
| /// Transaction constraints: | |
| /// * legacy/v0: size <= `PACKET_DATA_SIZE` | |
| /// * v1: size <= `v1::MAX_TRANSACTION_SIZE` | |
| fn sanitize_transaction_size( | |
| view: &UnsanitizedTransactionView<impl TransactionData>, | |
| ) -> Result<()> { | |
| let max_transaction_size = match view.version() { | |
| TransactionVersion::Legacy | TransactionVersion::V0 => solana_packet::PACKET_DATA_SIZE, | |
| TransactionVersion::V1 => solana_message::v1::MAX_TRANSACTION_SIZE, | |
| }; |
🤖 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/transaction-view/src/sanitize.rs` around lines 25 - 33, Update the
rustdoc above sanitize_transaction_size to remove the inaccurate “size <= 4096
bytes” constraint and describe the version-specific limits represented by
PACKET_DATA_SIZE for Legacy/V0 and v1::MAX_TRANSACTION_SIZE for V1.
Source: Path instructions
Comment on lines
+44
to
+50
| pub(crate) fn try_new(bytes: &[u8]) -> Result<Self> { | ||
| if Self::is_legacy_or_v0(bytes)? { | ||
| Self::try_new_as_legacy_or_v0(bytes) | ||
| } else { | ||
| Self::try_new_as_v1(bytes) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether TransactionData/TransactionView enforce a max byte length before framing.
rg -n -A5 -B5 'PACKET_DATA_SIZE|u16::MAX|MAX_TRANSACTION' solana/transaction-view/src/transaction_data.rs solana/transaction-view/src/transaction_view.rs 2>/dev/nullRepository: magicblock-labs/magicblock-engine
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files solana/transaction-view/src | sed -n '1,120p'
echo "== transaction_frame outline =="
ast-grep outline solana/transaction-view/src/transaction_frame.rs 2>/dev/null || true
echo "== transaction_frame relevant lines =="
cat -n solana/transaction-view/src/transaction_frame.rs | sed -n '1,220p'
echo "== transaction_data/transaction_view relevant usages =="
for f in solana/transaction-view/src/transaction_data.rs solana/transaction-view/src/transaction_view.rs; do
echo "--- $f ---"
wc -l "$f"
rg -n -A8 -B8 'TransactionFrame|try_new\(bytes|try_new_as_|slices?\(\)|signatures|static_account_keys|recent_blockhash|PACKET_DATA_SIZE|u16::MAX|max_len|max.*len|len\(\)' "$f" || true
done
echo "== search max limits around transaction-view =="
rg -n -A4 -B4 'PACKET_DATA_SIZE|MAX_TRANSACTION|u16::MAX|\.len\(\) *>|len\(\).*BOUND|bounded|maximum|Max|MAX' solana/transaction-view/src || trueRepository: magicblock-labs/magicblock-engine
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
root = Path("solana/transaction-view/src")
for p in sorted(root.rglob("*.rs")):
text = p.read_text(errors="replace")
if "TransactionFrame::try_new" in text or "try_new(bytes" in text:
print(f"--- {p} ---")
for i,line in enumerate(text.splitlines(),1):
if "TransactionFrame::try_new" in line or "try_new(bytes" in line or "PACKET_DATA_SIZE" in line or "u16::MAX" in line:
print(f"{i}: {line}")
PYRepository: magicblock-labs/magicblock-engine
Length of output: 3684
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact TransactionFrame::try_new call sites =="
rg -n 'TransactionFrame::try_new|try_new_unsanitized|try_new_sanitized|\.data\(\)' solana -g '*.rs' --glob '!target/**' | rg -n 'TransactionFrame::try_new|try_new_unsanitized|try_new_sanitized|\.data\(\)' | sed -n '1,220p'
echo "== candidates likely creating transaction data =="
rg -n 'struct .*TransactionData|impl TransactionData|fn data\(|data: \[u8;' solana -g '*.rs' --glob '!target/**' | sed -n '1,220p'Repository: magicblock-labs/magicblock-engine
Length of output: 23011
🌐 Web query:
solana-sdk PACKET_DATA_SIZE 1232 transaction data size u16::MAX
💡 Result:
In the Solana SDK, PACKET_DATA_SIZE is a constant defined as 1,232 bytes [1][2][3]. This value represents the maximum over-the-wire serialized size of a standard Solana transaction [4][5]. The limit is derived from the minimum MTU (Maximum Transmission Unit) for IPv6, which is 1,280 bytes, by subtracting 48 bytes for network overhead (40 bytes for the IPv6 header and 8 bytes for the fragment header) [1][5][6]. Regarding the relationship with u16::MAX: 1. Transaction Size Limit: The 1,232-byte limit applies to the entire serialized transaction, including signatures and the message [5][7]. This is distinct from u16::MAX (65,535), which is the maximum value that can be represented by a 16-bit unsigned integer. 2. Compact-u16 (shortvec): While the total transaction is restricted to 1,232 bytes, the Solana transaction format uses a variable-length encoding called compact-u16 (or shortvec) for length prefixes of arrays (such as instruction counts, account counts, or data lengths) [8]. This encoding allows these fields to represent values up to u16::MAX (65,535) [8]. However, because the total transaction size is strictly capped at 1,232 bytes, it is practically impossible to utilize the full 65,535 range for most of these fields in a single transaction [8]. 3. Future Proposals: There are ongoing community proposals (e.g., SIMD-0296) to increase the maximum transaction size to 4,096 bytes using QUIC, which would allow for larger data payloads while still remaining well below the u16::MAX limit [9].
Citations:
- 1: https://github.com/solana-labs/solana/blob/master/sdk/src/packet.rs
- 2: https://solana.com/docs/core/constants-reference
- 3: https://docs.rs/solana-packet/latest/solana_packet/constant.PACKET_DATA_SIZE.html
- 4: https://docs.rs/crate/solana-sdk/1.14.13/source/src/packet.rs
- 5: https://solana.com/docs/core/transactions/transaction-structure
- 6: https://solana.com/docs/core/transactions.md
- 7: https://solana.com/uk/docs/core/transactions/transaction-structure
- 8: https://bytes.solrengine.org/learn/transactions/compact-u16
- 9: https://github.com/solana-foundation/solana-improvement-documents/blob/main/proposals/0296-larger-transactions.md
Confirm transaction buffers stay smaller than u16::MAX before frame parsing.
TransactionFrame::try_new and related frame modules document/assume packet length is below u16::MAX, but the parser itself only accepts &[u8]. The public TransactionView::try_new_sanitized size check runs after framing; if an oversized buffer reaches try_new, the offset as u16 casts can wrap and the unsafe accessors can read the wrong section. Add the size guard before parsing, or enforce this bound when constructing TransactionView.
🤖 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/transaction-view/src/transaction_frame.rs` around lines 44 - 50,
Update TransactionFrame::try_new to reject any input whose length is not
strictly below u16::MAX before calling is_legacy_or_v0 or either frame parser,
returning the existing appropriate error type. Ensure all parsing and offset
casts occur only after this validation; keep the existing legacy/v0 and v1
dispatch unchanged.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/doc/spec.md`:
- Around line 160-164: Update the TransactionCheckResult section to describe the
actual CheckedTransactionDetails contents: the optional nonce address and the
compute budget and fee limits. Remove the outdated reference to a nonce value
and lamports per signature.
- Around line 129-135: Update the documented TransactionProcessingCallback trait
to extend InvokeContextCallback and add the required inspect_account method with
the address, AccountState, and writable-status parameters, while preserving the
existing methods.
In `@solana/transaction-view/benches/bytes.rs`:
- Around line 13-15: Update the comment above the values initialization in the
benchmark to describe the packet-sized range produced by the loop, rather than
claiming it contains all valid u16 values; leave the loop and serialization
behavior unchanged.
In `@solana/transaction-view/src/bytes.rs`:
- Around line 264-268: Add solana_packet::PACKET_DATA_SIZE to the use block of
the bytes test module so its existing test references resolve without changing
other imports or behavior.
- Around line 69-85: In read_compressed_u16, replace the undefined index
reference when advancing the offset with the consumed byte position derived from
offset.wrapping_add(i).checked_add(1), preserving the existing parse-error
handling on overflow.
In `@solana/transaction-view/src/sanitize.rs`:
- Around line 94-96: Correct the typo in the documentation comment’s “Sigantures
Constraint” heading to “Signatures Constraint,” without changing the surrounding
signature constraints.
- Around line 138-152: Update the rustdoc for sanitize_instructions to replace
the hard-coded NumInstructions <= 64 statement with the actual version-specific
constants: MAGICBLOCK_INSTRUCTION_TRACE_LENGTH for Magicblock and
solana_transaction_context::MAX_INSTRUCTION_TRACE_LENGTH for Legacy, V0, and V1.
Also update the account-limit documentation to include Magicblock’s 256 limit
while preserving the existing limits for other transaction versions.
In `@solana/transaction-view/src/transaction_config_frame.rs`:
- Around line 64-86: Update TransactionConfigFrame::try_new to replace the
mask_offset > 0 assert with a checked TransactionViewError return when
mask_offset is zero. Preserve normal tx-v1 parsing for positive offsets and keep
the existing mask validation, offset conversion, and array advancement flow
unchanged.
In `@solana/transaction-view/src/transaction_frame.rs`:
- Around line 348-362: Replace the invalid usize::from conversions for the u32
offsets in the static_account_keys path with explicit as usize casts. Update
static_account_keys to construct the Pubkey slice from the existing
start/account_bytes values instead of recomputing a raw pointer from
static_account_keys.offset, and apply the same conversion fix at the other
affected offset usage.
🪄 Autofix (Beta)
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: 91d53ae7-b860-4caa-82c5-78aa9ab97f6e
⛔ 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 (78)
solana/account/Cargo.tomlsolana/account/src/lib.rssolana/account/src/state_traits.rssolana/program-runtime/Cargo.tomlsolana/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/stable_log.rssolana/program-runtime/src/sysvar_cache.rssolana/program-runtime/src/vm.rssolana/svm/Cargo.tomlsolana/svm/doc/diagrams/context.texsolana/svm/doc/spec.mdsolana/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/clock-sysvar/src/lib.rssolana/svm/tests/example-programs/hello-solana/Cargo.tomlsolana/svm/tests/example-programs/hello-solana/src/lib.rssolana/svm/tests/example-programs/simple-transfer/Cargo.tomlsolana/svm/tests/example-programs/simple-transfer/src/lib.rssolana/svm/tests/example-programs/transfer-from-account/Cargo.tomlsolana/svm/tests/example-programs/transfer-from-account/src/lib.rssolana/svm/tests/example-programs/write-to-account/Cargo.tomlsolana/svm/tests/example-programs/write-to-account/src/lib.rssolana/svm/tests/integration_test.rssolana/svm/tests/mock_bank.rssolana/transaction-context/Cargo.tomlsolana/transaction-context/src/instruction.rssolana/transaction-context/src/instruction_accounts.rssolana/transaction-context/src/lib.rssolana/transaction-context/src/transaction.rssolana/transaction-context/src/transaction_accounts.rssolana/transaction-context/src/vm_addresses.rssolana/transaction-context/src/vm_slice.rssolana/transaction-view/Cargo.tomlsolana/transaction-view/benches/bytes.rssolana/transaction-view/benches/transaction_view.rssolana/transaction-view/src/address_table_lookup_frame.rssolana/transaction-view/src/bytes.rssolana/transaction-view/src/instructions_frame.rssolana/transaction-view/src/lib.rssolana/transaction-view/src/message_header_frame.rssolana/transaction-view/src/resolved_transaction_view.rssolana/transaction-view/src/result.rssolana/transaction-view/src/sanitize.rssolana/transaction-view/src/signature_frame.rssolana/transaction-view/src/static_account_keys_frame.rssolana/transaction-view/src/transaction_config_frame.rssolana/transaction-view/src/transaction_data.rssolana/transaction-view/src/transaction_frame.rssolana/transaction-view/src/transaction_version.rssolana/transaction-view/src/transaction_view.rs
🚧 Files skipped from review as they are similar to previous changes (59)
- solana/svm/tests/example-programs/clock-sysvar/Cargo.toml
- solana/svm/tests/example-programs/clock-sysvar/src/lib.rs
- solana/svm/tests/example-programs/transfer-from-account/Cargo.toml
- solana/svm/tests/example-programs/simple-transfer/Cargo.toml
- solana/transaction-view/src/lib.rs
- solana/svm/tests/example-programs/hello-solana/Cargo.toml
- solana/svm/tests/example-programs/hello-solana/src/lib.rs
- solana/transaction-context/Cargo.toml
- solana/svm/src/transaction_error_metrics.rs
- solana/svm/src/lib.rs
- solana/svm/tests/example-programs/write-to-account/Cargo.toml
- solana/svm/tests/example-programs/write-to-account/src/lib.rs
- solana/program-runtime/src/lib.rs
- solana/svm/Cargo.toml
- solana/svm/src/account_overrides.rs
- solana/transaction-view/Cargo.toml
- solana/program-runtime/src/loading_task.rs
- solana/svm/src/transaction_processing_callback.rs
- solana/account/Cargo.toml
- solana/svm/src/transaction_commit_result.rs
- solana/svm/src/transaction_execution_result.rs
- solana/transaction-view/src/transaction_view.rs
- solana/transaction-view/benches/transaction_view.rs
- solana/program-runtime/src/execution_budget.rs
- solana/transaction-context/src/vm_addresses.rs
- solana/svm/src/transaction_account_state_info.rs
- solana/svm/tests/example-programs/simple-transfer/src/lib.rs
- solana/transaction-view/src/transaction_data.rs
- solana/program-runtime/Cargo.toml
- solana/svm/tests/example-programs/transfer-from-account/src/lib.rs
- solana/program-runtime/src/vm.rs
- solana/transaction-view/src/instructions_frame.rs
- solana/svm/tests/concurrent_tests.rs
- solana/program-runtime/src/loaded_programs.rs
- solana/transaction-context/src/instruction.rs
- solana/transaction-context/src/instruction_accounts.rs
- solana/svm/src/program_loader.rs
- solana/program-runtime/src/program_cache_entry.rs
- solana/svm/doc/diagrams/context.tex
- solana/program-runtime/src/cpi.rs
- solana/svm/tests/integration_test.rs
- solana/svm/src/transaction_processing_result.rs
- solana/svm/src/rent_calculator.rs
- solana/svm/src/rollback_accounts.rs
- solana/transaction-context/src/transaction.rs
- solana/transaction-context/src/transaction_accounts.rs
- solana/svm/src/message_processor.rs
- solana/program-runtime/src/invoke_context.rs
- solana/transaction-context/src/vm_slice.rs
- solana/program-runtime/src/serialization.rs
- solana/svm/src/transaction_processor.rs
- solana/program-runtime/src/deploy.rs
- solana/svm/src/transaction_balances.rs
- solana/program-runtime/src/mem_pool.rs
- solana/program-runtime/src/memory.rs
- solana/program-runtime/src/memory_context.rs
- solana/svm/tests/mock_bank.rs
- solana/program-runtime/src/sysvar_cache.rs
- solana/program-runtime/src/program_metrics.rs
Comment on lines
+13
to
+15
| // Create a vector of all valid u16 values serialized into 16-byte buffers. | ||
| let mut values = Vec::with_capacity(PACKET_DATA_SIZE); | ||
| for value in 0..PACKET_DATA_SIZE as u16 { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment does not match the loop range.
The comment states "all valid u16 values". The loop only covers 0..PACKET_DATA_SIZE as u16. Align the comment with the packet-sized dataset.
✏️ Proposed doc fix
- // Create a vector of all valid u16 values serialized into 16-byte buffers.
+ // Create a vector of packet-sized u16 values serialized into 16-byte buffers.
let mut values = Vec::with_capacity(PACKET_DATA_SIZE);As per path instructions, "Check docs and rustdoc for factual consistency with the code. Flag only real mismatches, broken examples, stale comments".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Create a vector of all valid u16 values serialized into 16-byte buffers. | |
| let mut values = Vec::with_capacity(PACKET_DATA_SIZE); | |
| for value in 0..PACKET_DATA_SIZE as u16 { | |
| // Create a vector of packet-sized u16 values serialized into 16-byte buffers. | |
| let mut values = Vec::with_capacity(PACKET_DATA_SIZE); | |
| for value in 0..PACKET_DATA_SIZE as u16 { |
🤖 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/transaction-view/benches/bytes.rs` around lines 13 - 15, Update the
comment above the values initialization in the benchmark to describe the
packet-sized range produced by the loop, rather than claiming it contains all
valid u16 values; leave the loop and serialization behavior unchanged.
Source: Path instructions
Comment on lines
+69
to
+85
| for i in 0..3 { | ||
| // Implicitly checks that the offset is within bounds, no need | ||
| // to call check_remaining explicitly here. | ||
| let byte = *bytes | ||
| .get(offset.wrapping_add(i)) | ||
| .ok_or(TransactionViewError::ParseError)?; | ||
| // non-minimal encoding or overflow | ||
| if (i > 0 && byte == 0) || (i == 2 && byte > 3) { | ||
| return Err(TransactionViewError::ParseError); | ||
| } | ||
| result |= ((byte & 0x7F) as u16) << shift; | ||
| shift += 7; | ||
| if byte & 0x80 == 0 { | ||
| *offset = index.checked_add(1).ok_or(TransactionViewError::ParseError)?; | ||
| return Ok(result); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm `index` has no binding in bytes.rs and locate all call sites of read_compressed_u16.
rg -n -C3 '\bindex\b' solana/transaction-view/src/bytes.rs
rg -n -C2 '\bread_compressed_u16\s*\(' solana/transaction-view/srcRepository: magicblock-labs/magicblock-engine
Length of output: 6515
🏁 Script executed:
#!/bin/bash
sed -n '65,90p' solana/transaction-view/src/bytes.rsRepository: magicblock-labs/magicblock-engine
Length of output: 1088
Replace the undefined index in read_compressed_u16.
bytes.rs declares i in the loop, but line 82 reads index, so name resolution fails and the crate will not compile. Use the consumed byte index, i.e. offset.wrapping_add(i).checked_add(1).
🤖 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/transaction-view/src/bytes.rs` around lines 69 - 85, In
read_compressed_u16, replace the undefined index reference when advancing the
offset with the consumed byte position derived from
offset.wrapping_add(i).checked_add(1), preserving the existing parse-error
handling on overflow.
Comment on lines
+264
to
+268
| use { | ||
| super::*, | ||
| bincode::{DefaultOptions, Options, serialize_into}, | ||
| solana_short_vec::ShortU16, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify solana-packet is available to the transaction-view crate (normal or dev dependency).
fd -a Cargo.toml solana/transaction-view --max-depth 2 --exec cat -n
rg -n 'PACKET_DATA_SIZE|solana_packet' solana/transaction-view/srcRepository: magicblock-labs/magicblock-engine
Length of output: 2953
Import PACKET_DATA_SIZE in the bytes test module.
The test module uses PACKET_DATA_SIZE but only imports super::*, bincode items, and ShortU16, so the test module has an unresolved name. Add solana_packet::PACKET_DATA_SIZE to the module use block.
🤖 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/transaction-view/src/bytes.rs` around lines 264 - 268, Add
solana_packet::PACKET_DATA_SIZE to the use block of the bytes test module so its
existing test references resolve without changing other imports or behavior.
Comment on lines
+94
to
+96
| /// Sigantures Constraint: | ||
| /// * Number of signatures must equal: num_required_signatures | ||
| /// * Max signatures <= 12 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo in doc comment.
"Sigantures" should be "Signatures".
✏️ Proposed doc fix
-/// Sigantures Constraint:
+/// Signatures Constraint:As per path instructions, "Typos in identifiers, comments, or user-facing strings".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Sigantures Constraint: | |
| /// * Number of signatures must equal: num_required_signatures | |
| /// * Max signatures <= 12 | |
| /// Signatures Constraint: | |
| /// * Number of signatures must equal: num_required_signatures | |
| /// * Max signatures <= 12 |
🤖 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/transaction-view/src/sanitize.rs` around lines 94 - 96, Correct the
typo in the documentation comment’s “Sigantures Constraint” heading to
“Signatures Constraint,” without changing the surrounding signature constraints.
Source: Path instructions
Comment on lines
+138
to
+152
| /// Instructions Constraints | ||
| /// * NumInstructions <= 64 | ||
| /// * Per instruction: | ||
| /// * 0 < program_id_index < MaxProgramIdIndex | ||
| /// * all account indices < MaxAccountIndex | ||
| fn sanitize_instructions( | ||
| view: &UnsanitizedTransactionView<impl TransactionData>, | ||
| enable_instruction_accounts_limit: bool, | ||
| ) -> Result<()> { | ||
| let instructions_limit = match view.version() { | ||
| TransactionVersion::Magicblock => MAGICBLOCK_INSTRUCTION_TRACE_LENGTH, | ||
| TransactionVersion::V0 | TransactionVersion::V1 | TransactionVersion::Legacy => { | ||
| solana_transaction_context::MAX_INSTRUCTION_TRACE_LENGTH | ||
| } | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Instruction-limit doc does not match the code.
The doc states NumInstructions <= 64. The code applies solana_transaction_context::MAX_INSTRUCTION_TRACE_LENGTH for Legacy/V0/V1 and MAGICBLOCK_INSTRUCTION_TRACE_LENGTH (255) for Magicblock. Update the doc to name the constants. The account-limit doc at lines 116-118 also omits the Magicblock limit of 256.
✏️ Proposed doc fix
/// Instructions Constraints
-/// * NumInstructions <= 64
+/// * legacy/v0/v1: NumInstructions <= `MAX_INSTRUCTION_TRACE_LENGTH`
+/// * magicblock: NumInstructions <= `MAGICBLOCK_INSTRUCTION_TRACE_LENGTH`
/// * Per instruction:
/// * 0 < program_id_index < MaxProgramIdIndex
/// * all account indices < MaxAccountIndexAs per path instructions, "Check docs and rustdoc for factual consistency with the code. Flag only real mismatches, broken examples, stale comments".
🤖 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/transaction-view/src/sanitize.rs` around lines 138 - 152, Update the
rustdoc for sanitize_instructions to replace the hard-coded NumInstructions <=
64 statement with the actual version-specific constants:
MAGICBLOCK_INSTRUCTION_TRACE_LENGTH for Magicblock and
solana_transaction_context::MAX_INSTRUCTION_TRACE_LENGTH for Legacy, V0, and V1.
Also update the account-limit documentation to include Magicblock’s 256 limit
while preserving the existing limits for other transaction versions.
Source: Path instructions
Comment on lines
+64
to
+86
| pub(crate) fn try_new( | ||
| bytes: &[u8], | ||
| mask_offset: usize, | ||
| mask: u32, | ||
| offset: &mut usize, | ||
| ) -> Result<Self> { | ||
| assert!(mask_offset > 0, "txv1 mask offset must be greater than 0"); | ||
|
|
||
| Self::sanitize_mask(mask)?; | ||
| let num_values = mask.count_ones() as u8; | ||
| let mask_offset = try_u32_offset(mask_offset)?; | ||
| let values_offset = try_u32_offset(*offset)?; | ||
|
|
||
| // advance offset | ||
| advance_offset_for_array::<u32>(bytes, offset, num_values as u16)?; | ||
|
|
||
| Ok(Self { | ||
| mask_offset, | ||
| mask, | ||
| values_offset, | ||
| num_values, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all call sites of TransactionConfigFrame::try_new
rg -n -C5 'TransactionConfigFrame::try_new' --type=rustRepository: magicblock-labs/magicblock-engine
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked Rust files with relevant filenames:"
git ls-files | grep -E '(^|/)transaction_config_frame\.rs$|(^|/)transaction_frame\.rs$'
echo
echo "Find try_new definitions/usages (case/regex variants):"
rg -n -C5 'try_new|TransactionConfigFrame|txv1|mask_offset' --type=rust || true
echo
echo "Inspect target file if present:"
if [ -f solana/transaction-view/src/transaction_config_frame.rs ]; then
cat -n solana/transaction-view/src/transaction_config_frame.rs
fiRepository: magicblock-labs/magicblock-engine
Length of output: 376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "solana/transaction-view/src/transaction_config_frame.rs:"
cat -n solana/transaction-view/src/transaction_config_frame.rs
echo
echo "solana/transaction-view/src/transaction_frame.rs relevant lines:"
wc -l solana/transaction-view/src/transaction_frame.rs
sed -n '1,180p' solana/transaction-view/src/transaction_frame.rs | cat -n
echo
echo "Text search for relevant symbols:"
rg -n -C4 'TransactionConfigFrame|config_frame|mask_offset|mask_offset|try_new|sanitize_mask|values_offset|txv1' solana/transaction-view src solana 2>/dev/null || true
echo
echo "Search all files for assert mask_offset:"
rg -n -C4 'assert!.*mask_offset|mask_offset > 0|mask_offset == 0' . 2>/dev/null || trueRepository: magicblock-labs/magicblock-engine
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "transaction_frame.rs size:"
wc -l solana/transaction-view/src/transaction_frame.rs
echo
echo "transaction_frame.rs around config parsing:"
rg -n -C 12 'TransactionConfigFrame::try_new|transaction_config_frame|mask_offset|Configuration' solana/transaction-view/src/transaction_frame.rs
echo
echo "transaction_config_frame.rs first 120 lines:"
sed -n '60,105p' solana/transaction-view/src/transaction_config_frame.rs | cat -n
echo
echo "Result/error definitions:"
rg -n -C 4 'enum TransactionViewError|Result::Err|ParseError|Sananitize|Sanit|Sanitized|SanitizeError' --type=rustRepository: magicblock-labs/magicblock-engine
Length of output: 9606
Replace the reserved mask_offset == 0 panic with a checked parse error.
TransactionConfigFrame::try_new accepts raw transaction bytes, and current tx-v1 parsing starts at offset 4. Keep future calls unreachable only by invariants, not by panic. If mask_offset is 0 for a frame expected to be tx-v1, return TransactionViewError instead of failing through assert!.
🤖 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/transaction-view/src/transaction_config_frame.rs` around lines 64 -
86, Update TransactionConfigFrame::try_new to replace the mask_offset > 0 assert
with a checked TransactionViewError return when mask_offset is zero. Preserve
normal tx-v1 parsing for positive offsets and keep the existing mask validation,
offset conversion, and array advancement flow unchanged.
Source: Path instructions
Comment on lines
+348
to
+362
| let start = self.static_account_keys.offset as usize; | ||
| let end = start | ||
| + usize::from(self.static_account_keys.num_static_accounts) | ||
| * core::mem::size_of::<Pubkey>(); | ||
| let account_bytes = &bytes[start..end]; | ||
| unsafe { | ||
| core::slice::from_raw_parts( | ||
| bytes | ||
| .as_ptr() | ||
| .add(usize::from(self.static_account_keys.offset)) | ||
| as *const Pubkey, | ||
| usize::from(self.static_account_keys.num_static_accounts), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every usize::from call applied to a u32 offset field in the transaction-view crate.
rg -n -C2 'usize::from\(' solana/transaction-view/src
rg -n -C2 'offset: u32|_offset: u32' solana/transaction-view/srcRepository: magicblock-labs/magicblock-engine
Length of output: 21150
🌐 Web query:
Does Rust std implement From<u32> for usize?
💡 Result:
No, the Rust standard library does not implement From for usize [1][2]. The From trait is intended for infallible, cost-free conversions [1]. Because usize is platform-dependent—its size can be 16, 32, or 64 bits—a conversion from u32 to usize is not guaranteed to be infallible on all architectures [1][2]. On 16-bit platforms, where usize is only 16 bits wide, a u32 value may exceed the maximum value representable by usize, making the conversion fallible [1][2]. To perform this conversion safely, you should use the TryFrom trait instead, which is implemented for these types and handles the possibility of failure [1][2].
Citations:
- 1: https://users.rust-lang.org/t/why-is-there-no-impl-from-u32-for-usize/88059/1
- 2: https://users.rust-lang.org/t/why-is-there-no-impl-from-u32-for-usize/88059
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rustc --edition=2021 - <<'EOF' 2>&1
pub fn main() {
let x: u32 = 42;
let _: usize = usize::from(x);
}
EOFRepository: magicblock-labs/magicblock-engine
Length of output: 1439
Use a valid usize conversion for these u32 offsets.
usize::from does not implement From<u32>, so line 357 and line 382 fail to compile. Replace those calls with an explicit as usize conversion. In static_account_keys, reuse the already computed start/account_bytes slice rather than recomputing the pointer from the raw offset.
🤖 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/transaction-view/src/transaction_frame.rs` around lines 348 - 362,
Replace the invalid usize::from conversions for the u32 offsets in the
static_account_keys path with explicit as usize casts. Update
static_account_keys to construct the Pubkey slice from the existing
start/account_bytes values instead of recomputing a raw pointer from
static_account_keys.offset, and apply the same conversion fix at the other
affected offset usage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Imported the upstream Solana account, transaction-context, program-runtime, and
SVM crates, including their tests and SBF fixtures.
Why
The engine-specific runtime fork needs a distinct upstream baseline so each
customization can be reviewed as an immediate-parent diff instead of as part of
one large import.
Closes #32.
Impact
behavior at this stack boundary.
engine-specific storage and execution policy.
Reviewer notes
Treat this as a mechanical baseline import. Engine-specific divergences belong
in the dedicated descendant PRs.
Follow-up
The next four PRs customize account storage, transaction context, program
runtime, and SVM behavior independently.