Benchmark's successful origin api update

[muharem]

In the PR we trying to improve the EnsureOrigin trait under the "runtime-banchmarks" feature.

Address to frame/support/src/traits/dispatch.rs file to see the changes of the trait.

Problem:

Infinite recursion

The default implementations of EnsureOrigin::successful_origin and EnsureOrigin::try_successful_origin, triggers infinite recursion which easy to cause and hard to debug (you cant see the trace without tools like GDB or LLDB).

The error example:

╰─➤  ./target/debug/polkadot-parachain benchmark pallet --chain=collectives-polkadot-dev --steps=20 --repeat=1 --pallet=pallet_ambassador --extrinsic="*" --execution=native --wasm-execution=compiled --heap-pages=4096 --output=./parachains/runtimes/collectives/collectives-polkadot/src/weights/pallet_ambassador.rs
2023-01-10 19:24:20 assembling new collators for new session 0 at #0    
2023-01-10 19:24:20 assembling new collators for new session 1 at #0    

thread 'main' has overflowed its stack
fatal runtime error: stack overflow
[1]    29525 abort      ./target/debug/polkadot-parachain benchmark pallet  --steps=20 --repeat=1    

Infallible api

The successful_origin is infallible, where in reality there are implementations like EnsureNever which has no successful origin.

Solution

Remove successful_origin func from the trait. use try_successful_origin instead;

polkadot companion: https://github.com/paritytech/polkadot/pull/6598 cumulus companion: https://github.com/paritytech/cumulus/pull/2122

[ggwpez]

Now instead of unwrapping the try_success_origin calls in the benchmarks you can use the approach from here: https://github.com/paritytech/substrate/pull/12829#discussion_r1038135437

This skips the benchmark and marks it as weight=0 since it is un-invocable.

[muharem]

Now instead of unwrapping the try_success_origin calls in the benchmarks you can use the approach from here: #12829 (comment)

This skips the benchmark and marks it as weight=0 since it is un-invocable.

Returning Err as a default impl, was not good idea. For most of the cases it will be wrong implementation and might result an unexpected behaviour (0 weight). I revert it to unimplemented macro.

[bkchr]

Returning Err as a default impl, was not good idea. For most of the cases it will be wrong implementation and might result an unexpected behaviour (0 weight). I revert it to unimplemented macro.

We should just remove the default impl?

[muharem]

Returning Err as a default impl, was not good idea. For most of the cases it will be wrong implementation and might result an unexpected behaviour (0 weight). I revert it to unimplemented macro.

We should just remove the default impl?

My suggestion is to have default impl, where we just call unimplemented!(). I already pushed those changes, you can see here. This way we do not introduce any real default behaviour, and also do not require from a user to impl the function for every EnsureOrigin impl.

[muharem]

@bkchr we have the same approach here.

[muharem]

updated PR description

[bkchr]

This way we do not introduce any real default behaviour, and also do not require from a user to impl the function for every EnsureOrigin impl.

This doesn't really make any sense. Every EnsureOrigin implementation will need this function as it will be used at some point in some benchmark. Otherwise this EnsureOrigin isn't required to exist. If we also don't force to implement this method, it means that it may fails downstream and then some upstream pr will be required to fix it etc.

[muharem]

Some improvement could be that try_successful_origin returns NoSuccessfulOrigin (bad naming as always!) and then there could be a From<NoSuccessfulOrigin> for BenchmarkError returning BenchmarkError::Weightless, but not sure it is such a good idea to make this "automatic"

I will keep it simple for now, if no objections. The only possible error variant Err(()) now means no successful origin, which we have documented in docs.

[muharem]

bot merge

[paritytech-processbot[bot]]

Waiting for commit status.

[paritytech-processbot[bot]]

Merge cancelled due to error. Error: Statuses failed for 815c850509a539e96c035e604cfa740a9c863191

[muharem]

bot rebase

[paritytech-processbot[bot]]

Rebased

[muharem]

bot rebase

[paritytech-processbot[bot]]

Branch is already up-to-date

[muharem]

ignoring continuous-integration/gitlab-publish-docker-substrate-temporary CI job fail, not related to the proposed changes.