fa6801366d76a34f51a7d60be7d3710ed8e722db refactor: [rpc] Remove confusing and brittle integral casts (take 2) (MarcoFalke)
Pull request description:
Second take for cases which I seem to have missed in commit 94ddc2dced5736612e358a3b80f2bc718fbd8161.
When constructing an UniValue from integral values, historically (long ago), in some cases casts where needed. With the current UniValue constructor, only very few are actually needed.
Keeping the unused casts around is:
* confusing, because code readers do not understand why they are needed
* brittle, because some may copy them into new places, where they will lead to hard-to-find logic bugs, such as the ones fixed in pull https://github.com/bitcoin/bitcoin/pull/34112
So fix all issues by removing them, except for a few cases, where casting was required:
* `static_cast<bool>(fCoinBase)`, because `bool{fCoinBase}` only works on modern compilers.
This hardening refactor does not fix any bugs and does not change any behavior.
ACKs for top commit:
Sjors:
ACK fa6801366d76a34f51a7d60be7d3710ed8e722db
sedited:
ACK fa6801366d76a34f51a7d60be7d3710ed8e722db
Tree-SHA512: 77f03f496ea2a42987720cb4a36eb4e7a0d5b512ed7f029e41dd54c04fb4c1680f7cc13514acbc9f1bae991be4de3cf31c5a0d27c016a030f5749d132603f71e
This refactor does not change behavior. However, it avoids a vector
copy, which can lead to a minimal speed-up of 1%-5%, depending on the
call-site. This is mostly relevant for the fuzz tests and utils that
read large blobs of data (like a full block).
969c840db52da796c319f84c9a9a20b1de902ccf log,blocks: avoid `ComputeTotalSize` and `GetHash` work when logging is disabled (Lőrinc)
babfda332b6aa41143eb694193358ef2c76ebefe log,net: avoid `ComputeTotalSize` when logging is disabled (Lőrinc)
1658b8f82b99f9ba3b42a50ba1f72b19a38c8e75 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 969c840db52da796c319f84c9a9a20b1de902ccf
hodlinator:
re-ACK 969c840db52da796c319f84c9a9a20b1de902ccf
sedited:
Re-ACK 969c840db52da796c319f84c9a9a20b1de902ccf
danielabrozzoni:
reACK 969c840db52da796c319f84c9a9a20b1de902ccf
Tree-SHA512: 9780102d29778165144e3602d934ed4cb96660fd7b9ff2581b223c619e419139b8348e60f226af448702ae527736a1806d169b44342c5a82795590f664e16efe
d938947b3a89a61784a72c533df623f9eb2fec49 doc: Add "Using IWYU" to Developer Notes (Hennadii Stepanov)
e1a90bcecc823a4abaa2a57f393cad675a2ccbc0 iwyu: Do not export `crypto/hex_base.h` header (Hennadii Stepanov)
19a2edde50c38412712306bf527faad6cbf81c2c iwyu: Do not export C++ headers in most cases (Hennadii Stepanov)
Pull request description:
First two commits address comments from discussion in https://github.com/bitcoin/bitcoin/pull/33779:
- https://github.com/bitcoin/bitcoin/pull/33779#discussion_r2697579248
- https://github.com/bitcoin/bitcoin/pull/33779#discussion_r2697707343
The last commit adds a new section to the Developer Notes to document IWYU usage.
ACKs for top commit:
maflcko:
re-ACK d938947b3a89a61784a72c533df623f9eb2fec49 🚀
Tree-SHA512: 8d6c63e9d2fd190815211d80e654cb7379d16b6611b8851444f49bbbaa0fbc93557675fbcc558afd9a1cdf643570fba5eff9c1aecb5530f978797387b10b9a11
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>
