feat: fail intent if it will hit MaxInstructionTraceLengthExceeded on Solana#1162
feat: fail intent if it will hit MaxInstructionTraceLengthExceeded on Solana#1162
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThis change introduces CPI-budget validation to the magic intent bundle processing. A new constant Suggested reviewers
✨ Finishing Touches🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
254-309: 🧹 Nitpick | 🔵 TrivialMove CPI-budget validation before fee charging.
validate_cpi_budgetruns aftercharge_delegated_payer,fetch_current_commit_nonces,check_commit_limits, and the committee loop (includingmark_account_as_undelegatedmutations). While all of these are rolled back when the instruction errors, they do unnecessary work for an intent that will always be rejected. Since the CPI-budget check only needscommitted_accounts.len()and the intent shape (determined byopts.request_undelegation), you can validate very early — ideally right after the committee loop buildscommitted_accounts— to fail fast and avoid touching fee vaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/magicblock/src/schedule_transactions/process_schedule_commit.rs` around lines 254 - 309, The CPI-budget validation (base_intent.validate_cpi_budget) must run before doing fee operations and nonce/limit checks so we fail fast; move the call to validate_cpi_budget right after the committee loop finishes constructing committed_accounts (use committed_accounts.len() and opts.request_undelegation to build the same MagicBaseIntent/ MagicIntentBundle shape) and only proceed to fetch_current_commit_nonces, check_commit_limits, charge_delegated_payer, and mark_account_as_undelegated if the validation succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@programs/magicblock/src/magic_scheduled_base_intent.rs`:
- Around line 43-46: The constant CPI_LIMIT is named and documented as "Number
of CPIS limited to 64" but Solana's real bound is MAX_INSTRUCTION_TRACE_LENGTH
(total instructions including top-level + CPIs) and the code's comparisons treat
64 as a hard reject threshold; update to avoid off-by-one/semantic mismatch by
either renaming CPI_LIMIT to MAX_INSTRUCTION_TRACE_LENGTH and updating its
doc-comment to state it bounds total instruction trace length (so places using
CPI_LIMIT and BASE_STAGE_CPIS keep the same numeric value but correct meaning),
or if you want to preserve the "max CPIs" intent keep the symbol CPI_LIMIT and
change its value to 63 to maintain a one-instruction safety margin; adjust any
uses of CPI_LIMIT/BASE_STAGE_CPIS accordingly so checks remain consistent with
the chosen interpretation.
- Around line 375-391: The current validate_cpi_budget function logs a generic
"too many committed accounts" message without indicating which stage overflowed
or the CPI counts; update the ic_msg! call inside validate_cpi_budget to include
which stage exceeded CPI_LIMIT (commit or finalize), the offending stage's CPI
value (from commit_stage_cpis() or finalize_stage_cpis()), the other stage's CPI
value, and the CPI_LIMIT constant so operators can see "commit_stage_cpis=X,
finalize_stage_cpis=Y, CPI_LIMIT=Z" and which stage triggered the error; keep
returning InstructionError::MaxAccountsExceeded and use the existing
invoke_context/ic_msg! plumbing.
In `@test-integration/schedulecommit/test-scenarios/tests/01_commits.rs`:
- Around line 302-366: The helper function
assert_schedule_commit_cpi_budget_exceeded is duplicated between 01_commits.rs
and 02_commit_and_undelegate.rs; extract it into a shared test utils module
(e.g., tests/utils/mod.rs) and import it from both test files. Move the function
body (including get_context_with_delegated_committees usage,
ScheduleCommitTestContextFields destructuring, ComputeBudgetInstruction setup,
schedule_commit_cpi_instruction call, transaction creation/sending, and the
final assertions) into a single pub fn
assert_schedule_commit_cpi_budget_exceeded(...) in the utils module, update both
test files to use use
crate::tests::utils::assert_schedule_commit_cpi_budget_exceeded (or the correct
path) and remove the duplicated copies so tests compile and run against the
single shared helper.
---
Outside diff comments:
In `@programs/magicblock/src/schedule_transactions/process_schedule_commit.rs`:
- Around line 254-309: The CPI-budget validation
(base_intent.validate_cpi_budget) must run before doing fee operations and
nonce/limit checks so we fail fast; move the call to validate_cpi_budget right
after the committee loop finishes constructing committed_accounts (use
committed_accounts.len() and opts.request_undelegation to build the same
MagicBaseIntent/ MagicIntentBundle shape) and only proceed to
fetch_current_commit_nonces, check_commit_limits, charge_delegated_payer, and
mark_account_as_undelegated if the validation succeeds.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: cfa4ebef-dbbf-4142-ba78-4862910a736c
📒 Files selected for processing (6)
programs/magicblock/src/magic_scheduled_base_intent.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rstest-integration/schedulecommit/client/src/schedule_commit_context.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rstest-integration/schedulecommit/test-scenarios/tests/utils/mod.rs
| /// Number of CPIS limited to 64 on Solana | ||
| pub const CPI_LIMIT: usize = 64; | ||
| /// Fixed base CPIs per tx stage (SetComputeUnitLimit + SetComputeUnitPrice). | ||
| const BASE_STAGE_CPIS: usize = 2; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
CPI_LIMIT doc and threshold semantics.
Solana's actual constant is MAX_INSTRUCTION_TRACE_LENGTH = 64, which bounds total instructions in the trace (top-level + all CPIs), not strictly CPIs. With stepped formulas (2 + 3·n, 2 + 6·n, 2 + 5·n) the estimate never lands on exactly 64, so >= vs > is indistinguishable in practice today — but the naming and doc-comment suggest "max CPIs = 64" while the check semantics are "reject at 64 or more instructions", and a future helper that produces an estimate of exactly 64 would be incorrectly rejected.
Recommend either:
- Rename to
MAX_INSTRUCTION_TRACE_LENGTHand update the comment to match Solana's terminology, or - Keep the name but set it to
63(one less than Solana's hard limit) to preserve a safety margin against the inherent estimate imprecision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/magicblock/src/magic_scheduled_base_intent.rs` around lines 43 - 46,
The constant CPI_LIMIT is named and documented as "Number of CPIS limited to 64"
but Solana's real bound is MAX_INSTRUCTION_TRACE_LENGTH (total instructions
including top-level + CPIs) and the code's comparisons treat 64 as a hard reject
threshold; update to avoid off-by-one/semantic mismatch by either renaming
CPI_LIMIT to MAX_INSTRUCTION_TRACE_LENGTH and updating its doc-comment to state
it bounds total instruction trace length (so places using CPI_LIMIT and
BASE_STAGE_CPIS keep the same numeric value but correct meaning), or if you want
to preserve the "max CPIs" intent keep the symbol CPI_LIMIT and change its value
to 63 to maintain a one-instruction safety margin; adjust any uses of
CPI_LIMIT/BASE_STAGE_CPIS accordingly so checks remain consistent with the
chosen interpretation.
| pub fn validate_cpi_budget( | ||
| &self, | ||
| invoke_context: &&mut InvokeContext<'_>, | ||
| ) -> Result<(), InstructionError> { | ||
| let (commit_stage_cpis, finalize_stage_cpis) = | ||
| (self.commit_stage_cpis(), self.finalize_stage_cpis()); | ||
| if commit_stage_cpis >= CPI_LIMIT || finalize_stage_cpis >= CPI_LIMIT { | ||
| ic_msg!( | ||
| invoke_context, | ||
| "ScheduleCommit ERR: too many committed accounts.", | ||
| ); | ||
|
|
||
| Err(InstructionError::MaxAccountsExceeded) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Diagnostic: include which stage overflowed and by how much.
The log currently only says "too many committed accounts"; it doesn't tell operators whether the commit stage or the finalize stage tripped, nor the estimated CPI count vs CPI_LIMIT. When users hit this they'll need to figure out whether to split their intent or switch commit types — surfacing the numbers is cheap and very helpful.
✏️ Proposed change
pub fn validate_cpi_budget(
&self,
invoke_context: &&mut InvokeContext<'_>,
) -> Result<(), InstructionError> {
let (commit_stage_cpis, finalize_stage_cpis) =
(self.commit_stage_cpis(), self.finalize_stage_cpis());
- if commit_stage_cpis >= CPI_LIMIT || finalize_stage_cpis >= CPI_LIMIT {
+ if commit_stage_cpis >= CPI_LIMIT {
ic_msg!(
invoke_context,
- "ScheduleCommit ERR: too many committed accounts.",
+ "ScheduleCommit ERR: commit-stage CPI estimate {} >= limit {}",
+ commit_stage_cpis, CPI_LIMIT,
);
-
- Err(InstructionError::MaxAccountsExceeded)
- } else {
- Ok(())
+ return Err(InstructionError::MaxAccountsExceeded);
}
+ if finalize_stage_cpis >= CPI_LIMIT {
+ ic_msg!(
+ invoke_context,
+ "ScheduleCommit ERR: finalize-stage CPI estimate {} >= limit {}",
+ finalize_stage_cpis, CPI_LIMIT,
+ );
+ return Err(InstructionError::MaxAccountsExceeded);
+ }
+ Ok(())
}📝 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.
| pub fn validate_cpi_budget( | |
| &self, | |
| invoke_context: &&mut InvokeContext<'_>, | |
| ) -> Result<(), InstructionError> { | |
| let (commit_stage_cpis, finalize_stage_cpis) = | |
| (self.commit_stage_cpis(), self.finalize_stage_cpis()); | |
| if commit_stage_cpis >= CPI_LIMIT || finalize_stage_cpis >= CPI_LIMIT { | |
| ic_msg!( | |
| invoke_context, | |
| "ScheduleCommit ERR: too many committed accounts.", | |
| ); | |
| Err(InstructionError::MaxAccountsExceeded) | |
| } else { | |
| Ok(()) | |
| } | |
| } | |
| pub fn validate_cpi_budget( | |
| &self, | |
| invoke_context: &&mut InvokeContext<'_>, | |
| ) -> Result<(), InstructionError> { | |
| let (commit_stage_cpis, finalize_stage_cpis) = | |
| (self.commit_stage_cpis(), self.finalize_stage_cpis()); | |
| if commit_stage_cpis >= CPI_LIMIT { | |
| ic_msg!( | |
| invoke_context, | |
| "ScheduleCommit ERR: commit-stage CPI estimate {} >= limit {}", | |
| commit_stage_cpis, CPI_LIMIT, | |
| ); | |
| return Err(InstructionError::MaxAccountsExceeded); | |
| } | |
| if finalize_stage_cpis >= CPI_LIMIT { | |
| ic_msg!( | |
| invoke_context, | |
| "ScheduleCommit ERR: finalize-stage CPI estimate {} >= limit {}", | |
| finalize_stage_cpis, CPI_LIMIT, | |
| ); | |
| return Err(InstructionError::MaxAccountsExceeded); | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@programs/magicblock/src/magic_scheduled_base_intent.rs` around lines 375 -
391, The current validate_cpi_budget function logs a generic "too many committed
accounts" message without indicating which stage overflowed or the CPI counts;
update the ic_msg! call inside validate_cpi_budget to include which stage
exceeded CPI_LIMIT (commit or finalize), the offending stage's CPI value (from
commit_stage_cpis() or finalize_stage_cpis()), the other stage's CPI value, and
the CPI_LIMIT constant so operators can see "commit_stage_cpis=X,
finalize_stage_cpis=Y, CPI_LIMIT=Z" and which stage triggered the error; keep
returning InstructionError::MaxAccountsExceeded and use the existing
invoke_context/ic_msg! plumbing.
| pub fn init_committees(&self) -> Result<Vec<Signature>> { | ||
| const CHUNK_SIZE: usize = 5; | ||
| let chain_client = self.try_chain_client()?; | ||
|
|
||
| self.committees | ||
| .chunks(CHUNK_SIZE) | ||
| .map(|chunk| { | ||
| let ixs = self.init_committee_ixs(chunk); | ||
| let mut signers = | ||
| chunk.iter().map(|(p, _)| p).collect::<Vec<_>>(); | ||
| signers.push(&self.payer_chain); | ||
| (ixs, signers) | ||
| }) | ||
| .map(|(ixs, signers)| -> Result<Signature> { | ||
| let tx = Transaction::new_signed_with_payer( | ||
| &ixs, | ||
| Some(&self.payer_chain.pubkey()), | ||
| &signers, | ||
| self.try_chain_blockhash()?, | ||
| ); | ||
| chain_client | ||
| .send_and_confirm_transaction_with_spinner_and_config( | ||
| &tx, | ||
| self.commitment, | ||
| RpcSendTransactionConfig { | ||
| skip_preflight: true, | ||
| ..Default::default() | ||
| }, | ||
| ) | ||
| .with_context(|| { | ||
| format!( | ||
| "Failed to initialize committees. Transaction signature: {}", | ||
| tx.get_signature() | ||
| ) | ||
| }) | ||
| }) | ||
| .collect::<Result<Vec<_>>>() | ||
| } |
There was a problem hiding this comment.
Partial-failure semantics for chunked init/delegate.
init_committees now sends multiple transactions. If chunk N+1 fails, chunks 1..N are already committed on-chain and the returned Err provides no signal about what succeeded, leaving tests in a partially-initialized state. The same applies to delegate_committees (Lines 258-301). If this is acceptable for test harness usage, consider a comment documenting the non-atomic behavior; otherwise return the accumulated signatures alongside the error.
| // ----------------- | ||
| // CPI budget tests | ||
| // ----------------- | ||
|
|
||
| // Commit: commit_stage = 2 + 3*n; overflows CPI_LIMIT(64) at n=21 | ||
| // CommitFinalize: commit_stage = 2 + 1*n; overflows CPI_LIMIT(64) at n=62 | ||
|
|
||
| fn assert_schedule_commit_cpi_budget_exceeded( | ||
| committees: usize, | ||
| commit_type: ScheduleCommitType, | ||
| ) { | ||
| let ctx = get_context_with_delegated_committees( | ||
| committees, | ||
| UserSeeds::MagicScheduleCommit, | ||
| ); | ||
| let ScheduleCommitTestContextFields { | ||
| payer_chain: payer, | ||
| committees, | ||
| commitment, | ||
| ephem_client, | ||
| .. | ||
| } = ctx.fields(); | ||
|
|
||
| let mut ixs = vec![ | ||
| ComputeBudgetInstruction::set_compute_unit_limit(1_000_000), | ||
| ComputeBudgetInstruction::set_compute_unit_price(1_000_000), | ||
| ]; | ||
| let ix = schedule_commit_cpi_instruction( | ||
| payer.pubkey(), | ||
| magicblock_magic_program_api::id(), | ||
| magicblock_magic_program_api::MAGIC_CONTEXT_PUBKEY, | ||
| None, | ||
| &committees | ||
| .iter() | ||
| .map(|(p, _)| p.pubkey()) | ||
| .collect::<Vec<_>>(), | ||
| &committees.iter().map(|(_, pda)| *pda).collect::<Vec<_>>(), | ||
| commit_type, | ||
| ); | ||
| ixs.push(ix); | ||
|
|
||
| let blockhash = ephem_client.get_latest_blockhash().unwrap(); | ||
| let tx = Transaction::new_signed_with_payer( | ||
| &ixs, | ||
| Some(&payer.pubkey()), | ||
| &[&payer], | ||
| blockhash, | ||
| ); | ||
| let res = ephem_client | ||
| .send_and_confirm_transaction_with_spinner_and_config( | ||
| &tx, | ||
| *commitment, | ||
| RpcSendTransactionConfig { | ||
| skip_preflight: true, | ||
| ..Default::default() | ||
| }, | ||
| ); | ||
|
|
||
| let (tx_result_err, tx_err) = extract_transaction_error(res); | ||
| assert_is_instruction_error( | ||
| tx_err.unwrap(), | ||
| &tx_result_err, | ||
| InstructionError::MaxAccountsExceeded, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicated helper across test files.
assert_schedule_commit_cpi_budget_exceeded is defined identically here and in test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (Lines 897-954). Consider extracting it into tests/utils/mod.rs to avoid drift as more commit-type variants are added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-integration/schedulecommit/test-scenarios/tests/01_commits.rs` around
lines 302 - 366, The helper function assert_schedule_commit_cpi_budget_exceeded
is duplicated between 01_commits.rs and 02_commit_and_undelegate.rs; extract it
into a shared test utils module (e.g., tests/utils/mod.rs) and import it from
both test files. Move the function body (including
get_context_with_delegated_committees usage, ScheduleCommitTestContextFields
destructuring, ComputeBudgetInstruction setup, schedule_commit_cpi_instruction
call, transaction creation/sending, and the final assertions) into a single pub
fn assert_schedule_commit_cpi_budget_exceeded(...) in the utils module, update
both test files to use use
crate::tests::utils::assert_schedule_commit_cpi_budget_exceeded (or the correct
path) and remove the duplicated copies so tests compile and run against the
single shared helper.
1 participant
Summary
Right now user can schedule Intent with as many account as IX can contain. This can lead to situation where even Two stage commit flow will exceed Solana's limit of 64 CPIs.
This PR will reject such intents on scheduling level and make error explicit rather than the silent Intent failure.
Compatibility
Testing
Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests