feat: concurrent checkTx

[jinsan-line]

Related with: https://github.com/line/link/issues/1151

Description

To optimize performance, we need to increase concurrency. As a first step for it, I implement concurrent checkTx. The key change is to remove app.mtx.Lock() from the abci local client. An application, as an abci server, is better to protect itself from concurrency than an abci client. W/ current implementation, the abci local client protects an abci server but it decreases concurrency.

CONTRACT:

• Now, an application should protect itself from concurrent checkTx as an abci server that means it should be thread safe.

We'll also increase concurrency for other abci methods as much as possible in the future.

Please note how CounterApplication is changed for protecting itself from concurrent checkTx. It might be an example. I'll revise cosmos-sdk BaseApplication as well after got approval for this PR.

New ABCI methods:

• BeginRecheckTx
• EndRecheckTx

W/ current implementation, Commit assumes mempool is locked. In cosmos-sdk, BaseApplication needs this assumption only for SetCheckState() to sync up CheckState with DeliverState. Because Commit usually takes long time (about 500ms~1s), mempool is locked too long. SetCheckState() is not needed to be executed during Committing but only between Commit and rechecks. So at first I've added SetCheckState abci method but I thought it has too specific purpose because it's a part of abci api. I renamed it to BeginRecheckTx for general purpose and also add EndRecheckTx for symmetric manner. I think it is also good in term of comparing BeginBlock and EndBlock.

Please note that, in CounterApplication BeginRecheckTx, it syncs up mempoolTxCount (CheckState) with txCount (DeliverState). It's moved from Commit.

---

For contributor use:

• Wrote tests
• Updated CHANGELOG_PENDING.md
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Updated relevant documentation (docs/) and code comments
• Re-reviewed Files changed in the Github PR explorer

[jinsan-line]

Key change to remove a mutex.

[jinsan-line]

Key change to remove a mutex.

[tnasu]

Please note how CounterApplication is changed for protecting itself from concurrent checkTx. It might be an example. I'll revise cosmos-sdk BaseApplication as well after got approval for this PR.

I got it. Do you change it like a below in cosmos-sdk BaseApplication? It means you might revise to move mutex-lock into the Application side.

