Skip to content

Conversation

@thlorenz
Copy link
Collaborator

@thlorenz thlorenz commented Nov 21, 2025

Summary by CodeRabbit

  • Bug Fixes

    • More reliable undelegation handling: accounts are marked as undelegating sooner, completed undelegations are detected on-chain, and stale bank updates are avoided.
    • Safer undelegation scheduling and mutation flows to prevent incorrect undelegation state.
  • New Features

    • Aggressive deduplication for account fetches to skip already-completed undelegations.
  • Tests

    • Added tests and assertions covering undelegation completion, re-delegation, and unchanged undelegating states.

✏️ Tip: You can customize this high-level summary in your review settings.

Refactored account undelegation handling to be more robust and maintainable. Introduced a centralized decision function to determine if an account has finished undelegating on chain, properly manages the undelegating flag throughout the account lifecycle, and removes unreliable account refetching logic that was attempting to detect undelegation completion.

Details

The changes address issues with detecting when accounts have completed their undelegation process on chain. Previously, the system tried to infer undelegation completion from account owner changes, which was error-prone. Now there's a single source of truth for this decision.

Core Logic for Undelegation Detection

Created a new module account_still_undelegating_on_chain that centralizes the logic for deciding whether an account's undelegation is still pending or has completed. This function considers:

  • Whether the account is still delegated to us on chain
  • The delegation slot compared to when we last updated the account in the bank
  • The current delegation record state

Four cases are handled:

  1. Account was undelegated and remained so → clone as is
  2. Account was undelegated but re-delegated to us → clone with updated state
  3. Account was undelegated but re-delegated to another validator → clone as is
  4. Account's undelegation is still pending on chain → keep bank version

Account Undelegating Flag Management

Properly set and clear the undelegating flag in key locations:

  • Set when an undelegation is initiated (in mark_account_as_undelegating)
  • Cleared when accounts are mutated or cloned from subscription updates

Subscription Update Handling

Updated the subscription update handler to:

  • Retrieve and pass delegation records when processing updates for accounts owned by the delegation program
  • Use the centralized decision function to determine if the bank account should be kept or overwritten
  • Remove the subscription-time clearing of the undelegating flag to allow later detection of completion

Fetch and Clone Logic Improvements

  • Added should_refresh_undelegating_in_bank_account to check if an undelegating account has finished undelegating when it's fetched
  • Removed the earlier is_still_delegated_to_us method and the refetching logic that tried to detect undelegation completion by periodically checking delegation state
  • Removes the problematic "account should be watched" defensive check that was trying to refetch accounts

Testing

  • Added assert_not_undelegating macro for test assertions
  • Updated existing undelegation test to check the undelegating flag
  • Added new test test_write_undelegating_account_undelegated_to_other_validator to cover re-delegation to another validator
  • Renamed test from generic name to more descriptive test_write_undelegating_account_still_being_undelegated

@github-actions
Copy link

github-actions bot commented Nov 21, 2025

Manual Deploy Available

You can trigger a manual deploy of this PR branch to testnet:

Deploy to Testnet 🚀

Alternative: Comment /deploy on this PR to trigger deployment directly.

⚠️ Note: Manual deploy requires authorization. Only authorized users can trigger deployments.

Comment updated automatically when the PR is synchronized.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2025

Walkthrough

Adds undelegation tracking and handling across chainlink and program code: marks accounts as undelegating, propagates delegation records through fetch/clone, skips refreshes when undelegation is still pending on-chain, and updates tests and mock helpers accordingly.

Changes

