I found the logic of isDefined() to be confusing and subtle.
(baseType as a red herring variable name, hidden cast-clipping of forkId)
I think this variable rename and these comments should help future code
readers to see what is the behaviour of this function.
Details
Details
nothing substantial was changed.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- rename-basetype
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4619 Build 7301: Bitcoin ABC Buildbot (legacy) Build 7300: arc lint + arc unit
Event Timeline
| src/script/sighashtype.h | ||
|---|---|---|
| 70 | I think this adds to the confusion because this is not what the code is doing explicitly. Do you think it would benefit from the following in order to make it explicit? | |
| src/script/sighashtype.h | ||
|---|---|---|
| 70 | This is a bad idea. You want to clear the flags you know of and check that what remains is in range. At no point you want to touch the undefined flags. One 0-day in that piece of code was enough, there is no need to put efforts into introducing a second one. | |
| src/script/sighashtype.h | ||
|---|---|---|
| 70 | If we were to simplify this function into one line, it would be: return (sigHash & 0x3f >= SIGHASH_ALL) && (sigHash & 0x3f <= SIGHASH_SINGLE); . | |
Comment Actions
@jasonbcox it's still red, what am I supposed to do? If you want to refactor you can submit a revision and we can discuss that. The point of this Diff is only to clarify confusing code.
Comment Actions
@jasonbcox if you think these comments are a bad idea I can abandon this diff, no worries. I don't want to refactor the code in any way though ... even though it's apparently trivial, this is too scary for me given the history of this particular function. :-D
| src/script/sighashtype.h | ||
|---|---|---|
| 70 | This comment convey what the code effectively does, which is not what matter - if that was what the user was interested in, the users could do it, it's fairly trivial. The reason a function is used is because the caller do not need or want to care about WHAT this function does. It cares about the contract the function provides, and the contract is: is that sighashtype defined under the current consensus rules. The approach is to clear known flags and check that what remains is exclusively a valid base hash type. There is a reason why this approach is taken rather than the explicit list of criterion as listed in the comment. It is just not solid. What happen if a criterion is missing ? In one case, we'll get the test using that criteria to start failing, in the other we'll get a 0-day. And this is not just grandstanding as we actually got a 0-day in that code. | |
| 74 | Going for 0x3f here is the same issue. | |
| src/script/sighashtype.h | ||
|---|---|---|
| 74 | Yes, the reason I find this confusing is that the original code uses baseType = BaseSigHashType(...), so you would expect that baseType should be identical to getBaseType() which ranges 0--0x1f. But that is not at all the case. For invalid sighashes with bit 0x20 set, baseType is distinct from getBaseType(). Therefore it shouldn't be called 'baseType' at all. The conversion BaseSigHashType() is also misleading since it actually is an implicit reduction from uint32 (with forkid) to uint8 (without forkid). I.e., it doesn't create a basetype at all but instead its function is to strip forkid. Maybe there is a better way to clarify this. I will abandon this and someone else can figure it out. | |