18c1cc65e9 kernel: improve BITCOINKERNEL_WARN_UNUSED_RESULT usage (stickies-v)
Pull request description:
Similar to `[[nodiscard]]`, `BITCOINKERNEL_WARN_UNUSED_RESULT` is used to indicate that ignoring a function's return value is almost certainly a bug.
It is used in cases such as a resource leak (e.g. an owning handle returned by a `*_create` or `*_copy` function), or when the returned value is itself an error/status code. It is not used merely because discarding the result is wasteful, e.g. on getters or predicates.
Fix the incorrect usage, and properly document the attribute. Having this clearly documented helps avoid bikeshedding on future PRs.
ACKs for top commit:
w0xlt:
ACK 18c1cc65e9
stringintech:
ACK 18c1cc65
sedited:
ACK 18c1cc65e9
Tree-SHA512: ba5aa19f0b90ef12ed3c200449734d9aa67af166f2a2b29e321d3ec1e9b49483215c5078ae1dd945ea65650a2017f3101bf75abcc9d3388eb1a3d4c8070c22dd
Similar to [[nodiscard]], BITCOINKERNEL_WARN_UNUSED_RESULT is used
to indicate that ignoring a function's return value is almost
certainly a bug.
It is used in cases such as a resource leak (e.g. an owning handle returned
by a *_create or *_copy function), or when the returned value is itself an
error/status code. It is not used merely because discarding the result is
wasteful, e.g. on getters or predicates.
Fix the incorrect usage, and properly document the attribute.
6189335f6b kernel: doc: document wipe lifecycle and best entry nullability (csjones)
Pull request description:
Document on `btck_chainstate_manager_options_set_wipe_dbs` that a wipe must be followed by `btck_chainstate_manager_import_blocks` before the chainstate manager is used for anything else, as the existing kernel tests already do (specifically the `chainman_reindex*` kernel tests). Note in the `@return` of `btck_chainstate_manager_get_best_entry` that it can return null when no block headers have been loaded.
Background: I've been working on a bindings project using the libbitcoinkernel and tripped on a SIGSEGV calling the entry accessors on the null pointer returned by `get_best_entry` after a `(true, true)` wipe (#35293). The C++ wrapper handles this via `btck::check<>`, but bindings generated from the C header don't see the wrapper. Adding a nullability documentation hint helps generators produce the correct signature, and the `@note` on `set_wipe_dbs` documents the lifecycle that avoids the null in the first place.
Docs-only change; no tests ran.
ACKs for top commit:
sedited:
ACK 6189335f6b
alexanderwiederin:
ACK 6189335f6b
Tree-SHA512: 319d9704c9857c8eb12e8726d16710d8b5777404f8ecf50d832a5e5b0a9b9f0d8321074ecb8b985e5b03a6eb7119cc4eb73f36d096dc0149a73de7d6a74de4cb
Document btck_transaction_check and btck_block_check validation state output parameters as overwritten in-place. This matches their reset-on-entry behavior and avoids implying callers should preserve prior state.
88d9bc5aa4 kernel: Return btck_BlockValidationState from process_block_header API (yuvicc)
Pull request description:
This PR refactors `btck_chainstate_manager_process_block_header` to return `btck_BlockValidationState` by value instead of using out-parameters or boolean returns.
ACKs for top commit:
optout21:
ACK 88d9bc5aa4
stickies-v:
ACK 88d9bc5aa4
w0xlt:
reACK 88d9bc5aa4
hodlinator:
re-ACK 88d9bc5aa4
Tree-SHA512: f86b6e85aedafd78ae250930cbe34dc666c14d800e43cf8582d49aecb97faab801eff8dcc0250082ceebc3e8d32949839e030cf9f0023b56b23c8f7b7a741e49
Remove redundant int return from btck_chainstate_manager_process_block_header.
Previously returned both an int result and an output validation state parameter, creating ambiguity
where non-zero could mean either invalid header or processing failure. Since ProcessNewBlockHeaders
already provides complete validation info, the int return was redundant.
Co-authored-by: stringintech <stringintech@gmail.com>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Document on `btck_chainstate_manager_options_set_wipe_dbs` that a wipe must be followed by `btck_chainstate_manager_import_blocks` before the chainstate manager is used for anything else, as the existing kernel tests already do (e.g. `chainman_reindex_test`).
Note in the `@return` of `btck_chainstate_manager_get_best_entry` that it can return null when no block headers have been loaded.
Refactor the `@return` documentation to fit on a single line.
Drop the include_dummy_extranonce branch from the OP_0 padding
condition in CreateNewBlock(), so that the dummy extraNonce is
only appended when consensus actually requires it (heights <= 16,
where the BIP34 height push alone would yield a 1-byte scriptSig
and trigger bad-cb-length).
The include_dummy_extranonce option struct field is now unused by
the miner and is removed in the next commit. Callers still set it,
so that this commit compiles.
Regenerate the hardcoded coinbase / block hashes throughout the
unit and functional test suites and update the regtest assumeutxo
snapshot in chainparams.
Additional side-effects:
- Without the dummy extranonce, coinbase scriptSigs are 1 byte
shorter at heights > 16, making every block 1 byte smaller.
This shifts where block files wrap and therefore where pruning
boundaries land.
- feature_assumeutxo malleation cases:
- case 1: error message changes due to UTXO reordering, similar
to 8f2078af6a
- case 4: the corruption byte is swapped from \x82 to \x83
because \x82 happened to be the actual value at that
offset in the new snapshot.
Add btck_transaction_check() to the libbitcoinkernel C API, exposing
context-free transaction consensus validation (consensus/tx_check.h).
Introduces btck_TxValidationState with introspection and lifecycle
functions. btck_TxValidationResult is exposed for compatibility with
existing validation-state APIs, though btck_transaction_check currently
reaches only UNSET and CONSENSUS.
Includes C++ wrapper and test coverage for btck_transaction_check using
test vectors from tx_valid.json / tx_invalid.json.
14547eb489 kernel: guard btck::Handle move-assignment against self-move (Thomas)
Pull request description:
The move-assignment operator for `btck::Handle<>` in `src/kernel/bitcoinkernel_wrapper.h` unconditionally called `DestroyFunc(m_ptr)` before reading the source pointer. On a self-move (`h = std::move(h)`), this destroys the held resource and then restores the now-dangling pointer via `std::exchange(other.m_ptr, nullptr)` (since `&other == this`), which leaves `m_ptr` pointing at freed memory. The destructor then calls `DestroyFunc` again on it, resulting in a double-free.
Trace of `h = std::move(h)` with the old code, where `h.m_ptr == P`:
1. `DestroyFunc(m_ptr)` -> `delete P`. `this->m_ptr` still literally stores the now-dangling value `P`.
2. `std::exchange(other.m_ptr, nullptr)` — because `&other == this`, this reads the dangling `P` back, writes `nullptr` to `m_ptr`, and returns `P`.
3. `m_ptr = P` restores the dangling pointer.
4. `~Handle()` later runs `DestroyFunc(P)` -> double free, UB.
The copy-assignment operator already guards against self-assignment with `if (this != &other)`; the move variant should be symmetric. The standard library requires moved-from objects to be in a valid (at minimum, safely destructible) state, which the previous implementation violated when source and destination alias.
`Handle<>` is the base class of 16 public types in the kernel C++ API wrapper (`Transaction`, `Block`, `BlockHeader`, `ChainParams`, `Context`, `Coin`, `BlockValidationState`, `ScriptPubkey`, `TransactionOutput`, `Txid`, `OutPoint`, `TransactionInput`, `PrecomputedTransactionData`, `BlockHash`, `BlockSpentOutputs`, `TransactionSpentOutputs`), so self-move can arise from generic algorithms operating on containers of these types (`std::sort`, `std::remove`, erase-remove idioms, etc.).
Fix: mirror the copy-assignment pattern by guarding the move-assignment body with `if (this != &other)`, making a self-move a no-op.
Also extend `CheckHandle` in `src/test/kernel/test_kernel.cpp` to exercise self-move-assignment for every `Handle`-derived type, checking that the stored pointer and the serialized bytes (where applicable) are unchanged.
ACKs for top commit:
sedited:
Thanks, ACK 14547eb489
alexanderwiederin:
ACK 14547eb489
yuvicc:
lgtm ACK 14547eb489
Tree-SHA512: 334f5ad3045d5f9b06cc0dd096bc911a992773c59cc469765c2082975a9f7a90f2349b9ad94b4b5127290de1fab2f5424a621384c1b4bb9152de99f5da8ed6aa
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
The move-assignment operator for btck::Handle<> unconditionally called
DestroyFunc(m_ptr) before reading the source pointer. On a self-move
(h = std::move(h)), this destroyed the held resource and then reassigned
the now-dangling pointer back to m_ptr via std::exchange, leading to a
double-free when the object is later destroyed.
Mirror the existing self-check in the copy-assignment operator by
guarding the move-assignment with 'if (this != &other)' so a self-move
becomes a no-op, leaving the object in a valid state as required by the
standard library.
Handle<> is the base of 16 public types in the kernel C++ API wrapper
(Transaction, Block, BlockHeader, ChainParams, Context, Coin,
BlockValidationState, ScriptPubkey, TransactionOutput, Txid, OutPoint,
TransactionInput, PrecomputedTransactionData, BlockHash,
BlockSpentOutputs, TransactionSpentOutputs), so self-move can arise
from generic algorithms operating on containers of these types.
Extend CheckHandle in test_kernel to cover self-move-assignment for
every Handle-derived type.
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
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
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.
Allows callers to jump to any ancestor of a block tree entry by height
using the skiplist-backed GetAncestor, which runs in O(log N) rather
than walking back one entry at a time with btck_block_tree_entry_get_previous.
Includes a C++ convenience wrapper and tests in btck_block_tree_entry_tests.
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>
