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
b0a01299ed8cc2026e91a8dcad880b700e462cee test: Disable s390 build on travis (Wladimir J. van der Laan)
Pull request description:
Travis is consistently failing on s390 due to out of disk space issues,
which causes false positives. Disable the s390 build for now until
this is fixed some other way.
ACKs for top commit:
MarcoFalke:
ACK b0a01299ed8cc2026e91a8dcad880b700e462cee 🍠
jonasschnelli:
ACK b0a01299ed8cc2026e91a8dcad880b700e462cee
Tree-SHA512: ae8a78fa03bc620d802e4992d46929a0a4b801b50a6066d8f093f93fc5ebbc010d221c04787cbfc75436a2c40f9292257bf85fbb251593e21afff11756b6d8d4
10efc0487c442bccb0e4a9ac29452af1592a3cf2 Templatize ValidationState instead of subclassing (Jeffrey Czyz)
10e85d4adc9b7dbbda63e00195e0a962f51e4d2c Remove ValidationState's constructor (Jeffrey Czyz)
0aed17ef2892478c28cd660e53223c6dd1dc0187 Refactor FormatStateMessage into ValidationState (Jeffrey Czyz)
Pull request description:
This removes boilerplate code in the subclasses which otherwise only
differ by the result type.
The subclassing was introduced in a27a295.
ACKs for top commit:
MarcoFalke:
ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 🐱
ajtowns:
ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 -- looks good to me
jonatack:
ACK 10efc048 code review, build/tests green, nice cleanup
Tree-SHA512: 765dd52dde7d49b9a5c6d99d97c96f4492673e2aed0b0604faa88db0308fa4500a26bf755cca0b896be283874096c215932e1110a2d01dc012cd36a5fce58a42
f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f Add logging for CValidationInterface events (Jeffrey Czyz)
6edebacb2191373e76d79a4972d6192300976096 Refactor FormatStateMessage for clarity (Jeffrey Czyz)
72f3227c83810936e7a334304e5fd7c6dab8e91b Format CValidationState properly in all cases (Jeffrey Czyz)
428ac70095253225f64462ee15c595644747f376 Add VALIDATION to BCLog::LogFlags (Jeffrey Czyz)
Pull request description:
Add logging of `CValidationInterface` callbacks using a new `VALIDATIONINTERFACE` log flag (see #12994). A separate flag is desirable as the logging can be noisy and thus may need to be disabled without affecting other logging.
This could help debug issues where there may be race conditions at play, such as #12978.
ACKs for top commit:
jnewbery:
ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f
hebasto:
ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f
ariard:
ACK f9abf4a, only changes since 0cadb12 are replacing log indication `VALIDATIONINTERFACE` by `VALIDATION` and avoiding a forward declaration with a new include
ryanofsky:
Code review ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f. Just suggested changes since last review (thanks!)
Tree-SHA512: 3e0f6e2c8951cf46fbad3ff440971d95d526df2a52a2e4d6452a82785c63d53accfdabae66b0b30e2fe0b00737f8d5cb717edbad1460b63acb11a72c8f5d4236
fa569e1a9c5ad1bf8bdf866235b21aff56112224 ci: Set LC_ALL=C when running in qemu-s390x (MarcoFalke)
fa3d77623ecb25dde6b5f4be11626ed16966eb29 ci: Use debian to avoid apt install 404 errors (MarcoFalke)
fabb946090be2f604da3d7d4b1bbe93b79baf23e ci: Install needed gcc and qemu-user iff cross-compiling (MarcoFalke)
faba4672b64fb5ba89e5cb6299479887494b571a ci: Fix QEMU_USER_CMD parse issues (MarcoFalke)
fa5d709fb266c97d4db15006bf855e887a6c123b ci: Move wrap-qemu into separate script (MarcoFalke)
Pull request description:
Now that the ci system no longer propagates files from the docker container back to the host, the wrap-qemu script is broken.
To fix it, every statement in the script needs to be executed in the docker (with `DOCKER_EXEC`). Instead of juggling with triple escape sequences like `\\\"`, just move the script to a separate file and call it with `DOCKER_EXEC`.
Also, fix a bunch of other bugs that prevent running the ci system in qemu
See the `ci/README.md` on how to test. TLDR: Can be tested with (replace `arm` with `s390x` to run the s390x build):
```
FILE_ENV="./ci/test/00_setup_env_arm.sh" MAKEJOBS="-j9" ./ci/test_run_all.sh
```
ACKs for top commit:
laanwj:
Code review ACK fa569e1a9c5ad1bf8bdf866235b21aff56112224
Tree-SHA512: 84ebc44a4f0261ee6c29605a6896a1833ff6c81d729e6d08dd111941f570ce73221422bd3303e1108a266ec5eab2148bd5ee1cf6bc01477d8cc9a6c5bf2b34c2
ef63f5fc1136ad2a2cd080d44142a2ee3945c238 ci: Combine 32-bit build with CentOS 7 build (Sebastian Falbesoner)
Pull request description:
Combines the CentOS build with the 32-bit (i686) build to avoid Travis bottlenecks, as suggested in #17757 by MarcoFalke. This keeps most of the properties of the 32-bit build (dash as config shell, building QT5 GUI) and just builds it with depends inside the CentOS docker container.
Making the depends in `05_before_script.sh` with unset config shell (`CONFIG_SHELL=`)
6196e93001/ci/test/05_before_script.sh (L28)
caused problems for building the library libevent (resulting in a Makefile with no shell set (`SHELL=`)), that's why I set it explicitely to `/bin/bash` if we have a CentOS Docker container.
A Travis output of this 32-bit CentOS build can be seen here: https://travis-ci.org/theStack/bitcoin/jobs/634472394 (has been restarted once due to too long build time and appearance of the `CACHE_ERR_MSG`).
For anyone wanting to verify the outputs, I found these instructions useful to reproduce a Travis build locally: https://github.com/erdc/proteus/wiki/Replicating-the-TravisCI-Environment-on-your-Local-Machine (steps 1-3). In this case it's a bit tricky since you run Docker inside Docker -- within the Travis Docker container, the CentOS Docker container is created. To make this possible, the Docker socket has to be exposed to the Travis container via bind-mounting (`docker run -v /var/run/docker.sock:/var/run/docker.sock ...`), as suggested in https://stackoverflow.com/a/33003273.
Top commit has no ACKs.
Tree-SHA512: af508241cec3a10a66c37673d56691717b78375340e910fcdd3fb3870741eba623a436e1e85b26b54f013375611896f5411c5a7fec2437d367d27172230129fe
7e80f646b24a2abf3c031a649bcc706a695f80da Get the OutputType for a descriptor (Andrew Chow)
Pull request description:
Adds a `GetOutputType()` method to get the OutputType of a descriptor. Some descriptors don't have a determinate OutputType, so we actually use an `Optional<OutputType>`. For descriptors with indeterminate OutputType, we return `nullopt`.
`addr()` and `raw()` use OutputTypes as determined by the CTxDestination they have. For simplicity, `ScriptHash` destinations are `LEGACY` even though they could be `P2SH_SEGWIT`.
`combo()`, `pk()`, and `multi()` are `nullopt` as they either don't have an OutputType or they have multiple. `DescriptorImpl` defaults to `nullopt`.
`pkh()` is `LEGACY` as expected
`wpkh()` and `wsh()` are `BECH32` as expected.
`sh()` checks whether the sub-descriptor is `BECH32`. If so, it is `P2SH_SEGWIT`. Otherwise it is `LEGACY`.
The descriptor tests are updated to check the OutputType too.
ACKs for top commit:
fjahr:
ACK 7e80f646b24a2abf3c031a649bcc706a695f80da
meshcollider:
utACK 7e80f646b24a2abf3c031a649bcc706a695f80da
instagibbs:
cursory ACK 7e80f646b2
Sjors:
Code review ACK 7e80f646b24a2abf3c031a649bcc706a695f80da
jonatack:
ACK 7e80f64 code review/build/tests
Tree-SHA512: c5a813447b62e982435e1c948066f8d6c148c9ebffb0a5eb5a9028b173b01d5ead2f076a5ca3f7f37698538baa346f82a977ee48f583d89cb4e5ebd9111b2341
0e0fa27acb74b4f0075afcf59a0dff51a21baddb Get rid of VARINT default argument (Pieter Wuille)
Pull request description:
This removes the need for the non-strandard use of variadic macros.
ACKs for top commit:
ryanofsky:
Code review ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb. Only change since last review reverting outdated documentation change from earlier version of pr
jonatack:
ACK 0e0fa27 code review, built/ran tests/bitcoind
practicalswift:
ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb -- diff looks correct
MarcoFalke:
ACK 0e0fa27acb74b4f0075afcf59a0dff51a21baddb 📯
Tree-SHA512: 6e335e4b586d62112b7260a12481cd949d1b3bbdb83edf8db690348f0a01852e68504336ff3e072e5131a7c8cb404ef11a2f786f842b8d08bbf6ea0e688777b1
6dd59d2e491bc11ab26498668543e65440a3a931 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders)
4b8f1e989f3b969dc628b0801d5c31ebd373719c IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders)
Pull request description:
Regression introduced in https://github.com/bitcoin/bitcoin/pull/17621 which causes p2sh-segwit addresses to be erroneously missed.
Tests are only failing in 0.19 branch, likely because that release still uses p2sh-segwit addresses rather than bech32 by default.
I'll devise a test case to catch this going forward.
ACKs for top commit:
achow101:
ACK 6dd59d2e491bc11ab26498668543e65440a3a931
MarcoFalke:
ACK 6dd59d2
meshcollider:
Code review ACK 6dd59d2e491bc11ab26498668543e65440a3a931
Tree-SHA512: b3e0f320c97b8c1f814cc386840240cbde2761fee9711617b713d3f75a4a5dce2dff2df573d80873df42a1f4b74e816ab8552a573fa1d62c344997fbb6af9950
5855cc564fd456463e6d335830526675626923c6 bitcoin-wallet: Use PACKAGE_NAME in usage help (Luke Dashjr)
7f5db163a4ebc607d441e2457af10942be511d2c GUI: Use PACKAGE_NAME in modal overlay (Luke Dashjr)
Pull request description:
ACKs for top commit:
hebasto:
ACK 5855cc564fd456463e6d335830526675626923c6, checked with
fanquake:
ACK 5855cc564fd456463e6d335830526675626923c6 - checked `bitcoin-wallet` and a `--disable-wallet` `bitcoin-qt`.
Tree-SHA512: 3526eb122bfdbc63349d12251f17ffa20c7f3754af4ac9c554e6d36bb14b351f31c413c30401bb3d6e0e6200b72614dfc8475489b1f742b0423bd83fba758b94
e09c701e0110350f78366fb837308c086b6503c0 scripted-diff: Bump copyright of files changed in 2020 (MarcoFalke)
6cbe6209646db8914b87bf6edbc18c6031a16f1e scripted-diff: Replace CCriticalSection with RecursiveMutex (MarcoFalke)
Pull request description:
`RecursiveMutex` better clarifies that the mutex is recursive, see also the standard library naming: https://en.cppreference.com/w/cpp/thread/recursive_mutex
For that reason, and to avoid different people asking me the same question repeatedly (e.g. https://github.com/bitcoin/bitcoin/pull/15932#pullrequestreview-339175124 ), remove the outdated alias `CCriticalSection` with a scripted-diff
## Issue being fixed or feature implemented
Speed thing up: 8075fc0c61
Unify things: ff1a390224 (and _probably_
fix issues like https://gitlab.com/dashpay/dash/-/jobs/4304343867),
876f5c3a9f,
ed58cdda13
Let tsan tests finish on smaller/slower machines:
ba1e3360f9
## What was done?
pls see individual commits
## How Has This Been Tested?
run tests locally and my in gitlab ci
https://gitlab.com/UdjinM6/dash/-/jobs/4319419014
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] 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)_
## Issue being fixed or feature implemented
we missed it in https://github.com/dashpay/dash/pull/5385
## What was done?
## How Has This Been Tested?
## 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)_
