Poor error handling on invalid transaction input

[xlc]

I got this error

2019-02-01 18:16:09 Idle (0 peers), best: #8 (0xd658…0442), finalized #2 (0x7d0d…3f63), ⬇ 0 ⬆ 0
2019-02-01 18:16:11 Accepted a new tcp connection from 127.0.0.1:54393.
2019-02-01 18:16:12 Starting consensus session on top of parent 0xd6586ce0a41d9e1d2b14b6b198be0b10cc1c6382f6aeaebbc8a3db30faea0442
thread 'jsonrpc-eventloop-0' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:345:21
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
/Users/bryanchen/projects/substrate/runtime/src/lib.rs
242
1
2019-02-01 18:16:12 Prepared block for proposing at 9 [hash: 0xa96672127fd46cc50696845c2c18b7eb034e73897b00325f48effe43d8f2560a; parent_hash: 0xd658…0442; extrinsics: [0x10f8…04a3]]

Message from polkadot.js side

RPC-CORE: submitExtrinsic (extrinsic: Extrinsic): Hash:: [1002]: execution error
ExtError: submitExtrinsic (extrinsic: Extrinsic): Hash:: [1002]: execution error

After a whole afternoon of digging, the root cause is I removed the indices module, which changed the format of Address changed from enum Address to AccountId. This is a codec breaking change but I was unaware about it.

As the result, the polkadot.js still encodes the transaction with the enum format, and substrate failed to decode it and panicking at this unwrap while validating the transaction

substrate/core/sr-api-macros/src/decl_runtime_apis.rs

Lines 160 to 166 in e6d9814

	fn convert_between_block_types
	<I: #crate_::runtime_api::Encode, R: #crate_::runtime_api::Decode>(input: &I) -> R
	{
	<R as #crate_::runtime_api::Decode>::decode(
	&mut &#crate_::runtime_api::Encode::encode(input)[..]
	).unwrap()
	}

#1328 should improve the compatibility issue (which I am aiming to finish it early next week) that it allows polkadot.js to discover the correct types.
However, substrate should just handle the decode issue and reject the invalid encoded transaction instead of panic.

[bkchr]

Maybe we should return an error instead of unwrapping the Option. As I implemented this function, I was thinking that the node type always needs to be decodable in the runtime.
@xlc Can you say which runtime function should be called?
@tomusdrw can we somehow check that the transaction is valid, before calling into the runtime?

[xlc]

It is trying to call validate_transaction
I don't have stack trace currently but I can reproduce the error and give you full stack trace if needed

[bkchr]

No that is enough, ty. It would have paniced in the wasm runtime as well. But yeah, panic in native code is not that nice.

[tomusdrw]

@bkchr we should make sure that it's "structurally valid/compatible" by encoding/decoding if we know the runtime type that is used, then validate_transaction would just check semantics. I'm not sure if we can do the former without calling into runtime though we only use opaque types everywhere, no?