Avoid locking CTxMemPool::cs recursively in CTxMemPool::DynamicMemoryUsage() #19901

1. 
   hebasto commented at 6:22 pm on September 6, 2020: member

   This is another step to transit CTxMemPool::cs from RecursiveMutex to Mutex. Split out from #19306. (Along with #19652, #19854 and #19872 it is more than 2/3 of the way to the final goal).

   Thread safety annotations, lock assertions, and required explicit locking added. No behavior change.

   Please note that now, since #19668 has been merged, it is safe to apply AssertLockHeld() macros as they do not swallow compile time Thread Safety Analysis warnings.

2. refactor: Pass mempool memory usage to GetCoinsCacheSizeState() b581452b0e
3. refactor: Pass mempool memory usage to FlushStateToDisk() 3e4188e298
4. refactor: CTxMemPool::DynamicMemoryUsage() requires CTxMemPool::cs lock

   No change in behavior, some call sites already held the lock and a lock
   is added in the remaining sites.

   04515fc0f7
5. 
   MarcoFalke commented at 6:32 pm on September 6, 2020: member

   Looks like this is adding a lot of code (complexity). Not sure if that is worth it.
6. DrahtBot added the label Mempool on Sep 6, 2020
7. DrahtBot added the label P2P on Sep 6, 2020
8. DrahtBot added the label Validation on Sep 6, 2020
9. 
   hebasto commented at 6:43 pm on September 6, 2020: member

   Looks like this is adding a lot of code (complexity). Not sure if that is worth it.

   Since the memory unused by the mempool could be used by the coinscache, CTxMemPool::DynamicMemoryUsage() is buried in CChainState::GetCoinsCacheSizeState(). The code could be much simpler if every entity will have its own memory limit.

10. 
    hebasto commented at 6:51 pm on September 6, 2020: member

    @MarcoFalke

    Not sure if that is worth it.

    http://www.erisian.com.au/bitcoin-core-dev/log-2020-06-18.html#l-16:

    <sipa> recursive mutexes are evil

11. MarcoFalke removed the label Mempool on Sep 6, 2020
12. MarcoFalke removed the label P2P on Sep 6, 2020
13. MarcoFalke removed the label Validation on Sep 6, 2020
14. MarcoFalke added the label Refactoring on Sep 6, 2020
15. 
    DrahtBot commented at 9:54 pm on October 15, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20494 (refactor: Move node and wallet code out of src/interfaces by ryanofsky)
    • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
    • #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
    • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

16. DrahtBot added the label Needs rebase on Dec 1, 2020
17. 
    DrahtBot commented at 9:43 am on December 1, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

18. hebasto closed this on Aug 24, 2021

    ---

19. DrahtBot locked this on Aug 24, 2022

Contributors
hebasto MarcoFalke DrahtBot

Labels
Refactoring Needs rebase