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 missing cs_main locks when accessing chainActive #11596

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@practicalswift
Member

practicalswift commented Nov 2, 2017

  • Add missing cs_main locks when accessing chainActive. (The variable chainActive is guarded by the mutex cs_main.)
  • Add corresponding annotations (GUARDED_BY + EXCLUSIVE_LOCKS_REQUIRED).
@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Nov 2, 2017

Member

@sdaftuar @morcos Thanks for reviewing. Suggested changes incorporated! :-)

Member

practicalswift commented Nov 2, 2017

@sdaftuar @morcos Thanks for reviewing. Suggested changes incorporated! :-)

@fanquake fanquake added the Refactoring label Nov 3, 2017

@@ -2639,7 +2639,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
// enough, that fee sniping isn't a problem yet, but by implementing a fix
// now we ensure code won't be written that makes assumptions about
// nLockTime that preclude a fix later.
txNew.nLockTime = chainActive.Height();
{
LOCK(cs_main);

This comment has been minimized.

@promag

promag Nov 3, 2017

Member

Remove, all callers have cs_main locked, assert lock is held instead. Ping @ryanofsky as he says the goal is to removed recursive locks.

@promag

promag Nov 3, 2017

Member

Remove, all callers have cs_main locked, assert lock is held instead. Ping @ryanofsky as he says the goal is to removed recursive locks.

This comment has been minimized.

@practicalswift

practicalswift Nov 6, 2017

Member

@promag Thanks! Now fixed.

@practicalswift

practicalswift Nov 6, 2017

Member

@promag Thanks! Now fixed.

This comment has been minimized.

@practicalswift

practicalswift Nov 6, 2017

Member

@promag I'm getting an assertion failure when running test/functional/test_runner.py with your suggested patch applied. Does it pass for you? Reverting to previous version for now.

@practicalswift

practicalswift Nov 6, 2017

Member

@promag I'm getting an assertion failure when running test/functional/test_runner.py with your suggested patch applied. Does it pass for you? Reverting to previous version for now.

This comment has been minimized.

@promag

promag Nov 6, 2017

Member

I'll check.

@promag

promag Nov 6, 2017

Member

I'll check.

This comment has been minimized.

@practicalswift

practicalswift Mar 20, 2018

Member

@promag See @TheBlueMatt's comment about cs_main locking in CWallet::CreateTransaction(…): #11596 (comment)

@practicalswift

practicalswift Mar 20, 2018

Member

@promag See @TheBlueMatt's comment about cs_main locking in CWallet::CreateTransaction(…): #11596 (comment)

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Nov 6, 2017

Member

@practicalswift following #11596 (comment), I believe a7eb21a is the fix.

Member

promag commented Nov 6, 2017

@practicalswift following #11596 (comment), I believe a7eb21a is the fix.

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Nov 6, 2017

Member

@promag What is the reasoning behind the patch (a7eb21a) – what are the underlying locking requirements (exact variables being guarded)? Your patch is likely correct, but I'm trying to understand exactly why :-)

Member

practicalswift commented Nov 6, 2017

@promag What is the reasoning behind the patch (a7eb21a) – what are the underlying locking requirements (exact variables being guarded)? Your patch is likely correct, but I'm trying to understand exactly why :-)

Show outdated Hide outdated src/rest.cpp
Show outdated Hide outdated src/wallet/wallet.cpp
@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Nov 6, 2017

Member

@TheBlueMatt Thanks for reviewing. Feedback addressed.

Member

practicalswift commented Nov 6, 2017

@TheBlueMatt Thanks for reviewing. Feedback addressed.

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Nov 7, 2017

Member

Reverting the patch suggested by @promag. The locking is now made down in CreateTransaction as suggested by @TheBlueMatt.

Member

practicalswift commented Nov 7, 2017

Reverting the patch suggested by @promag. The locking is now made down in CreateTransaction as suggested by @TheBlueMatt.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 7, 2017

Contributor

utACK 2e441c9
Thanks!

Contributor

TheBlueMatt commented Nov 7, 2017

utACK 2e441c9
Thanks!

Show outdated Hide outdated src/rest.cpp
Show outdated Hide outdated src/rest.cpp
Show outdated Hide outdated src/rest.cpp
@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Nov 10, 2017

Member

Added commit 9b3d094 addressing @luke-jr:s feedback.

Please re-review :-)

