Joystream / substrate-node-joystream

Joystream Full Node
https://www.joystream.org
GNU General Public License v3.0
15 stars 16 forks source link

Substrate panic from the piece of code that is covered with tests and has no errors #78

Closed siman closed 5 years ago

siman commented 5 years ago

We observe a strange panic error in Substrate from the code that verifies the length of input of type Vec<u8>. The validation code is relatively simple.

The code that makes Substrate to panic:

The actual code is here: https://github.com/Joystream/substrate-forum-module/blob/development/src/lib.rs#L671

Below is a part of it that, I believe, is related to this panic error:

const ERROR_CATEGORY_TITLE_TOO_SHORT: &str = "Category title too short.";
const ERROR_CATEGORY_TITLE_TOO_LONG: &str = "Category title too long.";

/// Length constraint for input validation
#[cfg_attr(feature = "std", derive(Serialize, Deserialize, Debug))]
#[derive(Encode, Decode, Default, Clone, PartialEq, Eq)]
pub struct InputValidationLengthConstraint {

    /// Minimum length
    pub min : usize,

    /// Difference between minimum length and max length.
    /// While having max would have been more direct, this
    /// way makes max < min unrepresentable semantically, 
    /// which is safer.
    pub max_min_diff: usize,
}

impl InputValidationLengthConstraint {

    /// Helper for computing max
    pub fn max(&self) -> usize {
        self.min + self.max_min_diff
    }

    pub fn ensure_valid(
        &self, 
        length: usize, 
        too_short_msg: &'static str, 
        too_long_msg: &'static str
    ) -> Result<(),&'static str> {

        if length < self.min {
            Err(too_short_msg)
        }
        else if length > self.max() {
            Err(too_long_msg)
        }
        else {
            Ok(())
        }
    } 
}

decl_storage! {
    trait Store for Module<T: Trait> as Forum {
        pub CategoryTitleConstraint get(category_title_constraint) config(): InputValidationLengthConstraint;
    }
}

impl<T: Trait> Module<T> {

    fn ensure_category_title_is_valid(title: &Vec<u8>) -> dispatch::Result {
        <CategoryTitleConstraint<T>>::get().ensure_valid(
            title.len(),
            ERROR_CATEGORY_TITLE_TOO_SHORT,
            ERROR_CATEGORY_TITLE_TOO_LONG
        )
    }
}

The log with Substrate panic:

2019-06-14 10:20:30 Pre-sealed block for proposal at 314. Hash now 0xc150f6008b89ddad85a1db7f2976d53e91589cc737ade8d2057df2d8b0c5ed89, previously 0x351a1639359a4abd3a48212b123983e058a19c8a281bd25e8c2176c05436e350.
Hash: given=b09cdf4b661fb442f526f1607a8eb10427d755bbd4af0130533de12cba5af141, expected=91cdf8fb9afc1ef94623dfe0fa413b700893f320a2fb0ddbf2be04cda8808549

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

