coins,refactor: Reduce `getblockstats` RPC UTXO overhead estimation

l0rinc commented at 5:36 pm on December 9, 2024: contributor

The getblockstats RPC currently overestimates UTXO overhead by treating the fCoinBase bitfield as a separate bool in PER_UTXO_OVERHEAD. However, fCoinBase and nHeight are packed into a single 32-bit integer (both in memory and when serialized) - the extra bool in the overhead calculation is unnecessary.

This PR introduces the following changes across three commits:

Stores fCoinBase as a proper bool to minimize conversions.
Unifies the serialization style for better readability.
Adjusts UTXO overhead estimation to reflect the actual structure and updates related tests accordingly.

DrahtBot commented at 5:36 pm on December 9, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31449.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	Raimo33

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label CI failed on Dec 9, 2024

l0rinc force-pushed on Dec 9, 2024

DrahtBot removed the label CI failed on Dec 9, 2024

l0rinc marked this as ready for review on Dec 9, 2024

in src/test/fuzz/utxo_snapshot.cpp:140 in 5658c51871 outdated

103@@ -104,7 +104,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer)
104                 outfile << coinbase->GetHash();
105                 WriteCompactSize(outfile, 1); // number of coins for the hash
106                 WriteCompactSize(outfile, 0); // index of coin
107-                outfile << Coin(coinbase->vout[0], height, /*fCoinBaseIn=*/1);
108+                outfile << Coin(coinbase->vout[0], height, /*fCoinBaseIn=*/true);

sedited commented at 9:38 am on December 10, 2024:

Can a current utxo snapshot still be loaded with this change?

l0rinc commented at 9:50 am on December 10, 2024:

Coin already had a bool fCoinBaseIn parameter, this shouldn’t change anything.

But it would be really useful if you could show me how to test loading a current utxo snapshot to make absolutely sure.

sedited commented at 11:42 am on December 10, 2024:

I looked at the changes more closely now, and yes, this should not change the format. I also checked that doing a dumptxoutset produces the same data on both master and this PR.

l0rinc requested review from sedited on Jan 9, 2025

DrahtBot commented at 1:44 am on July 8, 2025: contributor

There hasn’t been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

coins,refactor: Migrate Coin#fCoinBase to bool d0603fad05

coins,refactor: Unify Coin Serialization styles 61cde55c56

rpc: Fix utxo_size_inc(_actual) calculation

`nHeight` and `fCoinBase` are stored in `Coin` as bitfields and are serialized as a single 32 bit number, so the UTXO overhead calculation shouldn't include an additional boolean

433942e840

l0rinc force-pushed on Jul 28, 2025

bitcoin deleted a comment on Aug 18, 2025

in src/rpc/blockchain.cpp:1866 in 433942e840

1861@@ -1862,8 +1862,8 @@ static inline bool SetHasKeys(const std::set<T>& set, const Tk& key, const Args&
1862     return (set.count(key) != 0) || SetHasKeys(set, args...);
1863 }
1864 
1865-// outpoint (needed for the utxo index) + nHeight + fCoinBase
1866-static constexpr size_t PER_UTXO_OVERHEAD = sizeof(COutPoint) + sizeof(uint32_t) + sizeof(bool);
1867+// outpoint (needed for the utxo index) + nHeight|fCoinBase
1868+static constexpr size_t PER_UTXO_OVERHEAD = sizeof(COutPoint) + sizeof(uint32_t);

maflcko commented at 8:00 am on September 1, 2025:

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?

l0rinc commented at 6:22 pm on September 1, 2025:

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.

Raimo33 commented at 10:29 pm on September 7, 2025: contributor

Code review ACK 61cde55c56952327d06297a133c30f6877eb62e6

last commit conflicts with https://github.com/bitcoin/bitcoin/pull/33184/ but I’d argue this PR could be merged first. My current knowledge prevents me from understanding the changes in test/functional/data/rpc_getblockstats.json and test/functional/rpc_getblockstats.py. Also it seems 433942e840e0b70d5872e5a5f8d84a42fa3ff05a could be split up further

coins,refactor: Reduce getblockstats RPC UTXO overhead estimation #31449

Code Coverage & Benchmarks

Reviews

Conflicts

coins,refactor: Reduce `getblockstats` RPC UTXO overhead estimation #31449