Specify which 1 byte push op codes are valid #487

Christewart commented at 11:08 PM on January 3, 2017: contributor

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

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

d84186c01c

luke-jr commented at 11:10 PM on January 3, 2017: member

@CodeShark @jl2012 @sipa

(Although I actually think it is clearer before this change...)

Update bip-0141.mediawiki

Clarifying rewording, `OP_0` is not a 1 byte push op code since it pushes the empty byte vector onto the stack.

608d5dc95f

afk11 commented at 11:46 PM on January 3, 2017: contributor

'valid' was probably enough for this, or mention that only OP_1NEGATE is disallowed?

luke-jr commented at 12:50 AM on January 4, 2017: member

@afk11 Reason "valid" was not enough, is that OP_1NEGATE is valid but not recognised as a witness script.

jl2012 commented at 3:26 AM on January 4, 2017: contributor

maybe making a footnote to say OP_1NEGATE is not included?

Christewart commented at 1:41 PM on January 4, 2017: contributor

@luke-jr By your definition of 'valid' isn't any 1 byte value 'valid'?

luke-jr commented at 2:46 PM on January 4, 2017: member

No, if it causes the script to fail, it is invalid.

Christewart commented at 10:10 PM on January 31, 2017: contributor

@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"

luke-jr commented at 10:29 PM on January 31, 2017: member

OP_1NEGATE does not cause a script to fail, it only causes the input spending it to be disallowed to include witness data.

jonathancross commented at 2:26 AM on November 7, 2017: contributor

What is the status of this PR? @luke-jr Are there specific changes you are requesting here?

Christewart commented at 7:56 PM on December 28, 2017: contributor

@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.

in bip-0141.mediawiki:86 in 608d5dc95f

  82 | @@ -83,7 +83,7 @@ If all transactions in a block do not have witness data, the commitment is optio
  83 |  
  84 |  === Witness program ===
  85 |  
  86 | -A <code>scriptPubKey</code> (or <code>redeemScript</code> as defined in BIP16/P2SH) that consists of a 1-byte push opcode (for 0 to 16) followed by a data push between 2 and 40 bytes gets a new special meaning. The value of the first push is called the "version byte". The following byte vector pushed is called the "witness program".
  87 | +A <code>scriptPubKey</code> (or <code>redeemScript</code> as defined in BIP16/P2SH) that consists of a select subset of opcodes (<code>OP_0,OP_1,OP_2,...,OP_16</code>) followed by a data push between 2 and 40 bytes gets a new special meaning. The value of the first push is called the "version byte". The following byte vector pushed is called the "witness program".

evoskuil commented at 8:14 PM on December 28, 2017:

consists of a select subset of opcodes (<code>OP_0,OP_1,OP_2,...,OP_16</code>)

This is incorrect as it implies any subset of 17 opcodes. The specification is any member of that set. It's also not clear what is meant by "select subset".

The value of the first push is called the "version byte".

It is unclear what is the value of OP_0, since it's push/stack value is [] (not 0), which isn't a byte and therefore cannot be a "version byte".

I suggest:

A <code>scriptPubKey</code> (or <code>redeemScript</code> as defined in BIP16/P2SH) that consists of one opcode from (<code>OP_0, OP_1, OP_2, ..., OP_16</code>) followed by a data push of between 2 and 40 bytes gets a new meaning. The logical value of the first push (i.e. [0..16]) is called the "script version". The stack value of the second push (of [2..40] bytes) is called the "witness program"

Christewart commented at 8:21 PM on December 28, 2017:

I think that explanation is pretty good. A nit is script version -> witness version.

evoskuil commented at 8:17 PM on December 28, 2017: contributor

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.

murchandamus commented at 6:46 PM on April 23, 2024: contributor

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?

murchandamus assigned sipa on Apr 23, 2024

sipa commented at 6:58 PM on April 23, 2024: member

ACK 608d5dc95f2ddcee32758fe73de6d68b99021e39

murchandamus merged this on Apr 23, 2024

murchandamus closed this on Apr 23, 2024