Vision: Add validation syntax for extrinsics enforced on transactions

[apopiak]

Substrate could provide some syntax to move (or copy) simple ensure! checks into transaction validation "automatically"/very easily.

e.g. with this mock syntax:

#[weight = T::WeightInfo::bid()]
#[validate(value > 0)]
pub fn bid(origin, id: T::AuctionId, #[compact] value: T::Balance) { }

or complementarily:

#[weight = T::WeightInfo::bid()]
#[validate(ensure!(value > 0, "bids need to be higher than 0"))]
pub fn bid(origin, id: T::AuctionId, #[compact] value: T::Balance) { }

Related to paritytech/polkadot-sdk#349 (could maybe be a way of implementing it?) Depends on paritytech/substrate#5678

[kianenigma]

Essentially, your domain of access in #[constraint = ...] is same as weight: You can only read transaction inputs and other data that is known prior to the execution (i.e. is static).

Also, I think the name can be better and is encouraging the user to "you don't need to understand this, it is magic". I prefer making sure the user understand what they are doing: #[validate = ..] fits well with the fact that the pool will validate this (using a runtime api called validate_transaction), but is probably ambiguous to someone unfamiliar. #[pool_validate = ..] might be better.

[bkchr]

While the general idea is nice. I'm not sure we really should provide an easy way for this.

The problem is that no one is gone pay for this. These checks should always be very lightweight, which we can not ensure as the user could do anything in there. Normally when we reject such stuff on chain, the sender pays for it. Now, no one is gonna pay for it.

If we implement this, we should do a lot of education around this and what is bad etc!

[xlc]

I think we can already achieve this via SignedExtension so this will just be a syntax sugar.

Another question is when does the validation happen? Before entering tx pool? During block production? After tx inclusion?

We have dry run RPC so we can use it to prevent people from submitting bad tx already.

So unless the plan is to also expose the constraint via metadata, I don't really see why we need this. We already have enough macro magics.

[bkchr]

As you already said, this is just syntactic sugar around SignedExtension and the validation always happens before entering the pool and before applying it on chain.

[kianenigma]

Another question is when does the validation happen? Before entering tx pool? During block production? After tx inclusion?

I don't think the runtime can and should specify that. The runtime provides the cheap checks via validate_transaction api, and when the client/pool calls that is up to them.

[apopiak]

While the general idea is nice. I'm not sure we really should provide an easy way for this.

The problem is that no one is gone pay for this. These checks should always be very lightweight, which we can not ensure as the user could do anything in there. Normally when we reject such stuff on chain, the sender pays for it. Now, no one is gonna pay for it.

If we implement this, we should do a lot of education around this and what is bad etc!

Sure, we will need to educate, but as mentioned by others we need to do that for ValidateUnsigned and SignedExtensions as well.

[bkchr]

Yes, but there is a difference between putting the shotgun on the table and putting them into some locker :P :D

[apopiak]

I wonder whether we disagree on the tradeoff between pruning txs before they enter the tx pool (which this would make easier IIUC) vs making sure that attackers pay for traffic they cause.

[bkchr]

making sure that attackers pay for traffic they cause. is directly related to pruning txs before they enter the tx pool

If I can send you tons of transactions that you reject before they enter the pool, you still pay the validation price and if there is some heavy computation hidden in this validation, it doesn't end good for you.

We already have seen that a lot struggled with custom SignedExtensions and transaction not being removed properly from the pool and whatever.

Let's take your example:

#[weight = T::WeightInfo::bid()]
#[validate(value > 0)]
pub fn bid(origin, id: T::AuctionId, #[compact] value: T::Balance) { }

Why should an UI send a value which is zero, when we know that zero isn't accepted? Correct, a good behaving UI wouldn't do this. However, when I'm trying to do be evil and sending transactions manually I would start sending with a value of zero. In my opinion you should pay for this, because this is misbehavior.

[tomusdrw]

I feel there are two separate issues here:

1. Making it easier to write and include pallet-specific SignedExtensions (related paritytech/substrate#3419 paritytech/substrate#5006).
2. Making it easier to express some extra requirements of particular dispatchables and verify them in validate_transaction instead of dispatch phase.

While I'm a huge fan of 1. (see below) for some more details, I don't like 2:

• it makes analysing the code harder, cause some assumptions are not expressed as part of the function body, but the metadata (kind of like solidity modifiers which imho are terrible).
• it enables the footgun of opening yourself up for a DoS attack. On the first glance it seems "better" to express the conditions as metadata, but since it's not possible to move all of them (due to DoS) you end up with a really unintuitive split. Doing too much in the pre-checks becomes super easy and I don't see any direct gains - Agree with Basti that this is mostly the client / UX issue, not the core responsibility.

Ad.1: I think this could be achieved with a VerifierSignedExtension<AllModules> composable signed extensions, where the condition would be to have all the auto-included pallet-specific extensions type AdditionalSigned = ();

[apopiak]

I agree that this opens the nodes to risk of DoS if the checks are too heavy. What you don't consider, IMO, is that the nodes will have to perform more work in the case where the extrinsic fails on an ensure inside of the function body. The transaction will be propagated through the network and clog up the transaction pool without contributing in any useful way to the state. Yes, the validators get payed for it, but that is because relevant network resources are used for it. If the transaction were rejected before going into the pool, the network would spend fewer resources on it.

[apopiak]

As a polemic example: You won't propagate a transaction whose encoding is wrong or which is not signed correctly. So, there will always be transactions that are rejected by the node. I think it should be up to the runtime developer to decide where/when the checks should happen.

[kianenigma]

I agree that this opens the nodes to risk of DoS if the checks are too heavy.

What you don't consider, IMO, is that the nodes will have to perform more work in the case where the extrinsic fails on an ensure inside of the function body. The transaction will be propagated through the network and clog up the transaction pool without contributing in any useful way to the state. Yes, the validators get payed for it, but that is because relevant network resources are used for it.

If the transaction were rejected before going into the pool, the network would spend fewer resources on it.

This is sensible from an engineering pov, but not economically. Validators would love to include lots of transactions that would fail on ensure, as they pay the full fee with a bit less work.

[tomusdrw]

If the transaction were rejected before going into the pool, the network would spend fewer resources on it.

The network in overall? - perhaps. A single node that receives such transactions? - not really.

You won't propagate a transaction whose encoding is wrong or which is not signed correctly. So, there will always be transactions that are rejected by the node.

Yes, we do draw a line at some point, but do it extremely carefully (on the safe side) to prevent DoS attacks.

I think it should be up to the runtime developer to decide where/when the checks should happen.

And it is via SignedExtensions. The way I read your proposal is that you want to introduce something that seamingly looks like a "good practice" or a "favoured pattern" - i.e. encourage developers to move their ensure statements to "metadata checks" justifying it by having the network do less work in the happy case. I.e. prioritize "network efficiency" / developer friendliness over security. I feel this isn't the right approach, cause it's too easy to shoot yourself in a foot with this one - we don't really have any tooling which would help you decide which checks are fine to be before entering the pool and which are not, also we don't have tooling that makes sure this doesn't get broken over time (for instance refactoring and accidental extra complexity). IMHO we should make all things possible, but we can also favour patterns that are known to be safe (experienced devs can optimize where it really matters in such case by introducing a SignedExtension).

[gui1117]

I think instead on focusing on way to express more logic to reject extrinsic when validating them in the transaction. Logic which is not economically profitable for the block producer, and might open DoS attack to the same block producer.

We should rather focus on making those checks easily identifiable to the UI so that they never send such invalid transaction, probably using metadata. Similar to what is proposed in https://github.com/paritytech/polkadot-sdk/issues/349

[tomusdrw]

We should rather focus on making those checks easily identifiable to the UI so that they never send such invalid transaction, probably using metadata. Similar to what is proposed in paritytech/polkadot-sdk#349

:+1: FWIW we have a pretty expressive Rust type system at our disposal. As an example: maybe instead of having u64 we can simply have NonZeroU64 as the input type and this is immediately (with no extra changes) available to UI via Metadata.

[apopiak]

If the transaction were rejected before going into the pool, the network would spend fewer resources on it.

This is sensible from an engineering pov, but not economically. Validators would love to include lots of transactions that would fail on ensure, as they pay the full fee with a bit less work.

But we already provide means for free transactions via unsigned transactions and also Pays::No, so we have to grapple with these questions and cannot just defer to "oh validators love to include faulty transactions for fees".

[stale[bot]]

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.

[kianenigma]

would be a nice luxury to have someday maybe.

[kianenigma]

interestingly, flies in the face of https://github.com/paritytech/polkadot-sdk/issues/32