refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

1. 
   hebasto commented at 4:15 pm on June 17, 2020: member

   This PR replaces RecursiveMutex CTxMemPool::cs with Mutex CTxMemPool:cs.

   All of the related code branches are covered by appropriate lock assertions to insure that the mutex locking policy has not been changed by accident

   Related to #19303.

2. DrahtBot added the label GUI on Jun 17, 2020
3. DrahtBot added the label Mempool on Jun 17, 2020
4. DrahtBot added the label Mining on Jun 17, 2020
5. DrahtBot added the label P2P on Jun 17, 2020
6. DrahtBot added the label Refactoring on Jun 17, 2020
7. DrahtBot added the label RPC/REST/ZMQ on Jun 17, 2020
8. DrahtBot added the label Tests on Jun 17, 2020
9. DrahtBot added the label TX fees and policy on Jun 17, 2020
10. DrahtBot added the label Validation on Jun 17, 2020
11. 
    DrahtBot commented at 11:20 pm on June 17, 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:

    • #19826 (Pass mempool reference to chainstate constructor by MarcoFalke)
    • #19806 (validation: UTXO snapshot activation by jamesob)
    • #19791 ([net processing] Move Misbehaving() to PeerManager by jnewbery)
    • #19753 (p2p: don’t add AlreadyHave transactions to recentRejects by troygiorshev)
    • #19652 (Add thread safety annotations to Mempool{Info}ToJSON() by hebasto)
    • #19647 (Add thread safety annotations to CTxMemPool methods by hebasto)
    • #19645 (Allow updating mempool-txn with cheaper witnesses by ariard)
    • #19610 (p2p: refactor AlreadyHave(), CInv::type, INV/TX processing by jonatack)
    • #19572 (ZMQ: Create “sequence” notifier, enabling client-side mempool tracking by instagibbs)
    • #19544 (refactor: Add ParseBool to rpc/util by fjahr)
    • #19498 (Tidy up ProcessOrphanTx by jnewbery)
    • #19488 (Refactor mempool.dat to be extensible, and store missing info by luke-jr)
    • #19478 (Remove CTxMempool::mapLinks data structure member by JeremyRubin)
    • #19339 (validation: re-delegate absurd fee checking from mempool to clients by gzhao408)
    • #19093 (RPC: Return transaction fee from testmempoolaccept by rajarshimaitra)
    • #18191 (Change UpdateForDescendants to use Epochs by JeremyRubin)
    • #18017 (txmempool: split epoch logic into class by ajtowns)
    • #13990 (Allow fee estimation to work with lower fees by ajtowns)
    • #12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

    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.

12. fanquake removed the label GUI on Jun 18, 2020
13. fanquake removed the label Mining on Jun 18, 2020
14. fanquake removed the label P2P on Jun 18, 2020
15. fanquake removed the label RPC/REST/ZMQ on Jun 18, 2020
16. fanquake removed the label TX fees and policy on Jun 18, 2020
17. fanquake removed the label Tests on Jun 18, 2020
18. fanquake removed the label Validation on Jun 18, 2020
19. 
    hebasto commented at 9:04 am on June 18, 2020: member

    From IRC:

    <sipa> eh, i agree - that change isn’t a step in the right direction @sipa Which direction is right?

20. 
    hebasto commented at 9:28 am on June 18, 2020: member

    From IRC:

    <jeremyrubin> I think it’s basically getting rid of a recursive mutex for code that’s still designed to take a recursive mutex <gwillen> it’s better than a true recursive mutex because it’s not possible to recurse by accident, you have to declare at call time which behavior you want (although better if you had to declare statically at compile time) <sipa> it looks like that <jeremyrubin> The correct refactor would be to make the code not do anything fancy with locks, or to just leave it <jeremyrubin> gwillen: I think the chances of a bug or error in custom logic is higher than a recursive mutex <jeremyrubin> accidental recursion seems unlikely… <jeremyrubin> and accidental recursion would be a bug lock or no <gwillen> (sorry I mean, accidental mutex recursion, that is, calling a function while holding a mutex, not expecting the callee to also lock it, resulting in the callee violating the caller’s invariants) <gwillen> (this is the fundamental problem of recursive mutexes) <gwillen> (and I assume the motivation behind hebasto’s refactor)

    Correct, @gwillen :)

21. 
    hebasto commented at 9:31 am on June 18, 2020: member

    Searching for concept (N)ACKs before separating digestible chunks for reviewing into smaller pulls.
22. hebasto marked this as ready for review on Jun 18, 2020
23. 
    MarcoFalke commented at 11:29 am on June 18, 2020: member

    Currently this adds a lot of code complexity. Also, it adds mental complexity to write code that doesn’t crash or deadlock/UB whereas the RecursiveMutex just works (TM).

    Maybe there is a way to achieve the same without the added complexity? For example, always force the caller to take the lock for the right scope. This would also solve issues of non-atomic RPC responses or at make them more visible. Though, it makes caller code slightly more verbose.

