Using bypass_limits=true is essentially fuzzing part of a
reorg only, and results in TRUC invariants unable to be
checked. Remove most instances of bypassing limits, leaving
one harness able to do so.
Github-Pull: #33504
Rebased-From: bbe8e9063c
Since #29412, we have not allowed mutated blocks to continue
being processed immediately the block is received, but this
is only done for the legacy BLOCK message.
Extend these checks as belt-and-suspenders to not allow
similar mutation strategies to affect relay by honest peers
by applying the check inside
PartiallyDownloadedBlock::FillBlock, immediately before
returning READ_STATUS_OK.
This also removes the extraneous CheckBlock call.
Github-Pull: #32646
Rebased-From: bac9ee4830
https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827 introduced an unsequenced operations with side-effects - which is undefined behavior, i.e. the right hand side can be evaluated before the left hand side, which happens to mutate it.
Tried:
```
clang++ --analyze -std=c++20 -I./src -I./src/test -I./src/test/fuzz src/test/fuzz/base_encode_decode.cpp src/psbt.cpp
```
but it didn't warn about UB.
Grepped for similar ones, but could find any other one in the codebase:
> grep -rnE --include='*.cpp' --include='*.h' '\b(\w+)\(([^)]*\b(\w+)\b[^)]*)\)\s*==\s*\3\.' .
```
./src/test/arith_uint256_tests.cpp:373: BOOST_CHECK(R1L.GetHex() == R1L.ToString());
./src/test/arith_uint256_tests.cpp:374: BOOST_CHECK(R2L.GetHex() == R2L.ToString());
./src/test/arith_uint256_tests.cpp:375: BOOST_CHECK(OneL.GetHex() == OneL.ToString());
./src/test/arith_uint256_tests.cpp:376: BOOST_CHECK(MaxL.GetHex() == MaxL.ToString());
./src/test/fuzz/cluster_linearize.cpp:565: assert(depgraph.FeeRate(best_anc.transactions) == best_anc.feerate);
./src/test/fuzz/cluster_linearize.cpp:646: assert(depgraph.FeeRate(found.transactions) == found.feerate);
./src/test/fuzz/cluster_linearize.cpp:765: assert(depgraph.FeeRate(chunk_info.transactions) == chunk_info.feerate);
./src/test/fuzz/base_encode_decode.cpp:95: assert(DecodeBase64PSBT(psbt, random_string, error) == error.empty());
./src/test/fuzz/key.cpp:102: assert(pubkey.data() == pubkey.begin());
./src/test/skiplist_tests.cpp:42: BOOST_CHECK(vIndex[from].GetAncestor(0) == vIndex.data());
./src/script/signingprovider.cpp:535: ComputeTapbranchHash(node.sub[1]->hash, node.sub[1]->hash) == node.hash) {
./src/pubkey.h:78: return vch.size() > 0 && GetLen(vch[0]) == vch.size();
./src/cluster_linearize.h:881: Assume(elem.inc.feerate.IsEmpty() == elem.pot_feerate.IsEmpty());
```
Hodlinator deduced the UB on Windows in https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2751723855
Github-Pull: #32141
Rebased-From: b1de59e896
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
In Base58 fuzz the two roundtrips are merged now, the new `decode_input` switches between a completely random input and a valid encoded one, to make sure the decoding passes more often.
The `max_ret_len` can also exceed the original length now and is being validated more thoroughly.
Github-Pull: #31917
Rebased-From: d5537c18a9
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: marcofleon <marleo23@proton.me>
They seem to cause timeouts:
> Issue 397734700: bitcoin-core:base58check_encode_decode: Timeout in base58check_encode_decode
The `encoded_string.empty()` check was corrected here to `decoded.empty()` to make sure the `(0, decoded.size() - 1)` range is always valid.
Github-Pull: #31917
Rebased-From: bad1433ef2
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: marcofleon <marleo23@proton.me>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
c73b59d47f fuzz: implement targets for PCP and NAT-PMP port mapping requests (Antoine Poinsot)
1695c8ab5b fuzz: in FuzzedSock::GetSockName(), return a random-length name (Antoine Poinsot)
0d472c1953 fuzz: never return an uninitialized sockaddr in FuzzedSock::GetSockName (Antoine Poinsot)
39b7e2b590 fuzz: add steady clock mocking to FuzzedSock (Antoine Poinsot)
6fe1c35c05 pcp: make NAT-PMP error codes uint16_t (Antoine Poinsot)
01906ce912 pcp: make the ToString method const (Antoine Poinsot)
Pull request description:
Based on https://github.com/bitcoin/bitcoin/pull/31022, this introduces a fuzz target for `PCPRequestPortMap` and `NATPMPRequestPortMap`.
Like in #31022 we set `CreateSock` to return a `Sock` which mocks the responses from the server and uses a mocked steady clock for the `Wait`s. Except here we simply respond with fuzzer-provided data until the client stop sending requests. We also sometimes inject errors and connection failures based on fuzzer-provided data.
We reuse the existing `FuzzedSock`, so a preparatory commit is included that adds steady clock mocking to it. This may be useful for other harnesses as well.
ACKs for top commit:
laanwj:
re-ACK c73b59d47f
marcofleon:
ACK c73b59d47f
dergoegge:
utACK c73b59d47f
Tree-SHA512: 24cd4d958a0999946a0c3d164a242fc3f0a0b66770630252b881423ad0065d29fdaab765014d193b705d3eff397f201d51a88a3ca80c63fd3867745e6f21bb2b
f919d919eb fuzz: Add fuzzing for max_ret_len in DecodeBase58/DecodeBase58Check (Lőrinc)
635bc58f46 test: Fuzz Base32/Base58/Base64 roundtrip conversions (Lőrinc)
5dd3a0d8a8 test: Extend base58_encode_decode.json with edge cases (Lőrinc)
ae40cf1a8e test: Add padding tests for Base32/Base64 (Lőrinc)
Pull request description:
Added fuzzed roundtrips for `base[32|58|64]` encoding to make sure encoding/decoding are symmetric.
Note that if we omit the padding in `EncodeBase32` we won't be able to decode it with `DecodeBase32`.
Added dedicated padding tests to cover failure behavior
Also moved over the Base58 json test edge cases from https://github.com/bitcoin/bitcoin/pull/30035
ACKs for top commit:
hodlinator:
re-ACK f919d919eb
achow101:
ACK f919d919eb
Tree-SHA512: 6a6c63d0a659b70d42aad7a8f37ce6e372756e2c88c84e7be5c1ff1f2a7c58860ed7113acbe1a9658a7d19deb91f0abe2ec527ed660335845cd1e0a9380b4295
ConsumeData() will always try to return a name as long as the requested size. It is more useful, and
closer to how `getsockname` would actually behave in reality, to return a random length name
instead.
This was hindering coverage in the PCP fuzz target as the addr len was set to the size of the
sockaddr_in struct and would exhaust all the provided data from the fuzzer.
Thanks to Marco Fleon for suggesting this.
Co-Authored-by: marcofleon <marleo23@proton.me>
The fuzz provider's `ConsumeData` may return less data than necessary
to fill the sockaddr struct and still return success. Fix this to avoid
the caller using uninitialized memory.
9b7023d31a Fuzz HRP of bech32 as well (Lőrinc)
c1a5d5c100 Split out bech32 separator char to header (Lőrinc)
Pull request description:
Instead of the static "bc" human-readable part, it's now randomly generated based on https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki and the extra restrictions in the code:
> The human-readable part, which is intended to convey the type of data, or anything else that is relevant to the reader. This part MUST contain 1 to 83 US-ASCII characters, with each character having a value in the range [33-126]. HRP validity may be further restricted by specific applications.
Since `bech32::Encode` rejects uppercase letters, we're actually generating values in the `[33-126] - ['A'-'Z']` range.
Split out of https://github.com/bitcoin/bitcoin/pull/30596/files#r1706957219
ACKs for top commit:
sipa:
ACK 9b7023d31a
achow101:
ACK 9b7023d31a
marcofleon:
Code review ACK 9b7023d31a. The separation into two targets and the new `GenerateRandomHRP` seem fine to me.
brunoerg:
code review ACK 9b7023d31a
Tree-SHA512: 22a261b8e7b5516e98f4e7990811954454595438a49a10191ed4ca42b5c71c5054fcc73f2d94e23b498ea833c7f1d5adb225f537ef1a24d15b428259450cdf98
386eecff5f doc: add release notes (ismaelsadeeq)
3eaa0a3b66 miner: init: add `-blockreservedweight` startup option (ismaelsadeeq)
777434a2cd doc: rpc: improve `getmininginfo` help text (ismaelsadeeq)
c8acd4032d init: fail to start when `-blockmaxweight` exceeds `MAX_BLOCK_WEIGHT` (ismaelsadeeq)
5bb31633cc test: add `-blockmaxweight` startup option functional test (ismaelsadeeq)
2c7d90a6d6 miner: bugfix: fix duplicate weight reservation in block assembler (ismaelsadeeq)
Pull request description:
* This PR attempts to fix the duplicate coinbase weight reservation issue we currently have.
* Fixes#21950
We reserve 4000 weight units for coinbase transaction in `DEFAULT_BLOCK_MAX_WEIGHT`
7590e93bc7/src/policy/policy.h (L23)
And also reserve additional `4000` weight units in the default `BlockCreationOptions` struct.
7590e93bc7/src/node/types.h (L36-L40)
**Motivation**
- This issue was first noticed during a review here https://github.com/bitcoin/bitcoin/pull/11100#discussion_r136157411)
- It was later reported in issue #21950.
- I also came across the bug while writing a test for building the block template. I could not create a block template above `3,992,000` in the block assembler, and this was not documented anywhere. It took me a while to realize that we were reserving space for the coinbase transaction weight twice.
---
This PR fixes this by consolidating the reservation to be in a single location in the codebase.
This PR then adds a new startup option `-blockreservedweight` whose default is `8000` that can be used to lower or increase the block reserved weight for block header, txs count, coinbase tx.
ACKs for top commit:
Sjors:
ACK 386eecff5f
fjahr:
Code review ACK 386eecff5f
glozow:
utACK 386eecff5f, nonblocking nits. I do think the release notes should be clarified more
pinheadmz:
ACK 386eecff5f
Tree-SHA512: f27efa1da57947b7f4d42b9322b83d13afe73dd749dd9cac49360002824dd41c99a876a610554ac2d67bad7485020b9dcc423a8e6748fc79d6a10de6d4357d4c
- This commit renamed coinbase_max_additional_weight to block_reserved_weight.
- Also clarify that the reservation is for block header, transaction count
and coinbase transaction.
fa8ade300f refactor: Avoid GCC false positive error (MarcoFalke)
fa40807fa8 ci: Enable DEBUG=1 for one GCC-12+ build to catch 117966 regressions (MarcoFalke)
Pull request description:
It is possible that someone accidentally removes the workaround in fa9e0489f5, or more likely that someone accidentally adds new code without the workaround.
Avoid this by adding a temporary CI check.
This can be tested by reverting the workaround and observing a failure.
ACKs for top commit:
hebasto:
ACK fa8ade300f, I've tested locally on Ubuntu 24.04.
Tree-SHA512: 7ee1538fd5304a5ab91ac8c7619a573548d7e0345592a1e9d38b3b73729e09e7c77a9ee703d64cf02a8218de3148376d7836e294abb939aa7533034ba36dfb6c
f5883286e3 Add a fuzz test for Num3072 multiplication and inversion (Pieter Wuille)
a26ce62894 Safegcd based modular inverse for Num3072 (Pieter Wuille)
91ce8cef2d Add benchmark for MuHash finalization (Pieter Wuille)
Pull request description:
This implements a safegcd-based modular inverse for MuHash3072. It is a fairly straightforward translation of [the libsecp256k1 implementation](https://github.com/bitcoin-core/secp256k1/pull/831), with the following changes:
* Generic for 32-bit and 64-bit
* Specialized for the specific MuHash3072 modulus (2^3072 - 1103717).
* A bit more C++ish
* Far fewer sanity checks
A benchmark is also included for MuHash3072::Finalize. The new implementation is around 100x faster on x86_64 for me (from 5.8 ms to 57 μs); for 32-bit code the factor is likely even larger.
For more information:
* [Original paper](https://gcd.cr.yp.to/papers.html) by Daniel J. Bernstein and Bo-Yin Yang
* [Implementation](https://github.com/bitcoin-core/secp256k1/pull/767) for libsecp256k1 by Peter Dettman; and the [final](https://github.com/bitcoin-core/secp256k1/pull/831) version
* [Explanation](https://github.com/bitcoin-core/secp256k1/blob/master/doc/safegcd_implementation.md) of the algorithm using Python snippets
* [Analysis](https://github.com/sipa/safegcd-bounds) of the maximum number of iterations the algorithm needs
* [Formal proof in Coq](https://medium.com/blockstream/a-formal-proof-of-safegcd-bounds-695e1735a348) by Russell O'Connor (for the 256-bit version of the algorithm; here we use a 3072-bit one).
ACKs for top commit:
achow101:
ACK f5883286e3
TheCharlatan:
Re-ACK f5883286e3
dergoegge:
tACK f5883286e3
Tree-SHA512: 275872c61d30817a82901dee93fc7153afca55c32b72a95b8768f3fd464da1b09b36f952f30e70225e766b580751cfb9b874b2feaeb73ffaa6943c8062aee19a
fa3c787b62 fuzz: Abort when global PRNG is used before SeedRand::ZEROS (MarcoFalke)
Pull request description:
This adds one more check to abort when global PRNG is used before SeedRand::ZEROS in fuzz tests. This is achieved by carving out the two remaining uses. First, `g_rng_temp_path_init`, and second the random fallback for `RANDOM_CTX_SEED`, which isn't used in fuzz tests anyway.
Requested in https://github.com/bitcoin/bitcoin/pull/31521#issuecomment-2554669015
Can be tested by reverting fadd568931 and observing an abort when running the `utxo_total_supply` fuzz target.
ACKs for top commit:
marcofleon:
ACK fa3c787b62
hodlinator:
re-ACK fa3c787b62
ryanofsky:
Code review ACK fa3c787b62. This adds a new check to make that sure that RNG is never seeded during fuzzing after the RNG has been used. Together with existing checks which ensure RNG can only be seeded with zeroes during fuzzing, and that RNG must was seeded at some point if used after fuzzing, this implies it must have been seeded by zeros before being used.
Tree-SHA512: 2614928d31c310309bd9021b3e5637b35f64196020fbf9409e978628799691d0efd3f4cf606be9a2db0ef60b010f890c2e70c910eaa2934a7fbf64cd1598fe22
2a92702baf init: Use size_t consistently for cache sizes (TheCharlatan)
65cde3621d kernel: Move default cache constants to caches (TheCharlatan)
8826cae285 kernel: Move non-kernel db cache size constants (TheCharlatan)
e758b26b85 kernel: Move kernel-specific cache size options to kernel (TheCharlatan)
d5e2c4a409 fuzz: Add fuzz test for checked and saturating add and left shift (TheCharlatan)
c03a2795a8 util: Add integer left shift helpers (TheCharlatan)
8bd5f8a38c [refactor] init: Simplify coinsdb cache calculation (TheCharlatan)
5db7d4d3d2 doc: Correct docstring describing max block tree db cache (TheCharlatan)
Pull request description:
Carrying non-kernel related fields in the cache sizes for the indexes is confusing for kernel library users. The cache sizes are set currently with magic numbers in bitcoin-chainstate. The comments for the cache size calculations are not completely clear. The constants for the cache sizes are also currently in `txdb.h`, which is not an ideal place for holding all cache size related constants.
Solve these things by moving the kernel-specific cache size fields to their own struct and moving the constants to either the node or the kernel cache sizes.
This slightly changes the way the cache is allocated if (and only if) the txindex and/or blockfilterindex is used. Since they are now given precedence over the block tree db cache, this results in a bit less cache being allocated to the block tree db, coinsdb and coins caches. The effect is negligible though, i.e. cache sizes with default dbcache reported through the logs are:
master:
```
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.0 MiB for transaction index database
* Using 49.0 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 335.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
this PR:
```
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.2 MiB for transaction index database
* Using 49.2 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 334.5 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
stickies-v:
re-ACK 2a92702baf
ryanofsky:
Code review ACK 2a92702baf. Changes since last review are fixing size options to use size_t instead of int64_t again, simplifying CheckedLeftShift more, and making other minor suggested cleanups
hodlinator:
re-ACK 2a92702baf
Tree-SHA512: 98376eaa0660b1b8c096a5ce1f3e7c8c30e7cd6644de36856c2d3e573108cfc9473c93ebb3952b7881047b5ae6c85c5b096e6726f30f35be58b98eca07c8c785
86d7135e36 [p2p] only attempt 1p1c when both txns provided by the same peer (glozow)
f7658d9b14 [cleanup] remove p2p_inv from AddTxAnnouncement (glozow)
063c1324c1 [functional test] getorphantxs reflects multiple announcers (glozow)
0da693f7e1 [functional test] orphan handling with multiple announcers (glozow)
b6ea4a9afe [p2p] try multiple peers for orphan resolution (glozow)
1d2e1d709c [refactor] move creation of unique_parents to helper function (glozow)
c6893b0f0b [txdownload] remove unique_parents that we already have (glozow)
163aaf285a [fuzz] orphanage multiple announcer functions (glozow)
22b023b09d [unit test] multiple orphan announcers (glozow)
96c1a822a2 [unit test] TxOrphanage EraseForBlock (glozow)
04448ce32a [txorphanage] add GetTx so that orphan vin can be read (glozow)
e810842acd [txorphanage] support multiple announcers (glozow)
62a9ff1870 [refactor] change type of unique_parents to Txid (glozow)
6951ddcefd [txrequest] GetCandidatePeers (glozow)
Pull request description:
Part of #27463.
(Transaction) **orphan resolution** is a process that kicks off when we are missing UTXOs to validate an unconfirmed transaction. We currently request missing parents by txid; BIP 331 also defines a way to [explicitly request ancestors](https://github.com/bitcoin/bips/blob/master/bip-0331.mediawiki#handle-orphans-better).
Currently, when we find that a transaction is an orphan, we only try to resolve it with the peer who provided the `tx`. If this doesn't work out (e.g. they send a `notfound` or don't respond), we do not try again. We actually can't, because we've already forgotten who else could resolve this orphan (i.e. all the other peers who announced the transaction).
What is wrong with this? It makes transaction download less reliable, particularly for 1p1c packages which must go through orphan resolution in order to be downloaded.
Can we fix this with BIP 331 / is this "duct tape" before the real solution?
BIP 331 (receiver-initiated ancestor package relay) is also based on the idea that there is an orphan that needs resolution, but it's just a new way of communicating information. It's not inherently more honest; you can request ancestor package information and get a `notfound`. So ancestor package relay still requires some kind of procedure for retrying when an orphan resolution attempt fails. See the #27742 implementation which builds on this orphan resolution tracker to keep track of what packages to download (it just isn't rebased on this exact branch). The difference when using BIP 331 is that we request `ancpkginfo` and then `pkgtxns` instead of the parent txids.
Zooming out, we'd like orphan handling to be:
- Bandwidth-efficient: don't have too many requests out at once. As already implemented today, transaction requests for orphan parents and regular download both go through the `TxRequestTracker` so that we don't have duplicate requests out.
- Not vulnerable to censorship: don't give up too easily, use all candidate peers. See e.g. https://bitcoincore.org/en/2024/07/03/disclose_already_asked_for/
- Load-balance between peers: don't overload peers; use all peers available. This is also useful for when we introduce per-peer orphan protection, since each peer will have limited slots.
The approach taken in this PR is to think of each peer who announces an orphan as a potential "orphan resolution candidate." These candidates include:
- the peer who sent us the orphan tx
- any peers who announced the orphan prior to us downloading it
- any peers who subsequently announce the orphan after we have started trying to resolve it
For each orphan resolution candidate, we treat them as having "announced" all of the missing parents to us at the time of receipt of this orphan transaction (or at the time they announced the tx if they do so after we've already started tracking it as an orphan). We add the missing parents as entries to `m_txrequest`, incorporating the logic of typical txrequest processing, which means we prefer outbounds, try not to have duplicate requests in flight, don't overload peers, etc.
ACKs for top commit:
marcofleon:
Code review ACK 86d7135e36
instagibbs:
reACK 86d7135e36
dergoegge:
Code review ACK 86d7135e36
mzumsande:
ACK 86d7135e36
Tree-SHA512: 618d523b86e60c3ea039e88326d50db4e55e8e18309c6a20e8f2b10ed9e076f1de0315c335fd3b8abdabcc8b53cbceb66fb59147d05470ea25b83a2b4bd9c877
This brings the format types closer to the standard library types:
* FormatStringCheck corresponds to std::basic_format_string, with
compile-time checks done via ConstevalFormatString
* RuntimeFormat corresponds to std::runtime_format, with no compile-time
checks done.
Also, it documents where no compile-time checks are done.
Now that we track all announcers of an orphan, it's not helpful to
consider an orphan provided by a peer that didn't send us this parent.
It can only hurt our chances of finding the right orphan when there are
multiple candidates.
Adapt the 2 tests in p2p_opportunistic_1p1c.py that looked at 1p1c
packages from different peers. Instead of checking that the right peer
is punished, we now check that the package is not submitted. We can't
use the functional test to see that the package was not considered
because the behavior is indistinguishable (except for the logs).
Different values are used for max_ret_len throughout the codebase (e.g., 21, 34, 78).
Theoretically, negative and zero values are also permitted. Let's stress-test those as well.
Co-authored-by: brunoerg <brunoely.gc@gmail.com>
This commit introduces symmetric encode-decode roundtrips for all bases.
Minor refactors were also included:
• Split each base into a separate fuzz target.
• Added symmetric encode-decode roundtrip tests for all bases.
• Removed trim testing for encoded_string, as Base58 does not use whitespace padding.
• Made comparisons stricter by removing unnecessary lowercase conversions for bases that have mixed-case alphabets.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
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
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
Same as https://github.com/llvm/llvm-project/pull/113951.
Avoids compile failures under clang-20 &
`D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES`:
```bash
In file included from /bitcoin/src/test/fuzz/addition_overflow.cpp:5:
/bitcoin/src/test/fuzz/FuzzedDataProvider.h:209:5: error: use of undeclared identifier 'abort'
209 | abort();
| ^
/bitcoin/src/test/fuzz/FuzzedDataProvider.h:250:5: error: use of undeclared identifier 'abort'
250 | abort();
```
50cce20013 test, refactor: Compact ccoins_access and ccoins_spend (Lőrinc)
0a159f0914 test, refactor: Remove remaining unbounded flags from coins_tests (Lőrinc)
c0b4b2c1ee test: Validate error messages on fail (Lőrinc)
d5f8d607ab test: Group values and states in tests into CoinEntry wrappers (Lőrinc)
ca74aa7490 test, refactor: Migrate GetCoinsMapEntry to return MaybeCoin (Lőrinc)
15aaa81c38 coins, refactor: Remove direct GetFlags access (Lőrinc)
6b733699cf coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointers (Lőrinc)
fc8c282022 coins, refactor: Make AddFlags, SetDirty, SetFresh static (Lőrinc)
cd0498eabc coins, refactor: Split up AddFlags to remove invalid states (Lőrinc)
Pull request description:
Similarly to https://github.com/bitcoin/bitcoin/pull/30849, this cleanup is intended to de-risk https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1739909068 by simplifying the coin cache public interface.
`CCoinsCacheEntry` provided general access to its internal flags state, even though, in reality, it could only be `clean`, `fresh`, `dirty`, or `fresh|dirty` (in the follow-up, we will remove `fresh` without `dirty`).
Once it was marked as `dirty`, we couldn’t set the state back to clean with `AddFlags(0)`—tests explicitly checked against that.
This PR refines the public interface to make this distinction clearer and to make invalid behavior impossible, rather than just checked by tests. We don't need extensive access to the internals of `CCoinsCacheEntry`, as many tests were simply validating invalid combinations in this way.
The last few commits contain significant test refactorings to make `coins_tests` easier to change in follow-ups.
ACKs for top commit:
andrewtoth:
Code Review ACK 50cce20013
laanwj:
Code review ACK 50cce20013
ryanofsky:
Code review ACK 50cce20013. Looks good! Thanks for the followups.
Tree-SHA512: c0d65f1c7680b4bb9cd368422b218f2473c2ec75a32c7350a6e11e8a1601c81d3c0ae651b9f1dae08400fb4e5d43431d9e4ccca305a718183f9a936fe47c1a6c
faf70cc994 Remove wallet::ParseISO8601DateTime, use ParseISO8601DateTime instead (MarcoFalke)
2222aecd5f util: Implement ParseISO8601DateTime based on C++20 (MarcoFalke)
Pull request description:
`boost::posix_time` in `ParseISO8601DateTime` has many issues:
* It parses random strings that are clearly invalid and returns a time value for them, see [1] below.
* None of the separators `-`, or `:`, or `T`, or `Z` are validated.
* It may crash when running under a hardened C++ library, see https://github.com/bitcoin/bitcoin/issues/28917.
* It has been unmaintained for years, so reporting or fixing any issues will most likely be useless.
* It pulls in a third-party dependency, when the functionality is already included in vanilla C++20.
Fix all issues by replacing it with a simple helper function written in C++20.
Fixes https://github.com/bitcoin/bitcoin/issues/28917.
[1] The following patch passes on current master:
```diff
diff --git a/src/wallet/test/rpc_util_tests.cpp b/src/wallet/test/rpc_util_tests.cpp
index 32f6f5ab46..c1c94c7116 100644
--- a/src/wallet/test/rpc_util_tests.cpp
+++ b/src/wallet/test/rpc_util_tests.cpp
@@ -12,6 +12,14 @@ BOOST_AUTO_TEST_SUITE(wallet_util_tests)
BOOST_AUTO_TEST_CASE(util_ParseISO8601DateTime)
{
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("964296"), 242118028800);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("244622"), 15023836800);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("+INfINITy"), 9223372036854);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("7000802 01"), 158734166400);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("7469-2 +INfINITy"), 9223372036854);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("maXimum-datE-time"), 253402300799);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("577737 114maXimum-datE-time"), 253402300799);
+
BOOST_CHECK_EQUAL(ParseISO8601DateTime("1970-01-01T00:00:00Z"), 0);
BOOST_CHECK_EQUAL(ParseISO8601DateTime("1960-01-01T00:00:00Z"), 0);
BOOST_CHECK_EQUAL(ParseISO8601DateTime("2000-01-01T00:00:01Z"), 946684801);
```
ACKs for top commit:
hebasto:
ACK faf70cc994, I have reviewed the code and it looks OK.
dergoegge:
utACK faf70cc994
Tree-SHA512: 9dd745a356d04acf6200e13a6af52c51a9e2a0eeccea110093ce5da147b3c669c0eda918e46db0164c081a78c8feae3fe557a4759bea18449a8ff2d090095931
