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
e94c9d171239c1fc44fa9c77a4595ecd626b767f [doc] Amend notes on benchmarking (dergoegge)
Pull request description:
This gives some more context on the motivation and larger picture of benchmarks.
ACKs for top commit:
l0rinc:
ACK e94c9d171239c1fc44fa9c77a4595ecd626b767f
instagibbs:
reACK e94c9d171239c1fc44fa9c77a4595ecd626b767f
darosior:
reACK e94c9d171239c1fc44fa9c77a4595ecd626b767f
brunoerg:
reACK e94c9d171239c1fc44fa9c77a4595ecd626b767f
Tree-SHA512: 2cbf51f283f2efc0938e7021ae48db51fe89caf9ef9780821e99fa745dff839e2d202ca956ce6cc48b8319db304069728e77883feefe486264eb1783a0610c93
66d21d0eb6517e04ebfb9fad4085e788de51b4dc qa: check parsed multipath descriptors dont share references (Antoine Poinsot)
09a1875ad8cddeb17c19af34b8282d37fed0937e miniscript: Make NodeRef a unique_ptr (Ava Chow)
9ccb46f91ac231d55a85bf2f61bd2f3276b54b01 miniscript: Ensure there is no NodeRef copy constructor or assignment operator (Ava Chow)
6d11c9c60b5b4a71f8f75cb4eee2f9d0021de310 descriptor: Add proper Clone function to miniscript::Node (Ava Chow)
Pull request description:
Multipath descriptors requires performing a deep copy, so a Clone function that does that is added to miniscript::Node instead of the current shallow copy.
Fixes#30864
ACKs for top commit:
darosior:
re-ACK 66d21d0eb6517e04ebfb9fad4085e788de51b4dc
hodlinator:
re-ACK 66d21d0eb6517e04ebfb9fad4085e788de51b4dc 🚀
brunoerg:
reACK 66d21d0eb6517e04ebfb9fad4085e788de51b4dc
Tree-SHA512: bea017497ed3cc0b2da2df7e3ccae1fa4a324769b7da1065963da131235bd8bfdcdfe337a3fabbb3ab4d3822611211fca6a9772e18e2ee1cb3d853e831ff6f88
Multipath descriptors requires performing a deep copy, so a Clone
function that does that is added to miniscript::Node instead of the
current shallow copy.
Co-Authored-By: Antoine Poinsot <darosior@protonmail.com>
d44626a9c2eb95b434edc7a22a70f2cce63c9d84 depends: Override default build type for `libevent` (Hennadii Stepanov)
Pull request description:
This PR fixes a regression for the `libevent` package introduced in https://github.com/bitcoin/bitcoin/pull/29835.
The `libevent` package defaults to the "Release" build type, which overrides our per-build-type optimization flags with `-O3`.
To prevent this behavior, set `CMAKE_BUILD_TYPE` to "None", consistent with how other packages are handled.
ACKs for top commit:
fanquake:
ACK d44626a9c2eb95b434edc7a22a70f2cce63c9d84
Tree-SHA512: 77abd2e28ad8dda86eb0548d8e49ecf23bac08a2e07dc35c71db62539aa659d471c863d361534c3cf693f9945c1b4f12de7e04eef05d11f8cc5e86d6eff5242d
fa80a7dac4be8a1d8e32d88e46da23f14e06bade test: Bump sync_mempools timeout in p2p_1p1c_network.py (MarcoFalke)
1111b0ac196d9d26c89c9f932c9784a99cdb218d ci: Add missing --combinedlogslen to test-each-commit task (MarcoFalke)
Pull request description:
This should address the two issues that happened in https://github.com/bitcoin/bitcoin/actions/runs/12885576442/job/35924329657?pr=25832#step:6:7601:
* The combined log isn't printed on a test failure.
* The timeout is too strict for the GHA virtual machines.
For reference, the output was:
```
...
149/315 - rpc_blockchain.py --v2transport passed, Duration: 10 s
150/315 - p2p_addrfetch.py passed, Duration: 1 s
151/315 - p2p_1p1c_network.py failed, Duration: 31 s
stdout:
2025-01-21T12:05:49.465000Z TestFramework (INFO): PRNG seed is: 6581340712385622842
2025-01-21T12:05:49.466000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20250121_120233/p2p_1p1c_network_207
2025-01-21T12:05:52.408000Z TestFramework (INFO): Fill mempools with large transactions to raise mempool minimum feerates
2025-01-21T12:05:52.408000Z TestFramework (INFO): Fill the mempool until eviction is triggered and the mempoolminfee rises
2025-01-21T12:05:59.692000Z TestFramework (INFO): Pre-send some transactions to nodes
2025-01-21T12:06:00.203000Z TestFramework (INFO): Submit full packages to node0
2025-01-21T12:06:00.220000Z TestFramework (INFO): Wait for mempools to sync
2025-01-21T12:06:20.384000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
self.run_test()
File "/home/runner/work/bitcoin/bitcoin/build/test/functional/p2p_1p1c_network.py", line 153, in run_test
self.sync_mempools(timeout=20)
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 803, in sync_mempools
raise AssertionError("Mempool sync timed out after {}s:{}".format(
AssertionError: Mempool sync timed out after 20s:
...
ACKs for top commit:
l0rinc:
utACK fa80a7dac4be8a1d8e32d88e46da23f14e06bade
glozow:
ACK fa80a7dac4be8a1d8e32d88e46da23f14e06bade
Tree-SHA512: b326b7906b184fb47abc50d0d7ec91a6c90d324997f2abc40f156f588090e8d89bd8486bb8950cac604e77b1b336142a47b53ad463b2670d81222814eeb313d4
The `libevent` package defaults to the "Release" build type, which
overrides our per-build-type optimization flags with `-O3`.
To prevent this behavior, set `CMAKE_BUILD_TYPE` to "None", consistent
with how other packages are handled.
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
01df180bfb82c7eafac4638ced249bee4409784b depends: add mold & ld.lld to gen_id (fanquake)
d032ac80633aa6dab7244ec66edd73f4c8ed4ff2 depends: add *FLAGS to gen_id (fanquake)
Pull request description:
The depends cache should be busted when flags change, the same as any other tooling change. I'd also like to start passing `*FLAGS` into depends inside the Guix env, which, without this change, doesn't bust the cache.
ACKs for top commit:
hebasto:
ACK 01df180bfb82c7eafac4638ced249bee4409784b.
Tree-SHA512: 3809359fe763af9dde484e0c6bd3e262c4c09fcbe2f96ccf64194f5f9f840f5476b9c9929cf7bda7b8c14efeffd369cdb8c233625b79a944e1380df20698246f
faaabfaea768deb7767c489d32fd2097fd180872 ci: Bump centos stream 10 (MarcoFalke)
Pull request description:
This is a follow-up to fa47baa03bcfcf44fb2ed05f009a32d32f860c45, which bumped the gcc version to avoid a warning bloat in the CI log. However, it is also required to bump python3, see https://github.com/bitcoin/bitcoin/issues/31476#issue-2735206340
> This will uncover an issue in the centos task that the correct python version is missing. I guess this should be fixed by installing and activating an acceptable python version.
Instead of bumping the packages individually in centos stream 9, just bump to stream 10.
ACKs for top commit:
fanquake:
ACK faaabfaea768deb7767c489d32fd2097fd180872
Tree-SHA512: a564ff3a2a0dc4d39874e87540e67072f293bbed82c8eca22266fcadc16c5571e0e41d38576a63e466b64d13f7e3acbd95be10cf2420de33127aa420eca3b928
6e29de21010fc5213176a6ba29f754ca72612ea0 ci: Supply `platform` argument to docker commands. (David Gumberg)
Pull request description:
I ran into this issue when following the instructions in `ci/README.md` for running CI locally.
Newer versions of docker require a `--platform` argument when building from a platform-specific image that differs from the host platform, I'm not sure when this change took place, but trying to build any of the cross-platform CI images on Docker 27.5.0 fails in the following manner:
```console
$ # From ci/README.md
$ env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh'
WARNING: The requested image's platform (linux/arm64/v8) does not match the detected host platform (linux/amd64/v4) and no specific platform was requested
Creating docker.io/arm64v8/debian:bookworm container to run in
+ docker build --file $BITCOIN_SRC/ci/test_imagefile --build-arg CI_IMAGE_NAME_TAG=docker.io/arm64v8/debian:bookworm --build-arg FILE_ENV=./ci/test/00_setup_env_arm.sh --label=bitcoin-ci-test --tag=ci_arm_linux $BITCOIN_SRC
[+] Building 0.6s (2/2) FINISHED docker:default
=> [internal] load build definition from test_imagefile 0.0s
=> => transferring dockerfile: 600B 0.0s
=> WARN: InvalidDefaultArgInFrom: Default value for ARG ${CI_IMAGE_NAME_TAG} results in empty or invalid base image name (line 8) 0.0s
=> ERROR [internal] load metadata for docker.io/arm64v8/debian:bookworm 0.5s
------
> [internal] load metadata for docker.io/arm64v8/debian:bookworm:
------
1 warning found (use docker --debug to expand):
- InvalidDefaultArgInFrom: Default value for ARG ${CI_IMAGE_NAME_TAG} results in empty or invalid base image name (line 8)
test_imagefile:8
--------------------
6 |
7 | ARG CI_IMAGE_NAME_TAG
8 | >>> FROM ${CI_IMAGE_NAME_TAG}
9 |
10 | ARG FILE_ENV
--------------------
ERROR: failed to solve: docker.io/arm64v8/debian:bookworm: failed to resolve source metadata for docker.io/arm64v8/debian:bookworm: no match for platform in manifest: not found
```
This branch fixes this by setting the `--platform` argument of `docker build` and `docker run` with an environment variable `CI_IMAGE_PLATFORM` for each platform specific job, and `linux/{$cpuarch}` for any native jobs.
Thi
## Steps to reproduce
1. Install relevant dependencies, on Ubuntu:
```bash
sudo apt install bash docker.io python3 qemu-user-static
```
2. Run one of the platform-specific CI images, e.g.:
```bash
env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh'
```
ACKs for top commit:
maflcko:
lgtm ACK 6e29de21010fc5213176a6ba29f754ca72612ea0
hebasto:
ACK 6e29de21010fc5213176a6ba29f754ca72612ea0
Tree-SHA512: 81b9fa8ec1f3d21619d37d864047c8d7917ef2c8536851f80facf7f1973dfe14628d7755f12d2a9c6edebb6cb16877c582d4d41cdab52b73b23c44f08c6e6b30
31a0e5f0905bfc6b22ceaaeca53466dfd74967ab depends: Qt 5.15.16 (fanquake)
Pull request description:
Contains a handful of miscellaneous bug fixes.
We can drop a few of our patches.
See https://github.com/qt/qtbase/compare/v5.15.14-lts-lgpl...v5.15.16-lts-lgpl.
ACKs for top commit:
hebasto:
ACK 31a0e5f0905bfc6b22ceaaeca53466dfd74967ab.
TheCharlatan:
ACK 31a0e5f0905bfc6b22ceaaeca53466dfd74967ab
Tree-SHA512: dd7b3332dd6ecb95189bc72364883425fb8869e03850791d2ee92555a37046c7abaaee16575a0396f1ce9674856b894563dbd36868c2cf46f9fee48028fd967b
fabefd9915802d53d8c83ae66e5cb259196d9422 ci: Turn CentOS task into native one (MarcoFalke)
Pull request description:
Cross-compiling to `i686-pc-linux-gnu` on CentOS in the CI is mostly redundant with the `ci/test/00_setup_env_i686_multiprocess.sh` task (albeit it using clang):
35bf426e02/ci/test/00_setup_env_i686_multiprocess.sh (L9-L12)
One task seems sufficient as a sanity check, given that there seems to be no real demand for this architecture anyway.
Turning the task into a native one makes it possible to run the task natively on aarch64 or any other supported architecture.
Also, remove the install of the `lbzip2` package, which is unused since commit a46065e36cf868265c909dc5edf29dc17be53c1f
Also, remove the `CONFIG_SHELL` env var, which is unused since the cmake migration. (`CONFIG_SHELL` in depends is still kept).
ACKs for top commit:
davidgumberg:
ACK fabefd9915
hebasto:
ACK fabefd9915802d53d8c83ae66e5cb259196d9422, tested locally on Ubuntu 24.10.
Tree-SHA512: 5a7b3131b379d11ef602e5821165861e9bdf61d605014bf8fcb33b8e12d8823450798af2d3289b96f7559dfa47b839bf939ddc0b3725efecfeac7ae570a981e7
160c27ec078346fbf07f9b84dc10cef2b9809327 doc: Update dependency installation for Debian/Ubuntu and CI (Adlai Chandrasekhar)
Pull request description:
This is similar to the recently-pushed 8d20348 and results in slightly cleaner systems for future Debian/Ubuntu builds.
According to the description for pkg-config, "pkgconf is a replacement for pkg-config, providing additional functionality while also maintaining compatibility. This package only provides a dependency link to the pkgconf package to help with package upgrades. It can be safely removed."
Thus the relevant sections of `doc/build-unix.md` and `depends/README.md` are updated.
ACKs for top commit:
maflcko:
weak ACK 160c27ec078346fbf07f9b84dc10cef2b9809327
fanquake:
ACK 160c27ec078346fbf07f9b84dc10cef2b9809327 - seems correct for modern distro versions, and using pkgconf on older ones also seems to work fine.
Tree-SHA512: fadeffe464073df91b706e30f560bfe332ce676521cc5d2044d3bf499f08d986ccaab0a10dd1178f626a90bbac3a4f8c445fe4f8e3a63960721664a247b758f7
f6a6d912059c66792f48689632d2a7f14f8bdad9 test: add check for getting SigningProvider for a CPubKey (Sebastian Falbesoner)
62a95f5af9b998e241eb72c16ba581e77c480126 test: refactor: move `CreateDescriptor` helper to wallet test util module (Sebastian Falbesoner)
493656763f73e5ef1cfb979a513c12983dca99dd desc spkm: Return SigningProvider only if we have the privkey (Ava Chow)
Pull request description:
If we know about a pubkey that's in our descriptor, but we don't have the private key, don't return a SigningProvider for that pubkey.
This is specifically an issue for Taproot outputs that use the H point as the resulting PSBTs may end up containing irrelevant information because the H point was detected as a pubkey each unrelated descriptor knew about.
Split from #29675
ACKs for top commit:
fjahr:
ACK f6a6d912059c66792f48689632d2a7f14f8bdad9
theStack:
re-ACK f6a6d912059c66792f48689632d2a7f14f8bdad9
furszy:
utACK f6a6d912059. Only reviewed the actual change in detail, not the test commit.
Tree-SHA512: 30a196e611a0c5d9ebe5baf6d896caaa6af66f1615463dbb0c31e52604d53cf342922bb9967b3c697b47083d76b0485c77a5f545bd6381247c8bc44321c70f97
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
According to the description for pkg-config, "pkgconf is a
replacement for pkg-config, providing additional functionality
while also maintaining compatibility. This package only provides
a dependency link to the pkgconf package to help with package
upgrades. It can be safely removed."
Thus several scripts and markdown files are updated.
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
fabeca3458b38a3d8930cb0cbc866388c3f120f1 refactor: Avoid UB in SHA3_256::Write (MarcoFalke)
fad4032b219e58300f8d0a74bbcf38ef26fa89d0 refactor: Drop unused UCharCast (MarcoFalke)
Pull request description:
It is UB to apply a distance to a pointer or iterator further than the
end itself, even if the distance is (partially) revoked later on.
Fix the issue by advancing the data pointer at most to the end.
This fix is required to adopt C++ safe buffers https://github.com/bitcoin/bitcoin/issues/31272.
Also included is a somewhat unrelated commit.
ACKs for top commit:
sipa:
utACK fabeca3458b38a3d8930cb0cbc866388c3f120f1
theuni:
utACK fabeca3458b38a3d8930cb0cbc866388c3f120f1
hebasto:
ACK fabeca3458b38a3d8930cb0cbc866388c3f120f1.
Tree-SHA512: 78c53691322b72c3ba9c25ec94eec275dbbbc3049b0ad45e7d9fb2df0afbbaa905b0d8fa7106a3582f937bb1dc15a7592c4ad2d80fe4cff1062a3acfd3638f08
fa3efb5729091a36a0e82316e9e4b7c09115dc2e refactor: Introduce struct to hold a runtime format string (MarcoFalke)
fa6adb0134408d9a5cb623a41f057d3807e2b006 lint: Remove unused and broken format string linter (MarcoFalke)
fadc6b9bac82e99a422d72935a74f3a5444274ed refactor: Check translatable format strings at compile-time (MarcoFalke)
fa1d5acb8d8e1842797604fbd1e7c69f6acdb6c7 refactor: Use TranslateFn type consistently (MarcoFalke)
eeee6cf2ffb24d930fc2fe77912788abf685cd4b refactor: Delay translation of _() literals (MarcoFalke)
Pull request description:
All translatable format strings are fixed. This change surfaces errors in them at compile-time.
The implementation achieves this by allowing to delay the translation (or `std::string` construction) that previously happened in `_()` by returning a new type from this function. The new type can be converted to `bilingual_str` where needed.
This can be tested by adding a format string error in an original string literal and observing a new compile-time failure.
Fixes https://github.com/bitcoin/bitcoin/issues/30530
ACKs for top commit:
stickies-v:
re-ACK fa3efb5729091a36a0e82316e9e4b7c09115dc2e
ryanofsky:
Code review ACK fa3efb5729091a36a0e82316e9e4b7c09115dc2e. Since last review added TranslateFn commit, clarified FormatStringCheck documentation, dropped redundant `inline` keyword
Tree-SHA512: 28fa1db11e85935d998031347bd519675d75c171c8323b0ed6cdd0b628c95250bb86b30876946cc48840ded541e95b8a152696f9f2b13a5f28f5673228ee0509
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.
They are not related to the txdb, so a better place for them is the
new kernel and node cache file. Re-use the default amount of kernel
cache for the default node cache.
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>