Member

practicalswift commented Nov 10, 2017

Added commit 9b3d094 addressing @luke-jr:s feedback.

Please re-review :-)

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 10, 2017

Contributor

@practicalswift why did you rebase this? It makes reviewing something that was previously reviewed much more difficult. Mind squashing the two commits? There seems to be no reason to have them separate.

Contributor

TheBlueMatt commented Nov 10, 2017

@practicalswift why did you rebase this? It makes reviewing something that was previously reviewed much more difficult. Mind squashing the two commits? There seems to be no reason to have them separate.

@luke-jr

utACK

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

Add missing cs_main locks when accessing chainActive
The variable chainActive is guarded by the mutex cs_main.

Github-Pull: #11596
Rebased-From: 4786fe1

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

Don't allow chainActive to change from when we get the results to whe…
…n we identify the chain they're for

Github-Pull: #11596
Rebased-From: 9b3d094
@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Nov 12, 2017

Member

Now squashed into one commit. Please re-review! :-)

@TheBlueMatt The reason for the rebase was that I wanted to get rebased on top of 76ea17c (IIRC) in order to get the build to pass – I always compile with -Werror=thread-safety-analysis :-)

The reason for the two commits was to allow separate review for the latter commit in the case that I had misunderstood @luke-jr:s suggestion and needed to revert.

Member

practicalswift commented Nov 12, 2017

Now squashed into one commit. Please re-review! :-)

@TheBlueMatt The reason for the rebase was that I wanted to get rebased on top of 76ea17c (IIRC) in order to get the build to pass – I always compile with -Werror=thread-safety-analysis :-)

The reason for the two commits was to allow separate review for the latter commit in the case that I had misunderstood @luke-jr:s suggestion and needed to revert.

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Nov 23, 2017

Member

Updated:

  • Added a few more missing cs_main locks.
  • Added a commit with the Clang thread safety analysis annotation chainActive GUARDED_BY(cs_main) and the corresponding EXCLUSIVE_LOCKS_REQUIRED(…) annotations that follow from that.

Please review :-)

Member

practicalswift commented Nov 23, 2017

Updated:

  • Added a few more missing cs_main locks.
  • Added a commit with the Clang thread safety analysis annotation chainActive GUARDED_BY(cs_main) and the corresponding EXCLUSIVE_LOCKS_REQUIRED(…) annotations that follow from that.

Please review :-)

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Nov 25, 2017

Contributor
Contributor

TheBlueMatt commented Nov 25, 2017

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Nov 29, 2017

Member

I've now pushed an updated version adding a missing OpenWallets() lock and some additional annotations.

@TheBlueMatt - to make the annotations 100 % complete and moved to the declarations we'll have to handle some header files for which the existence of cs_main is currently not known. More specifically the following header files:

  • net_processing.h
  • qt/transactiondesc.h
  • qt/transactionrecord.h
  • rpc/blockchain.h
  • wallet/init.h
  • wallet/wallet.h

What is the most appropriate way to handle cs_main in the respective cases above? I guess we don't want to sprinkle extern CCriticalSection cs_main; all over the place? :-)

Member

practicalswift commented Nov 29, 2017

I've now pushed an updated version adding a missing OpenWallets() lock and some additional annotations.

@TheBlueMatt - to make the annotations 100 % complete and moved to the declarations we'll have to handle some header files for which the existence of cs_main is currently not known. More specifically the following header files:

  • net_processing.h
  • qt/transactiondesc.h
  • qt/transactionrecord.h
  • rpc/blockchain.h
  • wallet/init.h
  • wallet/wallet.h

What is the most appropriate way to handle cs_main in the respective cases above? I guess we don't want to sprinkle extern CCriticalSection cs_main; all over the place? :-)

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Dec 4, 2017

Contributor

@practicalswift No, we aren't adding the annotations just to add the annotations. In order for them to be really effective and provide the kind of guarantees we want they need to be listed everywhere the function which needs the annotation is declared, not just ate the definition. I'd rather we leave them out than add them just to function declarations. The annotations are going to be highly infective for things like cs_main - they're gonna propagate backwards through the callstack pretty deep, but that's ok, its how it should be. We shouldn't avoid it, just gotta take the dive (though preferably not in this PR, please, lets just get this one merged).

