fa7612f253 ci: Download script_assets_test.json for Windows CI (MarcoFalke)
7777a13306 test: Move Fetching-print to download_from_url util (MarcoFalke)
faf96286ce test: move-only download_from_url to stand-alone util file (MarcoFalke)
fa0cc1c5a4 test: [doc] Remove outdated comment (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/34670 by adding a new `download_script_assets` Python helper and calling it.
ACKs for top commit:
hebasto:
re-ACK fa7612f253.
janb84:
re ACK fa7612f253
hodlinator:
utACK fa7612f253
Tree-SHA512: 73c2cb3a31f231174566fb880b82de92734b1679ef000f8d793d774b7e5f5a7b8c7994a3998ca78821115bdc80c16aada69cf596e92c083cf9b9a95c7cee16ea
5e35a9069d interpreter: remove clang-tidy suppression (fanquake)
4089682f5c ci: use Clang 22 in tidy task (fanquake)
7ea076f996 tidy: remove deprecated header (fanquake)
eb17f29aa5 tidy: clang-tidy is required (fanquake)
Pull request description:
Changes needed for moving to Clang 22 in the tidy job.
ACKs for top commit:
maflcko:
lgtm ACK 5e35a9069d
hebasto:
ACK 5e35a9069d, I have reviewed the code and it looks OK.
Tree-SHA512: 9ca6e841f7480b8abd78d5621d08a5bf80c2ff4facd7a0d76038ac1771bbf3d37dc2df19fa27583679177e4618db6294e2f2bb2129d9c25a53338b49ed71aac2
```bash
15 | #warning The ClangTidyModuleRegistry.h header is deprecated and will be removed in LLVM 24. All of the symbols it used to define have been moved into ClangTidyModule.h.
| ^~~~~~~
[100%] Linking CXX shared module libbitcoin-tidy.so
```
98e8af4bb9 wallet: Drain validation interface queue after notifications disconnect (Ava Chow)
52992ebe1c interfaces: Add waitForNotifications() to call SyncWithValidationInterfaceQueue() (Ava Chow)
Pull request description:
When the wallet disconnects chain notifications, it is expecting no further notifications to execute, but this is not the case. This results in test failures such as in #34354. Instead of disconnecting the notifications and continuing shutdown, we should wait for the validation interface queue to be drained before the rest of wallet shutdown. This is achieved by adding an `interfaces::Chain::waitForNotifications()` function which calls `SyncWithValidationInterfaceQueue()`.
Fixes#34354
ACKs for top commit:
stickies-v:
utACK 98e8af4bb9
furszy:
ACK 98e8af4bb9
rkrux:
crACK 98e8af4bb9
sedited:
ACK 98e8af4bb9
Tree-SHA512: 263628556f740cb633d3970c22a0dfdb52a644bd1d0cd5a69c2970524edbb0e25d592cb39fc9bf1d0c281eebce09578526e2958dffee9026fc7473db35bd0dec
da7f70a532 test: use port 0 for I2P addresses in p2p_private_broadcast.py (Vasil Dimov)
a8ebcfd34c test: let connections happen in any order in p2p_private_broadcast.py (Vasil Dimov)
67696b207f net: extend log message to include attempted connection type (Vasil Dimov)
Pull request description:
If the following two events happen:
* (likely) the automatic 10 initial connections are not made to all
networks
* (unlikely) the network-specific logic kicks in almost immediately.
It is using exponential distribution with a mean of 5 minutes
(`rng.rand_exp_duration(EXTRA_NETWORK_PEER_INTERVAL)`).
So if both happen, then the 11th connection may not be the expected
private broadcast, but a network-specific connection.
Fix this by retrieving the connection type from
`destinations_factory()`. This is more flexible because it allows
connections to happen in any order and does not break if e.g. the 11th
connection is not the expected first private broadcast.
This also makes the test run faster:
before: 19-44 sec
now: 10-25 sec
because for example there is no need to wait for the initial 10
automatic outbound connections to be made in order to proceed.
Fixes: https://github.com/bitcoin/bitcoin/issues/34387
ACKs for top commit:
achow101:
ACK da7f70a532
andrewtoth:
ACK da7f70a532
mzumsande:
Code Review ACK da7f70a532
Tree-SHA512: 7c293e59c15c148a438e0119343b05eb278205640658c99336d4caf4848c5bae92b48e15f325fa616cbc9d5f394649abfa02406a76e802cffbd3d312a22a6885
fa48f8c865 test: Add missing timeout_factor to zmq socket (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/34189
Otherwise, the test may intermittently fail on slow CI systems that have `--timeout-factor=` properly set.
It can be tested by running `./bld-cmake/test/functional/interface_zmq.py --timeout-factor=10` with this diff:
```diff
diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index c7be6abc3a..b14cf2aee6 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -166,2 +166,3 @@ void ValidationSignals::SyncWithValidationInterfaceQueue()
LOG_EVENT(fmt, local_name, __VA_ARGS__); \
+ UninterruptibleSleep(45ms); \
event(); \
diff --git a/test/functional/interface_zmq.py b/test/functional/interface_zmq.py
index 6717007626..eee377daea 100755
--- a/test/functional/interface_zmq.py
+++ b/test/functional/interface_zmq.py
@@ -176,3 +176,3 @@ class ZMQTest (BitcoinTestFramework):
for sub in subscribers:
- sub.socket.set(zmq.RCVTIMEO, recv_timeout*1000)
+ sub.socket.set(zmq.RCVTIMEO, int(recv_timeout * 1000))
@@ -271,3 +271,3 @@ class ZMQTest (BitcoinTestFramework):
[(topic, address) for topic in ["hashblock", "hashtx"]],
- recv_timeout=2) # 2 second timeout to check end of notifications
+ recv_timeout=0.2) # 2 second timeout to check end of notifications
self.disconnect_nodes(0, 1)
```
Before this pull: Test fails with `zmq.error.Again: Resource temporarily unavailable`
After this pull: Test passes
ACKs for top commit:
l0rinc:
code review ACK fa48f8c865
achow101:
ACK fa48f8c865
Tree-SHA512: 5ff8bdd807ff4b644daa634bb7b469da3da3915f58afba63a90e662df99cbebc86636e34e2b1b313c8629773aef2a239fb3025226a84d2ec22f6ecd4cea666c4
bbc8f1e0a7 ipc mining: Prevent ``Assertion `m_node.chainman' failed`` errors on early startup (Ryan Ofsky)
a7cabf92e4 init refactor: Only initialize node.notifications one time (Ryan Ofsky)
c8e332cb33 init refactor: Remove node.init accesss in AppInitInterfaces (Ryan Ofsky)
Pull request description:
This fixes ``Assertion `m_node.chainman' failed`` errors first reported https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602551596 when IPC mining methods are called before ChainstateManager is loaded.
The fix works by making the `Init.makeMining` method wait until chainstate data is loaded. It's probably the simplest possible fix but other alternatives like moving the wait to `Mining.createNewBlock` were discussed in the thread https://github.com/bitcoin/bitcoin/pull/34661#discussion_r2848176298 and could be implemented later without changes to clients.
ACKs for top commit:
Sjors:
utACK bbc8f1e0a7
ismaelsadeeq:
ACK bbc8f1e0a7
achow101:
ACK bbc8f1e0a7
Tree-SHA512: 3e2e4e28ccff364b2303efd06ce337a229c28609076638500acb29559f716a15ad99409c6970ce9ad91776d53e3f9d959f1bbbd144ea9a4a2fb578ddbf2da267
When unloading a wallet, there may be unexecuted callbacks in the
validation interface queue that can still execute after we have
completed all of the other wallet shutdown tasks. Instead of letting
these run in the background, once the notifications are disconnected,
wait for the queue to drain before continuing with wallet shutdown.
fa36adeb71 ci: [refactor] Drop last use of pwsh (MarcoFalke)
fae31b1e2f ci: [refactor] Move github_import_vs_env to python script (MarcoFalke)
Pull request description:
The use of pwsh was a bit confusing and inconsistent around the exit code. See also https://github.com/bitcoin/bitcoin/pull/32672
I think it is fine to drop it and purely use Bash/Python.
This also moves a bit of code from the github yaml to the python script.
ACKs for top commit:
m3dwards:
Looks good! re-ACK fa36adeb71
hodlinator:
re-ACK fa36adeb71
Tree-SHA512: 78edffc60c58c476b0acca5224150169d154b0b818114844a04295af9ba19b7cdf1fb2afb738f6cafd6172f0f477d546018ebf95061eb5bd8bbb35e065a129d4
fab51e470e test: Move valgrind.supp to the other sanitizer_suppressions files (MarcoFalke)
fa9cf81d39 test: Add missing resolve() to valgrind.supp file (MarcoFalke)
Pull request description:
(see commit message for details)
Can be tested via:
```
./bld-cmake/test/functional/feature_framework_testshell.py --valgrind
```
Before:
```
bitcoind exited with status 1 during initialization. ==1735813== FATAL: can't open suppressions file "/b-c/b-c-2/bld-cmake/contrib/valgrind.supp"
```
After: (passes)
Also, move the suppression file to all the other suppression files.
ACKs for top commit:
frankomosh:
Re-ACK fab51e470e
Tree-SHA512: d3e3e130c0e2292ca3ab9e80d2ebec6b4edc7914280ed90fb4af8a65cd1c9edd19064d86375a6787b864925fe0e47bab2321f89b9be8d1613a0aebf4ec2443fe
fa18be2f2b test: Fix typo (MarcoFalke)
fac932698f ci: Set TEST_RUNNER_PORT_MIN in test-each after cirrus runner switch (MarcoFalke)
Pull request description:
This is needed after the recent switch to cirrus runners in the task in commit c8c9c1e617.
Otherwise, the CI will fail:
```
node1 stderr Error: Unable to bind to 127.0.0.1:12321 on this computer. Bitcoin Core is probably already running.
```
(https://github.com/bitcoin/bitcoin/actions/runs/22398358349/job/64837827234?pr=31723#step:9:2605)
Also, include a random second commit, so that the CI task is run in this pull.
ACKs for top commit:
l0rinc:
ACK fa18be2f2b
willcl-ark:
ACK fa18be2f2b
Tree-SHA512: 6b63f645bf62d3e951ca155cddf3dc562b7ce675ccae4f9179e2202679685b5c147844eb350bd219b173fe2bb976376d0caa073d3e827a48c13aa015f4745b2c
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.
fa6af85634 refactor: Use static_cast<decltype(...)> to suppress integer sanitizer warning (MarcoFalke)
fa692974ac util: Fix UB in SetStdinEcho when ENOTTY (MarcoFalke)
Pull request description:
The call to `tcgetattr` may fail with `ENOTTY`, leaving the struct possibly uninitialized (UB).
Fix this UB by returning early when `isatty` fails, or when `tcgetattr` fails. (Same for Windows)
This can be tested by a command that fails valgrind before the change and passes after:
```
echo 'pipe' | valgrind --quiet ./bld-cmake/bin/bitcoin-cli -stdinrpcpass uptime
ACKs for top commit:
achow101:
ACK fa6af85634
l0rinc:
lightly tested code review ACK fa6af85634
sedited:
ACK fa6af85634
Tree-SHA512: 76e2fbcb6c323b17736ee057dbd5e932b2e8cbb7d9fe4488c1dc7ab6ea928a3cde7e72ca0a63f8c8c78871ccb8b669263b712c0e1b530d88f2d45ea41f071201
45133c589a doc: clarify `git range-diff` add/delete output (Lőrinc)
Pull request description:
### Problem
Range diffs in git are useful after PR rebases, but it has an easy-to-misread failure mode: if it cannot match a commit between the old and new ranges, it will show the old commit as removed (<) and the new commit as added (>), without showing any patch contents for that commit.
It can look like there were no code changes when in reality the commit was just treated as unrelated and needs full re-review.
### Example
```bash
git fetch upstream ff338fdb53dd76338a57
git range-diff ff338fdb53a6...dd76338a57b9
```
This produced output like:
```patch
1: 0ca4295f2e = 93: 139aa4b27e bench: add on-disk `HaveInputs` benchmark
2: 4b32181dbb < -: ---------- test: add `HaveInputs` call-path unit tests
-: ---------- > 94: 277c57f0c5 test: add `HaveInputs` call-path unit tests
3: 8c57687f86 ! 95: c0c94ec986 dbwrapper: have `Read` and `Exists` reuse `ReadRaw`
@@ Metadata
## Commit message ##
dbwrapper: have `Read` and `Exists` reuse `ReadRaw`
- `ExistsImpl` was removed since it duplicates `CDBWrapper::ReadImpl` (except that it copies the resulting string on success, but that will be needed for caching anyway).
+ `ExistsImpl` was removed since it duplicates `CDBWrapper::ReadImpl`.
```
Even though the subject matches, there is no diff shown because the commits did not match - the reviewer could think that only the commit message was changed.
This should be treated as **unmatched** rather than **unchanged**.
If you expected a match, you can try increasing the search effort:
```bash
git range-diff --creation-factor=95 ff338fdb53a6...dd76338a57b9
```
which would show for example:
```patch
1: 0ca4295f2e = 93: 139aa4b27e bench: add on-disk `HaveInputs` benchmark
2: 4b32181dbb ! 94: 277c57f0c5 test: add `HaveInputs` call-path unit tests
@@ Commit message
The tests document that `HaveInputs()` consults the cache first and that a cache miss pulls from the backing view via `GetCoin()`.
+ Co-authored-by: Novo <eunovo9@gmail.com>
+
## src/test/coins_tests.cpp ##
@@ src/test/coins_tests.cpp: BOOST_FIXTURE_TEST_CASE(ccoins_flush_behavior, FlushTest)
}
}
-+BOOST_AUTO_TEST_CASE(ccoins_haveinputs_cache_miss_uses_base_getcoin)
++BOOST_AUTO_TEST_CASE(ccoins_cache_behavior)
```
### Fix
This PR updates `doc/productivity.md` to raise awareness and document this pitfall and mentions `--creation-factor` as a knob to try when the output seems unexpectedly empty.
ACKs for top commit:
maflcko:
review ACK 45133c589a🏦
Sjors:
ACK 45133c589a
rkrux:
crACK 45133c5
sedited:
ACK 45133c589a
Tree-SHA512: 52dcf6db51425a3ac9789627f80233fb1e3437f7a351acf4a761504d9917837aa1ff8c964605a842ee099fae9842946784f7603f9bffa7051429b2f04b7900be
8834e4e86c test: remove appveyor reference in comment (Max Edwards)
Pull request description:
Appveyor is not longer used however the test still requires to check for permissions including 666 as otherwise the tests fail on Windows
Fixes: #32576
ACKs for top commit:
maflcko:
lgtm ACK 8834e4e86c
hebasto:
ACK 8834e4e86c.
Tree-SHA512: 655b44e52da5e0c6c11c79bb4f92c701c6e0e66dce8d7791ccf1d64e4561fe4d1d5f37c1317bead89c88e4d7208a278925168b419482a6be17abf93d0ebc5dfa
408d5b12e8 test: include response body in non-JSON HTTP error msg (Matthew Zipkin)
9dc653b3b4 test: threadpool, add coverage for all Submit() errors (furszy)
ce2a984ee3 test: cleanup, use HasReason in threadpool_tests.cpp (l0rinc)
d9c6769d03 test: refactor, decouple HasReason from test framework machinery (furszy)
dbbb780af0 test: move and simplify BOOST_CHECK ostream helpers (Hodlinator)
3b7cbcafcb test: ensure Stop() thread helps drain the queue (seduless)
ca101a2315 test: coverage for queued tasks completion after interrupt (furszy)
bf2c607aaa threadpool: active-wait during shutdown (furszy)
e88d274430 test: add threadpool Start-Stop race coverage (furszy)
8cd4a4363f threadpool: guard against Start-Stop race (furszy)
9ff1e82e7d test: cleanup, block threads via semaphore instead of shared_future (l0rinc)
Pull request description:
A few follow-ups to #33689, includes:
1) `ThreadPool` active-wait during shutdown:
Instead of just waiting for workers to finish processing tasks, `Stop()` now helps them actively.
This speeds up the JSON-RPC and REST server shutdown, resulting in a faster node shutdown when many requests remain unhandled. This wasn't included in the original PR due to the behavior change this introduces.
2) Decouple `HasReason` from the unit test framework machinery
This avoids providing the entire unit test framework dependency to low-level tests that only require access to the `HasReason` utility class. Examples are: `reverselock_tests.cpp`, `sync_tests.cpp`, `util_check_tests.cpp`, `util_string_tests.cpp`, `script_parse_tests.cpp` and `threadpool_tests.cpp`. These tests no longer gain access to unnecessary components like the chainstate, node context, caches, etc. It includes l0rinc's `threadpool_tests.cpp` `HasReason` changes.
3) Include response body in non-JSON HTTP error messages
Straight from pinheadmz [comment](https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2783817192), it makes debugging CI issues easier.
ACKs for top commit:
maflcko:
review ACK 408d5b12e8🕗
achow101:
ACK 408d5b12e8
hodlinator:
re-ACK 408d5b12e8
Tree-SHA512: 57aa0ef96886f32bf95a0bd7f87c878d31c9df9e34cb96de615eee703ce0824b5cfdf8f5c9cd19a3594559994295b5810c38c94f5efd6291cbbd83a95473357a
0a8d303d66 test: fix test_limit_enforcement_package (Greg Sanders)
Pull request description:
The current test has a couple issues:
1) the parent_tx_good is regenerating the exact same transaction that is already in the cluster, so it's resulting in no replacements on submission
2) once fixed, the additional fee needs to be allocated to the parent transaction in the package, not the child. If the RBF fees are allocated to the child, this triggers the package RBF logic, which requires no in-mempool ancestors to be present.
Fix the bug and add a few assertions to protect against regressions.
ACKs for top commit:
bensig:
ACK 0a8d303d66
achow101:
ACK 0a8d303d66
sipa:
ACK 0a8d303d66
Tree-SHA512: 0ba184d82edc5a502e9119a6876e80c4564c876fa51ee39293d47bd30c18bf3ded50fbd2f6f2a3394784fad05d8f6370a90682068b30358b077280abd2477252
17a079c2fb ci: fix vcpkg tools cache key collision between windows matrix jobs (will)
Pull request description:
The standard and fuzz matrix jobs share the same github.job value
(windows-native-dll), so both try to save the vcpkg tools cache with the
same key.
Since the tools are identical across build types, let them share a
single cache entry by restricting the save to the standard job only.
ACKs for top commit:
maflcko:
lgtm ACK 17a079c2fb
hebasto:
ACK 17a079c2fb, this should fix issues like https://github.com/bitcoin/bitcoin/pull/34559#issuecomment-3897330307.
Tree-SHA512: 2e83846bfc88947b4bc321baa23563e0c093cd4f55f11f8105c2ecf867b78028aa71aa4780f928103b7a9f1f2e3cf72dbb072f05e7925bc1d00069234acf23c9
The standard and fuzz matrix jobs share the same github.job value
(windows-native-dll), so both try to save the vcpkg tools cache with the
same key.
Since the tools are identical across build types, let them share a
single cache entry by restricting the save to the standard job only.
Avoid providing the entire unit test framework dependency to tests that only
require access to the HasReason utility class.
E.g. reverselock_tests.cpp, sync_tests.cpp, util_check_tests.cpp, util_string_tests.cpp,
and script_parse_tests.cpp only require access to HasReason and nothing else.
Move the operator<< overloads used by BOOST_CHECK_* out of the
unit test machinery test/setup_common, into test/util/common.h.
And replace the individual per-type ToString() overloads with
a single concept-constrained template that covers any type
exposing a ToString() method. This is important to not add
uint256.h and transaction_identifier.h dependencies to the
shared test/util/common.h file.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Instead of waiting for the workers to finish processing tasks, help
them actively inside Stop().
This speeds up the JSON-RPC and REST server shutdown procedure,
and results in a faster node shutdown when many requests remain unhandled
e5f0613503 net processing: Check if we are in ibd before processing block for txdownloadman (sedited)
ce8b692897 Add functional test exercising tx downloadman recently confirmed filter (Lőrinc)
Pull request description:
Calculating the rolling bloom filters for the txorphanage takes some CPU time from the scheduler thread. This can be observed for example in [this flamegraph](https://bitcoin-dev-tools.github.io/benchcoin/results/pr-172/20066462508/mainnet-default-instrumented-base-flamegraph.svg?x=920203898521&y=780), where handling the filter takes about 2.6% of total time (and most of the scheduler thread's time).
During ibd the entries in the tx download bloom filter are just continuously rolled over and aren't consumed, since no mempool entries are created by incoming transactions from peers during ibd. The mempool does accept transactions via RPC, or the wallet at the time, however these don't interact with the orphanage and the txdownloadman, because adding anything to those is guarded by IsInitialBlockDownload() checks as well.
We're usually latching ibd to false a few blocks before catching up to the tip, so this should also not significantly degrade the performance of the filter once fully caught up.
ACKs for top commit:
l0rinc:
ACK e5f0613503
instagibbs:
ACK e5f0613503
fjahr:
Code review ACK e5f0613503
Tree-SHA512: d667e677f5723c438cdf5b34f0f9c1ade7cc1b2e98530c23f14384514daa38217c4e7c3b756194b6831b590a487449c4514b52bf0fb461ae8083061722824270
The curl requirement has long been removed, when windows support was
added to the script. Also, the build support has long been dropped,
since the project switched from autotools to cmake.
33fbaed310 policy: don't CheckEphemeralSpends on reorg (Greg Sanders)
Pull request description:
Similar reasoning to https://github.com/bitcoin/bitcoin/pull/33504
During a deeper reorg it's possible that a long sequence of dust-having transactions that are connected in a linear fashion. On reorg, this could cause each subsequent "generation" to be rejected. These rejected transactions may contain a large amount of competitive fees via normal means.
PreCheck based `PreCheckEphemeralSpends` is left in place because we wouldn't have relayed them prior to the reorg.
ACKs for top commit:
darosior:
re-ACK 33fbaed310
ismaelsadeeq:
reACK 33fbaed310
sedited:
ACK 33fbaed310
Tree-SHA512: cf0a6945066e9f5f8f9a847394c2c1225facf475a8aa4bc811b436513eff79c0a720d4ad21ba6b0f1cc4dfdd61cf46acb148333ac592b2ee252953732326ad1d
