coinstats: avoid unnecessary Coin copy in ApplyHash #33410

pull sashass1315 wants to merge 0 commits into bitcoin:master from sashass1315:master changing 0 files +0 −0

sashass1315 commented at 8:47 AM on September 17, 2025: none

Replace local Coin coin = it->second; with const Coin& coin = it->second; when iterating outputs in ApplyHash. The hash serialization path only reads from the Coin, and outputs remains valid for the loop’s duration, so copying is unnecessary. This reduces potential allocations (due to CTxOut/CScript) and improves performance without changing behavior.
DrahtBot commented at 8:47 AM on September 17, 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/33410.



Reviews

See the guideline for information on the review process.

Type Reviewers

Concept ACK Raimo33

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.



LLM Linter (✨ experimental)

Possible places where named args may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

I don't see the git diff content in your message. Please paste the diff (or attach it), and I will scan the added lines and report any places to suggest naming positional integral literal arguments per your rules.

<sup>2025-11-26</sup>

Type	Reviewers
Concept ACK	Raimo33

in src/kernel/coinstats.cpp:96 in 5a56203f4e

  92 | @@ -93,7 +93,7 @@ static void ApplyHash(T& hash_obj, const Txid& hash, const std::map<uint32_t, Co
  93 |  {
  94 |      for (auto it = outputs.begin(); it != outputs.end(); ++it) {
  95 |          COutPoint outpoint = COutPoint(hash, it->first);
  96 | -        Coin coin = it->second;

l0rinc commented at 4:18 PM on September 17, 2025:

given the above warning, can you create a godbolt reproducer to check if the compiler can already deduce this or not?

Raimo33 commented at 5:33 PM on September 17, 2025: contributor

Concept ACK, I want to expand on l0rinc suggestion by saying that, given the above warning, I think this should only be merged if it results in a significant performance improvement. @sashass1315 are you able to run some before/after benchmarks?
luke-jr referenced this in commit 6712f43fdb on Sep 19, 2025
sashass1315 closed this on Nov 26, 2025
sashass1315 force-pushed on Nov 26, 2025

Contributors

coinstats: avoid unnecessary Coin copy in ApplyHash #33410

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)