Skip to content

feat: prevent commits for confined accounts - #690

Merged
thlorenz merged 12 commits into
masterfrom
thlorenz/confined
Dec 2, 2025
Merged

thlorenz merged 12 commits into
masterfrom
thlorenz/confined

Conversation

@thlorenz

@thlorenz thlorenz commented Nov 27, 2025 •

Copy link
Copy Markdown
Contributor

Summary

Implementing support for the "confined" account flag to prevent committing delegated accounts
that are marked as confined. Confined accounts are identified when their delegation authority
is set to the default pubkey (system program).

Details

Core Changes

  • Updated solana-account dependency: Upgraded to a version that includes the confined flag support
  • Added confined field to account modifications: Extended AccountModification and AccountModificationForInstruction structures to include the confined flag
  • Implemented confined flag in ResolvedAccountSharedData: Added getter and setter methods to support confined status on both Fresh and Bank account variants

Commit Prevention

Added validation in process_schedule_commit to reject any attempt to schedule commits for confined accounts, returning InvalidAccountData error when detected.

Tests

Added comprehensive test coverage for confined behavior:

  • Tests verifying that accounts delegated to validator authority are not confined
  • Tests verifying that accounts delegated to default authority (via confinement) are properly marked
  • Tests preventing schedule commit operations on confined accounts
  • Tests ensuring confinement detection works across multiple account scenarios

Summary by CodeRabbit

  • New Features

    • Added a confined state to account modification data and accessors to read/set confinement
    • Centralized delegation handling to propagate confined and delegated states consistently
  • Bug Fixes

    • Scheduling now rejects confined accounts to prevent invalid commits
  • Dependencies

    • Updated solana-account dependency
  • Tests

    • Added tests for confined delegation behavior and schedule-commit rejection scenarios

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

@coderabbitai

coderabbitai Bot commented Nov 27, 2025 •

Copy link
Copy Markdown
Contributor

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

Adds an optional boolean field confined to AccountModification and AccountModificationForInstruction, propagates it through API types, account-cloning paths (including BPF loader deploy), chainlink delegation handling, remote account accessors, mutation processing, and instruction construction. process_mutate_accounts now applies the confined flag to accounts; process_schedule_commit rejects confined accounts early. New tests cover confined delegation behavior and schedule-commit rejection. Workspace and patch entries for the solana-account git rev were bumped.

Possibly related PRs

Suggested reviewers

  • GabrielePicco
  • Dodecahedr0x
✨ 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/confined

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.

@github-actions

github-actions Bot commented Nov 27, 2025 •

Copy link
Copy Markdown
Contributor

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR implements support for the "confined" account flag to prevent committing delegated accounts that are marked as confined. Confined accounts are those delegated to the default pubkey (system program) rather than to a specific validator authority.

Key changes:

  • Upgraded solana-account dependency to include confined flag support
  • Extended account modification structures to include the confined field
  • Added validation to reject schedule commit operations on confined accounts

Reviewed changes

Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
Cargo.toml Updated solana-account dependency to version with confined flag support
Cargo.lock Locked solana-account dependency to new revision
test-integration/Cargo.toml Updated solana-account dependency in test integration workspace
test-integration/Cargo.lock Locked solana-account dependency in test integration workspace
magicblock-magic-program-api/src/instruction.rs Added confined field to AccountModification and AccountModificationForInstruction structs
magicblock-chainlink/src/remote_account_provider/remote_account.rs Implemented confined getter and setter methods on ResolvedAccountSharedData
magicblock-chainlink/src/chainlink/fetch_cloner.rs Refactored delegation record application logic and added confined flag handling
programs/magicblock/src/utils/instruction_utils.rs Added confined field mapping in instruction account modification processing
programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs Added confined field handling and test assertions
programs/magicblock/src/schedule_transactions/process_schedule_commit.rs Added validation to reject confined accounts during commit scheduling
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs Added tests for confined account validation
magicblock-account-cloner/src/lib.rs Added confined field to account modifications during cloning
magicblock-account-cloner/src/bpf_loader_v1.rs Added confined field to BPF loader account modifications

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 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 9656446 and 1ac8674.

📒 Files selected for processing (1)
  • programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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-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/schedule_transactions/process_schedule_commit_tests.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_tests.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:

  • programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
🧬 Code graph analysis (1)
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs (2)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (2)
  • confined (112-118)
  • delegated (95-101)
programs/magicblock/src/utils/instruction_utils.rs (1)
  • schedule_commit_instruction (43-59)

@bmuddha bmuddha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@GabrielePicco GabrielePicco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

