2f6fe4e4e9 ci: Build with --enable-werror by default, and document exceptions (Hennadii Stepanov)
Pull request description:
This PR prevents introducing of new compiler warnings in the master branch, e.g., #19986, #20162.
ACKs for top commit:
practicalswift:
cr ACK 2f6fe4e4e9: patch looks correct
MarcoFalke:
re-ACK 2f6fe4e4e9🏏
vasild:
ACK 2f6fe4e
Tree-SHA512: 23b5feb5bc472658c992d882ef61af23496f25adaa19f9c79bfaef5d2db273d44981aa93b1631a7d37cb58755283c1dacf3f2d68e501522d3fa8c965ab646d19
Tests that a fully signed transaction given to
signrawtransactionwithwallet is both unchanged and marked as complete.
This tests for a regression in 0.20 where the transaction would not be
marked as complete.
This behavior was apparently inadvertently broken in 5400ef6; without this
change our daily self-announcements frequently go unsent, because our
address is still in the peer's rolling bloom filter (for potentially many
days, depending on addr traffic).
cadb77a6ab net: Add compat.h header for htonl function (Hennadii Stepanov)
f796f0057b net: Drop unneeded headers when compat.h included (Hennadii Stepanov)
467c346448 net: Drop unneeded Windows headers in compat.h (Hennadii Stepanov)
Pull request description:
It is the `compat.h` header's job to provide platform-agnostic interfaces for internet operations.
No need in `#include <arpa/inet.h>` scattered around.
ACKs for top commit:
practicalswift:
re-ACK cadb77a6ab: patch looks even better
laanwj:
Code review ACK cadb77a6ab
Tree-SHA512: 625ff90b2806310ab856a6ca1ddb6d9a85aa70f342b323e8525a711dd12219a1ecec8373ec1dca5a0653ffb11f9b421753887b25615d991ba3132c1cca6a3c6e
2c69381f3d Removed redundant git pull from appveyor config. (Aaron Clauson)
8b99e609e7 Adjusted msvc compiler and linker settings to remove optimisations that are causing sporadic ABI issues on Visual Studio updates. (Aaron Clauson)
Pull request description:
The motivation for this PR is twofold:
1. Update the Qt binaries used by the appveyor CI job after a recent update to Visual Studio 2019 used in the Appveyor build image resulted in ABI incompatibilities,
2. Remove optimisations and debug information from the Bitcoin Core `Release` msvc build to reduce the chance of future ABI incompatibility issues for future Visual Studio updates.
The changes made in this PR are:
- Changed appveyor config file hash to use a new version of Qt pre-compiled binaries built for Visual Studio 2019 v16.8.1.
- Adjusted msvc compiler and linker settings to remove optimisations and debug information generation to help avoid future ABI issues on Visual Studio updates.
- Tidied up debug and release configuration blocks in common project file to avoid duplication.
- Updated appveyor config to use latest Visual Studio 2019 image.
- Bumped vcpkg version to tag `2020.11-1` for binary caching feature*.
See #20392 for related discussion.
*Binary caching is a new [vcpkg feature](https://github.com/microsoft/vcpkg/blob/master/docs/specifications/binarycaching.md) that allows dependency caching. This PR is not using the feature but by updating the vcpkg version it means it can be optionally used by other contributors in their own Appveyor configs. By caching the vcpkg dependencies using this feature my build times are reduced by approx 10 minutes.
ACKs for top commit:
MarcoFalke:
Concept ACK 2c69381f3d
laanwj:
Concept ACK 2c69381f3d
Tree-SHA512: 372f5b8b3b5f7e56b78045f9fc06a22fd9f5366d06e99e2f1eaad6d741680da74d0e6371e7bc580cb41f4d6696b2101b950167309d33e98242578b458ace813a
c82d15b6d1 depends: Do not force Precompiled Headers (PCH) for building Qt on Linux (Hennadii Stepanov)
Pull request description:
On CentOS 8 (Cirrus CI job) the forced `-pch` option breaks Qt build.
Removing `-pch` option does not affect build time for other systems:
- master (e2ff5e7b35):
```
$ time make -j 9 -C depends/ qt
...
Caching qt...
make: Leaving directory '/home/hebasto/guix/GitHub/bitcoin/depends'
real 4m22,359s
user 18m3,719s
sys 1m24,769s
```
- this PR:
```
$ time make -j 9 -C depends/ qt
...
Caching qt...
make: Leaving directory '/home/hebasto/guix/GitHub/bitcoin/depends'
real 4m14,862s
user 18m3,355s
sys 1m24,506s
```
Qt docs: https://doc.qt.io/qt-5/qmake-precompiledheaders.htmlFixes#20423
ACKs for top commit:
MarcoFalke:
review ACK c82d15b6d1
Tree-SHA512: 0f2a3712e90de881d00f8e56c363edde33dd4f5c117df5744ab4e51d0a8146331de7236bc8329d68ddd91535cd853e68ee80ef4cceb6a909786abfd8881b01e8
This moves the fee_estimates file management to the CBlockPolicyEstimator
Flush() method.
Co-authored-by: John Newbery <john@johnnewbery.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
This moves the CBlockPolicyEstimator to the NodeContext, which get rids
of two globals and allows us to conditionally create the
CBlockPolicyEstimator (and to remove a circular dep).
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Check both failure cases:
- Use a glibc symbol from a version that is too new
- Use a symbol from a library that is not in the allowlist
And also check a conforming binary.
Adding a similar check for Windows PE can be done in a separate PR.
fad7be584f test: Fix intermittent p2p_finerprint issue (MarcoFalke)
Pull request description:
A single sync_with_ping can't be used to drop a block announcement, as the block might be sent *after* the ping has been responded to.
Fix that by waiting for the block.
ACKs for top commit:
theStack:
ACK fad7be584f
Tree-SHA512: d43ba9d07273486858f65a26326cc6637ef743bf7b400e5048ba7eac266fb1893283e6503dd49f179caa1abab2977315fb70ba9fba34be9a817a74259d8e4034
fa5c4f12f5 ci: Adjust cirrus ci task names (MarcoFalke)
Pull request description:
The task names are too long for GitHub to display them properly without truncation in the "checks-view". Fix that by using a new naming scheme:
* Native builds don't mention "x86_64 Linux", as it is redundant, they do mention the OS (bionic or focal) in the name suffix
* Cross builds mention the target in the prefix and the OS (always bionic) in the suffix
* the macos native build simply says "macos native"
ACKs for top commit:
practicalswift:
ACK fa5c4f12f5: patch looks correct!
hebasto:
ACK fa5c4f12f5, I have reviewed the code and it looks OK, I agree it can be merged.
RiccardoMasutti:
ACK fa5c4f1
Tree-SHA512: 856deb0577c97c70069ef1d369991addc49522135c0ad9e382218fd79ba3d55a95d6c601288dcef0510764b92fbd30a9d7de32b08dc5be55482deab14049b892
1e62350ca2 refactor: Improve use of explicit keyword (Fabian Jahr)
c502a6dbfb lint: Use c++17 std in cppcheck linter (Fabian Jahr)
Pull request description:
I found the `extended-lint-cppcheck` linter still uses `std=c++11` when reviewing #20471. The only difference in the output after this change is one line is missing:
```
src/script/descriptor.cpp:159:5: warning: Struct 'PubkeyProvider' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
```
After some digging, I am still not sure why this one is ignored with c++17 when 40 other`noExplicitConstructor` warnings were still appearing.
In the second commit, I fix these warnings, adding `explicit` where appropriate and adding fixes to ignore otherwise.
ACKs for top commit:
practicalswift:
cr ACK 1e62350ca2: patch looks correct!
MarcoFalke:
review ACK 1e62350ca2
Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12
Tidied up debug and release configuration blocks in common project file to avoid duplication.
Updated appveyor config to use latest Visual Studio 2019 image.
Changed appveyor config file hash to use a new version of Qt pre-compiled binaries built for Visual Studio 2019 v16.8.1.
Bumped vcpkg version to tag '2020.11-1' for binary caching feature.
See #20392 for related discussion.
2b356117e9 ci: no-longer exclude feature_block in TSAN job (fanquake)
Pull request description:
The TSAN job is now running on Cirrus.
Increase the allocated memory to the [maximum allowed](https://cirrus-ci.org/guide/linux/#linux-containers).
ACKs for top commit:
jonasschnelli:
utACK 2b356117e9 - checked the CI run and confirmed that the feature_block runs: https://cirrus-ci.com/task/6008403543719936?command=ci#L3249
MarcoFalke:
review ACK 2b356117e9
Tree-SHA512: b774995600361c74bc3267b566e12add66a4604bdf88f6e3f69669edbb8d7aff6f20fdbf0ef98187be4730ce4e18b1939bbcecd993a5c5c1ff40b237c7921b71
8963b2c71f qt: Improve comments in WalletController::getOrCreateWallet() (Hennadii Stepanov)
5fcfee68af qt: Call setParent() in the parent's context (Hennadii Stepanov)
5659e73493 qt: Add ObjectInvoke template function (Hennadii Stepanov)
Pull request description:
The `setParent(parent)` internally calls `QCoreApplication::sendEvent(parent, QChildEvent)` that implies running in the thread which created the parent object. That is not the case always, and an internal assertion fails in the debug mode.
Steps to reproduce this issue on master (007e15dcd7) on Linux Mint 20 (x86_64):
```
$ make -C depends DEBUG=1
$ CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure
$ make
$ QT_FATAL_WARNINGS=1 lldb src/qt/bitcoin-qt -- --regtest -debug=qt
(lldb) target create "src/qt/bitcoin-qt"
Current executable set to '/home/hebasto/GitHub/bitcoin/src/qt/bitcoin-qt' (x86_64).
(lldb) settings set -- target.run-args "--regtest" "-debug=qt"
(lldb) run
Process 431562 launched: '/home/hebasto/GitHub/bitcoin/src/qt/bitcoin-qt' (x86_64)
# load wallet via GUI
Process 431562 stopped
* thread #24, name = 'QThread', stop reason = signal SIGABRT
frame #0: 0x00007ffff794518b libc.so.6`__GI_raise(sig=2) at raise.c:51:1
(lldb) bt
* thread #24, name = 'QThread', stop reason = signal SIGABRT
* frame #0: 0x00007ffff794518b libc.so.6`__GI_raise(sig=2) at raise.c:51:1
frame #1: 0x00007ffff7924859 libc.so.6`__GI_abort at abort.c:79:7
frame #2: 0x0000555556508ec4 bitcoin-qt`::qt_message_fatal((null)=<unavailable>, context=<unavailable>, message=<unavailable>) at qlogging.cpp:1690:15
frame #3: 0x00005555565099cf bitcoin-qt`QMessageLogger::fatal(this=<unavailable>, msg=<unavailable>) const at qlogging.cpp:796:21
frame #4: 0x000055555650479d bitcoin-qt`qt_assert_x(where=<unavailable>, what=<unavailable>, file=<unavailable>, line=<unavailable>) at qglobal.cpp:3088:46
frame #5: 0x0000555556685733 bitcoin-qt`QCoreApplicationPrivate::checkReceiverThread(receiver=0x0000555557b27510) at qcoreapplication.cpp:557:5
frame #6: 0x00005555567ced86 bitcoin-qt`QApplication::notify(this=0x00007fffffffd4a0, receiver=0x0000555557b27510, e=0x00007fff9a7f8ce0) at qapplication.cpp:2956:27
frame #7: 0x0000555556685d31 bitcoin-qt`QCoreApplication::notifyInternal2(receiver=0x0000555557b27510, event=0x00007fff9a7f8ce0) at qcoreapplication.cpp:1024:24
frame #8: 0x00005555566c9224 bitcoin-qt`QObjectPrivate::setParent_helper(QObject*) [inlined] QCoreApplication::sendEvent(event=<unavailable>, receiver=<unavailable>) at qcoreapplication.h:233:59
frame #9: 0x00005555566c9210 bitcoin-qt`QObjectPrivate::setParent_helper(this=0x00007fff85855260, o=0x0000555557b27510) at qobject.cpp:2036
frame #10: 0x00005555566c9b41 bitcoin-qt`QObject::setParent(this=<unavailable>, parent=<unavailable>) at qobject.cpp:1980:24
frame #11: 0x0000555555710be8 bitcoin-qt`WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> >) + 2534
...
```
Fixes#18835.
ACKs for top commit:
ryanofsky:
Code review ACK 8963b2c71f. No changes since last review, just rebase because of conflict on some adjacent lines
jonasschnelli:
utACK 8963b2c71f
Tree-SHA512: fef615904168717df3d8a0bd85eccc3eef990cc3e66c9fa280c8ef08ea009a7cb5a2a4f868ed0be3c0fe5bf683e8465850b5958deb896fdadd22d296186c9586
db058efeb0 sync: use HasReason() in double lock tests (Vasil Dimov)
a21dc469cc sync: const-qualify the argument of double_lock_detected() (Vasil Dimov)
6d3689fcf6 sync: print proper lock order location when double lock is detected (Vasil Dimov)
Pull request description:
Before:
```
Assertion failed: detected double lock at src/sync.cpp:153, details in debug log.
```
After:
```
Assertion failed: detected double lock for 'm' in src/test/sync_tests.cpp:40 (in thread ''), details in debug log.
```
ACKs for top commit:
jonasschnelli:
utACK db058efeb0
ajtowns:
ACK db058efeb0
hebasto:
ACK db058efeb0, tested on Linux Mint 20 (x86_64).
Tree-SHA512: 452ddb9a14e44bb174135b39f2219c76eadbb8a6c0e80d64a25f995780d6dbc7b570d9902616db94dbfabaee197b5828ba3475171a68240ac0958fb203a7acdb
053b4fbad8 doc: Release note regarding -rpcauth validation (João Barbosa)
46001323b1 rpc: Validate -rpcauth arguments (João Barbosa)
d37c813a43 rpc: Refactor to process -rpcauth once (João Barbosa)
Pull request description:
Invalid `-rpcauth` arguments are currently silently ignored. This make server initialization fail if any `-rpcauth` is invalid.
ACKs for top commit:
MarcoFalke:
review ACK 053b4fbad8
jonatack:
ACK 053b4fbad8
ryanofsky:
Code review ACK 053b4fbad8. Only changes since last review are moving a variable declaration and adding a comment, release notes, and a `const`.
Tree-SHA512: c99923d4a121f0c9f882b07f5402ea53e9b2d9455ad34468a094ffab1d64df26c82e1279734c0d42bc2e113eae7b581fbc3be52f3ed4a2d7450d11793afcf406
fada2dfcac test: Fix wallet_multiwallet issue on windows (MarcoFalke)
Pull request description:
The error message on windows:
> 2020-11-30T18:10:47.536032Z ListWalletDir: Error scanning C:\Users\user\AppData\Local\Temp\test_runner_₿_🏃_20201130_181042\wallet_multiwallet_0\node0\regtest\wallets\self_walletdat_symlink: boost::filesystem::status: The name of the file cannot be resolved by the system: "C:\Users\user\AppData\Local\Temp\test_runner_₿_🏃_20201130_181042\wallet_multiwallet_0\node0\regtest\wallets\self_walletdat_symlink\wallet.dat"
ACKs for top commit:
promag:
Code review ACK fada2dfcac. Although it could ignore (don't log) directories that lead to no permission error.
fanquake:
ACK fada2dfcac
Tree-SHA512: b475162cc3cd1574209d916605b229a79c8089714295f5e16569b71f958f0007d54dc76833938492d931387784588b11b73e3ef00f963540af42c079417f8d72
9b74461fa2 refactor: Assert before dereference in CWallet::GetDatabase (João Barbosa)
021feb3187 refactor: Drop redudant CWallet::GetDBHandle (João Barbosa)
Pull request description:
ACKs for top commit:
achow101:
Code Review ACK 9b74461fa2
meshcollider:
utACK 9b74461fa2
ryanofsky:
Code review ACK 9b74461fa2. Changes since last review: rebasing due to conflict, dropping wallet path commit c6a5cd7a64c78b162f545a3467d0fea7dcaadfcc as suggested in discussion, making GetDatabase() const in the earlier commit. Giving more descriptive title like
Tree-SHA512: 68cf3b5e9fe0acb3a5cd081086629989f213f1904cc344e5775767b56759a7d905b1e1c303afbe40f172ff81bf07f3719b59d8f6ec2de3fdd53cd0e2d220fb25
17a5f172fa fuzz: Make addrman fuzzing harness deterministic (practicalswift)
Pull request description:
Make `CAddrMan` fuzzing harness deterministic.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
Crypt-iQ:
utACK 17a5f172fa
Tree-SHA512: 725f983745233e9b616782247fa18847e483c074ca4336a5beea8a9009128c3a74b4d50a12662d8ca2177c2e1fc5fc121834df6b459ac0af43c931d77ef7c4d8
982e548a9a Don't set BDB flags when configuring without (Jonas Schnelli)
Pull request description:
Configuring `--without-bdb` on MacOS leads to a compile error (when BerkeleyDB is not installed). `brew --prefix berkeley-db4` always reports the target directory (even if not installed).
This PR prevents BDB_CFLAGS (et al) from being populated when configuring `--without-bdb`
```
ld: warning: directory not found for option '-L/Users/user/Documents/homebrew/Cellar/berkeley-db@4/4.8.30/lib'
ld: warning: directory not found for option '-L/Users/user/Documents/homebrew/Cellar/berkeley-db@4/4.8.30/lib'
ld: library not found for -ldb_cxx-4.8
ld: library not found for -ldb_cxx-4.8
```
ACKs for top commit:
promag:
Tested ACK 982e548a9a.
hebasto:
ACK 982e548a9a, tested on macOS 11 Big Sur.
Tree-SHA512: f8ca0adca0e18e2de4c0f99d5332cba70d957a9d31a357483b43dcf61c2ed4749d223eabadd45fdbf3ef0781c6b37217770e9aa935b5207eaf7f87c5bdfe9e95
1d578c078f doc: Add bash as an OpenBSD dependency (Emil Engler)
Pull request description:
If we require Python for the test framework, we should also require
bash. It is required for the linters and other scripts and does not
comes in a default OpenBSD installation.
ACKs for top commit:
practicalswift:
ACK 1d578c078f
RiccardoMasutti:
ACK 1d578c0
theStack:
ACK 1d578c078f
Tree-SHA512: ef14e801983093a99d4c28d134adbc589ca06e5437bf1ff34f8e593968c59793e9d99e8a48f577f847acdfc5185e193276526894b4c632c59d66d87939977910
86b1ab64b1 refactor: Replace deprecated Qt::SystemLocale{Short,Long}Date (Hennadii Stepanov)
Pull request description:
As all deprecated warning in Qt 5.15.0 were eliminated in #46, Qt 5.15.1 introduced another one that is fixed in this PR.
Required for https://github.com/bitcoin/bitcoin/pull/20182.
Details in Qt docs:
- https://doc.qt.io/qt-5/qdatetime.html#toString-1
- https://doc.qt.io/qt-5/qdate.html#toString-1
ACKs for top commit:
jarolrod:
Tested ACK 86b1ab6 on MacOS 10.15.7 and Arch Linux both with Qt 5.15.1
jonasschnelli:
Tested ACK 86b1ab64b1
Tree-SHA512: 1dbba8ee70c895bf58317172a9901cdbe5503b1d6258f51caaae88d88d332d9fbd4697c995192d31e3618ddfd532c5f5881289b3af1184422e5a9263a1224115
f15e780b9e refactor: Clean up CTxMemPool initializer list (Elle Mouton)
e3310692d0 refactor: Make CTxMemPool::m_check_ratio a const and a constructor argument (Elle Mouton)
9d4b4b2c2c refactor: Avoid double to int cast for nCheckFrequency (Elle Mouton)
Pull request description:
This PR cleans up the CTxMemPool interface by including the ratio used to determine when a mempool sanity check should run in the constructor of CTxMempool instead of using nCheckFrequency which required a cast from a double to a uint32_t. Since nCheckFrequency (now called m_check_ratio) is set in the constructor and only every read from there after, it can be turned into a const and no longer needs to be guarded by the 'cs' lock.
Since nCheckFrequency/m_check_ratio no longer needs to lock the 'cs' mutux, mutex lock line in the "CTxMempool::check" function can be moved below where the m_check_ratio variable is checked. Since the variable is 0 by default (meaning that "CTxMempool::check" will most likely not run its logic) this saves us from unnecessarily grabbing the lock.
ACKs for top commit:
jnewbery:
utACK f15e780b9e
MarcoFalke:
ACK f15e780b9e👘
glozow:
utACK f15e780b9e
theStack:
Code Review ACK f15e780b9e
Tree-SHA512: d83f3b5311ca128847b621e5e999c7e1bf0f4e6261d4cc090fb13e229a0f7eecd66ad997f654f50a838baf708d1515740aa3bffc244909a001d01fd5ae398b68
629a9299b2 Move WalletImpl from interfaces/wallet.cpp to wallet/interfaces.cpp (Russell Yanofsky)
2a26771d81 Move ChainImpl from interfaces/chain.cpp to node/interfaces.cpp (Russell Yanofsky)
12bd0fc9d7 Move NodeImpl from interfaces/node.cpp to node/interfaces.cpp (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
Move `NodeImpl` from `interfaces/node.cpp` to `node/interfaces.cpp`
Move `ChainImpl` from `interfaces/chain.cpp` to `node/interfaces.cpp`
Move `WalletImpl` from `interfaces/wallet.cpp` to `wallet/interfaces.cpp`
No changes to any classes (can review with `git diff --color-moved=dimmed_zebra`)
Motivation for this change is to move node and wallet code to respective directories where it might fit in better than `src/interfaces/`, but also to remove all unnecessary code from `src/interfaces/` to unblock #19160 review, which has been hung up partially because of code organization. Building on top of this PR, #19160 should now be able to organize interface implementations more understandably in `src/node/` `src/wallet/` `src/ipc/` and `src/init/` directories instead of having so much functionality all in `src/interfaces/`
ACKs for top commit:
promag:
Code review ACK 629a9299b2.
MarcoFalke:
review ACK 629a9299b2🔺
Tree-SHA512: 87c2b8fd51519bbd4e5ad3539a79debcf88c3bf021eb28c63f3f555186538b62a0c4cc1a3f07cfb4ff13aea8b0b2fdde505d81f22a5e5fd12a6e375b55a92ab8
ce9dd45422 Add [[nodiscard]] to RenameOver(...) (practicalswift)
9429a398e2 Handle rename failure in DumpMempool(...) by using RenameOver(...) return value (practicalswift)
Pull request description:
Handle rename failure in `DumpMempool(...)` by using the `RenameOver(...)` return value.
Add `[[nodiscard]]` to `RenameOver(...)` to reduce the risk of similar rename issues in the future.
ACKs for top commit:
vasild:
ACK ce9dd454
theStack:
ACK ce9dd45422🏷️
Tree-SHA512: 1e63d7f3061e1f6ea2df5750dbc1547a39bd50b6c529812a0c8a0c11d3100c241afdf14094e69b69a38bade7e54a12b2a42888545874398eaf5d02421b57e874
830ddf4139 Drop noop gcc version checks (Hennadii Stepanov)
Pull request description:
Since #20413 the minimum required GCC version is 7.
ACKs for top commit:
fanquake:
ACK 830ddf4139
Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964
