fix: refuse impossible sized intents#1390
Conversation
|
Warning Review limit reached
Next review available in: 48 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: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughChangesThis PR adds an intent size feasibility check enforced at the MagicSys trait level: Sequence Diagram(s)sequenceDiagram
participant Client
participant MagicSysAdapter
participant IntentSizeValidator
participant TaskBuilder
Client->>MagicSysAdapter: validate_intent_size(intent)
MagicSysAdapter->>IntentSizeValidator: fits(intent)
IntentSizeValidator->>TaskBuilder: build minimal commit/finalize tasks
IntentSizeValidator->>IntentSizeValidator: assemble transaction, check wire size
IntentSizeValidator-->>MagicSysAdapter: true/false
MagicSysAdapter-->>Client: Ok(()) or InstructionError::Custom(INTENT_TOO_LARGE_ERR)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
254-267: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInconsistent log level for the same unrecoverable error class.
LoadedAccountsDataSizeExceeded/CpiLimitErrorare logged aterror!inpatch_commit_strategy(Line 260) but onlywarn!inpatch_finalize_strategy(Line 505), even though both are documented as "Can't be handled." This asymmetry could cause finalize-stage failures of equal severity to be under-reported in alerting/dashboards.♻️ Suggested alignment
TransactionStrategyExecutionError::CpiLimitError(_, _) | TransactionStrategyExecutionError::LoadedAccountsDataSizeExceeded(_, _) => { // Can't be handled - warn!(error = ?err, "Finalization tasks exceeded execution limit"); + error!(error = ?err, "Finalization tasks exceeded execution limit"); Ok(ControlFlow::Break(())) }Also applies to: 502-512
🤖 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 `@magicblock-committor-service/src/intent_executor/two_stage_executor.rs` around lines 254 - 267, The same unrecoverable execution-limit errors are logged inconsistently between patch_commit_strategy and patch_finalize_strategy, so align the finalize-path logging with the commit-path severity. Update the handling in patch_finalize_strategy for TransactionStrategyExecutionError::CpiLimitError and TransactionStrategyExecutionError::LoadedAccountsDataSizeExceeded to use the same error-level logging and message style as the existing error! calls in patch_commit_strategy, keeping the strategy/error context consistent.
🤖 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 `@magicblock-committor-service/src/tasks/commit_stage_task.rs`:
- Around line 119-140: The `init_instruction` method contains a bare
`borsh::object_length(&self.chunks).unwrap()` in a production path, which must
not be left as-is. Replace the `.unwrap()` with proper error handling that
propagates or maps the failure from `object_length`, or, if this is a true
invariant, use an explicit `.expect("<clear invariant justification>")` so the
assumption is documented in `CommitStageTask::init_instruction` alongside the
existing safety note.
In `@magicblock-committor-service/src/tasks/utils.rs`:
- Around line 61-114: Both create_commit_task and create_commit_finalize_task
duplicate the same threshold-based delivery selection logic for CommitDelivery,
which risks divergence later. Extract the shared decision into a helper (for
example, a small function returning CommitDelivery) and reuse it from both
create_commit_task and create_commit_finalize_task, keeping the existing
CommittedAccount and base_account handling unchanged.
In `@magicblock-core/src/intent/schedule.rs`:
- Around line 64-281: `MagicIntentBundle` is missing the finalize variants in
its fee, callback, and action lookup aggregation logic. Update `calculate_fee`,
`has_callbacks`, and `get_action_mut` to also traverse `commit_finalize` and
`commit_finalize_and_undelegate` alongside `commit` and `commit_and_undelegate`,
using the existing helper methods on `MagicIntentBundle` and the underlying
intent types. Keep the same offset-based indexing in `get_action_mut` so
`BaseAction` resolution remains correct after including the finalize intents.
In `@programs/magicblock/src/magic_sys.rs`:
- Around line 48-58: The `validate_intent_size` function currently uses
`.expect(MAGIC_SYS_POISONED_MSG)` on the `MAGIC_SYS` read lock, which is not
allowed in production code under `programs/**`. Replace this with explicit error
handling that returns an `InstructionError` (or another appropriate domain
error) when the lock is poisoned or, if you believe the panic is truly
unreachable, add a clear invariant-based justification near the `MAGIC_SYS`
access. Keep the rest of the `MAGIC_SYS.read().as_ref().ok_or(...)?` flow intact
and ensure the function still delegates to `validate_intent_size(intent)` on
success.
In `@test-integration/programs/schedulecommit/src/lib.rs`:
- Around line 208-213: The match on ScheduleCommitType is no longer exhaustive
because it only handles CommitFinalize and CommitFinalizeAndUndelegate. Update
the commit_type match in the schedulecommit test flow to also cover Commit and
CommitAndUndelegate, or replace it with a wildcard if appropriate, so all
variants of ScheduleCommitType are handled. Use the ScheduleCommitType enum and
the commit_type match site in the test scenario as the key locations to update.
In
`@test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs`:
- Around line 381-388: The match on ScheduleCommitType is non-exhaustive and
must cover every variant to compile. Update the match in the test around
commit_type handling to include all ScheduleCommitType variants, using the
existing assertions for CommitFinalize and CommitFinalizeAndUndelegate and
adding the missing arm(s) with the appropriate behavior so the pattern match is
exhaustive.
---
Outside diff comments:
In `@magicblock-committor-service/src/intent_executor/two_stage_executor.rs`:
- Around line 254-267: The same unrecoverable execution-limit errors are logged
inconsistently between patch_commit_strategy and patch_finalize_strategy, so
align the finalize-path logging with the commit-path severity. Update the
handling in patch_finalize_strategy for
TransactionStrategyExecutionError::CpiLimitError and
TransactionStrategyExecutionError::LoadedAccountsDataSizeExceeded to use the
same error-level logging and message style as the existing error! calls in
patch_commit_strategy, keeping the strategy/error context consistent.
🪄 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: d8ef8270-e304-4d56-a29a-2749256db44f
📒 Files selected for processing (35)
magicblock-api/src/magic_sys_adapter.rsmagicblock-committor-service/src/intent_executor/two_stage_executor.rsmagicblock-committor-service/src/tasks/commit_finalize_task.rsmagicblock-committor-service/src/tasks/commit_stage_task.rsmagicblock-committor-service/src/tasks/commit_task.rsmagicblock-committor-service/src/tasks/intent_size_validator.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rsmagicblock-committor-service/src/transaction_preparator/mod.rsmagicblock-core/Cargo.tomlmagicblock-core/src/intent/mod.rsmagicblock-core/src/intent/schedule.rsmagicblock-core/src/traits.rsmagicblock-metrics/src/metrics/mod.rsprograms/magicblock/src/magic_scheduled_base_intent.rsprograms/magicblock/src/magic_sys.rsprograms/magicblock/src/schedule_transactions/process_schedule_intent_bundle.rsprograms/magicblock/src/test_utils/mod.rstest-integration/programs/schedulecommit-security/src/lib.rstest-integration/programs/schedulecommit/src/api.rstest-integration/programs/schedulecommit/src/lib.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/03_commit_limit.rstest-integration/schedulecommit/test-scenarios/tests/04_intent_size_limit.rstest-integration/schedulecommit/test-scenarios/tests/utils/mod.rstest-integration/schedulecommit/test-security/tests/01_invocations.rstest-integration/test-committor-service/tests/common.rstest-integration/test-committor-service/tests/test_delivery_preparator.rstest-integration/test-committor-service/tests/test_transaction_preparator.rstest-integration/test-task-scheduler/tests/test_schedule_magic_cpi_crank.rs
feat: add check into process_shcedule_commit
taco-paco
changed the title
taco-paco
changed the base branch from
master
to
refactor/committor/committor-program-removal
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
I found two mismatches between the validator estimate and the real committor execution shape.
One can still let oversized standalone-action intents through, and the other can reject valid commit intents with post-commit actions.
| let lookup_tables = | ||
| TransactionUtils::dummy_lookup_table(&lookup_table_keys); | ||
|
|
||
| TransactionUtils::assemble_tasks_tx( |
There was a problem hiding this comment.
This estimate misses the extra noop instruction that real standalone-action execution adds.
For action-only intents, IntentExecutor sets strategy.standalone_action_nonce = Some(intent_bundle.id), and assemble_tasks_tx_with_standalone_action_nonce then appends standalone_action_noop_instruction.
Since this path validates with assemble_tasks_tx and collects lookup-table keys without that nonce instruction, an action-only intent near the wire-size boundary can pass scheduling and still fail during committor preparation/execution.
That defeats the up-front refusal goal for this class of oversized intents.
Please size the standalone-action case with the same nonce instruction, or conservatively include its accounts and bytes in the estimate.
| .iter() | ||
| .map(Self::commit_task) | ||
| .collect(); | ||
| if let CommitType::WithBaseActions { base_actions, .. } = commit_type { |
There was a problem hiding this comment.
This double-counts post-commit base actions for Commit and CommitAndUndelegate.
The real TaskBuilderImpl commit-stage builders for those two cases only emit commit tasks, while finalize_tasks adds the WithBaseActions actions alongside the finalize tasks.
The size validator adds those actions here in the commit-stage estimate and then adds them again in finalize_tasks, so valid intents with post-commit actions can be rejected as too large even though the actual two-stage execution would fit. Please mirror the builder's stage placement and only include these actions in the finalize-stage estimate for Commit and CommitAndUndelegate.
Q: Do we have an example of one of these transactions/intents? |
There's a test added, now we reject such intent but before we would let it be scheduled |
That makes sense, but I’m asking whether we have an example of a real transaction we observed that couldn’t fit the current executor model. A synthetic one is easy to produce (for example with action data larger than the max transaction size, as in the tests). But that is only impossible with the current executor model and could be possible through a buffer. So I’d like to understand what we are seeing in prod. It makes sense to fail early anyway, and overall the PR LGTM assuming comments are addressed. |
3 participants
Summary
Refuse impossible intents. Some intents can't be executed even with ALTS and splitted into 2 txs. We reject this during schedyling.
NOTE: during calculation we assume the best case for ALTs. The real number of alts that will be used - unknown during tx execution.
Best case calculated as follow -
pubkeys.div_ceil(256), where 256 capacity of ALT.Proposal: we should probably create fresh alts for each case as otherwise tx could fail as too many ALTs is used within it, much more than truly reuired and we can't predict that case.
Breaking Changes
Test Plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests