log: avoid collecting GetSerializeSize data when compact block logging is disabled #33738

pull l0rinc wants to merge 3 commits into bitcoin:master from l0rinc:l0rinc/debug-log-serialization changing 10 files +31 −25
  1. l0rinc commented at 10:57 pm on October 29, 2025: contributor

    Context

    The new accounting options introduced in #32582 can be quite heavy, and are not needed when debug logging is disabled.

    Problem

    PartiallyDownloadedBlock::FillBlock() and PeerManagerImpl::SendBlockTransactions() accumulate transaction sizes for debug logging by calling ComputeTotalSize() in loops, which invokes expensive GetSerializeSize() serializations. The block header hash is also only computed for the debug log.

    Fixes

    Guard the size and hash calculations with LogAcceptCategory() checks so the serialization and hashing work only occurs when compact block debug logging is enabled.

    Also modernized the surrounding code a bit since the change is quite trivial.

    Reproducer

    You can test the change by starting an up-to-date bitcoind node with -debug=cmpctblock and observing compact block log lines such as:

    [cmpctblock] Successfully reconstructed block 00000000000000000001061eaa6c0fe79258e7f79606e67ac495765cb121a520 with 1 txn prefilled, 3122 txn from mempool (incl at least 3 from extra pool) and 641 txn (352126 bytes) requested

     0diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
     1index 58620c93cc..f16eb38fa5 100644
     2--- a/src/blockencodings.cpp
     3+++ b/src/blockencodings.cpp
     4@@ -186,6 +186,7 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const
     5
     6 ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active)
     7 {
     8+    LogInfo("PartiallyDownloadedBlock::FillBlock called");
     9     if (header.IsNull()) return READ_STATUS_INVALID;
    10
    11     block = header;
    12@@ -218,6 +219,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
    13     }
    14
    15     if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
    16+        LogInfo("debug log enabled");
    17         const uint256 hash{block.GetHash()}; // avoid cleared header
    18         uint32_t tx_missing_size{0};
    19         for (const auto& tx : vtx_missing) tx_missing_size += tx->ComputeTotalSize(); // avoid cleared txn_available
    20diff --git a/src/net_processing.cpp b/src/net_processing.cpp
    21index 5600c8d389..c081825f77 100644
    22--- a/src/net_processing.cpp
    23+++ b/src/net_processing.cpp
    24@@ -2470,6 +2470,7 @@ uint32_t PeerManagerImpl::GetFetchFlags(const Peer& peer) const
    25
    26 void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlock& block, const BlockTransactionsRequest& req)
    27 {
    28+    LogInfo("PeerManagerImpl::SendBlockTransactions called");
    29     BlockTransactions resp(req);
    30     for (size_t i = 0; i < req.indexes.size(); i++) {
    31         if (req.indexes[i] >= block.vtx.size()) {
    32@@ -2480,6 +2481,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
    33     }
    34
    35     if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
    36+        LogInfo("debug log enabled");
    37         uint32_t tx_requested_size{0};
    38         for (const auto i : req.indexes) tx_requested_size += block.vtx[i]->ComputeTotalSize();
    39         LogDebug(BCLog::CMPCTBLOCK, "Peer %d sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
    
  2. DrahtBot added the label Utils/log/libs on Oct 29, 2025
  3. DrahtBot commented at 10:57 pm on October 29, 2025: 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/33738.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator
    Concept ACK 0xB10C
    Stale ACK davidgumberg, sedited, danielabrozzoni

    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:

    • #32606 (p2p: Drop unsolicited CMPCTBLOCK from non-HB peer by davidgumberg)

    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.

  4. davidgumberg commented at 1:24 am on October 30, 2025: contributor
    Thanks for catching this, the PR seems correct to me, why is it in draft?
  5. sedited commented at 6:43 am on October 30, 2025: contributor
    Concept ACK
  6. in src/net_processing.cpp:2486 in 17856343f8
    2483+        if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
    2484+            tx_requested_size += resp.txn[i]->GetTotalSize();
    2485+        }
    2486     }
    2487 
    2488     LogDebug(BCLog::CMPCTBLOCK, "Peer %d sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
    


    maflcko commented at 6:43 am on October 30, 2025:

    in theory this seems racy, when the logging is toggled in another thread, but this is probably good enough to ignore.

    Though, on a different note, I wonder if there are other places that could benefit from CTransaction caching. Two full hashes are cached, so caching the size should be cheap in both time and space?


    l0rinc commented at 6:48 am on October 30, 2025:

    the hash is actually eagerly calculated as well - which should also be hidden behind a similar condition, was hoping someone will bring it up :)


    there aren’t any benchmarks for this but a ton of tests failed for the throw check above, so at least it has coverage.


    davidgumberg commented at 0:28 am on October 31, 2025:

    wonder if there are other places that could benefit from CTransaction caching. Two full hashes are cached, so caching the size should be cheap in both time and space?

    I don’t think we should be caching the sizes, since there aren’t really any performance critical users of GetTotalSize(), it’s invoked by these cmpctblock logs, many RPC’s when they do pretty-printing of transactions, and the qt interface’s transaction view, but there are critical paths where CTransaction()’s are constructed.


    the hash is actually eagerly calculated as well

    As far as I can tell, CTransaction’s hashes are computed and cached on construction: https://github.com/bitcoin/bitcoin/blob/6f359695c36c13af832b0b1021dcb3a234f6f219/src/primitives/transaction.cpp#L95-L96 https://github.com/bitcoin/bitcoin/blob/6f359695c36c13af832b0b1021dcb3a234f6f219/src/primitives/transaction.h#L343-L344

    Are you suggesting that the hashes should be computed lazily when CTransaction::GetHash() is first invoked?


    maflcko commented at 6:49 am on October 31, 2025:

    I don’t think we should be caching the sizes,

    Good point. I haven’t looked at the other places where it is used. I guess one could consider renaming GetTotalSize to ComputeTotalSize, to hopefully avoid similar issues of this kind in the future?


    l0rinc commented at 3:14 pm on October 31, 2025:

    As far as I can tell, CTransaction’s hashes are computed and cached on construction

    No, I mean uint256 hash = header.GetHash() is only needed for the debug log - let me try separating all calculations that are only needed for debugging. Given that we’re clearing header and txn_available I understand why the split wasn’t done before. But we can still use block for the hash and vtx_missing for the missing, so I think it should be possible to split the two. That’s a more invasive change but it would separate the performance critical calculations from the debugging data. https://github.com/bitcoin/bitcoin/blob/a0c254c13a3ef21e257cca3493446c632b636b15/src/primitives/block.cpp#L11-L14

    Are you suggesting that the hashes should be computed lazily when CTransaction::GetHash() is first invoked?

    I did investigate that in https://github.com/l0rinc/bitcoin/pull/21, but haven’t finished the investigation since the initial numbers weren’t convincing enough.

    I guess one could consider renaming GetTotalSize to ComputeTotalSize

    Thanks, pushed that in a separate commit (before the rest to showcase why we’re avoiding the call in the first place). It’s small enough that I didn’t bother with a scripted diff.

  7. maflcko commented at 6:43 am on October 30, 2025: member
    is there a benchmark that detects this?
  8. l0rinc marked this as ready for review on Oct 30, 2025
  9. DrahtBot requested review from sedited on Oct 31, 2025
  10. 0xB10C commented at 1:45 pm on October 31, 2025: contributor
    Concept ACK
  11. l0rinc force-pushed on Oct 31, 2025
  12. l0rinc force-pushed on Oct 31, 2025
  13. luke-jr referenced this in commit 065af0f17c on Nov 5, 2025
  14. danielabrozzoni approved
  15. danielabrozzoni commented at 9:12 am on November 8, 2025: member

    Code Review ACK 10e0e96e703a40b298b87e9943f85d5189b9e3dc

    Code looks good to me, I ran the tests locally, but I didn’t perform any additional testing beyond that.

  16. DrahtBot requested review from davidgumberg on Nov 8, 2025
  17. DrahtBot requested review from 0xB10C on Nov 8, 2025
  18. in src/blockencodings.cpp:233 in 10e0e96e70 outdated
    236+        LogDebug(BCLog::CMPCTBLOCK, "Successfully reconstructed block %s with %u txn prefilled, %u txn from mempool (incl at least %u from extra pool) and %u txn (%u bytes) requested\n", hash.ToString(), prefilled_count, mempool_count, extra_count, vtx_missing.size(), tx_missing_size);
    237+        if (vtx_missing.size() < 5) {
    238+            for (const auto& tx : vtx_missing) {
    239+                LogDebug(BCLog::CMPCTBLOCK, "Reconstructed block %s required tx %s\n", hash.ToString(), tx->GetHash().ToString());
    240+            }
    241         }
    


    vasild commented at 5:09 pm on November 20, 2025:

    Would it be useful to print the first 5, even if there are more? E.g.

    0Reconstructed block B required tx 111
    1Reconstructed block B required tx 222
    2Reconstructed block B required tx 333
    3Reconstructed block B required tx 444
    4Reconstructed block B required tx 555 and others
    

    l0rinc commented at 6:19 pm on November 20, 2025:
    Good idea. Should we do it in a follow-up instead?

    davidgumberg commented at 6:25 pm on November 20, 2025:

    I don’t think it’s very helpful to print 5, when I want to know the tx’es causing compact block reconstruction to fail I remove this if statement so that all missing tx’es are printed, and I have a hard time imagining a use case for only wanting the txid’s if it’s less than 5 or only wanting the first 5.

    I think we should print all of the missing transactions every time, but can definitely be discussed in a separate issue/pr.

  19. in src/blockencodings.cpp:207 in 10e0e96e70 outdated
    200+            if (vtx_missing.size() <= tx_missing_offset) {
    201                 return READ_STATUS_INVALID;
    202+            }
    203             block.vtx[i] = vtx_missing[tx_missing_offset++];
    204-            tx_missing_size += block.vtx[i]->GetTotalSize();
    205-        } else
    


    vasild commented at 5:18 pm on November 20, 2025:

    I am missing some high level context here, but is this guaranteed to iterate over all members of vtx_missing[]?

    I ask because down there we do iterate the full vtx_missing[] to derive tx_missing_size.


    l0rinc commented at 6:20 pm on November 20, 2025:
    I have tried explaining it in the commit message, can you please point out where more context is necessarry?

    davidgumberg commented at 6:52 pm on November 20, 2025:

    Is this guaranteed to iterate over all members of vtx_missing[]?

    Yes, because of the check below that:

    https://github.com/bitcoin/bitcoin/blob/6b2d17b13220ea69c33b8cab41fe059ff758874c/src/blockencodings.cpp#L211-L212

    We increment tx_missing_offset every time we take a transaction from vtx_missing, so the check enforces that we followed the if branch / took from vtx_missing just as many times as there are members in vtx_missing.


    hodlinator commented at 9:31 pm on December 18, 2025:

    nit: I found this last explanation helpful. Would change commit message:

    0- Since multiple arguments are invalidated after the first iteration (needed for
    1- efficient moving), we can't redo the exact same loop, but we can iterate the
    2- `missing` arg directly and use the block header (which we've copied at the
    3- beginning) for the header hash instead.
    4+ txn_available is invalidated after the first loop (needed for efficient moving),
    5+ so instead we loop through vtx_missing directly which is guaranteed to be
    6+ consumed by the first loop (see later check against tx_missing_offset).
    7+ We also switch from computing the hash from the header to the block.
    

    l0rinc commented at 11:07 am on December 22, 2025:
  20. in src/net_processing.cpp:2484 in 4e8bba381e outdated
    2482     }
    2483 
    2484-    LogDebug(BCLog::CMPCTBLOCK, "Peer %d sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
    2485+    if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
    2486+        uint32_t tx_requested_size{0};
    2487+        for (const auto i : req.indexes) tx_requested_size += block.vtx[i]->ComputeTotalSize();
    


    davidgumberg commented at 7:12 pm on November 20, 2025:

    nit:

    0        for (const auto &tx : resp.txn) tx_requested_size += tx->ComputeTotalSize();
    

    l0rinc commented at 11:07 am on December 22, 2025:
  21. DrahtBot requested review from davidgumberg on Nov 20, 2025
  22. davidgumberg commented at 10:02 pm on November 20, 2025: contributor

    crACK 10e0e96e703a40b298b8

    Lightly tested running on mainnet, logs for receiving look good:

    02025-11-20T21:56:39.778340Z [cmpctblock] Successfully reconstructed block 000000000000000000004152f92a1ec0a2f2e4356264001b11361f35247f641d with 1 txn prefilled, 3940 txn from mempool (incl at least 6 from extra pool) and 141 txn (64021 bytes) requested
    

    Haven’t received a GETBLOCKTXN request yet.

  23. in src/blockencodings.cpp:201 in 10e0e96e70 outdated
    195-    unsigned int tx_missing_size = 0;
    196     size_t tx_missing_offset = 0;
    197     for (size_t i = 0; i < txn_available.size(); i++) {
    198         if (!txn_available[i]) {
    199-            if (vtx_missing.size() <= tx_missing_offset)
    200+            if (vtx_missing.size() <= tx_missing_offset) {
    


    hodlinator commented at 9:07 pm on December 18, 2025:

    meganit: Feel free to ignore. Since we’re touching this line… it would be more natural to have what changes on the left hand side - We’re not testing that the vector shrinks too small, but rather that we don’t increment the index out of bounds.

    0            if (tx_missing_offset >= vtx_missing.size()) {
    

    Same but less itchy below:

    0-if (vtx_missing.size() != tx_missing_offset) {
    1+if (tx_missing_offset != vtx_missing.size()) {
    

    l0rinc commented at 11:07 am on December 22, 2025:
  24. hodlinator commented at 10:32 pm on December 18, 2025: contributor

    Reviewed 10e0e96e703a40b298b87e9943f85d5189b9e3dc

    Agree the situation on master is not ideal.

    However, when I see CTransaction I often think it should be renamed ImmutableTransaction (to counter CMutableTransaction).

    In that light, and since it caches hashes, it seems worth at least exploring computing sizes as we are scanning the bytes to eagerly compute either hash. Even if some future endeavor has us lazy-computing the hash (which I would be happy to see), size & hash could still be computed together.

    After realizing benchmarks were skewed, I replaced the block data from 2016 AD with a recent block to be more representative:

    0./build/bin/bitcoin-cli getblock 000000000000000000008ba770436ce094dc41adb020ce92eb00632b63af11a4 0 | xxd -r -p > src/bench/data/block413567.raw
    

    (For the issue on more representative block data, see #32554).


      0diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp
      1index e80ab60fcd..37cd75099d 100644
      2--- a/src/primitives/transaction.cpp
      3+++ b/src/primitives/transaction.cpp
      4@@ -78,22 +78,74 @@ bool CTransaction::ComputeHasWitness() const
      5     });
      6 }
      7 
      8-Txid CTransaction::ComputeHash() const
      9+namespace {
     10+class CombinedWriter
     11 {
     12-    return Txid::FromUint256((HashWriter{} << TX_NO_WITNESS(*this)).GetHash());
     13+    HashWriter m_hash_writer;
     14+    SizeComputer m_size_computer;
     15+
     16+public:
     17+    void write(std::span<const std::byte> src)
     18+    {
     19+        m_hash_writer.write(src);
     20+        m_size_computer.write(src);
     21+    }
     22+
     23+    template <typename T>
     24+    CombinedWriter& operator<<(const T& obj)
     25+    {
     26+        ::Serialize(*this, obj);
     27+        return *this;
     28+    }
     29+
     30+    size_t GetSize() const {
     31+        return m_size_computer.size();
     32+    }
     33+
     34+    uint256 GetHash() {
     35+        return m_hash_writer.GetHash();
     36+    }
     37+};
     38+} // namespace
     39+
     40+Txid CTransaction::ComputeHash(size_t& out_size) const
     41+{
     42+    if (HasWitness()) {
     43+        return Txid::FromUint256((HashWriter{} << TX_NO_WITNESS(*this)).GetHash());
     44+    }
     45+
     46+    CombinedWriter combined_writer;
     47+    combined_writer << TX_NO_WITNESS(*this);
     48+    out_size = combined_writer.GetSize();
     49+    auto result{combined_writer.GetHash()};
     50+
     51+    // Costly, just for verification
     52+    //assert(result == (HashWriter{} << TX_NO_WITNESS(*this)).GetHash());
     53+    //assert(out_size == ::GetSerializeSize(TX_NO_WITNESS(*this)));
     54+
     55+    return Txid::FromUint256(result);
     56 }
     57 
     58-Wtxid CTransaction::ComputeWitnessHash() const
     59+Wtxid CTransaction::ComputeWitnessHash(size_t& out_size) const
     60 {
     61     if (!HasWitness()) {
     62         return Wtxid::FromUint256(hash.ToUint256());
     63     }
     64 
     65-    return Wtxid::FromUint256((HashWriter{} << TX_WITH_WITNESS(*this)).GetHash());
     66+    CombinedWriter combined_writer;
     67+    combined_writer << TX_WITH_WITNESS(*this);
     68+    out_size = combined_writer.GetSize();
     69+    auto result{combined_writer.GetHash()};
     70+
     71+    // Costly, just for verification
     72+    //assert(result == (HashWriter{} << TX_WITH_WITNESS(*this)).GetHash());
     73+    //assert(out_size == ::GetSerializeSize(TX_WITH_WITNESS(*this)));
     74+
     75+    return Wtxid::FromUint256(result);
     76 }
     77 
     78-CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), version{tx.version}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
     79-CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), version{tx.version}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
     80+CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), version{tx.version}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash(/*out_size=*/m_size)}, m_witness_hash{ComputeWitnessHash(/*out_size=*/m_size)} {}
     81+CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), version{tx.version}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash(/*out_size=*/m_size)}, m_witness_hash{ComputeWitnessHash(/*out_size=*/m_size)} {}
     82 
     83 CAmount CTransaction::GetValueOut() const
     84 {
     85@@ -109,7 +161,7 @@ CAmount CTransaction::GetValueOut() const
     86 
     87 unsigned int CTransaction::GetTotalSize() const
     88 {
     89-    return ::GetSerializeSize(TX_WITH_WITNESS(*this));
     90+    return m_size;
     91 }
     92 
     93 std::string CTransaction::ToString() const
     94diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
     95index 295bce61b9..406f6fb9ff 100644
     96--- a/src/primitives/transaction.h
     97+++ b/src/primitives/transaction.h
     98@@ -313,9 +313,10 @@ private:
     99     const bool m_has_witness;
    100     const Txid hash;
    101     const Wtxid m_witness_hash;
    102+    size_t m_size;
    103 
    104-    Txid ComputeHash() const;
    105-    Wtxid ComputeWitnessHash() const;
    106+    Txid ComputeHash(size_t& out_size) const;
    107+    Wtxid ComputeWitnessHash(size_t& out_size) const;
    108 
    109     bool ComputeHasWitness() const;
    110 
    
    0cmake --build build -t bench_bitcoin && build/bin/bench_bitcoin -filter="DeserializeBlockTest" -min-time=10000
    

    Without diff (but still new block data):

    0|            ns/block |             block/s |    err% |       ins/block |       cyc/block |    IPC |      bra/block |   miss% |     total | benchmark
    1|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
    2|       13,695,535.27 |               73.02 |    0.1% |  172,027,205.56 |   50,480,492.19 |  3.408 |   7,936,417.71 |    0.1% |     32.95 | `DeserializeBlockTest`
    3|       13,719,279.25 |               72.89 |    0.1% |  172,026,940.33 |   50,563,252.02 |  3.402 |   7,936,357.15 |    0.1% |     32.84 | `DeserializeBlockTest`
    4|       13,736,674.56 |               72.80 |    0.2% |  172,027,205.58 |   50,631,723.07 |  3.398 |   7,936,417.76 |    0.1% |     32.98 | `DeserializeBlockTest`
    

    With new block data diff (size computed with hash):

    0|            ns/block |             block/s |    err% |       ins/block |       cyc/block |    IPC |      bra/block |   miss% |     total | benchmark
    1|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
    2|       13,758,327.17 |               72.68 |    0.1% |  172,313,875.87 |   50,716,919.21 |  3.398 |   7,948,257.78 |    0.1% |     32.97 | `DeserializeBlockTest`
    3|       13,770,950.59 |               72.62 |    0.1% |  172,313,875.88 |   50,762,181.95 |  3.395 |   7,948,257.79 |    0.1% |     32.83 | `DeserializeBlockTest`
    4|       13,743,023.50 |               72.76 |    0.1% |  172,313,864.58 |   50,656,651.53 |  3.402 |   7,948,295.76 |    0.1% |     32.89 | `DeserializeBlockTest`
    

      0diff --git a/src/crypto/sha256.h b/src/crypto/sha256.h
      1index 3ac771c5d0..b97ace5fcc 100644
      2--- a/src/crypto/sha256.h
      3+++ b/src/crypto/sha256.h
      4@@ -23,6 +23,7 @@ public:
      5     CSHA256();
      6     CSHA256& Write(const unsigned char* data, size_t len);
      7     void Finalize(unsigned char hash[OUTPUT_SIZE]);
      8+    uint64_t GetInputSize() const { return bytes; }
      9     CSHA256& Reset();
     10 };
     11 
     12diff --git a/src/hash.h b/src/hash.h
     13index 34486af64a..a4943fd69a 100644
     14--- a/src/hash.h
     15+++ b/src/hash.h
     16@@ -108,6 +108,8 @@ public:
     17         ctx.Write(UCharCast(src.data()), src.size());
     18     }
     19 
     20+    uint64_t GetInputSize() const { return ctx.GetInputSize(); }
     21+
     22     /** Compute the double-SHA256 hash of all data written to this object.
     23      *
     24      * Invalidates this object.
     25diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp
     26index e80ab60fcd..d9ebcfc1b7 100644
     27--- a/src/primitives/transaction.cpp
     28+++ b/src/primitives/transaction.cpp
     29@@ -78,22 +78,44 @@ bool CTransaction::ComputeHasWitness() const
     30     });
     31 }
     32 
     33-Txid CTransaction::ComputeHash() const
     34+Txid CTransaction::ComputeHash(size_t& out_size) const
     35 {
     36-    return Txid::FromUint256((HashWriter{} << TX_NO_WITNESS(*this)).GetHash());
     37+    if (HasWitness()) {
     38+        return Txid::FromUint256((HashWriter{} << TX_NO_WITNESS(*this)).GetHash());
     39+    }
     40+
     41+    HashWriter writer;
     42+    writer << TX_NO_WITNESS(*this);
     43+    out_size = writer.GetInputSize();
     44+    auto result{writer.GetHash()};
     45+
     46+    // Costly, just for verification
     47+    //assert(result == (HashWriter{} << TX_NO_WITNESS(*this)).GetHash());
     48+    //assert(out_size == ::GetSerializeSize(TX_NO_WITNESS(*this)));
     49+
     50+    return Txid::FromUint256(result);
     51 }
     52 
     53-Wtxid CTransaction::ComputeWitnessHash() const
     54+Wtxid CTransaction::ComputeWitnessHash(size_t& out_size) const
     55 {
     56     if (!HasWitness()) {
     57         return Wtxid::FromUint256(hash.ToUint256());
     58     }
     59 
     60-    return Wtxid::FromUint256((HashWriter{} << TX_WITH_WITNESS(*this)).GetHash());
     61+    HashWriter writer;
     62+    writer << TX_WITH_WITNESS(*this);
     63+    out_size = writer.GetInputSize();
     64+    auto result{writer.GetHash()};
     65+
     66+    // Costly, just for verification
     67+    //assert(result == (HashWriter{} << TX_WITH_WITNESS(*this)).GetHash());
     68+    //assert(out_size == ::GetSerializeSize(TX_WITH_WITNESS(*this)));
     69+
     70+    return Wtxid::FromUint256(result);
     71 }
     72 
     73-CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), version{tx.version}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
     74-CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), version{tx.version}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash()}, m_witness_hash{ComputeWitnessHash()} {}
     75+CTransaction::CTransaction(const CMutableTransaction& tx) : vin(tx.vin), vout(tx.vout), version{tx.version}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash(/*out_size=*/m_size)}, m_witness_hash{ComputeWitnessHash(/*out_size=*/m_size)} {}
     76+CTransaction::CTransaction(CMutableTransaction&& tx) : vin(std::move(tx.vin)), vout(std::move(tx.vout)), version{tx.version}, nLockTime{tx.nLockTime}, m_has_witness{ComputeHasWitness()}, hash{ComputeHash(/*out_size=*/m_size)}, m_witness_hash{ComputeWitnessHash(/*out_size=*/m_size)} {}
     77 
     78 CAmount CTransaction::GetValueOut() const
     79 {
     80@@ -109,7 +131,7 @@ CAmount CTransaction::GetValueOut() const
     81 
     82 unsigned int CTransaction::GetTotalSize() const
     83 {
     84-    return ::GetSerializeSize(TX_WITH_WITNESS(*this));
     85+    return m_size;
     86 }
     87 
     88 std::string CTransaction::ToString() const
     89diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
     90index 295bce61b9..406f6fb9ff 100644
     91--- a/src/primitives/transaction.h
     92+++ b/src/primitives/transaction.h
     93@@ -313,9 +313,10 @@ private:
     94     const bool m_has_witness;
     95     const Txid hash;
     96     const Wtxid m_witness_hash;
     97+    size_t m_size;
     98 
     99-    Txid ComputeHash() const;
    100-    Wtxid ComputeWitnessHash() const;
    101+    Txid ComputeHash(size_t& out_size) const;
    102+    Wtxid ComputeWitnessHash(size_t& out_size) const;
    103 
    104     bool ComputeHasWitness() const;
    105 
    
    0cmake --build build -t bench_bitcoin && build/bin/bench_bitcoin -filter="DeserializeBlockTest" -min-time=10000
    

    Without diff (but still new block data):

    0|            ns/block |             block/s |    err% |       ins/block |       cyc/block |    IPC |      bra/block |   miss% |     total | benchmark
    1|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
    2|       13,642,735.69 |               73.30 |    0.0% |  172,027,198.95 |   50,303,451.00 |  3.420 |   7,936,417.69 |    0.1% |     10.98 | `DeserializeBlockTest`
    3|       13,696,742.23 |               73.01 |    0.0% |  172,027,199.00 |   50,509,943.00 |  3.406 |   7,936,417.71 |    0.1% |     10.98 | `DeserializeBlockTest`
    4|       13,608,190.19 |               73.49 |    0.0% |  172,027,198.90 |   50,181,783.58 |  3.428 |   7,936,417.64 |    0.1% |     11.01 | `DeserializeBlockTest`
    

    With new block data diff (size read off CSHA256::bytes):

    0|            ns/block |             block/s |    err% |       ins/block |       cyc/block |    IPC |      bra/block |   miss% |     total | benchmark
    1|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
    2|       13,597,905.17 |               73.54 |    0.2% |  172,124,011.94 |   50,128,232.00 |  3.434 |   7,951,499.67 |    0.1% |     11.01 | `DeserializeBlockTest`
    3|       13,691,575.07 |               73.04 |    0.1% |  172,123,994.96 |   50,450,106.97 |  3.412 |   7,951,537.71 |    0.1% |     10.95 | `DeserializeBlockTest`
    4|       13,584,015.82 |               73.62 |    0.0% |  172,124,011.88 |   50,092,367.11 |  3.436 |   7,951,499.62 |    0.1% |     11.00 | `DeserializeBlockTest`
    

    I can see the point of GetTotalSize() having so few users though. Not going to N-A-C-K current PR state, but might be a shame to change the name to ComputeTotalSize if we later decide to return a cached size value.

  25. l0rinc force-pushed on Dec 22, 2025
  26. l0rinc commented at 11:06 am on December 22, 2025: contributor

    However, when I see CTransaction I often think it should be renamed ImmutableTransaction (to counter CMutableTransaction).

    I didn’t understand at first how that’s related, but slept on it and now I do :) I don’t mind experimenting with that in a separate PR (thanks for the detailed reproducers and benchmarks), had a similar-looking draft in https://github.com/l0rinc/bitcoin/pull/21 (caching the hashes lazily - caching the size would fit here better).

    If we want to cache size (or compute it alongside hashing), that should be evaluated based on broader call sites and overall construction cost, and done in a separate PR with focused benchmarks and review. I will investigate that in a separate PR.

    Might be a shame to change the name to ComputeTotalSize if we later decide to return a cached size value.

    I would consider that a feature: we should rename it back when we’re not computing it anymore as it’s confusing currently. This PR is intentionally narrow: it avoids debug-only serialization/hashing work when cmpctblock debug logging, I would like to avoid doing unrelated refactors here. Alternatively I can revert that commit, but it was explicitly requested.


    Rebased in separate empty push and and pushed 2 suggestions in https://github.com/bitcoin/bitcoin/compare/4f1b29d32dea5fbcefe92df6dd0540807cfac1b2..2d3b557f1d414c509b992caa8b96f8ef3bb2aa46

  27. l0rinc force-pushed on Dec 22, 2025
  28. DrahtBot added the label CI failed on Dec 22, 2025
  29. DrahtBot removed the label CI failed on Dec 22, 2025
  30. l0rinc force-pushed on Dec 22, 2025
  31. DrahtBot added the label CI failed on Dec 23, 2025
  32. l0rinc commented at 8:03 am on December 23, 2025: contributor
    close/open dance to retrigger CI - likely caused by GitHub outage
  33. l0rinc closed this on Dec 23, 2025

  34. l0rinc reopened this on Dec 23, 2025

  35. DrahtBot removed the label CI failed on Dec 23, 2025
  36. l0rinc requested review from danielabrozzoni on Jan 14, 2026
  37. l0rinc requested review from hodlinator on Jan 14, 2026
  38. l0rinc requested review from maflcko on Jan 14, 2026
  39. l0rinc requested review from vasild on Jan 14, 2026
  40. in src/blockencodings.cpp:201 in 211149ae3a outdated
    199-    unsigned int tx_missing_size = 0;
    200     size_t tx_missing_offset = 0;
    201     for (size_t i = 0; i < txn_available.size(); i++) {
    202         if (!txn_available[i]) {
    203-            if (vtx_missing.size() <= tx_missing_offset)
    204+            if (tx_missing_offset >= vtx_missing.size()) {
    


    davidgumberg commented at 6:29 pm on January 16, 2026:
    nit: Why do you flip this condition?

    l0rinc commented at 7:47 pm on January 16, 2026:

    davidgumberg commented at 7:52 pm on January 16, 2026:
    haha reviewer ping-pong :) seems fine, feel free to mark resolved.
  41. davidgumberg commented at 6:30 pm on January 16, 2026: contributor
    code-review reACK or crreACK 211149ae3a67f7a87dd113723f622ef26eb7c095
  42. hodlinator approved
  43. hodlinator commented at 9:03 pm on January 16, 2026: contributor

    crACK 211149ae3a67f7a87dd113723f622ef26eb7c095

    The PR is a step forward, I’m just sad I couldn’t recruit anyone yet to my wild-goose chase.

    The first commit renaming CTransaction::GetTotalSize => CTransaction::ComputeTotalSize irks me as I’ve taken the time to at least partially prove that the current name on master would probably be fine given that we incorporate something like “Second attempt, using CSHA256::bytes” in #33738#pullrequestreview-3595056869.

    I can understand the willingness to leave that as a possible follow-up, and as it is not certain that such a follow-up would be merged, the rename is tolerable.

  44. in src/blockencodings.cpp:227 in 211149ae3a
    234-        for (const auto& tx : vtx_missing) {
    235-            LogDebug(BCLog::CMPCTBLOCK, "Reconstructed block %s required tx %s\n", hash.ToString(), tx->GetHash().ToString());
    236+    if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
    237+        const uint256 hash{block.GetHash()}; // avoid cleared header
    238+        uint32_t tx_missing_size{0};
    239+        for (const auto& tx : vtx_missing) tx_missing_size += tx->ComputeTotalSize(); // avoid cleared txn_available
    


    sedited commented at 1:11 pm on January 19, 2026:
    This comment doesn’t seem to make sense to me. What does txn_available have to do with these missing stats? I’m also not sure if the other comment adds much.

    l0rinc commented at 1:39 pm on January 19, 2026:
    we’re mutating those values so I couldn’t just move the same algorithm down here

    sedited commented at 2:01 pm on January 19, 2026:
    If it is functionally the same (as you say in the commit message), I still don’t see why these comments are useful for a later reader of this code.

    l0rinc commented at 2:10 pm on January 19, 2026:
    I rarely add comments, here it was to explain that while it seems that there’s an easier way to access those values, since they’re mutated in the meantime, it wouldn’t work a so I think it may be useful for later readers as well. If you insist, I can remove them here.

    sedited commented at 2:43 pm on January 19, 2026:

    I rarely add comments, here it was to explain that while it seems that there’s an easier way to access those values

    I see. I did not have that same impression when reading this. Both cases just seem correct and straight forward.


    l0rinc commented at 2:46 pm on January 19, 2026:
    is it a nit or do you think I should remove it?

    l0rinc commented at 3:05 pm on January 19, 2026:

    Removed the comments, the commit message explains it anyway:

    Since txn_available is invalidated after the first loop (needed for efficient moving), we compute tx_missing_size by iterating vtx_missing directly. This is safe because the later tx_missing_offset check guarantees vtx_missing was fully consumed during reconstruction. Use block.GetHash() instead of header.GetHash(), since header is cleared before logging.

  45. sedited commented at 1:20 pm on January 19, 2026: contributor

    I can understand the willingness to leave that as a possible follow-up, and as it is not certain that such a follow-up would be merged, the rename is tolerable.

    If such a change is landed, the rename can also easily be reverted again.

  46. DrahtBot requested review from sedited on Jan 19, 2026
  47. l0rinc force-pushed on Jan 19, 2026
  48. l0rinc requested review from hodlinator on Jan 19, 2026
  49. l0rinc requested review from davidgumberg on Jan 19, 2026
  50. hodlinator approved
  51. hodlinator commented at 3:25 pm on January 19, 2026: contributor

    re-ACK d1a19ba63467dcd0c8b54957d9c77641f69e5fee

    Dropped some lower-impact comments.

  52. sedited approved
  53. sedited commented at 4:27 pm on January 19, 2026: contributor
    ACK d1a19ba63467dcd0c8b54957d9c77641f69e5fee
  54. DrahtBot added the label Needs rebase on Jan 19, 2026
  55. danielabrozzoni commented at 6:36 pm on January 19, 2026: member

    ACK d1a19ba63467dcd0c8b54957d9c77641f69e5fee

    I went through the code again since my last ACK. Everything looks good, but the PR needs a rebase, then I will ACK again.

    Changes since my last ACK:

    • In a9e236739d059de35fe046c76ca1a422eda6f2cb: we iterate over resp.txn instead of req.indexes when calculating the total size
    • In d1a19ba63467dcd0c8b54957d9c77641f69e5fee: drop comments
    • In d1a19ba63467dcd0c8b54957d9c77641f69e5fee: flip the if condition, see #33738 (review)
  56. refactor: rename `CTransaction::GetTotalSize` to signal that it's not cached
    Transaction hashes are cached, it may not be intuitive that their sizes are actually recalculated every time.
    This is done before the other refactors to clarify why we want to avoid calling this method;
    
    Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
    1658b8f82b
  57. log,net: avoid `ComputeTotalSize` when logging is disabled
    `PeerManagerImpl::SendBlockTransactions()` computed the total byte size of requested transactions for a debug log line by calling `ComputeTotalSize()` in a tight loop, triggering serialization even when debug logging is off.
    
    Guard the size accumulation with `LogAcceptCategory` so the serialization work only happens when the log line can be emitted.
    
    No behavior change when debug logging is enabled: the reported block hash, transaction count, and byte totals are the same.
    The bounds checks still run unconditionally; the debug-only loop iterates the already-validated response contents.
    
    Separating debug-only work from the critical path reduces risk and favors the performance-critical non-debug case.
    This also narrows the racy scope of when logging is toggled from another thread.
    babfda332b
  58. log,blocks: avoid `ComputeTotalSize` and `GetHash` work when logging is disabled
    `PartiallyDownloadedBlock::FillBlock()` computed the block header hash and summed missing transaction sizes for debug logging unconditionally, including when cmpctblock debug logging is disabled.
    
    Guard the debug-only hash and size computations with `LogAcceptCategory`.
    Since `txn_available` is invalidated after the first loop (needed for efficient moving), we compute `tx_missing_size` by iterating `vtx_missing` directly. This is safe because the later `tx_missing_offset` check guarantees `vtx_missing` was fully consumed during reconstruction.
    
    Use `block.GetHash()` instead of `header.GetHash()`, since header is cleared before logging.
    
    No behavior change when debug logging is enabled: the reported counts, hashes, and byte totals remain the same.
    969c840db5
  59. l0rinc force-pushed on Jan 19, 2026
  60. l0rinc commented at 7:29 pm on January 19, 2026: contributor
    Trivial rebase, include-only conflict.
  61. DrahtBot removed the label Needs rebase on Jan 19, 2026
  62. hodlinator approved
  63. hodlinator commented at 8:25 am on January 20, 2026: contributor

    re-ACK 969c840db52da796c319f84c9a9a20b1de902ccf

    Just a rebase to handle code moving from core_write.cpp to core_io.cpp.

  64. DrahtBot requested review from sedited on Jan 20, 2026
  65. DrahtBot requested review from danielabrozzoni on Jan 20, 2026

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-01-20 09:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me