Refactor - Cleanup error handling in program runtime #30693
Conversation
Lichtso
force-pushed
the
refactor/cleanup_error_handling
branch
7 times, most recently
from
1e9ee81
to
faa8fb3
Compare
Lichtso
force-pushed
the
refactor/cleanup_error_handling
branch
from
faa8fb3
to
16115c6
Compare
Codecov Report
@@ Coverage Diff @@
## master #30693 +/- ##
=========================================
- Coverage 81.5% 81.5% -0.1%
=========================================
Files 728 728
Lines 205881 205825 -56
=========================================
- Hits 167800 167751 -49
+ Misses 38081 38074 -7 |
Lichtso
force-pushed
the
refactor/cleanup_error_handling
branch
3 times, most recently
from
5454672
to
d5354d2
Compare
Lichtso
force-pushed
the
refactor/cleanup_error_handling
branch
from
d5354d2
to
58fa39d
Compare
| } else { | ||
| (entry.process_instruction)(self) | ||
| }; | ||
| let logger = self.get_log_collector(); |
There was a problem hiding this comment.
I always wondered what this extra if was for
program-runtime/src/stable_log.rs
Outdated
| pub fn program_failure( | ||
| log_collector: &Option<Rc<RefCell<LogCollector>>>, | ||
| program_id: &Pubkey, | ||
| err: &InstructionError, | ||
| err: &Box<dyn std::error::Error>, |
There was a problem hiding this comment.
This should be &dyn Error and then you &* at the call site
There was a problem hiding this comment.
Tried that already and it complains that the size of dyn std::error::Error is not known at compile time.
But I found a different solution: Use .as_ref() at call site.
There was a problem hiding this comment.
as_ref returns &T which is also what deref() returns for Box, so &* should work? And it's the common way to box -> dyn
There was a problem hiding this comment.
| @@ -37,7 +37,20 @@ use { | |||
| }, | |||
| }; | |||
|
|
|||
| pub type ProcessInstructionWithContext = fn(&mut InvokeContext) -> Result<(), InstructionError>; | |||
| #[macro_export] | |||
| macro_rules! declare_process_instruction { | |||
There was a problem hiding this comment.
Please add a comment explaining what this is for. In the context of this PR is
clear, but if you only look at the call sites they look pretty weird so people
will wonder what's going on
| } | ||
| }; | ||
| create_vm_time.stop(); | ||
|
|
||
| execute_time = Measure::start("execute"); | ||
| stable_log::program_invoke(&log_collector, &program_id, stack_height); |
There was a problem hiding this comment.
Why did you remove this log and the one when the program fails? The invoke
context will log with the loader id right? So this add a log with the actual
program id which is useful
There was a problem hiding this comment.
InvokeContext::process_executable_chain() logs program_id as well, not builtin_id.
There was a problem hiding this comment.
Ah that's what the cursed confusing if was for lol 😂
Lichtso
force-pushed
the
refactor/cleanup_error_handling
branch
6 times, most recently
from
cf2d255
to
c535c9f
Compare
Lichtso
force-pushed
the
refactor/cleanup_error_handling
branch
from
c535c9f
to
fe328d8
Compare
… stable_log::program_failure() calls from bpf_loader into InvokeContext::process_executable_chain().
…nto Box<dyn std::error::Error>.
Lichtso
force-pushed
the
refactor/cleanup_error_handling
branch
from
fe328d8
to
778c71d
Compare
Problem
We pack error types into deeply nested structures such as
EbpfError::UserError(BpfError::SyscallError(SyscallError::InstructionError(err)))andunpack and repack them multiple times when switching between the control flow of the program-runtime, the RBPF-VM and (CPI) syscalls. This is not only annoying to deal with, but also hinders the unification of built-in programs and syscalls, as that would require a common return type, which requires a common result type, which in turn requires a common error type.
Summary of Changes
BpfError::VerifierError,BpfError::SyscallError,rbpf::EbpfError::UserError,SyscallError::InstructionErrorand turns them into
Box<dyn std::error::Error>instead.ProcessInstructionWithContextfromInstructionErrorintoBox<dyn std::error::Error>.