Page MenuHomePhabricator

Extract CSipHasher to it's own file in crypto/ directory.
ClosedPublic

Authored by markblundeberg on Apr 13 2019, 00:31.

Details

Summary

This is a move-only commit with the exception of changes to includes.

backported from core PR14074 ( 4fb789e9b )

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
move-siphash
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5450
Build 8962: Bitcoin ABC Buildbot (legacy)
Build 8961: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Apr 13 2019, 16:05
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/Makefile.am
328 ↗(On Diff #8045)

This change needs to be reflected in the appropriate CMakeLists file for the cmake build as well.

src/coins.cpp
10 ↗(On Diff #8045)

I know this matches the backport, but adding this seems useless. Correct me if I'm wrong. Otherwise we should remove it.

src/crypto/siphash.cpp
61 ↗(On Diff #8045)

The previous iteration matches our code style (not to mention these parentheses are totally unnecessary). And this doesn't match the summary that this is a move-only change.

I think we can leave these parentheses changes out of this diff.

src/undo.h
14 ↗(On Diff #8045)

ditto

This revision now requires changes to proceed.Apr 13 2019, 16:05
markblundeberg added inline comments.
src/coins.cpp
10 ↗(On Diff #8045)

yes I thought so too -- however it this file needs PROTOCOL_VERSION which it used to get from coins.h via hash.h; but hash.h got removed from coins.h .

src/crypto/siphash.cpp
61 ↗(On Diff #8045)

Ah! Thanks for pointing this out. I had taken these files from the cherry picked commit and merely linted them, whereas I should instead do my own move-over from the old content. There are likely changes in the .h too which phabricator isn't picking up on so I'll do a move-over for that one too.

src/undo.h
14 ↗(On Diff #8045)

ditto :)

  • added cmakelists entry
  • created siphash.* files from ABC code instead of Core code
src/crypto/siphash.h
8

cstdint

deadalnix requested changes to this revision.Apr 14 2019, 19:35
This revision now requires changes to proceed.Apr 14 2019, 19:35
deadalnix added inline comments.
src/crypto/siphash.h
8

@Fabien We would really benefit from a linter warning about C headers.

markblundeberg marked an inline comment as done.

cstdint

jasonbcox added inline comments.
src/coins.cpp
10 ↗(On Diff #8045)

I see. Thanks for clarifying.

This revision is now accepted and ready to land.Apr 16 2019, 17:45