fa04f26aa7 test: Avoid race after connect_nodes (MarcoFalke)
Pull request description:
Wait until the connection is fully established on both sides (verack). Fixes#22714
ACKs for top commit:
kiminuo:
utACK fa04f26aa7
Tree-SHA512: bc2c44b44b688086ff84046924cf5251dd625584e93ce8fa17de27023855b32f3bb55109b846abbcec775e2836c7f3c5a81d6b4aff7c4ac065b9aefa044c1883
This is the same approach as for the addpeeraddress test in
`test/functional/rpc_net.py` in commit 869f1368.
The probability of collision when adding an addrman entry is
expected to be 1/2^16 = 1/65536 for an address from a different /16.
This change hopes to avoid these collisions by adding 1 tried entry
before adding 1 new table one, instead of 2 tried entries followed
by 2 new entries, which appears to have caused a collision in the CI.
To verify the regression test stills fails when expected:
- git checkout 181a120 && git cherry-pick ef242f5
- recompile bitcoind
- git checkout this branch and run test/functional/feature_asmap.py. Expected output:
```
AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed. !=
```
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
This commit adds specific expected reject reasons for segwit blocks
sent to the node, that are only showing up if one script threads
is used. For this reason, the node is started with the parameter
`-par=1`.
This commit adds specific expected reject reasons for segwit blocks
sent to the node, that are independent of whether multiple script threads
are activated or not.
fa4db8671b test: Activate all regtest softforks at height 1, unless overridden (MarcoFalke)
faad1e5ffd Introduce -testactivationheight=name@height setting (MarcoFalke)
fadb2ef2fa test: Add extra_args argument to TestChain100Setup constructor (MarcoFalke)
faa46986aa test: Remove version argument from build_next_block in p2p_segwit test (MarcoFalke)
fa086ef539 test: Remove unused ~TestChain100Setup (MarcoFalke)
Pull request description:
All softforks that are active at the tip of mainnet, should also be active from genesis in regtest. Otherwise their rules might not be enforced in user testing, thus making their testing less useful.
To still allow tests to check pre-softfork rules, a runtime argument can change the activation height.
ACKs for top commit:
laanwj:
Code review ACK fa4db8671b
theStack:
re-ACK fa4db8671b
Tree-SHA512: 6397d46ff56ebc48c007a4cda633904d6ac085bc76b4ecf83097c546c7eec93ac0c44b88083b2611b9091c8d1fb8ee1e314065de078ef15e922c015de7ade8bf
b69a106bcd test: use test_framework.p2p P2P_SERVICES in functional tests (Jon Atack)
Pull request description:
`P2P_SERVICES` is defined in `test/functional/test_framework/p2p.py`, so we can use it as a single definition for our functional tests. It may also be a tiny bit more efficient to use the constant rather than calculating `NODE_NETWORK | NODE_WITNESS` every time we need it in the tests.
ACKs for top commit:
laanwj:
Code review ACK b69a106bcd
klementtan:
crACK b69a106bcd
fanquake:
ACK b69a106bcd - didn't look at the formatting changes.
Tree-SHA512: f83e593663a69182986325d9ba2b4b787b87896d6648973f4f802f191a2573201b9e7d7e10e69662ef1965fa63268845726ed1aa5742a2e38dcccf4aebc6a961
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
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
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
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
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
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.
357f0c7233 ci: Enable more functional tests on Windows MSVC task (Hennadii Stepanov)
f55932678f qa: Fix "RuntimeError: Event loop is closed" on Windows (Hennadii Stepanov)
Pull request description:
On master (2161a05855), running functional tests that use the P2P interface ends with an error:
```
RuntimeError: Event loop is closed
```
This PR fixes this bug, and enables more functional tests on Windows MSVC CI task.
More details about bugfix:
- [What’s New In Python 3.7](https://docs.python.org/3/whatsnew/3.7.html#asyncio)
- https://bugs.python.org/issue33792
- actual [change](https://docs.python.org/3.8/library/asyncio-policy.html#asyncio.WindowsSelectorEventLoopPolicy) done in Python 3.8
Excluded tests, that are listed in the `EXCLUDE_TESTS` environment variable, need more thorough investigation to be enabled.
ACKs for top commit:
MarcoFalke:
review ACK 357f0c7233🌆
Tree-SHA512: d0ba85be81d55c934959ce7402a9c726598125e9751a1de179d16759d0e8b8a915de879c3a62c12d3564c5e0d9649ebd86963744449626efaa42d9eaa99ad3d0
util: Avoid locale dependent functions strtol/strtoll/strtoul/strtoull in ParseInt32/ParseInt64/ParseUInt32/ParseUInt64
fuzz: Assert equivalence between new and old Parse{Int,Uint}{8,32,64} functions
test: Add unit tests for ToIntegral<T>(const std::string&)
fa66a7d732 p2p: Rename fBlocksOnly, Add test (MarcoFalke)
fac66d0a39 test: Simplify p2p_blocksonly test with new miniwallet rescan_utxos method (MarcoFalke)
Pull request description:
`fBlocksOnly` has several issues:
* The name is confusing
* It is untested
Fix both.
ACKs for top commit:
laanwj:
Code review ACK fa66a7d732
Tree-SHA512: 4218f455eeb37297f74603d7d44895288605844ae828a40dfb7a70215f1a058ac5ad945a22732f5ebcad3ad375d54ba360bea69ea79639a30d4c88b042448f0f
