uint256: Remove unnecessary crypto/common.h use

kallewoof commented at 0:53 am on May 16, 2018: member

Alternative approach: #13258.

There is no reason for uint256::GetCheapHash() to support multi-platform endian-ness, since the cheap hash is never used outside of the internal system (it is used in CAddrInfo bucket stuff, and that’s it). We can remove an entire #include and probably some overhead as well. This will remove a number of chained dependencies from uint256 (crypto/common.h, compat/endian.h, and compat/byteswap.h).

Edit: due to the dependency on little endian in the tests for the address manager, #7078 switched to the ReadLE64() function. This test reverts that decision and updates the tests instead to work with big and little endian. These tests need to be rethought, possibly removed, as they test basically nothing right now*, but in the meantime, this will address the test errors for big endian.

(* they test that after adding a, b, and c to a bucket, adding d will cause a collision, but adding e will not…)

Empact commented at 0:58 am on May 16, 2018: member

For reference, added in #7078, 0.12.0

kallewoof commented at 1:05 am on May 16, 2018: member

@Empact Thanks! Wow, this was added because of false positive test failures? I don’t know if I can agree with that decision. Hard to test a fix though (no PPC/MIPS here).

sipa commented at 1:09 am on May 16, 2018: member

NACK, this is invalid C++. You cannot take a pointer to an object of type A and then dereference it as a pointer of type B, unless the types are compatible, or B is a char type.

This will likely work on little-endian systems, but will produce different results on big-endian ones. And regardless of architecture, this is undefined behaviour.

FWIW, on x86 ReadLE64 is compiled to a simple load from memory.

kallewoof force-pushed on May 16, 2018

kallewoof commented at 1:14 am on May 16, 2018: member

@sipa ea1d189

kallewoof commented at 1:21 am on May 16, 2018: member

OK, I tried switching to ReadBE64 from ReadLE64 and the addrman tests are failing all over the place. If this hash is a hash, that makes no sense whatsoever.

uint256: Remove unnecessary crypto/common.h use d50d0003a2

test: addrman: Track endian based collision points

The address manager tests currently depend on little endianness because they are written with assumptions about collisions occurring at specific
points. This should be revisited, and these tests should potentially be removed as they serve little purpose now.
For now, this commit adds support for big endian assumptions and determines the endianness of the system, basing assumptions on that.

656ddecd2d

kallewoof force-pushed on May 16, 2018

kallewoof commented at 5:16 am on May 16, 2018: member

I updated the tests to work on big endian. I also addressed @sipa’s point about not dereferencing different type.

laanwj added the label Refactoring on May 16, 2018

kallewoof commented at 5:54 am on May 17, 2018: member

There is an alternative approach to this in #13258.

kallewoof closed this on May 17, 2018

kallewoof deleted the branch on Jul 3, 2018

laanwj referenced this in commit 011c42c5bd on Nov 30, 2018

Munkybooty referenced this in commit 34cb575526 on Aug 2, 2021

Munkybooty referenced this in commit 26d4c048af on Aug 3, 2021

Munkybooty referenced this in commit 65ee5e6c96 on Aug 5, 2021

vijaydasmp referenced this in commit 0d05ddcf0e on Oct 10, 2021

vijaydasmp referenced this in commit e1bd4045f8 on Oct 10, 2021

vijaydasmp referenced this in commit 9f157bb4fa on Oct 12, 2021

vijaydasmp referenced this in commit 291fced17d on Oct 14, 2021

vijaydasmp referenced this in commit 45981b043c on Oct 14, 2021

vijaydasmp referenced this in commit 07687ad475 on Oct 20, 2021

vijaydasmp referenced this in commit d0724b5ee1 on Oct 21, 2021

pravblockc referenced this in commit f708ff12ec on Nov 18, 2021

DrahtBot locked this on Feb 15, 2022

uint256: Remove unnecessary crypto/common.h use #13242