Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ serde_json = "1.0"
serde_with = "3.16"
serial_test = "3.2"
sha3 = "0.10.8"
solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "5afcedc" }
solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "1beed4c" }
solana-account-decoder = { version = "2.2" }
solana-accounts-db = { version = "2.2" }
solana-account-decoder-client-types = { version = "2.2" }
Expand Down Expand Up @@ -229,6 +229,6 @@ features = ["dev-context-only-utils"]
# some solana dependencies have solana-storage-proto as dependency
# we need to patch them with our version, because they use protobuf-src v1.1.0
# and we use protobuf-src v2.1.1. Otherwise compilation fails
solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "5afcedc" }
solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "1beed4c" }
solana-storage-proto = { path = "./storage-proto" }
solana-svm = { git = "https://github.com/magicblock-labs/magicblock-svm.git", rev = "86c2cb0f" }
2 changes: 2 additions & 0 deletions magicblock-account-cloner/src/bpf_loader_v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ impl BpfUpgradableProgramModifications {
executable: Some(false),
rent_epoch: Some(u64::MAX),
delegated: Some(false),
confined: Some(false),
}
};

Expand All @@ -69,6 +70,7 @@ impl BpfUpgradableProgramModifications {
executable: Some(true),
rent_epoch: Some(u64::MAX),
delegated: Some(false),
confined: Some(false),
}
};

Expand Down
4 changes: 4 additions & 0 deletions magicblock-account-cloner/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ impl ChainlinkCloner {
data: Some(request.account.data().to_owned()),
executable: Some(request.account.executable()),
delegated: Some(request.account.delegated()),
confined: Some(request.account.confined()),
};

