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

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

   The getblockstats RPC 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.
2. 
   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
   ACK	sedited
   Stale ACK	Raimo33

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #34524 (refactor: [rpc] Remove confusing and brittle integral casts (take 2) by maflcko)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. DrahtBot added the label CI failed on Dec 9, 2024
4. l0rinc force-pushed on Dec 9, 2024
5. DrahtBot removed the label CI failed on Dec 9, 2024
6. l0rinc marked this as ready for review on Dec 9, 2024
7. in src/test/fuzz/utxo_snapshot.cpp: 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.
8. l0rinc requested review from sedited on Jan 9, 2025
9. l0rinc force-pushed on Jul 28, 2025
10. bitcoin deleted a comment on Aug 18, 2025
11. in src/rpc/blockchain.cpp:1866 in 433942e840 outdated

    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.
12. 
    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

13. DrahtBot added the label Needs rebase on Dec 27, 2025
14. l0rinc force-pushed on Jan 1, 2026
15. 
    l0rinc commented at 10:14 pm on January 1, 2026: contributor

    Rebased after https://github.com/bitcoin/bitcoin/commit/fa66e2d07a4b87d62382a54acf5fab6af77be24e#diff-decae4be02fb8a47ab4557fe74a9cb853bdfa3ec0fa1b515c0a1e5de91f4ad0bR1224-R1226
16. l0rinc force-pushed on Jan 1, 2026
17. DrahtBot removed the label Needs rebase on Jan 2, 2026
18. DrahtBot added the label Needs rebase on Jan 19, 2026
19. l0rinc force-pushed on Jan 19, 2026
20. DrahtBot removed the label Needs rebase on Jan 19, 2026
21. DrahtBot added the label Needs rebase on Feb 7, 2026
22. l0rinc force-pushed on Feb 7, 2026
23. DrahtBot removed the label Needs rebase on Feb 7, 2026
24. DrahtBot added the label CI failed on Feb 7, 2026
25. l0rinc force-pushed on Feb 8, 2026
26. DrahtBot removed the label CI failed on Feb 8, 2026
27. DrahtBot added the label Needs rebase on Feb 9, 2026
28. coins: make `Coin::fCoinBase` a bool

    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.

    1f309d1aa2
29. coins: pack `Coin` height/coinbase consistently

    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.

    76190489e6
30. rpc: fix getblockstats UTXO overhead accounting

    `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.

    5f36e0ff1e
31. l0rinc force-pushed on Feb 9, 2026
32. 
    l0rinc commented at 4:31 pm on February 9, 2026: contributor

    Rebased after https://github.com/bitcoin/bitcoin/commit/44afed4cd970ec38560ebb673c50b4f52da830c9
33. DrahtBot removed the label Needs rebase on Feb 9, 2026
34. DrahtBot added the label CI failed on Feb 9, 2026
35. DrahtBot removed the label CI failed on Feb 10, 2026
36. sedited approved
37. 
    sedited commented at 8:59 pm on March 3, 2026: contributor

    ACK 5f36e0ff1e7681988815664559fbc0e8c0b4b3dc

Contributors
l0rinc DrahtBot sedited maflcko Raimo33