9fa4076b20 test: Test merging implicit PSBTv0 with explicit PSBTv0 (w0xlt)
1660c18232 doc: Release notes for psbtv2 (Ava Chow)
470e52a5f8 fuzz: Enforce additional version invariants in PSBT fuzzer (Antoine Poinsot)
5bd0579c09 test: Tests for PSBT AddInput and AddOutput (Ava Chow)
b8b6e7f0c2 tests: Add PSBT unit test for ComputeTimeLock (Ava Chow)
0bc1c2e508 tests: Add test vectors from BIP 370 (Ava Chow)
e0e4dbdeb5 psbt: Change default psbt version to 2 (Ava Chow)
bcc1dca77b Add psbt_version to PSBT RPCs and default to v2 (Ava Chow)
ab38c30195 Implement PSBTv2 field merging (Ava Chow)
93e339e29f Implement PSBTv2 AddInput and AddOutput (Ava Chow)
b39c86ae60 Allow specifying PSBT version in constructor (Ava Chow)
dcc9a3c8df Implement PSBTv2 in decodepsbt (Ava Chow)
5770dbd39f Add PSBT::ComputeLockTime() (Ava Chow)
863cf47b33 Update test_framework/psbt.py for PSBTv2 (Ava Chow)
925161eaf0 Implement PSBTv2 fields de/ser (Ava Chow)
d9cf658ee0 Restrict joinpsbts to PSBTv0 only (Ava Chow)
3da0e16012 Replace PSBT.tx with PSBT::GetUnsignedTx and PSBT::GetUniqueID (Ava Chow)
c568624ff2 psbt: Return std::optional from PrecomputePSBTData (Ava Chow)
092de4f1f6 Replace PSBT::GetInputUTXO with PSBTInput::GetUTXO (Ava Chow)
82c9fe3179 psbt: Use PSBTInput and PSBTOutput fields instead of accessing global tx (Ava Chow)
95897507e9 psbt: AddInput and AddOutput should take only PSBTInput and PSBTOutput (Ava Chow)
1b7d323a72 Add PSBTInput::GetOutPoint (Ava Chow)
543d3e1cdc psbt: add PSBTv2 global tx fields (Ava Chow)
c01c7f068c psbt: Remove default constructor (Ava Chow)
9671aa08c2 psbt: add tx input and output fields in PSBTInput and PSBTOutput (Ava Chow)
990b084f11 Have PSBTInput and PSBTOutput know the PSBT's version (Ava Chow)
7eacc21ff6 psbt: make PSBT structs into classes (Ava Chow)
f926c326bb gui: Store PSBT in std::optional in PSBTOperationsDialog (Ava Chow)
1e2d146b47 psbt: Refactor duplicate key lookup and size checks (Ava Chow)
88384180d3 test: PSBTs should roundtrip through RPCs that do nothing (Ava Chow)
001877500d test: construct psbt with unknown field programmatically (David Gumberg)
0cb884e6df psbt: Fill hash preimages and taproot builder from SignatureData (Ava Chow)
Pull request description:
BIP 370 PSBTv2 introduces several new fields and different invariants for PSBT. This PR implements those new fields and restructures the PSBT implementation to match PSBTv2 but still remain compatible with PSBTv0.
ACKs for top commit:
nervana21:
re-ACK 9fa4076b20
theStack:
re-ACK 9fa4076b20
w0xlt:
ACK 9fa4076b20
Tree-SHA512: ab0a5ada4fa5fca27ba9ec9c291a44b30e69d6db11971957572d86c58c71c4caa4557dc25f403e1170ba4fac751306d074cc582defefc6e2fdd37be51c3d9dd0
c8d688f41c fuzz: send blocktxn messages in cmpctblock harness (Eugene Siegel)
d0333bfe99 fuzz: send compact blocks in cmpctblock harness (Eugene Siegel)
3c58efe2ac fuzz: mine blocks and send headers for them in cmpctblock harness (Eugene Siegel)
651622432d fuzz: create and send transactions in cmpctblock harness (Eugene Siegel)
8c9a3fd0e8 net, fuzz: move CMPCTBLOCK_VERSION to header, use in cmpctblock harness (Eugene Siegel)
6cd480f62f fuzz: initial compact block fuzz harness (Eugene Siegel)
Pull request description:
Adds a fuzz harness for testing compact blocks, similar to `process_message(s)`. It can make (in)valid blocks, reconstruct blocks with in-mempool txns, mark peers as HB, and has high stability in AFL++ (~98-99%).
Coverage is [here](https://crypt-iq.github.io/fuzz_coverage_reports/cmpctblock-aflpp-inputs-09182025/) (look in `src/blockencodings.cpp`, relevant compact block bits in `src/net_processing.cpp`).
ACKs for top commit:
instagibbs:
ACK c8d688f41c
marcofleon:
ACK c8d688f41c
Tree-SHA512: d904066921211b32eb75c9602908929f46ecd25482c518db36ed9f30e012b350b38a2df382f0faa21decd43bc2cabcc859193207da34be76010a32df1d9ac5c8
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.
1aa78cdab6 clusterlin: adopt STL ranges algorithms (refactor) (Pieter Wuille)
747da25360 feefrac: drop comparison and operator{<<,>>} for sorted wrappers (Pieter Wuille)
Pull request description:
Instead of having an unintuitive but strong implicit sort order on `FeeFrac` (first increasing feerate, then decreasing size), and separate overloaded `operator<<` and `operator>>` that implement a weak ordering that only looks at feerate, replace these with explicit wrapper classes which make the behavior more explicit (`ByRatio` and `ByRatioNegSize`).
This allows for things like `ByRatio{a} <= ByRatio{b}`, instead of the earlier `!(a >> b)`. It also supports usage inside `std::min`/`std::max`/`std::less`, and `std::greater`, so one can use:
* `std::max<ByRatioNegSize<FeeFrac>>(a, b)` to get the highest-feerate `FeeFrac`, tie-breaking by smallest size.
* `std::ranges::sort(v, std::greater<ByRatioNegSize<FeeFrac>>{});` to sort a vector that way.
Suggested in https://github.com/bitcoin/bitcoin/pull/34257#discussion_r2780475893.
ACKs for top commit:
achow101:
ACK 1aa78cdab6
sedited:
ACK 1aa78cdab6
ajtowns:
ACK 1aa78cdab6
Tree-SHA512: d76657b15f6d745e5ca01c67fd5b101fdc418e6301646d14e575b6564bfa2fe0eb40a95a7ff95a4420624ef6b67224d35e4713aa5bbc0d293e08fe44c0cc6db0
The global unsigned tx is decomposed into separate fields inside of
PSBT, which mirrors what PSBTv2 will do. However, we still need to get
the global unsigned tx so PSBT::GetUnsignedTx is introduced to do that.
In order to also have a stable unique ID, we also introduce
PSBT::GetUniqueID to replace uses of PSBT.tx.GetHash().
PSBTInput now has the previous txid and output index, and PSBTOutput has
the amount and script. We no longer need to access the global unsigned
tx for these fields.
Additionally, we can change iterating tx.vin and tx.vout to psbtx.inputs
and psbtx.outputs.
This is in prepration for use with PSBTv2 where the global unsigned tx
will not exist.
Instead of allowing PSBTs to be default constructor, force usage of the
deserialization constructor.
CombinePSBTs, DecodeBase64PSBT, and DecodeRawPSBT are all changed to
return std::optional or util::result rather than using an output
parameter to avoid the need for a default constructor.
PSBTInput should be aware of the previous txid, output index, and
sequence numbers for inputs, extracting them from the global
unsigned tx.
PSBTOutput should be aware of the output amount and script, extracting
them from the global unsigned tx.
This prepares for PSBTv2 where these fields are serialized.
Using an existing block or a newly created block, send a compact
block message to the target node. Some of the time, the harness
will prefill multiple transactions from the block. Adds a
FuzzedCBlockHeaderAndShortTxIDs class that's used to populate the
protected prefilledtxn and shorttxids fields.
The blocks may include mempool and non-mempool transactions. If the
block index was added to, reset the rng, mempool, and chainman. Also
move FinalizeHeader from p2p_headers_presync.cpp to util.h so that
the mining function can use it to create valid headers.
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
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
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
Previously, they were using a pattern of defininig a constant megabytes
symbol, and then multiplying that by 1_MiB.
It is easier to use the _MiB operator directly.
The move-assignment operator for btck::Handle<> unconditionally called
DestroyFunc(m_ptr) before reading the source pointer. On a self-move
(h = std::move(h)), this destroyed the held resource and then reassigned
the now-dangling pointer back to m_ptr via std::exchange, leading to a
double-free when the object is later destroyed.
Mirror the existing self-check in the copy-assignment operator by
guarding the move-assignment with 'if (this != &other)' so a self-move
becomes a no-op, leaving the object in a valid state as required by the
standard library.
Handle<> is the base of 16 public types in the kernel C++ API wrapper
(Transaction, Block, BlockHeader, ChainParams, Context, Coin,
BlockValidationState, ScriptPubkey, TransactionOutput, Txid, OutPoint,
TransactionInput, PrecomputedTransactionData, BlockHash,
BlockSpentOutputs, TransactionSpentOutputs), so self-move can arise
from generic algorithms operating on containers of these types.
Extend CheckHandle in test_kernel to cover self-move-assignment for
every Handle-derived type.
c5ec2d5313 logging: replace FormatLogStrInPlace with Format (stickies-v)
3b92ec2036 logging: replace BufferedLog with log::Entry (stickies-v)
8115001cd4 logging: pass log::Entry through to logging functions (stickies-v)
b414913c73 util: add timestamp and thread_name to log::Entry (stickies-v)
8a55b17751 util: make SourceLocation constructor explicit (stickies-v)
Pull request description:
Preparatory work to enable struct-based logging for kernel (#34374), but I think it's a mild improvement by itself too.
#34465 introduced `util::log::Entry`. This PR updates some internal functions and structs to make better use of that new plumbing.
Mostly a refactor, except for very minor timing differences for `timestamp` and `mocktime` which are now uniformly captured at `Entry` generation.
ACKs for top commit:
maflcko:
review ACK c5ec2d5313🚐
ryanofsky:
Code review ACK c5ec2d5313. Just suggested changes since last review: adding (void) std::move, making SourceLocation constructor explicit, dropping BufferedLog struct. Overall PR is a nice code simplification, and I don't think it has downsides other than buffered logs now containing [ignored](https://github.com/bitcoin/bitcoin/pull/34865#discussion_r3054379230) `should_ratelimit` fields, which can be fixed in a followup by separating log entries from log options.
sedited:
ACK c5ec2d5313
Tree-SHA512: 3943f380a426b2c1b405226f231db51548c737467a278936b36a4cc738e01a790258f0886817d7caa1dbba7874f2e7f9ad93c36a137fa35f721f2d988b9863aa
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
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
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.
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
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.
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.
037ea2c714 walletdb: Remove m_mock from SQLiteDatabase (Ava Chow)
59484e2fdb wallet: Make Mockable{Database,Batch} subclasses of SQLite classes (Ava Chow)
b69f989dc5 wallet, bench: Use TestingSetup in CoinSelection benchmark (Ava Chow)
e7d67c9fd9 test: Make duplicating MockableDatabases use cursor and batch (Ava Chow)
964eafb71c bench, wallet: Make WalletMigration's setup WalletBatch scoped (Ava Chow)
Pull request description:
`MockableDatabase` was introduced for the tests to avoid tying non-database tests to a particular database type. However, since the only database type now is sqlite, and because the mockable behavior is no longer used by the tests, we can replace usage of the `MockabeDatabase` with a SQLite database that lives only in memory.
This is particularly useful for future work that has the wallet make use of SQLite's capabilities more, which are less conducive to having a separate mock database implementation.
ACKs for top commit:
brunoerg:
code review ACK 037ea2c714
sedited:
Re-ACK 037ea2c714
furszy:
Code review ACK 037ea2c714
Tree-SHA512: 0a99c27ef4e590966b3af929bf3acf99666861905aabf150fe5660ea07c881a49935a4e7dcd676dcd5e70616898d89d872b6e156ae9c600de1361c1b2469b64d
fc736013a5 rpc: Add in_memory option to dumptxoutset with rollback (Fabian Jahr)
d0fd718948 test: Extend named pipe sqlite tool test to use rollback (Fabian Jahr)
ab9463efac test: Add dumptxoutset fork test (Fabian Jahr)
49d5e835a8 rpc: Don't invalidate blocks in dumptxoutset (Fabian Jahr)
fe58eb9850 blockstorage: Add DeletePruneLock (Fabian Jahr)
Pull request description:
This is an alternative approach to implement `dumptxoutset` with rollback that was discussed a few times. It does not rely on `invalidateblock` and `reconsiderblock` and instead creates a temporary copy of the coins DB, modifies this copy by rolling back as many blocks as necessary and then creating the dump from this temp copy DB. See also https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-1978480989, https://github.com/bitcoin/bitcoin/issues/32817#issuecomment-3012406102 and #29565 discussions.
The nice side-effects of this are that forks can not interfere with the rollback and network activity does not have to be suspended. But there are also some downsides when comparing to the current approach: this does require some additional disk space for the copied coins DB and performance is slower (master took 3m 17s vs 9m 16s in my last test with the code here, rolling back ~1500 blocks). However, there is also not much code being added here, network can stay active throughout and performance would stay constant with this approach while it would impact master if there were forks that needed to be invalidated as well (see #33444 for the alternative approach), so this could still be considered a good trade-off.
ACKs for top commit:
stratospher:
tested ACK fc73601. very nice!
sedited:
Re-ACK fc736013a5
theStack:
re-ACK fc736013a5
Tree-SHA512: d3d674f68184ac3ada87d969d0fca7bc38203ee939853864adcd235ee3a954914c7e351b817800b885a495606e323392c27d88ba8d8e018eaf8567c098eb0e9c
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.
