The mutex (required by TestBlockValidity) must be held after creating
the block, until TestBlockValidity is called. Otherwise, it is possible
that the chain advances in the meantime and leads to a crash in
TestBlockValidity:
Assertion failed: pindexPrev && pindexPrev == chainstate.m_chain.Tip() (validation.cpp: TestBlockValidity: 4338)
The diff can be reviewed with the git options
--ignore-all-space --function-context
Setting a large `-dbcache` size postpones the index writes until the coins cache size exceeds the specified limit.
This causes the final flush after manual termination to seemingly hang forever (e.g. tens of minutes for 20 GiB);
Now that the `dbcache` upper cap has been lifted, this will become even more apparent, so a warning will be shown when large UTXO sets are flushed (currently >1 GiB), such as:
> 2024-12-18T18:25:03Z Flushed fee estimates to fee_estimates.dat.
> 2024-12-18T18:25:03Z [warning] Flushing large (1 GiB) UTXO set to disk, it may take several minutes
> 2024-12-18T18:25:09Z Shutdown: done
Note that the related BCLog::BENCH units were also converted to `KiB` from `kB` to unify the bases.
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
By setting DANGER_DOCKER_BUILD_CACHE_HOST_DIR, the task-specific
docker images built during the CI run can be cached. This allows,
for example, ephemeral CI runners to reuse the docker images (or
layers of it) from earlier runs, by persisting the image cache
before the ephemeral CI runner is shut down. The cache keyed by
`CONTAINER_NAME`.
As --cache-to doesn't remove old cache files, the existing cache
is removed after a successful `docker build` and the newly cached
image is moved to it's location to avoid the cache from growing
indefinitly with old, unused layers.
When --cache-from doesn't find the directory, the cached version is
a cache-miss, or the cache can't be imported for whatever other reason,
it warns and `docker build` continues by building the docker image.
This feature is opt-in. The documentation for the cache type=local
can be found https://docs.docker.com/build/cache/backends/local/
This replaces https://github.com/bitcoin/bitcoin/pull/31377
b8710201fbd01bf2cba3c3b1fd312316b1ae22af guix: disable timezone tools & profiling in glibc (fanquake)
23b8a424fb06d2e669d147b4f58e640b6eab646a 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 b8710201fbd01bf2cba3c3b1fd312316b1ae22af
hebasto:
ACK b8710201fbd01bf2cba3c3b1fd312316b1ae22af.
Tree-SHA512: 0d9a0e7451cc42384bbdd0b46c740c7aa964dc12e3f0376de586bf90e57799ebb04675892861cb38a53b5ca0e265061fa7111596cf1c94171303d0d048785ab4
be1a2e5dfbdf4b3799f07e48c41b09b54b72f9b6 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 be1a2e5dfbdf4b3799f07e48c41b09b54b72f9b6
Tree-SHA512: 4d560385b94e8c7491aa19d2157d8a799617e08136601dc565a909d4c74e12582a1d273bc97ad7c2d0e57c5cf7377560ba02ef58c12f8991652322553740d2ba
e196190a284fc6ffd21a39cfae957fdd8b4657b6 cmake: Remove unused `BUILD_TESTING` variable from "dev-mode" preset (Hennadii Stepanov)
Pull request description:
On the master branch @ bb57017b2945d5e0bbd95c7f1a9369a8ab7c6fcd:
```
$ 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 e196190a284fc6ffd21a39cfae957fdd8b4657b6
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.
fadd568931a2d21e0f80e1efaf2281f5164fa20e fuzz: Fix misplaced SeedRand::ZEROS (MarcoFalke)
Pull request description:
After commit fae63bf13033adec80c7e6d73144a21ea3cfbc6d 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 fadd568931a2d21e0f80e1efaf2281f5164fa20e
brunoerg:
code review ACK fadd568931a2d21e0f80e1efaf2281f5164fa20e
hodlinator:
ACK fadd568931a2d21e0f80e1efaf2281f5164fa20e
Tree-SHA512: 54eadf19a1e850157a280fb252ece8797f37a9a50d3b0a01aa2c267bacbe8ef4ddea6cf3faadcbaa4ab9f53148edf08e3cee5dfb3eae928db582adf8373a5206
81cea5d4ee0e226f7c7145072de85829f4166ec9 Ensure m_tip_block is never ZERO (Sjors Provoost)
e058544d0e83aed691903d13d7e5775beb7e678f 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 81cea5d4ee0e226f7c7145072de85829f4166ec9
tdb3:
code review re ACK 81cea5d4ee0e226f7c7145072de85829f4166ec9
l0rinc:
ACK 81cea5d4ee0e226f7c7145072de85829f4166ec9
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 *'.
c991cea1a0c3ea99dc3e3919789ddf32a338a59d Remove processNewBlock() from mining interface (Sjors Provoost)
9a47852d88cf79a94ea2b7884f2dfa319bf37e4d Remove getTransactionsUpdated() from mining interface (Sjors Provoost)
bfc4e029d41ec3052d68f174565802016cb05d41 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 7b4d3249ced93ec5986500e43b324005ed89502f, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ced93ec5986500e43b324005ed89502f.
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 c991cea1a0c3ea99dc3e3919789ddf32a338a59d
ryanofsky:
Code review ACK c991cea1a0c3ea99dc3e3919789ddf32a338a59d. Since last review, just rebased to avoid conflicts in surrounding code, and edited a commit message
tdb3:
code review ACK c991cea1a0c3ea99dc3e3919789ddf32a338a59d
Tree-SHA512: 2138e54f920b26e01c068b24498c6a210c5c4358138dce0702ab58185d9ae148a18f04c97ac9f043646d40f8031618d80a718a176b1ce4779c237de6fb9c4a67
facb4d010ca5c898756bedee46069509900ebea0 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 facb4d010ca5c898756bedee46069509900ebea0. Nice cleanup, that should make this code less awkward to work with
TheCharlatan:
ACK facb4d010ca5c898756bedee46069509900ebea0
danielabrozzoni:
reACK facb4d010ca5c898756bedee46069509900ebea0
Tree-SHA512: b17977e12cd7c6e308c47e6a1aa920acecd4442696e46d1f30bd7c201e9898ca2d581ff0bf2cc9f7334e146c1b0c50925adb849c8c17f65dcdf6877be1c5f776
processNewBlock was added in 7b4d3249ced93ec5986500e43b324005ed89502f, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ced93ec5986500e43b324005ed89502f.
getTransactionsUpdated() is only needed by the implementation of waitFeesChanged() (not yet part of the interface).
It's very low level and not used by the proposed Template Provider.
This method was introduced in d8a3496b5ad27bea4c79ea0344f595cc1b95f0d3
and a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed.
fa9e0489f57968945d54ef56b275f51540f3e5e4 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 fa9e0489f57968945d54ef56b275f51540f3e5e4
hebasto:
ACK fa9e0489f57968945d54ef56b275f51540f3e5e4, tested on Ubuntu 24.10.
vasild:
ACK fa9e0489f57968945d54ef56b275f51540f3e5e4
Tree-SHA512: 0cc54f089f329592f7a92a6f938b7de46c92d5362615310748225a42789e858e871432721e3101271b00871d523af5fbaadba2f52433fe79e928b1d1253931f6
fae63bf13033adec80c7e6d73144a21ea3cfbc6d fuzz: Clarify that only SeedRandomStateForTest(SeedRand::ZEROS) is allowed (MarcoFalke)
fa18acb457e91cc0fa6b3640b6b55c6bc61572ee fuzz: Abort when using global PRNG without re-seed (MarcoFalke)
fa7809aeab838752af94c52977936a8c6555d315 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 fae63bf13033adec80c7e6d73144a21ea3cfbc6d
dergoegge:
utACK fae63bf13033adec80c7e6d73144a21ea3cfbc6d
marcofleon:
Tested ACK fae63bf13033adec80c7e6d73144a21ea3cfbc6d
Tree-SHA512: 4a0db69af7f715408edf4f8b08b44f34ce12ee2c79d33b336ad19a6e6bd079c4ff7c971af0a3efa428213407c1171f4e2837ec6a2577086c2f94cd15618a0892
f86678156a3d3ce6488b383a2d6d91d28fcd2b73 Check leaves size maximum in MerkleComputation (Sjors Provoost)
4d572882463b20818fcfbd0a2f6fa6c0168e4e4a refactor: use CTransactionRef in submitSolution (Sjors Provoost)
2e81791d907288c174aa05dc1b3816e6d988127c Drop TransactionMerklePath default position arg (Sjors Provoost)
39d3b538e6a2af8db85077e958970cdcd0ee7f7d 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 f86678156a3d3ce6488b383a2d6d91d28fcd2b73
itornaza:
re ACK f86678156a3d3ce6488b383a2d6d91d28fcd2b73
ryanofsky:
Code review ACK f86678156a3d3ce6488b383a2d6d91d28fcd2b73 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
52fd1511a774d1ff9e747b2ce88aa1fcb778ced8 test: drop scriptPubKeyIn arg from CreateNewBlock (Sjors Provoost)
ff41b9e296aba0237e068fab181523c50bf8c9d0 Drop script_pub_key arg from createNewBlock (Sjors Provoost)
7ab733ede444aee61ab52670c228eec811268c6f 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 52fd1511a774d1ff9e747b2ce88aa1fcb778ced8. No change since last review other than rebase
TheCharlatan:
Re-ACK 52fd1511a774d1ff9e747b2ce88aa1fcb778ced8
vasild:
ACK 52fd1511a774d1ff9e747b2ce88aa1fcb778ced8
Tree-SHA512: c4b3a53774d9a5dc90950e77f47a64dbb68f971baffbb9a0d8f59332ef8e52d0c039130c925bde73135b3d0e79e65d91d1df30dc4cff13f32d8a72e5c56669d8
If a reindex was interrupted while it was iterating
through the block files, genesis will already be connected
when the reindex resumes at the next startup.
In this case, a call to ActivateBestChainState() is not only unnecessary,
but it would connect multiple blocks without applying
-assumevalid, which is much slower.
This is because assumevalid requires us to have a header above
the minimum chainwork, but that header is unknown to us if it's in
a later blockfile not indexed yet.
This simplifies the code. The only reason to call ActivateBestChain()
here is to allow the main init thread to finish startup in a case of
-reindex. In this situation no second chainstate can exist anyway
because -reindex would have deleted any snapshot chainstate earlier.
This could change behavior slightly if -loadblocks was used when there is a
snapshot chainstate. In this case, there is no reason to call
ActivateBestChain() for that chainstate here - it will be called in
ImportBlocks() after all blocks have been indexed.
