459807d566 test: remove strict restrictions on rpc_deprecated (Pol Espinasa)
Pull request description:
Removed the wallet restrictions for `rpc_deprecated.py` and added specific test case for the current deprecated rpc.
`skip_test_if_missing_module` will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related. This PR ensures that other tests not related to wallet can be ran and only this specific test will be skipped if there's no wallet
For more context check https://github.com/bitcoin/bitcoin/pull/31278#discussion_r2011661090
ACKs for top commit:
maflcko:
lgtm ACK 459807d566
rkrux:
ACK 459807d
Tree-SHA512: 922b0fafe8fb5bd88a677ce8be5c3fe2fdd4d0aadcd32cc11738a714cd6f765f07e7e7158c829f8338db0d46a15c030437a1ea09a3187c072bebebb4ca53ad85
f974359e21 test: Add encodable PUSHDATA1 examples to feature_taproot (Greg Sanders)
Pull request description:
Inspired by discussion in https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743492906 I made an example adding coverage I think is missing, with some extra commentary that might help future contributors (including myself when I forget how it works again).
Open for suggestions how we can make it more welcoming beyond this.
cc darosior EthanHeilman sipa
ACKs for top commit:
janb84:
Re-ACK [f974359](f974359e21)
rkrux:
ACK f974359e21
Tree-SHA512: 7544d41c39c13d245a8a33522e53f22b4dd7593c069631978303e5a349cd12cf9d45bed648c391618c4732831232c4b82b8de2bf6cba5bf5e1232501db926122
58914ab459 fuzz: assert min diff between FeeFrac and CFeeRate (Pieter Wuille)
0c6bcfd8f7 feefrac: support both rounding up and down for Evaluate (Pieter Wuille)
ecf956ec9d feefrac: add support for evaluating at given size (Pieter Wuille)
7963aecead feefrac: add helper functions for 96-bit division (Pieter Wuille)
800c0dea9a feefrac: rework comments around Mul/MulFallback (Pieter Wuille)
fcfe008db2 feefrac fuzz: use arith_uint256 instead of ad-hoc multiply (Pieter Wuille)
46ff4220bf arith_uint256: modernize comparison operators (Pieter Wuille)
Pull request description:
The `FeeFrac` type represents a fraction, intended to be used for sats/vbyte or sats/WU. This PR adds functionality to evaluate that feerate for a given size, in order to obtain the fee it corresponds with (rounding down, or rounding up).
The motivation here is being able to do accurate feerate evaluations in cluster mempool block building heuristics (where rounding down is needed), but in principle this makes it possible to use `FeeFrac` as a more accurate replacement for `CFeeRate` (where for feerate estimation rounding up is desirable). Because of this, both rounding modes are implemented.
Unit tests are included for known-correct values, plus a fuzz test that verifies the result using `arith_uint256`.
ACKs for top commit:
l0rinc:
ACK 58914ab459
ismaelsadeeq:
reACK 58914ab459
glozow:
light code review ACK 58914ab459
Tree-SHA512: 362b88454bf355cae1f12d6430b1bb9ab66824140e12b27db7c48385f1e8db936da7d0694fb5aad2a00eb9e5fe3083a3a2c0cc40b2a68e2d37e07b3481d4eeae
Rather than use an ad-hoc reimplementation of wide multiplication inside the
fuzz test, reuse arith_uint256, which already has this. It's larger than what we
need here, but performance isn't a concern in this test, and it does what we need.
Since C++20, operator!= is implicitly defaulted using operator==, and
operator<, operator<=, operator>, and operator>= are defaulted using
operator<=>, so it suffices to just provide these two.
fadf8f078e test: Remove confusing and failing system time test (MarcoFalke)
Pull request description:
This was just added as a sanity check in fa013664ae by myself.
However, the test uses system time, so it may obviously (albeit rarely) fail.
Fix it by removing it.
Can be tested by running two bash loops at the same time:
`while ( ./bld-cmake/bin/test_bitcoin -t util_tests/util_time_GetTime ) ; do true ; done`
`while ( date -s "$(date -d 'now + 0.015 seconds' '+%Y-%m-%d %H:%M:%S.%3N')" && date -s "$(date -d 'now - 0.015 seconds' '+%Y-%m-%d %H:%M:%S.%3N')" ) ; do true ; done`
Eventually, it will fail:
```
test/util_tests.cpp(595): error: in "util_tests/util_time_GetTime": check ms_0 < GetTime<std::chrono::milliseconds>() has failed
test/util_tests.cpp(596): error: in "util_tests/util_time_GetTime": check us_0 < GetTime<std::chrono::microseconds>() has failed
*** 2 failures are detected in the test module "Bitcoin Core Test Suite"
ACKs for top commit:
janb84:
ACK [fadf8f0](fadf8f078e)
mabu44:
Tested ACK fadf8f078e
hebasto:
ACK fadf8f078e, tested on Ubuntu 24.10.
Tree-SHA512: fc468546f46a12804802df4f0e64d2898aca3db4df69602e5919ac31646c2fcb1e75b614fc2d1a3959c3db10fb0e315da5886d348b41589dba7cb43e618444a1
fa10a1ded5 ci: Use GITHUB_BASE_REF over hard-coded master (MarcoFalke)
fa0d0be05c ci: Merge master in test-each-commit task (take 2) (MarcoFalke)
Pull request description:
Calling the script `.github/ci-test-each-commit-exec.sh`, which merges `master`, obviously doesn't work, if the script itself is missing.
Fix it by a move-only to first merge `master` and then call the script.
ACKs for top commit:
l0rinc:
Code review ACK fa10a1ded5
sipa:
ACK fa10a1ded5, this fixed the CI issue in #31444.
Tree-SHA512: bcab2b03cb46d456e29f8d4237312a4525b9acd819578b26b4d5670ca14e075cf473b77b235b3063e06422325b627587f12dec7b4fbba134086d162c67dc81b3
* Run git config earlier and only once
* Run git merge in the yaml, before calling the bash script
* Run git reset in the yaml as well, for symmetry
* Replace "git merge --abort" with "git reset --hard", because it does
not fail when already up to date and no merge was started.
c5a7ffd1e8 preserve llvm profile env (Prabhat Verma)
Pull request description:
While generating `profraw` for fuzz tests using steps in [PR 32206](https://github.com/bitcoin/bitcoin/pull/32206) , the profraw was not being built at the desired location and only one `default.profraw` was being created which was being overwritten for multiple fuzz targets. This PR fixes that.
ACKs for top commit:
maflcko:
lgtm ACK c5a7ffd1e8
mabu44:
ACK c5a7ffd1e8
Tree-SHA512: 11f74caa8cba6f841aa899a5e294f658aed1b6a3d4cf68992609ea99fadb4a092b2350ffacea5c2d5eb377eb10082de018f27a1d6486a72460cb3905aaa15664
faa807bdf8 ci: Merge master in test-each-commit task (MarcoFalke)
Pull request description:
The `test-each-commit` task will often fail, when the CI config yaml is updated along with code changes.
This is because, GitHub seems to be merging the CI config on a fresh pull with the current target branch (`master`). However, the code changes are not.
A tedious workaround would be for every developer to rebase on every intermittent (https://github.com/bitcoin/bitcoin/issues/31946#issuecomment-2740911853) and non-intermittent CI issue.
However, fix this instead by merging with `master`.
ACKs for top commit:
laanwj:
ACK faa807bdf8
hebasto:
ACK faa807bdf8.
Tree-SHA512: 4849bd558dc6cdc7d86b95164ccee32ab7c08c9b7d31cf8ec5c8e9a2251fc819630f8fa9b929ed39e8e033c67bb006f0beb33e0de216e1224680be88c5fa0161
2e751f559a doc: Amend Qt 6 dependency packages for Ubuntu (Hennadii Stepanov)
Pull request description:
On older systems, such as Ubuntu 22.04, `qt6-tools-dev-tools` and `libgl-dev` are not treated as dependencies of `qt6-tools-dev` and `qt6-base-dev`, respectively. This PR explicitly lists them in the installation documentation.
Fixes https://github.com/bitcoin/bitcoin/issues/32210.
ACKs for top commit:
maflcko:
lgtm ACK 2e751f559a
laanwj:
Code review ACK 2e751f559a
Tree-SHA512: a6997c74c83789cb5fe5b97a719b8ff6e2180d5f6ae5502ccccfce3a22394d25eef05204ecda0a6deb368de77975e2a1da89b5749eff01a979f2f60843efebff
On older systems, such as Ubuntu 22.04, `qt6-tools-dev-tools` and
`libgl-dev` are not treated as dependencies of `qt6-tools-dev` and
`qt6-base-dev`, respectively. This change explicitly lists them in the
installation documentation.
a40bd374aa Get*Union: disallow nulltpr Refs (Greg Sanders)
57433502e6 CountDistinctClusters: nullptrs disallowed (Greg Sanders)
8bca0d325a TxGraphImpl::Compact: m_main_clusterset.m_removed is always empty (Greg Sanders)
2c5cf987e9 TxGraphImpl::PullIn: only allowed when staging exists (Greg Sanders)
Pull request description:
Was looking at my local coverage report, and noticed a few spots that will not or cannot be hit.
CountDistinctClusters, GetAncestorsUnion, and GetDescendantsUnion accept nullptrs, but the test harness never employs them. Disallow them.
We never call PullIn whenever there isn't staging, so just enforce that invariant via assertion.
Remaining places that are not covered:
1) Relinearize: Currently we seem to always start with a cold (not known to be optimal) cluster, and after one attempt at linearization result into something optimal. This means we never shortcircuit, nor run PostLinearization, nor store the quality as ACCEPTABLE. Reducing iterations causes these lines to be hit. sipa says he will take this on as varying the amount of iterations was meant to be done eventually anyways.
2) We never do a move assignment operator when the lvalue already has a `m_graph` (so we never call UnlinkRef) 3358b1d105/src/txgraph.cpp (L2097)
3) We never use the move constructor: 3358b1d105/src/txgraph.cpp (L2108)
ACKs for top commit:
sipa:
utACK a40bd374aa
glozow:
utACK a40bd374aa
Tree-SHA512: ca88297222e80e0d590889698899f892b9335cfa587a76a6c6ca62c8d846f208b6b0b9a9b1829bafabdb929a1a0c3a75f23edf7dd2b4f5e2dad0235e5bc68ba3
1. Do not set `C{PLUS}_INCLUDE_PATH` variables
The build system for Qt 6 differs entirely from that of Qt 5. Building a
set of native Qt 6 tools now forms a separate step when cross-compiling.
Under these new circumstances, the `C{PLUS}_INCLUDE_PATH` environment
variables may alter the default include directories for both native and
cross compilers.
Previously, we explicitly unset these variables when invoking clang for
cross-compiling; however, that approach proved suboptimal (see #30451).
This change sets the native toolchain for dependencies explicitly,
rather than relying on the `C{PLUS}_INCLUDE_PATH` environment variables.
Additionally, it facilitates the transition towards using clang for
building native tools when cross-compiling for macOS.
2. Add `ninja` package.
3. Adjust allowed symbol lists.
b96f1a696a add clang/llvm based coverage report generation (Prabhat Verma)
Pull request description:
Followed up from the [comment](https://github.com/bitcoin/bitcoin/issues/31927#issuecomment-2674522975) on the issue [#31927](https://github.com/bitcoin/bitcoin/issues/31927) , issues have been observed building coverage reports with `gcov` in MacOs and NixOs. This PR adds the steps to generate a coverage report based on the default llvm/clang tooling.
ACKs for top commit:
Crypt-iQ:
tACK b96f1a696a
hodlinator:
re-ACK b96f1a696a
janb84:
Re ACK [b96f1a6](b96f1a696a)
Tree-SHA512: bc54f170e84bb76b3eba7285bd49f051c0b99b784d583a550d8e51511497bcc4df8964bbe3991777648d2f829809db8eabb0cbf0d25f9da5e49e1cfc62f6d8d0
fa51310121 contrib: Warn about using libFuzzer for coverage check (MarcoFalke)
fa17cdb191 test: Avoid script check worker threads while fuzzing (MarcoFalke)
fa900bb2dc contrib: Only print fuzz output on failure (MarcoFalke)
fa82fe2c73 contrib: Use -Xdemangler=llvm-cxxfilt in deterministic-*-coverage (MarcoFalke)
fa7e931130 contrib: Add optional parallelism to deterministic-fuzz-coverage (MarcoFalke)
Pull request description:
This should make the `partially_downloaded_block` fuzz target even more deterministic.
Follow-up to https://github.com/bitcoin/bitcoin/pull/31841. Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018.
This bundles several changes:
* First, speed up the `deterministic-fuzz-coverage` helper by introducing parallelism.
* Then, a fix to remove spawned test threads or spawn them deterministically. (While testing this, high parallelism and thread contention may be needed)
### Testing
It can be tested via (setting 32 parallel threads):
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ partially_downloaded_block 32
```
Locally, on a failure, the output would look like:
```diff
....
- 150| 0| m_worker_threads.emplace_back([this, n]() {
- 151| 0| util::ThreadRename(strprintf("scriptch.%i", n));
+ 150| 1| m_worker_threads.emplace_back([this, n]() {
+ 151| 1| util::ThreadRename(strprintf("scriptch.%i", n));
...
```
This excerpt likely indicates that the script threads were started after the fuzz init function returned.
Similarly, for the scheduler thread, it would look like:
```diff
...
227| 0| m_node.scheduler = std::make_unique<CScheduler>();
- 228| 1| m_node.scheduler->m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { m_node.scheduler->serviceQueue(); });
+ 228| 0| m_node.scheduler->m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { m_node.scheduler->serviceQueue(); });
229| 0| m_node.validation_signals =
...
```
ACKs for top commit:
Prabhat1308:
re-ACK [`fa51310`](fa51310121)
hodlinator:
re-ACK fa51310121
janb84:
Re-ACK [fa51310](fa51310121)
Tree-SHA512: 1a935eb19da98c7c3810b8bcc5287e5649ffb55bf50ab78c414a424fef8e703839291bb24040a552c49274a4a0292910a00359bdff72fa29a4f53ad36d7a8720
28dc118001 fuzz: wallet: fix crypter target (brunoerg)
Pull request description:
The crypter target has an issue, it's calling `DecryptKey` with a random secret and a random public key that will unlikely be related to the key used to encrypt, so it won't have any effect. This PR changes fixes it and also removes the `DecryptSecret` call since this function is already (and only) called within `DecryptKey`.
ACKs for top commit:
maflcko:
lgtm ACK 28dc118001🥊
Tree-SHA512: e96b7d33879bf06eeec0726e74e8e0d7020997659bf97dfca5d7c1a7ba65c4d93c78e666b97eebde110564cef2eefc7209d3e3586e4658145827b14d1b01dfc9
fa69c42fdf refactor: Remove spurious virtual from final ~CZMQNotificationInterface (MarcoFalke)
Pull request description:
`virtual` does not make sense here, because:
* The class is `final`, thus the destructor isn't overridden in a derived class
* The destructor also isn't overriding the destructor of the base, clarified in commit 2b3ea39de4
* Clang 21 may warn about this
```
src/zmq/zmqnotificationinterface.h:25:13: error: virtual method '~CZMQNotificationInterface' is inside a 'final' class and can never be overridden [-Werror,-Wunnecessary-virtual-specifier]
25 | virtual ~CZMQNotificationInterface();
| ^
```
Fix all issues by removing it.
ACKs for top commit:
davidgumberg:
crACK fa69c42fdf
janb84:
ACK [fa69c42](fa69c42fdf)
TheCharlatan:
ACK fa69c42fdf
Tree-SHA512: 26ea977f31fe24c116d68dea6c583de7c6fc480877e1baefcde11db4ac191e352027d492ee6ad69a60fe4ff537e0841c638b3a3e81356d9e00c60030845fc96e
4774a0c923 test: fix spelling in Python code comment (John Bampton)
Pull request description:
Fixed a couple of typos
Top commit has no ACKs.
Tree-SHA512: 5334995672b2c7d4a9cb916f71dff6a2ce13dc7ced6bbc30ddb0fe8e0ae0b4094b675b3dfced1ffc1b92e3a33ee22df07af3032b8c2928f27051b6376dca3361
4a679936bb ci, windows: Do not exclude `wallet_migration.py` in command line (Hennadii Stepanov)
Pull request description:
This PR amends the recently merged https://github.com/bitcoin/bitcoin/pull/31176 to resolve a silent merge conflict with the previously merged https://github.com/bitcoin/bitcoin/pull/31248.
Since https://github.com/bitcoin/bitcoin/pull/31248, it is no longer necessary to use `--exclude wallet_migration.py`, as the test is skipped due to not using previous releases.
The `wallet_migration.py` test itself still needs to be fixed for Windows by someone who will work on https://github.com/bitcoin/bitcoin/issues/32192.
ACKs for top commit:
davidgumberg:
crACK 4a679936bb
Tree-SHA512: f42428016958cdaccb509cc49341e726eaf1314d85989a7b49888f3862dc4ea0c2988a4792ae62dd925302d0073906397801c8dd2fb06c23381d7cad38730249
Removed the wallet restrictions for rpc_deprecated.py and added specific test case for the current deprecated rpc.
skip_test_if_missing_module will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related.
