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

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.

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, vasild, optout21, achow101
Stale ACK	Raimo33

If your review is incorrectly listed, please copy-paste <code></code> 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.

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:None 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

l0rinc force-pushed on Jul 28, 2025

bitcoin deleted a comment on Aug 18, 2025

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.

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

DrahtBot added the label Needs rebase on Dec 27, 2025

l0rinc force-pushed on Jan 1, 2026

l0rinc commented at 10:14 PM on January 1, 2026: contributor

Rebased after https://github.com/bitcoin/bitcoin/commit/fa66e2d07a4b87d62382a54acf5fab6af77be24e#diff-decae4be02fb8a47ab4557fe74a9cb853bdfa3ec0fa1b515c0a1e5de91f4ad0bR1224-R1226

l0rinc force-pushed on Jan 1, 2026

DrahtBot removed the label Needs rebase on Jan 2, 2026

DrahtBot added the label Needs rebase on Jan 19, 2026

l0rinc force-pushed on Jan 19, 2026

DrahtBot removed the label Needs rebase on Jan 19, 2026

DrahtBot added the label Needs rebase on Feb 7, 2026

l0rinc force-pushed on Feb 7, 2026

DrahtBot removed the label Needs rebase on Feb 7, 2026

DrahtBot added the label CI failed on Feb 7, 2026

l0rinc force-pushed on Feb 8, 2026

DrahtBot removed the label CI failed on Feb 8, 2026

DrahtBot added the label Needs rebase on Feb 9, 2026

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

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

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

l0rinc force-pushed on Feb 9, 2026

l0rinc commented at 4:31 PM on February 9, 2026: contributor

Rebased after https://github.com/bitcoin/bitcoin/commit/44afed4cd970ec38560ebb673c50b4f52da830c9

DrahtBot removed the label Needs rebase on Feb 9, 2026

DrahtBot added the label CI failed on Feb 9, 2026

DrahtBot removed the label CI failed on Feb 10, 2026

sedited approved

sedited commented at 8:59 PM on March 3, 2026: contributor

ACK 5f36e0ff1e7681988815664559fbc0e8c0b4b3dc

sedited requested review from stickies-v on Mar 19, 2026

vasild approved

vasild commented at 10:27 AM on April 13, 2026: contributor

ACK 5f36e0ff1e7681988815664559fbc0e8c0b4b3dc

<details> <summary>Show Signature</summary>

vasild's public key is on openpgp.org

</details>

in src/test/coins_tests.cpp:522 in 1f309d1aa2 outdated

 518 | @@ -519,23 +519,23 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization)
 519 |      // Good example
 520 |      Coin cc1;
 521 |      SpanReader{"97f23c835800816115944e077fe7c803cfa57f29b36bf87c1d35"_hex} >> cc1;
 522 | -    BOOST_CHECK_EQUAL(cc1.fCoinBase, false);
 523 | +    BOOST_CHECK_EQUAL(cc1.IsCoinBase(), false);

optout21 commented at 9:01 AM on April 15, 2026:

1f309d1 coins: make Coin::fCoinBase a bool:

This change only changes to access through the accessor. However, not all occurrences are changed. My suggestion:

Don't change lines only to use the accessor (if there is other change, like type cast, then it's fine to also change it to use the accessor)
Alternatively, switch to using the accessor everywhere, and possibly make the field private, in a separate commit.

l0rinc commented at 5:49 PM on April 15, 2026:

Yes, thanks, the change was added 1.5 years ago, there are indeed a few places that could be unified since - but I think it's fine either way, we can do it in a follow-up if consistency is important here.

optout21 commented at 6:01 PM on April 15, 2026:

Makes sense, it's up to you, please feel free to resolve.

in src/kernel/coinstats.cpp:50 in 76190489e6 outdated

  46 | @@ -47,7 +47,7 @@ template <typename T>
  47 |  static void TxOutSer(T& ss, const COutPoint& outpoint, const Coin& coin)
  48 |  {
  49 |      ss << outpoint;
  50 | -    ss << static_cast<uint32_t>((coin.nHeight << 1) + coin.fCoinBase);
  51 | +    ss << ((uint32_t{coin.nHeight} << 1) | uint32_t{coin.fCoinBase});

optout21 commented at 9:19 AM on April 15, 2026:

7619048 coins: pack Coin height/coinbase consistently:

Suggestion: since this is used in 3 places, maybe it would make sense to do it in a single place (in a method in Coin)?

    //! Return the height and coinbase fields packed into a single uint32.
    uint32_t packed_height_and_cb() const {
        return (uint32_t(nHeight) << 1) | uint32_t{fCoinBase};
    }

l0rinc commented at 5:49 PM on April 15, 2026:

Good idea, but given the long review time I would prefer doing that in a follow-up instead.

optout21 commented at 6:01 PM on April 15, 2026:

Makes sense, it's up to you. My ACK is valid as it is, please feel free to resolve.

optout21 commented at 9:29 AM on April 15, 2026: contributor

crACK 5f36e0ff1e7681988815664559fbc0e8c0b4b3dc

This change improves the reported estimated UTXO storage overhead, though -- as discussed -- it is still just an estimate (due to varint packing). The change also improves type handling of the fCoinBase and nHeight fields of Coin, which is a nice improvement in code quality.

I've left some minor remarks regarding code reuse and some potential unnecessary changes.

l0rinc commented at 8:11 PM on April 20, 2026: contributor

rfm?

achow101 commented at 9:31 PM on April 20, 2026: member

This has a merge conflict with master, that for some reason github is not flagging. Needs rebase.

Edit: ok, so they actually aren't updating the pull refs

achow101 closed this on Apr 20, 2026

achow101 reopened this on Apr 20, 2026

achow101 commented at 10:37 PM on April 20, 2026: member

Unfortunately github is being dumb and I can't merge this because refs/pulls/31449/head does not match the actual head commit of this PR. ~~@l0rinc can you force push a different commit hash (e.g. change the timestamp) to see if that will update that ref?~~

Edit: I have a workaround.

achow101 commented at 10:52 PM on April 20, 2026: member

ACK 5f36e0ff1e7681988815664559fbc0e8c0b4b3dc

achow101 merged this on Apr 20, 2026

achow101 closed this on Apr 20, 2026

l0rinc deleted the branch on Apr 21, 2026

l0rinc commented at 8:47 AM on April 21, 2026: contributor

Thank you for the reviews and the merge workaround! Let me know if there's anything I can do to avoid this confusion next time.

For the record, the failing CI is also GitHub related:

/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=5 origin 5f36e0ff1e7681988815664559fbc0e8c0b4b3dc remote: Internal Server Error Error: fatal: unable to access 'https://github.com/bitcoin/bitcoin/': The requested URL returned error: 500 The process '/usr/bin/git' failed with exit code 128 Waiting 14 seconds before trying again

maflcko commented at 8:56 AM on April 21, 2026: member

Yeah, GitHub served corrupt data, which is not the first time. This one I tracked in https://github.com/maflcko/DrahtBot/issues/79, because with corrupt data, the bot won't function either.

I've also seen bugs where the GH web view was omitting files or hunks from the diff.

I guess this is a good reminder to check out pull requests locally, and sign the review result.