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.
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
FvmExecStatewe'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 theFvmExecState; all other threads can wait.Logic issues
FvmExecStatemust beClone, and every thread must clone it before executing, and later dispose of their instance. Today, ifexec_in_checkis 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.