Unlike exceptions, which can be ignored as they require extra try-catch
blocks, returning expected errors forces callers to always handle
submission failures.
Not throwing an exception also fixes an unclean shutdown bug
#34573 since we no longer throw when attempting to Submit()
from the libevent callback http_request_cb().
c1355493e2 refactor: fees: split fee rate format from fee estimate mode (ismaelsadeeq)
922ebf96ed refactor: move-only: move `FeeEstimateMode` enum to `util/fees.h` (ismaelsadeeq)
Pull request description:
### Motivation
Part of #34075
- The `FeeEstimateMode` enum was responsible for both selecting the fee estimation algorithm and specifying the fee rate' format.
#### Changes in this PR:
* The `FeeEstimateMode` enum (`UNSET`, `ECONOMICAL`, `CONSERVATIVE`) is moved to a new util/fees.h header.
* A new `FeeRateFormat `enum (`BTC_KVB`, `SAT_VB`) is introduced in `policy/feerate.h` for feerate formatting.
* The `CFeeRate::ToString()` method is updated to use `FeeRateFormat`.
* All relevant function calls have been updated to use the new `FeeRateFormat` enum for formatting and `FeeEstimateMode` for fee estimation mode.
This refactoring separates these unrelated responsibilities to improve code clarity.
ACKs for top commit:
l0rinc:
ACK c1355493e2
furszy:
utACK c1355493e2
musaHaruna:
ACK [c135549](c1355493e2) — reviewed in the context of PR [34075](https://github.com/bitcoin/bitcoin/pull/34075)
willcl-ark:
ACK c1355493e2
Tree-SHA512: 7cbe36350744313d3d688d3fd282a58c441af1818b1e8ad9cddbc911c499a5205f8d4a39c36b21fed60542db1ef763eb69752d141bcef3393bf33c0922018645
38fd85c676 http: replace WorkQueue and threads handling for ThreadPool (furszy)
c323f882ed fuzz: add test case for threadpool (TheCharlatan)
c528dd5f8c util: introduce general purpose thread pool (furszy)
6354b4fd7f tests: log node JSON-RPC errors during test setup (furszy)
45930a7941 http-server: guard against crashes from unhandled exceptions (furszy)
Pull request description:
This has been a recent discovery; the general thread pool class created for #26966, cleanly
integrates into the HTTP server. It simplifies init, shutdown and requests execution logic.
Replacing code that was never unit tested for code that is properly unit and fuzz tested.
Although our functional test framework extensively uses this RPC interface (that’s how
we’ve been ensuring its correct behavior so far - which is not the best).
This clearly separates the responsibilities:
The HTTP server now focuses solely on receiving and dispatching requests, while ThreadPool handles
concurrency, queuing, and execution.
This will also allows us to experiment with further performance improvements at the task queuing and
execution level, such as a lock-free structure or task prioritization or any other implementation detail
like coroutines in the future, without having to deal with HTTP code that lives on a different layer.
Note:
The rationale behind introducing the ThreadPool first is to be able to easily cherry-pick it across different
working paths. Some of the ones that are benefited from it are #26966 for the parallelization of the indexes
initial sync, #31132 for the parallelization of the inputs fetching procedure, #32061 for the libevent replacement,
the kernel API #30595 (https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2413702370) to avoid blocking validation among others use cases not publicly available.
Note 2:
I could have created a wrapper around the existing code and replaced the `WorkQueue` in a subsequent
commit, but it didn’t seem worth the extra commits and review effort. The `ThreadPool` implements
essentially the same functionality in a more modern and cleaner way.
ACKs for top commit:
Eunovo:
ReACK 38fd85c676
sedited:
Re-ACK 38fd85c676
pinheadmz:
ACK 38fd85c676
Tree-SHA512: a0330e54ed504330ca874c42d4e318a909f548b2fb9ac46db8badf5935b9eec47dc4ed503d1b6f98574418e3473420ea45f60498be05545c4325cfa89dcca689
- Introduce a `FeeRateFormat` enum and change `CFeeRate::ToString()`
to use it for `BTC/kvB` vs `sat/vB` output formatting.
- Handle all enum values, hence remove default case in `CFeeRate::ToString()`
and `assert(False)` when a `FeeRateFormat` value is not handled.
- Keep `FeeEstimateMode` focused on fee estimation behavior by removing fee rate format
values from `FeeEstimateMode`.
- Update all formatting call sites and tests to pass `FeeRateFormat` explicitly, separating fee rate format
from fee-estimation mode selection.
37cc2a2d95 logging: use util/log.h where possible (stickies-v)
bb8e9e7c4c logging: Move message formatting to util/log.h (stickies-v)
001f0a428e move-only: Move logging macros to util/log.h (stickies-v)
94c0adf4e8 move-onlyish: Move logging levels to util/log.h (stickies-v)
56d113cab0 move-only: move logging categories to logging/categories.h (stickies-v)
f5233f7e98 move-only: Move SourceLocation to util/log.h (stickies-v)
Pull request description:
This is a mostly move-only change. It's a small refactoring that allows logging macros to be used by including a simple `util/log.h` header instead of the full `logging.h` logging implementation. Most of the changes here were cherry-picked from #34374.
Original motivation for this change was to reduce the size and complexity of #34374 (kernel structured logging PR) and reduce the number of conflicts it causes with other PRs. But this should also make sense as a standalone change to have a clearer separation of concerns between log generation and log handling, and avoid needing to depend on the whole logging framework in call sites that only emit log messages.
Recommended to review with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
ACKs for top commit:
l0rinc:
diff ACK 37cc2a2d95
stickies-v:
re-ACK 37cc2a2d95
optout21:
crACK 37cc2a2d95
sedited:
ACK 37cc2a2d95
Tree-SHA512: c7a761323ae63f07ad290d4e3766ba1348a132c8cc68a9895fa9ae5c89206599c00646c42ef77223ac757b9d8bfe6c181bead15e7058e4d8966b3bac88a8f950
48161f6a05 wallet: introduce "tx amount exceeds balance when fees are included" error (stratospher)
b7fa609ed1 wallet: remove PreSelectedInputs (stratospher)
7819da2c16 walllet: use CoinsResult instead of PreSelectedInputs (stratospher)
e5474079f1 wallet: introduce GetAppropriateTotal() in CoinsResult (stratospher)
d8ea921d01 wallet: correctly reserve in CoinsResult::All() (stratospher)
7072d825e3 wallet: ensure COutput added in set are unique (stratospher)
fefa3be782 wallet: fix, make 'total_effective_amount' optional actually optional (stratospher)
Pull request description:
picks up https://github.com/bitcoin/bitcoin/pull/25269.
This PR re-implements the code path so that an error message is thrown when a transaction's total amount (including fees) exceeds the available balance. It also refactors the wallet's coin selection code.
1. the first 3 commits are unrelated to the code but few small bug fixes which are nice to fix. but also kind of impacts the remaining logic. (could PR separately if reviewers wish)
1. c467325aaf187d7f056bb1ea1cec6b7c4250af2e: make `total_effective_amount` optional actually optional
2. 2202ab597596c84fc49f8784e823372b7a9efcbe: ensure `set<shared_ptr<COutput>>` has unique COutput
3. a5ffbbf122d66fc4ad9b2e7c6d7d1dfa1816388e: Correctly reserve size when flattening `CoinsResult.coins` map to vector
3. the next 3 commits from 4745d5480ca5c3809edd51140e4d2c0433582844 replace the `PreSelectedInputs` struct with `CoinsResult` and removes `PreSelectedInputs`.
4. the last commit (e664484a6d34c1795ebb0925ab31faea5d64ab00) deals with the error message - `AmountWithFeeExceedsBalance` error inside `WalletModel::prepareTransaction` is never thrown and remains an unused code path. This is because `createTransaction` does not retrieve the fee when the process fails. The fee return arg is set only at the end of the process, when the transaction is successfully created. Therefore, if the transaction creation fails, the fee is not available inside `WalletModel::prepareTransaction` to trigger the `AmountWithFeeExceedsBalance` error.
This PR re-implements the feature inside `CreateTransactionInternal` and adds test coverage for it.
| on master | on PR |
|-----------|-------|
| <img src="https://github.com/user-attachments/assets/a903e687-2466-42c7-b898-5dec24bfe515" width="750" alt="Insufficient funds" /> | <img src="https://github.com/user-attachments/assets/74bb3c83-6132-4c09-91f0-0a446618b3c8" width="750" alt="AmountWithFeeExceedsBalance" /> |
the unreachable code path is removed in https://github.com/bitcoin-core/gui/pull/807 which requires this PR.
ACKs for top commit:
achow101:
ACK 48161f6a05
furszy:
utACK 48161f6
Tree-SHA512: a963fac8d6714f76571df8cf9aff70601536dc6faa4326fbb5892c3f080dc393f0d7c6e2d21879c7a2c898bf0092adb154376d9b0a8929b31575ce9d1d47dec2
before #25806, set<COutput> was used and would not
contain same COutputs in the set.
now we use set<shared_ptr<COutput>> and it might be
possible for 2 distinct shared_ptr (different pointer
address but same COutputs) to be added into the set.
so preserve previous behaviour by making sure values
in the set are also distinct
Preparation for a future commit where kernel's dependency
on logging.cpp is removed completely.
Replace usage of logging\.h with util/log\.h where it
suffices, and fix wrong includes according to iwyu.
4fec726c4d refactor: Simplify Interpret asmap function (Fabian Jahr)
79e97d45c1 doc: Add more extensive docs to asmap implementation (Fabian Jahr)
cf4943fdcd refactor: Use span instead of vector for data in util/asmap (Fabian Jahr)
385c34a052 refactor: Unify asmap version calculation and naming (Fabian Jahr)
fa41fc6a1a refactor: Operate on bytes instead of bits in Asmap code (Fabian Jahr)
Pull request description:
This is a second slice carved out of #28792. It contains the following changes that are crucial for the embedding of asmap data which is added the following PR in the series (probably this will remain in #28792).
The changes are:
- Modernizes and simplifies the asmap code by operating on `std::byte` instead of bits
- Unifies asmap version calculation and naming (previously it was called version and checksum interchangeably)
- Operate on a `span` rather than a vector in the asmap internal to prevent holding the asmap data in memory twice
- Add more extensive documentation to the asmap implementation
- Unify asmap casing in implemetation function names
The first three commits were already part of #28792, the others are new.
The documentation commit came out of feedback gathered at the latest CoreDev. The primary input for the documentation was the documentation that already existed in the Python implementation (`contrib/asmap/asmap.py`) but there are several other comments as well. Please note: I have also asked several LLMs to provide suggestions on how to explain pieces of the implementation and better demonstrate how the parts work together. I have copied bits and pieces that I liked but everything has been edited further by me and obviously all mistakes here are my own.
ACKs for top commit:
hodlinator:
re-ACK 4fec726c4d
sipa:
ACK 4fec726c4d
sedited:
Re-ACK 4fec726c4d
Tree-SHA512: 950a591c3fcc9ddb28fcfdc3164ad3fbd325fa5004533c4a8b670fbf8b956060a0daeedd1fc2fced1f761ac49cd992b79cabe12ef46bc60b2559a7a613d0e166
With this change, callers can use util/log.h to emit log messages and do not need to
include the full logging implementation in logging.h.
There's a potential performance impact with this change from an extra
`strprintf` call in log statements where `Logger::WillLogCategoryLevel` returns
true but `Logger::Enabled` returns false. This happens when bitcoind is run
with `-noprinttoconsole -nodebuglogfile` options.
For background, log macro arguments are supposed to be evaluated when
`Logger::WillLogCategoryLevel` returns true, even if log output is not enabled.
Changing this behavior would be reasonable but needs consideration in a
separate PR since not evaluating arguments in log statements has the potential
to change non-logging behavior.
The extra `strprintf` call could have been avoided by expanding this change and
making the `ShouldLog()` function return a tri-state DO_LOG / DO_NOT_LOG /
DO_NOT_LOG_ONLY_EVALUATE_ARGS value instead of a bool, but this complexity did
not seem warranted.
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Move logging macros to util/log.h so the entire codebase can use the same
macros.
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
This is not strictly move-only because BCLog::Level is now defined as a type
alias for util::log::Level;
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Introduce util/log.h as a lightweight header for code that only needs to emit
log messages. Move SourceLocation into this header so log-emitting code no
longer needs to include logging.h and depend on the full log management API.
This is a move-only change and the first change of several changes that
separate log generation from log handling. It also applies clang-format
suggestions to the moved code.
Review with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Currently, there are issues with headers in `batchpriority.cpp`:
1. `SCHED_BATCH` is not defined on all supported *BSD platforms.
2. `pthread.h` is necessary on other platforms.
This addresses both issues and fixes other includes.
14f99cfe53 rpc: make `uptime` monotonic across NTP jumps (Lőrinc)
a9440b1595 util: add `TicksSeconds` (Lőrinc)
Pull request description:
### Problem
`bitcoin-cli uptime` was derived from wall-clock time, so it could jump by large amounts when the system clock is corrected after `bitcoind` starts (e.g. on RTC-less systems syncing NTP).
This breaks the expectation that uptime reflects process runtime.
### Fix
Compute uptime from a [monotonic clock](https://en.cppreference.com/w/cpp/chrono/steady_clock.html) so it is immune to wall-clock jumps, and use that monotonic uptime for the RPC.
GUI startup time is derived from wall clock time minus monotonic uptime so it remains sensible after clock corrections.
### Reproducer
Revert the fix commit and run the `rpc_uptime` functional test (it should fail with `AssertionError: uptime should not jump with wall clock`):
Or alternatively:
```bash
cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc)
DATA_DIR=$(mktemp -d)
./build/bin/bitcoind -regtest -datadir="$DATA_DIR" -connect=0 -daemon
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" -rpcwait uptime
sleep 1
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" setmocktime $(( $(date +%s) + 20000000 ))
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" uptime
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" stop
```
<details>
<summary>Before (uptime jumps with wall clock)</summary>
```bash
Bitcoin Core starting
0
20000001
Bitcoin Core stopping
```
</details>
<details>
<summary>After (uptime stays monotonic)</summary>
```bash
Bitcoin Core starting
0
1
Bitcoin Core stopping
```
</details>
----------
Issue: https://github.com/bitcoin/bitcoin/issues/34326
ACKs for top commit:
maflcko:
review ACK 14f99cfe53🎦
willcl-ark:
tACK 14f99cfe53
w0xlt:
ACK 14f99cfe53
sedited:
ACK 14f99cfe53
Tree-SHA512: 3909973f58666ffa0b784a6df087031b9e34d2022d354900a4dbb6cbe1d36285cd92770ee71350ebf64d6e8ab212d8ff0cd851f7dca1ec46ee2f19b417f53984
ccf9172ab3 util: Remove `FilterHeaderHasher` (rustaceanrob)
Pull request description:
With respect to `std::unordered_map` documentation, the `Hash` type
defined in the template is over the `Key` and not `T`, the value. This
hasher is incorrectly named as the `FilterHeader` is the value within this map.
I consider this a bug as opposed to a refactor as the key and value
relationship is implied to be `filter header -> block hash` when it is
the opposite.
Further, the hasher for the key already exists via `BlockHasher`.
ref: https://en.cppreference.com/w/cpp/container/unordered_map.html
ACKs for top commit:
andrewtoth:
ACK ccf9172ab3
maflcko:
lgtm ACK ccf9172ab3
ismaelsadeeq:
ACK ccf9172ab3👍🏾
Tree-SHA512: 607602391bf337d4e25b04a6a643fa32c3ab4599009b181b46ecdb0705e8ff2af89a6192042453c9e8e44abcb2150589019f02c5c944ecdff41322c3e0ad45ac
40735450c0 Remove unused epochguard.h (Suhas Daftuar)
1a8494d16c Rework CTxMemPool::GetChildren() to not use epochs (Suhas Daftuar)
Pull request description:
Since #33591, the epoch-based graph traversal optimization logic is only used for `CTxMempool::GetChildren()`, a function that is only used in RPC code and tests. Rewrite it without epochs, and remove `util/epochguard.h` itself, as that was its last use.
This allows us to reduce per-transaction memory usage by 8 bytes, for no material loss. With the new TxGraph-based mempool implementation, I also don't foresee future uses for it, as TxGraph can do even better by using BitSet-based traversal tracking.
ACKs for top commit:
ajtowns:
ACK 40735450c0
instagibbs:
ACK 40735450c0
l0rinc:
code review ACK 40735450c0
Tree-SHA512: 7ce7c04835cd2425a71c4fd47f316b6fb7381caa27383de7ecc4aa81100fcf7bc5e062699b307c08e0b853b35f06710d9ac761d6e660af9f9331e708d36f2fe0
1911db8c6d string: add LineReader (Matthew Zipkin)
ee62405cce time: implement and test RFC1123 timestamp string (Matthew Zipkin)
eea38787b9 string: add AsciiCaseInsensitive{KeyEqual, Hash} for unordered map (Matthew Zipkin)
4e300df712 string: add `base` argument for ToIntegral to operate on hexadecimal (Matthew Zipkin)
0b0d9125c1 Modernize GetBindAddress() (Matthew Zipkin)
a0ca851d26 Make GetBindAddress() callable from outside net.cpp (Matthew Zipkin)
Pull request description:
This is a component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194). It is the first six commits of #32061 and provides a string-parsing utility (`LineReader`) that is also consumed by #34158.
These are the functions that are added / updated for HTTP and Torcontrol:
- `GetBindAddress()`: Given a socket, provides the bound address as a CService. Currently used by p2p but moved from `net` to `netbase` so other modules can call it.
- `ToIntegral()`: Already used to parse numbers from strings, added new argument `base = 10` so it can also be used to parse hexadecimal integers. HTTP chunked transfer-encoding uses hex-encoded integers to specify payload size: https://datatracker.ietf.org/doc/html/rfc7230.html#section-4.1
- `AsciiCaseInsensitive` comparators: Needed to store HTTP headers in an `unordered_map`. Headers are key-value pairs that are parsed with case-insensitive keys: https://httpwg.org/specs/rfc9110.html#rfc.section.5.1
- `FormatRFC1123DateTime()`: The required datetime format for HTTP headers (e.g. `Fri, 31 May 2024 19:18:04 GMT`)
- `LineReader`: Fields in HTTP requests are newline-terminated. This struct is given an input buffer and provides methods to read lines as strings.
ACKs for top commit:
maflcko:
review ACK 1911db8c6d👲
furszy:
utACK 1911db8c6d
sedited:
ACK 1911db8c6d
Tree-SHA512: bb8d3b7b18f158386fd391df6d377c9f5b181051dc258efbf2a896c42e20417a1b0b0d4637671ebd2829f6bc371daa15775625af989c19ef8aee76118660deff
This is a helper struct to parse HTTP messages from data in buffers
from sockets. HTTP messages begin with headers which are
CRLF-terminated lines (\n or \r\n) followed by an arbitrary amount of
body data. Whitespace is trimmed from the field lines but not the body.
https://httpwg.org/specs/rfc9110.html#rfc.section.5
https://httpwg.org/specs/rfc9110.html#rfc.section.5.1
Field names in HTTP headers are case-insensitive. These
structs will be used in the headers map to search by key.
In libevent field names are also converted to lowercase for comparison:
evhttp_find_header()
evutil_ascii_strcasecmp()
EVUTIL_TOLOWER_()
faa59b3679 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3 util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430 util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac4800959 util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2 util: Make Expected::value() throw (MarcoFalke)
fa1de1103f util: Add Unexpected::error() (MarcoFalke)
faa109f8be test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke)
Pull request description:
Reviewers requested more member functions In https://github.com/bitcoin/bitcoin/pull/34006.
They are currently unused, but bring the port closer to the original `std::expected` implementation:
* Make `Expected::value()` throw when no value exists
* Add `Unexpected::error()` methods
* Add `Expected<void, E>` specialization
* Add `Expected::value()&&` and `Expected::error()&&` methods
* Add `Expected::swap()`
Also, include a tiny tidy fixup:
* tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check
ACKs for top commit:
stickies-v:
re-ACK faa59b3679
ryanofsky:
Code review ACK faa59b3679. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.
hodlinator:
re-ACK faa59b3679
Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
This aligns it more with SanityCheckAsmap and reduces variable scope.
Also unify asmap casing in SanityCheckAsmap function name.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
This prevents holding the asmap data in memory twice.
The version hash changes due to spans being serialized without their size-prefix (unlike vectors).
Calculate the asmap version only in one place: A dedicated function in util/asmap.
The version was also referred to as asmap checksum in several places. To avoid confusion call it asmap version everywhere.
With respect to `std::unordered_map` documentation, the `Hash` type
defined in the template is over the `Key` and not `T`, the value. This
hasher is incorrectly named as the `FilterHeader` is the value within this map.
I consider this a bug as opposed to a refactor as the key and value
relationship is implied to be `filter header -> block hash` when it is
the opposite.
Further, the hasher for the key already exists via `BlockHasher`.
ref: https://en.cppreference.com/w/cpp/container/unordered_map.html
`IWYU pragma: export` enforces the transitive inclusion of the headers,
which undermines the purpose of IWYU.
The remained cases seem useful and could be considered separately:
- `<cassert>` in `util/check.h`
- `<filesystem>` in `util/fs.h`
- `<chrono>` in `util/time.h`
This is not expected to be needed in this codebase, but brings the
implementation closer to std::expected::value().
Also, add noexcept, where std::expected has them. This will make
operator-> and operator* terminate, when has_value() is false.
fa4cb13b52 test: [doc] Manually unify stale headers (MarcoFalke)
fa5f297748 scripted-diff: [doc] Unify stale copyright headers (MarcoFalke)
Pull request description:
Historically, the upper year range in file headers was bumped manually
or with a script.
This has many issues:
* The script is causing churn. See for example commit 306ccd4, or
drive-by first-time contributions bumping them one-by-one. (A few from
this year: https://github.com/bitcoin/bitcoin/pull/32008,
https://github.com/bitcoin/bitcoin/pull/31642,
https://github.com/bitcoin/bitcoin/pull/32963, ...)
* Some, or likely most, upper year values were wrong. Reasons for
incorrect dates could be code moves, cherry-picks, or simply bugs in
the script.
* The upper range is not needed for anything.
* Anyone who wants to find the initial file creation date, or file
history, can use `git log` or `git blame` to get more accurate
results.
* Many places are already using the `-present` suffix, with the meaning
that the upper range is omitted.
To fix all issues, this bumps the upper range of the copyright headers
to `-present`.
Further notes:
* Obviously, the yearly 4-line bump commit for the build system (c.f.
b537a2c02a) is fine and will remain.
* For new code, the date range can be fully omitted, as it is done
already by some developers. Obviously, developers are free to pick
whatever style they want. One can list the commits for each style.
* For example, to list all commits that use `-present`:
`git log --format='%an (%ae) [%h: %s]' -S 'present The Bitcoin'`.
* Alternatively, to list all commits that use no range at all:
`git log --format='%an (%ae) [%h: %s]' -S '(c) The Bitcoin'`.
<!--
* The lower range can be wrong as well, so it could be omitted as well,
but this is left for a follow-up. A previous attempt was in
https://github.com/bitcoin/bitcoin/pull/26817.
ACKs for top commit:
l0rinc:
ACK fa4cb13b52
rkrux:
re-ACK fa4cb13b52
janb84:
ACK fa4cb13b52
Tree-SHA512: e5132781bdc4417d1e2922809b27ef4cf0abb37ffb68c65aab8a5391d3c917b61a18928ec2ec2c75ef5184cb79a5b8c8290d63e949220dbeab3bd2c0dfbdc4c5
d9319b06cf refactor: unify container presence checks - non-trivial counts (Lőrinc)
039307554e refactor: unify container presence checks - trivial counts (Lőrinc)
8bb9219b63 refactor: unify container presence checks - find (Lőrinc)
Pull request description:
### Summary
Instead of counting occurrences in sets and maps, the C++20 `::contains` method expresses the intent unambiguously and can return early on first encounter.
### Context
Applied clang‑tidy's [readability‑container‑contains](https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html) check, though many cases required manual changes since tidy couldn't fix them automatically.
### Changes
The changes made here were:
| From | To |
|------------------------|------------------|
| `m.find(k) == m.end()` | `!m.contains(k)` |
| `m.find(k) != m.end()` | `m.contains(k)` |
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 1` | `!m.contains(k)` |
| `m.count(k) == 1` | `m.contains(k)` |
| `m.count(k) < 1` | `!m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
> Note that `== 1`/`!= 1`/`< 1` only apply to simple [maps](https://en.cppreference.com/w/cpp/container/map/contains)/[sets](https://en.cppreference.com/w/cpp/container/set/contains) and had to be changed manually.
There are many other cases that could have been changed, but we've reverted most of those to reduce conflict with other open PRs.
-----
<details>
<summary>clang-tidy command on Mac</summary>
```bash
rm -rfd build && \
cmake -B build \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-path)" \
-DCMAKE_C_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_CXX_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON
"$(brew --prefix llvm)/bin/run-clang-tidy" -quiet -p build -j$(nproc) -checks='-*,readability-container-contains' | grep -v 'clang-tidy'
```
</details>
Note: this is a take 2 of https://github.com/bitcoin/bitcoin/pull/33094 with fewer contentious changes.
ACKs for top commit:
optout21:
reACK d9319b06cf
sedited:
ACK d9319b06cf
janb84:
re ACK d9319b06cf
pablomartin4btc:
re-ACK d9319b06cf
ryanofsky:
Code review ACK d9319b06cf. I manually reviewed the full change, and it seems there are a lot of positive comments about this and no more very significant conflicts, so I will merge it shortly.
Tree-SHA512: e4415221676cfb88413ccc446e5f4369df7a55b6642347277667b973f515c3c8ee5bfa9ee0022479c8de945c89fbc9ff61bd8ba086e70f30298cbc1762610fe1
Replaces standalone `SipHashUint256` with an `operator()` overload in `PresaltedSipHasher`.
Updates all hasher classes (`SaltedUint256Hasher`, `SaltedTxidHasher`, `SaltedWtxidHasher`) to use `PresaltedSipHasher` internally, enabling the same constant-state caching optimization while keeping behavior unchanged.
Benchmark was also adjusted to cache the salting part.
faa23738fc refactor: Enable clang-tidy bugprone-unused-return-value (MarcoFalke)
fa114be27b Add util::Expected (std::expected) (MarcoFalke)
Pull request description:
Some low-level code could benefit from being able to use `std::expected` from C++23:
* Currently, some code is using `std::optional<E>` to denote an optional error. This is fine, but a bit confusing, because `std::optional` is normally used for values, not errors. Using `std::expected<void, E>` is clearer.
* Currently, some code is using `std::variant<V, E>` to denote either a value or an error. This is fine, but a bit verbose, because `std::variant` requires a visitor or get_if/holds_alternative instead of a simple call of the `operator bool` for `std::expected`.
In theory, `util::Result` could be taught to behave similar to `std::expected` (see https://github.com/bitcoin/bitcoin/pull/34005). However, it is unclear if this is the right approach:
* `util::Result` is mostly meant for higher level code, where errors come with translated error messages.
* `std::expected` is mostly meant for lower level code, where errors could be an enum, or any other type.
* https://github.com/bitcoin/bitcoin/pull/25665 aims to minimize the memory footprint of the error by wrapping it in a unique_ptr internally. `std::expected` requires the value and error to be "nested within it" (https://cplusplus.github.io/LWG/issue4141). So from a memory-layout perspective, the two are not compatible.
* `std::expected` also comes with `std::unexpected`, which also does not map cleanly to `util::Result`.
So just add a minimal drop-in port of `std::expected`.
ACKs for top commit:
romanz:
tACK faa23738fc
sedited:
Re-ACK faa23738fc
hodlinator:
ACK faa23738fc
rkrux:
light Code Review ACK faa23738fc
ryanofsky:
Code review ACK faa23738fc, only adding `value_or` implementation and `CheckedReturnTypes` clang-tidy commit since last review.
stickies-v:
ACK faa23738fc
Tree-SHA512: fdbd0f6bf439738ffe6a68da5522f1051537f8df9c308eb90bef6bd2e06931d79f1c5da22d5500765e9cb1d801d5be39e11e10d47c9659fec1a8c8804cb7c872
48840bfc2d refactor: Prefer `<=>` over multiple relational operators (Daniel Pfeifer)
5a0f49bd26 refactor: Remove all `operator!=` definitions (Daniel Pfeifer)
Pull request description:
Remove all `operator!=` definitions and provide `operator<=>` as a replacement where all relational comparison operators were defined before.
The compiler is able to deduce missing comparison operators from `operator!=` and `operator<=>`. The compiler provided operators have the following advantages:
1. less code
2. guaranteed consistency
Refactoring that changes the implementation, or replaces it with `= default` is left for a separate PR.
ACKs for top commit:
optout21:
utACK 48840bfc2d
Chand-ra:
tACK [`48840bf`](48840bfc2d). Built the PR and ran unit tests; everything passes.
maflcko:
review ACK 48840bfc2d🌖
stickies-v:
utACK 48840bfc2d. Pretty straightforward cleanup taking advantage of C++20 improvements, nice.
janb84:
ACK 48840bfc2d
sipa:
ACK 48840bfc2d
Tree-SHA512: 7fedc4abc451c7ad611e3a960ff939a35580667222009cb30ca546e564dc9161e3e8d4d1d7d44c538d961cc8f7adba6e6dbcebcd1be370bf33aef294d06f236b
