feat: cranked commits #656
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. |
📝 WalkthroughWalkthroughIntroduces AccountCloneRequest and updates the Cloner API to accept it; account-cloner now builds modify_accounts plus optional schedule/commit instructions and returns a fallible signed Transaction. Migrates task/timing identifiers from u64 to i64 across API, instruction, scheduler DB, service, and tests. Adds signer error propagation and allows validator authority as an accepted signer in commit processing. Updates many call sites and test helpers to the new signatures and adds an integration test exercising scheduled commits. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 (3)
🧰 Additional context used🧠 Learnings (13)📚 Learning: 2025-10-21T14:00:54.642ZApplied to files:
📚 Learning: 2025-11-12T09:46:27.553ZApplied to files:
📚 Learning: 2025-11-21T11:03:26.732ZApplied to files:
📚 Learning: 2025-11-20T08:57:07.217ZApplied to files:
📚 Learning: 2025-10-28T13:15:42.706ZApplied to files:
📚 Learning: 2025-10-21T13:06:38.900ZApplied to files:
📚 Learning: 2025-10-26T16:54:39.084ZApplied to files:
📚 Learning: 2025-11-04T13:22:38.811ZApplied to files:
📚 Learning: 2025-11-19T12:55:48.931ZApplied to files:
📚 Learning: 2025-11-19T09:34:37.917ZApplied to files:
📚 Learning: 2025-11-07T14:20:31.457ZApplied to files:
📚 Learning: 2025-11-19T11:31:24.218ZApplied to files:
📚 Learning: 2025-11-20T17:25:23.420ZApplied to files:
🧬 Code graph analysis (2)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (1)
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-task-scheduler/src/db.rs (1)
131-148: Consider adding bounds check when decrementing executions_left.The SQL UPDATE on line 140 decrements
executions_leftwithout verifying it's positive. If a task is somehow executed whenexecutions_leftis already 0, this would result in negative values persisting in the database. While validation occurs in the Magic program (as per learnings), defensive checks at the database layer can prevent data integrity issues from edge cases or concurrency.Consider adding a WHERE clause to ensure the decrement only occurs when executions_left > 0:
self.conn.execute( "UPDATE tasks SET executions_left = executions_left - 1, last_execution_millis = ?, updated_at = ? - WHERE id = ?", + WHERE id = ? AND executions_left > 0", params![last_execution, now, task_id], )?;Based on learnings.
magicblock-task-scheduler/src/service.rs (2)
191-213: Castingexecution_interval_millistou64relies on upstream validation
Duration::from_millis(task.execution_interval_millis as u64)assumesexecution_interval_millis >= 0. Given that the Magic program validates schedule parameters (iterations, minimum interval, etc.) before they reach the scheduler, this is fine, but quite implicit. Consider a debug assertion or a saturating clamp if you want extra robustness against corrupted DB state.Based on learnings
105-121: Bug confirmed: negative delay wraps to hugeDurationon task scheduler startupOn startup (line 116), when
last_execution_millis = 0(initial state) andnowis current system time (~1.7 billion milliseconds),next_execution.saturating_sub(now)yields a large negativei64. Casting tou64wraps it to an enormous positive value, causing tasks to be delayed for years instead of running immediately.Since
execution_interval_millisis validated on-chain to be positive (per program constraints), the issue manifests reliably whenever the scheduler starts with tasks that haven't been executed yet.- let timeout = Duration::from_millis( - next_execution.saturating_sub(now) as u64, - ); + let delay_ms = if next_execution <= now { + 0 + } else { + (next_execution - now) as u64 + }; + let timeout = Duration::from_millis(delay_ms);This ensures overdue tasks run immediately on startup.
📜 Review details
Configuration used: CodeRabbit UI
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 (29)
magicblock-account-cloner/Cargo.toml(1 hunks)magicblock-account-cloner/src/lib.rs(4 hunks)magicblock-chainlink/Cargo.toml(2 hunks)magicblock-chainlink/src/chainlink/fetch_cloner.rs(10 hunks)magicblock-chainlink/src/cloner/errors.rs(1 hunks)magicblock-chainlink/src/cloner/mod.rs(2 hunks)magicblock-chainlink/src/testing/cloner_stub.rs(2 hunks)magicblock-magic-program-api/src/args.rs(2 hunks)magicblock-magic-program-api/src/instruction.rs(1 hunks)magicblock-task-scheduler/src/db.rs(9 hunks)magicblock-task-scheduler/src/errors.rs(2 hunks)magicblock-task-scheduler/src/service.rs(6 hunks)magicblock-task-scheduler/tests/service.rs(3 hunks)programs/guinea/src/lib.rs(2 hunks)programs/magicblock/src/schedule_task/process_cancel_task.rs(1 hunks)programs/magicblock/src/schedule_task/process_schedule_task.rs(3 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(4 hunks)programs/magicblock/src/utils/instruction_utils.rs(3 hunks)test-integration/programs/flexi-counter/src/instruction.rs(5 hunks)test-integration/test-chainlink/src/ixtest_context.rs(2 hunks)test-integration/test-task-scheduler/src/lib.rs(3 hunks)test-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rs(3 hunks)test-integration/test-task-scheduler/tests/test_reschedule_task.rs(3 hunks)test-integration/test-task-scheduler/tests/test_schedule_error.rs(2 hunks)test-integration/test-task-scheduler/tests/test_schedule_task.rs(3 hunks)test-integration/test-task-scheduler/tests/test_schedule_task_signed.rs(1 hunks)test-integration/test-task-scheduler/tests/test_scheduled_commits.rs(1 hunks)test-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rs(2 hunks)test-integration/test-tools/src/transactions.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common 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
📚 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/cloner/mod.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rstest-integration/test-chainlink/src/ixtest_context.rsmagicblock-chainlink/src/testing/cloner_stub.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rsmagicblock-chainlink/Cargo.tomlmagicblock-account-cloner/src/lib.rs
📚 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:
magicblock-account-cloner/Cargo.toml
📚 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-task-scheduler/src/errors.rsmagicblock-task-scheduler/tests/service.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsprograms/magicblock/src/schedule_task/process_cancel_task.rsmagicblock-magic-program-api/src/instruction.rstest-integration/test-task-scheduler/tests/test_scheduled_commits.rsmagicblock-magic-program-api/src/args.rstest-integration/test-task-scheduler/tests/test_schedule_error.rstest-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rstest-integration/test-task-scheduler/tests/test_schedule_task_signed.rsprograms/magicblock/src/schedule_task/process_schedule_task.rstest-integration/test-task-scheduler/tests/test_reschedule_task.rstest-integration/programs/flexi-counter/src/instruction.rsmagicblock-task-scheduler/src/service.rsmagicblock-task-scheduler/src/db.rstest-integration/test-task-scheduler/tests/test_schedule_task.rsprograms/magicblock/src/utils/instruction_utils.rstest-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rsmagicblock-account-cloner/src/lib.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:
magicblock-task-scheduler/tests/service.rstest-integration/test-tools/src/transactions.rstest-integration/test-task-scheduler/tests/test_scheduled_commits.rstest-integration/test-task-scheduler/tests/test_schedule_error.rstest-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rstest-integration/test-task-scheduler/tests/test_reschedule_task.rstest-integration/test-task-scheduler/tests/test_schedule_task.rstest-integration/test-task-scheduler/tests/test_cancel_ongoing_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-task-scheduler/tests/service.rsmagicblock-chainlink/src/chainlink/fetch_cloner.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-tools/src/transactions.rstest-integration/test-chainlink/src/ixtest_context.rsmagicblock-chainlink/src/testing/cloner_stub.rstest-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rsmagicblock-account-cloner/src/lib.rs
📚 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:
test-integration/test-chainlink/src/ixtest_context.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-task-scheduler/tests/test_scheduled_commits.rstest-integration/test-task-scheduler/tests/test_schedule_error.rstest-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rstest-integration/test-task-scheduler/tests/test_schedule_task_signed.rstest-integration/test-task-scheduler/tests/test_reschedule_task.rstest-integration/programs/flexi-counter/src/instruction.rstest-integration/test-task-scheduler/src/lib.rstest-integration/test-task-scheduler/tests/test_schedule_task.rstest-integration/test-task-scheduler/tests/test_cancel_ongoing_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:
test-integration/test-task-scheduler/tests/test_schedule_error.rsprograms/magicblock/src/schedule_task/process_schedule_task.rstest-integration/programs/flexi-counter/src/instruction.rstest-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rsmagicblock-account-cloner/src/lib.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:
test-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rstest-integration/test-task-scheduler/tests/test_reschedule_task.rsmagicblock-task-scheduler/src/service.rs
🧬 Code graph analysis (12)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
programs/magicblock/src/validator.rs (1)
validator_authority_id(35-42)
test-integration/test-task-scheduler/tests/test_scheduled_commits.rs (4)
test-integration/programs/flexi-counter/src/instruction.rs (1)
create_schedule_task_ix(424-450)test-integration/test-task-scheduler/src/lib.rs (3)
create_delegated_counter(81-130)send_noop_tx(132-163)setup_validator(32-79)test-integration/programs/flexi-counter/src/state.rs (1)
pda(32-35)test-integration/test-tools/src/integration_test_context.rs (4)
airdrop_chain(484-495)send_transaction_ephem_with_preflight(718-732)get_transaction_ephem(922-931)try_ephem_client(265-270)
test-integration/test-task-scheduler/tests/test_schedule_error.rs (2)
test-integration/test-task-scheduler/src/lib.rs (1)
create_delegated_counter(81-130)test-integration/programs/flexi-counter/src/instruction.rs (1)
create_cancel_task_ix(452-463)
test-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rs (1)
test-integration/test-task-scheduler/src/lib.rs (1)
create_delegated_counter(81-130)
test-integration/test-task-scheduler/tests/test_schedule_task_signed.rs (1)
test-integration/test-task-scheduler/src/lib.rs (1)
create_delegated_counter(81-130)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (1)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (5)
account(188-210)account_shared_data(129-135)remote_slot(153-159)owner(54-60)owner(251-253)
test-integration/test-task-scheduler/tests/test_reschedule_task.rs (2)
test-integration/test-task-scheduler/src/lib.rs (1)
create_delegated_counter(81-130)test-integration/programs/flexi-counter/src/instruction.rs (1)
create_cancel_task_ix(452-463)
test-integration/programs/flexi-counter/src/instruction.rs (2)
magicblock-magic-program-api/src/args.rs (1)
id(142-147)test-integration/programs/flexi-counter/src/state.rs (1)
pda(32-35)
test-integration/test-task-scheduler/src/lib.rs (1)
test-integration/programs/flexi-counter/src/instruction.rs (2)
create_delegate_ix_with_commit_frequency_ms(278-308)create_init_ix(189-202)
test-integration/test-task-scheduler/tests/test_schedule_task.rs (2)
test-integration/test-task-scheduler/src/lib.rs (1)
create_delegated_counter(81-130)test-integration/programs/flexi-counter/src/instruction.rs (1)
create_cancel_task_ix(452-463)
test-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rs (2)
test-integration/test-task-scheduler/src/lib.rs (1)
create_delegated_counter(81-130)test-integration/programs/flexi-counter/src/instruction.rs (1)
create_cancel_task_ix(452-463)
magicblock-account-cloner/src/lib.rs (2)
programs/magicblock/src/validator.rs (1)
validator_authority_id(35-42)programs/magicblock/src/utils/instruction_utils.rs (2)
modify_accounts_instruction(162-195)schedule_task_instruction(211-226)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: Build Project
🔇 Additional comments (33)
magicblock-chainlink/Cargo.toml (2)
17-17: No formatting issues found — formatting is consistent.The
magicblock-delegation-programdependency on line 17 maintains consistent formatting and indentation with all other workspace dependencies in the section. All entries follow the identical pattern:<name> = { workspace = true }with consistent spacing and no indentation variations.
30-30: solana-signer dependency is correctly used for error handling.The dependency is actively imported and used in
magicblock-chainlink/src/cloner/errors.rswhereSignerErrorfromsolana_signeris incorporated into the error handling for the cloner module. This supports the transaction signing operations needed for the cranked commits feature.magicblock-account-cloner/Cargo.toml (1)
23-24: Dependencies are properly defined and actively used—no changes needed.Verification confirms all dependencies are correctly configured:
- Workspace definitions: Both
rand = "0.8.5"andsolana-instruction = { version = "2.2" }are defined in the workspace rootCargo.toml- Active usage:
rand::random()generates task IDs (line 113 in lib.rs)solana_instruction::Instructionconstructs Solana instructions (line 115+)- No redundancy:
solana-instructionandsolana-sdkserve complementary purposes—instruction construction vs. account metadata and SDK types respectivelyThe additions follow the correct workspace dependency pattern and introduce no unused or conflicting dependencies.
magicblock-task-scheduler/src/db.rs (3)
10-26: LGTM: i64 migration aligns with SQLite INTEGER type.The documentation clearly explains the rationale. Since SQLite's INTEGER type is a signed 64-bit value, using i64 is the correct choice for compatibility. The maximum positive value (2^63-1) remains more than sufficient for timestamp and frequency use cases.
28-39: Code is correct - all field types verified.The
ScheduleTaskRequeststruct (magicblock-magic-program-api/src/args.rs:120-131) usesi64for all three fields being accessed:id,execution_interval_millis, anditerations. The From implementation in db.rs correctly maps these fields without type issues. Per the learning note, task parameter validation is already performed in the Magic program before requests reach the scheduler, so no additional validation is needed here.
275-284: No issues found—all callers are compatible with the i64 return type change.The
get_task_idsfunction is only used in test files, and all callers perform generic operations (.len()checks and iteration) that are compatible with bothVec<u64>andVec<i64>. No production code depends on this function, and the return type change is safe.programs/guinea/src/lib.rs (1)
31-31: Verify that negative task IDs are properly validated by the magic program before reaching the scheduler.The type change from
u64toi64is consistent across all layers (guinea program, magicblock processor, and scheduler service) and aligns with SQLite compatibility requirements. However, I could not find validation logic that ensurestask_id >= 0in the code path from guinea → magicblock → scheduler. Theprocess_cancel_taskfunction acceptstask_id: i64and passes it directly toCancelTaskRequestwithout validation.Per the PR description and learnings provided, validation should occur in the magic program (on-chain), but this could not be confirmed in the code. Please verify that validation is enforced at the appropriate layer to prevent negative task IDs from causing issues in database lookups or scheduling logic.
programs/magicblock/src/schedule_task/process_schedule_task.rs (1)
16-16: MIN_EXECUTION_INTERVAL type change keeps validation consistent with i64 fieldsUpdating
MIN_EXECUTION_INTERVALtoi64keeps it in sync withexecution_interval_millis(which must also bei64for this comparison to compile) and with the broader move of scheduler-related fields toi64. The existing interval check logic remains correct and doesn’t need further adjustment here.test-integration/test-tools/src/transactions.rs (1)
50-66: Switch totry_signchanges error propagation; confirm error mapping is correctUsing
tx.try_sign(signers, blockhash)?;is a good move away from panicking, but it changes the failure mode: signer errors will now bubble up asclient_error::Error. Please confirm thatSignerErroris cleanly mapped intoclient_error::Error(and that callers expect signing to be fallible here).programs/magicblock/src/utils/instruction_utils.rs (2)
211-225: Non-signer scheduled task accounts look correctMarking scheduled-task accounts as read-only and non-signers (
AccountMeta::new_readonly(*account, false)) aligns with the design where only the payer/validator signs, and the scheduled instructions themselves run under the validator’s authority. This looks consistent with the updated authorization model.
231-251:i64task IDs in cancel helpers are consistent with the new APIChanging
task_idtoi64in bothcancel_taskandcancel_task_instructionmatches the updatedCancelTaskRequest/scheduler types and keeps the instruction payload consistent end-to-end. Just ensure all on-chain handlers and DB schemas have been migrated to the same signed type.magicblock-task-scheduler/src/service.rs (1)
245-250:i64task IDs in queue keys and DB helpers are consistentMigrating
task_queue_keystoHashMap<i64, Key>,unregister_task(&self, task_id: i64), andremove_task_from_queue(&mut self, task_id: i64)keeps the in-memory queue and DB accessors in sync with the new signed task ID type. The logic for removing from the queue remains unchanged; just ensure all call sites (including DB schema) have been updated toi64.Also applies to: 287-291, 48-53
magicblock-magic-program-api/src/args.rs (1)
105-147:i64task fields align API with scheduler/DB; ensure invariants are documentedUpdating task-related fields (
task_id,id,execution_interval_millis,iterations, andCancelTaskRequest::task_id) toi64brings the program API in line with the scheduler and DB (and avoids unsigned issues with SQLite). Since these were previouslyu64, please ensure the on-chain logic still enforces the intended invariants (e.g., positive intervals/iterations) and that callers are aware they must not send negative values.Based on learnings
magicblock-account-cloner/src/lib.rs (2)
365-386:clone_accountintegration withAccountCloneRequestis consistentSwitching
clone_accountto take anAccountCloneRequest, delegate totransaction_to_clone_regular_account, and then callmaybe_prepare_lookup_tableswhenrequest.account.delegated()keeps the previous behavior while supporting the new cranked-commit path. The error mapping toClonerError::FailedToCloneRegularAccount(request.pubkey, ..)is appropriate.
89-148: Account ordering is not an issue; task_id negative values possible but by design (temporary client-side generation)Upon verification:
Account ordering (NOT A PROBLEM)
The on-chainprocess_schedule_taskhandler validates all instruction accounts at line ~106:if !ix_accounts.contains(&account.pubkey). The ScheduleCommit instruction requires[validator_authority, MAGIC_CONTEXT_PUBKEY, request.pubkey]—all three are present in the accounts passed toschedule_task_instruction. The validation ensures account consistency automatically.Random
task_idrange (NOTED)
rand::random()does generate negative i64 values, andprocess_schedule_taskdoes not constrain task IDs to positive values. However, this is intentional and temporary: the TODO at line 102–104 indicates task ID generation will move to the program instead of the client (issue #625). The current design accepts any i64 and stores it in the database, which works but is semantically loose.SignerError mapping (CORRECT)
ClonerErrorhas a proper#[from] SignerErrorimplementation, so the?operator inclone_accountcorrectly convertsResult<Transaction, SignerError>toClonerResult<Signature>.magicblock-task-scheduler/tests/service.rs (1)
148-165: Timeout math still correct; i64→u64 casts are safe in this contextUsing
interval as u64in the timeout and post-cancel sleep preserves the previous behavior (10× and 2× the interval) while aligning with the i64 change onexecution_interval_millis. Givenintervalis positive in this test and validated at the program level in real flows, there’s no practical risk of negative-to-u64 surprises here.Also applies to: 197-197
magicblock-chainlink/src/cloner/errors.rs (1)
7-15: Good addition ofSignerErrorto the cloner error surfaceCapturing
solana_signerfailures via a dedicated transparentSignerErrorvariant is consistent with the rest ofClonerErrorand will simplify propagating signing issues from new cloner flows. No concerns.programs/magicblock/src/schedule_task/process_cancel_task.rs (1)
11-15:process_cancel_taski64 migration is consistent and safeUsing
i64fortask_idhere aligns with the updated instruction args and scheduler/DB types. The function only forwards the ID intoCancelTaskRequestand the TLS stash, and the existing tests (signed vs. unsigned authority) continue to exercise the path with small positive IDs, so behavior is preserved.Also applies to: 35-47, 68-86, 88-111
test-integration/test-task-scheduler/tests/test_schedule_task_signed.rs (1)
25-25: Delegated-counter helper call aligns with new signaturePassing
0ascommit_frequency_mstocreate_delegated_counterkeeps this test narrowly focused on the multi‑signer scheduling error case while matching the updated helper signature. Since this test doesn’t rely on periodic commits, that choice is appropriate.magicblock-magic-program-api/src/instruction.rs (1)
81-88: CancelTask.task_id i64 usage is consistently aligned across the workspaceVerification confirms full type consistency: all 27+ task_id references across the codebase—including CancelTaskRequest, database layer, schedulers, processors, and instruction handlers—use
i64. No remainingu64patterns exist. The change is complete and safe end-to-end.magicblock-task-scheduler/src/errors.rs (1)
23-25: Migration to i64 task IDs is complete and correctly implementedVerification confirms all error variants and callers have been properly updated:
ScheduleTaskRequest.idisi64(magicblock-magic-program-api/src/args.rs:122)DbTask.idisi64(magicblock-task-scheduler/src/db.rs:15)- Error variants
TaskNotFound(i64)andUnauthorizedReplacing(i64, String, String)match- The only error construction site (service.rs:229) correctly passes
task.idasi64No remaining u64 usages or inconsistencies found.
test-integration/test-chainlink/src/ixtest_context.rs (1)
9-9: AccountCloneRequest usage for native programs looks correctSwitching to
cloner::{AccountCloneRequest, Cloner}and constructingAccountCloneRequest { pubkey, account: program_stub.clone(), commit_frequency_ms: None }when seeding native programs into the bank is consistent with the new cloner API. UsingNoneforcommit_frequency_msis appropriate here since these static loader entries are not meant to be scheduled for cranked commits.Also applies to: 103-107
test-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rs (2)
33-34: Confirm semantics forcommit_frequency_ms = 0in delegated counter setupThe updated
create_delegated_counterhelper now requires acommit_frequency_ms: u32, and this test passes0for both payers. Depending on how the FlexiCounter/Magic program validates this field,0might either be a valid sentinel (no cranked commits) or be rejected as an invalid (non‑positive) frequency.Please confirm that
0is intentionally supported here; if it is a sentinel, consider documenting that in the helper or using a named constant to make the intent clear.
160-160: Castingiterationstou64is the right type fixComparing
counter.count(u64) toiterations as u64resolves the integer type mismatch in the assertion. Giveniterationsis a small positive literal (2) and, per existing program-level validation, is expected to be > 0 before scheduling, this cast is safe in this context. Based on learnings.test-integration/test-task-scheduler/src/lib.rs (1)
18-20: create_delegated_counter now cleanly wires commit_frequency_ms into delegationImporting
create_delegate_ix_with_commit_frequency_ms, extendingcreate_delegated_counterwith acommit_frequency_ms: u32parameter, and calling the new instruction helper in the delegation transaction aligns this test harness with the updated flexi-counter instruction API. This keeps the helper generic so individual tests can choose the frequency they need while reusing the same setup flow.Also applies to: 81-86, 115-118
test-integration/test-task-scheduler/tests/test_schedule_error.rs (1)
30-30: API alignment for delegation and cancel-task helpers looks correct
- Passing
0ascommit_frequency_msintocreate_delegated_counteris consistent with other tests and keeps this scenario focused on scheduler error handling rather than cranked commits.- Updated
create_cancel_task_ix(payer.pubkey(), task_id)usage matches the new instruction signature and preserves the rest of the cancellation flow.Also applies to: 126-127
test-integration/test-task-scheduler/tests/test_reschedule_task.rs (1)
29-29: Reschedule test updates are type-safe and consistent with new APIs
create_delegated_counter(&ctx, &payer, &mut validator, 0)matches the updated helper signature and the “no cranked commits” setup pattern.- Casting in
counter.count == 2 * iterations as u64correctly reconciles the u64 counter with theiterationsscalar without changing the logic.create_cancel_task_ix(payer.pubkey(), task_id)matches the simplified cancel-instruction signature.Also applies to: 155-156, 165-165
test-integration/test-task-scheduler/tests/test_schedule_task.rs (1)
29-29: Schedule-task test correctly tracks new types and helper signatures
create_delegated_counternow takescommit_frequency_ms; passing0is consistent with other tests that don’t exercise cranked commits.counter.count == iterations as u64is the right adjustment given the counter’s u64 type.create_cancel_task_ix(payer.pubkey(), task_id)matches the new cancel-instruction API.Also applies to: 121-122, 131-131
test-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rs (1)
29-29: Ongoing-task cancellation test updates are consistent and type-correct
- Delegation setup with
commit_frequency_ms = 0keeps this scenario focused on manual scheduling/cancel behavior.- The inequality
counter.count < iterations as u64properly handles the u64 counter type and still asserts that the task was interrupted before all iterations completed.create_cancel_task_ix(payer.pubkey(), task_id)matches the new instruction signature.Also applies to: 78-79, 147-148
magicblock-chainlink/src/chainlink/fetch_cloner.rs (1)
1396-1403: Airdrop path correctly uses AccountCloneRequest with no commit scheduleUsing
AccountCloneRequest { pubkey, account, commit_frequency_ms: None }inairdrop_account_if_emptyis a good choice: it reuses the same cloner pipeline while making it explicit that these synthetic system accounts should not get cranked commits configured via this API.test-integration/programs/flexi-counter/src/instruction.rs (3)
424-450: LGTM: API simplified by using constant MAGIC_PROGRAM_ID.The removal of the
magic_programparameter in favor of the constantMAGIC_PROGRAM_IDsimplifies the API and is appropriate when the program ID is fixed. The type changes toi64are consistent with the struct definitions updated earlier in this file.Also applies to: 452-463
22-24: Validation is properly enforced on-chain for task parameters.The concern regarding negative values is addressed: the Magic program validates that
execution_interval_millisis at leastMIN_EXECUTION_INTERVALand thatiterationsis at least 1, which rejects negative or invalid values. Thetask_idis randomly generated client-side and functions as an identifier; negative values do not affect correctness. The type changes toi64are compatible with SQLite while maintaining proper validation semantics on-chain.
274-281: I need to check how the instruction parameter flows from the test code to the account cloner to confirm type compatibility:Let me search the codebase directly for the delegation record and instruction structures:Based on my analysis of the script output and web research, I can now provide the rewritten review comment. The key findings are:
- The account cloner checks
if commit_frequency_ms > 0to determine whether to schedule commits- When
commit_frequency_ms = 0, commits are not scheduled (which is the default in the new function)- The
AccountCloneRequeststruct hasOption<u64>, meaningNonealso disables commits- The test default of 0 properly disables automatic commits
The review comment's verification request is fundamentally correct - 0 does disable automatic commits as intended. The refactoring properly enables the "cranked commits" feature while maintaining backward compatibility with a sensible default.
Verify that commit_frequency_ms = 0 correctly disables automatic commits in the delegation program.
The new
create_delegate_ix_with_commit_frequency_msfunction properly parameterizes the commit frequency, allowing tests to control this behavior. The original function now defaults to0. Based on the account cloner implementation, whencommit_frequency_ms > 0, commits are scheduled; otherwise, commits remain disabled. Please confirm this default behavior aligns with your testing requirements and that the delegation program handles the zero value correctly across all contexts where this instruction is used.Also applies to: 300-300
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
programs/magicblock/src/schedule_task/process_schedule_task.rs (1)
73-80: iterations ≥ 1 guard correctly enforces scheduler contract; verify zero-iterations coverageThis on-chain check cleanly rejects non‑positive
iterationswithInvalidInstructionData, matching the design where the Magic program is responsible for ensuring strictly positive iterations before they reach the scheduler/DB layer. Based on learningsTo be fully locked in, ensure there’s also test coverage for
iterations == 0somewhere in the suite (this test covers the negative case).#!/bin/bash # Verify there is at least one test that exercises iterations == 0 rg -n "iterations: 0" .magicblock-chainlink/src/testing/cloner_stub.rs (1)
14-15: Stub now cleanly uses AccountCloneRequest and explicitly documents ignoring commit_frequency_msThe cfg-gated import and the updated
clone_accountsignature/body correctly adaptClonerStubtoAccountCloneRequest, still just insertingrequest.pubkey/request.accountintoaccounts_bankand returning a defaultSignature. The new comment clarifying thatcommit_frequency_msis intentionally ignored by this test stub makes the limitation explicit; no further changes needed here.Also applies to: 73-81
test-integration/test-task-scheduler/tests/test_scheduled_commits.rs (1)
68-124: Make the polling condition less brittle by checking thresholds instead of exact intermediate valuesThe current loop requires observing exact counts
[1, 2]on both chain and ephem. If the first poll happens after a few scheduler ticks (e.g., both counters already at>= 3), the test will fail even though scheduled commits and cranked commits are behaving correctly.You can simplify and harden the test by breaking once both counters reach at least a small threshold (e.g., 2) and dropping the
chain_values/ephem_valuesvectors:- // Check that the counter value is properly incremented on mainnet - const MAX_TRIES: u32 = 30; - let mut tries = 0; - let mut chain_values = vec![1, 2]; - let mut ephem_values = vec![1, 2]; - loop { + // Check that the counter value is properly incremented on mainnet + const MAX_TRIES: u32 = 30; + let mut tries = 0; + loop { let chain_counter_account = expect!( ctx.try_chain_client().and_then(|client| client .get_account(&counter_pda) .map_err(|e| anyhow::anyhow!("Failed to get account: {}", e))), validator ); let chain_counter = expect!( FlexiCounter::try_decode(&chain_counter_account.data), validator ); let ephem_counter_account = expect!( ctx.try_ephem_client().and_then(|client| client .get_account(&counter_pda) .map_err(|e| anyhow::anyhow!("Failed to get account: {}", e))), validator ); let ephem_counter = expect!( FlexiCounter::try_decode(&ephem_counter_account.data), validator ); - let chain_value_index = - chain_values.iter().position(|x| x == &chain_counter.count); - if let Some(index) = chain_value_index { - chain_values.remove(index); - } - - let ephem_value_index = - ephem_values.iter().position(|x| x == &ephem_counter.count); - if let Some(index) = ephem_value_index { - ephem_values.remove(index); - } - - if chain_values.is_empty() && ephem_values.is_empty() { - break; - } + if chain_counter.count >= 2 && ephem_counter.count >= 2 { + break; + } tries += 1; if tries > MAX_TRIES { assert!( false, cleanup(&mut validator), - "Missed some values: ephem_values: {:?}, chain_values: {:?}", - ephem_values, - chain_values + "Counters did not reach at least 2: ephem: {}, chain: {}", + ephem_counter.count, + chain_counter.count, ); } std::thread::sleep(std::time::Duration::from_millis(50)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
magicblock-chainlink/src/chainlink/fetch_cloner.rs(10 hunks)magicblock-chainlink/src/testing/cloner_stub.rs(2 hunks)programs/guinea/src/lib.rs(2 hunks)programs/magicblock/src/schedule_task/process_schedule_task.rs(4 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(4 hunks)test-integration/test-task-scheduler/tests/test_scheduled_commits.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 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/testing/cloner_stub.rsprograms/magicblock/src/schedule_task/process_schedule_task.rsmagicblock-chainlink/src/chainlink/fetch_cloner.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/testing/cloner_stub.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsprograms/magicblock/src/schedule_task/process_schedule_task.rsmagicblock-chainlink/src/chainlink/fetch_cloner.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/testing/cloner_stub.rsprograms/magicblock/src/schedule_task/process_schedule_task.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 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/testing/cloner_stub.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-task-scheduler/tests/test_scheduled_commits.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:
test-integration/test-task-scheduler/tests/test_scheduled_commits.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsprograms/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:
test-integration/test-task-scheduler/tests/test_scheduled_commits.rsprograms/magicblock/src/schedule_task/process_schedule_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:
programs/magicblock/src/schedule_transactions/process_schedule_commit.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-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:
programs/magicblock/src/schedule_task/process_schedule_task.rs
📚 Learning: 2025-11-19T11:31:24.218Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs:85-114
Timestamp: 2025-11-19T11:31:24.218Z
Learning: In the magicblock-validator codebase, compressed delegation records are never subscribed to by design. Tests for compressed delegation flows should focus on verifying that the delegated account (e.g., counter PDA) is cloned as delegated and not subscribed, but do not need to check for subscription to delegation-record-like PDAs in the compressed path because subscriptions to compressed delegation records never occur.
Applied to files:
magicblock-chainlink/src/chainlink/fetch_cloner.rs
🧬 Code graph analysis (5)
magicblock-chainlink/src/testing/cloner_stub.rs (2)
magicblock-chainlink/src/cloner/mod.rs (1)
clone_account(24-27)magicblock-account-cloner/src/lib.rs (1)
clone_account(367-386)
test-integration/test-task-scheduler/tests/test_scheduled_commits.rs (4)
test-integration/programs/flexi-counter/src/instruction.rs (1)
create_schedule_task_ix(424-450)test-integration/test-task-scheduler/src/lib.rs (3)
create_delegated_counter(81-130)send_noop_tx(132-163)setup_validator(32-79)test-integration/programs/flexi-counter/src/state.rs (1)
pda(32-35)test-integration/test-tools/src/integration_test_context.rs (4)
airdrop_chain(484-495)send_transaction_ephem_with_preflight(718-732)get_transaction_ephem(922-931)try_ephem_client(265-270)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
programs/magicblock/src/validator.rs (1)
validator_authority_id(35-42)
programs/magicblock/src/schedule_task/process_schedule_task.rs (3)
programs/magicblock/src/validator.rs (1)
validator_authority_id(35-42)programs/magicblock/src/utils/instruction_utils.rs (1)
schedule_task_instruction(211-226)programs/magicblock/src/test_utils/mod.rs (1)
process_instruction(44-61)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (1)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (5)
account(188-210)account_shared_data(129-135)remote_slot(153-159)owner(54-60)owner(251-253)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (9)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
26-27: Good: Validator authority caching addresses past review feedbackThe optimization to cache
validator_authority_id()at line 131 and reuse it at line 163 correctly addresses the previous review's concern about repeatedly acquiring the globalVALIDATOR_AUTHORITYread lock inside the loop. This improves performance and clarifies intent.Also applies to: 131-131
125-127: Validator authority signer logic is correct and matches PR designThe validation logic correctly allows commits when the validator authority has signed the transaction (line 163), extending the authorization model to support cranked commits as intended. Key observations:
- The three-way check (program ownership OR account signer OR validator signer) properly implements the documented trust model
- Accounts must still be delegated (lines 151-158), limiting the validator authority's scope to delegated accounts
- The validator must actually sign the transaction to commit; this prevents unauthorized commits
- Error message at line 176 accurately reflects all three valid authorization paths
The security assumption that the validator authority is trusted to commit accounts directly is documented in the PR objectives and implemented as designed.
Also applies to: 160-182
programs/guinea/src/lib.rs (1)
142-166: LGTM! Function correctly updated to match enum variant.The function signature change is consistent with the
GuineaInstruction::CancelTaskenum variant update. The implementation correctly passes thetask_idthrough to the magic program without local validation, which aligns with the PR's stated approach of delegating validation to the magic program.programs/magicblock/src/schedule_task/process_schedule_task.rs (3)
16-16: MIN_EXECUTION_INTERVAL type now matches argument typeSwitching
MIN_EXECUTION_INTERVALtoi64keeps the comparison withargs.execution_interval_millistype-safe and consistent with the wider i64 migration; semantics remain unchanged (10 ms floor).
94-120: HashSet-based ix_accounts and validator-only signer enforcement look solidCollecting
ix_accountsinto aHashSet<Pubkey>plus cachingvalidator_authority_id()before the loops keeps the signer and presence checks efficient while preserving behavior. The addedic_msg!diagnostics for non-validator signers and missing accounts are clear and do not change the returnedInstructionErrors.
383-403: Negative-iterations test nicely exercises the new validation path
test_process_schedule_task_with_invalid_iterationsdrives theiterations < 1branch with a negative value and assertsInvalidInstructionData, which is exactly what we want for malformed schedule requests. This complements the new guard and helps prevent regressions.magicblock-chainlink/src/chainlink/fetch_cloner.rs (2)
27-27: AccountCloneRequest migration and clone fan-out look consistentImporting
AccountCloneRequestand updating all cloning call sites to constructAccountCloneRequest { pubkey, account, commit_frequency_ms: None }(or a computed value) preserves prior behavior while threading through the new metadata. Logging, dedup (acc_subs/delegated_acc_subsusingrequest.pubkeyandrequest.account.delegated()), and theJoinSetfan-out that spawnscloner.clone_account(request)tasks all remain coherent, so plain, delegated, refetched, and airdrop flows should behave as before apart from the new commit-scheduling data.Also applies to: 247-252, 637-642, 759-763, 663-666, 1019-1021, 1027-1031, 1055-1065, 1400-1407
848-905: commit_frequency_ms now correctly gated on accounts delegated to this validatorDeriving
commit_frequency_msfrom the delegation record only whenis_delegated_to_usand otherwise usingNone—while also settingaccount.set_delegated(is_delegated_to_us)—ensures that only accounts actually delegated to this validator can carry a non-Nonecommit frequency into the cloner. This avoids the earlier risk of scheduling cranked commits for accounts delegated elsewhere, while still cloning accounts lacking a delegation record as plain (non-delegated) entries and surfacing them viamissing_delegation_record.test-integration/test-task-scheduler/tests/test_scheduled_commits.rs (1)
14-67: End‑to‑end scheduled commit flow is wired correctlyThe setup (ephemeral validator, chain airdrop, noop tx to clone programs, delegated counter with
commit_frequency_ms, andcreate_schedule_task_ixsent viasend_transaction_ephem_with_preflight) is consistent with the scheduler/delegation APIs and exercises the cranked‑commit path as intended. Using small positiveexecution_interval_millis/iterationsvalues matches the on‑chain validation guarantees of the Magic program, so no extra validation is needed in this test. Based on learnings.
Fix #655
Assumptions:
Note:
i64. Types that should be positive (frequencies, iterations) are checked in the magic program so this does not introduce safety issues.u32is the SDK,u64in the delegation record, andi64in the database. The delegation program should check that the value does not overflow if usingu64as that would cause errors when scheduling cranked commitsSummary by CodeRabbit
New Features
Improvements
Changes
✏️ Tip: You can customize this high-level summary in your review settings.