* master:
  fix: don't unbork for same delegation slot as remote slot (#702)
  feat: use simplified configuration management (#685)
  Report Intent's patched errors (#667)
  fix: replace cargo-expand with syn-based verification (#611)
  fix: make db thread safe (#680)
  fix: reset program cache upon error (#696)
  chore: use smaller runners (#695)
  feat: cranked commits (#656)
  fix: refresh stuck undelegating accounts that are closed on-chain (#691)
  Fix: abort truncation if validator exited (#689)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (2)
programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs (2)

864-905: Single-account confined failure path is well specified

This test correctly sets the committee as both confined and delegated, asserts that invariant, and verifies ScheduleCommit fails with InvalidAccountData. It directly exercises the new confinement guard in process_schedule_commit.


907-959: Multi-account confined rejection is covered and asserts both flags

The three-account test now marks only committee_dos as confined, asserts both confined() and delegated() on that account, and confirms the whole ScheduleCommit fails with InvalidAccountData. This mirrors the single-account case and documents the invariant for the multi-account scenario.

📜 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 1ac8674 and 103322a.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • test-integration/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • Cargo.toml (2 hunks)
  • magicblock-account-cloner/src/lib.rs (3 hunks)
  • magicblock-chainlink/src/chainlink/fetch_cloner.rs (4 hunks)
  • magicblock-magic-program-api/src/instruction.rs (2 hunks)
  • programs/magicblock/src/schedule_transactions/process_schedule_commit.rs (1 hunks)
  • programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs (1 hunks)
  • programs/magicblock/src/utils/instruction_utils.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 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
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:

  • programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.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:

  • programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.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:

  • programs/magicblock/src/schedule_transactions/process_schedule_commit_tests.rs
  • magicblock-magic-program-api/src/instruction.rs
  • programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
  • magicblock-account-cloner/src/lib.rs
  • programs/magicblock/src/utils/instruction_utils.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_tests.rs
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.

Applied to files:

  • programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
📚 Learning: 2025-11-12T09:46:27.553Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.

Applied to files:

  • programs/magicblock/src/schedule_transactions/process_schedule_commit.rs
📚 Learning: 2025-11-13T09:38:43.804Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:64-102
Timestamp: 2025-11-13T09:38:43.804Z
Learning: In magicblock-processor's TransactionScheduler (scheduler/mod.rs line 59), the executor count is clamped to MAX_SVM_EXECUTORS (63) at initialization time, and executor IDs are assigned sequentially from 0 to count-1. This architectural guarantee ensures that executor IDs used in the bitmask-based AccountLock (scheduler/locks.rs) will always be within valid bounds for bit shifting operations, making runtime bounds checks unnecessary.

Applied to files:

  • programs/magicblock/src/schedule_transactions/process_schedule_commit.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:

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

Applied to files:

  • magicblock-chainlink/src/chainlink/fetch_cloner.rs
📚 Learning: 2025-11-24T08:45:11.164Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 675
File: Cargo.toml:145-145
Timestamp: 2025-11-24T08:45:11.164Z
Learning: The reqwest dependency version 0.11 is intentionally used in the magicblock-validator project to maintain compatibility with the Solana client dependencies.

Applied to files:

  • Cargo.toml
📚 Learning: 2025-11-24T14:21:00.996Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.996Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.

Applied to files:

  • Cargo.toml
🧬 Code graph analysis (2)
magicblock-account-cloner/src/lib.rs (1)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (1)
  • confined (112-118)
magicblock-chainlink/src/chainlink/fetch_cloner.rs (1)
magicblock-chainlink/src/remote_account_provider/remote_account.rs (5)
  • account (197-219)
  • delegated (95-101)
  • confined (112-118)
  • owner (54-60)
  • owner (260-262)
🔇 Additional comments (9)
Cargo.toml (1)

156-156: solana-account git revision exists and is up-to-date.

The solana-account dependency has been consistently updated to revision 1beed4c in both [workspace.dependencies] (line 156) and [patch.crates-io] (line 232). The revision exists in the repository and is currently at HEAD of the main branch.

Also applies to: 232-232

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

133-145: Early confinement check is correctly placed and typed

Checking acc.borrow().confined() before delegation/ownership logic and failing with InvalidAccountData cleanly enforces “confined accounts cannot be scheduled for commit” and aligns with the new tests’ expectations. No functional or ergonomics issues spotted here.

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

171-189: Confined flag propagation into instruction payload is correct

Passing confined: account_modification.confined into AccountModificationForInstruction keeps the wire format consistent with the API struct and ensures confined state is not silently dropped.

magicblock-account-cloner/src/lib.rs (2)

94-107: Cloner now preserves confined state for regular accounts

Including confined: Some(request.account.confined()) in the AccountModification ensures cloned regular accounts in the validator reflect the source confinement state, which is required for ScheduleCommit to enforce confinement consistently.


242-263: Explicitly marking cloned programs as non-confined is appropriate

Setting confined: Some(false) in both pre- and post-deploy program modifications matches the fact that programs are not delegated and therefore should never be treated as confined. This avoids accidental confinement flags on program accounts without changing other semantics.

magicblock-magic-program-api/src/instruction.rs (2)

111-123: AccountModification API cleanly extends with confined flag

Adding pub confined: Option<bool> as a trailing field on AccountModification is backward-compatible within the workspace and matches how other optional account attributes are modeled. Defaulting to None cleanly encodes “unspecified.”


125-136: Instruction-side AccountModification also mirrors confined state

Introducing confined: Option<bool> on AccountModificationForInstruction keeps the on-wire representation in sync with the public API struct and enables instructions to carry confinement state end-to-end. The default/serde derives remain valid.

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

498-521: LGTM! Well-structured refactoring of delegation logic.

The helper function correctly centralizes the delegation record application logic and clearly captures the confined account semantics: accounts with authority == Pubkey::default() are considered confined and treated as delegated to the validator for local control, while still being marked for commit prevention elsewhere in the codebase.

The method chaining approach is idiomatic and the return of commit_frequency_ms only when delegated to us is appropriate.


3046-3140: LGTM! Comprehensive test coverage for confined delegation behavior.

The test effectively verifies the key distinction between regular delegation (authority = validator) and confined delegation (authority = default/system program). Both scenarios correctly assert the delegated() and confined() states, along with proper owner restoration from the delegation record.

This test provides good coverage for the new confined account semantics introduced in this PR.

Comment on lines +423 to +426
self.apply_delegation_record_to_account(
&mut account,
&delegation_record,
);

@coderabbitai coderabbitai Bot Dec 2, 2025 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Make unused return value explicit for clarity.

In this subscription update context, the commit_frequency_ms return value is intentionally unused (line 273 hardcodes it to None). However, for clarity and to avoid potential clippy warnings, consider explicitly discarding the return value:

-                                self.apply_delegation_record_to_account(
-                                    &mut account,
-                                    &delegation_record,
-                                );
+                                let _ = self.apply_delegation_record_to_account(
+                                    &mut account,
+                                    &delegation_record,
+                                );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
self.apply_delegation_record_to_account(
&mut account,
&delegation_record,
);
let _ = self.apply_delegation_record_to_account(
&mut account,
&delegation_record,
);
🤖 Prompt for AI Agents
magicblock-chainlink/src/chainlink/fetch_cloner.rs around lines 423-426, the
call to apply_delegation_record_to_account returns a value (commit_frequency_ms)
that is intentionally unused; explicitly discard the return to make the intent
clear and silence clippy by assigning it to the underscore or using drop, e.g.
replace the bare call with let _ = self.apply_delegation_record_to_account(&mut
account, &delegation_record); (or
drop(self.apply_delegation_record_to_account(...))) so the unused value is
explicit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should only be done for results, but here we just don't need the commit frequency, so it's fine to just ignore the return value.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@thlorenz, you're absolutely right. The let _ = ... pattern is conventionally used for Result types to make it explicit that you're intentionally ignoring potential errors. Since apply_delegation_record_to_account returns Option<u64> (not a Result), it's perfectly idiomatic in Rust to call it without explicitly discarding the return value when you don't need it. The current code is fine as-is.


🧠 Learnings used
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.

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-chainlink/tests/ix_01_ensure-accounts.rs:85-114
Timestamp: 2025-11-19T11:31:24.218Z
Learning: In the magicblock-validator codebase, compressed delegation records are never subscribed to by design. Tests for compressed delegation flows should focus on verifying that the delegated account (e.g., counter PDA) is cloned as delegated and not subscribed, but do not need to check for subscription to delegation-record-like PDAs in the compressed path because subscriptions to compressed delegation records never occur.

Learnt from: 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.

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.

@thlorenz
thlorenz merged commit 02fd8fe into master Dec 2, 2025
18 checks passed
@thlorenz
thlorenz deleted the thlorenz/confined branch December 2, 2025 09:34
Dodecahedr0x pushed a commit that referenced this pull request Dec 2, 2025
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.

4 participants