Add guidelines for ValidateBasic

[robert-zaremba]

Summary

We don't have a clear guidelines about the ValidateBasic. We should have a good recommendation how to use it.

Problem Definition

ValidateBasic is happening during the CheckTx phase and it doesn't have access to the state. So it can verify only the current object. For complex objects, it can be an intensive operation.

Proposal

In my opinion the ValidateBasic should be strictly minimum to allow the chain charge a gas. So we shouldn't verify all fields there, but only minimum to allow running a transaction and charge user if he is not creating a correct transaction. The reason is that it costs a CPU to do ValidateBasic and in an extreme case, an attacker could DDOS the chain.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[amaury1093]

In my opinion, ValidateBasic should be as strict as possible.

Maybe we can charge a small constant fee in ValidateBasicMiddleware, so that it's not free computation.

[alpe]

I think it would make sense to view this with regards to the different processes to come up with a good plan.
A) mempool checkTX
B) mempool re-checkTX
C) DeliverTX
D) Simulation queries

A)+B)+D) are free of charge for the caller. C) fees depends on the min gas fee set on the node and can be 0.

[robert-zaremba]

For module design I have the following pattern:

• msg.ValidateBasic() -- minimalistic, because it's called in CheckTx and user doesn't pay any gas if it fails.
• msg.Validate(...) -- possible stateful, all strict validation, called at the beginning of Msg RPC method. Moreover we can charge extra fee here.

[aaronc]

• msg.Validate(...) -- possible stateful, all strict validation, called at the beginning of Msg RPC method. Moreover we can charge extra fee here.

We can't really make an interface method stateful because it has no access to state. Also I'd like to move away from interface methods to handlers (#10368) and I think this should be included in documentated guidelines. I do think we should allow modules to have stateful CheckTx handlers for messages, however, which I think accomplishes what you intend here.

Also, msg.Validate() could be another stateless method checked in CheckTx and generated by https://github.com/envoyproxy/protoc-gen-validate

[robert-zaremba]

We can't really make an interface method stateful because it has no access to state.

Here, msg.Validate(...) is not part of any interface. It is called manually and it's argument may vary (depending on the msg type), eg: msgA.Validate(ctx.BlockTime(), msgB.Validate(otherObject), where otherObject` is a relate object from the state.

[sunnya97]

The nice thing about msg.ValidateBasic is that it can be run by a client with no access to the state itself, such as a standalone CLI.

[aaronc]

We can't really make an interface method stateful because it has no access to state.

Here, msg.Validate(...) is not part of any interface. It is called manually and it's argument may vary (depending on the msg type), eg: msgA.Validate(ctx.BlockTime(), msgB.Validate(otherObject), where otherObject` is a relate object from the state.

Okay makes sense. It still would be better to do that with a handler than an instance method IMHO because of the interface/api module issues

[robert-zaremba]

It still would be better to do that with a handler than an instance method

OK, this will follow the new app wiring directions. The main thing to agree though is the ValidateBasic guideline.

[alexanderbez]

What is the exact proposal on ValidateBasic guidelines? Applications are able to implement these checks however they wish.

[robert-zaremba]

@alexanderbez this is an interface method, so we should document the best practice. Like I wrote above, doing too much in ValidateBasic is bad from economic perspective.

[robert-zaremba]

I like @AmauryM idea of adding / updating middleware for charging a small fee for ValidateBasic -> let's move it to other issue: #10984

[alexanderbez]

I'm not sure I agree with that perspective. I agree with @AmauryM's original post. ValidateBasic should check every possible (stateless) thing, i.e. it should be strict.

[robert-zaremba]

Then it should be called ValidateAllStateless ;)
@alexanderbez Why do you think we should do non trivial operations which are not needed in the to pass the minimum middleware set?

In Ethereum, only a strict minimum is checked on the mempool level and as much as possible is passed to the user - which is happening during tx execution.

[alexanderbez]

I don't think it matters really what you call it, the point is the function does as many checks as possible to ensure the message is "valid". And I would say they are needed. Anything that could make a message invalid (which doesn't depend on state), should be checked IMO.

[aaronc]

So generally checking fields and anything in memory is going to be cheaper than any sort of stateful processing. The Basic here refers to stateless as I understand it. Maybe it's a misnomer, but sounds like the guidance should be as strict as possible but fully stateless.

[robert-zaremba]

My problem with "as strict as possible" is that even some stateless operations may have significant cost (attribute decompression, some math, proof verification) and user should pay for it to avoid network spamming. In the PR I created, I can update the language in the guideline to:

Hence, we should only do the most necessary stateless sanity checks to enable middleware operations (eg: parsing the required signer accounts to validate a signature by a middleware) or stateless cheap operation not impacting the CheckTx phase performance.

How about that?