Default Prepare/Process proposal to always ACCEPT

[kocubinski]

Summary

As described in #13453 (comment), an alternate default implementation for PrepareProposal is to ACCEPT all transactions returned from the mempool without any validity checking. Similarly ProcessProposal could do zero validation and return ACCEPT as well.

Problem Definition

The current behavior executes runTx in runTxModeCheck during 3 separate ABCI lifecycle methods (CheckTx, PrepareProposal, and ProcessProposal) which may be overly conservative. Note that validity here means "the full antehandler chain ran without error." Transaction messages are not executed.

Perhaps this conservative behavior is too restrictive for chains who may want to include invalid transactions which they've inserted into their mempool. The current implementation supports overriding ProcessProposal behavior but not Prepare.

Proposal

One or more of these items may fulfill this issue:

• Default behavior to ACCEPT always in Prepare/Process Proposal
• Instead of overriding PrepareProposal, allow chains to set a validity check which is executed in both Prepare and Process
• Allow chains to override PrepareProposal directly.

Regardless of which direction we take please take care to not lose test coverage. The current implementation provides both prepare and process proposal with a branched state store from the last commit. This is exercised in our tests and should continue to be even if we default to a no-op, stateless prepare/process proposal.

[alexanderbez]

Just some thoughts:

• The default ProcessProposal is just a default. No one is required to use it. You don't like it semantics? Use your own implementation.

an alternate default implementation for PrepareProposal is to ACCEPT all transactions returned from the mempool without any validity checking

The are two reasons why we execute the AnteHandler flow during PrepareProposal:

1. Tx could've been auto-injected on Insert during CheckTx which could be invalid
2. The checks are lightweight and ensures invalid txs are not filling proposals/blocks

Similarly ProcessProposal could do zero validation and return ACCEPT as well.

You can write your own ProcessProposal. We encourage apps to do so. There are cases where the default REJECTS, such as the inability to decode a tx blob -- this should indicate something is very wrong.

Perhaps this conservative behavior is too restrictive for chains who may want to include invalid transactions which they've inserted into their mempool.

Why? This should be punishable behavior. Why are you inserting invalid txs into proposals?

However, is there is a strong desire from app devs to not run the AnteHandler flow during ProcessProposal or to even make it completely custom, we're open to that. We'll listen to the community's needs. Let's release an RC and see what chain devs think.

@prettymuchbryce

[prettymuchbryce]

My suggestion for ProcessProposal was merely a tactical suggestion to simplify the initial default behavior, and de-risk the upgrade for existing chains that plan to use the default behavior. Clearly the lightweight ProcessProposal logic performed by Tendermint that exists in Cosmos SDK v0.46 works, right? Or are there chains that are experiencing some problems solved by this new ProcessProposal default behavior? You would know better than me. In any case this doesn't impact us at all as we plan to use our own (via SetProcessProposal), so do whatever you think is best!

As far as PrepareProposal goes, we'd prefer to do any validation ourselves. We trust that our in-App mempool implementation will be correct and well-tested. Therefore we don't need you to run every transaction through the Antehandlers on our behalf a second time and suffer the performance implications that I described here. Let me know if I can clarify further.

Also, definitely always appreciate you all hearing out my suggestions. We understand we are just one user of Cosmos SDK, and our needs my not be reflective of the rest of the community. Still, I figure it's worth voicing my feedback so you have the most information possible to decide what works best for you and the SDK.

[alexanderbez]

My suggestion for ProcessProposal was merely a tactical suggestion to simplify the initial default behavior, and de-risk the upgrade for existing chains that plan to use the default behavior.

How is it de-risking? The current behavior is correct (we synced with the TM team on this).

Clearly the lightweight ProcessProposal logic performed by Tendermint that exists in Cosmos SDK v0.46 works, right? Or are there chains that are experiencing some problems solved by this new ProcessProposal default behavior?

Note sure what you mean? What problem exists? IIRC, we only REJECT when we cannot decode the tx blob.

As far as PrepareProposal goes, we'd prefer to do any validation ourselves. We trust that our in-App mempool implementation will be correct and well-tested

Recall, you define the AnteHandler and you're also aware of the execution context. So you can do whatever you want in that chain.

[prettymuchbryce]

Recall, you define the AnteHandler and you're also aware of the execution context. So you can do whatever you want in that chain.

That's true. I did not consider that initially. I think this effectively solves the problem as we can skip the entire AnteHandler chain during PrepareProposal by modifying the default AnteHandlers. Thanks @alexanderbez!