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
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
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
577a3e74c8 test: Add check for return type in `HasToBytes` concept (yuvicc)
1ad551281a kernel: Add Block Header serialization method (yuvicc)
86662623ec Add `SpanWriter` class for zero-allocation stream writing (yuvicc)
Pull request description:
This adds serialization for `btck_BlockHeader` API. Also, updated the `CheckHandle` to compare the byte content instead of size.
The changes here is done in two commits. First commit adds the `SpanWriter` class and next one moves the block header serialization to `SpanWriter`. See commit message for more details.
Follow-up to #33822 .
ACKs for top commit:
stickies-v:
re-ACK 577a3e74c8
alexanderwiederin:
ACK 577a3e74c8
theStack:
Code-review ACK 577a3e74c8
w0xlt:
ACK 577a3e74c8
Tree-SHA512: 1eda5b204588ccb23e9357f68c5529474e7d248736a371c47d8db71ba6ca95e121869514478ad7a519d190e4c30725f64fd1ef4dd9f97d2627dc4441e51458e0
Add `btck_block_header_to_bytes` serialization method to serialize a
`btck_BlockHeader` into an 80-byte buffer using `SpanWriter` to ensure
zero-allocation serialization.
0587c56091 kernel: Expose context-free block validation (w0xlt)
71f827c3c2 kernel: Expose consensus parameters (`btck_ConsensusParams`) (w0xlt)
Pull request description:
This PR exposes Bitcoin Core’s context‑free block checks to library users via a new C API entry point, `btck_check_block_context_free`.
Callers can validate a block’s structure (size/weight, coinbase rules, per‑tx context‑free checks) and optionally re‑run Proof‑of‑Work and Merkle‑root verification without touching chainstate, the block index, or the UTXO set.
Rationale
Clients embedding the kernel need a pure block sanity check without requiring node state or disk writes (candidate block validation, for example). This API offers that surface in a single call, with optional PoW/Merkle toggles to avoid redundant work when the header has already been validated or when Merkle verification is deferred.
ACKs for top commit:
yuvicc:
re-ACK 0587c56091
achow101:
ACK 0587c56091
sedited:
ACK 0587c56091
Tree-SHA512: 6bd53e4964909335d1f2fee30ff96c95a8dd2c84bcdfe11c50ba369301822e5dea9bbe2376bb6d6b4652875152071eb0446657042b00429f29581da4fcea71a9
75608547b4 kernel: Remove NONNULL annotation from destroy method (Alexander Wiederin)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/33796#discussion_r2959817508
### Summary
This PR removes the `BITCOINKERNEL_ARG_NONNULL` annotation from the `btck_block_validation_state_destroy` method in the Kernel API.
### Motivation
No other *_destroy function in the Kernel API carries the NONNULL annotation. Following the convention set by free(), destroy functions should accept null pointers.
### Usage:
Before:
```c
btck_BlockValidationState* state = NULL;
btck_block_validation_state_destroy(state); // violates nonnull contract
```
After:
```c
btck_BlockValidationState* state = NULL;
btck_block_validation_state_destroy(state); // well-defined
```
ACKs for top commit:
yuvicc:
lgtm! ACK 75608547b4
kevkevinpal:
ACK [7560854](75608547b4)
achow101:
ACK 75608547b4
janb84:
ACK 75608547b4
theStack:
ACK 75608547b4
w0xlt:
ACK 75608547b4
stickies-v:
ACK 75608547b4
Tree-SHA512: 9577098940f49b8f7fd1fb20afe750e9eaaf9d90d25ec3c4f423de223a1fbd0bca6d520061f576eb3923f18e1f2744b3f60d0d0715dfa3832fea83cb538170c2
No other *_destroy function in the Kernel API carries this annotation.
Following the convention set by free(), destroy functions should accept
null pointers.
3129d4a693 ci: Rename `TIDY_LLVM_V` to `IWYU_LLVM_V` in IWYU-specific code (Hennadii Stepanov)
0b489886f8 ci: Upgrade IWYU to 0.26 compatible with Clang 22 (Hennadii Stepanov)
Pull request description:
This PR upgrades IWYU to [0.26](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.26) and removes mapping workarounds for issues that have been fixed upstream.
ACKs for top commit:
fanquake:
ACK 3129d4a693
Tree-SHA512: 9a926e489573d040423461c039ecda7636beb70e8214a02c1c594cbd5b89d26331455a9872c38993fa5ee6d27fdfefc2375dedc369721b2933411542a57a3884
This introduces a context-free validation entry point for full blocks in
the kernel C and C++ APIs.
* Add `btck_block_check`, a C function that wraps `CheckBlock` and runs
header and body checks for a `btck_Block` using `btck_ConsensusParams`.
Callers provide a `btck_BlockValidationState` to receive the result
and supply a `btck_BlockCheckFlags` bitmask to control POW and
merkle-root verification.
* Add `btck_BlockCheckFlags` in the C API, plus the corresponding
`BlockCheckFlags` scoped enum in the C++ wrapper, including a
`*_ALL` convenience value.
* Add `Block::Check()` to the C++ wrapper to mirror the new C function
and return a bool while filling a `BlockValidationState`.
* Add a test `(btck_check_block_context_free)` that verifies a known
valid mainnet block passes with `BlockCheckFlags::ALL` and that
truncated block data fails deserialization.
Co-authored-by: yuvicc <yuvichh01@gmail.com>
Library users currently need to maintain a full context object to perform
context-free block validation. Exposing an opaque `btck_ConsensusParams`
struct allows callers to supply only the required consensus parameters,
resulting in a lighter-weight API and a clearer expression of the actual
validation behavior.
Co-authored-by: yuvicc <yuvichh01@gmail.com>
This patch addresses my own review comments from the review of PR 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.
0fe6fccec2 doc: Document rationale for using `IWYU pragma: export` (Hennadii Stepanov)
cfa3b10d50 iwyu, doc: Document `IWYU pragma: export` for `<logging/categories.h>` (Hennadii Stepanov)
015bea05e6 iwyu, doc: Document `IWYU pragma: export` for `<chrono>` (Hennadii Stepanov)
48bfcfedec iwyu, doc: Document `IWYU pragma: export` for `<threadsafety.h>` (Hennadii Stepanov)
179abb387f refactor: Move `StdMutex` to its own header (Hennadii Stepanov)
6d2952c3c3 serialize: Add missing `<span>` header (Hennadii Stepanov)
Pull request description:
This PR is a prerequisite for https://github.com/bitcoin/bitcoin/pull/34448. It was split into a separate PR to limit the scope and minimize potential merge conflicts.
The first commit improves the accuracy of IWYU suggestions within our heavily templated code. Note that, for now, the `serialize.h` header itself is excluded from IWYU inspection because it lacks a corresponding source file.
The remaining commits follow the Developer Notes [guidance](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu):
> Use `IWYU pragma: export` very sparingly, as this enforces transitive inclusion of headers and undermines the specific purpose of IWYU.
ACKs for top commit:
maflcko:
review ACK 0fe6fccec2👤
ajtowns:
utACK 0fe6fccec2
Tree-SHA512: dc2d4e3ff78b9707a1a26cb9b1c0a456de0d33c59e773bbf692344c2fceaff8936317479c5e898038f29134bc0e5d9d1ef7350e53512dd8e262f46ede578c4f9
74f71c5054 Remove Taproot activation height (Sjors Provoost)
Pull request description:
Drop `DEPLOYMENT_TAPROOT` from `consensus.vDeployments`.
Now that the commit (7c08d81e11) which changes taproot to be enforced for all blocks is included in v24.0, it seems a good time to remove no longer needed non-consensus code.
Clarify what is considered a `BuriedDeployment`.
We drop taproot from `getdeploymentinfo` RPC, rather than mark it as `buried`, because this is not a buried deployment in the sense of [BIP 90](https://github.com/bitcoin/bips/blob/master/bip-0090.mediawiki). This is because the activation height has been completely removed from the code, unlike the hardcoded `DEPLOYMENT_SEGWIT` height which is still relied on.[^1]
See discussion in #24737 and #26162.
[^1]: `DEPLOYMENT_SEGWIT` is used in `IsBlockMutated` to determine if witness data is allowed, it's used for [BIP147](https://github.com/bitcoin/bips/blob/master/bip-0147.mediawiki), to trigger `NeedsRedownload()` for users who upgraded after a decade, and for a few other things.
ACKs for top commit:
ajtowns:
reACK 74f71c5054
achow101:
ACK 74f71c5054
sedited:
Re-ACK 74f71c5054
darosior:
utACK 74f71c5054
Tree-SHA512: 217e0ee2e144ccfb04cf012f45b75d08f8541287a5531bd18aa81e38bad2f38d28b772137f471c46b63875840ec044cb61a2a832e3a9e89a6183e8ab36b1b9c9
9f28120a5b kernel: Add API function for getting a tx input's nSequence (Sebastian Falbesoner)
6b64b181d5 kernel: Add API function for getting a tx's nLockTime (Sebastian Falbesoner)
Pull request description:
This PR introduces two new C API functions to libbitcoinkernel:
* `btck_transaction_get_locktime` to access a transaction's `nLockTime` value
* `btck_transaction_input_get_sequence` to access a transaction input's `nSequence` value
Inspired by https://bnoc.xyz/t/forward-compatible-coinbase-locktimes-for-bip-54. After reading this I thought it would be a nice/useful showcase to check BIP54 compliance of (historical) blocks using bitcoinkernel, without having to manually deserialize the transaction (this is just about one of the four BIP54 rules though, especially the sigops limit is much more involved).
ACKs for top commit:
sedited:
ACK 9f28120a5b
yuvicc:
ACK 9f28120a5b
stickies-v:
ACK 9f28120a5b
Tree-SHA512: 9eae795d6e4b9b367bbfe2665b916121ef64031e8d10667c71741344b5eea4c2562862a937bdf1363cc66b67bb5d48392c9f44e52f0d92d2a5a65e10d061b703
f3bf63ec4f kernel: acquire coinstats cursor and block info atomically (w0xlt)
5e77072fa6 rpc: fix race condition in gettxoutsetinfo (w0xlt)
Pull request description:
Fixes#34263
A `CHECK_NONFATAL` assertion failure could occur in `gettxoutsetinfo` when a new block was connected between capturing `pindex` and validating it in `GetUTXOStats()`.
The fix removes the early `pindex` capture since `ComputeUTXOStats()` independently fetches the current best block under lock. The response now uses `stats.hashBlock` and `stats.nHeight` (the actual computed values) instead of the potentially stale `pindex`.
ACKs for top commit:
sedited:
ACK f3bf63ec4f
fjahr:
utACK f3bf63ec4f
rkrux:
Concept ACK f3bf63ec4f for removal of the race condition.
Tree-SHA512: c2d5cd5a1b4b4f1c22023c03970fea400a0b78005fa3d09d6567255615ab461c01b584d8a158651ee08769ec86fc4a1200f640ad58fdaa4879c335d90c891f6a
af0da2fce2 crypto: Use `secure_allocator` for `AES256CBC*::iv` (David Gumberg)
d53852be31 crypto: Use `secure_allocator` for `AES256_ctx` (David Gumberg)
8c6fedaa81 build: `lockedpool.cpp` kernel -> crypto (David Gumberg)
51ac1abf6f bench: Add wallet encryption benchmark (David Gumberg)
9a15872516 wallet: Make encryption derivation clock mockable (David Gumberg)
ae5485fa0d refactor: Generalize derivation target calculation (David Gumberg)
Pull request description:
Fixes#31744
Reuse `secure_allocator` for `AES256_ctx` in the aes 256 encrypters and decrypters and the `iv` of `AES256CBC` encrypters and decrypters. These classes are relevant to `CCrypter`, used for encrypting wallets, and my understanding is that if an attacker knows some or all of the contents of these data structures (`AES256_ctx` & `iv`) they might be able to decrypt a user's wallet.
Presently the `secure_allocator` tries to protect sensitive data with `mlock()` on POSIX systems and `VirtualLock()` on Windows to prevent memory being paged to disk, and by zero'ing out memory contents on deallocation with `memory_cleanse()` which is similar to `OPENSSL_cleanse()` by scaring compilers away from optimizing `memset` calls on non-Windows systems, and using `SecureZeroMemory()` on Windows.
ACKs for top commit:
achow101:
ACK af0da2fce2
furszy:
utACK af0da2fce2
theStack:
re-ACK af0da2fce2
Tree-SHA512: 49067934fd2f2b285fc7b1a7c853fd2d4475431b3a811ae511f61074dc71a99a0826c3ab40ab4a5dfc84b2b9914a90c920d2484b38ac19502e3bd6170ad27622
Acquire the cursor and block index under the same cs_main lock to
eliminate a potential race where a new block could be connected
between capturing the block info and acquiring the cursor, causing
the reported stats to reference a different block than the one
being iterated.
Drop DEPLOYMENT_TAPROOT from consensus.vDeployments.
Bump MinBIP9WarningHeight.
Clarify what is considered a BuriedDeployment and
drop taproot from getdeploymentinfo RPC.
Add a test to getblocktemplate to ensure the taproot
rule is still set.
Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
eafd530d20 kernel: avoid potential duplicate object in shared library/binary (Cory Fields)
24c3b47010 build: add kernel-specific warnings (Cory Fields)
Pull request description:
This is a revival of https://github.com/bitcoin/bitcoin/pull/31807
Introduces the [-Wunique-object-duplication](https://clang.llvm.org/docs/DiagnosticsReference.html#wunique-object-duplication) warning flag available in clang-21 for usage when building the kernel library. It warns of potential duplicate objects in shared libraries. REDUCE_EXPORTS needs to be ON to trigger it.
Though we have a C API now that manages exporting symbols, I think it is prudent to also avoid any duplicate symbols on the internal c++ side in case we ever to decide to expose some of its headers. It also not clear that all linkers would handle these cases correctly even in the current internal usage.
ACKs for top commit:
fanquake:
ACK eafd530d20
hebasto:
ACK eafd530d20.
Tree-SHA512: 81961b50f0268dbe076497e130857f5b4b9151c748d107ec15158d1511dd25bce745e0beeb127b9cea51cb2edd78032735600606a75f7ff8a3fd572acced42e0
In some cases, we'll want to be more aggressive or care about different things
when building the kernel. In this case, a warning is added for symbols which
may be duplicated between the kernel and downstream users.
This warning was introduced in clang 21, which is not yet the minimum
supported compiler version. REDUCE_EXPORTS needs to be ON to trigger it.
6f113cb184 txgraph: use fallback order to sort chunks (feature) (Pieter Wuille)
0a3351947e txgraph: use fallback order when linearizing (feature) (Pieter Wuille)
fba004a3df txgraph: pass fallback_order to TxGraph (preparation) (Pieter Wuille)
941c432a46 txgraph test: subclass TxGraph::Ref like mempool does (preparation) (Pieter Wuille)
39d0052cbf clusterlin: make optimal linearizations deterministic (feature) (Pieter Wuille)
8bfbba3207 txgraph: sort distinct-cluster chunks by equal-feerate-prefix size (feature) (Pieter Wuille)
e0bc73ba92 clusterlin: sort tx in chunk by feerate and size (feature) (Pieter Wuille)
6c1bcb2c7c txgraph: clear cluster's chunk index in ~Ref (preparation) (Pieter Wuille)
7427c7d098 txgraph: update chunk index on Compact (preparation) (Pieter Wuille)
3ddafceb9a txgraph: initialize Ref in AddTransaction (preparation) (Pieter Wuille)
Pull request description:
Part of #30289.
TxGraph's fundamental responsibility is deciding the order of transactions in the mempool. It relies on the `cluster_linearize.h` code to optimize it, but there can and often will be many different orderings that are essentially equivalent from a quality perspective, so we have to pick one. At a high level, the solution will involve one or more of:
* Deciding based on **internal identifiers** (`Cluster::m_sequence`, `DepGraphIndex`). This is very simple, but risks leaking information about transaction receive order.
* Deciding **randomly**, which is private, but may interfere with relay expectations, block propagation, and ability to monitor network behavior.
* Deciding **based on txid**, which is private and deterministic, but risks incentivizing grinding to get an edge (though we haven't really seen such behavior).
* Deciding **based on size** (e.g. prefer smaller transactions), which is somewhat related to quality, but not unconditionally (depending on mempool layout, the ideal ordering might call for smaller transactions first, last, or anywhere in between). It's also not a strong ordering as there can be many identically-sized transactions. However, if it were to encourage grinding behavior, incentivizing smaller transactions is probably not a bad thing.
As of #32545, the current behavior is primarily picking randomly, though inconsistently, as some code paths also use internal identifiers and size. #33335 sought to change it to use random (preferring size in a few places), with the downsides listed above.
This PR is an alternative to that, which changes the order to tie-break based on size everywhere possible, and use lowest-txid-first as final fallback. This is fully deterministic: for any given set of mempool transactions, if all linearized optimally, the transaction order exposed by TxGraph is deterministic.
The transactions within a chunk are sorted according to:
1. `PostLinearize` (which improves sub-chunk order), using an initial linearization created using the rules 2-5 below.
2. Topology (parents before children).
3. Individual transaction feerate (high to low)
4. Individual transaction weight (small to large)
5. Txid (low to high txid)
The chunks within a cluster are sorted according to:
1. Topology (chunks after their dependencies)
2. Chunk feerate (high to low)
3. Chunk weight (small to large)
4. Max-txid (chunk with lowest maximum-txid first)
The chunks across clusters are sorted according to:
1. Feerate (high to low)
2. Equal-feerate-chunk-prefix weight (small to large)
3. Max-txid (chunk with lowest maximum-txid first)
The equal-feerate-chunk-prefix weight of a chunk C is defined as the sum of the weights of all chunks in the same cluster as C, with the same feerate as C, up to and including C itself, in linearization order (but excluding such chunks that appear after C). This is a well-defined approximation of sorting chunks from small to large across clusters, while remaining consistent with intra-cluster linearization order.
ACKs for top commit:
ajtowns:
reACK 6f113cb184 it was good before and now it's better
instagibbs:
ACK 6f113cb184
marcofleon:
light crACK 6f113cb184
Tree-SHA512: 16dc43c62b7e83c81db1ee14c01e068ae2f06c1ffaa0898837d87271fa7179dd98baeb74abc9fe79220e01fdba6876defe60022c2b72badc21d770644a0fe0ac
Instead of returning a TxGraph::Ref from TxGraph::AddTransaction(),
pass in a TxGraph::Ref& which is updated to refer to the new transaction
in that graph.
This cleans up the usage somewhat, avoiding the need for dummy Refs in
CTxMemPoolEntry constructor calls, but the motivation is that a future
commit will allow a callback to passed to MakeTxGraph to define a
fallback order on the transaction objects. This does not work when a
Ref is created separately from the CTxMemPoolEntry it ends up living in,
as passing the newly-created Ref to the callback would be UB before it's
emplaced in its final CTxMemPoolEntry.
Serialize `Coin` metadata using the canonical (height << 1) | coinbase packing across `Coin` serialization, undo records, and coinstats hashing.
Cast the 31-bit `nHeight` bitfield to `uint32_t` before shifting to avoid signed promotion undefined behaviour.
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
fa0677d131 refactor: Use SpanReader over DataStream (MarcoFalke)
fad3eb3956 refactor: Use SpanReader over DataStream (MarcoFalke)
fa06e26764 refactor: [qt] Use SpanReader to avoid two vector copies (MarcoFalke)
fabd4d2e2e refactor: Avoid UB in SpanReader::ignore (MarcoFalke)
fa20bc2ec2 refactor: Use empty() over eof() in the streams interface (MarcoFalke)
fa879db735 test: Read debug log for self-checking comment (MarcoFalke)
Pull request description:
This changes all places, where possible, to use SpanReader over DataStream. This makes the code easier to read and reason about, because `SpanReader` can never write data. Also, the code should be minimally faster, because it avoids a full redundant copy of the whole vector of bytes.
ACKs for top commit:
stickies-v:
re-ACK fa0677d131
achow101:
ACK fa0677d131
janb84:
re ACK fa0677d131
sipa:
crACK fa0677d131
Tree-SHA512: 1d9f43fc6e71d481cf7b8f8457f479745ee331734649e9e2c2ab00ce5d317112796c77afc328612ed004e65ac5c16fc92279d760cfb012cfddce9098c4af810f
This refactor does not change behavior. However, it avoids a vector
copy, which can lead to a minimal speed-up of 1%-5%, depending on the
call-site. This is mostly relevant for the fuzz tests and utils that
read large blobs of data (like a full block).
a50d0b6720 build: don't pass on boost dependency to kernel consumers (Cory Fields)
Pull request description:
This is unnecessary now that the kernel now exports a (boost-less) API.
Noticed while slimming down boost dependencies in #34495.
ACKs for top commit:
stickies-v:
ACK a50d0b6720
hebasto:
ACK a50d0b6720, I have reviewed the code and it looks OK. I tested it by applying the Boost-specifc commits from https://github.com/bitcoin/bitcoin/pull/34143 and building with depends.
Tree-SHA512: e2d12356f41dd51dd729362121a33bd4f395821d53dd9a0bb0d5d6a53aba2ca2064e0709d9799dc6751b3d61ea576d2efc0e28296fdba26f2809dbcb0feabe44
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.
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
