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 813725dfec)

* fix tests

Co-authored-by: Justin Starry <justin@solana.com>
Co-authored-by: Jack May <jack@solana.com>

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"

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",

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<u64>,
     prefer_bpf: bool,
     use_bpf_jit: bool,
+    deactivate_feature_set: HashSet<Pubkey>,
 }
 
 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());

programs/ed25519-tests/Cargo.toml

@@ -0,0 +1,19 @@
+[package]
+name = "solana-ed25519-program-tests"
+version = "1.9.7"
+authors = ["Solana Maintainers <maintainers@solana.foundation>"]
+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"]

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(()));
+}

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]

runtime/src/builtins.rs

@@ -129,10 +129,15 @@ fn genesis_builtins() -> Vec<Builtin> {
             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",

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

sdk/src/precompiles.rs

@@ -5,9 +5,7 @@
 use {
     crate::{
         decode_error::DecodeError,
-        feature_set::{
+        feature_set::{prevent_calling_precompiles_as_programs, FeatureSet},
-            ed25519_program_enabled, 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,
         ),
     ];