test: Remove confusing DUMMY_P2WPKH_SCRIPT #26277

1. 
   maflcko commented at 9:18 am on October 7, 2022: member

   It is confusing because, it is not a P2WPKH script, and it is nonstandard.

   See also https://github.com/bitcoin/bitcoin/pull/26265/files#r989827855

   Fix all issues by removing it, and also remove the no longer needed -acceptnonstdtxn setting from the test.

2. maflcko force-pushed on Oct 7, 2022
3. 
   theStack commented at 10:17 am on October 7, 2022: contributor

   Concept ACK

   An alternative to introducing another (magic-number) constant would be to directly use script_to_p2wsh_script(CScript([OP_TRUE]))) instead, though it’s slightly longer.

4. test: Remove confusing DUMMY_P2WPKH_SCRIPT fa8a305ddd
5. maflcko force-pushed on Oct 7, 2022
6. 
   maflcko commented at 11:13 am on October 7, 2022: member

   Thanks, pushed an alternative that only modifies a single file to reduce merge-conflicting lines.
7. DrahtBot added the label Tests on Oct 7, 2022
8. 
   DrahtBot commented at 12:20 pm on October 7, 2022: contributor

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #26265 (POLICY: Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes by instagibbs)

   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.

9. theStack approved
10. 
    theStack commented at 1:45 pm on October 7, 2022: contributor

    Code-review ACK fa8a305ddde15abc0df03a3af0a03b92405d68f6 📜
11. 
    instagibbs commented at 2:13 pm on October 7, 2022: member

    ACK https://github.com/bitcoin/bitcoin/pull/26277/commits/fa8a305ddde15abc0df03a3af0a03b92405d68f6

    All the replacements of scripts and injection of witnesses look correct, and allows simplification/removal of other tests and utils.

12. darosior changes_requested
13. 
    darosior commented at 9:01 am on October 8, 2022: member

    Looks like you didn’t actually remove DUMMY_P2WPKH_SCRIPT’s definition from test_framework/script_util.py?
14. 
    fanquake commented at 2:09 am on October 10, 2022: member

    Looks like you didn’t actually remove DUMMY_P2WPKH_SCRIPT’s definition from test_framework/script_util.py?

    Yep. Looks like that and DUMMY_2_P2WPKH_SCRIPT can be deleted there.

15. 
    maflcko commented at 7:20 am on October 10, 2022: member

    DUMMY_2_P2WPKH_SCRIPT is already unused, so it seems better to remove them in a pull unrelated to this. As removing both would also remove the comment, which may or may not want to be kept. I didn’t want to have the discussion here. The only goal here is to cleanup the consensus test.
16. fanquake merged this on Oct 10, 2022
17. fanquake closed this on Oct 10, 2022

    ---

18. maflcko deleted the branch on Oct 10, 2022
19. sidhujag referenced this in commit b99793eb33 on Oct 10, 2022
20. bitcoin locked this on Oct 10, 2023

Contributors
maflcko theStack DrahtBot instagibbs darosior fanquake

Labels
Tests