Hold cs_main when reading chainActive via getTipLocator(). Remove assumeLocked(). by practicalswift · Pull Request #16033 · bitcoin/bitcoin

practicalswift · 2019-05-16T08:49:58Z

Problem description:

LockAnnotation lock(::cs_main) is a guarantee to the compiler thread analysis that ::cs_main is locked (when it couldn't be determined otherwise).

Despite being annotated with the locking guarantee ...

bitcoin/src/interfaces/chain.cpp

Lines 134 to 138 in 65526fc

    
           CBlockLocator getTipLocator() override 
        
           { 
        
               LockAnnotation lock(::cs_main); 
        
               return ::ChainActive().GetLocator(); 
        
           }

... getTipLocator() reads chainActive (via ::ChainActive()) without holding cs_main.

This can be verified by adding the following AssertLockHeld(cs_main):

$ git diff
diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index 59623284d..9fc693a0f 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -134,6 +134,7 @@ class LockImpl : public Chain::Lock
     CBlockLocator getTipLocator() override
     {
         LockAnnotation lock(::cs_main);
+        AssertLockHeld(::cs_main);
         return ::ChainActive().GetLocator();
     }
     Optional<int> findLocatorFork(const CBlockLocator& locator) override
$ make check
../build-aux/test-driver: line 107: 12881 Aborted                 "$@" > $log_file 2>&1
FAIL: qt/test/test_bitcoin-qt

ryanofsky · 2019-05-16T12:18:38Z

Can you also delete the LockingStateImpl class? There is no reason for the LockingStateImpl class to exist without assumeLocked. You just need to s/LockingStateImpl/LockImpl/ in the lock method and make LockImpl inherit from UniqueLock.

ryanofsky · 2019-05-16T14:07:26Z

Do you mean like this?

Thanks, yes that looks perfect.

src/wallet/test/wallet_tests.cpp

maflcko · 2019-05-16T15:34:15Z

Could squash all the refactoring except the change to /wallet/wallet.cpp, which could go into a separate commit?

DrahtBot · 2019-05-16T16:16:21Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16034 (Add assertion to make sure the LockAnnotation guarantees we give are truthful (ifdef DEBUG_LOCKORDER). Rename LockAnnotation to LockAssertion. by practicalswift)
#15921 (Tidy up ValidationState interface by jnewbery)
#15713 (refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

…ve redundant cs_main locks.

practicalswift · 2019-05-16T19:46:43Z

@MarcoFalke Thanks for the quick review! Updated accordingly. Please re-review :-)

maflcko · 2019-05-16T20:12:01Z

utACK 9402ef0

ryanofsky

utACK 00f941055fedb855473d5428f54c9d79786f6e23

practicalswift · 2019-05-16T20:49:15Z

@ryanofsky Thanks for the review. I think you utACK:ed the previous commit hash :-)

ryanofsky

utACK 9402ef0. Changes are consolidating commits and removing redundant lock2 cs_main calls

…(). Remove assumeLocked(). 9402ef0 Remove temporary method assumeLocked(). Remove LockingStateImpl. Remove redundant cs_main locks. (practicalswift) 593a8e8 wallet: Use chain.lock() instead of temporary chain.assumeLocked() (practicalswift) Pull request description: Fixes #16028. Problem description: `LockAnnotation lock(::cs_main)` is a guarantee to the compiler thread analysis that `::cs_main` is locked (when it couldn't be determined otherwise). Despite being annotated with the locking guarantee ... https://github.com/bitcoin/bitcoin/blob/65526fc8666fef35ef908dbc225f706bef642c7e/src/interfaces/chain.cpp#L134-L138 ... `getTipLocator()` reads `chainActive` (via `::ChainActive()`) without holding `cs_main`. This can be verified by adding the following `AssertLockHeld(cs_main)`: ``` $ git diff diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 5962328..9fc693a0f 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -134,6 +134,7 @@ class LockImpl : public Chain::Lock CBlockLocator getTipLocator() override { LockAnnotation lock(::cs_main); + AssertLockHeld(::cs_main); return ::ChainActive().GetLocator(); } Optional<int> findLocatorFork(const CBlockLocator& locator) override $ make check ../build-aux/test-driver: line 107: 12881 Aborted "$@" > $log_file 2>&1 FAIL: qt/test/test_bitcoin-qt ``` ACKs for commit 9402ef: MarcoFalke: utACK 9402ef0 ryanofsky: utACK 9402ef0. Changes are consolidating commits and removing redundant lock2 cs_main calls Tree-SHA512: 0a030bf0c07eb53194ecc246f973ef389dd42a0979f51932bf94bdf7e90c52473ae03be49718ee1629582b05dd8e0dc020b5a210318c93378ea4ace90c0f9f72

… add run-time check to it 9f85e9c scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift) de9b5db Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift) 3a80944 Move LockAnnotation to make it reflect the truth (practicalswift) cc25885 Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift) Pull request description: `LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise). Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises. This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`). Issues like the one described in #16028 will be discovered immediately with this PR merged. Changes in this PR: * Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code) * Move `LockAnnotation` in `wallet_tests` to make it reflect the truth * Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`) * Rename `LockAnnotation` to `LockAssertion` ACKs for commit 9f85e9: ryanofsky: utACK 9f85e9c. No changes at all since last review except clean rebase after base PR #16033 was merged Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a

