bips
bips copied to clipboard
bitcoin
Specify which 1 byte push op codes are valid
This adds documentation to BIP141 about which 1 byte push op codes are valid for segwit. This is needed because OP_1NEGATE is a 1 byte push op code, but is NOT a valid 1 byte push op code for segwit. See the implementation here for why OP_1NEGATE is not valid: https://github.com/bitcoin/bitcoin/blob/14d01309bed59afb08651f2b701ff90371b15b20/src/script/script.cpp#L228
@CodeShark @jl2012 @sipa
(Although I actually think it is clearer before this change...)
'valid' was probably enough for this, or mention that only OP_1NEGATE is disallowed?
@afk11 Reason "valid" was not enough, is that OP_1NEGATE is valid but not recognised as a witness script.
@luke-jr I've added a test case to inside of script_tests.cpp to test this, and the test case fails when we use OP_1NEGATE. Can you elaborate further on what you mean?
https://github.com/Christewart/bitcoin/blob/segwit_invalid_op1negate/src/test/script_tests.cpp#L1446-L1452
$ ./test_bitcoin --run_test=script_tests/op1_negate
Running 1 test case...
Failed witness program of invalid program version
test/script_tests.cpp(1451): error: in "script_tests/op1_negate": check s.IsWitnessProgram(version,witProgram) has failed
*** 1 failure is detected in the test module "Bitcoin Test Suite"
OP_1NEGATE does not cause a script to fail, it only causes the input spending it to be disallowed to include witness data.
What is the status of this PR? @luke-jr Are there specific changes you are requesting here?
@luke-jr It does not allow a witness script to be spent properly, which is what the context of this BIP is no? You cannot have OP_1NEGATE as a valid witness version which is all I am trying to say.
Having recently implemented this I agree that this section is poorly worded. IMO the issue isn't really that it is ambiguous with OP_1NEGATE but it is definitely not clear. I was going to bug this myself at the time. I've added my comments inline. There is no reason to refer to the script version as a byte, especially because it is not.
@luke-jr wrote:
No, if it causes the script to fail, it is invalid.
The objection is that the BIP implies that it is valid, because it is a "1-byte push opcode"...
@Christewart
The BIP actually says "1-byte push opcode (for 0 to 16)". Since -1 is not in the logical range 0..16 the BIP is technically correct, though it does require a reader to infer what is meant by "for 0 to 16". The numeric value of the opcode OP_1NEGATE is between the opcodes for 0 and 1..16, so that can add some confusion.
It is always better to be explicit, especially in a technical specification.
This still seems like a good clarification to add to the BIP. @Christewart: What do you think about making it a footnote as suggested by @jl2012?
