Add defensive testing extrinsic #1998

adelarja · 2023-10-24T03:21:55Z

Description

The trigger_defensive call has been added to the root-testing pallet. The idea is to have this pallet running on Rococo/Westend and use it to verify if the runtime monitoring works end-to-end.

To accomplish this, trigger_defensive dispatches an event when it is called.

Closes #1953

Checklist

My PR includes a detailed description as outlined in the "Description" section above
My PR follows the labeling requirements of this project (at minimum one label for T
required)
I have made corresponding changes to the documentation (if applicable)
I have added tests that prove my fix is effective or that my feature works (if applicable)

You can remove the "Checklist" section once all have been checked. Thank you for your contribution!

✄ -----------------------------------------------------------------------------

substrate/frame/root-testing/src/lib.rs

ggwpez · 2023-10-24T23:00:43Z

Good so far, now please also deploy to Rococo/Westend (as hinted in the issue #1953).

bkchr · 2023-10-25T09:40:25Z

substrate/frame/root-testing/src/lib.rs

+		#[pallet::weight(0)]
+		pub fn trigger_defensive(origin: OriginFor<T>) -> DispatchResult {
+			ensure_root(origin)?;
+			frame_support::defensive!("root_testing::trigger_defensive was called.");


@ggwpez so you want this to panic? I don't really get what you want to achieve?

defensive is just log error and debug_assert, which is nop on release build, so this is a bit pointless. if you want to test log error, just log error.

Yeah I know that this is using a debug assert, I was assuming @ggwpez wants to build with debug asserts enabled and then use it as overwrite.

The idea here is that we are currently not sure if the monitoring correctly picks up defensive failures. And we really should be notified about them, so this is just as test.
It could also be logged, but the defensive! macro also logs a magic string (DEFENSIVE_OP_PUBLIC_ERROR) that we can filter for and alert in the grafana.
Then we can call this and check that we got notifications in chat about this failure.

polkadot/runtime/rococo/Cargo.toml

polkadot/runtime/rococo/src/lib.rs

ggwpez

LGTM!
cc @PierreBesson I hope you are prepared 😆

PierreBesson · 2023-10-27T15:22:45Z

I would suggest to use a new special log level ("log target" in rust) called DEFENSIVE. This will make it easier to identify those log in monitoring systems as the log level can be easily indexed in systems such as Grafana Loki.

ggwpez · 2023-10-27T15:24:48Z

I would suggest to use a new special log level ("log target" in rust) called DEFENSIVE. This will make it easier to identify those log in monitoring systems as the log level can be easily indexed in systems such as Grafana Loki.

Oh yea, did not think about this. Currently we just use runtime, but we could switch it to runtime::defensive.
Could you please also do this @adelarja?

adelarja · 2023-10-27T23:00:04Z

I would suggest to use a new special log level ("log target" in rust) called DEFENSIVE. This will make it easier to identify those log in monitoring systems as the log level can be easily indexed in systems such as Grafana Loki.

Oh yea, did not think about this. Currently we just use runtime, but we could switch it to runtime::defensive. Could you please also do this @adelarja?

Sure, I can do it. But I'm not entirely sure about what do we need to do.

@PierreBesson do you mean doing something like:

log::info!(target: "DEFENSIVE", "Example log!");

@ggwpez Where are we setting the target to runtime?

ggwpez · 2023-10-28T12:36:44Z

@ggwpez Where are we setting the target to runtime?

It needs to be changed in the macros to runtime::defensive please:

polkadot-sdk/substrate/frame/support/src/traits/misc.rs

Line 58 in a706171

target: "runtime",

adelarja · 2023-10-28T15:57:51Z

@ggwpez Where are we setting the target to runtime?

It needs to be changed in the macros to runtime::defensive please:

polkadot-sdk/substrate/frame/support/src/traits/misc.rs

Line 58 in a706171

target: "runtime",

Ohh yes I saw that. I thought we needed to implement something more difficult haha. I've made the change.

ggwpez · 2023-10-29T10:07:59Z

Looks like there is a conflict with master, could you please resolve the conflicts? Then we can merge.

adelarja · 2023-10-30T02:49:16Z

Looks like there is a conflict with master, could you please resolve the conflicts? Then we can merge.

Fixed. The rococo and westend tests are passing.

PierreBesson · 2023-10-30T08:59:31Z

@PierreBesson do you mean doing something like:

log::info!(target: "DEFENSIVE", "Example log!");

@adelarja, having logs printed as runtime::defensive as @ggwpez suggested will do the job nicely. I see that you have already implemented this 👍 .

ggwpez · 2023-10-31T11:12:34Z

Seeing this in the CI:

error[E0046]: not all trait items implemented, missing: `RuntimeEvent`
   --> substrate/frame/utility/src/tests.rs:190:1
    |
 190 | impl pallet_root_testing::Config for Test {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `RuntimeEvent` in implementation
    |
    = help: implement the missing item: `type RuntimeEvent = /* Type */;`

Mostly all CI checks should be green. Once that is, we should be good to merge.

…a/polkadot-sdk into 1953-defensive-testing-extrinsic

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

# Description The `trigger_defensive` call has been added to the `root-testing` pallet. The idea is to have this pallet running on `Rococo/Westend` and use it to verify if the runtime monitoring works end-to-end. To accomplish this, `trigger_defensive` dispatches an event when it is called. Closes paritytech#1953 # Checklist - [x] My PR includes a detailed description as outlined in the "Description" section above - [ ] My PR follows the [labeling requirements](CONTRIBUTING.md#Process) of this project (at minimum one label for `T` required) - [ ] I have made corresponding changes to the documentation (if applicable) - [ ] I have added tests that prove my fix is effective or that my feature works (if applicable) You can remove the "Checklist" section once all have been checked. Thank you for your contribution! ✄ ----------------------------------------------------------------------------- --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* try revert separate cargo fetch * Revert "try revert separate cargo fetch" This reverts commit 906f53483a2ca588e73d94b63e7d2b079bf9870c. * try cargo fetch for wasm32-unknown-unknown * run cargo test with runtime-benchmarks feature only

adelarja added 3 commits October 23, 2023 22:24

Added draft.

43a8bde

Fixed some issues

eb6db71

Applied cargo fmt

dc91740

adelarja requested review from a team October 24, 2023 03:21

adelarja mentioned this pull request Oct 24, 2023

defensive! testing extrinsic #1953

Closed

ggwpez reviewed Oct 24, 2023

View reviewed changes

substrate/frame/root-testing/src/lib.rs Show resolved Hide resolved

paritytech-ci requested a review from a team October 24, 2023 10:20

ggwpez added the T2-pallets This PR/Issue is related to a particular pallet. label Oct 24, 2023

ggwpez assigned adelarja Oct 24, 2023

Added frame_support::defensive! call

e5aa227

bkchr reviewed Oct 25, 2023

View reviewed changes

paritytech-ci requested a review from a team October 25, 2023 09:41

Added pallet_root_testing to rococo and westend runtimes.

2e3699e

adelarja commented Oct 26, 2023

View reviewed changes

polkadot/runtime/rococo/Cargo.toml Outdated Show resolved Hide resolved

adelarja commented Oct 26, 2023

View reviewed changes

polkadot/runtime/rococo/src/lib.rs Outdated Show resolved Hide resolved

Switched pallet order in construct_runtime

c693199

ggwpez approved these changes Oct 27, 2023

View reviewed changes

bkchr approved these changes Oct 27, 2023

View reviewed changes

Changed runtime to runtime::defensive in frame_support misc trait

b5f59d6

ggwpez approved these changes Oct 29, 2023

View reviewed changes

Fixed merge conflicts

09888fd

adelarja changed the title ~~[WIP] 1953 defensive testing extrinsic~~ 1953 defensive testing extrinsic Oct 30, 2023

Merge branch 'master' into 1953-defensive-testing-extrinsic

8ef20cd

adelarja and others added 3 commits October 31, 2023 11:11

Fixed trait implementation in pallet-utility

f950f72

Merge branch '1953-defensive-testing-extrinsic' of github.com:adelarj…

795b4df

…a/polkadot-sdk into 1953-defensive-testing-extrinsic

Update Cargo.lock

4ca615c

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez merged commit 6e2f94f into paritytech:master Oct 31, 2023
109 of 110 checks passed

ggwpez changed the title ~~1953 defensive testing extrinsic~~ Add defensive testing extrinsic Oct 31, 2023

github-actions bot mentioned this pull request Feb 19, 2024

Update substrate/polkadot/cumulus from v1.3.0 to v1.6.0 moondance-labs/tanssi#419

Closed

github-actions bot mentioned this pull request Mar 13, 2024

Update polkadot-sdk from v1.3.0 to v1.7.2 moonbeam-foundation/moonbeam#2703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add defensive testing extrinsic #1998

Add defensive testing extrinsic #1998

adelarja commented Oct 24, 2023 •

edited

Loading

ggwpez commented Oct 24, 2023 •

edited

Loading

bkchr Oct 25, 2023

xlc Oct 25, 2023

bkchr Oct 25, 2023

ggwpez Oct 25, 2023 •

edited

Loading

ggwpez left a comment •

edited

Loading

PierreBesson commented Oct 27, 2023

ggwpez commented Oct 27, 2023

adelarja commented Oct 27, 2023

ggwpez commented Oct 28, 2023 •

edited

Loading

adelarja commented Oct 28, 2023

ggwpez commented Oct 29, 2023

adelarja commented Oct 30, 2023 •

edited

Loading

PierreBesson commented Oct 30, 2023 •

edited

Loading

ggwpez commented Oct 31, 2023

Add defensive testing extrinsic #1998

Add defensive testing extrinsic #1998

Conversation

adelarja commented Oct 24, 2023 • edited Loading

Description

Checklist

ggwpez commented Oct 24, 2023 • edited Loading

bkchr Oct 25, 2023

Choose a reason for hiding this comment

xlc Oct 25, 2023

Choose a reason for hiding this comment

bkchr Oct 25, 2023

Choose a reason for hiding this comment

ggwpez Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

ggwpez left a comment • edited Loading

Choose a reason for hiding this comment

PierreBesson commented Oct 27, 2023

ggwpez commented Oct 27, 2023

adelarja commented Oct 27, 2023

ggwpez commented Oct 28, 2023 • edited Loading

adelarja commented Oct 28, 2023

ggwpez commented Oct 29, 2023

adelarja commented Oct 30, 2023 • edited Loading

PierreBesson commented Oct 30, 2023 • edited Loading

ggwpez commented Oct 31, 2023

adelarja commented Oct 24, 2023 •

edited

Loading

ggwpez commented Oct 24, 2023 •

edited

Loading

ggwpez Oct 25, 2023 •

edited

Loading

ggwpez left a comment •

edited

Loading

ggwpez commented Oct 28, 2023 •

edited

Loading

adelarja commented Oct 30, 2023 •

edited

Loading

PierreBesson commented Oct 30, 2023 •

edited

Loading