Skip to content

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Aug 5, 2014

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.

@jgarzik
Copy link
Contributor

jgarzik commented Aug 5, 2014

Any chance of a regression test added to the test suite?

@sipa
Copy link
Member

sipa commented Aug 5, 2014

@jgarzik The tests already fail without this, on the affected environments.

@laanwj
Copy link
Member Author

laanwj commented Aug 5, 2014

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:

#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.

@Diapolo
Copy link

Diapolo commented Aug 6, 2014

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?

@laanwj
Copy link
Member Author

laanwj commented Aug 6, 2014

@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.

@Diapolo
Copy link

Diapolo commented Aug 6, 2014

@laanwj I would feel better if you could add a small comment about that and why it was changed.

@laanwj
Copy link
Member Author

laanwj commented Aug 6, 2014

Will add a comment.

@laanwj laanwj added Bug labels Aug 6, 2014
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 bitcoin#4634.
@BitcoinPullTester
Copy link

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.

@Diapolo
Copy link

Diapolo commented Aug 7, 2014

ACK, thanks for adding that comment.

@laanwj laanwj merged commit 6c23b08 into bitcoin:master Aug 8, 2014
laanwj added a commit that referenced this pull request Aug 8, 2014
6c23b08 CCoinsKeyHasher::operator() should return size_t (Wladimir J. van der Laan)
@danra
Copy link
Contributor

danra commented Sep 16, 2017

@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?

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Testnet fails to sync past block 380

6 participants