Add extrinsic weight and lenght check

[boundless-forest]

As call.validate_self_contained(info) does not contain any extrinsic length and weight checks, the transaction's max_gas_limit is not restricted and can be higher than the block gas limit. Add this check to the fp-self-contained scope together with Signed and Unsigned.

[boundless-forest]

@sorpaas Please take a reivew?

[boundless-forest]

@tgmichel @koushiro Any feedback about this?

[tgmichel]

Thank you for taking a look at this. I've been thinking about this change and here is why I don't think is necessary - at least not like this.

• SelfContained call performs it's own validation on the nonce, signature, sets the frame priority based on the priority fee etc So most of the Extra validation is already taken care of on the custom pallet-ethereum validation.
• The weight is checked with the pallet-ethereum::transact dispatchable weight hint.

Now, for the extrinsic length check. As far as I understand your proposal, CheckWeights::check_block_length will ExhaustResources if the extrinsic being validated exceeds the available block length on submission. I don't think we want that when submiting transactions, as users might send future transactions and even if it's not a future and users expect it to be included in the ready queue, it could perfectly fit in the next block, so rejecting on submission I don't think is expected behaviour.

However, we want to reject extrinsics which's lenght is greater than the block length upper bound. @sorpaas in our SelfContained validation we are not currently checking that (imagine a user sending a tx with huge input that exceeds the block length upper bound and that he could afford to eventually pay but is set as a future nonce), do you know if that is part of the transaction validity checks somewhere in sc-transaction-pool? I couldn't find it.

[sorpaas]

Substrate's extensions do not have support for extending custom validations, so I think the approach in this PR is the simplest way to fix the problem.

We should probably add a note in the docs that while self-contained does not behave exactly like unsigned, it does call unsigned validation functions.

[sorpaas]

I read the rest of the self-contained logic and I think this will add too much confusion. I'd prefer if we just duplicate some code, which in this case will actually be cleaner.

Please instead add the weight checking directly to validate_self_contained. All code needed can be copied from the CheckWeight struct's do_validate function.

[boundless-forest]

I read the rest of the self-contained logic and I think this will add too much confusion. I'd prefer if we just duplicate some code, which in this case will actually be cleaner.

Make sense. It seems a bit weird to add unsigned transaction logic under SelfContained validation. It must be noted if used in this way.

Please instead add the weight checking directly to validate_self_contained. All code needed can be copied from the CheckWeight struct's do_validate function.

Do you mean something like this? We might add another runtime parameter to the CheckedExtrinsic structure, such as CheckedExtrinsic<AccountId, Call, Extra, SelfContainedSignedInfo, Runtime>. Is that expected, or can we achieve this in another way?

CheckedSignature::SelfContained(signed_info) => {
    // let unsigned_validation = Extra::validate_unsigned(&self.function, info, len)?;
    let weight_validation = CheckWeight::<Runtime>::do_validate(info, len)?;

    let self_contained_validation =
        self.function.validate_self_contained(&signed_info).ok_or(
            TransactionValidityError::Invalid(InvalidTransaction::BadProof),
        )??;
    Ok(weight_validation.combine_with(self_contained_validation))
}

[sorpaas]

I was thinking about putting it here: https://github.com/paritytech/frontier/blob/master/frame/ethereum/src/lib.rs#L151

You may want to add it to pre_dispatch_self_contained as well.

[boundless-forest]

@sorpaas Please take a review again.