system-program: Remove zero lamport check on transfers (#17726)

* system-program: Move lamports == 0 check on transfers

* Address feedback

* Update stake split to explicitly allocate + assign

* Update stake tests referring to split instruction

* Revert whitespace

* Update split instruction index in test

* Remove unnecessary `assign_with_seed` from `split_with_seed`

* Fix stake instruction parser

* Update test to allow splitting into account with lamports

(cherry picked from commit 8f5e773)

# Conflicts:
#	runtime/src/system_instruction_processor.rs
#	sdk/src/feature_set.rs

programs/stake/src/stake_instruction.rs

@@ -261,13 +261,8 @@ pub fn split(
     split_stake_pubkey: &Pubkey,
 ) -> Vec<Instruction> {
     vec![
-        system_instruction::create_account(
-            authorized_pubkey, // Sending 0, so any signer will suffice
-            split_stake_pubkey,
-            0,
-            std::mem::size_of::<StakeState>() as u64,
-            &id(),
-        ),
+        system_instruction::allocate(split_stake_pubkey, std::mem::size_of::<StakeState>() as u64),
+        system_instruction::assign(split_stake_pubkey, &id()),
         _split(
             stake_pubkey,
             authorized_pubkey,
@@ -286,12 +281,10 @@ pub fn split_with_seed(
     seed: &str,                  // seed
 ) -> Vec<Instruction> {
     vec![
-        system_instruction::create_account_with_seed(
-            authorized_pubkey, // Sending 0, so any signer will suffice
+        system_instruction::allocate_with_seed(
             split_stake_pubkey,
             base,
             seed,
-            0,
             std::mem::size_of::<StakeState>() as u64,
             &id(),
         ),
@@ -766,7 +759,7 @@ mod tests {
                     &Pubkey::default(),
                     100,
                     &invalid_stake_state_pubkey(),
-                )[1]
+                )[2]
             ),
             Err(InstructionError::InvalidAccountData),
         );
@@ -852,7 +845,7 @@ mod tests {
                     &Pubkey::default(),
                     100,
                     &Pubkey::default(),
-                )[1]
+                )[2]
             ),
             Err(InstructionError::InvalidAccountOwner),
         );
@@ -863,7 +856,7 @@ mod tests {
                     &Pubkey::default(),
                     100,
                     &spoofed_stake_state_pubkey(),
-                )[1]
+                )[2]
             ),
             Err(InstructionError::IncorrectProgramId),
         );

runtime/src/system_instruction_processor.rs

