Skip to content

BIP141 fix #416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
luke-jr merged 1 commit into from
Jul 21, 2016
Merged

BIP141 fix #416

luke-jr merged 1 commit into from
Jul 21, 2016

Conversation

@jl2012
Copy link
Contributor

@jl2012 jl2012 commented Jul 10, 2016

@sipa

  1. Make the use of "witness program" consistent
  2. Fix: P2WSH script has a 10000 byte limit
  3. Remove the mention of extending 32 bytes witness program with softfork

CodeShark
CodeShark reviewed Jul 11, 2016
View reviewed changes
bip-0141.mediawiki
That is, CHECKSIG in a witness program is counted as only 1 sigop, and CHECKMULTISIG in a witness program is counted as 1 to 20 sigops according to the arguments. This rule applies to both native witness program and P2SH witness program.
In addition, opcodes within the P2WPKH/P2WSH program are counted identical to as previously within the P2SH check script.
That is, CHECKSIG in a P2WPKH/P2WSH program is counted as only 1 sigop, and CHECKMULTISIG in a P2WSH program is counted as 1 to 20 sigops according to the arguments. This rule applies to both native witness program and P2SH witness program.

Copy link
Contributor

@CodeShark CodeShark Jul 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we use the term "witness script" rather than "P2WPKH/P2WSH program" when talking about the unhashed sequence of op codes and data pushes that then get hashed into a P2WSH witness program. In the case of P2WPKH, the witness program is the pubkey hash and the script is implicit.

@jl2012
Copy link
Contributor Author

jl2012 commented Jul 11, 2016

@CodeShark fixed

@luke-jr
Copy link
Member

luke-jr commented Jul 15, 2016

@jl2012 You are an Author of this BIP, so any PRs you open will automatically get merged... so please don't open PRs until you are certain of a change.

In this case, I will wait for an explicit ACK since it sounds like you're checking with @sipa before desiring it to be merged.

sipa
sipa reviewed Jul 16, 2016
View reviewed changes
bip-0141.mediawiki Outdated
That is, CHECKSIG in a witness program is counted as only 1 sigop, and CHECKMULTISIG in a witness program is counted as 1 to 20 sigops according to the arguments. This rule applies to both native witness program and P2SH witness program.
Each P2WPKH input is counted as 1 sigop.

In addition, opcodes within a P2WSH witnessScript are counted identical to as previously within the P2SH check script.
Copy link
Member

@sipa sipa Jul 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this sentence is grammatically correct.

Copy link
Contributor

@CodeShark CodeShark Jul 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it should be "identically as" - not "identical to as"

@jl2012
Copy link
Contributor Author

jl2012 commented Jul 18, 2016

fixed and squashed

@jl2012 jl2012 mentioned this pull request Jul 18, 2016
Closed
@sipa
Copy link
Member

sipa commented Jul 21, 2016

ACK

@luke-jr luke-jr merged commit 850d103 into bitcoin:master Jul 21, 2016
@theStack theStack mentioned this pull request Jul 11, 2021
Merged
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Aug 1, 2021
Merge bitcoin/bitcoin#22429: test: refactor: fix segwit terminology (…
f2e41d1 
…s/witness_program/witness_script/)

8a2b58d test: fix segwit terminology (s/witness_program/witness_script/) (Sebastian Falbesoner)

Pull request description:

  This PR fixes wrong uses of the term "witness program", which according to [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Witness_program)  is defined as follows:
  > A scriptPubKey (or redeemScript 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".**

  In most cases where "witness program" is used in tests (concerns comments, variable names and in one instance even a function name) what we really want to denote is the "witness script". Thanks to [MarcoFalke for pointing this out in a review comment](bitcoin/bitcoin#22363 (comment))!

  Some historical background: At the time when the P2P segwit tests were first introduced (commit 330b0f3, PR #8149), the term "witness program" was not used consistently in BIP141: https://bitcoin.stackexchange.com/questions/46451/what-is-the-precise-definition-of-witness-program
  This was fixed in PR bitcoin/bips#416 later.

  So in some way, this PR can be seen as a very late follow-up to the BIP141 fix that also reflects these changes in the tests.

ACKs for top commit:
  josibake:
    tACK bitcoin/bitcoin@8a2b58d

Tree-SHA512: f36bb9e53d1b54b86bfa87ec12f33e3ebca64b5f59d97e9662fe35ba12c25e1c9a4f93a5425d0eaa3879dce9e50368d345555b927bfab76945511f873396892b
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 1, 2021
Merge bitcoin#22429: test: refactor: fix segwit terminology (s/witnes…
37caae2 
…s_program/witness_script/)

8a2b58d test: fix segwit terminology (s/witness_program/witness_script/) (Sebastian Falbesoner)

Pull request description:

  This PR fixes wrong uses of the term "witness program", which according to [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Witness_program)  is defined as follows:
  > A scriptPubKey (or redeemScript 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".**

  In most cases where "witness program" is used in tests (concerns comments, variable names and in one instance even a function name) what we really want to denote is the "witness script". Thanks to [MarcoFalke for pointing this out in a review comment](bitcoin#22363 (comment))!

  Some historical background: At the time when the P2P segwit tests were first introduced (commit 330b0f3, PR bitcoin#8149), the term "witness program" was not used consistently in BIP141: https://bitcoin.stackexchange.com/questions/46451/what-is-the-precise-definition-of-witness-program
  This was fixed in PR bitcoin/bips#416 later.

  So in some way, this PR can be seen as a very late follow-up to the BIP141 fix that also reflects these changes in the tests.

ACKs for top commit:
  josibake:
    tACK bitcoin@8a2b58d

Tree-SHA512: f36bb9e53d1b54b86bfa87ec12f33e3ebca64b5f59d97e9662fe35ba12c25e1c9a4f93a5425d0eaa3879dce9e50368d345555b927bfab76945511f873396892b
JaredTate pushed a commit to DigiByte-Core/digibyte that referenced this pull request Jul 9, 2025
@JaredTate
Merge bitcoin/bitcoin#22429: test: refactor: fix segwit terminology (…
3743953 
…s/witness_program/witness_script/)

8a2b58d test: fix segwit terminology (s/witness_program/witness_script/) (Sebastian Falbesoner)

Pull request description:

  This PR fixes wrong uses of the term "witness program", which according to [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Witness_program)  is defined as follows:
  > A scriptPubKey (or redeemScript 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".**

  In most cases where "witness program" is used in tests (concerns comments, variable names and in one instance even a function name) what we really want to denote is the "witness script". Thanks to [MarcoFalke for pointing this out in a review comment](bitcoin/bitcoin#22363 (comment))!

  Some historical background: At the time when the P2P segwit tests were first introduced (commit 330b0f3, PR #8149), the term "witness program" was not used consistently in BIP141: https://bitcoin.stackexchange.com/questions/46451/what-is-the-precise-definition-of-witness-program
  This was fixed in PR bitcoin/bips#416 later.

  So in some way, this PR can be seen as a very late follow-up to the BIP141 fix that also reflects these changes in the tests.

ACKs for top commit:
  josibake:
    tACK bitcoin/bitcoin@8a2b58d

Tree-SHA512: f36bb9e53d1b54b86bfa87ec12f33e3ebca64b5f59d97e9662fe35ba12c25e1c9a4f93a5425d0eaa3879dce9e50368d345555b927bfab76945511f873396892b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Proposed BIP modification

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jl2012 @luke-jr @sipa @CodeShark