refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

pull hebasto wants to merge 25 commits into bitcoin:master from hebasto:200616-mempool-mx changing 32 files +548 −283
  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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me