feat: configurable minimum crank frequency #717
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThis pull request introduces configurable minimum interval constraints for task scheduling while refactoring error handling across the codebase. Key changes include: adding a Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
programs/magicblock/src/schedule_task/process_schedule_task.rs (1)
154-162: Global mutable min frequency in tests can cause cross-test flakiness
fail_process_schedule_invalid_execution_intervalmutates the globalMIN_TASK_SCHEDULER_FREQUENCYviaset_min_task_scheduler_frequencyand never restores it. Because tests may run in any order (and potentially in parallel), this can:
- Leave the global set to the last value in the loop (e.g.,
1000), causing other tests that assume the default (e.g., 10ms) to fail intermittently.- Introduce nondeterministic behavior if multiple tests read or write this global concurrently.
At minimum, snapshot and restore the previous value inside this test so it doesn’t leak state:
#[test] fn fail_process_schedule_invalid_execution_interval() { - let (payer, pdas, transaction_accounts) = setup_accounts(0); - for frequency in [0, 10, 100, 1000] { + let original_frequency = min_task_scheduler_frequency(); + let (payer, pdas, transaction_accounts) = setup_accounts(0); + for frequency in [0, 10, 100, 1000] { set_min_task_scheduler_frequency(frequency); let args = ScheduleTaskArgs { task_id: 1, execution_interval_millis: frequency - 1, iterations: 1, instructions: vec![create_simple_ix()], }; let ix = InstructionUtils::schedule_task_instruction( &payer.pubkey(), args, &pdas, ); process_instruction( &ix.data, transaction_accounts.clone(), ix.accounts, Err(InstructionError::InvalidInstructionData), ); } + set_min_task_scheduler_frequency(original_frequency); }If your test harness runs tests in parallel, you may also want to serialize tests that touch this global (e.g., using a serial test attribute or a shared test-only lock) or ensure every such test fully configures the global to its expected value before use.
Also applies to: 361-383
📜 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)
config.example.toml(1 hunks)magicblock-config/src/config/scheduler.rs(1 hunks)magicblock-config/src/consts.rs(1 hunks)magicblock-config/src/tests.rs(3 hunks)magicblock-task-scheduler/src/service.rs(2 hunks)programs/magicblock/Cargo.toml(1 hunks)programs/magicblock/src/lib.rs(1 hunks)programs/magicblock/src/schedule_task/process_schedule_task.rs(4 hunks)programs/magicblock/src/task_scheduler_frequency.rs(1 hunks)test-integration/test-ledger-restore/src/lib.rs(1 hunks)test-integration/test-task-scheduler/src/lib.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
📚 Learning: 2025-11-12T09:46:27.553Z
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.
Applied to files:
programs/magicblock/src/lib.rstest-integration/test-ledger-restore/src/lib.rsmagicblock-config/src/consts.rsmagicblock-config/src/tests.rstest-integration/test-task-scheduler/src/lib.rsmagicblock-config/src/config/scheduler.rsprograms/magicblock/src/task_scheduler_frequency.rsmagicblock-task-scheduler/src/service.rsprograms/magicblock/src/schedule_task/process_schedule_task.rs
📚 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:
programs/magicblock/src/lib.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:
programs/magicblock/src/lib.rsprograms/magicblock/src/task_scheduler_frequency.rsmagicblock-task-scheduler/src/service.rsprograms/magicblock/src/schedule_task/process_schedule_task.rs
📚 Learning: 2025-11-20T17:25:23.444Z
Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.
Applied to files:
magicblock-config/src/consts.rsprograms/magicblock/src/task_scheduler_frequency.rsmagicblock-task-scheduler/src/service.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
programs/magicblock/src/task_scheduler_frequency.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
magicblock-task-scheduler/src/service.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:
programs/magicblock/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:
programs/magicblock/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:
programs/magicblock/src/schedule_task/process_schedule_task.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:
programs/magicblock/src/schedule_task/process_schedule_task.rs
🧬 Code graph analysis (4)
test-integration/test-ledger-restore/src/lib.rs (1)
magicblock-config/src/config/scheduler.rs (1)
default(16-22)
magicblock-config/src/tests.rs (1)
magicblock-task-scheduler/src/service.rs (1)
new(68-102)
magicblock-task-scheduler/src/service.rs (1)
programs/magicblock/src/task_scheduler_frequency.rs (1)
set_min_task_scheduler_frequency(17-22)
programs/magicblock/src/schedule_task/process_schedule_task.rs (3)
programs/magicblock/src/task_scheduler_frequency.rs (2)
min_task_scheduler_frequency(11-15)set_min_task_scheduler_frequency(17-22)programs/magicblock/src/utils/instruction_utils.rs (1)
schedule_task_instruction(212-227)programs/magicblock/src/test_utils/mod.rs (1)
process_instruction(44-61)
🔇 Additional comments (9)
config.example.toml (1)
210-213: LGTM!The configuration option is well-documented and follows existing patterns. The default value of 10 milliseconds is reasonable for a minimum task execution frequency, and the environment variable naming convention is consistent with other configuration options.
magicblock-config/src/consts.rs (1)
37-39: LGTM!The constant is appropriately typed as
i64(consistent with task scheduler types in the codebase) and has a clear comment header. The default value of 10 milliseconds matches the configuration example.programs/magicblock/Cargo.toml (1)
17-17: LGTM!The workspace dependency is correctly added to support the new
task_scheduler_frequencymodule functionality.test-integration/test-task-scheduler/src/lib.rs (1)
49-52: LGTM!The struct update syntax correctly applies default values for the new
min_frequency_millisfield while preserving the explicitreset: truesetting. This is a clean way to maintain backward compatibility with the expanded configuration structure.test-integration/test-ledger-restore/src/lib.rs (1)
150-153: LGTM!The initialization pattern is consistent with other test files and correctly applies default values for the new
min_frequency_millisfield.programs/magicblock/src/lib.rs (1)
6-6: LGTM!The new module is appropriately exposed as public API, enabling configuration of the minimum task scheduler frequency at runtime.
magicblock-config/src/tests.rs (2)
464-464: LGTM!The test correctly verifies that the default value for
min_frequency_millisis 10 when loading from the example configuration file.
518-518: LGTM!The test provides good coverage for environment variable overrides, ensuring that
MBV_TASK_SCHEDULER__MIN_FREQUENCY_MILLIScorrectly sets the minimum frequency value to 99 (overriding the default of 10).Also applies to: 572-572
magicblock-task-scheduler/src/service.rs (1)
76-76: Consider refactoring global state initialization formin_frequency_millis.The call to
set_min_task_scheduler_frequencymutates a process-wide static variable via RwLock. While the RwLock ensures thread-safe access, the design creates a potential issue: if multipleTaskSchedulerServiceinstances are initialized in the same process with different configurations (particularly in tests), the last instance initialized will overwrite the global state for all code paths that readmin_task_scheduler_frequency(). This is primarily a design concern rather than a concurrency issue, but it could lead to unexpected behavior if service instances are ever recreated with different configs.
| use crate::consts; | ||
|
|
||
| /// Configuration for the internal task scheduler. | ||
| #[derive(Deserialize, Serialize, Debug, Clone, Default)] | ||
| #[derive(Deserialize, Serialize, Debug, Clone)] | ||
| #[serde(rename_all = "kebab-case", deny_unknown_fields, default)] | ||
| pub struct TaskSchedulerConfig { | ||
| /// If true, clears all pending scheduled tasks on startup. | ||
| pub reset: bool, | ||
| /// The minimum frequency for a task to be executed, in milliseconds. | ||
| pub min_frequency_millis: i64, | ||
| } | ||
|
|
||
| impl Default for TaskSchedulerConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| reset: false, | ||
| min_frequency_millis: | ||
| consts::DEFAULT_TASK_SCHEDULER_MIN_FREQUENCY_MILLIS, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Config default wiring for min_frequency_millis looks good
Adding min_frequency_millis: i64 with a manual Default implementation that uses DEFAULT_TASK_SCHEDULER_MIN_FREQUENCY_MILLIS keeps TaskSchedulerConfig’s behavior predictable and compatible with #[serde(default)]. This cleanly exposes the configurable minimum for the scheduler while defaulting to the previous hard-coded value via the const.
If you want to reduce potential confusion between “frequency” and “interval” terminology, you might consider aligning field/docs naming (e.g., min_interval_millis) before this config surface becomes widely used, but that’s cosmetic and not a blocker.
🤖 Prompt for AI Agents
In magicblock-config/src/config/scheduler.rs around lines 3 to 23, rename the
field and doc from min_frequency_millis to min_interval_millis to avoid
“frequency vs interval” confusion: change the struct field name and its doc
comment, update the Default impl to set min_interval_millis =
consts::DEFAULT_TASK_SCHEDULER_MIN_FREQUENCY_MILLIS, and add a serde alias so
existing serialized configs still work (e.g., #[serde(alias =
"min-frequency-millis")] on the field) while keeping #[serde(rename_all =
"kebab-case", deny_unknown_fields, default)] intact.
| use std::sync::RwLock; | ||
|
|
||
| use lazy_static::lazy_static; | ||
| use magicblock_config::consts; | ||
|
|
||
| lazy_static! { | ||
| static ref MIN_TASK_SCHEDULER_FREQUENCY: RwLock<i64> = | ||
| RwLock::new(consts::DEFAULT_TASK_SCHEDULER_MIN_FREQUENCY_MILLIS); | ||
| } | ||
|
|
||
| pub fn min_task_scheduler_frequency() -> i64 { | ||
| *MIN_TASK_SCHEDULER_FREQUENCY | ||
| .read() | ||
| .expect("RwLock MIN_TASK_SCHEDULER_FREQUENCY poisoned") | ||
| } | ||
|
|
||
| pub fn set_min_task_scheduler_frequency(frequency: i64) { | ||
| let mut min_task_scheduler_frequency_lock = MIN_TASK_SCHEDULER_FREQUENCY | ||
| .write() | ||
| .expect("RwLock MIN_TASK_SCHEDULER_FREQUENCY poisoned"); | ||
| *min_task_scheduler_frequency_lock = frequency.max(1); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Global min frequency implementation is correct; consider an atomic as a lighter alternative
The lazy-initialized RwLock<i64> with a default from DEFAULT_TASK_SCHEDULER_MIN_FREQUENCY_MILLIS and clamping via frequency.max(1) achieves the desired behavior: a process-wide, configurable, strictly positive minimum interval that can be read from validation code and set during initialization.
Given that you only ever store and load a single scalar value, you could simplify and avoid lock poisoning by using an AtomicI64 instead of RwLock<i64> (reads/writes with Ordering::Relaxed would be sufficient for this use case). That’s an optional micro-optimization; the current implementation is functionally sound.
🤖 Prompt for AI Agents
In programs/magicblock/src/task_scheduler_frequency.rs lines 1-22, replace the
lazy_static RwLock<i64> with a process-wide AtomicI64 to avoid locking and
poisoning: import std::sync::atomic::{AtomicI64, Ordering}, declare a static
MIN_TASK_SCHEDULER_FREQUENCY: AtomicI64 =
AtomicI64::new(consts::DEFAULT_TASK_SCHEDULER_MIN_FREQUENCY_MILLIS), then change
min_task_scheduler_frequency() to return
MIN_TASK_SCHEDULER_FREQUENCY.load(Ordering::Relaxed) and change
set_min_task_scheduler_frequency(frequency: i64) to call
MIN_TASK_SCHEDULER_FREQUENCY.store(frequency.max(1), Ordering::Relaxed); remove
lazy_static and RwLock usages and imports.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
programs/magicblock/src/schedule_task/process_schedule_task.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
📚 Learning: 2025-11-12T09:46:27.553Z
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.
Applied to files:
programs/magicblock/src/schedule_task/process_schedule_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:
programs/magicblock/src/schedule_task/process_schedule_task.rs
📚 Learning: 2025-11-20T17:25:23.444Z
Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.
Applied to files:
programs/magicblock/src/schedule_task/process_schedule_task.rs
📚 Learning: 2025-12-03T09:33:48.684Z
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.684Z
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:
programs/magicblock/src/schedule_task/process_schedule_task.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:
programs/magicblock/src/schedule_task/process_schedule_task.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:
programs/magicblock/src/schedule_task/process_schedule_task.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
programs/magicblock/src/schedule_task/process_schedule_task.rs
🧬 Code graph analysis (1)
programs/magicblock/src/schedule_task/process_schedule_task.rs (1)
programs/magicblock/src/task_scheduler_frequency.rs (2)
min_task_scheduler_interval(11-13)set_min_task_scheduler_interval(15-17)
🔇 Additional comments (1)
programs/magicblock/src/schedule_task/process_schedule_task.rs (1)
11-15: Dynamic min-interval validation is correctly wired and cachedImporting
min_task_scheduler_intervaland caching it inmin_intervalbefore both the comparison and the error message gives you a single, consistent snapshot of the configured minimum and avoids extra atomic loads. This keeps the minimum execution interval check in the Magic program, which matches the existing design where schedule parameters are validated on-chain before reaching the scheduler service. Based on learnings, this is aligned with the intended validation flow.Also applies to: 62-71
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
programs/magicblock/src/schedule_task/process_schedule_task.rs (1)
366-367: Test uses dynamic minimum correctly; drop the unnecessary cloneUsing
min_task_scheduler_interval() - 1infail_process_schedule_invalid_execution_intervalkeeps the test robust against future changes to the configured minimum, since it will always construct an invalid interval relative to the current global.
transaction_accounts.clone()is still unnecessary here becausetransaction_accountsisn’t used after this call; you can pass it by value and avoid the extra allocation:- process_instruction( - &ix.data, - transaction_accounts.clone(), - ix.accounts, - Err(InstructionError::InvalidInstructionData), - ); + process_instruction( + &ix.data, + transaction_accounts, + ix.accounts, + Err(InstructionError::InvalidInstructionData), + );Also applies to: 377-377
programs/magicblock/src/task_scheduler_frequency.rs (1)
1-18: Global min‑interval implementation is sound;lazy_staticis now optionalThe
AtomicI64backing store withmin_task_scheduler_interval()/set_min_task_scheduler_interval()achieves the desired behavior: a process-wide, configurable, strictly positive minimum interval initialized fromDEFAULT_TASK_SCHEDULER_MIN_INTERVAL_MILLISand safe to read/write from multiple threads.Given that
AtomicI64::new(...)is const‑initializable, you could now droplazy_staticentirely and use a plainstatic:static MIN_TASK_SCHEDULER_INTERVAL: AtomicI64 = AtomicI64::new(consts::DEFAULT_TASK_SCHEDULER_MIN_INTERVAL_MILLIS as i64);This would slightly simplify the module and remove the
lazy_staticdependency, but it’s purely an optional cleanup.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
test-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
magicblock-config/src/config/scheduler.rs(1 hunks)magicblock-config/src/consts.rs(1 hunks)magicblock-config/src/tests.rs(4 hunks)magicblock-task-scheduler/src/service.rs(2 hunks)programs/magicblock/src/schedule_task/process_schedule_task.rs(4 hunks)programs/magicblock/src/task_scheduler_frequency.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
📚 Learning: 2025-11-12T09:46:27.553Z
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.
Applied to files:
programs/magicblock/src/schedule_task/process_schedule_task.rsmagicblock-config/src/tests.rsmagicblock-config/src/consts.rsprograms/magicblock/src/task_scheduler_frequency.rsmagicblock-task-scheduler/src/service.rsmagicblock-config/src/config/scheduler.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:
programs/magicblock/src/schedule_task/process_schedule_task.rsprograms/magicblock/src/task_scheduler_frequency.rsmagicblock-task-scheduler/src/service.rs
📚 Learning: 2025-11-20T17:25:23.444Z
Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.
Applied to files:
programs/magicblock/src/schedule_task/process_schedule_task.rsmagicblock-config/src/consts.rsprograms/magicblock/src/task_scheduler_frequency.rsmagicblock-task-scheduler/src/service.rs
📚 Learning: 2025-12-03T09:33:48.684Z
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.684Z
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:
programs/magicblock/src/schedule_task/process_schedule_task.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:
programs/magicblock/src/schedule_task/process_schedule_task.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
programs/magicblock/src/schedule_task/process_schedule_task.rsprograms/magicblock/src/task_scheduler_frequency.rs
📚 Learning: 2025-11-21T13:56:03.885Z
Learnt from: snawaz
Repo: magicblock-labs/magicblock-validator PR: 575
File: test-integration/programs/schedulecommit/src/utils/mod.rs:71-71
Timestamp: 2025-11-21T13:56:03.885Z
Learning: In Solana programs (BPF runtime), defensive checked conversions to `usize` (e.g., `try_into()`) are unnecessary when casting from `u64`. Solana exclusively runs in 64-bit environments where `usize` is 64 bits, so truncation is not a concern and simple `as usize` casts are appropriate.
Applied to files:
magicblock-config/src/consts.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
magicblock-task-scheduler/src/service.rs
🧬 Code graph analysis (3)
programs/magicblock/src/schedule_task/process_schedule_task.rs (1)
programs/magicblock/src/task_scheduler_frequency.rs (1)
min_task_scheduler_interval(12-14)
programs/magicblock/src/task_scheduler_frequency.rs (1)
magicblock-task-scheduler/src/service.rs (1)
new(68-102)
magicblock-task-scheduler/src/service.rs (1)
programs/magicblock/src/task_scheduler_frequency.rs (1)
set_min_task_scheduler_interval(16-18)
🔇 Additional comments (5)
magicblock-task-scheduler/src/service.rs (1)
16-18: Wiring config → global min interval looks correctImporting
set_min_task_scheduler_intervaland calling it fromTaskSchedulerService::newwithconfig.min_interval.as_millis()cleanly propagates the configured lower bound into the program-side validation global. Theset_helper’sinterval.max(1)clamp guarantees a sane floor even if the config is mis-set extremely low. Based on learnings, this keeps the Magic program as the single place that enforces schedule parameters while still making the minimum configurable.Also applies to: 76-77
magicblock-config/src/consts.rs (1)
38-39: Shared default for scheduler min interval is well-scopedDefining
DEFAULT_TASK_SCHEDULER_MIN_INTERVAL_MILLIS: u64 = 10here and reusing it in both config defaults and the program/global initializer keeps the minimum interval consistent across layers and avoids hard-coded literals.programs/magicblock/src/schedule_task/process_schedule_task.rs (1)
12-15: Dynamic min-interval validation is correctly wired and cachedSwitching to
min_task_scheduler_interval()and caching it inmin_intervalbefore both the comparison and error message gives you a single source of truth for the lower bound and avoids double-reading the global. This aligns the on-chain validation with the configurable scheduler minimum while keeping the log output consistent with the actual check.Also applies to: 62-71
magicblock-config/src/tests.rs (2)
1-4: Defaultmin_intervalcoverage via example config is solidImporting
Durationand assertingconfig.task_scheduler.min_interval == Duration::from_millis(10)intest_example_config_full_coveragenicely verifies that the example TOML and the default constant stay in sync with the scheduler’s configured minimum.Also applies to: 467-470
524-524: Double‑check humantime parsing of"99"forMIN_INTERVAL_MILLISHere you set:
EnvVarGuard::new("MBV_TASK_SCHEDULER__MIN_INTERVAL_MILLIS", "99"), ... assert_eq!( config.task_scheduler.min_interval, Duration::from_millis(99) );But
TaskSchedulerConfig.min_intervalis deserialized with#[serde(with = "humantime")], and humantime-style parsers usually interpret a bare integer like"99"as seconds rather than milliseconds. If that’s the case, this env var would actually produce ~99 seconds, not 99ms, and the assertion (and the_MILLISsuffix in the env var name) would be misleading.Consider either:
- Supplying a unit in the value, e.g.
"99ms", and keeping the ms expectation, or- Renaming the env var (e.g.
_MIN_INTERVAL) and adjusting the expected duration if you really intend seconds.Please confirm the actual behavior of
humantimein your environment and align the test and docs/config keys accordingly.Also applies to: 578-581
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
config.example.toml(1 hunks)magicblock-config/src/tests.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
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.
📚 Learning: 2025-11-12T09:46:27.553Z
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.
Applied to files:
magicblock-config/src/tests.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
config.example.toml
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
config.example.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:
config.example.toml
📚 Learning: 2025-11-04T13:22:38.811Z
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.
Applied to files:
config.example.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:
config.example.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:
config.example.toml
📚 Learning: 2025-12-03T09:33:48.684Z
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.684Z
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:
config.example.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:
config.example.toml
📚 Learning: 2025-11-19T12:55:48.931Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-tools/src/validator.rs:193-197
Timestamp: 2025-11-19T12:55:48.931Z
Learning: In the magicblock-validator codebase, when constructing arguments for the light CLI test validator in start_light_validator_with_config (test-integration/test-tools/src/validator.rs), the shlex::split approach is necessary because the light CLI does not handle whitespaces properly in validator arguments. The string concatenation + shlex parsing pattern should not be refactored to direct argument construction in this specific case.
Applied to files:
config.example.toml
📚 Learning: 2025-10-21T11:00:18.396Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/encoder.rs:176-187
Timestamp: 2025-10-21T11:00:18.396Z
Learning: In the magicblock validator, the current slot is always the root slot. The SlotEncoder in magicblock-aperture/src/encoder.rs correctly sets `root: slot` because there is no lag between current and root slots in this architecture.
Applied to files:
config.example.toml
🧬 Code graph analysis (1)
magicblock-config/src/tests.rs (1)
magicblock-task-scheduler/src/service.rs (1)
new(68-102)
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (51)
.github/workflows/ci-lint.yml(1 hunks).github/workflows/ci-test-integration.yml(1 hunks).github/workflows/ci-test-unit.yml(1 hunks).github/workflows/deploy-testnet-by-pr-comment.yml(1 hunks)magicblock-account-cloner/src/lib.rs(1 hunks)magicblock-accounts-db/src/index.rs(1 hunks)magicblock-accounts-db/src/lib.rs(2 hunks)magicblock-accounts/src/errors.rs(1 hunks)magicblock-api/src/errors.rs(3 hunks)magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs(1 hunks)magicblock-chainlink/src/remote_account_provider/errors.rs(2 hunks)magicblock-committor-program/src/lib.rs(1 hunks)magicblock-committor-program/src/state/changeset.rs(1 hunks)magicblock-committor-program/src/state/chunks.rs(1 hunks)magicblock-committor-service/src/compute_budget.rs(1 hunks)magicblock-committor-service/src/error.rs(1 hunks)magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(1 hunks)magicblock-committor-service/src/intent_executor/error.rs(2 hunks)magicblock-committor-service/src/intent_executor/mod.rs(1 hunks)magicblock-committor-service/src/intent_executor/task_info_fetcher.rs(3 hunks)magicblock-committor-service/src/persist/db.rs(4 hunks)magicblock-committor-service/src/service_ext.rs(1 hunks)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(4 hunks)magicblock-committor-service/src/transaction_preparator/error.rs(1 hunks)magicblock-config/src/lib.rs(2 hunks)magicblock-ledger/src/blockstore_processor/mod.rs(1 hunks)magicblock-ledger/src/database/db.rs(1 hunks)magicblock-ledger/src/database/rocks_db.rs(5 hunks)magicblock-ledger/src/store/api.rs(5 hunks)magicblock-magic-program-api/src/instruction.rs(2 hunks)magicblock-rpc-client/src/lib.rs(8 hunks)magicblock-rpc-client/src/utils.rs(4 hunks)magicblock-table-mania/src/lookup_table_rc.rs(1 hunks)magicblock-task-scheduler/src/errors.rs(2 hunks)magicblock-task-scheduler/src/service.rs(5 hunks)magicblock-validator-admin/src/claim_fees.rs(2 hunks)programs/guinea/src/lib.rs(1 hunks)programs/magicblock/src/schedule_task/process_schedule_task.rs(0 hunks)rust-toolchain.toml(1 hunks)test-integration/programs/flexi-counter/src/processor.rs(1 hunks)test-integration/programs/schedulecommit/src/utils/mod.rs(2 hunks)test-integration/rust-toolchain.toml(1 hunks)test-integration/schedulecommit/client/src/schedule_commit_context.rs(1 hunks)test-integration/schedulecommit/test-scenarios/tests/01_commits.rs(1 hunks)test-integration/test-committor-service/tests/test_ix_commit_local.rs(1 hunks)test-integration/test-runner/src/env_config.rs(1 hunks)test-integration/test-tools/src/integration_test_context.rs(4 hunks)test-integration/test-tools/src/transactions.rs(1 hunks)test-kit/src/lib.rs(1 hunks)tools/ledger-stats/src/accounts.rs(1 hunks)tools/ledger-stats/src/transaction_details.rs(1 hunks)
💤 Files with no reviewable changes (1)
- programs/magicblock/src/schedule_task/process_schedule_task.rs
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
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.
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.
Applied to files:
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.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-chainlink/src/chainlink/account_still_undelegating_on_chain.rsmagicblock-committor-program/src/state/changeset.rstest-integration/test-committor-service/tests/test_ix_commit_local.rsmagicblock-committor-service/src/intent_executor/mod.rstest-integration/programs/flexi-counter/src/processor.rsprograms/guinea/src/lib.rsmagicblock-committor-service/src/error.rsmagicblock-rpc-client/src/utils.rstest-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-validator-admin/src/claim_fees.rsmagicblock-config/src/lib.rstools/ledger-stats/src/accounts.rsmagicblock-chainlink/src/remote_account_provider/errors.rstools/ledger-stats/src/transaction_details.rsmagicblock-account-cloner/src/lib.rsmagicblock-rpc-client/src/lib.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rsmagicblock-api/src/errors.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rsmagicblock-accounts-db/src/lib.rsmagicblock-accounts-db/src/index.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-chainlink/src/chainlink/account_still_undelegating_on_chain.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/programs/flexi-counter/src/processor.rsprograms/guinea/src/lib.rsmagicblock-committor-service/src/error.rstest-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-validator-admin/src/claim_fees.rsmagicblock-chainlink/src/remote_account_provider/errors.rsmagicblock-account-cloner/src/lib.rsmagicblock-rpc-client/src/lib.rsmagicblock-api/src/errors.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:
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/programs/schedulecommit/src/utils/mod.rsmagicblock-chainlink/src/remote_account_provider/errors.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-chainlink/src/chainlink/account_still_undelegating_on_chain.rsmagicblock-task-scheduler/src/errors.rsmagicblock-chainlink/src/remote_account_provider/errors.rsmagicblock-rpc-client/src/lib.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-program/src/state/changeset.rstest-integration/test-committor-service/tests/test_ix_commit_local.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rsmagicblock-committor-service/src/intent_executor/mod.rstest-integration/programs/flexi-counter/src/processor.rsmagicblock-committor-service/src/error.rsmagicblock-validator-admin/src/claim_fees.rsmagicblock-committor-service/src/persist/db.rsmagicblock-account-cloner/src/lib.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.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-program/src/state/changeset.rsmagicblock-account-cloner/src/lib.rsmagicblock-committor-service/src/compute_budget.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.rstest-integration/schedulecommit/test-scenarios/tests/01_commits.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-tools/src/integration_test_context.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/schedulecommit/test-scenarios/tests/01_commits.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs
📚 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/mod.rsmagicblock-committor-service/src/service_ext.rsmagicblock-accounts/src/errors.rsmagicblock-committor-service/src/intent_executor/error.rsmagicblock-committor-service/src/transaction_preparator/delivery_preparator.rsmagicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rs
📚 Learning: 2025-12-01T16:02:05.367Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 703
File: magicblock-chainlink/src/submux/mod.rs:652-654
Timestamp: 2025-12-01T16:02:05.367Z
Learning: In magicblock-chainlink/src/submux/mod.rs, the subscribe_program method intentionally adds program_id to program_subs before attempting the subscription. This ensures that even if the initial subscription fails or only partially succeeds across clients, the reconnection logic will retry the subscription. This is a deliberate design pattern for resilience in the multi-client architecture and should not be "fixed" to remove entries on failure.
Applied to files:
magicblock-chainlink/src/remote_account_provider/errors.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-account-cloner/src/lib.rs
📚 Learning: 2025-11-12T09:46:27.553Z
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.
Applied to files:
magicblock-account-cloner/src/lib.rsmagicblock-task-scheduler/src/service.rsmagicblock-committor-service/src/intent_executor/task_info_fetcher.rs
📚 Learning: 2025-11-20T17:25:23.444Z
Learnt from: JMirval
Repo: magicblock-labs/magicblock-validator PR: 656
File: programs/guinea/src/lib.rs:33-33
Timestamp: 2025-11-20T17:25:23.444Z
Learning: In the magicblock-validator codebase, task IDs (task_id) are i64 values that can be negative. The task scheduler is designed to handle any i64 value including negative task IDs. Task IDs are randomly generated using rand::random() which produces values across the full i64 range. No validation is needed to restrict task IDs to positive values.
Applied to files:
magicblock-account-cloner/src/lib.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:
.github/workflows/ci-test-integration.yml
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-ledger/src/blockstore_processor/mod.rs
📚 Learning: 2025-11-21T13:56:03.885Z
Learnt from: snawaz
Repo: magicblock-labs/magicblock-validator PR: 575
File: test-integration/programs/schedulecommit/src/utils/mod.rs:71-71
Timestamp: 2025-11-21T13:56:03.885Z
Learning: In Solana programs (BPF runtime), defensive checked conversions to `usize` (e.g., `try_into()`) are unnecessary when casting from `u64`. Solana exclusively runs in 64-bit environments where `usize` is 64 bits, so truncation is not a concern and simple `as usize` casts are appropriate.
Applied to files:
magicblock-committor-service/src/compute_budget.rs
🧬 Code graph analysis (16)
magicblock-committor-program/src/state/changeset.rs (2)
magicblock-committor-service/src/stubs/changeset_committor_stub.rs (1)
len(39-41)magicblock-committor-program/src/state/chunks.rs (1)
chunk_size(130-132)
magicblock-committor-service/src/intent_executor/mod.rs (1)
test-integration/test-tools/src/conversions.rs (1)
err(8-8)
magicblock-committor-service/src/service_ext.rs (2)
magicblock-accounts/src/errors.rs (1)
from(68-70)magicblock-api/src/errors.rs (6)
from(104-106)from(110-112)from(116-118)from(122-124)from(130-134)from(138-140)
magicblock-committor-service/src/error.rs (1)
magicblock-rpc-client/src/lib.rs (1)
error(201-203)
magicblock-rpc-client/src/utils.rs (1)
test-integration/test-tools/src/conversions.rs (1)
err(8-8)
magicblock-committor-service/src/intent_executor/error.rs (3)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
from(279-281)new(52-59)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (4)
from(513-515)from(548-552)from(574-576)new(49-59)magicblock-rpc-client/src/lib.rs (3)
from(82-84)from(236-238)new(243-245)
magicblock-ledger/src/database/rocks_db.rs (1)
magicblock-ledger/src/database/db.rs (1)
raw_iterator_cf(105-107)
tools/ledger-stats/src/accounts.rs (1)
magicblock-committor-program/src/state/changeset.rs (1)
owner(119-124)
magicblock-task-scheduler/src/errors.rs (2)
magicblock-accounts/src/errors.rs (1)
from(68-70)magicblock-chainlink/src/remote_account_provider/errors.rs (1)
from(95-97)
magicblock-ledger/src/database/db.rs (1)
magicblock-ledger/src/database/rocks_db.rs (2)
raw_iterator_cf(209-211)batch(213-215)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (2)
magicblock-committor-service/src/intent_executor/error.rs (5)
from(29-31)from(152-154)from(276-281)value(121-133)value(248-256)magicblock-rpc-client/src/lib.rs (4)
from(82-84)from(236-238)new(243-245)error(201-203)
magicblock-task-scheduler/src/service.rs (1)
magicblock-magic-program-api/src/args.rs (1)
id(163-168)
magicblock-ledger/src/blockstore_processor/mod.rs (2)
magicblock-accounts-db/src/lib.rs (1)
slot(234-236)magicblock-ledger/src/database/columns.rs (8)
slot(131-131)slot(181-183)slot(249-251)slot(329-331)slot(404-406)slot(503-505)slot(577-579)slot(649-651)
magicblock-api/src/errors.rs (4)
magicblock-accounts/src/errors.rs (1)
from(68-70)magicblock-chainlink/src/remote_account_provider/errors.rs (1)
from(95-97)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)magicblock-task-scheduler/src/errors.rs (1)
from(52-56)
magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
magicblock-rpc-client/src/lib.rs (3)
new(243-245)from(82-84)from(236-238)magicblock-committor-service/src/intent_executor/error.rs (3)
from(29-31)from(152-154)from(276-281)
magicblock-accounts-db/src/lib.rs (2)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
slot(220-232)magicblock-ledger/src/database/columns.rs (8)
slot(131-131)slot(181-183)slot(249-251)slot(329-331)slot(404-406)slot(503-505)slot(577-579)slot(649-651)
🔇 Additional comments (63)
.github/workflows/deploy-testnet-by-pr-comment.yml (1)
162-162: No substantive changes detected.This file only contains a whitespace/formatting adjustment at the end of the file and is unrelated to the PR's core objective (configurable minimum frequency). No review needed.
magicblock-magic-program-api/src/instruction.rs (1)
28-29: Documentation alignment looks good.The indentation adjustments to the doc comments for
ScheduleCommitandScheduleCommitAndUndelegateimprove readability by aligning continuation text. No functional changes, no API surface modifications.Also applies to: 45-46
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)
17-18: LGTM!Trivial doc comment reflow for line length—no functional change.
magicblock-table-mania/src/lookup_table_rc.rs (1)
173-179: Display refactor from entry iteration tokeys()is correct and clearerUsing
.keys().map(|key| key.to_string())under the existing read lock is semantically equivalent to iterating(key, _)over the entries and simplifies the intent. No behavioral or concurrency changes, and the resultingcomma_separated_pubkeysstring remains the same.magicblock-committor-program/src/state/changeset.rs (1)
372-372: Idiomatic unsigned integer check, but verify PR scope alignment.The condition change from
> 0to!= 0is semantically equivalent for unsigned integers and aligns with Rust idioms. The existing test at line 449 validates correctness (expects 15 chunks for 547 bytes split into 39-byte chunks, which is correct: 547 / 39 = 14 remainder 10, so 14 + 1 = 15).However, this file change appears orthogonal to the PR objectives stated in #717 (configurable minimum crank frequency for task scheduling). The change is cosmetic and unrelated to task scheduling, error handling refactoring, or configuration changes mentioned in the AI summary.
Please clarify whether this style improvement was intentionally included in PR #717 or is mistakenly part of this PR. If it belongs, consider explicitly calling it out in the PR description. If it's incidental, consider separating it into its own style-focused PR to keep changes focused.
test-integration/test-tools/src/transactions.rs (1)
1-1: Lint suppression for test code is appropriate.Adding
#![allow(clippy::result_large_err)]at the crate level is reasonable for test infrastructure that uses Solana's RPC client libraries, whereclient_error::Erroris large by design. No functional concerns.test-integration/schedulecommit/test-scenarios/tests/01_commits.rs (1)
1-1: Lint suppression for test code is appropriate.Adding
#![allow(clippy::result_large_err)]at the crate level aligns with the test infrastructure updates and is reasonable for test code that necessarily uses Solana RPC libraries. No functional concerns.test-integration/test-tools/src/integration_test_context.rs (2)
1-1: Crate-level lint suppression is appropriate.Adding
#![allow(clippy::result_large_err)]at line 1 applies to the entire crate and is suitable for test infrastructure that necessarily uses Solana's RPC client libraries.
672-721: Verify intent of per-function lint suppressions.Lines 672, 689, and 706 add per-function
#[allow(clippy::result_large_err)]suppressions in addition to the crate-level suppression at line 1. This dual approach appears redundant—the crate-level attribute already covers all functions returningResult<_, client_error::Error>in this file. Additionally, other functions in the file (e.g.,send_transactionat line 856,send_instructions_with_payerat line 877,send_and_confirm_transactionat line 888) also return the same large error type but lack per-function suppressions.Clarify whether the per-function attributes are intentional or if they should be removed in favor of the crate-level suppression for consistency.
test-kit/src/lib.rs (1)
294-294: LGTM! Explicit lifetime improves consistency.The explicit lifetime parameter correctly reflects the borrow of
self.accountsdband aligns with the signature ofget_account(Line 282). This makes the API more consistent and clear.Note: This change appears unrelated to the PR's stated objective of making minimum crank frequency configurable, suggesting it's part of a broader codebase-wide refactoring to explicit lifetime annotations.
test-integration/schedulecommit/client/src/schedule_commit_context.rs (1)
286-286: LGTM! Explicit lifetime annotation aligns with Rust upgrade.The explicit lifetime annotation
'_correctly ties the returned struct's lifetime toself, which is appropriate since all fields inScheduleCommitTestContextFieldsare borrowed references. This change is likely required by the Rust toolchain upgrade to 1.91.1 and aligns with the broader pattern of adding explicit lifetime annotations across the codebase.magicblock-accounts-db/src/index.rs (1)
360-365: LGTM!Adding the explicit lifetime annotation
'_toAccountOffsetFinderclarifies the lifetime relationship between the returned finder andself. This aligns with the similar change toAccountsScanner<'_, F>inlib.rsand improves API clarity without changing runtime behavior.magicblock-accounts-db/src/lib.rs (2)
185-205: LGTM!The explicit lifetime annotation
'_in the return type accurately reflects that the returnedAccountsScannerborrows fromself(viastorageanditerator). This improves API clarity and is consistent with the similar change tooffset_finderinindex.rs.
246-274: LGTM!The
is_multiple_ofmethod improves readability over the manual modulo comparison. The zero-frequency edge case is already guarded by the assertion at line 66 (assert_ne!(snapshot_frequency, 0, ...)), so this change is safe.magicblock-committor-service/src/compute_budget.rs (1)
55-64: Saturating add correctly clamps total budget instead of wrappingSwitching to
saturating_add(self.base_budget)here avoids a potential wraparound when the per‑byte component is already nearu32::MAX, and keeps the behavior consistent withComputeBudget::total_budget(which also clamps atu32::MAX). The overflow story for this path now looks self‑consistent and safe.If you want to double‑check behavior, it’s worth adding/expanding a unit test that drives
bytes_counthigh enough that the per‑byte product exceedsu32::MAXand asserts thattotal_budgetreturnsu32::MAXrather than wrapping.test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
711-711: LGTM! More idiomatic divisibility check.Replacing the modulo operation with
is_multiple_ofimproves readability while maintaining identical behavior. This method is available since the Rust toolchain has been upgraded to 1.91.1.tools/ledger-stats/src/accounts.rs (1)
126-130: Use ofOption::is_none_oris semantically correct; ensure toolchain/MSRV is updated accordinglyThe new guard
if !owner.is_none_or(|owner| acc.owner.eq(&owner)) { return false; }correctly implements “filter by owner when provided, otherwise pass everything” and is equivalent to the previous
map_or(true, ...)pattern. Since this relies onOption::is_none_or(stabilized in recent Rust), just confirm the crate’s MSRV / CI toolchain is aligned (the PR summary mentions Rust 1.91.1, which is sufficient).magicblock-committor-program/src/state/chunks.rs (1)
187-187: LGTM: Explicit lifetime annotation improves clarity.Making the lifetime parameter explicit in the return type is good practice, especially for developers less familiar with lifetime elision rules. The change is correct and aligns with making public API types more explicit.
magicblock-ledger/src/blockstore_processor/mod.rs (1)
58-60: Verify thatis_multiple_ofis in scope foru64and preserves prior semantics
slotis explicitly declared asu64, and in standard Rustu64does not have an inherentis_multiple_ofmethod. Unless there is an in-scope extension trait (or a differentSlotnewtype used elsewhere that you meant to keep here), this call will not resolve and the module will fail to compile. Also, ifis_multiple_ofhas any special handling for0, this could subtly change when the first progress log fires compared to the previousslot % PROGRESS_REPORT_INTERVAL == 0check.Please confirm:
- which trait or type provides
is_multiple_offor thisu64slot, and- that its behavior is exactly equivalent to the old modulo check for all relevant values (especially at
slot == 0and around boundary multiples ofPROGRESS_REPORT_INTERVAL).If there is no shared extension trait in use here, consider reverting to the explicit modulo expression for this file or importing the same trait/prelude used in the other
is_multiple_ofcall sites to keep behavior consistent.magicblock-config/src/lib.rs (1)
70-70: The API change forValidatorParams::try_newis properly implemented across all callers.The return type change from
figment::Result<Self>toResult<Self, Box<figment::error::Error>>has been accommodated correctly:
- magicblock-validator/src/main.rs:85: Callers already handle errors through pattern matching (
matchonOk/Err), which works with any error type.- magicblock-config/src/tests.rs:26: Test caller uses
.expect(), which is compatible withBox<Error>since it implementsDisplay.No breaking issues exist. However, clarify whether this error-boxing pattern is intentional and part of a broader refactoring, or if it's specific to this PR's scope (issue #713 on configurable minimum frequency vs. issue #718).
programs/guinea/src/lib.rs (1)
163-163: LGTM! Efficient slice construction without cloning.The change from
&[payer_info.clone()]tostd::slice::from_ref(payer_info)eliminates an unnecessary clone and allocation while achieving the same result. This is more idiomatic and performant for single-element slices in CPI invocations.test-integration/programs/schedulecommit/src/utils/mod.rs (1)
89-89: LGTM! Consistent elimination of unnecessary clones.Both changes replace
&[account_info.clone()]withstd::slice::from_ref(account_info), avoiding unnecessary clones and allocations for single-element slices. This improves performance while maintaining identical behavior in the CPI invocations.Also applies to: 98-98
test-integration/programs/flexi-counter/src/processor.rs (1)
492-492: LGTM! Efficient single-element slice construction.The change from
&[payer_info.clone()]tostd::slice::from_ref(payer_info)eliminates an unnecessary clone and allocation. This is consistent with the refactoring pattern applied across the codebase and improves performance without changing behavior.magicblock-ledger/src/database/db.rs (2)
105-107: LGTM! Correct lifetime annotation for iterator.The lifetime annotation on
DBRawIterator<'_>correctly indicates that the iterator borrows from the database, ensuring it cannot outlive theDatabaseinstance. This aligns with the underlying RocksDB API requirements.
109-117: LGTM! Correct lifetime annotation for write batch.The lifetime annotation on
WriteBatch<'_>is necessary because the returned batch holds references to column families fromself.backend(line 113). This ensures the batch cannot outlive the database instance.magicblock-ledger/src/database/rocks_db.rs (5)
83-90: LGTM! Correct lifetime annotation for pinnable slice.The lifetime annotation on
DBPinnableSlice<'_>correctly indicates that the pinnable slice borrows directly from the database's underlying storage. This zero-copy optimization requires the lifetime constraint to prevent use-after-free.
102-117: LGTM! Consistent lifetime annotation for batched retrieval.The lifetime annotation on
DBPinnableSlice<'_>within the returned vector is consistent withget_pinned_cfand correctly ensures all returned slices borrow from the database.
172-190: LGTM! Correct lifetime annotation for typed iterator.The lifetime annotation on
DBIterator<'_>correctly ensures the iterator cannot outlive the database. The method logic remains unchanged—only the type signature is updated for lifetime safety.
192-207: LGTM! Consistent lifetime annotation for raw-key iterator.The lifetime annotation matches the pattern established in
iterator_cf, maintaining consistency across iterator methods.
209-211: LGTM! Correct lifetime annotation for raw iterator.The lifetime annotation on
DBRawIterator<'_>is consistent with other iterator methods and correctly matches the signature expected by the higher-levelDatabase::raw_iterator_cfwrapper in db.rs.test-integration/test-runner/src/env_config.rs (1)
52-57: Refactoring tois_none_oris safe and toolchain-compatibleThe switch from
.map_or(true, |setup| setup.devnet()/ephem())to.is_none_or(|setup| setup.devnet()/ephem())preserves semantics (None →true, Some → predicate result) and improves clarity. The workspace is pinned to Rust 1.91.1 via rust-toolchain.toml, which fully supportsOption::is_none_or(stabilized in 1.82.0). No toolchain compatibility concerns.test-integration/rust-toolchain.toml (1)
1-2: Aligns test-integration toolchain with root.This new file ensures test-integration uses the same Rust toolchain (1.91.1) as the root configuration, maintaining consistency across the workspace.
.github/workflows/ci-lint.yml (1)
26-26: Consistent with root toolchain update.The linting workflow now uses Rust 1.91.1, aligning with the root toolchain change. Lint tools typically have minimal compatibility concerns.
.github/workflows/ci-test-unit.yml (1)
26-26: Consistent with root toolchain update.Unit tests now run on Rust 1.91.1, ensuring the codebase is validated against the new toolchain version. This is essential for catching any compatibility issues early.
.github/workflows/ci-test-integration.yml (1)
1-83: The core implementation files already exist in the repository. The scheduler configuration withmin_intervalis present inmagicblock-config/src/config/scheduler.rsand actively used inmagicblock-task-scheduler/src/service.rs. Error types inmagicblock-task-scheduler/src/errors.rsalready include explicit boxing (e.g.,Box<solana_pubsub_client::nonblocking::pubsub_client::PubsubClientError>). These implementation files are not missing from the PR; they are established parts of the codebase. If changes to these files are intended, verify they appear in the actual PR diff.Likely an incorrect or invalid review comment.
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
364-364: LGTM!Removing the unused
HashMapimport from the test module is a clean-up that aligns with the broader refactoring in this PR.magicblock-account-cloner/src/lib.rs (1)
109-142: LGTM! Guard removal aligns with validation delegation.The
> 0guard was removed as validation is now handled by the task scheduler service, which treats zero intervals as no-ops and clamps other values to[min_interval, u32::MAX]. This aligns with the retrieved learning that task parameter validation is performed at the program/scheduler level rather than at the cloning layer.magicblock-task-scheduler/src/service.rs (3)
53-54: LGTM! Newmin_intervalfield for configurable task scheduling.The addition of
min_intervalenables the configurable minimum crank frequency as per PR objectives.
294-298: LGTM! Task ID captured before ownership transfer.Capturing
idbeforeprocess_request(task)consumes ownership ensures the ID is available for logging in recoverable error cases.
134-146: Theas_millis() as i64cast is safe in practice. Themin_intervalconfiguration is deserialized viahumantimeand defaults to 10ms with test values around 100ms, reflecting its intended use as a scheduler minimum interval. While theoretically au128toi64cast could truncate for durations exceeding ~292 million years, this is not a realistic concern given humantime's validation and the practical constraints of this use case.magicblock-rpc-client/src/utils.rs (2)
114-124: LGTM! Correct dereferencing of boxed errors.The
*errdereferencing correctly unwraps the boxedMagicBlockRpcClientErrorbefore passing tomap_magicblock_client_error, which expects an owned value.
143-174: LGTM! Return type updated to boxed error.The signature change to return
Box<solana_rpc_client_api::client_error::Error>aligns with the broader error boxing refactor across the codebase. The boxing is applied consistently to both theTransactionErrorfallback path and other error kinds.magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (5)
346-352: LGTM! Correct dereferencing of boxed error for mapping.The
*errcorrectly unwraps the boxedMagicBlockRpcClientErrorbefore passing tomap_magicblock_client_error.
509-516: LGTM! Consistent boxing pattern forMagicBlockRpcClientError.The explicit
Fromimplementation that boxes the error aligns with the codebase-wide refactor to reduce stack size of error enums. This pattern is consistent with similar implementations inmagicblock-rpc-client/src/lib.rsandmagicblock-committor-service/src/intent_executor/error.rs.
547-553: LGTM!Fromimpl forBufferExecutionErrorcorrectly boxes the error.The implementation correctly chains through
TransactionSendError::MagicBlockRpcClientErrorwith proper boxing.
566-577: LGTM!InternalErrorboxing is consistent with the refactor.The boxed variant and
Fromimplementation follow the same pattern as the other error types in this file and across the codebase.
358-365: The pattern match and function call work correctly. After destructuringTransactionSendError::MagicBlockRpcClientError(err), where the variant containsBox<MagicBlockRpcClientError>, the extractederris&Box<MagicBlockRpcClientError>. Rust's Deref coercion automatically dereferences this to&MagicBlockRpcClientErrorwhen passed todecide_rpc_error_flow, which expects that type. This is standard idiomatic Rust and compiles without issues.magicblock-committor-service/src/transaction_preparator/error.rs (1)
16-22: BoxedDeliveryPreparatorErrorand explicitFromkeep conversions intactThe boxing of
DeliveryPreparatorErrorand the manualFrom<DeliveryPreparatorError>correctly preserve?-based conversions while shrinking the enum payload and keepingsignature()behavior unchanged. Looks good.magicblock-committor-service/src/intent_executor/mod.rs (1)
687-692: Dereferencing boxed RPC error in mapper is correctPassing
*errhere correctly unwraps the boxedMagicBlockRpcClientErrorto the type expected bymap_magicblock_client_error, and aligns with the new boxed representation inInternalError. No issues spotted.magicblock-committor-service/src/intent_executor/error.rs (1)
21-32: BoxingMagicBlockRpcClientErrorinInternalErroris consistent and safeThe new boxed
MagicBlockRpcClientErrorvariant plus theFrom<MagicBlockRpcClientError>impl preserve existing?flows and keepTransactionStrategyExecutionError::frombehavior intact, whilesignature()still correctly delegates to the inner error. Looks consistent with the rest of the PR.Also applies to: 151-154
magicblock-validator-admin/src/claim_fees.rs (1)
81-84: Error mapping to boxedMagicBlockRpcClientErrorvariants looks goodWrapping
get_latest_blockhashandsend_and_confirm_transactionfailures into the more specific boxedMagicBlockRpcClientErrorvariants cleanly annotates the source operation without changing the control flow ofclaim_fees. All good.Also applies to: 93-97
magicblock-accounts/src/errors.rs (1)
59-71: BoxedCommittorServiceErrorvariant andFromimpl are correctThe
ScheduledCommitsProcessorError::CommittorSerivceError(Box<...>)plus the explicitFrom<CommittorServiceError>maintain previous ergonomics for?while standardizing on boxed errors as elsewhere in the repo. No issues found.magicblock-committor-service/src/service_ext.rs (1)
221-235: BoxedCommittorServiceErrorinCommittorServiceExtErrorpreservesawait??flowStoring
CommittorServiceErrorin aBoxand providing the explicitFromkeepsschedule_base_intents_waiting’sawait??working unchanged while aligning with the project-wide boxing pattern. Implementation looks solid.magicblock-task-scheduler/src/errors.rs (1)
11-16:TaskSchedulerError::Pubsubboxing and manualFromare appropriateThe change to
Pubsub(Box<PubsubClientError>)with a correspondingFromimpl maintains conversion ergonomics while standardizing on boxed errors. The transparent variant remains suitable for forwarding display/source. No concerns.Also applies to: 49-56
magicblock-ledger/src/store/api.rs (1)
207-233: ExplicitRwLockReadGuardlifetimes and doc clarifications look correctAdding the
'_, Slotlifetime toRwLockReadGuardreturn types incheck_lowest_cleanup_slotandensure_lowest_cleanup_slotis type-correct and preserves the existing locking discipline at call sites. The expanded comments forget_confirmed_signatures_for_address,write_transaction, anditer_transaction_statusesaccurately describe current behavior without altering logic.Also applies to: 421-431, 855-856, 1048-1050
magicblock-chainlink/src/remote_account_provider/errors.rs (1)
10-12: LGTM!The boxing of
PubsubClientErrorwith an explicitFromimplementation is a clean pattern that:
- Reduces the enum's stack size by heap-allocating the large inner error
- Maintains ergonomic error propagation via the
?operatorThis is consistent with the broader error handling refactor across the codebase.
Also applies to: 91-98
magicblock-committor-service/src/error.rs (1)
42-50: LGTM!The boxed variants for
FailedToSendInitChangesetAccountandFailedToConfirmInitChangesetAccountdon't have a blanketFromimplementation, which is appropriate here. These are specific error cases where explicit construction withBox::new(...)provides clearer intent at call sites, distinguishing them from generic RPC errors.magicblock-committor-service/src/intent_executor/task_info_fetcher.rs (2)
16-16: Good improvement: replacingunsafewith safe constunwrap().The change from
unsafe { NonZeroUsize::new_unchecked(...) }toNonZeroUsize::new(...).unwrap()eliminates unsafe blocks while maintaining compile-time evaluation. This works becauseOption::unwrapbecame const-stable in Rust 1.83, and the toolchain is now 1.91.1.Also applies to: 53-53
268-282: LGTM!The
MagicBlockRpcClientErrorboxing with explicitFromimplementation follows the established pattern. Thesignature()method at line 289 correctly accesses the boxed error viaerr.signature().magicblock-rpc-client/src/lib.rs (2)
50-85: LGTM!The error enum refactoring is well-structured:
- Four variants (
RpcClientError,GetLatestBlockhash,GetSlot,SendTransaction) are now boxed- The single
Fromimplementation maps generic RPC errors toRpcClientError, while operation-specific errors (blockhash, slot, send) are constructed explicitly with their semantic context preservedThis maintains clear error categorization while reducing enum stack size.
247-260: LGTM!All RPC method error paths correctly box errors:
- Operation-specific methods (
get_latest_blockhash,get_slot,send_transaction) construct their respective semantic error variants- Generic error paths use
.into()(line 318) or explicitRpcClientError(Box::new(e))constructionThe pattern is consistent and maintains clear error semantics throughout.
Also applies to: 280-280, 318-318, 352-361, 398-409, 574-588
magicblock-api/src/errors.rs (2)
15-25: LGTM!The boxed error variants for cross-crate errors (
AccountsError,AccountClonerError,LedgerError,ChainlinkError,CommittorServiceError,TaskSchedulerServiceError) follow a consistent pattern that:
- Reduces
ApiErrorenum size by heap-allocating large nested error types- Maintains ergonomic error propagation from dependent crates
Also applies to: 33-36, 92-95
102-141: LGTM!All six
Fromimplementations follow a consistent, correct pattern. The mapping ofTaskSchedulerErrortoTaskSchedulerServiceError(lines 137-140) preserves the existing API naming convention while enabling seamless error conversion.
| num-derive = { workspace = true } | ||
| num-traits = { workspace = true } | ||
| serde = { workspace = true, features = ["derive"] } | ||
| magicblock-config = { workspace = true } |
taco-paco
left a comment
taco-paco
left a comment
There was a problem hiding this comment.
LGTM! Don't like clamping though, should return an error IMO opinion to the user if he used unsupported value
Fix #713
This PR introduces a static variable used in the Magic program to make the minimum frequency for scheduled tasks configurable.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.