b8710201fb guix: disable timezone tools & profiling in glibc (fanquake)
23b8a424fb guix: bump glibc 2.31 to 7b27c450c34563a28e634cccb399cd415e71ebfe (fanquake)
Pull request description:
An additional commit has been backported to the 2.31 branch:
https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/release/2.31/master.
Pass `--disable-timezone-tools`: removes `var/profiles/x86_64-linux-gnu/sbin/zdump`.
Pass `--disable-profile`: profiling is disabled by default, but make that explicit.
ACKs for top commit:
theuni:
utACK b8710201fb
hebasto:
ACK b8710201fb.
Tree-SHA512: 0d9a0e7451cc42384bbdd0b46c740c7aa964dc12e3f0376de586bf90e57799ebb04675892861cb38a53b5ca0e265061fa7111596cf1c94171303d0d048785ab4
be1a2e5dfb doc: Install `py3-zmq` port on OpenBSD for `interface_zmq.py` (Hennadii Stepanov)
Pull request description:
On OpenBSD, Python's `zmq` module is provided as a separate [port](https://www.ports.to/path/net/py-zmq,python3.html).
This PR updates the OpenBSD Build Guide to include this port, enabling the `interface_zmq.py` functional test.
Also updates the documented OpenBSD version.
ACKs for top commit:
theStack:
Tested ACK be1a2e5dfb
Tree-SHA512: 4d560385b94e8c7491aa19d2157d8a799617e08136601dc565a909d4c74e12582a1d273bc97ad7c2d0e57c5cf7377560ba02ef58c12f8991652322553740d2ba
e196190a28 cmake: Remove unused `BUILD_TESTING` variable from "dev-mode" preset (Hennadii Stepanov)
Pull request description:
On the master branch @ bb57017b29:
```
$ cmake -B build --preset dev-mode -DWITH_MULTIPROCESS=OFF
<snip>
-- Configuring done (12.0s)
-- Generating done (0.1s)
CMake Warning:
Manually-specified variables were not used by the project:
BUILD_TESTING
-- Build files have been written to: /home/hebasto/git/bitcoin/build
```
This PR resolves the issue.
The removed `BUILD_TESTING` variable is a part of the [`CTest`](https://cmake.org/cmake/help/latest/module/CTest.html) module, which we do not include in the project.
ACKs for top commit:
TheCharlatan:
ACK e196190a28
Tree-SHA512: 8110a0f5bdcdd0844ce7dd75160a61d8b3aff95e12da1ec4d55c56c82da41145736da0fad072adeb97551c99e46683a3493435c3bac7d8e4e62ea6086f60fb7a
Also, extend the pass2.json test to the maximum depth possible. The two
tests are now similar to fail45.json and pass4.json, except for the
string element in the inner-most array.
Also, sort.
Use character literals instead of integer hex values (i.e. `'\x5b','\x0a', ...` instead of `0x5b, 0x0a, ...`) for generated headers.
This avoids C++11 narrowing warnings in a more concise way than using explicit char casts.
Extra whitespace is also removed between elements for brevity.
fadd568931 fuzz: Fix misplaced SeedRand::ZEROS (MarcoFalke)
Pull request description:
After commit fae63bf130 this must be placed even before test_setup. This is nice, because it makes the usage consistently appear in the first line.
The change is moving a `SeedRandomForTest(SeedRand::ZEROS)` to happen earlier. This is fine, because it will either have no effect, or make the code more deterministic, because after commit fae63bf, no other re-seeding other than `ZEROS` can happen in fuzz tests.
ACKs for top commit:
marcofleon:
Re ACK fadd568931
brunoerg:
code review ACK fadd568931
hodlinator:
ACK fadd568931
Tree-SHA512: 54eadf19a1e850157a280fb252ece8797f37a9a50d3b0a01aa2c267bacbe8ef4ddea6cf3faadcbaa4ab9f53148edf08e3cee5dfb3eae928db582adf8373a5206
81cea5d4ee Ensure m_tip_block is never ZERO (Sjors Provoost)
e058544d0e Make m_tip_block an std::optional (Sjors Provoost)
Pull request description:
Suggested in https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1844244309
ACKs for top commit:
fjahr:
re-ACK 81cea5d4ee
tdb3:
code review re ACK 81cea5d4ee
l0rinc:
ACK 81cea5d4ee
Tree-SHA512: 31a75ba29e3d567bab32e4e7925a419d9d7a4d2d85ed1c1012116d8d22adc14d31d5b4ce5f6c499c994188dcd26a01cced05be74f94c892fc90ae17a6783a472
The std::span constructor requires std::ranges::borrowed_range, which
tries to protect against dangling references.
One way to disable the check is to specify the std::span's element type
as const in the constructor call.
Otherwise, a compile error will look like:
include/c++/span: note: candidate constructor not viable: no known conversion from 'std::vector<unsigned char>' to 'const span<unsigned char>' for 1st argument
| span(const span&) noexcept = default;
| ^ ~~~~~~~~~~~
...
include/c++/span: note: candidate template ignored: constraints not satisfied [with _Range = std::vector<unsigned char>]
| span(_Range&& __range)
| ^
include/c++/span: note: because 'std::vector<unsigned char>' does not satisfy 'borrowed_range'
| && (ranges::borrowed_range<_Range> || is_const_v<element_type>)
| ^
include/c++/bits/ranges_base.h: note: because 'std::vector<unsigned char>' does not satisfy '__maybe_borrowed_range'
| = range<_Tp> && __detail::__maybe_borrowed_range<_Tp>;
| ^
include/c++/bits/ranges_base.h: note: because 'is_lvalue_reference_v<std::vector<unsigned char> >' evaluated to false
| = is_lvalue_reference_v<_Tp>
| ^
include/c++/bits/ranges_base.h: note: and 'enable_borrowed_range<remove_cvref_t<vector<unsigned char, allocator<unsigned char> > > >' evaluated to false
| || enable_borrowed_range<remove_cvref_t<_Tp>>;
| ^
include/c++/span: note: and 'is_const_v<element_type>' evaluated to false
| && (ranges::borrowed_range<_Range> || is_const_v<element_type>)
| ^
This is possible and safe, because std::span can implicitly convert into
Span, if needed.
Changing this function is required, because std::span requires the
extent template parameter to be specified as well.
Instead of explicilty specifying them, just let the compiler derive the
template parameters correctly.
Otherwise, there would be a compile error later on:
src/wallet/test/db_tests.cpp:39:37: error: no matching function for call to ‘as_bytes<const char>(<brace-enclosed initializer list>)’
...
/usr/include/c++/11/span:420:5: note: candidate: ...
| as_bytes(span<_Type, _Extent> __sp) noexcept
| ^~~~~~~~
/usr/include/c++/11/span:420:5: note: template argument deduction/substitution failed:
src/wallet/test/db_tests.cpp:39:37: note: couldn’t deduce template parameter ‘_Extent’
| return std::as_bytes<const char>({str.data(), str.size()});
| ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
For Span, iterators are just raw data pointers. However, for std::span
they are not.
This change makes it explicit where data pointers are expected.
Otherwise, there could be a compile error later on:
No known conversion from 'iterator' (aka '__normal_iterator<const std::byte *, std::span<const std::byte, 18446744073709551615>>') to 'std::byte *'.
c991cea1a0 Remove processNewBlock() from mining interface (Sjors Provoost)
9a47852d88 Remove getTransactionsUpdated() from mining interface (Sjors Provoost)
bfc4e029d4 Remove testBlockValidity() from mining interface (Sjors Provoost)
Pull request description:
There are three methods in the mining interface that can be dropped. The Template Provider doesn't need them and other application should probably not use them either.
1. `processNewBlock()` was added in 7b4d3249ce, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ce.
Dropping it was suggested in https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2404460342
2. `getTransactionsUpdated()`: this is used in the implementation of #31003 `waitFeesChanged`. It's not very useful generically because the mempool updates very frequently.
3. `testBlockValidity()`: it might be useful for mining application to have a way to check the validity of a block template they modified, but the Stratum v2 Template Provider doesn't do that, and this method is a bit brittle (e.g. the block needs to build on the tip).
ACKs for top commit:
TheCharlatan:
Re-ACK c991cea1a0
ryanofsky:
Code review ACK c991cea1a0. Since last review, just rebased to avoid conflicts in surrounding code, and edited a commit message
tdb3:
code review ACK c991cea1a0
Tree-SHA512: 2138e54f920b26e01c068b24498c6a210c5c4358138dce0702ab58185d9ae148a18f04c97ac9f043646d40f8031618d80a718a176b1ce4779c237de6fb9c4a67
facb4d010c refactor: Move GuessVerificationProgress into ChainstateManager (MarcoFalke)
Pull request description:
Currently the function is standalone, which means any passed-in data like `TxData` or the block pointer needs to be taken from the `ChainstateManager` and passed in. This is currently verbose and may become even more verbose if the function is reworked in the future. As the function can not be called without a `ChainstateManager` in production code anyway, make it a member function on the class.
ACKs for top commit:
ryanofsky:
Code review ACK facb4d010c. Nice cleanup, that should make this code less awkward to work with
TheCharlatan:
ACK facb4d010c
danielabrozzoni:
reACK facb4d010c
Tree-SHA512: b17977e12cd7c6e308c47e6a1aa920acecd4442696e46d1f30bd7c201e9898ca2d581ff0bf2cc9f7334e146c1b0c50925adb849c8c17f65dcdf6877be1c5f776
processNewBlock was added in 7b4d3249ce, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ce.
getTransactionsUpdated() is only needed by the implementation of waitFeesChanged() (not yet part of the interface).
fa9e0489f5 refactor: Use immediate lambda to work around GCC bug 117966 (MarcoFalke)
Pull request description:
Currently the libstdc++ debug mode can only be used with version 11, or 15 (and later), due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117966
This seems restrictive.
Add a temporary workaround for now, which makes the global (temporary) `std::span` local to a lambda.
ACKs for top commit:
theuni:
utACK fa9e0489f5
hebasto:
ACK fa9e0489f5, tested on Ubuntu 24.10.
vasild:
ACK fa9e0489f5
Tree-SHA512: 0cc54f089f329592f7a92a6f938b7de46c92d5362615310748225a42789e858e871432721e3101271b00871d523af5fbaadba2f52433fe79e928b1d1253931f6
fae63bf130 fuzz: Clarify that only SeedRandomStateForTest(SeedRand::ZEROS) is allowed (MarcoFalke)
fa18acb457 fuzz: Abort when using global PRNG without re-seed (MarcoFalke)
fa7809aeab fuzz: Add missing SeedRandomStateForTest(SeedRand::ZEROS) (MarcoFalke)
Pull request description:
This is the first step toward improving fuzz stability and determinism (https://github.com/bitcoin/bitcoin/issues/29018).
A fuzz target using the global test-only PRNG will now abort if the seed is re-used across fuzz inputs.
Also, temporarily add `SeedRandomStateForTest(SeedRand::ZEROS)` to all affected fuzz targets. This may slow down the libfuzzer leak detector, but it will disable itself after some time, or it can be disabled explicitly with `-detect_leaks=0`.
In a follow-up, each affected fuzz target can be stripped of the global random use and a local `RandomMixin` (or similar) can be added instead.
(Can be tested by removing any one of the re-seed calls and observing a fuzz abort)
ACKs for top commit:
hodlinator:
ACK fae63bf130
dergoegge:
utACK fae63bf130
marcofleon:
Tested ACK fae63bf130
Tree-SHA512: 4a0db69af7f715408edf4f8b08b44f34ce12ee2c79d33b336ad19a6e6bd079c4ff7c971af0a3efa428213407c1171f4e2837ec6a2577086c2f94cd15618a0892
f86678156a Check leaves size maximum in MerkleComputation (Sjors Provoost)
4d57288246 refactor: use CTransactionRef in submitSolution (Sjors Provoost)
2e81791d90 Drop TransactionMerklePath default position arg (Sjors Provoost)
39d3b538e6 Rename merkle branch to path (Sjors Provoost)
Pull request description:
This PR implements the refactors suggested in https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2354931253.
ACKs for top commit:
tdb3:
code review re-ACK f86678156a
itornaza:
re ACK f86678156a
ryanofsky:
Code review ACK f86678156a only changes since last review are a whitespace change and adding an Assume statement to check for size_t -> uint32_t overflows
Tree-SHA512: 661b5d5d0e24b2269bf33ab1484e37c36e67b32a7796d77ca3b1856d3043378b081ad43c32a8638b46fa8c0de51c823fd9747dd9fc81f958f20d327bf330a47c
52fd1511a7 test: drop scriptPubKeyIn arg from CreateNewBlock (Sjors Provoost)
ff41b9e296 Drop script_pub_key arg from createNewBlock (Sjors Provoost)
7ab733ede4 rpc: rename coinbase_script to coinbase_output_script (Sjors Provoost)
Pull request description:
Providing a script for the coinbase transaction is only done in test code and for (unoptimized) CPU solo mining.
Production miners use the `getblocktemplate` RPC which omits the coinbase transaction entirely from its block template, leaving it to external (pool) software to construct it.
This commit removes the `script_pub_key argument` from `createNewBlock()` in the Mining interface.
A coinbase script can still be passed via `BlockCreateOptions` instead. Tests are modified to do so.
ACKs for top commit:
ryanofsky:
Code review ACK 52fd1511a7. No change since last review other than rebase
TheCharlatan:
Re-ACK 52fd1511a7
vasild:
ACK 52fd1511a7
Tree-SHA512: c4b3a53774d9a5dc90950e77f47a64dbb68f971baffbb9a0d8f59332ef8e52d0c039130c925bde73135b3d0e79e65d91d1df30dc4cff13f32d8a72e5c56669d8
fa0e30b93a fuzz: Fix test_runner error reporting (MarcoFalke)
Pull request description:
The error reporting is confusing, because right now it prints:
https://cirrus-ci.com/task/4846031060336640?logs=ci#L4931
```
...
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 411, in <module>
main()
File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 199, in main
run_once(
File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 376, in run_once
assert len(done_stat) == 1
^^^^^^^^^^^^^^^^^^^
AssertionError
```
This is harmless, but confusing.
Fix it by collecting statistics only when the program has not aborted. (Can be reviewed with `--color-moved=dimmed-zebra`)
Also, reword the error message to align it with error messages in other test_runners in this repo.
ACKs for top commit:
dergoegge:
utACK fa0e30b93a
brunoerg:
code review ACK fa0e30b93a
marcofleon:
Tested ACK fa0e30b93a. Prints out the error for the target that crashed. Much clearer than the current error message.
Tree-SHA512: 5e8d3fc0e4837b3264ff0c3cb322fe7fe2ec7af48d35e2a14f82080d03ace793963c3314611b0a170a38e200497d7ba703d9c35c9a7ed3272d93e43f0f0e4c2b
2b9ff4a66d build: use `-mbig-obj` for mingw-w64 Debug builds (fanquake)
Pull request description:
Windows cross builds using `-O0` (`-DCMAKE_BUILD_TYPE=Debug`) currently fail to compile, as some objects have too many sections. As a convenience, add `-mbig-obj` to our compile flags when using the `Debug` build type, so that if someone tries to build this way, it will work.
This would also be needed if we switched the depends flags to -O0. (maybe in #29796).
`-mbig-obj`
> On PE/COFF target this option forces the use of big object
> file format, which allows more than 32768 sections.
Closes#28109. Seems unlikely that we are going to break up the relevant object files, and the main issue is still the inclusion of Boost.
ACKs for top commit:
theuni:
utACK 2b9ff4a66d
hebasto:
ACK 2b9ff4a66d, tested in the following scenarios:
Tree-SHA512: 9ad36de172629a8b7e5371fe3cd75ac2f3c29856040569052cc59e42825eec9121e012dd2178e00b163173c98e78f79dd16b8cee2c93daa2ee0d7e99799325cd
a10bb400e8 depends: Fix CXXFLAGS on NetBSD (Hennadii Stepanov)
Pull request description:
This PR corrects an issue where `CXXFLAGS` were mistakenly overridden by `CFLAGS`. This behaviour was introduced in 7e7b3e42fa (from https://github.com/bitcoin/bitcoin/pull/22380).
On the master branch:
```
$ gmake --no-print-directory -C depends print-x86_64_netbsd_CXXFLAGS
x86_64_netbsd_CXXFLAGS=-pipe -std=c11
```
With this PR:
```
$ gmake --no-print-directory -C depends print-x86_64_netbsd_CXXFLAGS
x86_64_netbsd_CXXFLAGS=-pipe -std=c++20
```
ACKs for top commit:
theuni:
utACK a10bb400e8
Tree-SHA512: 0c842db2965ebb0a58693394715922810235d9e5f2a7416fe258eb252dbd68ec04f90a0f7948abe938caf94a9194cca7deb53a08335c4404cce3a40c5cb44944
46e207d329 cmake: Link `bitcoin_consensus` as a library (Hennadii Stepanov)
Pull request description:
The [`TARGET_OBJECTS`](https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#genex:TARGET_OBJECTS) generator expression was introduced in the staging branch when we aimed to build the libbitcoinconsensus shared library. However, `bitcoin_consensus` is a `STATIC` library, not an `OBJECT` library.
This change updates the build system to link `bitcoin_consensus` normally to `test_bitcoin`, resolving [linking issues](https://github.com/bitcoin/bitcoin/issues/31456#issuecomment-2538798107) when building with clang-cl.
ACKs for top commit:
TheCharlatan:
ACK 46e207d329
theuni:
utACK 46e207d329
Tree-SHA512: b5400be8e8350f80c9fc8b66c4a22032a51578e409eb1817309116fbf0bddeb5fcadd5fda685c98859730ee6cc904adb29d54207387732c8b574a1feb2be906f
3353d4a5e9 depends: Ignore prefix directory on OpenBSD (Hennadii Stepanov)
Pull request description:
On OpenBSD, the prefix directory is named as follows:
```
$ gmake --no-print-directory -C depends print-x86_64_openbsd_prefix
x86_64_openbsd_prefix=/home/hebasto/dev/bitcoin/depends/amd64-unknown-openbsd7.6
```
This name does not match any pattern in `depends/.gitignore`.
This PR resolves this issue.
ACKs for top commit:
tdb3:
ACK 3353d4a5e9
theuni:
utACK 3353d4a5e9
theStack:
Tested ACK 3353d4a5e9🐟
Tree-SHA512: 82dfff1af974aa43c21e5e5a4483256d5ab4efdf1a15073fb864e635eff52eb8414346cda125f097af59e3342ac031a52683529f4e64df9fc60c8783fcd85e74
To avoid future code changes from reintroducing the ambiguity fixed
by the previous commit, mark m_tip_block private and Assume that
it's not set to uint256::ZERO.
Belt and suspenders for future code changes.
Currently this function is only called from TransactionMerklePath() which sets leaves to the block transactions, so the Assume always holds.