coins,refactor: Reduce getblockstats RPC UTXO overhead estimation
#31449
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31449. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
l0rinc
force-pushed
the
l0rinc/coin-fCoinBase-bool
branch
from
889b86e to
72ec388
Compare
l0rinc
force-pushed
the
l0rinc/coin-fCoinBase-bool
branch
from
72ec388 to
433942e
Compare
| // outpoint (needed for the utxo index) + nHeight + fCoinBase | ||
| static constexpr size_t PER_UTXO_OVERHEAD = sizeof(COutPoint) + sizeof(uint32_t) + sizeof(bool); | ||
| // outpoint (needed for the utxo index) + nHeight|fCoinBase | ||
| static constexpr size_t PER_UTXO_OVERHEAD = sizeof(COutPoint) + sizeof(uint32_t); |
There was a problem hiding this comment.
isn't the serialized format using varint compression (or leveldb compression) anyway, so any hardcoded overhead will be wrong here anyway?
I'd say it is fine to leave this as-is, because both this pull and current master are equally "wrong" and in the presence of doubt, it seems better to not change the code?
There was a problem hiding this comment.
yes, both are over-estimating, but this is closer to the actual functioning, the previous one explicitly adds the bool which isn't separate.
We can also just fix it by not making this a constexpr but a function of the actual values, but I'd argue the current versions is still better.
|
Code review ACK 61cde55 last commit conflicts with #33184 but I'd argue this PR could be merged first. |
The coinbase flag is semantically boolean but was stored as an unsigned bitfield. Store it as a `bool : 1` bitfield to better reflect intent and avoid relying on implicit integral conversions at call sites. Update users to prefer `Coin::IsCoinBase()` and to pass explicit `true`/`false` values where appropriate.
l0rinc
force-pushed
the
l0rinc/coin-fCoinBase-bool
branch
from
433942e to
cae3b55
Compare
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.
`Coin` packs height and the coinbase flag into a single 32-bit value, so `utxo_size_inc(_actual)` should not count an additional boolean. Update the calculation and adjust the `rpc_getblockstats` test expectations.
l0rinc
force-pushed
the
l0rinc/coin-fCoinBase-bool
branch
from
cae3b55 to
0096d01
Compare
5 participants
The
getblockstatsRPC currently overestimates UTXO overhead by treating thefCoinBasebitfield as an additionalboolinPER_UTXO_OVERHEAD.However,
fCoinBaseandnHeightare 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:
fCoinBaseas a bool bitfield to reduce implicit conversions at call sites.nHeighttouint32_tbefore shifting to avoid signed-promotion UB).