fix: move JournalInspectTr to host.rs with proper error stashing#172
Merged
fix: move JournalInspectTr to host.rs with proper error stashing#172
Conversation
Previously, custom instruction wrappers in instructions.rs manually
stashed DB errors via match + ctx.error() when calling journal inspect
methods. This was error-prone and had a bug where some paths halted with
FatalExternalError without stashing the error first, causing panics.
This refactoring:
- Defines JournalInspectTr trait in host.rs with two implementations:
- Journal<DB>: returns Result<T, DB::Error> directly
- MegaContext: stashes errors in self.error() and returns Err(())
- Simplifies all 4 instruction call sites to let Ok(...) = ... else { halt }
- Changes HostExt gas methods to return Option (stash errors internally)
- Replaces load_account with inspect_account_delegated in CREATE
- Adds ErrorInjectingDatabase test utility and 3 integration tests
flyq
approved these changes
Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
instructions.rspreviously had a bug where some paths halted withFatalExternalErrorwithout stashing the underlying DB error inctx.error(), causing panics when revm tried to drain the stashed error.JournalInspectTrtrait tohost.rswith two implementations:Journal<DB>: returnsResult<T, DB::Error>for direct error propagationMegaContext: stashes errors inself.error()asContextError::Customand returnsErr(())let Ok(...) = context.host.inspect_*(...) else { halt(FatalExternalError); return; }HostExtgas methods (sstore_set_storage_gas,new_account_storage_gas,create_contract_storage_gas) to returnOptioninstead ofResult, stashing errors internally (consistent with revm'sHosttrait pattern)load_accountwithinspect_account_delegatedinstorage_gas_ext::createto avoid marking the creator account warmErrorInjectingDatabasetest utility for simulating DB errorsinspect_storage(SSTORE) andinspect_account_delegated(CALL, STATICCALL) are properly stashed and surfaced asEVMError::CustomTest plan
cargo check -p mega-evmpassescargo check -p mega-evm --target riscv64imac-unknown-none-elf --no-default-featurespasses (no_std)cargo test -p mega-evm— all tests pass (including 3 new db_error tests)cargo clippy --workspace --lib --examples --tests --benches --all-features --locked— no warningscargo fmt --all --check— formatted