Avoid locking cs_main in some wallet RPC #12559

pull promag wants to merge 1 commits into bitcoin:master from promag:2018-02-avoid-cs_main-lock changing 2 files +3 −4

promag commented at 4:24 PM on February 27, 2018: member
Avoid locking cs_main in the folllowing wallet RPC:
- decoderawtransaction
- getnewaddress
- getrawchangeaddress
- setlabel
promag force-pushed on Feb 27, 2018
fanquake added the label RPC/REST/ZMQ on Feb 27, 2018
laanwj added the label Wallet on Mar 5, 2018
laanwj commented at 9:40 PM on March 5, 2018: member

Can you please add some rationale as to how you checked that the cs_main lock can be safely removed from these functions?
promag commented at 9:55 PM on March 5, 2018: member

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 commented at 10:32 PM on March 5, 2018: member

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 commented at 2:42 PM on April 24, 2018: member

@laanwj that would be detected either by review or by lock order check?
MarcoFalke commented at 7:26 PM on April 27, 2018: member

This +6/-7 change should go into a single commit
promag force-pushed on Apr 27, 2018
promag commented at 8:06 PM on April 27, 2018: member

@MarcoFalke Squashed 909cad7...eb29e2536. Updated PR description.
MarcoFalke commented at 7:09 PM on June 24, 2018: member

Needs rebase due to travis bug
promag force-pushed on Jun 24, 2018
promag commented at 7:13 PM on June 24, 2018: member

Thanks @MarcoFalke, rebased.
achow101 commented at 10:14 PM on July 9, 2018: member

utACK ca7a86acbc3bd0c9f24962e41b9aed36a7a229f4
MarcoFalke commented at 5:32 PM on July 10, 2018: member

utACK ca7a86a. Agree that lock-order inversion is caught by travis or review.
DrahtBot added the label Needs rebase on Jul 20, 2018
promag force-pushed on Aug 12, 2018
promag commented at 12:41 AM on August 12, 2018: member

Rebased.
DrahtBot removed the label Needs rebase on Aug 12, 2018
MarcoFalke commented at 6:35 PM on August 20, 2018: member

re-utACK 8c00f6569fb1f69c63a62025f07f7f821ed63064
rpc: Avoid locking cs_main in some wallet RPC 00f58f8c48
promag force-pushed on Aug 23, 2018
promag commented at 12:48 AM on August 23, 2018: member

Removed changes from RPC's related to accounts to avoid conflict with #14023. Updated OP.
MarcoFalke commented at 1:17 AM on August 23, 2018: member

re-utACK 00f58f8c48db05dce9dceed73a0028482e037f0f
laanwj commented at 5:38 PM on August 23, 2018: member

utACK 00f58f8c48db05dce9dceed73a0028482e037f0f
laanwj merged this on Aug 23, 2018
laanwj closed this on Aug 23, 2018
laanwj referenced this in commit 540bf8aacc on Aug 23, 2018
promag deleted the branch on Aug 23, 2018
deadalnix referenced this in commit 24d55d31a9 on Jan 14, 2020
Munkybooty referenced this in commit ceaf7cde1c on Sep 8, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels