Update nChainTx to 64bit type about bitcoin

[maflcko]

For reference, I removed nChainTx from consensus in #29349. The next steps would be to add a member to CChain to account for the sum of nTx, and update it when SetTip is called. Then, update the RPC, wallet and estimate functions to derive the nChainTx they need from the nChainTx at the tip, walking the relevant portion of the chain.

This is possible, but I am not sure if it is worth it, because:

• It is more code overall.
• The RPC, wallet, and logging code is slower when it has to walk the chain.

The benefit mainly seems to be to reduce the memory footprint? On 64-bit platforms, sizeof(CBlockIndex) is 144. Adding 4 bytes or removing 4 bytes is less than 10% of cost or savings. Ref: https://godbolt.org/z/rKEPYTh4j

Removing nTx is even harder, so less worth it. Especially, given that it could be reduced to 16 bits in the memory layout, if needed.

This leads to an alternative solution to not increase the memory footprint: Move the 32+32 bits used for nTx and nChainTx into a single 64-bit blob, where the 16 bit are for nTx and the remaining for nChainTx. But, again, the overall heap memory usage shouldn't change much, either way.

[ajtowns]

Background: #13875 and #1677

[russeree]

Would this be a hard fork because it would be loosening the conditions for validation?

[mzumsande]

Would this be a hard fork because it would be loosening the conditions for validation?

I don't think so, since I don't think the value of nChainTx is used in validation (or do you see a spot where it is?). It's used for logging (progress estimation), and whether it is 0 is used in HaveNumChainTxs. The latter could become problematic in case of an overflow, since if the unsigned int would overflow exactly to zero (which is a bit unlikely given that there are a few thousand txns in each block but ceratainly possible) the node would crash in

bitcoin/src/validation.cpp

Line 3017 in 478ac18

assert(pindexTest->HaveNumChainTxs() || pindexTest->nHeight == 0);

.

[russeree]

^^^ The above was the line that I was referencing. Thank you.

[Sjors]

There was also the idea of dropping this number entirely: #13875 (comment)

Though see also #13875 (comment)

[maflcko]

Though see also #13875 (comment)

This is nTx, not nChainTx.

[ryanofsky]

This leads to an alternative solution to not increase the memory footprint: Move the 32+32 bits used for nTx and nChainTx into a single 64-bit blob, where the 16 bit are for nTx and the remaining for nChainTx. But, again, the overall heap memory usage shouldn't change much, either way.

This does seem like an nice and easy solution. Would be good to incorporate in #29331

As a tangent, I was thinking along the same lines last week before implementing #29370 and came up with worse solution, taking advantage of the fact that when nChainTx is set, you can derive nTx by subtracting nChainTx - pprev->nChainTx. In case it's interesting:

nChainTx hack

class CBlockIndex
{
public:
    CBlockIndex* pprev{nullptr};

    // Indicate whether num_tx_value contains total number of transactions in
    // this block, or total number of transactions in the chain up and including
    // this block.
    enum NumTxType { UNSET = 0, BLOCK_TX = 1, CHAIN_TX = 2 };
    NumTxType num_tx_type : 4 {UNSET};
    uint64_t num_tx_value : 60;

    uint64_t NumBlockTx()
    {
        if (num_tx_type == BLOCK_TX) return num_tx_value;
        if (num_tx_type == CHAIN_TX) {
            assert(pprev && pprev->num_tx_type == CHAIN_TX);
            return num_tx_value - pprev->num_tx_value;
        }
        return 0;
    }

    uint64_t NumChainTx()
    {
        return num_tx_type == CHAIN_TX ? num_tx_value : 0;
    }
};

[maflcko]

This does seem like an nice and easy solution. Would be good to incorporate in #29331

Not sure if it so easy. On 32-bit platforms, nTx currently starts at byte 60, so making it 16-bit will still eat 32-bit with padding. So forcing a 64-bit blob of both values is needed, but that requires changing all code that uses either field. Not sure if worth it for a +-5% memory difference?

[ryanofsky]

