Merge pull request #6 from magicblock-labs/bmuddha/feat/improved-txn-diagnostics

feat: on failed txn checks, return more diagnostics info

Author: bmuddha

Cargo.lock

@@ -127,4 +127,4 @@ shuttle-test = [
 ]
 
 [patch.crates-io]
-solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "a892d2" }
+solana-account = { git = "https://github.com/magicblock-labs/solana-account.git", rev = "731fa50" }

Cargo.toml

@@ -1,5 +1,6 @@
+use solana_pubkey::Pubkey;
 use solana_svm_transaction::svm_message::SVMMessage;
-use solana_transaction_error::{TransactionError, TransactionResult};
+use solana_transaction_error::TransactionError;
 
 use crate::account_loader::LoadedTransaction;
 
@@ -24,7 +25,7 @@ impl LoadedTransaction {
     pub(crate) fn validate_accounts_access(
         &self,
         message: &impl SVMMessage,
-    ) -> TransactionResult<()> {
+    ) -> Result<(), (TransactionError, Pubkey)> {
         let payer = self.accounts.first().map(|(_, acc)| acc);
         if payer.map(|p| p.privileged()).unwrap_or_default() {
             // Payer has privileged access, so we can skip the validation.
@@ -33,10 +34,10 @@ impl LoadedTransaction {
 
         // For non-privileged payers, validate the rest of the accounts.
         // Skip the fee payer (index 0), as its writability is validated elsewhere.
-        for (i, (_, acc)) in self.accounts.iter().enumerate().skip(1) {
+        for (i, (pk, acc)) in self.accounts.iter().enumerate().skip(1) {
             // Enforce that any account intended to be writable must be a delegated account.
             if message.is_writable(i) && !acc.delegated() {
-                return Err(TransactionError::InvalidWritableAccount);
+                return Err((TransactionError::InvalidWritableAccount, *pk));
             }
         }
         Ok(())

src/access_permissions.rs

@@ -2141,55 +2141,6 @@ mod tests {
         assert_eq!(account.rent_epoch(), RENT_EXEMPT_RENT_EPOCH);
     }
 
-    #[test]
-    fn test_collect_rent_from_account_rent_paying() {
-        let feature_set = FeatureSet::all_enabled();
-        let rent_collector = RentCollector {
-            epoch: 1,
-            ..RentCollector::default()
-        };
-
-        let address = Pubkey::new_unique();
-        let mut account = AccountSharedData::from(Account {
-            lamports: 1,
-            ..Account::default()
-        });
-
-        assert_eq!(
-            collect_rent_from_account(&feature_set, &rent_collector, &address, &mut account),
-            CollectedInfo::default()
-        );
-        assert_eq!(account.rent_epoch(), 0);
-        assert_eq!(account.lamports(), 1);
-    }
-
-    #[test]
-    fn test_collect_rent_from_account_rent_enabled() {
-        let feature_set =
-            all_features_except(Some(&[feature_set::disable_rent_fees_collection::id()]));
-        let rent_collector = RentCollector {
-            epoch: 1,
-            ..RentCollector::default()
-        };
-
-        let address = Pubkey::new_unique();
-        let mut account = AccountSharedData::from(Account {
-            lamports: 1,
-            data: vec![0],
-            ..Account::default()
-        });
-
-        assert_eq!(
-            collect_rent_from_account(&feature_set, &rent_collector, &address, &mut account),
-            CollectedInfo {
-                rent_amount: 1,
-                account_data_len_reclaimed: 1
-            }
-        );
-        assert_eq!(account.rent_epoch(), 0);
-        assert_eq!(account.lamports(), 0);
-    }
-
     // Ensure `TransactionProcessingCallback::inspect_account()` is called when
     // loading accounts for transaction processing.
     #[test]

src/account_loader.rs

@@ -125,37 +125,6 @@ mod tests {
         solana_sdk_ids::system_program,
     };
 
-    #[test]
-    fn test_new_fee_payer_only() {
-        let fee_payer_address = Pubkey::new_unique();
-        let fee_payer_account = AccountSharedData::new(100, 0, &Pubkey::default());
-        let fee_payer_rent_epoch = fee_payer_account.rent_epoch();
-
-        const TEST_RENT_DEBIT: u64 = 1;
-        let rent_collected_fee_payer_account = {
-            let mut account = fee_payer_account.clone();
-            account.set_lamports(fee_payer_account.lamports() - TEST_RENT_DEBIT);
-            account.set_rent_epoch(fee_payer_rent_epoch + 1);
-            account
-        };
-
-        let rollback_accounts = RollbackAccounts::new(
-            None,
-            fee_payer_address,
-            rent_collected_fee_payer_account,
-            TEST_RENT_DEBIT,
-            fee_payer_rent_epoch,
-        );
-
-        let expected_fee_payer_account = fee_payer_account;
-        match rollback_accounts {
-            RollbackAccounts::FeePayerOnly { fee_payer_account } => {
-                assert_eq!(expected_fee_payer_account, fee_payer_account);
-            }
-            _ => panic!("Expected FeePayerOnly variant"),
-        }
-    }
-
     #[test]
     fn test_new_same_nonce_and_fee_payer() {
         let nonce_address = Pubkey::new_unique();

src/rollback_accounts.rs

@@ -469,8 +469,28 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
                     // This check is unnecessary for other load results (like `FeesOnly`),
                     // as those states imply the transaction failed to load these other accounts,
                     // and the fee payer is validated separately.
-                    if let Err(err) = loaded_transaction.validate_accounts_access(tx) {
-                        processing_results.push(Err(err));
+                    if let Err((err, offender)) = loaded_transaction.validate_accounts_access(tx) {
+                        // If an account access violation was detected, we construct a fake
+                        // execution for the convenience of the user, so that the transaction
+                        // will be persisted to the ledger with some useful debug information
+                        let execution_details = TransactionExecutionDetails {
+                            status: Err(err),
+                            log_messages: Some(vec![format!(
+                                "Account {offender} was used as writeable\
+                                without being delegated to this ER"
+                            )]),
+                            accounts_data_len_delta: 0,
+                            return_data: None,
+                            executed_units: 0,
+                            inner_instructions: None,
+                        };
+                        let txn = ExecutedTransaction {
+                            loaded_transaction,
+                            execution_details,
+                            programs_modified_by_tx: Default::default(),
+                        };
+                        let result = ProcessedTransaction::Executed(Box::new(txn));
+                        processing_results.push(Ok(result));
                         continue;
                     }
                     // observe all the account balances before executing the transaction
@@ -2398,86 +2418,6 @@ mod tests {
         );
     }
 
-    #[test]
-    fn test_validate_transaction_fee_payer_rent_paying() {
-        let lamports_per_signature = 5000;
-        let message = new_unchecked_sanitized_message(Message::new_with_blockhash(
-            &[],
-            Some(&Pubkey::new_unique()),
-            &Hash::new_unique(),
-        ));
-        let compute_budget_limits = process_compute_budget_instructions(
-            SVMMessage::program_instructions_iter(&message),
-            &FeatureSet::default(),
-        )
-        .unwrap();
-        let fee_payer_address = message.fee_payer();
-        let mut rent_collector = RentCollector::default();
-        rent_collector.rent.lamports_per_byte_year = 1_000_000;
-        let min_balance = rent_collector.rent.minimum_balance(0);
-        let transaction_fee = lamports_per_signature;
-        let starting_balance = min_balance - 1;
-        let mut fee_payer_account = AccountSharedData::new(starting_balance, 0, &Pubkey::default());
-        fee_payer_account.set_delegated(true);
-        let fee_payer_rent_debit = rent_collector
-            .get_rent_due(
-                fee_payer_account.lamports(),
-                fee_payer_account.data().len(),
-                fee_payer_account.rent_epoch(),
-            )
-            .lamports();
-        assert!(fee_payer_rent_debit > 0);
-
-        let mut mock_accounts = HashMap::new();
-        mock_accounts.insert(*fee_payer_address, fee_payer_account.clone());
-        let mock_bank = MockBankCallback {
-            account_shared_data: Arc::new(RwLock::new(mock_accounts)),
-            ..Default::default()
-        };
-        let mut account_loader = (&mock_bank).into();
-
-        let mut error_counters = TransactionErrorMetrics::default();
-        let result =
-            TransactionBatchProcessor::<TestForkGraph>::validate_transaction_nonce_and_fee_payer(
-                &mut account_loader,
-                &message,
-                CheckedTransactionDetails::new(None, lamports_per_signature),
-                &Hash::default(),
-                FeeStructure::default().lamports_per_signature,
-                &rent_collector,
-                &mut error_counters,
-                &mock_bank,
-            );
-
-        let post_validation_fee_payer_account = {
-            let mut account = fee_payer_account.clone();
-            account.set_rent_epoch(1);
-            account.set_lamports(starting_balance - transaction_fee - fee_payer_rent_debit);
-            account
-        };
-
-        assert_eq!(
-            result,
-            Ok(ValidatedTransactionDetails {
-                rollback_accounts: RollbackAccounts::new(
-                    None, // nonce
-                    *fee_payer_address,
-                    post_validation_fee_payer_account.clone(),
-                    fee_payer_rent_debit,
-                    0, // rent epoch
-                ),
-                compute_budget_limits,
-                fee_details: FeeDetails::new(transaction_fee, 0),
-                loaded_fee_payer_account: LoadedTransactionAccount {
-                    loaded_size: fee_payer_account.data().len(),
-                    account: post_validation_fee_payer_account,
-                    rent_collected: fee_payer_rent_debit,
-                },
-                fee_payer_address: *fee_payer_address,
-            })
-        );
-    }
-
     #[test]
     fn test_validate_transaction_fee_payer_not_found() {
         let lamports_per_signature = 5000;