From b79abb4fab62da487c6834926ef309d4c1b69011 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Mon, 22 Aug 2022 23:38:56 +0100 Subject: [PATCH] Relax authority signer check for lookup table creation (#27248) * Relax authority signer check for lookup table creation * cli: support creating lookup tables without authority signer * add another create lookup table ix function * improve help message --- cli/src/address_lookup_table.rs | 66 +++++++++----- cli/tests/address_lookup_table.rs | 6 +- .../tests/create_lookup_table_ix.rs | 87 ++++++++++++++----- .../address-lookup-table/src/instruction.rs | 35 +++++++- .../address-lookup-table/src/processor.rs | 22 ++++- sdk/src/feature_set.rs | 5 ++ 6 files changed, 171 insertions(+), 50 deletions(-) diff --git a/cli/src/address_lookup_table.rs b/cli/src/address_lookup_table.rs index 7f0fa9d31..9096fc1d4 100644 --- a/cli/src/address_lookup_table.rs +++ b/cli/src/address_lookup_table.rs @@ -3,8 +3,8 @@ use { clap::{App, AppSettings, Arg, ArgMatches, SubCommand}, solana_address_lookup_table_program::{ instruction::{ - close_lookup_table, create_lookup_table, deactivate_lookup_table, extend_lookup_table, - freeze_lookup_table, + close_lookup_table, create_lookup_table, create_lookup_table_signed, + deactivate_lookup_table, extend_lookup_table, freeze_lookup_table, }, state::AddressLookupTable, }, @@ -14,7 +14,7 @@ use { solana_remote_wallet::remote_wallet::RemoteWalletManager, solana_sdk::{ account::from_account, clock::Clock, commitment_config::CommitmentConfig, message::Message, - pubkey::Pubkey, sysvar, transaction::Transaction, + pubkey::Pubkey, signer::Signer, sysvar, transaction::Transaction, }, std::sync::Arc, }; @@ -22,7 +22,8 @@ use { #[derive(Debug, PartialEq, Eq)] pub enum AddressLookupTableCliCommand { CreateLookupTable { - authority_signer_index: SignerIndex, + authority_pubkey: Pubkey, + authority_signer_index: Option, payer_signer_index: SignerIndex, }, FreezeLookupTable { @@ -67,10 +68,23 @@ impl AddressLookupTableSubCommands for App<'_, '_> { .arg( Arg::with_name("authority") .long("authority") + .value_name("AUTHORITY_PUBKEY") + .takes_value(true) + .validator(is_pubkey) + .help( + "Lookup table authority address [default: the default configured keypair]. \ + WARNING: Cannot be used for creating a lookup table for a cluster running v1.11 + or earlier which requires the authority to sign for lookup table creation.", + ) + ) + .arg( + Arg::with_name("authority_signer") + .long("authority-signer") .value_name("AUTHORITY_SIGNER") .takes_value(true) + .conflicts_with("authority") .validator(is_valid_signer) - .help("Lookup table authority [default: the default configured keypair]") + .help("Lookup table authority keypair [default: the default configured keypair].") ) .arg( Arg::with_name("payer") @@ -230,16 +244,16 @@ pub fn parse_address_lookup_table_subcommand( )]; let authority_pubkey = if let Ok((authority_signer, Some(authority_pubkey))) = - signer_of(matches, "authority", wallet_manager) + signer_of(matches, "authority_signer", wallet_manager) { bulk_signers.push(authority_signer); - Some(authority_pubkey) + authority_pubkey + } else if let Some(authority_pubkey) = pubkey_of(matches, "authority") { + authority_pubkey } else { - Some( - default_signer - .signer_from_path(matches, wallet_manager)? - .pubkey(), - ) + default_signer + .signer_from_path(matches, wallet_manager)? + .pubkey() }; let payer_pubkey = if let Ok((payer_signer, Some(payer_pubkey))) = @@ -261,7 +275,8 @@ pub fn parse_address_lookup_table_subcommand( CliCommandInfo { command: CliCommand::AddressLookupTable( AddressLookupTableCliCommand::CreateLookupTable { - authority_signer_index: signer_info.index_of(authority_pubkey).unwrap(), + authority_pubkey, + authority_signer_index: signer_info.index_of(Some(authority_pubkey)), payer_signer_index: signer_info.index_of(payer_pubkey).unwrap(), }, ), @@ -452,11 +467,13 @@ pub fn process_address_lookup_table_subcommand( ) -> ProcessResult { match subcommand { AddressLookupTableCliCommand::CreateLookupTable { + authority_pubkey, authority_signer_index, payer_signer_index, } => process_create_lookup_table( &rpc_client, config, + *authority_pubkey, *authority_signer_index, *payer_signer_index, ), @@ -515,10 +532,11 @@ pub fn process_address_lookup_table_subcommand( fn process_create_lookup_table( rpc_client: &RpcClient, config: &CliConfig, - authority_signer_index: usize, + authority_address: Pubkey, + authority_signer_index: Option, payer_signer_index: usize, ) -> ProcessResult { - let authority_signer = config.signers[authority_signer_index]; + let authority_signer = authority_signer_index.map(|index| config.signers[index]); let payer_signer = config.signers[payer_signer_index]; let get_clock_result = rpc_client @@ -528,10 +546,12 @@ fn process_create_lookup_table( CliError::RpcRequestError("Failed to deserialize clock sysvar".to_string()) })?; - let authority_address = authority_signer.pubkey(); let payer_address = payer_signer.pubkey(); - let (create_lookup_table_ix, lookup_table_address) = - create_lookup_table(authority_address, payer_address, clock.slot); + let (create_lookup_table_ix, lookup_table_address) = if authority_signer.is_some() { + create_lookup_table_signed(authority_address, payer_address, clock.slot) + } else { + create_lookup_table(authority_address, payer_address, clock.slot) + }; let blockhash = rpc_client.get_latest_blockhash()?; let mut tx = Transaction::new_unsigned(Message::new( @@ -539,10 +559,12 @@ fn process_create_lookup_table( Some(&config.signers[0].pubkey()), )); - tx.try_sign( - &[config.signers[0], authority_signer, payer_signer], - blockhash, - )?; + let mut keypairs: Vec<&dyn Signer> = vec![config.signers[0], payer_signer]; + if let Some(authority_signer) = authority_signer { + keypairs.push(authority_signer); + } + + tx.try_sign(&keypairs, blockhash)?; let result = rpc_client.send_and_confirm_transaction_with_spinner_and_config( &tx, config.commitment, diff --git a/cli/tests/address_lookup_table.rs b/cli/tests/address_lookup_table.rs index 5d370d48c..9bf7db320 100644 --- a/cli/tests/address_lookup_table.rs +++ b/cli/tests/address_lookup_table.rs @@ -42,7 +42,8 @@ fn test_cli_create_extend_and_freeze_address_lookup_table() { // Create lookup table config.command = CliCommand::AddressLookupTable(AddressLookupTableCliCommand::CreateLookupTable { - authority_signer_index: 0, + authority_pubkey: keypair.pubkey(), + authority_signer_index: None, payer_signer_index: 0, }); let response: CliAddressLookupTableCreated = @@ -156,7 +157,8 @@ fn test_cli_create_and_deactivate_address_lookup_table() { // Create lookup table config.command = CliCommand::AddressLookupTable(AddressLookupTableCliCommand::CreateLookupTable { - authority_signer_index: 0, + authority_pubkey: keypair.pubkey(), + authority_signer_index: Some(0), payer_signer_index: 0, }); let response: CliAddressLookupTableCreated = diff --git a/programs/address-lookup-table-tests/tests/create_lookup_table_ix.rs b/programs/address-lookup-table-tests/tests/create_lookup_table_ix.rs index b91a31802..406648485 100644 --- a/programs/address-lookup-table-tests/tests/create_lookup_table_ix.rs +++ b/programs/address-lookup-table-tests/tests/create_lookup_table_ix.rs @@ -3,20 +3,29 @@ use { common::{assert_ix_error, overwrite_slot_hashes_with_slots, setup_test_context}, solana_address_lookup_table_program::{ id, - instruction::create_lookup_table, + instruction::{create_lookup_table, create_lookup_table_signed}, + processor::process_instruction, state::{AddressLookupTable, LOOKUP_TABLE_META_SIZE}, }, solana_program_test::*, solana_sdk::{ - clock::Slot, instruction::InstructionError, pubkey::Pubkey, rent::Rent, signature::Signer, - signer::keypair::Keypair, transaction::Transaction, + clock::Slot, feature_set, instruction::InstructionError, pubkey::Pubkey, rent::Rent, + signature::Signer, signer::keypair::Keypair, transaction::Transaction, }, }; mod common; +pub async fn setup_test_context_without_authority_feature() -> ProgramTestContext { + let mut program_test = ProgramTest::new("", id(), Some(process_instruction)); + program_test.deactivate_feature( + feature_set::relax_authority_signer_check_for_lookup_table_creation::id(), + ); + program_test.start_with_context().await +} + #[tokio::test] -async fn test_create_lookup_table() { +async fn test_create_lookup_table_idempotent() { let mut context = setup_test_context().await; let test_recent_slot = 123; @@ -25,8 +34,7 @@ async fn test_create_lookup_table() { let client = &mut context.banks_client; let payer = &context.payer; let recent_blockhash = context.last_blockhash; - let authority_keypair = Keypair::new(); - let authority_address = authority_keypair.pubkey(); + let authority_address = Pubkey::new_unique(); let (create_lookup_table_ix, lookup_table_address) = create_lookup_table(authority_address, payer.pubkey(), test_recent_slot); @@ -35,7 +43,7 @@ async fn test_create_lookup_table() { let transaction = Transaction::new_signed_with_payer( &[create_lookup_table_ix.clone()], Some(&payer.pubkey()), - &[payer, &authority_keypair], + &[payer], recent_blockhash, ); @@ -45,7 +53,7 @@ async fn test_create_lookup_table() { .await .unwrap() .unwrap(); - assert_eq!(lookup_table_account.owner, crate::id()); + assert_eq!(lookup_table_account.owner, id()); assert_eq!(lookup_table_account.data.len(), LOOKUP_TABLE_META_SIZE); assert_eq!( lookup_table_account.lamports, @@ -59,6 +67,47 @@ async fn test_create_lookup_table() { assert_eq!(lookup_table.addresses.len(), 0); } + // Second create should succeed too + { + let recent_blockhash = client + .get_new_latest_blockhash(&recent_blockhash) + .await + .unwrap(); + let transaction = Transaction::new_signed_with_payer( + &[create_lookup_table_ix], + Some(&payer.pubkey()), + &[payer], + recent_blockhash, + ); + + assert_matches!(client.process_transaction(transaction).await, Ok(())); + } +} + +#[tokio::test] +async fn test_create_lookup_table_not_idempotent() { + let mut context = setup_test_context_without_authority_feature().await; + + let test_recent_slot = 123; + overwrite_slot_hashes_with_slots(&mut context, &[test_recent_slot]); + + let client = &mut context.banks_client; + let payer = &context.payer; + let recent_blockhash = context.last_blockhash; + let authority_keypair = Keypair::new(); + let authority_address = authority_keypair.pubkey(); + let (create_lookup_table_ix, ..) = + create_lookup_table_signed(authority_address, payer.pubkey(), test_recent_slot); + + let transaction = Transaction::new_signed_with_payer( + &[create_lookup_table_ix.clone()], + Some(&payer.pubkey()), + &[payer, &authority_keypair], + recent_blockhash, + ); + + assert_matches!(client.process_transaction(transaction).await, Ok(())); + // Second create should fail { context.last_blockhash = client @@ -97,11 +146,11 @@ async fn test_create_lookup_table_use_payer_as_authority() { } #[tokio::test] -async fn test_create_lookup_table_without_signer() { - let mut context = setup_test_context().await; +async fn test_create_lookup_table_missing_signer() { + let mut context = setup_test_context_without_authority_feature().await; let unsigned_authority_address = Pubkey::new_unique(); - let mut ix = create_lookup_table( + let mut ix = create_lookup_table_signed( unsigned_authority_address, context.payer.pubkey(), Slot::MAX, @@ -122,15 +171,14 @@ async fn test_create_lookup_table_without_signer() { async fn test_create_lookup_table_not_recent_slot() { let mut context = setup_test_context().await; let payer = &context.payer; - let authority_keypair = Keypair::new(); - let authority_address = authority_keypair.pubkey(); + let authority_address = Pubkey::new_unique(); let ix = create_lookup_table(authority_address, payer.pubkey(), Slot::MAX).0; assert_ix_error( &mut context, ix, - Some(&authority_keypair), + None, InstructionError::InvalidInstructionData, ) .await; @@ -142,17 +190,10 @@ async fn test_create_lookup_table_pda_mismatch() { let test_recent_slot = 123; overwrite_slot_hashes_with_slots(&mut context, &[test_recent_slot]); let payer = &context.payer; - let authority_keypair = Keypair::new(); - let authority_address = authority_keypair.pubkey(); + let authority_address = Pubkey::new_unique(); let mut ix = create_lookup_table(authority_address, payer.pubkey(), test_recent_slot).0; ix.accounts[0].pubkey = Pubkey::new_unique(); - assert_ix_error( - &mut context, - ix, - Some(&authority_keypair), - InstructionError::InvalidArgument, - ) - .await; + assert_ix_error(&mut context, ix, None, InstructionError::InvalidArgument).await; } diff --git a/programs/address-lookup-table/src/instruction.rs b/programs/address-lookup-table/src/instruction.rs index 80a6ddb7a..573dbe561 100644 --- a/programs/address-lookup-table/src/instruction.rs +++ b/programs/address-lookup-table/src/instruction.rs @@ -80,10 +80,43 @@ pub fn derive_lookup_table_address( /// Constructs an instruction to create a table account and returns /// the instruction and the table account's derived address. +/// +/// # Note +/// +/// This instruction requires the authority to be a signer but +/// in v1.12 the address lookup table program will no longer require +/// the authority to sign the transaction. +pub fn create_lookup_table_signed( + authority_address: Pubkey, + payer_address: Pubkey, + recent_slot: Slot, +) -> (Instruction, Pubkey) { + create_lookup_table_common(authority_address, payer_address, recent_slot, true) +} + +/// Constructs an instruction to create a table account and returns +/// the instruction and the table account's derived address. +/// +/// # Note +/// +/// This instruction doesn't require the authority to be a signer but +/// until v1.12 the address lookup table program still requires the +/// authority to sign the transaction. pub fn create_lookup_table( authority_address: Pubkey, payer_address: Pubkey, recent_slot: Slot, +) -> (Instruction, Pubkey) { + create_lookup_table_common(authority_address, payer_address, recent_slot, false) +} + +/// Constructs an instruction to create a table account and returns +/// the instruction and the table account's derived address. +fn create_lookup_table_common( + authority_address: Pubkey, + payer_address: Pubkey, + recent_slot: Slot, + authority_is_signer: bool, ) -> (Instruction, Pubkey) { let (lookup_table_address, bump_seed) = derive_lookup_table_address(&authority_address, recent_slot); @@ -95,7 +128,7 @@ pub fn create_lookup_table( }, vec![ AccountMeta::new(lookup_table_address, false), - AccountMeta::new_readonly(authority_address, true), + AccountMeta::new_readonly(authority_address, authority_is_signer), AccountMeta::new(payer_address, true), AccountMeta::new_readonly(system_program::id(), false), ], diff --git a/programs/address-lookup-table/src/processor.rs b/programs/address-lookup-table/src/processor.rs index 39d36b988..faf2dc055 100644 --- a/programs/address-lookup-table/src/processor.rs +++ b/programs/address-lookup-table/src/processor.rs @@ -9,6 +9,7 @@ use { solana_program_runtime::{ic_msg, invoke_context::InvokeContext}, solana_sdk::{ clock::Slot, + feature_set, instruction::InstructionError, program_utils::limited_deserialize, pubkey::{Pubkey, PUBKEY_BYTES}, @@ -58,7 +59,12 @@ impl Processor { instruction_context.try_borrow_instruction_account(transaction_context, 0)?; let lookup_table_lamports = lookup_table_account.get_lamports(); let table_key = *lookup_table_account.get_key(); - if !lookup_table_account.get_data().is_empty() { + let lookup_table_owner = *lookup_table_account.get_owner(); + if !invoke_context + .feature_set + .is_active(&feature_set::relax_authority_signer_check_for_lookup_table_creation::id()) + && !lookup_table_account.get_data().is_empty() + { ic_msg!(invoke_context, "Table account must not be allocated"); return Err(InstructionError::AccountAlreadyInitialized); } @@ -67,7 +73,11 @@ impl Processor { let authority_account = instruction_context.try_borrow_instruction_account(transaction_context, 1)?; let authority_key = *authority_account.get_key(); - if !authority_account.is_signer() { + if !invoke_context + .feature_set + .is_active(&feature_set::relax_authority_signer_check_for_lookup_table_creation::id()) + && !authority_account.is_signer() + { ic_msg!(invoke_context, "Authority account must be a signer"); return Err(InstructionError::MissingRequiredSignature); } @@ -116,6 +126,14 @@ impl Processor { return Err(InstructionError::InvalidArgument); } + if invoke_context + .feature_set + .is_active(&feature_set::relax_authority_signer_check_for_lookup_table_creation::id()) + && crate::check_id(&lookup_table_owner) + { + return Ok(()); + } + let table_account_data_len = LOOKUP_TABLE_META_SIZE; let rent = invoke_context.get_sysvar_cache().get_rent()?; let required_lamports = rent diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index f3c869480..a0770e5fa 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -502,6 +502,10 @@ pub mod disable_cpi_setting_executable_and_rent_epoch { solana_sdk::declare_id!("B9cdB55u4jQsDNsdTK525yE9dmSc5Ga7YBaBrDFvEhM9"); } +pub mod relax_authority_signer_check_for_lookup_table_creation { + solana_sdk::declare_id!("FKAcEvNgSY79RpqsPNUV5gDyumopH4cEHqUxyfm8b8Ap"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -622,6 +626,7 @@ lazy_static! { (check_ping_ancestor_requests::id(), "ancestor hash repair socket ping/pong support #26963"), (incremental_snapshot_only_incremental_hash_calculation::id(), "only hash accounts in incremental snapshot during incremental snapshot creation #26799"), (disable_cpi_setting_executable_and_rent_epoch::id(), "disable setting is_executable and_rent_epoch in CPI #26987"), + (relax_authority_signer_check_for_lookup_table_creation::id(), "relax authority signer check for lookup table creation #27205"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()