test: refactor: fix segwit terminology (s/witness_program/witness_script/) #22429

1. 
   theStack commented at 4:02 pm on July 11, 2021: member

   This PR fixes wrong uses of the term “witness program”, which according to BIP141 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!

   Some historical background: At the time when the P2P segwit tests were first introduced (commit 330b0f31ee5719d94f9e52dfc83c5d82168241f9, 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 https://github.com/bitcoin/bips/pull/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.

2. test: fix segwit terminology (s/witness_program/witness_script/) 8a2b58db9e
3. fanquake added the label Tests on Jul 11, 2021
4. 
   MarcoFalke commented at 4:45 pm on July 11, 2021: member

   Could do as a scripted diff? :sweat_smile:
5. 
   theStack commented at 5:26 pm on July 11, 2021: member

   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:

   https://github.com/theStack/bitcoin/blob/8a2b58db9ee6a14d36b5d8e430b35f18e7c7b0c5/test/functional/p2p_segwit.py#L1810

   (Or, split up into two commits: one scripted-diff and one that changes “witness program” occurences manually; seems to be overkill though?)

6. ShubhamPalriwala approved
7. 
   ShubhamPalriwala commented at 5:30 am on July 22, 2021: contributor

   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.

8. 
   josibake commented at 12:54 pm on July 26, 2021: member

   tACK https://github.com/bitcoin/bitcoin/pull/22429/commits/8a2b58db9ee6a14d36b5d8e430b35f18e7c7b0c5

   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

9. hg333 approved
10. 
    DrahtBot commented at 2:38 am on August 1, 2021: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #22593 by theStack

    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.

11. 
    MarcoFalke commented at 2:59 pm on August 1, 2021: member

    cr ACK 8a2b58db9ee6a14d36b5d8e430b35f18e7c7b0c5
12. MarcoFalke merged this on Aug 1, 2021
13. MarcoFalke closed this on Aug 1, 2021

    ---

14. sidhujag referenced this in commit 37caae2f93 on Aug 1, 2021
15. DrahtBot locked this on Aug 18, 2022

Contributors
theStack MarcoFalke ShubhamPalriwala josibake hg333 DrahtBot

Labels
Tests