uint256: Remove unnecessary crypto/common.h dependency #13258

kallewoof · 2018-05-17T05:53:58Z

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

Empact · 2018-05-17T06:06:44Z

All the GetCheapHash calls are relative to CHashWriter and BlockHasher, how about putting it higher up?

I'm not crazy about the template approach because it's overly general and doesn't guarantee that there are 64 bits there to read.

kallewoof · 2018-05-17T06:20:42Z

I'm not crazy about the template approach because it's overly general and doesn't guarantee that there are 64 bits there to read.

Yeah, but a uint256 does not guarantee it is random, either, so you still have to use this cautiously, even before this patch.

Putting it in CHashWriter may be a better idea, for sure. I'll try that and see what it looks like.

kallewoof · 2018-05-17T06:37:00Z

@Empact Changed to your suggested approach. No more template, and code looks cleaner. Only gotcha here is people may be tempted to GetHash().GetCheapHash() which will do double-double-SHA256, instead of double-SHA256. Considering the limited use, I think that gotcha is acceptable.

Empact · 2018-05-17T06:41:27Z

Nice, looks much better IMO.

Empact · 2018-05-17T06:41:57Z

src/addrman.cpp

Unnecessary include.

Empact · 2018-05-17T06:53:51Z

utACK 015e0f7

Empact · 2018-05-17T06:58:13Z

I'd close #13242 in favor of this - as this offers endianness consistency, test stability, and better code organization.

kallewoof · 2018-05-17T07:03:09Z

Yeah, it got too big for the desired effect. I do want to get rid of the unnecessary endianness consistency, though, but I think rewriting those tests to be a bit more useful is a necessary middle-step.

DrahtBot · 2018-07-22T23:49:13Z

The last travis run for this pull request was 66 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

DrahtBot · 2018-09-07T18:54:44Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14624 (Some simple improvements to the RNG code by sipa)
#13462 (Simplify common case of CHashWriter and drop SerializeHash by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sipa · 2018-09-10T01:27:17Z

utACK 361ecbf3e9ffe83800e845426b6cc835267c1932

maflcko · 2018-09-10T01:54:41Z

utACK 361ecbf3e9ffe83800e845426b6cc835267c1932

Empact · 2018-09-10T17:42:37Z

src/validation.h

nit: Could benefit from a comment referencing GetCheapHash to explain the implementation.

Okay. Got 3 utACKs so will fix this if another rebase becomes necessary or other changes are suggested.

Could add a commit with "doc: bla..." on top of the branch (not touching the existing commits)?

Good idea. Done!

Empact · 2018-09-10T17:42:55Z

re-utACK 361ecbf

laanwj · 2018-09-13T11:51:24Z

src/hash.h

I don't think this comment is still appropriate. As it samples the output, which went through a hash function, this requirement about the distribution of the input of the hash writer no longer holds.

Good point. I also wonder about the 'cheap' part here, as it's actually just taking the prefix of the real hash, but keeping for now.

ken2812221 · 2018-09-17T00:03:15Z

re-utACK 93baaf9

maflcko · 2018-09-17T19:34:25Z

re-utACK 93baaf9c3e

promag

utACK 93baaf9, sorry the nit 🏃

promag · 2018-09-17T22:16:14Z

src/validation.h

Damn. Fixed.

promag · 2018-09-18T10:27:31Z

utACK bf2e010.

maflcko · 2018-11-07T18:28:18Z

utACK bf2e010

Empact · 2018-11-07T20:33:39Z

re-utACK bf2e010

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

Summary: bf2e01097 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 Backport of Core PR13258 bitcoin/bitcoin#13258 Test Plan: make check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4277

…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

merge bitcoin#13258, bitcoin#14951

…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

kallewoof mentioned this pull request May 17, 2018

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

Closed

kallewoof changed the title ~~uint256: Remove unnecessary crypto/common.h use (alternative)~~ uint256: Remove unnecessary crypto/common.h dependency (alternative) May 17, 2018

kallewoof changed the title ~~uint256: Remove unnecessary crypto/common.h dependency (alternative)~~ uint256: Remove unnecessary crypto/common.h dependency May 17, 2018

fanquake added the Refactoring label May 17, 2018

kallewoof force-pushed the uint256-no-crypto-alt branch from 1dbe85d to a02e3e6 Compare May 17, 2018 06:35

Empact reviewed May 17, 2018

View reviewed changes

kallewoof force-pushed the uint256-no-crypto-alt branch from a02e3e6 to 015e0f7 Compare May 17, 2018 06:47

DrahtBot closed this Jul 22, 2018

DrahtBot reopened this Jul 22, 2018

DrahtBot mentioned this pull request Aug 11, 2018

refactor: Removal of circular dependency between index/txindex, validation and index/base #13942

Closed

kallewoof force-pushed the uint256-no-crypto-alt branch from 015e0f7 to 361ecbf Compare September 10, 2018 01:05

Empact reviewed Sep 10, 2018

View reviewed changes

laanwj reviewed Sep 13, 2018

View reviewed changes

kallewoof force-pushed the uint256-no-crypto-alt branch from bf27187 to 93baaf9 Compare September 14, 2018 02:44

promag reviewed Sep 17, 2018

View reviewed changes

uint256: Remove unnecessary crypto/common.h use

bf2e010

kallewoof force-pushed the uint256-no-crypto-alt branch from 93baaf9 to bf2e010 Compare September 18, 2018 05:27

DrahtBot mentioned this pull request Oct 31, 2018

Some simple improvements to the RNG code #14624

Merged

laanwj merged commit bf2e010 into bitcoin:master Nov 30, 2018

kallewoof deleted the uint256-no-crypto-alt branch October 17, 2019 08:36

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 21, 2021

Merge pull request #4499 from vijaydasmp/backport_v18_vijay_batch4_3

dc93063

merge bitcoin#13258, bitcoin#14951

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

uint256: Remove unnecessary crypto/common.h dependency #13258

uint256: Remove unnecessary crypto/common.h dependency #13258

Uh oh!

Conversation

kallewoof commented May 17, 2018 • edited by DrahtBot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented May 17, 2018

Uh oh!

kallewoof commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented May 17, 2018

Uh oh!

Empact commented May 17, 2018

Uh oh!

kallewoof commented May 17, 2018

Uh oh!

DrahtBot commented Jul 22, 2018

Uh oh!

DrahtBot commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

sipa commented Sep 10, 2018

Uh oh!

maflcko commented Sep 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Empact commented Sep 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Sep 17, 2018

Uh oh!

maflcko commented Sep 17, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Sep 18, 2018

Uh oh!

maflcko commented Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Nov 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kallewoof commented May 17, 2018 •

edited by DrahtBot

Loading

Empact commented May 17, 2018 •

edited

Loading

kallewoof commented May 17, 2018 •

edited

Loading

DrahtBot commented Sep 7, 2018 •

edited

Loading

maflcko commented Nov 7, 2018 •

edited

Loading