Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Avoid Ext calls from outside of runtime #647

Closed
wants to merge 2 commits into from

Conversation

svyatonik
Copy link
Contributor

Alternative fix of this panic:

thread '<unnamed>' panicked at 'Externalities not allowed to fail within runtime: "Trie lookup error: Invalid state root: 0x609a�~@�3f5a"', libcore/result.rs:945:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:227
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:467
   5: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:350
   6: rust_begin_unwind
             at libstd/panicking.rs:328
   7: core::panicking::panic_fmt
             at libcore/panicking.rs:71
   8: core::result::unwrap_failed
   9: <substrate_state_machine::ext::Ext<'a, B> as substrate_state_machine::Externalities>::storage
  10: substrate_state_machine::prove_execution
  11: <substrate_client::call_executor::LocalCallExecutor<B, E> as substrate_client::call_executor::CallExecutor<Block>>::prove_at_state
  12: <substrate_client::client::Client<B, E, Block> as substrate_network::chain::Client<Block>>::execution_proof
  13: <substrate_network::protocol::Protocol<B, S>>::handle_packet
  14: <substrate_network::service::ProtocolHandler<B, S> as substrate_network_libp2p::traits::NetworkProtocolHandler>::read

It should be fixed as @arkpar suggested - by checking if the state is not pruned (he's preparing PR). However, calling 'unsafe' Ext methods (storage && exists_storage) from outside of runtime should be avoided (since Ext expect-s backend calls to succeed) => this PR removes temporary Ext creation for those calls.

@svyatonik svyatonik added the A0-please_review Pull request needs code review. label Sep 3, 2018
@@ -126,8 +126,9 @@ where
H::Out: Ord + Encodable
{
fn storage(&self, key: &[u8]) -> Option<Vec<u8>> {
self.overlay.storage(key).map(|x| x.map(|x| x.to_vec())).unwrap_or_else(||
self.backend.storage(key).expect("Externalities not allowed to fail within runtime"))
use {try_read_overlay_value};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the {s needed here?

self.backend.storage(key).expect("Externalities not allowed to fail within runtime"))
use {try_read_overlay_value};
try_read_overlay_value(self.overlay, self.backend, key)
.expect("Externalities not allowed to fail within runtime")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably just do .map_err(|e| error!(target: "state-machine", "Externalities call failed! {:?}", e)).ok().and_then(|inner| inner), which then will at least avoid the panic. better would be to shuffle the Err through into the wasm interpreter so the execution can be early-exited. not sure if that's possible yet, though.

@gavofyork gavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. A8-looksgood labels Sep 3, 2018
@svyatonik
Copy link
Contributor Author

I'm not feeling good implementing the option#1 - it is just the ignoring of an error. Like if it was implemented in that way, we would never see today's panic => there would never be a #648 . Everyone ignore logs until something bad happens && in this case nothing bad will happen (except for that guy who has tried to run a light client).

Looks like option#2 is available for WasmExecutor (at least we're able to return Result from env functions). At the same time the only way (at least I know about - maybe there are other ways to bail out?) to skip frames when we're executing native code is to panic. And panic in this case returns us to original implementation of this PR.

So I would prefer to close this PR if it is a mustntgrumble

@svyatonik svyatonik closed this Sep 3, 2018
@svyatonik svyatonik deleted the fix_missing_storage_panic branch September 6, 2018 05:58
lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020
* Remove Sudo

* Bump versions

* Fixes

* Remove other mentions of sudo

* Remove sudo from Cargo
helin6 pushed a commit to boolnetwork/substrate that referenced this pull request Jul 25, 2023
* Add Staking Miner and Introspector to usage list

* Update README.md

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants