diff --git a/magicblock-aperture/src/tests.rs b/magicblock-aperture/src/tests.rs index 643fbb737..d41dd1ed7 100644 --- a/magicblock-aperture/src/tests.rs +++ b/magicblock-aperture/src/tests.rs @@ -27,6 +27,8 @@ use crate::{ EventProcessor, }; +const LAMPORTS_PER_SOL: u64 = 1_000_000_000; + /// A test helper to create a unique WebSocket connection channel pair. fn ws_channel() -> (WsConnectionChannel, Receiver) { static CHAN_ID: AtomicU32 = AtomicU32::new(0); @@ -100,7 +102,9 @@ mod event_processor { #[tokio::test] async fn test_account_update() { let (state, env) = setup(); - let acc = env.create_account_with_config(1, 1, guinea::ID).pubkey(); + let acc = env + .create_account_with_config(LAMPORTS_PER_SOL, 1, guinea::ID) + .pubkey(); let (tx, mut rx) = ws_channel(); // Subscribe to both the specific account and the program that owns it. @@ -140,7 +144,9 @@ mod event_processor { #[tokio::test] async fn test_transaction_update() { let (state, env) = setup(); - let acc = env.create_account_with_config(1, 42, guinea::ID).pubkey(); + let acc = env + .create_account_with_config(LAMPORTS_PER_SOL, 42, guinea::ID) + .pubkey(); let (tx, mut rx) = ws_channel(); let ix = Instruction::new_with_bincode( @@ -201,7 +207,9 @@ mod event_processor { let (state, env) = setup(); // Test multiple subscriptions to the same ACCOUNT. - let acc1 = env.create_account_with_config(1, 1, guinea::ID).pubkey(); + let acc1 = env + .create_account_with_config(LAMPORTS_PER_SOL, 1, guinea::ID) + .pubkey(); let (acc_tx1, mut acc_rx1) = ws_channel(); let (acc_tx2, mut acc_rx2) = ws_channel(); @@ -227,7 +235,9 @@ mod event_processor { assert_receives_update(&mut acc_rx2, "second account subscriber").await; // Test multiple subscriptions to the same PROGRAM. - let acc2 = env.create_account_with_config(1, 1, guinea::ID).pubkey(); + let acc2 = env + .create_account_with_config(LAMPORTS_PER_SOL, 1, guinea::ID) + .pubkey(); let (prog_tx1, mut prog_rx1) = ws_channel(); let (prog_tx2, mut prog_rx2) = ws_channel(); let prog_encoder = ProgramAccountEncoder { diff --git a/magicblock-api/src/magic_validator.rs b/magicblock-api/src/magic_validator.rs index 4cc41fa63..e1e856c3a 100644 --- a/magicblock-api/src/magic_validator.rs +++ b/magicblock-api/src/magic_validator.rs @@ -157,7 +157,6 @@ impl MagicValidator { let GenesisConfigInfo { genesis_config, validator_pubkey, - .. } = create_genesis_config_with_leader( u64::MAX, &validator_pubkey, @@ -274,7 +273,7 @@ impl MagicValidator { }); validator::init_validator_authority(identity_keypair); - + let base_fee = config.validator.base_fees.unwrap_or_default(); let txn_scheduler_state = TransactionSchedulerState { accountsdb: accountsdb.clone(), ledger: ledger.clone(), @@ -303,7 +302,7 @@ impl MagicValidator { let node_context = NodeContext { identity: validator_pubkey, faucet, - base_fee: config.validator.base_fees.unwrap_or_default(), + base_fee, featureset: txn_scheduler_state.environment.feature_set.clone(), }; let transaction_scheduler = diff --git a/magicblock-config/src/validator.rs b/magicblock-config/src/validator.rs index 890cb712a..7eeec0ac6 100644 --- a/magicblock-config/src/validator.rs +++ b/magicblock-config/src/validator.rs @@ -35,7 +35,6 @@ pub struct ValidatorConfig { #[derive_env_var] #[clap_from_serde_skip] // Skip because it defaults to None #[arg(help = "The base fees to use for the validator.")] - #[serde(default = "default_base_fees")] pub base_fees: Option, /// Uses alpha2 country codes following https://en.wikipedia.org/wiki/ISO_3166-1 @@ -65,7 +64,7 @@ impl Default for ValidatorConfig { millis_per_slot: default_millis_per_slot(), sigverify: default_sigverify(), fqdn: default_fqdn(), - base_fees: default_base_fees(), + base_fees: None, country_code: default_country_code(), claim_fees_interval_secs: default_claim_fees_interval_secs(), } @@ -84,10 +83,6 @@ fn default_fqdn() -> Option { None } -fn default_base_fees() -> Option { - None -} - fn default_country_code() -> CountryCode { CountryCode::for_alpha2("US").unwrap() } diff --git a/magicblock-processor/Cargo.toml b/magicblock-processor/Cargo.toml index 1cde50705..5419c62c4 100644 --- a/magicblock-processor/Cargo.toml +++ b/magicblock-processor/Cargo.toml @@ -20,14 +20,14 @@ magicblock-metrics = { workspace = true } magicblock-program = { workspace = true } solana-account = { workspace = true } +solana-address-lookup-table-program = { workspace = true } solana-bpf-loader-program = { workspace = true } solana-compute-budget-program = { workspace = true } solana-feature-set = { workspace = true } solana-fee = { workspace = true } solana-fee-structure = { workspace = true } -solana-address-lookup-table-program = { workspace = true } -solana-program = { workspace = true } solana-loader-v4-program = { workspace = true } +solana-program = { workspace = true } solana-program-runtime = { workspace = true } solana-pubkey = { workspace = true } solana-rent-collector = { workspace = true } @@ -36,8 +36,8 @@ solana-svm = { workspace = true } solana-svm-transaction = { workspace = true } solana-system-program = { workspace = true } solana-transaction = { workspace = true } -solana-transaction-status = { workspace = true } solana-transaction-error = { workspace = true } +solana-transaction-status = { workspace = true } [dev-dependencies] guinea = { workspace = true } diff --git a/magicblock-processor/src/executor/mod.rs b/magicblock-processor/src/executor/mod.rs index e8e511c17..30cca792c 100644 --- a/magicblock-processor/src/executor/mod.rs +++ b/magicblock-processor/src/executor/mod.rs @@ -57,8 +57,10 @@ pub(super) struct TransactionExecutor { /// A read lock held during a slot's processing to synchronize with critical global /// operations like `AccountsDb` snapshots. sync: StWLock, + /// Hacky temporary solution to allow automatic airdrops, the flag + /// is tightly contolled and will be removed in the nearest future /// True when auto airdrop for fee payers is enabled (auto_airdrop_lamports > 0). - pub is_auto_airdrop_lamports_enabled: bool, + is_auto_airdrop_lamports_enabled: bool, } impl TransactionExecutor { diff --git a/magicblock-processor/src/executor/processing.rs b/magicblock-processor/src/executor/processing.rs index 0be6a6040..98a656452 100644 --- a/magicblock-processor/src/executor/processing.rs +++ b/magicblock-processor/src/executor/processing.rs @@ -61,28 +61,31 @@ impl super::TransactionExecutor { self.commit_failed_transaction(txn, status.clone()); FAILED_TRANSACTIONS_COUNT.inc(); tx.map(|tx| tx.send(status)); + // NOTE: + // Transactions that failed to load, cannot have touched the thread + // local storage, thus there's no need to clear it before returning return; } }; // The transaction has been processed, we can commit the account state changes - // Failed transactions still pay fees, so we need to commit the accounts even if the transaction failed + // NOTE: + // Failed transactions still pay fees, so we need to + // commit the accounts even if the transaction failed let feepayer = *txn.fee_payer(); self.commit_accounts(feepayer, &processed, is_replay); let result = processed.status(); - if result.is_ok() { + if result.is_ok() && !is_replay { // If the transaction succeeded, check for potential tasks // that may have been scheduled during the transaction execution // TODO: send intents here as well once implemented - if !is_replay { - while let Some(task) = ExecutionTlsStash::next_task() { - // This is a best effort send, if the tasks service has terminated - // for some reason, logging is the best we can do at this point - let _ = self.tasks_tx.send(task).inspect_err(|_| - error!("Scheduled tasks service has hung up and is no longer running") - ); - } + while let Some(task) = ExecutionTlsStash::next_task() { + // This is a best effort send, if the tasks service has terminated + // for some reason, logging is the best we can do at this point + let _ = self.tasks_tx.send(task).inspect_err(|_| + error!("Scheduled tasks service has hung up and is no longer running") + ); } } @@ -109,9 +112,6 @@ impl super::TransactionExecutor { transaction: [SanitizedTransaction; 1], tx: TxnSimulationResultTx, ) { - // Defensively clear any stale data from previous calls - ExecutionTlsStash::clear(); - let (result, _) = self.process(&transaction); let result = match result { Ok(processed) => { @@ -171,44 +171,11 @@ impl super::TransactionExecutor { let mut result = output.processing_results.pop().expect( "single transaction result is always present in the output", ); - - let gasless = self.environment.fee_lamports_per_signature == 0; - // If we are running in the gasless mode, we should not allow - // any mutation of the feepayer account, since that would make - // it possible for malicious actors to perform transfer operations - // from undelegated feepayers to delegated accounts, which would - // result in validator losing funds upon balance settling. - if gasless { - let undelegated_feepayer_was_modified = result - .as_ref() - .ok() - .and_then(|r| r.executed_transaction()) - .and_then(|txn| { - let first_acc = txn.loaded_transaction.accounts.first(); - let rollback_lamports = rollback_feepayer_lamports( - &txn.loaded_transaction.rollback_accounts, - ); - first_acc.map(|acc| (acc, rollback_lamports)) - }) - .map(|(acc, rollback_lamports)| { - (acc.1.is_dirty() - && (acc.1.lamports() != 0 || rollback_lamports != 0)) - && !acc.1.delegated() - && !acc.1.privileged() - }) - .unwrap_or(false); - - if undelegated_feepayer_was_modified - && !self.is_auto_airdrop_lamports_enabled - { - if let Ok(ProcessedTransaction::Executed(ref mut executed)) = - &mut result - { - executed.execution_details.status = - Err(TransactionError::InvalidAccountForFee) - } - } + // Verify that account state invariants haven't been violated + if let Ok(ref mut processed) = result { + self.verify_account_states(processed); } + (result, output.balances) } @@ -368,9 +335,82 @@ impl super::TransactionExecutor { let _ = self.accounts_tx.send(account); } } + + /// Ensure that no post execution account state violations occurred: + /// 1. No modification of the non-delegated feepayer in gasless mode + /// 2. No illegal account resizing when the balance is zero + fn verify_account_states(&self, processed: &mut ProcessedTransaction) { + let ProcessedTransaction::Executed(executed) = processed else { + return; + }; + let txn = &executed.loaded_transaction; + let feepayer = txn.accounts.first(); + let rollback_lamports = + rollback_feepayer_lamports(&txn.rollback_accounts); + + let gasless = self.environment.fee_lamports_per_signature == 0; + if gasless { + // If we are running in the gasless mode, we should not allow + // any mutation of the feepayer account, since that would make + // it possible for malicious actors to peform transfer operations + // from undelegated feepayers to delegated accounts, which would + // result in validator loosing funds upon balance settling. + let undelegated_feepayer_was_modified = feepayer + .map(|acc| { + (acc.1.is_dirty() + && !self.is_auto_airdrop_lamports_enabled + && (acc.1.lamports() != 0 || rollback_lamports != 0)) + && !acc.1.delegated() + && !acc.1.privileged() + }) + .unwrap_or_default(); + if undelegated_feepayer_was_modified { + executed.execution_details.status = + Err(TransactionError::InvalidAccountForFee); + let logs = executed + .execution_details + .log_messages + .get_or_insert_default(); + let msg = "Feepayer balance has been modified illegally".into(); + logs.push(msg); + return; + } + } + // SVM ignores rent exemption enforcement for accounts, which have + // 0 lamports, so it's possible to call realloc on account with zero + // balance bypassing the runtime checks. In order to prevent this + // edge case we perform explicit post execution check here. + for (i, (pubkey, acc)) in txn.accounts.iter().enumerate() { + if !acc.is_dirty() { + continue; + } + let Some(rent) = self.environment.rent_collector else { + continue; + }; + if acc.lamports() == 0 && acc.data().is_empty() { + continue; + } + let rent_exemption_balance = + rent.get_rent().minimum_balance(acc.data().len()); + if acc.lamports() >= rent_exemption_balance { + continue; + } + let error = Err(TransactionError::InsufficientFundsForRent { + account_index: i as u8, + }); + executed.execution_details.status = error; + let logs = executed + .execution_details + .log_messages + .get_or_insert_default(); + let msg = format!("Account {pubkey} has violated rent exemption"); + logs.push(msg); + return; + } + } } -// A utils to extract the rollback lamports of the feepayer +// A utility to extract the rollback lamports of the feepayer fn rollback_feepayer_lamports(rollback: &RollbackAccounts) -> u64 { match rollback { RollbackAccounts::FeePayerOnly { fee_payer_account } => { diff --git a/magicblock-processor/src/lib.rs b/magicblock-processor/src/lib.rs index d01b9a9d9..a1696e933 100644 --- a/magicblock-processor/src/lib.rs +++ b/magicblock-processor/src/lib.rs @@ -46,9 +46,10 @@ pub fn build_svm_env( } // We have a static rent which is setup once at startup, - // and never changes afterwards. For now we use the same - // values as the vanila solana validator (default()) - let rent_collector = Box::leak(Box::new(RentCollector::default())); + // and never changes afterwards, so we just extend the + // lifetime to 'static by leaking the allocation. + let rent_collector = RentCollector::default(); + let rent_collector = Box::leak(Box::new(rent_collector)); TransactionProcessingEnvironment { blockhash, diff --git a/magicblock-processor/tests/fees.rs b/magicblock-processor/tests/fees.rs index 3c1898313..4632053b1 100644 --- a/magicblock-processor/tests/fees.rs +++ b/magicblock-processor/tests/fees.rs @@ -7,6 +7,7 @@ use solana_keypair::Keypair; use solana_program::{ instruction::{AccountMeta, Instruction}, native_token::LAMPORTS_PER_SOL, + rent::Rent, }; use solana_pubkey::Pubkey; use solana_transaction_error::TransactionError; @@ -31,8 +32,9 @@ fn setup_guinea_instruction( ix_data: &GuineaInstruction, is_writable: bool, ) -> (Instruction, Pubkey) { + let balance = Rent::default().minimum_balance(128); let account = env - .create_account_with_config(LAMPORTS_PER_SOL, 128, guinea::ID) + .create_account_with_config(balance, 128, guinea::ID) .pubkey(); let meta = if is_writable { AccountMeta::new(account, false) @@ -137,13 +139,9 @@ async fn test_escrowed_payer_success() { let fee_payer_initial_balance = env.get_payer().lamports(); let escrow_initial_balance = env.get_account(escrow).lamports(); - const ACCOUNT_SIZE: usize = 1024; - let (ix, account_to_resize) = setup_guinea_instruction( - &env, - &GuineaInstruction::Resize(ACCOUNT_SIZE), - true, - ); + let (ix, _) = + setup_guinea_instruction(&env, &GuineaInstruction::PrintSizes, false); let txn = env.build_transaction(&[ix]); env.execute_transaction(txn) @@ -152,13 +150,11 @@ async fn test_escrowed_payer_success() { let fee_payer_final_balance = env.get_payer().lamports(); let escrow_final_balance = env.get_account(escrow).lamports(); - let final_account_size = env.get_account(account_to_resize).data().len(); let mut updated_accounts = HashSet::new(); while let Ok(acc) = env.dispatch.account_update.try_recv() { updated_accounts.insert(acc.account.pubkey); } - println!("escrow: {escrow}\naccounts: {updated_accounts:?}"); assert_eq!( fee_payer_final_balance, fee_payer_initial_balance, "primary payer should not be charged" @@ -176,10 +172,6 @@ async fn test_escrowed_payer_success() { !updated_accounts.contains(&env.payer.pubkey()), "orginal payer account update should not have been sent" ); - assert_eq!( - final_account_size, ACCOUNT_SIZE, - "instruction side effects should be committed on success" - ); } /// Verifies the fee payer is charged even when the transaction fails during execution. @@ -269,7 +261,7 @@ async fn test_escrow_charged_for_failed_transaction() { #[tokio::test] async fn test_transaction_gasless_mode() { // Initialize the environment with a base fee of 0. - let env = ExecutionTestEnv::new_with_fee(0); + let env = ExecutionTestEnv::new_with_config(0); let mut payer = env.get_payer(); payer.set_lamports(1); // Not enough to cover standard fee payer.set_delegated(false); // Explicitly set the payer as NON-delegated. @@ -315,7 +307,7 @@ async fn test_transaction_gasless_mode() { #[tokio::test] async fn test_transaction_gasless_mode_with_not_existing_account() { // Initialize the environment with a base fee of 0. - let env = ExecutionTestEnv::new_with_fee(0); + let env = ExecutionTestEnv::new_with_config(0); let mut payer = env.get_payer(); payer.set_lamports(1); // Not enough to cover standard fee payer.set_delegated(false); // Explicitly set the payer as NON-delegated. @@ -365,8 +357,9 @@ async fn test_transaction_gasless_mode_with_not_existing_account() { #[tokio::test] async fn test_transaction_gasless_mode_not_existing_feepayer() { // Initialize the environment with a base fee of 0. - let payer = Keypair::new(); - let env = ExecutionTestEnv::new_with_payer_and_fees(&payer, 0); + let env = ExecutionTestEnv::new_with_config(0); + let payer = env.get_payer().pubkey; + env.accountsdb.remove_account(&payer); // Simple noop instruction that does not touch the fee payer account let ix = Instruction::new_with_bincode( @@ -398,7 +391,7 @@ async fn test_transaction_gasless_mode_not_existing_feepayer() { // Verify that the payer balance is zero (or doesn't exist) let final_balance = env .accountsdb - .get_account(&payer.pubkey()) + .get_account(&payer) .unwrap_or_default() .lamports(); assert_eq!( diff --git a/programs/guinea/src/lib.rs b/programs/guinea/src/lib.rs index c64284876..e141af857 100644 --- a/programs/guinea/src/lib.rs +++ b/programs/guinea/src/lib.rs @@ -14,6 +14,8 @@ use solana_program::{ program::{invoke, set_return_data}, program_error::ProgramError, pubkey::Pubkey, + rent::Rent, + sysvar::Sysvar, }; entrypoint::entrypoint!(process_instruction); @@ -40,7 +42,15 @@ fn resize_account( mut accounts: slice::Iter, size: usize, ) -> ProgramResult { + let feepayer = next_account_info(&mut accounts)?; let account = next_account_info(&mut accounts)?; + let rent = ::get()?; + let new_account_balance = rent.minimum_balance(size) as i64; + let delta = new_account_balance - account.try_lamports()? as i64; + **account.try_borrow_mut_lamports()? = new_account_balance as u64; + let feepayer_balance = feepayer.try_lamports()? as i64; + **feepayer.try_borrow_mut_lamports()? = (feepayer_balance - delta) as u64; + account.realloc(size, false)?; Ok(()) } diff --git a/test-integration/schedulecommit/test-scenarios/tests/01_commits.rs b/test-integration/schedulecommit/test-scenarios/tests/01_commits.rs index e2869303a..8a4015a95 100644 --- a/test-integration/schedulecommit/test-scenarios/tests/01_commits.rs +++ b/test-integration/schedulecommit/test-scenarios/tests/01_commits.rs @@ -8,10 +8,10 @@ use program_schedulecommit::{ ScheduleCommitCpiArgs, ScheduleCommitInstruction, }; use schedulecommit_client::{verify, ScheduleCommitTestContextFields}; -use solana_program::instruction::InstructionError; use solana_rpc_client::rpc_client::SerializableTransaction; use solana_rpc_client_api::config::RpcSendTransactionConfig; use solana_sdk::{ + instruction::InstructionError, native_token::LAMPORTS_PER_SOL, pubkey::Pubkey, signature::{Keypair, Signature}, @@ -168,7 +168,7 @@ fn test_committing_account_delegated_to_another_validator() { // Schedule commit of account delegated to another validator let res = schedule_commit_tx(&ctx, &payer, &player, player_pda, false); - // We expect IllegalOwner error since account isn't delegated to our validator + // We expect InvalidAccountForFee error since account isn't delegated to our validator let (_, tx_err) = extract_transaction_error(res); assert_eq!( tx_err.unwrap(), @@ -205,7 +205,7 @@ fn test_undelegating_account_delegated_to_another_validator() { // Schedule undelegation of account delegated to another validator let res = schedule_commit_tx(&ctx, &payer, &player, player_pda, true); - // We expect IllegalOwner error since account isn't delegated to our validator + // We expect InvalidWritableAccount error since account isn't delegated to our validator let (_, tx_err) = extract_transaction_error(res); assert_eq!(tx_err.unwrap(), TransactionError::InvalidWritableAccount); }); diff --git a/test-kit/src/lib.rs b/test-kit/src/lib.rs index e7057feee..c5572c140 100644 --- a/test-kit/src/lib.rs +++ b/test-kit/src/lib.rs @@ -81,13 +81,7 @@ impl ExecutionTestEnv { /// 4. Pre-loads a test program (`guinea`) for use in tests. /// 5. Funds a default `payer` keypair with 1 SOL. pub fn new() -> Self { - Self::new_with_fee(Self::BASE_FEE) - } - - pub fn new_with_payer_and_fees(payer: &Keypair, fee: u64) -> Self { - let mut ctx = Self::new_with_fee(fee); - ctx.payer = payer.insecure_clone(); - ctx + Self::new_with_config(Self::BASE_FEE) } /// Creates a new, fully initialized validator test environment with given base fee @@ -98,7 +92,7 @@ impl ExecutionTestEnv { /// 3. Spawns a `TransactionScheduler` with one worker thread. /// 4. Pre-loads a test program (`guinea`) for use in tests. /// 5. Funds a default `payer` keypair with 1 SOL. - pub fn new_with_fee(fee: u64) -> Self { + pub fn new_with_config(fee: u64) -> Self { init_logger!(); let dir = tempfile::tempdir().expect("creating temp dir for validator state");