diff --git a/magicblock-chainlink/src/accounts_bank.rs b/magicblock-chainlink/src/accounts_bank.rs index 2d9f81510..c4aadfb57 100644 --- a/magicblock-chainlink/src/accounts_bank.rs +++ b/magicblock-chainlink/src/accounts_bank.rs @@ -51,6 +51,7 @@ pub mod mock { } pub fn undelegate(&self, pubkey: &Pubkey) -> &Self { + self.set_undelegating(pubkey, true); self.set_delegated(pubkey, false) } @@ -64,6 +65,21 @@ pub mod mock { self.set_owner(pubkey, dlp::id()).undelegate(pubkey); } + pub fn set_undelegating( + &self, + pubkey: &Pubkey, + undelegating: bool, + ) -> &Self { + trace!("Set account {pubkey} undelegating={undelegating}"); + let mut accounts = self.accounts.lock().unwrap(); + if let Some(account) = accounts.get_mut(pubkey) { + account.set_undelegating(undelegating); + } else { + panic!("Account not found in bank: {pubkey}"); + } + self + } + #[allow(dead_code)] pub fn dump_account_keys(&self, include_blacklisted: bool) -> String { let mut output = String::new(); diff --git a/magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs b/magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs new file mode 100644 index 000000000..615020773 --- /dev/null +++ b/magicblock-chainlink/src/chainlink/account_still_undelegating_on_chain.rs @@ -0,0 +1,211 @@ +use dlp::state::DelegationRecord; +use log::*; +use solana_pubkey::Pubkey; + +/// Decides if an account that is undelegating should be updated +/// (overwritten) by the remote account state and the `undelegating` flag cleared. +/// +/// The only case when an account should not be updated is when the following is true: +/// +/// - account is still delegated to us on chain +/// - the delegation slot is older than the slot at which we last fetched +/// the account state from chain. +/// +/// # Arguments +/// - `pubkey`: the account pubkey +/// - `is_delegated_on_chain`: whether the account is currently delegated to us on chain +/// - `remote_slot_in_bank`: the chain slot at which we last fetched and cloned state +/// of the account in our bank +/// - `delegation_record`: the delegation record associated with the account in our bank, if found +/// - `validator_auth`: the validator authority pubkey +/// - returns `true` if the account is still undelegating, `false` otherwise. +pub(crate) fn account_still_undelegating_on_chain( + pubkey: &Pubkey, + is_delegated_to_us_on_chain: bool, + remote_slot_in_bank: u64, + deleg_record: Option, + validator_auth: &Pubkey, +) -> bool { + // In the case of a subscription update for an account that was undelegating + // we know that the undelegation or associated commit or possibly a previous + // commit made after we subscribed to the account was completed, otherwise + // there would be no update. + // + // Now the account could be in one the following states: + // + // A) the account was undelegated and remained so + // B) the account was undelegated and was re-delegated to us or the system + // program (broadcast account) + // C) the account was undelegated and was re-delegated to another validator + // D) the account's undelegation request did not complete. + // In case of a subscription update the commit (or a commit scheduled previously) did trigger an update. + // Alternatively someone may be accessing the account while undelegation is still pending. + // Thus the account is still delegated to us on chain. + // + // In the case of D) we want to keep the bank version of the account. + // + // In all other cases we want to clone the remote version of the account into + // our bank which will automatically set the correct delegated state and + // untoggle the undelegating flag. + if is_delegated_to_us_on_chain { + // B) or D) + // Since the account was found to be delegated we must have + // found a delegation record and thus have the delegation slot. + let delegation_slot = deleg_record + .as_ref() + .map(|d| d.delegation_slot) + .unwrap_or_default(); + if delegation_slot < remote_slot_in_bank { + // The last update of the account was after the last delegation + // Therefore the account was not redelegated which indicates + // that the undelegation is still not completed. Case (D)) + debug!( + "Undelegation for {pubkey} is still pending. Keeping bank account.", + ); + true + } else { + // This is a re-delegation to us after undelegation completed. + // Case (B)) + debug!( + "Undelegation completed for account {pubkey} and it was re-delegated to us at slot: ({delegation_slot}).", + ); + magicblock_metrics::metrics::inc_undelegation_completed(); + false + } + } else if let Some(deleg_record) = deleg_record { + // Account delegated to other (Case C)) -> clone as is + debug!( + "Account {pubkey} was undelegated and re-delegated to another validator. authority: {}, delegated_to: {}", + validator_auth, deleg_record.authority + ); + magicblock_metrics::metrics::inc_undelegation_completed(); + false + } else { + // Account no longer delegated (Case A)) -> clone as is + debug!("Account {pubkey} was undelegated and remained so"); + magicblock_metrics::metrics::inc_undelegation_completed(); + false + } +} + +#[cfg(test)] +mod tests { + use dlp::state::DelegationRecord; + use solana_pubkey::Pubkey; + + use super::*; + + fn create_delegation_record(delegation_slot: u64) -> DelegationRecord { + DelegationRecord { + authority: Pubkey::default(), + owner: Pubkey::default(), + delegation_slot, + lamports: 1000, + commit_frequency_ms: 100, + } + } + + #[test] + fn test_account_was_undelegated_and_remained_so() { + // Case A: The account was undelegated and remained so. + // Conditions: + // - is_delegated: false (account is not delegated to us on chain) + // - deleg_record: None (no delegation record associated) + // Expected: true (should override/clone as is) + + let pubkey = Pubkey::default(); + let is_delegated = false; + let remote_slot = 100; + let deleg_record = None; + + assert!(!account_still_undelegating_on_chain( + &pubkey, + is_delegated, + remote_slot, + deleg_record, + &Pubkey::default(), + )); + } + + #[test] + fn test_account_was_undelegated_and_redelegated_to_us() { + // Case B: The account was undelegated and was re-delegated to us. + // Conditions: + // - is_delegated: true (account is delegated to us on chain) + // - delegation_slot >= remote_slot (delegation happend after we last updated the account) + // Expected: true (should override/update) + + let pubkey = Pubkey::default(); + let is_delegated = true; + let remote_slot = 100; + + // 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(), + )); + + // Subcase B2: delegation_slot > remote_slot + let delegation_slot = 101; + let deleg_record = Some(create_delegation_record(delegation_slot)); + assert!(!account_still_undelegating_on_chain( + &pubkey, + is_delegated, + remote_slot, + deleg_record, + &Pubkey::default(), + )); + } + + #[test] + fn test_account_was_undelegated_and_redelegated_to_another() { + // Case C: The account was undelegated and then re-delegated to another validator. + // Conditions: + // - is_delegated: false (not delegated to us on chain) + // - deleg_record: Some(...) (but record exists, maybe describing delegation to another) + // Expected: true (should override/clone as is) + + let pubkey = Pubkey::default(); + let is_delegated = false; + let remote_slot = 100; + // Value doesn't matter for this path + let delegation_slot = 90; + let deleg_record = Some(create_delegation_record(delegation_slot)); + + assert!(!account_still_undelegating_on_chain( + &pubkey, + is_delegated, + remote_slot, + deleg_record, + &Pubkey::default(), + )); + } + + #[test] + fn test_account_undelegation_pending() { + // Case D: The account's undelegation request did not complete. + // Conditions: + // - is_delegated: true + // - delegation_slot < remote_slot (delegation is older than remote update, implying pending undelegation) + // Expected: false (should NOT override, keep bank account) + + let pubkey = Pubkey::default(); + let is_delegated = true; + let remote_slot = 100; + let delegation_slot = 99; + let deleg_record = Some(create_delegation_record(delegation_slot)); + + assert!(account_still_undelegating_on_chain( + &pubkey, + is_delegated, + remote_slot, + deleg_record, + &Pubkey::default(), + )); + } +} diff --git a/magicblock-chainlink/src/chainlink/fetch_cloner.rs b/magicblock-chainlink/src/chainlink/fetch_cloner.rs index ea5cfa441..f5c6fa416 100644 --- a/magicblock-chainlink/src/chainlink/fetch_cloner.rs +++ b/magicblock-chainlink/src/chainlink/fetch_cloner.rs @@ -12,6 +12,7 @@ use dlp::{ }; use log::*; use magicblock_core::traits::AccountsBank; +use magicblock_metrics::metrics; use solana_account::{AccountSharedData, ReadableAccount}; use solana_pubkey::Pubkey; use solana_sdk::system_program; @@ -23,7 +24,10 @@ use tokio::{ use super::errors::{ChainlinkError, ChainlinkResult}; use crate::{ - chainlink::blacklisted_accounts::blacklisted_accounts, + chainlink::{ + account_still_undelegating_on_chain::account_still_undelegating_on_chain, + blacklisted_accounts::blacklisted_accounts, + }, cloner::{errors::ClonerResult, Cloner}, remote_account_provider::{ program_account::{ @@ -177,10 +181,10 @@ where // TODO: if we get a lot of subs and cannot keep up we need to put this // on a separate task so the fetches of delegation records can happen in // parallel - let resolved_account = + let (resolved_account, deleg_record) = self.resolve_account_to_clone_from_forwarded_sub_with_unsubscribe(update) .await; - if let Some(mut account) = resolved_account { + if let Some(account) = resolved_account { // Ensure that the subscription update isn't out of order, i.e. we don't already // hold a newer version of the account in our bank let out_of_order_slot = self @@ -201,6 +205,41 @@ where continue; } + if let Some(in_bank) = + self.accounts_bank.get_account(&pubkey) + { + if in_bank.undelegating() { + // We expect the account to still be delegated, but with the delegation + // program owner + debug!("Received update for undelegating account {pubkey} delegated in bank={} delegated on chain={}", in_bank.delegated(), account.delegated()); + + // This will only be true in the following case: + // 1. a commit was triggered for the account + // 2. a commit + undelegate was triggered for the account -> undelegating + // 3. we receive the update for (1.) + // + // Thus our state is more up to date and we don't need to update our + // bank. + if account_still_undelegating_on_chain( + &pubkey, + account.delegated(), + in_bank.remote_slot(), + deleg_record, + &self.validator_pubkey, + ) { + continue; + } + } else if in_bank.owner().eq(&dlp::id()) { + debug!( + "Received update for {pubkey} owned by deleg program not marked as undelegating" + ); + } + } else { + warn!( + "Received update for {pubkey} which is not in bank" + ); + } + // Once we clone an account that is delegated to us we no longer need // to receive updates for it from chain // The subscription will be turned back on once the committor service schedules @@ -216,29 +255,6 @@ where ); } } - // Check if this is an undelegation completion - // Conditions: - // 1. In bank: account is delegated - // 2. In bank: owner is dlp::id() indicating undelegation was triggered - // 3. In update: owner is not dlp::id() - // NOTE: this check will be simpler once we have the `undelegating` flag - if let Some(in_bank) = - self.accounts_bank.get_account(&pubkey) - { - if in_bank.delegated() - && in_bank.owner().eq(&dlp::id()) - && !account.owner().eq(&dlp::id()) - { - debug!( - "Undelegation completed for account: {pubkey}" - ); - magicblock_metrics::metrics::inc_undelegation_completed(); - } - } - - // When cloning from subscription update, reset undelegating flag - // since the subscription update reflects current chain state - account.set_undelegating(false); if account.executable() { self.handle_executable_sub_update(pubkey, account) @@ -323,7 +339,7 @@ where async fn resolve_account_to_clone_from_forwarded_sub_with_unsubscribe( &self, update: ForwardedSubscriptionUpdate, - ) -> Option { + ) -> (Option, Option) { let ForwardedSubscriptionUpdate { pubkey, account } = update; let owned_by_delegation_program = account.is_owned_by_delegation_program(); @@ -405,17 +421,20 @@ where subs_to_remove.insert(pubkey); } - Some(account.into_account_shared_data()) + ( + Some(account.into_account_shared_data()), + Some(delegation_record), + ) } else { // If the delegation record is invalid we cannot clone the account // since something is corrupt and we wouldn't know what owner to // use, etc. - None + (None, None) } } else { // If no delegation record exists we must assume the account itself is // a delegation record or metadata - Some(account.into_account_shared_data()) + (Some(account.into_account_shared_data()), None) }; if !subs_to_remove.is_empty() { @@ -430,23 +449,23 @@ where // In case of errors fetching the delegation record we cannot clone the account Ok(Err(err)) => { error!("failed to fetch delegation record for {pubkey}: {err}. not cloning account."); - None + (None, None) } Err(err) => { error!("failed to fetch delegation record for {pubkey}: {err}. not cloning account."); - None + (None, None) } } } else { // Accounts not owned by the delegation program can be cloned as is // No unsubscription needed for undelegated accounts - Some(account) + (Some(account), None) } } else { // This should not happen since we call this method with sub updates which always hold // a fresh remote account error!("BUG: Received subscription update for {pubkey} without fresh account: {account:?}"); - None + (None, None) } } @@ -476,8 +495,11 @@ where ) -> Option { let delegation_record_pubkey = delegation_record_pda_from_delegated_account(&account_pubkey); + let was_watching_deleg_record = self + .remote_account_provider + .is_watching(&delegation_record_pubkey); - match self + let res = match self .remote_account_provider .try_get_multi_until_slots_match( &[delegation_record_pubkey], @@ -506,25 +528,30 @@ where } } Err(_) => None, - } - } + }; - /// Checks if an account marked as undelegating is still delegated to our - /// validator. If not, returns false to indicate the account should be - /// refetched from chain. If still delegated to us, returns true to indicate - /// the bank version should be used. - async fn is_still_delegated_to_us(&self, pubkey: Pubkey) -> bool { - let min_context_slot = self.remote_account_provider.chain_slot(); - match self - .fetch_and_parse_delegation_record(pubkey, min_context_slot) - .await + if !was_watching_deleg_record + // Handle edge case where it was cloned in the meantime. + // The small possiblility of a fetch + clone of this delegation record being in process + // still exits, but it's negligible + && self + .accounts_bank + .get_account(&delegation_record_pubkey) + .is_none() { - Some(delegation_record) => { - delegation_record.authority.eq(&self.validator_pubkey) - || delegation_record.authority.eq(&Pubkey::default()) + // We only subscribed to fetch the delegation record, so unsubscribe now + if let Err(err) = self + .remote_account_provider + .unsubscribe(&delegation_record_pubkey) + .await + { + error!( + "Failed to unsubscribe from delegation record {delegation_record_pubkey}: {err}" + ); } - None => false, } + + res } /// Tries to fetch all accounts in `pubkeys` and clone them into the bank. @@ -535,6 +562,8 @@ where /// - **mark_empty**: optional list of accounts that should be added as empty if not found on /// chain /// - **slot**: optional slot to use as minimum context slot for the accounts being cloned + /// + /// NOTE: accounts fetched here have not been found in the bank async fn fetch_and_clone_accounts( &self, pubkeys: &[Pubkey], @@ -579,12 +608,11 @@ where trace!("Fetched {accs:?}"); - let (not_found, in_bank, plain, owned_by_deleg, programs) = + let (not_found, plain, owned_by_deleg, programs) = accs.into_iter().zip(pubkeys).fold( - (vec![], vec![], vec![], vec![], vec![]), + (vec![], vec![], vec![], vec![]), |( mut not_found, - mut in_bank, mut plain, mut owned_by_deleg, mut programs, @@ -634,13 +662,13 @@ where )); } } - ResolvedAccount::Bank(pubkey) => { - in_bank.push(pubkey); + ResolvedAccount::Bank((pubkey, slot)) => { + error!("We should not be fetching accounts that are already in bank: {pubkey}:{slot}"); } }; } } - (not_found, in_bank, plain, owned_by_deleg, programs) + (not_found, plain, owned_by_deleg, programs) }, ); @@ -649,10 +677,6 @@ where .iter() .map(|(pubkey, slot)| (pubkey.to_string(), *slot)) .collect::>(); - let in_bank = in_bank - .iter() - .map(|(p, _)| p.to_string()) - .collect::>(); let plain = plain.iter().map(|(p, _)| p.to_string()).collect::>(); let owned_by_deleg = owned_by_deleg @@ -664,7 +688,7 @@ where .map(|(p, _, _)| p.to_string()) .collect::>(); trace!( - "Fetched accounts: \nnot_found: {not_found:?} \nin_bank: {in_bank:?} \nplain: {plain:?} \nowned_by_deleg: {owned_by_deleg:?}\nprograms: {programs:?}", + "Fetched accounts: \nnot_found: {not_found:?} \nplain: {plain:?} \nowned_by_deleg: {owned_by_deleg:?}\nprograms: {programs:?}", ); } @@ -689,17 +713,6 @@ where ); } - // For accounts already in bank we don't need to do anything - if log::log_enabled!(log::Level::Trace) { - trace!( - "Accounts already in bank: {:?}", - in_bank - .iter() - .map(|(p, _)| p.to_string()) - .collect::>() - ); - } - // We mark some accounts as empty if we know that they will never exist on chain if log::log_enabled!(log::Level::Trace) && !clone_as_empty.is_empty() { trace!( @@ -711,46 +724,6 @@ where ); } - // For accounts in the bank that are marked as undelegating, check if they're still - // delegated to us. If not, we need to refetch them from chain instead of using the - // bank version. - let mut accounts_to_refetch = vec![]; - for (pubkey, slot) in &in_bank { - if let Some(bank_account) = self.accounts_bank.get_account(pubkey) { - if bank_account.undelegating() { - // Check if still delegated to us - if !self.is_still_delegated_to_us(*pubkey).await { - debug!( - "Account {pubkey} marked as undelegating is no longer delegated to us, refetching from chain" - ); - accounts_to_refetch.push((*pubkey, *slot)); - } - } - } - } - - // Remove accounts that need to be refetched from in_bank list - let _in_bank: Vec<_> = in_bank - .into_iter() - .filter(|(pubkey, _)| { - !accounts_to_refetch.iter().any(|(p, _)| p == pubkey) - }) - .collect(); - - // Add accounts that need to be refetched to the plain list - // (they will be fetched from chain) - let mut plain = plain; - for (pubkey, _slot) in accounts_to_refetch { - if let Some(account) = self - .remote_account_provider - .try_get(pubkey) - .await? - .fresh_account() - { - plain.push((pubkey, account)); - } - } - // Calculate min context slot: use the greater of subscription slot or last chain slot let min_context_slot = slot.map(|subscription_slot| { subscription_slot.max(self.remote_account_provider.chain_slot()) @@ -1058,6 +1031,49 @@ where }) } + /// Determines if the account finished undelegating on chain. + /// If it has finished undelegating, we should refresh it in the bank. + /// - **pubkey**: the account pubkey + /// - **in_bank**: the account as it exists in the bank + /// + /// Returns true if the account should be refreshed in the bank + async fn should_refresh_undelegating_in_bank_account( + &self, + pubkey: &Pubkey, + in_bank: &AccountSharedData, + ) -> bool { + if in_bank.undelegating() { + debug!("Fetching undelegating account {pubkey}. delegated={}, undelegating={}", in_bank.delegated(), in_bank.undelegating()); + let deleg_record = self + .fetch_and_parse_delegation_record( + *pubkey, + self.remote_account_provider.chain_slot(), + ) + .await; + let delegated_on_chain = deleg_record.as_ref().is_some_and(|dr| { + dr.authority.eq(&self.validator_pubkey) + || dr.authority.eq(&Pubkey::default()) + }); + if !account_still_undelegating_on_chain( + pubkey, + delegated_on_chain, + in_bank.remote_slot(), + deleg_record, + &self.validator_pubkey, + ) { + debug!( + "Account {pubkey} marked as undelegating will be overridden since undelegation completed" + ); + return true; + } + } else if in_bank.owner().eq(&dlp::id()) { + debug!( + "Account {pubkey} owned by deleg program not marked as undelegating" + ); + } + false + } + /// Fetch and clone accounts with request deduplication to avoid parallel fetches of the same account. /// This method implements the new logic where: /// 1. Check synchronously if account is in bank, return immediately if found @@ -1077,7 +1093,7 @@ where // We cannot clone blacklisted accounts, thus either they are already // in the bank (e.g. native programs) or they don't exist and the transaction // will fail later - let pubkeys = pubkeys + let mut pubkeys = pubkeys .iter() .filter(|p| !self.blacklisted_accounts.contains(p)) .collect::>(); @@ -1092,6 +1108,29 @@ where let mut await_pending = vec![]; let mut fetch_new = vec![]; + let mut in_bank = vec![]; + for pubkey in pubkeys.iter() { + if let Some(account_in_bank) = + self.accounts_bank.get_account(pubkey) + { + let should_refresh_undelegating = self + .should_refresh_undelegating_in_bank_account( + pubkey, + &account_in_bank, + ) + .await; + if should_refresh_undelegating { + debug!("Account {pubkey} completed undelegation which we missed and is fetched again"); + metrics::inc_unstuck_undelegation_count(); + } + if !should_refresh_undelegating { + // Account is in bank and subscribed correctly - no fetch needed + trace!("Account {pubkey} found in bank in valid state, no fetch needed"); + in_bank.push(*pubkey); + } + } + } + pubkeys.retain(|p| !in_bank.contains(p)); // Check pending requests and bank synchronously { @@ -1101,26 +1140,6 @@ where .expect("pending_requests lock poisoned"); for pubkey in pubkeys { - // Check synchronously if account is in bank and subscribed when it should be - if let Some(account_in_bank) = - self.accounts_bank.get_account(pubkey) - { - // NOTE: we defensively correct accounts that we should have been watching but - // were not for some reason. We fetch them again in that case. - // This actually would point to a bug in the subscription logic. - // TODO(thlorenz): remove this once we are certain (by perusing logs) that this - // does not happen anymore - if account_in_bank.owner().eq(&dlp::id()) - || account_in_bank.delegated() - || self.blacklisted_accounts.contains(pubkey) - || self.is_watching(pubkey) - { - continue; - } else if !self.is_watching(pubkey) { - debug!("Account {pubkey} should be watched but wasn't"); - } - } - // Check if account fetch is already pending if let Some(requests) = pending.get_mut(pubkey) { let (sender, receiver) = oneshot::channel(); diff --git a/magicblock-chainlink/src/chainlink/mod.rs b/magicblock-chainlink/src/chainlink/mod.rs index 1ef11a28b..c5805c688 100644 --- a/magicblock-chainlink/src/chainlink/mod.rs +++ b/magicblock-chainlink/src/chainlink/mod.rs @@ -26,6 +26,7 @@ use crate::{ submux::SubMuxClient, }; +mod account_still_undelegating_on_chain; mod blacklisted_accounts; pub mod config; pub mod errors; diff --git a/magicblock-chainlink/src/remote_account_provider/remote_account.rs b/magicblock-chainlink/src/remote_account_provider/remote_account.rs index ada3bc48c..bc401a35b 100644 --- a/magicblock-chainlink/src/remote_account_provider/remote_account.rs +++ b/magicblock-chainlink/src/remote_account_provider/remote_account.rs @@ -109,14 +109,6 @@ impl ResolvedAccountSharedData { self } - pub fn undelegating(&self) -> bool { - use ResolvedAccountSharedData::*; - match self { - Fresh(account) => account.undelegating(), - Bank(account) => account.undelegating(), - } - } - pub fn set_remote_slot(&mut self, remote_slot: Slot) -> &mut Self { use ResolvedAccountSharedData::*; match self { diff --git a/magicblock-chainlink/src/testing/mod.rs b/magicblock-chainlink/src/testing/mod.rs index 423576a64..c924633c4 100644 --- a/magicblock-chainlink/src/testing/mod.rs +++ b/magicblock-chainlink/src/testing/mod.rs @@ -339,6 +339,36 @@ macro_rules! assert_cloned_as_empty_placeholder { ($cloner:expr, $pubkeys:expr, $slot:expr) => {{}}; } +#[macro_export] +macro_rules! assert_not_undelegating { + ($cloner:expr, $pubkeys:expr, $slot:expr) => {{ + use solana_account::ReadableAccount; + for pubkey in $pubkeys { + let account = $cloner + .get_account(pubkey) + .expect(&format!("Expected account {} to be cloned", pubkey)); + assert!( + !account.undelegating(), + "Expected account {} to not be undelegating", + pubkey + ); + assert_eq!( + account.remote_slot(), + $slot, + "Expected account {} to have remote slot {}", + pubkey, + $slot + ); + assert_ne!( + account.owner(), + &dlp::id(), + "Expected account {} to not be owned by the delegation program", + pubkey, + ); + } + }}; +} + #[macro_export] macro_rules! assert_remain_undelegating { ($cloner:expr, $pubkeys:expr, $slot:expr) => {{ @@ -347,6 +377,11 @@ macro_rules! assert_remain_undelegating { let account = $cloner .get_account(pubkey) .expect(&format!("Expected account {} to be cloned", pubkey)); + assert!( + account.undelegating(), + "Expected account {} to remain undelegating", + pubkey + ); assert_eq!( account.remote_slot(), $slot, diff --git a/magicblock-chainlink/tests/01_ensure-accounts.rs b/magicblock-chainlink/tests/01_ensure-accounts.rs index f0fd34017..62723bac4 100644 --- a/magicblock-chainlink/tests/01_ensure-accounts.rs +++ b/magicblock-chainlink/tests/01_ensure-accounts.rs @@ -4,7 +4,8 @@ use log::*; use magicblock_chainlink::{ assert_cloned_as_delegated, assert_cloned_as_undelegated, assert_not_cloned, assert_not_found, assert_not_subscribed, - assert_remain_undelegating, assert_subscribed_without_delegation_record, + assert_not_undelegating, assert_remain_undelegating, + assert_subscribed_without_delegation_record, testing::deleg::add_delegation_record_for, }; use solana_account::{Account, AccountSharedData}; @@ -174,7 +175,7 @@ async fn test_write_existing_account_other_authority() { // Account is in the process of being undelegated and its owner is the delegation program // ----------------- #[tokio::test] -async fn test_write_account_being_undelegated() { +async fn test_write_undelegating_account_undelegated_to_other_validator() { let TestContext { chainlink, rpc_client, @@ -183,7 +184,48 @@ async fn test_write_account_being_undelegated() { .. } = setup(CURRENT_SLOT).await; - let authority = Pubkey::new_unique(); + let other_authority = Pubkey::new_unique(); + let pubkey = Pubkey::new_unique(); + + // The account was re-delegated to other validator on chain + let account = Account { + owner: dlp::id(), + ..Default::default() + }; + let owner = Pubkey::new_unique(); + rpc_client.add_account(pubkey, account); + + add_delegation_record_for(&rpc_client, pubkey, other_authority, owner); + + // The same account is already marked as undelegated in the bank + // (set the owner to the delegation program and mark it as _undelegating_) + let mut shared_data = AccountSharedData::from(Account { + owner: dlp::id(), + data: vec![0; 100], + ..Default::default() + }); + shared_data.set_undelegating(true); + shared_data.set_remote_slot(CURRENT_SLOT); + bank.insert(pubkey, shared_data); + + let pubkeys = [pubkey]; + let res = chainlink.ensure_accounts(&pubkeys, None).await.unwrap(); + debug!("res: {res:?}"); + assert_not_undelegating!(cloner, &pubkeys, CURRENT_SLOT); +} + +#[tokio::test] +async fn test_write_undelegating_account_still_being_undelegated() { + let TestContext { + chainlink, + rpc_client, + bank, + cloner, + validator_pubkey, + .. + } = setup(CURRENT_SLOT).await; + + let authority = validator_pubkey; let pubkey = Pubkey::new_unique(); // The account is still delegated to us on chain @@ -204,6 +246,7 @@ async fn test_write_account_being_undelegated() { ..Default::default() }); shared_data.set_remote_slot(CURRENT_SLOT); + shared_data.set_undelegating(true); bank.insert(pubkey, shared_data); let pubkeys = [pubkey]; diff --git a/programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs b/programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs index e26d45705..2339af969 100644 --- a/programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs +++ b/programs/magicblock/src/mutate_accounts/process_mutate_accounts.rs @@ -126,6 +126,11 @@ pub(crate) fn process_mutate_accounts( account_key, ); + // While an account is undelegating and the delegation is not completed, + // we will never clone/mutate it. Thus we can safely untoggle this flag + // here. + account.borrow_mut().set_undelegating(false); + if let Some(lamports) = modification.lamports { ic_msg!( invoke_context, diff --git a/programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs b/programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs index 5c9ca050c..787f799de 100644 --- a/programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs +++ b/programs/magicblock/src/schedule_transactions/process_schedule_base_intent.rs @@ -14,7 +14,7 @@ use crate::{ }, schedule_transactions::check_magic_context_id, utils::{ - account_actions::set_account_owner_to_delegation_program, + account_actions::mark_account_as_undelegating, accounts::{ get_instruction_account_with_idx, get_instruction_pubkey_with_idx, }, @@ -129,7 +129,6 @@ pub(crate) fn process_schedule_base_intent( } else { None }; - let scheduled_intent = ScheduledBaseIntent::try_new( args, intent_id, @@ -138,15 +137,21 @@ pub(crate) fn process_schedule_base_intent( &construction_context, )?; - if let Some(undelegated_accounts_ref) = undelegated_accounts_ref { - // Change owner to dlp + let mut undelegated_pubkeys = vec![]; + if let Some(undelegated_accounts_ref) = undelegated_accounts_ref.as_ref() { + // Change owner to dlp and set undelegating flag // Once account is undelegated we need to make it immutable in our validator. - undelegated_accounts_ref - .into_iter() - .for_each(|(_, account_ref)| { - set_account_owner_to_delegation_program(account_ref); - account_ref.borrow_mut().set_undelegating(true); - }); + for (pubkey, account_ref) in undelegated_accounts_ref.iter() { + undelegated_pubkeys.push(pubkey.to_string()); + mark_account_as_undelegating(account_ref); + } + } + if !undelegated_pubkeys.is_empty() { + ic_msg!( + invoke_context, + "Scheduling undelegation for accounts: {}", + undelegated_pubkeys.join(", ") + ); } let action_sent_signature = diff --git a/programs/magicblock/src/schedule_transactions/process_schedule_commit.rs b/programs/magicblock/src/schedule_transactions/process_schedule_commit.rs index bf0662f8a..a97f90a05 100644 --- a/programs/magicblock/src/schedule_transactions/process_schedule_commit.rs +++ b/programs/magicblock/src/schedule_transactions/process_schedule_commit.rs @@ -16,7 +16,7 @@ use crate::{ }, schedule_transactions, utils::{ - account_actions::set_account_owner_to_delegation_program, + account_actions::mark_account_as_undelegating, accounts::{ get_instruction_account_with_idx, get_instruction_pubkey_with_idx, get_writable_with_idx, @@ -198,10 +198,13 @@ pub(crate) fn process_schedule_commit( // that point // NOTE: this owner change only takes effect if the transaction which // includes this instruction succeeds. - set_account_owner_to_delegation_program(acc); + // + // We also set the undelegating flag on the account in order to detect + // undelegations for which we miss updates + mark_account_as_undelegating(acc); ic_msg!( invoke_context, - "ScheduleCommit: account {} owner set to delegation program", + "ScheduleCommit: Marking account {} as undelegating", acc_pubkey ); } diff --git a/programs/magicblock/src/utils/account_actions.rs b/programs/magicblock/src/utils/account_actions.rs index 22a98ce89..fff842bc1 100644 --- a/programs/magicblock/src/utils/account_actions.rs +++ b/programs/magicblock/src/utils/account_actions.rs @@ -15,8 +15,7 @@ pub(crate) fn set_account_owner( } /// Sets proper values on account during undelegation -pub(crate) fn set_account_owner_to_delegation_program( - acc: &RefCell, -) { +pub(crate) fn mark_account_as_undelegating(acc: &RefCell) { set_account_owner(acc, DELEGATION_PROGRAM_ID); + acc.borrow_mut().set_undelegating(true); } diff --git a/test-integration/Cargo.lock b/test-integration/Cargo.lock index d894508b1..7c76ccce9 100644 --- a/test-integration/Cargo.lock +++ b/test-integration/Cargo.lock @@ -3603,7 +3603,7 @@ dependencies = [ "solana-rpc", "solana-rpc-client", "solana-sdk", - "solana-svm 2.2.1 (git+https://github.com/magicblock-labs/magicblock-svm.git?rev=11bbaf2)", + "solana-svm 2.2.1 (git+https://github.com/magicblock-labs/magicblock-svm.git?rev=e480fa2)", "solana-transaction", "tempfile", "thiserror 1.0.69", @@ -3791,7 +3791,7 @@ dependencies = [ "solana-metrics", "solana-sdk", "solana-storage-proto 0.2.3", - "solana-svm 2.2.1 (git+https://github.com/magicblock-labs/magicblock-svm.git?rev=11bbaf2)", + "solana-svm 2.2.1 (git+https://github.com/magicblock-labs/magicblock-svm.git?rev=e480fa2)", "solana-timings", "solana-transaction-status", "thiserror 1.0.69", @@ -3858,7 +3858,7 @@ dependencies = [ "solana-pubkey", "solana-rent-collector", "solana-sdk-ids", - "solana-svm 2.2.1 (git+https://github.com/magicblock-labs/magicblock-svm.git?rev=11bbaf2)", + "solana-svm 2.2.1 (git+https://github.com/magicblock-labs/magicblock-svm.git?rev=e480fa2)", "solana-svm-transaction", "solana-system-program", "solana-transaction", @@ -3935,7 +3935,7 @@ dependencies = [ "solana-program", "solana-pubsub-client", "solana-sdk", - "solana-svm 2.2.1 (git+https://github.com/magicblock-labs/magicblock-svm.git?rev=11bbaf2)", + "solana-svm 2.2.1 (git+https://github.com/magicblock-labs/magicblock-svm.git?rev=e480fa2)", "solana-timings", "thiserror 1.0.69", "tokio", @@ -9121,7 +9121,7 @@ dependencies = [ [[package]] name = "solana-svm" version = "2.2.1" -source = "git+https://github.com/magicblock-labs/magicblock-svm.git?rev=11bbaf2#11bbaf2249aeb16cec4111e86f2e18a0c45ff1f2" +source = "git+https://github.com/magicblock-labs/magicblock-svm.git?rev=e480fa2#e480fa202f0680476b51b2d41210667ffc241bf4" dependencies = [ "ahash 0.8.12", "log",