f708498293c27f63507cfa08c74909ba3d1aa675 torcontrol: Limit reconnect timeout to max seconds and log delay in whole seconds (Eval EXEC)
Pull request description:
I'm reviewing the Tor controller's reconnect-related code and noticed that the reconnect timeout had no limit. This could lead to excessively long delays.
This PR introduces a maximum reconnect timeout of 600 seconds (10 minutes) to prevent excessive delays in reconnection attempts. It also updates the log message to display the retry delay in whole seconds for better readability.
ACKs for top commit:
mabu44:
ACK f708498293c27f63507cfa08c74909ba3d1aa675
laanwj:
Code review ACK f708498293c27f63507cfa08c74909ba3d1aa675
luke-jr:
utACK f708498293c27f63507cfa08c74909ba3d1aa675
Tree-SHA512: 8f18c6c84da6b4e7328638fd74539fbd3dd44f46c5107638de56b72fc079487690861199ceba1197ca34421dcedf79a1ca6bacf2a918a683e71bce9ff710b5d4
fa310cc6f499e3e3dd58dc382e04e9db3f02bc3b test: Fix intermittent issue in p2p_orphan_handling.py (MarcoFalke)
Pull request description:
The test may fail intermittently when the `net` thread is lagging while calling `DeleteNode`. This may result in a split `getdata`, meaning that `peer2.wait_for_parent_requests([int(parent_peekaboo_AB["txid"], 16), int(parent_missing["txid"], 16)])` fails.
Fix it by adding a sync on the `net` thread.
Fixes#31700
ACKs for top commit:
mzumsande:
Code Review ACK fa310cc6f499e3e3dd58dc382e04e9db3f02bc3b
Tree-SHA512: e4a58093ab5b9e280c479b845fecb5d228e65519ea3dc2111b393202225fd0feded423e8812452454b6b9348cb37a9c1b01b9d1b1802e9f4aa76b9e56b4b54ef
fa4fb6a8f15c295a2a3d3ffd737e115b8be46c1f fuzz: Use serial task runner to increase fuzz stability (MarcoFalke)
Pull request description:
Leaking a scheduler with a non-empty queue from the fuzz initialization phase into the fuzz target execution phase is problematic, because it messes with coverage data. This in turn is problematic, because it leads to:
* Decrease in fuzz target execution stability (non-determinism when running the fuzz target).
* Decrease in fuzz input merge stability (non-determinism when selecting a minimum set of fuzz input to reach maximum coverage), which leads to qa-assets bloat.
Fix one such issue. Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018
Can be tested via: `RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake $PWD/../b-c-qa-assets/fuzz_corpora/ partially_downloaded_block`.
The failure is non-deterministic (obviously) and will show coverage in validation signals such as `UpdatedBlockTip` before this change and will have this one fixed after this change.
ACKs for top commit:
marcofleon:
ACK fa4fb6a8f15c295a2a3d3ffd737e115b8be46c1f
dergoegge:
Code review ACK fa4fb6a8f15c295a2a3d3ffd737e115b8be46c1f
Tree-SHA512: fd1f66562c1d3c21553c7dd324399cdc16faa2fedfdb8e7544ea6a68b8b356e7c81d81815ecf70e0d334307dab6b275c1889b3b889b6f15eec514beee22c95f4
d190f0facc8379da7610d7161e532d57c6a7eb96 test, contrib: Fix signer/miner command line escaping (Ryan Ofsky)
0d2eefca8bf3cb64e3f2f912ed32118524430967 test, refactor: Add TestNode.binaries to hold binary paths (Ryan Ofsky)
Pull request description:
Add new `TestNode.binaries` object to manage paths to bitcoin binaries.
The `binaries` 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 in general to add new binaries, and new 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.
These changes were originally part of #31375 but made that PR harder to review because they were unrelated to the other changes there. If this PR can get merged first, python changes in #31375 will be simple, and the test framework changes here should also get a higher quality review.
ACKs for top commit:
maflcko:
re-review-ACK d190f0facc8379da7610d7161e532d57c6a7eb96 🍓
Sjors:
ACK d190f0facc8379da7610d7161e532d57c6a7eb96
vasild:
ACK d190f0facc8379da7610d7161e532d57c6a7eb96
Tree-SHA512: 5a6c0553cd2822585810d827ef1c1772cbf3097d3336daf733f8378dd3da79c00fc3721e50ed0f7455908fbd7a509e9739f9be33f588d6bc1aaa400b9d75c650
ffff4a293ad878494e12f8f00108cc99ee2b713e bench: Update span-serialize comment (MarcoFalke)
fa4d6ec97bcb1790a7cd4363a13fda7c80c3dd90 refactor: Avoid false-positive gcc warning (MarcoFalke)
fa942332b40c97375af0722f32f7575bca3af819 scripted-diff: Bump copyright headers after std::span changes (MarcoFalke)
fa0c6b7179c062b7ca92d120455ce02a9f4e9e19 refactor: Remove unused Span alias (MarcoFalke)
fade0b5e5e6e80e3da1ab6448b6212244bafa5d3 scripted-diff: Use std::span over Span (MarcoFalke)
fadccc26c03db00a2be3f703aa7e5eec4312bd2e refactor: Make Span an alias of std::span (MarcoFalke)
fa27e36717ec18d64b7ff7bba71b8f0c202ba31d test: Fix broken span_tests (MarcoFalke)
fadf02ef8bf96ad5b3b8e34fd425b31b555f4371 refactor: Return std::span from MakeUCharSpan (MarcoFalke)
fa720b94be17fa9e7c91188710e6a04939ceab11 refactor: Return std::span from MakeByteSpan (MarcoFalke)
Pull request description:
`Span` has some issues:
* It does not support fixed-size spans, which are available through `std::span`.
* It is confusing to have it available and in use at the same time with `std::span`.
* It does not obey the standard library iterator build hardening flags. See https://github.com/bitcoin/bitcoin/issues/31272 for a discussion. For example, this allows to catch issues like the one fixed in commit fabeca3458b38a3d8930cb0cbc866388c3f120f1.
Both types are type-safe and can even implicitly convert into each other in most contexts.
However, exclusively using `std::span` seems less confusing, so do it here with a scripted-diff.
ACKs for top commit:
l0rinc:
reACK ffff4a293ad878494e12f8f00108cc99ee2b713e
theuni:
ACK ffff4a293ad878494e12f8f00108cc99ee2b713e.
Tree-SHA512: 9cc2f1f43551e2c07cc09f38b1f27d11e57e9e9bc0c6138c8fddd0cef54b91acd8b14711205ff949be874294a121910d0aceffe0e8914c4cff07f1e0e87ad5b8
fac3d93c2ba84899c2c6516b5449f61ef653d9fa fuzz: Speed up *_package_eval fuzz targets a bit (MarcoFalke)
fa40fd043ab23eb8948c208ca82f75f3d40bb2e4 fuzz: [refactor] Avoid confusing c-style cast (MarcoFalke)
Pull request description:
Each target is at least 10% faster for me when running over the current set of qa-assets, which seems nice.
The changes `outpoints_value` from a map to an unordered map, which is safe, because the element order is not used in the fuzz test and the map is only used for lookup.
(`mempool_outpoints` can't be changed, because the order matters here. Using unordered_set here may result in a non-deterministic fuzz target, given the same fuzz input.)
ACKs for top commit:
l0rinc:
ACK fac3d93c2ba84899c2c6516b5449f61ef653d9fa
dergoegge:
Code review ACK fac3d93c2ba84899c2c6516b5449f61ef653d9fa
Tree-SHA512: 8ae5d4e281505aff76a4003d6e9ea388dbb73860e167385bd6a0a201b3acc939db29ee212594952a9e80e85b3cc4cd726ce6dd49551f74013cb4da8a15cbdfb3
c8fab356171a0e283d5716647e3243c04810ac51 ci: remove -Wno-error=deprecated-declarations from ASAN (fanquake)
a130bbd154d535b80fe9f602a13bf2e322817881 Squashed 'src/leveldb/' changes from 04b5790928..4188247086 (fanquake)
Pull request description:
Cherry-picks two commits from upstream (302786e211, e829478c6a), which remove the usage of `std::aligned_storage/std::aligned_union`.
Note the first cherry-pick is not clean, because due to Google tooling issues, it accidently contained a revert of the prior two commits. See https://github.com/google/leveldb/pull/1249 for more details.
Also see https://issues.chromium.org/issues/388068052, although note that they [reverted the roll to latest leveldb](https://issues.chromium.org/issues/388068052#comment9). I'm guessing due to the acidental reversion issue above.
ACKs for top commit:
l0rinc:
ACK c8fab356171a0e283d5716647e3243c04810ac51
darosior:
ACK c8fab356171a0e283d5716647e3243c04810ac51 -- checked it's a clean subtree pull from https://github.com/bitcoin-core/leveldb-subtree/tree/bitcoin-fork
dergoegge:
utACK c8fab356171a0e283d5716647e3243c04810ac51
Tree-SHA512: 966e61b9ac88af5ae7bf71514bfd5bbdbd8c38c7af65feb6d5e4415062dcff5896dc33fe968ded3462cc599abd921d49ee8336db3e12ed3f59c91ceb949317b7
387385ba1edf9febdc75d39bd77b35b29714b3d0 test: replace assert with assert_equal and assert_greater_than (Chandra Pratap)
Pull request description:
In `test/functional/interface_usdt_net.py`, `assert_equal` is already used to check for equality between objects. Replace `assert.*==` with `assert_equal` and `assert.*>` with `assert_greater_than` to further easify debugging.
Relevant issue: #23119
ACKs for top commit:
maflcko:
lgtm ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
0xB10C:
had a quick look, lgtm ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
theStack:
utACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
brunoerg:
code review ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
i-am-yuvi:
Great! ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
Tree-SHA512: 741a3d98288c9999f62bcbaa3806716b0519ec9b521e1e6e17aa458392245f6eff886af6cb601c66f2147e0265ff1eae57cea3dcfd67af93bef6dff25b056935
cec14ee47d71d8dd5ca8f90b760d807c3c8933a5 test: switch wallet_crosschain.py to signet (Sjors Provoost)
9c2951541c22c1e2fb9406095487ce8c5c360769 test: drop testnet4 from wallet_crosschain.py (Sjors Provoost)
Pull request description:
It's sufficient to check only one test network, so this PR reverts the addition of testnet4 from #29775.
Testnet3 is deprecated. Instead of moving to testnet4, which might also be deprecated in the future, use signet.
ACKs for top commit:
fjahr:
utACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5
maflcko:
lgtm ACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5 🌰
Tree-SHA512: c5aad6e7d251957f090145eac906f7985fddc3e3ba82df7184d72b961f9c856d324a1065ac98323b75501d136bd7b669fcc2565b9e66b0743eb3f3906ef37570
20fe41e9e83d510fd467f5a999d55a614b16ef89 test: avoid disk space warning for non-regtest (Sjors Provoost)
Pull request description:
`feature_config_args.py` incorrectly assumed that its testnet4 node would not log a disk space warning.
But when #31978 increased `m_assumed_blockchain_size` on testnet4 from 1 to 11 GiB, it triggered this bug on my RAM disk, see https://github.com/bitcoin/bitcoin/tree/master/test#speed-up-test-runs-with-a-ram-disk
This PR fixes the issue by using `-prune` which prevents the warning.
ACKs for top commit:
fjahr:
ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89
maflcko:
lgtm ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89
rkrux:
ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89
Tree-SHA512: f4bbb3ede307e06bf097a3cf7a4099eacc9388e33f551e1d0c4c5f53747bfa593a4b22e5d2e713ce6dd8adf91602fade36fbec9cfc2b250a6b1cf09f11bc8473
In test/functional/interface_usdt_net.py, assert_equal is already
used to check for equality between objects. Replace 'assert.*=='
with 'assert_equal' and 'assert.*>' with 'assert_greater_than'
to further easify debugging.
Pass bitcoin binary command lines from test framework to signet/miner utility
using shell escaping so they are unambigous and don't get mangled if they
contain spaces.
This change is not needed for tests to pass currently, but is a useful change
to avoid CI failures in followup PR
https://github.com/bitcoin/bitcoin/pull/31375 and to avoid other bugs.
21e9d39a3725cd6107b742f0cb97f65b3640201b docs: add release notes for 31603 (brunoerg)
a8b548d75d9a376c9bb66e06bb918c876416d615 test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys (brunoerg)
c7afca3d62cf5d3ea9b98d5a76e4e54cac07bc3c test: descriptor: check whitespace into keys (brunoerg)
cb722a3cea16a04844c83e56fd6deaa1f0dc0a7e descriptor: check whitespace in ParsePubkeyInner (brunoerg)
50856695ef6c02ecbaa0cf448567355b6b86b510 test: fix descriptors in `ismine_tests` (brunoerg)
Pull request description:
Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment (e.g. `pk( KEY)`, `pk(KEY )` or `pk( KEY )`). I have noticed that one of the reasons is that the `DecodeBase58` function simply ignore these whitespaces.
This PR changes the `ParsePubkeyInner ` to reject pubkeys that contain a whitespace at the beginning and/or at the end. We will only check the whitespace in some RPCs (e.g. `importdescriptors`), but an already imported descriptor won't be affected by this check, especially because we store descriptors from `ToString`.
For context: https://github.com/brunoerg/bitcoinfuzz/issues/72
ACKs for top commit:
rkrux:
tACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
darosior:
re-ACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
sipa:
utACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
Tree-SHA512: 54f48a89a235517e5cdc29a46dceeb7dabbee93c7616a166288ff3f90131808eb0ece43b0797a11fe827a5f7bd51d65e3e75c16789b0a42020934cabb684cc8f
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>
1f9b2e150ce5aa192226d2daae73f7d678c47d65 cmake: Require `zip` only for `deploy` target (Hennadii Stepanov)
0aeff2995138c247f1b1f843c30eb547e199f5f7 cmake: Check for `makensis` tool before using it (Hennadii Stepanov)
Pull request description:
For `x86_64-w64-mingw32` and `*-apple-darwin` targets, the optional `deploy` target requires dedicated tools: `makensis` and `zip`, respectively.
This PR introduces a uniform checks for those tools when attempting to build the `deploy` target, ensuring they are not required for configuring and building any other targets.
Here is an example of workflow for `x86_64-w64-mingw32`:
```
$ # `nsis` is not installed
$ cmake -B build -G "GNU Makefiles" --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
$ cmake --build build -j $(nproc)
$ cmake --build build -t deploy
Error: NSIS not found.
Please install NSIS and/or ensure that its executable is accessible to the find_program() command—
for example, by setting the MAKENSIS_EXECUTABLE variable or another relevant CMake variable.
Then re-run cmake to regenerate the build system.
Built target deploy
$ sudo apt install nsis
$ cmake -B build
$ cmake --build build -t deploy
...
[100%] Generating bitcoin-win64-setup.exe
[100%] Built target deploy
```
Fixes https://github.com/bitcoin/bitcoin/issues/32018.
ACKs for top commit:
hodlinator:
re-ACK 1f9b2e150ce5aa192226d2daae73f7d678c47d65
fanquake:
ACK 1f9b2e150ce5aa192226d2daae73f7d678c47d65
Tree-SHA512: 5e2bd28a13bd8fa7c4ba8cf1756d200a4651afe83c463d76ece10027cca343e124eff97012a5368028f761df60f420ab891106b4e33b50045051d57c7464ff98
This reverts the changes to test/functional/wallet_crosschain.py
in commit 74a04f9e7ad6a16988149cc3438b9ce13c91cdb9.
It's sufficient to check only one test network. The next commit
will change that network away from testnet3.
54e6eacc1fccd602897d9e3025c62f83194ffd5b test: Enable ResetCoverageCounters beyond Linux (janb84)
Pull request description:
In PR [#31901](https://github.com/bitcoin/bitcoin/pull/31901), Coverage.cpp was introduced as a separate utility file, based on existing code. However, the macro defined in Coverage.cpp was limited to Clang and Linux, which caused issues for users on macOS when using the newly introduced deterministic test tooling.
This change adds fallback functions which are used when building without code coverage on non linux env.
This adds support for macOS to ResetCoverageCounters. ResetCoverageCounters is used by the unit tests in `g_rng_temp_path_init` to support the deterministic unit test tooling. It is also used in fuzz tests to completely suppress coverage from anything init-related.
See [Readme](https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md) on how to test this for deterministic unit & fuzz test.
Suggestion for test files:
- for unit test: `util_string_tests`
- for fuzz test: `addition_overflow `
These files should give deterministic results
ACKs for top commit:
maflcko:
review-only ACK 54e6eacc1fccd602897d9e3025c62f83194ffd5b
hodlinator:
re-ACK 54e6eacc1fccd602897d9e3025c62f83194ffd5b
Tree-SHA512: dd71da6f76d4fc9e64bf521bbfe5e7483d77c2ca0380f9e692502e64b529068ea33f21b19399481feb7c6780a23d893d8b7f733cef641a2db18a13397c98deea
7d34c19853e7a5528d69c5f30580e7e9712e61f0 ci: Drop ENABLE_HARDENING=OFF from clang-tidy (David Gumberg)
Pull request description:
Split out from #32071
It's not clear why this was added in the first place, but it is not necessary currently.
https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2723888193https://github.com/bitcoin/bitcoin/pull/24753.
ACKs for top commit:
fanquake:
ACK 7d34c19853e7a5528d69c5f30580e7e9712e61f0
Tree-SHA512: 9ffab10349f52aed77d002155da59f13f0954e6bc1ac1edbddcbb9e70aa2db3b0b0307eb35f8b791589f2157705a96d463589d4270f4873e9037a9ef88807b56
fa3b4427158d48f7d899582580f8f6a7b1bc981d ci: Use Cirrus dockerfile cache (MarcoFalke)
Pull request description:
The lint task is problematic, because:
* It doesn't check modifications to `ci/lint_imagefile`
* It calls a separate script that installs packages on every run (taking time)
* It uses `*_cache` instructions to cache some installed parts, but not all
Fix all issues by using `ci/lint_imagefile` (https://cirrus-ci.org/guide/docker-builder-vm/#dockerfile-as-a-ci-environment)
ACKs for top commit:
willcl-ark:
ACK fa3b4427158d48f7d899582580f8f6a7b1bc981d
Tree-SHA512: 243d78219639b83721d4e5cb32d16e5c208a61c919d04646279be5825ba92d97c490b5d4d28ea103eb820b1a259904574cb3e32eaca3f11c3031810e3d87ff4a
6f9f415a4fa3a3c1dc87c207b0192a5ed0a91e0f doc: shallow clone qa-assets (Lőrinc)
Pull request description:
While reviewing https://github.com/bitcoin/bitcoin/pull/31457#pullrequestreview-2690077410 I noticed that cloning `qa-assets` takes a lot of time - shallow cloning should suffice here.
I haven't checked the other clones in this file but suggestion are welcome.
ACKs for top commit:
maflcko:
lgtm ACK 6f9f415a4fa3a3c1dc87c207b0192a5ed0a91e0f
Tree-SHA512: 21bd676c7709dbf7fd30b239d0a72f9c230453ed8f8a1b5319ac92ef9c5e67780939f095a239dd31bcb4550f8d69eaed4931a221e19cb0b957f18fac623c4a01
52482cb24400f8c44ba9628aaaecb7c04b11beb2 test: Check datadir cleanup after assumeutxo was successful (Fabian Jahr)
Pull request description:
I noticed that the proper datadir cleanup after a successful restart of an assumutxo node does not seem to be covered in our tests. This is added here.
ACKs for top commit:
l0rinc:
utACK 52482cb24400f8c44ba9628aaaecb7c04b11beb2
mabu44:
Re-ACK 52482cb24400f8c44ba9628aaaecb7c04b11beb2
Prabhat1308:
re-ACK [`52482cb`](52482cb244)
TheCharlatan:
Re-ACK 52482cb24400f8c44ba9628aaaecb7c04b11beb2
Tree-SHA512: cc941afeba250050eaccf5112255d961253fec9b2683545454a0d2fbe4d542178394b301d169a9dd79edbf6b5d478d95282727dbb0aca96ee79d4cd1ff80f19b
7ebc458a8cb994bc3c0c129da61353968d955bc2 qt: doc: adapt outdated binary paths to CMake changes (Sebastian Falbesoner)
Pull request description:
Adapt the qt-related instances of outdated binary paths to `./build/bin/...` (see [#30454](https://github.com/bitcoin/bitcoin/pull/30454) and the more recently merged [#31161](https://github.com/bitcoin/bitcoin/pull/31161)). According to `$ git grep src/qt.*bitcoin` there should be no more left to address.
ACKs for top commit:
maflcko:
lgtm ACK 7ebc458a8cb994bc3c0c129da61353968d955bc2
Sjors:
utACK 7ebc458a8cb994bc3c0c129da61353968d955bc2
fanquake:
ACK 7ebc458a8cb994bc3c0c129da61353968d955bc2
hebasto:
ACK 7ebc458a8cb994bc3c0c129da61353968d955bc2.
Tree-SHA512: 8cd6579fdf209ec4ee3c4c9cfb94cb11d5d5115068d31613d356ca1303214dc4461580535c2d3f2773f373a4271e9a82df25596d8369eef8235822f7030d88bd
4cd95a2921805f447a8bcecc6b448a365171eb93 refactor: modernize remaining outdated trait patterns (Lőrinc)
ab2b67fce20fd7d8017f8a26425cab99e91f420d scripted-diff: modernize outdated trait patterns - values (Lőrinc)
8327889f358289f918d04ddb9469fb5562720bf4 scripted-diff: modernize outdated trait patterns - types (Lőrinc)
Pull request description:
The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11).
The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly.
I've modified the instances I found in the codebase one-by-one (noticed them while investigating https://github.com/bitcoin/bitcoin/pull/31868), and afterwards extracted scripted diff commits to do the trivial ones automatically.
The last commit contains the values that were easier done manually.
I've excluded changes from `src/bench/nanobench.h`, `src/leveldb`, `src/minisketch`, `src/span.h` and `src/sync.h` - let me know if you think they should be included instead.
A few of the code changes can also be reproduced by clang-tidy (but not all of them):
```bash
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc)
run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,modernize-type-traits' -fix $(git grep -lE '::(value|type)' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h')
```
ACKs for top commit:
laanwj:
Concept and code review ACK 4cd95a2921805f447a8bcecc6b448a365171eb93
Tree-SHA512: a4bcf0f267c0f4e02983b4d548ed6f58d464ec379ac5cd1f998b9ec0cf698b53a9f2557a05a342b661f1d94adefc9a0ce2dc8f764d49453aaea95451e2c4c581
3301d2cbe8c3b76c97285d75fa59637cb6952d0b qa: Wait for txindex to avoid race condition (Hodlinator)
9bfb0d75ba10591cc6c9620f9fd1ecc0e55e7a48 qa: Remove unnecessary -txindex args (Hodlinator)
7ac281c19cd3d11f316dbbb3308eabf1ad4f26d6 qa: Add missing coverage of corrupt indexes (Hodlinator)
Pull request description:
- Add synchronization in 3 places where if the Transaction Index happens to be slow, we get rare test failures when querying it for transactions (one such case experienced on Windows, prompting investigation).
- Remove unnecessary TxIndex initialization in some tests.
- Add some test coverage where TxIndex aspect could be tested in feature_init.py.
ACKs for top commit:
fjahr:
re-ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
mzumsande:
Code Review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
furszy:
Code review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
Prabhat1308:
Concept ACK [`3301d2c`](3301d2cbe8)
Tree-SHA512: 7c2019e38455f344856aaf6b381faafbd88d53dc88d13309deb718c1dcfbee4ccca7c7f1b66917395503a6f94c3b216a007ad432cc8b93d0309db9805f38d602
36b0713edc4655f6e0c291975d6d280fbc89cf2e test: fix intermittent failure in wallet_reorgsrestore.py (furszy)
Pull request description:
In response to #32066 intermittent failure.
Wait until the node's process has fully stopped before starting a new instance of it.
Same behavior as in the [tool_wallet.py](698f86964c/test/functional/tool_wallet.py (L540)) test.
ACKs for top commit:
maflcko:
lgtm ACK 36b0713edc4655f6e0c291975d6d280fbc89cf2e
Chand-ra:
tACK [36b0713](36b0713edc)
Tree-SHA512: 8e01493ef1fb58589479f3e12d7429d02ca75a2183d5f79d3b6a2fbf13334878926274a20857f1b4729afc1d30b65789daed229ce06ba236b91d949b73f45d5a
02942056fd861581503a8a35a06dcf22d4ba1473 test: fix intermittent failure in p2p_orphan_handling.py (Martin Zumsande)
Pull request description:
If the mocktime is bumped before the node has successfully disconnected the peer, the requests for both parents could be spread over two GETDATAS: The first time `GetRequestsToSend` is invoked it would only request one tx from peer2, because the other one would only be available after peer1 was disconnected and its outstanding txrequest cleared.
So two GETDATAs would be sent, which would make the test fail.
Fixes#31700
ACKs for top commit:
maflcko:
lgtm ACK 02942056fd861581503a8a35a06dcf22d4ba1473
instagibbs:
ACK 02942056fd
Tree-SHA512: 769200898345da197d86d673d9506f08f0a64b72a456e7e7c988ac37450d9c54ec65da1c8447c566c8578f7cfccdc5723ea680e636bfbe0b3d38265e5ef57774
Non-Linux linkers require a fallback implementation for when coverage is not enabled.
The fallbacks are marked weak to have lower precedence than built-in implementations when available, removing ambiguity from the linker.
9157d9e449870851ef455e077249ac46fc2df24c build: use make < 3.82 syntax for define directive (Sjors Provoost)
Pull request description:
From the GNU make 3.82 [release announcement](https://lists.gnu.org/archive/html/info-gnu/2010-07/msg00023.html) (2010):
> The 'define' make directive now allows a variable assignment operator
after the variable name, to allow for simple, conditional, or appending
multi-line variable assignment.
macOS ships with 3.81 (2006). This caused the multiprocess config options to be ignored.
Fixes#32068
ACKs for top commit:
ryanofsky:
Code review ACK 9157d9e449870851ef455e077249ac46fc2df24c. This is a pretty unusual bug and I don't understand how it wasn't causing any errors with make 3.81, just causing the flags to be ignored.
Tree-SHA512: a07322e25ef18296264379a2704f31c654df196d3ea09fe712885c38813e54d758a2d603ee9f7a302da8011fba6d139aa30a356175ca99df728ade2572a87560
fa9cf38ab666b50b0d8e82cb17f9e5a8a613547d scripted-diff: test: Rename send_message to send_without_ping (MarcoFalke)
fa4356717d6efcc41709f62a5116c62519d1a853 test: Prefer send_and_ping over send_message+sync_with_ping (MarcoFalke)
Pull request description:
`send_message` is problematic, because it is easy to forget a `sync_with_ping` (or other `wait_until`), leading to intermittent test failures. (Example: https://github.com/bitcoin/bitcoin/pull/31837#discussion_r1950370246)
There are more uses of `send_and_ping` in the codebase than `send_message`, so in most cases `send_and_ping` is needed anyway.
For the remaining cases, clearly document that no sync happens by renaming `send_message` to `send_without_ping`.
ACKs for top commit:
instagibbs:
ACK fa9cf38ab6
Tree-SHA512: 31caa6568d292ae3d3dda931a94aaa30cc1205ec2ef537a484393eb55687f86c212f1e751ac4a7636610bdf591502a50995dc63bf02f97be9fdc482072160b07
d5537c18a9034647ba4c9ed4008abd7fee33989e fuzz: make sure DecodeBase58(Check) is called with valid values more often (Lőrinc)
bad1433ef2b5b02ac4b1c6c1d9482c513e5b2192 fuzz: Always restrict base conversion input lengths (Lőrinc)
Pull request description:
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/30746, expanding coverage by:
* restricting every input for the base58 conversions, capping max sizes to `100` instead of `1000` or all available input (suggested by marcofleon in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963718683) since most actual usage has lengths of e.g. `21`, `34`, `78`.
* providing more valid values to the decoder (suggested by maflcko in https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1957847712) by randomly providing a random input or a valid encoded one; this also enables unifying the roundtrip tests to a single roundtrip per fuzz.
ACKs for top commit:
mzumsande:
Code Review / lightly tested ACK d5537c18a9034647ba4c9ed4008abd7fee33989e
maflcko:
review ACK d5537c18a9034647ba4c9ed4008abd7fee33989e 🚛
Tree-SHA512: 50365654cdac8a38708a7475eaa43396642b7337e2ee8999374c3faafff4f05457abc1a54c701211e0ed24d36c12af77bcad17b49695699be42664f2be660659
2819c514825577dc8d73690882f25117d412ed9f test: Use rpc_deprecated only for testing deprecation (Fabian Jahr)
Pull request description:
The comment in `functional/rpc_deprecated.py` says "This test should be used to verify correct behaviour of deprecated RPC methods with and without the -deprecatedrpc flags." I think we can get rid of the "with" part since we can assume that every deprecated RPC is already tested in at least one other functional test. (I didn't look but I could verify in our coverage if someone has doubts about that.) In order for this test to continue working, the flag will need to be used there. Otherwise this seems to prescribe copy+pasting a basic test from another file and I don't see a good reason for that.
ACKs for top commit:
maflcko:
lgtm ACK 2819c514825577dc8d73690882f25117d412ed9f
janb84:
re ACK [2819c51](2819c51482)
polespinasa:
reACK 2819c514825577dc8d73690882f25117d412ed9f
Tree-SHA512: 96edfd07be863ad19f99feb27afbc2c3ad53560c93c93eac8de8d766ad8c46e5aa02fd013fec99af794cbe9adca8e459d5b80b454caea7e67b2388003e010bb6
feature_config_args.py incorrectly assumed that its testnet4 node
would not log a disk space warning.
0683b8ebf33386d5c05140df89df10b1853d7c7e increased m_assumed_blockchain_size
on testnet4 from 1 to 11 GiB which triggers this bug on more
systems, e.g. a RAM disk.
Prevent the warning by setting -prune for these nodes.
Fix the same issue in feature_signet.py
From the GNU make 3.82 release announcement:
* The 'define' make directive now allows a variable assignment operator
after the variable name, to allow for simple, conditional, or appending
multi-line variable assignment.
macOS ships with 3.81. This caused the multiprocess config options
to be ignored.
Fixes#32068
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Wait until the node's process has fully stopped before starting a new instance.
Since the same code is used in tool_wallet.py, this consolidates the behavior
into a 'kill_process()' function.
send_message only drops the bytes in a buffer and a sync is needed to
avoid intermittent test issues. Change the name of the method to make
this more apparent during review.
-BEGIN VERIFY SCRIPT-
sed -i 's/send_message(/send_without_ping(/g' $( git grep -l 'send_message(' )
-END VERIFY SCRIPT-
