Handle witness version and OP_N results as an unsigned char #12937

pull Empact wants to merge 1 commits into bitcoin:master from Empact:unsigned-char-witnessversion changing 10 files +20 −18

Empact commented at 4:21 AM on April 11, 2018: member

According to BIP 141, the version range is 0-16 and stored as a byte. Similarly, DecodeOP_N presents a range of 0-16. Thus an unsigned char is a natural/direct representation for the value.

Removes the need for 2 casts in script/standard.cpp.

Handle witness version and OP_N results as an unsigned char

According to BIP 141, the version range is 0-16 and stored as a byte.
Similarly, DecodeOP_N presents a range of 0-16. Thus an unsigned char
is a natural/direct representation for the value.

Removes the need for 2 casts.

269b94336d

fanquake added the label Refactoring on Apr 11, 2018
randolf commented at 5:27 AM on April 11, 2018: contributor

Although internally it's stored as a byte, I wonder, are there other projects that might be relying on this API with "int" parameters instead of "unsigned char" parameters? If so, this could introduce an incompatibility (which should be avoidable by providing "int" parameterized subroutines alongside these "unsigned char" parameterized subroutines that are also marked as "deprecated" (and their documentation should note the correct subroutines to use).

What are your thoughts on this?
Empact commented at 7:11 AM on April 11, 2018: member
Good question, looking over the method signatures affected:

The following are static in script/interpreter.cpp, so not externally visible
- VerifyWitnessProgram
- WitnessSigOps
DecodeOP_N is static in script/script.h. It could be moved to script/script.cpp if we also moved Solver to that file, from script/standard.cpp

CScript::IsWitnessProgram is necessarily publicly visible.

In both of the latter cases, we could overload the method in a deprecated way to enable ongoing support for any external callers. I'll take y'alls call as to whether that's necessary.
laanwj commented at 2:46 PM on April 11, 2018: member

Sorry, but I don't see the point, to be honest. This is consensus critical code, if there's no good reason to change it (and "this type looks better" or "we can avoid a cast here" isn't a good reason like that) we shouldn't.
sdaftuar commented at 2:51 PM on April 11, 2018: member

I was about to say the same as @laanwj -- I think we should have a higher threshold for changing consensus critical code, to justify the review burden. Concept NACK.
MarcoFalke commented at 2:58 PM on April 11, 2018: member

Closing for now, since there seems to be disagreement whether to do stylistic refactoring to consensus critical code.
MarcoFalke closed this on Apr 11, 2018
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels