Add api to append data in offchain storage from extrinsic [original: StorageValueRef::mutate panic without gracefully returning error]

sudipghimire533 commented 2 years ago

Sample code:

#[pallet::weight(10_000)]
pub fn do_something(origin: OriginFor<T>) -> DispatchResult {
    let writer = sp_runtime::offchain::storage::StorageValueRef::local(b"some-key");
    let write_status = writer.mutate::<SomeType>(||{
        // some computation
       Ok(some_value)
    });

  ensure!(write_status.is_ok(), Error::<T>::MutateError);

  Ok(())
}

Given that above function is a extrinsic. This call will fail with panic

Panic message

Version: 3.0.0-monthly-2021-10-43d75ba-x86_64-linux-gnu

   0: sp_panic_handler::set::{{closure}}
   1: std::panicking::rust_panic_with_hook
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/panicking.rs:610:17
   2: std::panicking::begin_panic_handler::{{closure}}
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/panicking.rs:502:13
   3: std::sys_common::backtrace::__rust_end_short_backtrace
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/sys_common/backtrace.rs:139:18
   4: rust_begin_unwind
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/panicking.rs:498:5
   5: core::panicking::panic_fmt
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/core/src/panicking.rs:107:14
   6: core::panicking::panic_display
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/core/src/panicking.rs:63:5
   7: core::option::expect_failed
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/core/src/option.rs:1637:5
   8: std::thread::local::LocalKey<T>::with
   9: sp_io::offchain::local_storage_get_version_1
  10: sp_io::offchain::local_storage_get
  11: sp_runtime::offchain::storage::StorageValueRef::mutate
  12: <pallet_ocw::pallet::Call<T> as frame_support::traits::dispatch::UnfilteredDispatchable>::dispatch_bypass_filter
  13: <node_template_runtime::Call as frame_support::traits::dispatch::UnfilteredDispatchable>::dispatch_bypass_filter
  14: <node_template_runtime::Call as sp_runtime::traits::Dispatchable>::dispatch
  15: <sp_runtime::generic::checked_extrinsic::CheckedExtrinsic<AccountId,Call,Extra> as sp_runtime::traits::Applyable>::apply
  16: frame_executive::Executive<System,Block,Context,UnsignedValidator,AllPallets,COnRuntimeUpgrade>::apply_extrinsic
  17: <node_template_runtime::Runtime as sp_block_builder::runtime_decl_for_BlockBuilder::BlockBuilder<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32,sp_runtime::traits::BlakeTwo256>,sp_runtime::generic::unchecked_extrinsic::UncheckedExtrinsic<sp_runtime::multiaddress::MultiAddress<<<sp_runtime::MultiSignature as sp_runtime::traits::Verify>::Signer as sp_runtime::traits::IdentifyAccount>::AccountId,()>,node_template_runtime::Call,sp_runtime::MultiSignature,(frame_system::extensions::check_spec_version::CheckSpecVersion<node_template_runtime::Runtime>,frame_system::extensions::check_tx_version::CheckTxVersion<node_template_runtime::Runtime>,frame_system::extensions::check_genesis::CheckGenesis<node_template_runtime::Runtime>,frame_system::extensions::check_mortality::CheckMortality<node_template_runtime::Runtime>,frame_system::extensions::check_nonce::CheckNonce<node_template_runtime::Runtime>,frame_system::extensions::check_weight::CheckWeight<node_template_runtime::Runtime>,pallet_transaction_payment::ChargeTransactionPayment<node_template_runtime::Runtime>)>>>>::apply_extrinsic
  18: sp_block_builder::runtime_decl_for_BlockBuilder::apply_extrinsic_native_call_generator::{{closure}}
  19: std::panicking::try
  20: std::thread::local::LocalKey<T>::with
  21: sc_executor::native_executor::WasmExecutor::with_instance::{{closure}}
  22: sc_executor::wasm_runtime::RuntimeCache::with_instance
  23: <sc_executor::native_executor::NativeElseWasmExecutor<D> as sp_core::traits::CodeExecutor>::call
  24: sp_state_machine::execution::StateMachine<B,H,N,Exec>::execute_aux
  25: sp_state_machine::execution::StateMachine<B,H,N,Exec>::execute_using_consensus_failure_handler
  26: <sc_service::client::client::Client<B,E,Block,RA> as sp_api::CallApiAt<Block>>::call_api_at
  27: sp_block_builder::runtime_decl_for_BlockBuilder::apply_extrinsic_call_api_at
  28: <node_template_runtime::RuntimeApiImpl<__SR_API_BLOCK__,RuntimeApiImplCall> as sp_block_builder::BlockBuilder<__SR_API_BLOCK__>>::BlockBuilder_apply_extrinsic_runtime_api_impl
  29: <node_template_runtime::RuntimeApiImpl<Block,C> as sp_api::ApiExt<Block>>::execute_in_transaction
  30: <sc_basic_authorship::basic_authorship::Proposer<B,Block,C,A,PR> as sp_consensus::Proposer<Block>>::propose::{{closure}}
  31: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
  32: std::panicking::try
  33: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll
  34: <tracing_futures::Instrumented<T> as core::future::future::Future>::poll
  35: tokio::park::thread::CachedParkThread::block_on
  36: tokio::runtime::handle::Handle::block_on
  37: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut
  38: tokio::runtime::task::harness::Harness<T,S>::poll
  39: tokio::runtime::blocking::pool::Inner::run
  40: std::sys_common::backtrace::__rust_begin_short_backtrace
  41: core::ops::function::FnOnce::call_once{{vtable.shim}}
  42: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/alloc/src/boxed.rs:1694:9
      <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/alloc/src/boxed.rs:1694:9
      std::sys::unix::thread::Thread::new::thread_start
             at /rustc/db9d1b20bba1968c1ec1fc49616d4742c1725b4b/library/std/src/sys/unix/thread.rs:106:17
  43: start_thread
  44: clone