…ion and add run-time check to it 9f85e9c scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift) de9b5db Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift) 3a80944 Move LockAnnotation to make it reflect the truth (practicalswift) cc25885 Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift) Pull request description: `LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise). Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises. This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`). Issues like the one described in bitcoin#16028 will be discovered immediately with this PR merged. Changes in this PR: * Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code) * Move `LockAnnotation` in `wallet_tests` to make it reflect the truth * Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`) * Rename `LockAnnotation` to `LockAssertion` ACKs for commit 9f85e9: ryanofsky: utACK 9f85e9c. No changes at all since last review except clean rebase after base PR bitcoin#16033 was merged Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a

…tor(). Remove assumeLocked() Summary: bitcoin/bitcoin@593a8e8 wallet: Use chain.lock() instead of temporary chain.assumeLocked() bitcoin/bitcoin@9402ef0 Remove temporary method assumeLocked(). Remove LockingStateImpl. Remove redundant cs_main locks. --- This is a backport of Core [[bitcoin/bitcoin#16033 | PR16033]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6132

…Locator(). Remove assumeLocked().

…ion and add run-time check to it 9f85e9c scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift) de9b5db Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift) 3a80944 Move LockAnnotation to make it reflect the truth (practicalswift) cc25885 Move LockAnnotation from threadsafety.h (imported code) to sync.h (our code) (practicalswift) Pull request description: `LockAnnotation lock(mutex);` is a guarantee to the compiler thread-analysis that `mutex` is locked (when it couldn't be determined otherwise). Before this PR it was possible to make the mistake of adding a `LockAnnotation` where the correct mutex is _not_ held. This in turn makes the thread-analysis reasoning being based on incorrect premises. This PR adds an assertion in the `LockAnnotation` ctor which checks that the guarantees given by us at compile-time are held also in practice (`ifdef DEBUG_LOCKORDER`). Issues like the one described in bitcoin#16028 will be discovered immediately with this PR merged. Changes in this PR: * Move `LockAnnotation` from `threadsafety.h` (imported code) to `sync.h` (our code) * Move `LockAnnotation` in `wallet_tests` to make it reflect the truth * Make sure the compile-time locking promises given via `LockAnnotation`:s hold also in practice at runtime (`ifdef DEBUG_LOCKORDER`) * Rename `LockAnnotation` to `LockAssertion` ACKs for commit 9f85e9: ryanofsky: utACK 9f85e9c. No changes at all since last review except clean rebase after base PR bitcoin#16033 was merged Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a

fanquake added the Refactoring label May 16, 2019

This was referenced May 16, 2019

getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee #16028

Closed

refactoring: Rename LockAnnotation to LockAssertion and add run-time check to it #16034

Merged

This comment has been minimized.

Sign in to view

maflcko reviewed May 16, 2019

View reviewed changes

src/wallet/test/wallet_tests.cpp Outdated Show resolved Hide resolved

practicalswift added 2 commits May 16, 2019 21:42

wallet: Use chain.lock() instead of temporary chain.assumeLocked()

593a8e8

Remove temporary method assumeLocked(). Remove LockingStateImpl. Remo…

9402ef0

…ve redundant cs_main locks.

practicalswift force-pushed the assumeLocked branch from 00f9410 to 9402ef0 Compare May 16, 2019 19:46

ryanofsky approved these changes May 16, 2019

View reviewed changes

maflcko merged commit 9402ef0 into bitcoin:master May 17, 2019

practicalswift deleted the assumeLocked branch April 10, 2021 19:38

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#16033: Hold cs_main when reading chainActive via getTip…

4bc0ae1

…Locator(). Remove assumeLocked().

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hold cs_main when reading chainActive via getTipLocator(). Remove assumeLocked().#16033

Hold cs_main when reading chainActive via getTipLocator(). Remove assumeLocked().#16033
maflcko merged 2 commits intobitcoin:masterfrom
practicalswift:assumeLocked

practicalswift commented May 16, 2019

Uh oh!

ryanofsky commented May 16, 2019

Uh oh!

This comment has been minimized.

ryanofsky commented May 16, 2019

Uh oh!

Uh oh!

maflcko commented May 16, 2019

Uh oh!

DrahtBot commented May 16, 2019 •

edited

Loading

Uh oh!

practicalswift commented May 16, 2019

Uh oh!

maflcko commented May 16, 2019

Uh oh!

ryanofsky left a comment

Uh oh!

practicalswift commented May 16, 2019

Uh oh!

ryanofsky left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	CBlockLocator getTipLocator() override
	{
	LockAnnotation lock(::cs_main);
	return ::ChainActive().GetLocator();
	}

Conversation

practicalswift commented May 16, 2019

Uh oh!

ryanofsky commented May 16, 2019

Uh oh!

This comment has been minimized.

ryanofsky commented May 16, 2019

Uh oh!

Uh oh!

maflcko commented May 16, 2019

Uh oh!

DrahtBot commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented May 16, 2019

Uh oh!

maflcko commented May 16, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented May 16, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrahtBot commented May 16, 2019 •

edited

Loading