[FRAME] MQ processor should be transactional

[ggwpez]

Follow up to https://github.com/paritytech/polkadot-sdk/pull/2356#discussion_r1395637202 where it was noticed that the MQ pallet does not enforce transactional processing.
Should probably be fixed, but it a breaking change, therefore creating a new issue for it.

[asoliman92]

I'd like to pick this up please @ggwpez

[lean-apple]

Is it in progress or I can try also ?

[ggwpez]

Yea seems to be stale, going to re-assign. Thanks @lean-apple!

[dharjeezy]

I want to work on this, @ggwpez I would think including the transactional attribute on the extrinsics would solve this, but as per the breaking change, should we introduce new extrinsics that are annotated transactional and deprecate the old one?

[ggwpez]

The transactional attribute is not already the default for all extrinsics.
This issue is about the this call (and the subsequent inner call to the MessageProcessor):https://github.com/paritytech/polkadot-sdk/blob/ae0b3bf6733e7b9e18badb16128a6b25bef1923b/substrate/frame/message-queue/src/lib.rs#L1240

When the message processor does some storage modifications and returns an error - then the assumption is most likely that all modifications are rolled back. But this is currently not the case.
Although this is a breaking change, i would label this as a fix to the current code. Creating a new extrinsic and explaining what it does/means and hoping that people will use it wont work i think.

[dharjeezy]

Hello @ggwpez does this means the function Self::process_message_payload should get executed within the storage::with_transaction context?

[ggwpez]

Yea could work, or rather only the call into the message processor within that function.