TSan builds export all sorts of symbols that aren't part of the
allowlist and report sanitizer failures when enabled with glibc
compatibility enabled.
The result of `check-symbols` is valuable but Bitcoin does not
run them during their CI runs and for certain developer-oriented
builds, it's proving to be more of an obstruction. It will still
be enabled for all other builds and will prove to be important
in backporting Guix pull requests, a finger on the pulse, but that
doesn't apply to dev-oriented builds.
5e7e4c9f6e57f5333bd17a20b0c85a78d032998e refactor: replace RecursiveMutex g_maplocalhost_mutex with Mutex (w0xlt)
a7da1409bc9f614009f76c1bfc55f029ff1265e4 scripted-diff: rename cs_mapLocalHost -> g_maplocalhost_mutex (w0xlt)
Pull request description:
This PR is related to #19303 and gets rid of the `RecursiveMutex cs_mapLocalHost`.
ACKs for top commit:
shaavan:
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e
theStack:
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e
hebasto:
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 961171e346fe385e16db9830115a8096f4ca2499bbea11a08c02ca808638dfb63c434ab9d66392c71e85be6352c8a2b6a0054b5a61aaabd28d71581fed5beae7
5574e6ed52d6effd3b7beff0f09b44449202a585 refactor: replace RecursiveMutex `m_callbacks_mutex` with Mutex (Sebastian Falbesoner)
3aa258109e3f3e0b1bfc4f811cbadfd6d516208c scripted-diff: rename `m_cs_callbacks_pending` -> `m_callbacks_mutex` (Sebastian Falbesoner)
Pull request description:
This PR is related to #19303 and gets rid of the RecursiveMutex `m_cs_callbacks_pending`. All of the critical sections (6 in total) only directly access the guarded elements, i.e. it is not possible that within one section another one is called, and we can use a regular Mutex:
807169e10b/src/scheduler.cpp (L138-L145)807169e10b/src/scheduler.cpp (L152-L160)807169e10b/src/scheduler.cpp (L169-L172)807169e10b/src/scheduler.cpp (L184-L187)807169e10b/src/scheduler.cpp (L197-L199)807169e10b/src/scheduler.cpp (L203-L206)
Also, it is renamed to adapt to the (unwritten) naming convention to use the `_mutex` suffix rather than the `cs_` prefix.
ACKs for top commit:
hebasto:
ACK 5574e6ed52d6effd3b7beff0f09b44449202a585, I have reviewed the code and it looks OK, I agree it can be merged.
w0xlt:
crACK 5574e6e
Tree-SHA512: ba4b151d956582f4c7183a1d51702b269158fc5e2902c51e6a242aaeb1c72cfcdf398f9ffa42e3072f5aba21a8c493086a5fe7c450c271322da69bd54c37ed1f
fa6a548f5485ad5be0106e4727812559aefc5a20 ci: Disable s390x gui tests for now (MarcoFalke)
Pull request description:
ACKs for top commit:
hebasto:
ACK fa6a548f5485ad5be0106e4727812559aefc5a20, tested locally with
Tree-SHA512: 077bcda0b4612dfbf538eb2999ce9f32ec3739c2ed5709f7c4c5d1a39738fbe6e549bd54b237152fd808736c15af1024e67c95dc87b4c3102738543522cb14d7
8c277b19c8f262e550cffe263e6d910b687ac882 refactor: Make m_cs_fee_estimator non-recursive (Hennadii Stepanov)
5ee5b696b588695ff78aaac08d5d85154f1953cf refactor: Add non-thread-safe CBlockPolicyEstimator::_removeTx helper (Hennadii Stepanov)
5c3033d45e5ec15499ce7a0222ffa0210a0f66bc Add thread safety annotations to CBlockPolicyEstimator public functions (Hennadii Stepanov)
Pull request description:
This PR eliminates the only place that `m_cs_fee_estimator` is recursively locked by refactoring out `_removeTx` member function.
Related to #19303.
ACKs for top commit:
theStack:
Code-review ACK 8c277b19c8f262e550cffe263e6d910b687ac882
amadeuszpawlik:
ACK 8c277b19c8f262e550cffe263e6d910b687ac882 reviewed, built and ran tests
Tree-SHA512: 65b0b59460d3d5fadf7e75e916b2898b0dcfafdf5b278ef8c3975660f67c9f88ae4b937944313bd36d7513a7a53e1e5859aaf4a6deb4a1aea089936b101635a1
cdb41d5573b1e2ed1bc1d8d1dc9f77e82672ee1f doc: Install Rosetta on M1-macOS for qt in depends (Hennadii Stepanov)
Pull request description:
On master (c609e10545492aba480ff17aff7eefc13a0b5cd8) `make -C depends qt` on Apple Silicon based macOS 11.4 ends with an error:
```
/bin/sh: /Users/hebasto/bitcoin/depends/work/build/aarch64-apple-darwin20.5.0/qt/5.12.11-6c4d47a8f8f/qtbase/bin/moc: Bad CPU type in executable
```
Installing Rosetta 2 fixes it.
Explanation. On Apple Silicon macOS the `qt` package in depends actually is cross compiled. All native tools (including `moc`) are x86_64 binaries, that require Rosetta 2 to run.
ACKs for top commit:
promag:
ACK cdb41d5573b1e2ed1bc1d8d1dc9f77e82672ee1f.
fanquake:
ACK cdb41d5573b1e2ed1bc1d8d1dc9f77e82672ee1f - I have not tested after installing Rosetta 2, but I saw the same issue during my first cross-compile on an M1 box.
Zero-1729:
ACK cdb41d5573b1e2ed1bc1d8d1dc9f77e82672ee1f
Tree-SHA512: fb06a32d6fb40f405ce856b44f5d3af0c51089886f3be79e509e5c325614d7af58ce4480c064c17e0efb695a1f69f68d533c417f9631d46d8a630aba60ce4433
fad0867d6ab9430070aa7d60bf7617a6508e0586 Cleanup -includeconf error message (MarcoFalke)
fa9f711c3746ca3962f15224285a453744cd45b3 Fix crash when parsing command line with -noincludeconf=0 (MarcoFalke)
Pull request description:
The error message has several issues:
* It may crash instead of cleanly shutting down, when `-noincludeconf=0` is passed
* It doesn't quote the value
* It includes an erroneous trailing `\n`
* It is redundantly mentioning `"-includeconf cannot be used from commandline;"` several times, when once should be more than sufficient
Fix all issues by:
* Replacing `get_str()` with `write()` to fix the crash and quoting issue
* Remove the `\n` and only print the first value to fix the other issues
Before:
```
$ ./src/bitcoind -noincludeconf=0
terminate called after throwing an instance of 'std::runtime_error'
what(): JSON value is not a string as expected
Aborted (core dumped)
$ ./src/bitcoind -includeconf='a b' -includeconf=c
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf=a b
-includeconf cannot be used from commandline; -includeconf=c
```
After:
```
$ ./src/bitcoind -noincludeconf=0
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf=true
$ ./src/bitcoind -includeconf='a b' -includeconf=c
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf="a b"
```
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34493
Testcase: https://github.com/bitcoin/bitcoin/files/6515429/clusterfuzz-testcase-minimized-system-6328535926046720.log
```
FUZZ=system ./src/test/fuzz/fuzz ./clusterfuzz-testcase-minimized-system-6328535926046720.log
```
See https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md
ACKs for top commit:
sipa:
utACK fad0867d6ab9430070aa7d60bf7617a6508e0586
Tree-SHA512: b44af93be6bf71b43669058c1449c4c6999f03b5b01b429851b149b12d77733408cb207e9a3edc6f0bffd6030c4c52165e8e23a1c2718ff5082a6ba254cc94a4
9053b88b1c15f57cdcff2fc1c761efebb2ebfefe update docstring in feature_csv_activation.py (Pierre K)
Pull request description:
These changes in the test documentation reflect the changes introduced in #17921.
ACKs for top commit:
MarcoFalke:
review ACK 9053b88
Tree-SHA512: 17fb954baded8dab1c869dd48b76b516150bae616c792c573e4114d4adfdd40195745c56570aa3050cc0015ee496acd7ec178df8ba14831dd22f9722fda84da2
d1292f25f272401da0c58580521c74b1fa03a9ad Avoid the use of abs64 in timedata (Pieter Wuille)
Pull request description:
Fixes#20135.
ACKs for top commit:
kallewoof:
ACK d1292f25f272401da0c58580521c74b1fa03a9ad
jonatack:
ACK d1292f25f272401da0c58580521c74b1fa03a9ad code/logic review, verified there are no remaining callers of `abs64()`, verified no warnings in a debug build
practicalswift:
ACK d1292f25f272401da0c58580521c74b1fa03a9ad
MarcoFalke:
ACK d1292f25f272401da0c58580521c74b1fa03a9ad 🎹
Tree-SHA512: d17e95c668eb5e02ea546433b3d1b5a0ccbfb2c9cec62fa67dad1844d7e278a2576fbc0b75bddbf4db9af7331e978148c7bef7fce7e6a07e0eb917ef1392f302
fa56eda58e5ec2f2345bbe14c798e83f2abb4728 log: Avoid treating remote misbehvior as local system error (MarcoFalke)
fa492895b572a1196ca8652006b6fc2fa1d16951 refactor: Switch ValidationState mode to C++11 enum class (MarcoFalke)
Pull request description:
When logging failures of `CheckBlockHeader` (high-hash), they are always logged as system error. This is problematic for several reasons:
* Submitting a blockheader that fails `CheckBlockHeader` over RPC will result in a debug log line that starts with `ERROR`. Proper behaviour should be to log not anything and instead only return the failure reason to the RPC user. This pull does not fix this issue entirely, but is a good first step in the right direction.
* A misbehaving peer that sends us an invalid block header that fails `CheckBlockHeader` will result in a debug log line that starts with `ERROR`. Proper behavior should be to log the remote peer misbehavior if logging for that category was enabled. This pull fixes this issue for `CheckBlockHeader` and other functions can be adjusted as well if needed in follow-ups. This should be a good first step in the right direction.
ACKs for top commit:
practicalswift:
re-ACK fa56eda58e5ec2f2345bbe14c798e83f2abb4728
Tree-SHA512: 9793191f5cb57bdff7c93926e94877e8ca2ef89dcebcf9eb155899c733961839ec7c3f9b9f001dc082ada4234fe6e75f6df431301678d6822325840771166d77
6fdfeebcc79df62c8bf1cf4b6e9e97d6aefb3eb3 refactor: Replace RecursiveMutex with Mutex in rpc/server.cpp (Hennadii Stepanov)
Pull request description:
The functions that could lock this mutex, i.e., `SetRPCWarmupStatus()`, `SetRPCWarmupFinished()`, `RPCIsInWarmup()`, `CRPCTable::execute()`, do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the `g_rpc_warmup_mutex` could be a non-recursive mutex.
Related to #19303.
ACKs for top commit:
laanwj:
ACK 6fdfeebcc79df62c8bf1cf4b6e9e97d6aefb3eb3
MarcoFalke:
ACK 6fdfeebcc79df62c8bf1cf4b6e9e97d6aefb3eb3
Tree-SHA512: 05a8ac58c0cd6a3c9afad9e06ad78059642e3e97715e129f379c0bf6dccdb58e70d05d965f23e7432fd3f02d7f97967a778ffb8e424837891d9d785a9e98964c
cc5c0d2299b09c58cd9962ca5075ffa53f2633c0 refactor: Fix formatting of timedata.cpp (Hennadii Stepanov)
c2410ceb844a443caf6dd8c6df976b9e24724d06 refactor: Replace RecursiveMutex with Mutex in timedata.cpp (Hennadii Stepanov)
Pull request description:
Only `GetTimeOffset()` and `AddTimeData()` functions lock this mutex. They do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the `g_timeoffset_mutex` could be a non-recursive mutex.
Related to #19180.
ACKs for top commit:
MarcoFalke:
ACK cc5c0d2299b09c58cd9962ca5075ffa53f2633c0 , checked the second commit with --word-diff-regex=. --ignore-all-space -U0 🦉
vasild:
ACK cc5c0d22 verified that recursion is not happening
Tree-SHA512: 38f6df689374d4a1a0e9aedb3ed5e885d8285c4da6b75f9bc84ae036936a159ef8276462db33b4f4dd5c71c6312fa9b45380f7a5726959665bc71dc39031be88
78c8f4fe11706cf5c165777c2ca122bd933b8b6a refactor: Replace RecursiveMutex with Mutex in netbase.cpp (Hennadii Stepanov)
Pull request description:
The functions that could lock this mutex, i.e., `{S,G}etProxy()`, `{S,G}etNameProxy()`, `HaveNameProxy()`, `IsProxy()`, do not call itself recursively, and do not call each other either directly or indirectly. Therefore, the `g_proxyinfo_mutex` could be a non-recursive mutex.
Related to #19180.
ACKs for top commit:
MarcoFalke:
ACK 78c8f4fe11706cf5c165777c2ca122bd933b8b6a , reviewed with the -W git option 👮
vasild:
ACK 78c8f4fe verified that recursion does not happen
Tree-SHA512: fc077fb371f38af5d05f1383c6bebf9926167c257892936fefd2d4fe6f679ca40124d25099e09f645d8ec266df222f96c5d0f9fd39eddcad15cbde0b427bc205
## Issue being fixed or feature implemented
LLMQ_400_85 requires four days worth of LLMQs
https://github.com/dashpay/dash/blob/master/src/llmq/params.h#L416
## What was done?
## How Has This Been Tested?
n/a
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Description
Pull request was inspired by the need to debug lock problems when
working on https://github.com/dashpay/dash/pull/5352.
As far as I'm aware, only macOS has `-Werror=thread-safety` as part of
its default `CXXFLAGS` despite the capability being present on Linux as
well. This PR introduces thread safety checks for that into our thread
sanitizer build.
Additionally, since we're using Clang, something that on first glimpse,
appears to be something that `stacktraces.cpp` isn't happy with, due to
`-Wl,-wrap` being available only on GCC, that no longer seems to be the
case, since the version of Clang with comes with `focal`, its `lld`
_does_ have support for `-wrap` (see [man page for `lld` on
`focal`](https://manpages.ubuntu.com/manpages/focal/en/man1/lld.1.html)).
The current `stable` version of Clang/LLVM is 15, at the time of this
pull request (see https://apt.llvm.org/) but `focal` ships with an older
version, requiring us to use the official LLVM APT repository. I feel we
should be testing with recent compilers alongside the ones shipped by
LTS distributions.
Certain bugs are only made apparent when testing on rolling release
distros or distros that have faster update cycles, like Fedora (see
https://github.com/dashpay/dash/pull/5295 for an illustration of that),
which ship with more recent compilers. Until we overhaul our CI systems
to test using those distros directly (our current infrastructure is
centered around using a "development image" with an LTS distro as the
base), this is the best we can do.
A similar pull request testing against the latest GCC stable will be
welcome as that is currently outside the scope of this PR as the changes
made were to make sure that builds were operating as expected on
Clang/LLVM 15.
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Missing docs
## What was done?
Added docs
## How Has This Been Tested?
In release of v19.1
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
gmp can't be detected on macos when installed via `brew` atm
## What was done?
detect package prefix and adjust CPPFLAGS and LDFLAGS accordingly
## How Has This Been Tested?
`./configure`
before: `configure: error: libgmp headers missing`
after: passes
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
9e111db088e4137865ae068d206c769994ea0a29 test: set a name for CI Docker containers (fanquake)
Pull request description:
Addresses one part of #16664, by making it easier to identify CI containers that are running locally. By default Docker will generate random names, like `peaceful_rubin`, with this change, we explicitly set names for all containers.
ACKs for top commit:
MarcoFalke:
ACK 9e111db088e4137865ae068d206c769994ea0a29
Tree-SHA512: 0a29ada0d8cf6b0e9ae7a35f4f6df7a3dcc448523ceaed01371124360d6e3d1bf351172104a5fb629488eeaa57994ba04134dcb83c261eb1dfd2f0d73edf5f60