let modify_ix = InstructionUtils::modify_accounts_instruction(vec![
Expand Down Expand Up @@ -238,13 +239,15 @@ impl ChainlinkCloner {
&validator_kp.pubkey(),
);

// Programs aren't marked as confined since they are also never delegated
let pre_deploy_mod_instruction = {
let pre_deploy_mods = vec![AccountModification {
pubkey: program_id,
lamports: Some(lamports),
owner: Some(loader_v4::id()),
executable: Some(true),
data: Some(pre_deploy_loader_state),
confined: Some(false),
..Default::default()
}];
InstructionUtils::modify_accounts_instruction(
Expand All @@ -256,6 +259,7 @@ impl ChainlinkCloner {
let post_deploy_mods = vec![AccountModification {
pubkey: program_id,
data: Some(post_deploy_loader_state),
confined: Some(false),
..Default::default()
}];
InstructionUtils::modify_accounts_instruction(
Expand Down
155 changes: 131 additions & 24 deletions magicblock-chainlink/src/chainlink/fetch_cloner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -419,19 +419,14 @@ where
delegation_record.owner
);
}
let is_delegated_to_us = delegation_record
.authority
.eq(&self.validator_pubkey) ||
// TODO(thlorenz): @ once the delegation program supports
// delegating to specific authority we need to remove the below
delegation_record.authority.eq(&Pubkey::default());

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

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


// For accounts delegated to us, always unsubscribe from the delegated account
if is_delegated_to_us {
if account.delegated() {
subs_to_remove.insert(pubkey);
}

Expand Down Expand Up @@ -500,6 +495,31 @@ where
})
}

/// Applies delegation record settings to an account: sets the owner,
/// delegation status, and confined status based on the delegation
/// record's authority field.
/// Returns commit frequency if account is delegated to us
fn apply_delegation_record_to_account(
&self,
account: &mut ResolvedAccountSharedData,
delegation_record: &DelegationRecord,
) -> Option<u64> {
let is_confined = delegation_record.authority.eq(&Pubkey::default());
let is_delegated_to_us =
delegation_record.authority.eq(&self.validator_pubkey)
|| is_confined;

account
.set_owner(delegation_record.owner)
.set_confined(is_confined)
.set_delegated(is_delegated_to_us);
if is_delegated_to_us {
Some(delegation_record.commit_frequency_ms)
} else {
None
}
}

/// Fetches and parses the delegation record for an account, returning the
/// parsed DelegationRecord if found and valid, None otherwise.
async fn fetch_and_parse_delegation_record(
Expand Down Expand Up @@ -866,20 +886,11 @@ where
};

trace!("Delegation record found for {pubkey}: {delegation_record:?}");
let is_delegated_to_us = delegation_record
.authority
.eq(&self.validator_pubkey) ||
// TODO(thlorenz): @ once the delegation program supports
// delegating to specific authority we need to remove the below
delegation_record.authority.eq(&Pubkey::default());
account
.set_owner(delegation_record.owner)
.set_delegated(is_delegated_to_us);
if is_delegated_to_us {
Some(delegation_record.commit_frequency_ms)
} else {
None
}

self.apply_delegation_record_to_account(
&mut account,
&delegation_record,
)
} else {
missing_delegation_record
.push((pubkey, account.remote_slot()));
Expand Down Expand Up @@ -3032,6 +3043,102 @@ mod tests {
);
}

#[tokio::test]
async fn test_confined_delegation_behavior() {
init_logger();
let validator_pubkey = random_pubkey();
let account_owner = random_pubkey();
const CURRENT_SLOT: u64 = 100;

// Account 1: Delegated to validator authority -> Not confined
let account1_pubkey = random_pubkey();
let account1 = Account {
lamports: 1_000_000,
data: vec![1, 2, 3],
owner: dlp::id(), // Owned by DLP initially (as it is delegated)
executable: false,
rent_epoch: 0,
};

// Account 2: Delegated to default pubkey -> Confined
let account2_pubkey = random_pubkey();
let account2 = Account {
lamports: 2_000_000,
data: vec![4, 5, 6],
owner: dlp::id(),
executable: false,
rent_epoch: 0,
};

let FetcherTestCtx {
accounts_bank,
fetch_cloner,
rpc_client,
..
} = setup(
[
(account1_pubkey, account1.clone()),
(account2_pubkey, account2.clone()),
],
CURRENT_SLOT,
validator_pubkey,
)
.await;

// Add delegation record for Account 1 (Authority = Validator)
add_delegation_record_for(
&rpc_client,
account1_pubkey,
validator_pubkey,
account_owner,
);

// Add delegation record for Account 2 (Authority = Default/System)
add_delegation_record_for(
&rpc_client,
account2_pubkey,
Pubkey::default(),
account_owner,
);

// Fetch and clone both accounts
fetch_cloner
.fetch_and_clone_accounts(
&[account1_pubkey, account2_pubkey],
None,
None,
AccountFetchOrigin::GetAccount,
None,
)
.await
.expect("Failed to fetch and clone accounts");

// Verify not confined Account
let cloned_account1 = accounts_bank
.get_account(&account1_pubkey)
.expect("Account 1 not found");
assert!(cloned_account1.delegated(), "Account 1 should be delegated");
assert!(
!cloned_account1.confined(),
"Account 1 (delegated to validator) should NOT be confined"
);
assert_eq!(cloned_account1.owner(), &account_owner);

// Verify confined Account
let cloned_account2 = accounts_bank
.get_account(&account2_pubkey)
.expect("Account 2 not found");
assert!(
cloned_account2.delegated(),
"Account 2 should be delegated (to us, via confinement)"
);
assert!(
cloned_account2.confined(),
"Account 2 (delegated to default) SHOULD be confined"
);
assert_eq!(cloned_account2.owner(), &account_owner);
}

#[tokio::test]
async fn test_fetch_and_clone_undelegating_account_that_is_closed_on_chain()
{
Expand Down
17 changes: 17 additions & 0 deletions magicblock-chainlink/src/remote_account_provider/remote_account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,23 @@ impl ResolvedAccountSharedData {
self
}

pub fn confined(&self) -> bool {
use ResolvedAccountSharedData::*;
match self {
Fresh(account) => account.confined(),
Bank(account) => account.confined(),
}
}

pub fn set_confined(&mut self, confined: bool) -> &mut Self {
use ResolvedAccountSharedData::*;
match self {
Fresh(account) => account.set_confined(confined),
Bank(account) => account.set_confined(confined),
}
self
}

pub fn set_remote_slot(&mut self, remote_slot: Slot) -> &mut Self {
use ResolvedAccountSharedData::*;
match self {
Expand Down
2 changes: 2 additions & 0 deletions magicblock-magic-program-api/src/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ pub struct AccountModification {
// https://github.com/magicblock-labs/magicblock-validator/issues/580
pub rent_epoch: Option<u64>,
pub delegated: Option<bool>,
pub confined: Option<bool>,
}

#[derive(Default, Clone, Serialize, Deserialize, Debug, PartialEq, Eq)]
Expand All @@ -131,4 +132,5 @@ pub struct AccountModificationForInstruction {
// https://github.com/magicblock-labs/magicblock-validator/issues/580
pub rent_epoch: Option<u64>,
pub delegated: Option<bool>,
pub confined: Option<bool>,
}
11 changes: 11 additions & 0 deletions programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,14 @@ pub(crate) fn process_mutate_accounts(
);
account.borrow_mut().set_delegated(delegated);
}
if let Some(confined) = modification.confined {
ic_msg!(
invoke_context,
"MutateAccounts: setting confined to {}",
confined
);
account.borrow_mut().set_confined(confined);
}
}

if lamports_to_debit != 0 {
Expand Down Expand Up @@ -320,6 +328,7 @@ mod tests {
data: Some(vec![1, 2, 3, 4, 5]),
rent_epoch: None,
delegated: Some(true),
confined: Some(true),
};
let ix = InstructionUtils::modify_accounts_instruction(vec![
modification.clone(),
Expand All @@ -346,6 +355,7 @@ mod tests {
let account_authority: AccountSharedData =
accounts.drain(0..1).next().unwrap();
assert!(!account_authority.delegated());
assert!(!account_authority.confined());
assert_matches!(
account_authority.into(),
Account {
Expand All @@ -363,6 +373,7 @@ mod tests {
let modified_account: AccountSharedData =
accounts.drain(0..1).next().unwrap();
assert!(modified_account.delegated());
assert!(modified_account.confined());
assert_matches!(
modified_account.into(),
Account {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,16 @@ pub(crate) fn process_schedule_commit(
get_instruction_pubkey_with_idx(transaction_context, idx as u16)?;
let acc =
get_instruction_account_with_idx(transaction_context, idx as u16)?;

if acc.borrow().confined() {
ic_msg!(
invoke_context,
"ScheduleCommit ERR: account {} is confined and cannot be committed",
acc_pubkey
);
return Err(InstructionError::InvalidAccountData);
}

{
if opts.request_undelegation {
// Check if account is writable and also undelegated
Expand Down
Loading