From 1f309d1aa2f3386cc17531f4c6369e3bbd46ee1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Mon, 9 Dec 2024 13:25:25 +0100 Subject: [PATCH 1/3] 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. --- src/coins.h | 2 +- src/core_io.cpp | 2 +- src/rpc/blockchain.cpp | 2 +- src/test/coins_tests.cpp | 8 ++++---- src/test/fuzz/utxo_snapshot.cpp | 4 ++-- src/validation.cpp | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/coins.h b/src/coins.h index 4b39c0bacd2..e58586dd6cd 100644 --- a/src/coins.h +++ b/src/coins.h @@ -37,7 +37,7 @@ public: CTxOut out; //! whether containing transaction was a coinbase - unsigned int fCoinBase : 1; + bool fCoinBase : 1; //! at which height this containing transaction was included in the active block chain uint32_t nHeight : 31; diff --git a/src/core_io.cpp b/src/core_io.cpp index 7492e9ca50f..eeda115699f 100644 --- a/src/core_io.cpp +++ b/src/core_io.cpp @@ -480,7 +480,7 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry ScriptToUniv(prev_txout.scriptPubKey, /*out=*/o_script_pub_key, /*include_hex=*/true, /*include_address=*/true); UniValue p(UniValue::VOBJ); - p.pushKV("generated", static_cast(prev_coin.fCoinBase)); + p.pushKV("generated", prev_coin.IsCoinBase()); p.pushKV("height", prev_coin.nHeight); p.pushKV("value", ValueFromAmount(prev_txout.nValue)); p.pushKV("scriptPubKey", std::move(o_script_pub_key)); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 262b6bab0e3..9838bda6722 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1222,7 +1222,7 @@ static RPCHelpMan gettxout() UniValue o(UniValue::VOBJ); ScriptToUniv(coin->out.scriptPubKey, /*out=*/o, /*include_hex=*/true, /*include_address=*/true); ret.pushKV("scriptPubKey", std::move(o)); - ret.pushKV("coinbase", static_cast(coin->fCoinBase)); + ret.pushKV("coinbase", coin->IsCoinBase()); return ret; }, diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 8c0756d8528..f56e369cc5e 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -519,7 +519,7 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization) // Good example Coin cc1; SpanReader{"97f23c835800816115944e077fe7c803cfa57f29b36bf87c1d35"_hex} >> cc1; - BOOST_CHECK_EQUAL(cc1.fCoinBase, false); + BOOST_CHECK_EQUAL(cc1.IsCoinBase(), false); BOOST_CHECK_EQUAL(cc1.nHeight, 203998U); BOOST_CHECK_EQUAL(cc1.out.nValue, CAmount{60000000000}); BOOST_CHECK_EQUAL(HexStr(cc1.out.scriptPubKey), HexStr(GetScriptForDestination(PKHash(uint160("816115944e077fe7c803cfa57f29b36bf87c1d35"_hex_u8))))); @@ -527,7 +527,7 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization) // Good example Coin cc2; SpanReader{"8ddf77bbd123008c988f1a4a4de2161e0f50aac7f17e7f9555caa4"_hex} >> cc2; - BOOST_CHECK_EQUAL(cc2.fCoinBase, true); + BOOST_CHECK_EQUAL(cc2.IsCoinBase(), true); BOOST_CHECK_EQUAL(cc2.nHeight, 120891U); BOOST_CHECK_EQUAL(cc2.out.nValue, 110397); BOOST_CHECK_EQUAL(HexStr(cc2.out.scriptPubKey), HexStr(GetScriptForDestination(PKHash(uint160("8c988f1a4a4de2161e0f50aac7f17e7f9555caa4"_hex_u8))))); @@ -535,7 +535,7 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization) // Smallest possible example Coin cc3; SpanReader{"000006"_hex} >> cc3; - BOOST_CHECK_EQUAL(cc3.fCoinBase, false); + BOOST_CHECK_EQUAL(cc3.IsCoinBase(), false); BOOST_CHECK_EQUAL(cc3.nHeight, 0U); BOOST_CHECK_EQUAL(cc3.out.nValue, 0); BOOST_CHECK_EQUAL(cc3.out.scriptPubKey.size(), 0U); @@ -871,7 +871,7 @@ Coin MakeCoin() Coin coin; coin.out.nValue = m_rng.rand32(); coin.nHeight = m_rng.randrange(4096); - coin.fCoinBase = 0; + coin.fCoinBase = false; return coin; } diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp index 67290f7d306..29089743a09 100644 --- a/src/test/fuzz/utxo_snapshot.cpp +++ b/src/test/fuzz/utxo_snapshot.cpp @@ -137,7 +137,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer) outfile << coinbase->GetHash(); WriteCompactSize(outfile, 1); // number of coins for the hash WriteCompactSize(outfile, 0); // index of coin - outfile << Coin(coinbase->vout[0], height, /*fCoinBaseIn=*/1); + outfile << Coin(coinbase->vout[0], height, /*fCoinBaseIn=*/true); height++; } } @@ -149,7 +149,7 @@ void utxo_snapshot_fuzz(FuzzBufferType buffer) outfile << coinbase->GetHash(); WriteCompactSize(outfile, 1); // number of coins for the hash WriteCompactSize(outfile, 999); // index of coin - outfile << Coin{coinbase->vout[0], /*nHeightIn=*/999, /*fCoinBaseIn=*/0}; + outfile << Coin{coinbase->vout[0], /*nHeightIn=*/999, /*fCoinBaseIn=*/false}; } assert(outfile.fclose() == 0); } diff --git a/src/validation.cpp b/src/validation.cpp index c200c3d6cd8..01475736491 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2214,7 +2214,7 @@ DisconnectResult Chainstate::DisconnectBlock(const CBlock& block, const CBlockIn COutPoint out(hash, o); Coin coin; bool is_spent = view.SpendCoin(out, &coin); - if (!is_spent || tx.vout[o] != coin.out || pindex->nHeight != coin.nHeight || is_coinbase != coin.fCoinBase) { + if (!is_spent || tx.vout[o] != coin.out || pindex->nHeight != coin.nHeight || is_coinbase != coin.IsCoinBase()) { if (!is_bip30_exception) { fClean = false; // transaction output mismatch } From 76190489e6c89ad091ebba0781c585afc9a39f1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Mon, 9 Dec 2024 13:30:04 +0100 Subject: [PATCH 2/3] 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. --- src/coins.h | 4 ++-- src/kernel/coinstats.cpp | 2 +- src/undo.h | 3 ++- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/coins.h b/src/coins.h index e58586dd6cd..bfd6c5b0d7e 100644 --- a/src/coins.h +++ b/src/coins.h @@ -27,7 +27,7 @@ * A UTXO entry. * * Serialized format: - * - VARINT((coinbase ? 1 : 0) | (height << 1)) + * - VARINT((height << 1) | (coinbase ? 1 : 0)) * - the non-spent CTxOut (via TxOutCompression) */ class Coin @@ -62,7 +62,7 @@ public: template void Serialize(Stream &s) const { assert(!IsSpent()); - uint32_t code = nHeight * uint32_t{2} + fCoinBase; + uint32_t code{(uint32_t{nHeight} << 1) | uint32_t{fCoinBase}}; ::Serialize(s, VARINT(code)); ::Serialize(s, Using(out)); } diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp index d287ec4be6e..53039e57101 100644 --- a/src/kernel/coinstats.cpp +++ b/src/kernel/coinstats.cpp @@ -47,7 +47,7 @@ template static void TxOutSer(T& ss, const COutPoint& outpoint, const Coin& coin) { ss << outpoint; - ss << static_cast((coin.nHeight << 1) + coin.fCoinBase); + ss << ((uint32_t{coin.nHeight} << 1) | uint32_t{coin.fCoinBase}); ss << coin.out; } diff --git a/src/undo.h b/src/undo.h index 5591fe6cc82..13b92349510 100644 --- a/src/undo.h +++ b/src/undo.h @@ -23,7 +23,8 @@ struct TxInUndoFormatter { template void Ser(Stream &s, const Coin& txout) { - ::Serialize(s, VARINT(txout.nHeight * uint32_t{2} + txout.fCoinBase )); + uint32_t nCode{(uint32_t{txout.nHeight} << 1) | uint32_t{txout.fCoinBase}}; + ::Serialize(s, VARINT(nCode)); if (txout.nHeight > 0) { // Required to maintain compatibility with older undo format. ::Serialize(s, (unsigned char)0); From 5f36e0ff1e7681988815664559fbc0e8c0b4b3dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Mon, 9 Dec 2024 14:02:49 +0100 Subject: [PATCH 3/3] 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. --- src/rpc/blockchain.cpp | 4 ++-- test/functional/data/rpc_getblockstats.json | 12 ++++++------ test/functional/rpc_getblockstats.py | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 9838bda6722..eba10f0ddc7 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1898,8 +1898,8 @@ static inline bool SetHasKeys(const std::set& set, const Tk& key, const Args& return (set.contains(key)) || SetHasKeys(set, args...); } -// 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); static RPCHelpMan getblockstats() { diff --git a/test/functional/data/rpc_getblockstats.json b/test/functional/data/rpc_getblockstats.json index 7d7460aacc8..e0c0eb39fbc 100644 --- a/test/functional/data/rpc_getblockstats.json +++ b/test/functional/data/rpc_getblockstats.json @@ -143,8 +143,8 @@ "txs": 1, "utxo_increase": 2, "utxo_increase_actual": 1, - "utxo_size_inc": 163, - "utxo_size_inc_actual": 75 + "utxo_size_inc": 161, + "utxo_size_inc_actual": 74 }, { "avgfee": 4440, @@ -182,8 +182,8 @@ "txs": 2, "utxo_increase": 3, "utxo_increase_actual": 2, - "utxo_size_inc": 235, - "utxo_size_inc_actual": 147 + "utxo_size_inc": 232, + "utxo_size_inc_actual": 145 }, { "avgfee": 21390, @@ -221,8 +221,8 @@ "txs": 5, "utxo_increase": 6, "utxo_increase_actual": 4, - "utxo_size_inc": 441, - "utxo_size_inc_actual": 300 + "utxo_size_inc": 435, + "utxo_size_inc_actual": 296 } ] } \ No newline at end of file diff --git a/test/functional/rpc_getblockstats.py b/test/functional/rpc_getblockstats.py index 29d70a056e2..75e09639297 100755 --- a/test/functional/rpc_getblockstats.py +++ b/test/functional/rpc_getblockstats.py @@ -171,16 +171,16 @@ class GetblockstatsTest(BitcoinTestFramework): genesis_stats = self.nodes[0].getblockstats(0) assert_equal(genesis_stats["blockhash"], "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206") assert_equal(genesis_stats["utxo_increase"], 1) - assert_equal(genesis_stats["utxo_size_inc"], 117) + assert_equal(genesis_stats["utxo_size_inc"], 116) assert_equal(genesis_stats["utxo_increase_actual"], 0) assert_equal(genesis_stats["utxo_size_inc_actual"], 0) self.log.info('Test tip including OP_RETURN') tip_stats = self.nodes[0].getblockstats(tip) assert_equal(tip_stats["utxo_increase"], 6) - assert_equal(tip_stats["utxo_size_inc"], 441) + assert_equal(tip_stats["utxo_size_inc"], 435) assert_equal(tip_stats["utxo_increase_actual"], 4) - assert_equal(tip_stats["utxo_size_inc_actual"], 300) + assert_equal(tip_stats["utxo_size_inc_actual"], 296) self.log.info("Test when only header is known") block = self.generateblock(self.nodes[0], output="raw(55)", transactions=[], submit=False)