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
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.
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.
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.
c9d8582235 fuzz: remove redundant CScript method calls from script harness (Bruno Garcia)
Pull request description:
The script harness was calling `GetSigOpCount`, `HasValidOps`, `IsPayToAnchor`, `IsPayToScriptHash`, `IsPayToWitnessScriptHash`, and `IsPushOnly` on the fuzzed CScript. All of these are already covered by the `script_ops` harness, which is the dedicated harness for CScript member methods. Also, add the missing `IsPayToAnchor` call to `script_ops` so the move preserves coverage.
ACKs for top commit:
maflcko:
lgtm ACK c9d8582235
Tree-SHA512: f69c9f217b5422e24714f985a8c0f7fcfb2c1b1e9117a0e54e7d31139bedfec9279a756ceb9b8860f7574b7cf59fb17c1063596176de8de4800824dc7866f33c
The script harness was calling GetSigOpCount, HasValidOps,
IsPayToAnchor, IsPayToScriptHash, IsPayToWitnessScriptHash, and
IsPushOnly on the fuzzed CScript. All of these are already covered
by the script_ops harness, which is the dedicated harness for
CScript member methods.
Also add the missing IsPayToAnchor call to script_ops so the move
preserves coverage.
5f36e0ff1e rpc: fix getblockstats UTXO overhead accounting (Lőrinc)
76190489e6 coins: pack `Coin` height/coinbase consistently (Lőrinc)
1f309d1aa2 coins: make `Coin::fCoinBase` a bool (Lőrinc)
Pull request description:
The [`getblockstats` RPC](https://github.com/bitcoin/bitcoin/pull/10757) currently overestimates UTXO overhead by treating the `fCoinBase` bitfield as an additional `bool` in `PER_UTXO_OVERHEAD`.
However, `fCoinBase` and `nHeight` are stored as bitfields and effectively packed into a single 32-bit value; counting an extra bool in the overhead calculation is unnecessary.
This PR introduces the following changes across three commits:
* Store `fCoinBase` as a `bool` bitfield to reduce implicit conversions at call sites.
* Use a consistent height/coinbase packing style across `Coin` serialization, undo serialization, and coinstats hashing (casting `nHeight` to `uint32_t` before shifting to avoid signed-promotion UB).
* Adjust UTXO overhead estimation to match the actual `Coin` layout and update the related tests accordingly.
ACKs for top commit:
achow101:
ACK 5f36e0ff1e
sedited:
ACK 5f36e0ff1e
vasild:
ACK 5f36e0ff1e
optout21:
crACK 5f36e0ff1e
Tree-SHA512: f4a44debed358e9e130da9d6fae5f89289daa34f0bdb7155edc3e9b691c219451f4c80b1e16aca5b011f0fa2fa975484ef1af4ca4563b7c6ba4ca9dd133f30be
6ac49373aa test: Add clean shutdown to Socks5Server (optout)
Pull request description:
Add clean shutdown to `Socks5Server` test utility, fix a sporadic CI failure.
Closes#34849.
The `Socks5Server` utility handles multiple incoming connections, which are handled in separate background threads. Its `stop()` method unblocks and waits for the main background thread cleanly, but it doesn't attempt to wait for the completion of handler threads. There is no guarantee that the handler threads are finished after `stop()` returns, which can lead to IO errors.
In the reported sporadic test failures, the test in `p2p_private_broadcast.py` uses the Socks5 server, and makes a node connect to/through it. Then it stops the Socks5 server, and then it stops the node. However, if a connection handler is still active, that can lead to errors, as the socket is closed.
The change attempts to fix this by storing all handler threads and connections, and attempting to shut them down before `stop()` returns.
Notes:
- ~~I was not able to reliably reproduce the failure locally.~~
Update: I could reproduce the failure, see https://github.com/bitcoin/bitcoin/issues/34849#issuecomment-4126331780 .
Running the relevant test:
```sh
build/test/functional/test_runner.py p2p_private_broadcast.py
```
ACKs for top commit:
andrewtoth:
re-ACK 6ac49373aa
achow101:
ACK 6ac49373aa
vasild:
ACK 6ac49373aa
w0xlt:
ACK 6ac49373aa
Tree-SHA512: c63a62a516925252c450c9b7931539aedac2e44566bd4fe217aa54e6ca1438c8f50a100c714166b7cc67786b00f42c1a36e4916d63311842a77293cd2e102356
07b9b13b45 doc: add integer type conventions in btck api remarks (Alexander Wiederin)
ba6287a449 kernel: align height parameters to int32_t in btck API (Alexander Wiederin)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/34885#discussion_r3093370576
### Summary
Align height-related parameters and return values in the kernel API to use `int32_t`.
### Motivation
The convention in the API is to use fixed-width types like `int32_t` for data values (e.g. heights), and plain `int`/`unsigned int` for boolean-like values and flags. Two functions were not following this:
- `btck_chain_get_height`: header declared `int32_t` but implementation used `int`
- `btck_chain_get_by_height`: both header and implementation used plain `int`
### Changes
- `btck_chain_get_height`: align `.cpp` to match header (`int` -> `int32_t`)
- `btck_chain_get_by_height`: update both header and `.cpp` (`int` -> `int32_t`)
- `CountEntries` in `bitcoinkernel_wrapper.h`: align return type to `int32_t`
- `GetByHeight` in `bitcoinkernel_wrapper.h`: align parameter type to `int32_t`
- Document integer type conventions in the `@page remarks` section of `bitcoinkernel.h`
*Note: This is technically a breaking change for C consumers who have function pointer declarations matching the old signature. The Rust, Go, Java and Python bindings should not be affected.*
ACKs for top commit:
musaHaruna:
Code review ACK [07b9b13](07b9b13b45)
sedited:
ACK 07b9b13b45
stringintech:
ACK 07b9b13
Tree-SHA512: 1c6c5c3113c82f7c5f07871c1aa996b71191d0b190d000d48774a866fcc08514cdabcc13a9de2fed8b5d824da050258db73d73e6aa0b80d828af1533128fe1cd
c54f37c1ba cli, rpc: add -rpcid option for custom request IDs (Torkel Rogstad)
Pull request description:
Add a `-rpcid` CLI argument to `bitcoin-cli` that allows setting a custom string as the JSON-RPC request ID instead of the hardcoded default of 1. This enables correlating requests and responses when debugging or when multiple clients are making concurrent calls.
On the server side, include the request ID in the RPC debug log line.
Tests are added in `test/functional/interface_bitcoin_cli.py` for this.
ACKs for top commit:
maflcko:
lgtm ACK c54f37c1ba
stickies-v:
ACK c54f37c1ba
sedited:
ACK c54f37c1ba
Tree-SHA512: b693a50a2be5dc7797e5b61bddc4c10237888b52065d1b68029d7211542e0dd44f2b3dfb9b3bd7c5e4d9026766817de66a7ef4f29a8d97a268554ba775063e10
3e5dc61035 rpc, refactor: gettxoutsetinfo race condition fix follow-ups (rkrux)
Pull request description:
This patch addresses my own review comments from the review of #34451. If these are found helpful, it makes sense to do them now after the previous PR was merged and backported.
Pasting the comments below that also explains the changes:
- Move the pindex declaration below now that it is not used earlier.
- stats was being generated partially in both these ComputeUTXOStats functions, which reads oddly to me. Now that the pcursor is also moved and passed to this function, which reads oddly as well, I believe we can refactor this function to completely build the stats inside this function. A side benefit is that by removing the stats and pcursor arguments, the function signature becomes quite similar to its namesake, which in turn becomes a straightforward wrapper of this function.
ACKs for top commit:
w0xlt:
ACK 3e5dc61035
sedited:
ACK 3e5dc61035
Tree-SHA512: b8e4a4ebfe4935aa97920cb7068445ea93e571f80e679b8791343ac8750b48484d4288e083e07bf433b397cb6071171a2b77d34758a4627056b34cc63d06f0f4
Replace hard-coded MiB byte conversions (e.g. `1024*1024`, `1<<20`, `1048576`) with the existing `_MiB` literal to improve readability and avoid repeating constants.
In the few spots where arithmetic involves signed values, the result is identical to the previous code assuming those quantities never turn negative.
Also switch to brace init on every declaration assigned from `_MiB`/`_GiB` literals so a future oversized value (e.g. `unsigned int x{4096_MiB}`) becomes a compile error through the C++11 narrowing check instead of silently truncating.
Extend unit tests to cover the 32-bit `size_t` overflow boundary and to assert equivalence for integer and floating-point conversions.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
Introduce `operator""_GiB`, sharing the overflow-checked conversion logic with the existing `operator""_MiB`.
Use `1_GiB` in a few existing places where it is a drop-in replacement (e.g. `1024_MiB`, `1<<30`) and extend unit tests to cover boundary behavior.
ActivateBestChainStep() is always called with non-nullptr pindexMostWork parameter,
change the type of the parameter from pointer to reference to enforce this.
Also rename the parameter (prefix p doesn't make sense any more).
To minimize chance of erroneous nullptr dereference, `CChain::Next()`
is changed to take a reference instead of a pointer.
Call sites have been adapted. Notably, NextSyncBlock() now checks
the FindFork() result before calling into Next(), because
the fork lookup may return null.
The `CChain::Contains()` method dereferences its input without checking,
potentially resulting in nullptr-dereference if invoked with `nullptr`.
To avoid this possibility, its input is changed to a reference instead.
Call sites are adapted accoringly, extra nullptr-check is added as
needed.
