Age | Commit message (Collapse) | Author |
|
773c42b265fb2212b5cb8785b7226a206d063543 tests: Test that a fully signed tx given to signrawtx is unchanged (Andrew Chow)
Pull request description:
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.
ACKs for top commit:
MarcoFalke:
ACK 773c42b265 🕶
Tree-SHA512: b32afd5f2667c817ee59c37af78cb37352e7c22d0271833d446fd61f11b354974694128a3b408f214db459caef3fd67ecdbf1664280ac2de602215f05aa4a6de
|
|
fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2 refactor: Use C++17 std::array where possible (MarcoFalke)
Pull request description:
Using the C++11 std::array with explicit template parameters is problematic because overshooting the size will fill the memory with default constructed types.
For example,
```cpp
#include <array>
#include <iostream>
int main()
{
std::array<int, 3> a{1, 2};
for (const auto& i : a) {
std::cout << i << std::endl; // prints "1 2 0"
}
}
```
ACKs for top commit:
jonasschnelli:
Code Review ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2
practicalswift:
cr ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2
vasild:
ACK fac7ab1d
promag:
Code review ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2.
Tree-SHA512: ef7e872340226e0d6160e6fd66c6ca78b2ef9c245fa0ab27fe4777aac9fba8d5aaa154da3d27b65dec39a6a63d07f1063c3a8ffb667a98ab137756a1a0af2656
|
|
e373959d6fe90cc4507024a6b31a706bfc5bd0c8 Android : Ensure pic build for bdb (Block Mechanic)
Pull request description:
This pr ensures android builds for the BDB dependency have the pic flag enabled. Android builds were failing to link with reloc errors.
ACKs for top commit:
laanwj:
Code review ACK e373959d6fe90cc4507024a6b31a706bfc5bd0c8
jonasschnelli:
utACK e373959d6fe90cc4507024a6b31a706bfc5bd0c8
Tree-SHA512: 68319ed7cc0bd295eaa87dd53ba051daeb1456bc3ab9b48ca0c4b831a9c8da1073480478efde73689f0e403e37409a8459229264656f05ba5fef6c257a74f977
|
|
faa05854f832405231c9198787a4eafe2cd4c5f0 util: Remove probably misleading TODO (MarcoFalke)
fac5efe730ab5b8694920547203deefc5252d294 util: Add Assume() identity function (MarcoFalke)
fa861569dcfff9e72686dc5f30b1faa645b4d54e util: Allow Assert(...) to be used in all contexts (practicalswift)
Pull request description:
This is needed for #20138. Please refer to the added documentation for motivation.
ACKs for top commit:
practicalswift:
cr ACK faa05854f832405231c9198787a4eafe2cd4c5f0
jnewbery:
utACK faa05854f832405231c9198787a4eafe2cd4c5f0
hebasto:
ACK faa05854f832405231c9198787a4eafe2cd4c5f0, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 72165fbd898b92ab9a79b070993fa1faa86c2e3545b6645e72c652bda295d5107bc298d0482bf3aaf0926fc0c3e6418a445c0e073b08568c44231f547f76a688
|
|
86e6add5ca7d425faa314103df6617044a910ab3 doc: rename CODEOWNERS to REVIEWERS (Adam Jonas)
Pull request description:
This PR renames the CODEOWNERS file to REVIEWERS and works with DrahtBot to leave a comment requesting a review.
Testing of the functionality was done on https://github.com/adamjonas/bitcoin-codeowners-sandbox/pulls.
~EDIT: [after further testing of a fake organization](https://github.com/jonasorg/bitcoin-codeowners-sandbox-org/pulls), it appears that in order to be automatically tagged, the reviewer requires write level permissions. This is undocumented and will obviously close the circle of who can be tagged and so this PR reverts the addition of the file in #18949.~
~This removes a line not being parsed in the CODEOWNERS file introduced in #18949. While the pattern [checked out](https://github.com/bitcoin/bitcoin/pull/18949#issuecomment-648859076) using `git ls-files`, it is preventing the CODEOWNERS file from automatically tagging reviewers. For future modifications to this file, note that [any line failing to parse causes the entire file to stop identifying codeowners](http://www.benjaminoakes.com/git/2018/08/10/Testing-changes-to-GitHub-CODEOWNERS/#:~:text=warning).~
~I experimented in a [sandbox repo](https://github.com/adamjonas/bitcoin-codeowners-sandbox/pulls) to discover the offending line and this change appears to fix the problem.~
ACKs for top commit:
laanwj:
Doesn't look like github is going to fix this any time soon and besides REVIEWERS is a better name so ACK 86e6add5ca7d425faa314103df6617044a910ab3
Tree-SHA512: b3425eca4be62f829f0edcfa08232df310680835c2f60e9fa36956a6b59a2b0cd0ab580c572c87affccd843fbd849f4f99e4b3d9d7b6070ab117953f04e17f5a
|
|
installed
d3ef947524a07f8d7fbad5b95781ab6cacb1cb49 build: Check that Homebrew's berkeley-db4 package is actually installed (Hennadii Stepanov)
Pull request description:
On master (a0489f3472f3799dc1ece32a59556fd239c4c14b) the `configure` script is not able to determine that Homebrew's `berkeley-db4` package is uninstalled. This causes a compile error on macOS.
With this PR, and with the [uninstalled](https://stackoverflow.com/questions/20802320/detect-if-homebrew-package-is-installed) `berkeley-db4` package:
```
% ./configure -q
configure: error: libdb_cxx headers missing, Bitcoin Core requires this library for BDB wallet support (--without-bdb to disable BDB wallet support)
```
Related #20478.
ACKs for top commit:
promag:
Tested ACK d3ef947524a07f8d7fbad5b95781ab6cacb1cb49.
willcl-ark:
tACK d3ef947524a07f8d7fbad5b95781ab6cacb1cb49
jonasschnelli:
utACK d3ef947524a07f8d7fbad5b95781ab6cacb1cb49
Tree-SHA512: 8dc532e08249ec63bd357594aa458d314b6e8537fc63f5b1d509c84d0d71d5b1f70172caa1a7efe2fc8af31c829e7982a0695cf3fbe5cbc477019550269915e1
|
|
|
|
2f6fe4e4e9e9e35e713c0a20cf891b023592110a 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 2f6fe4e4e9e9e35e713c0a20cf891b023592110a: 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.
|
|
|
|
cadb77a6ab8a3e6f56062cfaec4dd8168c71b39d net: Add compat.h header for htonl function (Hennadii Stepanov)
f796f0057bc7dad8e7065831b07f432fc0fb9f08 net: Drop unneeded headers when compat.h included (Hennadii Stepanov)
467c34644861a5267601255650e27c7aadab31dc 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 cadb77a6ab8a3e6f56062cfaec4dd8168c71b39d: patch looks even better
laanwj:
Code review ACK cadb77a6ab8a3e6f56062cfaec4dd8168c71b39d
Tree-SHA512: 625ff90b2806310ab856a6ca1ddb6d9a85aa70f342b323e8525a711dd12219a1ecec8373ec1dca5a0653ffb11f9b421753887b25615d991ba3132c1cca6a3c6e
|
|
2c69381f3de5091e103cb8bef299aba321503e7c Removed redundant git pull from appveyor config. (Aaron Clauson)
8b99e609e7da5dd3601e9214d8f869e96108fffe 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 2c69381f3de5091e103cb8bef299aba321503e7c
laanwj:
Concept ACK 2c69381f3de5091e103cb8bef299aba321503e7c
Tree-SHA512: 372f5b8b3b5f7e56b78045f9fc06a22fd9f5366d06e99e2f1eaad6d741680da74d0e6371e7bc580cb41f4d6696b2101b950167309d33e98242578b458ace813a
|
|
Qt on Linux
c82d15b6d189983ebf82f35ade33d830870f25c6 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 (e2ff5e7b35d71195278d2a2ed9485f141de33d7a):
```
$ 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.html
Fixes #20423
ACKs for top commit:
MarcoFalke:
review ACK c82d15b6d189983ebf82f35ade33d830870f25c6
Tree-SHA512: 0f2a3712e90de881d00f8e56c363edde33dd4f5c117df5744ab4e51d0a8146331de7236bc8329d68ddd91535cd853e68ee80ef4cceb6a909786abfd8881b01e8
|
|
|
|
fad7be584ffaf8099cc099d9378ba831c9483260 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 fad7be584ffaf8099cc099d9378ba831c9483260
Tree-SHA512: d43ba9d07273486858f65a26326cc6637ef743bf7b400e5048ba7eac266fb1893283e6503dd49f179caa1abab2977315fb70ba9fba34be9a817a74259d8e4034
|
|
fa5c4f12f5d017d8dbe7700ca9688189eacbf32b 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 fa5c4f12f5d017d8dbe7700ca9688189eacbf32b: patch looks correct!
hebasto:
ACK fa5c4f12f5d017d8dbe7700ca9688189eacbf32b, I have reviewed the code and it looks OK, I agree it can be merged.
RiccardoMasutti:
ACK fa5c4f1
Tree-SHA512: 856deb0577c97c70069ef1d369991addc49522135c0ad9e382218fd79ba3d55a95d6c601288dcef0510764b92fbd30a9d7de32b08dc5be55482deab14049b892
|
|
explicit usage
1e62350ca20898189904a88dfef9ea11ddcd8626 refactor: Improve use of explicit keyword (Fabian Jahr)
c502a6dbfb854ca827a5a3925394f9e09d29b898 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 1e62350ca20898189904a88dfef9ea11ddcd8626: patch looks correct!
MarcoFalke:
review ACK 1e62350ca20898189904a88dfef9ea11ddcd8626
Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12
|
|
On CentOS 8 the forced '-pch' option breaks Qt build.
Removing '-pch' option does not affect build time for other Linux
systems.
|
|
causing sporadic 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.
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.
|
|
|
|
2b356117e94f9ef27b67a8e98663f5d676f58c11 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 2b356117e94f9ef27b67a8e98663f5d676f58c11 - checked the CI run and confirmed that the feature_block runs: https://cirrus-ci.com/task/6008403543719936?command=ci#L3249
MarcoFalke:
review ACK 2b356117e94f9ef27b67a8e98663f5d676f58c11
Tree-SHA512: b774995600361c74bc3267b566e12add66a4604bdf88f6e3f69669edbb8d7aff6f20fdbf0ef98187be4730ce4e18b1939bbcecd993a5c5c1ff40b237c7921b71
|
|
8963b2c71f120b2746396c4987392f0105c8dd60 qt: Improve comments in WalletController::getOrCreateWallet() (Hennadii Stepanov)
5fcfee68af47d4a891ae9c9964d73886f0f01d7d qt: Call setParent() in the parent's context (Hennadii Stepanov)
5659e73493fcdfb5d0cb9d686c24c4fbe1c217ed 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 (007e15dcd7f8b42501e31cc36343655c53027077) 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 8963b2c71f120b2746396c4987392f0105c8dd60. No changes since last review, just rebase because of conflict on some adjacent lines
jonasschnelli:
utACK 8963b2c71f120b2746396c4987392f0105c8dd60
Tree-SHA512: fef615904168717df3d8a0bd85eccc3eef990cc3e66c9fa280c8ef08ea009a7cb5a2a4f868ed0be3c0fe5bf683e8465850b5958deb896fdadd22d296186c9586
|
|
detected
db058efeb0821cb5022e3b29e0aff3627d7aaf83 sync: use HasReason() in double lock tests (Vasil Dimov)
a21dc469ccf076ca3b07b1adbd8bf667145f1c44 sync: const-qualify the argument of double_lock_detected() (Vasil Dimov)
6d3689fcf6cff397187028344570489db3e6ecf4 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 db058efeb0821cb5022e3b29e0aff3627d7aaf83
ajtowns:
ACK db058efeb0821cb5022e3b29e0aff3627d7aaf83
hebasto:
ACK db058efeb0821cb5022e3b29e0aff3627d7aaf83, tested on Linux Mint 20 (x86_64).
Tree-SHA512: 452ddb9a14e44bb174135b39f2219c76eadbb8a6c0e80d64a25f995780d6dbc7b570d9902616db94dbfabaee197b5828ba3475171a68240ac0958fb203a7acdb
|
|
The TSAN job is now running on Cirrus.
Increase the allocated memory to the maximum allowed.
|
|
053b4fbad8729308774d5e7b53f53c12627fa88b doc: Release note regarding -rpcauth validation (João Barbosa)
46001323b1f4a57d8d6805f1bc39a5b8d401f0c5 rpc: Validate -rpcauth arguments (João Barbosa)
d37c813a43166f559a4e2d1c22e7243f70301291 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 053b4fbad8729308774d5e7b53f53c12627fa88b
jonatack:
ACK 053b4fbad8729308774d5e7b53f53c12627fa88b
ryanofsky:
Code review ACK 053b4fbad8729308774d5e7b53f53c12627fa88b. Only changes since last review are moving a variable declaration and adding a comment, release notes, and a `const`.
Tree-SHA512: c99923d4a121f0c9f882b07f5402ea53e9b2d9455ad34468a094ffab1d64df26c82e1279734c0d42bc2e113eae7b581fbc3be52f3ed4a2d7450d11793afcf406
|
|
fada2dfcac1c4b47ee76b877d91d515cf1d36410 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 fada2dfcac1c4b47ee76b877d91d515cf1d36410. Although it could ignore (don't log) directories that lead to no permission error.
fanquake:
ACK fada2dfcac1c4b47ee76b877d91d515cf1d36410
Tree-SHA512: b475162cc3cd1574209d916605b229a79c8089714295f5e16569b71f958f0007d54dc76833938492d931387784588b11b73e3ef00f963540af42c079417f8d72
|
|
9b74461fa293453a9eb0b1717b30b3f7fa778d91 refactor: Assert before dereference in CWallet::GetDatabase (João Barbosa)
021feb3187b207d511561c1f0ffd7f9e5e0c9c1d refactor: Drop redudant CWallet::GetDBHandle (João Barbosa)
Pull request description:
ACKs for top commit:
achow101:
Code Review ACK 9b74461fa293453a9eb0b1717b30b3f7fa778d91
meshcollider:
utACK 9b74461fa293453a9eb0b1717b30b3f7fa778d91
ryanofsky:
Code review ACK 9b74461fa293453a9eb0b1717b30b3f7fa778d91. 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
|
|
|
|
|
|
2d8099c713dfd4b546150fd53c2e4f364b9009f4 Mention units of MAX_STANDARD_ policy constants (Pieter Wuille)
84e29c7c0141b52044020ec0c5dfa8a462b7e97f Mention in validation that IsWitnessStandard tests for P2TR (Pieter Wuille)
f867cbcc268a3bfaeef5510a7e40e6d3c0818b6d Clean up assets test minimizer LDFLAGS (Pieter Wuille)
ea0e78677bdbe3313f594118c500cf7784c56970 Document additional IsWitnessStandard behavior (Pieter Wuille)
6040de9a46725826330cd63cdf76e2121a18e728 Add comments on CPubKey::IsValid (Pieter Wuille)
8dbb7de67ce0a71f5fc54289c0ff048ac8dd0acc Add comments to VerifyTaprootCommitment (Pieter Wuille)
cdf900cbf26db05c7edb398ea645f1d23049d810 Document need_vin_vout_mismatch argument to make_spender (Pieter Wuille)
18246ed5f09dd078fa1410b7ec2ba4379cc5e032 Fix and improve taproot_construct comments (Pieter Wuille)
Pull request description:
Addressing some review comments raised here: https://github.com/bitcoin/bitcoin/pull/19953#pullrequestreview-512238027 and https://github.com/bitcoin/bitcoin/pull/19953#pullrequestreview-513499921
ACKs for top commit:
jonatack:
ACK 2d8099c per `git range-diff 5009159 4f10965 2d8099c`
ariard:
ACK 2d8099c, only changes are comment light improvements on IsValid/IsWitnessStandard.
Tree-SHA512: c4881546c379ea8efc7ef99a43cbf3b9cd3f9dde5fd97a07ee66f2b593c78aef0bd8784853c5c9c737b66c269241a1048bbbdd6c964a3d872efd8ba0ec410b68
|
|
17a5f172fa9ec509b1c3f950ee8dfb6f025534d2 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 17a5f172fa9ec509b1c3f950ee8dfb6f025534d2
Tree-SHA512: 725f983745233e9b616782247fa18847e483c074ca4336a5beea8a9009128c3a74b4d50a12662d8ca2177c2e1fc5fc121834df6b459ac0af43c931d77ef7c4d8
|
|
|
|
982e548a9a78b1b0abad59b54c780b6b06570452 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 982e548a9a78b1b0abad59b54c780b6b06570452.
hebasto:
ACK 982e548a9a78b1b0abad59b54c780b6b06570452, tested on macOS 11 Big Sur.
Tree-SHA512: f8ca0adca0e18e2de4c0f99d5332cba70d957a9d31a357483b43dcf61c2ed4749d223eabadd45fdbf3ef0781c6b37217770e9aa935b5207eaf7f87c5bdfe9e95
|
|
ec4a46dd9c158e1a0de144e9abb260da941aa702 build: Drop unneeded IOKit framework dependency (Hennadii Stepanov)
65afe4cb69e80278de0f584b6eeb6741f02a31eb build: Drop unneeded ApplicationServices framework dependency (Hennadii Stepanov)
Pull request description:
Bitcoin Core codebase does not contain direct dependencies on the `ApplicationServices` and `IOKit` frameworks.
ACKs for top commit:
jonasschnelli:
utACK ec4a46dd9c158e1a0de144e9abb260da941aa702
practicalswift:
cr ACK ec4a46dd9c158e1a0de144e9abb260da941aa702: patch looks correct!
promag:
Tested ACK ec4a46dd9c158e1a0de144e9abb260da941aa702 (not depends build).
Tree-SHA512: 47b5ad87d761992850133a921f07d485565c70ba2909a3289050f406e6dbd39ad49e1aeeb6cad79c6914385a72ddffd273dfadd69259a35545a13cd17d0e5043
|
|
1d578c078f0ce00cb032d3c6c689fd199b8d2f35 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 1d578c078f0ce00cb032d3c6c689fd199b8d2f35
RiccardoMasutti:
ACK 1d578c0
theStack:
ACK 1d578c078f0ce00cb032d3c6c689fd199b8d2f35
Tree-SHA512: ef14e801983093a99d4c28d134adbc589ca06e5437bf1ff34f8e593968c59793e9d99e8a48f577f847acdfc5185e193276526894b4c632c59d66d87939977910
|
|
Qt::SystemLocale{Short,Long}Date
86b1ab64b1a5b56518787ef16ea54ddbbc97d83e 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 86b1ab64b1a5b56518787ef16ea54ddbbc97d83e
Tree-SHA512: 1dbba8ee70c895bf58317172a9901cdbe5503b1d6258f51caaae88d88d332d9fbd4697c995192d31e3618ddfd532c5f5881289b3af1184422e5a9263a1224115
|
|
f15e780b9e57554c723bc02aa41150ecf3e3a8c9 refactor: Clean up CTxMemPool initializer list (Elle Mouton)
e3310692d0e9720e960b9785274ce1f0b58b4cd7 refactor: Make CTxMemPool::m_check_ratio a const and a constructor argument (Elle Mouton)
9d4b4b2c2c49774523de740d6492ee5b1ee15e74 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 f15e780b9e57554c723bc02aa41150ecf3e3a8c9
MarcoFalke:
ACK f15e780b9e57554c723bc02aa41150ecf3e3a8c9 👘
glozow:
utACK https://github.com/bitcoin/bitcoin/pull/20222/commits/f15e780b9e57554c723bc02aa41150ecf3e3a8c9
theStack:
Code Review ACK f15e780b9e57554c723bc02aa41150ecf3e3a8c9
Tree-SHA512: d83f3b5311ca128847b621e5e999c7e1bf0f4e6261d4cc090fb13e229a0f7eecd66ad997f654f50a838baf708d1515740aa3bffc244909a001d01fd5ae398b68
|
|
629a9299b2a7241a3fa7d597cb34abcbe1af9255 Move WalletImpl from interfaces/wallet.cpp to wallet/interfaces.cpp (Russell Yanofsky)
2a26771d8161d30be1853a35acfee588cce03634 Move ChainImpl from interfaces/chain.cpp to node/interfaces.cpp (Russell Yanofsky)
12bd0fc9d70333c54c83ebb08c734272dbd330c2 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 629a9299b2a7241a3fa7d597cb34abcbe1af9255.
MarcoFalke:
review ACK 629a9299b2a7241a3fa7d597cb34abcbe1af9255 🔺
Tree-SHA512: 87c2b8fd51519bbd4e5ad3539a79debcf88c3bf021eb28c63f3f555186538b62a0c4cc1a3f07cfb4ff13aea8b0b2fdde505d81f22a5e5fd12a6e375b55a92ab8
|
|
RenameOver(...) return value. Add [[nodiscard]] to RenameOver(...).
ce9dd45422e1f4ecce6df68da086b8bfc2100756 Add [[nodiscard]] to RenameOver(...) (practicalswift)
9429a398e291a1b5edcfc657b94fcaf52cd1d8f9 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 ce9dd45422e1f4ecce6df68da086b8bfc2100756 🏷️
Tree-SHA512: 1e63d7f3061e1f6ea2df5750dbc1547a39bd50b6c529812a0c8a0c11d3100c241afdf14094e69b69a38bade7e54a12b2a42888545874398eaf5d02421b57e874
|
|
This reverts #18949 since CODEOWNERS require write permission.
Instead the REVIEWERS file uses DrahtBot to tag reviewers in PR
comments.
|
|
79bff8e48aca961ec271b0d592aca9278b981e2f Remove NODISCARD (practicalswift)
4848e711076c6ebc5d841feb83baeb6d2bc76c94 scripted-diff: Use [[nodiscard]] (C++17) instead of NODISCARD (practicalswift)
Pull request description:
Remove obsolete `NODISCARD` `ifdef` forest. Use `[[nodiscard]]` (C++17).
ACKs for top commit:
theStack:
ACK 79bff8e48aca961ec271b0d592aca9278b981e2f
fanquake:
ACK 79bff8e48aca961ec271b0d592aca9278b981e2f
Tree-SHA512: 56dbb8e50ed97ecfbce28cdc688a01146108acae49a943e338a8f983f7168914710d36e38632f6a7c200ba6c6ac35b2519e97d6c985e8e7eb23223f13bf985d6
|
|
830ddf413934226d0b6ca99165916790cc52ca18 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 830ddf413934226d0b6ca99165916790cc52ca18
Tree-SHA512: 36264661d6ced1683a0c907efba7c700502acaf8e9fd50d9066bc9c7b877b25165b0684c2d7fe74bd58e500a77d7702bdbdd53691c274f29e4abccd241c10964
|
|
2f5dfe4a7ff12b6b57427374142cdf7e266b73bc depends: build qt in c++17 mode (fanquake)
104e859c9755aee5708ea1934454d88b10c266ff builds: don't pass -silent to qt when building in debug mode (fanquake)
e2c500636cb767347ae2b913345788ad3c3e8279 depends: build zeromq with -std=c++17 (fanquake)
2374f2fbef4359476fe3184e2402a2cc741cefad depends: build Boost with -std=c++17 (fanquake)
2dde55702da30ea568cac8a1d1cbddd652d6958e depends: build bdb with -std=c++17 (fanquake)
Pull request description:
In packages where we are passing `-std=c++11` switch to `-std=c++17`, or, `-std=c++1z` in the case of Qt.
This PR also contains a [commit](https://github.com/bitcoin/bitcoin/commit/104e859c9755aee5708ea1934454d88b10c266ff) that improves debug output when building Qt for debugging (`DEBUG=1`).
Now we'll get output like this:
```bash
g++ -c -pipe -ffunction-sections -O2 -fPIC -std=c++11 -fno-exceptions <lots more> ../../corelib/kernel/qcoreapplication.cpp
```
rather than just:
```bash
compiling ../../corelib/kernel/qcoreapplication.cpp
```
Note that when you look at the DEBUG output for these changes when building Qt, you'll see objects being compiled with a mix of C++11 and C++17. The breakdown is roughly:
1. `qmake` built with `-std=c++11`:
```bash
Creating qmake...
make[1]: Entering directory '<trim>/qt/5.9.8-4110fa99945/qtbase/qmake'
g++ -c -o project.o -std=c++11 -ffunction-sections -O2 -g <trim> <trim>/qt/5.9.8-4110fa99945/qtbase/qmake/project.cpp
# when qmake, Qt also builds some of it's corelib, such as corelib/global/qmalloc.cpp
g++ -c -o qmalloc.o -std=c++11 -ffunction-sections -O2 -g <trim> <trim>/qt/5.9.8-4110fa99945/qtbase/src/corelib/global/qmalloc.cpp
```
2. `qmake` is run, and passed our build options, including `-c++std`:
```bash
make[1]: Entering directory '<trim>/qt/5.9.8-4110fa99945/qtbase'
<trim>qt/5.9.8-4110fa99945/qtbase/bin/qmake -o Makefile qtbase.pro -- -bindir <trim>/native/bin -c++std c++1z -confirm-license <trim>
```
3. After some cleaning and configuring, we actually start to build Qt, as well as it's tools and internal libs:
```bash
Building qt...
make[1]: Entering directory '<trim>/qt/5.9.8-4110fa99945/qtbase/src'
# build libpng, zlib etc
gcc -c -m64 -pipe -pipe -O1 <trim> -o .obj/png.o png.c
# build libQt5Bootstrap, using C++11, which again compiles qmalloc.cpp
make[2]: Entering directory '<trim>/qt/5.9.8-4110fa99945/qtbase/src/tools/bootstrap'
g++ -c -pipe -ffunction-sections -O2 -fPIC -std=c++11 <trim> -o .obj/qmalloc.o ../../corelib/global/qmalloc.cpp
# build a bunch of tools like moc, rcc, uic, qfloat16-tables, qdbuscpp2xml, using C++11
g++ -c -pipe -O2 -std=c++11 -fno-exceptions -Wall -W <trim> -o .obj/rcc.o rcc.cpp
# from here, Qt is compiled with -std=c++1z, including qmalloc.cpp, for the third and final time:
g++ -c -include .pch/Qt5Core <trim> -g -Og -fPIC -std=c++1z -fvisibility=hidden <trim> -o .obj/qmalloc.o global/qmalloc.cpp
```
4. Finally, build tools like `lrelease`, `lupdate`, etc, but back to using -std=c++11
```bash
make[1]: Entering directory '<trim>/qt/5.9.8-4110fa99945/qttools/src/linguist/lrelease'
g++ -c -pipe -O2 -std=c++11 -fno-exceptions -Wall -W <trim> -o .obj/translator.o ../shared/translator.cpp
```
If you dump the debug info from the built Qt libs, they should also tell you that they were compiled with `C++17`:
```bash
objdump -g bitcoin/depends/x86_64-pc-linux-gnu/lib/libQt5Core.a
GNU C++17 9.3.0 -m64 -mtune=generic -march=x86-64 -g -O1 -Og -std=c++17 -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -fasynchronous-unwind-tables -fstack-protector-strong -fstack-clash-protection -fcf-protection
```
ACKs for top commit:
laanwj:
Code review ACK https://github.com/bitcoin/bitcoin/pull/20471/commits/2f5dfe4a7ff12b6b57427374142cdf7e266b73bc
practicalswift:
cr ACK 2f5dfe4a7ff12b6b57427374142cdf7e266b73bc: patch looks correct
fjahr:
Code review ACK 2f5dfe4a7ff12b6b57427374142cdf7e266b73bc
hebasto:
ACK 2f5dfe4a7ff12b6b57427374142cdf7e266b73bc, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: fc5e9d7c7518c68349c8228fb1aead829850373efc960c9b8c079096a83d1dad19c62a9730fce5802322bf07e320960fd47851420d429eda0a87c307f4e8b03a
|
|
3ebde2143aa98af213872b98b474d904e55056f7 [test] Fix wait condition in disconnect_p2ps (Amiti Uttarwar)
Pull request description:
#19315 currently has a [test failure](https://cirrus-ci.com/task/4545582645641216) because of a race. `disconnect_p2ps` is intended to have a `wait_until` clause that prevents this race, but the conditional doesn't match since its comparing two different object types. `MY_SUBVERSION` is defined in messages.py as a byte string, but is compared to the value returned by the RPC. This PR simply converts types to ensure they match, which should prevent the race from occurring.
HUGE PROPS TO jnewbery for discovering the issue 🔎
ACKs for top commit:
jnewbery:
ACK 3ebde2143aa98af213872b98b474d904e55056f7
glozow:
Code review ACK https://github.com/bitcoin/bitcoin/pull/20522/commits/3ebde2143aa98af213872b98b474d904e55056f7
Tree-SHA512: ca096b80a3e4d757a645f38846d6dc89d6a3d35c3435513a72d278e305faddd4aff9e75a767941b51b2abbf59c82679bac1e9a0140d6f285efe3053e51bcc2a8
|
|
8008ef770f3d0b14d03e22371314500373732143 qt: unlock wallet "OK" button bugfix (Michael Dietz)
Pull request description:
When trying to send a transaction from an encrypted wallet, the ask
passphrase dialog would not allow the user to click the "OK" button
and proceed. Therefore it was impossible to send a transaction
through the gui. It was not enabling the "OK" button after the
passphrase was entered by the user, because it was using the same
form validation logic as the "Change passphrase" flow.
I reported this in a comment in https://github.com/bitcoin-core/gui/issues/136. But then I realized this seems to be a flat out bug.
ACKs for top commit:
MarcoFalke:
review ACK 8008ef770f3d0b14d03e22371314500373732143
hebasto:
ACK 8008ef770f3d0b14d03e22371314500373732143, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: cc09b34c7f3aea09729e1c7ccccff05dc11fec56fee2ad369f2d862979572b1edd8b7e738ffe6e91d35d071b819b0c3e0f5d48bf5e27427a80af4a28893f8aaf
|
|
matching RPC endpoint wallet
89bdad5b25ae4ac03a486f729a5b58ae6f21946d RPC/Wallet: unloadwallet: Allow specifying wallet_name param matching RPC endpoint (Luke Dashjr)
Pull request description:
Allow specifying the `wallet_name` param to `unloadwallet` on RPC wallet endpoints, so long as it matches the endpoint wallet.
ACKs for top commit:
jonatack:
ACK 89bdad5b25ae4ac03a486f729a5b58ae6f21946d
MarcoFalke:
review ACK 89bdad5b25ae4ac03a486f729a5b58ae6f21946d
Tree-SHA512: efb399c33f7b5596870a26a8680f453ca47aa7a6db4e550f9435d13044f1c4bad0ae11e8f0205213409d08b75c4188c3be782e54aafab1f65b97eb8cf5c252a9
|
|
MY_SUBVERSION is defined in messages.py as a byte string, but here we were
comparing this value to the value returned by the RPC. Convert to ensure the
types match.
|
|
When trying to send a transaction from an encrypted wallet, the ask
passphrase dialog would not allow the user to click the "OK" button
and proceed. Therefore it was impossible to send a transaction
through the gui. It was not enabling the "OK" button after the
passphrase was entered by the user, because it was using the same
form validation logic as the "Change passphrase" flow.
|