Invalid signatures passed to a pallet's method stop block import

[nahuseyoum]

We have a pallet with a function that accepts an SR25199 signature and verifies it. Our implementation looks like this:

pub trait Trait: system::Trait {
   ...
   type Public: IdentifyAccount<AccountId = Self::AccountId>;
   type Signature: Verify<Signer = Self::Public> + Member + Decode + Encode;
}

And a verify method which verifies the signature

pub struct Proof {
   pub signer: T::AccountId,
   pub signature: T::Signature,
}

fn verify_signature(
       proof: &Proof<T::Signature, T::AccountId>,
       signed_payload: &[u8]
   ) -> Result<(), Error<T>> 
{
    match proof.signature.verify(signed_payload,&proof.signer) 
    {
        true => Ok(()),
        false => Err(<Error<T>>::UnauthorizedTransaction.into()),
    }
}

This setup worked fine for us until recently where we discovered that our network was stopping to finalise with an error about Signature verification.

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

Version: 2.0.0-rc2-5eb246fb6-x86_64-linux-gnu

0: sp_panic_handler::set::{{closure}}
1: std::panicking::rust_panic_with_hook
at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libstd/panicking.rs:476
2: std::panicking::begin_panic
3: frame_executive::Executive<System,Block,Context,UnsignedValidator,AllModules,COnRuntimeUpgrade>::execute_block
4: <node_runtime::Runtime as sp_api::runtime_decl_for_Core::Core<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32,sp_runtime::traits::BlakeTwo256>,sp_runtime::generic::unchecked_extrinsic::UncheckedExtrinsic<<pallet_indices::Module<node_runtime::Runtime> as sp_runtime::traits::StaticLookup>::Source,node_runtime::Call,sp_runtime::MultiSignature,(frame_system::CheckSpecVersion<node_runtime::Runtime>, frame_system::CheckTxVersion<node_runtime::Runtime>, frame_system::CheckGenesis<node_runtime::Runtime>, frame_system::CheckEra<node_runtime::Runtime>, frame_system::CheckNonce<node_runtime::Runtime>, frame_system::CheckWeight<node_runtime::Runtime>, pallet_transaction_payment::ChargeTransactionPayment<node_runtime::Runtime>, pallet_grandpa::equivocation::ValidateEquivocationReport<node_runtime::Runtime>)>>>>::execute_block
5: std::panicking::try::do_call
6: __rust_maybe_catch_panic
at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libpanic_unwind/lib.rs:86
7: std::thread::local::LocalKey::with
8: sc_executor::native_executor::WasmExecutor::with_instance::{{closure}}
9: sc_executor::wasm_runtime::RuntimeCache::with_instance
10: <sc_executor::native_executor::NativeExecutor as sp_core::traits::CodeExecutor>::call
11: sp_state_machine::StateMachine<B,H,N,Exec>::execute_aux
12: sp_state_machine::StateMachine<B,H,N,Exec>::execute_using_consensus_failure_handler
13: <sc_service::client::call_executor::LocalCallExecutor<B,E> as sc_client_api::call_executor::CallExecutor>::contextual_call
14: <sc_service::client::client::Client<B,E,Block,RA> as sp_api::CallApiAt>::call_api_at
15: sp_api::runtime_decl_for_Core::execute_block_call_api_at
16: sp_api::Core::execute_block
17: <&sc_service::client::client::Client<B,E,Block,RA> as sp_consensus::block_import::BlockImport>::import_block
18: <sc_finality_grandpa::import::GrandpaBlockImport<BE,Block,Client,SC> as sp_consensus::block_import::BlockImport>::import_block
19: <sc_consensus_babe::BabeBlockImport<Block,Client,Inner> as sp_consensus::block_import::BlockImport>::import_block
20: sp_consensus::import_queue::import_single_block
21: <futures_util::future::poll_fn::PollFn as core::future::future::Future>::poll
22: <futures_util::future::future::Map<Fut,F> as core::future::future::Future>::poll
23: <futures_util::future::future::flatten::Flatten<Fut,::Output> as core::future::future::Future>::poll
24: <futures_util::future::poll_fn::PollFn as core::future::future::Future>::poll
25: std::panicking::try::do_call
26: __rust_maybe_catch_panic
at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libpanic_unwind/lib.rs:86
27: std::panicking::try
28: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll
29: <std::future::GenFuture as core::future::future::Future>::poll
30: std::thread::local::LocalKey::with
31: futures_executor::local_pool::block_on
32: tokio::runtime::task::core::Core<T,S>::poll
33: std::panicking::try::do_call
34: __rust_maybe_catch_panic
at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libpanic_unwind/lib.rs:86
35: tokio::runtime::task::harness::Harness<T,S>::poll
36: tokio::runtime::blocking::pool::Inner::run
37: tokio::runtime::context::enter
38: std::sys_common::backtrace::__rust_begin_short_backtrace
39: std::panicking::try::do_call
40: __rust_maybe_catch_panic
at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libpanic_unwind/lib.rs:86
41: core::ops::function::FnOnce::call_once{{vtable.shim}}
42: <alloc::boxed::Box as core::ops::function::FnOnce>::call_once
at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/liballoc/boxed.rs:1015
43: <alloc::boxed::Box as core::ops::function::FnOnce>::call_once
at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/liballoc/boxed.rs:1015
std::sys_common::thread::start_thread
at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libstd/sys_common/thread.rs:13
std::sys::unix::thread::Thread::new::thread_start
at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libstd/sys/unix/thread.rs:80
44: start_thread
45: clone

Thread 'tokio-runtime-worker' panicked at 'Signature verification failed.', <::std::macros::panic macros>:2

This is a bug. Please report it at:

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

After investigating this issue we discovered that the nodes start throwing this exception when we submit a signature that fails validation.
We looked into it a bit more and found the root cause to be this PR: d615043

Prior to this change, the verify method (defined here: primitives/runtime/src/traits.rs) uses a method called sr25519_verify to simply do the verification and return the result (defined here: primitives/io/src/lib.rs).

After this PR, sr25519_verify adds all signature verification requests into a vector and does a batch verification when a block is executed, so if we pass in an invalid signature to our pallet, instead of the pallet returning an error and continuing (as it normally does), it errors when trying to execute the block.

Do you have any suggestions on how to fix this problem?

Thanks.