Implement ValidateUnsigned as SignedExtension

[tomusdrw]

A follow up on #5003
Related: #3419
Closes: paritytech/polkadot-sdk#365

This PR removes a special role ValidateUnsigned has in executive. Instead we have a ValidateUnsigned extension defined in frame_system, which dispatches the calls instead.

This is not properly tested yet, and possibly not something we want to include for 2.0, but still wanted to make the PR visible.

Let me know what you think, CC @bkchr @jimmychu0807

[kianenigma]

I def. like the idea better as now we don't have two thing flying around doing the same thing: ValidateUnsigned and SignedExtension. But I think in the very long term, we want only the latter. Even with this PR, it is a bit confusing: You as a runtime dev have two options:

• implement ValidateUnsigned and system will do the rest of the magic for you (if I have understood your code correctly -- had to read it really fast)
• Implement your own SignedExtension.

If we can find a nice way to be able to have the nice features of ValidateUnsigned into SignedExtension, then I think that is much better. Based on the issue discussions the main problem with SignedExtension seem to be the glowing list of Extra.

[tomusdrw]

Based on the issue discussions the main problem with SignedExtension seem to be the glowing list of Extra.

Exactly this, so it's definitely possible for every pallet to expose it's own SignedExtension, but it seems like an overkill and it adds a lot of boilerplate - i.e. you need to change your UncheckedExtrinsic::Extra implementation every time you add a new module that has unsigned transactions which seems insane.

I think that ValidateUnsigned implementation for a pallet should be the way to go in the future. It's simple and does the job really well. We already aggregate all ValidateUnsigned impls from modules and implement ValidateUnsigned for the entire Runtime, and it's makes it definitely less effort.

If ever anyone needs the additional info passed to SignedExtensions they can definitely implement one for their module. Also we can revisit the set of methods of SignedExtensions as suggested in #3419

[gavofyork]

@tomusdrw is this a draft or ready for review?

[tomusdrw]

To be frank, I'm not actively working on this. I was hoping that we will merge that after 2.0 is released, as I didn't put a lot of work into testing it's correctness (and was quite surprised that none of our tests broke :)).

That said, we can review that and let @gnunicorn decide if we want to include in 2.0 or rather put on hold. IMHO this makes the Applyable API way cleaner which might help with #5130 as well.

[bkchr]

When you forget to add ValidateUnsigned to the list of signed extensions, any unsigned transaction will be valid.

This is a huge security problem of this implementation.

[tomusdrw]

Re-wrote using different approach of altering only pre_dispatch. Still want to add tests tomorrow.

[tomusdrw]

This is a breaking change. Previously if you made an unsigned call to non-existent module it would merely return UnknownTransaction, which is interpreted by the txpool as "not valid now, might be valid at some point in the future". Which is sort of ok, but it has undersirable consequence of not banning the transaction - so basically you can spam the txpool with (one; the same) transaction to non-existent module to make it constantly re-verify it.
I think it's more wise to reject such transaction (pretty much the same would happen if you try to call a non-existent function (we return InvalidTransaction::Call)

[tomusdrw]

Tests added, should be ready to re-review.

[gavofyork]

For signed transactions, SignedExtension works by default-pass (blacklisting), not whitelisting.

[tomusdrw]

That's only partially true, since the ValidTransaction produced will have no provides tags, which will be rejected by the transaction pool anyway.

Now CheckedExtrinsic will explicitly return InvalidTransaction error if no SignedExtension provides any tags in validate call. I also unified that with pre_dispatch now, so basically it's a whitelist now (in a sense that you need at least one SignedExtension to provide some tags, don't need to whitelist specific calls or anything).

[gavofyork]

Honestly, I'm not especially keen on this refactor.

I see almost no improvement in the API; the only advatage here that I can see is that we get to lose one generic param in Executive (which could perhaps be lost in a better way by combining with AllModules) and one generic param elsewhere in validate/apply.

There are, however, several costs; we have an additional param going into validate and we have an additional datapath out of SignedExtension::pre_dispatch with the (rather opaque concept of) IsFullyValidated. We also have an additional item in SignedExtra that must be there or random other stuff in a runtime will break.

