test: refactor: fix segwit terminology (s/witness_program/witness_script/) #22429
Conversation
|
Could do as a scripted diff? 😅 |
That'd work well for renaming variables and constants (s/witness_program/witness_script/, s/MAX_PROGRAM_LENGTH/MAX_WITNESS_SCRIPT_LENGTH/), but not for longer comments, where "witness program" sometimes needs to remain unchanged. E.g. here, if I understood correctly: (Or, split up into two commits: one scripted-diff and one that changes "witness program" occurences manually; seems to be overkill though?) |
There was a problem hiding this comment.
Concept ACK!
This PR makes sure that developers do not get confused over the witness script and witness program concepts. This especially will help people who are reading the codebase and trying to understand its workings without getting confused between these two terms.
|
tACK 8a2b58d I compiled and ran the functional tests to ensure everything is still passing. +1 on the concept, consistent naming is super helpful for new people on-boarding to the codebase |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones: If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
cr ACK 8a2b58d |
…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
This PR fixes wrong uses of the term "witness program", which according to BIP141 is defined as follows:
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!
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.