Hold mempool.cs for the duration of ATMP. #12368

1. 
   TheBlueMatt commented at 6:54 PM on February 6, 2018: member

   This resolves an issue where getrawmempool() can race mempool notification signals. Intuitively we use mempool.cs as a "read lock" on the mempool with cs_main being the write lock, so holding the read lock intermittently while doing write operations is somewhat strange.

   This also avoids the introduction of cs_main in getrawmempool() which reviewers objected to in the previous fix in #12273

2. Hold mempool.cs for the duration of ATMP.

   This resolves an issue where getrawmempool() can race mempool
   notification signals. Intuitively we use mempool.cs as a "read
   lock" on the mempool with cs_main being the write lock, so holding
   the read lock intermittently while doing write operations is
   somewhat strange.
   This also avoids the introduction of cs_main in getrawmempool()
   which reviewers objected to in the previous fix in #12273

   85aa8398f5
3. MarcoFalke added this to the milestone 0.16.0 on Feb 6, 2018
4. 
   MarcoFalke commented at 7:22 PM on February 6, 2018: member

   Thanks, fixes the test failures for me. (At least the wallet and mempool related ones, heh)

   -----BEGIN PGP SIGNED MESSAGE-----
   Hash: SHA512
   
   ACK 85aa8398f5d13c659299b81cdae377462b4f8316
   -----BEGIN PGP SIGNATURE-----
   
   iQIcBAEBCgAGBQJaefz+AAoJENLqSFDnUoslrz4P/07HOkA1I4XAQTGUcFpWYVgM
   LdLJIdSY1HcflR/l9L07oNU7wZgKMGRVYMU+fn6sB1FPmJVJAWURSKBBrq1dr37h
   AHIkHG4nZAJuxcU1oi+s0n8/KxPh1mLPi+NsTcUm88c1DcWFQQUripYIiSh0f5tv
   uFSdIwVYc7es3BdGoSwHhK4d/+tZIZQ9GCUITYfciqiK0YpHSAZ0RF+xM0nHADdC
   XDMr0igQ0DmSGrM4yzdbZUciVcxASC+oeM6i5Jgck9fFNIfaQziNIWOk2PBEsAfg
   KBj4mkOPjvqmLyxp8FCWaVKxt2VXE1/bQq0pneGkIyJvEri6I39tU0lG6R1ReBm7
   WJtSx+XMvsVQU7A8GvcEQ7oCq0pA63T2+YA9p00TRXBYjireDgmhmzrSXkgmxaD8
   bw8WZexF1GfBLkKgR4zgjWkYOKvnRqUOKgPoCfTXkyaSxloeOg3pVF/XDMD0A1Yw
   pX0ASzgEapm8epJVojF7tLwEEGvzFfaFR/UOA70MHb7k4DtNCjyhgwrjppK3+X32
   3BqIzFPaq4w8s4zqJosVKCxP41fzi8uOLPyZetWd2y/JE5dv4FTGuiE5NWIBzyLh
   5fGErIBdYAZdQyLQWvCHdJvBVsvscNo/ZL7mXFl4IbdPmUieEz7OnrU82tbb+xMe
   ngOWNw5mjJFBtkyLuIme
   =ovgc
   -----END PGP SIGNATURE-----
   

5. laanwj added the label Mempool on Feb 6, 2018
6. Add braces to meet code style on line-after-the-one-changed. 02fc886363
7. MarcoFalke added the label Needs backport on Feb 6, 2018
8. 
   MarcoFalke commented at 9:15 PM on February 6, 2018: member

   re-utACK 02fc8863630a20e75230f8bc3ba1051c480ae560 (second commit obviously not for backport)

9. 
   promag commented at 10:20 PM on February 6, 2018: member

   I don't think this is correct because of lock order. You correctly say

   // mempool "read lock" (held through GetMainSignals().TransactionAddedToMempool())
   

   And when it hits CWallet::TransactionAddedToMempool the lock order will be cs_main, mempool.cs, cs_wallet.

   But, for instance, listtransactions RPC when eventually reaches WalletTxToJSON, the lock order will be cs_main, cs_wallet, mempool.cs.

   Correct me if I'm wrong, but can't this create a deadlock?

10. 
    TheBlueMatt commented at 10:24 PM on February 6, 2018: member

    There is no lock order across TransactionAddedToMempool as it is called on a background thread (and can wait as long as it wants).

11. 
    promag commented at 10:34 PM on February 6, 2018: member

    Right, GetMainSignals().TransactionAddedToMempool enqueues the signal!

    utACK 02fc886.

12. 
    sipa commented at 3:21 AM on February 7, 2018: member

    utACK 02fc8863630a20e75230f8bc3ba1051c480ae560. We probably want to rethink how locking of chainstate/mempool works later on (either by using actual rwlocks, or by allowing the mempool to lag behind chain validation).

13. 
    devrandom commented at 5:27 PM on February 7, 2018: none

    utACK https://github.com/bitcoin/bitcoin/pull/12368/commits/02fc8863630a20e75230f8bc3ba1051c480ae560

14. laanwj merged this on Feb 8, 2018
15. laanwj closed this on Feb 8, 2018

    ---

16. laanwj referenced this in commit d57d10ee96 on Feb 8, 2018
17. laanwj referenced this in commit b8947554dd on Feb 8, 2018
18. laanwj referenced this in commit 3f5012beb6 on Feb 8, 2018
19. laanwj removed the label Needs backport on Feb 8, 2018
20. HashUnlimited referenced this in commit b0149c57b4 on Mar 16, 2018
21. HashUnlimited referenced this in commit 55b7e0b357 on Mar 16, 2018
22. ccebrecos referenced this in commit fca2771ede on Sep 14, 2018
23. ccebrecos referenced this in commit c8b594c31b on Sep 14, 2018
24. PastaPastaPasta referenced this in commit 22ee217b06 on Mar 14, 2020
25. PastaPastaPasta referenced this in commit 9c9a1ab8f7 on Mar 14, 2020
26. PastaPastaPasta referenced this in commit 78d303c3fd on Mar 15, 2020
27. LarryRuane referenced this in commit 79d2a21ee2 on Mar 4, 2021
28. LarryRuane referenced this in commit 1c88e51cd2 on Mar 4, 2021
29. ckti referenced this in commit d2f38bd658 on Mar 28, 2021
30. LarryRuane referenced this in commit a4730c8a0e on Apr 13, 2021
31. LarryRuane referenced this in commit 829e801fd9 on Apr 13, 2021
32. zkbot referenced this in commit 0e36226271 on Apr 17, 2021
33. DrahtBot locked this on Sep 8, 2021

Milestone
0.16.0