Document the CI wrapper as the reproducible IWYU entrypoint instead of suggesting ad hoc native runs.
Also describe how to handle suspected false positives, explain when local `IWYU pragma` workarounds are appropriate, and add an example rationale to an existing pragma.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Daniel Pfeifer <daniel@pfeifer-mail.de>
eab72d14d7 refactor: use SignOptions for MutableTransactionSignatureCreator (Sjors Provoost)
5ed41752c5 refactor: use SignOptions for SignTransaction (Sjors Provoost)
dc4a5d1270 refactor: use PSBTFillOptions for filling and signing (Sjors Provoost)
Pull request description:
Replace the `sign`, `finalize` , `bip32derivs` and `sighash_type` arguments that are passed to `FillPSBT()` and `SignPSBTInput()` with a `PSBTFillOptions` struct.
```cpp
struct PSBTFillOptions {
bool sign{true};
std::optional<int> sighash_type{std::nullopt};
bool finalize{true};
bool bip32_derivs{true};
};
```
Additionally this PR introduces:
```cpp
struct SignOptions {
int sighash_type{SIGHASH_DEFAULT};
};
```
This is used by `SignTransaction` and the `MutableTransactionSignatureCreator` constructor.
These changes make it easier to add additional options later without large code churn, such as `avoid_script_path` proposed in #32857. It also makes the use of default boolean options safer compared to positional arguments that can easily get mixed up.
ACKs for top commit:
w0xlt:
reACK eab72d14d7
optout21:
ACK eab72d14d7
sedited:
ACK eab72d14d7
Tree-SHA512: 097e3d042e794c9f47d03e1aafad184a4525aa765d274f6497122d4f41603e902191df6fbf9ce846dbcd7372a159b67e2234da7341ec6a6776be5685e3e6e6ff
Replace the sign, finalize , bip32derivs and sighash_type arguments which
are passed to FillPSBT() and SignPSBTInput() with a PSBTFillOptions struct.
This makes it easier to add additional options later without large code
churn, such as avoid_script_path proposed in #32857. It also makes the
use of default boolean options safer compared to positional arguments
that can easily get mixed up.
8deed0df06 doc: add release notes for PR 34911 (rkrux)
1a85ca1dff rpc, mempool: rpcdeprecate `bip125-replaceable` key in mempool RPCs reponses (rkrux)
f89d18c3b1 rpc, mempool: rpcdeprecate `fullrbf` key in getmempoolinfo RPC response (rkrux)
Pull request description:
mempoolfullrbf=1 behaviour has been the default since v28 and the argument has
been removed since v29 subsequently.
The getmempoolinfo RPC need not return the deprecated `fullrbf` key in the
response anymore unless the user requests it via -deprecatedrpc=fullrbf node
argument.
Also, remove the `bip125-replaceable` key from the mempool RPCs
responses (because it, too, has been deprecated since v29) unless the user
requests it via -deprecatedrpc=bip125 node argument.
ACKs for top commit:
optout21:
ACK 8deed0df06
sedited:
ACK 8deed0df06
instagibbs:
ACK 8deed0df06
Tree-SHA512: dfbbdf04ae25dafe81758bd85715af80d736e21bb913ab09ff8fd8225587b7a1bf1dd6b0f7ea05135504e11d43371d98e5f5fc6e3717d581e870e8c2bf97811b
a1e534bbf0 doc: clarify clang-tidy in developer notes (rkrux)
Pull request description:
Recently, I ran clang-tidy in my system and had to figure out setting it up
correcty on my system (macOS) & understanding its output while tweaking
few arguments that made the output easier to parse.
This patch documents those in the developer notes.
ACKs for top commit:
sedited:
ACK a1e534bbf0
Tree-SHA512: e95cd44cdad9a9011c18136ec7f1db4e73f78f11c517f94c14cf128beaaea150881868c27e7c5c710c9cab94d285d61dc5e5278f347c229f5df495cd49c532ab
ef21e29298 doc: update llvm based coverage example (Max Edwards)
Pull request description:
The old example was working on my Mac but when run on Linux I was finding that I had missing coverage information.
On some systems the old example of using xargs to pass list of profraw files can exceed xargs's argument limit resulting in multiple executions of llvm-profdata merge command which overwrites it's output file losing coverage information. This PR switches the example to using a file with a list of filenames.
There are other ways to solve this like using bash's globbing or a one liner with temporary file but I felt like this was the most simple, robust and easy to understand.
ACKs for top commit:
vasild:
ACK ef21e29298
sedited:
ACK ef21e29298
Tree-SHA512: 2726d9b38088c1c0b6f04d4622bdc4ea28e16b13a962a3afc842798aa5e0eee4682752c73707c5580a43bdb9d0d84d542afd357f166e4dbb7f6beb7afc0522c8
14547eb489 kernel: guard btck::Handle move-assignment against self-move (Thomas)
Pull request description:
The move-assignment operator for `btck::Handle<>` in `src/kernel/bitcoinkernel_wrapper.h` unconditionally called `DestroyFunc(m_ptr)` before reading the source pointer. On a self-move (`h = std::move(h)`), this destroys the held resource and then restores the now-dangling pointer via `std::exchange(other.m_ptr, nullptr)` (since `&other == this`), which leaves `m_ptr` pointing at freed memory. The destructor then calls `DestroyFunc` again on it, resulting in a double-free.
Trace of `h = std::move(h)` with the old code, where `h.m_ptr == P`:
1. `DestroyFunc(m_ptr)` -> `delete P`. `this->m_ptr` still literally stores the now-dangling value `P`.
2. `std::exchange(other.m_ptr, nullptr)` — because `&other == this`, this reads the dangling `P` back, writes `nullptr` to `m_ptr`, and returns `P`.
3. `m_ptr = P` restores the dangling pointer.
4. `~Handle()` later runs `DestroyFunc(P)` -> double free, UB.
The copy-assignment operator already guards against self-assignment with `if (this != &other)`; the move variant should be symmetric. The standard library requires moved-from objects to be in a valid (at minimum, safely destructible) state, which the previous implementation violated when source and destination alias.
`Handle<>` is the base class of 16 public types in the kernel C++ API wrapper (`Transaction`, `Block`, `BlockHeader`, `ChainParams`, `Context`, `Coin`, `BlockValidationState`, `ScriptPubkey`, `TransactionOutput`, `Txid`, `OutPoint`, `TransactionInput`, `PrecomputedTransactionData`, `BlockHash`, `BlockSpentOutputs`, `TransactionSpentOutputs`), so self-move can arise from generic algorithms operating on containers of these types (`std::sort`, `std::remove`, erase-remove idioms, etc.).
Fix: mirror the copy-assignment pattern by guarding the move-assignment body with `if (this != &other)`, making a self-move a no-op.
Also extend `CheckHandle` in `src/test/kernel/test_kernel.cpp` to exercise self-move-assignment for every `Handle`-derived type, checking that the stored pointer and the serialized bytes (where applicable) are unchanged.
ACKs for top commit:
sedited:
Thanks, ACK 14547eb489
alexanderwiederin:
ACK 14547eb489
yuvicc:
lgtm ACK 14547eb489
Tree-SHA512: 334f5ad3045d5f9b06cc0dd096bc911a992773c59cc469765c2082975a9f7a90f2349b9ad94b4b5127290de1fab2f5424a621384c1b4bb9152de99f5da8ed6aa
fa204100e1 streams: Remove confusing DataStream::in_avail() (MarcoFalke)
fa5ab0220e move-only: Extract ProcessPong() helper (MarcoFalke)
Pull request description:
The `in_avail` alias is confusing (see commit message), so remove it. Also extract the `ProcessPong()` helper, as the only place that called `in_avail`.
ACKs for top commit:
l0rinc:
code review ACK fa204100e1
sedited:
ACK fa204100e1
Tree-SHA512: b5a069abb471c42d39e91a3c55fc7f18fb9ccf8591408b3dd43210d0287785369b6a5d6b59b847d85247a494f208dbbe4a7e88b24caf9b3e6c8fea2157cd0ee0
2a90b6132a Add release notes for exportasmap (Fabian Jahr)
8cb2d926b4 rpc: Add exportasmap RPC (Fabian Jahr)
Pull request description:
This depends on the embedded data PR itself (#28792), until merge this will remain in draft status. All commit but the last one are from there.
There has been interest in exporting the embedded data back into a file. This is implemented here with a simple `exportasmap` RPC which provides this functionality. The exported file can be used to verify the data integrity or statistical analysis using e.g. `contrib/asmap-tool.py`.
ACKs for top commit:
achow101:
ACK 2a90b6132a
sedited:
ACK 2a90b6132a
seduless:
Tested ACK 2a90b6132a
Tree-SHA512: c8453078efe916ed95bff0695aabd9123d805970e037fad5e9317f4d43e2a4b21970030265bc82b00f3a222ee3db11346eef1fb6fc9393429b9bc6449f1830f1
fa43da21f1 refactor: Run ShouldWarnOversizedDbCache calculation in u64 (MarcoFalke)
fa5801762e util: Return uint64_t from _MiB and _GiB operators (MarcoFalke)
fa9ddb01c9 test: Use MiB operator directly in cuckoocache_tests (MarcoFalke)
Pull request description:
Currently, the `_MiB` literal operator returns a value of type `size_t`. This is brittle and confusing:
* It is inherently impossible to represent larger values, like storage device usage.
* Similarly, it is not possible to even type an upper cap on the memory, see the failure in https://github.com/bitcoin/bitcoin/pull/34692#issuecomment-3973281952
* Using `size_t` isn't required here. The function is evaluated at compile time, and even 32-bit compilers can evaluate an `uint64_t` at compile time.
* Using `size_t` here encourages it to be used in more places, which will likely lead to more bugs and CVEs, such as https://github.com/bitcoin/bitcoin/pull/34109, https://github.com/bitcoin/bitcoin/pull/33724, etc.
Fix all issues, by:
* Marking the operator `consteval`, to ensure it is really only called at compile-time.
* Returning an `uint64_t` value.
* Using it in the place where it was previously not possible.
Review note:
This should have no downside, because the C++11 narrowing checks continue to work as expected. For example, typing `uint8_t{1_MiB};` will continue to fail with the correct error message `error: constant expression evaluates to 1048576 which cannot be narrowed to type 'uint8_t' (aka 'unsigned char') [-Wc++11-narrowing]` (like it does on current master).
ACKs for top commit:
l0rinc:
ACK fa43da21f1
achow101:
ACK fa43da21f1
hebasto:
ACK fa43da21f1, I have reviewed the code and it looks OK.
hodlinator:
ACK fa43da21f1
Tree-SHA512: e19f6bd18c519c2a56d44fbc9e65b02e630bc71170f02a8a8955bf5c9bb2924b55ec2f810de2bbc961162aa668b7383c9fe9b5261fa1de7d9e8413003c32fa87
On some systems the old example of using xargs to pass list of profraw
files can exceed xargs's argument limit resulting in multiple executions
of llvm-profdata merge command which overwrites it's output file losing
coverage information. Switch to using a file with a list of filenames.
Recently, I ran clang-tidy in my system and had to figure out setting it up
correcty on my system (macOS) & understanding its output while tweaking
few arguments that made the output easier to parse.
This patch documents those in the developer notes.
The alias of the size() method is confusing, because:
* It claims to be part of the Bitcoin Core stream subset (streams
interface), but this is not used by any other stream interface. Mostly
the `write(std::span)` and `read(std::span)` define the stream
interface.
* It casts the size_t to i32, but the only place that calls the function
casts that back to size_t.
* Providing this alias for size() without a proper reason is confusing.
Fix all issues by removing it and using the size() method.
RPCs: getrawmempool, getmempoolancestors, getmempooldescendants, getmempoolentry
This key has been marked deprecated since v29, it can be
removed from the RPC response now unless the user requests it via the
-deprecatedrpc=bip125 node argument.
mempoolfullrbf=1 behaviour has been the default since v28 and the argument has
been removed since v29 subsequently. The getmempoolinfo RPC need not return
the deprecated `fullrbf` key in the response anymore unless the user requests
it via -deprecatedrpc=fullrbf node argument.
e6430b2773 bench: make `setup()` use single-iteration epochs (Lőrinc)
ba0078e3bf bench: fix ephemeral spend inputs (Lőrinc)
b8b7f896e8 bench: drop duplicate balance benchmark (Lőrinc)
Pull request description:
### Context
Found while reviewing https://github.com/bitcoin/bitcoin/pull/35018, which led to a few additional benchmark fixes.
### Problem
`MempoolCheckEphemeralSpends` populated every child input by writing to `vin[0]`.
That left only one child input pointing at the parent transaction, while the remaining inputs kept default prevouts.
`WalletBalanceMine` duplicated `WalletBalanceClean` exactly.
The same review also showed that nanobench `setup()` can be misused as though it runs once per timed call.
It actually runs once per epoch, so using it with multiple epoch iterations can silently leave later iterations with different preconditions.
### Fix
Write each ephemeral-spend prevout to the matching child input and assert that the last input spends the last parent output.
Remove the duplicate wallet balance benchmark registration.
Add a nanobench assertion that `setup()` is only used with `epochIterations(1)`.
> [!NOTE]
> This PR included a few benchmark adjustments originally which made the tests unstable and were reverted.
ACKs for top commit:
davidgumberg:
crACK e6430b2773
sedited:
ACK e6430b2773
Tree-SHA512: 667c67975d7a3e21333f6c8a4e037ad700bc6dcbd7039e002534e4f2a9328195a618b78accf097e7c43402a1f1b25e32b0c39642fd465729ab4c337abe4f23f1
df44afdc98 kernel: expose btck_block_tree_entry_get_ancestor (Peter Zafonte)
Pull request description:
Callers building a block locator from a stale tip currently have to walk back one entry at a time using btck_block_tree_entry_get_previous. For large step sizes this means hundreds of C API calls per locator entry.
This exposes GetAncestor via btck_block_tree_entry_get_ancestor, which uses the existing CBlockIndex skiplist to reach any ancestor height in O(log N) and works on any chain, not just the active chain.
Includes a C++ convenience wrapper and tests ~~covering same-height, genesis, and out-of-range cases~~ in btck_block_tree_entry_tests.
See previous discussion here #34843
ACKs for top commit:
alexanderwiederin:
ACK df44afdc98
sedited:
ACK df44afdc98
Tree-SHA512: e953b634235129fa972e7f148016e8aee7bad61e16d50191d6724d11c71fe0f7623152ff61aa4c4ec5c8a5b500aaeb36613b5b2ff1e404d01ed7b65aa861f7aa
`setup()` in nanobench runs once per epoch, not once per timed call.
If an epoch executes the benchmark body multiple times, `setup()` can silently leave later iterations with different preconditions.
Make `setup()` force `epochIterations(1)` itself: keep rejecting incompatible larger explicit epoch sizes, but allow existing single-iteration callers such as `-sanity-check`.
With `setup()` handling this centrally, remove the redundant `epochIterations(1)` calls from the benchmarks that use it.
Co-authored-by: David Gumberg <davidzgumberg@gmail.com>
`MempoolCheckEphemeralSpends` wrote every prevout to `tx2.vin[0]` instead of `tx2.vin[i]`.
That left only one child input pointing at the parent transaction, while the remaining inputs kept default prevouts.
Write each prevout to `vin[i]` instead.
Add an assertion that the last child input spends the last parent output.
Co-authored-by: David Gumberg <davidzgumberg@gmail.com>
3bf3b6d59a net: log SOCKS5 auth before sending (takeshikurosawaa)
b2debc9276 net: cleanup SOCKS5 auth logging (takeshikurosawaa)
Pull request description:
`Socks5()` logs the supplied `username` and `password` in `BCLog::PROXY`
when the server selects `USER_PASS`.
Keep the log entry for the auth path, but omit the credentials.
Log the message before sending the authentication data.
No functional behavior change intended.
ACKs for top commit:
davidgumberg:
crACK 3bf3b6d59a
theStack:
ACK 3bf3b6d59a
sedited:
ACK 3bf3b6d59a
Tree-SHA512: ef90fca297b954c8659d108259b7f25216c0c0ddb7305539154f14a7bfdad807b70b7ddd0cf6e4ad0127d0a1ccfdfbb3f4785d7662b134409375ff1830b9df9b
Previously, they were using a pattern of defininig a constant megabytes
symbol, and then multiplying that by 1_MiB.
It is easier to use the _MiB operator directly.
The move-assignment operator for btck::Handle<> unconditionally called
DestroyFunc(m_ptr) before reading the source pointer. On a self-move
(h = std::move(h)), this destroyed the held resource and then reassigned
the now-dangling pointer back to m_ptr via std::exchange, leading to a
double-free when the object is later destroyed.
Mirror the existing self-check in the copy-assignment operator by
guarding the move-assignment with 'if (this != &other)' so a self-move
becomes a no-op, leaving the object in a valid state as required by the
standard library.
Handle<> is the base of 16 public types in the kernel C++ API wrapper
(Transaction, Block, BlockHeader, ChainParams, Context, Coin,
BlockValidationState, ScriptPubkey, TransactionOutput, Txid, OutPoint,
TransactionInput, PrecomputedTransactionData, BlockHash,
BlockSpentOutputs, TransactionSpentOutputs), so self-move can arise
from generic algorithms operating on containers of these types.
Extend CheckHandle in test_kernel to cover self-move-assignment for
every Handle-derived type.
c5ec2d5313 logging: replace FormatLogStrInPlace with Format (stickies-v)
3b92ec2036 logging: replace BufferedLog with log::Entry (stickies-v)
8115001cd4 logging: pass log::Entry through to logging functions (stickies-v)
b414913c73 util: add timestamp and thread_name to log::Entry (stickies-v)
8a55b17751 util: make SourceLocation constructor explicit (stickies-v)
Pull request description:
Preparatory work to enable struct-based logging for kernel (#34374), but I think it's a mild improvement by itself too.
#34465 introduced `util::log::Entry`. This PR updates some internal functions and structs to make better use of that new plumbing.
Mostly a refactor, except for very minor timing differences for `timestamp` and `mocktime` which are now uniformly captured at `Entry` generation.
ACKs for top commit:
maflcko:
review ACK c5ec2d5313🚐
ryanofsky:
Code review ACK c5ec2d5313. Just suggested changes since last review: adding (void) std::move, making SourceLocation constructor explicit, dropping BufferedLog struct. Overall PR is a nice code simplification, and I don't think it has downsides other than buffered logs now containing [ignored](https://github.com/bitcoin/bitcoin/pull/34865#discussion_r3054379230) `should_ratelimit` fields, which can be fixed in a followup by separating log entries from log options.
sedited:
ACK c5ec2d5313
Tree-SHA512: 3943f380a426b2c1b405226f231db51548c737467a278936b36a4cc738e01a790258f0886817d7caa1dbba7874f2e7f9ad93c36a137fa35f721f2d988b9863aa
32325d1777 tests: Add test for mempool-invalid wallet tx (Anthony Towns)
25e063d950 wallet: Add separate balance info for non-mempool wallet txs (Anthony Towns)
81e763f1e5 wallet: Have GetBalance report used amount directly without two calls (Anthony Towns)
Pull request description:
Changes `getbalances` to report the sum of txos spent by transactions that aren't confirmed nor in the mempool (eg due to being part of too long a mempool chain, or spending non-standard outputs, or having a datacarrier output that exceeds `-datacarriersize`, etc). Those values are added to the trusted/untrusted_pending/immature/used fields as appropriate (where previously they were skipped), and subtracted from the new nonmempool field, so that the sum of all fields remains the same.
For example:
```
$ bitcoin-cli -regtest getbalances
{
"mine": {
"trusted": 6049.99999220,
"untrusted_pending": 0.00000000,
"immature": 3200.00000780,
"nonmempool": -100.00000000
},
"lastprocessedblock": {
"hash": "3ab4582226d5e8ad76438db48d76e822c31bce2cdbc7ba82a5d974a277515d0d",
"height": 221
}
}
```
Closes#11887
ACKs for top commit:
achow101:
ACK 32325d1777
w0xlt:
lgtm ACK 32325d1777
musaHaruna:
Code Review ACK [32325d1](32325d1777)
Tree-SHA512: 142581944d1b3213067e219e3b8205f27b89007e545149c01b801bad38fe730c5b2bfdfe6a2064c3649889f66ec48ec7616982564d00e3d83837249e925d8f16
7c75244ade Change pindexMostWork parameter of ActivateBestChainStep() to reference (optout)
c5eb283bca Change CChain::FindFork() to take ref (optout)
20b58e281a Change CChain::Next() to take reference (optout)
fe2d6e25e0 Change CChain::Contains() to take reference (optout)
db56bcd692 test: Add CChain::FindFork() tests (optout)
8333abdd91 test: Add CChain basic tests (optout)
Pull request description:
Refactor `CChain` methods (`Contains()`, `Next()`, `FindFork()`) to use references instead of pointers, to minimize the risk of accidental `nullptr` dereference (memory access violation). Also add missing unit tests to the `CChain` class.
The `CChain::Contains()` method (in `src/chain.h`) dereferences its input without checking. The `Next()` method also calls into this with a `nullptr` if invoked with `nullptr`. While most call sites have indirect guarantee that the input is not `nullptr`, it's not easy to establish this to all call sites with high confidence. These methods are publicly available. There is no known high-level use case to trigger this error, but the fix is easy, and makes the code safer.
Changes:
- Add basic unit tests for `CChain` class methods
- Add unit tests for `CChain::FindFork()`
- Change `CChain::Contains()` to take reference
- Change `CChain::Next()` to take reference
- Change `CChain::FindFork()` to take reference
- Change `pindexMostWork` parameter of `ActivateBestChainStep()` to reference
- Rename changed parameters (`* pindex` --> `& index`)
Alternative. A simpler change is to stick with pointers, with extra checks where needed, see #34416 .
This change is remotely related to and indirectly triggered by #32875 .
Further ideas, not considered in this PR:
- Change `InvalidateBlock()` and `PreciousBlock()` to take references.
- Change `CChain` internals to store references instead of pointers
- Change CChain to always have at least one element (genesis), that way there is always genesis and tip.
- Check related methods to return reference (guaranteed non-null) -- `FindFork`, `FindEarliestAtLeast`, `FindForkInGlobalIndex`, `blockman.AddToBlockIndex`, etc.
ACKs for top commit:
l0rinc:
reACK 7c75244ade
maflcko:
re-review ACK 7c75244ade🌅
achow101:
ACK 7c75244ade
hodlinator:
re-ACK 7c75244ade
Tree-SHA512: 122f40120058f7e1f0273b3afed9c54966c05f06b6f2fee45bc48430617f24a5e4320a9bb7bb0ac986f2accfa22fabae5cc941b949758ddca2e9fcd472b46c33
af0ee28eb6 refactor: use _MiB consistently for Mebibyte conversions (Lőrinc)
b3edd30aa2 util: add _GiB for Gibibyte conversions (Lőrinc)
Pull request description:
### Problem
Byte-size conversions in the codebase currently show up in many equivalent formats (multiplication/division chains, shifts, hex/binary literals), which creates a maintenance burden and makes review error-prone - especially considering the architectural differences of `size_t`.
Inspired by https://github.com/bitcoin/bitcoin/pull/34305#discussion_r2734720002, it seemed appropriate to unify `Mebibyte` usage across the codebase and add `Gibibyte` support with 32/64 bit `size_t` validation.
### Fix
This PR refactors those call sites to use `""_MiB` (existing) and `""_GiB` (new), and adds the encountered value/pattern replacements to unit tests to make review straightforward, and to ensure the conversions remain valid.
The literals are overflow-checked when converting to `size_t`, and unit tests cover the 32-bit boundary cases.
Concretely, it replaces patterns such as:
* `1024*1024`, `1<<20`, `0x100000`, `1048576`, `/ 1024 / 1024`, `* (1.0 / 1024 / 1024)` → `1_MiB` or `double(1_MiB)`
* `1024*1024*1024`, `1<<30`, `0x40000000`, `1024_MiB`, `>> 30` → `1_GiB`
(added unit tests for each replacement category to ease review)
Additionally, declarations whose initializer reads a `_MiB`/`_GiB` literal are switched to braced initialization so a future oversized value is rejected at compile time through the narrowing check instead of silently truncating.
### Note
In the few places where arithmetic involves signed values, the result is identical to the previous code assuming those quantities never become negative.
ACKs for top commit:
achow101:
ACK af0ee28eb6
janb84:
ACK af0ee28eb6
maflcko:
review ACK af0ee28eb6 🖍
hodlinator:
re-ACK af0ee28eb6
Tree-SHA512: 55286ce3f833f88335394a74e9e0b95c7d023e5cdc9ded40accbbbcd870101e4dcc05926865d6bef4c1be1ebd648aa3fdf947ef9575633ccfe56691f145d7a2d
4abc0c2e04 doc: update release process to mention security advisories pre-announcements (Antoine Poinsot)
Pull request description:
We missed a pre-announcement with the release of 31.0. This updates the release process docs to make sure we check those before publishing a major release.
ACKs for top commit:
achow101:
ACK 4abc0c2e04
jonatack:
ACK 4abc0c2e04
Tree-SHA512: 63d1def3d4144de7992b539d41a496c98bc81b33e2a9c27af55933a696050123a0c967253093728f0553f13aca12acec8a51efbeda0936077646c379c37219d6
fa1015bbcb refactor: Use NodeClock::time_point for m_connected (MarcoFalke)
fa244b984c refactor: Use NodeClock::time_point for m_last_send/recv and m_ping_start (MarcoFalke)
fa2605b204 refactor: Use NodeClock::time_point for CNetMessage::m_time (MarcoFalke)
fa644e625b refactor: Use NodeClock::duration for m_last_ping_time/m_min_ping_time/m_ping_wait (MarcoFalke)
333316f6be doc: Fix typo "eviction criterium" -> "eviction criterion" (MarcoFalke)
fa54fb0129 refactor: gui: Accept up to nanoseconds in formatDurationStr, but clarify they are ignored (MarcoFalke)
fab88884b7 refactor: Avoid manual chrono casts with * or / (MarcoFalke)
facfce37f6 util: Add NodeClock::epoch alias (MarcoFalke)
fa41e072b3 refactor: Use NodeClock alias over deprecated GetTime (MarcoFalke)
Pull request description:
It is a bit confusing to have some code use the deprecated `GetTime`, which returns a duration and not a time point, and other code to use `NodeClock` time points.
Fix a few more places to properly use time_point types.
ACKs for top commit:
stickies-v:
re-ACK fa1015bbcb
seduless:
re-ACK fa1015bbcb
naiyoma:
ACK fa1015bbcb
sedited:
ACK fa1015bbcb
Tree-SHA512: 7c8df1a9025271b08a40fd0d176bcbbf90920bc4d83a6e1c8cfaad2a894632af2b9a1aca5c3c9ddc3803e559dd168244121fd188ef22f399d60075ff194a9140
9fe5896a44 tor: torcontrol disconnect on too many lines to avoid OOM (David Gumberg)
8b68287bf9 test: Make torcontrol max line length test stricter and test boundaries. (David Gumberg)
ab5889796f refactor: torcontrol add connection checks to restart_with_mock (David Gumberg)
Pull request description:
LLM disclosure: Found with the help of Claude Opus 4.6, fix, test, description, and commit messages written by me.
------
This fixes a low-severity issue where a misbehaving Tor control daemon can cause
bitcoind to OOM by sending continuation lines without sending `250 OK` or
similar.
This issue is not that serious because if your tor control daemon is malicious you are already in all kinds of trouble, but as a matter of robustness this should be fixed.
The fix is to prevent the `TorControlConnection::m_message` buffer from growing
without bound by by limiting the number of lines handled by `TorControlConnection::ProcessBuffer()`
to `MAX_LINE_COUNT = 1000`. Now the most memory that can be occupied by
`m_message` is on the order of `MAX_LINE_LENGTH * MAX_LINE_COUNT= 100MB`
Although this is not compliant with the Tor control protocol in general,
where commands like `GETINFO ns/all` will likely return thousands of
lines, it is more than sufficient for handling the replies from the
commands that are used by a node:
<details>
<summary>
#### Tor control commands used by Bitcoin Core
</summary>
`AUTHENTICATE`: 1 line:
The server responds with 250 OK on success or 515 Bad
authentication if the authentication cookie is incorrect. Tor closes
the connection on an authentication failure.
https://spec.torproject.org/control-spec/commands.html#authenticate
`GETINFO net/listener/socks`: 2 lines
A quoted, space-separated list of the locations where Tor is
listening...
https://spec.torproject.org/control-spec/commands.html#getinfo
`AUTHCHALLENGE SAFECOOKIE`: 1 line
If the server accepts the command, the server reply format is:
```
"250 AUTHCHALLENGE" SP "SERVERHASH=" ServerHash SP "SERVERNONCE="
ServerNonce CRLF
```
https://spec.torproject.org/control-spec/commands.html#authenticate
`PROTOCOLINFO`: 4-5 lines
The server reply format is:
```
250-PROTOCOLINFO" SP PIVERSION CRLF \*InfoLine "250 OK" CRLF
InfoLine = AuthLine / VersionLine / OtherLine
```
(https://spec.torproject.org/control-spec/commands.html#protocolinfo)
`ADD_ONION`: 2-3 lines for Bitcoin Core's tor control client.
The server reply format is:
```
"250-ServiceID=" ServiceID CRLF
["250-PrivateKey=" KeyType ":" KeyBlob CRLF]
*("250-ClientAuth=" ClientName ":" ClientBlob CRLF)
"250 OK" CRLF
```
...
The server response will only include a private key if the server
was requested to generate a new keypair
...
If client authorization is enabled using the “BasicAuth” flag (which
is v2 only), the service will not be accessible to clients without
valid authorization data (configured with the “HidServAuth” option).
The list of authorized clients is specified with one or more
“ClientAuth” parameters. If “ClientBlob” is not specified for a
client, a new credential will be randomly generated and returned."
https://spec.torproject.org/control-spec/commands.html#add_onion
We don't set the `BasicAuth` flag, so the response will not include any
`ClientAuthLines`.
</details>
## Reproduce
To reproduce this issue, the following script or similar can be used as the
misbehaving Tor control daemon:
```python
#!/usr/bin/env python3
"""
A fake Tor control service that never finishes its reply. Sends unlimited
continuation lines ("250-...") without ever sending the final "250 ...".
Each line accumulates in m_message.lines with no cap. Bitcoind OOMs.
"""
import socket
import time
PORT = 19191
server = socket.create_server(("127.0.0.1", PORT))
conn, _ = server.accept()
conn.recv(4096) # Receive PROTOCOLINFO
time_start = time.time()
try:
while True:
conn.sendall(b"250-Ceaseless\r\n" * 10000)
except (BrokenPipeError, ConnectionResetError):
elapsed = time.time() - time_start
print(f"Node disconnected after {elapsed:.2f}s")
```
**🟡¡This will OOM, run in a container, VM, or some sandbox with memory limits!🟡**
Start a node with `-torcontrol=127.0.0.1=19191`.
E.g. with systemd:
```bash
systemd-run --user --scope -p MemoryMax=2G -p MemorySwapMax=0 bitcoind -regtest -torcontrol=127.0.0.1:19191
```
ACKs for top commit:
fjahr:
ACK 9fe5896a44
danielabrozzoni:
Code review ACK 9fe5896a44
janb84:
ACK. 9fe5896a44
sedited:
ACK 9fe5896a44
Tree-SHA512: ccbeba40c096e1fa3911c75c49e3a5c403712f646d77329de48017a19d1f0caa2ee4cc148b6c6473f68e55d7da04f17eb67748b5bf4dede3579b944ee5370cf5
These benchmark inputs are immutable fixture bytes, so `DataStream` adds an unnecessary owned buffer and the setup needed to recreate or preserve its state.
Use `SpanReader` for block deserialization in `checkblock` instead.
This keeps `DeserializeBlockTest` focused on deserialization work, while `CheckBlockTest` still uses untimed setup only to rebuild a fresh uncached `CBlock` for the timed `CheckBlock()` call.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
`PrevectorDeserialize` only needs a reusable read-only view over fixed serialized bytes.
Keeping a mutable `DataStream` around just to call `Rewind()` is unnecessary.
Rebuild a fresh `SpanReader` for each benchmark run and remove `DataStream::Rewind()`, whose remaining use was this benchmark-only reset path.
The benchmark can now serialize exactly the 1000 entries it deserializes, so drop the stale extra element that used to avoid full consumption.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
`TestBlockAndIndex` still deserialized its fixed block fixture through `DataStream` and appended a dummy byte to avoid compaction after full consumption.
Use `SpanReader` for that fixture instead.
This removes the leftover dummy-byte workaround and reads the immutable fixture through a read-only view.
When `DataStream` is fully consumed, both `read()` and `ignore()` reset it to an empty state by clearing the backing buffer and resetting the read position.
Call `clear()` in both places instead of open-coding the same state transition.
This keeps the behavior unchanged while documenting the fully-consumed reset in one place.
Remove the unused `Compact()` method as well - it has been unused for a long time and can be added back if it is ever needed.
`CDBIterator::GetKey()` only deserializes the current LevelDB key once.
`GetKeyImpl()` already exposes the current key as a contiguous borrowed byte span, and `GetKey()` creates a fresh local reader and only performs immediate forward reads before returning.
Switch this path to `SpanReader` so the key bytes are read in place instead of being copied into a temporary `DataStream`.
This keeps the same exception swallowing and `bool` return semantics while avoiding the extra allocation and copy.
The preceding test locks down the subtle safety property that matters here: a failed decode must not consume the current iterator entry.
Note that the same simplification does not apply to `GetValue()`, because that path deobfuscates the value bytes in place first and still needs an owning mutable buffer.
The upcoming change will replace the temporary owning `DataStream` inside `CDBIterator::GetKey()` with a borrowed reader over the current LevelDB key bytes.
The copied `DataStream` currently insulates the iterator entry from a failed decode, so the optimization is only safe if a deserialization failure still returns `false` and leaves the same key/value readable afterward.
Extend `dbwrapper_iterator` to read a one-byte key as a `uint16_t`.
The read must fail, return `false`, and still allow the same key and value to be read afterward.
This would fail if `GetKey()` stopped swallowing deserialization exceptions, or if a failed decode started consuming shared iterator state instead of only temporary reader state.
Drop the dead `const_cast` in the test while here, since `dbw` is already non-const.
Locking down that contract first makes the following `SpanReader` switch a behavior-preserving optimization.
