Deprecate mantlemint

[nghuyenthevinh2000]

For starter, I am new to the ecosystem so don't really understand why mantlemint exists in the first place.

When having a conversation with echelon , I am aware that mantlemint is created to fix some sort of tendermint query. I think it is related to this one: tendermint/tendermint#6048

There are many blocking for query in Tendermint due to usage of Mutex. The original terra team seemed to build Mantlemint to deal with this. It is something I am not very certain of. It would be great if terra-money team can shed some lights on this. Cc @Jared-TFL @javiersuweijie

If it is the case, I suggest to deprecate the usage of Mantlemint upon upgrade to CometBFT v37 because v35 has implemented the fix.

[echel0n-TR]

Doing a bit of research around this I could find these issue tickets
tendermint/tendermint#6122
cosmos/cosmos-sdk#8602
tendermint/tendermint#6048

[aeuser999]

Also to highlight that the last link that @echel0n-TR mentioned is directly applicable to Terra v1 in that TFL mainly commented about mantlemint in the issue (and the issue still appears to possibly not have been resolved in Tendermint).

That Tendermint Issue#6048 was closed, but it appears that the reason it was closed was due to this comment, which provided a commit that changed Mutex to RWMutex , and was pulled into this Tendermint PR#6306 and merged to main. It went for about two months, and then the author of the PR seemed to have had doubts, and reverted back in this Tendermint PR#7106 in which a commenter and the author mentioned:

I'm definitely fine reverting this but am a little sad we don't have better visibility into the problem. I think better metrics and improved testnets will hopefully help us be a bit more sure of problems like this in the future.

Agreed. And I think we can (maybe?) delete a lot of the locking of the clients in the future around ABCI++ and work with the RPC/client protocls.

So, the issue was really never resolved at the tendermint layer (at least due to Tendermint Issue#6048) in terms of the issues that Mantlemint looked to accomplish.

Just passing that along if helpful.

[echel0n-TR]

We've actually implemented the removal of the mutex's for ABCI client and found it to work rather well, you can see our changes in our custom branch of tendermint in our repo https://github.com/terra-rebels/tendermint/tree/v0.34.24-tr.0

[aeuser999]

The removal of Mutex may work for reading, but wouldn't you want a Mutex (Mutual Exclusion/Lock) in order to prevent corruption of data for functions where there can be a write (or where the read is used by a writing method that requires that no write has happened between the time of the read and the write it is about to commit)? It seems that RWMutex may be better than having no Mutex since it would hold the lock for a writer, but allow others to read without a lock. It seems like getting rid of Mutex all together may only be helpful for calls that have no bearing on state? Let me know though too if I am missing something in my thinking (which I would welcome - I just may be missing something in all this that is for sure)?

It seems like the comment thread in 6048, 6173, 5411, 6171, 6306, and 7106 at least regarding where Mutex is needed, is summarized by:

• melekes: "It may be possible to completely remove mtx from Info and Query calls."
• alexanderbez: "Curious why Info and Query need to be serialized to begin with?"
• hanjukim from TFL:

2. Changing the Mutex made some improvements. However,
3. You can only have one iterator at a time for accessing LevelDB which means it has to be synchronous and it blocks everything until it finishes.
4. There is a major blocker function in the staking module of Cosmos SDK: https://github.com/terra-project/cosmos-sdk/blob/v0.39.2/x/staking/keeper/delegation.go#L52-L65
5. Since changes in the KV changes the state, we couldn't find easy way to make it compatible without hard/soft forking. (i.e. making it able to query with height parameter)

• hanjukim submitted a PR with RWMutex changes, and melekes mentioned: "While I support the change, this could be breaking for some ABCI applications, so we need to document the requirements in https://github.com/tendermint/spec/blob/master/spec/abci/client-server.md when using the local client."
• hanjukim: "we found a problem from launchpad."
• alexanderbez: "what was the issue you found? Is it still possible to use a read-lock?"
• hanjukim: "It seems to work with terra-money/cosmos-sdk@facf3b1, but we were too worried to apply this in production. Since major slow downs are caused by IAVL, this can help but not significant impact as we expect."
• There was also this issue linked "Remove localClient.mtx"
• Which opened this other issue "Should ABCI applications deal with concurrency?"
• By the time it made it to issue 6171, alexanderbez mentioned: " it might be helpful to describe why all calls are serialized, specifically the ones that have no bearing on state mutations."
• In 6306, alexanderbez mentioned: "I think we only want to make Info and Query calls read-locked. Everything else should stay write-locked if I'm not mistaken?"
• tychoish: "Is there a reason for leaving calls that don't modify the client structs under write-locks?" (later in 7106 tychoish put in the revert PR)

But, that is promising to hear your development and testing in abci/client/local_client.go shows the Mutex locking may be part of the speed portion of the issues they meant to accomplish with mantlemint (or the issues the others talked through in the tendermint issues and PR's) :)

[echel0n-TR]

More then happy to implement the RWMutex fix as well and test it, we've seen no data corruption come from removing all the mutexes in the local client, but thats not to say it could not happen.