Switch ValidateUnsigned over to SignedExtension

[jimmychu0807]

Currently to validate an unsigned transaction we require to implement ValidateUnsigned and need to register the unsigned validation in construct_runtime!. We should remove all this functionality. The validation of an unsigned transaction should be done by a signed extension. By default, signed extension should deny any unsigned transaction. Only if there is a SignedExtension that validates an unsigned transaction and approves it, we should approve it to the calling context.

As validating an unsigned transaction is in most cases always a very similar operation, we should provide an extendable SignedExtension that can be used by any module to validate its unsigned transactions.

1. Implement the new functionality into SignedExtension to validate unsigned transactions.
2. Remove ValidateUnsigned and all the related functionality and move all the code over to SignedExtension.
3. Write the generic SignedExtension for validating unsigned transactions.

All 3 points should be do-able in separate prs. Mentoring and help can be provided by @bkchr

[tomusdrw]

Isn't that a duplicate of paritytech/substrate#3419 ?

[kianenigma]

Related, but no. Doing this PR should be done anyhow, leaving SignedExtension with 4 functions. The other issue would try to further shrink/generalize this into a generic set of two funcitons.

[tomusdrw]

Right. I've been thinking about this lately a bit though, and I feel that having every pallet expose it's own SignedExtension seems like an overkill. I think that we will end up with a trait that pretty much looks exactly like ValidateUnsigned, but the way it's invoked will be a bit different. Currently it has a special meaning when declaring a runtime, and I think that instead there should be a generic SignedExtension that just calls into pallet's ValidateUnsigned (aggregating results from multiple pallets).

So to address points from the original description:

1. This is already possible via validate_unsigned in SignedExtension, so no change needed here.
2. That might not be necessary, my gut feeling that it would be more ergonomic to stay with ValidateUnsigned as described above.
3. That's definitely something we need.

[erasmospunk]

I have been working on this issue and implemented some things:

Implement new SignedExtension logic to validate unsigned transactions

With the new rules SignedExtension::validate_unsigned will return an error
by default, specifically UnknownTransaction::NoUnsignedValidator to indicate
that unsigned validation was not implemented.
For SignedExtension tuples, there is a new logic as well:

- If all return NoUnsignedValidator (none of them did the check),
  it is considered invalid and Err(NoUnsignedValidator) is returned.

- If one returns an Err(_), it is considered invalid and that error is
  returned. The first error occurred will be returned.

- If at least one returns Ok(_) and no error is returned, it is
  considered valid.

This behaviour applies to validate_unsigned and pre_dispatch_unsigned.
The difference is that pre_dispatch_unsigned will create a Pre::default()
for each NoUnsignedValidator returned (assuming the call succeeds).

Some others are:

• Make ImOnLine work with the SignedExtension
• Implement a whitelist for inherents to avoid them failing in the traits::Applyable stage if no signed extension could process them (this is not 100% complete)

Here is some of the code (haven't pushed the whitelist macro yet): https://github.com/erasmospunk/substrate/tree/issue_4419_solution

Due to the pull request paritytech/substrate#4998, all the unsigned (and inherents) transactions are processed by the CheckWeight so the whitelist is not needed by default.

If I could help finishing this issue, let me know.

[tomusdrw]

@erasmospunk have you seen: paritytech/substrate#5006 ?

My main critique regarding having every pallet have it's own SignedExtension is that the transaction format changes with every new pallet that you want to add.

We could consider doing a single signed extension that groups all the "Validators", i.e.:

// Instead of Vec we should probably just use tuple, to make it less dynamic.
struct Validators(Vec<Box<dyn SignedExtension<AdditionalData=()>>);
impl SignedExtension for Validators {
 ...
}

That way you can simply add a new extension, but without changing the Extra format (that already contains a bunch of (), which looks ugly).
However I feel that implementing ValidateUnsigned as in paritytech/substrate#5006 is a bit more developer friendly.

[erasmospunk]

@tomusdrw yes, I saw it few days ago (forgot to subscribe this issue and missed it). I did something similar in the very beginning with the difference that you reuse ValidateUnsigned.

The SignedExtra tuple contains generic checks frame_system::Check* and pallet specific checks like pallet_transaction_payment, pallet_contracts and with the removal of ValidateUnsigned we would should add the pallet_im_online. So if the generic checks were separate from the pallet checks, the later could be in principle generated by a macro and combined to form the final SignedExtra?

I'm not sure how to solve the ugly () case, maybe using Default::default()?

[gui1117]

looking at this discussion and paritytech/substrate#5006 (comment)

I'm wondering why not removing unsigned (non-inherent) extrinsic altogether ?
So that the only remaining extrinsics are: inherent and signed extrinsics.

Any unsigned transaction can already be replaced by a signed extrinsic with signed extension and Pays::No (e.g. polkadot claim attest)
We probably need to come up with a mecanism to ensure that the signed extension is used.
(maybe we can use integrity_test hook here, but I don't know how)

Then we can just deprecate all unsigned transaction related logic or remove it.

The main con of deprecating/removing unsigned logic is that maybe some extrinsic will have useless signature just for the sake of it being signed.

[tomusdrw]

@thiolliere there are still at least two reasons which make unsigned extrinsics interesting.

1. They can be easily de-duplicated on the transaction pool level, cause transaction containing exactly the same data will look identical (byte-wise) so it can easily be compared by hash. This lowers the risk of DoS vector, cause the verification doesn't even need to be performed if the transaction has been seen previously.
2. Second reason is nonce usage - with signed transaction you need to keep track of it, for unsigned you simply pack the data and send it, without caring how many transactions you have seen previously. This also allows for parallelism (although that could be simulated by using two different keys to sign stuff).

[gui1117]

ah indeed these are good reasons, maybe we could still have a specific public account which none are not checked but seems to be a trickier implementation than our current implementation (or maybe another info in DispathcInfo)

[shawntabrizi]

Needs documentation and clear directions

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.