sp-io: report runtime panic messages to the host by default#12684
Open
hitchho wants to merge 2 commits into
Open
sp-io: report runtime panic messages to the host by default#12684hitchho wants to merge 2 commits into
hitchho wants to merge 2 commits into
Conversation
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.
Description
Makes
improved_panic_error_reportingthe default behavior ofsp-io's runtime panic handler: panics (and OOM aborts) now marshal their message to the host via thePanicHandler::abort_on_panichost function instead of logging and executingunreachable. Callers getRuntime panicked: <message>(sc_executor_common::Error::AbortedDueToPanic) instead of the opaque:The old behavior remains available behind a new opt-out feature,
legacy_panic_error_reporting, for runtimes that must target a host without thePanicHandlerhost functions.improved_panic_error_reportingis kept as a deprecated no-op so existingCargo.tomls don't break.Motivation
The
abort_on_panicmechanism has existed for years, yet outsidesc-executor's own test runtime it appears to have virtually no production adoption — chains ship with the opaque-trap default and users pay for it at debugging time. Concrete example from galacticcouncil/hydration-node#1491: a SCALE decode failure of a runtime-API parameter (Bad input data provided to dry_run_call: Codec error…) surfaced over public RPC as a bare wasm trap, was misdiagnosed as a runtime execution bug, and took a chopsticks fork to root-cause — the actual panic message only ever reached the node's local logs. With this change the message reaches the RPC caller directly.Compatibility
sc-executorhas captured the panic message (HostState::panic_message→AbortedDueToPanic) since 2022.ext_panic_handler_abort_on_panic_version_1.PanicHandler, but it instantiates withallow_missing_func_imports: true, so PVFs built with the new default still prepare and execute; a panic during validation traps just as it does today (outcome-identical: candidate invalid).legacy_panic_error_reporting.Review notes
substrate/client/executor/runtime-testalready enablesimproved_panic_error_reporting, so executor tests exercise the new-default path today; trap-asserting tests (call_not_existing_function,unreachable_intrinsic, host-allocator failure) cover genuine traps and are unaffected.