Make CWallet::ListCoins atomic

promag commented at 11:45 am on February 2, 2018: member

Fix a potencial race in CWallet::ListCoins.

Replaces cs_main and cs_wallet locks by assertions in CWallet::AvailableCoins.

promag commented at 11:46 am on February 2, 2018: member

Note to reviewers, 2nd commit is just indentation, see 1st commit. I’ll squash before merge.

fanquake added the label Wallet on Feb 2, 2018

promag force-pushed on Feb 2, 2018

MarcoFalke commented at 4:19 pm on February 2, 2018: member

Just noting that you can fixup right now. It is possible to ignore-all-space in git diff

promag commented at 6:30 pm on February 2, 2018: member

I know but IIRC it’s not possible to add comments with that option, so I’ve decided to split for now.

in src/wallet/wallet.cpp:2202 in 834529f3c0 outdated

2197@@ -2198,111 +2198,109 @@ CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const
2198 
2199 void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const CCoinControl *coinControl, const CAmount &nMinimumAmount, const CAmount &nMaximumAmount, const CAmount &nMinimumSumAmount, const uint64_t nMaximumCount, const int nMinDepth, const int nMaxDepth) const
2200 {
2201+    AssertLockHeld(cs_main);
2202+    AssertLockHeld(cs_wallet);

jamesob commented at 7:32 pm on February 2, 2018:

Both are recursive locks, so any reason we can’t just use LOCK2 here?

promag commented at 7:36 pm on February 2, 2018:

Long term plan is to remove recursive locks. Correct me if I’m wrong @TheBlueMatt.

ryanofsky commented at 7:40 pm on February 2, 2018:

I think in general we want to move away from recursive locking: https://github.com/bitcoin/bitcoin/blob/85123be78df5a4e9447e70f9ee727416d45843d7/src/sync.h#L92

http://www.zaval.org/resources/library/butenhof1.html has some interesting rationale and history

promag commented at 7:42 pm on February 2, 2018:

Thanks @ryanofsky for the reference.

jamesob commented at 8:37 pm on February 2, 2018:

Ah, thanks for the clarification.

laanwj commented at 10:33 am on February 8, 2018:

Yes, recursive locking is one of the codebase’s sins that we inherited and should be corrected in the long run.

ryanofsky commented at 12:54 pm on February 8, 2018:

Yes, recursive locking is one of the codebase’s sins that we inherited and should be corrected in the long run.

If anyone is interested in this, #11640 followed by #11599 are steps towards removing recursive locking. #11640 has one ack and needs more review.

devrandom commented at 5:09 pm on February 3, 2018: none

ACK https://github.com/bitcoin/bitcoin/pull/12333/commits/834529f3c097386b82feb01bd2cc3e0f90418813

ryanofsky commented at 8:13 pm on February 5, 2018: member

utACK 834529f3c097386b82feb01bd2cc3e0f90418813. To make the locking change easier to evaluate, I might give the second commit (834529f3c097386b82feb01bd2cc3e0f90418813) a description indicating it is an indentation-only change, instead of squashing it.

sipa commented at 9:08 pm on February 5, 2018: member

utACK 834529f3c097386b82feb01bd2cc3e0f90418813 (very easy to review with -w / ?w=1).

promag force-pushed on Feb 5, 2018

promag commented at 9:50 pm on February 5, 2018: member

Followed @ryanofsky suggestion, 1st commit and 2nd commit.

ryanofsky commented at 10:23 pm on February 5, 2018: member

utACK 4848f093c4755794ca57c6514cb1e3c484cf3867, only change since last review is commit description

promag commented at 0:35 am on February 6, 2018: member

Conflicts with #10605.

[wallet] Make CWallet::ListCoins atomic 1beea7af92

[wallet] Indent only change of CWallet::AvailableCoins 2f960b5070

in src/wallet/test/wallet_tests.cpp:680 in 4848f093c4 outdated

676@@ -677,11 +677,11 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
677 
678     // Lock both coins. Confirm number of available coins drops to 0.
679     std::vector<COutput> available;
680+    LOCK2(cs_main, wallet->cs_wallet);

laanwj commented at 10:31 am on February 8, 2018:

This locks the cs_main and cs_wallet for the entire rest of this test, which is somewhat awkward scoping-wise, is that intentional?

promag commented at 10:49 am on February 8, 2018:

Nothing else happens and below the lock is again required so I’ve decided for the minimal diff. I can lock only until ListCoins below if you prefer.

laanwj commented at 6:06 pm on February 8, 2018:

Yes please, For readability I prefer locks to be taken at the beginning of a block.

promag commented at 6:19 pm on February 8, 2018:

Done, 2 blocks instead of one.

promag force-pushed on Feb 8, 2018

laanwj merged this on Feb 8, 2018

laanwj closed this on Feb 8, 2018

laanwj referenced this in commit d405beea26 on Feb 8, 2018

markblundeberg referenced this in commit 3979bf03ba on Jun 5, 2019

jtoomim referenced this in commit 1333cc9115 on Jun 29, 2019

jonspock referenced this in commit 2c1056d027 on Jul 4, 2019

proteanx referenced this in commit 4d52f0671d on Jul 5, 2019

jonspock referenced this in commit 0b4a16fa7a on Jul 9, 2019

PastaPastaPasta referenced this in commit 1b8e85e408 on Jun 9, 2020

PastaPastaPasta referenced this in commit d95f0aa68b on Jun 9, 2020

PastaPastaPasta referenced this in commit 4547b1ef4f on Jun 10, 2020

PastaPastaPasta referenced this in commit 44483b4353 on Jun 10, 2020

PastaPastaPasta referenced this in commit 68b39aa6d1 on Jun 11, 2020

PastaPastaPasta referenced this in commit b87a4ac145 on Jun 11, 2020

LarryRuane referenced this in commit f8ea7211a6 on Mar 4, 2021

LarryRuane referenced this in commit 5b2f7a6ea7 on Apr 13, 2021

zkbot referenced this in commit 0e36226271 on Apr 17, 2021

DrahtBot locked this on Sep 8, 2021

Make CWallet::ListCoins atomic #12333