Wallet: breaking cs_wallet into more fine-grained locks #17667

Once #16426 merged, splitting our big cs_wallet into multiple ones would make sense because multiple RPCs won't be in conflict anymore to hold cs_main.

New locks like cs_mapWallet and cs_mapTxSpends, while handling well cases like we don't want to commit twice a coin to different transactions or return twice an address to avoid key reuse. Multiple "read" RPCs like listtransactions or listsinceblock should work smoothly concurrently without stopping each other.

I don't know exactly what is possible but I think there is room for improvements and I don't plan to do it so feel free to dig into it.

See logs https://bitcoincore.reviews/16426.html for more context.

There is some overlap here with the descriptor wallet project: https://github.com/bitcoin/bitcoin/projects/12, particularly 32f73f57450d4bab188d73c9d6a9e8060b75a7c7 from #17261 which starts to lock CWallet::cs_wallet and FillableSigningProvider::cs_KeyStore mutexes in different place.

Overall, I'm not so enthusiastic about adding more fine-grained mutexes to the wallet. I think you could probably get better performance and fewer bugs using a small number of mutexes (maybe even just one), but just being more careful to keep the mutex locked as briefly as possible, and doing blocking operations like database writes with the mutex unlocked.

That's just an overall inclination, though. There may be different wallet functions which don't need to interact with each other and really should break off and do their own synchronization.

Yes don't have spent that much time thinking on it, so don't have any strong inclination, just wanted to raise the awareness than if cs_main get removed it's now worthy to do work to improve performance, either way with more fine-grained locks or short-lived, just-in-time mutex holding

cs_main was removed from the wallet. You may open a new issue if there is anything else to be done here.

@MarcoFalke FYI, the idea is about introducing more fine-grained wallet data structures locks, it's not about removing cs_main. We still have hundreds of lock taking of cs_wallet (though I don't plan to work on that).

Ok, I guess it is fine to just implement this if there is need, but not need to keep an issue open for it?

I started working on decoupling some responsibilities from cs_wallet without knowing about this issue. PR #25620 (open discussion there)

Ok, I guess it is fine to just implement this if there is need, but not need to keep an issue open for it?

Yep no need to keep open. Nice we keep awareness as a project wallet refactors are welcome.