Failing to decode the SCALE encoded input in the state_call RPC causes a runtime panic

[jasl]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Description of bug

When trying upload an Ink contract via https://contracts-ui.substrate.io/add-contract

The node's runtime will panic.

====================

Version: 3.0.0-dev-49734dd1d72

   0: backtrace::capture::Backtrace::new
   1: sp_panic_handler::set::{{closure}}
   2: std::panicking::rust_panic_with_hook
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/std/src/panicking.rs:702:17
   3: std::panicking::begin_panic_handler::{{closure}}
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/std/src/panicking.rs:588:13
   4: std::sys_common::backtrace::__rust_end_short_backtrace
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/std/src/sys_common/backtrace.rs:138:18
   5: rust_begin_unwind
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/std/src/panicking.rs:584:5
   6: core::panicking::panic_fmt
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/core/src/panicking.rs:142:14
   7: kitchensink_runtime::api::dispatch
   8: std::thread::local::LocalKey<T>::with
   9: sc_executor::native_executor::WasmExecutor<H>::with_instance::{{closure}}
  10: sc_executor::wasm_runtime::RuntimeCache::with_instance
  11: <sc_executor::native_executor::NativeElseWasmExecutor<D> as sp_core::traits::CodeExecutor>::call
  12: sp_state_machine::execution::StateMachine<B,H,Exec>::execute_aux
  13: sp_state_machine::execution::StateMachine<B,H,Exec>::execute_using_consensus_failure_handler
  14: <sc_service::client::call_executor::LocalCallExecutor<Block,B,E> as sc_client_api::call_executor::CallExecutor<Block>>::call
  15: <sc_rpc::state::state_full::FullState<BE,Block,Client> as sc_rpc::state::StateBackend<Block,Client>>::call
  16: <tokio::runtime::blocking::task::BlockingTask<T> as core::future::future::Future>::poll
  17: std::panicking::try
  18: tokio::runtime::task::harness::Harness<T,S>::poll
  19: tokio::runtime::blocking::pool::Inner::run
  20: std::sys_common::backtrace::__rust_begin_short_backtrace
  21: core::ops::function::FnOnce::call_once{{vtable.shim}}
  22: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/alloc/src/boxed.rs:1934:9
      <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/alloc/src/boxed.rs:1934:9
      std::sys::unix::thread::Thread::new::thread_start
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/std/src/sys/unix/thread.rs:108:17
  23: __pthread_deallocate


Thread 'tokio-runtime-worker' panicked at 'Bad input data provided to instantiate: unexpected first byte decoding Option', bin/node/runtime/src/lib.rs:1793

This is a bug. Please report it at:

	https://github.com/paritytech/substrate/issues/new


====================

Version: 3.0.0-dev-49734dd1d72

   0: backtrace::capture::Backtrace::new
   1: sp_panic_handler::set::{{closure}}
   2: std::panicking::rust_panic_with_hook
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/std/src/panicking.rs:702:17
   3: std::panicking::begin_panic_handler::{{closure}}
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/std/src/panicking.rs:588:13
   4: std::sys_common::backtrace::__rust_end_short_backtrace
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/std/src/sys_common/backtrace.rs:138:18
   5: rust_begin_unwind
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/std/src/panicking.rs:584:5
   6: core::panicking::panic_fmt
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/core/src/panicking.rs:142:14
   7: kitchensink_runtime::api::dispatch
   8: std::thread::local::LocalKey<T>::with
   9: sc_executor::native_executor::WasmExecutor<H>::with_instance::{{closure}}
  10: sc_executor::wasm_runtime::RuntimeCache::with_instance
  11: <sc_executor::native_executor::NativeElseWasmExecutor<D> as sp_core::traits::CodeExecutor>::call
  12: sp_state_machine::execution::StateMachine<B,H,Exec>::execute_aux
  13: sp_state_machine::execution::StateMachine<B,H,Exec>::execute_using_consensus_failure_handler
  14: <sc_service::client::call_executor::LocalCallExecutor<Block,B,E> as sc_client_api::call_executor::CallExecutor<Block>>::call
  15: <sc_rpc::state::state_full::FullState<BE,Block,Client> as sc_rpc::state::StateBackend<Block,Client>>::call
  16: <tokio::runtime::blocking::task::BlockingTask<T> as core::future::future::Future>::poll
  17: std::panicking::try
  18: tokio::runtime::task::harness::Harness<T,S>::poll
  19: tokio::runtime::blocking::pool::Inner::run
  20: std::sys_common::backtrace::__rust_begin_short_backtrace
  21: core::ops::function::FnOnce::call_once{{vtable.shim}}
  22: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/alloc/src/boxed.rs:1934:9
      <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/alloc/src/boxed.rs:1934:9
      std::sys::unix::thread::Thread::new::thread_start
             at /rustc/7b46aa594c4bdc507fbd904b6777ca30c37a9209/library/std/src/sys/unix/thread.rs:108:17
  23: __pthread_deallocate


