uint256: Remove unnecessary crypto/common.h use #13242
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
For reference, added in #7078, 0.12.0 |
Contributor
Author
|
@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). |
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 |
kallewoof
force-pushed
the
uint256-no-crypto
branch
from
caa75a7 to
ea1d189
Compare
Contributor
Author
|
@sipa ea1d189 |
Contributor
Author
|
OK, I tried switching to |
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.
kallewoof
force-pushed
the
uint256-no-crypto
branch
from
ea1d189 to
656ddec
Compare
Contributor
Author
|
I updated the tests to work on big endian. I also addressed @sipa's point about not dereferencing different type. |
Contributor
Author
|
There is an alternative approach to this in #13258. |
laanwj
added a commit
that referenced
this pull request
Nov 30, 2018
bf2e010 uint256: Remove unnecessary crypto/common.h use (Karl-Johan Alm) Pull request description: This is an alternative to #13242 which keeps the `ReadLE64` part, but moves the `crypto/common.h` dependency into `crypto/common.h` as a function outside of `uint256`. **Reason:** this change will remove dependencies for `uint256` to `crypto/common.h`, `compat/endian.h`, and `compat/byteswap.h`. This PR removes the need to update tests to be endian-aware/-independent, but keeps the (arguably dubious) `ReadLE64` part (which was only introduced to fix the tests, not for any functionality). Tree-SHA512: 78b35123cdb185b3b3ec59aba5ca8a5db72624d147f2d6a5484ffa5ce626a72f782a01dc6893fc8f5619b03e2eae7b5a03b0df5d43460f3bda428e719e188aec
vijaydasmp
pushed a commit
to vijaydasmp/dash
that referenced
this pull request
Oct 21, 2021
…ndency bf2e010 uint256: Remove unnecessary crypto/common.h use (Karl-Johan Alm) Pull request description: This is an alternative to bitcoin#13242 which keeps the `ReadLE64` part, but moves the `crypto/common.h` dependency into `crypto/common.h` as a function outside of `uint256`. **Reason:** this change will remove dependencies for `uint256` to `crypto/common.h`, `compat/endian.h`, and `compat/byteswap.h`. This PR removes the need to update tests to be endian-aware/-independent, but keeps the (arguably dubious) `ReadLE64` part (which was only introduced to fix the tests, not for any functionality). Tree-SHA512: 78b35123cdb185b3b3ec59aba5ca8a5db72624d147f2d6a5484ffa5ce626a72f782a01dc6893fc8f5619b03e2eae7b5a03b0df5d43460f3bda428e719e188aec
pravblockc
pushed a commit
to pravblockc/dash
that referenced
this pull request
Nov 18, 2021
…ndency bf2e010 uint256: Remove unnecessary crypto/common.h use (Karl-Johan Alm) Pull request description: This is an alternative to bitcoin#13242 which keeps the `ReadLE64` part, but moves the `crypto/common.h` dependency into `crypto/common.h` as a function outside of `uint256`. **Reason:** this change will remove dependencies for `uint256` to `crypto/common.h`, `compat/endian.h`, and `compat/byteswap.h`. This PR removes the need to update tests to be endian-aware/-independent, but keeps the (arguably dubious) `ReadLE64` part (which was only introduced to fix the tests, not for any functionality). Tree-SHA512: 78b35123cdb185b3b3ec59aba5ca8a5db72624d147f2d6a5484ffa5ce626a72f782a01dc6893fc8f5619b03e2eae7b5a03b0df5d43460f3bda428e719e188aec
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 inCAddrInfobucket stuff, and that's it). We can remove an entire#includeand probably some overhead as well. This will remove a number of chained dependencies fromuint256(crypto/common.h,compat/endian.h, andcompat/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...)