d5adb9d09b doc: fix doxygen links to threads in developer-notes.md (Matthew Zipkin)
Pull request description:
The "threads" section of `developer-notes.md` has links to anchor tags in the code generated by doxygen. As far as I can tell this was introduced in #18645 and changes to this section of this document have continued the pattern. The problem is, the content at `https://doxygen.bitcoincore.org` gets re-rendered daily and those anchor tags are generated internally by doxygen, so they are all broken now.
This PR adds doxygen syntax `\anchor XXXX` comments in the code where functions that run in these threads are defined, and then those stable, human-readable anchor tags are applied to the links in the doc.
I have generated the doxygen output from this branch, hosted it on my own web server, and created a modified `developer-notes.md` with these anchor tags and my server as host for demonstration:
https://gist.github.com/pinheadmz/ed3dda7d3c8d589e3989040519190b84#threads
Just note when looking at this:
- `main` is at the bottom of the html page so it might not look right at first
- `initload` is a lambda inside `AppInitMain` so thats where doxygen renders the anchor
ACKs for top commit:
fanquake:
ACK d5adb9d09b
rkrux:
lgtm ACK d5adb9d
Tree-SHA512: c5517823a2d668b01318b3dae3d76fdd9db8a74d8c721aeb748e4f4a6cb56cb4d24e34b2590a41f8553992005cab368fca4ce322a4f204cec16ce338337ae9ee
c17cc76a18 test: speed up feature_dbcrash (will)
Pull request description:
On my machine dbcrash takes 17 minutes to run (wihtout `--usecli`), and is making on the order of 200,000 RPC calls. The bulk of these come from miniwallet's `send_self_transfer_multi` which calls miniwallets `sendrawtransaction`, which does:
```python
def sendrawtransaction(self, *, from_node, tx_hex, maxfeerate=0, **kwargs):
txid = from_node.sendrawtransaction(hexstring=tx_hex, maxfeerate=maxfeerate, **kwargs)
self.scan_tx(from_node.decoderawtransaction(tx_hex))
return txid
```
The second `decoderawtransaction` here doubles the number of RPC calls per send, and feature_dbcrash doesn't use the miniwallet utxo list outside of setup and rescans, it already tracks it's own `utxo_list`, so this is wasted work.
By creating the transaction and sending using a direct node RPC to send, we can halve the number of rpc calls in this section.
This change reduces the runtime to 9 minutes for me.
ACKs for top commit:
maflcko:
review ACK c17cc76a18📐
Tree-SHA512: fa9cf574f280b776446ac994baa7bfd94200d4a3dd8f52e138b0005dcbeb87b357172ce8c470cd743bb5021328aaf24ef4c0ac81a6dc15dd36ca30a3618dc400
fad4f417d1 test: Use operator<< for time_points instead of manual TickSinceEpoch (MarcoFalke)
Pull request description:
This partially reverts the changes from commit 020166080c to testnet4_miner_tests.cpp
Also, remove some confusing IWYU pragmas. Those were inconsistently added in 8c58f63578, but without any rationale why adding them is the correct approach. The correct approach should be done in a proper follow-up, with a clear rationale.
ACKs for top commit:
Sjors:
ACK fad4f417d1
sedited:
ACK fad4f417d1
Tree-SHA512: d88813d5c294b07a567a952c9f4b5351b005ebf1dbd8f31988f922d6d3bd6916cc1553f0d90e23cfaa4517906df176971feb5bc8a7de1938e7896cbe40fe6661
0774eaaf0c util: Require integers for SaturatingAdd() and AdditionOverflow() (Hodlinator)
a815e3e262 rpc: Correct type for tx_sigops (Hodlinator)
Pull request description:
* Correct copy-paste error in RPC code
* Require proper integers for `SaturatingAdd()` and `AdditionOverflow()` in src/util/overflow.h
These changes increase the type safety of the code and were done while exploring increasing the type-safety of `CAmount` (currently just a `typedef` of `int64_t`).
The first commit has nothing to do with overflow but is along for the ride if reviewers agree.
ACKs for top commit:
maflcko:
lgtm ACK 0774eaaf0c
winterrdog:
ACK 0774eaaf0c
sedited:
ACK 0774eaaf0c
Tree-SHA512: a245ad52cfd1c257151aea1a1ed4b6769415c1dddc7c405d8bbe71b9f3abc512a6d890a45cbc8381718be16274e337cc876e6e6da11dc35de71bea83bece6634
This partially reverts the changes from commit
020166080c to testnet4_miner_tests.cpp
Also, remove some confusing IWYU pragmas. Those were inconsistently
added in 8c58f63578, but without any
rationale why adding them is the correct approach. The correct approach
should be done in a proper follow-up, with a clear rationale.
2e9fdcc6da doc: add feature deprecation and removal process to developer notes (Guillermo Fernandes)
Pull request description:
Closes#31980
Adds a dedicated **"Feature deprecation and removal process"** section to `doc/developer-notes.md` covering the full deprecation lifecycle for all major feature categories.
## What's added
**General principles**
- Grace period is one major release (deprecated in N, removed in N+1)
- Deprecation and removal both require release notes
- Deprecated features should remain accessible via a re-enable flag during the grace period
**Per-category guidance covering:**
- RPC methods and fields (`-deprecatedrpc=<feature>` pattern, help text requirements, worked example pointing to #31278)
- Startup options (`LogWarning`/`InitWarning` on use, help text update)
- REST interface (document in `doc/REST-interface.md`)
- ZMQ (document in `doc/zmq.md`)
- Wallet settings (defer to RPC or startup option process depending on exposure)
This consolidates the process that currently exists only implicitly across PRs and issue discussions into one place for contributors to reference.
ACKs for top commit:
maflcko:
lgtm ACK 2e9fdcc6da
polespinasa:
ACK 2e9fdcc6da
stickies-v:
ACK 2e9fdcc6da
sedited:
ACK 2e9fdcc6da
Tree-SHA512: 1d43df410664a45f937bcbd250664f13379168ca90e3024bea506e21a88177e201dcb4fadade705735099e3b8aaa2102a3080ad005bffb3aecb8f08d530d4277
1e5d3b4f0d doc: add release note for mining option validation (Sjors Provoost)
0317f52022 ci: enforce iwyu for touched files (Sjors Provoost)
8c58f63578 refactor: have mining files include what they use (Sjors Provoost)
3bb6498fb0 mining: store block create options in NodeContext (Sjors Provoost)
4637cd157d mining: reject invalid block create options (Sjors Provoost)
8daac1d6eb mining: add block create option helpers (Sjors Provoost)
128da7c3ff miner: add block_max_weight to BlockCreateOptions (Sjors Provoost)
fa81e51eae mining: parse block creation args in mining_args (Sjors Provoost)
020166080c mining: use interface for tests, bench and fuzzers (Sjors Provoost)
44082bea47 interfaces: make Mining use const NodeContext (Sjors Provoost)
d4368e059c move-only: add node/mining_types.h (Sjors Provoost)
6aeb1fbea2 test: cover IPC blockmaxweight policy (Sjors Provoost)
63b23ea1e9 test: regression test for waitNext mining policy (Sjors Provoost)
24750f8b31 test: add createNewBlock failure helper (Sjors Provoost)
63ee9cd15b test: misc interface_ipc_mining.py improvements (Sjors Provoost)
Pull request description:
Although this PR is primarily a refactor, _there are behavior changes_ documented in the release note:
- the IPC mining interface now rejects out-of-range block template options instead of silently clamping them;
- startup now rejects `-blockmaxweight` values lower than `-blockreservedweight`, instead of allowing them to be clamped later.
The interaction between node startup options like `-blockreservedweight` and runtime options, especially those passed via IPC, is confusing.
They're combined in `BlockAssembler::Options`, which this PR gets rid of in favour of `BlockCreateOptions`.
`BlockCreateOptions` is used by interface clients. As before, IPC clients have access to a safe / sane subset, whereas RPC and test code can use all fields. The same type is also used to store mining defaults parsed once during node startup in `NodeContext`.
The maximum block weight setting (`block_max_weight`) is optional. When read from startup options it matches `-blockmaxweight`; when provided by callers it is a runtime override. `Merge()` fills unset fields from startup defaults while preserving caller-provided values.
This all happens in commits `mining: add block create option helpers` and `mining: store block create options in NodeContext`, and requires some preparation to keep things easy to review.
We get rid of `BlockAssembler::Options` but this is used in many tests. Since large churn is inevitable, we might as well switch all tests, bench and fuzzers over to the Mining interface. The `mining: use interface for tests, bench and fuzzers` commit does that, dramatically reducing direct use of `BlockAssembler`. Two exceptions are documented in the commit message. Because `test_block_validity` wasn't available via the interface and the block_assemble benchmark needs it, it's moved from `BlockAssembler::Options` to `BlockCreateOptions` (still not exposed via IPC).
We need access to mining related structs from both the miner and node initialization code. To avoid having to pull in all of `BlockAssembler` for the latter, the `move-only: add node/mining_types.h` commit introduces `node/mining_types.h` and moves `BlockCreateOptions`, `BlockWaitOptions` and `BlockCheckOptions` there from `src/node/types.h`.
I considered also moving `DEFAULT_BLOCK_MAX_WEIGHT`, `DEFAULT_BLOCK_RESERVED_WEIGHT`, `MINIMUM_BLOCK_RESERVED_WEIGHT` and `DEFAULT_BLOCK_MIN_TX_FEE` there from `policy.h`, since they are distinct from relay policy and not needed by the kernel. But this seems more appropriate for a follow-up and requires additional discussion.
---
I kept variable renaming and other formatting changes to a minimum to ease review with `--color-moved=dimmed-zebra`.
## Commit summary
Tests and test cleanup:
- `test: misc interface_ipc_mining.py improvements`
- `test: add assert_create_fails helper`
- `test: regression test for waitNext mining policy`
- `test: cover IPC blockmaxweight policy`
Refactoring test/bench/fuzz callers:
- `interfaces: make Mining use const NodeContext`
- `mining: use interface for tests, bench and fuzzers`
Moving mining interface types:
- `move-only: add node/mining_types.h`
Separating startup defaults from runtime options:
- `mining: parse block creation args in mining_args`: adds `node/mining_args.{h,cpp}` and moves mining option parsing out of `init.cpp`, without storing the parsed values yet.
- `miner: add block_max_weight to BlockCreateOptions`: moves the runtime maximum block weight setting into `BlockCreateOptions` as an optional value, so it can later be defaulted from startup args when unset.
- `mining: add block create option helpers`: centralizes block template option defaulting and merging, removes `BlockAssembler::Options`, and preserves behavior except for dropping the `Specified ` prefix from startup option error messages.
- `mining: reject invalid block create options`: checks typed `BlockCreateOptions` before block template creation, so invalid runtime options are rejected instead of silently clamped. Startup validation also rejects `-blockmaxweight` values lower than `-blockreservedweight`.
- `mining: store block create options in NodeContext`: stores the startup mining options in `NodeContext` as `BlockCreateOptions`, so startup defaults and runtime overrides can be merged with the same option type.
Include hygiene, CI and release note:
- `refactor: have mining files include what they use`
- `ci: enforce iwyu for touched files`
- `doc: add release note for mining option validation`
ACKs for top commit:
w0xlt:
reACK 1e5d3b4f0d
sedited:
ACK 1e5d3b4f0d
ryanofsky:
Code review ACK 1e5d3b4f0d. Looks good, thanks for the updates!
Tree-SHA512: 28c715023cb78f02775caa787b243c994bd0f8ce4559afc8db9301e93400ebbc74963626a4afe65ae15bcc16b9192d051a745839f4c804848d50746ea5a224b4
1d66963749 log: clarify VerifyDB summary log (ViniciusCestarii)
Pull request description:
The final `LogInfo` message about "No coin database inconsistencies" was printed unconditionally, even for check levels 0-2 where no coin DB verification runs and `nGoodTransactions` stays 0. Split into an always-on completion line and a coin-DB result line gated on `nCheckLevel >= 3 && !skipped_l3_checks`.
Before (checklevel=1):
`Verification: No coin database inconsistencies in last 6 blocks (0 transactions)`
After (checklevel=1):
`Verification: checked last 6 blocks at level 1`
ACKs for top commit:
sedited:
ACK 1d66963749
Tree-SHA512: a6c6689cff2f1942a44764a914a1ee01c92efa51489d72f456fccaa1f9ee0640449b55600d43e7dc89d622414854d53e8dfc7e8c87fe454b06e44f76075db267
faf6afd99d doc: Move mutex and thread section into guideline section (MarcoFalke)
fa514caad7 doc: move-only Valgrind section (MarcoFalke)
fa0202f31d doc: move-only Python section (MarcoFalke)
fa37606c65 doc: Regroup clang-tidy rules (MarcoFalke)
fa9c2ddea9 doc: Fix to use lower-case anchors in links to C++ Core Guidelines (MarcoFalke)
Pull request description:
The anchors in isocpp links were recently broken, so fix them to point to the correct anchor.
Also, move/regroup 3 sections while touching the file.
ACKs for top commit:
fanquake:
ACK faf6afd99d
sedited:
ACK faf6afd99d
Tree-SHA512: 0067eb23a6a8cccfaee5df0df347529f17db39473602fa51bc2f3e53c9709934bb25fca51f6ed58c4896437c890789f29facf54d15a7ebbbd247a2ebb1c0b5cd
08c3c37d12 bitcoin-cli: note -rpcclienttimeout is not implemented for IPC connections (Ryan Ofsky)
Pull request description:
The `-rpcclienttimeout` option applies only to HTTP connections. When `bitcoin-cli` connects via IPC, the timeout is silently ignored. Clarify this in the help string.
Noted by pinheadmz in https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-3922849038
ACKs for top commit:
sedited:
ACK 08c3c37d12
Tree-SHA512: 82c99bbcb6a776636197407e4371047d6f1386cbe78823358fb5cceb080a976a4f0ac2c195b7ca734acc81d7e5f32e77887aa11a61860596e3bb8dfd2f523531
a154c05d49 cmake: Check dependencies after build option interaction (Hennadii Stepanov)
Pull request description:
At present, `CMakeLists.txt` interleaves configuration-option handling with dependency discovery. As a result, unnecessary checks may be performed. For example:
```
$ cmake -B build --preset dev-mode -DBUILD_FOR_FUZZING=ON
<snip>
-- Found PkgConfig: /usr/bin/pkg-config (found version "2.3.0")
-- Found ZeroMQ: /usr/lib64 (found suitable version "4.3.5", minimum required is "4.0.0")
-- Performing Test HAVE_USDT_H
-- Performing Test HAVE_USDT_H - Success
-- Found USDT: /usr/include
-- Found QRencode: /usr/lib64/libqrencode.so (found version "4.1.1")
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Performing Test HAVE_STDATOMIC
-- Performing Test HAVE_STDATOMIC - Success
-- Found WrapAtomic: TRUE
-- Found OpenGL: /usr/lib64/libOpenGL.so
-- Found WrapOpenGL: TRUE
-- Found WrapVulkanHeaders: /usr/include
-- Found Qt: /usr/lib64/cmake/Qt6 (found suitable version "6.10.1", minimum required is "6.2")
CMake Warning at CMakeLists.txt:206 (message):
BUILD_FOR_FUZZING=ON will disable all other targets and force
BUILD_FUZZ_BINARY=ON.
<snip>
```
This PR restructures the code to ensure that all dependencies are checked only after the final build option interaction.
ACKs for top commit:
fanquake:
ACK a154c05d49
sedited:
Re-ACK a154c05d49
Tree-SHA512: f863316770a1e27e6c08736932f73f297add326943e9c32c721f7e771f38b0fdc651abfed61add5ee4924cfbf2afff91fde005d41d1fd239d401184b6caf5453
fa24693819 test: Allow --usecli in tests that already support it (MarcoFalke)
fa8d4d5c35 test: Catch CalledProcessError to support --usecli in feature_dbcrash.py (MarcoFalke)
faf0f848ef test: use echojson to allow rpc_named_arguments.py --usecli (MarcoFalke)
faf993ee44 test: Stop node before modifying config to support rpc_users.py --usecli (MarcoFalke)
fa4fc8c1d7 test: Set TestNode url field early, so that feature_loadblock.py --usecli works (MarcoFalke)
Pull request description:
Some tests disallow to be run under `--usecli`. This reduces the coverage and risks that bugs in the bitcoin-cli go unnoticed.
The commits should be self-explanatory and can be reviewed and tested one-by-one.
ACKs for top commit:
willcl-ark:
ACK fa24693819
Tree-SHA512: e34077be98f88ad1e8649600a5f43fc8c77e4ebb03bbccd88c33f2d67882ccdd52b5d18bcfbfc611dff3ebf7455f8e624a88d062aa1863c5eb813bbf4f48e58b
7be0d6fa18 test: remove the lazy import of util in authproxy (rkrux)
779f444680 test: move out JSONRPCException from authproxy to util (rkrux)
Pull request description:
I noticed this issue while reviewing #34773 where a lazy import
is added in the __call__ method of the AuthServiceProxy class in
authproxy.py
There's a circular dependency between authproxy.py and util.py
due to which the former can't use the common utility functions
and thus lazy imports are used as a workaround.
This patch set breaks the dependency so that authproxy.py can use
the utility functions from util.py in a standard fashion. Few tests that
explicitly use get_rpc_proxy and JSONRPCException needed to have
their imports updated.
ACKs for top commit:
maflcko:
review ACK 7be0d6fa18 🏽
Tree-SHA512: 56775cb13d989342ba9482edb255170d695ce5c2d5efbbd64586e0d5463af16467dbf9efe8a0411bde4dfb9bb531839284b2d6f5d828080171d847b70570977d
5faf2ad880 doc: add release notes for deprecation of wallet rbf & bip125 fields (rkrux)
aba24a9b62 wallet: remove "RPC Only" from -walletrbf option help description (rkrux)
97f7cc0233 wallet: mark -walletrbf startup option as deprecated (rkrux)
c4a7613e6a wallet: mark `bip125-replaceable` key as deprecated in transaction RPCs (rkrux)
Pull request description:
Partially fixes#32661.
This patch set is in line with the deprecation of outdated
BIP 125 opt-in RBF signalling and fullrbf in wallet transaction
RPCs and startup options.
ACKs for top commit:
achow101:
ACK 5faf2ad880
w0xlt:
ACK 5faf2ad880
polespinasa:
code reviewed ACK 5faf2ad880
Tree-SHA512: fe6e57f49bef7245b2f564ba705647fb49f0bd370da2e9cfdce45c64a2d8b33ea10a8a802c6619c6382a9bbd2b0e2e4792b08077bc4cfa9b03f7916e2185652a
f05b1a3532 rpc: Fix for duplicate external signers case (optout)
Pull request description:
A straightforward fix for a minor bug in a low-probability case; when there are multiple signers with the same fingerprint, the signers following the duplicate are also thrown away (due to a `break` instead of `continue`).
Background. Bitcoin core can work with _external signers_. They can be configured using the `-signer=<cmd>` argument. The `enumeratesigners` RPC can be used to retrieve the available signers.
Precondition A minor bug was found in a special case:
- multiple external signers are detected, and
- at least two of them are duplicate, i.e., have identical fingerprint, and
- the duplicates are followed by at least one additional signer (that is, the last one is not a duplicate),
Current behavior: Only one of the duplicates is kept, however all subsequent signers are also ignored. This last part is probably unintended, thus it's a minor bug.
To put it in another way, the set of fingerprints returned depends on the actual order in which the devices are returned by the external tool, which may be arbitrary.
Expected behavior: De-duplicate the found signers by fingerprint:
- keep one signer for each fingerprint
- keep all non-duplicates.
Fix: The early `break` of the loop of signers upon duplication has been changed to `continue`.
Test added/extended: `RPCSignerTest` in `test/functional/rpc_signer.py` has been extended with a case of multiple duplicates.
ACKs for top commit:
l0rinc:
ACK f05b1a3532
achow101:
ACK f05b1a3532
polespinasa:
code review ACK f05b1a3532
naiyoma:
ACK f05b1a3532
Tree-SHA512: ac685e233192ecaf5bd535dc66badeb84d78212b3242df71f11c682828955a40167438e16fccc667cd8beef574bc4137a586f44b0f119d7574422c70bb078293
Previously we could fall back to using an unspecialized implementation of std::numeric_limits<T> which would compile as long as the numeric operators existed, but would return 0 for min() & max().
So that util is not dependent on authproxy at all and going
forward authproxy can use util methods.
Reviewing with --color-moved=dimmed-zebra option can be helpful.
dfe5d6a81d fuzz: apply node context reset pattern to p2p_handshake (frankomosh)
Pull request description:
Follow-up to #34302. Applies the node context reset pattern from fabf8d1 to `p2p_handshake`.
Previous code pattern created local `AddrMan` and `node::Warnings` objects, and passed them to `PeerManager::make`. `connman` was left holding a dangling `reference_wrapper<AddrMan>` across iterations, since the local objects destruct at iteration end while `connman` is global.
Like in fabf8d1 , reset and reinstall `node.addrman` and `node.peerman` on each iteration. This PR also removes `includes` made unused by this or prior refactors.
ACKs for top commit:
nervana21:
tACK dfe5d6a81d
maflcko:
review ACK dfe5d6a81d🦏
sedited:
ACK dfe5d6a81d
Tree-SHA512: 141ddec03c6d37f76a3b2d94701d18c851e85ea74e57716abb69ecc955d30371e342c6e267d2669ad853fe2d95fb77dd2fb506e4233ae3a88501d59ee1bbae30
Adds a dedicated "Feature deprecation and removal process" section to
developer-notes.md covering the deprecation lifecycle for RPC methods,
startup options, REST interface, and ZMQ.
Closes#31980
faf02674b3 refactor: Set TestNode.cli only after RPC is connected (MarcoFalke)
fae376cafb refactor: Inline get_rpc_proxy (MarcoFalke)
fa00c7c7a4 test: Allow rpc_bind.py --usecli (MarcoFalke)
fa8f25118c refactor: Use create_new_rpc_connection in wallet_multiwallet.py (MarcoFalke)
fa3ae6c7d3 test: Allow feature_shutdown.py --usecli (MarcoFalke)
fa2a3683d5 test: Allow mining_getblocktemplate_longpoll.py --usecli (MarcoFalke)
fa37c6a529 test: [move-only] Extract create_new_rpc_connection (MarcoFalke)
Pull request description:
Some tests disallow to be run under `--usecli`. This reduces the coverage and risks that bugs in the bitcoin-cli go unnoticed.
The commits should be self-explanatory and can be reviewed and tested one-by-one.
ACKs for top commit:
willcl-ark:
reACK faf02674b3
Tree-SHA512: 83cd6a696e6dd6efd4f2d295e65bfac51fe26404c37f25936808005cc6136e469a30aebaac547af1c722ed5ac827eaf009a150d82420b6b4e242e89305475abe
7777a92a92 ci: Use path with spaces on windows as well (MarcoFalke)
fac6c4270d ci: Put space and non-ASCII char in scratch dir (MarcoFalke)
fa38759823 ci: Require $FILE_ENV (MarcoFalke)
Pull request description:
It seems unlikely that many users have a space in their paths, but it seems a use-case worth enough to be tested by CI, so that it does not have to be done manually. Ref https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065 / https://mirror.b10c.me/bitcoin-bitcoin/33929/#discussion_r2590523065
So do that here, and also add a non-ASCII char while touching.
Also, fix all tests that are broken and assume no space exists in paths.
ACKs for top commit:
hebasto:
ACK 7777a92a92.
sedited:
ACK 7777a92a92
Tree-SHA512: eceb1f6c932c6966cdca8ca8df750081ec5134db5e5f558f7d955716409117bec7c8585d75865e2c98bc1ae7394f3ce64dff87bcebe1e68591afaeef1831d6dd
c03107acf5 ci: switch to GitHub cache for all runners (willcl-ark)
Pull request description:
Cirrus is winding down, and github now offers more than 10GB cache.
Switch to GH cache for all runner-types. Simplify configure-docker action.
ACKs for top commit:
maflcko:
review ACK c03107acf5🚴
Tree-SHA512: b6111c7559a86eed8488a4b0775df812a303a99eed5c80297593936e61d1d5e2ce72fe2fa615625816672414e6947ac4d93b9fd2925522fba06417ea4711ce79
fa99a3ccac ci: Enable pipefail in 03_test_script.sh (MarcoFalke)
Pull request description:
The CI script is problematic, because it is written in Bash, without pipefail enabled. Thus, some failures are silently ignored.
Enabling pipefail is a bit tedious, because:
* The IWYU task has no (`--verbose`) ccache output, so the pipe fails after `grep` [1]. Also, right now on master, the if silently skips: `ci/test/03_test_script.sh: line 122: [: : integer expression expected`.
* The Alpine task has `Hits:` twice in the output, so the pipe fails after `head -1` [2]
Not sure what the easiest way to fix this would be. Some options are:
* Just use `tail -1` and `0` as fallback: `hit_rate=$(ccache --show-stats | grep "Hits:" | tail -1 | sed 's/.*(\(.*\)%).*/\1/' || echo "0")`
* Properly parse, using Python and `--print-stats` (this pull)
[1]
```
+ ccache --version
+ head -n 1
ccache version 4.11.2
+ ccache --show-stats --verbose
Cache directory: /home/admin/actions-runner/_work/_temp/ccache_dir
Config file: /home/admin/actions-runner/_work/_temp/ccache_dir/ccache.conf
System config file: /etc/ccache.conf
Stats updated: Tue Feb 17 08:40:20 2026
Local storage:
Cache size (GB): 0.0 / 2.0 ( 0.00%)
Files: 0
Hits: 0
Misses: 0
Reads: 0
Writes: 0
++ ccache --show-stats
++ grep Hits:
++ head -1
++ sed 's/.*(\(.*\)%).*/\1/'
+ hit_rate=
Command '['docker', 'exec', '--env', 'DANGER_RUN_CI_ON_HOST=1', 'f5e8f319c22101ada5be9d4c5fd7d883ce37b830e86ec64627cb7d2b96749053', '/home/admin/actions-runner/_work/_temp/ci/test/03_test_script.sh']' returned non-zero exit status 1.
Error: Process completed with exit code 1.
```
[2]
```
+ ccache --version
+ head -n 1
ccache version 4.12.1
+ ccache --show-stats --verbose
Cache directory: /home/admin/actions-runner/_work/_temp/ccache_dir
Config file: /home/admin/actions-runner/_work/_temp/ccache_dir/ccache.conf
System config file: /etc/ccache.conf
Stats updated: Tue Feb 17 08:40:35 2026
Cacheable calls: 873 / 873 (100.0%)
Hits: 846 / 873 (96.91%)
Direct: 822 / 846 (97.16%)
Preprocessed: 24 / 846 ( 2.84%)
Misses: 27 / 873 ( 3.09%)
Successful lookups:
Direct: 822 / 873 (94.16%)
Preprocessed: 24 / 51 (47.06%)
Local storage:
Cache size (GB): 2.0 / 2.0 (99.95%)
Files: 2580
Cleanups: 13
Hits: 846 / 873 (96.91%)
Misses: 27 / 873 ( 3.09%)
Reads: 1772
Writes: 52
++ ccache --show-stats
++ grep Hits:
++ head -1
++ sed 's/.*(\(.*\)%).*/\1/'
+ hit_rate=96.91
Command '['docker', 'exec', '--env', 'DANGER_RUN_CI_ON_HOST=1', '272a66a48206f1f6096612e196127ce46ea4dbff5dc14be3a4a20c4ee523956f', '/home/admin/actions-runner/_work/_temp/ci/test/03_test_script.sh']' returned non-zero exit status 141.
Error: Process completed with exit code 1.
```
ACKs for top commit:
willcl-ark:
ACK fa99a3ccac
Tree-SHA512: e5b0ddad8f279bd48102543b0496fa2ecdfc6938d208078595a60b96680467a80504b21acdecd86204b82ce2770eede23e498f04c6a9cee59634f83d44cfe094
fa89098888 test: Document rare failure in test_inv_block better (MarcoFalke)
Pull request description:
This test may fail intermittently, see the tracking issue https://github.com/bitcoin/bitcoin/issues/19732 and https://github.com/bitcoin-core/gui/actions/runs/25819423445/job/75856716144?pr=936#step:10:5303 specifically:
```
2026-05-13T19:37:19.624321Z TestFramework (INFO): Tx should be received at node 1 after 64 seconds
2026-05-13T20:17:19.627627Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
File "/home/runner/work/_temp/test/functional/test_framework/test_framework.py", line 143, in main
self.run_test()
File "/home/runner/work/_temp/build/test/functional/p2p_tx_download.py", line 413, in run_test
test()
File "/home/runner/work/_temp/build/test/functional/p2p_tx_download.py", line 130, in test_inv_block
self.sync_mempools()
File "/home/runner/work/_temp/test/functional/test_framework/test_framework.py", line 721, in sync_mempools
raise AssertionError("Mempool sync timed out after {}s:{}".format(
AssertionError: Mempool sync timed out after 2400s:
{'2db3a22f9370eb32f110597882acebef1c76cf319837bac69b829917730d2349'}
set()
```
The test assumes that node1 gets the tx from its honest outbound peer (node0). However, the same connection is inbound from the view of node0, so node0 will use an exponentially distributed delay, which may be more than the expected timeout.
Document this clearer, so that the this specific failure can simply be re-run without having to spend a long time thinking and debugging it every time it happens.
ACKs for top commit:
willcl-ark:
ACK fa89098888
Tree-SHA512: b7fa1a584bc61595d7e1b73ed1fbe446604515c24786565e76b385c7be3278cdb8909d8aeabe6d3f05685281851836f2267b97d7f022d634d833d2124ba53c00
d61053d97b build: Drop libevent from bitcoin-cli link libraries (Fabian Jahr)
798d051c80 cli: Remove libevent usage (Fabian Jahr)
376e7ef07c util: Expose IOErrorIsPermanent in sock header (Fabian Jahr)
5d562430de netbase: Add timeout parameter to ConnectDirectly (Fabian Jahr)
a988ac592f cli: Add HTTPResponseHeaders class for parsing response headers (Fabian Jahr)
c471c5085b common: Add unused UrlEncode function (Fabian Jahr)
9687ef1bd9 ci: Tolerate unused free functions in intermediate commits (Fabian Jahr)
Pull request description:
Part of the effort to remove the libevent dependency altogether, see #31194
This takes the parsing logic from the [`HTTPHeaders` class](d549f01caa) from #32061 and puts it into `bitcoin-cli` as a small `HTTPResponseHeaders` class with a comment to revisit potentially sharing this code somehow. This decoupled the two pulls which seems like the most sensible way to deal with this since the actual overlap is very small compared to the impact of each of the pulls which should ideally not block each other.
Otherwise the change itself replaces the libevent-based HTTP client with a simple synchronous implementation which uses the `Sock` class directly.
ACKs for top commit:
hodlinator:
re-ACK d61053d97b
theStack:
re-ACK d61053d97b
w0xlt:
ACK d61053d97b
Tree-SHA512: a3580a45faf540ee844aac8cb1dc056a89e8e11b45781d2807baa4736d5c0934284c6066206101b6984111a48a186d67845545d07639b623cb35ccc2d85d3ab2
Cirrus is winding down and github now offers more than 10GB cache.
Switch to GH cache for all runner-types. Simplify docker build arg
construction, and reduce the number of needed action permissions.
18c1cc65e9 kernel: improve BITCOINKERNEL_WARN_UNUSED_RESULT usage (stickies-v)
Pull request description:
Similar to `[[nodiscard]]`, `BITCOINKERNEL_WARN_UNUSED_RESULT` is used to indicate that ignoring a function's return value is almost certainly a bug.
It is used in cases such as a resource leak (e.g. an owning handle returned by a `*_create` or `*_copy` function), or when the returned value is itself an error/status code. It is not used merely because discarding the result is wasteful, e.g. on getters or predicates.
Fix the incorrect usage, and properly document the attribute. Having this clearly documented helps avoid bikeshedding on future PRs.
ACKs for top commit:
w0xlt:
ACK 18c1cc65e9
stringintech:
ACK 18c1cc65
sedited:
ACK 18c1cc65e9
Tree-SHA512: ba5aa19f0b90ef12ed3c200449734d9aa67af166f2a2b29e321d3ec1e9b49483215c5078ae1dd945ea65650a2017f3101bf75abcc9d3388eb1a3d4c8070c22dd
Read configured mining options once during startup instead of parsing
options like -blockmaxweight each time a block template is generated.
Store the parsed startup options as BlockCreateOptions. Members left unset
keep representing unset options; hardcoded defaults are applied by
FlattenMiningOptions() before the options are used. IPC overrides and node
defaults can then be merged with the same option type used by
BlockAssembler.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Check BlockCreateOptions before block template creation instead of
clamping runtime values. This makes invalid runtime block creation
options, including those passed by IPC mining clients, fail explicitly
instead of silently mining with different values than the caller
requested.
Runtime option validation now uses the same error wording as startup
option validation. Startup validation also rejects -blockmaxweight
values lower than -blockreservedweight instead of allowing them to be
clamped later.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Move block template defaulting into helper functions for
BlockCreateOptions. FlattenMiningOptions() fills hardcoded defaults and
MergeMiningOptions() overlays defaults without replacing caller-provided
values.
Use the shared option type in BlockAssembler so IPC callers and internal
callers can pass through the same options path. This commit does not
change behavior, except for dropping the "Specified " prefix from startup
option error messages.
Keep the -blockmintxfee ParseMoney check in ReadMiningArgs() instead of
CheckMiningOptions(), because CheckMiningOptions() only sees the parsed
CFeeRate value and not the original string.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This new optional replaces nBlockMaxWeight.
Use uint64_t for the block weight options to match BlockAssembler's
nBlockWeight accounting and the IPC schema's blockReservedWeight type.
A negative -blockmaxweight value is now parsed as 0 instead of an
overflowed signed value before validation rejects it.
The new block_max_weight option is not exposed to IPC clients.
Move the argument parsing for -blockmaxweight, -blockreservedweight,
-blockmintxfee out of init.cpp to a dedicated mining_args.cpp.
This is mostly a refactor and keeps the existing validation checks. It
does switch the weight arguments to GetArg<uint64_t>, introduced in
bitcoin/bitcoin#34582, so very large or negative weight values can be
reported differently in error messages.
Have most tests, benchmarks and fuzzers go through the mining interface.
This is a refactor: it does not change what blocks are created, just how
the creation calls are made. This avoids most direct test, benchmark and
fuzzer use of node::BlockAssembler::Options, making it easier to drop in a
later commit.
Two exceptions which use BlockAssembler directly:
- one check in test/miner_tests.cpp needs m_package_feerates
- fuzz/tx_pool.cpp Finish() doesn't have access to a NodeContext
Move test_block_validity from BlockAssembler::Options to
BlockCreateOptions so bench/block_assemble.cpp can continue to set it.
Just like coinbase_output_script, this is not exposed to IPC clients.
Inline options variable in places where it's only needed once.
We also drop one unused PrepareBlock declaration and one unused
implementation.
TestChain100Setup::CreateBlock no longer needs a chainstate argument,
which in turn means it can be dropped from CreateAndProcessBlock. Using
the Mining interface here also requires marking the test
KernelNotifications chainstate as loaded after LoadVerifyActivateChainstate().
The next commit switches test helpers that take a const NodeContext& to
create blocks through the Mining interface, so MakeMining needs to
accept a const NodeContext too.
Verify that -blockmaxweight is honored both when an IPC block template is first created and after waitNext() refreshes the template.
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
