Skip to content

Make vpwallets usage thread safe #13028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 30, 2018
Merged

Make vpwallets usage thread safe #13028

merged 1 commit into from
Apr 30, 2018

Conversation

promag
Copy link
Contributor

@promag promag commented Apr 19, 2018

This PR turns the functions introduced in #13017 thread safe. This is required to correctly support dynamically loading wallets, which is implemented in #10740.

@promag
Copy link
Contributor Author

promag commented Apr 19, 2018 •
edited
Loading

Please review #13017 first. (already merged).

maflcko
maflcko reviewed Apr 19, 2018
View reviewed changes
src/wallet/wallet.cpp Outdated
@@ -35,9 +35,11 @@
#include <boost/algorithm/string/replace.hpp>

static std::vector<CWallet*> vpwallets;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it compile if you add a GUARDED_BY here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @MarcoFalke!

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@promag promag force-pushed the 2018-04-cs_wallets branch from ae632bc to 03e8d00 Compare April 19, 2018 15:25
@jnewbery
Copy link
Contributor

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 jnewbery mentioned this pull request Apr 19, 2018
Merged
@promag
Copy link
Contributor Author

promag commented Apr 20, 2018

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.

@promag promag mentioned this pull request Apr 20, 2018
Closed
@promag promag force-pushed the 2018-04-cs_wallets branch from 03e8d00 to c6f5b4f Compare April 23, 2018 06:59
@promag
Copy link
Contributor Author

promag commented Apr 23, 2018

#13017 is merged, rebased.

jonasschnelli
jonasschnelli reviewed Apr 23, 2018
View reviewed changes
src/wallet/wallet.cpp
return !vpwallets.empty();
}

std::vector<CWallet*> GetWallets()
{
LOCK(cs_wallets);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jonasschnelli jonasschnelli Apr 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jnewbery jnewbery mentioned this pull request Apr 23, 2018
Closed
10 tasks
@jonasschnelli jonasschnelli mentioned this pull request Apr 23, 2018
Merged
@promag promag force-pushed the 2018-04-cs_wallets branch from c6f5b4f to e2f58f4 Compare April 24, 2018 16:26
@jonasschnelli
Copy link
Contributor

utACK e2f58f4

conscott
conscott reviewed Apr 28, 2018
View reviewed changes
Copy link
Contributor

@conscott conscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK e2f58f4

@laanwj laanwj merged commit e2f58f4 into bitcoin:master Apr 30, 2018
laanwj added a commit that referenced this pull request Apr 30, 2018
@laanwj
Merge #13028: Make vpwallets usage thread safe
783bb64 
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
@promag promag deleted the 2018-04-cs_wallets branch April 30, 2018 16:01
@Bushstar Bushstar mentioned this pull request May 1, 2018
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 14, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
a3386a3 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 14, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
10182d0 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 14, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
e4b122f 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
5d02fa5 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
bdf4fb3 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
93248e4 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
c0c4693 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 16, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
af06fb5 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 16, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
1217f87 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 26, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
2d8a3d1 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request May 10, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
35fa9fe 
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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request May 10, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#13028: Make vpwallets usage thread safe
3d73de3 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
5 more reviewers

@jonasschnelli jonasschnelli jonasschnelli left review comments

@jnewbery jnewbery jnewbery left review comments

@maflcko maflcko maflcko left review comments

@practicalswift practicalswift practicalswift left review comments

@conscott conscott conscott left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@promag @jnewbery @jonasschnelli @maflcko @practicalswift @conscott @laanwj @fanquake