stack backtrace:
   0: substrate_panic_handler::set::{{closure}}::h41c84304a3d7b8c9 (0x10a7674a0)
   1: std::panicking::rust_panic_with_hook::hfcf2d0777bc6c409 (0x10a8c6ad0)
   2: std::panicking::begin_panic::hf60a109f4abac58e (0x10a469be4)
   3: srml_executive::Executive<System,Block,Context,Payment,AllModules>::execute_block::h3266066fafbe65cf (0x10a43156c)
   4: joystream_node_runtime::api::dispatch::h3c3d553b2355e9f5 (0x10a4737bc)
   5: std::panicking::try::do_call::h1b2f79e67e3c623b (0x10980cc03)
   6: ___rust_maybe_catch_panic (0x10a8cf76e)
   7: substrate_executor::native_executor::safe_call::hcd6fee49db7b8320 (0x1095e7d2a)
   8: std::thread::local::LocalKey<T>::with::h6b79b91fc97f5fef (0x1097865a7)
   9: <joystream_node::service::Executor as substrate_executor::native_executor::NativeExecutionDispatch>::dispatch::he84475f9290c126f (0x109639211)
  10: std::thread::local::LocalKey<T>::with::h607c87b9cef7afef (0x109784a0f)
  11: substrate_state_machine::StateMachine<H,B,T,O,Exec>::execute_aux::hb983d86d3e4cf34f (0x1096e8f52)
  12: substrate_state_machine::StateMachine<H,B,T,O,Exec>::execute_call_with_native_else_wasm_strategy::h50742d1fcc6e8c63 (0x109722a53)
  13: substrate_state_machine::StateMachine<H,B,T,O,Exec>::execute_using_consensus_failure_handler::hbb369b98300f778a (0x10971515f)
  14: <substrate_client::call_executor::LocalCallExecutor<B,E> as substrate_client::call_executor::CallExecutor<Block,substrate_primitives::hasher::blake2::Blake2Hasher>>::call_at_state::hba5212f0d6091f23 (0x10997900f)
  15: substrate_client::client::Client<B,E,Block,RA>::lock_import_and_run::{{closure}}::hbbb34053d3d9ece0 (0x109673695)
  16: <substrate_client::client::Client<B,E,Block,RA> as substrate_consensus_common::block_import::BlockImport<Block>>::import_block::h9b2683a6dbe59f15 (0x10966271e)
  17: <substrate_finality_grandpa::import::GrandpaBlockImport<B,E,Block,RA,PRA> as substrate_consensus_common::block_import::BlockImport<Block>>::import_block::h5d7b0aec72d6a710 (0x1099726bc)
  18: <futures::future::map::Map<A,F> as futures::future::Future>::poll::hb544965c3d4e73bb (0x10992111d)
  19: <futures::future::map_err::MapErr<A,F> as futures::future::Future>::poll::h994c67b75daca32c (0x109bb3fe1)
  20: <futures::future::either::Either<A,B> as futures::future::Future>::poll::h41b4f6cde19593ca (0x109bada0f)
  21: futures::future::catch_unwind::<impl futures::future::Future for std::panic::AssertUnwindSafe<F>>::poll::h400a3e45970d4982 (0x1099172b5)
  22: std::panicking::try::do_call::hdd4da0c5faa4f5b5 (0x10980dd2a)
  23: ___rust_maybe_catch_panic (0x10a8cf76e)
  24: futures::future::chain::Chain<A,B,C>::poll::he7db237fdc710540 (0x109ba24c2)
  25: <futures::future::select::Select<A,B> as futures::future::Future>::poll::h1ecba6dc6b95fd72 (0x109b3bbb1)
  26: futures::future::chain::Chain<A,B,C>::poll::h72b4d9a785f315c0 (0x109b9e6d8)
  27: futures::task_impl::std::set::hbccee342ee202223 (0x10a724d7c)
  28: futures::task_impl::Spawn<T>::poll_future_notify::h947d1bef75aae0e0 (0x10a725b8e)
  29: std::panicking::try::do_call::h63a10a2fad527d28 (0x10a728f93)
  30: ___rust_maybe_catch_panic (0x10a8cf76e)
  31: tokio_threadpool::task::Task::run::h82cabb444366390f (0x10a729459)
  32: tokio_threadpool::worker::Worker::run_task::h243009daf7a2b80b (0x10a721591)
  33: tokio_threadpool::worker::Worker::run::h9b2a71f9de4a4a68 (0x10a720cb9)
  34: tokio_trace_core::dispatcher::with_default::h884a9a28845fbe5b (0x10a708b94)
  35: std::thread::local::LocalKey<T>::with::h163f3f7947aac8d4 (0x10a709894)
  36: std::thread::local::LocalKey<T>::with::h2a508ecc781564d4 (0x10a709c4c)
  37: std::thread::local::LocalKey<T>::with::h1f89b3fb71a2e7fc (0x10a709a80)
  38: tokio::runtime::threadpool::builder::Builder::build::{{closure}}::h5ac92315dc79fe17 (0x10a70af78)
  39: std::thread::local::LocalKey<T>::with::ha153a624c608db97 (0x10a729ec8)
  40: std::thread::local::LocalKey<T>::with::h4150021d23e50fa1 (0x10a7298d9)
  41: std::sys_common::backtrace::__rust_begin_short_backtrace::hef1d75f4b7d9ac98 (0x10a728ca1)
  42: std::panicking::try::do_call::hb2994a6e9acac638 (0x10a728fff)
  43: ___rust_maybe_catch_panic (0x10a8cf76e)
  44: core::ops::function::FnOnce::call_once{{vtable.shim}}::he91286be7c2571b8 (0x10a722d66)
  45: <alloc::boxed::Box<F> as core::ops::function::FnOnce<A>>::call_once::hf8766009c029bc09 (0x10a8b336d)
  46: std::sys::unix::thread::Thread::new::thread_start::hfdef4649a42cf26c (0x10a8cecdd)
  47: __pthread_body (0x7fff5800e2ea)
  48: __pthread_start (0x7fff58011248)

Thread 'tokio-runtime-worker-1' panicked at 'Storage root must match that calculated.', /Users/siman/.cargo/git/checkouts/substrate-21e9e9267949a89d/6dfc3e8/srml/executive/src/lib.rs:243

This is a bug. Please report it at:

    https://www.joystream.org/

Category title too long.
jacogr commented 5 years ago

You are using usize here. In native it is u64, in WASM it is u32. So there is native/WASM mismatch. Be explicit, converting to either u64 or u32 depending on what you want.

(The polkadot-js/api will not actually support constructing usize going forward, so you can still use it in back-end code, but the API will not allow construction - specifically due to the fact it causes mismatches)

siman commented 5 years ago

Good to know this pitfall! Need to check it by updating to u32 or u16.

bedeho commented 5 years ago

Resolved in https://github.com/Joystream/substrate-forum-module/pull/22