Page MenuHomePhabricator

First pass at HashType class
ClosedPublic

Authored by jasonbcox on Dec 14 2017, 02:10.

Details

Reviewers
deadalnix
schancel
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGING5a348ed0ef72: First pass at HashType class
rABC5a348ed0ef72: First pass at HashType class
Summary

Breaking T61 up into multiple diffs, with this being the first. I'm looking for early feedback on the design of HashType before utilizing it throughout the codebase.

Test Plan

I plan to write tests for the HashType class itself after a first-pass on reviewing this change. Existing tests should cover all usages of the class.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
hash
Lint
Lint Passed
SeverityLocationCodeMessage
Auto-Fixsrc/script/interpreter.cpp:1CFMTCode style violation
Unit
No Test Coverage
Build Status
Buildable 1370
Build 1370: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Dec 14 2017, 02:10
src/script/hashtype.h
25

I may be able to get rid of this and replace it with a default constructor once HashType is used throughout the codebase. We'll see.

deadalnix requested changes to this revision.Dec 14 2017, 15:17

Do not add a new cpp file for this. This'll prevent any inlining. Make the call COW, not mutable. having random setters do not help.

src/script/hashtype.h
8

Remove

25

Make it explicit.

30

withFoo and return a modified copy.

34

dito

This revision now requires changes to proceed.Dec 14 2017, 15:17

Collapsed hashtype.cpp into hashtype.h and fixed style according to feedback.

deadalnix requested changes to this revision.Dec 15 2017, 16:52
deadalnix added inline comments.
src/script/hashtype.h
20 ↗(On Diff #2134)

I Guess SigHashType would be more descriptive.

27 ↗(On Diff #2134)

Remove. They are not setter and even if they were, this adds 0 value to the code.

38 ↗(On Diff #2134)

Considering these 3 refers to the same variable, I'd create a C++11 style enum and one method.

40 ↗(On Diff #2134)

dito

51 ↗(On Diff #2134)

dito, except they are actually getters.

56 ↗(On Diff #2134)

GetType() and return the enum value.

58 ↗(On Diff #2134)

dito

63 ↗(On Diff #2134)

getRawSigHashType

This revision now requires changes to proceed.Dec 15 2017, 16:52

Changed name HashType -> SigHashType, merged withX for base sig hash type to a single method accepting an enum type, and other minor fixes.

schancel added inline comments.
src/script/hashtype.h
20

Same about class name.

src/script/interpreter.cpp
1221 ↗(On Diff #2185)

Can we split these changes out into another diff? It would be nice to land the class and tests, and have a stacked diff for implementing it in this class.

I can help you out with how to make that happened if needed.

src/script/sighashtype.h
48 ↗(On Diff #2185)

I think Amaury was maybe looking for something like this:

I think he was looking for something like this:

bool hasFlag(SigHashFlag flag) const { return (sigHash & 0x1f) == flag; }
bool setFlag(SigHashFlag flag) const { ... }
54 ↗(On Diff #2185)

Per discussion, let's change this to:

(sigHash & SIGHASH_FORKID) == SIGHASH_FORKID

and the same for anyone can spend.

schancel requested changes to this revision.Dec 18 2017, 17:49
This revision now requires changes to proceed.Dec 18 2017, 17:49
src/script/sighashtype.h
48 ↗(On Diff #2185)

It doesn't make sens for flags, but for all/none/single, yes.

54 ↗(On Diff #2185)

Why ?

jasonbcox added inline comments.
src/script/sighashtype.h
54 ↗(On Diff #2185)

Although it makes logical sense to have !! we agreed it wasn't good style nor easy to read.

jasonbcox marked 6 inline comments as done.

Added tests, removed proof of concept implementation to be featured in a separate diff, and minor fixes.

deadalnix requested changes to this revision.Dec 19 2017, 04:21
deadalnix added inline comments.
src/script/sighashtype.h
50 ↗(On Diff #2192)

getter

This revision now requires changes to proceed.Dec 19 2017, 04:21

Nice. This is getting there.

src/Makefile.am
1 ↗(On Diff #2192)

Any idea why this file is included?

1 ↗(On Diff #2192)

Nevermind. I was looking at changes since the last diff, and there wasn't any for obvious reasons.

Replaced hasBaseSigHash with getter.

deadalnix requested changes to this revision.Dec 20 2017, 02:29

It's getting there. The code itself looks good, the test can be improved.

src/test/script_sighashtype_tests.cpp
22 ↗(On Diff #2194)

You need to have a base type in addition to flags.

49 ↗(On Diff #2194)

If BASESIGHASH_NONE doesn't work by itself, then there is no need for prefix. Just go for BaseSigHashType::NONE . If it does work, then use that.

74 ↗(On Diff #2194)

You need to be testing more than one flag, and flag removal. You also need to check the change in the base type preserve flags.

This revision now requires changes to proceed.Dec 20 2017, 02:29

Enforced base sighash, reduced BaseSigHashType names, and added tests.

This revision is now accepted and ready to land.Dec 20 2017, 19:07
This revision was automatically updated to reflect the committed changes.