fix: prevent removal of delegated/undelegating accounts on eviction #724
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThis PR adds an optional remote_slot: Option field to account modification types and propagates it through cloning, BPF loader upgradable program handling, instruction construction, and modify-accounts instruction assembly. process_mutate_accounts now applies remote_slot when provided and tests updated/added. Chainlink logging strings were enriched and account removal was made conditional based on undelegating/delegated state. A default remove_account_conditionally method was added to AccountsBank. The schedulecommit test program received a SetCount instruction and undelegation guard. .gitignore was updated to ignore magicblock-test-storage/. 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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks)magicblock-account-cloner/src/bpf_loader_v1.rs(2 hunks)magicblock-account-cloner/src/lib.rs(4 hunks)magicblock-chainlink/src/chainlink/fetch_cloner.rs(3 hunks)magicblock-chainlink/src/chainlink/mod.rs(1 hunks)magicblock-core/src/traits.rs(1 hunks)magicblock-magic-program-api/src/instruction.rs(2 hunks)programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs(3 hunks)programs/magicblock/src/utils/instruction_utils.rs(1 hunks)test-integration/configs/schedulecommit-conf-fees.ephem.toml(1 hunks)test-integration/programs/schedulecommit/src/api.rs(2 hunks)test-integration/programs/schedulecommit/src/lib.rs(5 hunks)test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
📚 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-account-cloner/src/bpf_loader_v1.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/mod.rsprograms/magicblock/src/mutate_accounts/process_mutate_accounts.rsprograms/magicblock/src/utils/instruction_utils.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
magicblock-account-cloner/src/bpf_loader_v1.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/mod.rsmagicblock-core/src/traits.rsprograms/magicblock/src/mutate_accounts/process_mutate_accounts.rsprograms/magicblock/src/utils/instruction_utils.rstest-integration/programs/schedulecommit/src/lib.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-account-cloner/src/bpf_loader_v1.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/mod.rsprograms/magicblock/src/mutate_accounts/process_mutate_accounts.rsprograms/magicblock/src/utils/instruction_utils.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.
Applied to files:
magicblock-account-cloner/src/bpf_loader_v1.rsmagicblock-account-cloner/src/lib.rsmagicblock-chainlink/src/chainlink/mod.rsprograms/magicblock/src/mutate_accounts/process_mutate_accounts.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-chainlink/src/chainlink/mod.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-chainlink/src/chainlink/mod.rs
📚 Learning: 2025-12-01T16:02:05.367Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 703
File: magicblock-chainlink/src/submux/mod.rs:652-654
Timestamp: 2025-12-01T16:02:05.367Z
Learning: In magicblock-chainlink/src/submux/mod.rs, the subscribe_program method intentionally adds program_id to program_subs before attempting the subscription. This ensures that even if the initial subscription fails or only partially succeeds across clients, the reconnection logic will retry the subscription. This is a deliberate design pattern for resilience in the multi-client architecture and should not be "fixed" to remove entries on failure.
Applied to files:
magicblock-chainlink/src/chainlink/mod.rstest-integration/programs/schedulecommit/src/lib.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
programs/magicblock/src/mutate_accounts/process_mutate_accounts.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.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/programs/schedulecommit/src/lib.rstest-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
📚 Learning: 2025-11-25T11:07:20.001Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/programs/flexi-counter/src/processor.rs:643-680
Timestamp: 2025-11-25T11:07:20.001Z
Learning: In the compressed delegation architecture, ownership reassignment during undelegation is handled by the delegation program itself, not by the owner program's external undelegate handler. The external undelegate handler (e.g., `process_external_undelegate_compressed` in flexi-counter) is a callback for program-specific cleanup (restoring lamports and data) after the delegation program has already reassigned ownership back to the original owner stored in the CompressedDelegationRecord.
Applied to files:
test-integration/programs/schedulecommit/src/lib.rsmagicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.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:
test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs
🧬 Code graph analysis (5)
magicblock-account-cloner/src/lib.rs (1)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
remote_slot(162-168)
magicblock-chainlink/src/chainlink/mod.rs (2)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (4)
account(197-219)delegated(95-101)owner(54-60)owner(260-262)magicblock-chainlink/src/remote_account_provider/lru_cache.rs (1)
remove(104-119)
programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs (2)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (2)
remote_slot(162-168)account(197-219)programs/magicblock/src/utils/instruction_utils.rs (1)
modify_accounts_instruction(162-197)
test-integration/programs/schedulecommit/src/lib.rs (1)
programs/magicblock/src/magic_context.rs (1)
deserialize(18-26)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (1)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (5)
delegated(95-101)owner(54-60)owner(260-262)remote_slot(162-168)account(197-219)
🔇 Additional comments (24)
.gitignore (1)
22-22: LGTM!Adding
magicblock-test-storage/to the gitignore in the Ledger section is appropriate and aligns with the integration test changes. The path follows the existing pattern and should be ignored.test-integration/configs/schedulecommit-conf-fees.ephem.toml (1)
23-23: Intentional reset behavior addition for test consistency.Adding
reset = trueto theaccountsdbsection mirrors the existingreset = truein theledgersection, ensuring both database components reset between ephemeral test runs. This supports clean test isolation for the new account-preservation scenarios being tested.Please confirm: Does resetting
accountsdbbetween test invocations align with your test strategy, or should individual tests handle account state cleanup independently? This is particularly relevant for integration tests that verify account preservation across failures.magicblock-core/src/traits.rs (1)
19-29: LGTM! Clean conditional removal utility.The implementation correctly combines
get_account, predicate evaluation, andremove_accountinto a single operation. The default trait method ensures backward compatibility.magicblock-chainlink/src/chainlink/mod.rs (1)
222-252: LGTM! Correctly implements conditional removal with comprehensive logging.The logic correctly preserves accounts that are either undelegating or delegated, preventing data loss as described in the PR objectives. The removal condition
!undelegating && !delegatedensures that only accounts in neither state are removed from the bank.The trace logging provides excellent visibility into removal decisions, including the specific states that triggered retention.
magicblock-chainlink/src/chainlink/fetch_cloner.rs (3)
220-233: LGTM! Improved debug logging for undelegating account state.The enhanced logging now includes both in-bank and on-chain account states (delegated flag, owner, remote_slot), which will significantly help with debugging delegation/undelegation flow issues. This aligns well with the PR's goal of preventing incorrect account removal during undelegation.
278-287: LGTM! Helpful trace logging for valid account state.The conditional trace logging provides useful diagnostic information without impacting performance when trace logging is disabled.
430-433: LGTM! More accurate terminology.Changing "Cloning delegated account" to "Resolving delegated account" better describes what's happening at this point in the flow—the account is being resolved with delegation record context before cloning.
test-integration/programs/schedulecommit/src/lib.rs (1)
35-36: LGTM! Sentinel constant for testing undelegation failure.Using
u64::MAX - 1as a sentinel value is appropriate for test purposes, as it's highly unlikely to conflict with normal counter values.magicblock-account-cloner/src/bpf_loader_v1.rs (2)
40-56: LGTM! Correct propagation of remote_slot for program data account.The
remote_slotis correctly captured fromloaded_program.remote_slotand included in theAccountModification, ensuring the remote slot is persisted to the bank for BPF upgradeable program data accounts.
59-77: LGTM! Consistent remote_slot propagation for program ID account.Both the program data and program ID modifications now include
remote_slot, maintaining consistency in how remote slot information flows through the cloning process.magicblock-magic-program-api/src/instruction.rs (2)
126-138: LGTM! Consistent field addition to instruction variant.The
remote_slotfield is appropriately mirrored inAccountModificationForInstructionto ensure the value can be transmitted through on-chain instructions.
111-124: LGTM! Core API addition for remote_slot tracking.Adding
remote_slot: Option<u64>toAccountModificationandAccountModificationForInstructionenables tracking the remote slot throughout the account cloning and modification pipeline. The field is correctly propagated ininstruction_utils.rsand will be serialized as part of theModifyAccountsinstruction via bincode.Note: Instructions have a 1232-byte size limit. Monitor instruction data size as more fields are added to
AccountModificationForInstruction, especially when building instructions with many account modifications.test-integration/programs/schedulecommit/src/api.rs (2)
259-267: LGTM! New helper for setting counter value.The
set_count_instructionhelper follows the same pattern asincrease_count_instructionand correctly constructs the instruction with the newSetCountvariant.
187-192: No actionable concerns. Themodify_accounts: falsesetting is intentional and consistent across all usages. When false, PDA account counts are not incremented during the CPI, which is the current design. No tests expect or rely on account modification behavior.magicblock-account-cloner/src/lib.rs (4)
94-104: LGTM! Correct remote_slot propagation for regular account cloning.The
remote_slotis correctly extracted fromrequest.account.remote_slot()and included in theAccountModification. This ensures the remote slot is persisted to the bank when accounts are cloned.
225-226: LGTM! Good practice capturing remote_slot before consuming program.Extracting
program_remote_slotbeforeprogram.try_into_deploy_data_and_ixs_v4()ensures the value is available for use in subsequent modification blocks, sincetry_intoconsumes the program.
245-259: LGTM! Remote slot included in pre-deploy modification.The
remote_slotis correctly set in the pre-deployAccountModification, ensuring consistency through the LoaderV4 deployment flow.
261-272: LGTM! Remote slot included in post-deploy modification.Consistent with the pre-deploy modification, ensuring the
remote_slotis preserved through the complete deployment process.programs/magicblock/src/utils/instruction_utils.rs (1)
174-186: LGTM! Completes the remote_slot propagation chain.The
remote_slotfield is correctly mapped fromAccountModificationtoAccountModificationForInstruction, ensuring the value is included in the instruction data sent to the on-chain program.test-integration/schedulecommit/test-scenarios/tests/02_commit_and_undelegate.rs (2)
7-16: LGTM! Appropriate imports for the new test.The imports correctly pull in
set_count_instructionandFAIL_UNDELEGATION_COUNTfrom the schedulecommit program API.
607-709: LGTM! Valuable integration test for failed undelegation scenario.This test verifies the core PR objective—that accounts are preserved when undelegation fails. The test flow is:
- Set counter to
FAIL_UNDELEGATION_COUNT(triggers error 111 in undelegate handler)- Attempt commit + undelegate
- Verify commit was scheduled (undelegation failure is handled gracefully)
- Verify subsequent modification fails (account remains in undelegating state)
The
set_counterclosure withexpect_failureparameter is a clean way to handle both success and expected-failure cases.programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs (3)
226-233: LGTM!The
remote_slothandling follows the established pattern for optional field modifications. The implementation is consistent with other fields likedelegatedandconfined, including proper logging.
331-341: LGTM!The addition of
remote_slot: Nonemaintains completeness in this explicit field test. The dedicatedtest_mod_remote_slottest handles the actual remote_slot functionality.
636-676: LGTM!The test is well-structured and follows existing patterns. It correctly verifies that
remote_slotis persisted on the modified account. Based on learnings, this is distinct from the delegation record slot and the test appropriately validates theAccountSharedDataremote_slot value.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test-integration/programs/schedulecommit/src/lib.rs (1)
621-641: Remove redundant second deserialization and fix log typo in undelegation guard.This block deserializes
MainAccounttwice with equivalent checks and has a misleading error message:
- The second
match MainAccount::deserialize(&mut data.as_ref())repeats the samecount == FAIL_UNDELEGATION_COUNTguard.- The first error log says
"Failed to delegate"instead of “deserialize”.You can simplify and clarify as follows (same suggestion as earlier review):
{ let data = delegated_account.try_borrow_data()?; match MainAccount::try_from_slice(&data) { Ok(counter) => { msg!("counter: {:?}", counter); if counter.count == FAIL_UNDELEGATION_COUNT { return Err(ProgramError::Custom(111)); } } - Err(err) => msg!("Failed to delegate: {:?}", err), - } - match MainAccount::deserialize(&mut data.as_ref()) { - Ok(counter) => { - msg!("counter: {:?}", counter); - if counter.count == FAIL_UNDELEGATION_COUNT { - return Err(ProgramError::Custom(111)); - } - } - Err(err) => msg!("Failed to de: {:?}", err), + Err(err) => msg!("Failed to deserialize: {:?}", err), } };Behavior is unchanged while avoiding duplicate work and fixing the log text.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-integration/programs/schedulecommit/src/lib.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Learnt from: 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-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/programs/schedulecommit/src/lib.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:
test-integration/programs/schedulecommit/src/lib.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
test-integration/programs/schedulecommit/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/programs/schedulecommit/src/lib.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/programs/schedulecommit/src/lib.rs
📚 Learning: 2025-12-01T16:02:05.367Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 703
File: magicblock-chainlink/src/submux/mod.rs:652-654
Timestamp: 2025-12-01T16:02:05.367Z
Learning: In magicblock-chainlink/src/submux/mod.rs, the subscribe_program method intentionally adds program_id to program_subs before attempting the subscription. This ensures that even if the initial subscription fails or only partially succeeds across clients, the reconnection logic will retry the subscription. This is a deliberate design pattern for resilience in the multi-client architecture and should not be "fixed" to remove entries on failure.
Applied to files:
test-integration/programs/schedulecommit/src/lib.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/programs/schedulecommit/src/lib.rs
📚 Learning: 2025-11-25T11:07:20.001Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/programs/flexi-counter/src/processor.rs:643-680
Timestamp: 2025-11-25T11:07:20.001Z
Learning: In the compressed delegation architecture, ownership reassignment during undelegation is handled by the delegation program itself, not by the owner program's external undelegate handler. The external undelegate handler (e.g., `process_external_undelegate_compressed` in flexi-counter) is a callback for program-specific cleanup (restoring lamports and data) after the delegation program has already reassigned ownership back to the original owner stored in the CompressedDelegationRecord.
Applied to files:
test-integration/programs/schedulecommit/src/lib.rs
🔇 Additional comments (2)
test-integration/programs/schedulecommit/src/lib.rs (2)
35-36: Sentinel constant for undelegation failure looks fine.Using
u64::MAX - 1as a dedicatedFAIL_UNDELEGATION_COUNTsentinel is clear and unlikely to collide with normal counts in this test program.
411-431:process_set_countimplementation is consistent and correct.Handler correctly deserializes
MainAccount, setscounttovalue, re‑serializes into the PDA, and logs with accurate “set_count” messaging. Mirrorsprocess_increase_countsemantics appropriately.
* master: Some optimizations for intent execution (#711)
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test-integration/programs/schedulecommit/src/api.rs(1 hunks)test-integration/programs/schedulecommit/src/lib.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Learnt from: 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.
📚 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:
test-integration/programs/schedulecommit/src/lib.rs
📚 Learning: 2025-12-03T09:33:48.707Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.
Applied to files:
test-integration/programs/schedulecommit/src/lib.rs
📚 Learning: 2025-12-03T09:36:01.527Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).
Applied to files:
test-integration/programs/schedulecommit/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/programs/schedulecommit/src/lib.rs
📚 Learning: 2025-12-01T16:02:05.367Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 703
File: magicblock-chainlink/src/submux/mod.rs:652-654
Timestamp: 2025-12-01T16:02:05.367Z
Learning: In magicblock-chainlink/src/submux/mod.rs, the subscribe_program method intentionally adds program_id to program_subs before attempting the subscription. This ensures that even if the initial subscription fails or only partially succeeds across clients, the reconnection logic will retry the subscription. This is a deliberate design pattern for resilience in the multi-client architecture and should not be "fixed" to remove entries on failure.
Applied to files:
test-integration/programs/schedulecommit/src/lib.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/programs/schedulecommit/src/lib.rs
🔇 Additional comments (3)
test-integration/programs/schedulecommit/src/api.rs (1)
259-267:set_count_instructionhelper correctly mirrorsincrease_count_instruction.Instruction construction (program_id, single writable committee meta, Borsh payload) matches the existing pattern and looks correct.
test-integration/programs/schedulecommit/src/lib.rs (2)
119-124:SetCountdocumentation now matches its behavior.The docs clearly state that this instruction sets the PDA counter to an arbitrary value and describe the account layout appropriately.
182-183: Dispatcher wiring forSetCountlooks correct.Routing
SetCount(value)toprocess_set_count(accounts, value)aligns with the existing pattern used forIncreaseCount.
Summary
Fix incorrect account removal during eviction by preventing delegated or undelegating accounts
from being removed from the bank when unsubscribed.
Additionally fixing old vs. new delegation detection by persisting remote slot to bank.
Details
When accounts are unsubscribed from chainlink, they get queued for removal from the bank. However,
accounts that are still in the process of undelegating or are still delegated should not be
removed as this could lead to data loss if the undelegation fails to complete on chain.
Additionally, if an account update is received before the undelegation completes, it could
overwrite the in-bank account state and incorrectly set
undelegatingto false.Adding
remote_slottoAccountModificationand setting it when mutating account was secondpart of fix.
Test Changes
test_committing_after_failed_undelegationthat verifies accountsare preserved when undelegation fails
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.