Thread 'tokio-runtime-worker' panicked at 'local_storage_get can be called only in the offchain call context with
                                OffchainDb extension', /<username>/.cargo/git/checkouts/substrate-7e08433d4c370a21/bf9683e/primitives/io/src/lib.rs:1039

This is a bug. Please report it at:

Expected

As far as I can say, this error arise because we cannot have read access to offchain storage ( and for mutate function we still need read access to reference to previous value on which later we would write ).

It would be wise for us to return a error type rather than doing a panic?

A novice question.

I actually do not understand why reading from offchain storage is not allowed outside offchain worker context. Is it because of possible time consumption if reading large chunk from storage?

Actual context

I am trying to keep a vector of some data in offchain storage, Source of data to this vector is extrinsic call. So it is required that we keep push a data for every call. Is there any idiomatic way to do so?

bkchr commented 2 years ago

Because accessing the local storage could easily introduce non determinism. Aka you could accidentally store this value in the state and it could be different per node leading to a different storage root.

We could probably add some api to append data.

sudipghimire533 commented 2 years ago

Thanks for explaining it. Yeah we should definitely add an api to append data. Also we may try returning an error than just having panic along the way?

bkchr commented 2 years ago

No, the panic is "valid" there. You are doing some "illegal" operation, so panicking there is correct.

sudipghimire533 commented 2 years ago

That also sounds legit though. About having an api to append, is there any blocker? or possible limitation? Maybe I could look on that or someone from parity will

xlc commented 2 years ago

You will want to use the offchain index API https://github.com/paritytech/substrate/pull/5200

sudipghimire533 commented 2 years ago

We could probably add some api to append data.

Renamed issue accordingly.

sudipghimire533 commented 2 years ago

For those who happen to land here, here is a quick fix: ( please consider side-effect )

My original intent was ( I was just getting started with substrate & all ) that I need to keep adding incoming parameters in offchain storage block wise. For this purpose, temporary fix was something like:

Add a storage with some name like _TemporaryUse
In extrinsic keep modifying/adding data to that stirage as you wish
In on_finalize hook: read all the data from _TemporaryUse and put it to offchain storage at once under key current_block_numer and then ::kill the _TemporaryUse

This however bring unwanted effect depending on how much size and how intensive calculation is required. I however adapted to different approach, might be useful for someone who is still learning.

Also don't know if having an api to append to offchain storage would be worth as not all storage types will be appendable. So might not be worth to handle such narrow case. Feel free to close this @bkchr if you think so

paritytech / substrate