Cohort / File(s) Summary
Mock & Testing
magicblock-chainlink/src/accounts_bank.rs, magicblock-chainlink/src/testing/mod.rs
Adds set_undelegating(&Pubkey, bool) -> &Self to the mock accounts bank; adds exported macro assert_not_undelegating and updates assert_remain_undelegating to check undelegating state.
Chainlink: undelegation decision
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs, magicblock-chainlink/src/chainlink/mod.rs
New module account_still_undelegating_on_chain with pub(crate) fn account_still_undelegating_on_chain(...) -> bool and unit tests; module declared in mod.rs.
Fetch/Clone pipeline
magicblock-chainlink/src/chainlink/fetch_cloner.rs
Propagates optional DelegationRecord with account updates, updates resolve signatures to return (Option<AccountSharedData>, Option<DelegationRecord>), adds should_refresh_undelegating_in_bank_account, introduces fetch_and_clone_accounts_with_dedup dedup path, and adjusts undelegation refresh/unsubscribe logic and metrics.
Remote account provider
magicblock-chainlink/src/remote_account_provider/remote_account.rs
Removes pub fn undelegating(&self) -> bool from ResolvedAccountSharedData.
Tests
magicblock-chainlink/tests/01_ensure-accounts.rs
Renames and adds tests: test_write_undelegating_account_undelegated_to_other_validator() and test_write_undelegating_account_still_being_undelegated(); updates imports to use assert_not_undelegating.
Program: account mutation
programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs
Clears the undelegating flag on accounts at the start of mutation processing.
Program: scheduling & actions
programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs, programs/magicblock/src/schedule_transactions/process_schedule_commit.rs, programs/magicblock/src/utils/account_actions.rs
Replaces set_account_owner_to_delegation_program with mark_account_as_undelegating; mark_account_as_undelegating sets owner to delegation program and sets the undelegating flag; scheduling/commit paths updated to collect pubkeys and call new function and to log undelegation events.

Sequence Diagram(s)

