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

Add AssertLockHeld assertions in CWallet::ListCoins #10605

Merged
merged 2 commits into from Aug 31, 2018

Conversation

Projects
None yet
7 participants
@ryanofsky
Copy link
Contributor

commented Jun 15, 2017

Fixes TODO from #10295

@fanquake fanquake added the Wallet label Jun 16, 2017

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

utACK 5c7a7de

@laanwj laanwj changed the title Add AssertLockHeld assertions in CWallet::ListCoins [dotnotmerge] Add AssertLockHeld assertions in CWallet::ListCoins Jun 24, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/listlock branch 2 times, most recently from 8392051 to cc69e88 Aug 29, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/listlock branch from cc69e88 to a6c6340 Feb 9, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/listlock branch from a6c6340 to 8740c61 Apr 6, 2018

@ryanofsky ryanofsky changed the title [dotnotmerge] Add AssertLockHeld assertions in CWallet::ListCoins Add AssertLockHeld assertions in CWallet::ListCoins Apr 6, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

Be careful here: this is dependent on a PR that has not been merged yet. Do not merge.

Dependent PR #10244 got merged, so this is now safe to merge too.


Rebased 5c7a7de -> 8392051 (pr/listlock.1 -> pr/listlock.2) due to conflict with #11126
Rebased 8392051 -> cc69e88 (pr/listlock.2 -> pr/listlock.3)
Rebased cc69e88 -> a6c6340 (pr/listlock.3 -> pr/listlock.4) due to conflicts with #10295 and #12333
Rebased a6c6340 -> 8740c61 (pr/listlock.4 -> pr/listlock.5)

@promag

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

utACK 8740c61.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/listlock branch from 8740c61 to 545e85e Apr 11, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

Rebased 8740c61 -> 545e85e (pr/listlock.5 -> pr/listlock.6) due to conflict with #12920

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

Would you mind adding the clang lock annotations here as well?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2018

The last travis run for this pull request was 101 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@DrahtBot DrahtBot reopened this Jul 21, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 21, 2018

@ryanofsky Are you still working on this? If so, mind to respond to my previous question from April?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 22, 2018

Missing locking annotations:

wallet/wallet.cpp:2362:5: error: calling function 'AvailableCoins' requires holding mutex 'cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
    AvailableCoins(availableCoins);
    ^
wallet/wallet.cpp:2373:5: error: calling function 'ListLockedCoins' requires holding mutex 'cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
    ListLockedCoins(lockedCoins);
    ^
@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

Closing as this seems to be inactive, let me know if you start work on this again and I should reopen it.

@laanwj laanwj closed this Aug 31, 2018

@laanwj laanwj added the Up for grabs label Aug 31, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2018

Closing as this seems to be inactive, let me know if you start work on this again and I should reopen it.

This PR can be reopened. There isn't actually any work that needs to be done here since the clang errors @MarcoFalke reported were fixed by @skeees in #13423 (f393a53).

@MarcoFalke MarcoFalke reopened this Aug 31, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

@ryanofsky What I mean with locking annotations is the clang-annotations in the header file:

diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 05ae9a1bbf..da326517c0 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -764,7 +764,7 @@ public:
     /**
      * Return list of available coins and locked coins grouped by non-change output address.
      */
-    std::map<CTxDestination, std::vector<COutput>> ListCoins() const;
+    std::map<CTxDestination, std::vector<COutput>> ListCoins() const EXCLUSIVE_LOCKS_REQUIRED(cs_main, cs_wallet);
 
     /**
      * Find non-change parent output.

Without these, it is only a best-guess runtime-check that the correct locks are taken for this method.

Add EXCLUSIVE_LOCKS_REQUIRED to CWallet::ListCoins
Suggested by MarcoFalke <falke.marco@gmail.com> in
#10605 (comment)
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

utACK 545e85e (given that the clang lock annotations are included in this commit)

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

utACK 62b6f0f

@promag

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

utACK 62b6f0f.

@MarcoFalke MarcoFalke merged commit 62b6f0f into bitcoin:master Aug 31, 2018

2 checks passed

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

MarcoFalke added a commit that referenced this pull request Aug 31, 2018

Merge #10605: Add AssertLockHeld assertions in CWallet::ListCoins
62b6f0f Add EXCLUSIVE_LOCKS_REQUIRED to CWallet::ListCoins (Russell Yanofsky)
545e85e Add AssertLockHeld assertions in CWallet::ListCoins (Russell Yanofsky)

Pull request description:

  Fixes TODO from #10295

Tree-SHA512: 2dd03a8217e5e1313aa2119cb530e0c0daf3ae3a751b6fdec611df57b8090201a90b52ff05f8f696e978a1344aaf21989d67a03beb5ef6ef79b77be38d04b451

Bushstar added a commit to Bushstar/Feathercoin that referenced this pull request Sep 4, 2018

HashUnlimited added a commit to ToDoThings/bitcoin that referenced this pull request Sep 10, 2018

Add EXCLUSIVE_LOCKS_REQUIRED to CWallet::ListCoins
Suggested by MarcoFalke <falke.marco@gmail.com> in
bitcoin#10605 (comment)

jfhk added a commit to jfhk/bitcoin that referenced this pull request Nov 14, 2018

Add EXCLUSIVE_LOCKS_REQUIRED to CWallet::ListCoins
Suggested by MarcoFalke <falke.marco@gmail.com> in
bitcoin#10605 (comment)

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Nov 26, 2018

Add EXCLUSIVE_LOCKS_REQUIRED to CWallet::ListCoins
Suggested by MarcoFalke <falke.marco@gmail.com> in
bitcoin#10605 (comment)
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.