24. 
    hebasto commented at 11:38 am on June 18, 2020: member

    Currently this adds a lot of code complexity. Also, it adds mental complexity to write code that doesn’t crash or deadlock/UB whereas the RecursiveMutex just works (TM).

    A function that locks RecursiveMutex there are no guarantees that protected invariants are held before locking. This adds mental complexity to read code.

    Maybe there is a way to achieve the same without the added complexity? For example, always force the caller to take the lock for the right scope. This would also solve issues of non-atomic RPC responses or at make them more visible.

    I assume these steps are much easier for non-recursive mutex, no?

25. DrahtBot added the label Needs rebase on Jun 19, 2020
26. 
    gwillen commented at 7:34 pm on June 19, 2020: contributor

    @hebasto Do you have any thoughts on “function takes bool locked” versus “split function into _locked and _unlocked variants”?

    In some of the cases here, I see that the latter would require some (maybe substantial) refactoring to avoid code duplication, and I assume that’s why you didn’t go that route. But that would not only eliminate conditional locking (which is scary), it would probably allow the use of RAII locks. I’m curious to hear your thoughts.

27. 
    hebasto commented at 12:41 pm on June 20, 2020: member

    @gwillen

    @hebasto Do you have any thoughts on “function takes bool locked” versus “split function into _locked and _unlocked variants”?

    For new code I prefer clean “split function into _locked and _unlocked variants” like #19238 (review). Unfortunately, that is not the case in this PR.

    In some of the cases here, I see that the latter would require some (maybe substantial) refactoring to avoid code duplication, and I assume that’s why you didn’t go that route.

    You are correct. I’ve used the “function takes bool locked” approach in 72f7486b5ebe96762c5d5a68849c61e58c812ffd and 21787abedd7a0808c0175a0b1d795df97fd3b970 as a quick-and-dirty solution to fix broken tests.

    But that would not only eliminate conditional locking (which is scary), it would probably allow the use of RAII locks. I’m curious to hear your thoughts.

    Hmm… In 21787abedd7a0808c0175a0b1d795df97fd3b970 I’ve had to drop the RAII lock in favor of a pair ENTER_CRITICAL_SECTION() - LEAVE_CRITICAL_SECTION(). That is why I dont’t like this approach.

28. hebasto force-pushed on Jun 20, 2020
29. 
    hebasto commented at 1:04 pm on June 20, 2020: member

    Rebased 4e00526c689c4164d02d8ca76331f3ed5da7b13c -> c9e7d011d69bb1ef965945bf90d7441165430808 (pr19306.01 -> pr19306.02) due to the conflict with #19293.
30. DrahtBot removed the label Needs rebase on Jun 20, 2020
31. DrahtBot added the label Needs rebase on Jun 21, 2020
32. hebasto force-pushed on Jun 23, 2020
33. 
    hebasto commented at 5:30 pm on June 23, 2020: member

    Rebased c9e7d011d69bb1ef965945bf90d7441165430808 -> 3fc8fa23fc4ff2978c89bba46f08e746b6e4c154 (pr19306.02 -> pr19306.03) due to the conflicts with #18027 and #19198.
34. DrahtBot removed the label Needs rebase on Jun 23, 2020
35. DrahtBot added the label Needs rebase on Jun 24, 2020
36. hebasto force-pushed on Jun 24, 2020
37. 
    hebasto commented at 6:17 pm on June 24, 2020: member

    Rebased 3fc8fa23fc4ff2978c89bba46f08e746b6e4c154 -> ff3d969891b0687219906f18e66c5bb499915968 (pr19306.03 -> pr19306.04) due to the conflict with https://github.com/bitcoin-core/gui/pull/11.
38. DrahtBot removed the label Needs rebase on Jun 24, 2020
39. DrahtBot added the label Needs rebase on Jul 1, 2020
40. hebasto force-pushed on Jul 3, 2020
41. 
    hebasto commented at 9:12 am on July 3, 2020: member

    Rebased ff3d969891b0687219906f18e66c5bb499915968 -> 511670449669116df5488cd4f807de620e55a7e3 (pr19306.04 -> pr19306.05) due to the conflict with #19331.
42. DrahtBot removed the label Needs rebase on Jul 3, 2020
43. DrahtBot added the label Needs rebase on Jul 11, 2020
44. hebasto force-pushed on Jul 16, 2020
45. 
    hebasto commented at 6:57 pm on July 16, 2020: member

    Rebased 511670449669116df5488cd4f807de620e55a7e3 -> 0c03cea32d3ab30a58e62bbe42af6ebef016ede4 (pr19306.05 -> pr19306.06) due to the conflicts with #19174 and #19474.
46. DrahtBot removed the label Needs rebase on Jul 16, 2020
47. DrahtBot added the label Needs rebase on Jul 22, 2020
48. hebasto force-pushed on Jul 29, 2020
49. 
    hebasto commented at 11:32 am on July 29, 2020: member

    Rebased 0c03cea32d3ab30a58e62bbe42af6ebef016ede4 -> 656cba72f475603497e318ed3f01db4ab694b2af (pr19306.06 -> pr19306.07) due to the conflicts with #18044 and #18637.
