fix: double undelegation #676
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. |
📝 WalkthroughWalkthroughNarrowed Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-integration/programs/schedulecommit/src/lib.rs (1)
1-21: Minor doc and ergonomics clean‑ups around the new twice‑undelegate pathTwo small nits that could reduce future confusion:
- The account‑reference doc comment for
ScheduleCommitAndUndelegateCpiTwiceappears copied from the...ModAfter/undelegate callback path and doesn’t match how this instruction is actually invoked (payer, magic context, MagicBlock program, then committee PDAs). Consider updating the comment to mirrorScheduleCommitCpi’s documented layout.use std::ops::Deref;is only needed to call.deref()on a temporary Vec inprocess_schedulecommit_and_undelegation_cpi_twice. You could simplify and drop that import by storing the collected refs in a local and passing a slice:- let ix = create_schedule_commit_ix( - payer, - remaining.iter().collect::<Vec<_>>().deref(), - magic_context, - magic_program, - ); + let remaining_refs: Vec<&AccountInfo> = remaining.iter().collect(); + let ix = create_schedule_commit_ix( + payer, + &remaining_refs, + magic_context, + magic_program, + );Both changes are optional but would make the code and docs clearer.
Also applies to: 98-109
📜 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 (9)
magicblock-accounts/src/scheduled_commits_processor.rs(3 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(1 hunks)programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(3 hunks)programs/magicblock/src/utils/account_actions.rs(1 hunks)test-integration/programs/schedulecommit/Cargo.toml(1 hunks)test-integration/programs/schedulecommit/src/api.rs(1 hunks)test-integration/programs/schedulecommit/src/lib.rs(6 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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:
programs/magicblock/src/utils/account_actions.rsprograms/magicblock/src/schedule_transactions/process_schedule_base_intent.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsprograms/magicblock/src/magic_scheduled_base_intent.rsmagicblock-accounts/src/scheduled_commits_processor.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_transactions/process_schedule_base_intent.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.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:
programs/magicblock/src/magic_scheduled_base_intent.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:
programs/magicblock/src/magic_scheduled_base_intent.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-accounts/src/scheduled_commits_processor.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:
test-integration/programs/schedulecommit/Cargo.toml
🧬 Code graph analysis (7)
test-integration/programs/schedulecommit/src/api.rs (2)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (10)
committees(63-66)committees(67-67)committees(74-77)committees(78-78)committees(124-127)committees(128-128)committees(135-138)committees(139-139)committees(183-186)committees(187-187)test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
committees(153-163)committees(165-169)
programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs (1)
programs/magicblock/src/utils/account_actions.rs (1)
perform_account_undelegation(18-23)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
programs/magicblock/src/utils/account_actions.rs (1)
perform_account_undelegation(18-23)programs/magicblock/src/utils/accounts.rs (1)
get_writable_with_idx(104-111)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (4)
test-integration/programs/schedulecommit/src/api.rs (1)
schedule_commit_and_undelegate_cpi_twice(223-247)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-29)test-integration/test-tools/src/integration_test_context.rs (2)
assert_ephemeral_transaction_error(640-648)fetch_ephemeral_logs(144-146)test-integration/test-tools/src/scheduled_commits.rs (1)
extract_scheduled_commit_sent_signature_from_logs(16-28)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
create_schedule_commit_ix(41-66)test-integration/programs/schedulecommit/src/api.rs (2)
pda_and_bump(282-286)pda_seeds_with_bump(268-273)
programs/magicblock/src/magic_scheduled_base_intent.rs (2)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
delegated(95-101)programs/magicblock/src/utils/accounts.rs (2)
get_instruction_account_with_idx(65-73)get_instruction_pubkey_with_idx(94-102)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
programs/magicblock/src/magic_scheduled_base_intent.rs (2)
is_undelegate(101-103)is_undelegate(144-150)
🔇 Additional comments (13)
programs/magicblock/src/utils/account_actions.rs (1)
18-23: Core fix for double undelegation - LGTM!The renamed function now explicitly sets
delegated=falsein addition toundelegating=true. This prevents the same account from being scheduled for undelegation twice, as subsequent attempts will fail theacc.delegated()check in the validation logic (lines 221, 141 in other files).The explicit
borrow_mut()on line 20 is necessary to maintain the mutable reference across both state updates.programs/magicblock/src/magic_scheduled_base_intent.rs (1)
214-233: Validation strengthened to prevent double undelegation - LGTM!The validation now requires accounts to be both writable AND delegated (line 221) before allowing undelegation. This is consistent with the validation in
process_schedule_commit.rs(lines 136-148) and works in conjunction withperform_account_undelegationsettingdelegated=falseto prevent the same account from being undelegated twice.The flow is:
- First undelegation attempt: passes validation (writable=true, delegated=true)
perform_account_undelegationsets delegated=false immediately- Second undelegation attempt: fails validation (delegated=false)
The error message correctly reflects both requirements (line 227).
programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs (2)
17-17: Import updated for renamed function - LGTM!
140-148: Undelegation call updated - LGTM!The call site correctly uses the renamed
perform_account_undelegationfunction. The new function behavior (setting bothundelegating=trueanddelegated=false) is appropriate for this undelegation flow.programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
135-148: Delegated check added to prevent double undelegation - LGTM!The validation now requires accounts to be both writable AND delegated (line 141) before undelegation, preventing double undelegation. The condition correctly uses
||(OR) to fail if either requirement is not met.The comment on lines 136-138 provides good context: "SVM doesn't check delegated, so we need to do extra checks here. Otherwise account could be undelegated twice."
This logic is consistent with the validation in
magic_scheduled_base_intent.rs(lines 214-233).
191-211: Undelegation execution updated - LGTM!The call to
perform_account_undelegation(line 205) correctly uses the renamed function, which now sets bothundelegating=trueanddelegated=false. This ensures subsequent attempts to undelegate the same account will fail the validation check added at lines 136-148.magicblock-accounts/src/scheduled_commits_processor.rs (1)
92-134: Acknowledge the relaxed validation design, but flag the documented edge case for undelegation failures.The validation relaxation is intentional—the code assumes intents scheduled by magic-program are valid, deferring all checks to scheduling time. However, the
process_undelegation_requestsmethod (lines 159-162) contains an explicit comment acknowledging a gap: "if subscriptions fail, undelegated accounts stay in a problematic state in the validator and are not synced from chain."If accounts are evicted between scheduling and commit execution,
chainlink.undelegation_requested()calls may fail. These failures are logged but do not block commit processing. The code notes (line 163) that a retry mechanism should be implemented, but currently is not.Recommendation: Consider whether account eviction between scheduling and processing is a realistic scenario for your deployment model. If so, implementing the noted retry mechanism would prevent undelegated accounts from getting stuck in this problematic state.
test-integration/programs/schedulecommit/Cargo.toml (1)
11-11: New magicblock_magic_program_api workspace dependency looks correctThe added dependency aligns with the new
MagicBlockInstructionusage insrc/lib.rs; existingcustom-heap/custom-panicfeatures remain as expected for Solana programs (based on learnings).test-integration/programs/schedulecommit/src/api.rs (1)
223-247: Twice-undelegate CPI builder is consistent with existing helpersThe new
schedule_commit_and_undelegate_cpi_twicemirrors the existing CPI helpers (payer/context/program/committees layout and players payload) and cleanly wires to the new instruction variant. No issues from this side.test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (3)
7-11: Importing the new CPI helper is appropriateBringing
schedule_commit_and_undelegate_cpi_twiceinto scope keeps the API usage localized with the other schedule-commit helpers; no issues here.
165-211: Helper for “twice” path is consistent with existing helpers
commit_and_undelegate_two_accounts_twicemirrorscommit_and_undelegate_two_accounts(false)but swaps in the new CPI; context extraction, account vectors, and transaction construction remain identical, which is what we want to isolate the double-undelegate behavior.
571-603: New double‑undelegation test matches intended behaviorThe test drives the new helper, checks for the strengthened error message, and verifies via logs/signature that no scheduled commit is confirmed when the double undelegation fails—all consistent with the PR’s goal of preventing a second undelegation.
test-integration/programs/schedulecommit/src/lib.rs (1)
458-561: Includemagic_programinaccount_infosfor the CPI to succeedThe
invoke_signedcall inprocess_schedulecommit_and_undelegation_cpi_twiceomitsmagic_programfromaccount_infos. According to the official Solana documentation, "the program id of the instruction being issued must also be included inaccount_infos", and the caller must provide the AccountInfo for every account required by the instruction, including other programs. Without this, the CPI will fail at runtime before reaching the MagicBlock logic.Fix by including
magic_programinaccount_infos:- // All accounts that will be passed to the CPI - let mut account_infos = Vec::with_capacity(2 + remaining.len()); - account_infos.push(payer.clone()); - account_infos.push(magic_context.clone()); - account_infos.extend(remaining.iter().cloned()); + // All accounts that will be passed to the CPI, including the target program + let mut account_infos = Vec::with_capacity(3 + remaining.len()); + account_infos.push(magic_program.clone()); + account_infos.push(payer.clone()); + account_infos.push(magic_context.clone()); + account_infos.extend(remaining.iter().cloned());This keeps the instruction metas (payer, context, committees) intact while satisfying
invoke_signed's requirement.⛔ Skipped due to learnings
Learnt from: bmuddha Repo: magicblock-labs/magicblock-validator PR: 578 File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27 Timestamp: 2025-10-21T14:00:54.642Z Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
LGTM after addressing coderabbit comment and using that higher level test context.
| .ephem_client | ||
| .as_ref() | ||
| .unwrap() | ||
| .confirm_transaction(&scheduled_commmit_sent_sig) |
There was a problem hiding this comment.
Use pub fn confirm_transaction_ephem here instead (test-tools/src/integration_test_context.rs:686)
|
|
||
| /// Sets proper values on account during undelegation | ||
| pub(crate) fn mark_account_as_undelegating(acc: &RefCell<AccountSharedData>) { | ||
| pub(crate) fn perform_account_undelegation(acc: &RefCell<AccountSharedData>) { |
There was a problem hiding this comment.
I'd still name this mark_as_undelegated because the full undelegation is still performed by the committor service, but don't have a very strong opinion.
taco-paco
force-pushed
the
fix/magic-program/enfores-delegated-on-undelegation
branch
from
cd59dbb to
f73b77d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
571-603: Reuseconfirm_transaction_ephemhelper instead of raw client confirmThe new test duplicates the direct
ephem_client.confirm_transactionusage pattern. Prefer calling the existingconfirm_transaction_ephemhelper on the context for consistency and less boilerplate in tests.As noted in a previous review, this centralizes ephemeral confirmation behavior in
IntegrationTestContext.
📜 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 (9)
magicblock-accounts/src/scheduled_commits_processor.rs(3 hunks)programs/magicblock/src/magic_scheduled_base_intent.rs(1 hunks)programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(3 hunks)programs/magicblock/src/utils/account_actions.rs(1 hunks)test-integration/programs/schedulecommit/Cargo.toml(1 hunks)test-integration/programs/schedulecommit/src/api.rs(1 hunks)test-integration/programs/schedulecommit/src/lib.rs(6 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 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:
programs/magicblock/src/utils/account_actions.rsprograms/magicblock/src/schedule_transactions/process_schedule_base_intent.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsmagicblock-accounts/src/scheduled_commits_processor.rsprograms/magicblock/src/magic_scheduled_base_intent.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_transactions/process_schedule_base_intent.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.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/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.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/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.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:
programs/magicblock/src/schedule_transactions/process_schedule_commit.rsprograms/magicblock/src/magic_scheduled_base_intent.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:
test-integration/programs/schedulecommit/Cargo.toml
📚 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-accounts/src/scheduled_commits_processor.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-accounts/src/scheduled_commits_processor.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:
programs/magicblock/src/magic_scheduled_base_intent.rs
📚 Learning: 2025-11-04T10:48:00.070Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
programs/magicblock/src/magic_scheduled_base_intent.rs
🧬 Code graph analysis (7)
programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs (1)
programs/magicblock/src/utils/account_actions.rs (1)
perform_account_undelegation(18-23)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (4)
test-integration/programs/schedulecommit/src/api.rs (1)
schedule_commit_and_undelegate_cpi_twice(223-247)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-29)test-integration/test-tools/src/integration_test_context.rs (2)
assert_ephemeral_transaction_error(640-648)fetch_ephemeral_logs(144-146)test-integration/test-tools/src/scheduled_commits.rs (1)
extract_scheduled_commit_sent_signature_from_logs(16-28)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
programs/magicblock/src/utils/account_actions.rs (1)
perform_account_undelegation(18-23)programs/magicblock/src/utils/accounts.rs (1)
get_writable_with_idx(104-111)
test-integration/programs/schedulecommit/src/api.rs (2)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (10)
committees(63-66)committees(67-67)committees(74-77)committees(78-78)committees(124-127)committees(128-128)committees(135-138)committees(139-139)committees(183-186)committees(187-187)test-integration/schedulecommit/client/src/schedule_commit_context.rs (2)
committees(153-163)committees(165-169)
test-integration/programs/schedulecommit/src/lib.rs (2)
test-integration/schedulecommit/test-security/tests/01_invocations.rs (1)
create_schedule_commit_ix(41-66)test-integration/programs/schedulecommit/src/api.rs (2)
pda_and_bump(282-286)pda_seeds_with_bump(268-273)
magicblock-accounts/src/scheduled_commits_processor.rs (2)
programs/magicblock/src/magic_scheduled_base_intent.rs (2)
is_undelegate(101-103)is_undelegate(144-150)test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)
base_intent(558-563)
programs/magicblock/src/magic_scheduled_base_intent.rs (2)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
delegated(95-101)programs/magicblock/src/utils/accounts.rs (2)
get_instruction_account_with_idx(65-73)get_instruction_pubkey_with_idx(94-102)
🔇 Additional comments (9)
programs/magicblock/src/magic_scheduled_base_intent.rs (1)
214-233: LGTM! Double-undelegation prevention implemented correctly.The validation now requires both
writableanddelegatedstatus before allowing undelegation. This prevents the double-undelegation issue described in the PR objectives: whenperform_account_undelegationsetsdelegatedto false (as seen in account_actions.rs), subsequent undelegation attempts will fail this check.magicblock-accounts/src/scheduled_commits_processor.rs (1)
92-134: LGTM! Preprocessing logic simplified with relaxed validation.The function now returns only a 2-tuple (removing
excluded_pubkeys) and implements a key assumption change: accounts not found in AccountsDB are logged but no longer excluded from the intent. The implementation assumes that intents successfully scheduled by magic-program are valid at scheduling time, shifting validation responsibility upstream.This aligns with the PR objectives stating that ScheduledCommitsProcessorImpl now assumes scheduled intents are valid and that some checks have been relaxed. The info log at line 116 helps identify potential evictions or implementation bugs without blocking intent execution.
programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs (1)
17-17: LGTM! Function rename applied consistently.The import and call site correctly use the renamed
perform_account_undelegationfunction. The new name better reflects that the function performs undelegation by setting bothundelegatinganddelegatedflags.Also applies to: 146-146
programs/magicblock/src/utils/account_actions.rs (1)
17-23: LGTM! Core double-undelegation fix implemented.The function now sets both
undelegating(true)anddelegated(false). The addition ofset_delegated(false)(line 22) is the critical fix that prevents double-undelegation: when an account is scheduled for undelegation, itsdelegatedflag is immediately set to false, causing subsequent undelegation attempts to fail thedelegated()check in validation (as seen in magic_scheduled_base_intent.rs and process_schedule_commit.rs).The rename from
mark_account_as_undelegatingtoperform_account_undelegationaccurately reflects the expanded behavior.programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
135-148: LGTM! Validation and undelegation consistent with other files.The enhanced validation (lines 136-148) correctly requires both
writableanddelegatedstatus before allowing undelegation, with a clear comment explaining why: "SVM doesn't check delegated, so we need to do extra checks here. Otherwise account could be undelegated twice."The call to
perform_account_undelegation(line 205) is correctly placed after validation and uses the renamed function that sets both flags to prevent double-undelegation.This implementation is consistent with the validation logic in magic_scheduled_base_intent.rs.
Also applies to: 205-205
test-integration/programs/schedulecommit/Cargo.toml (1)
11-11: Add magicblock_magic_program_api workspace dependencyThe new dependency aligns with the
MagicBlockInstructionusage inprogram-schedulecommit/src/lib.rsand looks correct for a workspace-local crate.Please just confirm that
magicblock_magic_program_apiis defined in the workspace members socargo testbuilds all integration programs cleanly.test-integration/programs/schedulecommit/src/api.rs (1)
223-247: New twice-undelegate CPI helper matches existing patterns
schedule_commit_and_undelegate_cpi_twiceis consistent with the other schedule‑commit CPI helpers (same account metas shape, same players/committees wiring) and correctly targets the newScheduleCommitAndUndelegateCpiTwicevariant.test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
165-211: Helper for twice-undelegate flow is consistent with existing helpers
commit_and_undelegate_two_accounts_twicereuses the same context setup, players/committees extraction, and transaction construction as the existing one‑ and two‑account helpers, so the new test will exercise the intended CPI path without surprises.test-integration/programs/schedulecommit/src/lib.rs (1)
98-109: Enum variant and dispatcher wiring look correctThe new
ScheduleCommitAndUndelegateCpiTwice(Vec<Pubkey>)variant and itsprocess_instructionmatch arm cleanly mirror the existingScheduleCommitAndUndelegateCpiModAfterpath, so instruction parsing and routing for the twice‑undelegate flow are correctly hooked up.Also applies to: 167-173
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_transactions/process_schedule_commit.rs (1)
135-148: Undelegation precondition is correct; comment wording is invertedThe new check:
- Ensures the account is writable via
get_writable_with_idx, and- Ensures
acc.borrow().delegated()is truebefore allowing undelegation when
request_undelegationis set. That’s exactly what you need to prevent a second undelegation attempt afterdelegatedhas been cleared.The inline comment, however, says “also undelegated,” which is the opposite of what’s being checked. Suggest flipping it to avoid confusion:
- // Check if account is writable and also undelegated + // Check if account is writable and also delegatedEverything else in this block looks good.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
magicblock-accounts/src/scheduled_commits_processor.rs(5 hunks)programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs(2 hunks)programs/magicblock/src/schedule_transactions/process_schedule_commit.rs(3 hunks)programs/magicblock/src/utils/account_actions.rs(1 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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:
programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.rsmagicblock-accounts/src/scheduled_commits_processor.rsprograms/magicblock/src/utils/account_actions.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_transactions/process_schedule_base_intent.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rsprograms/magicblock/src/schedule_transactions/process_schedule_commit.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/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.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/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.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:
programs/magicblock/src/schedule_transactions/process_schedule_commit.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-accounts/src/scheduled_commits_processor.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:
programs/magicblock/src/utils/account_actions.rs
🧬 Code graph analysis (4)
programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs (1)
programs/magicblock/src/utils/account_actions.rs (1)
mark_account_as_undelegated(18-23)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (4)
test-integration/programs/schedulecommit/src/api.rs (1)
schedule_commit_and_undelegate_cpi_twice(223-247)test-integration/schedulecommit/test-scenarios/tests/utils/mod.rs (1)
get_context_with_delegated_committees(16-29)test-integration/test-tools/src/integration_test_context.rs (3)
assert_ephemeral_transaction_error(640-648)fetch_ephemeral_logs(144-146)confirm_transaction_ephem(686-700)test-integration/test-tools/src/scheduled_commits.rs (1)
extract_scheduled_commit_sent_signature_from_logs(16-28)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)
programs/magicblock/src/utils/account_actions.rs (1)
mark_account_as_undelegated(18-23)programs/magicblock/src/utils/accounts.rs (1)
get_writable_with_idx(104-111)
magicblock-accounts/src/scheduled_commits_processor.rs (1)
programs/magicblock/src/magic_scheduled_base_intent.rs (2)
is_undelegate(101-103)is_undelegate(144-150)
🔇 Additional comments (7)
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (1)
7-11: New CPI helper import is correctly wired and usedThe added
schedule_commit_and_undelegate_cpi_twiceimport is used below in the new helper and matches the API signature fromprogram_schedulecommit::api; no issues here.magicblock-accounts/src/scheduled_commits_processor.rs (3)
347-385: Updatedprocessflow is consistent with the newpreprocess_intentcontractThe new pipeline (
take_scheduled_actions→preprocess_intent→ build metas under a single lock → dedupe undelegation pubkeys viaHashSet→process_undelegation_requests→schedule_base_intent) looks sound:
- You avoid holding the
intents_meta_maplock across preprocessing.- Dedupe of
pubkeys_being_undelegatedprevents redundantundelegation_requestedcalls.- Early return on empty
scheduled_base_intentsavoids needless work.No functional issues from this change.
326-340:build_sent_commitchange to always-emptyexcluded_pubkeyslooks safeGiven
ScheduledBaseIntentMetano longer tracksexcluded_pubkeys, hardcodingexcluded_pubkeys: vec![]inSentCommitcleanly preserves the existing API shape while dropping unused data. Assuming downstream treats an empty list as “no explicit exclusions,” this is a reasonable transitional state.No change requested.
410-421:ScheduledBaseIntentMeta::newsimplification is appropriateSwitching
ScheduledBaseIntentMeta::newto take only the intent and derive:
included_pubkeysviaget_committed_pubkeys().unwrap_or_default()requested_undelegationviaintent.is_undelegate()removes now-unnecessary plumbing and keeps all metadata derived from the canonical
ScheduledBaseIntent. This matches the overall direction of trusting the magic-program’s scheduling-time validation.Looks good as-is.
programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs (1)
123-147: Use ofmark_account_as_undelegatedhere is consistent with commit path semanticsUsing
mark_account_as_undelegated(account_ref)when buildingCommitAndUndelegateintents ensures:
- Owner is switched to the delegation program.
undelegatingis set anddelegatedis cleared at scheduling time.That aligns this CPI scheduling path with the direct
ScheduleCommitpath and supports the “no double undelegation” invariant.No changes requested.
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
191-211: Immediatemark_account_as_undelegatedcall correctly enforces single-undelegation invariantUsing
mark_account_as_undelegated(acc)here ensures that, once the undelegating commit is scheduled:
- The owner is changed to the delegation program.
undelegatingis set.delegatedis cleared.Given the earlier precondition
acc.borrow().delegated()for undelegation requests, this guarantees subsequent attempts to undelegate the same account will fail the delegated check, matching the PR’s objective to prevent double undelegation.The log message still saying “undelegating” matches the fact that the actual on-chain undelegation is completed later by the committor.
No change needed.
programs/magicblock/src/utils/account_actions.rs (1)
17-23:mark_account_as_undelegatedimplementation matches validator-side expectationsThis helper now atomically:
- Sets the owner to
DELEGATION_PROGRAM_ID.- Marks the account as
undelegating.- Clears the
delegatedflag.That’s exactly what the updated scheduling/commit logic relies on (writable +
delegated()for the first undelegation, anddelegated() == falsethereafter). Borrowing mutably once for both flag updates is also a nice cleanup over multiple borrows.Looks good.
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Show resolved
Hide resolved
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
Show resolved
Hide resolved
taco-paco
deleted the
fix/magic-program/enfores-delegated-on-undelegation
branch
Prior newly added test
test_committing_and_undelegating_two_accounts_twicewoudln't pass, since same account could be scheduled for undelegation twice, see how that could of been done inprocess_schedulecommit_and_undelegation_cpi_twice.Now we set
is_delegated=falseonce undelegation requested, then 2nd undelegation will fail onacc.delegated()check inmagic-programAdditionally the
ScheduledCommitsProcessorImpladapted. We assume that all intents that got successfully scheduled due to magic-program execution are valid, and all account states that are present there are valid, were delegated and existed in accountsdb.Due to above we relax checks in
ScheduledCommitsProcessorImplSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.