Change assert(is_err()) to assert_noop to check state consistency on errors

[hirschenberger]

fixes #8545

[hirschenberger]

I think I need some help here with the failing test.

When I change the expected error type to a wrong value e.g. TransactionValidityError::Invalid(InvalidTransaction::BadProof) I get this, which is what I expect:

---- tests::bad_extrinsic_not_inserted stdout ----
on_initialize(1)
thread 'tests::bad_extrinsic_not_inserted' panicked at 'assertion failed: `(left == right)`
  left: `Err(TransactionValidityError::Invalid(InvalidTransaction::Future))`,
 right: `Err(TransactionValidityError::Invalid(InvalidTransaction::BadProof))`', frame/executive/src/lib.rs:860:13

When I change the type to the correct value: TransactionValidityError::Invalid(InvalidTransaction::Future) I get this strange error:

---- tests::bad_extrinsic_not_inserted stdout ----
on_initialize(1)
thread 'tests::bad_extrinsic_not_inserted' panicked at 'assertion failed: `(left == right)`
 left: `[43, 22, 252, 42, 247, 106, 220, 153, 188, 63, 218, 120, 117, 228, 9, 45, 42, 26, 239, 243, 16, 226, 217, 97, 143, 195, 251, 246, 181, 70, 68, 178]`,
right: `[65, 171, 6, 54, 28, 188, 146, 219, 140, 67, 51, 124, 41, 234, 137, 189, 241, 58, 79, 94, 30, 245, 108, 142, 60, 124, 58, 24, 198, 160, 189, 158]`', frame/executive/src/lib.rs:860:13

[shawntabrizi]

@hirschenberger this means you caught a bug :)

[hirschenberger]

Yippie ;-)

Any hints where it goes wrong? The lists are no UTF-8 strings and I assume also no SCALE encoded data. Do you think it's a substrate or rust issue?

[shawntabrizi]

@hirschenberger it means that in the test, there is some extrinic which is being called, and returning an error. But it is also changing the state of the chain. This is ultimately what the noop assert tests for.

You need to look into the extrinsic to see if you can catch where it can fail after already changing state. I will look into this now, it can be a little complex and sensitive to solve these issues.

BTW can you please include your DOT / KSM address? Have you done that already in another PR?

[hirschenberger]

I see, excited to see what the reason for the state change is. I'll also look in the code and try to help with the analysis.

Yes I already got a tip from you in another PR: #8541 (comment)

I was confused about the two binary lists and thought it was due to a wrong error type that was returned. How about changing assert_noop to display a descriptive error that the state changed by just adding a third parameter to the assert_eq in the macro?

[gavofyork]

There is no bug here.

The function in question (apple_extrinsic) is not a dispatchable, therefore it does not conform to the usual no-change-on-error. apply_extrinsic is allowed to, and indeed will, mutate the state if given an erroring transaction since it is assumed that the transaction is added to the block regardless of whether it happened to be successful or not.

In this case, just revert the test to how it was before (or, if you like, use assert_err!() instead to ensure the error is correct).

[shawntabrizi]

@hirschenberger no force push please :)

If you need help with git, happy to do so.

[hirschenberger]

I think I'm done. CI got stuck.

[gui1117]

bot merge

[ghost]

Trying merge.