111864ac30126dc64a9e21d4e1b5e3d9ef4e5358 qa: Avoid duplicating output in case the diff is the same (Hodlinator)
c2e28d455af8fbb8d6074dc26590e61b9764d761 ci: Enable `wallet_multiwallet.py` in "Windows, test cross-built" job (Hodlinator)
850a80c1999e671b6cce33d8545af06adf5f77f0 qa: Disable parts of the test when running under Windows or root (Hodlinator)
fb803e3c79e52305df74ae30e77fd36900a49c24 qa: Test scanning errors individually (Hodlinator)
ed43ce57cce53612f13ac7c6db59fa7ac60e31c4 qa: Check for platform-independent part of error message (Hodlinator)
64a098a9b6263dbdeea25f89f4c9fe3c53943dd1 refactor(qa): Break apart ginormous run_test() (Hodlinator)
bb1aff7ed7e4bd6618dfe75b5faa9956c3adead4 move-only(qa): Move wallet creation check down to others (Hodlinator)
d1a4ddb58ef676d4e7436cc3bcdf5fb3008b4b6f refactor(qa): Lift out functions to outer scopes (Hodlinator)
c811e47367d531b69c10e3fc976df764e79f13e2 scripted-diff: self.nodes[0] => node (Hodlinator)
73cf858911056717a4ebe97cd250f3a506136eff refactor(qa): Remove unused option (Hodlinator)
Pull request description:
Makes the functional test compatible with *Linux->Windows cross-built executables*.
Main parts:
* Commit "qa: Check for platform-independent part of error message" switches to match on platform-independent part of error message.
* Commit "qa: Test scanning errors individually" disentangles code causing the same error message substring, based on #31410.
* Commit "qa: Disable parts of the test when running under Windows or root" enables the test to be run on Windows, based in part on https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-3554721014.
Also:
* Removes unused option in wallet_multiwallet.py.
* Breaks apart wallet_multiwallet.py's `run_test()` into smaller test functions.
* Improves `assert_equal()` output for dicts.
Fixes#31409.
ACKs for top commit:
achow101:
ACK 111864ac30126dc64a9e21d4e1b5e3d9ef4e5358
janb84:
re ACK 111864ac30126dc64a9e21d4e1b5e3d9ef4e5358
w0xlt:
reACK 111864ac30126dc64a9e21d4e1b5e3d9ef4e5358
Tree-SHA512: 4e3ff92588ac9f2611fc963ce62097b6c0dd4d4eb8da7952c72619c7b554ff3cae5163fe1886d4d9bbd7af1acca5b846411e7f5b46f9bddb08719b61108efbba
Normally, when a symlinked test script is executed directly (e.g.,
`./bld-cmake/test/functional/wallet_disable.py`), Python's default
behavior is to resolve the symlink of the script itself, setting
`sys.path[0]` to the directory containing the physical source file.
Consequently, the test framework util.py is imported from the source
tree, and `Path(__file__).parents[3]` correctly resolves to the source
root.
However, `feature_framework_testshell.py` is unique because it manually
inserts `Path(__file__).parent` into `sys.path`. That refers to the
build tree and when importing the test framework util.py, the
`Path(__file__).parents[3]` will incorrectly point to the build
directory instead of the source root.
Use `.resolve()` to ensure the Valgrind suppressions file path is always
calculated relative to the physical source file, regardless of how the
framework was imported.
Prior to this commit, tool_bitcoin.py was failing:
```sh
$ ./bld-cmake/test/functional/tool_bitcoin.py --valgrind
TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "./test/functional/test_framework/test_framework.py", line 138, in main
self.setup()
~~~~~~~~~~^^
File "./test/functional/test_framework/test_framework.py", line 269, in setup
self.setup_network()
~~~~~~~~~~~~~~~~~~^^
File "./test/functional/tool_bitcoin.py", line 38, in setup_network
assert all(node.args[:len(node_argv)] == node_argv for node in self.nodes)
~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
```
This commit fixes this issue by running `bitcoin` under valgrind. Also,
it comes with other improvements:
* Drop the outdated valgrind 3.14 requirement, because there is no
distro that ships a version that old anymore.
* Drop the VALGRIND_SUPPRESSIONS_FILE env var handling, because it was
presumably never used since it was introduced. Also, the use-case
seems limited.
Review note:
The set_cmd_args was ignoring the --valgrind test option.
In theory, this could be fixed by refactoring Binaries::node_argv() to
be used here. However, for now, just re-implement the node_argv logic in
set_cmd_args to prepend the valgrind cmd.
I feel it'd be easier to debug intermittent test failures if the
error message is present in the logs instead of error code. So,
switching order of error code and message in the `try_rpc` function
to aid error debugging.
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
The encoding arg is confusing, because it is not applied consistently
for all IO.
Also, it is useless, as the majority of files are ASCII encoded, which
are fine to encode and decode with any mode.
Moreover, UTF-8 is already required for most scripts to work properly,
so setting the encoding twice is redundant.
So remove the encoding from most IO. It would be fine to remove from all
IO, however I kept it for two files:
* contrib/asmap/asmap-tool.py: This specifically looks for utf-8
encoding errors, so it makes sense to sepecify the utf-8 encoding
explicitly.
* test/functional/test_framework/test_node.py: Reading the debug log in
text mode specifically counts the utf-8 characters (not bytes), so it
makes sense to specify the utf-8 encoding explicitly.
e9cd45e3d3c7592265ebf67387090b3df1501df4 test: set number of RPC server threads to 2 (furszy)
Pull request description:
The default `-rpcthreads` value spawns 16 HTTP server threads for each node.
Running the functional test suite with default `rpcthreads` can exhaust file
descriptors or hit other resource limits very easily (more when tests are run
in parallel).
Furthermore, having 16 threads is unnecessary since they are mostly idle. We
run RPC calls on a single RPC connection and wait for it result synchronously.
There is (almost) never two RPC calls occurring concurrently.
Because of this, the threads are mostly idle, so we can safely limit the number
of them to two.
Note for reviewers:
I checked this does not introduce any timing regression but would be good
to double-check it on your end too. We could add another thread if needed.
Just the 16 threads default value is too high and unnecessary.
ACKs for top commit:
maflcko:
lgtm ACK e9cd45e3d3c7592265ebf67387090b3df1501df4
l0rinc:
ACK e9cd45e3d3c7592265ebf67387090b3df1501df4
kevkevinpal:
ACK [e9cd45e](e9cd45e3d3)
andrewtoth:
ACK e9cd45e3d3c7592265ebf67387090b3df1501df4
Tree-SHA512: a777286f4a890fb87f5df72cd2ccfdc628657206a4b3e995044e5a0d12987b8c78a7cf7d684cc4e92605aa782aaeebc44e9f754752c3a524152fac94fa30f4b5
The default `-rpcthreads` value spawns 16 HTTP server threads for each node.
Running the functional test suite with default `rpcthreads` can exhaust file
descriptors or hit other resource limits very easily.
Moreover, having 16 threads is unnecessary since they are mostly idle. We
run RPC calls on a single RPC connection and wait for it result synchronously.
There is (almost) never two RPC calls occurring concurrently.
Because of this, the threads are mostly idle, so we can safely limit the number
of them to two.
Depending on the host machine, a default `par` value can spawn up to 15 script verification threads for each node.
Running the functional test suite with default `par` can exhaust file descriptors or hit other resource limits when many threads are spawned.
These threads are mostly idle and the same code paths are executed with a value of `par=2`.
Limit this to 2 for functional tests that do not override the default option.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
b104d442277090337ce405d92f1398b7cc9bcdb7 test: Remove RPCOverloadWrapper (Ava Chow)
4d32c19516fdb65b364638fa088b8d7167b438b6 test: Replace importpubkey (Ava Chow)
fe838dd391be669ccd0765b95f81c25fecfd3636 test: Replace usage of addmultisigaddress (Ava Chow)
d3142077794f4e910d3cdc749020d725e30feb24 test: Replace usage of importaddress (Ava Chow)
fcc457573f9b39e6f173a4f51c45d7dbb47e7ab0 test: Replace importprivkey with wallet_importprivkey (Ava Chow)
94c87bbbd06eb9a57930b9f59315533cfbe8f460 test: Remove unnecessary importprivkey from wallet_createwallet (Ava Chow)
Pull request description:
`RPCOverloadWrapper` implemented overloads for legacy wallet only RPCs so that the same function call could be used within tests for both legacy wallets and descriptor wallets. With legacy wallets now removed, there is no need to continue to have these overloads.
For `importaddress`, `addmultisigaddress`, and `importpubkey`, the uses of these are converted to `importdescriptors`.
For `importprivkey`, a new helper function `wallet_imporprivkey` is introduced that does what the overload did. This is mainly to reduce verbosity as `importprivkey` was more widely used throughout the tests.
Some tests that used these RPCs are now also no longer relevant and have been removed.
ACKs for top commit:
Sjors:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
pablomartin4btc:
cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7
rkrux:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
w0xlt:
ACK b104d44227
Tree-SHA512: ded2f73829e2ce28466d4a9738eb382783ad990daee5d1859dbc4d354e6f8eec0c483ed5ecb1287fe0dd24ac332065b733a30d71b126b841bd7cd49e9a094b6d
importprivkey was a legacy wallet only RPC which had a helper for
descriptor wallets in tests. Add wallet_importprivkey helper and use it
wherever importprivkey is used (other than backward compatibility tests)
10845cd7cc89fc83b4ee844dc577b391720bba9c qa: Add feature_framework_startup_failures.py (Hodlinator)
28e282ef9ae94ede4aace6b97ff18c66cb72a001 qa: assert_raises_message() - Stop assuming certain structure for exceptions (Hodlinator)
1f639efca5e71a0ff208415d94e408a74778d4db qa: Work around Python socket timeout issue (Hodlinator)
9b24a403fae4b896ff7705519bd48c877b4e621b qa: Only allow calling TestNode.stop() after connecting (Hodlinator)
6ad21b4c0114029d16d334bf8d437834708a295e qa: Include ignored errors in RPC connection timeout (Hodlinator)
879243e81fd55f54739fbdae4d5b8666f5acb9a9 qa refactor: wait_for_rpc_connection - Treat OSErrors the same (Hodlinator)
Pull request description:
Improves handling of startup errors in functional tests and puts tests in place to ensure knock-on errors don't creep in.
- `wait_for_rpc_connection()` now appends specific failures leading up to the `Unable to connect to bitcoind` error to that error message:
`[node 0] Unable to connect to bitcoind after 60s (ignored errors: {'missing_credentials': 1, 'OSError.ECONNREFUSED': 239}, latest error: ConnectionRefusedError(111, 'Connection refused'))`
- Fixes Windows Python issue where `socket.timeout` exceptions end up with unset `errno`-fields.
- Also adds comments, refactors code, improves logging.
The underlying purpose is to ensure developer efficiency in finding root causes of test failures.
Prior iterations of the PR partially focused on fixing the same issue as #31620.
Originally inspired by #30390.
### Testing
Can be tested by reverting either faf2f2c654d9aa18b2f49a157956f9ab0fce302a or fae3bf6b870eb0f9cddd1adac82ba72890806ae3 from #31620, or the "qa: Avoid calling stop-RPC if not connected" from this PR, and running *feature_framework_startup_failures.py*.
ACKs for top commit:
l0rinc:
ACK 10845cd7cc89fc83b4ee844dc577b391720bba9c
ryanofsky:
Code review ACK 10845cd7cc89fc83b4ee844dc577b391720bba9c. Only changes since last review were adding a new commit tweaking assert_raises_message(), extending the new test to have a self-check, and to pass through all options to child tests instead of a hardcoded list of options. I left some cleanup suggestions below but they are not important.
Tree-SHA512: f0235c5cbb6d1bb85d8dc5de492a08a34f6edc83499cbf0a5f9a3824809ff84635888c62c9c01101e3cc9ef9f1cdee2c9ab6537fea6feeb005b29f428caf8b22
In the functional tests there are lots of cases where we assert != which
this new util will replace, we also are adding the imports and the new assertion
111465d72dd35e42361fc2a089036f652417ed37 test: Remove unused attempts parameter from wait_until (Fabian Jahr)
5468a23eb9a3fd2b0c08dbca69fe3df58af42530 test: Add check_interval parameter to wait_until (Fabian Jahr)
16c87d91fd4d7709fa9d8824d5b641ef71821931 test: Introduce ensure_for helper (Fabian Jahr)
Pull request description:
A repeating pattern in the functional tests is that the test sleeps for a while to ensure that a certain condition is still true after some amount of time has elapsed. Most recently a new case of this was added in #30807. This PR here introduces an `ensure` helper to streamline this functionality.
Some approach considerations:
- It is possible to construct this by reusing `wait_until` and wrapping it in `try` internally. However, the logger output of the failing wait would still be printed which seems irritating. So I opted for simplified but similar internals to `wait_until`.
- This implementation starts for a failure in the condition right away which has the nice side-effect that it might give feedback on a failure earlier than is currently the case. However, in some cases, it may be expected that the condition may still be false at the beginning and then turns true until time has run out, something that would work when the test sleeps without checking in a loop. I decided against this design (and even against adding it as an option) because such a test design seems like it would be racy either way.
- I have also been going back and forth on naming. To me `ensure` works well but I am also not a native speaker, happy consider a different name if others don't think it's clear enough.
ACKs for top commit:
maflcko:
re-ACK 111465d72dd35e42361fc2a089036f652417ed37 🍋
achow101:
ACK 111465d72dd35e42361fc2a089036f652417ed37
tdb3:
code review re ACK 111465d72dd35e42361fc2a089036f652417ed37
furszy:
utACK 111465d72dd35e42361fc2a089036f652417ed37
Tree-SHA512: ce01a4f3531995375a6fbf01b27d51daa9d4c3d7cd10381be6e86ec5925d2965861000f7cb4796b8d40aabe3b64c4c27e2811270e4e3c9916689575b8ba4a2aa
Keep the "-upnp" option as a hidden arg for one major version in order
to show a more user friendly error to people who had this option set in
their config file.
ec317bc44b0cb089419d809b5fef38ecb9423051 test: update satoshi_round function (naiyoma)
Pull request description:
This PR refactors `satoshi_round` to accept different rounding modes and make rounding a required argument.
Continuation of https://github.com/bitcoin/bitcoin/pull/23225
ACKs for top commit:
maflcko:
review ACK ec317bc44b0cb089419d809b5fef38ecb9423051
achow101:
ACK ec317bc44b0cb089419d809b5fef38ecb9423051
AngusP:
ACK ec317bc44b0cb089419d809b5fef38ecb9423051
Tree-SHA512: 070f0aa6f66e58bff7412cae6b71f5f4ab8c718c7b5eeba4bb604fe22c6416a1ced0474294f504e1c28943ddc073104466b5944b43bae27e42bee5ca85afa468
Refactor satoshi_round function to accept different rounding modes.
Updated call site to use the revised `satoshi_round` function.
Co-authored-by: Kate Salazar <52637275+katesalazar@users.noreply.github.com>
bca346a97056748f1e3fb899f336d56d9fd45a64 net: require P2P binds to succeed (Vasil Dimov)
af552534ab83c572d3bc3f93ccaee5c1961ccab5 net: report an error if unable to bind on the Tor listening addr:port (Vasil Dimov)
9a7e5f4d68dadc64a675f32d1e91199d6b1aa095 net: don't extra bind for Tor if binds are restricted (Vasil Dimov)
Pull request description:
Make it possible to disable the Tor binding on `127.0.0.1:8334` and stop startup if any P2P bind fails instead of "if all P2P binds fail".
Fixes https://github.com/bitcoin/bitcoin/issues/22726
Fixes https://github.com/bitcoin/bitcoin/issues/22727
ACKs for top commit:
achow101:
ACK bca346a97056748f1e3fb899f336d56d9fd45a64
cbergqvist:
ACK bca346a97056748f1e3fb899f336d56d9fd45a64
pinheadmz:
ACK bca346a97056748f1e3fb899f336d56d9fd45a64
Tree-SHA512: fabef89a957191eea4f3e3b6109d2b8389f27ecc74440a920b0c10f31fff00a85bcfd1eb3c91826c7169c618f4de8a8d0a260e2caf40fd854f07ea9a980d8603
c9dacd958d7c1e98b08a7083c299d981e4c11193 test: Check that non empty version packet is ignored and no disconnection happens (stratospher)
997cc00b950a7d1b7f2a3971282685f4e81d87d2 test: Check that disconnection happens when AAD isn't filled (stratospher)
b5e6238fdbba5c777a5adfa4477dac51a82f4448 test: Check that disconnection happens when garbage sent/received are different (stratospher)
ad1482d5a20e6b155184a43d0724d2dcd950ce52 test: Check that disconnection happens when wrong garbage terminator is sent (stratospher)
e351576862471fc77b1e798a16833439e23ff0b4 test: Check that disconnection happens when >4095 garbage bytes is sent (stratospher)
e075fd131d668d9d1ba3c8566624481c4a57032d test: Introduce test types and modify v2 handshake function accordingly (stratospher)
7d07daa62311bdb0e2ce23d0b55f711f5088bd28 log: Add V2 handshake timeout (stratospher)
d4a1da8543522a213ac75761131d878eedfd4a5b test: Make global TRANSPORT_VERSION variable an instance variable (stratospher)
c642b08c4e45cb3a625a867ebd66c0ae51bde212 test: Log when the garbage is actually sent to transport layer (stratospher)
86cca2cba230c10324c6aedd12ae9655b83b2856 test: Support disconnect waiting for add_p2p_connection (stratospher)
bf9669af9ccc33dcade09bceb27d6745e9d9a75a test: Rename early key response test and move random_bitflip to util (stratospher)
Pull request description:
Add tests for the following v2 handshake scenarios:
1. Disconnection happens when > `MAX_GARBAGE_LEN` bytes garbage is sent
2. Disconnection happens when incorrect garbage terminator is sent
3. Disconnection happens when garbage bytes are tampered with
4. Disconnection happens when AAD of first encrypted packet after the garbage terminator is not filled
5. bitcoind ignores non-empty version packet and no disconnection happens
All these tests require a modified v2 P2P class (different from `EncryptedP2PState` used in `v2_p2p.py`) to implement our custom handshake behaviour based on different scenarios and have been kept in a single test file (`test/functional/p2p_v2_misbehaving.py`). Shifted the test in `test/functional/p2p_v2_earlykeyresponse.py` which is of the same pattern to this file too.
ACKs for top commit:
achow101:
ACK c9dacd958d7c1e98b08a7083c299d981e4c11193
mzumsande:
ACK c9dacd958d7c1e98b08a7083c299d981e4c11193
theStack:
Code-review ACK c9dacd958d7c1e98b08a7083c299d981e4c11193
Tree-SHA512: 90df81f0c7f4ecf0a47762d290a618ded92cde9f83d3ef3cc70e1b005ecb16125ec39a9d80ce95f99e695d29abd63443240cb5490aa57c5bc8fa2e52149a0672
In the Tor case, this prevents us from telling the Tor daemon to send
our incoming connections from the Tor network to an address where we
do not listen (we tried to listen but failed probably because another
application is already listening).
In the other cases (IPv4/IPv6 binds) this also prevents unpleasant
surprises caused by continuing operations even on bind failure. For
example, another application may be listening on portX, bitcoind tries
to bind on portX and portY, only succeeds with portY and continues
operation leaving the user thinking that his bitcoind is listening on
portX whereas another application is listening (the error message in
the log could easily be missed).
Avoid having the functional testing framework start multiple `bitcoind`s
that try to listen on the same `127.0.0.1:18445` (Tor listen for
regtest) if `bind_to_localhost_only` is set to `False`.
Also fix a typo in `test-shell.md` related to `bind_to_localhost_only`.
Fixes https://github.com/bitcoin/bitcoin/issues/22727
Early key response test is a special kind of test which requires
modified v2 handshake functions. More such tests can be added
where v2 handshake functions send incorrect garbage terminator,
excess garbage bytes etc.. Hence, rename p2p_v2_earlykey.py to a
general test file name - p2p_v2_misbehaving.py.
random_bitflip function (used in signature tests prior to this
commit) can be used in p2p_v2_misbehaving test to generate wrong
garbage terminator, wrong garbage bytes etc..
So, move the function to util.
In the functional tests, we often compare dicts with assert_equal, but the
output makes it very hard to tell exactly which entry in the dicts don't
match when there are a lot of entries and only minor differences. Change
the output to make it clearer.
a478c817b2f62b7334b36e331a2e37fe8380c754 test: replace `Callable`/`Iterable` with their `collections.abc` alternative (PEP 585) (stickies-v)
4b9afb18e6b9e16d7b299820f3a1382986a451d4 scripted-diff: use PEP 585 built-in collection types for verify-binary script (Sebastian Falbesoner)
d516cf83ed2da86dfefb395cd46f8a894907b88e test: use built-in collection types for type hints (Python 3.9 / PEP 585) (Sebastian Falbesoner)
Pull request description:
With Python 3.9 / [PEP 585](https://peps.python.org/pep-0585/), [type hinting has become a little less awkward](https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections), as for collection types one doesn't need to import the corresponding capitalized types (`Dict`, `List`, `Set`, `Tuple`, ...) anymore, but can use the built-in types directly (see https://peps.python.org/pep-0585/#implementation for the full list).
This PR applies the replacement for all Python scripts (i.e. in the contrib and test folders) for the basic types, i.e.:
- typing.Dict -> dict
- typing.List -> list
- typing.Set -> set
- typing.Tuple -> tuple
For an additional check, I ran mypy 1.6.1 on both master and the PR branch via
```
$ mypy --ignore-missing-imports --explicit-package-bases $(git ls-files "*.py")
```
and verified that the output is identical -- (from the 22 identified problems, most look like false-positives, it's probably worth it to go deeper here and address them in a follow-up though).
ACKs for top commit:
stickies-v:
ACK a478c817b2f62b7334b36e331a2e37fe8380c754
fanquake:
ACK a478c817b2f62b7334b36e331a2e37fe8380c754
Tree-SHA512: 6948c905f6abd644d84f09fcb3661d7edb2742e8f2b28560008697d251d77a61a1146ab4b070e65b0d27acede7a5256703da7bf6eb1c7c3a897755478c76c6e8
fe3ac3700d31a6329056fee983d91dd8e4c987c1 test: replace random_bytes with randbytes #28720 (ns-xvrn)
Pull request description:
With Python upgraded to 3.9 replaced the `random_bytes` function in util of functional tests and replaced it's usage with `random.randbytes`.
Closes#28720.
ACKs for top commit:
maflcko:
lgtm ACK fe3ac3700d31a6329056fee983d91dd8e4c987c1
BrandonOdiwuor:
ACK fe3ac3700d31a6329056fee983d91dd8e4c987c1
stickies-v:
ACK fe3ac3700d31a6329056fee983d91dd8e4c987c1, thanks for picking this up
kristapsk:
utACK fe3ac3700d31a6329056fee983d91dd8e4c987c1
Tree-SHA512: f65a75e73ebd840c2936eb133d42bccd552f25b717c8ca25c18d06e0593e12f292389cfcc0a0b0759004b67a46ea0c8ac237973ef90f246139778230be1e64e1
