From 1658b8f82b99f9ba3b42a50ba1f72b19a38c8e75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 31 Oct 2025 14:51:04 +0100 Subject: [PATCH 1/3] 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> --- src/bench/duplicate_inputs.cpp | 2 +- src/blockencodings.cpp | 2 +- src/core_io.cpp | 2 +- src/net_processing.cpp | 2 +- src/primitives/transaction.cpp | 2 +- src/primitives/transaction.h | 4 ++-- src/qt/transactiondesc.cpp | 2 +- src/rpc/blockchain.cpp | 2 +- src/rpc/mempool.cpp | 2 +- src/test/fuzz/transaction.cpp | 4 ++-- 10 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/bench/duplicate_inputs.cpp b/src/bench/duplicate_inputs.cpp index b59d14af892..5b1ca5f552f 100644 --- a/src/bench/duplicate_inputs.cpp +++ b/src/bench/duplicate_inputs.cpp @@ -58,7 +58,7 @@ static void DuplicateInputs(benchmark::Bench& bench) naughtyTx.vout[0].nValue = 0; naughtyTx.vout[0].scriptPubKey = SCRIPT_PUB; - uint64_t n_inputs = (((MAX_BLOCK_SERIALIZED_SIZE / WITNESS_SCALE_FACTOR) - (CTransaction(coinbaseTx).GetTotalSize() + CTransaction(naughtyTx).GetTotalSize())) / 41) - 100; + uint64_t n_inputs = (((MAX_BLOCK_SERIALIZED_SIZE / WITNESS_SCALE_FACTOR) - (CTransaction(coinbaseTx).ComputeTotalSize() + CTransaction(naughtyTx).ComputeTotalSize())) / 41) - 100; for (uint64_t x = 0; x < (n_inputs - 1); ++x) { naughtyTx.vin.emplace_back(Txid::FromUint256(GetRandHash()), 0, CScript(), 0); } diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index 49900482f98..eebf7bf466a 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -203,7 +203,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< if (vtx_missing.size() <= tx_missing_offset) return READ_STATUS_INVALID; block.vtx[i] = vtx_missing[tx_missing_offset++]; - tx_missing_size += block.vtx[i]->GetTotalSize(); + tx_missing_size += block.vtx[i]->ComputeTotalSize(); } else block.vtx[i] = std::move(txn_available[i]); } diff --git a/src/core_io.cpp b/src/core_io.cpp index a789d5ca947..1171b4c9594 100644 --- a/src/core_io.cpp +++ b/src/core_io.cpp @@ -432,7 +432,7 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry entry.pushKV("txid", tx.GetHash().GetHex()); entry.pushKV("hash", tx.GetWitnessHash().GetHex()); entry.pushKV("version", tx.version); - entry.pushKV("size", tx.GetTotalSize()); + entry.pushKV("size", tx.ComputeTotalSize()); entry.pushKV("vsize", (GetTransactionWeight(tx) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR); entry.pushKV("weight", GetTransactionWeight(tx)); entry.pushKV("locktime", (int64_t)tx.nLockTime); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3fff1db522b..89f93781831 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2577,7 +2577,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo return; } resp.txn[i] = block.vtx[req.indexes[i]]; - tx_requested_size += resp.txn[i]->GetTotalSize(); + tx_requested_size += resp.txn[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); diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index c8bbd9f821e..fc8a70bf01a 100644 --- a/src/primitives/transaction.cpp +++ b/src/primitives/transaction.cpp @@ -107,7 +107,7 @@ CAmount CTransaction::GetValueOut() const return nValueOut; } -unsigned int CTransaction::GetTotalSize() const +unsigned int CTransaction::ComputeTotalSize() const { return ::GetSerializeSize(TX_WITH_WITNESS(*this)); } diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index 2ccdb4e44bc..7d4c6bf5b9a 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -332,11 +332,11 @@ public: CAmount GetValueOut() const; /** - * Get the total transaction size in bytes, including witness data. + * Calculate the total transaction size in bytes, including witness data. * "Total Size" defined in BIP141 and BIP144. * @return Total transaction size in bytes */ - unsigned int GetTotalSize() const; + unsigned int ComputeTotalSize() const; bool IsCoinBase() const { diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp index 918d0af95d1..d9c97d85297 100644 --- a/src/qt/transactiondesc.cpp +++ b/src/qt/transactiondesc.cpp @@ -279,7 +279,7 @@ QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wall strHTML += "
" + tr("Comment") + ":
" + GUIUtil::HtmlEscape(wtx.value_map["comment"], true) + "
"; strHTML += "" + tr("Transaction ID") + ": " + rec->getTxHash() + "
"; - strHTML += "" + tr("Transaction total size") + ": " + QString::number(wtx.tx->GetTotalSize()) + " bytes
"; + strHTML += "" + tr("Transaction total size") + ": " + QString::number(wtx.tx->ComputeTotalSize()) + " bytes
"; strHTML += "" + tr("Transaction virtual size") + ": " + QString::number(GetVirtualTransactionSize(*wtx.tx)) + " bytes
"; strHTML += "" + tr("Output index") + ": " + QString::number(rec->getOutputIndex()) + "
"; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 763de836890..7de01ac641a 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2053,7 +2053,7 @@ static RPCHelpMan getblockstats() int64_t tx_size = 0; if (do_calculate_size) { - tx_size = tx->GetTotalSize(); + tx_size = tx->ComputeTotalSize(); if (do_mediantxsize) { txsize_array.push_back(tx_size); } diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 363e5e382e9..7657910512a 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -1031,7 +1031,7 @@ static UniValue OrphanToJSON(const node::TxOrphanage::OrphanInfo& orphan) UniValue o(UniValue::VOBJ); o.pushKV("txid", orphan.tx->GetHash().ToString()); o.pushKV("wtxid", orphan.tx->GetWitnessHash().ToString()); - o.pushKV("bytes", orphan.tx->GetTotalSize()); + o.pushKV("bytes", orphan.tx->ComputeTotalSize()); o.pushKV("vsize", GetVirtualTransactionSize(*orphan.tx)); o.pushKV("weight", GetTransactionWeight(*orphan.tx)); UniValue from(UniValue::VARR); diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp index b96fb71b1f4..da915713f7e 100644 --- a/src/test/fuzz/transaction.cpp +++ b/src/test/fuzz/transaction.cpp @@ -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); From babfda332b6aa41143eb694193358ef2c76ebefe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 31 Oct 2025 15:41:38 +0100 Subject: [PATCH 2/3] 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. --- src/net_processing.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 89f93781831..f4c4ae284a1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2570,17 +2570,19 @@ uint32_t PeerManagerImpl::GetFetchFlags(const Peer& peer) const void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlock& block, const BlockTransactionsRequest& req) { BlockTransactions resp(req); - unsigned int tx_requested_size = 0; for (size_t i = 0; i < req.indexes.size(); i++) { if (req.indexes[i] >= block.vtx.size()) { Misbehaving(peer, "getblocktxn with out-of-bounds tx indices"); return; } resp.txn[i] = block.vtx[req.indexes[i]]; - tx_requested_size += resp.txn[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); + if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) { + uint32_t tx_requested_size{0}; + for (const auto& tx : resp.txn) tx_requested_size += tx->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); + } MakeAndPushMessage(pfrom, NetMsgType::BLOCKTXN, resp); } From 969c840db52da796c319f84c9a9a20b1de902ccf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 31 Oct 2025 15:41:35 +0100 Subject: [PATCH 3/3] 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. --- src/blockencodings.cpp | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index eebf7bf466a..fd528309902 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -192,40 +192,44 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< { if (header.IsNull()) return READ_STATUS_INVALID; - uint256 hash = header.GetHash(); block = header; block.vtx.resize(txn_available.size()); - unsigned int tx_missing_size = 0; size_t tx_missing_offset = 0; for (size_t i = 0; i < txn_available.size(); i++) { if (!txn_available[i]) { - if (vtx_missing.size() <= tx_missing_offset) + if (tx_missing_offset >= vtx_missing.size()) { return READ_STATUS_INVALID; + } block.vtx[i] = vtx_missing[tx_missing_offset++]; - tx_missing_size += block.vtx[i]->ComputeTotalSize(); - } else + } else { block.vtx[i] = std::move(txn_available[i]); + } } // Make sure we can't call FillBlock again. header.SetNull(); txn_available.clear(); - if (vtx_missing.size() != tx_missing_offset) + if (vtx_missing.size() != tx_missing_offset) { return READ_STATUS_INVALID; + } // Check for possible mutations early now that we have a seemingly good block IsBlockMutatedFn check_mutated{m_check_block_mutated_mock ? m_check_block_mutated_mock : IsBlockMutated}; - if (check_mutated(/*block=*/block, - /*check_witness_root=*/segwit_active)) { + if (check_mutated(/*block=*/block, /*check_witness_root=*/segwit_active)) { return READ_STATUS_FAILED; // Possible Short ID collision } - 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); - if (vtx_missing.size() < 5) { - for (const auto& tx : vtx_missing) { - LogDebug(BCLog::CMPCTBLOCK, "Reconstructed block %s required tx %s\n", hash.ToString(), tx->GetHash().ToString()); + if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) { + const uint256 hash{block.GetHash()}; + uint32_t tx_missing_size{0}; + for (const auto& tx : vtx_missing) tx_missing_size += tx->ComputeTotalSize(); + 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); + if (vtx_missing.size() < 5) { + for (const auto& tx : vtx_missing) { + LogDebug(BCLog::CMPCTBLOCK, "Reconstructed block %s required tx %s\n", hash.ToString(), tx->GetHash().ToString()); + } } }