App::Commit competes with App::CheckTx for a lock over a cache + bad caching behaviour

[raulk]

Concurrency issues

This is poor concurrency design. Under certain lock starvation scenarios, it can prevent us from making consensus/chain progress.

Key principle in blockchain client design: never ever allow your consensus to be conditioned by untrusted external requests (mpool, sendRawTransaction, which invoke CheckTx); let alone for just a cache.

Instead, we should track the height of the FvmExecState we've cached, and in CheckTx, we should compare the consensus height with our cached height. If they diverged, the first CheckTx thread to notice must recycle the FvmExecState; all other threads can wait.

Logic issues

FvmExecState must be Clone, and every thread must clone it before executing, and later dispose of their instance. Today, if exec_in_check is enabled, CheckTx will execute the transaction and the resulting state tree will be the start state tree of the next CheckTx request, thus causing us to diverge from the chain state when checking transactions, until the next block is committed.