Make vpwallets usage thread safe#13028
Conversation
|
|
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
Does it compile if you add a GUARDED_BY here?
There was a problem hiding this comment.
Good point @MarcoFalke!
Please add GUARDED_BY(cs_wallets) and verify by building with --enable-werror using clang.
ae632bc to
03e8d00
Compare
|
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). |
@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. |
03e8d00 to
c6f5b4f
Compare
|
#13017 is merged, rebased. |
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
c6f5b4f to
e2f58f4
Compare
|
utACK e2f58f4 |
e2f58f4 wallet: Make vpwallets usage thread safe (João Barbosa) Pull request description: This PR turns the functions introduced in #13017 thread safe. This is required to correctly support dynamically loading wallets, which is implemented in #10740. Tree-SHA512: efaa09e501636cf957aa33de83719ce09dc0c2a19daff741a94ef10d6b7ba5dee538355b80c96ead995140f99f5df0c92fb0e22ae1adb8f397eb478280c8d8c7
e2f58f4 wallet: Make vpwallets usage thread safe (João Barbosa) Pull request description: This PR turns the functions introduced in bitcoin#13017 thread safe. This is required to correctly support dynamically loading wallets, which is implemented in bitcoin#10740. Tree-SHA512: efaa09e501636cf957aa33de83719ce09dc0c2a19daff741a94ef10d6b7ba5dee538355b80c96ead995140f99f5df0c92fb0e22ae1adb8f397eb478280c8d8c7
This PR turns the functions introduced in #13017 thread safe. This is required to correctly support dynamically loading wallets, which is implemented in #10740.