The libtool unsorted 'find' determinism issue seemed to have been solved
in gcc-9's git: d41cd173e23ebea7c758644d6ad6e0fde1c2e3a6 or SVN: r262451
Furthermore, it seems that Ubuntu Focal 20.04 LTS is going to ship with
gcc 9 and mingw-w64 7, which will match what we have now.
-----
A note on this:
Careful observers will see that previously I stated that all released
versions of gcc were bootstrapped with a libtool 2.2.7a, meaning that
they all had the unsorted 'find' determinism issue first resolved in
libtool 2.2.7b.
However, I was mistaken, gcc's ltmain.sh CLAIMS it was generated by
libtool 2.2.7a, but it was in fact edited manually. It seems that gcc
maintains their own versions of ltmain.sh and libtool.m4, and only
sometimes backports patches from upstream.
Quite confusing.
This is no longer needed after 3bef7c22 in the mingw-w64 git repository,
which is first included in mingw-w64 v7.0.0.
As of the previous bump to our Guix time machine, we now use mingw-w64
v7.0.0.
Most of the mingw-w64 toolchain changes have now been upstreamed, we can
point to a commit that exists upstream.
NOTE: I'm not changing the URL yet until we see that Guix upstream will
accept all my patches for macOS.
-----
The Guix tree that's referred to by this commit contains the following
changes relevant to our mingw-w64 build:
b066c25026
Adds a PACKAGES-WITH-*PATCHES procedure which we can use in the future
to apply patches to packages if those patches are not considered
appropriate to upstream Guix
4719b71572
Adds mingw-w64 (the libc itself) reproducibility patches, taken from
debian.
79825bee07 + 401d28e433 + c1c50cb5b0
Add mingw-w64 specific binutils patches, taken from debian.
Specifically, the "Make DLL import libraries reproducible" patch made
libbitcoinconsensus.dll.a build reproducibly. The followup commits
were hotfixes for my mistakes.
0f864175dc
Bumps mingw-w64 to v7.0.0. This is the first release that enables
secure APIs by default (which we need), and gains _FORTIFY_SOURCE
support. This will also be what Ubuntu Focal 20.04 LTS releases with.
cdf00cf75d
Bumps NSIS to v3.05. This is the first release that includes a fix for
a reproducibility bug found by some of the electrum developers. See
details here: https://sourceforge.net/p/nsis/bugs/1230/
This bump will includes a couple of commits which improve the
reproducibility of the mingw-w64 toolchain. Most of which came from
debian. They will be upstreamed as upstream Guix release timeline
allows.
- Add "--no-insert-timestamp" LDFLAG for x86_64-w64-mingw32 builds
"The option --no-insert-timestamp can be used to insert a zero value for
the timestamp, this ensuring that binaries produced from identical
sources will compare identically." - ld(1)
- Set "SetDateSave off" in NSIS script
From https://nsis.sourceforge.io/Docs/Chapter4.html#flags
"This command sets the file date/time saving flag which is used by the
File command to determine whether or not to save the last write date and
time of the file, so that it can be restored on installation. Valid
flags are 'on' and 'off'. 'on' is the default."
- Add commented out NSIS options for reproducibility debugging in NSIS
script
- Make ZIPs deterministic by reseting file modification times to
SOURCE_DATE_EPOCH using touch(1) (Reference:
https://reproducible-builds.org/docs/archives/)
fa6e01f2a163511a735088895ab02232b150801b doc: block-relay-only is not blocksonly (MarcoFalke)
Pull request description:
Those are different concepts, see https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.19.0.1.md#p2p-changes for the block-relay-only nodes.
ACKs for top commit:
jonatack:
ACK fa6e01f
hebasto:
ACK fa6e01f2a163511a735088895ab02232b150801b
Tree-SHA512: 6de2c81201b62ed59e504a3a6f164068600182e1bbf63eda7f9db3160507bdba091c13882ee0e75e713f0832bfaf5973a86eba3b94588d5b72196f05ae0a9c9a
6112a209828c43930f677c45461339cdf68a56e9 test: replace (send_message + sync_with_ping) with send_and_ping (Jon Atack)
Pull request description:
This is a follow-up to faf1d047313e71658fb31f6b94fdd5d37705ab85 yesterday.
ACKs for top commit:
vasild:
utACK 6112a20
MarcoFalke:
ACK 6112a209828c43930f677c45461339cdf68a56e9 🎞
Tree-SHA512: 749644ac9a1ef0e1aa6c3ac5e899eb3fa7fb9c0909352f922a80412df2bc0e539692a7757af550eff4d4914cbe57b0c75ce3948f569acc7a52852e91a55ad457
222253e37d9702181fe754d776177962d6794f30 chainparams: Bump assumed valid hash (MarcoFalke)
Pull request description:
See https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-branch-off
ACKs for top commit:
fanquake:
ACK 222253e37d9702181fe754d776177962d6794f30 - matches output on my node. Also have 3.8GB for the chainstate dir. If anyone wants to know what commands they should run / what to check; I have some docs on [assumevalid updates](https://github.com/fanquake/core-review/blob/master/update-assumevalid.md).
elichai:
ACK 222253e37d9702181fe754d776177962d6794f30
theStack:
ACK 222253e37d
instagibbs:
ACK 222253e37d all mainnet params, did not check testnet
Tree-SHA512: 856e75f0c6663ab478768d80618f11ac109ef30058661f58ed7f2a3a615dd5fb8f8d648c996eff77949036c1633119085b25cd4c25250a0850c943c02cddf3d7
9eefc6e92fa1acef6eddd87886ed80510b439a57 gui: Delete progress dialog instead of hidding it (João Barbosa)
ee9e88ba2734b81d0ffe23fd45c4f69a970c6494 wallet: Handle duplicate fileid exception (João Barbosa)
Pull request description:
Handle the duplicate fileid exception thrown at `CheckUniqueFileid` in tow cases:
- when duplicate wallets are set on the command line - catch in `LoadWallets`;
- when a duplicate wallet is loaded dynamically - catch in `LoadWallet`.
Fixes#16776.
ACKs for top commit:
jonatack:
Re-ACK 9eefc6e92fa1acef6eddd87886ed80510b439a57 no change since last review 68e0ff0e1f530c942721aab49cf67ffc07104628
hebasto:
re-ACK 9eefc6e92fa1acef6eddd87886ed80510b439a57
Tree-SHA512: 46e3c1cd6708b54e2d1c4973a74c8d5428822e04cecbc147cf200eb034efa385e867bd749c7c639020e83c9813fae8fed64a851bdd99abf60c33b07e0363f5d5
a46484c8b3715725f5dc0b8ad1bf921880ed9af1 build: Detect gmtime_* definitions via configure (Ben Woosley)
Pull request description:
Something has changed in the mingw-w64 headers such that we
no-longer compile when using 7.0.0.
```bash
util/time.cpp: In function 'std::__cxx11::string FormatISO8601DateTime(int64_t)':
util/time.cpp:84:9: error: 'gmtime_r' was not declared in this scope
if (gmtime_r(&time_val, &ts) == nullptr) {
^~~~~~~~
util/time.cpp: In function 'std::__cxx11::string FormatISO8601Date(int64_t)':
util/time.cpp:97:9: error: 'gmtime_r' was not declared in this scope
if (gmtime_r(&time_val, &ts) == nullptr) {
```
Looking at time.h, it seems that `gmtime_r()` is only available when
`_POSIX_C_SOURCE` is defined. This must have been the case for 6.0.0
(which we compile fine using), but no-longer seems to be for 7.0.0?
I've checked that adding `-D_POSIX_C_SOURCE=200112L` to our compile
flags does fix the issue above.
However, an alternative solution seems to be to just use `gmtime_s()`
instead, when compiling with `mingw-w64`, as `gmtime_r()` [just wraps
`gmtime_s()` anyways](7c03b11bf1/mingw-w64-headers/crt/time.h (L284)).
I've tested this change crosss-compiling on Debian Bullseye ([mingw-w64 7.0.0](https://packages.debian.org/source/bullseye/mingw-w64))
and Buster ([mingw-w64 6.0.0](https://packages.debian.org/source/buster/mingw-w64)).
ACKs for top commit:
laanwj:
ACK a46484c8b3715725f5dc0b8ad1bf921880ed9af1
Tree-SHA512: 7cf1a81060b9625d64de40b77341d74704cc8ae1358d25d7e2909685dc83a7a9762260d72e47806e9f0a5cbabf88d0239ec9e0fd0ebd3731b1d206b075f43a63
740614e3c36b1b6906e30027990bbca92d3edd9b qt: Translations update pre-branch (Wladimir J. van der Laan)
Pull request description:
Update translations from transifex before the 0.20 branch-off. As we don't have translations for the master branch (only for release versions), this is the last opportunity (for this release) to get up to date translations on master.
ACKs for top commit:
elichai:
utACK 740614e3c36b1b6906e30027990bbca92d3edd9b
fanquake:
ACK 740614e3c36b1b6906e30027990bbca92d3edd9b
Tree-SHA512: 108b82bac967009cab2257d77f0fbc38d18497ffdfe34b06aba35a1894b9b7ebff8ba4671219f4e20ebc42f984ed9ef65c5d41d21ac08dadfef798474f0b2b3b
faa9491870bf29583a989c407694d01f49b65bec ci: Use Focal for fuzzers (MarcoFalke)
Pull request description:
This gives us access to clang-10, as well as a newer version of valgrind
ACKs for top commit:
fanquake:
ACK faa9491870bf29583a989c407694d01f49b65bec - [Clang 10](https://packages.ubuntu.com/focal/clang) and [valgrind 3.15](https://packages.ubuntu.com/focal/valgrind).
practicalswift:
ACK faa9491870bf29583a989c407694d01f49b65bec -- diff looks correct & contemporary clang is better than vintage clang
Tree-SHA512: 0e67232673434c0309db79c1054e3e981115083585945967e346f4d58792635832100f89911428aab928155e44e5f401207a023681ae008fdb5280cf02c4d427
7777703958937ec0ae609b1ee882f1bf2d113d10 doc: Explain new test logging (MarcoFalke)
Pull request description:
Explain logging added in #18472 and #16975
ACKs for top commit:
jonatack:
ACK 7777703
Tree-SHA512: 3a0aa7bab32a6753d8894d29cf82604b044b23e512102dd275b717eefda3c2212dbf43ea7e9155267350dd9f3bc5badba2eb660152db3efeab30a04f52126c95
fac3716b09bb9ee121db629873d9694a95cae942 test: check that peer is connected when calling sync_* (MarcoFalke)
Pull request description:
Without a connection there is no way to sync, so we can fail early and don't have to wait for the timeout
ACKs for top commit:
jonatack:
ACK fac3716b09bb9
Tree-SHA512: 12f771473c23e152dae4bfb201fadb2c3530cf439de64fea07d048734614543080a5d05c9c36e6e398c6a69c8279f609d34706599571814172a11bcfbea4a3b9
00559229588feb19de2a0cb7506f70c483a1f433 test: add BIP37 'filterclear' test to p2p_filter.py (Sebastian Falbesoner)
Pull request description:
Integrates the message type `filterclear` to the test framework and adds a simple test to `p2p_filter.py`, checking that arbitrary txs get relayed again after deleting the filter.
ACKs for top commit:
naumenkogs:
utACK 00559229588feb19de2a0cb7506f70c483a1f433
Tree-SHA512: fe64e99a526865770707d8077b9968d3923f248045ec7fa56cd380dba85ac77a71a473d244ef3aede2fc0d287b8d7c6bc0156b6033b0c949c2058cc08e255697
41b0baf43c243b64b394e774e336475a489cca2b gui: Handle WalletModel::unload asynchronous (João Barbosa)
ab31b9d6fe7b39713682e3f52d11238dbe042c16 Fix wallet unload race condition (Russell Yanofsky)
Pull request description:
This PR consists in two fixes. The first fixes a concurrency issues with `boost::signals2`. The second fixes a wallet model destruction while it's being used.
From boost signal documentation at https://www.boost.org/doc/libs/1_72_0/doc/html/signals2/thread-safety.html:
> When a signal is invoked by calling signal::operator(), the invocation first acquires a lock on the signal's mutex. Then it obtains a handle to the signal's slot list and combiner. Next it releases the signal's mutex, before invoking the combiner to iterate through the slot list.
This means that `UnregisterValidationInterface` doesn't prevent more calls to that interface. The fix consists in capturing the `shared_ptr<CValidationInterface>` in each internal slot.
The GUI bug is fixed by using a `Qt::QueuedConnection` in the `WalletModel::unload` connection.
ACKs for top commit:
ryanofsky:
Code review ACK 41b0baf43c243b64b394e774e336475a489cca2b. Only change is moving assert as suggested
hebasto:
ACK 41b0baf43c243b64b394e774e336475a489cca2b, tested on Linux Mint 19.3.
Tree-SHA512: 4f712d8de65bc1214411831250de5dc0a9fd505fb84da5baf9f2cc4d551bc3abffc061616f00afe43dba7525af2cd96c9b54aeead9383145e3b8801f25d85f50
0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37 gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged (João Barbosa)
Pull request description:
Each 250ms the slot `WalletModel::pollBalanceChanged` is called which, at worst case, calls `Wallet::GetBalance`. This is a waste of resources since most of the time there aren't new transactions or new blocks. Fix this by early checking if cache is dirty or not.
The actual balance computation can still hang the GUI thread but that is tracked in #16874 and should be fixed with a solution similar to #17135.
ACKs for top commit:
hebasto:
ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
jonasschnelli:
utACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37
instagibbs:
ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37
ryanofsky:
Code review ACK 0933a37078e1ce3a3d70983c3e7f4b3ac6c3fa37, but I would prefer (not strongly) for #17905 to be merged first. This PR can be simpler if it is based on #17905, so tryGetBalances can just be left alone instead of changing into to a more complicated tryGetBalancesIfNeeded function, and then getting changed back later when we want to optimize it out.
jonatack:
ACK 0933a37078e based primarily on code review, despite a lot of manual testing with a large 177MB wallet.
Tree-SHA512: 18db35bf33a7577666658c8cb0b57308c8474baa5ea95bf1468cd8531a69857d8915584f6ac505874717aa6aabeb1b506ac77630f8acdb6651afab89275e38a1
83e1d92413e262e6a876336ec433a6fbc335223a test: listsinceblock block height checks (Jon Atack)
Pull request description:
This is the second commit of #17535.
This PR extends a listsinceblock test to check the new transaction 'blockheight' field recently added in #17437. It also cleans up code in the test function without changing or removing existing checks.
ACKs for top commit:
fjahr:
tested ACK 83e1d92413e262e6a876336ec433a6fbc335223a
ryanofsky:
Code review ACK 83e1d92413e262e6a876336ec433a6fbc335223a. Nice test improvements!
Tree-SHA512: 92874b49a3bc0236500495f32dfcf683e1971ca3d4c51702c69ed4ce7dfce21273754f02f93d1243d73793701d9fdf49e14b149477cd249cbbd9e4e8d5bd49f8
fa156999695ddaeb016d8320bee62f8d96679d55 test: Add basic test for BIP 37 (MarcoFalke)
Pull request description:
This does not add full coverage, but should be a good start and can be extended in the future. Currently, none of the BIP 37 p2p code has test coverage.
ACKs for top commit:
practicalswift:
Code review ACK fa156999695ddaeb016d8320bee62f8d96679d55 -- more testing coverage is better than less testing coverage
Tree-SHA512: d52e8be79240dffb769105c087ae0ae9305d599282546e4ca7379c4c7add2dbcd668265b46670aa07c357638044cf0f61a6fab7dba8971dd0f80c8f99768686e
e980214bc4fd49530e8d50fe0a6657b8583bc6b5 serialization: prevent int overflow for big Coin::nHeight (pierrenn)
Pull request description:
This is an attempt to fix fuzzer issues 1,2,8 reported by practicalswift here : https://github.com/bitcoin/bitcoin/issues/18046
The fuzzer harness doesn't prevent deserialization of unrealistic high values for `Coin::nHeight`. In the [provided examples](https://github.com/bitcoin/bitcoin/issues/18046), we have :
- `blockundo_deserialize` : the varint `0x8DD88DD700` is deserialized as `3944983552` in `Coin::nHeight` (`TxInOutFormatter::Unser`)
- `coins_deserialize` : the varint `0x8DD5D5EC40` is deserialized as `3939874496` similarly
- `txundo_deserialize`: the varint `0x8DCD828F01` is deserialized as `3921725441` in `Coin::nHeight` (`Coin::Unserialize`)
Since `Coin::nHeight` is 31 bit long, multiplying a large value by 2 triggers the fuzzer.
AFAIK those values are unrealistic (~70k years for the smallest..). I've looked a bit a reducing the range of values the fuzzer can deserialize, but this seems to be too much code change for not much.
Hence this PR chooses to static cast `nHeight` when re-serializing; it seems to be the less intrusive/safest way to prevent the fuzzer output.
Another more "upstream" approach would be to limit `Coin::nHeight` values to something more realistic, e.g. `0xFFFFFFF` (~5k years) :
de3a30bab2/src/undo.h (L39) and de3a30bab2/src/coins.h (L71)
Thanks !
NB: i was also not sure about the component/area to prefix the PR/commit with.. ?
ACKs for top commit:
practicalswift:
ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5 -- patch looks correct
promag:
ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5.
sipa:
utACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5
MarcoFalke:
re-ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5 🎑
ryanofsky:
Code review ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5. Just removed ternary ? 1 : 0 and replaced / 2 with >> 1 since last review
Tree-SHA512: 905fc9e5e52a6857abee4a1c863751767835965804bb8c39474f27a120f65399ff4ba7a49ef1da0ba565379f8c12095bd384b6c492cf06776f01b2db68d522b8
11a520f6793e21e0a8a9301f5ec4c28a48131b85 tests: Add fuzzing harness for functions/classes in random.h (practicalswift)
64d277bbbcbd464b2a795bae011ee808298a42ca tests: Add fuzzing harness for LimitedString (serialize.h) (practicalswift)
f205cf7fef5618aaa96f016fda168eedfd9da437 tests: Add fuzzing harness for functions/classes in span.h (practicalswift)
9718f38f54357f15b8a27e060aed56f91015112d tests: Add fuzzing harness for functions/classes in merkleblock.h (practicalswift)
a16ea051f915eb4c975fe06f89470aa99d99d7e4 tests: Add fuzzing harness for functions/classes in flatfile.h (practicalswift)
Pull request description:
* Add fuzzing harness for functions/classes in `flatfile.h`
* Add fuzzing harness for functions/classes in `merkleblock.h`
* Add fuzzing harness for functions/classes in `span.h`
* Add fuzzing harness for `LimitedString` (`serialize.h`)
* Add fuzzing harness for functions/classes in `random.h`
Top commit has no ACKs.
Tree-SHA512: 6f7e0f946f1062d51216990cde9672b4e896335152548ace3d8711e4969c3e3c8566d01d915b72adcda5c1caa9c2e34da6b7473b55a229f5b77239d3b0ba4b67
