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
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
fad4f44645 test: Set peertimeout in write_config (MarcoFalke)
Pull request description:
This avoids having to remember to set it whenever mocktime is used with
peer connections. Also, it might help avoiding disconnects when
attaching a debugger to a running test.
ACKs for top commit:
laanwj:
Concept and code review ACK fad4f44645
Tree-SHA512: 00c742571c0524c1b3f55e0217433ef7aa2dccccc12650caab98b4cf9231669f37fc589c7475f28d5725ffe2436c76205920eaece4a47fd27dc8872421a48e5c
This is done in order to prepare the make_utxo helper to use MiniWallet,
which only supports creating transactions with single inputs, i.e. we
need to create amounts small enough to be funded by coinbase transactions
(50 BTC).
This is done in order to prepare the make_utxo helper to use MiniWallet,
which only supports creating transactions with single inputs, i.e. we
need to create amounts small enough to be funded by coinbase transactions
(50 BTC).
PR #22697 introduced a reproducible issue in commit 181a1207 that causes the
addrman tried table to fail consistency checks and significantly lose peer
entries when the `-asmap` configuration option is used.
The issue occurs on 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` and recompile
- launch bitcoind with `-asmap` and `-checkaddrman=1` config options
- restart bitcoind
- bitcoind aborts on second call to `CAddrMan::Check()`
This commit adds a regression test to reproduce the case; it passes or fails
with the same error.
Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
The helper `send_large_txs` in its current interface has a fee_rate
parameter, implying that it would create a transaction with exactly that
rate. Unfortunately, this fee rate is only passed to MiniWallet's
`create_self_transfer` method, which can't know that we append several
tx outputs after, increasing the tx's vsize and decreasing it's fee rate
accordingly.
In our case, the fee rate is off by several orders of magnitude, as the
tx's vsize changes changes from 96 to 67552 vbytes (>700x), i.e. the
value passed to this function is neither really a fee rate nor an
absolute fee, but something in-between, which is very confusing.
Clarify the interface by passing an absolute fee that is deducted in the end
(and verified, via testmempoolaccept) and also describe how we come up with the
value passed.
08634e82c6 fix typos in logging messages (ShubhamPalriwala)
d447ded6ba replace: self.nodes[0] with node (ShubhamPalriwala)
dddca3899c test: use MiniWallet in mempool_limit.py (ShubhamPalriwala)
Pull request description:
This is a PR proposed in #20078
This PR enables running another non-wallet functional test even when the wallet is disabled thanks to the MiniWallet, i.e. it can be run even when bitcoin-core is compiled with --disable-wallet.
It also includes changes in wallet.py in the form of a new method, `create_large_transactions()` for the MiniWallet to create large transactions.
Efforts for this feature started in #20874 but were not continued and that PR was closed hence I picked this up.
To test this PR locally, compile and build bitcoin-core without the wallet and run:
```
$ test/functional/mempool_limit.py
```
ACKs for top commit:
amitiuttarwar:
ACK 08634e8, only git changes since last push (and one new line).
Zero-1729:
ACK 08634e82c6🧉
Tree-SHA512: 0f744ad26bf7a5a784aac1ed5077b59c95a36d1ff3ad0087ffd10ac8d5979f7362c63c20c2ce2bfa650fda02dfbcd60b1fceee049a2465c8d221cce51c20369f
This avoids having to remember to set it whenever mocktime is used with
peer connections. Also, it might help avoiding disconnects when
attaching a debugger to a running test.
The new name describes better what the bool does and also limits the confusion of the three different concepts:
* fBlocksOnly (This bool to skip tx invs)
* -blocksonly (A setting to ignore incoming txs)
* block-relay-only (A connection type in the block-relay-only P2P graph)
fa55c3dc1b Raise InitError when peers.dat is invalid or corrupted (MarcoFalke)
fa4e2ccfd8 Inline ReadPeerAddresses (MarcoFalke)
fa5aeec80c Move LoadAddrman from init to addrdb (MarcoFalke)
Pull request description:
peers.dat is silently erased when it can not be parsed or when it appears corrupted. Fix that by notifying the user. This might help in the following examples:
* The user provided the database, but picked the wrong one.
* A future version of Bitcoin Core wrote the file and it can't be read.
* The file was corrupted by a logic bug in Bitcoin Core.
* The file was corrupted by a disk failure.
ACKs for top commit:
jonatack:
Code review re-ACK fa55c3dc1b per `git range-diff eb1f570 fa59c6d fa55c3` and verified the new tests fail on master, except "Check mocked addrman is valid", as expected
prayank23:
tACK fa55c3dc1b
vasild:
ACK fa55c3dc1b
Tree-SHA512: 78264a78ee570a3c3262cf9c8542b5ffaffa5f52da1eef66c8c381f346989272967cfe1769c573502d9d7d3f7ad68c3ac3b2ec734185d2e4e7595b7122b14196
fa676dbac8 test: pep-8 whitespace (MarcoFalke)
faed284eab test: Avoid intermittent test failure in feature_csv_activation.py (MarcoFalke)
Pull request description:
Otherwise there will be disconnects if the test runs longer than the default peertimeout (60s):
```
node0 2021-09-05T20:28:30.973116Z (mocktime: 2021-09-01T07:17:29Z) [net] [net.cpp:1323] [InactivityCheck] socket receive timeout: 393061s peer=0
```
Fix that by skipping `InactivityCheck` via a large `-peertimeout`.
ACKs for top commit:
fanquake:
ACK fa676dbac8
Tree-SHA512: 061c0585a805aa2f8e55c4beedd4b8498a2951f33d60aa3632dda0a284db3a627d14a23dbd57e8a66c69a1612f39418e3a755c8ca97f6ae1105c0d70f0d1a801
fa7db1cbf7 [test] checks descendants limtis for second generation Package descendants (ritickgoenka)
Pull request description:
This PR adds a new functional test to test the new descendant limits for packages that were proposed in #21800.
```
+----------------------+
| |
| M1 |
| ^ ^ |
| M2 ^ |
| . ^ |
| . ^ |
| . ^ |
| . ^ |
| M24 ^ |
| ^ |
| P1 |
| ^ |
| P2 |
| |
+----------------------+
```
This test is for checking a transaction to fail its descendant count limits because of a combination of mempool descendants, package direct descendants, and package indirect descendants.
In this test, P1 has M1 as a mempool ancestor, P2 has no in-mempool ancestors, but when combined P2 has M1 as an ancestor and M1 exceeds descendant_limits (23 in-mempool descendants + 2 in-package descendants, a total of 26 including itself)
ACKs for top commit:
ryanofsky:
Code review ACK fa7db1cbf7. Only were suggested changes since last review: simplifying test and dropping P3 transaction as John suggested, and adding assert_equal I suggested
glozow:
ACK fa7db1cbf7
jnewbery:
ACK fa7db1cbf7
Tree-SHA512: d1eb993550ac8ce31cbe42e17c6522a213ede66970d5d9391f31a116477ab5889fefa6ff2df6ceadd63a28c1be1ad893b0e8e449247e9ade2ca61dc636508d68
e6998838e5 doc: Add IPv6 address to zmq example (nthumann)
8abe5703a9 test: Add IPv6 test to zmq (nthumann)
ded449b726 zmq: Enable IPv6 on listening socket (nthumann)
Pull request description:
This PR adds support for listening on IPv6 addresses with bitcoinds ZMQ interface, just like the RPC server.
Currently, it is not possible to specify an IPv6 address, as the `ZMQ_IPV6` [socket option](http://api.zeromq.org/master:zmq-setsockopt#toc27) is not set and therefore the ZMQ initialization fails, if one does so. The absence of this option has also been noted [here](https://github.com/bitcoin/bitcoin/issues/15198#issuecomment-617378512).
With this PR one can e.g. set `-zmqpubhashblock=tcp://[::1]:28333` to listen on the IPv6 loopback address.
ACKs for top commit:
laanwj:
Code review ACK e6998838e5
theStack:
Tested ACK e6998838e5🌱
Tree-SHA512: 43c3043d8d5c79794d475926259c1be975b694db4fcc1f7750a9a28e242f0fa1b531735a63ea5777498003aa5834f6243f39742d0f3941f2f37593d0c7890700
fa0b916971 scripted-diff: Use generate* from TestFramework (MarcoFalke)
Pull request description:
This is needed for #22567.
By using the newly added `generate*` member functions of the test framework, it paves the way to make it easier to implicitly call `sync_all` after block generation to avoid intermittent issues.
ACKs for top commit:
jonatack:
ACK fa0b916971
Tree-SHA512: e74a324b60250a87c08847cdfd7b6ce3e1d89b891659fd168f6dd7dc0aa718d0edd28285374a613f462f34f4ef8e12c90ad44fb58721c91b2ea691406ad22c2a