50. DrahtBot removed the label Needs rebase on Jul 29, 2020
51. DrahtBot added the label Needs rebase on Jul 30, 2020
52. hebasto force-pushed on Aug 2, 2020
53. 
    hebasto commented at 9:50 pm on August 2, 2020: member

    Rebased 656cba72f475603497e318ed3f01db4ab694b2af -> e23248c51c092269a33cde7ad0ff70a815876396 (pr19306.07 -> pr19306.08) due to the conflicts with #18011, #19569, and #19604.
54. 
    hebasto commented at 10:47 pm on August 2, 2020: member

    Some commits split out into #19647. So please start reviewing from #19647.
55. DrahtBot removed the label Needs rebase on Aug 3, 2020
56. DrahtBot added the label Needs rebase on Aug 10, 2020
57. Wiphawee112 approved
58. 
    Wiphawee112 commented at 1:57 pm on August 19, 2020: none
59. hebasto force-pushed on Aug 21, 2020
60. 
    hebasto commented at 6:56 am on August 21, 2020: member

    Rebased e23248c51c092269a33cde7ad0ff70a815876396 -> 95b4daa91ea4eecd3f345a20f285fb0528f5070d (pr19306.08 -> pr19306.09) due to the conflict with #19569.
61. DrahtBot removed the label Needs rebase on Aug 21, 2020
62. DrahtBot added the label Needs rebase on Aug 24, 2020
63. hebasto force-pushed on Aug 24, 2020
64. 
    hebasto commented at 7:14 pm on August 24, 2020: member

    Rebased 95b4daa91ea4eecd3f345a20f285fb0528f5070d -> 1faf43ac3b2cb2a116f501e56c2cd6fed903409c (pr19306.09 -> pr19306.10) due to the conflict with #19704.
65. DrahtBot removed the label Needs rebase on Aug 24, 2020
66. DrahtBot added the label Needs rebase on Aug 31, 2020
67. laanwj referenced this in commit 99a8eb6051 on Sep 4, 2020
68. hebasto marked this as a draft on Sep 4, 2020
69. refactor: Prevent double lock in MempoolToJSON() 8f1767d115
70. refactor: Add thread context to AcceptToMemoryPool() call sites c903bf7389
71. refactor: Prevent double lock in AcceptToMemoryPool() 7878cfc4af
72. refactor: Specify CChainState::FlushStateToDisk() by mempool mutex state b3d44b9dca
73. refactor: Avoid excessive mempool locking in FlushStateToDiskHelper() afcf9161ce
74. refactor: Remove excessive locking in CTxMemPool methods 52ec60d7bd
75. refactor: Make CTxMemPool::ClearPrioritisation() private f2825fa7b5
76. refactor: Add negative thread safety annotations to CTxMemPool ec98daf0ab
77. refactor: Prevent double lock in CTxMemPool::check() 26da318c12
78. refactor: Prevent double lock in CTxMemPool::clear() 87d624c46c
79. refactor: Prevent double lock in CTxMemPool::isSpent() b9b8dfd957
80. refactor: Prevent double lock in CTxMemPool::GetMinFee() 12922a0b65
81. refactor: Prevent double lock in CTxMemPool::GetTransactionAncestry() a249d59dc7
82. refactor: Prevent double lock in CTxMemPool::IsLoaded() 89dfadc589
83. refactor: Prevent double lock in CTxMemPool::size() e1eab65372
84. refactor: Prevent double lock in CTxMemPool::exists() ae48014680
85. refactor: Prevent double lock in CTxMemPool::get() ce19fb90d4
86. refactor: Prevent double lock in CTxMemPool::infoAll() 5eac81656b
87. refactor: Prevent double lock in CTxMemPool::DynamicMemoryUsage() 1b68b74485
88. refactor: Prevent double lock in CTxMemPool::RemoveUnbroadcastTx() a2eb36cb98
89. refactor: Prevent double lock in CTxMemPool::GetUnbroadcastTxs() cbcdf19e49
90. refactor: Prevent double lock in PartiallyDownloadedBlock::InitData() 5639ecbb7d
91. refactor: Prevent double lock in BlockAssembler::CreateNewBlock(() c37916a81b
92. refactor: Prevent double lock in CheckInputsFromMempoolAndCache() e6a0279063
93. refactor: Replace RecursiveMutex with Mutex in CTxMemPool a887d73dcb
94. hebasto force-pushed on Sep 6, 2020
95. 
    hebasto commented at 12:46 pm on September 6, 2020: member

    Rebased 1faf43ac3b2cb2a116f501e56c2cd6fed903409c -> a887d73dcb05d59067635aff91baf85e0c7c7396 (pr19306.10 -> pr19306.12) due to the merge conflicts.
96. hebasto marked this as ready for review on Sep 6, 2020
97. DrahtBot removed the label Needs rebase on Sep 6, 2020
98. hebasto marked this as a draft on Sep 6, 2020
99. sidhujag referenced this in commit 45b8840fd3 on Sep 9, 2020
100. hebasto closed this on Aug 24, 2021

     ---

101. DrahtBot locked this on Aug 24, 2022

Contributors
hebasto DrahtBot MarcoFalke gwillen Wiphawee112

Labels
Refactoring Mempool