fa13090d2048c9a1e475eb25de756763f8adddb8 contrib: Remove optimize-pngs.py script, which lives in the maintainer repo (MarcoFalke)
Pull request description:
Moved to https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/master/optimize-pngs.py
Bitcoin Core should focus on the full node implementation, not on scripts to compress png images.
This script is only used when new PNG files are added to the repo. This happens about once every two years. So fetching the script from the other repo should not be a burden, but removing it from this repo is going to cut down on the meta files we need to maintain in the main repo.
ACKs for top commit:
practicalswift:
ACK fa13090d2048c9a1e475eb25de756763f8adddb8 -- `+0 lines, -82 lines` :)
promag:
ACK fa13090d2048c9a1e475eb25de756763f8adddb8.
hebasto:
ACK fa13090d2048c9a1e475eb25de756763f8adddb8, verified that script is already [moved](https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/56).
Tree-SHA512: 37d111adae769bcddc6ae88041032d5a2b8b228fec67f555c8333c38de3992f5138b30bea868d7d6d6b7f966a47133e5853134373b149ab23cba3b8b560ecb31
Stop the build if a warning is emitted due to `-Wgnu` and
`--enable-werror` has been used. As usual - this would help notice such
a warning that is about to be introduced in new code.
This is a followup to
https://github.com/bitcoin/bitcoin/pull/18088
build: ensure we aren't using GNU extensions
2a780980983f4b4aaae75817e57e7ed308713561 wallet: Make sure no WalletDescriptor members are uninitialized after construction (practicalswift)
ff046aeeba8d4f3ff210d37ba020616c12450ab3 wallet: Make sure no DescriptorScriptPubKeyMan members are uninitialized after construction (practicalswift)
Pull request description:
This is a small folllow-up to #16528 ("Native Descriptor Wallets using DescriptorScriptPubKeyMan") which was merged in to `master` a couple of hours ago.
Make sure no `DescriptorScriptPubKeyMan` or `WalletDescriptor` members are left uninitialized after construction.
Before this change `bool m_internal` was left uninitialized when using the `DescriptorScriptPubKeyMan(WalletStorage&, WalletDescriptor&)` ctor.
The same goes for the now initialized integers which were left uninitialized when using the `WalletDescriptor()` ctor.
ACKs for top commit:
instagibbs:
utACK 2a78098098
fjahr:
Code review ACK 2a780980983f4b4aaae75817e57e7ed308713561
Sjors:
utACK 2a78098
achow101:
ACK 2a780980983f4b4aaae75817e57e7ed308713561
brakmic:
Code review ACK 2a780980983f4b4aaae75817e57e7ed308713561
meshcollider:
utACK 2a780980983f4b4aaae75817e57e7ed308713561
Tree-SHA512: c98e035268fdc7f65a423b73ac0cf010b0ef7c5e679b3cf170c1813efac8ab5c657dcbaf43c746770bea59e4772bfefe4caa834f1175260c39c7f35d92946ba5
0ae8f18dfe143051fec6ae10ea7df10142e3ff2f build: add -Wgnu to compile flags (fanquake)
3a0fd7726b8b916de6cce33bb67f48990575f923 Remove use of non-standard zero variadic macros (Ben Woosley)
49f6178c3e5e3ad54a419da9d8523207da17fc64 Drop unused LOG_TIME_MICROS helper (Ben Woosley)
5d4999951ee32e333b511245862628e80f83b703 prevector: Avoid unnamed struct, which is a GNU extension (DesWurstes)
Pull request description:
Since we [started using](https://github.com/bitcoin/bitcoin/pull/7165) the `ax_cxx_compile_stdcxx.m4` macro we've been passing `[noext]` to indicate that we don't want to use an extended mode, i.e GNU extensions. Speaking to Cory he clarified that the intention was to "require only vanilla c++11 and turn _off_ extension support so they would fail to compile".
However in the codebase we are currently making use of some GNU extensions. We should either remove there usage, or at least amend our CXX compiler checks. I'd prefer the former.
#### anonymous structs
```bash
./prevector.h:153:9: warning: anonymous structs are a GNU extension [-Wgnu-anonymous-struct]
struct {
```
This is fixed in b849212c1e.
#### variadic macros
```bash
./undo.h:57:50: warning: must specify at least one argument for '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]
::Unserialize(s, VARINT(nVersionDummy));
```
This is taken care of in #18087.
The `LOG_TIME_*` macros introduced in #16805 make use of a [GNU extension](https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html).
```bash
In file included from validation.cpp:22:
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
BCLog::Timer<std::chrono::milliseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
^
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:101:92: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
BCLog::Timer<std::chrono::seconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
^
6 warnings generated.
```
This is fixed in 081a0ab64eb442bc85c4d4a4d3bc2c8e97ac2a6d and 612e8e138b97fc5ad2f38847300132a8fc423c3f.
#### prevention
To ensure that usage doesn't creep back in we can add [`-Wgnu`](https://clang.llvm.org/docs/DiagnosticsReference.html#wgnu) to our compile time flags, which will make Clang warn whenever it encounters GNU extensions.
This would close#14130.
Also related to #17230, where it's suggested we use a GNU extension, the `gnu::pure` attribute.
ACKs for top commit:
practicalswift:
ACK 0ae8f18dfe143051fec6ae10ea7df10142e3ff2f -- diff looks correct
MarcoFalke:
ACK 0ae8f18dfe143051fec6ae10ea7df10142e3ff2f
vasild:
utACK 0ae8f18df
dongcarl:
ACK 0ae8f18dfe143051fec6ae10ea7df10142e3ff2f
Tree-SHA512: c517404681ef8edf04c785731d26105bac9f3c9c958605aa24cbe399c649e7c5ee0c4aa8e714fd2b2d335e2fbea4d571e09b0dec36678ef871f0a6683ba6bb7f
f85203097f78d9daa1d35c4097a80beab31da2a4 lockedpool: avoid sensitive data in core files (FreeBSD) (Vasil Dimov)
Pull request description:
This is a followup to
23991ee53 / https://github.com/bitcoin/bitcoin/pull/15600
to also use madvise(2) on FreeBSD to avoid sensitive data allocated
with secure_allocator ending up in core files in addition to preventing
it from going to the swap.
ACKs for top commit:
sipa:
ACK f85203097f78d9daa1d35c4097a80beab31da2a4 if someone verifies this works as intended on *BSD.
laanwj:
ACK f85203097f78d9daa1d35c4097a80beab31da2a4
practicalswift:
Code-review ACK f85203097f78d9daa1d35c4097a80beab31da2a4 assuming a reviewer with FreeBSD access verifies that the PR goal is achieved :)
Tree-SHA512: 2e6d4ab6a9fbe18732c8ba530eacc17f58128c97140758b80c905b5b838922a2bcaa5f9abc45ab69d5a1a2baa0cba322f006048b60a877228e089c7e64dadd2a
fa2cce4391b0b1bda325f695bb45f7b565c8e8ea wallet: Remove trailing whitespace from potential translation strings (MarcoFalke)
fa59cc1c977cce8f1f28374ac2169970ca78a35f wallet: Report full error message in wallettool (MarcoFalke)
fae7776690c37104d2d4949429c5f84e6a33c576 wallet: Avoid translating RPC errors when creating txs (MarcoFalke)
fae51a5c6f4270a1088e6295b10a8cc45988ae46 wallet: Avoid translating RPC errors when loading wallets (MarcoFalke)
Pull request description:
Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
`CreateWalletFromFile` does not know its caller, so this commit changes it
to return a `bilingual_str` to the caller.
Fixes#17072
ACKs for top commit:
laanwj:
ACK fa2cce4391b0b1bda325f695bb45f7b565c8e8ea, checked that no new translation messages are added compared to master.
hebasto:
ACK fa2cce4391b0b1bda325f695bb45f7b565c8e8ea
Tree-SHA512: c6a943ae9c3689ea3c48c20d26de6e4970de0257a1f1eec57a2bded67a4af9dcc5c45b2d64659d6fb4c4bc4d8103e28483ea3d14bb850df8db0ff9e8e5c77ee2
faec3dc2adc487af97c22408f9f0bfe33f44a230 init: Remove boost from ThreadImport (MarcoFalke)
Pull request description:
Can be tested by calling `-reindex` or `-loadblock` and then pressing `CTRL`+`C`.
Should print something like:
```
...
2020-04-27T19:34:31Z [loadblk] Reindexing block file blk00005.dat...
^C2020-04-27T19:34:32Z [loadblk] Shutdown requested. Exit ThreadImport
2020-04-27T19:34:32Z [qt-init] Interrupting HTTP server
...
```
ACKs for top commit:
laanwj:
Code review ACK faec3dc2adc487af97c22408f9f0bfe33f44a230
hebasto:
ACK faec3dc2adc487af97c22408f9f0bfe33f44a230, tested on Linux Mint 19.3 (x86_64) both `bitcoind` and `bitcoin-qt` binaries.
Tree-SHA512: e105af18d98296d82ec99f48e478cf44577e3c32f7e4b47617a7bc7cbf71d6becb92722f229a1be38d58ad29712704509ad9740d8ab8cd3104cf90057664b437
38e49ded8bd079f8da8b270b39f81cc5cf3ada11 tests: Add fuzzing harness for MessageSign, MessageVerify and other functions in util/message.h (practicalswift)
Pull request description:
Add fuzzing harness for `MessageSign`, `MessageVerify` and other functions in `util/message.h`.
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:
vasild:
utACK 38e49ded8bd079f8da8b270b39f81cc5cf3ada11
Tree-SHA512: 4f83718365d9c7e772a4ccecb31817bf17117efae2bfaf6e9618ff17908def0c8b97b5fa2504d51ab38b2e6f82c046178dd751495cc37ab4779c0b1ac1a4d211
b56607a89ba112083f2b0a7b64ab18d66b26e2be Remove CCoinsViewCache::GetValueIn(...) (practicalswift)
Pull request description:
Remove `CCoinsViewCache::GetValueIn(...)`.
Fixes#18858.
It seems like `GetValueIn` was added in #748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in #8498 ("Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...", merged in 2017).
`CCoinsViewCache::GetValueIn(…)` performs money summation like this:
```c++
CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const
{
if (tx.IsCoinBase())
return 0;
CAmount nResult = 0;
for (unsigned int i = 0; i < tx.vin.size(); i++)
nResult += AccessCoin(tx.vin[i].prevout).out.nValue;
return nResult;
}
```
Note that no check is done to make sure that the resulting `nResult` is such that it stays within the money bounds (`MoneyRange(nResult)`), or that the summation does not trigger a signed integer overflow.
Proof of concept output:
```
coins.cpp:243:17: runtime error: signed integer overflow: 9223200000000000000 + 2100000000000000 cannot be represented in type 'long'
GetValueIn = -9221444073709551616
```
Proof of concept code:
```c++
CMutableTransaction mutable_transaction;
mutable_transaction.vin.resize(4393);
Coin coin;
coin.out.nValue = MAX_MONEY;
assert(MoneyRange(coin.out.nValue));
CCoinsCacheEntry coins_cache_entry;
coins_cache_entry.coin = coin;
coins_cache_entry.flags = CCoinsCacheEntry::DIRTY;
CCoinsView backend_coins_view;
CCoinsViewCache coins_view_cache{&backend_coins_view};
CCoinsMap coins_map;
coins_map.emplace(COutPoint{}, std::move(coins_cache_entry));
coins_view_cache.BatchWrite(coins_map, {});
const CAmount total_value_in = coins_view_cache.GetValueIn(CTransaction{mutable_transaction});
std::cout << "GetValueIn = " << total_value_in << std::endl;
```
ACKs for top commit:
MarcoFalke:
ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be
promag:
Code review ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be.
jb55:
ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be
hebasto:
ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6
f5a3a5b9ab362c58fa424261f313aa9cf46d2a98 gui: Add close window shortcut (Miguel Herranz)
Pull request description:
CMD+W is the standard shortcut in macOS to close a window without
exiting the program.
This adds support to use the shortcut in both main and debug windows.
ACKs for top commit:
jonasschnelli:
Tested ACK f5a3a5b9ab362c58fa424261f313aa9cf46d2a98
hebasto:
ACK f5a3a5b9ab362c58fa424261f313aa9cf46d2a98, tested on Linux Mint 19.3 by manually opening available dialogs and sub-windows, and applying the `Ctrl+W` shortcut. Also tested with "Minimize on close" option enabled / disabled.
Tree-SHA512: 39851f6680cf97c334d5759c6f8597cb45685359417493ff8b0566672edbd32303fa15ac4260ec8ab5ea1458a600a329153014f25609e1db9cf399aa851ae2f9
Enable -Wconditional-uninitialized to warn on potentially uninitialized
reads.
Fix the sole such warning in Bitcoin Core in GetRdRand(): r1 would be
set to 0 on rdrand failure, so initializing it to 0 is a non-functional
change.
From "Intel 64 and IA-32 ArchitecturesSoftware Developer's Manual" [1],
page 1711: "CF=1 indicates that the data in the destination is valid.
Otherwise CF=0 and the data in the destination operand will be returned
as zeros for the specified width."
[1] https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf
489ebfd7a16443d8263c048d55622da297df7c39 tests: feature_backwards_compatibility.py test downgrade after upgrade (Andrew Chow)
Pull request description:
After upgrading the node, try to go back to the original version to make sure that using a newer node version does not prevent the wallet file from being downgraded again.
ACKs for top commit:
MarcoFalke:
ACK 489ebfd7a16443d8263c048d55622da297df7c39
Tree-SHA512: 86231de6514b3657912fd9d6621212166fd2b29b591fc97120092c548babcf1d6f50b5bd103b28cecde395a26809134f01c1a198725596c3626420de3fd1f017
2742c3428633b6ceaab6714635dc3adb74bf121b test: add factor option to adjust test timeouts (Harris)
Pull request description:
This PR adds a new option **factor** that can be used to adjust timeouts in various functional tests.
Several timeouts and functions from `authproxy`, `mininode`, `test_node` and `util` have been adapted to use this option. The factor-option definition is located in `test_framework.py`.
Fixes https://github.com/bitcoin/bitcoin/issues/18266
Also Fixes https://github.com/bitcoin/bitcoin/issues/18834
ACKs for top commit:
MarcoFalke:
Thanks! ACK 2742c3428633b6ceaab6714635dc3adb74bf121b
Tree-SHA512: 6d8421933ba2ac1b7db00b70cf2bc242d9842c48121c11aadc30b0985c4a174c86a127d6402d0cd73b993559d60d4f747872d21f9510cf4806e008349780d3ef
After upgrading the node, try to go back to the original version to make
sure that using a newver node version does not prevent the wallet file
from being downgraded again.
2748e8793267126c5b40621d75d1930e358f057e script: prevent UB when computing abs value for num opcode serialize (pierrenn)
Pull request description:
This was reported by practicalswift here #18046
It seems that the original author of the line used a reference to glibc `abs`: https://github.com/lattera/glibc/blob/master/stdlib/abs.c
However depending on some implementation details this can be undefined behavior for unusual values.
A detailed explanation of the UB is provided here : https://stackoverflow.com/questions/17313579/is-there-a-safe-way-to-get-the-unsigned-absolute-value-of-a-signed-integer-with (by [Billy O'Neal](https://twitter.com/malwareminigun))
Simple relevant godbolt example : https://godbolt.org/z/yRwtCG
Thanks!
ACKs for top commit:
sipa:
ACK 2748e8793267126c5b40621d75d1930e358f057e
MarcoFalke:
ACK 2748e8793267126c5b40621d75d1930e358f057e, only checked that the bitcoind binary does not change with clang -O2 🎓
practicalswift:
ACK 2748e8793267126c5b40621d75d1930e358f057e
Tree-SHA512: 539a34c636c2674c66cb6e707d9d0dfdce63f59b5525610ed88da10c9a8d59d81466b111ad63b850660cef3750d732fc7755530c81a2d61f396be0707cd86dec
Apparently the -X flag doesn't work as expected in makensis v2.x
For example:
makensis -V2 share/setup.nsi -X'OutFile "test.exe"'
Will output:
OutFile expects 1 parameters, got 0.
Usage: OutFile install_output.exe
So let's instead construct the file using POSIX-compliant commands and
shell constructs
