Make vpwallets usage thread safe #13028

1. 
   promag commented at 1:52 pm on April 19, 2018: member

   This PR turns the functions introduced in #13017 thread safe. This is required to correctly support dynamically loading wallets, which is implemented in #10740.
2. 
   promag commented at 1:52 pm on April 19, 2018: member

   Please review #13017 first. (already merged).
3. in src/wallet/wallet.cpp:37 in ae632bc2f3 outdated

   34@@ -35,9 +35,11 @@
   35 #include <boost/algorithm/string/replace.hpp>
   36 
   37 static std::vector<CWallet*> vpwallets;
   

   ---

   ---

   

   MarcoFalke commented at 2:50 pm on April 19, 2018:

   Does it compile if you add a GUARDED_BY here?

   ---

   

   practicalswift commented at 2:57 pm on April 19, 2018:

   Good point @MarcoFalke!

   Please add GUARDED_BY(cs_wallets) and verify by building with --enable-werror using clang.

   ---

   

   promag commented at 3:25 pm on April 19, 2018:

   Done.
4. promag force-pushed on Apr 19, 2018
5. 
   jnewbery commented at 9:16 pm on April 19, 2018: member

   Concept ACK.

   Note that this will allow dynamically loading wallets, but more is required before we add dynamic unloading of wallets (this doesn’t prevent one thread from removing a wallet while another thread still has a pointer to that wallet).

6. 
   promag commented at 1:22 am on April 20, 2018: member

   this doesn’t prevent one thread from removing a wallet while another thread still has a pointer to that wallet @jnewbery true, that’s one of the reasons to switch to shared pointers. A wallet can be unregistered and only (enqueued-to-)(unloaded+released) when reference count is zero.

7. fanquake added the label Wallet on Apr 20, 2018
8. promag force-pushed on Apr 23, 2018
9. 
   promag commented at 6:59 am on April 23, 2018: member

   #13017 is merged, rebased.
10. in src/wallet/wallet.cpp:68 in c6f5b4fd0d outdated

    63     return !vpwallets.empty();
    64 }
    65 
    66 std::vector<CWallet*> GetWallets()
    67 {
    68+    LOCK(cs_wallets);
    

    ---

    ---

    

    jonasschnelli commented at 12:10 pm on April 23, 2018:

    IMO the GetWallet call with returning a vector of raw pointers is dangerous. This is why I used the callable/lamda approach in #12587 (see https://github.com/bitcoin/bitcoin/pull/12587/files#diff-74d273024bfb20e7f09b87a23cd26f4dR41).

    A concurrency mess-up with retrieving the pointers under the cs_wallet lock (copy of the vector), then continue outside of the cs_wallet lock may happen in the future.

    ---

    

    jonasschnelli commented at 12:12 pm on April 23, 2018:

    This problem obviously also is also true for GetWallet() and - for unloading - moving to a shared pointer and weak-unloading at a later stage is probably unavoidable.

    ---

    

    jnewbery commented at 4:50 pm on April 23, 2018:

    Right now there’s no way to free wallets. Obviously we’ll need to change this when we add an unloadwallet RPC, but for now this approach is sufficient to allow us to add a loadwallet and createwallet RPC.

    Would it be sufficient to add a comment to these functions warning of the danger if we add a way to free wallets?

    ---

    

    jonasschnelli commented at 6:04 pm on April 23, 2018:

    Maybe a comment… I think its also okay to just be aware of this and fix it once we have a way to remove pointers from the array.

    ---

    

    promag commented at 6:09 pm on April 23, 2018:

    I’m working on that with shared pointers. I can include this commit there, but IMO this is not worse than master so can go in to at least protect concurrency for loadwallet.
11. wallet: Make vpwallets usage thread safe e2f58f421b
12. promag force-pushed on Apr 24, 2018
13. 
    jonasschnelli commented at 9:29 am on April 26, 2018: contributor

    utACK e2f58f421b1a6e360bbf7efdfbba398918ce19d3
14. 
    conscott commented at 10:35 am on April 28, 2018: contributor

    Tested ACK e2f58f421b1a6e360bbf7efdfbba398918ce19d3
15. laanwj merged this on Apr 30, 2018
16. laanwj closed this on Apr 30, 2018

    ---

17. laanwj referenced this in commit 783bb6455e on Apr 30, 2018
18. promag deleted the branch on Apr 30, 2018
19. PastaPastaPasta referenced this in commit a3386a3fda on Apr 14, 2020
20. PastaPastaPasta referenced this in commit 10182d0444 on Apr 14, 2020
21. PastaPastaPasta referenced this in commit e4b122fbf7 on Apr 14, 2020
22. PastaPastaPasta referenced this in commit 5d02fa540e on Apr 15, 2020
23. PastaPastaPasta referenced this in commit bdf4fb3cfc on Apr 15, 2020
24. PastaPastaPasta referenced this in commit 93248e49d4 on Apr 15, 2020
25. PastaPastaPasta referenced this in commit c0c46932d5 on Apr 15, 2020
26. PastaPastaPasta referenced this in commit af06fb5cd6 on Apr 16, 2020
27. PastaPastaPasta referenced this in commit 1217f87cb7 on Apr 16, 2020
28. PastaPastaPasta referenced this in commit 2d8a3d1295 on Apr 26, 2020
29. PastaPastaPasta referenced this in commit 35fa9feec2 on May 10, 2020
30. PastaPastaPasta referenced this in commit 3d73de3381 on May 10, 2020
31. ckti referenced this in commit ea688a5fbd on Mar 28, 2021
32. MarcoFalke locked this on Sep 8, 2021

Contributors
promag MarcoFalke practicalswift jnewbery jonasschnelli conscott

Labels
Wallet