fix: don't unbork for same delegation slot as remote slot

[thlorenz]

Summary

Fix incorrect unborking logic when delegation and undelegation occur in the same slot.

Details

We had an account that was delegated in slot X. The only update that we received from that account was also from slot X.
Thus, the logic that was trying to determine if this is a new delegation, assumed that it was since the delegation slot was not smaller than the last remote slot that we had.
This was changed to assume that it could be the old delegation if those slots are equal.

The only downside is that if someone manages to delegate undelegate and re-delegate all in the same slot, we will think that it is still the old delegation and that the account is still undelegating.
However, without access to some delegation index, this case is not avoidable.

Summary by CodeRabbit

• Bug Fixes

  • Improved account undelegation state detection by adjusting the boundary condition for determining when accounts remain in an undelegating state, addressing edge cases that occur during rapid delegation transitions.

• Tests

  • Updated test coverage to validate the adjusted boundary logic and documented edge cases where delegation, undelegation, and redelegation operations may occur within the same time window.

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

[github-actions]

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]

📝 Walkthrough

Walkthrough

The change modifies the boundary condition check in the account_still_undelegating_on_chain function from a strict less-than comparison (<) to a less-than-or-equal comparison (<=). This alters when a bank account is classified as still undelegating, now including the case where delegation_slot equals remote_slot. The corresponding test cases were updated to reflect this modified boundary, with the equality case being commented out and a note added regarding potential issues in scenarios involving delegation, undelegation, and redelegation within the same slot.

Possibly related PRs

• feat: detecting accounts stuck in undelegating state and fixing that #664: Directly modifies the same account_still_undelegating_on_chain function and its associated tests with the boundary check change from < to <=.

Suggested reviewers

• GabrielePicco
• bmuddha

✨ 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/stricter-unborking

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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 (2)

magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (2)

145-155: Remove or replace commented-out test code.

While the note at lines 142-143 provides valuable context about the bug fix, commented-out test code is a code smell. Consider either:

1. Removing lines 145-155 entirely, keeping only the explanatory note
2. Converting this to an active test that validates the new behavior (expecting true when delegation_slot == remote_slot)

Option 1 (preferred): Remove the commented code

-        // NOTE: this case led to incorrect unborking if delegation + undelegation + redelegation
-        // happend all in the same slot
-        // Subcase B1: delegation_slot == remote_slot
-        /*
-        let delegation_slot = 100;
-        let deleg_record = Some(create_delegation_record(delegation_slot));
-        assert!(!account_still_undelegating_on_chain(
-            &pubkey,
-            is_delegated,
-            remote_slot,
-            deleg_record,
-            &Pubkey::default(),
-        ));
-        */
+        // NOTE: When delegation_slot == remote_slot (same-slot operations), we now
+        // treat this as "still undelegating" to avoid incorrect unborking. This is
+        // tested in test_account_undelegation_pending_same_slot below.

Option 2: Convert to test the new behavior

-        // NOTE: this case led to incorrect unborking if delegation + undelegation + redelegation
-        // happend all in the same slot
-        // Subcase B1: delegation_slot == remote_slot
-        /*
+        // NOTE: When delegation_slot == remote_slot, we can't determine order within
+        // the slot, so we conservatively treat it as still undelegating (case D).
+        // This is now tested separately in test_account_undelegation_pending_same_slot.
+        
+        // Subcase B2: delegation_slot > remote_slot (clear re-delegation)
-        let delegation_slot = 100;
-        let deleg_record = Some(create_delegation_record(delegation_slot));
-        assert!(!account_still_undelegating_on_chain(
-            &pubkey,
-            is_delegated,
-            remote_slot,
-            deleg_record,
-            &Pubkey::default(),
-        ));
-        */
-
-        // Subcase B2: delegation_slot > remote_slot

---

194-214: Add explicit test coverage for the same-slot edge case.

The current test_account_undelegation_pending tests delegation_slot < remote_slot (99 < 100), but there's no explicit test for the critical edge case this PR fixes: delegation_slot == remote_slot. This is the core scenario mentioned in the PR description.

Add a new test case:

#[test]
fn test_account_undelegation_pending_same_slot() {
    // Case D (same-slot edge case): The account's undelegation request did not complete.
    // When delegation and remote fetch happen in the same slot, we cannot determine
    // the order of events within that slot, so we conservatively treat it as still
    // undelegating to avoid incorrect unborking.
    // Conditions:
    // - is_delegated: true
    // - delegation_slot == remote_slot (same slot, indeterminate ordering)
    // Expected: true (should NOT override, keep bank account)

    let pubkey = Pubkey::default();
    let is_delegated = true;
    let remote_slot = 100;
    let delegation_slot = 100; // Same slot as remote_slot
    let deleg_record = Some(create_delegation_record(delegation_slot));

    assert!(account_still_undelegating_on_chain(
        &pubkey,
        is_delegated,
        remote_slot,
        deleg_record,
        &Pubkey::default(),
    ));
}

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f207e85 and 87edfaf.

📒 Files selected for processing (1)

• magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (3 hunks)

🧰 Additional context used

🧠 Learnings (5)

📓 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.

📚 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

📚 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

📚 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

🧬 Code graph analysis (1)

magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)

magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)

• remote_slot (145-151)

🔇 Additional comments (1)

magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs (1)

58-58: Boundary condition change appropriately addresses same-slot edge case.

Changing from < to <= correctly treats the equality case (delegation_slot == remote_slot_in_bank) as "still undelegating" rather than "re-delegation completed." When delegation and remote fetch occur in the same slot, we cannot determine the precise ordering of events within that slot, so the conservative approach is to keep the bank state and avoid incorrect unborking.

[bmuddha]

LGTM