Non-zero return code from DeliverTx causes endless Tx processing loop in v0.31.6

[re7eal]

Tendermint version (use tendermint version or git rev-parse --verify HEAD if installed from source): v0.31.6

ABCI app (name for built-in, URL for self-written if it's publicly available): https://github.com/ndidplatform/smart-contract
It should also be reproducible with built-in or any ABCI apps.

Environment:

• OS (e.g. from /etc/os-release): Linux, macOS
• Install tools:
• Others:

What happened:
Txs that are not returned with code 0 in DeliverTx loop back for processing again creating an endless loop of new blocks with the same Txs.
This behavior is caused by a bug fix [mempool] #3322 When a block is committed, only remove committed txs from the mempool that were valid (ie. ResponseDeliverTx.Code == 0).

What you expected to happen:
Non-zero return from DeliverTx should somehow remove that tx from mempool. Maybe leave it as ABCI app developer responsibility?

Have you tried the latest version: yes

How to reproduce it (as minimally and precisely as possible):
Return non-zero code in DeliverTx

Logs (paste a small part showing an error (< 10 lines) or link a pastebin, gist, etc. containing more of the log file):

Config (you can paste only the changes you've made):

node command runtime flags:

/dump_consensus_state output for consensus bugs

Anything else we need to know:
The documentation (https://tendermint.com/docs/app-dev/abci-spec.html#message-types) states that SetOption, Query, CheckTx, DeliverTx should return an application-specific response Code uint32. Putting a self defined response code as a returned Code uint32 whenever there is an error in business logic when going through DeliverTx should somehow has a way to remove that Tx from mempool.
If that is not the case then; When should an ABCI return non-zero code and when should it return 0? If ABCI needs to return 0 even when there's an error in business logic then where should I put an error code for client app?

[melekes]

Ethan: "The attack described in #3322 might just be unavoidable. Perhaps when Code !=0 we just need to also remove from the cache and make it the users responsibility to resubmit"

Me: "I don't like putting too much on the client mostly because we don't specify requirements & expectations for a client anywhere + our own http client will fail in the above case. Other option may be a new code (saying "this tx will never be valid") and exponential retry instead of simple Reap, but that would require a lot of work. I will revert the fix."

[ebuchman]

Merged #3701 to develop and will cut v0.37.1 today (nice coincidence in numbers there!).

Let's move discussion about this to the original issue - #3322

Thanks for flagging it @re7eal !