0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d kernel: Move block tree db open to BlockManager constructor (TheCharlatan)
7fbb1bc44b1461f008284533f1667677e729f0c0 kernel: Move block tree db open to block manager (TheCharlatan)
57ba59c0cdf20de322afabe4a132ad17e483ce77 refactor: Remove redundant reindex check (TheCharlatan)
Pull request description:
Before this change the block tree db was needlessly re-opened during startup when loading a completed snapshot. Improve this by letting the block manager open it on construction. This also simplifies the test code a bit.
The change was initially motivated to make it easier for users of the kernel library to instantiate a BlockManager that may be used to read data from disk without loading the block index into a cache.
ACKs for top commit:
maflcko:
re-ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d 🏪
achow101:
ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d
mzumsande:
re-ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d
Tree-SHA512: fe3d557a725367e549e6a0659f64259cfef6aaa565ec867d9a177be0143ff18a2c4a20dd57e35e15f97cf870df476d88c05b03b6a7d9e8d51c568d9eda8947ef
fa8ade300f421dcc3b0cd956ab03a50a5ae80646 refactor: Avoid GCC false positive error (MarcoFalke)
fa40807fa830ee9724b5cfeef263263aaa6ce5d9 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 fa9e0489f57968945d54ef56b275f51540f3e5e4, 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 fa8ade300f421dcc3b0cd956ab03a50a5ae80646, I've tested locally on Ubuntu 24.04.
Tree-SHA512: 7ee1538fd5304a5ab91ac8c7619a573548d7e0345592a1e9d38b3b73729e09e7c77a9ee703d64cf02a8218de3148376d7836e294abb939aa7533034ba36dfb6c
f5883286e32b625aab3dd80c74d6adb4f37f0a80 Add a fuzz test for Num3072 multiplication and inversion (Pieter Wuille)
a26ce628942243fc9848a63bfdfa5e61f5e936f3 Safegcd based modular inverse for Num3072 (Pieter Wuille)
91ce8cef2d8955d980ab7e89fbf74e8b29adf178 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 f5883286e32b625aab3dd80c74d6adb4f37f0a80
TheCharlatan:
Re-ACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
dergoegge:
tACK f5883286e32b625aab3dd80c74d6adb4f37f0a80
Tree-SHA512: 275872c61d30817a82901dee93fc7153afca55c32b72a95b8768f3fd464da1b09b36f952f30e70225e766b580751cfb9b874b2feaeb73ffaa6943c8062aee19a
fa3c787b62af6abaac35a8f0d785becdb8871cc0 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 fadd568931a2d21e0f80e1efaf2281f5164fa20e and observing an abort when running the `utxo_total_supply` fuzz target.
ACKs for top commit:
marcofleon:
ACK fa3c787b62af6abaac35a8f0d785becdb8871cc0
hodlinator:
re-ACK fa3c787b62af6abaac35a8f0d785becdb8871cc0
ryanofsky:
Code review ACK fa3c787b62af6abaac35a8f0d785becdb8871cc0. 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
223081ece651dc616ff63d9ac447eedc5c2a28fa scripted-diff: rename block and undo functions for consistency (Lőrinc)
baaa3b284671ba28dbbcbb43851ea46175fd2b13 refactor,blocks: remove costly asserts and modernize affected logs (Lőrinc)
fa39f27a0f8b8d14f6769d48f43999a3a1148e4f refactor,blocks: deduplicate block's serialized size calculations (Lőrinc)
dfb2f9d004860c95fc6f0d4a016a9c038d53a475 refactor,blocks: inline `WriteBlockToDisk` (Lőrinc)
42bc4914658d9834a653bd1763aa8f0d54355480 refactor,blocks: inline `UndoWriteToDisk` (Lőrinc)
86b85bb11f8999eb59e34bd026b0791dc866f2eb bench: add SaveBlockBench (Lőrinc)
34f9a0157aad7c10ac364b7e4602c5f74c1f9e20 refactor,bench: rename bench/readblock.cpp to bench/readwriteblock.cpp (Lőrinc)
Pull request description:
`UndoWriteToDisk` and `WriteBlockToDisk` were delegating a subset of their functionality to single-use methods that didn't optimally capture a meaningful chunk of the algorithm, resulting in calculating things twice (serialized size, header size).
This change inlines the awkward methods (asserting that all previous behavior was retained), and in separate commits makes the usages less confusing.
Besides making the methods slightly more intuitive, the refactorings reduce duplicate calculations as well.
The speed difference is insignificant for now (~0.5% for the new `SaveBlockToDiskBench`), but are a cleanup for follow-ups such as https://github.com/bitcoin/bitcoin/pull/31539
ACKs for top commit:
ryanofsky:
Code review ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa. Since last review, "Save" was renamed to "Write", uint32_t references were dropped, some log statements and comments were improved as suggested, and a lot of tweaks made to commits and commit messages which should make this easier to review.
hodlinator:
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa
TheCharlatan:
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa
andrewtoth:
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa
Tree-SHA512: 951bc8ad3504c510988afd95c561e3e259c6212bd14f6536fe56e8eb5bf5c35c32a368bbdb1d5aea1acc473d7e5bd9cdcde02008a148b05af1f955e413062d5c
c0045e6cee06bc0029fb79b5a531aa1f2b817424 Add test for multipath miniscript expression (David Gumberg)
b4ac48090f259dbef567b49fa36a8bf192209710 descriptor: Use InferXOnlyPubkey for miniscript XOnly pubkey from script (Ava Chow)
4c50c21f6bfc1d88846be571055b481ab14b086f tests: Check ExpandPrivate matches for both parsed descriptors (Ava Chow)
092569e8580b7c2c13b6cc9d29bcb4c5e85bbb44 descriptor: Try the other parity in ConstPubkeyProvider::GetPrivKey() (Ava Chow)
Pull request description:
When a `ConstPubkeyProvider` is xonly, the stored pubkey does not necessarily have the correct parity bit. `ToPrivateString()` is correctly handling this by looking up the keys for both parity bits, but `GetPrivKey` does not. This results in not finding the private key when it is actually available if its pubkey has the other parity bit value.
To fix this, this key finding is refactored into `GetPrivKey()` so that its behavior is corrected, and `ToPrivateString()` is changed to use `GetPrivKey()` as well.
Additionally, the descriptor test checks are updated to include a check for `ExpandPrivate()` to verify that both the parsed public and private descriptors produce `SigningProvider`s with the same contents.
Fixes#31589
ACKs for top commit:
Pttn:
ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
davidgumberg:
utACK c0045e6cee
kevkevinpal:
Concept and Code review ACK [c0045e6](c0045e6cee)
furszy:
ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
theStack:
re-ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
rkrux:
Concept ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
Tree-SHA512: 3dcf2a802b996e0680a3f819075e5a689eb22e484c81ea79b40ec04197ee4ba3f6b9c87c45dfe8a847c9b805b2fd0fad77ffb92a93e65dc3aad74d69d9e3d97f
Make the block db open RAII style by calling it in the BlockManager
constructor.
Before this change the block tree db was needlessly re-opened during
startup when loading a completed snapshot. Improve this by letting the
block manager open it on construction. This also simplifies the test
code a bit.
The change was initially motivated to make it easier for users of the
kernel library to instantiate a BlockManager that may be used to read
data from disk without loading the block index into a cache.
This commit is done in preparation for the next commit. Here, the block
tree options are moved to the blockmanager options and the block tree is
instantiated through a helper method of the BlockManager, which is
removed again in the next commit.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
b30cc71e853cbaaec66e7e23d7d1a32468079ee0 doc: fix typos (Adlai Chandrasekhar)
Pull request description:
In the unrelated PR #31621 the linter reported a few typos, that are fixed in this commit. I used the "doc" prefix as it only modifies comments, so none of the more significant prefixes seem appropriate.
ACKs for top commit:
maflcko:
lgtm ACK b30cc71e853cbaaec66e7e23d7d1a32468079ee0
Tree-SHA512: 7bba2d928fc0b98f62f96d9abf6dba98f699b386b75730271fa3e7b57a8a220df2265b699007f066e585e1db2ee3cbe5a272b74a8c153f6f8814c01e6de7a3ee
2a92702bafca5c78b270a9502a22cb9deac02cfc init: Use size_t consistently for cache sizes (TheCharlatan)
65cde3621dbb9ac7d210d4926e7601c4adf5f498 kernel: Move default cache constants to caches (TheCharlatan)
8826cae285490439dc1f19b25fa70b2b9e62dfe8 kernel: Move non-kernel db cache size constants (TheCharlatan)
e758b26b85da27ef44f3d2c924f3f08e8c1f4fdf kernel: Move kernel-specific cache size options to kernel (TheCharlatan)
d5e2c4a4097c799433cfc5367c61568fad2c784e fuzz: Add fuzz test for checked and saturating add and left shift (TheCharlatan)
c03a2795a8e044d17835bbf03de0c64dc7b41da8 util: Add integer left shift helpers (TheCharlatan)
8bd5f8a38ce903c05606841ebed1902398cb0b14 [refactor] init: Simplify coinsdb cache calculation (TheCharlatan)
5db7d4d3d28bd1269a09955b4695135c86c4827d 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 2a92702bafca5c78b270a9502a22cb9deac02cfc
ryanofsky:
Code review ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc. 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 2a92702bafca5c78b270a9502a22cb9deac02cfc
Tree-SHA512: 98376eaa0660b1b8c096a5ce1f3e7c8c30e7cd6644de36856c2d3e573108cfc9473c93ebb3952b7881047b5ae6c85c5b096e6726f30f35be58b98eca07c8c785
86d7135e36efd39781cf4c969011df99f0cbb69d [p2p] only attempt 1p1c when both txns provided by the same peer (glozow)
f7658d9b1475ecaa5cb8e543e5c66a3a3a2dc1fb [cleanup] remove p2p_inv from AddTxAnnouncement (glozow)
063c1324c143d98e6d5108bb51b3ca59b45f9b85 [functional test] getorphantxs reflects multiple announcers (glozow)
0da693f7e129fccaecf9a2c177083d2e80d37781 [functional test] orphan handling with multiple announcers (glozow)
b6ea4a9afe2d8bbf49b6b6c42f0a3ce4390c4535 [p2p] try multiple peers for orphan resolution (glozow)
1d2e1d709ce3d95d409254c860347bc3fedf30e1 [refactor] move creation of unique_parents to helper function (glozow)
c6893b0f0b7b205c8da4b9d281a55c9eb843b582 [txdownload] remove unique_parents that we already have (glozow)
163aaf285af91b49c2d788463dc1e1654c88ade6 [fuzz] orphanage multiple announcer functions (glozow)
22b023b09da3e2fe00467c77b105a61c1961081f [unit test] multiple orphan announcers (glozow)
96c1a822a274689611f409246ef1573906b0083e [unit test] TxOrphanage EraseForBlock (glozow)
04448ce32a3bc4b6d12293f71e40333abe35c224 [txorphanage] add GetTx so that orphan vin can be read (glozow)
e810842acda6fe56e0536ebecfbb9d17d26e1513 [txorphanage] support multiple announcers (glozow)
62a9ff187076686b39dca64ad4f2f439da0875d1 [refactor] change type of unique_parents to Txid (glozow)
6951ddcefd9e05f31ee7634bbfbf1d19e04ec00e [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 86d7135e36efd39781cf4c969011df99f0cbb69d
instagibbs:
reACK 86d7135e36efd39781cf4c969011df99f0cbb69d
dergoegge:
Code review ACK 86d7135e36efd39781cf4c969011df99f0cbb69d
mzumsande:
ACK 86d7135e36efd39781cf4c969011df99f0cbb69d
Tree-SHA512: 618d523b86e60c3ea039e88326d50db4e55e8e18309c6a20e8f2b10ed9e076f1de0315c335fd3b8abdabcc8b53cbceb66fb59147d05470ea25b83a2b4bd9c877
This avoids having to rely on implicit casts when passing them to the
various functions allocating the caches.
This also ensures that if the requested amount of db_cache does not fit
in a size_t, it is clamped to the maximum value of a size_t.
Also take this opportunity to make the total amounts of cache in the
chainstate manager a size_t too.
Carrying non-kernel related fields in the cache sizes for the indexes is
confusing for kernel library users. The cache sizes also are set
currently with magic numbers in bitcoin-chainstate. The comments for the
cache size calculations are also not completely clear.
Solve these things by moving the kernel-specific cache size fields to
their own struct.
This slightly changes the way the cache is allocated 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 branch:
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)
The helpers are used in the following commits to increase the safety of
conversions during cache size calculations.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
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.
a96b84cb1b76e65a639e62f0224f534f89858c18 fuzz: Abort when calling system time without setting mock time (marcofleon)
ff21870e20b2391b684cc50fdd6879805055d6a1 fuzz: Add SetMockTime() to necessary targets (marcofleon)
Pull request description:
This PR expands the `CheckGlobals` utility that was introduced in https://github.com/bitcoin/bitcoin/pull/31486 and should help with fuzz stability (https://github.com/bitcoin/bitcoin/issues/29018).
System time shouldn't be used when running a fuzz test, as it is likely to introduce instability (non-determinism). This PR identifies and fixes the targets that were calling system time without setting mock time at the start of an iteration.
Removing`SetMockTime()` from any one of these targets should result in a crash and a message describing the issue.
ACKs for top commit:
achow101:
ACK a96b84cb1b76e65a639e62f0224f534f89858c18
dergoegge:
Code review ACK a96b84cb1b76e65a639e62f0224f534f89858c18
brunoerg:
crACK a96b84cb1b76e65a639e62f0224f534f89858c18
Tree-SHA512: e093a9feb8a397954f7b1416dfa8790b2733f09d5ac51fda5a9d225a55ebd8f99135aa52bdf5ab531653ad1a3739c4ca2b5349c1d989bb4b009ec8eaad684f7d
- The package feerates are ordered by the sequence in which
packages are selected for inclusion in the block template.
- The commit also tests this new behaviour.
Co-authored-by: willcl-ark <will@256k1.dev>
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).
This means we no longer return parents we already have in the
m_unique_parents result from MempoolRejectedTx.
We need to separate the loop that checks AlreadyHave parents from the
loop that adds parents as announcements, because we may do the latter
loop multiple times for different peers.
Add ability to add and track multiple announcers per orphan transaction,
erasing announcers but not the entire orphan.
The tx creation code in orphanage_tests needs to be updated so that each
tx is unique, because the CountOrphans() check assumes that calling
EraseForPeer necessarily means its orphans are deleted.
Unused for now.
b29d68f942e333d2cfdd6be8d49fe484e0d15e11 test: descriptor: fix test for `MaxSatisfactionWeight` (brunoerg)
Pull request description:
To get the maximum size of a satisfaction for a descriptor with no max sig, the parameter `use_max_sig` should be false.
ACKs for top commit:
fjahr:
utACK b29d68f942e333d2cfdd6be8d49fe484e0d15e11
achow101:
ACK b29d68f942e333d2cfdd6be8d49fe484e0d15e11
tdb3:
re ACK b29d68f942e333d2cfdd6be8d49fe484e0d15e11
furszy:
utACK b29d68f942e
Tree-SHA512: 8559718d126e60ce21a34183f74d227546108b43e3897e49622d6677ed9e7707caa962fd811d8787bd4dafc48a0e779ef11050d5990293faa2f91ded4aaa4f4b
fa494a1d53f3f030fafe7b533d72b2200428a0fd refactor: Specify const in std::span constructor, where needed (MarcoFalke)
faaf4800aa752dde63b8987b1eb0de4e54acf717 Allow std::span in stream serialization (MarcoFalke)
faa5391f77037601875cf4ed154bc42840d34b12 refactor: test: Return std::span from StringBytes (MarcoFalke)
fa86223475353cc994cc2563ba5aecc406d00815 refactor: Avoid passing span iterators when data pointers are expected (MarcoFalke)
faae6fa5f614425f6d58af6f224d4f5aae3e1bed refactor: Simplify SpanPopBack (MarcoFalke)
facc4f120b067af6f94f3125cecc9dafff3e5d57 refactor: Replace fwd-decl with proper include (MarcoFalke)
fac3a782eaf3fa5f12cd908ef6dbc874d4b0e2ba refactor: Avoid needless, unsafe c-style cast (MarcoFalke)
Pull request description:
The `std::span` type is already used in some parts of the codebase, and in most contexts can implicitly convert to and from `Span`. However, the two types are not identical in behavior and trying to use one over the other can result in compile failures in some contexts.
Fix all those issues by allowing either `Span` or `std::span` in any part of the codebase.
All of the changes are also required for the scripted-diff to replace `Span` with `std::span` in https://github.com/bitcoin/bitcoin/pull/31519
ACKs for top commit:
sipa:
utACK fa494a1d53f3f030fafe7b533d72b2200428a0fd
fjahr:
Code review ACK fa494a1d53f3f030fafe7b533d72b2200428a0fd
achow101:
ACK fa494a1d53f3f030fafe7b533d72b2200428a0fd
theuni:
utACK fa494a1d53f3f030fafe7b533d72b2200428a0fd.
adamandrews1:
utACK fa494a1d53
Tree-SHA512: 9440941823e884ff5d7ac161f58b9a0704d8e803b4c91c400bdb5f58f898e4637d63ae627cfc7330e98a721fc38285a04641175aa18d991bd35f8b69ed1d74c4
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>)
| ^