Not sure if it so easy. On 32-bit platforms, nTx currently starts at byte 60, so making it 16-bit will still eat 32-bit with padding.

I didn't test on a 32-bit platform, but aren't bitfields a good solution to this? The following seems to work for me, increasing nChainTx range without increasing CBlockIndex size on x86_64 (144 bytes):

diff

--- a/src/chain.h
+++ b/src/chain.h
@@ -178,7 +178,7 @@ public:
     //! Note: this value is faked during UTXO snapshot load to ensure that
     //! LoadBlockIndex() will load index entries for blocks that we lack data for.
     //! @sa ActivateSnapshot
-    unsigned int nTx{0};
+    uint16_t nTx : 16 {0};
 
     //! (memory only) Number of transactions in the chain up to and including this block.
     //! This value will be non-zero only if and only if transactions for this block and all its parents are available.
@@ -188,7 +188,7 @@ public:
     //! have the underlying block data available during snapshot load.
     //! @sa AssumeutxoData
     //! @sa ActivateSnapshot
-    unsigned int nChainTx{0};
+    uint64_t nChainTx : 48 {0};
 
     //! Verification status of this block. See enum BlockStatus
     //!
@@ -412,7 +412,12 @@ public:
 
         READWRITE(VARINT_MODE(obj.nHeight, VarIntMode::NONNEGATIVE_SIGNED));
         READWRITE(VARINT(obj.nStatus));
-        READWRITE(VARINT(obj.nTx));
+
+        unsigned int num_tx;
+        SER_WRITE(obj, num_tx = obj.nTx);
+        READWRITE(VARINT(num_tx));
+        SER_READ(obj, obj.nTx = num_tx);
+
         if (obj.nStatus & (BLOCK_HAVE_DATA | BLOCK_HAVE_UNDO)) READWRITE(VARINT_MODE(obj.nFile, VarIntMode::NONNEGATIVE_SIGNED));
         if (obj.nStatus & BLOCK_HAVE_DATA) READWRITE(VARINT(obj.nDataPos));
         if (obj.nStatus & BLOCK_HAVE_UNDO) READWRITE(VARINT(obj.nUndoPos));
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5119,7 +5119,7 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin
 
     double fTxTotal;
 
-    if (pindex->nChainTx <= data.nTxCount) {
+    if (static_cast<int64_t>(pindex->nChainTx) <= data.nTxCount) {
         fTxTotal = data.nTxCount + (nNow - data.nTime) * data.dTxRate;
     } else {
         fTxTotal = pindex->nChainTx + (nNow - pindex->GetBlockTime()) * data.dTxRate;

[maflcko]

bitfields

Jup, that didn't work for me. Or maybe I did something wrong. See https://godbolt.org/z/z6EoWKbjY

   60:0-15 |   int16_t nTx
   64:0-47 |   int64_t nChainTx

(nTx eats byte 60-64, even though it is only a two byte bitfield)

[ryanofsky]

Jup, that didn't work for me. Or maybe I did something wrong. See https://godbolt.org/z/z6EoWKbjY

It's not that surprising that the compiler adds padding so the struct member with uint64_t type is inside a field aligned to 8 bytes. But if you move the two bitfield members to begin on an 8 byte boundary (like between the nDataPos and nUndoPos fields, or to the beginning of the struct, it does get packed without extra padding). So I think the diff above is an easy way to avoid consuming extra memory on 64 bit platforms, even though we could go further with more changes to get the same optimization on 32 bit.

[ajtowns]

bitfields

Jup, that didn't work for me. Or maybe I did something wrong. See https://godbolt.org/z/z6EoWKbjY

   60:0-15 |   int16_t nTx
   64:0-47 |   int64_t nChainTx

(nTx eats byte 60-64, even though it is only a two byte bitfield)

I think the problem here is you have an odd number of 32-bit pointers prior to nTx, so it's misaligned to be combined with nChainTx. So you end up with nTx [4B-6B padding] nChainTx nStatus [4B padding]. Moving nStatus prior to nTx gives an even number of 32-bit objects prior to nTx, so drops the size back down.