highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-06-04 10:12:28 +02:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
969c840db5 log,blocks: avoid `ComputeTotalSize` and `GetHash` work when logging is disabled (Lőrinc)
babfda332b log,net: avoid `ComputeTotalSize` when logging is disabled (Lőrinc)
1658b8f82b refactor: rename `CTransaction::GetTotalSize` to signal that it's not cached (Lőrinc)

Pull request description:

  ### Context

  The new accounting options introduced in https://github.com/bitcoin/bitcoin/pull/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

  <details>
  <summary>Test patch</summary>

  ```patch
  diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
  index 58620c93cc..f16eb38fa5 100644
  --- a/src/blockencodings.cpp
  +++ b/src/blockencodings.cpp
  @@ -186,6 +186,7 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const

   ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active)
   {
  +    LogInfo("PartiallyDownloadedBlock::FillBlock called");
       if (header.IsNull()) return READ_STATUS_INVALID;

       block = header;
  @@ -218,6 +219,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
       }

       if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
  +        LogInfo("debug log enabled");
           const uint256 hash{block.GetHash()}; // avoid cleared header
           uint32_t tx_missing_size{0};
           for (const auto& tx : vtx_missing) tx_missing_size += tx->ComputeTotalSize(); // avoid cleared txn_available
  diff --git a/src/net_processing.cpp b/src/net_processing.cpp
  index 5600c8d389..c081825f77 100644
  --- a/src/net_processing.cpp
  +++ b/src/net_processing.cpp
  @@ -2470,6 +2470,7 @@ uint32_t PeerManagerImpl::GetFetchFlags(const Peer& peer) const

   void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlock& block, const BlockTransactionsRequest& req)
   {
  +    LogInfo("PeerManagerImpl::SendBlockTransactions called");
       BlockTransactions resp(req);
       for (size_t i = 0; i < req.indexes.size(); i++) {
           if (req.indexes[i] >= block.vtx.size()) {
  @@ -2480,6 +2481,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
       }

       if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
  +        LogInfo("debug log enabled");
           uint32_t tx_requested_size{0};
           for (const auto i : req.indexes) tx_requested_size += block.vtx[i]->ComputeTotalSize();
           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);
  ```

  </details>

ACKs for top commit:
  davidgumberg:
    reACK 969c840db5
  achow101:
    ACK 969c840db5
  hodlinator:
    re-ACK 969c840db5
  sedited:
    Re-ACK 969c840db5
  danielabrozzoni:
    reACK 969c840db5

Tree-SHA512: 9780102d29778165144e3602d934ed4cb96660fd7b9ff2581b223c619e419139b8348e60f226af448702ae527736a1806d169b44342c5a82795590f664e16efe
This commit is contained in:
Ava Chow
2026-01-20 15:41:30 -08:00
parent bc3c4cd8b2 969c840db5
commit a365c9fe1f
10 changed files with 31 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
src/test/fuzz/transaction.cpp
View File

@@ -68,7 +68,7 @@ FUZZ_TARGET(transaction, .init = initialize_transaction)
}
(void)tx.GetHash();
(void)tx.GetTotalSize();
(void)tx.ComputeTotalSize();
try {
(void)tx.GetValueOut();
} catch (const std::runtime_error&) {
@@ -92,7 +92,7 @@ FUZZ_TARGET(transaction, .init = initialize_transaction)
(void)AreInputsStandard(tx, coins_view_cache);
(void)IsWitnessStandard(tx, coins_view_cache);
if (tx.GetTotalSize() < 250'000) { // Avoid high memory usage (with msan) due to json encoding
if (tx.ComputeTotalSize() < 250'000) { // Avoid high memory usage (with msan) due to json encoding
{
UniValue u{UniValue::VOBJ};
TxToUniv(tx, /*block_hash=*/uint256::ZERO, /*entry=*/u);