5fd4836019 init: change shutdown order of load block thread and scheduler (Martin Zumsande)
Pull request description:
This avoids situations during a reindex, in which the shutdown doesn't finish since `LimitValidationInterfaceQueue()` is called by the load block thread when the scheduler is already stopped, in which case it would block indefinitely. This can lead to intermittent failures in `feature_reindex.py` (#30424), which I could locally reproduce with
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 74f0e4975c..be1706fdaf 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3446,6 +3446,7 @@ static void LimitValidationInterfaceQueue(ValidationSignals& signals) LOCKS_EXCL
AssertLockNotHeld(cs_main);
if (signals.CallbacksPending() > 10) {
+ std::this_thread::sleep_for(std::chrono::milliseconds(50));
signals.SyncWithValidationInterfaceQueue();
}
}
```
It has also been reported by users running `reindex-chainstate` (#23234).
I thought for a bit about potential downsides of changing this order, but couldn't find any.
Fixes#30424Fixes#23234
ACKs for top commit:
maflcko:
review ACK 5fd4836019
hebasto:
re-ACK 5fd4836019.
tdb3:
ACK 5fd4836019
BrandonOdiwuor:
Code Review ACK 5fd4836019
Tree-SHA512: 3b8894e99551c5d4392b55eaa718eee05841a7287aeef2978699e1d633d5234399fa2f5a3e71eac1508d97845906bd33e0e63e5351855139e7be04c421359b36
7cb9367157 rpc: keep .cookie if it was not generated (Roman Zeyde)
Pull request description:
Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock).
ACKs for top commit:
willcl-ark:
re-ACK 7cb9367157
achow101:
ACK 7cb9367157
kristapsk:
re-ACK 7cb9367157
stickies-v:
ACK 7cb9367157
Tree-SHA512: 0960dbc457975b0e0535f3d814824a879d7f85c9f1191537415b3fc253429a316a8e4badde56c8bc139778f132392983cec5fbe03891fb15ff61d3bc3f6e681b
Review note: The changes are complete, because self.options.descriptors
is set to None in parse_args (test_framework.py).
A value of None implies -disablewallet, see the previous commit.
So if a call to add_wallet_options is missing, it will lead to a test
failure when the wallet is compiled in.
The previous diff touched most files in ./test/, so bump the headers to
avoid having to touch them again for a bump later.
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./test/
-END VERIFY SCRIPT-
- Get rid of hardcoded wallet "" names and -wallet="" args
- Get rid of setup_nodes (-wallet, -nowallet, -disablewallet) argument rewriting
Motivation:
- Simplify test framework behavior so it's easier to write new tests without
having arguments mangled by the framework
- Make tests more readable, replacing unexplained "" string literals with
default_wallet_name references
- Make it trivial to update default wallet name and wallet data filename for
sqlite #19077 testing
- Stop relying on -wallet arguments to create wallets, so it is easy to change
-wallet option in the future to only load existing wallets not create new
ones (to avoid accidental wallet creation, and encourage use of wallet
encryption and descriptor features)
No longer create a default wallet. The default wallet will still be
loaded if it exists and not other wallets were specified (anywhere,
including settings.json, bitcoin.conf, and command line).
Tests are updated to be started with -wallet= if they need the default
wallet.
Added test to wallet_startup.py testing that no default wallet is
created and that it is loaded if it exists and no other wallets were
specified.
1abcecc40c Tests: Use self.chain instead of 'regtest' in almost all current tests (Jorge Timón)
Pull request description:
Simply avoiding the hardcoded string in more places for consistency.
It can also allow for more easily reusing tests for other chains other than regtest.
Separated from #8994 .
Continues #16509 .
It is still not complete (ie to be complete, we need the -chain parameter in #16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in #8994 ] ). But while being incomplete like #16509 , it's quite simple to review and another step forward IMO.
ACKs for top commit:
Sjors:
re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files.
elichai:
Code review ACK 1abcecc40c
ryanofsky:
Code review ACK 1abcecc40c.
ryanofsky:
Code review ACK 1abcecc40c
Tree-SHA512: 5620de6dab235ca8bd8670d6366c7b9f04f0e3ca9c5e7f87765b38e16ed80c17d7d1630c0d5fd7c5526f070830d94dc74cc2096d8ede87dc7180ed20569509ee