With a refactoring that adds no functionality like this, the other indicator I look at is code size change. Admittedly here, there's an extra test module and plenty of docs, it's still clear that this overall adds code to the main implementation. Not a good sign.

It is highly questionable whether ValidateUnsigned should be subjugated to SignedExtension and whether the latter should see its API bloated in that cause. The more I consider it, the more I'm inclined to say simply that the two are actually quite orthogonal and ought to be sibling APIs. SignedExtensions are individually opt-in (like, for example, the SignedExtension which blocks parachains transactions currently in Kusama) that are not necessarily a part of a pallet. ValidateUnsigneds are OTOH inexticable parts of pallets.

[gavofyork]

If we really want to combine ValidateUnsigned and SignedExtension, then I think a better direction would be first to consider the possibility of introducing some way to allow a pallet to state that a SignedExtension were mandatory for the runtime and have it auto-included without the need to collated SignedExtensions manually as is the case now. We might then revisit this PR in so much as to add the API that would allow SignedExtensions to be used for the various ValidateUnsigned use-cases that we have. We could then move the ValidateUnsigned usages over and retire the API.

[tomusdrw]

I see almost no improvement in the API; the only advatage here that I can see is that we get to lose one generic param in Executive (which could perhaps be lost in a better way by combining with AllModules) and one generic param elsewhere in validate/apply.

The main improvement imho is unified validation pipeline for signed and unsigned transactions, and decoulping unsigned transactions from Executive (the generic parameter you've mentioned).

we have an additional param going into validate

I see that more as an improvement actually - having TransactionSource in SignedExtension is basically a missing part in my previous PR - I think it should simply be there for consistency.

we have an additional datapath out of SignedExtension::pre_dispatch with the (rather opaque concept of) IsFullyValidated.

Only added to address Basti's critique about unsigned transactions being valid by default, removed in new approach. However I think this is a moot point anyway, since even though validate is going to return Ok the ValidTransaction has no provides tags which would cause the pool to reject it anyway.

We also have an additional item in SignedExtra that must be there or random other stuff in a runtime will break.

I agree this is suboptimal, I want to get rid of all () in SignedExtra by introducing a single SignedExtension that is able to group a tuple of SignedExtension<AdditionalData=()>. So that all
adding/removing no-argument validator-like extensions would not alter SignedExtra type. But
that's rather a separate PR.

It is highly questionable whether ValidateUnsigned should be subjugated to SignedExtension and whether the latter should see its API bloated in that cause.

The point is that currently SignedExtensions make an impression of being able to handle unsgined transactions while in reality it's not possible at all. You can't create a SignedExtension and whitelist a call there, because UnsignedValidator takes precedence anyway. SignedExtension can only be used to further restrict unsigned transactions (or augment the ValidTransaction result). I think having two mechanisms that seem to address the same problem is confusing and that's why I believe this unification makes sense.

---

(Sorry for a wall of text above, tl;dr is below)

@gavofyork I've made another attempt to reduce the surface of API changes:

1. The InvalidTransaction::NoUnsignedValidator is special cases for *_unsigned pipeline so that it doesn't short-circuit, but rather all SignedExtensions are allowed to whitelist a transaction. This changes the semantics only for unsigned transactions (whitelist).
2. Same behavior for signed transactions is now reverted, so as previously they are valid by default (yet they will be rejected by the transaction pool anyway).

If that approach is going to be rejected as well I will split the PR into smaller improvements/fixes that don't change the semantics, namely:

• Extra tests and docs
• Passing TransactionSource to SignedExtension
• Returning InvalidTransaction instead of UnknownTransaction in case of unsigned validation failure.

[gavofyork]

Happy for the incremental improvements. Also happy to continue with the approach to try to merge the APIs, but like I say in my previous comment, I think that the primary aspect of the approach must be to allow a pallet to state that a SignedExtension be mandatory for a runtime that includes the pallet since the SignedExtension API is entirely independent of the pallet APIs, and we would then need to handle it appropriately in any edge-cases (e.g. with instanceable pallets).

[tomusdrw]

Closing. Will split into smaller PRs.