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.
Details
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
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
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- hash
- Lint
Lint Passed Severity Location Code Message Auto-Fix src/script/interpreter.cpp:1 CFMT Code style violation - Unit
No Test Coverage - Build Status
Buildable 1370 Build 1370: arc lint + arc unit
Event Timeline
| 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. | |
| 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 |
Comment Actions
Changed name HashType -> SigHashType, merged withX for base sig hash type to a single method accepting an enum type, and other minor fixes.
| 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. |
| 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. |
Comment Actions
Added tests, removed proof of concept implementation to be featured in a separate diff, and minor fixes.
| src/script/sighashtype.h | ||
|---|---|---|
| 50 ↗ | (On Diff #2192) | getter |
Comment Actions
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. |