Avoid the need to construct a CBlockIndex object just to compute work for a header,
when its nBits value suffices for that.
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
Just don't call this function when it won't have any effect.
Note that we can't remove the LookupBlockIndex call, since `last_received_header`
is needed to check if new headers were received (`received_new_header`).
07a926474b5a6fa1d3d4656362a0117611f6da2f node: change a tx-relay on/off flag to enum (Vasil Dimov)
Pull request description:
Previously the `bool relay` argument to `BroadcastTransaction()` designated:
```
relay=true: add to the mempool and broadcast to all peers
relay=false: add to the mempool
```
Change this to an `enum`, so it is more readable and easier to extend with a 3rd option. Consider these example call sites:
```cpp
Paint(true);
// Or
Paint(/*is_red=*/true);
```
vs
```cpp
Paint(RED);
```
The idea for putting `TxBroadcastMethod` into `node/types.h` by Ryan.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not logically depend on the other commits from there.
ACKs for top commit:
optout21:
ACK 07a926474b5a6fa1d3d4656362a0117611f6da2f
kevkevinpal:
ACK [07a9264](07a926474b)
laanwj:
Concept and code review ACK 07a926474b5a6fa1d3d4656362a0117611f6da2f. Agree with the general reasoning and the change in #29415 is a valid motivation to change this interface.
glozow:
utACK 07a926474b5a6fa1d3d4656362a0117611f6da2f
Tree-SHA512: ec8f6fa56a6d2422a0fbd5941ff2792685e8d8e7b9dd50bba9f3e21ed9b4a4a26c89b0d7e4895d48f30b7a635f2eddd894af26b5266410952cbdaf5c40b42966
1a1f46c2285994908df9c11991c1f363c9733087 refactor/doc: Add blockman param to `GetTransaction` doc comment and reorder out param (Musa Haruna)
Pull request description:
Follow-up to [#27125](https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1190350876)
This PR addresses a minor documentation and style nit mentioned during review:
- Adds the missing `@param[in] blockman` line to the `GetTransaction()` doc comment.
- Moves the output parameter `hashBlock` to the end of both the function
declaration and definition, as suggested in the comment.
ACKs for top commit:
l0rinc:
ACK 1a1f46c2285994908df9c11991c1f363c9733087
maflcko:
re-lgtm-ut-cr-rfm-ACK 1a1f46c2285994908df9c11991c1f363c9733087
kevkevinpal:
reACK [1a1f46c](1a1f46c228)
Tree-SHA512: 5807a1ae6c383e691e948648dcb1e029620eaff3dcdff73d88c6dc268a7af5559a30c491b72f038b3f7e812e1845f4f063b49bd3671edfac1bb3a170c84be4f5
b6f8c48946cbfceb066de660c485ae1bd2c27cc1 coins: increase default `dbbatchsize` to 32 MiB (Lőrinc)
8bbb7b8bf8e3b2b6465f318ec102cc5275e5bf8c refactor: Extract default batch size into kernel (Lőrinc)
Pull request description:
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
When the in-memory UTXO set is flushed to LevelDB (after IBD or AssumeUTXO load), it does so in batches to manage memory usage during the flush.
A hidden `-dbbatchsize` config option exists to modify this value. This PR only changes the default from `16` MiB to `32` MiB.
Using a larger default reduces the overhead of many small writes and improves I/O efficiency (especially on HDDs). It may also help LevelDB optimize writes more effectively (e.g., via internal ordering).
The change is meant to speed up a critical part of IBD: dumping the accumulated work to disk.
### Context
The UTXO set has grown significantly since [2017](https://github.com/bitcoin/bitcoin/pull/10148/files#diff-d102b6032635ce90158c1e6e614f03b50e4449aa46ce23370da5387a658342fdR26-R27), when the original fixed 16 MiB batch size was chosen.
With the current multi-gigabyte UTXO set and the common practice of using larger `-dbcache` values, the fixed 16 MiB batch size leads to several inefficiencies:
* Flushing the entire UTXO set often requires thousands of separate 16 MiB write operations.
* Particularly on HDDs, the cumulative disk seek time and per-operation overhead from numerous small writes significantly slow down the flushing process.
* Each `WriteBatch` call incurs internal LevelDB overhead (e.g., MemTable handling, compaction triggering logic). More frequent, smaller batches amplify this cumulative overhead.
Flush times of 20-30 minutes are not uncommon, even on capable hardware.
### Considerations
As [noted by sipa](https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587500105), flushing involves a temporary memory usage increase as the batch is prepared. A larger batch size naturally leads to a larger peak during this phase. Crashing due to OOM during a flush is highly undesirable, but now that [#30611](https://github.com/bitcoin/bitcoin/pull/30611) is merged, the most we'd lose is the first hour of IBD.
Increasing the LevelDB write batch size from 16 to 32 MiB raised the measured peaks by ~70 MiB in my tests during UTXO dump. The option remains hidden, and users can always override it.
The increased peak memory usage (detailed below) is primarily attributed to LevelDB's `leveldb::Arena` (backing MemTables) and the temporary storage of serialized batch data (e.g., `std::string` in `CDBBatch::WriteImpl`).
Performance gains are most pronounced on systems with slower I/O (HDDs), but some SSDs also show measurable improvements.
### Measurements:
AssumeUTXO proxy, multiple runs with error bars (flushing time is faster that the measured loading + flushing):
* Raspberry Pi, dbcache=500: ~30% faster with 32 MiB vs 16 MiB, peak +~75 MiB and still < 1 GiB.
* i7 + HDD: results vary by dbcache, but 32 MiB usually beats 16 MiB and tracks close to 64 MiB without the larger peak.
* i9 + fast NVMe: roughly flat across 16/32/64 MiB. The goal here is to avoid regressions, which holds.
### Reproducer:
```bash
# Set up a clean demo environment
rm -rfd demo && mkdir -p demo
# Build Bitcoin Core
cmake -B build -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc)
# Start bitcoind with minimal settings without mempool and internet connection
build/bin/bitcoind -datadir=demo -stopatheight=1
build/bin/bitcoind -datadir=demo -blocksonly=1 -connect=0 -dbcache=3000 -daemon
# Load the AssumeUTXO snapshot, making sure the path is correct
# Expected output includes `"coins_loaded": 184821030`
build/bin/bitcoin-cli -datadir=demo -rpcclienttimeout=0 loadtxoutset ~/utxo-880000.dat
# Stop the daemon and verify snapshot flushes in the logs
build/bin/bitcoin-cli -datadir=demo stop
grep "FlushSnapshotToDisk: completed" demo/debug.log
```
---
This PR originally proposed 64 MiB, then a dynamic size, but both were dropped: 64 MiB increased peaks more than desired on low-RAM systems, and the dynamic variant underperformed across mixed hardware. 32 MiB is a simpler default that captures most of the gains with a modest peak increase.
For more details see: https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-3234329502
---
While the PR isn't about IBD in general, rather about a critical section of it, I have measured a reindex-chainstate until 900k blocks, showing a 1% overall speedup:
<details>
<summary>Details</summary>
```python
COMMITS="e6bfd95d5012fa1d91f83bf4122cb292afd6277f af653f321b135a59e38794b537737ed2f4a0040b"; \
STOP=900000; DBCACHE=10000; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done; echo "") && \
hyperfine \
--sort command \
--runs 1 \
--export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
--parameter-list COMMIT ${COMMITS// /,} \
--prepare "killall bitcoind 2>/dev/null; rm -f $DATA_DIR/debug.log; git checkout {COMMIT}; git clean -fxd; git reset --hard && \
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release && ninja -C build bitcoind && \
./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 10" \
--cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
"COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0"
e6bfd95d50 Merge bitcoin-core/gui#881: Move `FreespaceChecker` class into its own module
af653f321b coins: derive `batch_write_bytes` from `-dbcache` when unspecified
Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = e6bfd95d5012fa1d91f83bf4122cb292afd6277f)
Time (abs ≡): 25016.346 s [User: 30333.911 s, System: 826.463 s]
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = af653f321b135a59e38794b537737ed2f4a0040b)
Time (abs ≡): 24801.283 s [User: 30328.665 s, System: 834.110 s]
Relative speed comparison
1.01 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = e6bfd95d5012fa1d91f83bf4122cb292afd6277f)
1.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=10000 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = af653f321b135a59e38794b537737ed2f4a0040b)
```
</details>
ACKs for top commit:
laanwj:
Concept and code review ACK b6f8c48946cbfceb066de660c485ae1bd2c27cc1
TheCharlatan:
ACK b6f8c48946cbfceb066de660c485ae1bd2c27cc1
andrewtoth:
ACK b6f8c48946cbfceb066de660c485ae1bd2c27cc1
Tree-SHA512: a72008feca866e658f0cb4ebabbeee740f9fb13680e517b9d95eaa136e627a9dd5ee328456a2bf040401f4a1977ffa7446ad13f66b286b3419ff0c35095a3521
51093d6ae1d415b8dfa47f11cb634a87bd97e8a9 test: resolve symlinks in which result for capnp (David Gumberg)
Pull request description:
On Fedora, `/bin/` and `/usr/bin` are symlinked, and on one of my boxes (although I could not reproduce this behavior in a docker container), `/bin` comes before `/usr/bin` in `$PATH`, so `which capnp` reports `/bin/capnp`, and `capnp_dir` is set to `/include`, and the test fails:
```console
$ ./build/test/functional/interface_ipc.py
2025-10-30T20:43:43.753812Z TestFramework (INFO): PRNG seed is: 8370468257027235753
2025-10-30T20:43:43.754163Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_b9kjzj2a
terminate called after throwing an instance of 'kj::ExceptionImpl'
what(): mp/proxy.capnp:6: failed: Import failed: /capnp/c++.capnp
Aborted (core dumped)
```
This changes the functional test to resolve any symlinks in the `capnp` binary path reported by `which`.
ACKs for top commit:
TheCharlatan:
utACK 51093d6ae1d415b8dfa47f11cb634a87bd97e8a9
ryanofsky:
Code review ACK 51093d6ae1d415b8dfa47f11cb634a87bd97e8a9
Tree-SHA512: 17a3e16c3ef50d19e65c18bd12636f287b41e54fc14629e2eb6efb8f9532af7e0e0d404e4e234eeba92473b7ae18d97144a953d28523670308e78e4c4fbb7137
78d4d36730d44de93690b43f900a9202517291f6 test: Format strings in `*.rs` (rustaceanrob)
Pull request description:
`format!` strings may contain variables within the string representation. This is a lint as of a recent `rustc` nightly version.
ACKs for top commit:
maflcko:
lgtm ACK 78d4d36730d44de93690b43f900a9202517291f6
TheCharlatan:
ACK 78d4d36730d44de93690b43f900a9202517291f6
rkrux:
crACK 78d4d36730d44de93690b43f900a9202517291f6
Tree-SHA512: d6da94682dfa35964be4d7bba323847bae040dcec921e3d4ee2f25400751fa3af40fafe27805c2d6587d00a8ff54cc6af22ca46bf8911f13a200e73e77daa019
facf8b771a192ba17c8c4f9c43f248d83b3c8015 ci: Add missing python3-dev package for riscv64 (MarcoFalke)
Pull request description:
This is required to compile the pip wheels on native riscv64.
ACKs for top commit:
fanquake:
ACK facf8b771a192ba17c8c4f9c43f248d83b3c8015
Tree-SHA512: 7305deda4f2a7c2be5a82f4fcbc110f20a154374d98442e56d50175edda7f37a68b8e4cc1d84fc1fbc69ec1cc28559bbe795cc553fae8bd2e5effc36b0e534a2
fa4b52bd16189d40761c5976b8427e30779aba23 fuzz: refactor memcpy to std::ranges::copy to work around ubsan warn (MarcoFalke)
Pull request description:
Using std::ranges::copy from the C++ standard library has a few benefits here:
* It has the additional benefit of being a bit more type safe and document the byte cast explicitly.
* The compiler will likely optimize it to the same asm, but performance doesn't really matter here anyway.
* It has defined semantics for empty source ranges.
Fixes https://github.com/bitcoin/bitcoin/issues/33643
ACKs for top commit:
marcofleon:
tACK fa4b52bd16189d40761c5976b8427e30779aba23
dergoegge:
utACK fa4b52bd16189d40761c5976b8427e30779aba23
Tree-SHA512: 04fcf096e3cfc526e996c9313ec6e0a4d12c382fa19cb846b51564d33de2f0ef78a588fc6a936da0c76ca8bc9d9db4a824c36d99413db4f538a98239864d48f0
66667d6512294fd5dd02161b7c68c19af0865865 test: Use same rpc timeout for authproxy and cli (MarcoFalke)
Pull request description:
It seems odd to use different timeouts (and timeout factors) depending on whether the Python RPC proxy is used, or the bitcoin rpc command line interface.
Fix it by using the same timeout.
This can be tested by introducing a timeout error and checking it happens with and without `--usecli` after the exact same time.
Example timeout error:
```diff
diff --git a/test/functional/mining_template_verification.py b/test/functional/mining_template_verification.py
index de0833c596..e0f93a2b1e 100755
--- a/test/functional/mining_template_verification.py
+++ b/test/functional/mining_template_verification.py
@@ -173,7 +173,7 @@ class MiningTemplateVerificationTest(BitcoinTestFramework):
self.log.info("Submitting this block should succeed")
assert_equal(node.submitblock(block.serialize().hex()), None)
- node.waitforblockheight(2)
+ node.waitforblockheight(200000)
def transaction_test(self, node, block_0_height, tx):
self.log.info("make block template with a transaction")
```
Example cmd: `./bld-cmake/test/functional/mining_template_verification.py --timeout-factor=0.1 --usecli`.
ACKs for top commit:
brunoerg:
ACK 66667d6512294fd5dd02161b7c68c19af0865865
stickies-v:
tACK 66667d6512294fd5dd02161b7c68c19af0865865
Tree-SHA512: c8c21d8b9fb60ab192e3bbd45b317b96a40e10bf03704148613ac3cbdaae4abc2c03c4afbd504309ea0958201267c0d2a4bc5b40aa020917175c47e080ffe292
5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded) (Sebastian Falbesoner)
Pull request description:
Currently in our tests, all ECDSA signatures passing verification have sizes of 69 bytes and above (that's the DER-encoded size, i.e. counted without the sighash flag byte) [1]. This PR adds test coverage for the minimum-sized valid case of 8 bytes, by taking an interesting testnet transaction that I stumbled upon:
https://mempool.space/testnet/tx/c6c232a36395fa338da458b86ff1327395a9afc28c5d2daa4273e410089fd433
Note that this is a very obscure construction that only works because the public key used isn't contained in the locking script, but calculated and provided later at spending time (see https://bitcointalk.org/index.php?topic=1729534.msg17309060#msg17309060 for an explainer), to match the message (sighash) and picked signature. So this doesn't represent a use-case that really makes sense in practice, but it can still appear in a block (not in mempool though, due to `SCRIPT_VERIFY_CONST_SCRIPTCODE`), and having test-coverage seems useful.
Can be tested with same patch below (tests crash with the condition `>= 9`, but pass with `>= 8`).
[1] this can be verified by applying the following patch and running the tests:
```diff
diff --git a/src/pubkey.cpp b/src/pubkey.cpp
index a4ca9a170a..bee0caa603 100644
--- a/src/pubkey.cpp
+++ b/src/pubkey.cpp
@@ -288,7 +288,9 @@ bool CPubKey::Verify(const uint256 &hash, const std::vector<unsigned char>& vchS
/* libsecp256k1's ECDSA verification requires lower-S signatures, which have
* not historically been enforced in Bitcoin, so normalize them first. */
secp256k1_ecdsa_signature_normalize(secp256k1_context_static, &sig, &sig);
- return secp256k1_ecdsa_verify(secp256k1_context_static, &sig, hash.begin(), &pubkey);
+ bool ret = secp256k1_ecdsa_verify(secp256k1_context_static, &sig, hash.begin(), &pubkey);
+ if (ret) assert(vchSig.size() >= 69);
+ return ret;
}
```
ACKs for top commit:
ajtowns:
ACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 lgtm
fjahr:
tACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777
real-or-random:
utACK 5fa81e239a interesting case
Tree-SHA512: d1f0612fdb71c9238ca0420f574f6f246e60dbd11970b23f21d082c759a89ff98a13b12a1f6266f14f20539ec437b7ab79322082278da32984ddfee2d8893356
Using std::ranges::copy from the C++ standard library has a few benefits
here:
* It has the additional benefit of being a bit more type safe and
document the byte cast explicitly.
* The compiler will likely optimize it to the same asm, but performance
doesn't really matter here anyway.
* It works around an UB-Sanitizer bug, when the source range is empty.
Fixes https://github.com/bitcoin/bitcoin/issues/33643
fa0fa0f70087d08fe5a54832b96799bd14293279 refactor: Revert "disable self-assign warning for tests" (MarcoFalke)
faed118fb30fbc303e9d4c70569abfee397f1759 build: Bump clang minimum supported version to 17 (MarcoFalke)
Pull request description:
Most supported operating systems ship with clang-17 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.
(Apart from dropping the small workaround, this bump allows the `ci_native_nowallet_libbitcoinkernel` CI to run on riscv64 without running into an ICE with clang-16.)
This patch will only be released in version 31.x, next year (2026).
For reference:
* https://packages.debian.org/bookworm/clang-19
* https://packages.ubuntu.com/noble/clang (clang-18)
* CentOS-like 8/9/10 ship clang-17 (and later) via Stream
* FreeBSD 12/13 ship clang-17 (and later) via packages
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (clang21); No idea about OpenSuse Leap
On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:
* https://packages.debian.org/bookworm/g++ (g++-12)
* https://packages.ubuntu.com/jammy/g++ (g++-11)
* https://apt.llvm.org/, or nix, or guix, or compile clang from source, ...
*Ubuntu 22.04 LTS does not ship with clang-16 (the previous minimum required), nor with clang-17, so one of the above workarounds is needed there.*
macOS 14 is unaffected, and the previous minimum requirement of Xcode15.0 remains, see also 919e6d01e9/depends/hosts/darwin.mk (L3-L4). (Modulo compiling the fuzz tests, which requires 919e6d01e9/.github/workflows/ci.yml (L149))
ACKs for top commit:
janb84:
Concept ACK fa0fa0f70087d08fe5a54832b96799bd14293279
l0rinc:
Code review ACK fa0fa0f70087d08fe5a54832b96799bd14293279
hebasto:
ACK fa0fa0f70087d08fe5a54832b96799bd14293279.
Tree-SHA512: 5973cec39982f80b8b43e493cde012d9d1ab75a0362300b007d155db9f871c6341e7e209e5e63f0c3ca490136b684683de270136d62cb56f6b00b0ac0331dc36
5555bce994b648f836c35a02570f22ae9ad36da3 ci: Document why IN_GETOPT_BIN env var is needed on macOS (MarcoFalke)
fabe516440c96bb7a6a6902195684d3802d64139 ci: Export the container id in python script (MarcoFalke)
fa6aa9f42faac78aefee98af3a536ae7113ab61e ci: Retry image building once on failure (MarcoFalke)
fa4dbe04d7824f58a0083b07e86912d33efc9f7e ci: Allow overwriting check option in run() helper (MarcoFalke)
fa8e4de5c31dc7dceb3f12143aab0d1c46cdb080 ci: Use os.environ[key] access when value must be set (MarcoFalke)
Pull request description:
This should fix https://github.com/bitcoin/bitcoin/issues/33640.
It also contains a few refactor cleanups, which are explained in the corresponding commits.
ACKs for top commit:
l0rinc:
Code review reACK 5555bce994b648f836c35a02570f22ae9ad36da3
kevkevinpal:
ACK [5555bce](5555bce994)
davidgumberg:
crACK 5555bce994
Tree-SHA512: f1ea95b0650e57d6a9f97c575a11ee461832c0715c3d1a24dbfe12ccc5366f295639d4c4827f1d01da460ddf00917ecaa627e7dbd12e405770db6c53c3778a9c
53b34c80c631ee3f5ae652315592924f6935e0f1 ci: use pycapnp 2.2.1 in mac native job (fanquake)
865432869c0d20482d2869abef4d0ac6aaf4deb0 ci: remove Python version comment from mac config (fanquake)
Pull request description:
Switch to using v2.2.1 in the mac native job. Remove the git clone & install step.
ACKs for top commit:
maflcko:
lgtm ACK 53b34c80c631ee3f5ae652315592924f6935e0f1
l0rinc:
crACK 53b34c80c631ee3f5ae652315592924f6935e0f1
hebasto:
ACK 53b34c80c631ee3f5ae652315592924f6935e0f1.
Tree-SHA512: e756694c14431aacb3e48104331da88285c7500b4c4599c698f50d721d428ffe61258be075ef526b93c15aa3331f38535ca95249a2ef3ebfc804f61479095d9b
53e4951a5b5b9d166d278db4240513d09b447f58 Switch to ANSI Windows API in `fsbridge::fopen()` function (Hennadii Stepanov)
dbe770d9210666a366f055d52b9f34fa8a3d7305 Switch to ANSI Windows API in `Win32ErrorString()` function (Hennadii Stepanov)
06d0be4e22cef08fd7517f42ee82a44475c6363b Remove no longer necessary `WinCmdLineArgs` class (Hennadii Stepanov)
f366408492f6205ee20fe23e5104813de45dd4b1 cmake: Set process code page to UTF-8 on Windows (Hennadii Stepanov)
dccbb178065f05810a0fad57a86bca2f10995ecf Set minimum supported Windows version to 1903 (May 2019 Update) (Hennadii Stepanov)
Pull request description:
The main goal is to remove [deprecated](https://github.com/bitcoin/bitcoin/issues/32361) code (removed in C++26).
This PR employs Microsoft's modern [approach](https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page) to handling UTF-8:
> Until recently, Windows has emphasized "Unicode" -W variants over -A APIs. However, recent releases have used the ANSI code page and -A APIs as a means to introduce UTF-8 support to apps. If the ANSI code page is configured for UTF-8, then -A APIs typically operate in UTF-8. This model has the benefit of supporting existing code built with -A APIs without any code changes.
TODO:
- [x] Handle application manifests properly when building with MSVC.
- [x] Bump the minimum supported Windows version to 1903 (May 2019 Update).
- [x] Remove all remaining use cases of the deprecated `std:wstring_convert`.
- The instance in `subprocess.h` will be addressed in a follow-up PR, as additional tests are likely needed.
- The usage in `common/system.cpp` is handled in https://github.com/bitcoin/bitcoin/pull/32566.
Resolves partially https://github.com/bitcoin/bitcoin/issues/32361.
ACKs for top commit:
laanwj:
re-ACK 53e4951a5b5b9d166d278db4240513d09b447f58
hodlinator:
re-ACK 53e4951a5b5b9d166d278db4240513d09b447f58
davidgumberg:
untested crACK 53e4951a5b
Tree-SHA512: 0dbe9badca8b979ac2b4814fea6e4a7e53c423a1c96cb76ce894253137d3640a87631a5b22b9645e8f0c2a36a107122eb19ed8e92978c17384ffa8b9ab9993b5
57f7c68821d96cc096db624cb06b7a252d038300 test: add functional test for `TestShell` (matching doc example) (Sebastian Falbesoner)
53874f7934d50f68ea46df68b2bcd11bad834730 doc: test: update TestShell example instructions/options (Sebastian Falbesoner)
Pull request description:
This PR adds a functional framework test for the `TestShell` class. The primary motivation for this is to avoid that the example instructions for the interactive Python shell in `test-shell.md` get outdated or broken without noticing, a problem we had already several times in the past (see #26520, #27906, #31415). Having a copy is still not perfect, as docs and functional test have to be kept in sync, but I don't expect this to be a problem in practice, assuming the hint in the functional test comment is hopefully noticed if changes are made.
Alternatively, the example instructions in `test-shell.md` could be removed with a hint to the functional test (I tend to prefer to keep the docs as-is though, showing the full REPL interaction).
The first commit contain two small removal fix-ups in `test-shell.md` regarding the result of the `createwallet` RPC call and the mentioning of the `noshutdown` option that was removed earlier (see #31620). Could be backported to v30.
ACKs for top commit:
brunoerg:
ACK 57f7c68821d96cc096db624cb06b7a252d038300
stratospher:
ACK 57f7c68.
pinheadmz:
ACK 57f7c68821d96cc096db624cb06b7a252d038300
Tree-SHA512: 25c35af46b742bbefce7838708437529bbf613fa3d1f0fba590cacef0acdde82b3a78c7a01459c73eaac26ce3f1041e54092d098f0fc94a8c76ac0b4f4970338
1a7fb5eeeef3575c1e7c27915c9b98695191299d fees: return current block height in estimateSmartFee (ismaelsadeeq)
ab49480d9be4e54aa9db1247b8499f957ba9d166 fees: rename fees_args to block_policy_estimator_args (ismaelsadeeq)
06db08a43568910702207a7963b375e1a7446689 fees: refactor: rename fees to block_policy_estimator (ismaelsadeeq)
6dfdd7e034dd3620f0f8ed54dfe20fa407b5382f fees: refactor: rename policy_fee_tests.cpp to feerounder_tests.cpp (ismaelsadeeq)
Pull request description:
This PR is a simple refactoring that does four things:
1. Renames `test/policy_fee_tests.cpp` to `test/feerounder_tests.cpp`.
2. Renames `policy/fees.{h,cpp}` to `policy/fees/block_policy_estimator.{h,cpp}`.
3. Renames `policy/fees_args.cpp` to `policy/fees/block_policy_estimator_args.cpp`.
4. Modifies `estimateSmartFee` to return the block height at which the estimate was made by adding a `best_height` unsigned int value to the `FeeCalculation` struct.
**Motivation**
In preparation for adding a new fee estimator, the `fees` directory is created so we can organize code into `block_policy_estimator` and `mempool` because
a) It would be clunky to add more code directly under `fees`.
b) Having `policy/fees.{h,cpp}` and `policy/mempool.{h,cpp}` would also be undesirable.
Therefore, it makes sense to structure the it as `policy/fees/block_policy_estimator`, `policy/fees/mempool`, etc.
Hence test file were also updated accordingly.
The current block height is also returned because later in #30157 we log the height at which each estimate is made (at the debug log category of fee estimation :) ). This feature is particularly useful for empirical data analysis.
ACKs for top commit:
maflcko:
re-ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d 🐤
polespinasa:
re ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d
willcl-ark:
ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d
janb84:
re ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d
Tree-SHA512: fef7ace2a9f262ec0361fb7a46df5108afc46b5c4b059caadf2fd114740aefbb2592389d11646c13d0e28bf0ef2cfcfbab3e659c4d4288b8ebe64725fd1963c0
944e5ff848f656d2ee6202b2330f3ae178ad0fbe doc: mention key removal in rpc interface modification (rkrux)
Pull request description:
A discussion in a previous PR 32618 prompted me to add this note: https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2181951390
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
maflcko:
lgtm ACK 944e5ff848f656d2ee6202b2330f3ae178ad0fbe
stickies-v:
ACK 944e5ff848f656d2ee6202b2330f3ae178ad0fbe
glozow:
lgtm ACK 944e5ff848f656d2ee6202b2330f3ae178ad0fbe
Tree-SHA512: f64c086c99e7c73a3ae7d60b2e8e06c8e7a3a49305a66d5c5a96db9b4ebbd01928ab5ccbcbdac26f400d16662f84469c448625e1f55ec2a9a920eff8a05fc379
02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d ci, iwyu: Treat warnings as errors for specific directories (Hennadii Stepanov)
57a3eac387bd26689aed7682b248b648dba42779 refactor: Fix includes in `index` directory (Hennadii Stepanov)
bdb8eadcdc193f398ebad83911d3297b5257e721 refactor: Fix includes in `crypto` directory (Hennadii Stepanov)
56f2a689a2016ba2ae9cc40833447dff648af809 ci: Do not patch `leveldb` to workaround UB in "tidy" CI job (Hennadii Stepanov)
Pull request description:
This PR is the first step towards treating IWYU warnings as errors. At this stage, it applies only to the `crypto` and `index` directories.
ACKs for top commit:
maflcko:
re-ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d 💮
ryanofsky:
Code review ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d. Just rebased and update tidy patch comment again since last review
willcl-ark:
ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d
Tree-SHA512: 1c966e01c47bf3e7d225faa3b819367f757430e2d71e9582fa82d67307aabe3f0d76f69346ee180192e7f5ab194ecc58d2b8ecf178eab26ba3309a6b55bff4b6
59c4898994bde3d86168075f0031c9d5a9ac5c8f guix: remove python-pydantic-core input from LIEF (fanquake)
9f2a6927d3a9fc1ac536f8fb24a89582e39f24d6 guix: use Clang & LLVM 19 for macOS build (fanquake)
9570ddbec9cb20c268f78ff5e581a65e00864773 guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2 (fanquake)
7b5cc276aa0a7aeea7e535b0fd30a0b6811000d9 guix: patch around riscv issue with newer (2.40+) binutils (fanquake)
91b5cbaabbca49a8bd9df6da2506070b31482892 ci: use Debian Trixie for macOS cross job (fanquake)
Pull request description:
5cb84f2013 isn't super recent, but it's enough to get access to some newer packages, such as LLVM 19, and avoids having to add any further work arounds for things that we know are fixed later (i.e nsis). Once things upstream have stabilized a bit more (the `core-updates` branch was fairly recently merged), we could look at bumping to something newer.
Package updates:
(base) glibc 2.35 -> 2.39
binutils 2.38 -> 2.41
diffutils 3.8 -> 3.10
gawk 5.2.1 -> 5.3.0
git-minimal 2.45.2 -> 2.46.0
grep 3.8 -> 3.11
gzip 1.12 -> 1.13
linux-headers 6.1.106 -> 6.1.119
make 4.3 -> 4.4.1
xz 5.2.8 -> 5.4.5
CMake 3.30 becomes available.
Clang/LLVM 19 becomes available.
Could be used for #32764.
ACKs for top commit:
hebasto:
re-ACK 59c4898994bde3d86168075f0031c9d5a9ac5c8f.
willcl-ark:
ACK 59c4898994bde3d86168075f0031c9d5a9ac5c8f
Tree-SHA512: c44965d5a315e4c862f5e40d8e98c645713405fec72a61055f95b6c68b7d2dcc69a61a084e397a4556d4c1df18f1cfa7a905234643fe4a7df9c58d486e26c097
664657ed134365588914c2cf6a3975ce368a4f49 bugfix: disallow label for ranged descriptors & allow external non-ranged descriptors to have label (scgbckbone)
Pull request description:
Motivation:
* ranged descriptors MUST not be able to have label (current impl allows it)
* external non-ranged descriptor MUST be able to have label (current impl disallows it, **if** `internal=false` is provided via importdescriptor user data)
Repro steps:
* create blank wallet and import descriptors
* external has `label=test` (not internal)
```
conn = bitcoind.create_wallet(wallet_name=w_name, disable_private_keys=True, blank=True,
passphrase=None, avoid_reuse=False, descriptors=True)
descriptors = [
{
"timestamp": "now",
"label": "test",
"active": True,
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/0/*)#erexmnep",
"internal": False
},
{
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/1/*)#ghu8xxfe",
"active": True,
"internal": True,
"timestamp": "now"
},
]
r = conn.importdescriptors(descriptors)
print(r)
```
response:
```
[{'error': {'code': -8,
'message': 'Internal addresses should not have a label'},
'success': False,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
But in above, ONLY external has a label.
If you remove `internal: False` from external descriptor import object - it will import no problem:
```
[{'success': True,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
Even tho it should NOT, as the descriptor is ranged. Current implementation relies on checking user provided data to decide whether desc is ranged.
ACKs for top commit:
achow101:
ACK 664657ed134365588914c2cf6a3975ce368a4f49
rkrux:
lgtm crACK 664657ed134365588914c2cf6a3975ce368a4f49
Tree-SHA512: 9e70aea620019c29950ba417d4ae38d65cd94a4f6fcabbc021d67b031de1c44c27d6f6f5cb7e6950a099eb6e58bed9be764d4c6347195daeccb14a5d95c123b2
58e55b17e632dbd4425dd64825b087f242ac4b7b test: descriptor: cover invalid multi/multi_a cases (brunoerg)
Pull request description:
This PR adds test coverage for invalid `multi()` and `multi_a()` cases, see:
1. 53eb5593f0/src/script/descriptor.cpp (L1819-L1821)
2. 53eb5593f0/src/script/descriptor.cpp (L1835-L1837)
3. 53eb5593f0/src/script/descriptor.cpp (L1838-L1840)
We could also exercise to exceed the number of keys - 20 for `multi` and 999 for `multi_a`.
ACKs for top commit:
maflcko:
lgtm ACK 58e55b17e632dbd4425dd64825b087f242ac4b7b
darosior:
utACK 58e55b17e632dbd4425dd64825b087f242ac4b7b
glozow:
ACK 58e55b17e632dbd4425dd64825b087f242ac4b7b
Tree-SHA512: 0983e9c70e4bef13fa21b2e22e17c2e86eda0950f6271a42b24b91eef22c3277659a862a78bd511c9e14c92859070b3bf2968cfa24de0a1397de1f824946c757
0465574c127907df9b764055a585e8281bae8d1d test: Fixes send_blocks_and_test docs (Sergi Delgado Segura)
09c95f21e71d196120e6c9d0b1d1923a4927408d test: Adds block tiebreak over restarts tests (Sergi Delgado Segura)
18524b072e6bdd590a9f6badd15d897b5ef5ce54 Make nSequenceId init value constants (Sergi Delgado Segura)
8b91883a23aac64a37d929eeae81325e221d177d Set the same best tip on restart if two candidates have the same work (Sergi Delgado Segura)
5370bed21e0b04feca6ec09738ecbe792095a338 test: add functional test for complex reorgs (Pieter Wuille)
ab145cb3b471d07a2e8ee79edde46ec67f47d580 Updates CBlockIndexWorkComparator outdated comment (Sergi Delgado Segura)
Pull request description:
This PR grabs some interesting bits from https://github.com/bitcoin/bitcoin/pull/29284 and fixes some edge cases in how block tiebreaks are dealt with.
## Regarding #29284
The main functionality from the PR was dropped given it was not an issue anymore, however, reviewers pointed out some comments were outdated https://github.com/bitcoin/bitcoin/pull/29284#discussion_r1522023578 (which to my understanding may have led to thinking that there was still an issue) it also added test coverage for the aforementioned case which was already passing on master and is useful to keep.
## New functionality
While reviewing the superseded PR, it was noticed that blocks that are loaded from disk may face a similar issue (check https://github.com/bitcoin/bitcoin/pull/29284#issuecomment-1994317785 for more context).
The issue comes from how tiebreaks for equal work blocks are handled: if two blocks have the same amount of work, the one that is activatable first wins, that is, the one for which we have all its data (and all of its ancestors'). The variable that keeps track of this, within `CBlockIndex` is `nSequenceId`, which is not persisted over restarts. This means that when a node is restarted, all blocks loaded from disk are defaulted the same `nSequenceId`: 0.
Now, when trying to decide what chain is best on loading blocks from disk, the previous tiebreaker rule is not decisive anymore, so the `CBlockIndexWorkComparator` has to default to its last rule: whatever block is loaded first (has a smaller memory address).
This means that if multiple same work tip candidates were available before restarting the node, it could be the case that the selected chain tip after restarting does not match the one before.
Therefore, the way `nSequenceId` is initialized is changed to:
- 0 for blocks that belong to the previously known best chain
- 1 to all other blocks loaded from disk
ACKs for top commit:
sipa:
utACK 0465574c127907df9b764055a585e8281bae8d1d
TheCharlatan:
ACK 0465574c127907df9b764055a585e8281bae8d1d
furszy:
Tested ACK 0465574c127907df9b764055a585e8281bae8d1d.
Tree-SHA512: 161da814da03ce10c34d27d79a315460a9c98d019b85ee35bc5daa991ed3b6a2e69a829e421fc70d093a83cf7a2e403763041e594df39ed1991445e54c16532a
51877f2fc5eb02b4229258b4b43731c4da843793 test: Update BIP324 test vectors (Tim Ruffing)
Pull request description:
This updates the hardcoded test vectors from BIP324. The test vectors had to be regenerated (in the aux files of the BIP) because there was a bug in the script used for generating them (https://github.com/bitcoin/bips/pull/2016).
ACKs for top commit:
jonatack:
ACK 51877f2fc5eb02b4229258b4b43731c4da843793
theStack:
ACK 51877f2fc5eb02b4229258b4b43731c4da843793
Tree-SHA512: 59f4075e286067b11fce98667c860f3083b6cca8a2e49da8783ccdce8e32c34fd3e1943191d24dcf5bb68d8a2540726d99f7c29e8b0f104032ccb82423ca8d82
- Also move them to policy/fees/ and update includes
- Note: the block_policy_estimator_args.h include in block_policy_estimator_args.cpp was done manually.
13f36c020f0329b5e975282b45292fdf2a495e31 clang-format: regenerate configs (Lőrinc)
Pull request description:
Updates `.clang-format` file to reflect [latest supported Clang-Format standards](https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html) while preserving most of the existing formatting behavior.
Note that [`AfterStruct` brace placement](https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072678126) was originally aligned here with `AfterClass`, but was reverted by reviewer demand.
ACKs for top commit:
maflcko:
re-ACK 13f36c020f0329b5e975282b45292fdf2a495e31 🖼
achow101:
ACK 13f36c020f0329b5e975282b45292fdf2a495e31
hodlinator:
re-ACK 13f36c020f0329b5e975282b45292fdf2a495e31
Tree-SHA512: 02bd9d8a22a9af268297aeddd1f2b2cce079fddd0e1f764d6e9650bb614cb7bcfbd20b38d6e4e5db1744b3dd1ba540380010c085f2cbc0be8aa936f21d27d8de
5ded99a7f007b142f6b0ec89e0c71ef281b42684 fuzz: MockMempoolMinFee in wallet_fees (brunoerg)
c9a7a198d9e81e99de99a2aaff1687d13d6674e8 test: move MockMempoolMinFee to util/txmempool (brunoerg)
adf67eb21baf39a222b65480e45ae76f093e8f66 fuzz: create FeeEstimatorTestingSetup to set fee_estimator (brunoerg)
ff10a37e99271125a9ece92bae571f7b78fb9e22 fuzz: mock CBlockPolicyEstimator in wallet_fuzz (brunoerg)
f591c3becafcdd7c81722c647865a1f908b6469a fees: make estimateSmartFee/HighestTargetTracked virtual for mocking (brunoerg)
19273d0705fcd2fbde686bc3b5b2375f691e303d fuzz: set mempool options in wallet_fees (brunoerg)
Pull request description:
Some functions in `wallet/fees.cpp` (fuzzed by the wallet_fees target) depends on some mempool stuff - e.g. relay current min fee, smart fee and max blocks estimation, relay dust fee and other ones. For better fuzzing of it, it would be great to have these values/interactions. That said, this PR enhances the `wallet_fees` target by:
- Setting mempool options - `min_relay_feerate`, `dust_relay_feerate` and `incremental_relay_feerate` - when creating the `CTxMemPool`.
- Creates a `ConsumeMempoolMinFee` function which is used to have a mempool min fee (similar approach from `MockMempoolMinFee` from unit test).
- Mock `CBlockPolicyEstimator` - estimateSmartFee/HighestTagretTracket functions, especifically. It's better to mock it then trying to interact to CBlockPolicyEstimator in order to have some effective values due to performance.
Note that I created `FeeEstimatorTestingSetup` because we cannot set `m_node.fee_estimator` in `ChainTestingSetup` since fae8c73d9e4eba4603447bb52b6e3e760fbf15f8.
ACKs for top commit:
maflcko:
re-ACK 5ded99a7f007b142f6b0ec89e0c71ef281b42684 🎯
ismaelsadeeq:
Code review ACK 5ded99a7f007b142f6b0ec89e0c71ef281b42684
Tree-SHA512: 13d2af042098afd237ef349437021ea841069d93d4c3e3a32e1b562c027d00c727f375426709d34421092993398caf7ba8ff19077982cb6f470f8938a44e7754
45bd8914658a675d00aa9c83373d6903a8a9ece8 log: split assumevalid ancestry-failure-reason message (Lőrinc)
6c13a38ab51caf1fa7502f746e33bbf86153a541 log: separate script verification reasons (Lőrinc)
f2ea6f04e79b6646b9320a910694a22c5520977d refactor: untangle assumevalid decision branches (Lőrinc)
9bc298556cb02cfa1382bbaa9e5638006b403576 validation: log initial script verification state (Lőrinc)
4fad4e992c49a532e3a8928965f242cb311eeb29 test: add assumevalid scenarios scaffold (Lőrinc)
91ac64b0a66fc792eabd0a9bb5bb22459c852c6d log: reword `signature validations` to `script verification` in `assumevalid` log (Lőrinc)
Pull request description:
### Summary
Users can encounter cases where script checks are unexpectedly enabled (e.g. after reindex, or when `assumevalid`/`minimumchainwork` gates fail). Without an explicit line, they must infer state from the absence of a message, which is incomplete and error-prone.
The existing "Assuming ancestors of block …" line does not reliably indicate whether script checks are actually enabled, which makes debugging/benchmarking confusing.
### What this changes
We make the initial **script-verification** state explicit and log **why** checks are enabled to avoid confusion.
* Always log the first script-verification state on startup, **before** the first `UpdateTip`.
* Flatten the nested `assumevalid` conditionals into a linear gating sequence for readability.
* Extend the functional test to assert the old behavior with the new reason strings.
This is a **logging-only** test change it shouldn't change any other behavior.
### Example output
The state (with reason) is logged at startup and whenever the reason changes, e.g.:
* `Disabling script verification at block #904336 (000000000000000000014106b2082b1a18aaf3091e8b337c6fed110db8c56620).`
* `Enabling script verification at block #912527 (000000000000000000010bb6aa3ecabd7d41738463b6c6621776c2e40dbe738a): block too recent relative to best header.`
* `Enabling script verification at block #912684 (00000000000000000001375cf7b90b2b86e559d05ed92ca764d376702ead3858): block height above assumevalid height.`
------
Follow-up to https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2329269037
ACKs for top commit:
Eunovo:
re-ACK 45bd891465
achow101:
ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
hodlinator:
re-ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
yuvicc:
ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
andrewtoth:
ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
ajtowns:
ACK 45bd8914658a675d00aa9c83373d6903a8a9ece8
Tree-SHA512: 58328d7c418a6fe18f1c7fe1dd31955bb6fce8b928b0df693f6200807932eb5933146300af886a80a1d922228d93faf531145186dae55ad4ad1f691970732eca
b63428ac9ce2c903670409b3e47b9f6730917ae8 rpc: refactor: use more (Maybe)Arg<std::string_view> (stickies-v)
037830ca0dbb6ede9f9d72691c756f4bae6c97e2 refactor: increase string_view usage (stickies-v)
b3bf18f0bac0ffe18206ee20642e11264ba0c99d rpc: refactor: use string_view in Arg/MaybeArg (stickies-v)
Pull request description:
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](d127b25199 (diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a32374R32))) with raw pointers being implemented.
This PR aims to improve on this by returning a trivially copyable `std::string_view` (`Arg`) or `std::optional<std::string_view>` (`MaybeArg`), modernizing the interface without introducing any additional copying overhead. In doing so, it also generalizes whether we return by value or by pointer/reference using `std::is_trivially_copyable_v` instead of defining the types manually.
In cases where functions currently take a `const std::string&` and it would be too much work / touching consensus logic to update them (`signmessage.cpp`), a `std::string` copy is made (which was already happening anyway).
The last 2 commits increase usage of the `{Arg,MaybeArg}<std::string_view>` helpers, and could be dropped/pruned if anything turns out to be controversial - I just think it's a nice little cleanup.
ACKs for top commit:
maflcko:
re-ACK b63428ac9ce2c903670409b3e47b9f6730917ae8 🎉
achow101:
ACK b63428ac9ce2c903670409b3e47b9f6730917ae8
pablomartin4btc:
re-ACK [b63428a](b63428ac9c)
w0xlt:
reACK b63428ac9c
Tree-SHA512: b4942c353a1658c22a88d8c9b402c288ad35265a3b88aa2072b1f9b6d921cd073194ed4b00b807cb48ca440f47c87ef3d8e0dd1a5d814be58fc7743f26288277
65a10fc3c52ea09a4794345bcf607dff908c783a p2p: add assertion for BlockTransactionsRequest indexes (frankomosh)
58be359f6b240528e4df23296dec65202f28a773 fuzz: add a target for DifferenceFormatter Class (frankomosh)
Pull request description:
Adds a fuzz test for the [`DifferenceFormatter`](e3f416dbf7/src/blockencodings.h (L22-L42)) (used in [`BlockTransactionsRequest`](https://github.com/bitcoin/bitcoin/blob/master/src/blockencodings.h#L44-L54), [BIP 152](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki)). The DifferenceFormatter class implements differential encoding for compact block transactions (BIP 152). This PR ensures that its strictly-monotonic property is maintained. It complements the tests in [`blocktransactionsrequest_deserialize`](9703b7e6d5/src/test/fuzz/deserialize.cpp (L314)).
Additionally, there's an added invariant check after GETBLOCKTXN deserialization in `net_processing.cpp`.
ACKs for top commit:
Crypt-iQ:
tACK 65a10fc3c52ea09a4794345bcf607dff908c783a
achow101:
ACK 65a10fc3c52ea09a4794345bcf607dff908c783a
dergoegge:
Code review ACK 65a10fc3c52ea09a4794345bcf607dff908c783a
Tree-SHA512: 70659cf045e99bb5f753763c7ddac094cb2883c202c899276cbe616889afa053b2d5e831f99d6386d4d1e4118cd35fa0b14b54667853fe067f6efe2eb77b4097
This was added in commit b705bade44973e61655d5f847f49d97fb5bb8393, but I
keep forgetting the background that this is needed for the retry Bash
script. So document it.
This refactor does not change behavior, but it has a few benefits:
* The shellcheck SC2086 warning is disabled for the whole command, but
is only needed for the CI_CONTAINER_CAP env var. So in Python, only
pass this one env var to shlex.split() for proper word splitting.
* Future logic improvements can be implemented in Python.
The comments are moved, which can be checked via the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
fa70e23de75baaf8c1ef6836ffe8ca73562c8937 ci: Drop libFuzzer from msan fuzz task (MarcoFalke)
Pull request description:
libFuzzer is mostly unmaintained (https://llvm.org/docs/LibFuzzer.html#status), and it isn't really needed by the CI tasks. While it provides some additional stats like rss or the max input byte size, they are not essential. Dropping libFuzzer here would also drop the "60 seconds sanity check" for empty folders, but I think this is an acceptable price to pay to silence false-positives that were hit for years.
Also, there seems to be a history of intermittent false-positive msan warnings (https://github.com/bitcoin/bitcoin/pull/33600#issuecomment-3391921802).
It is unclear what exactly is causing the false-positives, so just disable libFuzzer in this task for now, to work around them.
ACKs for top commit:
kevkevinpal:
ACK [fa70e23](fa70e23de7)
dergoegge:
ACK fa70e23de75baaf8c1ef6836ffe8ca73562c8937
Tree-SHA512: c3e5958b8378ba30f51d923f97a84dec2ee60af8b9c2a4f13bc8de486a490031468371120e421384aa198ffec591db554e636935ab3c6d4de5e870238f5079f2
fa37153288ca420420636046ef6b8c4ba7e5a478 util: Abort on failing CHECK_NONFATAL in debug builds (MarcoFalke)
fa0dc4bdffb06b6f0c192fe1aa02b4dfdcdc6e15 test: Allow testing of check failures (MarcoFalke)
faeb58fe668662d8262c4cc7c54ad2af756dbe3b refactor: Set G_ABORT_ON_FAILED_ASSUME when G_FUZZING_BUILD (MarcoFalke)
Pull request description:
A failing `CHECK_NONFATAL` will throw an exception. This is fine and even desired in production builds, because the program may catch the exception and give the user a way to easily report the bug upstream.
However, in debug development builds, exceptions for internal bugs are problematic:
* The exception could accidentally be caught and silently ignored
* The exception does not include a full stacktrace, possibly making debugging harder
Fix all issues by turning the exception into an abort in debug builds.
This can be tested by reverting the hunks to `src/rpc/node.cpp` and `test/functional/rpc_misc.py` and then running the functional or fuzz tests.
ACKs for top commit:
achow101:
ACK fa37153288ca420420636046ef6b8c4ba7e5a478
ryanofsky:
Code review ACK fa37153288ca420420636046ef6b8c4ba7e5a478, just catching subprocess.CalledProcessError in test fixing up a comment since last review
stickies-v:
ACK fa37153288ca420420636046ef6b8c4ba7e5a478
Tree-SHA512: 2d892b838ccef6f9b25a066e7c2f6cd6f5acc94aad1d91fce62308983bd3f5c5d724897a76de4e3cc5c3678ddadc87e2ee8c87362965373526038e598dfb0101
cc5dda1de333cf7aa10e2237ee2c9221f705dbd9 headerssync: Make HeadersSyncState more flexible and move constants (Hodlinator)
8fd1c2893e6768223069d8b2fdec033b026cb2eb test(headerssync): Test returning of pow_validated_headers behavior (Hodlinator)
7b00643ef5f932116ee303af9984312b27c040f1 test(headerssync): headers_sync_chainwork test improvements (Hodlinator)
04eeb9578c60ce5661f285f6bde996569fafdcc3 doc(test): Improve comments (Hodlinator)
fe896f8faa7883f33169fe3e6dddb91feaca23e1 refactor(test): Store HeadersSyncState on the stack (Hodlinator)
f03686892a9c07e87e6dd12027d988fe188b1f9e refactor(test): Break up headers_sync_state (Hodlinator)
e984618d0b9946dc11f1087adf22a4cfbf9c1a77 refactor(headerssync): Process spans of headers (Hodlinator)
a4ac9915a95eb865779cf4627dd518d94c01032b refactor(headerssync): Extract test constants ahead of breakup into functions (Hodlinator)
Pull request description:
### Background
As part of the release process we often run *contrib/devtools/headerssync-params.py* and increase the values of the constants `HEADER_COMMITMENT_PERIOD` and `REDOWNLOAD_BUFFER_SIZE` in *src/headerssync.cpp* as per *doc/release-process.md* (example: 11a2d3a63e90cdc1920ede3c67d52a9c72860e6b). This helps fine tune the memory consumption per `HeadersSyncState`-instance in the face of malicious peers.
(The `REDOWNLOAD_BUFFER_SIZE`/`HEADER_COMMITMENT_PERIOD` ratio determines how many Headers Sync commitment bits must match between PRESYNC & REDOWNLOAD phases before we start permanently storing headers from a peer. For more details see comments in *src/headerssync.h* and *contrib/devtools/headerssync-params.py*).
### Problem: Not feeding back headers until completing sync
During v30 release process #33274 made `REDOWNLOAD_BUFFER_SIZE` exceed the `target_blocks` constant used to control the length of chains generated for testing Headers Sync (`15000`, *headers_sync_chainwork_tests.cpp*).
The `HeadersSyncState::m_redownloaded_headers`-buffer now does not reach the `REDOWNLOAD_BUFFER_SIZE`-threshold during those unit tests. As a consequence `HeadersSyncState::PopHeadersReadyForAcceptance()` will not start feeding back headers until the PoW threshold has been met. While this will not cause the unit test to start failing on master, it means we have gone from testing behavior that resembles mainnet (way more than `REDOWNLOAD_BUFFER_SIZE` headers to reach the PoW limit), to behavior that is not possible/expected there.
### Solution
Avoid testing this unrealistic condition of completing Headers Sync before reaching `REDOWNLOAD_BUFFER_SIZE` by making tests able to define their own values through the new `HeadersSyncParams` instead of having them hard-coded for all chains & tests.
### Commits
* First 6 commits refactor and improve the unit tests in order to clarify latter changes.
* We then add checks for the behavior around the `REDOWNLOAD_BUFFER_SIZE` threshold.
* The main change: we extract the section from *headerssync.cpp* containing the constants to *kernel/chainparams.cpp*, making `HeadersSyncState` no longer hard-coded to mainnet.
### Notes
This PR used to be called "headerssync: Preempt unrealistic unit test behavior".
ACKs for top commit:
l0rinc:
reACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9
marcofleon:
code review ACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9
danielabrozzoni:
reACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9
Tree-SHA512: ccc824dcbbb8ad5ae98c3bf5808b38467aac0230739898a758c9b939eecd74f982df088fa0ba81cc1c1732f19a607b135a6e9577bb9fcf7f8570567ce92f66e6
d0e1bbad016cc4949094daea2934712f92dfeecd test: repeat block malleability test with relayable block over P2P (Musa Haruna)
Pull request description:
This PR adds a functional test to repeat the existing malleability check for oversized coinbase witness nonce size using a block that is small enough to be relayed over the P2P network.
This addresses the TODO in test_block_malleability by ensuring behavior is consistent between submitblock RPC and P2P relay.
ACKs for top commit:
maflcko:
lgtm ACK d0e1bbad016cc4949094daea2934712f92dfeecd
janb84:
re ACK d0e1bbad016cc4949094daea2934712f92dfeecd
glozow:
utACK d0e1bbad016cc4949094daea2934712f92dfeecd
Tree-SHA512: 05aec4fade5af8043f40274a8d2f3cf3f540acd038138975bdefbbbc81e105792d6d2588256a2ee5ddb1e05b37fe2d0b3d287160d2dbe86e1aac7cfa9cc02116
faa9d10c84bc6b465cbca266468990cc716b4300 refactor: Construct g_verify_flag_names on first use (MarcoFalke)
Pull request description:
The current usage of the `g_verify_flag_names` map seems fine and I can not see a static initialization order fiasco here.
However, it seems brittle to hope this remains the case in the future. Also, it triggers a msan false-positive in the fuzz CI task. (C.f https://github.com/bitcoin-core/qa-assets/actions/runs/18352815555/job/52413137315?pr=241#step:7:5245)
So just apply the "Construct on first use" idiom.
ACKs for top commit:
kevkevinpal:
ACK [faa9d10](faa9d10c84)
ajtowns:
ACK faa9d10c84bc6b465cbca266468990cc716b4300
janb84:
lgtm ACK faa9d10c84bc6b465cbca266468990cc716b4300
stickies-v:
ACK faa9d10c84bc6b465cbca266468990cc716b4300
Tree-SHA512: 6685dfc91c99a8245722e07fac99a7a6d58586c30964be7ccd74a176dfbf00c6255c8594621e2909640763924f51d3efd4ce65ed65eaeeb1d05c2fd01fe63604
8f7673257a1a86717c1d83770dc857fc254df107 miner: fix empty mempool case for waitNext() (Sjors Provoost)
Pull request description:
Block template fees are calculated by looping over `new_tmpl->vTxFees` and return (early) once the `fee_threshold` is exceeded.
This left an edge case when the mempool is empty, which this commit fixes and adds a test for.
Also update `test/functional/interface_ipc.py` to reflect the new behavior,
Fixes https://github.com/Sjors/sv2-tp/issues/9
ACKs for top commit:
optout21:
ACK 8f7673257a1a86717c1d83770dc857fc254df107
cedwies:
tACK 8f76732
sipa:
utACK 8f7673257a1a86717c1d83770dc857fc254df107
zaidmstrr:
Concept ACK [8f76732](8f7673257a)
Tree-SHA512: ef200fe95e96f810e425283bc37f945c4bf5efa16f4b74820b8a07968f30c5146bca213a372124be84b48beead5dfd35f2b5d10d188d0a465f847ebab61de10a
