From 2120ef580864651f2e16096fd6daaee709cbe3cf Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 17 Feb 2022 00:44:44 +0000 Subject: [PATCH] Fix ed25519 builtin program handling (backport #23182) (#23195) * Fix ed25519 builtin program handling (#23182) * Fix ed25519 builtin program handling * Fix tests * Add integration tests for processing transactions with ed25519 ixs * Fix another test * fix formatting (cherry picked from commit 813725dfec7aae2772b5f9436c42b824801c6353) * fix tests Co-authored-by: Justin Starry Co-authored-by: Jack May --- Cargo.lock | 11 +++ Cargo.toml | 1 + program-test/src/lib.rs | 36 +++++++- programs/ed25519-tests/Cargo.toml | 19 ++++ .../tests/process_transaction.rs | 87 +++++++++++++++++++ runtime/src/bank.rs | 18 ++-- runtime/src/builtins.rs | 19 +++- runtime/src/non_circulating_supply.rs | 2 +- sdk/src/precompiles.rs | 6 +- 9 files changed, 182 insertions(+), 17 deletions(-) create mode 100644 programs/ed25519-tests/Cargo.toml create mode 100644 programs/ed25519-tests/tests/process_transaction.rs diff --git a/Cargo.lock b/Cargo.lock index d823098e167ce9..80eea424fac509 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4875,6 +4875,17 @@ dependencies = [ "solana-sdk", ] +[[package]] +name = "solana-ed25519-program-tests" +version = "1.9.7" +dependencies = [ + "assert_matches", + "ed25519-dalek", + "rand 0.7.3", + "solana-program-test", + "solana-sdk", +] + [[package]] name = "solana-entry" version = "1.9.7" diff --git a/Cargo.toml b/Cargo.toml index 2cdca26d7ffa20..56d8b2dc99606f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,6 +48,7 @@ members = [ "program-test", "programs/address-lookup-table", "programs/address-lookup-table-tests", + "programs/ed25519-tests", "programs/bpf_loader", "programs/bpf_loader/gen-syscall-list", "programs/compute-budget", diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index dfecd564d240af..3b7723ca982297 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -25,6 +25,7 @@ use { account_info::AccountInfo, clock::Slot, entrypoint::{ProgramResult, SUCCESS}, + feature_set::FEATURE_NAMES, fee_calculator::{FeeCalculator, FeeRateGovernor}, genesis_config::{ClusterType, GenesisConfig}, hash::Hash, @@ -41,7 +42,7 @@ use { solana_vote_program::vote_state::{VoteState, VoteStateVersions}, std::{ cell::RefCell, - collections::HashMap, + collections::{HashMap, HashSet}, convert::TryFrom, fs::File, io::{self, Read}, @@ -58,7 +59,10 @@ use { tokio::task::JoinHandle, }; // Export types so test clients can limit their solana crate dependencies -pub use {solana_banks_client::BanksClient, solana_program_runtime::invoke_context::InvokeContext}; +pub use { + solana_banks_client::{BanksClient, BanksClientError}, + solana_program_runtime::invoke_context::InvokeContext, +}; pub mod programs; @@ -469,6 +473,7 @@ pub struct ProgramTest { compute_max_units: Option, prefer_bpf: bool, use_bpf_jit: bool, + deactivate_feature_set: HashSet, } impl Default for ProgramTest { @@ -499,6 +504,7 @@ impl Default for ProgramTest { compute_max_units: None, prefer_bpf, use_bpf_jit: false, + deactivate_feature_set: HashSet::default(), } } } @@ -728,6 +734,13 @@ impl ProgramTest { .push(Builtin::new(program_name, program_id, process_instruction)); } + /// Deactivate a runtime feature. + /// + /// Note that all features are activated by default. + pub fn deactivate_feature(&mut self, feature_id: Pubkey) { + self.deactivate_feature_set.insert(feature_id); + } + fn setup_bank( &self, ) -> ( @@ -767,6 +780,25 @@ impl ProgramTest { ClusterType::Development, vec![], ); + + // Remove features tagged to deactivate + for deactivate_feature_pk in &self.deactivate_feature_set { + if FEATURE_NAMES.contains_key(deactivate_feature_pk) { + match genesis_config.accounts.remove(deactivate_feature_pk) { + Some(_) => debug!("Feature for {:?} deactivated", deactivate_feature_pk), + None => warn!( + "Feature {:?} set for deactivation not found in genesis_config account list, ignored.", + deactivate_feature_pk + ), + } + } else { + warn!( + "Feature {:?} set for deactivation is not a known Feature public key", + deactivate_feature_pk + ); + } + } + let target_tick_duration = Duration::from_micros(100); genesis_config.poh_config = PohConfig::new_sleep(target_tick_duration); debug!("Payer address: {}", mint_keypair.pubkey()); diff --git a/programs/ed25519-tests/Cargo.toml b/programs/ed25519-tests/Cargo.toml new file mode 100644 index 00000000000000..7cc7a830080085 --- /dev/null +++ b/programs/ed25519-tests/Cargo.toml @@ -0,0 +1,19 @@ +[package] +name = "solana-ed25519-program-tests" +version = "1.9.7" +authors = ["Solana Maintainers "] +repository = "https://github.com/solana-labs/solana" +license = "Apache-2.0" +homepage = "https://solana.com/" +edition = "2021" +publish = false + +[dev-dependencies] +assert_matches = "1.5.0" +ed25519-dalek = "=1.0.1" +rand = "0.7.0" +solana-program-test = { path = "../../program-test", version = "=1.9.7" } +solana-sdk = { path = "../../sdk", version = "=1.9.7" } + +[package.metadata.docs.rs] +targets = ["x86_64-unknown-linux-gnu"] diff --git a/programs/ed25519-tests/tests/process_transaction.rs b/programs/ed25519-tests/tests/process_transaction.rs new file mode 100644 index 00000000000000..503e2aae642853 --- /dev/null +++ b/programs/ed25519-tests/tests/process_transaction.rs @@ -0,0 +1,87 @@ +use { + assert_matches::assert_matches, + rand::thread_rng, + solana_program_test::*, + solana_sdk::{ + ed25519_instruction::new_ed25519_instruction, + feature_set, + signature::Signer, + transaction::{Transaction, TransactionError}, + transport::TransportError, + }, +}; + +#[tokio::test] +async fn test_success() { + let mut context = ProgramTest::default().start_with_context().await; + + let client = &mut context.banks_client; + let payer = &context.payer; + let recent_blockhash = context.last_blockhash; + + let privkey = ed25519_dalek::Keypair::generate(&mut thread_rng()); + let message_arr = b"hello"; + let instruction = new_ed25519_instruction(&privkey, message_arr); + + let transaction = Transaction::new_signed_with_payer( + &[instruction], + Some(&payer.pubkey()), + &[payer], + recent_blockhash, + ); + + assert_matches!(client.process_transaction(transaction).await, Ok(())); +} + +#[tokio::test] +async fn test_failure() { + let mut context = ProgramTest::default().start_with_context().await; + + let client = &mut context.banks_client; + let payer = &context.payer; + let recent_blockhash = context.last_blockhash; + + let privkey = ed25519_dalek::Keypair::generate(&mut thread_rng()); + let message_arr = b"hello"; + let mut instruction = new_ed25519_instruction(&privkey, message_arr); + + instruction.data[0] += 1; + + let transaction = Transaction::new_signed_with_payer( + &[instruction], + Some(&payer.pubkey()), + &[payer], + recent_blockhash, + ); + + assert_matches!( + client.process_transaction(transaction).await, + Err(TransportError::TransactionError( + TransactionError::InvalidAccountIndex + )) + ); +} + +#[tokio::test] +async fn test_success_call_builtin_program() { + let mut program_test = ProgramTest::default(); + program_test.deactivate_feature(feature_set::prevent_calling_precompiles_as_programs::id()); + let mut context = program_test.start_with_context().await; + + let client = &mut context.banks_client; + let payer = &context.payer; + let recent_blockhash = context.last_blockhash; + + let privkey = ed25519_dalek::Keypair::generate(&mut thread_rng()); + let message_arr = b"hello"; + let instruction = new_ed25519_instruction(&privkey, message_arr); + + let transaction = Transaction::new_signed_with_payer( + &[instruction], + Some(&payer.pubkey()), + &[payer], + recent_blockhash, + ); + + assert_matches!(client.process_transaction(transaction).await, Ok(())); +} diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index f92139ecf3dbbd..5f5d0b159533d1 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -7101,7 +7101,7 @@ pub(crate) mod tests { cluster_type: ClusterType::MainnetBeta, ..GenesisConfig::default() })); - let sysvar_and_builtin_program_delta0 = 11; + let sysvar_and_builtin_program_delta0 = 12; assert_eq!( bank0.capitalization(), 42 * 42 + sysvar_and_builtin_program_delta0 @@ -8856,7 +8856,7 @@ pub(crate) mod tests { // not being eagerly-collected for exact rewards calculation bank0.restore_old_behavior_for_fragile_tests(); - let sysvar_and_builtin_program_delta0 = 11; + let sysvar_and_builtin_program_delta0 = 12; assert_eq!( bank0.capitalization(), 42 * 1_000_000_000 + sysvar_and_builtin_program_delta0 @@ -8991,7 +8991,7 @@ pub(crate) mod tests { // not being eagerly-collected for exact rewards calculation bank.restore_old_behavior_for_fragile_tests(); - let sysvar_and_builtin_program_delta = 11; + let sysvar_and_builtin_program_delta = 12; assert_eq!( bank.capitalization(), 42 * 1_000_000_000 + sysvar_and_builtin_program_delta @@ -13041,25 +13041,25 @@ pub(crate) mod tests { if bank.slot == 0 { assert_eq!( bank.hash().to_string(), - "DqaWg7EVKzb5Fpe92zNBtXAWqLwcedgHDicYrCBnf3QK" + "CMCWTWsU67zjmayMhSMGBTzHbW1WMCtkM5m7xk9qSnY5" ); } if bank.slot == 32 { assert_eq!( bank.hash().to_string(), - "AYdhzhKrM74r9XuZBDGcHeFzg2DEtp1boggnEnzDjZSq" + "4kbXeShX8vMnRuuADCkxSEir1oc2PrBNbx6vPkWcDtJU" ); } if bank.slot == 64 { assert_eq!( bank.hash().to_string(), - "EsbPVYzo1qz5reEUH5okKW4ExB6WbcidkVdW5mzpFn7C" + "CSZ8QCDF8qhqKDxafPzjNJpHcRAXmQzAb8eUi1Emt35E" ); } if bank.slot == 128 { assert_eq!( bank.hash().to_string(), - "H3DWrQ6FqbLkFNDxbWQ62UKRbw2dbuxf3oVF2VpBk6Ga" + "Ewh1SYKy8eiSE77sEvjav33SznfWYSwa5TwqbiYWseG2" ); break; } @@ -13287,7 +13287,7 @@ pub(crate) mod tests { // No more slots should be shrunk assert_eq!(bank2.shrink_candidate_slots(), 0); // alive_counts represents the count of alive accounts in the three slots 0,1,2 - assert_eq!(alive_counts, vec![10, 1, 7]); + assert_eq!(alive_counts, vec![11, 1, 7]); } #[test] @@ -13335,7 +13335,7 @@ pub(crate) mod tests { .map(|_| bank.process_stale_slot_with_budget(0, force_to_return_alive_account)) .sum(); // consumed_budgets represents the count of alive accounts in the three slots 0,1,2 - assert_eq!(consumed_budgets, 11); + assert_eq!(consumed_budgets, 12); } #[test] diff --git a/runtime/src/builtins.rs b/runtime/src/builtins.rs index 84f125d976f325..c4b6ae6bf047bb 100644 --- a/runtime/src/builtins.rs +++ b/runtime/src/builtins.rs @@ -129,10 +129,15 @@ fn genesis_builtins() -> Vec { solana_sdk::secp256k1_program::id(), dummy_process_instruction, ), + Builtin::new( + "ed25519_program", + solana_sdk::ed25519_program::id(), + dummy_process_instruction, + ), ] } -/// place holder for secp256k1, remove when the precompile program is deactivated via feature activation +/// place holder for precompile programs, remove when the precompile program is deactivated via feature activation fn dummy_process_instruction( _first_instruction_account: usize, _data: &[u8], @@ -172,6 +177,18 @@ fn feature_builtins() -> Vec<(Builtin, Pubkey, ActivationType)> { feature_set::prevent_calling_precompiles_as_programs::id(), ActivationType::RemoveProgram, ), + // TODO when feature `prevent_calling_precompiles_as_programs` is + // cleaned up also remove "ed25519_program" from the main builtins + // list + ( + Builtin::new( + "ed25519_program", + solana_sdk::ed25519_program::id(), + dummy_process_instruction, + ), + feature_set::prevent_calling_precompiles_as_programs::id(), + ActivationType::RemoveProgram, + ), ( Builtin::new( "address_lookup_table_program", diff --git a/runtime/src/non_circulating_supply.rs b/runtime/src/non_circulating_supply.rs index d00a4974957cff..187f478ebfc1d6 100644 --- a/runtime/src/non_circulating_supply.rs +++ b/runtime/src/non_circulating_supply.rs @@ -278,7 +278,7 @@ mod tests { ..GenesisConfig::default() }; let mut bank = Arc::new(Bank::new_for_tests(&genesis_config)); - let sysvar_and_native_program_delta = 11; + let sysvar_and_native_program_delta = 12; assert_eq!( bank.capitalization(), (num_genesis_accounts + num_non_circulating_accounts + num_stake_accounts) * balance diff --git a/sdk/src/precompiles.rs b/sdk/src/precompiles.rs index 5262c292318aa7..89b40d7de56d67 100644 --- a/sdk/src/precompiles.rs +++ b/sdk/src/precompiles.rs @@ -5,9 +5,7 @@ use { crate::{ decode_error::DecodeError, - feature_set::{ - ed25519_program_enabled, prevent_calling_precompiles_as_programs, FeatureSet, - }, + feature_set::{prevent_calling_precompiles_as_programs, FeatureSet}, instruction::CompiledInstruction, pubkey::Pubkey, }, @@ -88,7 +86,7 @@ lazy_static! { ), Precompile::new( crate::ed25519_program::id(), - Some(ed25519_program_enabled::id()), + Some(prevent_calling_precompiles_as_programs::id()), crate::ed25519_instruction::verify, ), ];