7912cd4125 bench: Fix WalletMigration benchmark (pablomartin4btc)
Pull request description:
The keys and scripts created for the Legacy Wallet needed to be persisted in order for the migration to work properly.
Fixes#32277.
ACKs for top commit:
achow101:
ACK 7912cd4125
davidgumberg:
Tested ACK 7912cd4125
furszy:
utACK 7912cd4125
Tree-SHA512: fe7b8e0a80d4d030ad3fd6446717ee09a260ab2bd6140bc817bdca52d233e3af8a8fed2d754743ca2ba022f7d2c8615a36b5070991d12942c13835e8f72e359f
7a93544cdc doc: Fix and clarify description of ZMQ message format (Jiri Jakes)
Pull request description:
This change stresses that all ZMQ messages share the same structure and that they differ only in the format of the bodies. Previously this was not clear.
Further it removes the notion of endianness of 32-byte hashes, as it was misleading, and replaces it with the term 'reversed byte order' (as opposed to natural or normal byte order produced by hashing functions).
Additionally, it states that ZMQ 32-byte hashes are in the same format as in RPC. Previously it incorrectly stated that the two were in different formats.
[Rendered](https://github.com/jirijakes/bitcoin/blob/zmq-doc/doc/zmq.md).
Fixes#31856.
ACKs for top commit:
w0xlt:
Code review ACK 7a93544cdc
achow101:
ACK 7a93544cdc
ryanofsky:
Code review ACK 7a93544cdc. Nice changes. Documentation seems less repetitive and easier to understand now
Tree-SHA512: 8c5ab047c5fd9b5b6910d691b725886d7743dfd01510735b46e43d01c2d0d25ec52d79d71ec75dbeb142e96a88ad503d69ee14b971e3cdaeb8fd85e5292a8c21
a4041c77f0 test: Handle empty string returned by CLI as None in RPC tests (Brandon Odiwuor)
Pull request description:
Partially Fixes https://github.com/bitcoin/bitcoin/issues/32264
Some tests are failing when `bitcoin-cli` returns an empty string. This change treats an empty response as `None`. See https://github.com/bitcoin/bitcoin/issues/32264#issuecomment-2807616694
This fixes the error for:
- feature_bip68_sequence.py
- feature_nulldummy.py
- feature_signet.py
- mining_mainnet.py
- rpc_scanblocks.py
- rpc_scantxoutset.py
- wallet_descriptor.py --descriptors
ACKs for top commit:
maflcko:
lgtm ACK a4041c77f0
achow101:
ACK a4041c77f0
pablomartin4btc:
ACK a4041c77f0
mzumsande:
ACK a4041c77f0
Tree-SHA512: 2f1a416a18e0b3eebdb014c2e2e8dadf1d46b15c231cb61f577d47f5e551994ab0e2aeb7c179c01be7c1f07ebc03476236d29cf2d04c358ffb1fae985aa385c9
32dcec269b rpc: update RPC help of `createpsbt` (rkrux)
931117a46f rpc: update the doc for `data` field in `outputs` argument (rkrux)
8134a6b5d4 rpc: add cli example for `walletcreatefundedpsbt` RPC (rkrux)
Pull request description:
### add cli example for `walletcreatefundedpsbt` and `createpsbt` RPCs
The only example present earlier was one that creates an OP_RETURN output. This
lack of examples has discouraged me earlier to use this RPC. Adding an example
that creates PSBT sending bitcoin to address, a scenario that is much more common.
### rpc: update the doc for `data` field in `outputs` argument
It was not evident to me that this field creates an `OP_RETURN` output until
I read the code and tried it out. Thus, making the doc explicitly mention it.
This affects docs of the following RPCs:
`bumpfee`, `psbtbumpfee`, `send`, `walletcreatefundedpsbt`, `createpsbt`,
and `createrawtransaction`
ACKs for top commit:
sipa:
utACK 32dcec269b
1440000bytes:
utACK 32dcec269b
achow101:
ACK 32dcec269b
ryanofsky:
Concept ACK 32dcec269b. These seem like helpful clarifications, but I did not look into the details
Tree-SHA512: f994488ba7d52d00960fc52064bb419cf548e29822fe23d6ee0452fdf514dd93f089145eddb32b8086a7918cf8cf33a4c3f16bfcb7948f3c9d5afd95e8d3a1cb
7749d929a0 Remove support for RNDR/RNDRRS for aarch64 on Linux (laanwj)
Pull request description:
This hardware feature is
- Rarely supported on SoCs (and broken on like half of the chips that support it in the first place) (#31817). It is not clear if, or how, the brokenness will be worked around in the kernel, but working around it in user space seems the wrong thing to do, this is not the place to maintain special workarounds for specific hardware (which despite that, was attempted in #31826, but had to be reverted in #31908 due to other problems).
- Apparently not compiled into the release binary anymore (https://github.com/bitcoin/bitcoin/issues/31817#issuecomment-2795885962). Did check this at the time, but a build system change must have caused this, and went undetected.
- Hard to test in CI (as well as manually), due to unavailability of hardware.
Better to remove it.
This reverts commit aee5404e02 from #26839.
Closes#31817.
ACKs for top commit:
sipa:
utACK 7749d929a0
davidgumberg:
utACK 7749d929a0
achow101:
ACK 7749d929a0
w0xlt:
utACK 7749d929a0
Tree-SHA512: d243ad7f745fb46f711f24b6983d9ea1d94e5d8ee60959229bafdba5caa210a60801a1c2cb5b558a0e72f365371b32285aee9a8d0cd24a60589adc7b03dd6a44
05117e6e17 rpc: clarify longpoll behavior (Sjors Provoost)
5315278e7c Have createNewBlock() wait for a tip (Sjors Provoost)
64a2795fd4 rpc: handle shutdown during long poll and wait methods (Sjors Provoost)
a3bf43343f rpc: drop unneeded IsRPCRunning() guards (Sjors Provoost)
f9cf8bd0ab Handle negative timeout for waitTipChanged() (Sjors Provoost)
Pull request description:
This PR prevents Mining interface methods from sometimes crashing when called during startup before a tip is connected. It also makes other improvements like making more RPC methods usable from the GUI. Specifically this PR:
- Adds an `Assume` check to disallow passing negative timeout values to `Mining::waitTipChanged`
- Makes `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods usable from the GUI when `-server=1` is not set.
- Changes `Mining::waitTipChanged` to return `optional<BlockRef>` instead of `BlockRef` and return `nullopt` instead of crashing if there is a timeout or if the node is shut down before a tip is connected.
- Changes `Mining::waitTipChanged` to not time out before a tip is connected, so it is convenient and safe to call during startup, and only returns `nullopt` on early shutdowns.
- Changes `Mining::createNewBlock` to block and wait for a tip to be connected if it is called on startup instead of crashing. Also documents that it will return null on early shutdowns.
This allows `waitNext()` (added in https://github.com/bitcoin/bitcoin/pull/31283) to safely assume `TipBlock()` isn't `null`, not even during a scenario of early shutdown.
Finally this PR clarifies long poll behaviour, mostly by adding code comments, but also through an early `break`.
ACKs for top commit:
achow101:
ACK 05117e6e17
ryanofsky:
Code review ACK 05117e6e17, just updated a commit message since last review
TheCharlatan:
ACK 05117e6e17
vasild:
ACK 05117e6e17
Tree-SHA512: 277c285a6e73dfff88fd379298190b264254996f98b93c91c062986ab35c2aa5e1fbfec4cd71d7b29dc2d68e33f252b5cfc501345f54939d6bd78599b71fec04
fa21f83d29 ci: Use G++ in valgrind tasks (MarcoFalke)
fabd05bf65 refactor: Fix net_processing iwyu includes (MarcoFalke)
fa1622db20 refactor: Make node_id a const& in RemoveBlockRequest (MarcoFalke)
Pull request description:
Currently, `valgrind` is not usable on a default build with GCC. Specifically, `p2p_compactblocks.py --valgrind` gives a false-positive in `RemoveBlockRequest` when comparing `node_id` with `from_peer`. According to the upstream bug report, this happens because both symbols are on the stack and the compiler can more aggressively optimize the compare (order). See https://bugs.kde.org/show_bug.cgi?id=472329#c7
It is possible to work around this bug by pulling at least one value from the stack. For example, by making `from_peer` a `const` reference. Alternatively, by replacing `auto [node_id, list_it]` with `const auto& [node_id, list_it]`, which is done here.
I think this workaround is acceptable, because it does not look like valgrind can trivially fix this. The alternative would be to add a (temporary?) suppression.
Fixes https://github.com/bitcoin/bitcoin/issues/27741
Also, fix iwyu includes, while touching this module.
Also, switch the CI valgrind scripts to use G++.
ACKs for top commit:
achow101:
ACK fa21f83d29
TheCharlatan:
ACK fa21f83d29
darosior:
utACK fa21f83d29
ryanofsky:
Code review ACK fa21f83d29. Code changes all look good but I'm a little confused about purpose of the third commit, so left a question about that
Tree-SHA512: 7b92cdafd525a5ac53ae2c1a7a92e599bc9b5fd5d315a694b493cd5079ac323d884393b57aa18581b7789247a588c9a27d47698de25b340bc76fc9f1dd1850b4
The obfuscation (XOR) operations are currently done byte-by-byte during serialization. Buffering the reads will enable batching the obfuscation operations later.
Different operating systems handle file caching differently, so reading larger batches (and processing them from memory) is measurably faster, likely because of fewer native fread calls and reduced lock contention.
Note that `ReadRawBlock` doesn't need buffering since it already reads the whole block directly.
Unlike `ReadBlockUndo`, the new `ReadBlock` implementation delegates to `ReadRawBlock`, which uses more memory than a buffered alternative but results in slightly simpler code and a small performance increase (~0.4%). This approach also clearly documents that `ReadRawBlock` is a logical subset of `ReadBlock` functionality.
The current implementation, which iterates over a fixed-size buffer, provides a more general alternative to Cory Fields' solution of reading the entire block size in advance.
Buffer sizes were selected based on benchmarking to ensure the buffered reader produces performance similar to reading the whole block into memory. Smaller buffers were slower, while larger ones showed diminishing returns.
------
> macOS Sequoia 15.3.1
> C++ compiler .......................... Clang 19.1.7
> cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ && cmake --build build -j$(nproc) && build/bin/bench_bitcoin -filter='ReadBlockBench' -min-time=10000
Before:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,271,441.67 | 440.25 | 0.1% | 11.00 | `ReadBlockBench`
After:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,738,971.29 | 575.05 | 0.2% | 10.97 | `ReadBlockBench`
------
> Ubuntu 24.04.2 LTS
> C++ compiler .......................... GNU 13.3.0
> cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ && cmake --build build -j$(nproc) && build/bin/bench_bitcoin -filter='ReadBlockBench' -min-time=20000
Before:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 6,895,987.11 | 145.01 | 0.0% | 71,055,269.86 | 23,977,374.37 | 2.963 | 5,074,828.78 | 0.4% | 22.00 | `ReadBlockBench`
After:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 5,771,882.71 | 173.25 | 0.0% | 65,741,889.82 | 20,453,232.33 | 3.214 | 3,971,321.75 | 0.3% | 22.01 | `ReadBlockBench`
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
Made every OpenBlockFile#fReadOnly value explicit.
Replaced hard-coded values in ReadRawBlock with STORAGE_HEADER_BYTES.
Changed `STORAGE_HEADER_BYTES` and `UNDO_DATA_DISK_OVERHEAD` to `uint32_t` to avoid casts.
Also added `LIFETIMEBOUND` to the `AutoFile` parameter of `BufferedFile`, which stores a reference to the underlying `AutoFile`, allowing Clang to emit warnings if the referenced `AutoFile` might be destroyed while `BufferedFile` still exists.
Without this attribute, code with lifetime violations wouldn't trigger compiler warnings.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Renames the constant to be less verbose and better reflect its purpose:
it represents the size of the storage header that precedes serialized block data on disk,
not to be confused with a block's own header.
-BEGIN VERIFY SCRIPT-
git grep -q "STORAGE_HEADER_BYTES" $(git ls-files) && echo "Error: Target name STORAGE_HEADER_BYTES already exists in the codebase" && exit 1
sed -i 's/BLOCK_SERIALIZATION_HEADER_SIZE/STORAGE_HEADER_BYTES/g' $(git grep -l 'BLOCK_SERIALIZATION_HEADER_SIZE')
-END VERIFY SCRIPT-
`AutoFile{OpenUndoFile(pos)}` was still in scope when `FlushUndoFile(pos.nFile)` was called, which could lead to file handle conflicts or other unexpected behavior.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Reorganized error handling in block-related operations by grouping related operations together within the same scope.
In `ReadBlockUndo()` and `ReadBlock()`, moved all deserialization operations, comments and checksum verification inside a single try/catch block for cleaner error handling.
In `WriteBlockUndo()`, consolidated hash calculation and data writing operations within a common block to better express their logical relationship.
8fe001d597 doc: Updates how to reproduce fuzz CI failure locally (Sergi Delgado Segura)
Pull request description:
The current version of the doc does not explain how to reproduce a recent fuzzing CI failure (not yet part of the corpora). Add instructions on how to manually create a crash file based on a report.
ACKs for top commit:
maflcko:
lgtm ACK 8fe001d597
glozow:
ACK 8fe001d597
Tree-SHA512: 7436d71a30bbbffc34770027f1deeacca2de528d8d1b333431d6070c2ba779ecfcdaf25dc791d2154ba4dd37824d06aed2695a8412d7ca1f29e5bd1796d42aeb
babb9f5db6 depends: remove non-native libmultiprocess build (Cory Fields)
5d105fb8c3 depends: Switch libmultiprocess packages to use local git subtree (Ryan Ofsky)
9b35518d2f depends, moveonly: split up int_get_build_id function (Ryan Ofsky)
2d373e2707 lint: Add exclusions for libmultiprocess subtree (Ryan Ofsky)
e88ab394c1 doc: Update documentation to explain libmultiprocess subtree (Ryan Ofsky)
d4bc563982 cmake: Fix clang-tidy "no input files" errors (Ryan Ofsky)
abdf3cb645 cmake: Fix warnings from boost headers (Ryan Ofsky)
8532fcb1c3 cmake: Fix ctest mptest "Unable to find executable" errors (Ryan Ofsky)
d597ab1dee cmake: Support building with libmultiprocess subtree (Ryan Ofsky)
69f0d4adb7 scripted-diff: s/WITH_MULTIPROCESS/ENABLE_IPC/ in cmake (Ryan Ofsky)
a2f28e4be9 Squashed 'src/ipc/libmultiprocess/' content from commit 35944ffd23fa (Ryan Ofsky)
d6244f85c5 depends: Update libmultiprocess library to simplify cmake subtree build (Ryan Ofsky)
Pull request description:
This adds the [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library and code generator as a subtree in `src/ipc/libmultiprocess` and allows it to be built with the cmake `-DENABLE_IPC` option, which is disabled by default.
This PR does not entirely remove the depends system [libmultiprocess package](https://github.com/bitcoin/bitcoin/blob/master/depends/packages/native_libmultiprocess.mk) because the package is useful when cross compiling. (A cross-compiling cmake build cannot easily build and run a native code generation tool.) However, it does update the depends package to build from the new git subtree, instead of being downloaded separately from github, so the same sources are used to build both the runtime library and the code generator.
This PR includes the following manual changes (not created automatically with `git subtree add`) which just update the build system and documentation:
- [`d6244f85c509` depends: Update libmultiprocess library to simplify cmake subtree build](d6244f85c5)
- [`69f0d4adb72c` scripted-diff: s/WITH_MULTIPROCESS/ENABLE_IPC/ in cmake](69f0d4adb7)
- [`d597ab1dee6b` cmake: Support building with libmultiprocess subtree](d597ab1dee)
- [`8532fcb1c30d` cmake: Fix ctest mptest "Unable to find executable" errors](8532fcb1c3)
- [`abdf3cb6456f` cmake: Fix warnings from boost headers](abdf3cb645)
- [`d4bc5639829f` cmake: Fix clang-tidy "no input files" errors](d4bc563982)
- [`e88ab394c163` doc: Update documentation to explain libmultiprocess subtree](e88ab394c1)
- [`2d373e27071f` lint: Add exclusions for libmultiprocess subtree](2d373e2707)
- [`9b35518d2f3f` depends, moveonly: split up int_get_build_id function](9b35518d2f)
- [`5d105fb8c3ff` depends: Switch libmultiprocess packages to use local git subtree](5d105fb8c3)
- [`babb9f5db641` depends: remove non-native libmultiprocess build](babb9f5db6)
---
Previous minisketch subtree PR #23114 may be useful for comparison
Instructions for subtree verification can be found:
- https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees
- https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh
TL&DR:
```sh
git remote add --fetch libmultiprocess https://github.com/chaincodelabs/libmultiprocess.git
test/lint/git-subtree-check.sh -r src/ipc/libmultiprocess
```
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
Sjors:
re-ACK babb9f5db6
TheCharlatan:
tACK babb9f5db6
vasild:
ACK babb9f5db6
Tree-SHA512: 43d4eecca5aab63e55c613de935965666eaced327f9fe859a0e9c9b85f7685dc16c5c8d6e03e09ca998628c5d468633f4f743529930b037049abe8e0101e0143
This hardware feature is
- rarely supported on SoCs (and broken on like half of the chips that support it in the first place) (#31817)
- apparently not compiled into the release binary (https://github.com/bitcoin/bitcoin/issues/31817#issuecomment-2795885962)
- hard to test in CI, due to unavailable of hardware
Better to remove it.
This reverts commit aee5404e02.
Closes#31817.
ff0194a7ce miniscript: convert non-critical asserts to CHECK_NONFATAL (Antoine Poinsot)
Pull request description:
The Miniscript code contains assertions to prevent ending up in an insane state or prevent UB, but also to enforce logical invariants. For the latter it is not necessary to crash the program if they are broken. Raising an exception suffices, especially as this code is often called through the RPC interface which can in turn handle the exception and the user can report it to developers.
This revives #28678 from Pieter Wuille.
ACKs for top commit:
hodlinator:
ACK ff0194a7ce
TheCharlatan:
ACK ff0194a7ce
brunoerg:
code review ACK ff0194a7ce
Tree-SHA512: 8ed8f7b494e46ecf7cdebe75120cd0ffe543b6bc289bf882dac631fe2ec2cae590d5f7bc2316e52db085791694b136dffbc71c40c1e16886fa53ab00bd8cabd0
ec81a72b36 net: Add randomized prefix to Tor stream isolation credentials (laanwj)
c47f81e8ac net: Rename `_randomize_credentials` Proxy parameter to `tor_stream_isolation` (laanwj)
Pull request description:
Add a class TorsStreamIsolationCredentialsGenerator that generates unique credentials based on a randomly generated session prefix and an atomic counter. Use this in `ConnectThroughProxy` instead of a simple atomic int counter.
This makes sure that different launches of the application won't share the same credentials, and thus circuits, even in edge cases.
Example with `-debug=proxy`:
```
2025-03-31T16:30:27Z [proxy] SOCKS5 sending proxy authentication 0afb2da441f5c105-0:0afb2da441f5c105-0
2025-03-31T16:30:31Z [proxy] SOCKS5 sending proxy authentication 0afb2da441f5c105-1:0afb2da441f5c105-1
```
Thanks to hodlinator in https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2020973352 for the idea.
ACKs for top commit:
hodlinator:
re-ACK ec81a72b36
jonatack:
ACK ec81a72b36
danielabrozzoni:
tACK ec81a72b36
Tree-SHA512: 195f5885fade77545977b91bdc41394234ae575679cb61631341df443fd8482cd74650104e323c7dbfff7826b10ad61692cca1284d6810f84500a3488f46597a
The current version of the doc does not explain how to reproduce a recent fuzzing CI failure
(not yet part of the corpora). Add instructions on how to manually create a crash file based
on a report.
faa3ce3199 fuzz: Avoid influence on the global RNG from peerman m_rng (MarcoFalke)
faf4c1b6fc fuzz: Disable unused validation interface and scheduler in p2p_headers_presync (MarcoFalke)
fafaca6cbc fuzz: Avoid setting the mock-time twice (MarcoFalke)
fad22149f4 refactor: Use MockableSteadyClock in ReportHeadersPresync (MarcoFalke)
fa9c38794e test: Introduce MockableSteadyClock::mock_time_point and ElapseSteady helper (MarcoFalke)
faf2d512c5 fuzz: Move global node id counter along with other global state (MarcoFalke)
fa98455e4b fuzz: Set ignore_incoming_txs in p2p_headers_presync (MarcoFalke)
faf2e238fb fuzz: Shuffle files before testing them (MarcoFalke)
Pull request description:
This should make the `p2p_headers_presync` fuzz target more deterministic.
Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018.
The first commits adds an `ElapseSteady` helper and type aliases. The second commit uses those helpers in `ReportHeadersPresync` and in the fuzz target to increase determinism.
### Testing
It can be tested via (setting 32 parallel threads):
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ p2p_headers_presync 32
```
The failing diff is contained in the commit messages, if applicable.
ACKs for top commit:
Crypt-iQ:
tACK faa3ce3199
janb84:
Re-ACK [faa3ce3](faa3ce3199)
marcofleon:
ACK faa3ce3199
Tree-SHA512: 7e2e0ddf3b4e818300373d6906384df57a87f1eeb507fa43de1ba88cf03c8e6752a26b6e91bfb3ee26a21efcaf1d0d9eaf70d311d1637b671965ef4cb96e6b59
868816d962 refactor: Remove SetHexDeprecated (marcofleon)
6b63218ec2 qt: Update SetHexDeprecated to FromHex (marcofleon)
Pull request description:
This is part of https://github.com/bitcoin/bitcoin/pull/32189. I'm separating this out because it's not immediately obvious that it's just a refactor. `SetHexDeprecated()` doesn't do any correctness checks on the input, while `FromHex()` does, so it's theoretically possible that there's a behavior change.
Replaces `uint256::SetHexDeprecated()` calls with `Txid::FromHex()` in four locations:
- `TransactionTableModel::updateTransaction`
- `TransactionView::contextualMenu`
- `TransactionView::abandonTx`
- `TransactionView::bumpFee`
The input strings in these cases aren't user input, so they should only be valid hex strings from `GetHex()` (through `TransactionRecord::getTxHash()`). These conversions should be safe without additional checks.
ACKs for top commit:
laanwj:
Code review ACK 868816d962
w0xlt:
Code review ACK 868816d962
BrandonOdiwuor:
Code Review ACK 868816d962
TheCharlatan:
ACK 868816d962
hebasto:
ACK 868816d962, I have reviewed the code and it looks OK.
Tree-SHA512: 121f149dcc7358231d0327cb3212ec96486a88410174d3c74ab8cbd61bad35185bc0a9740d534492b714811f72a6736bc7ac6eeae590c0ea1365c61cc791da37
a2bc330da8 feefrac test: avoid integer overflow (bugfix) (Pieter Wuille)
Pull request description:
The `feefrac_mul_div` fuzz test fails after #30535 with the following (base64) input: `Nb6Fc/97AACAAAD/ewAAgAAAAIAAAACAAAAAoA==` (see https://cirrus-ci.com/task/5240029192126464?logs=ci#L3353).
This is caused by an internal multiplication inside `CFeeRate` that *just* exceeds the limit of the `int64_t` type. Fix that by tightening the bounds slightly further.
ACKs for top commit:
sr-gi:
utACK a2bc330da8
instagibbs:
ACK a2bc330da8
glozow:
ACK a2bc330da8, was able to reproduce + verify this fix
Tree-SHA512: cfbcdc8becfd518f4349ddc00c9af3ed0a23bb9534af71cc21df167d7038e5967127e5d97c4b3e8aeff6bf071c4f630c32ffaf81d8ec227954d21fdcbe205333
This should avoid the remaining non-determistic code coverage paths.
Without this patch, the tool would report a diff (only when running
without libFuzzer):
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32
It should be sufficient to set it once. Especially, if the dynamic value
is only used by ResetAndInitialize.
This also avoids non-determistic code paths, when ResetAndInitialize may
re-initialize m_next_inv_to_inbounds.
Without this patch, the tool would report a diff:
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32
...
- 1126| 3| m_next_inv_to_inbounds = now + m_rng.rand_exp_duration(average_interval);
- 1127| 3| }
+ 1126| 10| m_next_inv_to_inbounds = now + m_rng.rand_exp_duration(average_interval);
+ 1127| 10| }
1128| 491| return m_next_inv_to_inbounds;
...
This allows the clock to be mockable in tests. Also, replace cs_main
with GetMutex() while touching this function.
Also, use the ElapseSteady test helper in the p2p_headers_presync fuzz
target to make it more deterministic.
The m_last_presync_update variable is a global that is not reset in
ResetAndInitialize. However, it is only used for logging, so completely
disable it for now.
Without this patch, the tool would report a diff:
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32
...
4468| 81| auto now = std::chrono::steady_clock::now();
4469| 81| if (now < m_last_presync_update + std::chrono::milliseconds{250}) return;
- ^80
+ ^79
...
This refactor clarifies that the MockableSteadyClock::mock_time_point
has millisecond precision by defining a type an using it.
Moreover, a ElapseSteady helper is added which can be re-used easily.
The global m_headers_presync_stats is not reset in ResetAndInitialize.
This may lead to non-determinism.
Fix it by incrementing the global node id counter instead.
Without this patch, the tool would report a diff:
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../qa-assets/fuzz_corpora/ p2p_headers_presync 32
...
2587| 3.73k| if (best_it == m_headers_presync_stats.end()) {
------------------
- | Branch (2587:17): [True: 80, False: 3.65k]
+ | Branch (2587:17): [True: 73, False: 3.66k]
------------------
...
When iterating over all fuzz input files in a folder, the order should
not matter.
However, shuffling may be useful to detect non-determinism.
Thus, shuffle in fuzz.cpp, when using neither libFuzzer, nor AFL.
Also, shuffle in the deterministic-fuzz-coverage tool, when using
libFuzzer.
Replace `uint256::SetHexDeprecated()` calls with `Txid::FromHex()`
in four locations:
- TransactionTableModel::updateTransaction
- TransactionView::contextualMenu
- TransactionView::abandonTx
- TransactionView::bumpFee
The input strings are generally expected to be valid hex strings
from `GetHex()`. However, due to the potentially unpredictable return
value of `.data(TransactionTableModel::TxHashRole)`, check the
`Txid::FromHex` result in `contextualMenu` and return early if the
transaction hash is invalid. The other two functions, `abandonTx`
and `bumpFee` will only be called if the context menu is enabled.
924f25f6fc bench: Match ConnectBlock tx output counts (monlovesmango)
Pull request description:
There turned out to be a mismatch in the tx output counts which caused 'ConnectBlockMixedEcdsaSchnorr' benchmark to run slower than 'ConnectBlockAllEcdsa' and 'ConnectBlockAllSchnorr'. This commit makes the tx output counts uniform across all benchmarks.
This commit also renames the 'taproot_tx' variable to 'tx' to reflect that this variable represents a general tx and not just a taproot tx.
ACKs for top commit:
davidgumberg:
Tested ACK 924f25f6fc
Prabhat1308:
reACK [`924f25f`](924f25f6fc)
janb84:
re ACK [924f25f](924f25f6fc)
josibake:
ACK 924f25f6fc
Tree-SHA512: bbf33e0c31b0c46571fd5d6ecd32426e7e823f9e156fd3d39a975bd5f0c1b6cd3dda55fa869cb0954c68dcf28cf4d0a0af40a72e440c1c78380b5b98e1eb6615
fac978fb21 test: Remove fragile and ancient release 0.17 wallet test (MarcoFalke)
Pull request description:
The test checks that the 0.17 wallet rejects wallet files created in "the future".
This is nice, and good to know. However,
* The 0.17 release is ancient and should be unused outside of tests, especially to load future wallets.
* The test intermittently fails, due to ancient RPC server bugs, that were fixed in the meantime. [1]
* Albeit they are not identical, the 0.18 release is still checked in this test, so any theoretical bug that would be caught by 0.17 is hopefully still caught by 0.18 as well.
So fix all issues by removing the test case.
[1] For example from https://api.cirrus-ci.com/v1/task/6161588714995712/logs/ci.log:
```
190/321 - [1mwallet_backwards_compatibility.py --descriptors[0m failed, Duration: 23 s
[17:21:40.700]
[17:21:40.700] [1mstdout:
[17:21:40.700] [0m2025-04-02T21:21:16.575000Z TestFramework (INFO): PRNG seed is: 5772716217847090743
[17:21:40.700] 2025-04-02T21:21:16.580000Z TestFramework (INFO): Initializing test directory /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20250402_210134/wallet_backwards_compatibility_134
[17:21:40.700] 2025-04-02T21:21:26.378000Z TestFramework (INFO): Test wallet backwards compatibility...
[17:21:40.700] 2025-04-02T21:21:33.191000Z TestFramework (INFO): Testing 0.19 addmultisigaddress case (#18075)
[17:21:40.700] 2025-04-02T21:21:33.637000Z TestFramework (INFO): Test that a wallet made on master can be opened on:
[17:21:40.700] 2025-04-02T21:21:33.637000Z TestFramework (INFO): - 250000
[17:21:40.700] 2025-04-02T21:21:34.055000Z TestFramework (INFO): - 240001
[17:21:40.700] 2025-04-02T21:21:34.435000Z TestFramework (INFO): - 230000
[17:21:40.700] 2025-04-02T21:21:34.858000Z TestFramework (INFO): - 220000
[17:21:40.700] 2025-04-02T21:21:35.614000Z TestFramework (INFO): - 210000
[17:21:40.700] 2025-04-02T21:21:35.707000Z TestFramework (INFO): Test descriptor wallet incompatibility on:
[17:21:40.700] 2025-04-02T21:21:35.707000Z TestFramework (INFO): - 200100
[17:21:40.700] 2025-04-02T21:21:35.878000Z TestFramework (INFO): - 190100
[17:21:40.700] 2025-04-02T21:21:36.021000Z TestFramework (INFO): - 180100
[17:21:40.700] 2025-04-02T21:21:36.319000Z TestFramework (INFO): Test descriptor wallet incompatibility with 0.17
[17:21:40.700] 2025-04-02T21:21:37.328000Z TestFramework (INFO): Test that 0.21 cannot open wallet containing tr() descriptors
[17:21:40.700] 2025-04-02T21:21:37.356000Z TestFramework (INFO): Test that a wallet can upgrade to and downgrade from master, from:
[17:21:40.700] 2025-04-02T21:21:37.361000Z TestFramework (INFO): - 250000
[17:21:40.700] 2025-04-02T21:21:37.665000Z TestFramework (INFO): - 240001
[17:21:40.700] 2025-04-02T21:21:37.970000Z TestFramework (INFO): - 230000
[17:21:40.700] 2025-04-02T21:21:38.439000Z TestFramework (INFO): - 220000
[17:21:40.700] 2025-04-02T21:21:38.793000Z TestFramework (INFO): - 210000
[17:21:40.700] 2025-04-02T21:21:39.470000Z TestFramework (INFO): Stopping nodes
[17:21:40.700]
[17:21:40.700]
[17:21:40.700] [1mstderr:
[17:21:40.700] [0mTraceback (most recent call last):
[17:21:40.700] File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_backwards_compatibility.py", line 389, in <module>
[17:21:40.700] BackwardsCompatibilityTest(__file__).main()
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 206, in main
[17:21:40.700] exit_code = self.shutdown()
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 379, in shutdown
[17:21:40.700] self.stop_nodes()
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 643, in stop_nodes
[17:21:40.700] node.stop_node(wait=wait, wait_until_stopped=False)
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/test_node.py", line 397, in stop_node
[17:21:40.700] self.stop()
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/coverage.py", line 50, in __call__
[17:21:40.700] return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/authproxy.py", line 132, in __call__
[17:21:40.700] response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/authproxy.py", line 106, in _request
[17:21:40.700] return self._get_response()
[17:21:40.700] File "/ci_container_base/test/functional/test_framework/authproxy.py", line 169, in _get_response
[17:21:40.700] http_response = self.__conn.getresponse()
[17:21:40.700] File "/usr/lib/python3.10/http/client.py", line 1375, in getresponse
[17:21:40.700] response.begin()
[17:21:40.700] File "/usr/lib/python3.10/http/client.py", line 318, in begin
[17:21:40.700] version, status, reason = self._read_status()
[17:21:40.700] File "/usr/lib/python3.10/http/client.py", line 287, in _read_status
[17:21:40.700] raise RemoteDisconnected("Remote end closed connection without"
[17:21:40.700] http.client.RemoteDisconnected: Remote end closed connection without response
[17:21:40.700] [node 10] Cleaning up leftover process
[17:21:40.700] [node 9] Cleaning up leftover process
[17:21:40.700] [node 8] Cleaning up leftover process
[17:21:40.700] [node 7] Cleaning up leftover process
[17:21:40.700] [node 6] Cleaning up leftover process
[17:21:40.700] [node 5] Cleaning up leftover process
[17:21:40.700] [node 4] Cleaning up leftover process
[17:21:40.700] [node 3] Cleaning up leftover process
[17:21:40.700] [node 2] Cleaning up leftover process
[17:21:40.700] [node 1] Cleaning up leftover process
[17:21:40.700] [node 0] Cleaning up leftover process
ACKs for top commit:
laanwj:
Code review ACK fac978fb21
janb84:
Re ACK [fac978f](fac978fb21)
pablomartin4btc:
re ACK fac978fb21
BrandonOdiwuor:
Code Review ACK fac978fb21
Tree-SHA512: 13acdfc6be4293a0ff45ae20b26ba60636e130097da380b7b51716faaa950320462399bef55e74b3cedc82944586dcc1bfd078babb96edb03c4efdb8f40af5a4
b639417b39 net: Add Tor extended SOCKS5 error codes (laanwj)
Pull request description:
Add support for reporting Tor extended SOCKS5 error codes as defined here:
- https://spec.torproject.org/socks-extensions.html#extended-error-codes
- https://gitlab.torproject.org/tpo/core/arti/-/blob/main/crates/tor-socksproto/src/msg.rs?ref_type=heads#L183
These give a more direct indication of the problem in case of errors connecting to hidden services, for example:
```
2025-04-02T10:34:13Z [net] Socks5() connect to [elided].onion:8333 failed: onion service descriptor can not be found
```
In the C Tor implementation, to get these one should set the "ExtendedErrors" flag on the "SocksPort" definition, introduced in version 0.4.3.1.
In Arti, extended error codes are always enabled.
Also, report the raw error code in case of unknown reply values.
ACKs for top commit:
1440000bytes:
utACK b639417b39
w0xlt:
utACK b639417b39
pablomartin4btc:
utACK b639417b39
Tree-SHA512: b30e65cb0f5c9183701373b0ee64cdec40680a3de1a1a365b006538c4d0b7ca8a047d7c6f81a7f5b8a36bae3a20b47a4c2a9850423c7034866e3837fa8fdbfe2
e419b0e17f refactor: Remove manual CDBBatch size estimation (Lőrinc)
8b5e19d8b5 refactor: Delegate to LevelDB for CDBBatch size estimation (Lőrinc)
751077c6e2 Coins: Add `kHeader` to `CDBBatch::size_estimate` (Lőrinc)
Pull request description:
### Summary
The manual batch size estimation of `CDBBatch` serialized size was [added](e66dbde6d1) when LevelDB [didn't expose this functionality yet](https://github.com/google/leveldb/commit/69e2bd2).
The PR refactors the logic to use the native `leveldb::WriteBatch::ApproximateSize()` function, structured in 3 focused commits to incrementally replace the old behavior safely.
### Context
The previous manual size calculation initialized the estimate to 0, instead of LevelDB's header size (containing an 8-byte sequence number followed by a 4-byte count).
This PR corrects that and transitions to the now-available native LevelDB function for improved accuracy and maintainability.
### Approach
The fix and refactor follow a strangle pattern over three commits:
* correct the initialization bug in the existing manual calculation, isolating the fix and ensuring the subsequent assertions use the corrected logic;
* introduce the native `ApproximateSize()` method alongside the corrected manual one, adding assertions to verify their equivalence at runtime;
* remove the verified manual calculation logic and assertions, leaving only the native method.
ACKs for top commit:
sipa:
utACK e419b0e17f
TheCharlatan:
ACK e419b0e17f
laanwj:
Code review ACK e419b0e17f
Tree-SHA512: a12b973dd480d4ffec4ec89a119bf0b6f73bde4e634329d6e4cc3454b867f2faf3742b78ec4a3b6d98ac4fb28fb2174f44ede42d6c701ed871987a7274560691
459807d566 test: remove strict restrictions on rpc_deprecated (Pol Espinasa)
Pull request description:
Removed the wallet restrictions for `rpc_deprecated.py` and added specific test case for the current deprecated rpc.
`skip_test_if_missing_module` will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related. This PR ensures that other tests not related to wallet can be ran and only this specific test will be skipped if there's no wallet
For more context check https://github.com/bitcoin/bitcoin/pull/31278#discussion_r2011661090
ACKs for top commit:
maflcko:
lgtm ACK 459807d566
rkrux:
ACK 459807d
Tree-SHA512: 922b0fafe8fb5bd88a677ce8be5c3fe2fdd4d0aadcd32cc11738a714cd6f765f07e7e7158c829f8338db0d46a15c030437a1ea09a3187c072bebebb4ca53ad85
