Can be reviewed with -W --ignore-all-space
Fixes:
* Calling ConsumeRandomLengthByteVector 4 times, when 2 is enough.
* Slow execution speed: Finalize is expensive because it invokes
division. Speed up the target by calling Finalize() at most twice per
fuzz input.
f6e4db27ce test: add aarch64-apple-darwin platform entry to get_previous_releases (Zero-1729)
Pull request description:
Over the course of reviewing a PR, I had to edit `test/get_previous_releases.py` (after I ran `git clean -xdff`) to run the backwards compatibility tests (e.g. `wallet_upgradewallet`, `feature_backwards_compatibility`, etc.), as currently on master, running the script as indicated in [`test/README.md`](https://github.com/bitcoin/bitcoin/blob/master/test/README.md), for example, on an M1 machine results in the following error, as the `aarch64-apple-darwin*` platform entry is presently not recognised:
> Output from an M1 machine running macOS v11.5.2
```sh
$ test/get_previous_releases.py -b v0.20.1 v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2
Releases directory: releases
Not sure which binary to download for aarch64-apple-darwin20.6.0
```
As a quick fix, this PR adds the missing `aarch64-apple-darwin*` platform entry. Running the script now results in fetching the old binaries, as expected:
```sh
$ test/get_previous_releases.py -b v0.20.1 v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2
Releases directory: releases
Fetching: https://bitcoincore.org/bin/bitcoin-core-0.20.1/bitcoin-0.20.1-osx64.tar.gz
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 20.9M 0 0 0 0 0 0 --:--:-- 0:00:02 --:--:-- 0
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 20.9M 100 20.9M 0 0 136k 0 0:02:37 0:02:37 --:--:-- 95607
Checksum matched
…
Checksum matched
```
After this patch, the backwards compatibility tests also run successfully, as expected.
**Note**: I am open to other possible solutions.
---
Steps to reproduce:
> Ensure you take out the binaries in `releases` if they already exist.
Try running `test/get_previous_releases.py -b v0.15.2` or similar to fetch the old release binaries.
Top commit has no ACKs.
Tree-SHA512: a238d909b70a61be622234bc49b05d2e91a8acfc5ea348d29f2c8a927fb793cb97365e558571e3f46d6a5650c4f3c6e28fa126c6e56b38e1eb98f7c3e3594d0f
fa3669f72f fuzz: Move all addrman fuzz targets to one file (MarcoFalke)
fa7a883f5a addrman: Replace assert with throw on corrupt data (MarcoFalke)
fa298971e6 Refactor: Turn the internal addrman check helper into a forced check (MarcoFalke)
fae5c633dc move-only: Move CAddrMan::Check to cpp file (MarcoFalke)
Pull request description:
Assert should only be used for program internal logic errors, not to sanitize external user input.
The assert was introduced via the debug-only runtime option `-checkaddrman` in commit 803ef70fd9, thus won't need a backport.
Also, it doesn't really make sense to continue when the deserialized addrman doesn't pass the sanity check.
For example, if `nLastSuccess` is negative, it would later result in integer overflows. Thus, this patch fixes#22931.
Also,
Fixes#22503Fixes#22504Fixes#22519Closes#22498
Steps to test:
```
mkdir -p /tmp/test_235/regtest/
echo 'H4sIAAAAAAAAA/u1f+stZmUGYgELgwPRakfBKBgFo2AUjIJRMApGwSgYBaNgFIyCUTBswdyGpFnLjUKjP9e0bvjYusl6b+L2e7Vs2dd6N//Pua0/xQUALJAn93IQAAA=' | base64 --decode | zcat > /tmp/test_235/regtest/peers.dat
./src/qt/bitcoin-qt -regtest -datadir=/tmp/test_235/ -checkaddrman=1 -printtoconsole | grep -A2 'Loading P2P addresses'
```
Output before:
```
2021-09-10T11:28:37Z init message: Loading P2P addresses…
2021-09-10T11:28:37Z ADDRMAN CONSISTENCY CHECK FAILED!!! err=-16
bitcoin-qt: addrman.cpp:765: void CAddrMan::Check() const: Assertion `false' failed.
(program crashes)
```
Output after:
```
2021-09-10T11:26:00Z init message: Loading P2P addresses…
2021-09-10T11:26:00Z Error: Invalid or corrupt peers.dat (Corrupt data. Consistency check failed with code -16: iostream error). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/tmp/test_235/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.
(program exits)
```
ACKs for top commit:
naumenkogs:
ACK fa3669f72f
jnewbery:
Code review ACK fa3669f72f
vasild:
ACK fa3669f72f
Tree-SHA512: 687e4a4765bbc66495152fa7a49d28ee84b405dc5370ba87b4016b5593e45f54c4ce5cae579e4d433e0e082d20fc263969fa602679c911accef0adb2d6213bd6
This removes a bunch of boilerplate, makes the code easier to read.
Also, C++11 member initialization avoids accidental uninitialized
members.
Can be reviewed with the git option "--word-diff-regex=." or with "git
difftool --tool=meld".
f680d27155 test: use MiniWallet for make_utxo helper in feature_rbf.py (Sebastian Falbesoner)
0f27524602 test: scale amounts in test_doublespend_tree down by factor 10 (Sebastian Falbesoner)
d1e2481274 test: scale amounts in test_doublespend_chain down by factor 10 (Sebastian Falbesoner)
Pull request description:
This PR aims to further increase MiniWallet usage in the functional test feature_rbf.py by using it in the `make_utxo(...)` helper, which is the only part that needs a wallet for most sub-tests. In order to do that, the amounts for the utxos have to be scaled down in two sub-tests first (`test_doublespend_chain` and `test_doublespend_tree`, see first two commits), since we need amounts passed to `make_utxo` than can be funded by only one input. For creating UTXOs with a value of 50 BTC, we'd need to implement a method for consolidating multiple utxos into one first, which seems to be overkill.
Note that after this PR's change, there is only one sub-test left (`test_rpc`) that needs the wallet compiled into bitcoind.
ACKs for top commit:
MarcoFalke:
review ACK f680d27155🦐
Tree-SHA512: 46c8c245086a9e79855c4ede2f8f412333cf2658136805196b203b3567c89398d77fcb80715c0bb72fdc84331cc67544b2fdc259193a3adcb2fc36e147c26fce
The benchmarks can now run much longer due to the minimum of 10ms or
directly with -min_time. With -min_time=20000 I could trigger two ubsan
errors in the benchmarks, which are fixed in this commit by using
unsigned type and adding "& 0xFF".
- use ALLOW_BOOL for -list arg instead of ALLOW_ANY
- touch up `-asymptote=<n1,n2,n3...>` help
- pack Args struct a bit more efficiently
- handle args in alphabetical order
When it is not easily possible to stabilize benchmark machine and code
the argument -min_time can be used to specify a minimum duration
that a benchmark should take. E.g. choose -min_time=1000 if you
are willing to wait about 1 second for each benchmark result.
The default is now set to 10ms instead of 0, which should make runs on
fast machines more stable with negligible slowdown.
Moves copying of the setup into the benchmark loop so it is possible
to run the loop for an arbitrary number of times.
The overhead due to copying the candidates inside the loop is about 3%.
Moves some of the setup into the benchmark loop so it is possible to run
the loop for an arbitrary number of times. Due to recent optimizations
in #22974 the benchmark now runs much faster, so the inner loop now calls
Good() 32 times as often to get better numbers.
Renamed the benchmark to AddrManAddThenGood because that's now what is
actually tested. To get the the number of just Good(), one needs to
subtract the benchmark result of AddrManAdd.
`crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all.
Make the check in `configure.ac` check for this architecture explicitly.
For the release binaries, the current `configure.ac` check happens
to work: it enables it on aarch64 but disables it for armhf. However
some combination of compiler version and settings might ostensibly cause
this check to succeed on armhf (as reported on IRC). So make the 64-bit
platform requirement explicit.
Introduced in #19055, MuHashDiv benchmark used to multiply with a loop
based on epochIterations. That does not do what it is supposed to do,
because epochIterations() is determined automatically from nanobench.
Also, multiplication is not needed for the algorithm (as pointed out by
a comment in #19055), so it's better to remove this loop.
Most importantly, this update fixes a bug in nanobench that always
disabled performance counters on linux.
It also adds another sanitizer suppression that is caught in clang++ 12.
This speeds up compilation of the whole program because the included
header file is smaller.
Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
a43b8e9555 build: set OSX_MIN_VERSION to 10.15 (fanquake)
Pull request description:
Taken out of #20744, as splitting up some of the build changes was mentioned [here](https://github.com/bitcoin/bitcoin/pull/22937#discussion_r707303172).
This is required to use `std::filesystem` on macOS, as support for it only landed in the libc++.dylib shipped with 10.15. So if we want to move to using `std::filesystem` for `23.0`, this bump is required.
See also: https://developer.apple.com/documentation/xcode-release-notes/xcode-11-release-notes
> Clang now supports the C++17 \<filesystem\> library for iOS 13, macOS 10.15, watchOS 6, and tvOS 13.
macOS 10.15 was released in October 2019. macOS OS's seem to have a life of about 3 years, so it's possible that 10.14 will become officially unsupported by the end of 2021 and prior to the release of 23.0.
Guix builds:
```bash
bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
abc8b749be65f1339dcdf44bd1ed6ade2533b8e3b5030ad1dde0ae0cede78136 guix-build-a43b8e955558/output/dist-archive/bitcoin-a43b8e955558.tar.gz
1edcc301eb4c02f3baa379beb8d4c78e661abc24a293813bc9d900cf7255b790 guix-build-a43b8e955558/output/x86_64-apple-darwin19/SHA256SUMS.part
e9dbb5594a664519da778dde9ed861c3f0f631525672e17a67eeda599f16ff44 guix-build-a43b8e955558/output/x86_64-apple-darwin19/bitcoin-a43b8e955558-osx-unsigned.dmg
11b23a17c630dddc7594c25625eea3de42db50f355733b9ce9ade2d8eba3a8f3 guix-build-a43b8e955558/output/x86_64-apple-darwin19/bitcoin-a43b8e955558-osx-unsigned.tar.gz
257ba64a327927f94d9aa0a68da3a2695cf880b3ed1a0113c5a966dcc426eb5e guix-build-a43b8e955558/output/x86_64-apple-darwin19/bitcoin-a43b8e955558-osx64.tar.gz
```
ACKs for top commit:
hebasto:
ACK a43b8e9555
jarolrod:
ACK a43b8e9
Tree-SHA512: 9ac77be7cb56c068578860a3b2b8b7487c9e18b71b14aedd77a9c663f5d4bb19756d551770c02ddd12f1797beea5757b261588e7b67fb53509bb998ee8022369
cdaab90662 Add test for addrman consistency check on restart with asmap (Jon Atack)
869f136816 Add test for rpc addpeeraddress with "tried" argument (Jon Atack)
ef242f5213 Allow passing "tried" to rpc addpeeraddress to call CAddrMan::Good() (Jon Atack)
Pull request description:
This pull adds a `tried` argument to RPC addpeeraddress and a regression test for the recent addrman/asmap changes and issue.
PR #22697 introduced a reproducible bug in commit 181a1207 that fails addrman consistency checks and causes it to significantly lose peer entries when the `-asmap` configuration option is used.
The issue occurs upon bitcoind restart due to an initialization order change in `src/init.cpp` in that commit, whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before.
Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263.
```
addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses
ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted
```
How to reproduce:
- `git checkout 181a1207`, build, and launch bitcoind with the `-asmap` and `-checkaddrman=1` configuration options enabled
- restart bitcoind
- bitcoind aborts on the second call to the addrman consistency checks in `CAddrMan::Check()`
How to test this pull:
- `git checkout 181a1207`, cherry pick the first commit of this branch, build, git checkout this branch, run `test/functional/rpc_net.py`, which should pass, and then run `test/functional/feature_asmap.py`, which should fail with the following output:
```
AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed.
```
ACKs for top commit:
jnewbery:
utACK cdaab90662
mzumsande:
re-ACK cdaab90662 (based on code review of diff to d586817)
vasild:
ACK cdaab90662
Tree-SHA512: 0251a18fea629b62486fc907d7ab0e96c6df6fadb9e4d62cff018bc681afb6ac31e0e7258809c0a88f91e4a36c4fb0b16ed294ce47ef30585217de89c3342399
faa81f9486 test: Add addrman deserialization error tests (MarcoFalke)
Pull request description:
Add missing test coverage
ACKs for top commit:
jonatack:
Light code review ACK faa81f9486 and ran the test
Tree-SHA512: 8b254ba912c83473125faaf7df02a33a99840b40460bdce1486991a01de9ba6371c053354318f09b69fdc18c823bca3f2f7d341db0f8950e22d8435acbaa9cf5
fa7e3f1fc1 test: Replace MiniWallet scan_blocks with rescan_utxos (MarcoFalke)
Pull request description:
This avoids having to fiddle with the `start` and `num` parameters and instead use the `scantxoutset` RPC functionality via `rescan_utxos`.
ACKs for top commit:
Shubhankar-Gambhir:
ACK fa7e3f1, all tests were succesfull
theStack:
re-ACK fa7e3f1fc1
Tree-SHA512: 6f47d2acac9f180b2b0f8f04797e74ecb1fc180f6b164c67813a3a1f97acea54baed74e5e0a3512e3babf76b105c09e1ba4cad818c83c7cb2beb7377b4c96954
c17f554fcc Fix BlockAssembler::AddToBlock, CTxMemPool::PrioritiseTransaction logging (Jon Atack)
Pull request description:
This is a tale of two fees, er, fee rates... indeed, one is misdescribed as a fee, and the other is incorrectly called a fee rate.
From this review discussion: https://github.com/bitcoin/bitcoin/pull/22689#discussion_r695866211 (thanks to John Newbery).
ACKs for top commit:
laanwj:
Code review ACK c17f554fcc
Tree-SHA512: 3d9df3209a72562c5f9bbf815923d5b089d04491b8d19caa2c04158c501b47ef01e47f1c32d89adcbaf3c6357329507f65b4bb2963214c3451bbfa61ac812530
9bdda50151 Enable TLS in links in documentation (Jeremy Rand)
Pull request description:
This PR enables TLS in several documentation links, which improves security.
ACKs for top commit:
fanquake:
ACK 9bdda50151
Tree-SHA512: 9d04d8771a9daf3c3b9914ff324e2eabfdf3ff5ae7f7dc92b84a1f3527010ceb860e73873a8f24d6051763eb472d9ea324ccbd6129a40318a520ca88c05f0586
fac67b393b ci: Set --nocleanup for Windows functional tests (MarcoFalke)
Pull request description:
Example: https://cirrus-ci.com/task/6292402450202624
ACKs for top commit:
hebasto:
ACK fac67b393b
Tree-SHA512: b8da82d1492f66b43e4afc525805a42709a7b13e64320681801e3e5b546da3d82933f5248464900620aa89eac4629f0b21cde623caf45950f1341979b47fa621
57ce20307e fuzz: allow lower number of sources (Martin Zumsande)
acf656d540 fuzz: Use public interface to fill addrman tried tables (Martin Zumsande)
eb2e113df1 addrman: Improve performance of Good (Martin Zumsande)
Pull request description:
Currently, `CAddrman::Good()` is rather slow because the process of moving an addr from new to tried involves looping over the new tables twice:
1) In `Good_()`, there is a loop searching for a new bucket the addr is currently in, but this information is never used except for aborting if it is not found anywhere (since [this commit](e6b343d880 (diff-49d1faa58beca1ee1509a247e0331bb91f8604e30a483a7b2dea813e6cea02e2R263)) it is no longer passed to `MakeTried`)
This is unnecessary because in a non-corrupted addrman, an address that is not in New must be either in Tried or not at all in addrman, both cases in which we'd return early in `Good_()` and never get to this point.
I removed this loop (and left a check for `nRefCount` as a belt-and-suspenders check).
2) In `MakeTried()`, which is called from `Good_()`, another loop removes all instances of this address from new. This can be spedup by stopping the search at `nRefCount==0`. Further reductions in `nRefCount` would only lead to an assert anyway.
Moreover, the search can be started at the bucket determined by the source of the addr for which `Good` was called, so that if it is present just once in New, no further buckets need to be checked.
While calls to `Good()` are not that frequent normally, the performance gain is clearly seen in the fuzz target `addman_serdeser`, where, because of the slowness in creating a decently filled addrman, a shortcut was created that would directly populate the tried tables by reaching into addrman's internals, bypassing `Good()` (#21129).
I removed this workaround in the second commit: Using `Good()` is still slower by a factor of 2 (down from a factor of ~60 before), but I think that this compensated by the advantages of not having to reach into the internal structures of addrman (see https://github.com/jnewbery/bitcoin/pull/18#issuecomment-775218676).
[Edit]: For benchmark results see https://github.com/bitcoin/bitcoin/pull/22974#issuecomment-919435266 and https://github.com/bitcoin/bitcoin/pull/22974#issuecomment-920445700 - the benchmark `AddrManGood` shows a significant speedup by a factor >100.
ACKs for top commit:
naumenkogs:
ACK 57ce20307e
jnewbery:
ACK 57ce20307e
laanwj:
Code review ACK 57ce20307e
theStack:
ACK 57ce20307e
vasild:
ACK 57ce20307e
Tree-SHA512: fb6dfc198f2e28bdbb41cef9709828f22d83b4be0e640a3155ca42e771b6f58466de1468f54d773e794f780a79113f9f7d522032e87fdd75bdc4d99330445198
7c3712fa32 cli: Display all proxies in -getinfo (klementtan)
Pull request description:
**Changes**: Display all proxies in `-getinfo`
**Motivation**:
* Currently `-getinfo` only return the proxy of the first network in `getnetworkinfo`.
* This PR will display all unique proxies in `getnetworkinfo` as suggested in https://github.com/bitcoin/bitcoin/issues/17314#issue-514543978
>List all proxies, at least if they're different from the IPv4 one
**Testing**:
You can verify this change by starting bitcoind with
```shell
./src/bitcoind -signet --proxy=127.0.0.1:9050 --i2psam=127.0.0.1:7656
```
Execute `-getinfo`
```shell
./src/bitcoin-cli -signet -getinfo
```
ACKs for top commit:
laanwj:
Tested ACK 7c3712fa32
prayank23:
utACK 7c3712fa32
Tree-SHA512: 9eae97866220227f30ca4585f52799fa66fc1135047d869c4aabe598aee1a9414cb9e1c4a8d19165e52d65005f3c6d4bcc37463ace0ddb44389dfbcd4ca74095
This avoids intermittent issues in the CI Task when a test passes
successfully. For example:
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\test_runner_₿_🏃_20210920_072037\\feature_versionbits_warning_89\\alert.txt'
ebe49b5b7c test: fix confusing off-by-one nValue in feature_coinstatsindex.py (Sebastian Falbesoner)
Pull request description:
Due to evil floating-point arithmetic, the creation of one of the transaction outputs in feature_coinstatsindex.py leads to it's nValue being off by one satoshi: the Python expression `int(21.99 * COIN)` doesn't yield 2199000000 as expected, but 2198999999.
This makes the test more confusing than necessary (w.r.t. the expected `gettxoutsetinfo` values), and could also cause problems if the value is ever changed. Fix by using a `Decimal` type for specifying the value in BTC, rather than using a bare floating-point.
ACKs for top commit:
MarcoFalke:
cr ACK ebe49b5b7c
Tree-SHA512: c74c51dbf99818f3d1c881873e0c053a649e4fed9b36767ff971dd3a48bff7122afea4e07cc9925236570368b45579f63e443701f2aaef838a0fafdbe986dfd4
f58f697c98 doc: remove WSL install instructions and point to upstream (fanquake)
Pull request description:
There's not really any need for us to have to replicate (ever-changing) instructions for installing an operating system in our build documentation.
ACKs for top commit:
laanwj:
ACK f58f697c98
hebasto:
ACK f58f697c98
Tree-SHA512: 3ca90cf7696909503ca500756c718f019c410b1e24c139e7e9fdc8726d1c37a7b6c869fdac1cb7ab8c8065bc49c808778ca5985f54d4a698c162866d00397d48
