Revert "feat(executor): validate no confined account is modified" (#745)

Author: GabrielePicco

Cargo.lock

@@ -131,7 +131,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 = "57158728" }
+solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "1beed4c" }
 solana-account-decoder = { version = "2.2" }
 solana-account-decoder-client-types = { version = "2.2" }
 solana-account-info = { version = "2.2" }
@@ -216,10 +216,11 @@ features = ["lz4"]
 # 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 = "57158728" }
+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 = "3e9456ec4" }
 # Fork is used to enable `disable_manual_compaction` usage
 # Fork is based on commit d4e9e16 of rocksdb (parent commit of 0.23.0 release)
 # without patching update isn't possible due to conflict with solana deps
-rocksdb = { git = "https://github.com/magicblock-labs/rust-rocksdb.git", rev = "6d975197" }
+# TODO(edwin): remove once solana deps upgraded and are using rust-rocksdb 0.25.0(likely)
+rocksdb = { git = "https://github.com/magicblock-labs/rust-rocksdb.git", rev = "6d975197" }

Cargo.toml

@@ -101,10 +101,10 @@ impl AccountsDb {
                 // atomic counter. New readers will see the latest update.
                 acc.commit();
                 // check whether the account's owner has changed
-                if !acc.owner_changed() {
+                if !acc.owner_changed {
                     return;
                 }
-                // and perform some index bookkeeping to ensure a correct owner
+                // and perform some index bookkeeping to ensure correct owner
                 let _ = self
                     .index
                     .ensure_correct_owner(pubkey, account.owner())

magicblock-accounts-db/src/lib.rs

@@ -341,18 +341,16 @@ impl super::TransactionExecutor {
 
     /// Ensure that no post execution account state violations occurred:
     /// 1. No modification of the non-delegated feepayer in gasless mode
-    /// 2. No lamport modification of confined accounts
+    /// 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 logs = executed
-            .execution_details
-            .log_messages
-            .get_or_insert_default();
         let gasless = self.environment.fee_lamports_per_signature == 0;
         if gasless {
             // If we are running in the gasless mode, we should not allow
@@ -361,50 +359,40 @@ impl super::TransactionExecutor {
             // from undelegated feepayers to delegated accounts, which would
             // result in validator loosing funds upon balance settling.
             let undelegated_feepayer_was_modified = feepayer
-                .map(|(_, acc)| {
-                    let mutated = acc
-                        .as_borrowed()
-                        .map(|a| a.lamports_changed())
-                        // NOTE: this branch can be taken only, if the account
-                        // has been upgraded to the Owned variant, which indicates
-                        // that it has been resized, which in turn is a clear
-                        // violation of the feepayer immutability rule in this mode
-                        .unwrap_or(true);
-                    !self.is_auto_airdrop_lamports_enabled
-                        && mutated
-                        && !acc.delegated()
-                        && !acc.privileged()
+                .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 msg = "Feepayer balance has been illegally modified".into();
+                let logs = executed
+                    .execution_details
+                    .log_messages
+                    .get_or_insert_default();
+                let msg = "Feepayer balance has been modified illegally".into();
                 logs.push(msg);
-                return;
             }
         }
-        for (pubkey, acc) in &txn.accounts {
-            if !acc.confined() {
-                continue;
-            }
-            // If the confined account was modified in any way that affected its lamport
-            // balance, then an corresponding marker must have been set, in which case we
-            // fail the transaction, since this is regarded as a validator draining attack
-            let balance_changed = acc
-                .as_borrowed()
-                .map(|a| a.lamports_changed())
-                .unwrap_or(true);
+    }
+}
 
-            if balance_changed {
-                executed.execution_details.status =
-                    Err(TransactionError::UnbalancedTransaction);
-                let msg = format!(
-                    "Confined account {pubkey} has been illegally modified"
-                );
-                logs.push(msg);
-                break;
-            }
+// A utility to extract the rollback lamports of the feepayer
+fn rollback_feepayer_lamports(rollback: &RollbackAccounts) -> u64 {
+    match rollback {
+        RollbackAccounts::FeePayerOnly { fee_payer_account } => {
+            fee_payer_account.lamports()
+        }
+        RollbackAccounts::SameNonceAndFeePayer { nonce } => {
+            nonce.account().lamports()
         }
+        RollbackAccounts::SeparateNonceAndFeePayer {
+            fee_payer_account,
+            ..
+        } => fee_payer_account.lamports(),
     }
 }

magicblock-processor/src/executor/processing.rs

@@ -1,6 +1,7 @@
 use std::{collections::HashSet, time::Duration};
 
 use guinea::GuineaInstruction;
+use magicblock_core::traits::AccountsBank;
 use solana_account::{ReadableAccount, WritableAccount};
 use solana_keypair::Keypair;
 use solana_program::{
@@ -50,7 +51,7 @@ async fn test_insufficient_fee() {
     let env = ExecutionTestEnv::new();
     let mut payer = env.get_payer();
     payer.set_lamports(ExecutionTestEnv::BASE_FEE - 1);
-    payer.commit();
+    payer.commmit();
 
     let (ix, _) =
         setup_guinea_instruction(&env, &GuineaInstruction::PrintSizes, false);
@@ -104,7 +105,7 @@ async fn test_non_delegated_payer_rejection() {
     let mut payer = env.get_payer();
     payer.set_delegated(false); // Mark the payer as not delegated
     let fee_payer_initial_balance = payer.lamports();
-    payer.commit();
+    payer.commmit();
 
     let (ix, _) =
         setup_guinea_instruction(&env, &GuineaInstruction::PrintSizes, false);
@@ -132,7 +133,7 @@ async fn test_escrowed_payer_success() {
     payer.set_lamports(ExecutionTestEnv::BASE_FEE - 1);
     payer.set_delegated(false);
     let escrow = ephemeral_balance_pda_from_payer(&payer.pubkey);
-    payer.commit();
+    payer.commmit();
 
     env.fund_account(escrow, LAMPORTS_PER_SOL); // Fund the escrow PDA
 
@@ -219,7 +220,7 @@ async fn test_escrow_charged_for_failed_transaction() {
     payer.set_lamports(0);
     payer.set_delegated(false);
     let escrow = ephemeral_balance_pda_from_payer(&payer.pubkey);
-    payer.commit();
+    payer.commmit();
     let account = env
         .create_account_with_config(LAMPORTS_PER_SOL, 0, guinea::ID) // Account with no data
         .pubkey();
@@ -265,7 +266,7 @@ async fn test_transaction_gasless_mode() {
     payer.set_lamports(1); // Not enough to cover standard fee
     payer.set_delegated(false); // Explicitly set the payer as NON-delegated.
     let initial_balance = payer.lamports();
-    payer.commit();
+    payer.commmit();
 
     let ix = Instruction::new_with_bincode(
         guinea::ID,
@@ -311,7 +312,7 @@ async fn test_transaction_gasless_mode_with_not_existing_account() {
     payer.set_lamports(1); // Not enough to cover standard fee
     payer.set_delegated(false); // Explicitly set the payer as NON-delegated.
     let initial_balance = payer.lamports();
-    payer.commit();
+    payer.commmit();
 
     let ix = Instruction::new_with_bincode(
         guinea::ID,
@@ -350,3 +351,51 @@ async fn test_transaction_gasless_mode_with_not_existing_account() {
         "payer balance should not change in gasless mode"
     );
 }
+
+/// Verifies that in zero-fee ("gasless") mode, transactions are processed
+/// successfully even when the fee payer does not exists.
+#[tokio::test]
+async fn test_transaction_gasless_mode_not_existing_feepayer() {
+    // Initialize the environment with a base fee of 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(
+        guinea::ID,
+        &GuineaInstruction::PrintSizes,
+        vec![],
+    );
+    let txn = env.build_transaction(&[ix]);
+    let signature = txn.signatures[0];
+
+    // In a normal fee-paying mode, this execution would fail.
+    env.execute_transaction(txn)
+        .await
+        .expect("transaction should succeed in gasless mode");
+
+    // Verify the transaction was fully processed and broadcast successfully.
+    let status = env
+        .dispatch
+        .transaction_status
+        .recv_timeout(Duration::from_millis(100))
+        .expect("should receive a transaction status update");
+
+    assert_eq!(status.signature, signature);
+    assert!(
+        status.result.result.is_ok(),
+        "Transaction execution should be successful"
+    );
+
+    // Verify that the payer balance is zero (or doesn't exist)
+    let final_balance = env
+        .accountsdb
+        .get_account(&payer)
+        .unwrap_or_default()
+        .lamports();
+    assert_eq!(
+        final_balance, 0,
+        "payer balance of a not existing feepayer should be 0 in gasless mode"
+    );
+}

magicblock-processor/tests/fees.rs

@@ -74,7 +74,7 @@ rayon = "1.10.0"
 schedulecommit-client = { path = "schedulecommit/client" }
 serde = "1.0.217"
 serial_test = "3.2.0"
-solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "57158728" }
+solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "1beed4c" }
 solana-loader-v2-interface = "2.2"
 solana-loader-v3-interface = "4.0"
 solana-loader-v4-interface = "2.1"
@@ -106,5 +106,5 @@ url = "2.5.0"
 # and we use protobuf-src v2.1.1. Otherwise compilation fails
 solana-storage-proto = { path = "../storage-proto" }
 # same reason as above
-rocksdb = { git = "https://github.com/magicblock-labs/rust-rocksdb.git", rev = "6d975197" }
-solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "57158728" }
+solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "1beed4c" }
+rocksdb = { git = "https://github.com/magicblock-labs/rust-rocksdb.git", rev = "6d975197" }