How do we reason about the interaction of this mutex with updateMtx, as well as the client mutex? Is there perhaps a relatively easy way of visualizing it, given the complexity of the callback system?

Yeah, it's not easy to reason about all the mutexes and I was not happy to add a new one. At least we know we plan to eliminate the variables recheckCursor and recheckEnd in #895, which will also remove the new recheckMtx mutex.

In the meantime, below is a list of all the places where these mutexes are locked. I still have to find a good way to visualize their interactions. After a first analysis I can say that updateMtx and recheckMtx could be both locked simultaneously during a brief time in the recheckTxs function, called at the end of Update. This can make globalCb to wait until recheckMtx is released, but this doesn't lead to a deadlock. CheckTx is already blocked for being executed during Update because Update has updateMtx taken. And I don't see how localClient's mutex can interfere with the other two.

CListMempool.updateMtx

updateMtx is used by the following CListMempool methods:

• CheckTx
  • updateMtx is locked during the whole execution
  • calls CheckTxAsync, which locks localClient 's mutex
  • then calls ReqRes.SetCallback, which locks it's own mutex
• Lock and Unlock
  • These methods are only used to lock updateMtx during the whole execution of state.execution.Commit, which calls, in order:
    • mempool.FlushAppConn
    • mempool.Update (see below)

updateMtx is also locked during the whole execution of:

• ReapMaxBytesMaxGas
• ReapMaxTxs
• Flush

CListMempool.recheckMtx

recheckMtx is locked during the whole execution of:

• initRecheckCursors, called at the beginning of recheckTxs, which is called at the end of Update.
• resCbRecheck, when processing CheckTx responses of type Recheck.
• recheckCursorIsNil, called:
  • at the beginning of globalCb
  • at the beginning of the callback function returned by reqResCb, though it's actually not really needed here.

localClient's mtx

There are two methods locking the mutex that are relevant for the mempool:

• SetResponseCallback
  • used to set CListMempool.globalCb in CListMempool's constructor
• CheckTxAsync, which is called by:
  - mempool.CheckTx
  - mempool.recheckTxs (after calling initRecheckCursors)

I am not sure to understand why a new mutex is actually needed. Is it be possible, please, to have an example of bad interleaving? As far as I know, asynchronous checks are executed one at a time, whatever the client is, so this should fine.

If additional concurrency control is required, I would opt for using the already existing CListMempool.updateMtx, and not adding a new one. The rationale is that re-checks do change the fields of the mempool. Hence, they should be guarded under the same mutex as other mempool methods. As discussed here, this requires to make the mutex reentrant.

I have tried only with updateMtx but it doesn't work. Btw, the recheck logic only changes the mempool with RemoveTxByKey, which is currently not guarded by any mutex. RemoveTxByKey is called by Update, which holds updateMtx, and by resCbRecheck that doesn't lock. We could just use updateMtx to guard RemoveTxByKey only if it is reentrant, as you said.

I added a new test TestMempoolRecheckRace that shows more clearly the problem. The test is:

1. add a bunch of transactions to the mempool
2. update one transaction to force rechecking the rest
3. add again one transaction → this will fail with a data race for the variable recheckCursor, which is accessed from the functions reqResCb and recheckTxs.

(This new test actually a simplified version of TestMempoolNoCacheOverflow, so it's a bit redundant and we can merge them.)

I am not sure to understand why a new mutex is actually needed. Is it be possible, please, to have an example of bad interleaving? As far as I know, asynchronous checks are executed one at a time, whatever the client is, so this should fine.

If additional concurrency control is required, I would opt for using the already existing CListMempool.updateMtx, and not adding a new one. The rationale is that re-checks do change the fields of the mempool. Hence, they should be guarded under the same mutex as other mempool methods. As discussed here, this requires to make the mutex reentrant.