Thread 'tokio-runtime-worker' panicked at 'Bad input data provided to instantiate: unexpected first byte decoding Option', bin/node/runtime/src/lib.rs:1793

This is a bug. Please report it at:

	https://github.com/paritytech/substrate/issues/new

2022-10-23 05:13:40 Evicting failed runtime instance error=Runtime panicked: Bad input data provided to instantiate: unexpected first byte decoding Option
2022-10-23 05:13:40 Evicting failed runtime instance error=Runtime panicked: Bad input data provided to instantiate: unexpected first byte decoding Option

Steps to reproduce

It can be reproduced since polkadot-v0.9.31 and later, may relates to this PR #12429

• cargo contract new flipper then cargo contract build for flipper.contract
• cargo build --release -p node-cli then target/release/substrate --dev
• Open https://contracts-ui.substrate.io/add-contract upload flipper.contract

[jasl]

IMO it looks like ContractsUI hasn't compatible with latest pallet-contracts Runtime API change, so it's pass bad RPC call, but this shouldn't made the Runtime crash

[athei]

I don't think behavior is specific to contracts. It is what happens when the state_call RPC can't decode the arguments. If this is desired behavior is another question.

[jasl]

I don't think behavior is specific to contracts. It is what happens when the state_call RPC can't decode the arguments. If this is desired behavior is another question.

I don't think this relates to recent contracts Runtime API change either.
The Runtime shouldn't panic when given a bad input, it should just reject it.

[athei]

The title of a issue is a bit of a clickbait, though. Cause the runtime only panics when you have direct access to a node via RPC. And only this one node's runtimes panics. Purely offchain. In this case it might be a bit ugly but not at all dangerous.

Still, I think it shouldn't be a panic but an error.

[jasl]

clickbait

Do you think it's more clear to rename the title to "Bad Runtime RPC call lead the Runtime panic"?

[athei]

I renamed in a way that I think describes the problem.

[bkchr]

Panic is completely fine. This is done this way since always and otherwise we would need to wrap return type of a runtime function into an extra result. If you don't follow the Abi, there is no other way than panicking because the input is invalid.

[jasl]

Panic is completely fine.

Does it means the node need to recreate one when the Runtime panicked? I'm concerning that could be a DoS vector of node (not the network).

[bkchr]

No. You need to reset the memory. However, we reset the memory anyway for every new call into the runtime. So, there is no difference to the normal behavior.

[jasl]

No. You need to reset the memory. However, we reset the memory anyway for every new call into the runtime. So, there is no difference to the normal behavior.

So the only issue here is this should be a error or just suppress it, or end-user will confuse this

[bkchr]

or end-user will confuse this

Who is the end user here? The node operator?

[jasl]

or end-user will confuse this

Who is the end user here? The node operator?

Yes, the node operator, including me, when I saw "This is a bug. Please report it at:" my instant thinking is collecting info and submit to here, then you explain to me this is not bug, it's design, but the node said it's a bug, so I guess others would submit this too

Another old story is #12354
3 years ago I first met this, I submit to Substrate, I remembered it's you told me that's because DB broken and close the issue, so I know what the problem is.

But our users (node operators) don't know that, 3 years I got maybe hundreds report (The DB growth faster than we thought) for that issue because the node says it's a bug, please submit to Phala team.

So I'd proposal it is worthy to suppress expected errors, not raise "Please report it"

[bkchr]

"This is a bug. Please report it at:"

Yeah that is because it panicked in the native runtime. A panic in the wasm runtime doesn't print this.

For the wasm runtime we have the following feature: https://github.com/paritytech/substrate/blob/master/primitives/io/Cargo.toml#L91

When this feature is enabled, the panics will be returned as an error of the execution and not directly printed to the log.