9eac5a0529 [functional test] transaction orphan handling (glozow)
61e77bb901 [test framework] make it easier to fast-forward setmocktime (glozow)
Pull request description:
I was doing some mutation testing (through reckless refactoring) locally and found some specific behaviors in orphan handling that weren't picked up by tests. Adding some of these test cases now can maybe help with reviewing refactors like #28031.
- Parent requests aren't sent immediately. A delay is added and the requests are filtered by AlreadyHaveTx before they are sent, which means you can't use fake orphans to probe precise arrival timing of a tx.
- Parent requests include all that are not AlreadyHaveTx. This means old confirmed parents may be requested.
- The node does not give up on orphans if the peer responds to a parent request with notfound. This means that if a parent is an old confirmed transaction (in which notfound is expected), the orphan should still be resolved.
- Rejected parents can cause an orphan to be dropped, but it depends on the reason and only based on txid.
- Rejected parents can cause an orphan to be rejected too, by both wtxid and txid.
- Requests for orphan parents should be de-duplicated with "regular" txrequest. If a missing parent has the same hash as an in-flight request, it shouldn't be requested.
- Multiple orphans with overlapping parents should not cause duplicated parent requests.
ACKs for top commit:
instagibbs:
reACK 9eac5a0529
dergoegge:
reACK 9eac5a0529
achow101:
ACK 9eac5a0529
fjahr:
Code review ACK 9eac5a0529
Tree-SHA512: 85488dc6a3f62cf0c38e7dfe7839c01215b44b172d1755b18164d41d01038f3a749451241e4eba8b857fd344a445740b21d6382c45977234b21460e3f53b1b2a
Have each TestNode keep track of the last timestamp it called
setmocktime with, and add a bumpmocktime() function to bump by a
number of seconds. Makes it easy to fast forward n seconds without
keeping track of what the last timestamp was.
ba8ab4fc54 test: cover addrv2 support in anchors.dat with a TorV3 address (Matthew Zipkin)
b4bee4bbf4 test: add keep_alive option to socks5 proxy in test_framework (Matthew Zipkin)
5aaf988ccc test: cover TorV3 address in p2p_addrv2_relay (Matthew Zipkin)
80f64a3d40 test: add support for all networks in CAddress in messages.py (brunoerg)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27140
Adds test coverage for https://github.com/bitcoin/bitcoin/pull/20516 to ensure that https://github.com/bitcoin/bitcoin/issues/20511 is completed and may be closed.
This PR adds a test case to `feature_anchors.py` where an onion v3 address is set as a blocks-only relay peer and then shutdown, ensuring that the address is saved to anchors.dat in addrv2 format. We then ensure that bitcoin attempts to reconnect to that anchor address on restart.
To compute the addrv2 serialization of the onion v3 address, I added logic to `CAddress` in `messages.py`. This new logic is covered by extending `p2p_addrv2_relay.py` to include an onion v3 address. Future work will be adding coverage for ipv6, torv2 and cjdns in these modules and also `feature_proxy.py`
Also includes de/serialization unit test for `CAddress` in test framework.
ACKs for top commit:
jonatack:
ACK ba8ab4fc54
brunoerg:
crACK ba8ab4fc54
willcl-ark:
ACK ba8ab4fc54
Tree-SHA512: 7220e30d7cb975903d9ac575a7215a08e8f784c24c5741561affcbde12fb92cbf8704cb42e66494b788ba6ed4bb255fb0cc327e4f2190fae50c0ed9f336c0ff0
8a20f765cc test: drop duplicate getaddrs from p2p_getaddr_caching (Martin Zumsande)
feb0096139 test: fix intermittent failure in p2p_getaddr_caching (Martin Zumsande)
Pull request description:
Fixes#28133
In the consistency check, it's not enough to check that our address/port is unique, only the combination of source and target must be unique. Otherwise, the OS may reuse ports for connections to different `-addrbind`, which was happening in the failed runs.
While at it, the second commit cleans up duplicate `getaddr` messages in `p2p_getaddr_caching.py` that do nothing but generate `Ignoring repeated "getaddr"` log messages (and cleans up some whitespace the python linter complains about).
ACKs for top commit:
vasild:
ACK 8a20f765cc
Tree-SHA512: eabe4727d7887f729074076f6333a918bba8cb34b8e3baaa83f167b441b0daa24f7c4824abcf03a9538a2ef14b2d826ff19aeffcb93a6c20735253a9678aac9c
bee2d57a65 script: update flake8 to 6.1.0 (Jon Atack)
38c3fd846b test: python E721 updates (Jon Atack)
Pull request description:
Update our functional tests per [E721](https://www.flake8rules.com/rules/E721.html) enforced by [flake8 6.1.0](https://flake8.pycqa.org/en/latest/release-notes/6.1.0.html), and update our CI lint task to use that release. This makes the following linter output on current master with flake8 6.1.0 green.
```
$ ./test/lint/lint-python.py ; ./test/lint/lint-spelling.py
test/functional/p2p_invalid_locator.py:35:16: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
test/functional/test_framework/siphash.py:34:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
test/functional/test_framework/siphash.py:64:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
src/test/fuzz/descriptor_parse.cpp:88: occurences ==> occurrences
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
ACKs for top commit:
MarcoFalke:
lgtm ACK bee2d57a65
Tree-SHA512: f3788a543ca98e44eeeba1d06c32f1b11eec95d4aef068aa1b6b5c401261adfa3fb6c6d6c769f3fe6839d78e74a310d5c926867e7c367d6513a53d580fd376f3
fafe43cb6c scripted-diff: Use blocks_path where possible (MarcoFalke)
fa060c15fb test: Add blocks_path property to TestNode (MarcoFalke)
faba4fc325 test: Drop 22.x node from TxindexCompatibilityTest (MarcoFalke)
fa7f65b0f8 test: Use clean chain in MempoolCompatibilityTest (MarcoFalke)
Pull request description:
The node in this test was never really needed, because the compatibility tests shouldn't be used to test previous releases. (The test suite of the previous release itself should be used for that). So remove it.
Also, other test changes. (See individual commits)
ACKs for top commit:
theStack:
Code-review ACK fafe43cb6c
Tree-SHA512: 289f54695bf5310663ab38ebf1aa457f53d0aafae56e6657be0e75bf96b303165bad417dc7eaf4c40f0639aa92ce139e5bacb318a2eabab1f8e23a811cabe0cc
Only the combined addr:port of source and destination
must be unique. If the destination is different, the same addr:port
for the source may be used by the OS.
read() fails in text mode when the unicode hasn't been fully written
yet. Fixes: "wallet_importdescriptors.py: can't decode bytes in position
228861-228863: unexpected end of data"
(https://github.com/bitcoin/bitcoin/issues/28030)
20b49460b3 test: remove race in the user-agent reception check (Vasil Dimov)
Pull request description:
In `add_p2p_connection()` we connect to `bitcoind` from the Python client and check that it has received our version string.
This check looked up the last/newest entry from `getpeerinfo` RPC, assuming that it must be the connection we have just opened. But this will not be the case if a new inbound or outbound connection is made to/from `bitcoind` in the meantime.
Instead of the last entry in `getpeerinfo`, check all and find the one which corresponds to our connection using our outgoing address:port tuple which is unique.
ACKs for top commit:
jonatack:
re-ACK 20b49460b3
MarcoFalke:
Concept ACK 20b49460b3
Tree-SHA512: 61fd3359ef11ea830021ede0e745497a7b60690c32d21c47cd12ff79f78907bb45e922c9d01e5d7ff614a8cd5e4560d39a3fc86b01b200429773a23ace3917e4
5cf44275c8 test: refactor: deduplicate legacy ECDSA signing for tx inputs (Sebastian Falbesoner)
Pull request description:
There are several instances in functional tests and the framework (MiniWallet, feature_block.py, p2p_segwit.py) where we create a legacy ECDSA signature for a certain transaction's input by doing the following steps:
1. calculate the `LegacySignatureHash` with the desired sighash type
2. create the actual digital signature by calling `ECKey.sign_ecdsa` on the signature message hash calculated above
3. put the DER-encoded result as CScript data push into tx input's scriptSig
Create a new helper `sign_input_legacy` which hides those details and takes only the necessary parameters (tx, input index, relevant scriptPubKey, private key, sighash type [SIGHASH_ALL by default]). For further convenience, the signature is prepended to already existing data-pushes in scriptSig, in order to avoid rehashing the transaction after calling the new signing function.
ACKs for top commit:
dimitaracev:
ACK `5cf4427`
achow101:
ACK 5cf44275c8
pinheadmz:
ACK 5cf44275c8
Tree-SHA512: 8f0e4fb2c3e0f84fac5dbc4dda87973276242b0f628034272a7f3e45434c1e17dd1b26a37edfb302dcaf380dbfe98b0417391ace5e0ac9720155d8fba702031e
faf902858d test: Check expected_stderr after stop (MarcoFalke)
Pull request description:
This fixes a bug where stderr wasn't checked for the shutdown sequence.
Fix that by waiting for the shutdown to finish and then check stderr.
ACKs for top commit:
theStack:
ACK faf902858d
Tree-SHA512: a70cd1e6cda84d542782e41e8b59741dbcd472c0d0575bcef5cbfd1418473ce94efe921481d557bae3fbbdd78f1c49c09c48872883c052d87c5c9a9a51492692
The Socks5 server we use in the test framework would disconnect
by default immediately after the handshake and sometimes would
not register as a connected peer by bitcoind.
The thread does not only load blocks, it loads the mempool and,
in a future commit, will start the indexes as well.
Also, renamed the 'ThreadImport' function to 'ImportBlocks'
And the 'm_load_block' class member to 'm_thread_load'.
-BEGIN VERIFY SCRIPT-
sed -i "s/ThreadImport/ImportBlocks/g" $(git grep -l ThreadImport -- ':!/doc/')
sed -i "s/loadblk/initload/g" $(git grep -l loadblk -- ':!/doc/release-notes/')
sed -i "s/m_load_block/m_thread_load/g" $(git grep -l m_load_block)
-END VERIFY SCRIPT-
In `add_p2p_connection()` we connect to `bitcoind` from the Python
client and check that it has received our version string.
This check looked up the last/newest entry from `getpeerinfo` RPC,
assuming that it must be the connection we have just opened. But this
will not be the case if a new inbound or outbound connection is made
to/from `bitcoind` in the meantime.
Instead of the last entry in `getpeerinfo`, check all and find the one
which corresponds to our connection using our outgoing address:port
tuple which is unique.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Jon Atack <jon@atack.com>
There are several instances in functional tests and the framework
(MiniWallet, feature_block.py, p2p_segwit.py) where we create a legacy
ECDSA signature for a certain transaction's input by doing the following
steps:
1) calculate the `LegacySignatureHash` with the desired sighash type
2) create the actual digital signature by calling `ECKey.sign_ecdsa`
on the signature message hash calculated above
3) put the DER-encoded result as CScript data push into
tx input's scriptSig
Create a new helper `sign_input_legacy` which hides those details and
takes only the necessary parameters (tx, input index, relevant
scriptPubKey, private key, sighash type [SIGHASH_ALL by default]). For
further convenience, the signature is prepended to already existing
data-pushes in scriptSig, in order to avoid rehashing the transaction
after calling the new signing function.
4f4d039a98 test: add ellswift test vectors from BIP324 (stratospher)
a31287718a test: Add ellswift unit tests (stratospher)
714fb2c02a test: Add python ellswift implementation to test framework (stratospher)
Pull request description:
Built on top of https://github.com/bitcoin/bitcoin/pull/26222.
This PR introduces Elligator swift encoding and decoding in the functional test framework. It's used in #24748 for writing p2p encryption tests.
ACKs for top commit:
sipa:
ACK 4f4d039a98
theStack:
ACK 4f4d039a98🐊
Tree-SHA512: 32bc8e88f715f2cd67dc04cd38db92680872072cb3775478e2c30da89aa2da2742992779ea14da2f1faca09228942cfbd86d6957402b24bf560244b389e03540
6c97757a48 script: appease spelling linter (Jon Atack)
1316119ce7 script: update ignored-words.txt (Jon Atack)
146c861da2 script: update linter dependencies (Jon Atack)
92408224a4 test: fix PEP484 no implicit optional argument types errors (Jon Atack)
f86a301433 script, test: add missing python type annotations (Jon Atack)
Pull request description:
With these updates, `./test/lint/lint-python.py` and `./test/lint/lint-spelling.py` should be green again for developers using relatively recent Python dependencies, in particular mypy 0.991 (released 11/2022) and later. Please see the commit messages for details.
ACKs for top commit:
fanquake:
ACK 6c97757a48
Tree-SHA512: 8a46a4d36d5978affdcecf4f2ace20ca1b52d483e098304911a2169afe60ccb9b042fa90c04b762d94f3ce53d2cafe6f24476ae839867a770c7f31e7e7242d99
Fix warnings for these files when ./test/lint/lint-python.py is run using
mypy 0.991 (released 11/2022) and later:
$ test/lint/lint-python.py
test/functional/test_framework/coverage.py:23: error: Incompatible default for argument "coverage_logfile" (default has type "None", argument has type "str") [assignment]
test/functional/test_framework/coverage.py:23: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
test/functional/test_framework/util.py:318: error: Incompatible default for argument "timeout" (default has type "None", argument has type "int") [assignment]
test/functional/test_framework/util.py:318: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
test/functional/test_framework/util.py:318: error: Incompatible default for argument "coveragedir" (default has type "None", argument has type "str") [assignment]
test/functional/interface_rest.py:67: error: Incompatible default for argument "query_params" (default has type "None", argument has type "dict[str, Any]") [assignment]
test/functional/interface_rest.py:67: note: PEP 484 prohibits implicit Optional. Accordingly, mypy has changed its default to no_implicit_optional=True
Verified using https://github.com/hauntsaninja/no_implicit_optional
For details, see:
https://mypy-lang.blogspot.com/2022/11/mypy-0990-released.html
Fix warnings for these files when ./test/lint/lint-python.py is run using
mypy 0.991 (released 11/2022) and later:
"By default the bodies of untyped functions are not checked, consider using
--check-untyped-defs [annotation-unchecked]"
For details, see:
https://mypy-lang.blogspot.com/2022/11/mypy-0990-released.html
aaaa3aefbd test: Use TestNode *_path properties where possible (MarcoFalke)
dddd89962b test: Allow pathlib.Path as RPC argument via authproxy (MarcoFalke)
fa41614a0a scripted-diff: Use wallets_path and chain_path where possible (MarcoFalke)
fa493fadfb test: Use wallet_dir lambda in wallet_multiwallet test where possible (MarcoFalke)
Pull request description:
It seems inconsistent, fragile and verbose to:
* Call `get_datadir_path` to recreate the path that already exists as field in TestNode
* Call `os.path.join` with the hardcoded chain name or `self.chain` to recreate the TestNode `chain_path` property
* Sometimes even use the hardcoded node dir name (`"node0"`)
Fix all issues by using the TestNode properties.
ACKs for top commit:
willcl-ark:
re-ACK aaaa3aefbd
theStack:
Code-review ACK aaaa3aefbd🌊
Tree-SHA512: e4720278085beb8164e1fe6c1aa18f601558a9263494ce69a83764c1487007de63ebb51d1b1151862dc4d5b49ded6162a5c1553cd30ea1c28627d447db4d8e72
d4fb58ae8a test: EC: optimize scalar multiplication of G by using lookup table (Sebastian Falbesoner)
1830dd8820 test: add secp256k1 module with FE (field element) and GE (group element) classes (Pieter Wuille)
Pull request description:
This PR rewrites a portion of `test_framework/key.py`, in a compatible way, by introducing classes that encapsulate field element and group element logic, in an attempt to be more readable and reusable.
To maximize readability, the group element logic does not use Jacobian coordinates. Instead, group elements just store (affine) X and Y coordinates directly. To compensate for the performance loss this causes, field elements are represented as fractions. This undoes most, but not all, of the performance loss, and there is a few % slowdown (as measured in `feature_taproot.py`, which heavily uses this).
The upside is that the implementation for group laws (point doubling, addition, subtraction, ...) is very close to the mathematical description of elliptic curves, and this extends to potential future extensions (e.g. ElligatorSwift as needed by #27479).
ACKs for top commit:
achow101:
ACK d4fb58ae8a
theStack:
re-ACK d4fb58ae8a
stratospher:
tested ACK d4fb58a. really liked how this PR makes the secp256k1 code in the tests more intuitive and easier to follow!
Tree-SHA512: 9e0d65d7de0d4fb35ad19a1c19da7f41e5e1db33631df898c6d18ea227258a8ba80c893dab862b0fa9b0fb2efd0406ad4a72229ee26d7d8d733dee1d56947f18
32e2ffc393 Remove the syscall sandbox (fanquake)
Pull request description:
After initially being merged in #20487, it's no-longer clear that an internal syscall sandboxing mechanism is something that Bitcoin Core should have/maintain, especially when compared to better maintained/supported alterantives, i.e [firejail](https://github.com/netblue30/firejail).
There is more related discussion in #24771.
Note that given where it's used, the sandbox also gets dragged into the kernel.
If it's removed, this should not require any sort of deprecation, as this was only ever an opt-in, experimental feature.
Closes#24771.
ACKs for top commit:
davidgumberg:
crACK 32e2ffc393
achow101:
ACK 32e2ffc393
dergoegge:
ACK 32e2ffc393
Tree-SHA512: 8cf71c5623bb642cb515531d4a2545d806e503b9d57bfc15a996597632b06103d60d985fd7f843a3c1da6528bc38d0298d6b8bcf0be6f851795a8040d71faf16
On my machine, this speeds up the functional test feature_taproot.py by
a factor of >1.66x (runtime decrease from 1m16.587s to 45.334s).
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Seems odd to hardcode all parent directory names in the path for no good
reason.
Also, add wallet_path property to TestNode.
Also, rework wallet_backup.py test for scripted-diff in the next commit.
In functional tests it is a quite common scenario to generate fresh
elliptic curve keypairs, which is currently a bit cumbersome as it
involves multiple steps, e.g.:
privkey = ECKey()
privkey.generate()
privkey_wif = bytes_to_wif(privkey.get_bytes())
pubkey = privkey.get_pubkey().get_bytes()
Simplify this by providing a new `generate_keypair` helper function that
returns the private key either as `ECKey` object or as WIF-string
(depending on the boolean `wif` parameter) and the public key as
byte-string; these formats are what we mostly need (currently we don't
use `ECPubKey` objects from generated keypairs anywhere).
With this, most of the affected code blocks following the pattern above
can be replaced by one-liners, e.g.:
privkey, pubkey = generate_keypair(wif=True)
Note that after this commit, the only direct uses of `ECKey` remain in
situations where we want to set the private key explicitly, e.g. in
MiniWallet (test/functional/test_framework/wallet.py) or the test for
the signet miner script (test/functional/tool_signet_miner.py).
After initially being merged in #20487, it's no-longer clear that an
internal syscall sandboxing mechanism is something that Bitcoin Core
should have/maintain, especially when compared to better
maintained/supported alterantives, i.e firejail.
Note that given where it's used, the sandbox also gets dragged into the
kernel.
There is some related discussion in #24771.
This should not require any sort of deprecation, as this was only ever
an opt-in, experimental feature.
Closes#24771.
It seems odd to return `EXIT_SUCCESS` when the node aborted
execution due a fatal internal error or any post-init problem
that triggers an unrequested shutdown.
e.g. blocks or coins db I/O errors, disconnect block failure,
failure during thread import (external blocks loading process
error), among others.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
eefe56967b bugfix: Fix incorrect debug.log config file path (Ryan Ofsky)
3746f00be1 init: Error if ignored bitcoin.conf file is found (Ryan Ofsky)
398c3719b0 lint: Fix lint-format-strings false positives when format specifiers have argument positions (Ryan Ofsky)
Pull request description:
Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen:
- One case reported in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043) happens when a `bitcoin.conf` file in the default datadir (e.g. `$HOME/.bitcoin/bitcoin.conf`) has a `datadir=/path` line that sets different datadir containing a second `bitcoin.conf` file. Currently the second `bitcoin.conf` file is ignored with no warning.
- Another way this could happen is if a `-conf=` command line argument points to a configuration file with a `datadir=/path` line and that path contains a `bitcoin.conf` file, which is currently ignored.
This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant `-datadir` or `-conf` settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored.
ACKs for top commit:
pinheadmz:
re-ACK eefe56967b
willcl-ark:
re-ACK eefe56967b
TheCharlatan:
ACK eefe56967b
Tree-SHA512: 939a98a4b271b5263d64a2df3054c56fcde94784edf6f010d78693a371c38aa03138ae9cebb026b6164bbd898d8fd0845a61a454fd996e328fd7bcf51c580c2b
To avoid `bad-txns-premature-spend-of-coinbase` error,
when getting a utxo (using `get_utxo`) to create a new
transaction `get_utxo` shouldn't return by default
immature coinbase.
This change makes the `bitcoin-wallet` binary path customizable in the
same way how it can be done now with other ones, including `bitcoind`,
`bitcoin-cli` and `bitcoin-util`.
fac395e5eb ci: Bump ci/lint/Dockerfile (MarcoFalke)
fa6eb65167 test: Use python3.8 pow() (MarcoFalke)
88881cf7ac Bump python minimum version to 3.8 (MarcoFalke)
Pull request description:
There is no pressing reason to drop support for 3.7, however there are several maintenance issues:
* There is no supported operating system that ships 3.7 by default. (debian:buster is EOL and unmaintained to the extent that it doesn't run in the CI environment. See https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1484988445)
* Compiling python 3.7 from source is also unsupported on at least macos, according to https://github.com/bitcoin/bitcoin/pull/24017#issuecomment-1107820790
* Recent versions of lief require 3.8, see https://github.com/bitcoin/bitcoin/pull/27507#issuecomment-1517561645
Fix all maintenance issues by bumping the minimum.
ACKs for top commit:
RandyMcMillan:
ACK fac395e
fjahr:
ACK fac395e5eb
fanquake:
ACK fac395e5eb
Tree-SHA512: c198decdbbe29d186d73ea3f6549d8a38479383495d14a965a2f9211ce39637b43f13a4c2a5d3bf56e2d468be4bbe49b4ee8e8e19ec69936ff43ddf2b714c712
