From ad7d9c9b5b55d5bf776cce2e49c4d3c57db08293 Mon Sep 17 00:00:00 2001 From: Rob Walker Date: Mon, 13 Jan 2020 14:50:59 -0800 Subject: [PATCH 1/2] Remove create account bandaid now that requires signature --- runtime/src/system_instruction_processor.rs | 79 +++++++++++++++++++-- sdk/src/system_instruction.rs | 4 +- 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/runtime/src/system_instruction_processor.rs b/runtime/src/system_instruction_processor.rs index 4594a5d7ddf87b..ab4d285584f8d1 100644 --- a/runtime/src/system_instruction_processor.rs +++ b/runtime/src/system_instruction_processor.rs @@ -84,10 +84,8 @@ fn finish_create_account( } // if it looks like the `to` account is already in use, bail - if to.account.lamports != 0 - || !to.account.data.is_empty() - || !system_program::check_id(&to.account.owner) - { + // (note that the id check is also enforced by message_processor) + if !to.account.data.is_empty() || !system_program::check_id(&to.account.owner) { debug!( "CreateAccount: invalid argument; account {} already in use", to.unsigned_key() @@ -116,7 +114,7 @@ fn finish_create_account( return Err(SystemError::InvalidAccountDataLength.into()); } - // guard against sysvars being assigned + // guard against sysvars being made if sysvar::check_id(&program_id) { debug!("Assign: program id {} invalid", program_id); return Err(SystemError::InvalidProgramId.into()); @@ -136,12 +134,17 @@ fn assign_account_to_program( keyed_account: &mut KeyedAccount, program_id: &Pubkey, ) -> Result<(), InstructionError> { + // no work to do, just return + if keyed_account.account.owner == *program_id { + return Ok(()); + } + if keyed_account.signer_key().is_none() { debug!("Assign: account must sign"); return Err(InstructionError::MissingRequiredSignature); } - // guard against sysvars being assigned + // guard against sysvars being made if sysvar::check_id(&program_id) { debug!("Assign: program id {} invalid", program_id); return Err(SystemError::InvalidProgramId.into()); @@ -515,6 +518,7 @@ mod tests { #[test] fn test_create_already_in_use() { + solana_logger::setup(); // Attempt to create system account in account already owned by another program let new_program_owner = Pubkey::new(&[9; 32]); let from = Pubkey::new_rand(); @@ -538,7 +542,8 @@ mod tests { assert_eq!(from_lamports, 100); assert_eq!(owned_account, unchanged_account); - let mut owned_account = Account::new(10, 0, &Pubkey::default()); + // Attempt to create system account in account that already has data + let mut owned_account = Account::new(0, 1, &Pubkey::default()); let unchanged_account = owned_account.clone(); let result = create_account( &mut KeyedAccount::new(&from, true, &mut from_account), @@ -551,6 +556,19 @@ mod tests { let from_lamports = from_account.lamports; assert_eq!(from_lamports, 100); assert_eq!(owned_account, unchanged_account); + + // Verify that create_account works even if `to` has a non-zero balance + let mut owned_account = Account::new(1, 0, &Pubkey::default()); + let result = create_account( + &mut KeyedAccount::new(&from, true, &mut from_account), + &mut KeyedAccount::new(&owned_key, true, &mut owned_account), + 50, + 2, + &new_program_owner, + ); + assert_eq!(result, Ok(())); + assert_eq!(from_account.lamports, from_lamports - 50); + assert_eq!(owned_account.lamports, 1 + 50); } #[test] @@ -700,6 +718,14 @@ mod tests { ), Err(InstructionError::MissingRequiredSignature) ); + // no change, no signature needed + assert_eq!( + assign_account_to_program( + &mut KeyedAccount::new(&from, false, &mut from_account), + &system_program::id(), + ), + Ok(()) + ); assert_eq!( process_instruction( @@ -816,6 +842,45 @@ mod tests { ) } + #[test] + fn test_create_account_no_transfer() { + solana_logger::setup(); + let (genesis_config, alice_keypair) = create_genesis_config(100); + let alice_pubkey = alice_keypair.pubkey(); + let bob_keypair = Keypair::new(); + let bob_pubkey = bob_keypair.pubkey(); + + // Fund to account to bypass AccountNotFound error + let bank = Bank::new(&genesis_config); + let bank_client = BankClient::new(bank); + bank_client + .transfer(50, &alice_keypair, &bob_pubkey) + .unwrap(); + let program_id = Pubkey::new_rand(); + + // test system_instruction: that alice's signature is not asked for + let instruction = + system_instruction::create_account(&alice_pubkey, &bob_pubkey, 0, 1, &program_id); + + assert!(!instruction.accounts[0].is_signer); + + // test system_instruction_processor: that alice's signature is needed + assert!(bank_client + .send_instruction(&bob_keypair, instruction) + .is_ok()); + + assert_eq!(bank_client.get_balance(&alice_pubkey).unwrap(), 50); + assert_eq!( + bank_client.get_account(&bob_pubkey).unwrap().unwrap(), + Account { + data: vec![0; 1], + owner: program_id, + lamports: 50, + ..Account::default() + } + ); + } + #[test] fn test_system_unsigned_transaction() { let (genesis_config, alice_keypair) = create_genesis_config(100); diff --git a/sdk/src/system_instruction.rs b/sdk/src/system_instruction.rs index fd66b78e11422f..86fe26710d3b01 100644 --- a/sdk/src/system_instruction.rs +++ b/sdk/src/system_instruction.rs @@ -147,7 +147,7 @@ pub fn create_account( program_id: &Pubkey, ) -> Instruction { let account_metas = vec![ - AccountMeta::new(*from_pubkey, true), + AccountMeta::new(*from_pubkey, lamports != 0), // no signature required if no transfer AccountMeta::new(*to_pubkey, true), ]; Instruction::new( @@ -173,7 +173,7 @@ pub fn create_account_with_seed( program_id: &Pubkey, ) -> Instruction { let account_metas = vec![ - AccountMeta::new(*from_pubkey, true), + AccountMeta::new(*from_pubkey, lamports != 0), AccountMeta::new(*to_pubkey, false), ] .with_signer(base); From f4b4842d2fddaae906345e535e488690bb3f5383 Mon Sep 17 00:00:00 2001 From: Rob Walker Date: Wed, 15 Jan 2020 11:24:11 -0800 Subject: [PATCH 2/2] shrink scope of this PR to bandaid --- runtime/src/system_instruction_processor.rs | 52 --------------------- sdk/src/system_instruction.rs | 4 +- 2 files changed, 2 insertions(+), 54 deletions(-) diff --git a/runtime/src/system_instruction_processor.rs b/runtime/src/system_instruction_processor.rs index ab4d285584f8d1..73ef4e0d65f83b 100644 --- a/runtime/src/system_instruction_processor.rs +++ b/runtime/src/system_instruction_processor.rs @@ -134,11 +134,6 @@ fn assign_account_to_program( keyed_account: &mut KeyedAccount, program_id: &Pubkey, ) -> Result<(), InstructionError> { - // no work to do, just return - if keyed_account.account.owner == *program_id { - return Ok(()); - } - if keyed_account.signer_key().is_none() { debug!("Assign: account must sign"); return Err(InstructionError::MissingRequiredSignature); @@ -718,14 +713,6 @@ mod tests { ), Err(InstructionError::MissingRequiredSignature) ); - // no change, no signature needed - assert_eq!( - assign_account_to_program( - &mut KeyedAccount::new(&from, false, &mut from_account), - &system_program::id(), - ), - Ok(()) - ); assert_eq!( process_instruction( @@ -842,45 +829,6 @@ mod tests { ) } - #[test] - fn test_create_account_no_transfer() { - solana_logger::setup(); - let (genesis_config, alice_keypair) = create_genesis_config(100); - let alice_pubkey = alice_keypair.pubkey(); - let bob_keypair = Keypair::new(); - let bob_pubkey = bob_keypair.pubkey(); - - // Fund to account to bypass AccountNotFound error - let bank = Bank::new(&genesis_config); - let bank_client = BankClient::new(bank); - bank_client - .transfer(50, &alice_keypair, &bob_pubkey) - .unwrap(); - let program_id = Pubkey::new_rand(); - - // test system_instruction: that alice's signature is not asked for - let instruction = - system_instruction::create_account(&alice_pubkey, &bob_pubkey, 0, 1, &program_id); - - assert!(!instruction.accounts[0].is_signer); - - // test system_instruction_processor: that alice's signature is needed - assert!(bank_client - .send_instruction(&bob_keypair, instruction) - .is_ok()); - - assert_eq!(bank_client.get_balance(&alice_pubkey).unwrap(), 50); - assert_eq!( - bank_client.get_account(&bob_pubkey).unwrap().unwrap(), - Account { - data: vec![0; 1], - owner: program_id, - lamports: 50, - ..Account::default() - } - ); - } - #[test] fn test_system_unsigned_transaction() { let (genesis_config, alice_keypair) = create_genesis_config(100); diff --git a/sdk/src/system_instruction.rs b/sdk/src/system_instruction.rs index 86fe26710d3b01..fd66b78e11422f 100644 --- a/sdk/src/system_instruction.rs +++ b/sdk/src/system_instruction.rs @@ -147,7 +147,7 @@ pub fn create_account( program_id: &Pubkey, ) -> Instruction { let account_metas = vec![ - AccountMeta::new(*from_pubkey, lamports != 0), // no signature required if no transfer + AccountMeta::new(*from_pubkey, true), AccountMeta::new(*to_pubkey, true), ]; Instruction::new( @@ -173,7 +173,7 @@ pub fn create_account_with_seed( program_id: &Pubkey, ) -> Instruction { let account_metas = vec![ - AccountMeta::new(*from_pubkey, lamports != 0), + AccountMeta::new(*from_pubkey, true), AccountMeta::new(*to_pubkey, false), ] .with_signer(base);