Contributor

TheBlueMatt commented Dec 4, 2017

@practicalswift No, we aren't adding the annotations just to add the annotations. In order for them to be really effective and provide the kind of guarantees we want they need to be listed everywhere the function which needs the annotation is declared, not just ate the definition. I'd rather we leave them out than add them just to function declarations. The annotations are going to be highly infective for things like cs_main - they're gonna propagate backwards through the callstack pretty deep, but that's ok, its how it should be. We shouldn't avoid it, just gotta take the dive (though preferably not in this PR, please, lets just get this one merged).

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Dec 4, 2017

Member

@TheBlueMatt Is that an utACK for this PR as-is? :-)

Member

practicalswift commented Dec 4, 2017

@TheBlueMatt Is that an utACK for this PR as-is? :-)

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Dec 4, 2017

Contributor

If you remove the annotation commit to revert to where people had previously reviewed this, I'm happy to review again.

Contributor

TheBlueMatt commented Dec 4, 2017

If you remove the annotation commit to revert to where people had previously reviewed this, I'm happy to review again.

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Dec 4, 2017

Member

@TheBlueMatt Reverted and pushed. Now only keeping the lock addition (removed adding annotations). Looks good? :-)

Member

practicalswift commented Dec 4, 2017

@TheBlueMatt Reverted and pushed. Now only keeping the lock addition (removed adding annotations). Looks good? :-)

Show outdated Hide outdated src/init.cpp
Show outdated Hide outdated src/init.cpp

@TheBlueMatt TheBlueMatt referenced this pull request Dec 14, 2017

Merged

Add LookupBlockIndex #11041

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Feb 22, 2018

Member

@TheBlueMatt Thanks for reviewing! Feedback address. Would you mind re-reviewing? :-)

Member

practicalswift commented Feb 22, 2018

@TheBlueMatt Thanks for reviewing! Feedback address. Would you mind re-reviewing? :-)

@luke-jr

luke-jr approved these changes Mar 1, 2018

utACK

Show outdated Hide outdated src/init.cpp
@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Mar 2, 2018

Member

@luke-jr Good point! Comment updated. Please re-review :-)

Member

practicalswift commented Mar 2, 2018

@luke-jr Good point! Comment updated. Please re-review :-)

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 2, 2018

Member

Since https://github.com/bitcoin/bitcoin/pull/11226/files#diff-349fbb003d5ae550a2e8fa658e475880R447 is closed, could you amend the second commit to include the clang annotation?

Member

MarcoFalke commented Mar 2, 2018

Since https://github.com/bitcoin/bitcoin/pull/11226/files#diff-349fbb003d5ae550a2e8fa658e475880R447 is closed, could you amend the second commit to include the clang annotation?

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Mar 12, 2018

Member

@MarcoFalke Good idea! Annotations added. Please review :-)

Member

practicalswift commented Mar 12, 2018

@MarcoFalke Good idea! Annotations added. Please review :-)

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 13, 2018

Member

Why did you un-remove the double-lock at "The block database contains a block which appears to be from the future."?

Member

luke-jr commented Mar 13, 2018

Why did you un-remove the double-lock at "The block database contains a block which appears to be from the future."?

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Mar 13, 2018

Member

@luke-jr I'm actually investigating a dead-lock right now which seems to have been introduced during the last rebase. Which double-lock are you referring to? Do you have a diff?

I'm investigating as we speak.

Member

practicalswift commented Mar 13, 2018

@luke-jr I'm actually investigating a dead-lock right now which seems to have been introduced during the last rebase. Which double-lock are you referring to? Do you have a diff?

I'm investigating as we speak.

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Mar 13, 2018

Member

@luke-jr You mean this one?

