refactor: disable default std::hash for CTransactionRef #35101

Sjors commented at 7:48 PM on April 17, 2026: member

While working on #33922 I initially forgot to add CTransactionRefComp to the std::unordered_map<CTransactionRef defined there. This PR turns that into a compiler error. See #33922 (review)

This change triggers a false positive IWYU error, and an inconsistent one at that: our CI wants <variant>, while a manual build on Ubuntu (version 0.26 with clang version 22.1.1) wants <string_view>.

Various workarounds were discussed in:

Addressed by back-porting:

https://github.com/include-what-you-use/include-what-you-use/pull/2013

DrahtBot added the label Refactoring on Apr 17, 2026

DrahtBot commented at 7:48 PM on April 17, 2026: 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/35101.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, w0xlt, achow101
Approach ACK	hebasto

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

in src/primitives/transaction.h:426 in 276da6236a outdated

 421 | + *  comparing by pointer. Use CTransactionRefSaltedHash or provide a custom
 422 | + *  hasher. */
 423 | +template <>
 424 | +struct hash<CTransactionRef> {
 425 | +    size_t operator()(const CTransactionRef&) const = delete;
 426 | +};

hebasto commented at 5:54 PM on April 18, 2026:

e5d04fb46a68bd884d8eefae1014aaa3dd79fa6a:

This approach doesn't handle scenarios where a container is declared, but no member functions involving hashing are explicitly called. For example, this compiles.

I suggest a more defensive approach relying on the conditions that must be satisfied for the std::hash<CTransactionRef> specialization to be enabled:

template <>
struct hash<CTransactionRef> {
    hash() = delete;
};

Here are two examples on https://godbolt.org/:

A container with the default hasher: fails.
A container with the custom hasher: passes.

Sjors commented at 10:21 AM on April 19, 2026:

Thanks, marking draft while I study this suggestion.

Sjors commented at 8:35 AM on April 24, 2026:

I added hash() = delete;, but also kept the earlier variant for clarity.

in src/primitives/transaction.h:419 in 276da6236a

 414 | + * workaround.
 415 | + */
 416 | +#ifndef _LIBCPP_VERSION
 417 | +template <class T>
 418 | +struct hash; // IWYU pragma: keep
 419 | +#endif

hebasto commented at 6:00 PM on April 18, 2026:

276da6236af12bb7aee12f74f8138f2c9bb16176:

This changes follows this suggestion, excluding libc++, which is not used in the IWYU CI job.

Looks fine to me.

hebasto commented at 8:34 AM on April 20, 2026:

I can confirm that the bug has been fixed upstream in https://github.com/include-what-you-use/include-what-you-use/pull/2013.

Sjors marked this as a draft on Apr 19, 2026

Sjors commented at 10:55 AM on April 20, 2026: member

Since this isn't urgent, I think I'll wait for #35101 (review) to land in a release.

Sjors force-pushed on Apr 24, 2026

Sjors commented at 8:19 AM on April 24, 2026: member

I took the IWYU patch to see if it actually does the job (I still plan to wait for it to be backported to the clang_22 branch).

Sjors force-pushed on Apr 24, 2026

DrahtBot added the label CI failed on Apr 24, 2026

Sjors commented at 8:35 AM on April 24, 2026: member

The IWYU patch works. I also implemented @hebasto's suggestion: #35101 (review)

Sjors force-pushed on Apr 24, 2026

DrahtBot removed the label CI failed on Apr 24, 2026

Sjors commented at 5:02 PM on April 25, 2026: member

Apparently the patch won't be backported: https://github.com/include-what-you-use/include-what-you-use/pull/2013#issuecomment-4313034887

So I'll just keep it here.

Sjors force-pushed on Apr 25, 2026

Sjors marked this as ready for review on Apr 25, 2026

hebasto commented at 6:23 AM on April 27, 2026: member

Apparently the patch won't be backported: include-what-you-use/include-what-you-use#2013 (comment)

So I'll just keep it here.

FWIW, the IWYU maintainer wrote:

But if actually backported, it should be done along with the two preceding commits.

hebasto commented at 6:24 AM on April 27, 2026: member

Approach ACK 16a6b05441f0a195b3df89c8b570f57230ad0f1f.

Sjors commented at 8:28 AM on April 27, 2026: member

Previous two commits, I don't think we need them here:

https://github.com/include-what-you-use/include-what-you-use/commit/28d70e6a776c735fec8fe5ad630657533000591b: about false positive includes of <print>, which afaik we haven't encountered, and is unrelated to this change anyway.
https://github.com/include-what-you-use/include-what-you-use/commit/d1e42bf8bebae7269ae83e999b49f11144afeeda: related to <iosfwd>, so also seems unrelated to this PR

