From e5ea16fad840b12c62ae36e7dd1287552ee63f12 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Sun, 6 Jun 2021 18:27:29 +0000 Subject: [PATCH] system-program: Remove zero lamport check on transfers (#17726) (#17764) * 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 8f5e773caf41edc30f26a48cca5b4b1ffedf0a8d) Co-authored-by: Jon Cinque --- programs/stake/src/stake_instruction.rs | 19 +++------ runtime/src/system_instruction_processor.rs | 47 ++++++++++++++------- runtime/tests/stake.rs | 18 +++----- sdk/src/feature_set.rs | 5 +++ tokens/src/commands.rs | 2 +- transaction-status/src/parse_stake.rs | 14 +++--- 6 files changed, 59 insertions(+), 46 deletions(-) diff --git a/programs/stake/src/stake_instruction.rs b/programs/stake/src/stake_instruction.rs index fb20a38ad5..e89be82e7a 100644 --- a/programs/stake/src/stake_instruction.rs +++ b/programs/stake/src/stake_instruction.rs @@ -261,13 +261,8 @@ pub fn split( split_stake_pubkey: &Pubkey, ) -> Vec { vec![ - system_instruction::create_account( - authorized_pubkey, // Sending 0, so any signer will suffice - split_stake_pubkey, - 0, - std::mem::size_of::() as u64, - &id(), - ), + system_instruction::allocate(split_stake_pubkey, std::mem::size_of::() 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 { 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::() 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), ); diff --git a/runtime/src/system_instruction_processor.rs b/runtime/src/system_instruction_processor.rs index 155ea991e0..bab232ca5c 100644 --- a/runtime/src/system_instruction_processor.rs +++ b/runtime/src/system_instruction_processor.rs @@ -2,7 +2,7 @@ use log::*; use solana_sdk::{ account::{AccountSharedData, ReadableAccount, WritableAccount}, account_utils::StateMut, - ic_msg, + feature_set, ic_msg, instruction::InstructionError, keyed_account::{from_keyed_account, get_signers, keyed_account_at_index, KeyedAccount}, nonce, @@ -200,7 +200,9 @@ fn transfer( lamports: u64, invoke_context: &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: &dyn InvokeContext, ) -> Result<(), InstructionError> { - if lamports == 0 { + if !invoke_context.is_feature_active(&feature_set::system_transfer_zero_check::id()) + && lamports == 0 + { return Ok(()); } @@ -666,21 +670,21 @@ 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, - &[to].iter().cloned().collect::>(), + &[from, to].iter().cloned().collect::>(), &MockInvokeContext::new(vec![]), ), Ok(()) @@ -870,11 +874,11 @@ 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, @@ -882,7 +886,7 @@ mod tests { &[owned_key].iter().cloned().collect::>(), &MockInvokeContext::new(vec![]), ); - assert_eq!(result, Ok(())); + assert_eq!(result, Err(InstructionError::MissingRequiredSignature)); } #[test] @@ -1094,9 +1098,7 @@ 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, @@ -1106,6 +1108,21 @@ mod tests { .is_ok(),); 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); } #[test] diff --git a/runtime/tests/stake.rs b/runtime/tests/stake.rs index 400420ccc5..5f1d36bd1d 100644 --- a/runtime/tests/stake.rs +++ b/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 ); } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 377918eca6..f6cf3f9067 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -143,6 +143,10 @@ pub mod add_missing_program_error_mappings { solana_sdk::declare_id!("3QEUpjhgPEt92nz3Mqf6pABkHPGCQwSvKtyGMq4SuQyL"); } +pub mod system_transfer_zero_check { + solana_sdk::declare_id!("BrTR9hzw4WBGFP65AJMbpAo64DcA3U6jdPSga9fMV5cS"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -179,6 +183,7 @@ lazy_static! { (stake_program_v4::id(), "solana_stake_program v4"), (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"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/tokens/src/commands.rs b/tokens/src/commands.rs index 3951f704f9..d9adc24317 100644 --- a/tokens/src/commands.rs +++ b/tokens/src/commands.rs @@ -1467,7 +1467,7 @@ mod tests { )); // Same recipient, same lockups } - const SET_LOCKUP_INDEX: usize = 4; + const SET_LOCKUP_INDEX: usize = 5; #[test] fn test_set_split_stake_lockup() { diff --git a/transaction-status/src/parse_stake.rs b/transaction-status/src/parse_stake.rs index a0f4ed7d81..1f733459e3 100644 --- a/transaction-status/src/parse_stake.rs +++ b/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);