Page MenuHomePhabricator

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

Authored by markblundeberg on Sat, Apr 13, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Sat, Apr 13, 00:31
Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Apr 13, 00:31
jasonbcox requested changes to this revision.Sat, Apr 13, 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.Sat, Apr 13, 16:05
markblundeberg marked 2 inline comments as done.Sat, Apr 13, 16:16
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 :)

markblundeberg marked 4 inline comments as done.Sat, Apr 13, 16:29
  • added cmakelists entry
  • created siphash.* files from ABC code instead of Core code
deadalnix added inline comments.Sun, Apr 14, 14:23
src/crypto/siphash.h
8 ↗(On Diff #8049)

cstdint

deadalnix requested changes to this revision.Sun, Apr 14, 19:35
This revision now requires changes to proceed.Sun, Apr 14, 19:35
deadalnix added a subscriber: Fabien.Sun, Apr 14, 19:36
deadalnix added inline comments.
src/crypto/siphash.h
8 ↗(On Diff #8049)

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

markblundeberg marked an inline comment as done.

cstdint

deadalnix accepted this revision.Mon, Apr 15, 11:54
jasonbcox accepted this revision.Tue, Apr 16, 17:45
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.Tue, Apr 16, 17:45
This revision was automatically updated to reflect the committed changes.