Make CWallet::ListCoins atomic #12333

pull promag wants to merge 2 commits into bitcoin:master from promag:2018-02-atomic-listcoins changing 2 files +91 −87
  1. 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.

  2. 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.

  3. fanquake added the label Wallet on Feb 2, 2018
  4. promag force-pushed on Feb 2, 2018
  5. 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

  6. 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.

  7. 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.

  9. 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.

  10. sipa commented at 9:08 PM on February 5, 2018: member

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

  11. promag force-pushed on Feb 5, 2018
  12. promag commented at 9:50 PM on February 5, 2018: member

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

  13. ryanofsky commented at 10:23 PM on February 5, 2018: member

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

  14. promag commented at 12:35 AM on February 6, 2018: member

    Conflicts with #10605.

  15. [wallet] Make CWallet::ListCoins atomic 1beea7af92
  16. [wallet] Indent only change of CWallet::AvailableCoins 2f960b5070
  17. 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.

  18. promag force-pushed on Feb 8, 2018
  19. laanwj merged this on Feb 8, 2018
  20. laanwj closed this on Feb 8, 2018
  21. laanwj referenced this in commit d405beea26 on Feb 8, 2018
  22. markblundeberg referenced this in commit 3979bf03ba on Jun 5, 2019
  23. jtoomim referenced this in commit 1333cc9115 on Jun 29, 2019
  24. jonspock referenced this in commit 2c1056d027 on Jul 4, 2019
  25. proteanx referenced this in commit 4d52f0671d on Jul 5, 2019
  26. jonspock referenced this in commit 0b4a16fa7a on Jul 9, 2019
  27. PastaPastaPasta referenced this in commit 1b8e85e408 on Jun 9, 2020
  28. PastaPastaPasta referenced this in commit d95f0aa68b on Jun 9, 2020
  29. PastaPastaPasta referenced this in commit 4547b1ef4f on Jun 10, 2020
  30. PastaPastaPasta referenced this in commit 44483b4353 on Jun 10, 2020
  31. PastaPastaPasta referenced this in commit 68b39aa6d1 on Jun 11, 2020
  32. PastaPastaPasta referenced this in commit b87a4ac145 on Jun 11, 2020
  33. LarryRuane referenced this in commit f8ea7211a6 on Mar 4, 2021
  34. LarryRuane referenced this in commit 5b2f7a6ea7 on Apr 13, 2021
  35. zkbot referenced this in commit 0e36226271 on Apr 17, 2021
  36. DrahtBot locked this on Sep 8, 2021
Contributors
promagMarcoFalkejamesobryanofskylaanwjdevrandomsipa
Labels
Wallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-11 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me