refactor: Default uint256::operator==, add operator<=> #35896

pull maflcko wants to merge 4 commits into bitcoin:master from maflcko:2608-uint256-compare changing 5 files +99 −24
  1. maflcko commented at 11:58 AM on August 5, 2026: member

    Some refactors with rationale:

    • Default the uint256 base blob equals operator, because this is standard C++20 practise.
    • Add the uint256 base blob <=> operator, because this is standard C++20 practise. Also, transaction_identifier already offers such an operator. This allows to remove the non-standard Compare() function.
    • Add a [[noreturn]] to the assertion failure helper that does not return. This is standard C++11 practise.
  2. refactor: uint256::operator==() = default
    This is standard C++20, and may allow a compiler to optimize a bit more.
    fa6df14c23
  3. DrahtBot added the label Refactoring on Aug 5, 2026
  4. DrahtBot commented at 11:58 AM on August 5, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35896.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    ACK optout21, Sjors, purpleKarrot, hebasto, w0xlt

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  5. maflcko force-pushed on Aug 5, 2026
  6. DrahtBot added the label CI failed on Aug 5, 2026
  7. DrahtBot commented at 12:27 PM on August 5, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/31003613845/job/92298123116</sub> <sub>LLM reason (✨ experimental): CI failed because IWYU (include-what-you-use) detected/include dependencies issues and generated a required change (printed “Failure generated from IWYU”), causing the test script to exit with code 1.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  8. DrahtBot removed the label CI failed on Aug 5, 2026
  9. in src/uint256.h:71 in fa6423a501 outdated 
      71 | -        if (cmp > 0) return 1;
  72 | -        return 0;
  73 | +    constexpr std::strong_ordering operator<=>(const base_blob& other) const
  74 | +    {
  75 | +        return std::lexicographical_compare_three_way(m_data.begin(), m_data.end(), other.m_data.begin(), other.m_data.end(),
  76 | +                                                      [](auto a, auto b) { return a <=> b; });
    purpleKarrot commented at 2:42 PM on August 5, 2026:

    Why no = default?

    maflcko commented at 3:11 PM on August 5, 2026:

    That would re-enable the GCC false-positive bug (https://github.com/bitcoin/bitcoin/pull/35501#issuecomment-5190566382)

    Happy to push whatever reviewers prefer.

    I wonder if we should give up on those GCC warnings, because i don't recall that they were ever right.

    l0rinc commented at 5:08 PM on August 5, 2026:

    This seems related to #33637 (doing similar migration of src/arith_uint256.h). Can you please cover these with tests and benchmarks before changing them to prove it's just a refactor?

    Sjors commented at 6:03 PM on August 5, 2026:

    I briefly ran the below (vibe coded) benchmark on macOS (M4) and Ubuntu x86_64. Looks like == and != got faster (avoid a full memcmp), but < and > got slower.

    Comparison Local AppleClang old → new x86_64 GCC old → new
    ==, first byte differs 0.495 → 0.586 1.933 → 0.492
    ==, identical 8.860 → 0.587 1.733 → 0.770
    ==, last byte differs 8.651 → 0.587 1.930 → 0.771
    <, first byte differs 0.504 → 0.494 1.928 → 0.394
    <, identical 8.842 → 8.439 1.732 → 7.502
    <, last byte differs 8.613 → 8.358 1.926 → 6.927

    <details><summary>src/bench/uint256.cpp</summary>

    diff --git a/src/bench/CMakeLists.txt b/src/bench/CMakeLists.txt
index 3c81e7986d..080a8ac848 100644
--- a/src/bench/CMakeLists.txt
+++ b/src/bench/CMakeLists.txt
@@ -53,6 +53,7 @@ add_executable(bench_bitcoin
   strencodings.cpp
   txgraph.cpp
   txorphanage.cpp
+  uint256.cpp
   util_time.cpp
   verify_script.cpp
 )
diff --git a/src/bench/uint256.cpp b/src/bench/uint256.cpp
new file mode 100644
--- /dev/null
+++ b/src/bench/uint256.cpp
@@ -0,0 +1,90 @@
+// Copyright (c) 2026-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://opensource.org/license/mit.
+
+#include <bench/bench.h>
+#include <random.h>
+#include <uint256.h>
+
+#include <cstddef>
+#include <utility>
+#include <vector>
+
+namespace {
+
+enum class Difference {
+    NONE,
+    FIRST_BYTE,
+    LAST_BYTE,
+};
+
+constexpr size_t NUM_PAIRS{4'096};
+
+std::vector<std::pair<uint256, uint256>> MakePairs(Difference difference)
+{
+    FastRandomContext rng{/*fDeterministic=*/true};
+    std::vector<std::pair<uint256, uint256>> pairs;
+    pairs.reserve(NUM_PAIRS);
+
+    for (size_t i{0}; i < NUM_PAIRS; ++i) {
+        uint256 lhs{rng.rand256()};
+        uint256 rhs{lhs};
+        if (difference != Difference::NONE) {
+            const size_t position{difference == Difference::FIRST_BYTE ? 0 : uint256::size() - 1};
+            lhs.begin()[position] = i % 2 == 0 ? 0 : 255;
+            rhs.begin()[position] = i % 2 == 0 ? 255 : 0;
+        }
+        pairs.emplace_back(lhs, rhs);
+    }
+    return pairs;
+}
+
+template <typename Comparator>
+void Comparison(benchmark::Bench& bench, Difference difference, Comparator comparator)
+{
+    const auto pairs{MakePairs(difference)};
+    bench.batch(pairs.size()).unit("comparison").run([&] {
+        for (const auto& [lhs, rhs] : pairs) {
+            ankerl::nanobench::doNotOptimizeAway(comparator(lhs, rhs));
+        }
+    });
+}
+
+void Uint256EqualIdentical(benchmark::Bench& bench)
+{
+    Comparison(bench, Difference::NONE, [](const uint256& lhs, const uint256& rhs) { return lhs == rhs; });
+}
+
+void Uint256EqualFirstByteDifferent(benchmark::Bench& bench)
+{
+    Comparison(bench, Difference::FIRST_BYTE, [](const uint256& lhs, const uint256& rhs) { return lhs == rhs; });
+}
+
+void Uint256EqualLastByteDifferent(benchmark::Bench& bench)
+{
+    Comparison(bench, Difference::LAST_BYTE, [](const uint256& lhs, const uint256& rhs) { return lhs == rhs; });
+}
+
+void Uint256LessIdentical(benchmark::Bench& bench)
+{
+    Comparison(bench, Difference::NONE, [](const uint256& lhs, const uint256& rhs) { return lhs < rhs; });
+}
+
+void Uint256LessFirstByteDifferent(benchmark::Bench& bench)
+{
+    Comparison(bench, Difference::FIRST_BYTE, [](const uint256& lhs, const uint256& rhs) { return lhs < rhs; });
+}
+
+void Uint256LessLastByteDifferent(benchmark::Bench& bench)
+{
+    Comparison(bench, Difference::LAST_BYTE, [](const uint256& lhs, const uint256& rhs) { return lhs < rhs; });
+}
+
+} // namespace
+
+BENCHMARK(Uint256EqualIdentical);
+BENCHMARK(Uint256EqualFirstByteDifferent);
+BENCHMARK(Uint256EqualLastByteDifferent);
+BENCHMARK(Uint256LessIdentical);
+BENCHMARK(Uint256LessFirstByteDifferent);
+BENCHMARK(Uint256LessLastByteDifferent);

    </details>

    Sjors commented at 6:28 PM on August 5, 2026:

    This fixes the regression, by using = default and suppressing the warning, but maybe yolo:

    #if defined(__GNUC__) && !defined(__clang__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-overread"
#endif
    constexpr auto operator<=>(const base_blob&) const = default;
#if defined(__GNUC__) && !defined(__clang__)
#pragma GCC diagnostic pop
#endif
    Sjors commented at 6:35 PM on August 5, 2026:

    See maybe:

    If we go for a suppression, maybe we should file a bug report for the specific case?

    Sjors commented at 7:35 PM on August 5, 2026:

    A hybrid approach could early return if the first element is unequal, and use ReadBE64 for the rest. That seems to be beat = default. Though I'm not sure if we want to compete with the compiler.

    maflcko commented at 7:48 PM on August 5, 2026:

    I guess any workaround requires GCC not to detect the optimization, in which case it will perform worse.

    I've switched to = default, which is the correct code.

    Anyone using GCC, can ignore the warning or fix the upstream bug about the false-positive warning.

    Sjors commented at 8:02 PM on August 5, 2026:

    I asked my agent to make a minimal example for a gcc bug report and a potential fix, but instead it figured out a bug on our side.

    We can just use = default plus:

    diff --git a/src/util/check.h b/src/util/check.h
--- a/src/util/check.h
+++ b/src/util/check.h
@@ -65,7 +65,7 @@ public:
 };
 
 /** Internal helper */
-void assertion_fail(const std::source_location& loc, std::string_view assertion);
+[[noreturn]] void assertion_fail(const std::source_location& loc, std::string_view assertion);
 
 /** Helper for CHECK_NONFATAL() */
 template <typename T>
    Sjors commented at 8:10 PM on August 5, 2026:

    Slop explainer: https://gist.github.com/Sjors/6b9a4258a9bd347017be884298fde794

    w0xlt commented at 10:40 PM on August 5, 2026:

    Good explanation.

    l0rinc commented at 10:44 PM on August 5, 2026:

    instead it figured out a bug on our side

    Yeah, I had a similar problem in https://github.com/bitcoin/bitcoin/pull/34132/changes#diff-44bef8fa93d168379ef331d9e11d333be06ce98a8f37ec65f8a987846b90a151R338

    maflcko commented at 7:07 AM on August 6, 2026:

    Ok, the noreturn finding is great.

    Minimal Godbolt repro to play with this: https://godbolt.org/z/ofcvMfjq7 (toggle the noreturn)

    The reason why I was confused by GCC, is because the warning is so brittle. E.g. switching from C++20 to C++14 makes the warning go away. (Other code transformations in the source code make the warning go away as well)

    I guess GCC can only find the issue, when the source:

    • does not rule out a nullptr for tx
    • The tx struct has the txid not in the first field
    • The source should allow the optimizer to see the memcmp on the tx.txid field, while tx could still plausibly be nullptr
    fanquake commented at 9:34 AM on August 6, 2026:

    See #35911, for if we wanted to detect/add missing [[noreturn]].

    maflcko commented at 10:26 AM on August 6, 2026:

    Just dropping some more fun-facts here:

    maflcko commented at 11:12 AM on August 6, 2026:

    Asked an LLM to run the vibe coded benchmark:

    <details><summary>LLM generated markdown table</summary>

    Clang-21

    Benchmark revert fa5cbb890 (Use std::memcmp) c940fd751 (merge-base) fa6df14c2 (== defaulted) faec059df (<=> added) fa588e9e0 (noreturn)
    Uint256EqualFirstByteDifferent 0.67 ns 0.69 ns 0.70 ns 0.68 ns 0.66 ns
    Uint256EqualIdentical 0.68 ns 0.69 ns 0.70 ns 0.69 ns 0.67 ns
    Uint256EqualLastByteDifferent 0.68 ns 0.69 ns 0.70 ns 0.69 ns 0.67 ns
    Uint256LessFirstByteDifferent 1.04 ns 1.07 ns 1.09 ns 1.06 ns 1.09 ns
    Uint256LessIdentical 1.33 ns 1.32 ns 1.35 ns 1.33 ns 1.26 ns
    Uint256LessLastByteDifferent 1.16 ns 1.17 ns 1.19 ns 1.16 ns 1.10 ns

    GCC-15

    Benchmark revert fa5cbb890 (Use std::memcmp) c940fd751 (merge-base) fa6df14c2 (== defaulted) faec059df (<=> added) fa588e9e0 (noreturn)
    Uint256EqualFirstByteDifferent 0.68 ns 1.30 ns 0.70 ns 0.69 ns 0.70 ns
    Uint256EqualIdentical 1.09 ns 1.55 ns 1.13 ns 1.11 ns 1.13 ns
    Uint256EqualLastByteDifferent 1.11 ns 1.29 ns 1.13 ns 1.12 ns 1.13 ns
    Uint256LessFirstByteDifferent 1.15 ns 1.15 ns 1.18 ns 1.39 ns 1.43 ns
    Uint256LessIdentical 1.32 ns 1.34 ns 1.37 ns 1.47 ns 1.52 ns
    Uint256LessLastByteDifferent 1.13 ns 1.19 ns 1.17 ns 1.38 ns 1.43 ns

    </details>

    So I guess the takeaway is:

    • Clang: Code is irrelevant and everything is optimized down to the same.
    • GCC: the merge-base was worse than the old std::memcmp implementation for equality comparisons.
    • GCC: default operator<=>() is worse for Less comparisons. It is up to the GCC compiler to optimize this further in the future, like Clang.
    Sjors commented at 11:47 AM on August 6, 2026:

    default operator<=>() is worse for Less comparisons.

    Only slightly worse though, not as dramatic as the earlier std::lexicographical_compare_three_way implementation.

  10. in src/primitives/transaction_identifier.h:37 in fa6423a501 
      41 |  
  42 |      template <typename Other>
  43 | -    bool operator==(const Other& other) const { return Compare(other) == 0; }
  44 | +    constexpr bool operator==(const Other& other) const
  45 | +    {
  46 | +        static_assert(std::is_same_v<Other, transaction_identifier<has_witness>>, "Forbidden comparison type");
    purpleKarrot commented at 2:44 PM on August 5, 2026:

    Why use a template and then assert the type to match? Why not define the function for this concrete type instead?

    purpleKarrot commented at 2:47 PM on August 5, 2026:

    Also, the template argument list is redundant in this context.

    The most aggressive simplification would be:

    friend bool operator==(transaction_identifier const&, transaction_identifier const&) = default;
    maflcko commented at 3:11 PM on August 5, 2026:

    thx, done

    maflcko commented at 3:25 PM on August 5, 2026:

    To clarify for other reviewers, this will change the compiler error message to stuff like:

    error: calling a private constructor of class 'transaction_identifier<true>'
   51 |     (void)(txid.ToUint256()==wtxid);

    or:

    error: invalid operands to binary expression ('Txid' (aka 'transaction_identifier<false>') and 'Wtxid' (aka 'transaction_identifier<true>'))
   52 |     (void)(txid<=>wtxid);
      |            ~~~~^  ~~~~~

    but anything is fine here, as long as compilation fails.

    Sjors commented at 5:37 PM on August 5, 2026:

    I got no known conversion from transaction_identifier<true> to transaction_identifier<false> when doing txid == wtxid;, but that's good enough.

  11. maflcko force-pushed on Aug 5, 2026
  12. hebasto approved
  13. hebasto commented at 4:22 PM on August 5, 2026: member

    ACK fa2f56e5c87aee5e041c162e96633f85901a1306, tested on Alpine Linux v3.24.1.

  14. in src/uint256.h:70 in fa2f56e5c8 
      70 | -        if (cmp < 0) return -1;
  71 | -        if (cmp > 0) return 1;
  72 | -        return 0;
  73 | +    constexpr std::strong_ordering operator<=>(const base_blob& other) const
  74 | +    {
  75 | +        return std::lexicographical_compare_three_way(m_data.begin(), m_data.end(), other.m_data.begin(), other.m_data.end(),
    Sjors commented at 5:24 PM on August 5, 2026:

    In fa2f56e5c87aee5e041c162e96633f85901a1306 refactor: Add uint256::operator<=>(): maybe add a comment that the reason this isn't = default is to avoid a false-positive GCC 14 -Wstringop-overread warning.

    See #35896 (review)

  15. Sjors commented at 5:38 PM on August 5, 2026: member

    ~ACK~ code review fa2f56e5c87aee5e041c162e96633f85901a1306

    Benchmark shows a performance regression: #35896 (review)

  16. refactor: Add uint256::operator<=>()
    There is already a non-standard and internally used Compare() function,
and an standard operator<().

Also, there is already transaction_identifier::operator<=>().

It seems more consistent to remove the internal Compare() and have a
single standard C++20 <=> operator.
    faec059dfe
  17. maflcko force-pushed on Aug 5, 2026
  18. purpleKarrot commented at 8:41 PM on August 5, 2026: contributor

    Nice, only 5 lines remaining! :-)

    I would ACK this, but there are some failing tests.

    As a potential follow-up, the boolean template parameter may be replaced with a tag, allowing more than two possible instantiations. This would allow using the same generic implementation for a strong type like BlockHash. Or even better, replace it with a policy that can control the size in bytes and how it can be constructed from a source.

    You may want to have a look at the spec at https://purplekarrot.github.io/std-bitcoin/VOCABULARY.html#bitcoin.hashid-class-template-basic-hash-id and the reference implementation at https://github.com/purpleKarrot/std-bitcoin/blob/master/module/bitcoin.hash_id.cpp for inspiration. This will also help #35904.

  19. DrahtBot added the label CI failed on Aug 5, 2026
  20. DrahtBot commented at 9:13 PM on August 5, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/31040921494/job/92424764517</sub> <sub>LLM reason (✨ experimental): CI failed due to a C++ build error: -Werror=stringop-overread in /usr/arm-linux-gnueabihf/include/c++/14/array while compiling psbt_wallet_tests.cpp.o.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  21. refactor: Mark assertion_fail as [[noreturn]]
    Found by Sjors in https://github.com/bitcoin/bitcoin/pull/35896#discussion_r3723634042

This should also fix GCC warnings such as https://github.com/bitcoin/bitcoin/pull/35896#issuecomment-5197419300
    fa588e9e0f
  22. maflcko commented at 7:15 AM on August 6, 2026: member

    Thanks for all the reviews:

    • @l0rinc about the benchmarks (which I had in mind but then forgot) and @Sjors for vibe coding some
    • @Sjors for challenging my claim that this is a GCC false-positive and pointing out the plausible UB
    • @purpleKarrot for pushing toward =default

    All review comments should be addressed now.

  23. DrahtBot removed the label CI failed on Aug 6, 2026
  24. in src/primitives/transaction_identifier.h:35 in faec059dfe outdated 
      42 | -    template <typename Other>
  43 | -    bool operator==(const Other& other) const { return Compare(other) == 0; }
  44 | -    template <typename Other>
  45 | -    std::strong_ordering operator<=>(const Other& other) const { return Compare(other) <=> 0; }
  46 | +    constexpr bool operator==(const transaction_identifier&) const = default;
  47 | +    constexpr auto operator<=>(const transaction_identifier&) const = default;
    purpleKarrot commented at 9:21 AM on August 6, 2026:

    Nitpick: When a function is = defaulted, the compiler can deduce whether it is constexpr.

    maflcko commented at 9:43 AM on August 6, 2026:

    In C++20 this still forces the compiler to check that constexpr works here. https://godbolt.org/z/escbK4vzP

    I guess you are referring to C++23 https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2448r2.html?

    I think I'll leave as-is for now, because either it doesn't matter, or it does matter, in which case it should be done for the whole codebase with clang-tidy, or with a git-grep linter. C.f.:

    $ git grep 'constexpr .* = default'
src/arith_uint256.h:    constexpr arith_uint256() = default;
src/hash.h:    constexpr ChainCode() = default;
src/minisketch/src/lintrans.h:    constexpr RecLinTrans() = default;
src/minisketch/src/lintrans.h:    constexpr RecLinTrans() = default;
src/random.h:    constexpr RandomMixin() noexcept = default;
src/script/verify_flags.h:    constexpr script_verify_flags(const script_verify_flags&) = default;
src/script/verify_flags.h:    constexpr script_verify_flags(script_verify_flags&&) = default;
src/script/verify_flags.h:    constexpr script_verify_flags& operator=(const script_verify_flags&) = default;
src/script/verify_flags.h:    constexpr script_verify_flags& operator=(script_verify_flags&&) = default;
src/script/verify_flags.h:    constexpr ~script_verify_flags() = default;
src/uint256.h:    constexpr uint160() = default;
src/uint256.h:    constexpr uint256() = default;
src/util/bitset.h:        constexpr IteratorEnd() = default;
src/util/bitset.h:        constexpr IteratorEnd(const IteratorEnd&) = default;
src/util/bitset.h:        constexpr Iterator(const Iterator&) noexcept = default;
src/util/bitset.h:        constexpr Iterator& operator=(const Iterator&) noexcept = default;
src/util/bitset.h:    constexpr IntBitSet(const IntBitSet&) noexcept = default;
src/util/bitset.h:    constexpr IntBitSet& operator=(const IntBitSet&) noexcept = default;
src/util/bitset.h:    friend constexpr bool operator==(const IntBitSet& a, const IntBitSet& b) noexcept = default;
src/util/bitset.h:        constexpr IteratorEnd() = default;
src/util/bitset.h:        constexpr IteratorEnd(const IteratorEnd&) = default;
src/util/bitset.h:        constexpr Iterator(const Iterator&) noexcept = default;
src/util/bitset.h:        constexpr Iterator& operator=(const Iterator&) noexcept = default;
src/util/bitset.h:    constexpr MultiIntBitSet(const MultiIntBitSet&) noexcept = default;
src/util/bitset.h:    constexpr MultiIntBitSet& operator=(const MultiIntBitSet&) noexcept = default;
src/util/bitset.h:    friend constexpr bool operator==(const MultiIntBitSet& a, const MultiIntBitSet& b) noexcept = default;
src/util/btcsignals.h:    constexpr connection() noexcept = default;
src/util/btcsignals.h:    constexpr signal() noexcept = default;
src/util/feefrac.h:    constexpr inline FeeFrac(const FeeFrac&) noexcept = default;
src/util/feefrac.h:    constexpr inline FeeFrac& operator=(const FeeFrac&) noexcept = default;
  25. in src/uint256.h:68 in faec059dfe outdated 
      71 | -        if (cmp > 0) return 1;
  72 | -        return 0;
  73 | -    }
  74 | -
  75 | -    friend constexpr bool operator<(const base_blob& a, const base_blob& b) { return a.Compare(b) < 0; }
  76 | +    constexpr std::strong_ordering operator<=>(const base_blob& other) const = default;
    purpleKarrot commented at 9:24 AM on August 6, 2026:

    Same here. Also, you could deduce the return type, like you do in transaction_identifier.h.

    maflcko commented at 9:43 AM on August 6, 2026:

    Yes, same here: Either this doesn't matter, or it does, in which case it should be enforced by CI for the whole codebase.

  26. purpleKarrot commented at 9:27 AM on August 6, 2026: contributor

    I see you added a commit to fix the build. Maybe reorder the commits so that each individual commit can be built? This is valuable for bisecting.

  27. maflcko commented at 9:43 AM on August 6, 2026: member

    bisecting

    bisectability is enforced by the CI task "test ancestor commits". The build warning here is just a warning and does not prevent a build. Also, the warning is pre-existing, see #35501 (comment)

    I think it is nice for current and future reviewers to be able to see the warning and then see that it really does go away, but no strong opinion.

  28. purpleKarrot commented at 10:12 AM on August 6, 2026: contributor

    ACK fa588e9e0f8019d855dbc41199814564c27d5256

  29. DrahtBot requested review from Sjors on Aug 6, 2026
  30. DrahtBot requested review from hebasto on Aug 6, 2026
  31. Sjors commented at 11:52 AM on August 6, 2026: member

    ACK fa588e9e0f8019d855dbc41199814564c27d5256

    • @l0rinc about the benchmarks (which I had in mind but then forgot) and @Sjors for vibe coding some

    There's still not here, followup?

  32. maflcko referenced this in commit fa61fad417 on Aug 6, 2026
  33. maflcko referenced this in commit fa7bc75e84 on Aug 6, 2026
  34. maflcko force-pushed on Aug 6, 2026
  35. DrahtBot added the label CI failed on Aug 6, 2026
  36. maflcko commented at 12:33 PM on August 6, 2026: member

    Sure, pushed, but happy to drop it again. For anyone wanting to apply it to any earlier commit: Just run git show $commit_id | git apply.

  37. bench: Add base_blob compare bench via uint256
    Contributed by Sjors in https://github.com/bitcoin/bitcoin/pull/35896#discussion_r3722884353

This benchmark can be run on top of any earlier commit by applying the
diff of this commit before building.
    fa2e76d397
  38. maflcko force-pushed on Aug 6, 2026
  39. DrahtBot removed the label CI failed on Aug 6, 2026
  40. optout21 commented at 2:52 PM on August 6, 2026: contributor

    ACK fa2e76d397a4be6d98d3a43f4df923fa592523ea Switching to default spaceship operators is a clear improvement. Codereview, local tests (by commits). Out of curiosity I looked for other Compare methods, found only in leveldb and miniscript. Fun fact: all 4 commits start with the characters fa.

  41. DrahtBot requested review from purpleKarrot on Aug 6, 2026
  42. Sjors commented at 3:09 PM on August 6, 2026: member

    ACK fa2e76d397a4be6d98d3a43f4df923fa592523ea

  43. purpleKarrot commented at 4:36 PM on August 6, 2026: contributor

    In my opinion, a benchmark is useful (or necessary) to justify a non-obvious implementation. Using = default should not require any justification. But it is up to you.

    ACK fa2e76d397a4be6d98d3a43f4df923fa592523ea

  44. maflcko commented at 4:50 PM on August 6, 2026: member

    Using = default should not require any justification.

    Happy to ack a pull removing it again after merge, but this now has 3 acks, and one reviewer asked for it to be included. Also, GitHub CI died, so I can't really push here anyway without taking down the PR.

  45. hebasto approved
  46. hebasto commented at 4:55 PM on August 6, 2026: member

    re-ACK fa2e76d397a4be6d98d3a43f4df923fa592523ea.

  47. fanquake referenced this in commit 21fd2fb619 on Aug 6, 2026
  48. Sjors commented at 5:34 PM on August 6, 2026: member

    It's fine to keep the benchmark, in case we feel tempted to improve the implementation, or the compiler regresses. Also, it's not a priority obvious that = default is as good as what we had before.

  49. Sjors referenced this in commit c659fc07a7 on Aug 6, 2026
  50. w0xlt commented at 6:42 PM on August 6, 2026: contributor

    ACK fa2e76d397a4be6d98d3a43f4df923fa592523ea as a simplification/refactor, not as a performance optimization.

    I extended the benchmark locally to cover independent comparisons, set/map lookups, and sorting. It showed no consistent improvement: lookups were effectively unchanged, while sorting results were small and compiler-dependent. The equality microbenchmark improved substantially with GCC, but not with Clang.

    Given these results, I would be fine with dropping the new benchmark file.

  51. maflcko added this to the milestone 32.0 on Aug 6, 2026
  52. fanquake merged this on Aug 7, 2026
  53. fanquake closed this on Aug 7, 2026
  54. maflcko deleted the branch on Aug 7, 2026
  55. maflcko commented at 8:55 AM on August 7, 2026: member

    It's fine to keep the benchmark, in case we feel tempted to improve the implementation, or the compiler regresses.

    I also used it to confirm that the following diff is results in the same performance:

    diff --git a/src/primitives/transaction_identifier.h b/src/primitives/transaction_identifier.h
index 9479d2b..c7ef0dd 100644
--- a/src/primitives/transaction_identifier.h
+++ b/src/primitives/transaction_identifier.h
@@ -33,3 +33,2 @@ public:
 
-    constexpr bool operator==(const transaction_identifier&) const = default;
     constexpr auto operator<=>(const transaction_identifier&) const = default;
diff --git a/src/uint256.h b/src/uint256.h
index d722916..9e56a56 100644
--- a/src/uint256.h
+++ b/src/uint256.h
@@ -61,4 +61,2 @@ public:
 
-    constexpr bool operator==(const base_blob&) const = default;
-
     /** Lexicographic ordering

    A special op==() is not needed, when op<=>() is default.

  56. maflcko commented at 10:57 AM on August 7, 2026: member

    Also confirmed that the minimal GCC performance regression is fixed in GCC-16.

    <!-- However, when testing clang-24 with libc++-24, it was way worse than std::memcmp. Maybe someone should fix libc++?

Contributors
maflckoDrahtBotpurpleKarrotl0rincSjorsw0xltfanquakehebastooptout21


SjorspurpleKarrot

Labels
Refactoring

Milestone
32.0

Linked (view graph)
#35911 Warn on and add missing [[noreturn]]
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-08-11 12:51 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me