Refactor - Cleanup error handling in program runtime

Cargo.toml

@@ -289,7 +289,7 @@ signal-hook = "0.3.14"
 smpl_jwt = "0.7.1"
 socket2 = "0.4.7"
 soketto = "0.7"
-solana_rbpf = "=0.2.40"
+solana_rbpf = "=0.3.0"
 solana-account-decoder = { path = "account-decoder", version = "=1.16.0" }
 solana-address-lookup-table-program = { path = "programs/address-lookup-table", version = "=1.16.0" }
 solana-banks-client = { path = "banks-client", version = "=1.16.0" }

ledger/src/blockstore_processor.rs

@@ -2975,7 +2975,7 @@ pub mod tests {
 
         fn mock_processor_ok(
             invoke_context: &mut InvokeContext,
-        ) -> std::result::Result<(), InstructionError> {
+        ) -> std::result::Result<(), Box<dyn std::error::Error>> {
             invoke_context.consume_checked(1)?;
             Ok(())
         }
@@ -3006,7 +3006,7 @@ pub mod tests {
 
         fn mock_processor_err(
             invoke_context: &mut InvokeContext,
-        ) -> std::result::Result<(), InstructionError> {
+        ) -> std::result::Result<(), Box<dyn std::error::Error>> {
             let instruction_errors = get_instruction_errors();
 
             invoke_context.consume_checked(1)?;
@@ -3017,10 +3017,12 @@ pub mod tests {
                 .get_instruction_data()
                 .first()
                 .expect("Failed to get instruction data");
-            Err(instruction_errors
-                .get(*err as usize)
-                .expect("Invalid error index")
-                .clone())
+            Err(Box::new(
+                instruction_errors
+                    .get(*err as usize)
+                    .expect("Invalid error index")
+                    .clone(),
+            ))
         }
 
         let mut bankhash_err = None;

program-runtime/src/invoke_context.rs

@@ -38,7 +38,27 @@ use {
     },
 };
 
-pub type ProcessInstructionWithContext = fn(&mut InvokeContext) -> Result<(), InstructionError>;
+/// Adapter so we can unify the interfaces of built-in programs and syscalls
+#[macro_export]
+macro_rules! declare_process_instruction {
+    ($cu_to_consume:expr) => {
+        pub fn process_instruction(
+            invoke_context: &mut InvokeContext,
+        ) -> Result<(), Box<dyn std::error::Error>> {
+            if invoke_context
+                .feature_set
+                .is_active(&feature_set::native_programs_consume_cu::id())
+            {
+                invoke_context.consume_checked($cu_to_consume)?;
+            }
+            process_instruction_inner(invoke_context)
+                .map_err(|err| Box::new(err) as Box<dyn std::error::Error>)
+        }
+    };
+}
+
+pub type ProcessInstructionWithContext =
+    fn(&mut InvokeContext) -> Result<(), Box<dyn std::error::Error>>;
 
 #[derive(Clone)]
 pub struct BuiltinProgram {
@@ -50,7 +70,7 @@ impl std::fmt::Debug for BuiltinProgram {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
         // These are just type aliases for work around of Debug-ing above pointers
         type ErasedProcessInstructionWithContext =
-            fn(&'static mut InvokeContext<'static>) -> Result<(), InstructionError>;
+            fn(&'static mut InvokeContext<'static>) -> Result<(), Box<dyn std::error::Error>>;
 
         // rustc doesn't compile due to bug without this work around
         // https://github.com/rust-lang/rust/issues/50280
@@ -689,26 +709,25 @@ impl<'a> InvokeContext<'a> {
                 self.transaction_context
                     .set_return_data(program_id, Vec::new())?;
 
-                let is_builtin_program = builtin_id == program_id;
                 let pre_remaining_units = self.get_remaining();
-                let result = if is_builtin_program {
-                    let logger = self.get_log_collector();
-                    stable_log::program_invoke(&logger, &program_id, self.get_stack_height());
-                    (entry.process_instruction)(self)
-                        .map(|()| {
-                            stable_log::program_success(&logger, &program_id);
-                        })
-                        .map_err(|err| {
-                            stable_log::program_failure(&logger, &program_id, &err);
-                            err
-                        })
-                } else {
-                    (entry.process_instruction)(self)
-                };
+                let logger = self.get_log_collector();
+                stable_log::program_invoke(&logger, &program_id, self.get_stack_height());
+                let result = (entry.process_instruction)(self)
+                    .map(|()| {
+                        stable_log::program_success(&logger, &program_id);
+                    })
+                    .map_err(|err| {
+                        stable_log::program_failure(&logger, &program_id, err.as_ref());
+                        if let Some(err) = err.downcast_ref::<InstructionError>() {
+                            err.clone()
+                        } else {
+                            InstructionError::ProgramFailedToComplete
+                        }
+                    });
                 let post_remaining_units = self.get_remaining();
                 *compute_units_consumed = pre_remaining_units.saturating_sub(post_remaining_units);
 
-                if is_builtin_program
+                if builtin_id == program_id
                     && result.is_ok()
                     && *compute_units_consumed == 0
                     && self
@@ -739,13 +758,13 @@ impl<'a> InvokeContext<'a> {
     }
 
     /// Consume compute units
-    pub fn consume_checked(&self, amount: u64) -> Result<(), InstructionError> {
+    pub fn consume_checked(&self, amount: u64) -> Result<(), Box<dyn std::error::Error>> {
         self.log_consumed_bpf_units(amount);
         let mut compute_meter = self.compute_meter.borrow_mut();
         let exceeded = *compute_meter < amount;
         *compute_meter = compute_meter.saturating_sub(amount);
         if exceeded {
-            return Err(InstructionError::ComputationalBudgetExceeded);
+            return Err(Box::new(InstructionError::ComputationalBudgetExceeded));
         }
         Ok(())
     }
@@ -986,14 +1005,14 @@ mod tests {
 
     #[test]
     fn test_program_entry_debug() {
-        #[allow(clippy::unnecessary_wraps)]
         fn mock_process_instruction(
             _invoke_context: &mut InvokeContext,
-        ) -> Result<(), InstructionError> {
+        ) -> Result<(), Box<dyn std::error::Error>> {
             Ok(())
         }
-        #[allow(clippy::unnecessary_wraps)]
-        fn mock_ix_processor(_invoke_context: &mut InvokeContext) -> Result<(), InstructionError> {
+        fn mock_ix_processor(
+            _invoke_context: &mut InvokeContext,
+        ) -> Result<(), Box<dyn std::error::Error>> {
             Ok(())
         }
         let builtin_programs = &[
@@ -1014,7 +1033,7 @@ mod tests {
     #[allow(clippy::integer_arithmetic)]
     fn mock_process_instruction(
         invoke_context: &mut InvokeContext,
-    ) -> Result<(), InstructionError> {
+    ) -> Result<(), Box<dyn std::error::Error>> {
         let transaction_context = &invoke_context.transaction_context;
         let instruction_context = transaction_context.get_current_instruction_context()?;
         let instruction_data = instruction_context.get_instruction_data();
@@ -1048,7 +1067,7 @@ mod tests {
         if let Ok(instruction) = bincode::deserialize(instruction_data) {
             match instruction {
                 MockInstruction::NoopSuccess => (),
-                MockInstruction::NoopFail => return Err(InstructionError::GenericError),
+                MockInstruction::NoopFail => return Err(Box::new(InstructionError::GenericError)),
                 MockInstruction::ModifyOwned => instruction_context
                     .try_borrow_instruction_account(transaction_context, 0)?
                     .set_data_from_slice(&[1])?,
@@ -1098,14 +1117,15 @@ mod tests {
                     desired_result,
                 } => {
                     invoke_context.consume_checked(compute_units_to_consume)?;
-                    return desired_result;
+                    return desired_result
+                        .map_err(|err| Box::new(err) as Box<dyn std::error::Error>);
                 }
                 MockInstruction::Resize { new_len } => instruction_context
                     .try_borrow_instruction_account(transaction_context, 0)?
                     .set_data(vec![0; new_len as usize])?,
             }
         } else {
-            return Err(InstructionError::InvalidInstructionData);
+            return Err(Box::new(InstructionError::InvalidInstructionData));
         }
         Ok(())
     }

program-runtime/src/loaded_programs.rs

@@ -137,7 +137,7 @@ impl LoadedProgram {
         account_size: usize,
         use_jit: bool,
         metrics: &mut LoadProgramMetrics,
-    ) -> Result<Self, EbpfError> {
+    ) -> Result<Self, Box<dyn std::error::Error>> {
         let mut load_elf_time = Measure::start("load_elf_time");
         let executable = Executable::load(elf_bytes, loader.clone())?;
         load_elf_time.stop();

program-runtime/src/stable_log.rs

@@ -5,7 +5,7 @@
 use {
     crate::{ic_logger_msg, log_collector::LogCollector},
     itertools::Itertools,
-    solana_sdk::{instruction::InstructionError, pubkey::Pubkey},
+    solana_sdk::pubkey::Pubkey,
     std::{cell::RefCell, rc::Rc},
 };
 
@@ -103,7 +103,7 @@ pub fn program_success(log_collector: &Option<Rc<RefCell<LogCollector>>>, progra
 pub fn program_failure(
     log_collector: &Option<Rc<RefCell<LogCollector>>>,
     program_id: &Pubkey,
-    err: &InstructionError,
+    err: &dyn std::error::Error,
 ) {
     ic_logger_msg!(log_collector, "Program {} failed: {}", program_id, err);
 }

program-test/src/lib.rs

@@ -100,7 +100,7 @@ fn get_invoke_context<'a, 'b>() -> &'a mut InvokeContext<'b> {
 pub fn builtin_process_instruction(
     process_instruction: solana_sdk::entrypoint::ProcessInstruction,
     invoke_context: &mut InvokeContext,
-) -> Result<(), InstructionError> {
+) -> Result<(), Box<dyn std::error::Error>> {
     set_invoke_context(invoke_context);
 
     let transaction_context = &invoke_context.transaction_context;
@@ -134,8 +134,8 @@ pub fn builtin_process_instruction(
 
     // Execute the program
     process_instruction(program_id, &account_infos, instruction_data).map_err(|err| {
-        let err = u64::from(err);
-        stable_log::program_failure(&log_collector, program_id, &err.into());
+        let err: Box<dyn std::error::Error> = Box::new(InstructionError::from(u64::from(err)));
+        stable_log::program_failure(&log_collector, program_id, err.as_ref());
         err
     })?;
     stable_log::program_success(&log_collector, program_id);

programs/address-lookup-table/src/processor.rs

@@ -6,7 +6,7 @@ use {
             LOOKUP_TABLE_MAX_ADDRESSES, LOOKUP_TABLE_META_SIZE,
         },
     },
-    solana_program_runtime::{ic_msg, invoke_context::InvokeContext},
+    solana_program_runtime::{declare_process_instruction, ic_msg, invoke_context::InvokeContext},
     solana_sdk::{
         clock::Slot,
         feature_set,
@@ -18,14 +18,10 @@ use {
     std::convert::TryFrom,
 };
 
-pub fn process_instruction(invoke_context: &mut InvokeContext) -> Result<(), InstructionError> {
-    // Consume compute units if feature `native_programs_consume_cu` is activated,
-    if invoke_context
-        .feature_set
-        .is_active(&feature_set::native_programs_consume_cu::id())
-    {
-        invoke_context.consume_checked(750)?;
-    }
+declare_process_instruction!(750);
+pub fn process_instruction_inner(
+    invoke_context: &mut InvokeContext,
+) -> Result<(), InstructionError> {
     let transaction_context = &invoke_context.transaction_context;
     let instruction_context = transaction_context.get_current_instruction_context()?;
     let instruction_data = instruction_context.get_instruction_data();