fa6db79302d28e6b31b07b24580430614ffcd0e8 test: Avoid shutdown race in NetworkThread (MarcoFalke)
Pull request description:
Locally, I am seeing rare intermittent exceptions in the network thread:
```
stderr:
Exception in thread NetworkThread:
Traceback (most recent call last):
File "/python3.10/threading.py", line 1016, in _bootstrap_inner
self.run()
File "./test/functional/test_framework/p2p.py", line 744, in run
self.network_event_loop.run_forever()
File "/python3.10/asyncio/base_events.py", line 603, in run_forever
self._run_once()
File "/python3.10/asyncio/base_events.py", line 1871, in _run_once
event_list = self._selector.select(timeout)
AttributeError: 'NoneType' object has no attribute 'select'
```
I can reproduce this intermittently via `while ./bld-cmake/test/functional/test_runner.py $(for i in {1..400}; do echo -n "tool_rpcauth "; done) -j 400 ; do true ; done`.
I suspect this is a race where the shutdown starts the close of the network thread while it is starting.
A different exception showing this race can be reproduced via:
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 610aa4ccca..64561e157c 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -741,6 +741,7 @@ class NetworkThread(threading.Thread):
def run(self):
"""Start the network thread."""
+ import time;time.sleep(.1)
self.network_event_loop.run_forever()
def close(self, *, timeout=10):
```
It is trivial to reproduce via any test (e.g. `./bld-cmake/test/functional/tool_rpcauth.py`) and shows a similar traceback to the one above:
```
Exception in thread NetworkThread:
Traceback (most recent call last):
File "/python3.10/threading.py", line 1016, in _bootstrap_inner
self.run()
File "./test/functional/test_framework/p2p.py", line 745, in run
self.network_event_loop.run_forever()
File "/python3.10/asyncio/base_events.py", line 591, in run_forever
self._check_closed()
File "/python3.10/asyncio/base_events.py", line 515, in _check_closed
raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed
```
So fix the second runtime error in hope of fixing the first one as well.
ACKs for top commit:
brunoerg:
code review ACK fa6db79302d28e6b31b07b24580430614ffcd0e8
Tree-SHA512: ca352ebf7929456ea2bbfcfe4f726adcbfcfb3dc0edeaddae7f6926f998888f0bd8b987ddef60308266eeab6bffa7ebdc32f5908db9de5404df95635dae4a8f6
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.
88b0647f027a608acb61ec32329d19f8e5b0a9fd wallet: Always write last hardened cache flag in migrated wallets (Ava Chow)
8a08eef645eeb3e1991a80480c5ee232bfceeb37 tests: Check that the last hardened cache upgrade occurs (Ava Chow)
Pull request description:
#32597 set the descriptor cache upgraded flag for newly created wallets, but migrated wallets still did not have the flag set when they are migrated. For consistency, and to avoid an unnecessary upgrade, we should be setting this flag for migrated wallets.
The flag would end up being set anyways at the end of migration when the wallet is reloaded as it would perform the automatic upgrade at that time. However, this is unnecessary and we should just set it from the get go.
This PR also adds a couple tests to verify that the flag is being set, and that the upgrade is being performed.
ACKs for top commit:
cedwies:
re-ACK 88b0647
rkrux:
lgtm ACK 88b0647f027a608acb61ec32329d19f8e5b0a9fd
pablomartin4btc:
ACK 88b0647f027a608acb61ec32329d19f8e5b0a9fd
Tree-SHA512: 7d0850db0ae38eedd1e6a3bfaa548c6c612182291059fb1a47279a4c4984ee7914ecd02d8c7e427ef67bf9f5e67cbc57a7ae4412fad539e1bf3e05c512a60d69
With this change, tests can specify `self.extra_init = [{ipcbind: True}]` to
start a node listening on an IPC socket, instead of needing to choose which
node binary to invoke and what `self.extra_args=[["-ipcbind=..."]]` value to
pass to it.
The eliminates boilerplate code #30437 (interface_ipc_mining.py), #32297
(interface_ipc_cli.py), and #33201 (interface_ipc.py) previously needed in
their test setup.
Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to
the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in
#30437 and `bitcoin-cli` in #32297 to find the `bitcoin` binary and call
`bitcoin -m` to start nodes with IPC support. This way the new tests can run
whenever the ENABLE_IPC build option is enabled, instead of only running when
the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
When loading an older wallet without the last hardened cache, an
automatic upgrade should be performed. Check this in
wallet_backwards_compatibility.py
When migrating a wallet, the migrated wallet should always have the last
hardened cache, so verify in wallet_migration.py
This adds a missing catch for BaseException (e.g. SystemExit), which
would otherwise be silently ignored.
Also, remove the redundant other catches, which are just calling
log.exception with a redundant log message.
Before this change, when a functional test is run without building
the source, the error message suggested that previous release binaries
were missing.
When no previous release version is set, make the error message more
specifically about bitcoind.
666016e56b28b77f798dc85c767b95c1ca0abfae ci: use --usecli in one of the CI jobs (Martin Zumsande)
7ea248a020997fcbbd6d62a4ec191aa858c463ca test: Disable several (sub)tests with cli (Martin Zumsande)
f420b6356b6f886638282a6aa9309b9982768775 test: skip subtests that check for wrong types with cli (Martin Zumsande)
6530d0015b958412825e1b8ae2aaefeee4372f08 test: add function to convert to json for height_or_hash params (Martin Zumsande)
54d28722baeac84950bbe6da1d315b9202012259 test: Don't send empty named args with cli (Martin Zumsande)
cca422060e96abbdca68a931fd45653738923caa test: convert tuple to json for cli (Martin Zumsande)
af34e980866e16970e0c4b837f56cd29038ae8bc test: make rpc_psbt.py usable with --usecli (Martin Zumsande)
8f8ce9e1740dfb5249b20d2bf759c23080367553 test: rename .rpc to ._rpc and remove unnecessary uses (Martin Zumsande)
5b088859863224a94514c78ea841d7314ec8fa50 test: enable functional tests with large rpc args for cli (Martin Zumsande)
7d5352ac7373e2beb9af671cef0a1a1e9ecb6658 test: use -stdin for large rpc commands (Martin Zumsande)
6c364e0c10de4d8036e696b3726bb0acffb94617 test: Enable various tests for usage with cli (Martin Zumsande)
Pull request description:
Fixes#32264
I looked into all current failures listed in the issue, as well all tests that are already disabled for the cli with `self.supports_cli = False`. There are several reasons why existing tests fail with `--usecli` on many systems, the most important ones are:
- Most common reason is that the test executes a RPC call with a large arg that exceeds `MAX_ARG_STRLEN` of the OS, which is usually 128kb on linux: This is fixed by using `-stdin` for these large calls (idea by 0xB10C)
- they test specifically the rpc interface - nothing to do there except disabling.
- Some functional test submit wrong types to params on purpose to test the error message (which is different when using the cli) - deactivated these specific subtests locally for the cli when there is just one or two of them, deactivated the entire tests when there are more spots
- When python sets `None` for an arg, the cli converts this to 'null' in `arg_to_cli`. This is fine e.g. for boolean args, but doesn't work for strings where it's interpreted as the string 'null'. Bypass this for named args by not including args in case the value is `None` for the cli is used (it's effectively the same as leaving the optional arg out).
- the `height_or_hash` param used in some RPC needs to be converted to a JSON (effectively adding full quotes).
- Some tests were marked with `self.supports_cli = False` in the past but run fine on master today - enabled those.
In total, this PR fixes all tests that fail on master and reduces the number of tests that are deactivated (`self.supports_cli = False`) from 40 to 21.
It also adds `--usecli` to one CI job (multiprocess, i686, DEBUG) to detect regressions.
ACKs for top commit:
maflcko:
re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae 🔀
pinheadmz:
re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae
Tree-SHA512: 7a1efd212649ca100b236a1239294d40ecd36e2720e3b173a230b14545bb40b135111db7fed8a0d1448120f5387da146a03f1912e2028c8d03a0b6a3ca8761b0
941b8f54c0d35d3243bb6083f3b52681d1b9a555 ci: run get_previous_releases as part of test cross win job (Max Edwards)
5e2182140bcd26afbe89b13d1d83ece6d5a89731 test: increment mocked time for migrating wallet backups (Max Edwards)
5174565802f426db1e9c8200a9421f82f61a5c99 ci: disable feature_unsupported_utxo_db functional test (Max Edwards)
3dc90d69a64f8bb39af27fa755683589b1bc76a7 test: remove mempool.dat before copying (Max Edwards)
67a6b20d5030ee51bf6d5e0fd77c9bdc8bafa96b test: add windows support to get previous releases script (Max Edwards)
1a1b478ca31be1f670754a47da17863271e46b7b scripted-diff: rename tarball to archive (Max Edwards)
4f06dc848460c887ad8337702ed900ba78725906 test: remove building from source from get prev releases script (Max Edwards)
Pull request description:
This PR updates the `test/get_previous_releases.py` script to also work on Windows by changing to be pure python rather than using unix tools such as `curl` and `tar`.
This enables additional functional tests to run such as `wallet_migration.py`, `mempool_compatability.py` and `wallet_backwards_compatibility.py`.
Unfortunately `feature_unsupported_utxo_db.py` _could_ run but this test requires Bitcoin `v0.14.3` which will not run under windows with emojis in the data directory (as the functional test runner has by default) . This test could be run as it's own step in the ci workflow file and would pass but as it's quite an old version / feature I have assumed it's not worth worrying about and best just to exclude.
Two tests needed to be slightly modified to run under windows. Both were issues with trying to overwrite a file that already exists which windows seems to be more strict on than the unix based systems.
Finally, building from source has been dropped from the `get_previous_releases.py` script. This had not been updated after the move to cmake and so it was assumed that nobody could have been using that feature.
ACKs for top commit:
maflcko:
re-ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555 🍪
achow101:
ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555
hodlinator:
re-ACK 941b8f54c0d35d3243bb6083f3b52681d1b9a555
Tree-SHA512: 22933d0ec278b9b0ffcd2a8e90026e1a3631b00186e7f78bd65be925049021e319367d488c36a82ab526a07b264bac18c2777f87ca1174b231ed49fed56d11cb
Using the get_previous_releases.py script to build from source only works for
releases prior to v29 due to removal of Autotools (in favor of CMake). It also
does not support building on Windows, and we are adding support for downloading
Windows release binaries in later commits of this PR.
As there were no complaints during review, it is assumed nobody uses this
functionality.
a5ac43d98d1ad3ebed934f2c50208a85aae17e5e doc: Add release notes describing bitcoin wrapper executable (Ryan Ofsky)
258bda80c009a25d1f1bdeffccf9ed1ffde29cb2 doc: Mention bitcoin wrapper executable in documentation (Ryan Ofsky)
d2739d75c911c8bf73a4d3005c57add1ae4a67ae build: add bitcoin.exe to windows installer (Sjors Provoost)
ba649c00063a43b59a63db17b509179a658a8d9a ci: Run multiprocess tests through wrapper executable (Ryan Ofsky)
29bdd743bb843f8b8ed2e426b6df36e9d7e54215 test: Support BITCOIN_CMD environment variable (Ryan Ofsky)
9c8c68891b43053acfe7b8eb9d2e0d2bcfcb4e1e multiprocess: Add bitcoin wrapper executable (Ryan Ofsky)
5076d20fdb70a4bfafc4bdfe8293e347cb6bfa78 util: Add cross-platform ExecVp and GetExePath functions (Ryan Ofsky)
Pull request description:
Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion.
Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983.
---
Initial implementation of this feature is deliberately minimal so the UX can evolve in response to feedback and there are not too many details to debate and discuss in a single PR. But many improvements are possible or planned:
- Adding manpage and bash completions.
- Showing nicer error messages that detect if an executable isn't installed and suggest how to fix [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
- Showing wrapper command lines in subcommand in help output [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405). This could be done conditionally as suggested in the comment or be unconditional.
- Showing wrapper command lines in subcommand error output. There is a bitcoin-cli error pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091152243) that is needlessly confusing.
- Integrating help so `bitcoin help subcommand` invokes `bitcoin subcommand -h`. `bitcoin -h subcommand` should also be supported and be equivalent [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
- Adding support for `bitcoin-util` subcommands. Ideal interface would probably be more like `bitcoin grind` not `bitcoin util grind` but this has been punted for now. Supporting subcommands directly would require some ArgsManager modifications
- Adding a dedicated python functional test for the wrapper. Right now there is some CI coverage by setting the `BITCOIN_CMD` variable, but this doesn't cover things like the help output and version output, and support for different directory layouts.
- Better `--multiprocess` (`-m`) / `--monolithic` (`-M`) default selection. Right now, default is monolithic but it probably makes sense to chose more intelligently depending on whether -ipc options are enabled and what binaries are available.
- Maybe parsing `bitcoin.conf` and supporting options to control wrapper behavior like custom locations or preferences or aliases.
- Better command command line usability. Allow combining short options like (`-ah`). Allow fuzzy matching of subcommands or suggestions if you misspell. (suggested by stickies in review club)
- Not directly related to this PR but `bitcoin-cli named` implementation used by the wrapper should do a better job disambiguating named arguments from base64 arguments ending in = as pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). A review club meeting for it took place in https://bitcoincore.reviews/31375
ACKs for top commit:
Sjors:
utACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
achow101:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
vasild:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
theStack:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
ismaelsadeeq:
fwiw my last review implied an ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
hodlinator:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Tree-SHA512: 570e6a4ff8bd79ef6554da3d01f36c0a7c6d2dd7dace8f8732eca98f4a8bc2284474a9beadeba783114fe2f3dd08b2041b3da7753bae0b7f881ec50668cb821f
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
Would otherwise cause an OverflowError in feature_framework_startup_failures.py when calling subprocess.run() with 60 * factor.
Fixes#32506
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
Support new BITCOIN_CMD environment variable in functional test to be able to
test the new bitcoin wrapper executable and run other commands through it
instead of calling them directly.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
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)
Removes all legacy wallet specific functional tests.
Also removes the --descriptor and --legacy-wallet options as these are
no longer necessary with the legacy wallet removed.
2f2ab47bf74f4da37aad75a186cb0bb16e8af579 Release notes (Pol Espinasa)
bf194c920cf768d1339d41aef1441a78e2f5fcbe wallet, rpc: deprecate settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR deprecates the settxfee RPC and paytxfee setting, marking it for removal in Bitcoin Core 31.0.
**Motivation**
The PR was initially motivated by https://github.com/bitcoin/bitcoin/issues/31088. The intention was to create a new function `settxfeerate` to allow users to set a static fee rate in `sat/vB` instead of `btc/kvB`.
The `settxfee` RPC allows users to set a static fee rate for all transactions created by the wallet. However, in a dynamic fee environment, this can lead to poor fee choices, either overpaying when the mempool is empty or underpaying when congestion is high. The preferred approach is to rely on fee estimation, which is designed to adapt to network conditions, and is the one by default. Same argument apply for `paytxfee` setting.
During discussion the consensus was that static fee settings are a footgun and that users should instead specify the fee rate per transaction if they don't want to rely on the fee estimation. Given this, rather than introducing a `settxfeerate` alternative, this PR goes towards removing `settxfee` and `paytxfee` entirely.
**Key Changes**
`settxfee` and `paytxfee` is now deprecated and will be removed in Bitcoin Core 31.0.
Users should rely on fee estimation or explicitly specify a fee rate when constructing transactions.
**Impact on Users**
If users currently use settxfee or paytxfee, they should transition to specifying fees per transaction.
No immediate breakage in 30.0 (must use `-deprecatedrpc=settxfee`), but `settxfee` and `paytxfee` will be removed in 31.0.
**Alternative Approaches Considered**
A settxfeerate alternative (using sat/vB) was initially proposed but ultimately rejected in favor of deprecating static fee setting entirely.
**Notes for removal**
- When removing paytxfee we should also update txconfirmtarget startup option help text.
- Get back the comment from `rpc_deprecated.py` test. [+info](https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1998876768)
ACKs for top commit:
fjahr:
re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
ismaelsadeeq:
re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
rkrux:
Concept and utACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
Tree-SHA512: 0272812cbe5a519737c5d0683acc2072e67559792b4a6472bca8b23426e5ce9e88a3a1eba987feda70a082b8b474b3126893848628d7bf11e1520357b18c8d3e
Add new TestNode.binaries object to manage paths to bitcoin binaries.
Having this object makes it possible for the test framework to exercise the
bitcoin wrapper executable introduced in
https://github.com/bitcoin/bitcoin/pull/31375 and also makes it easier to add
new binaries and options and environment variables controlling how they are
invoked, because logic for invoking them that was previously spread out is now
consolidated in one place.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
Require that sqlite is available in order to compile the wallet. Removes
instances of USE_SQLITE since it is no longer possible to not have
sqlite available.
The NO_SQLITE option is dropped from depends.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
This change:
1. Collects build artifacts in dedicated locations.
2. Allows running bitcoin-chainstate.exe with bitcoinkernel.dll directly
from the build tree on Windows.
faf2f2c654d9aa18b2f49a157956f9ab0fce302a test: Avoid redundant stop and error spam on shutdown (MarcoFalke)
fae3bf6b870eb0f9cddd1adac82ba72890806ae3 test: Avoid redundant stop and error spam on startup failure (MarcoFalke)
fa0dc09b9002f0bcae63af6af8d37fb3e0040ef4 test: Remove --noshutdown flag (MarcoFalke)
fad441fba07877ea78ed6020fde11828307273a6 test: Treat leftover process as error (MarcoFalke)
Pull request description:
The `--noshutdown` flag is brittle, confusing, and redundant:
* Someone wanting to inspect the state after a test failure will likely also want to debug the state on the python side, so the option is redundant with `--pdbonfailure`. If there was a use case to replicate `--pdbonfailure` without starting pdb, a dedicated flag could be added for that use case.
* It is brittle to use the flag for a passing test, because it will disable checks in the test. For example, on shutdown LSan will perform a leak check, and the test framework will check that the node did not crash, and it will check that the node did not print errors to stderr.
Fix all issues by removing it.
Also, tidy up startup error messages to be less confusing as a result.
ACKs for top commit:
hodlinator:
re-ACK faf2f2c654d9aa18b2f49a157956f9ab0fce302a
pablomartin4btc:
re tACK faf2f2c654d9aa18b2f49a157956f9ab0fce302a
Tree-SHA512: 46d7ae59c7be88b93f1f9e0b6be21af0fc101e646512e2c5e725682cb18bfec8aa010e0ebe89ce9ffe239e5caac0da5f81cc97b79e738d26ca5fa31930e8e4e3
1b51616f2e3b58a1c63a19e6dba8e7e9c2aefdeb test: improve rogue calls in mining functions (i-am-yuvi)
Pull request description:
#31403 follow-up, see [comment](https://github.com/bitcoin/bitcoin/pull/31403#pullrequestreview-2498806354)
- Rename `invalid_call` parameter to `called_by_framework` in `generateblock`, `generatetoaddress` and `generatetodescriptor` mining methods to better express its intended usage.
- Add explicit assertion message clarifying that these functions should only be called by TestFramework itself to maintain proper node synchronization.
ACKs for top commit:
maflcko:
lgtm ACK 1b51616f2e3b58a1c63a19e6dba8e7e9c2aefdeb
achow101:
ACK 1b51616f2e3b58a1c63a19e6dba8e7e9c2aefdeb
hodlinator:
re-ACK 1b51616f2e3b58a1c63a19e6dba8e7e9c2aefdeb
Prabhat1308:
ACK [1b51616](1b51616f2e)
Tree-SHA512: 56832626fe54dcaa07dacb4f9c960c0a83fad3fb12272155114ac697856c59b7f44805e1152eddeec7a5e8f7daf487382dc01b5b9ae2e74b62b2df6bd1f81f77
Trying to shut down a node after a test failure may fail and lead to an
RPC error.
Also, it is confusing to sidestep the existing fallback to kill any
leftover nodes on a test failure.
So just rely on the fallback.
Idea by Hodlinator.
Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Trying to immediately shut down a node after a startup failure without
waiting for the RPC to be fully up will in most cases just fail and lead
to an RPC error.
Also, it is confusing to sidestep the existing fallback to kill any
leftover nodes on a test failure.
So just rely on the fallback.
If the `releases` directory exists, but still only a subset of the
necessary previous release binaries are available, the test fails by
throwing an exception (sometimes leading to follow-up exceptions like
"AssertionError: [node 0] Error: no RPC connection") and printing out
a stack trace, which can be confusing and at a first glance suggests
that the node crashed or some alike.
Improve this by checking and printing out *all* of the missing release
binaries and failing with an explicit error in this case. Also add an
info on how to download previous releases binaries.
Noticed while testing #30328.
Can be tested by e.g.
$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py