func NewCounterApplication() *CounterApplication {
 @@ -225,6 +235,8 @@ func (app *CounterApplication) DeliverTx(req abci.RequestDeliverTx) abci.Respons

 func (app *CounterApplication) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
 	txValue := txAsUint64(req.Tx)
 	app.mempoolTxCountMtx.Lock()
 	defer app.mempoolTxCountMtx.Unlock()
 	if txValue != uint64(app.mempoolTxCount) {
 		return abci.ResponseCheckTx{
 			Code: code.CodeTypeBadNonce,
 @@ -234,14 +246,20 @@ func (app *CounterApplication) CheckTx(req abci.RequestCheckTx) abci.ResponseChe
 	return abci.ResponseCheckTx{Code: code.CodeTypeOK}
 }

 func (app *CounterApplication) BeginRecheckTx(abci.RequestBeginRecheckTx) abci.ResponseBeginRecheckTx {
 	app.mempoolTxCountMtx.Lock()
 	defer app.mempoolTxCountMtx.Unlock()
 	app.mempoolTxCount = app.txCount
 	return abci.ResponseBeginRecheckTx{Code: code.CodeTypeOK}
 }

 func txAsUint64(tx []byte) uint64 {

[jinsan-line]

@tnasu

Do you change it like a below in cosmos-sdk BaseApplication? It means you might revise to move mutex-lock into the Application side.

Actually, not. I have little bit different idea how to revise cosmos-sdk BaseApplication unlike CounterApplication. CounterApplication just shows that an application should protect itself from concurrent checking tx. CounterApplication has a counter and this counter is a critical section. So mutex is common way to protect it.

Otherwise, CheckTx in cosmos-sdk BaseApplication is actually already thread safe. That's the reason why I maintain that server is better to protect itself than client. The only thing, BaseApplication should protect, is sequence of accounts. We need to serialize checkings from the same account. If you're interested how to implement it, I'll request a review to you. (I'm already working on it)

Thanks,

[egonspace]

Do you mean you would move setCheckState from Commit abci to BeginRecheckTx which is called before recheck?

Let me figure out the structure of current implementation

tendermint.Commit() {
  mempool.lock()
  app.commit() --> {
    setCheckState()
  }
  mempool.Update() --> {
    mempool.recheck()
  }
  mempool.unlock()
}

If I understand your intentions well, is it right to change as below?

tendermint.Commit() {
  mempool.lock()
  app.commit()
  mempool.Update() --> {
    app.BeforeRecheck() --> {
      setCheckState()
    }
    mempool.recheck()
    app.AfterRecheck()
  }
  mempool.unlock()
}

What I'm curious about is exactly what moving the setCheckState has to do with the long lock-time of the Commit(). Isn't it the same lock time since you're moving it inside the tendermint.Commit anyway?

[jinsan-line]

@egonspace

What I'm curious about is exactly what moving the setCheckState has to do with the long lock-time of the Commit(). Isn't it the same lock time since you're moving it inside the tendermint.Commit anyway?

Please note that mempool.Lock() is relocated just before mempool.Update() in execution.go.

https://github.com/line/tendermint/blob/469eaafcaaba2e0f7c4749aeb70733747d9e5a26/state/execution.go#L238-L243

[egonspace]

I didn't see that. The explanation makes me understand the code. I'd like to suggest two things.

1. I don't like having to pass the block header again when we call BeforeRecheckTxSync.
   Wouldn't this be possible?

• remove app.deliverState = nil from BaseApp.Commit()
• and then we may write BeforeRecheckTxSync as following

header := app.deliverState.ctx.BlockHeader()
app.setCheckState(header)
app.deliverState = nil

2. It is also related to number 1, but from the app's point of view, the recheck process does not seem to be an important logic in the procedure. Wouldn't it be better to understand it as a PostCommit process? How about to make abci with CommitSync() and PostCommit() instead of BeforeRecheck and EndRecheck?
   Wouldn't it be better to add a commit-related abci because we might think that commit process is handled in two stages?

[jinsan-line]

@egonspace

I don't like having to pass the block header again.

Please could you share the reason why you don't like it? I think we need to reset deliverState at Commit but I'll check it again.

PostCommit() instead of BeforeRecheck and EndRecheck

I think BeginEnd/Recheck is better because it has explicit meaning. I think PostCommit has little ambiguous meaning. In abci server perspective, how could abci server decide what to do for Commit and PostCommit each?

[egonspace]

@jinsan-line

Please could you share the reason why you don't like it? I think we need to reset deliverState at Commit but I'll check it again.

As mentioned above, what we're trying to do is divide what Commit() did into two functions. Two tasks require continuity in input. In xxx, when the name is Commit(), xxx(), it is to reset the checkState from the header that was last committed. I think it's better to use header it has for the following reasons.

• for data consistency with Commit() and xxx()
• we may not rely on tendermint.
• we can completely rule out the possibility of getting the wrong header.

Of course, we'll have to go over whether this is possible...

I think BeginEnd/Recheck is better because it has explicit meaning. I think PostCommit is little ambiguous meaning. In abci server perspective, what should abci server do for Commit and PostCommit each?

If you like explicit names, wouldn't a name like ClearCheckState be better? From the app's perspective, the start and end of the recheck do not seem to be important. Because what we want to do with the app is to reset the checkState (I think the deliverState can also be reset here), I would rather have an explicit name associated with this work. And we don't require EndRecheckTx now.

I personally don't prefer to argue with the name of the function, but I would like to gather a lot of thoughts and decide because it's hard to change the name of abci once we decide it. @tnasu , @torao I hope the consensus team gives us a lot of opinions.

[jinsan-line]

@egonspace

for data consistency with Commit() and xxx()

The consistency should be considered between BeginRecheck and Recheck but not between Commit and BeginRecheck. Tendermint, as an abci client and the mempool owner, has ownership for rechecking context. I still think it's better to pass Header as it is.

what we're trying to do is divide what Commit() did into two functions.

I think we're trying to revise ABCI but not just divide a specific an api into two parts. I think we need to revise interface even though, at first, this issue is initiated due to specific reason to call SetCheckState() in Commit(). When I think why SetCheckState() should be called during Commit, I get a personal conclusion that there is no proper phase (api) to call it. Because this phase just prepare for rechecking, I think Begin/EndRecheck is good for it.

If you like explicit names, wouldn't a name like ClearCheckState be better?

Please note my PR description. I also considered the specific name, SetCheckState(), at first. But, IMHO, I still think it's not proper for api.

So at first I've added SetCheckState abci method but I thought it has too specific purpose because it's a part of abci api. I renamed it to BeginRecheckTx for general purpose and also add EndRecheckTx for symmetric manner. I think it is also good in term of comparing BeginBlock and EndBlock.

it's hard to change the name of abci once we decide it

I think we need general approach with the same reason..

[egonspace]

Your detailed explanation has led me to understand that resetting of CheckState is a necessary process for recheck, not for commit. I know you're absolutely right. Thank you.

[egonspace]

Good Job!

[wetcod]

LGTM

[torao]

About Mutex: As far as I'm looking at the source code, I think the reason for using Mutex.Lock() for all RPCs in localClient is to protect the Application -based implementations from their state conflicts, rather than to protect the server from concurrency. However, I think it's better for as framework to make the Application implementers decide whether it's necessary to protect their state since only the Application knows whether the RPC process will actually cause the state conflict. And if we need to protect them in localClient, it's better to use JobQueue/WorkerThreadPool pattern than the Mutex that blocks caller thread.

As a side note, I find it odd with the naming XxxAsync() for the blocking methods in localClient.

[torao]

I thought about the names XxxAsync() some more after I wrote the above comment, maybe the implementors of localClient chose these names because they plan to make them asynchronous in the future?

Right now, most of the XxxAsync() methods in localClient like the following template:

func (app *localClient) XxxAsync(req types.RequestXxx) *ReqRes {
  app.mutex.Lock()
  defer app.mutex.Unlock()
  res := app.Application.Xxx(req)
  return app.callback(types.ToRequestXxx(req), types.ToResponseToXxx(res))
}

I don't know if the goroutine can be used for this purpose, or what ThreadPool or Future features are best in golang, but, ideally, the end result should be a "correct" asynchronous API as follows:

// i.e., the maximum number of threads that can run RPC simultaneously from one client instance is four.
// Specifying 1 is the same as using `Mutex.Lock()` for `Appcation` implementors.
threadPool := NewThreadPool(max_thread_size=4)

func (app *localClient) XxxAsync(req types.RequestXxx) {
  threadPool.exec(func(){
    res := app.Application.Xxx(req)
    app.callback(types.ToRequestXxx(req), types.ToResponseToXxx(res))
  })
}

[torao]

Now, this fix is fine as it is, but we think it's necessary to adopt a uniform policy for the fixing of numbering for tags in ProtocolBuffers. For example, tags added by LINE Blockchain should start at some magic-number such as 15 (that fits into 1-byte in PB) or 1000 (human-readable), as is done in VRF and BLS signature aggregation.

[torao]

It won't have a big impact, but it's better to lock in an IIFE to minimize the scope of the lock and avoid unnecessary locking during the calculation of metrics.

err = func(){
  blockExec.mempool.Lock()
  defer blockExec.mempool.Unlock()
  return blockExec.mempool.Update(block, deliverTxResponses, TxPreCheck(state))
}()

[kukugi]

LGTM 👍

[jinsan-line]

@torao

for all RPCs in localClient is to protect the Application -based implementations from their state conflicts, rather than to protect the server from concurrency

Application is an abci server so I think it's the same protect the Application and protect the server.