CCoinsKeyHasher::operator() should return size_t #4635

1. 
   laanwj commented at 11:36 am on August 5, 2014: member

   It currently returns uint64_t, which on older boost (at least 1.46) causes failures on 32-bit systems. This problem was introduced in bc42503.

   Fixes #4634.

2. 
   jgarzik commented at 11:50 am on August 5, 2014: contributor

   Any chance of a regression test added to the test suite?
3. 
   sipa commented at 11:50 am on August 5, 2014: member

   @jgarzik The tests already fail without this, on the affected environments.
4. 
   laanwj commented at 12:12 pm on August 5, 2014: member

   Right, the tests already failed. But no one has been running them with that version of boost, at least on a 32-bit system.

   The Boost version used by the pulltester is even older than 1.46, and it passed:

   0#define BOOST_LIB_VERSION "1_40"
   

   It’s also fine in 1.54+. Seemingly the problem with non-size_t-hashers only exists in a relatively small range of boost versions.

5. 
   Diapolo commented at 9:24 am on August 6, 2014: none

   Not sure I understand, but key.GetHash(salt) gives a uint64_t and we now cast that to size_t? Shouldn’t some more places be size_t then and not just this changed return value?
6. 
   laanwj commented at 9:37 am on August 6, 2014: member

   @diapolo GetHash() is a 64-bit hash function so it will always give a 64-bit result. Casting it to size_t is just to make boost happy. It may chop off the upper 32 bits on systems where size_t is 32 bits, but for a well-designed hash function that doesn’t matter as the properties stay the same.
7. 
   Diapolo commented at 1:22 pm on August 6, 2014: none

   @laanwj I would feel better if you could add a small comment about that and why it was changed.
8. 
   laanwj commented at 2:45 pm on August 6, 2014: member

   Will add a comment.
9. laanwj added the label Bug on Aug 6, 2014
10. laanwj added the label Priority High on Aug 6, 2014
11. CCoinsKeyHasher::operator() should return size_t

    It currently returns uint64_t, which on older boost (at least 1.46) causes
    test failures on 32-bit systems.
    
    This problem was introduced in bc42503.
    
    Fixes #4634.

    6c23b08203
12. 
    BitcoinPullTester commented at 7:43 am on August 7, 2014: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4635_6c23b082033b627f31170166c07ab35fa6be9343/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
13. 
    Diapolo commented at 8:29 am on August 7, 2014: none

    ACK, thanks for adding that comment.
14. laanwj merged this on Aug 8, 2014
15. laanwj closed this on Aug 8, 2014

    ---

16. laanwj referenced this in commit f5d99075bf on Aug 8, 2014
17. 
    danra commented at 9:24 am on September 16, 2017: contributor

    @sipa Since std::unordered_map is now used instead of boost::unordered_map, would it be beneficial to revert SaltedOutpointHasher::operator() to return uint_64t?
18. DrahtBot locked this on Sep 8, 2021

Contributors
laanwj jgarzik sipa Diapolo BitcoinPullTester danra

Labels
Bug