Rather than two lots of logic doing roughly the same thing, dependent on if
you're compiling on Linux or macOS, combine the .DS store generation into
macdeployqtplus.
This also removes the -fancy and -volname options.
Rather than using OSX_QT_TRANSLATIONS which must be manually updated,
and we forget to update anyway, i.e: #19059, automatically find and copy
available translations from the translations directory.
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](104e859c97) 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 2f5dfe4a7f
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
Adjusted version flag behavior in bitcoin-tx, bitcoin-wallet, and
bitcoind to match. Added functionality in gen-manpages.sh to warning when
attempting to generate man pages for binaries built from a dirty
branch.
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 3ebde2143a
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
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.
e416cfc92bf51f6fd088ab61c2306c5e73877dd0 Add MAX_STANDARD_SCRIPTSIG_SIZE to policy (sanket1729)
Pull request description:
Bitcoin core has a standardness rule for max satisfaction script sig size.
This PR adds to the policy header file so that it is documented along with
along policy rules. The initial reasoning that 1650 is an implicit
limit(would not reach assuming all other policy rules are being
followed) is outdated.
As we now know, bitcoin transactions can have spend conditions are more than
just signatures and there may exist p2sh transactions involving 100 byte
preimages that maybe non-standard because of this rule. Because this
rule is no longer implicit, we should explicitly document it in policy
header file
ACKs for top commit:
sipa:
utACK e416cfc92bf51f6fd088ab61c2306c5e73877dd0
practicalswift:
cr ACK e416cfc92bf51f6fd088ab61c2306c5e73877dd0
theStack:
Code Review ACK e416cfc92bf51f6fd088ab61c2306c5e73877dd0
Tree-SHA512: 1a91ee23dfb6085807e04dd0687d7a443e0f3e0f52d0a995a6599dff28533b0b599afba2724735d93948a64a3e25d0bc016ce3e771c0bd453eef78b22dc2369d
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.
Bitcoin core has a standardness rule for max satisfaction script sig size.
This PR adds to the policy header file so that it is documented along with
along policy rules. The initial reasoning that 1650 is an implicit
limit(would not reached assuming all other policy rules are being
followed) is outdated.
As we now know, bitcoin transactions can have spend conditions are more than
just signatures and there may exist p2sh transactions involving 100 byte
preimages that maybe non-standard because of this rule. Because this
rule is no longer implicit, we should explicitly document it in policy
header file
95975dd08d8fdaaeaf28e0d06b861ce2748c17b6 sync: detect double lock from the same thread (Vasil Dimov)
4df6567e4cbb4677e8048de2f8008612e1b860b9 sync: make EnterCritical() & push_lock() type safe (Vasil Dimov)
Pull request description:
Double lock of the same (non-recursive) mutex from the same thread would produce an undefined behavior. Detect this from `DEBUG_LOCKORDER` and react similarly to the deadlock detection.
This came up during discussion in another, related PR: https://github.com/bitcoin/bitcoin/pull/19238#discussion_r442394521.
ACKs for top commit:
laanwj:
code review ACK 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6
hebasto:
re-ACK 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6
Tree-SHA512: 375c62db7819e348bfaecc3bd82a7907fcd8f5af24f7d637ac82f3f16789da9fc127dbd0e37158a08e0dcbba01a55c6635caf1d8e9e827cf5a3747f7690a498e
fa18e7cbc5ea6aaba94dca4ebdc850c9db141f89 This change to the appveyor CI config for msvc builds reverses a change introduced in #19960. It re-applies a setting to inform vcpkg to only build release vesions of the dependencies rather than the default of debug and release. (Aaron Clauson)
Pull request description:
This change to the appveyor CI config for msvc builds reverses a change introduced in #19960. It re-applies a setting to inform vcpkg to only build release versions of the dependencies rather than the default of debug and release.
It had been expected that the vcpkg manifest mechanism introduced in #19960 would do this automatically but it turns out not to be the case.
ACKs for top commit:
MarcoFalke:
ACK fa18e7cbc5ea6aaba94dca4ebdc850c9db141f89 if green
hebasto:
ACK fa18e7cbc5ea6aaba94dca4ebdc850c9db141f89, AppVeyor build takes 5 minutes less.
Tree-SHA512: 427e7e78190c20e0d85dad9b29beed2b6fa13f99c6bc72bcc1839dfb51237a7cc785ab707b4f851c527c1bb0d3e7ebad9e640969e19d29778584bbaeec75cecf
e3e7446305329ce96e9cf5f5161658eb2e1ea888 Add lifetimebound to attributes for general-purpose usage (Cory Fields)
1d58cc7cb040a70f768b632f294db4e0797d3a34 span: add lifetimebound attribute (Cory Fields)
62733fee874bfe7e833e71380eb8efd6a3126fbd span: (almost) match std::span's constructor behavior (Cory Fields)
Pull request description:
Replaces #19382 with a different approach. See [this comment](https://github.com/bitcoin/bitcoin/pull/19382#discussion_r446332852) for the reasoning behind the switch.
--
Description from #19382:
See [here](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0936r0.pdf) for more detail on lifetimebound.
This is implemented using preprocesor macros rather than configure checks in order to keep span.h self-contained.
The ```[[clang::lifetimebound]]``` syntax was chosen over ```__attribute__((lifetimebound))``` because the former is more flexible and works to guard ```this``` as well as function parameters, and also because at least for now, it's available only in clang.
There are currently no violations in our codebase, but this can easily be tested by inserting one like this somewhere and compiling with a modern clang:
```c++
Span<const int> bad(std::vector<int>{1,2,3});
```
The result:
> warning: temporary whose address is used as value of local variable 'bad' will be destroyed at the end of the full-expression [-Wdangling]
Span<const int> bad(std::vector<int>{1,2,3});
```
ACKs for top commit:
sipa:
ACK e3e7446305329ce96e9cf5f5161658eb2e1ea888
ajtowns:
ACK e3e7446305329ce96e9cf5f5161658eb2e1ea888 (drive by; only a quick skim of code and some basic sanity checks)
MarcoFalke:
review ACK e3e7446305329ce96e9cf5f5161658eb2e1ea888 🔗
jonatack:
ACK e3e7446 change since last review is adding `[[clang::lifetimebound]]` as `LIFETIMEBOUND` to src/attributes.h as suggested in https://github.com/bitcoin/bitcoin/pull/19387#issuecomment-650752959.
Tree-SHA512: 05a3440ee595ef0e8d693a2820b360707695c016a68e15df47c20cd8d053646cc6c8cca8addd7db40e72b3fce208879a41c8102ba7ae9223e4366e5de1175211
This is a replacement of the QMetaObject::invokeMethod functor overload
which is available in Qt 5.10+.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
3eb6f8b2e61c24a22ea9396d86672307845f35eb wallet (not for backport): improve upgradewallet error messages (Jon Atack)
ca8cd893bb56bf5d455154b0498b1f58f77d20ed wallet: fix and improve upgradewallet error responses (Jon Atack)
99d56e357159c7154f69f28cb5587c5ca20d6594 wallet: fix and improve upgradewallet result responses (Jon Atack)
2498b04ce88696a3216fc38b7d393906b733e8b1 Don't upgrade to HD split if it is already supported (Andrew Chow)
c46c18b788cb0862aafbb116fd37936cbed6a431 wallet: refactor GetClosestWalletFeature() (Jon Atack)
Pull request description:
This follows up on #18836 and #20282 to fix and improve the as-yet unreleased `upgradewallet` feature and also implement review follow-up in https://github.com/bitcoin/bitcoin/pull/18836#discussion_r519328607.
This PR fixes 4 upgradewallet issues:
- this bug: https://github.com/bitcoin/bitcoin/pull/20403#discussion_r526063920
- it returns nothing in the absence of an RPC error, which isn't reassuring for users
- it returns the same thing both in the case of a successful upgrade and when no upgrade took place
- the error message object is currently dead code
This PR fixes the above and provides:
...user feedback to not silently return without upgrading
```
{
"wallet_name": "disable private keys",
"previous_version": 169900,
"current_version": 169900,
"result": "Already at latest version. Wallet version unchanged."
}
```
...better feedback after successfully upgrading
```
{
"wallet_name": "watch-only",
"previous_version": 159900,
"current_version": 169900,
"result": "Wallet upgraded successfully from version 159900 to version 169900."
}
```
...helpful error responses
```
{
"wallet_name": "blank",
"previous_version": 169900,
"current_version": 169900,
"error": "Cannot downgrade wallet from version 169900 to version 159900. Wallet version unchanged."
}
{
"wallet_name": "blank",
"previous_version": 130000,
"current_version": 130000,
"error": "Cannot upgrade a non HD split wallet from version 130000 to version 169899 without upgrading to support pre-split keypool. Please use version 169900 or no version specified."
}
```
updated help:
```
upgradewallet ( version )
Upgrade the wallet. Upgrades to the latest version if no version number is specified.
New keys may be generated and a new wallet backup will need to be made.
Arguments:
1. version (numeric, optional, default=169900) The version number to upgrade to. Default is the latest wallet version.
Result:
{ (json object)
"wallet_name" : "str", (string) Name of wallet this operation was performed on
"previous_version" : n, (numeric) Version of wallet before this operation
"current_version" : n, (numeric) Version of wallet after this operation
"result" : "str", (string, optional) Description of result, if no error
"error" : "str" (string, optional) Error message (if there is one)
}
```
ACKs for top commit:
achow101:
ACK 3eb6f8b
MarcoFalke:
review ACK 3eb6f8b2e61c24a22ea9396d86672307845f35eb 🛡
Tree-SHA512: b767314069e26b5933b123acfea6aa40708507f504bdb22884da020a4ca1332af38a7072b061e36281533af9f4e236d94d3c129daf6fe5b55241127537038eed
e95aaefe2540cb76969818fcc2ff77d33448ed5a build: Avoid secp256k1.h include from system (Niklas Gögge)
Pull request description:
While building i ran into an error because i had a version of `secp256k1.h` under `/usr/local/include` that was incompatible with the secp256k1 code in the repository. This caused a problem because `$(BOOST_CPPFLAGS)` contained `-I/usr/local/include` and the include paths are searched by the compiler in order from left to right, so in the end `$(BITCOIN_INCLUDES)` contained `-I/usr/local/include` before `-I$(srcdir)/secp256k1/include` which caused the compiler to find `secp256k1.h` under `/usr/local/include`.
Looking at git blame i am wondering how this has not happened to anyone else in several years: cb89e18845/src/Makefile.am (L25)
I am on macOS 10.15.
ACKs for top commit:
laanwj:
Code review ACK e95aaefe2540cb76969818fcc2ff77d33448ed5a
hebasto:
ACK e95aaefe2540cb76969818fcc2ff77d33448ed5a, tested on macOS 11 Big Sur by adding `#error` into `/usr/local/include/secp256k1.h`.
Tree-SHA512: 1f0b395725936c179ab60dee3582ec7b21e2f9c0f1895e160d84a487cf0db16d0c7aa47d05800e0aded31685b4362056cac9b9ecca1bb8c308a4c5a810e8dc1d
fa05d19bd6ba619bb3f9aabc05c439cd18d34544 test: Fix intermittent issue in mempool_compatibility (MarcoFalke)
Pull request description:
Fixes https://cirrus-ci.com/task/5141306890518528?command=ci#L6076
The version is too old to understand getmempoolinfo()[loaded], so it is never called when starting (See https://cirrus-ci.com/task/5141306890518528?command=ci#L5541)
ACKs for top commit:
achow101:
ACK fa05d19bd6ba619bb3f9aabc05c439cd18d34544
Tree-SHA512: e912d5dff6236d2d4ac608f9f3b3e255cc2b611f36d79bfe4e2a940709833a947c682d7703327887e1753eab30b95eb2615d7e2c21ce4bca4f089e717cbb51c4
fa69c2c78455fd0dc436018fece9ff7fc83a180d wallet: Do not treat default constructed types as None-type (MarcoFalke)
fac4e136fa3d0fab7fde900a6be921313e16e7a6 refactor: Change pointer to reference because it can not be null (MarcoFalke)
Pull request description:
Equating `0==None` and `""==None` is confusing, unneeded and undocumented
ACKs for top commit:
jonatack:
ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d
achow101:
ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d
Sjors:
tACK fa69c2c78455fd0dc436018fece9ff7fc83a180d modulo `unset`
Tree-SHA512: c4c8d0ad80c6697621d356a9545caf28ca2facc82bb2fa8e70eceb52372d25f0685237c73688c4b01da0e75d213c77c0d45011a8bdfe81ea783d85f045786dac