@@ -2,7 +2,7 @@ use log::*;
 use solana_sdk::{
     account::{AccountSharedData, ReadableAccount},
     account_utils::StateMut,
-    ic_msg,
+    feature_set, ic_msg,
     instruction::InstructionError,
     keyed_account::{from_keyed_account, get_signers, next_keyed_account, KeyedAccount},
     nonce,
@@ -200,7 +200,9 @@ fn transfer(
     lamports: u64,
     invoke_context: &mut dyn InvokeContext,
 ) -> Result<(), InstructionError> {
-    if lamports == 0 {
+    if !invoke_context.is_feature_active(&feature_set::system_transfer_zero_check::id())
+        && lamports == 0
+    {
         return Ok(());
     }
 
@@ -225,7 +227,9 @@ fn transfer_with_seed(
     lamports: u64,
     invoke_context: &mut dyn InvokeContext,
 ) -> Result<(), InstructionError> {
-    if lamports == 0 {
+    if !invoke_context.is_feature_active(&feature_set::system_transfer_zero_check::id())
+        && lamports == 0
+    {
         return Ok(());
     }
 
@@ -659,22 +663,27 @@ mod tests {
     fn test_create_with_zero_lamports() {
         // create account with zero lamports transferred
         let new_owner = Pubkey::new(&[9; 32]);
-        let from = solana_sdk::pubkey::new_rand();
-        let from_account = AccountSharedData::new_ref(100, 1, &solana_sdk::pubkey::new_rand()); // not from system account
+        let from = Pubkey::new_unique();
+        let from_account = AccountSharedData::new_ref(100, 0, &Pubkey::new_unique()); // not from system account
 
-        let to = solana_sdk::pubkey::new_rand();
+        let to = Pubkey::new_unique();
         let to_account = AccountSharedData::new_ref(0, 0, &Pubkey::default());
 
         assert_eq!(
             create_account(
-                &KeyedAccount::new(&from, false, &from_account), // no signer
-                &KeyedAccount::new(&to, false, &to_account),
+                &KeyedAccount::new(&from, true, &from_account),
+                &KeyedAccount::new(&to, true, &to_account),
                 &to.into(),
                 0,
                 2,
                 &new_owner,
+<<<<<<< HEAD
                 &[to].iter().cloned().collect::<HashSet<_>>(),
                 &mut MockInvokeContext::default(),
+=======
+                &[from, to].iter().cloned().collect::<HashSet<_>>(),
+                &MockInvokeContext::new(vec![]),
+>>>>>>> 8f5e773ca (system-program: Remove zero lamport check on transfers (#17726))
             ),
             Ok(())
         );
@@ -863,19 +872,19 @@ mod tests {
         );
         assert_eq!(result, Err(InstructionError::MissingRequiredSignature));
 
-        // support creation/assignment with zero lamports (ephemeral account)
+        // Don't support unsigned creation with zero lamports (ephemeral account)
         let owned_account = AccountSharedData::new_ref(0, 0, &Pubkey::default());
         let result = create_account(
             &KeyedAccount::new(&from, false, &from_account),
-            &KeyedAccount::new(&owned_key, false, &owned_account),
+            &KeyedAccount::new(&owned_key, true, &owned_account),
             &owned_address,
             0,
             2,
             &new_owner,
             &[owned_key].iter().cloned().collect::<HashSet<_>>(),
             &mut MockInvokeContext::default(),
         );
-        assert_eq!(result, Ok(()));
+        assert_eq!(result, Err(InstructionError::MissingRequiredSignature));
     }
 
     #[test]
@@ -1087,18 +1096,36 @@ mod tests {
         assert_eq!(from_keyed_account.account.borrow().lamports, 50);
         assert_eq!(to_keyed_account.account.borrow().lamports, 51);
 
-        // test unsigned transfer of zero
-        let from_keyed_account = KeyedAccount::new(&from, false, &from_account);
-
+        // test signed transfer of zero
         assert!(transfer(
             &from_keyed_account,
             &to_keyed_account,
             0,
             &mut MockInvokeContext::default(),
         )
         .is_ok(),);
+<<<<<<< HEAD
         assert_eq!(from_keyed_account.account.borrow().lamports, 50);
         assert_eq!(to_keyed_account.account.borrow().lamports, 51);
+=======
+        assert_eq!(from_keyed_account.account.borrow().lamports(), 50);
+        assert_eq!(to_keyed_account.account.borrow().lamports(), 51);
+
+        // test unsigned transfer of zero
+        let from_keyed_account = KeyedAccount::new(&from, false, &from_account);
+
+        assert_eq!(
+            transfer(
+                &from_keyed_account,
+                &to_keyed_account,
+                0,
+                &MockInvokeContext::new(vec![]),
+            ),
+            Err(InstructionError::MissingRequiredSignature)
+        );
+        assert_eq!(from_keyed_account.account.borrow().lamports(), 50);
+        assert_eq!(to_keyed_account.account.borrow().lamports(), 51);
+>>>>>>> 8f5e773ca (system-program: Remove zero lamport check on transfers (#17726))
     }
 
     #[test]

runtime/tests/stake.rs

@@ -11,9 +11,7 @@ use solana_sdk::{
     message::Message,
     pubkey::Pubkey,
     signature::{Keypair, Signer},
-    system_instruction::SystemError,
     sysvar::{self, stake_history::StakeHistory},
-    transaction::TransactionError,
 };
 use solana_stake_program::{
     stake_instruction::{self},
@@ -171,7 +169,7 @@ fn test_stake_create_and_split_single_signature() {
 
 #[test]
 fn test_stake_create_and_split_to_existing_system_account() {
-    // Ensure stake-split does not allow the user to promote an existing system account into
+    // Ensure stake-split allows the user to promote an existing system account into
     // a stake account.
 
     solana_logger::setup();
@@ -229,7 +227,7 @@ fn test_stake_create_and_split_to_existing_system_account() {
         existing_lamports
     );
 
-    // Verify the split fails because the account is already in use
+    // Verify the split succeeds with lamports in the destination account
     let message = Message::new(
         &stake_instruction::split_with_seed(
             &stake_address, // original
@@ -241,16 +239,12 @@ fn test_stake_create_and_split_to_existing_system_account() {
         ),
         Some(&staker_keypair.pubkey()),
     );
-    assert_eq!(
-        bank_client
-            .send_and_confirm_message(&[&staker_keypair], message)
-            .unwrap_err()
-            .unwrap(),
-        TransactionError::InstructionError(0, SystemError::AccountAlreadyInUse.into())
-    );
+    bank_client
+        .send_and_confirm_message(&[&staker_keypair], message)
+        .expect("failed to split into account with lamports");
     assert_eq!(
         bank_client.get_balance(&split_stake_address).unwrap(),
-        existing_lamports
+        existing_lamports + lamports / 2
     );
 }
 

sdk/src/feature_set.rs

@@ -142,6 +142,21 @@ pub mod stake_program_v4 {
     solana_sdk::declare_id!("Dc7djyhP9aLfdq2zktpvskeAjpG56msCU1yexpxXiWZb");
 }
 
+<<<<<<< HEAD
+=======
+pub mod memory_ops_syscalls {
+    solana_sdk::declare_id!("ENQi37wsVhTvFz2gUiZAAbqFEWGN2jwFsqdEDTE8A4MU");
+}
+
+pub mod add_missing_program_error_mappings {
+    solana_sdk::declare_id!("3QEUpjhgPEt92nz3Mqf6pABkHPGCQwSvKtyGMq4SuQyL");
+}
+
+pub mod system_transfer_zero_check {
+    solana_sdk::declare_id!("BrTR9hzw4WBGFP65AJMbpAo64DcA3U6jdPSga9fMV5cS");
+}
+
+>>>>>>> 8f5e773ca (system-program: Remove zero lamport check on transfers (#17726))
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -176,6 +191,12 @@ lazy_static! {
         (set_upgrade_authority_via_cpi_enabled::id(), "set upgrade authority instruction via cpi calls for upgradable programs"),
         (keccak256_syscall_enabled::id(), "keccak256 syscall"),
         (stake_program_v4::id(), "solana_stake_program v4"),
+<<<<<<< HEAD
+=======
+        (memory_ops_syscalls::id(), "add syscalls for memory operations"),
+        (add_missing_program_error_mappings::id(), "add missing program error mappings"),
+        (system_transfer_zero_check::id(), "perform all checks for transfers of 0 lamports"),
+>>>>>>> 8f5e773ca (system-program: Remove zero lamport check on transfers (#17726))
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()

tokens/src/commands.rs

@@ -1289,7 +1289,7 @@ mod tests {
         )); // Same recipient, same lockups
     }
 
-    const SET_LOCKUP_INDEX: usize = 4;
+    const SET_LOCKUP_INDEX: usize = 5;
 
     #[test]
     fn test_set_stake_lockup() {

transaction-status/src/parse_stake.rs

@@ -309,21 +309,25 @@ mod test {
         );
         assert!(parse_stake(&message.instructions[0], &keys[0..5]).is_err());
 
-        let instructions = stake_instruction::split(&keys[2], &keys[0], lamports, &keys[1]);
+        // This looks wrong, but in an actual compiled instruction, the order is:
+        //  * split account (signer, allocate + assign first)
+        //  * stake authority (signer)
+        //  * stake account
+        let instructions = stake_instruction::split(&keys[2], &keys[1], lamports, &keys[0]);
         let message = Message::new(&instructions, None);
         assert_eq!(
-            parse_stake(&message.instructions[1], &keys[0..3]).unwrap(),
+            parse_stake(&message.instructions[2], &keys[0..3]).unwrap(),
             ParsedInstructionEnum {
                 instruction_type: "split".to_string(),
                 info: json!({
                     "stakeAccount": keys[2].to_string(),
-                    "newSplitAccount": keys[1].to_string(),
-                    "stakeAuthority": keys[0].to_string(),
+                    "newSplitAccount": keys[0].to_string(),
+                    "stakeAuthority": keys[1].to_string(),
                     "lamports": lamports,
                 }),
             }
         );
-        assert!(parse_stake(&message.instructions[1], &keys[0..2]).is_err());
+        assert!(parse_stake(&message.instructions[2], &keys[0..2]).is_err());
 
         let instruction = stake_instruction::withdraw(&keys[1], &keys[0], &keys[2], lamports, None);
         let message = Message::new(&[instruction], None);