9f85e9cb3d scripted-diff: Rename LockAnnotation to LockAssertion (practicalswift)
de9b5dbca3 Make sure the compile-time locking promises given via LockAnnotation:s hold also in practice at runtime (ifdef DEBUG_LOCKORDER) (practicalswift)
3a809446b3 Move LockAnnotation to make it reflect the truth (practicalswift)
cc2588579c 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 9f85e9cb3d687862128ddf464d2bc2462b8627f0. No changes at all since last review except clean rebase after base PR #16033 was merged
Tree-SHA512: fb80e78fe362adfd6ea8405bcb142c09b99f834fe8be4397282b223ca2c3a2bb9719a074a47a043b44757f840b239a6fcd2f98d14771f8729204834ecf608c3a
This backport does not include changes that depend on bitcoin pr 18037
70a6b529f306ff72ea1badf25e970a92b2b17ab3 lint-cppcheck: Remove -DHAVE_WORKING_BOOST_SLEEP_FOR (Anthony Towns)
294937b39de5924e772f8ed90d35c53290c8acab scheduler_tests: re-enable mockforward test (Anthony Towns)
cea19f685915be8affb2203184a549576194413f Drop unused reverselock.h (Anthony Towns)
d0ebd93270758ea97ea956b8821e17a2d001ea94 scheduler: switch from boost to std (Anthony Towns)
b9c426012770d166e6ebfab27689be44e6e89aa5 sync.h: add REVERSE_LOCK (Anthony Towns)
306f71b4eb4a0fd8e64f47dc008bc235b80b13d9 scheduler: don't rely on boost interrupt on shutdown (Anthony Towns)
Pull request description:
Replacing boost functionality with C++11 stuff.
Motivated by #18227, but should stand alone. Changing from `boost::condition_var` to `std::condition_var` means `threadGroup.interrupt_all` isn't enough to interrupt `serviceQueue` anymore, so that means calling `stop()` before `join_all()` is needed. And the existing reverselock.h code doesn't work with sync.h's DebugLock code (because the reversed lock won't be removed from `g_lockstack` which then leads to incorrect potential deadlock warnings), so I've replaced that with a dedicated class and macro that's aware of our debug lock behaviour.
Fixes#16027, Fixes#14200, Fixes#18227
ACKs for top commit:
laanwj:
ACK 70a6b529f306ff72ea1badf25e970a92b2b17ab3
Tree-SHA512: d1da13adeabcf9186d114e2dad9a4fdbe2e440f7afbccde0c13dfbaf464efcd850b69d3371c5bf8b179d7ceb9d81f4af3cc22960b90834e41eaaf6d52ef7d331
# Conflicts:
# src/reverselock.h
# src/rpc/misc.cpp
# src/scheduler.cpp
# src/scheduler.h
# src/sync.cpp
# src/sync.h
# src/test/reverselock_tests.cpp
# src/test/scheduler_tests.cpp
# src/test/test_dash.cpp
# test/lint/extended-lint-cppcheck.sh
fa5e373365 validation: Add cs_main locking annotations (MarcoFalke)
fa5c346c5a doc: Add comment to cs_main and mempool::cs (MarcoFalke)
fafe941bdd test: Add missing validation locks (MarcoFalke)
fac4558462 sync: Add RecursiveMutex type alias (MarcoFalke)
Pull request description:
Both the chain state and the transaction pool are validation specific, but access to them is protected by two locks. The two locks have the following semantics:
* Writing to the chain state or adding transactions to the transaction pool -> Take both `cs_main` and `mempool::cs`
* Reading either or removing transactions from the the transaction pool -> Take only the appropriate lock
Tree-SHA512: 6f6e612ffc391904c6434a79a4f3f8de1b928bf0a3e3434b73561037b395e2b40a70a5a4bd8472dd230e9eacc8e5d5374c904a3c509910cf3971dd7ff59a626c
686c5456f2fcf7e301907223d16a85f7eb378c6c Fix missing header in sync.h (João Barbosa)
Pull request description:
`std::string` is referenced in `sync.h` but the relevant header is not explicitly included as required by current guideline. Furthermore on osx 10.14.6 with clang-900.0.31 the following error occurs:
```
In file included from threadinterrupt.cpp:6:
In file included from ./threadinterrupt.h:8:
./sync.h:206:21: error: implicit instantiation of undefined template 'std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >'
std::string lockname;
```
ACKs for top commit:
practicalswift:
ACK 686c5456f2fcf7e301907223d16a85f7eb378c6c
laanwj:
ACK 686c5456f2fcf7e301907223d16a85f7eb378c6c
Tree-SHA512: 7c1acdfa5b0dd148d1114e14c9450d5907006e63e1a04e82ed8a1e29757925476e6f8ef6024b0c6d1bb596623115209ad580d5035be1e4785337bd01b738c9f2
1e3bcd251768baeb95e555d51d2dc787a6b2acee [net_processing] Add thread safety annotations (Jesse Cohen)
f393a533bebc088985f94c725b9af881500ba998 Annotate AssertLockHeld() with ASSERT_CAPABILITY() for thread safety analysis (Jesse Cohen)
Pull request description:
(note that this depends on #13417)
This commit fully annotates all globals in net_processing with clang thread safety annotations. Subsequent commits will begin transitioning some of this data away from cs_main into locks that are local to net_processing. Static thread safety analysis should it easier to verify correctness of that process.
Tree-SHA512: b47aa410cb9ada21072370176aea9a74c575643fa1ee8cf1d43c8e28675eef17f33e5242ac422f840e8178e132ecb58412034c6334b68f1b57c686df80d4e8e2
* scripted-diff: Replace #include "" with #include <> (ryanofsky)
-BEGIN VERIFY SCRIPT-
for f in \
src/*.cpp \
src/*.h \
src/bench/*.cpp \
src/bench/*.h \
src/compat/*.cpp \
src/compat/*.h \
src/consensus/*.cpp \
src/consensus/*.h \
src/crypto/*.cpp \
src/crypto/*.h \
src/crypto/ctaes/*.h \
src/policy/*.cpp \
src/policy/*.h \
src/primitives/*.cpp \
src/primitives/*.h \
src/qt/*.cpp \
src/qt/*.h \
src/qt/test/*.cpp \
src/qt/test/*.h \
src/rpc/*.cpp \
src/rpc/*.h \
src/script/*.cpp \
src/script/*.h \
src/support/*.cpp \
src/support/*.h \
src/support/allocators/*.h \
src/test/*.cpp \
src/test/*.h \
src/wallet/*.cpp \
src/wallet/*.h \
src/wallet/test/*.cpp \
src/wallet/test/*.h \
src/zmq/*.cpp \
src/zmq/*.h
do
base=${f%/*}/ relbase=${base#src/} sed -i "s:#include \"\(.*\)\"\(.*\):if test -e \$base'\\1'; then echo \"#include <\"\$relbase\"\\1>\\2\"; else echo \"#include <\\1>\\2\"; fi:e" $f
done
-END VERIFY SCRIPT-
Signed-off-by: Pasta <pasta@dashboost.org>
* scripted-diff: Replace #include "" with #include <> (Dash Specific)
-BEGIN VERIFY SCRIPT-
for f in \
src/bls/*.cpp \
src/bls/*.h \
src/evo/*.cpp \
src/evo/*.h \
src/governance/*.cpp \
src/governance/*.h \
src/llmq/*.cpp \
src/llmq/*.h \
src/masternode/*.cpp \
src/masternode/*.h \
src/privatesend/*.cpp \
src/privatesend/*.h
do
base=${f%/*}/ relbase=${base#src/} sed -i "s:#include \"\(.*\)\"\(.*\):if test -e \$base'\\1'; then echo \"#include <\"\$relbase\"\\1>\\2\"; else echo \"#include <\\1>\\2\"; fi:e" $f
done
-END VERIFY SCRIPT-
Signed-off-by: Pasta <pasta@dashboost.org>
* build: Remove -I for everything but project root
Remove -I from build system for everything but the project root,
and built-in dependencies.
Signed-off-by: Pasta <pasta@dashboost.org>
# Conflicts:
# src/Makefile.test.include
* qt: refactor: Use absolute include paths in .ui files
* qt: refactor: Changes to make include paths absolute
This makes all include paths in the GUI absolute.
Many changes are involved as every single source file in
src/qt/ assumes to be able to use relative includes.
Signed-off-by: Pasta <pasta@dashboost.org>
# Conflicts:
# src/qt/dash.cpp
# src/qt/optionsmodel.cpp
# src/qt/test/rpcnestedtests.cpp
* test: refactor: Use absolute include paths for test data files
* Recommend #include<> syntax in developer notes
* refactor: Include obj/build.h instead of build.h
* END BACKPORT #11651 Remove trailing whitespace causing travis failure
* fix backport 11651
Signed-off-by: Pasta <pasta@dashboost.org>
* More of 11651
* fix blockchain.cpp
Signed-off-by: pasta <pasta@dashboost.org>
* Add missing "qt/" in includes
* Add missing "test/" in includes
* Fix trailing whitespaces
Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: MeshCollider <dobsonsa68@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
76ea17c79 Add mutex requirement for AddToCompactExtraTransactions(…) (practicalswift)
4616c825a Use -Wthread-safety-analysis if available (+ -Werror=thread-safety-analysis if --enable-werror) (practicalswift)
7e319d639 Fix -Wthread-safety-analysis warnings. Change the sync.h primitives to std from boost. (Matt Corallo)
Pull request description:
* Add mutex requirement for `AddToCompactExtraTransactions(…)`.
* Use `-Wthread-safety-analysis` if available.
* Rebased on top of https://github.com/TheBlueMatt/bitcoin/commits/2017-08-test-10923 - now includes: Fix -Wthread-safety-analysis warnings. Change the sync.h primitives to std from boost.
Tree-SHA512: fb7365f85daa2741c276a1c899228181a8d46af51db7fbbdffceeaff121a3eb2ab74d7c8bf5e7de879bcc5042d00d24cb4649c312d51caba45a3f6135fd8b38f
64fb0ac Declare single-argument (non-converting) constructors "explicit" (practicalswift)
Pull request description:
Declare single-argument (non-converting) constructors `explicit`.
In order to avoid unintended implicit conversions.
For a more thorough discussion, see ["C.46: By default, declare single-argument constructors explicit"](http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c46-by-default-declare-single-argument-constructors-explicit) in the C++ Core Guidelines (Stroustrup & Sutter).
Tree-SHA512: e0c6922e56b11fa402621a38656d8b1122d16dd8f160e78626385373cf184ac7f26cb4c1851eca47e9b0dbd5e924e39a85c3cbdcb627a05ee3a655ecf5f7a0f1
6e8c48dc5 Add const to methods that do not modify the object for which it is called (practicalswift)
Pull request description:
Tree-SHA512: a6888111ba16fb796e320e60806e1a77d36f545989b5405dc7319992291800109eab0b8e8c286b784778f41f1ff5289e7cb6b4afd7aec77f385fbcafc02cffc1
032ba3f RPC help documentation for addnode peerinfo. (Gregory Maxwell)
90f13e1 Add release notes for addnode changes. (Gregory Maxwell)
50bd12c Break addnode out from the outbound connection limits. (Gregory Maxwell)
This allows us to use function/variable/class attributes to specify locking
requisites, allowing problems to be detected during static analysis.
This works perfectly with newer Clang versions (tested with 3.3-3.7). For older
versions (tested 3.2), it compiles fine but spews lots of false-positives.
Rebased by @laanwj:
- update for RPC methods added since 84d13ee: setmocktime,
invalidateblock, reconsiderblock. Only the first, setmocktime, required a change,
the other two are thread safe.
- ensures a consistent usage in header files
- also add a blank line after the copyright header where missing
- also remove orphan new-lines at the end of some files
Use misc methods of avoiding unnecesary header includes.
Replace int typedefs with int##_t from stdint.h.
Replace PRI64[xdu] with PRI[xdu]64 from inttypes.h.
Normalize QT_VERSION ifs where possible.
Resolve some indirect dependencies as direct ones.
Remove extern declarations from .cpp files.
o Remove unused Leave and GetLock functions
o Make Enter and TryEnter private.
o Simplify Enter and TryEnter.
boost::unique_lock doesn't really know whether the
mutex it wraps is locked or not when the defer_lock
option is used.
The boost::recursive_mutex does not expose this
information, so unique_lock only infers this
knowledge. When taking the lock is defered, it
(randomly) assumes that the lock is not taken.
boost::unique_lock has the following definition:
unique_lock(Mutex& m_,defer_lock_t):
m(&m_),is_locked(false)
{}
bool owns_lock() const
{
return is_locked;
}
Thus it is a mistake to check owns_lock() in Enter
and TryEnter - they will always return false.