in ci/test/01_iwyu.patch:635 in 16a6b05441

 631 | @@ -632,3 +632,48 @@ See: https://github.com/include-what-you-use/include-what-you-use/blob/clang_21/
 632 |   };
 633 |   
 634 |   const char* stdlib_cpp_public_headers[] = {
 635 | +

vasild commented at 9:15 AM on April 27, 2026:

Offtopic, so we maintain patches for IWYU into a single file. Multiple patches appended into a single file. I think that is prone to conflicts when adding / removing stuff from that file and also a maintenance headache when two separate patches that are to be added in 01_iwyu.patch touch the same file.

In this case the newly patched std_symbol_map.inc was not present before in 01_iwyu.patch. It would be better to keep each patchset into a separate file. So, for example in this PR a new file like 52f85e1f4d99.diff is added.

Sjors commented at 12:26 PM on April 27, 2026:

I moved the new patch to its own file.

in ci/test/01_iwyu.patch:639 in 16a6b05441

 631 | @@ -632,3 +632,48 @@ See: https://github.com/include-what-you-use/include-what-you-use/blob/clang_21/
 632 |   };
 633 |   
 634 |   const char* stdlib_cpp_public_headers[] = {
 635 | +
 636 | +
 637 | +Map std::hash to its providing standard headers.
 638 | +Backport of https://github.com/include-what-you-use/include-what-you-use/pull/2013
 639 | +(commit 52f85e1f4d990f55fc6556d543eb051d79364a16) to the clang_22 release

vasild commented at 9:18 AM on April 27, 2026:

https://github.com/include-what-you-use/include-what-you-use/commit/52f85e1f4d990f55fc6556d543eb051d79364a16 contains changes also to mapgen/iwyu-mapgen-std-symbol.py which are omitted here. I think it is fine, just mentioning.

in src/primitives/transaction.h:408 in 16a6b05441

 402 | @@ -403,4 +403,16 @@ struct CMutableTransaction
 403 |  typedef std::shared_ptr<const CTransaction> CTransactionRef;
 404 |  template <typename Tx> static inline CTransactionRef MakeTransactionRef(Tx&& txIn) { return std::make_shared<const CTransaction>(std::forward<Tx>(txIn)); }
 405 |  
 406 | +namespace std {
 407 | +/** Disable default std::hash for CTransactionRef to prevent accidentally
 408 | + *  comparing by pointer. Use CTransactionRefSaltedHash or provide a custom

vasild commented at 9:20 AM on April 27, 2026:

There is no CTransactionRefSaltedHash.

Sjors commented at 10:38 AM on April 27, 2026:

Indeed, that's introduced in #33922.

Sjors commented at 12:27 PM on April 27, 2026:

(switched to CTransactionRefHash)

vasild approved

vasild commented at 9:25 AM on April 27, 2026: contributor

ACK 16a6b05441f0a195b3df89c8b570f57230ad0f1f (modulo the reference to the nonexistent CTransactionRefSaltedHash)

<details> <summary>Show Signature</summary>

vasild's public key is on openpgp.org

</details>

DrahtBot requested review from hebasto on Apr 27, 2026

ci: backport iwyu PR 2013 std::hash mapping

Cherry-pick include-what-you-use commit 52f85e1f4d99 onto the clang_22
branch tracked by ci/test/00_setup_env_native_iwyu.sh, fixing the false
positive where std::hash was mapped to <string_view>/<variant> instead of
its real provider (<functional>, <memory>, etc).

47d68cd981

refactor: disable default std::hash for CTransactionRef

The default std::hash for shared_ptr compares by pointer.

CTransactionRefHash or a custom hasher should be used instead.

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

a9301cfa07

Sjors force-pushed on Apr 27, 2026

Sjors commented at 12:31 PM on April 27, 2026: member

fixed comment to refer to CTransactionRefHash instead of CTransactionRefSaltedHash, see #35101 (review)
moved patch to its own file, see #35101 (review)
updated PR description to mention the patch

vasild approved

vasild commented at 1:47 PM on April 29, 2026: contributor

ACK a9301cfa0730d1d08c9e982f990dfc8aa0b3a27a

<details> <summary>Show Signature</summary>

vasild's public key is on openpgp.org

</details>

w0xlt commented at 6:28 PM on April 30, 2026: contributor

Concept ACK

sedited commented at 9:48 AM on May 26, 2026: contributor

Concept~0

Not sure if this is worth it. Seems like a bug that can be caught to me.

Sjors commented at 11:07 AM on May 26, 2026: member

@sedited #33922 had multiple ACKs with that bug in it though.

sedited commented at 4:49 PM on May 26, 2026: contributor

@sedited #33922 had multiple ACKs with that bug in it though.

Ah, I misunderstood then. My impression was you ran into this before submitting the PR.

sedited requested review from ismaelsadeeq on May 26, 2026

Sjors commented at 5:43 PM on May 26, 2026: member

It was discovered here: #33922 (review)

sedited requested review from maflcko on Jun 2, 2026

maflcko commented at 12:46 PM on June 2, 2026: member

It was discovered here: #33922 (comment)

Instead of hiding the motivation for a pull request 27 comments deep into a thread, it would be better to put the motivation in the pull request description. Currently the pull description is a random collection of links to upstream iwyu threads, which seem irrelevant implementation details outside of the underlying motivation.

Sjors commented at 8:23 AM on June 5, 2026: member

@maflcko the motivation is the very first sentence of the PR description:

While working on #33922 I initially forgot to add CTransactionRefComp to the std::unordered_map<CTransactionRef defined there.

I added a link to the specific comment.

maflcko commented at 9:22 PM on June 9, 2026: member

Yes, I've seen that, but it doesn't say the underlying risk/downside: Comparing pointers does not compare the objects itself. Maybe that is obvious, but the code also mentions it, so I think it can't hurt to put the words in the description. (GitHub has already broken (direct) links to some review comment for some reason, so purely relying on links here doesn't seem ideal)

But just a nit, up to you.

w0xlt commented at 11:38 PM on June 9, 2026: contributor

lgtm ACK a9301cfa0730d1d08c9e982f990dfc8aa0b3a27a

achow101 commented at 5:38 PM on June 10, 2026: member

ACK a9301cfa0730d1d08c9e982f990dfc8aa0b3a27a

achow101 merged this on Jun 10, 2026

achow101 closed this on Jun 10, 2026