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

    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.

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

    ACK fa7f553781795cf0240b77e2aad42745f4ebc2c3

    Thanks for the fix!

Labels

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-07-31 20:50 UTC

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