Unsigned transactions not validated during Executive::apply_extrinsic

[tomusdrw]

It seems that while you can't propagate incorrect unsigned transaction cause it won't be accepted to the pool you can still craft a block with bullshit and it's going to be processed just fine.

We should add a call to validate_unsigned in case we detect the extrinsic is unsigned transaction in apply_extrinsic and check that it returns TransactionValidity::Valid.

Alternatively the UnsignedValidator should have two different methods, where one is called during validate_transaction and second during apply_extrinsic phase. For convenience we can have a default implementation for the latter to just check for TransactionValidity::Valid.

[kianenigma]

Not sure how this can be confirmed but:

• I was for some time under the impression that every transaction (local or propagated) will go through validate_transaction anyhow and that is enough. I think you recently told me that it is not the case and only propagated txs that a node receives from the outer world go through this. (or the other way around; can't remember)

either way: if a transaction can avoid going through validate_transaction then: apply_extrinsic and validate_transaction should literally do the same checks and do the same steps (almost) except one is stateless and does not touch the state; hence the issue is valid.

(on a minor note that it would great if we document when executive calls are made by the client; tracking them due to api gluing with [pre/post]fixes is a bit hard and can cause such issues.)

[tomusdrw]

I was for some time under the impression that every transaction (local or propagated) will go through validate_transaction

Every transaction that goes into the pool indeed goes through validate_transaction. But when we execute a block received over the network, it's not really the case - we just call apply_extrinsic and in current implementation of Executive::apply_extrinsic you can find some duplication (signature checking) with validate_transaction.

[kianenigma]

Okay then re-thinking out-load here:

Every inherent need to be submitted (for the first time) to some node and that node will for sure run it through validate_transaction. Then any other node, as the receiver of this block containing the inherent, can simply trust that the at least one of the nodes has run validate_transaction() on it and I can trust that. Ok But then you realize that you actually can't trust that since a node can be malicious hence yeah back to _every apply_extrinsic() should also check the inherent.

Unless if since we are explicitly talking about inherent then it is not needed? I don't know the detailed definition of an inherent and what level of security/trust it should carry.

[kianenigma]

I keep coming back to this question: what should go through validate_ext and what should not. (everything, including things that come from the pool, goes through apply_ext so I think it is safe to assume that everything goes through apply_extrinsic at least once). So far I see in the code:

• signature check: both.
• nonce check: both.
• pay fee (including tips): both.
• check resource limit (size, weight, etc.): ONLY apply_ext.

I can come up with some arguments myself and convince myself that the above, which is based on the code, is correct, but would be nice if we can discuss:

• Is my list above correct? if so, is there a particular reasoning behind any of them?
• Why pay fees for just being pulled out of the pool? Especially since the pool (by which I mean validate_transaction) does not seem to care about the resouce limit. Hence, it is likely that you pay a bit while being processed in the pool but you are actually not executed in the block production. It also seems like if you do everything correctly and the chain processes your tx you end up paying twice? once for validate and once for apply?
• Also, isn't validate supposed to not change the state? In this case, paying for tips or fees in validate doesn't sound correct.

Finally, also looking at #3102 (cc @gavofyork) I see that

• TakeFee() only takes fees in the validate phase which is not the same as before so either must have been incorrect.

[tomusdrw]

Is my list above correct? if so, is there a particular reasoning behind any of them?

Seems about right.

Why pay fees for just being pulled out of the pool? [...] Also, isn't validate supposed to not change the state?

Indeed validate_transaction does not alter state. We only call make_payment to make sure that the sender is actually able to pay it, but that state modification is discarded.
Transactions are also validated one-by-one. We don't care about block limits, cause we don't know the order of transactions. So validate_transaction is always called in the context of the latest block, but transaction that we check is the only transaction in that block.

validate_transaction can be called multiple times and the changes are never persisted, it only exists for the transaction pool to be able to determine some rough metadata about the transaction, like:

1. relationship between other transaction - to be able to figure out proper ordering for propagation and block production
2. Structural and rudimental semantical validity (like are the signatures correct, and if it's recent) - basically all the cheap checks you can do in isolation to make sure we store as little junk transactions as possible in the pool.
3. Fee payment - this one is tricky, cause even though the user has fee at the time of validation there might be some other transactions that alter the state before it (note the opposite is also true, but that's a sacrifice we are willing to take), although it's cheap enough to be worth doing anyhow to discard non-payers quickly.
4. Priority, longevity, etc.

The pool can re-validate transactions at any point in time and drop the ones that became stale or are no longer valid (due to balance or nonce change).

Block production and block execution does not call validate_transaction at all, cause it would be too wasteful to do that. Instead it relies on apply_extrinsic and this means that all this basic verification (+ some additional checks) need to happen there as well.

[kianenigma]

@tomusdrw this needs re-visiting with the new transaction pre_dispatch and validate.

The issue of not using UnsignedValidator for signed transaction exists; we still don't use it in pre_dispatch_unsigned afaicr. If this is confirmed to be an issue, I can sort it out soon.

[tomusdrw]

It is confirmed, unless someone proves to me that UnsignedTransactions will never require any validation and the way it was used in imonline is just wrong (i.e. we verify the signature there).