feat: use SetLoadedAccountsDataSizeLimit for tx containing DLP ix #800
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. |
📝 WalkthroughWalkthroughThe PR updates the magicblock-delegation-program git revision in Cargo.toml files and adds accounts data size budget tracking to task handling. A new Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
🧰 Additional context used📓 Path-based instructions (2){magicblock-*,programs,storage-proto}/**⚙️ CodeRabbit configuration file
Files:
{test-*,tools}/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (12)📓 Common learnings📚 Learning: 2025-11-24T14:21:00.996ZApplied to files:
📚 Learning: 2025-10-26T16:54:39.084ZApplied to files:
📚 Learning: 2025-11-20T08:57:07.217ZApplied to files:
📚 Learning: 2025-11-21T11:03:26.756ZApplied to files:
📚 Learning: 2025-11-07T18:19:48.996ZApplied to files:
📚 Learning: 2025-11-21T10:22:07.520ZApplied to files:
📚 Learning: 2025-12-03T09:33:48.707ZApplied to files:
📚 Learning: 2025-11-07T13:09:52.253ZApplied to files:
📚 Learning: 2025-11-19T09:34:37.917ZApplied to files:
📚 Learning: 2025-10-14T09:56:14.047ZApplied to files:
📚 Learning: 2025-12-03T09:36:01.527ZApplied to files:
🧬 Code graph analysis (1)test-integration/test-committor-service/tests/test_intent_executor.rs (1)
🔇 Additional comments (14)
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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rs
🧰 Additional context used
📓 Path-based instructions (1)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/task_strategist.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/task_strategist.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/utils.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.toml
🧬 Code graph analysis (5)
magicblock-committor-service/src/tasks/mod.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-201)magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)
magicblock-committor-service/src/tasks/args_task.rs (2)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(88-88)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-201)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(88-88)
magicblock-committor-service/src/tasks/utils.rs (4)
magicblock-committor-service/src/tasks/args_task.rs (2)
compute_units(175-183)accounts_size_budget(185-201)magicblock-committor-service/src/tasks/buffer_task.rs (2)
compute_units(128-132)accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (3)
compute_units(86-86)compute_units(270-272)accounts_size_budget(88-88)magicblock-committor-service/src/compute_budget.rs (1)
compute_unit_price(190-199)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
magicblock-committor-service/src/tasks/utils.rs (2)
tasks_accounts_size_budget(131-138)budget_instructions(140-154)
🔇 Additional comments (8)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
134-140: LGTM!The implementation of
accounts_size_budgetforBufferTaskis correct and follows a consistent pattern. UsingAccountSizeClass::Hugefor the Commit variant is appropriate for large account operations.
1-4: Adddlpas a dependency toCargo.toml.The imports from the
dlpcrate cannot be resolved becausedlpis not listed as a dependency inmagicblock-committor-service/Cargo.toml. This will cause a compilation error. Thedlpcrate must be added as a workspace or path dependency to enable the imports ofCommitStateFromBufferArgs,commit_size_budget, andAccountSizeClass.⛔ Skipped due to learnings
Learnt from: Dodecahedr0x Repo: magicblock-labs/magicblock-validator PR: 639 File: Cargo.toml:58-58 Timestamp: 2025-11-24T14:21:00.996Z Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.magicblock-committor-service/src/tasks/args_task.rs (1)
185-201: LGTM!The implementation correctly handles all
ArgsTaskTypevariants:
- Commit, CommitDiff, Undelegate, and Finalize appropriately use the corresponding size budget functions with
AccountSizeClass::Huge- BaseAction returns 0, which is correct since base actions don't require account data size budgets
The implementation is consistent with the
BufferTaskpattern.magicblock-committor-service/src/tasks/task_strategist.rs (2)
227-232: LGTM!The update correctly computes the accounts size budget using
tasks_accounts_size_budgetand passes it as the third parameter tobudget_instructions. This aligns with the new three-parameter signature and maintains the existing logic flow.
265-270: LGTM!The update correctly mirrors the changes in
attempt_lookup_tables, computing the size budgets and passing them tobudget_instructions. The implementation is consistent and correct.magicblock-committor-service/src/tasks/utils.rs (3)
131-138: LGTM: Pattern mirrorstasks_compute_unitscorrectly.The implementation correctly aggregates
accounts_size_budget()across all tasks using the same pattern astasks_compute_units. The approach is consistent with existing learnings that confirm tasks return budgets for individual tasks, and transaction-level budgets are computed as sums.Based on learnings: compute units (and similarly, accounts size budgets) represent the budget for a single task, with the total computed as the sum across all tasks in a transaction.
69-69: LGTM: Correctly integrates accounts size budget into transaction assembly.The call to
Self::tasks_accounts_size_budget(tasks)properly computes the aggregated budget and passes it tobudget_instructions. The integration is consistent with the existing pattern for compute units.
140-154: Verify behavior whenaccounts_size_budgetis zero.The new instruction
ComputeBudgetInstruction::set_loaded_accounts_data_size_limit(accounts_size_budget)is always included in the budget instructions, even when the budget might be zero. From the relevant code snippets,ArgsTaskType::BaseActionreturns0foraccounts_size_budget(), meaning a transaction composed entirely ofBaseActiontasks would pass0to this instruction.Verify whether:
- Passing
0is valid forset_loaded_accounts_data_size_limit- A
0value means "no limit", "default limit", or causes an error- Whether this instruction should be conditionally included only when
accounts_size_budget > 0#!/bin/bash # Description: Check Solana SDK documentation and usage patterns for set_loaded_accounts_data_size_limit # to understand behavior when passed 0 or if there are minimum value requirements. # Search for set_loaded_accounts_data_size_limit implementation and usage rg -n -A 10 -B 2 'set_loaded_accounts_data_size_limit' --type rust # Look for any validation or special handling of 0 values rg -n -A 5 'set_loaded_accounts_data_size_limit.*\b0\b' --type rust # Search for documentation or comments about this instruction rg -n -B 5 -A 10 'SetLoadedAccountsDataSizeLimit' --type rust
snawaz
force-pushed
the
snawaz/size-budget
branch
from
8fc14ec to
557f4f1
Compare
snawaz
force-pushed
the
snawaz/size-budget
branch
2 times, most recently
from
ddd40d3 to
c44bdbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
magicblock-committor-service/src/tasks/mod.rs (1)
88-89: Documentation could be more comprehensive.While documentation has been added, it could still be enhanced per the previous review suggestion to include:
- That this represents a per-task budget (analogous to
compute_units())- The unit is bytes
- The relationship between compute budget and accounts data size budget
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
🧰 Additional context used
📓 Path-based instructions (2)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rs
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-20T08:57:07.217Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-21T11:03:26.756Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 664
File: magicblock-chainlink/src/testing/mod.rs:342-370
Timestamp: 2025-11-21T11:03:26.756Z
Learning: In the magicblock-validator codebase, avoid leaving review comments that merely acknowledge code is correct or well-structured when there is no actionable suggestion, improvement, or issue to flag. Only comment when there is something specific to recommend, fix, or clarify.
Applied to files:
Cargo.toml
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧬 Code graph analysis (5)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
magicblock-committor-service/src/tasks/utils.rs (2)
tasks_accounts_size_budget(131-138)budget_instructions(140-154)
magicblock-committor-service/src/tasks/mod.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-201)magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-201)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/utils.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (2)
compute_units(175-183)accounts_size_budget(185-201)magicblock-committor-service/src/tasks/buffer_task.rs (2)
compute_units(128-132)accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (3)
compute_units(86-86)compute_units(271-273)accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/args_task.rs (2)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
🔇 Additional comments (10)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
104-114: LGTM—test boundary adjustments reflect new budget instruction.The test value adjustments (679→671, 680→673) and the accompanying comment accurately document the transaction size boundaries after adding the third budget instruction for
SetLoadedAccountsDataSizeLimit. The strategy transition from Args to FromBuffer at the new boundary is correct.
111-111: Fix typo in comment.Line 111 contains a typo: "execeds" should be "exceeds".
🔎 Proposed fix
- // of size 1644 (which is the max limit), but while the size of raw bytes for 671 is within - // 1232 limit, the size for 672 execeds by 1 (1233). That is why we used + // of size 1644 (which is the max limit), but while the size of raw bytes for 671 is within + // 1232 limit, the size for 672 exceeds by 1 (1233). That is why we usedLikely an incorrect or invalid review comment.
magicblock-committor-service/src/tasks/task_strategist.rs (2)
227-232: LGTM—accounts size budget computation and propagation.The addition of
size_budgetscomputation viatasks_accounts_size_budget()and its propagation tobudget_instructions()correctly extends the budgeting pipeline to include account data size limits. The implementation follows the established pattern for compute budget aggregation.Based on learnings, this budget represents the sum of per-task account data size budgets, analogous to how compute units are aggregated.
265-270: LGTM—consistent implementation across call sites.The size budget computation and propagation pattern is correctly duplicated in this second call site (
collect_lookup_table_keys), ensuring consistency in how budget instructions are constructed throughout the codebase.magicblock-committor-service/src/tasks/buffer_task.rs (2)
1-4: LGTM—necessary imports added.The imports correctly bring in
commit_size_budgetandAccountSizeClassfrom thedlpcrate, which are required for the newaccounts_size_budget()implementation.
134-140: LGTM—accounts size budget implementation for BufferTask.The implementation of
accounts_size_budget()correctly returnscommit_size_budget(AccountSizeClass::Huge)for Commit tasks, consistent with the pattern used inArgsTask. SinceBufferTaskTypecurrently only has theCommitvariant, the match statement appropriately covers all cases.Based on learnings, this budget represents the account data size limit for a single task, mirroring how
compute_units()represents the compute budget per task.magicblock-committor-service/src/tasks/utils.rs (3)
131-138: LGTM!The
tasks_accounts_size_budgethelper function follows the same clean pattern astasks_compute_units, properly aggregating per-task budgets. The implementation is consistent with the existing codebase patterns.
69-69: LGTM!The integration correctly computes the total accounts size budget using the new helper function and passes it to
budget_instructions. This properly enables account data size limit budgeting for task transactions.
140-154: Theset_loaded_accounts_data_size_limitAPI method is available and correctly implemented.The Solana ComputeBudgetInstruction API (v2.2) provides
set_loaded_accounts_data_size_limit(bytes: u32)as documented. The implementation correctly adds the third instruction and updates the function signature accordingly. This is a breaking change to the public API ofbudget_instructions.magicblock-committor-service/src/tasks/args_task.rs (1)
1-9: These budget functions do not appear to exist in themagicblock-delegation-programcrate.The instruction_builder module in the delegation-program repository exports builders for
delegate,undelegate,commit_state,finalize, andinit_validator_fees_vault— but does not exportcommit_size_budget,commit_diff_size_budget,finalize_size_budget, orundelegate_size_budget. The code in lines 185–201 actively calls these non-existent functions, which will cause compilation failures. Either these functions must be added to the delegation-program crate's instruction_builder module, or the imports and usages need to reference the correct functions that actually exist.⛔ Skipped due to learnings
Learnt from: bmuddha Repo: magicblock-labs/magicblock-validator PR: 578 File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27 Timestamp: 2025-10-21T14:00:54.642Z Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
snawaz
force-pushed
the
snawaz/size-budget
branch
from
c44bdbb to
120eca0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
🧰 Additional context used
📓 Path-based instructions (2)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/utils.rs
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧠 Learnings (12)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/utils.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/utils.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-20T08:57:07.217Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-21T11:03:26.756Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 664
File: magicblock-chainlink/src/testing/mod.rs:342-370
Timestamp: 2025-11-21T11:03:26.756Z
Learning: In the magicblock-validator codebase, avoid leaving review comments that merely acknowledge code is correct or well-structured when there is no actionable suggestion, improvement, or issue to flag. Only comment when there is something specific to recommend, fix, or clarify.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
Cargo.toml
🧬 Code graph analysis (4)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-205)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
magicblock-committor-service/src/tasks/utils.rs (2)
tasks_accounts_size_budget(131-138)budget_instructions(140-154)
magicblock-committor-service/src/tasks/mod.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-205)magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)
magicblock-committor-service/src/tasks/args_task.rs (2)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
🔇 Additional comments (7)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
227-232: LGTM! Consistent integration of account size budgets.The changes correctly compute the total accounts size budget by summing individual task budgets (line 227) and pass it to
budget_instructions(lines 228-232). This follows the established pattern for compute unit budgeting and aligns with the learning that transactions comprise multiple tasks with summed budgets.The same pattern is correctly applied in
collect_lookup_table_keys(lines 265-270).magicblock-committor-service/src/tasks/buffer_task.rs (1)
134-140: Verify the size class choice forBufferTaskis intentional.
BufferTask::CommitusesAccountSizeClass::Huge, whileArgsTask::CommitusesAccountSizeClass::Dynamicbased on actual account data length. More importantly,ArgsTask::Commitoptimizes directly toBufferTask::Commit(see args_task.rs lines 130-137), meaning the same task data transitions from a precise Dynamic size budget to a conservative Huge size budget.This inconsistency could affect transaction sizing and batching differently than expected. Confirm whether this behavior is intentional or if
BufferTaskshould useAccountSizeClass::Dynamic(data.len() as u32)to maintain consistency withArgsTask.magicblock-committor-service/src/tasks/utils.rs (3)
131-138: LGTM! Consistent implementation pattern.The new helper function correctly mirrors the pattern of
tasks_compute_units, summing account size budgets across all tasks. The implementation is clean and consistent with the established codebase patterns.
66-70: LGTM! Call site correctly updated.The call to
budget_instructionshas been properly updated to pass the accounts size budget using the newtasks_accounts_size_budgethelper function. The three arguments now align with the updated function signature.
140-154: All call sites have been updated correctly for the breaking API change.The function signature change (adding
accounts_size_budgetparameter and expanding the return array from 2 to 3 instructions) has been properly propagated across the codebase. All three call sites now pass the required three arguments:
magicblock-committor-service/src/tasks/utils.rs:66magicblock-committor-service/src/tasks/task_strategist.rs:228magicblock-committor-service/src/tasks/task_strategist.rs:266Return value handling is correct at all sites—the
[Instruction; 3]array is properly used as slice references (&budget_instructions) where expected. No error handling issues detected in the file.test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
104-106: LGTM! Test boundary adjustment aligned with new budgeting.The test has been appropriately adjusted from 679 to 671 bytes to reflect the transaction size limits under the new account size budgeting implementation. This ensures the test correctly validates the Args strategy boundary.
109-115: LGTM! Well-documented boundary test.This test correctly validates the transition from Args to FromBuffer commit strategy. The detailed comment (lines 110-113) effectively explains the boundary condition nuance and why 673 bytes was chosen instead of 672. This documentation will help future maintainers understand the precise boundary behavior.
snawaz
force-pushed
the
snawaz/size-budget
branch
from
120eca0 to
0ee286d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
Cargo.toml (1)
102-102: Consider removing the commented-out local path dependency.The commented line serves as a developer convenience but may cause confusion in the codebase. Since the git dependency on line 101 is the canonical approach, removing the commented alternative keeps the manifest clean.
magicblock-committor-service/src/tasks/mod.rs (1)
88-90: Add documentation for the new trait method.The
accounts_size_budgetmethod lacks documentation. Add a doc comment explaining:
- The purpose of this method (per-task account data size limit)
- What the returned
u32value represents (bytes of account data size budget)- How it relates to
compute_units()(both are per-task budgets)Based on learnings, this budget represents the account data size limit for a single task, similar to how
compute_units()represents the compute budget for a single task.🔎 Suggested documentation
+ /// Returns the accounts data size budget required for this task in bytes. + /// This budget limits the total size of account data that can be loaded + /// for this task's instruction execution. Similar to `compute_units()`, + /// this represents the budget for a single task. fn accounts_size_budget(&self) -> u32;magicblock-committor-service/src/tasks/args_task.rs (1)
197-197:BaseActionshould compute a conservative account size budget or document why zero-sizing is safe.
BaseActiontasks includeaccount_metas_per_program(accounts that will be loaded), yetaccounts_size_budget()returns0, meaning noset_loaded_accounts_data_size_limit()is enforced for these instructions.Since only
ShortAccountMeta(metadata only) is available at transaction-building time, actual account data cannot be inspected to compute size likeCommitandCommitDiffdo. However, similar tasks (Undelegate,Finalize) conservatively useAccountSizeClass::Huge. Either adopt the same conservative approach forBaseAction, or explicitly document why zero-sizing is safe.🔎 Proposed fix using conservative sizing
- ArgsTaskType::BaseAction(_) => 0, + ArgsTaskType::BaseAction(_) => { + // BaseAction loads accounts from account_metas_per_program, + // but actual data size is unknown at build time. + // Use conservative Huge budget to avoid runtime failures. + dlp::instruction_builder::base_action_size_budget(AccountSizeClass::Huge) + }Note: Verify that
base_action_size_budgetexists in the dlp crate, or use an appropriate alternative size budget function.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
🧰 Additional context used
📓 Path-based instructions (2)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rs
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧠 Learnings (12)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-20T08:57:07.217Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-21T11:03:26.756Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 664
File: magicblock-chainlink/src/testing/mod.rs:342-370
Timestamp: 2025-11-21T11:03:26.756Z
Learning: In the magicblock-validator codebase, avoid leaving review comments that merely acknowledge code is correct or well-structured when there is no actionable suggestion, improvement, or issue to flag. Only comment when there is something specific to recommend, fix, or clarify.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
Cargo.toml
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧬 Code graph analysis (5)
magicblock-committor-service/src/tasks/buffer_task.rs (3)
magicblock-committor-service/src/persist/types/commit_strategy.rs (1)
args(17-23)magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-205)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/task_strategist.rs (3)
magicblock-committor-service/src/compute_budget.rs (1)
default(15-21)magicblock-table-mania/src/compute_budget.rs (1)
default(90-97)programs/magicblock/src/schedule_transactions/transaction_scheduler.rs (1)
default(25-36)
magicblock-committor-service/src/tasks/utils.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (2)
compute_units(175-183)accounts_size_budget(185-205)magicblock-committor-service/src/tasks/buffer_task.rs (2)
compute_units(128-132)accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (3)
compute_units(86-86)compute_units(271-273)accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/args_task.rs (2)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/mod.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-205)magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)
🔇 Additional comments (4)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
104-105: LGTM!The test name and parameter adjustment accurately reflect the recalibrated size boundary for the Args strategy after introducing the accounts_size_budget feature.
109-114: LGTM! Helpful boundary explanation.The updated test correctly reflects the strategy transition from Args to FromBuffer at the 671/673 boundary. The comment clearly explains why 672 cannot be used (exceeds the 1232-byte raw limit despite the same encoded size), which aids future maintainability.
magicblock-committor-service/src/tasks/utils.rs (2)
131-138: LGTM! Consistent pattern.The new
tasks_accounts_size_budgetfunction correctly follows the established pattern oftasks_compute_units, summing individual task budgets to compute the total for a transaction. This consistency aids maintainability.Based on learnings, this pattern of summing per-task budgets for transactions is established in the codebase.
69-69: LGTM!The integration of
tasks_accounts_size_budgetintoassemble_tasks_txis correct and follows the established pattern for compute units.
snawaz
force-pushed
the
snawaz/size-budget
branch
3 times, most recently
from
e702030 to
aa2ba9f
Compare
snawaz
force-pushed
the
snawaz/size-budget
branch
from
aa2ba9f to
1336276
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @magicblock-committor-service/src/tasks/utils.rs:
- Around line 131-144: Remove the debugging println! calls and the commented-out
map line from the tasks_accounts_size_budget function: delete the two println!
lines and the commented //.map(|task| task.as_ref().accounts_size_budget()) so
the function simply maps each task via task.as_ref().accounts_size_budget(),
sums the results, and returns the u32 total; keep the existing iterator/map/sum
logic and signatures (tasks_accounts_size_budget,
BaseTask::accounts_size_budget, BaseTask::task_type remain unchanged).
In @test-integration/test-committor-service/tests/test_ix_commit_local.rs:
- Around line 109-115: In the test function
test_ix_commit_order_book_change_673_bytes update the comment typo: change
"execeds" to "exceeds" in the block that explains why 673 is used; locate the
comment near the call to commit_book_order_account(673,
CommitStrategy::FromBuffer, false) and correct the single-word spelling error to
keep the explanatory note accurate.
♻️ Duplicate comments (1)
magicblock-committor-service/src/tasks/utils.rs (1)
146-164: CRITICAL: Fix zero accounts_size_budget causing transaction failures.This implementation has two issues:
CRITICAL: Setting
SetLoadedAccountsDataSizeLimit(0)whenaccounts_size_budgetis 0 will prevent the transaction from loading any account data, including the fee payer account, causing transactions to fail at runtime. This scenario occurs when transactions contain onlyBaseActiontasks that return 0 fromaccounts_size_budget().MAJOR: Debug
println!statements (lines 151-154) should not be in production code.Solution: Either conditionally omit the
SetLoadedAccountsDataSizeLimitinstruction when the budget is 0, or enforce a safe minimum value greater than 0.🔎 Proposed fixes
Option 1: Conditionally omit the instruction when budget is 0
pub fn budget_instructions( compute_units: u32, compute_unit_price: u64, accounts_size_budget: u32, - ) -> [Instruction; 3] { - println!( - "budget_instructions: {:10}, {:10}, {}", - compute_units, compute_unit_price, accounts_size_budget - ); - [ - ComputeBudgetInstruction::set_compute_unit_limit(compute_units), - ComputeBudgetInstruction::set_compute_unit_price( - compute_unit_price, - ), - ComputeBudgetInstruction::set_loaded_accounts_data_size_limit( - accounts_size_budget, - ), - ] + ) -> Vec<Instruction> { + let mut instructions = vec![ + ComputeBudgetInstruction::set_compute_unit_limit(compute_units), + ComputeBudgetInstruction::set_compute_unit_price(compute_unit_price), + ]; + + if accounts_size_budget > 0 { + instructions.push( + ComputeBudgetInstruction::set_loaded_accounts_data_size_limit( + accounts_size_budget, + ), + ); + } + + instructions }Option 2: Enforce a safe minimum budget
pub fn budget_instructions( compute_units: u32, compute_unit_price: u64, accounts_size_budget: u32, ) -> [Instruction; 3] { - println!( - "budget_instructions: {:10}, {:10}, {}", - compute_units, compute_unit_price, accounts_size_budget - ); + // Ensure a minimum budget to allow loading at least the fee payer account + // and system program accounts. A typical account requires ~128-200 bytes. + const MIN_ACCOUNTS_SIZE_BUDGET: u32 = 10_240; // 10 KB minimum + let safe_budget = accounts_size_budget.max(MIN_ACCOUNTS_SIZE_BUDGET); + [ ComputeBudgetInstruction::set_compute_unit_limit(compute_units), ComputeBudgetInstruction::set_compute_unit_price( compute_unit_price, ), ComputeBudgetInstruction::set_loaded_accounts_data_size_limit( - accounts_size_budget, + safe_budget, ), ] }Note: If choosing Option 1, update all callers to handle
Vec<Instruction>instead of[Instruction; 3].
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rstest-integration/Cargo.tomltest-integration/test-committor-service/tests/test_ix_commit_local.rs
🧰 Additional context used
📓 Path-based instructions (2)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/args_task.rs
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/Cargo.tomltest-integration/test-committor-service/tests/test_ix_commit_local.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
Cargo.tomltest-integration/Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.tomltest-integration/Cargo.toml
📚 Learning: 2025-11-20T08:57:07.217Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-21T11:03:26.756Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 664
File: magicblock-chainlink/src/testing/mod.rs:342-370
Timestamp: 2025-11-21T11:03:26.756Z
Learning: In the magicblock-validator codebase, avoid leaving review comments that merely acknowledge code is correct or well-structured when there is no actionable suggestion, improvement, or issue to flag. Only comment when there is something specific to recommend, fix, or clarify.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
Cargo.toml
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/test-committor-service/tests/test_ix_commit_local.rs
🧬 Code graph analysis (5)
magicblock-committor-service/src/tasks/utils.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (3)
task_type(222-230)compute_units(175-183)accounts_size_budget(185-215)magicblock-committor-service/src/tasks/buffer_task.rs (3)
task_type(147-151)compute_units(128-132)accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (4)
task_type(96-96)compute_units(86-86)compute_units(271-273)accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/mod.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-215)magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-215)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
magicblock-committor-service/src/tasks/utils.rs (2)
tasks_accounts_size_budget(131-144)budget_instructions(146-164)
magicblock-committor-service/src/tasks/args_task.rs (2)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
🔇 Additional comments (11)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
104-106: LGTM! Test threshold correctly updated.The test rename and parameter adjustment from 679 to 671 bytes correctly reflects the new transaction size constraints introduced by the SetLoadedAccountsDataSizeLimit instruction.
test-integration/Cargo.toml (1)
60-60: LGTM! Dependency revision update aligns with PR objectives.The magicblock-delegation-program revision update to
b0498dff63eb704fb17a8296f59a7021beea574dis consistent with the PR's goal of consuming the new account size classification API from the DLP program.Cargo.toml (1)
101-101: LGTM! Consistent dependency update across workspace.The workspace dependency revision matches the test-integration update, ensuring all workspace members use the same DLP API version for the new account size budgeting feature.
magicblock-committor-service/src/tasks/mod.rs (1)
88-90: LGTM! Trait method addition follows established pattern.The
accounts_size_budgetmethod addition to theBaseTasktrait parallels the existingcompute_unitsmethod and enables per-task account data size budgeting forSetLoadedAccountsDataSizeLimitinstructions. The documentation requirement was addressed in a previous commit.magicblock-committor-service/src/tasks/buffer_task.rs (2)
1-4: LGTM! Imports support the new account size budgeting feature.The new imports from the DLP crate provide the necessary types and functions for computing account size budgets.
134-140: LGTM! Conservative budget allocation for buffer tasks.The implementation uses
AccountSizeClass::Hugefor buffer commits, which is appropriate since buffer-based tasks are used for larger account data that exceeds inline argument limits. This conservative approach ensures sufficient budget allocation without risking transaction failure.magicblock-committor-service/src/tasks/args_task.rs (2)
1-9: LGTM! Imports aligned with updated DLP dependency.The imports for account size budgeting functions and
AccountSizeClassare now available from the updated magicblock-delegation-program dependency (aliased asdlp), as evidenced by the concurrent revision updates in the Cargo.toml files.
185-215: LGTM! Task-specific budget calculations provide accurate sizing.The implementation appropriately routes each task type to its corresponding size budget function:
- Commit/CommitDiff: Uses
Dynamicbased on actual account data length for precise budgeting- BaseAction: Calculates budget by assuming
Smallfor action account metas andMediumfor the handler account—a reasonable heuristic given that actual account data is unavailable at transaction construction time- Undelegate/Finalize: Conservatively uses
Hugeas these operations may interact with accounts of unknown sizeThe
as u32casts and multiplication at line 200 are safe in practice given Solana's transaction size limits and 10MB maximum account data constraint.magicblock-committor-service/src/tasks/task_strategist.rs (2)
227-232: LGTM with dependency on utils.rs fix.The size budget computation and propagation are correctly implemented. However, this depends on the
budget_instructionsimplementation in utils.rs properly handling the zero-budget edge case (see critical issue flagged in utils.rs review).
265-270: LGTM with dependency on utils.rs fix.The size budget computation and propagation are correctly implemented. However, this depends on the
budget_instructionsimplementation in utils.rs properly handling the zero-budget edge case (see critical issue flagged in utils.rs review).magicblock-committor-service/src/tasks/utils.rs (1)
69-69: LGTM with dependency on budget_instructions fix.The size budget is correctly computed and passed to
budget_instructions. However, this depends onbudget_instructionsproperly handling the zero-budget edge case (see critical issue in the review of lines 146-164).
snawaz
force-pushed
the
snawaz/size-budget
branch
from
1336276 to
2627d2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI Agents
In @magicblock-committor-service/src/intent_executor/mod.rs:
- Around line 622-623: Remove the commented-out debug print (the line containing
println!("prepared_message: {:#?}", prepared_message);) from
intent_executor::mod.rs to eliminate clutter; if runtime debugging is desired
later, replace it with a log crate call such as debug! or trace! that references
the prepared_message variable instead of leaving the commented println in place.
In @magicblock-committor-service/src/intent_executor/single_stage_executor.rs:
- Line 61: Remove the debug println! in single_stage_executor.rs that prints
execution_result; either delete the println!("execution_result: {:#?}",
execution_result); line or replace it with a debug-level log via the log crate
(use log::debug with a descriptive message and the execution_result's debug
formatting), and ensure the module imports/uses the log crate so debug logs are
emitted consistently with the project's logging configuration.
- Line 136: Remove the stray println!("patch_strategy") in
single_stage_executor.rs (same kind of debug print as at line 61); either delete
it or replace it with a structured debug log call using the log crate (e.g., a
debug message like "Patching strategy for CommitIDError") and ensure the log
crate is imported/initialized so production logging is used instead of stdout.
In @magicblock-committor-service/src/intent_executor/task_info_fetcher.rs:
- Line 200: Replace the temporary println! debug prints in task_info_fetcher
(e.g., the println!("fetcher! commit_ids: {:?}", result); and the similar print
at line ~233) with structured logging or remove them; specifically, change them
to log crate calls like debug!("fetcher commit_ids={:?}", result) (or info! with
contextual fields if observability is required) so logs respect levels and
sinks, or delete the statements if they were only temporary.
In @magicblock-committor-service/src/tasks/task_builder.rs:
- Around line 137-138: Replace the ad-hoc stdout print in task_builder.rs (the
println! call that prints "task_builder commit_ids: {:?}", commit_ids) with the
logging framework: either remove it if no longer needed or change it to an
appropriate log macro such as debug! or trace! from the log crate (and ensure
the macro is imported/available in this module). Keep the message text but avoid
printing sensitive data (consider redacting or summarizing commit_ids if needed)
and ensure the module uses the project's logging level conventions.
In @magicblock-committor-service/src/tasks/utils.rs:
- Around line 151-165: budget_instructions currently always returns a fixed
[Instruction; 3] including SetLoadedAccountsDataSizeLimit( accounts_size_budget
), which breaks when accounts_size_budget == 0; change budget_instructions to
return a Vec<Instruction> and build the vector conditionally: always push
set_compute_unit_limit and set_compute_unit_price, and only push
set_loaded_accounts_data_size_limit when accounts_size_budget > 0 (or
alternatively enforce a MIN_SAFE_LIMIT before pushing). Update any callers
(e.g., tasks_accounts_size_budget usage sites) to accept Vec<Instruction>
instead of [Instruction; 3].
In @test-integration/test-committor-service/tests/test_intent_executor.rs:
- Around line 367-375: Replace the hardcoded
RpcClient::new("http://localhost:7799".to_string()) with the test fixture's
client by using fixture.rpc_client.clone(); specifically, in the match arm that
checks for IntentExecutorError::FailedToFinalizeError (where sig is available)
replace the RpcClient creation call with let rpc_client =
fixture.rpc_client.clone(); and keep the existing
print_log_messages(&rpc_client, sig).await; so the test uses the configured
client instead of a fixed URL.
In @test-integration/test-committor-service/tests/test_ix_commit_local.rs:
- Around line 109-115: Update the misspelled word in the comment inside
test_ix_commit_order_book_change_673_bytes: change "execeds" to "exceeds" in the
explanatory block above the call to commit_book_order_account(673,
CommitStrategy::FromBuffer, false). Ensure only the comment text is modified.
In @test-integration/test-committor-service/tests/utils/transactions.rs:
- Around line 71-112: Extract the duplicated retry logic in print_log_messages
and tx_logs_contain into a new async helper (e.g., fetch_transaction_with_retry)
that takes (&RpcClient, &Signature) and returns the
EncodedConfirmedTransactionWithStatusMeta; replace the inline loop in both
functions with calls to that helper; replace the .unwrap() on transaction.meta
with .expect("transaction metadata") and change the println!("logs...") to a
logging call (log::debug! or log::info!) for consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.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.rstest-integration/Cargo.tomltest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rs
🧰 Additional context used
📓 Path-based instructions (2)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rs
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/Cargo.tomltest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
🧠 Learnings (15)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/error.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/error.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 Learning: 2025-11-13T09:38:43.804Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:64-102
Timestamp: 2025-11-13T09:38:43.804Z
Learning: In magicblock-processor's TransactionScheduler (scheduler/mod.rs line 59), the executor count is clamped to MAX_SVM_EXECUTORS (63) at initialization time, and executor IDs are assigned sequentially from 0 to count-1. This architectural guarantee ensures that executor IDs used in the bitmask-based AccountLock (scheduler/locks.rs) will always be within valid bounds for bit shifting operations, making runtime bounds checks unnecessary.
Applied to files:
magicblock-committor-service/src/intent_executor/error.rs
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/utils.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rs
📚 Learning: 2025-11-20T08:57:07.217Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-21T11:03:26.756Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 664
File: magicblock-chainlink/src/testing/mod.rs:342-370
Timestamp: 2025-11-21T11:03:26.756Z
Learning: In the magicblock-validator codebase, avoid leaving review comments that merely acknowledge code is correct or well-structured when there is no actionable suggestion, improvement, or issue to flag. Only comment when there is something specific to recommend, fix, or clarify.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
🧬 Code graph analysis (8)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (1)
magicblock-aperture/src/requests/http/simulate_transaction.rs (1)
result(75-81)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
magicblock-committor-service/src/tasks/utils.rs (2)
tasks_accounts_size_budget(132-149)budget_instructions(151-165)
magicblock-committor-service/src/tasks/args_task.rs (2)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/utils.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (2)
compute_units(175-183)accounts_size_budget(185-215)magicblock-committor-service/src/tasks/buffer_task.rs (2)
compute_units(128-132)accounts_size_budget(134-140)
magicblock-committor-service/src/tasks/mod.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-215)magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-215)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
test-integration/test-tools/src/integration_test_context.rs (1)
signature(66-68)
test-integration/test-committor-service/tests/test_intent_executor.rs (1)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
print_log_messages(72-112)
🔇 Additional comments (16)
test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
104-105: LGTM!The test rename and parameter adjustment from 679 to 671 bytes correctly reflects the updated transaction size constraints described in the PR objectives.
magicblock-committor-service/src/intent_executor/error.rs (1)
179-180: LGTM! Offset correctly updated for the new budget instruction.The offset adjustment from 2 to 3 correctly accounts for the addition of
SetLoadedAccountsDataSizeLimitalongside the existing compute-budget instructions. The accompanying comment accurately documents this change.magicblock-committor-service/src/tasks/mod.rs (1)
88-90: LGTM! The trait method extension is well-documented.The new
accounts_size_budgetmethod is properly documented and extends theBaseTasktrait to support per-task accounts-data-size budgeting. The doc comment clearly explains its purpose and relationship toSetLoadedAccountsDataSizeLimit.Based on learnings, this budget represents the account data size limit for a single task, analogous to how
compute_units()represents the compute budget for a single task.magicblock-committor-service/src/tasks/buffer_task.rs (2)
1-4: LGTM! Import additions support the new size budget functionality.The new imports from the
dlpcrate (CommitStateFromBufferArgs,commit_size_budget, andAccountSizeClass) are correctly added to support theaccounts_size_budgetimplementation.
134-140: LGTM! Conservative size budget implementation for buffer tasks.The implementation correctly returns
commit_size_budget(AccountSizeClass::Huge)for buffer commit tasks. UsingAccountSizeClass::Hugeis a conservative and safe approach for buffer-based tasks.magicblock-committor-service/src/tasks/args_task.rs (2)
1-9: LGTM! Import additions are correct and necessary.The new imports from the
dlpcrate support theaccounts_size_budgetimplementation. These include size budget calculation functions andAccountSizeClassfor different task types.
185-215: LGTM! Comprehensive size budget implementation for all task types.The implementation correctly handles size budgets for each task variant:
- Commit/CommitDiff: Uses
Dynamicsize classification based on actual account data length, providing accurate budget allocation.- BaseAction: Computes budget from
Mediumclass for the primary account plusSmallclass for each additional account, which appropriately accounts for the loaded account set.- Undelegate/Finalize: Uses
Hugeclass conservatively, ensuring sufficient budget for these operations.The dynamic sizing for Commit/CommitDiff is particularly good as it avoids over-allocation while ensuring correctness.
magicblock-committor-service/src/tasks/task_strategist.rs (2)
223-258: LGTM!The integration of
tasks_accounts_size_budgetfollows the same pattern astasks_compute_units, maintaining consistency in the codebase.
260-277: LGTM!The changes correctly propagate the accounts size budget through the lookup table collection logic.
magicblock-committor-service/src/tasks/utils.rs (3)
3-3: LGTM!The import of
DLP_PROGRAM_DATA_SIZE_CLASSis necessary for computing account size budgets.
61-79: LGTM!The integration of
tasks_accounts_size_budgetinto transaction assembly is correct and follows the established pattern for compute units.
132-149: LGTM!The function correctly:
- Handles the empty tasks case
- Sums per-task account size budgets
- Adjusts for the DLP program data size class that would otherwise be counted N times (once per task) when it should only be counted once
The debug prints and commented code from the previous review have been successfully removed.
test-integration/test-committor-service/tests/test_intent_executor.rs (4)
18-18: LGTM!The import of
IntentExecutorErroris necessary for the enhanced error handling in the test recovery path.
55-55: LGTM!The import of
print_log_messagesutility enables better observability during test execution for debugging finalization failures.
157-162: LGTM!The enhanced assertion provides better debugging information by including the actual error value in the failure message.
377-382: LGTM!The enhanced assertion improves debuggability by including both the result and patched_errors in failure messages.
magicblock-committor-service/src/intent_executor/single_stage_executor.rs
Outdated
Show resolved
Hide resolved
magicblock-committor-service/src/intent_executor/single_stage_executor.rs
Outdated
Show resolved
Hide resolved
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs
Outdated
Show resolved
Hide resolved
test-integration/test-committor-service/tests/utils/transactions.rs
Outdated
Show resolved
Hide resolved
snawaz
force-pushed
the
snawaz/size-budget
branch
from
2627d2b to
970fba2
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI Agents
In @magicblock-committor-service/src/tasks/buffer_task.rs:
- Around line 134-140: The accounts_size_budget method always returns
commit_size_budget(AccountSizeClass::Huge) for BufferTask commit variants; add a
brief clarifying comment inside or above the accounts_size_budget function
(referencing accounts_size_budget, BufferTaskType::Commit, and
AccountSizeClass::Huge) explaining that BufferTask uses a fixed Huge size class
because it prepares large accounts and should not use dynamic sizing like
ArgsTask, to make the rationale explicit for future readers.
In @magicblock-committor-service/src/tasks/task_builder.rs:
- Around line 137-138: Replace the temporary stdout debug print in
task_builder.rs (the println!("task_builder commit_ids: {:?}", commit_ids) call)
with the logging framework: either remove it or change it to a log::debug! (or
trace!) call and ensure the module imports the log macros (or uses the tracing
crate consistently with trace!/debug!), so the message uses the project's
logging infrastructure and includes the commit_ids variable.
In @magicblock-committor-service/src/tasks/utils.rs:
- Around line 151-164: budget_instructions currently always emits
SetLoadedAccountsDataSizeLimit(accounts_size_budget) which breaks transactions
when accounts_size_budget == 0; change budget_instructions to return a
Vec<Instruction> instead of [Instruction;3], create a vec with
ComputeBudgetInstruction::set_compute_unit_limit and ::set_compute_unit_price
always, and only push
ComputeBudgetInstruction::set_loaded_accounts_data_size_limit when
accounts_size_budget > 0 (or enforce a MIN_SAFE_LIMIT before pushing); update
callers of budget_instructions to accept a Vec<Instruction>.
In @test-integration/test-committor-service/tests/test_intent_executor.rs:
- Around line 367-375: The test creates a new RpcClient with
RpcClient::new("http://localhost:7799") which hardcodes the URL; replace that
with the already-configured fixture client by using fixture.rpc_client (or
fixture.rpc_client.clone() if ownership is required) and pass it to
print_log_messages(&fixture.rpc_client, sig) instead of constructing a new
client.
In @test-integration/test-committor-service/tests/test_ix_commit_local.rs:
- Around line 109-114: Update the typo in the comment inside
test_ix_commit_order_book_change_673_bytes: change "execeds" to "exceeds" in the
explanatory comment above the call to commit_book_order_account(673,
CommitStrategy::FromBuffer, false). Ensure only the comment text is modified (no
code logic changes) and run tests to verify nothing else is affected.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.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.rstest-integration/Cargo.tomltest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rs
🧰 Additional context used
📓 Path-based instructions (2)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rs
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/Cargo.toml
🧠 Learnings (17)
📓 Common learnings
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/error.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-committor-service/src/intent_executor/mod.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-committor-service/src/intent_executor/error.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rstest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/intent_executor/task_info_fetcher.rstest-integration/test-committor-service/tests/utils/transactions.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_builder.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rstest-integration/test-committor-service/tests/test_intent_executor.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-11-13T09:38:43.804Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:64-102
Timestamp: 2025-11-13T09:38:43.804Z
Learning: In magicblock-processor's TransactionScheduler (scheduler/mod.rs line 59), the executor count is clamped to MAX_SVM_EXECUTORS (63) at initialization time, and executor IDs are assigned sequentially from 0 to count-1. This architectural guarantee ensures that executor IDs used in the bitmask-based AccountLock (scheduler/locks.rs) will always be within valid bounds for bit shifting operations, making runtime bounds checks unnecessary.
Applied to files:
magicblock-committor-service/src/intent_executor/error.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rsmagicblock-committor-service/src/intent_executor/single_stage_executor.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-committor-service/src/intent_executor/single_stage_executor.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
Cargo.tomltest-integration/Cargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
Cargo.tomltest-integration/Cargo.toml
📚 Learning: 2025-11-20T08:57:07.217Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-21T11:03:26.756Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 664
File: magicblock-chainlink/src/testing/mod.rs:342-370
Timestamp: 2025-11-21T11:03:26.756Z
Learning: In the magicblock-validator codebase, avoid leaving review comments that merely acknowledge code is correct or well-structured when there is no actionable suggestion, improvement, or issue to flag. Only comment when there is something specific to recommend, fix, or clarify.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
Cargo.toml
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rs
🧬 Code graph analysis (6)
magicblock-committor-service/src/tasks/mod.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-215)magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
magicblock-committor-service/src/tasks/utils.rs (2)
tasks_accounts_size_budget(132-149)budget_instructions(151-165)
test-integration/test-committor-service/tests/test_intent_executor.rs (1)
test-integration/test-committor-service/tests/utils/transactions.rs (1)
print_log_messages(72-112)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-215)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/utils.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (2)
compute_units(175-183)accounts_size_budget(185-215)magicblock-committor-service/src/tasks/buffer_task.rs (2)
compute_units(128-132)accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (3)
compute_units(86-86)compute_units(271-273)accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/args_task.rs (2)
magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
🔇 Additional comments (24)
test-integration/Cargo.toml (1)
60-60: LGTM! Dependency revision synchronized with workspace.The magicblock-delegation-program dependency revision is correctly synchronized with the workspace dependency, maintaining consistency between production and test code.
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
200-200: Replaceprintln!with proper logging or remove debug statements.Using
println!in production service code bypasses structured logging and writes directly to stdout. Thelogcrate is already imported and used elsewhere in this file. These debug prints should be removed if temporary or converted to proper log statements.Likely an incorrect or invalid review comment.
233-233: Replaceprintln!with proper logging or remove debug statements.Using
println!in production service code bypasses structured logging and writes directly to stdout. Thelogcrate is already imported and used elsewhere in this file. These debug prints should be removed if temporary or converted to proper log statements.Likely an incorrect or invalid review comment.
test-integration/test-committor-service/tests/utils/transactions.rs (1)
71-112: Consider extracting shared retry logic into a helper function.The retry logic in
print_log_messagesduplicates the pattern fromtx_logs_contain(lines 132-158). Extracting this into a shared helper function would improve maintainability.⛔ Skipped due to learnings
Learnt from: Dodecahedr0x Repo: magicblock-labs/magicblock-validator PR: 639 File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881 Timestamp: 2025-12-03T09:33:48.707Z Learning: Repo: magicblock-labs/magicblock-validator PR: 639 Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local) Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.Learnt from: Dodecahedr0x Repo: magicblock-labs/magicblock-validator PR: 639 File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353 Timestamp: 2025-12-03T09:36:01.527Z Learning: Repo: magicblock-labs/magicblock-validator File: magicblock-chainlink/src/remote_account_provider/mod.rs Context: consolidate_fetched_remote_accounts Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).Cargo.toml (1)
101-101: Account size classification APIs present in selected revision.Revision
b0498dff63eb704fb17a8296f59a7021beea574dincludes theAccountSizeClassenum with coarse-grained account size classifications (Tiny, Small, Medium, Large, ExtraLarge, Huge, Dynamic) and size budget computation functions for DLP instructions (commit_diff_size_budget,finalize_size_budget,undelegate_size_budget, etc.). This supports the PR's data-budget calculation requirements.magicblock-committor-service/src/tasks/mod.rs (1)
88-90: LGTM! Well-documented trait method addition.The new
accounts_size_budget()method is well-documented and follows the same pattern ascompute_units(). The documentation clearly explains its purpose for use withSetLoadedAccountsDataSizeLimit.Based on learnings, this method returns the per-task accounts data size budget (in bytes), analogous to how
compute_units()returns the per-task compute budget.magicblock-committor-service/src/tasks/buffer_task.rs (1)
1-4: LGTM! Imports are appropriate.The new imports correctly support the
accounts_size_budget()implementation, bringing in the necessary types and functions from the DLP crate.magicblock-committor-service/src/intent_executor/error.rs (1)
179-180: OFFSET constant correctly updated from 2 to 3 to match budget instruction count.The
budget_instructions()function atmagicblock-committor-service/src/tasks/utils.rs:155returns exactly[Instruction; 3], containingset_compute_unit_limit,set_compute_unit_price, andset_loaded_accounts_data_size_limit. The OFFSET constant matches this, and all call sites inutils.rs:67andtask_strategist.rs:228,266use the same function, ensuring consistent prepending of 3 budget instructions. Thechecked_subusage at error.rs:198 safely maps instruction error indices to task indices.magicblock-committor-service/src/tasks/args_task.rs (2)
4-8: LGTM! Import additions for size budget helpers.The new imports from
dlp::instruction_builderprovide the necessary budget calculation functions for each task type.
185-215: LGTM! Well-structured accounts_size_budget implementation.The implementation correctly:
- Uses
AccountSizeClass::Dynamicwith actual data length forCommitandCommitDifftasks, enabling precise budgeting.- Computes
BaseActionbudget usingAccountSizeClass::Mediumplus per-account overhead for program accounts (assumingSmallclass).- Uses conservative
AccountSizeClass::HugeforUndelegateandFinalizewhere account sizes are unknown at build time.This addresses the previous concern about
BaseActionreturning zero budget.test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
104-105: LGTM! Test updated for new boundary values.Test renamed and threshold value adjusted from 679 to 671 bytes to reflect updated size calculations.
magicblock-committor-service/src/tasks/utils.rs (3)
3-3: LGTM! Import for DLP program data size constant.
70-70: LGTM! Properly passes accounts size budget to budget_instructions.
132-149: LGTM! Correct deduplication of shared program account budget.The logic correctly subtracts
(N-1) * DLP_PROGRAM_DATA_SIZE_CLASS.size_budget()to avoid counting the DLP program account multiple times when aggregating budgets across tasks. Based on learnings, the total compute budget for a transaction is the sum of all tasks' budgets, so this deduplication is necessary.magicblock-committor-service/src/tasks/task_strategist.rs (2)
227-232: LGTM! Correct integration of accounts size budget in ALT estimation.The size budget is properly computed and passed to
budget_instructionsfor accurate transaction size estimation when attempting lookup tables.
265-270: LGTM! Consistent budget calculation for lookup table key collection.Follows the same pattern as
attempt_lookup_tables, ensuring consistent budget handling across both call sites.test-integration/test-committor-service/tests/test_intent_executor.rs (8)
18-18: LGTM! Import updated to includeIntentExecutorError.The expanded import enables proper error handling in recovery test scenarios.
55-55: LGTM! Import forprint_log_messagesutility.Enables enhanced observability during test failure debugging.
116-116: LGTM! Updated instruction index in error message.The expected error message now references "Instruction 3" instead of a lower index, correctly reflecting the addition of
SetLoadedAccountsDataSizeLimitas the third compute budget instruction.
157-161: LGTM! Improved assertion message format.The assertion now includes the actual error for better debugging when tests fail.
168-168: LGTM! Instruction index updated for undelegation error.Reflects the shifted instruction indices due to the additional compute budget instruction.
217-217: LGTM! Instruction index updated for action error.
277-277: LGTM! Instruction index updated for CPI limits error.
377-382: LGTM! Enhanced assertion with error context.The assertion now includes both the result and patched errors for better debugging output.
test-integration/test-committor-service/tests/test_intent_executor.rs
Outdated
Show resolved
Hide resolved
snawaz
force-pushed
the
snawaz/size-budget
branch
from
970fba2 to
c2e3b13
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @magicblock-committor-service/src/tasks/utils.rs:
- Around line 135-137: The early return of 0 for empty tasks leads to
SetLoadedAccountsDataSizeLimit(0) being added to budget_instructions which
blocks loading any account data (including the fee payer); update the logic in
the function that computes the accounts_size_budget so it never returns 0 —
either enforce a minimum safe nonzero limit (e.g., return 1) when
tasks.is_empty() or change the caller that builds budget_instructions to skip
adding SetLoadedAccountsDataSizeLimit when the computed budget is 0; update
references to SetLoadedAccountsDataSizeLimit and budget_instructions accordingly
so the transaction can always load required accounts.
- Around line 154-164: The current routine unconditionally builds
ComputeBudgetInstruction::set_loaded_accounts_data_size_limit(accounts_size_budget)
which will create SetLoadedAccountsDataSizeLimit(0) when accounts_size_budget ==
0 and break transactions; change the function to return Vec<Instruction>
(instead of [Instruction; 3]) and push
ComputeBudgetInstruction::set_compute_unit_limit(compute_units) and
ComputeBudgetInstruction::set_compute_unit_price(compute_unit_price) always, but
only push
ComputeBudgetInstruction::set_loaded_accounts_data_size_limit(accounts_size_budget)
when accounts_size_budget > 0; update all callers of this function to accept
Vec<Instruction> and adjust any error-handling offsets or assumptions that
relied on a fixed instruction count.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.tomlmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/mod.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rstest-integration/Cargo.tomltest-integration/test-committor-service/tests/test_intent_executor.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rs
🧰 Additional context used
📓 Path-based instructions (2)
{magicblock-*,programs,storage-proto}/**
⚙️ CodeRabbit configuration file
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue.
These should not be categorized as trivial or nit-level concerns.
Request proper error handling or explicit justification with invariants.
Files:
magicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/task_strategist.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rs
{test-*,tools}/**
⚙️ CodeRabbit configuration file
Usage of
.unwrap()or.expect()in test code is acceptable and may be treated as trivial.
Files:
test-integration/Cargo.tomltest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
🧠 Learnings (15)
📓 Common learnings
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-aperture/src/requests/http/get_fee_for_message.rs:25-31
Timestamp: 2025-11-04T13:22:38.811Z
Learning: In magicblock-aperture, request size validation (including limits on message size) is enforced at the request ingestion layer, before handlers like get_fee_for_message are invoked. Therefore, handlers do not need additional size guards on decoded data.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-committor-service/src/intent_executor/error.rstest-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
magicblock-committor-service/src/intent_executor/error.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-10-26T08:49:31.543Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 585
File: magicblock-committor-service/src/tasks/buffer_task.rs:111-115
Timestamp: 2025-10-26T08:49:31.543Z
Learning: In the magicblock-committor-service, compute units returned by the `compute_units()` method in task implementations (such as `BufferTask`, `ArgsTask`, etc.) represent the compute budget for a single task. Transactions can comprise multiple tasks, and the total compute budget for a transaction is computed as the sum of the compute units of all tasks included in that transaction.
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/tasks/buffer_task.rsmagicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rsmagicblock-committor-service/src/tasks/mod.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-committor-service/src/tasks/utils.rsmagicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
test-integration/Cargo.tomlCargo.toml
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
test-integration/test-committor-service/tests/utils/transactions.rstest-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-11-20T08:57:07.217Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 650
File: magicblock-chainlink/src/submux/subscription_task.rs:13-99
Timestamp: 2025-11-20T08:57:07.217Z
Learning: In the magicblock-validator repository, avoid posting review comments that merely confirm code is correct or matches intended behavior without providing actionable feedback, suggestions for improvement, or identifying potential issues. Such confirmatory comments are considered unhelpful noise by the maintainers.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-21T11:03:26.756Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 664
File: magicblock-chainlink/src/testing/mod.rs:342-370
Timestamp: 2025-11-21T11:03:26.756Z
Learning: In the magicblock-validator codebase, avoid leaving review comments that merely acknowledge code is correct or well-structured when there is no actionable suggestion, improvement, or issue to flag. Only comment when there is something specific to recommend, fix, or clarify.
Applied to files:
Cargo.toml
📚 Learning: 2025-11-07T18:19:48.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: test-integration/programs/schedulecommit/Cargo.toml:19-20
Timestamp: 2025-11-07T18:19:48.996Z
Learning: In Solana programs (crates depending on solana-program), the empty feature flags `custom-heap = []` and `custom-panic = []` are standard declarations required to avoid compiler warnings. These should not be flagged as unused features in Cargo.toml files for Solana program crates.
Applied to files:
Cargo.toml
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_intent_executor.rs
🧬 Code graph analysis (6)
magicblock-committor-service/src/tasks/buffer_task.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-215)magicblock-committor-service/src/tasks/mod.rs (1)
accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/task_strategist.rs (1)
magicblock-committor-service/src/tasks/utils.rs (2)
tasks_accounts_size_budget(132-149)budget_instructions(151-165)
magicblock-committor-service/src/tasks/utils.rs (4)
magicblock-committor-service/src/compute_budget.rs (4)
total_budget(32-34)total_budget(55-63)total_budget(201-206)compute_unit_price(190-199)magicblock-committor-service/src/tasks/args_task.rs (2)
compute_units(175-183)accounts_size_budget(185-215)magicblock-committor-service/src/tasks/buffer_task.rs (2)
compute_units(128-132)accounts_size_budget(134-140)magicblock-committor-service/src/tasks/mod.rs (3)
compute_units(86-86)compute_units(271-273)accounts_size_budget(89-89)
magicblock-committor-service/src/tasks/mod.rs (2)
magicblock-committor-service/src/tasks/args_task.rs (1)
accounts_size_budget(185-215)magicblock-committor-service/src/tasks/buffer_task.rs (1)
accounts_size_budget(134-140)
test-integration/test-committor-service/tests/utils/transactions.rs (2)
test-integration/test-tools/src/integration_test_context.rs (1)
signature(66-68)magicblock-rpc-client/src/lib.rs (1)
signature(90-98)
test-integration/test-committor-service/tests/test_intent_executor.rs (1)
test-integration/test-tools/src/conversions.rs (1)
err(8-8)
🔇 Additional comments (17)
test-integration/test-committor-service/tests/test_intent_executor.rs (2)
115-115: LGTM! Error message indices correctly updated.The instruction indices in error messages are consistently incremented to reflect the new
OFFSETvalue of 3 inerror.rs. This ensures error parsing tests remain accurate with the additional budget instruction.Also applies to: 167-167, 216-216, 276-276
156-160: Good improvement to assertion messages.Adding debug output (
err,res,patched_errors) to the assertion messages will significantly improve test failure diagnostics.Also applies to: 366-371
test-integration/test-committor-service/tests/utils/transactions.rs (2)
72-127: Excellent refactoring to eliminate duplication.Extracting
fetch_tx_logsandprint_tx_logsaddresses the past review feedback about duplicated retry logic. The helper functions are well-structured and will improve maintainability.Note: The
.unwrap()on line 118 is acceptable for test utilities per the coding guidelines.
130-149: Good adaptation for CommitState/CommitDiff variability.The special handling to match both "CommitState" and "CommitDiff" when the needle is "CommitState" is well-justified by the comment. This approach correctly handles the size-dependent decision in CommitTask while maintaining test simplicity.
test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)
104-114: Test thresholds correctly adjusted for new size budget logic.The updated test names and parameters (671 and 673 bytes) align with the new accounts size budget implementation. The comments clearly explain why 673 is used instead of 672 (avoids exceeding the 1232-byte raw data limit), and the CommitStrategy change to
FromBufferis appropriate for this threshold.
112-112: Fix typo (already flagged twice in previous reviews).Line 112 contains "execeds" which should be "exceeds". This typo has been flagged in two previous reviews but remains unfixed.
🔎 Proposed fix
- // 1232 limit, the size for 672 execeds by 1 (1233). That is why we used + // 1232 limit, the size for 672 exceeds by 1 (1233). That is why we usedLikely an incorrect or invalid review comment.
test-integration/Cargo.toml (1)
64-66: Dependency revision is valid and contains the expected AccountSizeClass API.The revision
b0498dff63eb704fb17a8296f59a7021beea574dintroduces a newAccountSizeClassmodule with size computation for multiple instructions, confirming it provides the functionality required by the PR.Cargo.toml (1)
101-103: LGTM! Dependency update aligns with PR objectives.The git revision update to
b0498dff63eb704fb17a8296f59a7021beea574dbrings in the account size classification API required for the newSetLoadedAccountsDataSizeLimitfunctionality described in the PR objectives.magicblock-committor-service/src/tasks/mod.rs (1)
88-90: LGTM! Trait method properly documented.The
accounts_size_budget()method is well-documented and correctly positioned in theBaseTasktrait. The documentation clearly explains its purpose for use withSetLoadedAccountsDataSizeLimit.magicblock-committor-service/src/tasks/buffer_task.rs (2)
1-4: LGTM! Imports align with updated dependency.The new imports from the
dlpcrate (CommitStateFromBufferArgs, commit_size_budget, AccountSizeClass) are required for the accounts size budgeting functionality and should be available in the updated delegation-program revision.
134-140: LGTM! Conservative size budget for buffer tasks.The implementation correctly uses
AccountSizeClass::Hugefor buffer commit tasks. This conservative approach is appropriate sinceBufferTaskis designed for large accounts that require buffer preparation, unlikeArgsTaskwhich uses dynamic sizing based on actual account data length.magicblock-committor-service/src/tasks/args_task.rs (2)
1-9: LGTM! Imports rely on updated delegation-program API.The imported size budget functions and
AccountSizeClassfrom thedlpcrate are part of the new account size classification API mentioned in the PR objectives. These should be available in the updated delegation-program revision (b0498dff63eb704fb17a8296f59a7021beea574d).
185-215: LGTM! Well-designed per-task-type size budgeting.The implementation appropriately tailors size budgets to each task type:
- Commit/CommitDiff: Dynamic sizing based on actual account data length for optimal resource allocation
- BaseAction: Computes budget from account metadata, conservatively assuming additional accounts are Small class
- Undelegate/Finalize: Conservative Huge class sizing for safety
This design balances optimization (dynamic sizing where data is available) with safety (conservative estimates where it's not).
magicblock-committor-service/src/tasks/task_strategist.rs (2)
227-232: LGTM! Correctly propagates size budgets in ALT attempt.The code properly computes the aggregate accounts size budget from all tasks and passes it to
budget_instructionsas the third parameter, enabling size limit enforcement when attempting address lookup tables.
265-270: LGTM! Consistent size budget propagation.The second call site follows the same correct pattern: computing the aggregate size budget and passing it to
budget_instructions. Both call sites intask_strategist.rsnow properly propagate accounts size budgets.magicblock-committor-service/src/tasks/utils.rs (2)
70-70: Integration looks correct.The call to
tasks_accounts_size_budget(tasks)correctly integrates the new accounts size budget calculation into transaction assembly. However, ensure the underlying functions handle edge cases properly (see other comments).
144-148: Verify the DLP adjustment assumption across all task types.The calculation assumes that each task's
accounts_size_budget()includesDLP_PROGRAM_DATA_SIZE_CLASS.size_budget()exactly once. The helper functions (commit_size_budget,commit_diff_size_budget,call_handler_size_budget,undelegate_size_budget,finalize_size_budget) are imported from the externaldlp::instruction_buildermodule, which is not available in this repository for inspection. Without access to the dlp crate's source code, this assumption cannot be verified from the codebase alone. Either document the invariant clearly or provide explicit proof that all task budget helpers include DLP exactly once.
snawaz
force-pushed
the
snawaz/size-budget
branch
from
c2e3b13 to
6c5178f
Compare
Solana runtime allocates 64 MB of buffer space to load all accounts for a transaction. In most cases, we do not need such a large buffer, yet we still pay the cost in terms of compute units (CU).
This PR uses
SetLoadedAccountsDataSizeLimitwhen invoking DLP instructions to reduce unnecessary account data allocation and improve compute efficiency.The DLP program now exposes a coarse-grained account size classification API that allows callers to compute a safe upper bound on the total loaded account data without fetching accounts on-chain. This PR consumes that API to:
SetLoadedAccountsDataSizeLimitcompute-budget instruction alongside existing compute-budget instructions.No behavioral changes are introduced to instruction execution.