diff --git a/src/init.cpp b/src/init.cpp
index e7544e8..f18a947 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1429,7 +1429,6 @@ bool AppInitMain()
         nStart = GetTimeMillis();
         do {
             try {
-                LOCK(cs_main);
                 UnloadBlockIndex();
                 pcoinsTip.reset();
                 pcoinsdbview.reset();
Member

practicalswift commented Mar 13, 2018

@luke-jr You mean this one?

diff --git a/src/init.cpp b/src/init.cpp
index e7544e8..f18a947 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1429,7 +1429,6 @@ bool AppInitMain()
         nStart = GetTimeMillis();
         do {
             try {
-                LOCK(cs_main);
                 UnloadBlockIndex();
                 pcoinsTip.reset();
                 pcoinsdbview.reset();

@practicalswift practicalswift changed the title from Add missing cs_main locks when accessing chainActive to Add missing cs_main locks when accessing chainActive [wip] Mar 13, 2018

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 14, 2018

Member

Search init.cpp for "The block database contains a block which appears to be from the future.". It grabs cs_main immediately before that. But cs_main is already held here now.

Member

luke-jr commented Mar 14, 2018

Search init.cpp for "The block database contains a block which appears to be from the future.". It grabs cs_main immediately before that. But cs_main is already held here now.

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Mar 19, 2018

Member

I've now reworked this PR and split it up in three commits:

  • Add missing LOCK(cs_main):s where required for chainActive access
  • Annotation: Add chainActive GUARDED_BY(cs_main) – does not change run-time behaviour
  • Annotation: Add EXCLUSIVE_LOCKS_REQUIRED(...) as implied by the chainActive guard – does not change run-time behaviour

Please re-review :-)

/cc @sdaftuar @morcos @promag @TheBlueMatt @luke-jr @MarcoFalke

Member

practicalswift commented Mar 19, 2018

I've now reworked this PR and split it up in three commits:

  • Add missing LOCK(cs_main):s where required for chainActive access
  • Annotation: Add chainActive GUARDED_BY(cs_main) – does not change run-time behaviour
  • Annotation: Add EXCLUSIVE_LOCKS_REQUIRED(...) as implied by the chainActive guard – does not change run-time behaviour

Please re-review :-)

/cc @sdaftuar @morcos @promag @TheBlueMatt @luke-jr @MarcoFalke

@practicalswift practicalswift changed the title from Add missing cs_main locks when accessing chainActive [wip] to Add missing cs_main locks when accessing chainActive Mar 19, 2018

practicalswift added a commit to practicalswift/bitcoin that referenced this pull request Mar 20, 2018

practicalswift added a commit to practicalswift/bitcoin that referenced this pull request Mar 23, 2018

practicalswift added a commit to practicalswift/bitcoin that referenced this pull request Apr 3, 2018

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Apr 3, 2018

Member

Rebased!

Member

practicalswift commented Apr 3, 2018

Rebased!

practicalswift added a commit to practicalswift/bitcoin that referenced this pull request Apr 9, 2018

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Apr 9, 2018

Member

Rebased!

Member

practicalswift commented Apr 9, 2018

Rebased!

practicalswift added a commit to practicalswift/bitcoin that referenced this pull request Apr 14, 2018

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Apr 14, 2018

Member

Rebased! Please re-review :-)

Member

practicalswift commented Apr 14, 2018

Rebased! Please re-review :-)

@TheBlueMatt

I guess this is somewhat duplicative with #11652, no? My review comments there apply here too.

BOOST_CHECK(it != wallet->mapWallet.end());
it->second.SetMerkleBranch(chainActive.Tip(), 1);
std::map<uint256, CWalletTx>::iterator it;
{

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Apr 17, 2018

Contributor

Can you just take both locks instead of adding a new scope and lots of diff lines? Its in a test, locks shouldn't matter much.

@TheBlueMatt

TheBlueMatt Apr 17, 2018

Contributor

Can you just take both locks instead of adding a new scope and lots of diff lines? Its in a test, locks shouldn't matter much.

Show outdated Hide outdated src/interfaces/wallet.cpp
@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Apr 26, 2018

Member

Rebased and updated. Please re-review :-)

Member

practicalswift commented Apr 26, 2018

Rebased and updated. Please re-review :-)

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Apr 30, 2018

Member

Closing. Will add new PR based on #11652 and #13083.

Member

practicalswift commented Apr 30, 2018

Closing. Will add new PR based on #11652 and #13083.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment