Skip to content
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.

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

Avoid locking cs_main in some wallet RPC #12559

Merged
merged 1 commit into from Aug 23, 2018

Conversation

Projects
None yet
6 participants
@promag
Copy link
Member

commented Feb 27, 2018

Avoid locking cs_main in the folllowing wallet RPC:

  • decoderawtransaction
  • getnewaddress
  • getrawchangeaddress
  • setlabel

@promag promag force-pushed the promag:2018-02-avoid-cs_main-lock branch Feb 27, 2018

@laanwj laanwj added the Wallet label Mar 5, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

Can you please add some rationale as to how you checked that the cs_main lock can be safely removed from these functions?

@promag

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2018

I've manually checked what calls were made in each RPC, and checked that none uses something protected by cs_main. I guess this could be checked by AssertLockIsHeld or similar, but it's not widely adopted yet.

@laanwj

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

Thanks. What I'm a little bit worried about is that some of the CWallet calls might internally lock cs_main, which will cause potential deadlocks due to lock ordering if cs_wallet is already held. Or maybe not now, but that someone will do so in the future.

@promag

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2018

@laanwj that would be detected either by review or by lock order check?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

This +6/-7 change should go into a single commit

@promag promag force-pushed the promag:2018-02-avoid-cs_main-lock branch Apr 27, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2018

@MarcoFalke Squashed 909cad7...eb29e2536. Updated PR description.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jun 24, 2018

Needs rebase due to travis bug

@promag promag force-pushed the promag:2018-02-avoid-cs_main-lock branch Jun 24, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2018

Thanks @MarcoFalke, rebased.

@achow101

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

utACK ca7a86acbc3bd0c9f24962e41b9aed36a7a229f4

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

utACK ca7a86a. Agree that lock-order inversion is caught by travis or review.

@promag promag force-pushed the promag:2018-02-avoid-cs_main-lock branch Aug 12, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2018

Rebased.

@DrahtBot DrahtBot removed the Needs rebase label Aug 12, 2018

@DrahtBot DrahtBot referenced this pull request Aug 14, 2018

Closed

[wallet] Kill accounts #13825

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

re-utACK 8c00f6569fb1f69c63a62025f07f7f821ed63064

@DrahtBot DrahtBot referenced this pull request Aug 22, 2018

Merged

Remove accounts rpcs #14023

@promag promag force-pushed the promag:2018-02-avoid-cs_main-lock branch to 00f58f8 Aug 23, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2018

Removed changes from RPC's related to accounts to avoid conflict with #14023. Updated OP.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

re-utACK 00f58f8

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

utACK 00f58f8

@laanwj laanwj merged commit 00f58f8 into bitcoin:master Aug 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 23, 2018

Merge #12559: Avoid locking cs_main in some wallet RPC
00f58f8 rpc: Avoid locking cs_main in some wallet RPC (João Barbosa)

Pull request description:

  Avoid locking `cs_main` in the folllowing wallet RPC:
   - `decoderawtransaction`
   - `getnewaddress`
   - `getrawchangeaddress`
   - `setlabel`

Tree-SHA512: 54089766b2a969a17479af6c60e8ce151fac1f8cec268d43c61e679d5d17e76d17e414240c9ca2bfd280165f3a04e24a51310eb283591cd601a7eebc8b2423ea

@promag promag deleted the promag:2018-02-avoid-cs_main-lock branch Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.