sequenceDiagram
    participant Bank as Local Bank
    participant Fetch as Fetch/Clone
    participant OnChain as On-Chain RPC
    participant Provider as Remote Provider

    Fetch->>Bank: Inspect local account state
    alt Local account marked undelegating
        Fetch->>OnChain: Query DelegationRecord & remote slot
        OnChain-->>Fetch: (DelegationRecord, remote_slot)
        Fetch->>Fetch: account_still_undelegating_on_chain?
        alt Still undelegating
            Fetch-->>Provider: Block remote update (skip refresh)
            Note over Fetch,Bank: keep local undelegating state
        else Undelegation complete or re-delegated
            Fetch->>Provider: Allow refresh / fetch latest account
            Provider-->>Bank: Apply updated account (maybe unmark)
            Note over Fetch,Bank: increment unstuck metric if applicable
        end
    else Not undelegating
        Fetch->>Provider: Normal fetch/update flow
        Provider-->>Bank: Apply update
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Attention areas:
    • magicblock-chainlink/src/chainlink/fetch_cloner.rs — high-density control-flow and data-shape changes (resolve return type, dedup path, metrics).
    • magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs — verify branching and metrics in unit tests.
    • programs/magicblock/src/schedule_transactions/* and utils/account_actions.rs — confirm undelegating flag semantics and that owner/flag changes align with scheduling logic.
    • Tests and mocks (accounts_bank.rs, tests/01_ensure-accounts.rs`) — ensure mocks reflect new behavior and tests exercise new edge cases.

Possibly related PRs

Suggested reviewers

  • GabrielePicco
  • bmuddha

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and directly summarizes the main objective: detecting and fixing accounts stuck in undelegating state, which aligns with the PR's core refactoring of undelegation handling.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch thlorenz/the-great-unborking

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4104f4 and 139449f.

⛔ Files ignored due to path filters (1)
  • test-integration/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1 hunks)
  • magicblock-chainlink/src/chainlink/fetch_cloner.rs (16 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-18T08:47:39.702Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/tests/04_redeleg_other_separate_slots.rs:158-165
Timestamp: 2025-11-18T08:47:39.702Z
Learning: In magicblock-chainlink tests involving compressed accounts, `set_remote_slot()` sets the slot of the `AccountSharedData`, while `compressed_account_shared_with_owner_and_slot()` sets the slot of the delegation record. These are two different fields and both calls are necessary.

Applied to files:

  • magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.

Applied to files:

  • magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs
  • magicblock-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/chainlink/account_still_undelegating_on_chain.rs
  • magicblock-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/chainlink/account_still_undelegating_on_chain.rs
  • magicblock-chainlink/src/chainlink/fetch_cloner.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/account_still_undelegating_on_chain.rs
  • magicblock-chainlink/src/chainlink/fetch_cloner.rs
🧬 Code graph analysis (2)
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)
magicblock-metrics/src/metrics/mod.rs (1)
  • inc_undelegation_completed (491-493)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (4)
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)
  • account_still_undelegating_on_chain (22-89)
magicblock-chainlink/src/chainlink/blacklisted_accounts.rs (1)
  • blacklisted_accounts (6-29)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (3)
  • account (180-202)
  • delegated (95-101)
  • slot (203-215)
magicblock-metrics/src/metrics/mod.rs (1)
  • inc_unstuck_undelegation_count (495-497)
⏰ 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: Build Project
  • GitHub Check: run_make_ci_test
🔇 Additional comments (8)
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)

5-28: LGTM: Clear function contract and documentation.

The function signature and documentation clearly explain the decision logic for handling undelegating accounts. The four cases (A-D) are well-documented.

magicblock-chainlink/src/chainlink/fetch_cloner.rs (7)

184-241: Well-structured undelegation handling in subscription updates.

The logic correctly determines whether to apply subscription updates for undelegating accounts:

  1. Retrieves the delegation record alongside the account (line 184-186)
  2. Checks if the account in the bank is undelegating (line 211)
  3. Calls account_still_undelegating_on_chain to decide whether to skip the update (lines 223-230)
  4. Handles edge cases: accounts owned by the delegation program but not marked as undelegating (lines 232-236), and accounts not in the bank (lines 237-241)

The flow ensures that updates are only skipped when the undelegation is truly still pending on chain.


339-469: Consistent propagation of delegation records through return paths.

The function signature change to return (Option<AccountSharedData>, Option<DelegationRecord>) is implemented correctly. All return paths are updated:

  • Valid delegated account: returns (Some(account), Some(delegation_record)) (lines 424-427)
  • Invalid delegation record: returns (None, None) (line 432)
  • No delegation record: returns (Some(account), None) (line 437)
  • Error paths: return (None, None) (lines 452, 456, 462, 468)

This enables downstream code to use the delegation record for decision-making.


498-555: Conditional unsubscription prevents race conditions.

The unsubscription logic correctly handles temporary subscriptions:

  • Tracks whether the delegation record was already being watched (lines 498-500)
  • Only unsubscribes if it was subscribed temporarily for this fetch AND it's not in the bank (lines 533-542)
  • Acknowledges a negligible race condition where a concurrent fetch+clone could be in progress (lines 534-536)

This prevents incorrectly unsubscribing from delegation records that are legitimately watched.


1034-1076: Centralized decision logic for refreshing undelegating accounts.

The function correctly determines whether to refresh an undelegating account:

  1. Checks if the account is marked as undelegating (line 1045-1046)
  2. Fetches the latest delegation record from the chain (lines 1047-1052)
  3. Determines if the account is currently delegated on chain (lines 1053-1056)
  4. Calls account_still_undelegating_on_chain to make the decision (lines 1057-1063)
  5. Returns true only when undelegation has completed (lines 1064-1068)

The function also logs an edge case where an account is owned by the delegation program but not marked as undelegating (lines 1069-1074).


1096-1133: Efficient pre-filtering prevents redundant fetches.

The deduplication logic correctly pre-filters accounts:

  1. Filters out blacklisted accounts (lines 1096-1099)
  2. For each account, checks if it's in the bank (lines 1113-1115)
  3. For undelegating accounts, determines if they need refreshing via should_refresh_undelegating_in_bank_account (lines 1116-1121)
  4. Increments unstuck_undelegation_count metric when detecting completed undelegations that were missed (line 1124)
  5. Skips fetching accounts that are in the bank and don't need refresh (lines 1126-1130)
  6. Retains only accounts that need to be fetched (line 1133)

This prevents unnecessary network fetches for accounts that are already up-to-date in the bank.


665-667: Defensive error logging for unexpected state.

The error message correctly flags an unexpected condition: an account returned as ResolvedAccount::Bank during fetch. This shouldn't occur because fetch_and_clone_accounts_with_dedup pre-filters accounts that are already in the bank. The error log helps debug violations of this invariant.


1558-2781: Comprehensive test coverage for undelegation and deduplication scenarios.

The test suite thoroughly validates:

  • Single and multiple account fetching (lines 1758-2247)
  • Delegated accounts with valid/invalid delegation records (lines 1828-2247)
  • Stale delegation records and accounts (lines 2250-2377)
  • Race condition prevention for delegation record unsubscription (lines 2380-2460)
  • Concurrent request deduplication (lines 2463-2530)
  • Undelegation subscription behavior (lines 2533-2594)
  • Parallel fetch prevention for multiple accounts (lines 2597-2700)

The tests use clear assertion macros and validate both success paths and edge cases, including metrics tracking.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (2)
magicblock-chainlink/src/accounts_bank.rs (1)

49-56: Stub now tracks undelegating flag; consider updating force_undelegation docs

AccountsBankStub::undelegate now sets undelegating = true and delegated = false, and the new set_undelegating helper mirrors the existing set_owner/set_delegated patterns (including panicking if the account is missing), which is appropriate for a test stub.

The comment on force_undelegation still only mentions set_owner and set_delegated, but via undelegate it now also sets the undelegating flag. Consider updating the comment for clarity:

-        /// Here we mark the account as undelegated in our validator via:
-        /// - set_owner to delegation program
-        /// - set_delegated to false
+        /// Here we mark the account as undelegated in our validator via:
+        /// - set_owner to delegation program
+        /// - set_delegated to false
+        /// - set_undelegating to true

Also applies to: 68-81

programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs (1)

17-21: Base-intent scheduling now marks undelegating accounts and logs them

Using mark_account_as_undelegating on undelegated_accounts_ref ensures the same owner+flag semantics as the commit path, and doing it before persisting ScheduledBaseIntent keeps the validator’s view consistent with what was scheduled.

Collecting the affected pubkeys into undelegated_pubkeys for a single ic_msg! improves observability without affecting control flow; the extra allocation is minor given the expected account counts.

Also applies to: 140-155

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca51651 and e4104f4.

📒 Files selected for processing (11)
  • magicblock-chainlink/src/accounts_bank.rs (2 hunks)
  • magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1 hunks)
  • magicblock-chainlink/src/chainlink/fetch_cloner.rs (16 hunks)
  • magicblock-chainlink/src/chainlink/mod.rs (1 hunks)
  • magicblock-chainlink/src/remote_account_provider/remote_account.rs (0 hunks)
  • magicblock-chainlink/src/testing/mod.rs (2 hunks)
  • magicblock-chainlink/tests/01_ensure-accounts.rs (4 hunks)
  • programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs (1 hunks)
  • programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs (2 hunks)
  • programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2 hunks)
  • programs/magicblock/src/utils/account_actions.rs (1 hunks)
💤 Files with no reviewable changes (1)
  • magicblock-chainlink/src/remote_account_provider/remote_account.rs
🧰 Additional context used
🧠 Learnings (9)
📓 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:

  • programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs
  • magicblock-chainlink/src/chainlink/mod.rs
  • programs/magicblock/src/utils/account_actions.rs
  • programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
  • programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs
  • magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs
  • magicblock-chainlink/src/chainlink/fetch_cloner.rs
  • magicblock-chainlink/tests/01_ensure-accounts.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/mutate_accounts/process_mutate_accounts.rs
  • programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
  • programs/magicblock/src/schedule_transactions/process_schedule_base_intent.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
  • magicblock-chainlink/src/testing/mod.rs
  • magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs
  • magicblock-chainlink/src/chainlink/fetch_cloner.rs
  • magicblock-chainlink/tests/01_ensure-accounts.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
  • magicblock-chainlink/src/testing/mod.rs
  • magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs
  • magicblock-chainlink/tests/01_ensure-accounts.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_commit.rs
  • programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs
📚 Learning: 2025-11-19T09:34:37.917Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: test-integration/test-chainlink/tests/ix_remote_account_provider.rs:62-63
Timestamp: 2025-11-19T09:34:37.917Z
Learning: In test-integration/test-chainlink/tests/ix_remote_account_provider.rs and similar test files, the `_fwd_rx` receiver returned by `init_remote_account_provider()` is intentionally kept alive (but unused) to prevent "receiver dropped" errors on the sender side. The pattern `let (remote_account_provider, _fwd_rx) = init_remote_account_provider().await;` should NOT be changed to `let (remote_account_provider, _) = ...` because dropping the receiver would cause send() operations to fail.

Applied to files:

  • magicblock-chainlink/src/testing/mod.rs
  • magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs
  • magicblock-chainlink/src/chainlink/fetch_cloner.rs
  • magicblock-chainlink/tests/01_ensure-accounts.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:

  • magicblock-chainlink/src/testing/mod.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/account_still_undelegating_on_chain.rs
  • magicblock-chainlink/src/chainlink/fetch_cloner.rs
  • magicblock-chainlink/tests/01_ensure-accounts.rs
🧬 Code graph analysis (7)
magicblock-chainlink/src/chainlink/mod.rs (1)
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)
  • account_still_undelegating_on_chain (22-89)
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1)
programs/magicblock/src/utils/account_actions.rs (1)
  • mark_account_as_undelegating (18-21)
magicblock-chainlink/src/testing/mod.rs (4)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (4)
  • account (180-202)
  • remote_slot (145-151)
  • owner (54-60)
  • owner (243-245)
magicblock-chainlink/src/accounts_bank.rs (1)
  • get_account (106-108)
test-kit/src/lib.rs (1)
  • get_account (282-292)
magicblock-accounts-db/src/lib.rs (1)
  • get_account (335-338)
programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs (1)
programs/magicblock/src/utils/account_actions.rs (1)
  • mark_account_as_undelegating (18-21)
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (2)
magicblock-metrics/src/metrics/mod.rs (1)
  • inc_undelegation_completed (491-493)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
  • remote_slot (145-151)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (4)
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)
  • account_still_undelegating_on_chain (22-89)
magicblock-chainlink/src/chainlink/blacklisted_accounts.rs (1)
  • blacklisted_accounts (6-29)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (3)
  • account (180-202)
  • delegated (95-101)
  • slot (203-215)
magicblock-metrics/src/metrics/mod.rs (1)
  • inc_unstuck_undelegation_count (495-497)
magicblock-chainlink/tests/01_ensure-accounts.rs (1)
test-integration/test-magicblock-api/tests/test_claim_fees.rs (1)
  • validator_pubkey (31-34)
⏰ 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). (1)
  • GitHub Check: run_make_ci_lint
🔇 Additional comments (17)
magicblock-chainlink/src/chainlink/mod.rs (1)

29-29: Wire-up of account_still_undelegating_on_chain module looks correct

The new submodule declaration cleanly registers the helper without widening the public API; this matches the usage pattern from fetch_cloner and keeps the decision logic encapsulated under chainlink.

programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (2)

17-25: Importing mark_account_as_undelegating aligns with new undelegation helper

Switching the import to account_actions::mark_account_as_undelegating keeps this call site in sync with the renamed/extended helper that also flips the undelegating flag, centralizing undelegation pre-state handling.


190-210: Using mark_account_as_undelegating when scheduling undelegation is consistent and safe

Calling mark_account_as_undelegating(acc) after the writability and ownership checks correctly:

  • Locks the account by setting the owner to the delegation program.
  • Marks it as undelegating so chainlink can apply the new stuck-undelegation logic.

The extra commentary and log message accurately describe these side effects, and the borrow scopes around acc avoid RefCell borrow conflicts.

programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs (1)

129-133: Clearing undelegating on mutation relies on global invariants—please confirm

Untoggling the flag with account.borrow_mut().set_undelegating(false); before applying modifications matches the design that only non-pending accounts are ever mutated, and Solana’s transaction semantics ensure this change is rolled back if the instruction fails later.

This does, however, depend on the invariant stated in the comment—that accounts whose undelegation is still pending are never routed through process_mutate_accounts. Please double‑check that the fetch/clone and scheduling flows fully enforce this, so we don’t accidentally drop the undelegating flag while the on-chain undelegation is still incomplete.

programs/magicblock/src/utils/account_actions.rs (1)

17-21: mark_account_as_undelegating correctly encapsulates undelegation pre-state

Having a single helper that sets the owner to DELEGATION_PROGRAM_ID and flips undelegating to true keeps the undelegation pre-state consistent across process_schedule_base_intent and process_schedule_commit. The implementation is straightforward and matches the documented intent.

magicblock-chainlink/tests/01_ensure-accounts.rs (3)

7-8: LGTM! Import changes support new undelegation test assertions.

The new test helper imports (assert_not_undelegating, assert_remain_undelegating, assert_subscribed_without_delegation_record) align with the expanded undelegation test coverage introduced in this PR.


178-215: LGTM! Test correctly validates re-delegation to another validator scenario.

The renamed test now clearly describes the scenario: an account marked as undelegating in the bank is re-delegated to a different validator on-chain. The test setup properly:

  • Creates a separate other_authority for the on-chain delegation record
  • Marks the bank account as undelegating
  • Asserts that the undelegating flag is cleared when the remote state is refreshed

This corresponds to Case C in the account_still_undelegating_on_chain logic.


217-256: LGTM! Test correctly validates pending undelegation scenario.

This new test validates Case D from the account_still_undelegating_on_chain logic: when an account is marked as undelegating in the bank and remains delegated to the validator on-chain (with no newer delegation), the undelegating flag should persist. The test properly:

  • Sets up the account as still delegated to the validator on-chain
  • Marks the bank account as undelegating
  • Verifies the undelegating flag remains set after the fetch
magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (2)

5-28: LGTM! Well-documented function signature with clear contract.

The function documentation clearly explains the decision logic and the single case (Case D) where the undelegating flag should persist. The parameter list provides all necessary context for making the determination.


75-88: LGTM! Cases A and C correctly handle completed undelegations.

Both cases appropriately return false (indicating undelegation completed and account should be refreshed), increment the completion metric, and provide debug logging for observability.

magicblock-chainlink/src/chainlink/fetch_cloner.rs (7)

208-241: LGTM! Subscription listener correctly integrates undelegation checks.

The subscription update handler now:

  1. Retrieves delegation records alongside account data
  2. Checks if the bank account is marked as undelegating
  3. Uses account_still_undelegating_on_chain to determine whether to skip the update (Case D)
  4. Provides defensive logging for edge cases

The parameter passing to account_still_undelegating_on_chain at lines 223-229 correctly provides:

  • account.delegated() for is_delegated_to_us_on_chain
  • in_bank.remote_slot() for remote_slot_in_bank
  • deleg_record which may be None

339-469: LGTM! Function signature updated to provide delegation record context.

The resolve_account_to_clone_from_forwarded_sub_with_unsubscribe function now returns a tuple (Option<AccountSharedData>, Option<DelegationRecord>) instead of just the account. All return paths are consistently updated:

  • Valid delegated account: (Some(account), Some(delegation_record))
  • Account without delegation record: (Some(account), None)
  • Error cases: (None, None)

This enables the caller to use the delegation record for undelegation decisions.


491-555: LGTM! Delegation record fetching handles subscription lifecycle correctly.

The fetch_and_parse_delegation_record function properly:

  1. Tracks whether the delegation record was already being watched (line 498-500)
  2. Fetches and parses the delegation record
  3. Only unsubscribes if this call initiated the subscription AND the record wasn't cloned concurrently (lines 533-552)

The edge case handling at lines 537-540 defensively checks if the delegation record was cloned during the fetch operation to avoid unsubscribing from a record that's now in the bank.


1034-1074: LGTM! Function correctly determines when to refresh undelegating accounts.

The should_refresh_undelegating_in_bank_account function:

  1. Checks if the bank account is marked as undelegating (line 1044)
  2. Fetches the current delegation record from chain (lines 1046-1051)
  3. Determines if the account is still delegated on-chain (lines 1052-1055)
  4. Calls account_still_undelegating_on_chain and returns the inverse (lines 1056-1067)

The logic inversion at line 1056-1062 is correct:

  • If account_still_undelegating_on_chain returns true (Case D - still undelegating), this function returns false (don't refresh)
  • If account_still_undelegating_on_chain returns false (Cases A, B, C - undelegation completed), this function returns true (do refresh)

1110-1132: LGTM! Efficient pre-fetch filtering for undelegating accounts.

The code adds an optimization to check undelegating accounts before fetching:

  1. For each account in the bank, checks if it's undelegating and needs refresh (lines 1115-1120)
  2. If undelegation completed, marks for re-fetch and increments the unstuck metric (lines 1121-1124)
  3. If still undelegating, skips the fetch (lines 1125-1129)
  4. Filters out accounts that don't need fetching (line 1132)

This prevents unnecessary network calls for accounts still in the undelegating state (Case D).


690-692: LGTM! Trace logging updated to reflect new account categorization.

The trace logging now includes the owned_by_deleg category separately, aligning with the updated 4-way categorization (not_found, plain, owned_by_deleg, programs).


665-667: Remove suggestion to use panic!() or unreachable!()—current error logging is appropriate.

The filtering at line 1137 does remove accounts found in bank before calling fetch_and_clone_accounts, upholding the invariant. However, ResolvedAccount::Bank can still occur at lines 665-667 due to race conditions or concurrent access—a fetch initiated after filtering may encounter the account in the bank by the time try_get_multi() returns. Error logging is defensive programming here, not an unreachable case.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2025

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

CodeRabbit commands

These commands are invoked using PR/Issue comments.

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai evaluate custom pre-merge check --instructions <custom-checks-instructions> --name <custom-checks-title> [--mode <error|warning>] to validate the custom pre-merge checks instructions. Defaults to error when --mode is omitted.
    • @coderabbitai ignore pre-merge checks to override pre-merge checks and get an approval on PR.
    • @coderabbitai run pre-merge checks to run pre-merge checks on the pull request.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary or Summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or `` anywhere in the PR title to generate the title automatically.

CodeRabbit configuration file (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, support, documentation and community

  • Visit our status page to check the current availability of CodeRabbit.
  • Create a ticket on our support page for assistance with any issues or questions.
  • Visit our documentation site for detailed information on how to use CodeRabbit.
  • Join our Discord community to connect with other users and get help from the community.
  • Follow us on X/Twitter for updates and announcements.

@thlorenz
Copy link
Collaborator Author

@coderabbitai configuration

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 21, 2025

CodeRabbit configuration
language: en-US
tone_instructions: ''
early_access: true
enable_free_tier: true
reviews:
  profile: assertive
  request_changes_workflow: false
  high_level_summary: true
  high_level_summary_instructions: ''
  high_level_summary_placeholder: Summary
  high_level_summary_in_walkthrough: false
  auto_title_placeholder: ''
  auto_title_instructions: ''
  review_status: true
  commit_status: true
  fail_commit_status: false
  collapse_walkthrough: false
  changed_files_summary: true
  sequence_diagrams: true
  estimate_code_review_effort: true
  assess_linked_issues: true
  related_issues: true
  related_prs: true
  suggested_labels: true
  labeling_instructions: []
  auto_apply_labels: true
  suggested_reviewers: true
  auto_assign_reviewers: false
  in_progress_fortune: false
  poem: false
  path_filters: []
  path_instructions: []
  abort_on_close: true
  disable_cache: false
  auto_review:
    enabled: true
    auto_incremental_review: true
    ignore_title_keywords: []
    labels: []
    drafts: false
    base_branches: []
    ignore_usernames: []
  finishing_touches:
    docstrings:
      enabled: true
    unit_tests:
      enabled: true
  pre_merge_checks:
    docstrings:
      mode: warning
      threshold: 80
    title:
      mode: warning
      requirements: ''
    description:
      mode: warning
    issue_assessment:
      mode: warning
    custom_checks: []
  tools:
    ast-grep:
      rule_dirs: []
      util_dirs: []
      essential_rules: true
      packages: []
    shellcheck:
      enabled: true
    ruff:
      enabled: true
    markdownlint:
      enabled: true
    github-checks:
      enabled: true
      timeout_ms: 90000
    languagetool:
      enabled: true
      enabled_rules: []
      disabled_rules: []
      enabled_categories: []
      disabled_categories: []
      enabled_only: false
      level: default
    biome:
      enabled: true
    hadolint:
      enabled: true
    swiftlint:
      enabled: true
    phpstan:
      enabled: true
      level: default
    phpmd:
      enabled: true
    phpcs:
      enabled: true
    golangci-lint:
      enabled: true
    yamllint:
      enabled: true
    gitleaks:
      enabled: true
    checkov:
      enabled: true
    detekt:
      enabled: true
    eslint:
      enabled: true
    flake8:
      enabled: true
    rubocop:
      enabled: true
    buf:
      enabled: true
    regal:
      enabled: true
    actionlint:
      enabled: true
    pmd:
      enabled: true
    clang:
      enabled: true
    cppcheck:
      enabled: true
    semgrep:
      enabled: true
    circleci:
      enabled: true
    clippy:
      enabled: true
    sqlfluff:
      enabled: true
    prismaLint:
      enabled: true
    pylint:
      enabled: true
    oxc:
      enabled: true
    shopifyThemeCheck:
      enabled: true
    luacheck:
      enabled: true
    brakeman:
      enabled: true
    dotenvLint:
      enabled: true
    htmlhint:
      enabled: true
    checkmake:
      enabled: true
    osvScanner:
      enabled: true
chat:
  art: true
  auto_reply: true
  integrations:
    jira:
      usage: auto
    linear:
      usage: auto
knowledge_base:
  opt_out: false
  web_search:
    enabled: true
  code_guidelines:
    enabled: true
    filePatterns: []
  learnings:
    scope: auto
  issues:
    scope: auto
  jira:
    usage: auto
    project_keys: []
  linear:
    usage: auto
    team_keys: []
  pull_requests:
    scope: auto
  mcp:
    usage: auto
    disabled_servers: []
code_generation:
  docstrings:
    language: en-US
    path_instructions: []
  unit_tests:
    path_instructions: []
issue_enrichment:
  planning:
    enabled: false

Copy link
Contributor

@bmuddha bmuddha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thlorenz thlorenz merged commit f80c598 into master Nov 21, 2025
31 of 32 checks passed
@thlorenz thlorenz deleted the thlorenz/the-great-unborking branch November 21, 2025 12:14
thlorenz added a commit that referenced this pull request Nov 21, 2025
* master:
  feat: detecting accounts stuck in undelegating state and fixing that (#664)
  feat: ActionArgs::default() to abstract escrow_index (#662)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants