Merge bitcoin#29656: chainparams: Change nChainTx type to uint64_t

bf0efb4 scripted-diff: Modernize naming of nChainTx and nTxCount (Fabian Jahr)
72e5d1b test: Add basic check for nChainTx type (Fabian Jahr)
dc2938e chainparams: Change nChainTx to uint64_t (Fabian Jahr)

Pull request description:

  This picks up the work from bitcoin#29331 and closes bitcoin#29258.

  This simply changes the type and addresses the comments from bitcoin#29331 by changing the type in all relevant places and removing unnecessary casts. This also adds an extremely simple unit test.

  Additionally this modernizes the name of `nChainTx` which helps reviewers check all use of the symbol and can make silent merge conflicts.

ACKs for top commit:
  maflcko:
    only rebase in scripted-diff, re-ACK bf0efb4 🔈
  glozow:
    reACK bf0efb4 via range-diff

Tree-SHA512: ee4020926d0800236fe655d0c7b127215ab36b553b04d5f91494f4b7fac6e1cfe7ee298b07c0983db5a3f4786932acaa54f5fd2ccd45f2fcdcfa13427358dc3b

Author: glozow

src/chain.h

@@ -102,7 +102,7 @@ enum BlockStatus : uint32_t {
      *
      * If a block's validity is at least VALID_TRANSACTIONS, CBlockIndex::nTx will be set. If a block and all previous
      * blocks back to the genesis block or an assumeutxo snapshot block are at least VALID_TRANSACTIONS,
-     * CBlockIndex::nChainTx will be set.
+     * CBlockIndex::m_chain_tx_count will be set.
      */
     BLOCK_VALID_TRANSACTIONS =    3,
 
@@ -173,8 +173,7 @@ class CBlockIndex
     //! This value will be non-zero if this block and all previous blocks back
     //! to the genesis block or an assumeutxo snapshot block have reached the
     //! VALID_TRANSACTIONS level.
-    //! Change to 64-bit type before 2024 (assuming worst case of 60 byte transactions).
-    unsigned int nChainTx{0};
+    uint64_t m_chain_tx_count{0};
 
     //! Verification status of this block. See enum BlockStatus
     //!
@@ -254,10 +253,10 @@ class CBlockIndex
      * Does not imply the transactions are consensus-valid (ConnectTip might fail)
      * Does not imply the transactions are still stored on disk. (IsBlockPruned might return true)
      *
-     * Note that this will be true for the snapshot base block, if one is loaded, since its nChainTx value will have
+     * Note that this will be true for the snapshot base block, if one is loaded, since its m_chain_tx_count value will have
      * been set manually based on the related AssumeutxoData entry.
      */
-    bool HaveNumChainTxs() const { return nChainTx != 0; }
+    bool HaveNumChainTxs() const { return m_chain_tx_count != 0; }
 
     NodeSeconds Time() const
     {

src/kernel/chainparams.cpp

@@ -180,7 +180,7 @@ class CMainParams : public CChainParams {
         chainTxData = ChainTxData{
             // Data from RPC: getchaintxstats 4096 000000000000000000026811d149d4d261995ec5b3f64f439a0a10e1a464af9a
             .nTime    = 1704194835,
-            .nTxCount = 946728933,
+            .tx_count = 946728933,
             .dTxRate  = 6.569290261471664,
         };
     }
@@ -272,15 +272,15 @@ class CTestNetParams : public CChainParams {
             {
                 .height = 2'500'000,
                 .hash_serialized = AssumeutxoHash{uint256S("0xf841584909f68e47897952345234e37fcd9128cd818f41ee6c3ca68db8071be7")},
-                .nChainTx = 66484552,
+                .m_chain_tx_count = 66484552,
                 .blockhash = uint256S("0x0000000000000093bcb68c03a9a168ae252572d348a2eaeba2cdf9231d73206f")
             }
         };
 
         chainTxData = ChainTxData{
             // Data from RPC: getchaintxstats 4096 000000000001323071f38f21ea5aae529ece491eadaccce506a59bcc2d968917
             .nTime    = 1703579240,
-            .nTxCount = 67845391,
+            .tx_count = 67845391,
             .dTxRate  = 1.464436832560951,
         };
     }
@@ -312,7 +312,7 @@ class SigNetParams : public CChainParams {
             chainTxData = ChainTxData{
                 // Data from RPC: getchaintxstats 4096 0000000870f15246ba23c16e370a7ffb1fc8a3dcf8cb4492882ed4b0e3d4cd26
                 .nTime    = 1706331472,
-                .nTxCount = 2425380,
+                .tx_count = 2425380,
                 .dTxRate  = 0.008277759863833788,
             };
         } else {
@@ -382,7 +382,7 @@ class SigNetParams : public CChainParams {
             {
                 .height = 160'000,
                 .hash_serialized = AssumeutxoHash{uint256S("0xfe0a44309b74d6b5883d246cb419c6221bcccf0b308c9b59b7d70783dbdf928a")},
-                .nChainTx = 2289496,
+                .m_chain_tx_count = 2289496,
                 .blockhash = uint256S("0x0000003ca3c99aff040f2563c2ad8f8ec88bd0fd6b8f0895cfaf1ef90353a62c")
             }
         };
@@ -498,21 +498,21 @@ class CRegTestParams : public CChainParams
             {   // For use by unit tests
                 .height = 110,
                 .hash_serialized = AssumeutxoHash{uint256S("0x6657b736d4fe4db0cbc796789e812d5dba7f5c143764b1b6905612f1830609d1")},
-                .nChainTx = 111,
+                .m_chain_tx_count = 111,
                 .blockhash = uint256S("0x696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c")
             },
             {
                 // For use by fuzz target src/test/fuzz/utxo_snapshot.cpp
                 .height = 200,
                 .hash_serialized = AssumeutxoHash{uint256S("0x4f34d431c3e482f6b0d67b64609ece3964dc8d7976d02ac68dd7c9c1421738f2")},
-                .nChainTx = 201,
+                .m_chain_tx_count = 201,
                 .blockhash = uint256S("0x5e93653318f294fb5aa339d00bbf8cf1c3515488ad99412c37608b139ea63b27"),
             },
             {
                 // For use by test/functional/feature_assumeutxo.py
                 .height = 299,
                 .hash_serialized = AssumeutxoHash{uint256S("0xa4bf3407ccb2cc0145c49ebba8fa91199f8a3903daf0883875941497d2493c27")},
-                .nChainTx = 334,
+                .m_chain_tx_count = 334,
                 .blockhash = uint256S("0x3bb7ce5eba0be48939b7a521ac1ba9316afee2c7bada3a0cca24188e6d7d96c0")
             },
         };

src/kernel/chainparams.h

@@ -50,11 +50,11 @@ struct AssumeutxoData {
     //! The expected hash of the deserialized UTXO set.
     AssumeutxoHash hash_serialized;
 
-    //! Used to populate the nChainTx value, which is used during BlockManager::LoadBlockIndex().
+    //! Used to populate the m_chain_tx_count value, which is used during BlockManager::LoadBlockIndex().
     //!
     //! We need to hardcode the value here because this is computed cumulatively using block data,
     //! which we do not necessarily have at the time of snapshot load.
-    unsigned int nChainTx;
+    uint64_t m_chain_tx_count;
 
     //! The hash of the base block for this snapshot. Used to refer to assumeutxo data
     //! prior to having a loaded blockindex.
@@ -69,7 +69,7 @@ struct AssumeutxoData {
  */
 struct ChainTxData {
     int64_t nTime;    //!< UNIX timestamp of last known number of transactions
-    int64_t nTxCount; //!< total number of transactions between genesis and that timestamp
+    uint64_t tx_count; //!< total number of transactions between genesis and that timestamp
     double dTxRate;   //!< estimated number of transactions per second after that timestamp
 };
 

src/node/blockstorage.cpp

@@ -410,11 +410,11 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
         m_snapshot_height = au_data.height;
         CBlockIndex* base{LookupBlockIndex(*snapshot_blockhash)};
 
-        // Since nChainTx (responsible for estimated progress) isn't persisted
+        // Since m_chain_tx_count (responsible for estimated progress) isn't persisted
         // to disk, we must bootstrap the value for assumedvalid chainstates
         // from the hardcoded assumeutxo chainparams.
-        base->nChainTx = au_data.nChainTx;
-        LogPrintf("[snapshot] set nChainTx=%d for %s\n", au_data.nChainTx, snapshot_blockhash->ToString());
+        base->m_chain_tx_count = au_data.m_chain_tx_count;
+        LogPrintf("[snapshot] set m_chain_tx_count=%d for %s\n", au_data.m_chain_tx_count, snapshot_blockhash->ToString());
     } else {
         // If this isn't called with a snapshot blockhash, make sure the cached snapshot height
         // is null. This is relevant during snapshot completion, when the blockman may be loaded
@@ -449,15 +449,15 @@ bool BlockManager::LoadBlockIndex(const std::optional<uint256>& snapshot_blockha
                 if (m_snapshot_height && pindex->nHeight == *m_snapshot_height &&
                         pindex->GetBlockHash() == *snapshot_blockhash) {
                     // Should have been set above; don't disturb it with code below.
-                    Assert(pindex->nChainTx > 0);
-                } else if (pindex->pprev->nChainTx > 0) {
-                    pindex->nChainTx = pindex->pprev->nChainTx + pindex->nTx;
+                    Assert(pindex->m_chain_tx_count > 0);
+                } else if (pindex->pprev->m_chain_tx_count > 0) {
+                    pindex->m_chain_tx_count = pindex->pprev->m_chain_tx_count + pindex->nTx;
                 } else {
-                    pindex->nChainTx = 0;
+                    pindex->m_chain_tx_count = 0;
                     m_blocks_unlinked.insert(std::make_pair(pindex->pprev, pindex));
                 }
             } else {
-                pindex->nChainTx = pindex->nTx;
+                pindex->m_chain_tx_count = pindex->nTx;
             }
         }
         if (!(pindex->nStatus & BLOCK_FAILED_MASK) && pindex->pprev && (pindex->pprev->nStatus & BLOCK_FAILED_MASK)) {

src/rpc/blockchain.cpp

@@ -1725,16 +1725,16 @@ static RPCHelpMan getchaintxstats()
 
     UniValue ret(UniValue::VOBJ);
     ret.pushKV("time", (int64_t)pindex->nTime);
-    if (pindex->nChainTx) {
-        ret.pushKV("txcount", pindex->nChainTx);
+    if (pindex->m_chain_tx_count) {
+        ret.pushKV("txcount", pindex->m_chain_tx_count);
     }
     ret.pushKV("window_final_block_hash", pindex->GetBlockHash().GetHex());
     ret.pushKV("window_final_block_height", pindex->nHeight);
     ret.pushKV("window_block_count", blockcount);
     if (blockcount > 0) {
         ret.pushKV("window_interval", nTimeDiff);
-        if (pindex->nChainTx != 0 && past_block.nChainTx != 0) {
-            const auto window_tx_count = pindex->nChainTx - past_block.nChainTx;
+        if (pindex->m_chain_tx_count != 0 && past_block.m_chain_tx_count != 0) {
+            const auto window_tx_count = pindex->m_chain_tx_count - past_block.m_chain_tx_count;
             ret.pushKV("window_tx_count", window_tx_count);
             if (nTimeDiff > 0) {
                 ret.pushKV("txrate", double(window_tx_count) / nTimeDiff);
@@ -2799,7 +2799,7 @@ UniValue CreateUTXOSnapshot(
     result.pushKV("base_height", tip->nHeight);
     result.pushKV("path", path.utf8string());
     result.pushKV("txoutset_hash", maybe_stats->hashSerialized.ToString());
-    result.pushKV("nchaintx", tip->nChainTx);
+    result.pushKV("nchaintx", tip->m_chain_tx_count);
     return result;
 }
 

src/test/blockchain_tests.cpp

@@ -110,4 +110,11 @@ BOOST_FIXTURE_TEST_CASE(get_prune_height, TestChain100Setup)
     CheckGetPruneHeight(blockman, chain, 100);
 }
 
+BOOST_AUTO_TEST_CASE(num_chain_tx_max)
+{
+    CBlockIndex block_index{};
+    block_index.m_chain_tx_count = std::numeric_limits<uint64_t>::max();
+    BOOST_CHECK_EQUAL(block_index.m_chain_tx_count, std::numeric_limits<uint64_t>::max());
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/test/fuzz/utxo_snapshot.cpp

@@ -113,9 +113,9 @@ FUZZ_TARGET(utxo_snapshot, .init = initialize_chain)
             if (index->nHeight == chainman.GetSnapshotBaseHeight()) {
                 auto params{chainman.GetParams().AssumeutxoForHeight(index->nHeight)};
                 Assert(params.has_value());
-                Assert(params.value().nChainTx == index->nChainTx);
+                Assert(params.value().m_chain_tx_count == index->m_chain_tx_count);
             } else {
-                Assert(index->nChainTx == 0);
+                Assert(index->m_chain_tx_count == 0);
             }
         }
         Assert(g_chain->size() == coinscache.GetCacheSize());

src/test/pmt_tests.cpp

@@ -29,10 +29,10 @@ BOOST_FIXTURE_TEST_SUITE(pmt_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(pmt_test1)
 {
-    static const unsigned int nTxCounts[] = {1, 4, 7, 17, 56, 100, 127, 256, 312, 513, 1000, 4095};
+    static const unsigned int tx_counts[] = {1, 4, 7, 17, 56, 100, 127, 256, 312, 513, 1000, 4095};
 
     for (int i = 0; i < 12; i++) {
-        unsigned int nTx = nTxCounts[i];
+        unsigned int nTx = tx_counts[i];
 
         // build a block with some dummy transactions
         CBlock block;

src/test/util/chainstate.h

@@ -99,7 +99,7 @@ CreateAndActivateUTXOSnapshot(
                 assert(pindex->IsValid(BlockStatus::BLOCK_VALID_TREE));
                 pindex->nStatus = BlockStatus::BLOCK_VALID_TREE;
                 pindex->nTx = 0;
-                pindex->nChainTx = 0;
+                pindex->m_chain_tx_count = 0;
                 pindex->nSequenceId = 0;
                 pindex = pindex->pprev;
             }

src/test/validation_chainstatemanager_tests.cpp

@@ -285,7 +285,7 @@ struct SnapshotTestSetup : TestChain100Setup {
         const auto& au_data = ::Params().AssumeutxoForHeight(snapshot_height);
         const CBlockIndex* tip = WITH_LOCK(chainman.GetMutex(), return chainman.ActiveTip());
 
-        BOOST_CHECK_EQUAL(tip->nChainTx, au_data->nChainTx);
+        BOOST_CHECK_EQUAL(tip->m_chain_tx_count, au_data->m_chain_tx_count);
 
         // To be checked against later when we try loading a subsequent snapshot.
         uint256 loaded_snapshot_blockhash{*chainman.SnapshotBlockhash()};
@@ -465,7 +465,7 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_loadblockindex, TestChain100Setup)
         if (i < last_assumed_valid_idx && i >= assumed_valid_start_idx) {
             index->nStatus = BlockStatus::BLOCK_VALID_TREE;
             index->nTx = 0;
-            index->nChainTx = 0;
+            index->m_chain_tx_count = 0;
         }
 
         ++num_indexes;