wallet: Use wallet name instead of pointer on unload/release #16716

1. 
   promag commented at 1:08 AM on August 25, 2019: member

   Fixes #16668. Wallet name is unique so it can be used instead of pointer.

2. wallet: Use wallet name instead of pointer on unload/release d9d8984270
3. fanquake added the label Wallet on Aug 25, 2019
4. 
   DrahtBot commented at 2:42 AM on August 25, 2019: member

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

5. 
   practicalswift commented at 12:11 PM on August 25, 2019: contributor

   Concept ACK

   Thanks for fixing this.

6. ryanofsky approved
7. 
   ryanofsky commented at 5:30 PM on August 26, 2019: member

   utACK d9d8984270dbb004ec94f8dbb289be2bc9e4dbc3. Alternately I think it might be possible to use an intptr_t set instead of a string set to get around the undefined behavior described in the issue.

   Aside from the undefined behavior, I'm not sure the either the existing code or the new code is safe from race conditions where between delete wallet and g_unloading_wallet_set.erase another wallet object (possibly the same wallet) is loaded and unloaded, resulting in the assert after g_unloading_wallet_set.insert triggering, or causing UnloadWallet to return too early before a wallet was unloaded. Replacing the set with a map to refcounts might fix this, though at that point it might be worth moving away from shared_ptr and using another mechanism to delay unloading a wallet while it's being used by RPCs.

8. 
   meshcollider commented at 9:57 AM on August 27, 2019: contributor

   utACK d9d8984270dbb004ec94f8dbb289be2bc9e4dbc3

9. 
   promag commented at 12:10 AM on August 28, 2019: member

   Alternately I think it might be possible to use an intptr_t @ryanofsky I don't think, see https://stackoverflow.com/a/9492910.

   another wallet object (possibly the same wallet) is loaded and unloaded

   I also think that wouldn't be possible:

   https://github.com/bitcoin/bitcoin/blob/a7be1cc92be4946c4f042bccd3a1b007657f3241/src/wallet/rpcwallet.cpp#L2757-L2764

   moving away from shared_ptr

   What's the problem with shared_ptr?

   Anyway we could fail to unload if wallet is busy - meaning that the caller wouldn't block.

10. fanquake requested review from achow101 on Aug 30, 2019
11. fanquake requested review from instagibbs on Aug 30, 2019
12. 
    instagibbs commented at 1:47 PM on August 30, 2019: member

    utACK https://github.com/bitcoin/bitcoin/pull/16716/commits/d9d8984270dbb004ec94f8dbb289be2bc9e4dbc3

13. fanquake referenced this in commit e9ef1b2c2e on Aug 31, 2019
14. fanquake merged this on Aug 31, 2019
15. fanquake closed this on Aug 31, 2019

    ---

16. sidhujag referenced this in commit 1914c7850e on Aug 31, 2019
17. PastaPastaPasta referenced this in commit 2be6319ca1 on Sep 11, 2021
18. PastaPastaPasta referenced this in commit 3188d9af98 on Sep 11, 2021
19. PastaPastaPasta referenced this in commit 6f9cb7a8e8 on Sep 12, 2021
20. PastaPastaPasta referenced this in commit 5145ef9b9e on Sep 12, 2021
21. PastaPastaPasta referenced this in commit 2bbde0320e on Sep 12, 2021
22. PastaPastaPasta referenced this in commit 5484e05400 on Sep 14, 2021
23. PastaPastaPasta referenced this in commit 44c7bb618e on Sep 14, 2021
24. DrahtBot locked this on Dec 16, 2021

Review Requested
achow101instagibbs