test: Suppress implicit-unsigned-integer-truncation:SaltedCoinsCacheHasher::operator() #35773

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2607-test-supp-uint-hash-trunc changing 1 files +1 −0
  1. maflcko commented at 12:59 PM on July 22, 2026: member

    The truncation of u64 to size_t is intentional here, but it would be nice to document that for ubsan.

    Otherwise, ubsan will print warnings about this. E.g. on 32-bit platforms:

    /ci_container_base/src/coins.h:255:16: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long long') of value 18400304222395891501 (64-bit, unsigned) to type 'size_t' (aka 'unsigned int') changed the value to 2265382701 (32-bit, unsigned)
    

    This is a bit tedious to test on 64-bit platforms, but one can use a diff like:

    diff --git a/src/coins.h b/src/coins.h
    index c854893bcb..906be9efae 100644
    --- a/src/coins.h
    +++ b/src/coins.h
    @@ -246,3 +246,3 @@ public:
         /** Hash a transaction ID, itself a cryptographic hash, as one jumbo block. */
    -    size_t operator()(const Txid& id) const noexcept
    +    uint32_t operator()(const Txid& id) const noexcept
         {
    @@ -252,3 +252,3 @@ public:
         /** Hash an outpoint as its txid jumbo block followed by the zero-extended index as one normal block. */
    -    size_t operator()(const COutPoint& id) const noexcept
    +    uint32_t operator()(const COutPoint& id) const noexcept
         {
    

    and:

    $ UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./bld-cmake/bin/test_bitcoin 
    
    ./src/coins.h:255:16: runtime error: implicit conversion from type 'uint64_t' (aka 'unsigned long') of value 17092028281225243117 (64-bit, unsigned) to type 'uint32_t' (aka 'unsigned int') changed the value to 291269101 (32-bit, unsigned)
    
  2. test: Suppress implicit-unsigned-integer-truncation:SaltedCoinsCacheHasher::operator() fa7f553781
  3. DrahtBot added the label Tests on Jul 22, 2026
  4. DrahtBot commented at 12:59 PM on July 22, 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/35773.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    ACK l0rinc, sedited

    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. in test/sanitizer_suppressions/ubsan:64 in fa7f553781
      60 | @@ -61,6 +61,7 @@ implicit-integer-sign-change:crypto/
      61 |  implicit-integer-sign-change:TxConfirmStats::removeTx
      62 |  implicit-integer-sign-change:EvalScript
      63 |  implicit-signed-integer-truncation:crypto/
      64 | +implicit-unsigned-integer-truncation:SaltedCoinsCacheHasher::operator()
    


    l0rinc commented at 6:59 PM on July 22, 2026:

    The alternative would be to make the narrowing explicit:

    size_t operator()(const Txid& id) const noexcept
    {
        return static_cast<size_t>(m_hasher.Hash(id.ToUint256()));
    }
    

    but that would just add noise for a conversion that only narrows on platforms we want to sunset, right?


    It has always bothered me, though, that these suppressions do not explain why they are safe. Could we add a short comment here?

    # Standard unordered containers require size_t hashes, truncating the 64-bit SipHash result on 32-bit platforms
    implicit-unsigned-integer-truncation:SaltedCoinsCacheHasher::operator()
    

    maflcko commented at 7:16 AM on July 30, 2026:

    I don't mind making the cast explicit, but if there was a reason to make it explicit, it should ideally be done for all hasher (etc), not just for a single one.

    Same for the comment: If a comment is added, it should also be added to the other places. Recall that implicit-unsigned-integer-truncation:crypto/ is a catch-all suppression for the whole folder, covering PresaltedSipHasher etc.

    So my preference would be to leave this as is (minimally complete, and correct), or create a larger, but also complete patch. Happy to push such a patch, or review one, if someone creates one.


    l0rinc commented at 5:35 PM on July 30, 2026:

    I don't mind making the cast explicit

    I'm fine with the change as-is, I don't think we should do the casts, I just documented my understanding of what alternatives we were choosing from. I rather think we should stop supporting 32-bit architectures overall - is there any objection to this? Or should I just push a PR removing 32-bit support and see what happens?

    If a comment is added, it should also be added to the other places

    Agree, I'll push such a change


    maflcko commented at 6:10 PM on July 30, 2026:

    I rather think we should stop supporting 32-bit architectures overall - is there any objection to this? Or should I just push a PR removing 32-bit support and see what happens?

    See the discussion on the issue. Maybe the devs can be asked again in an IRC meeting, and then a release note can be added to ask users if it would be a problem to remove, but I wouldn't rush it into the next release on short notice.


    maflcko commented at 6:32 PM on July 30, 2026:

    Also, it looks like a riscv32 ci config was recently added for the kernel, so it could even be a goal to support 32-bit for the kernel, even if it is removed for the node?

  6. l0rinc approved
  7. l0rinc commented at 7:13 PM on July 22, 2026: contributor

    ACK fa7f553781795cf0240b77e2aad42745f4ebc2c3

    Thanks for the fix!

  8. sedited approved
  9. sedited commented at 7:50 PM on August 4, 2026: contributor

    ACK fa7f553781795cf0240b77e2aad42745f4ebc2c3

  10. maflcko added this to the milestone 32.0 on Aug 4, 2026
  11. sedited merged this on Aug 4, 2026
  12. sedited closed this on Aug 4, 2026

  13. maflcko deleted the branch on Aug 4, 2026
Labels

Milestone
32.0


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-24 07:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me