[test] more specific feature_segwit test error messages and fixing incorrect comments #18384

pull glozow wants to merge 1 commits into bitcoin:master from glozow:segwit-test-errormsg changing 1 files +24 −24
  1. glozow commented at 5:56 PM on March 19, 2020: member

    Followup to this comment on functional test feature_segwit.py verifying that unsigned witness transactions are invalid.

    (1) Changes 8 error messages from "mandatory-script-verify-flag" to "non-mandatory-script-verify-flag" and with more specific error messages. (2) Edits comments that incorrectly describe the test, namely that the v variable corresponds to using P2WSH versus P2WPKH, not witness versions.

  2. DrahtBot added the label Tests on Mar 19, 2020
  3. theStack commented at 10:28 AM on March 24, 2020: member

    Concept ACK -- it's definitely a good thing if error assertions get more specific. Are there more assertion strings in the same file that could be improved in the course of this PR?

  4. glozow commented at 11:00 PM on March 24, 2020: member

    Concept ACK -- it's definitely a good thing if error assertions get more specific. Are there more assertion strings in the same file that could be improved in the course of this PR?

    Actually quite a few of thefeature_segwit.py tests just have "mandatory-script-verify-flag". I'm a beginner so was just looking to address comments I see, but definitely seems a good idea to see if the other tests in the file can be improved in the same way.

    Also noticed in p2p_segwit.py, most of the tests don't have error messages at all.

  5. glozow renamed this:
    [test] more specific feature_segwit test error message
    [test] more specific feature_segwit test error messages
    on Mar 28, 2020
  6. glozow force-pushed on Mar 31, 2020
  7. glozow force-pushed on Apr 1, 2020
  8. amitiuttarwar commented at 11:01 PM on April 6, 2020: contributor

    Thanks for these test improvements! The code looks good to me, but I have some stylistic suggestions-

    • The PR description gets added to the merge commit (you can check out recent commits).. so I'd recommend keeping it focused on describing the high level functionality of the PR.
    • Specific commit-level explanations can be on the commit messages themselves. Leaving the hashes out of the description also has the benefit of not needing to update every time you force push. I suspect your current commit messages are the product of some commits being squashed together, so I'd recommend revisiting to make them have a one-line description, followed by any additional description you think is helpful. here's an example of the rough format I like to use: https://github.com/bitcoin/bitcoin/pull/18038/commits/e82fdd329c812623c6e8a9022322fe43e1000c5b
  9. glozow renamed this:
    [test] more specific feature_segwit test error messages
    [test] more specific feature_segwit test error messages and fixing incorrect comments
    on Apr 7, 2020
  10. glozow commented at 8:21 PM on April 7, 2020: member

    Thanks for these test improvements! The code looks good to me, but I have some stylistic suggestions-

    • The PR description gets added to the merge commit (you can check out recent commits).. so I'd recommend keeping it focused on describing the high level functionality of the PR.
    • Specific commit-level explanations can be on the commit messages themselves. Leaving the hashes out of the description also has the benefit of not needing to update every time you force push. I suspect your current commit messages are the product of some commits being squashed together, so I'd recommend revisiting to make them have a one-line description, followed by any additional description you think is helpful. here's an example of the rough format I like to use: e82fdd3

    Thanks!! Cleaned up the description.

  11. glozow force-pushed on Apr 7, 2020
  12. glozow force-pushed on Apr 7, 2020
  13. in test/functional/feature_segwit.py:120 in de6fc36cf4 outdated 
     116 | @@ -117,8 +117,12 @@ def run_test(self):
 117 |  
 118 |          balance_presetup = self.nodes[0].getbalance()
 119 |          self.pubkey = []
 120 | -        p2sh_ids = []  # p2sh_ids[NODE][VER] is an array of txids that spend to a witness version VER pkscript to an address for NODE embedded in p2sh
 121 | -        wit_ids = []  # wit_ids[NODE][VER] is an array of txids that spend to a witness version VER pkscript to an address for NODE via bare witness
 122 | +        # p2sh_ids[NODE][VER=0] is an array of txids that spend to P2WPKH scripts to an address for NODE embedded in p2sh
    amitiuttarwar commented at 11:23 PM on April 7, 2020:

    maybe instead of VER you can say TYPE since 0 -> P2WPKH and 1 -> P2WSH in both cases?

    glozow commented at 11:45 PM on April 7, 2020:

    This is very true haha, changed!

  14. glozow force-pushed on Apr 7, 2020
  15. glozow force-pushed on Apr 7, 2020
  16. amitiuttarwar commented at 11:46 PM on April 7, 2020: contributor

    utACK b844fe742ccf683dd41d15e628423c130e9db1f1

  17. in test/functional/feature_segwit.py:199 in b844fe742c outdated 
     194 | @@ -195,8 +195,8 @@ def run_test(self):
 195 |          self.log.info("Verify witness txs without witness data are invalid after the fork")
 196 |          self.fail_accept(self.nodes[2], 'non-mandatory-script-verify-flag (Witness program hash mismatch)', wit_ids[NODE_2][WIT_V0][2], sign=False)
 197 |          self.fail_accept(self.nodes[2], 'non-mandatory-script-verify-flag (Witness program was passed an empty witness)', wit_ids[NODE_2][WIT_V1][2], sign=False)
 198 | -        self.fail_accept(self.nodes[2], 'non-mandatory-script-verify-flag (Witness program hash mismatch)', p2sh_ids[NODE_2][WIT_V0][2], sign=False, redeem_script=witness_script(False, self.pubkey[2]))
 199 | -        self.fail_accept(self.nodes[2], 'non-mandatory-script-verify-flag (Witness program was passed an empty witness)', p2sh_ids[NODE_2][WIT_V1][2], sign=False, redeem_script=witness_script(True, self.pubkey[2]))
    MarcoFalke commented at 4:33 PM on April 8, 2020:

    Why are those lines changed to remove the named arguments? That makes the tests harder to read, I think.

    glozow commented at 7:19 PM on April 8, 2020:

    Oops! Must be leftover from me trying to shorten the lines or something. Fixed now.

  18. in test/functional/feature_segwit.py:181 in b844fe742c outdated 
     182 | +        self.fail_accept(self.nodes[0], "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_0][WIT_V1][0], sign=False)
 183 |          # unsigned with redeem script
 184 | -        self.fail_accept(self.nodes[0], "mandatory-script-verify-flag", p2sh_ids[NODE_0][WIT_V0][0], False, witness_script(False, self.pubkey[0]))
 185 | -        self.fail_accept(self.nodes[0], "mandatory-script-verify-flag", p2sh_ids[NODE_0][WIT_V1][0], False, witness_script(True, self.pubkey[0]))
 186 | +        self.fail_accept(self.nodes[0], "non-mandatory-script-verify-flag (Witness program hash mismatch)", p2sh_ids[NODE_0][WIT_V0][0], False, witness_script(False, self.pubkey[0]))
 187 | +        self.fail_accept(self.nodes[0], "non-mandatory-script-verify-flag (Witness program was passed an empty witness)", p2sh_ids[NODE_0][WIT_V1][0], False, witness_script(True, self.pubkey[0]))
    MarcoFalke commented at 4:33 PM on April 8, 2020:

    nit: could add named arguments to the bool False to clarify its meaning?

    glozow commented at 7:22 PM on April 8, 2020:

    added

  19. in test/functional/feature_segwit.py:161 in b844fe742c outdated 
     156 | @@ -157,8 +157,8 @@ def run_test(self):
 157 |          self.skip_mine(self.nodes[2], p2sh_ids[NODE_2][WIT_V1][0], True)  # block 427
 158 |  
 159 |          self.log.info("Verify unsigned p2sh witness txs without a redeem script are invalid")
 160 | -        self.fail_accept(self.nodes[2], "mandatory-script-verify-flag", p2sh_ids[NODE_2][WIT_V0][1], False)
 161 | -        self.fail_accept(self.nodes[2], "mandatory-script-verify-flag", p2sh_ids[NODE_2][WIT_V1][1], False)
 162 | +        self.fail_accept(self.nodes[2], "script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][WIT_V0][1], False)
 163 | +        self.fail_accept(self.nodes[2], "script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][WIT_V1][1], False)
    MarcoFalke commented at 4:34 PM on April 8, 2020:

    Same here, since you touch this line anyway

    glozow commented at 7:22 PM on April 8, 2020:

    added

  20. MarcoFalke approved
  21. MarcoFalke commented at 4:35 PM on April 8, 2020: member

    Concept ACK. Will test after the nits are addressed in some way.

  22. glozow force-pushed on Apr 8, 2020
  23. glozow commented at 7:22 PM on April 8, 2020: member

    Fixed named variables and added some for readability.

    Also realized something that I should have fixed originally - changed the constant names from WIT_V0 to P2WPKH and WIT_V1 to P2WSH. Doesn't change much, but adds some lines to the diff.

  24. glozow force-pushed on Apr 8, 2020
  25. glozow force-pushed on Apr 15, 2020
  26. glozow requested review from MarcoFalke on Apr 21, 2020
  27. in test/functional/feature_segwit.py:161 in 9b5710700e outdated 
     162 |  
 163 |          self.log.info("Verify unsigned p2sh witness txs without a redeem script are invalid")
 164 | -        self.fail_accept(self.nodes[2], "mandatory-script-verify-flag", p2sh_ids[NODE_2][WIT_V0][1], False)
 165 | -        self.fail_accept(self.nodes[2], "mandatory-script-verify-flag", p2sh_ids[NODE_2][WIT_V1][1], False)
 166 | +        self.fail_accept(self.nodes[2], "script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WPKH][1], sign=False)
 167 | +        self.fail_accept(self.nodes[2], "script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WSH][1], sign=False)
    MarcoFalke commented at 2:34 PM on April 21, 2020:

    why are you removing the mandatory- prefix?

    glozow commented at 4:19 PM on April 21, 2020:

    Sorry about that, I'm such a rookie. Fixed now.

  28. MarcoFalke approved
  29. MarcoFalke commented at 2:36 PM on April 21, 2020: member

    Please add a newline in the commit message after the commit subject line and before the commit body. Otherwise the whole commit message will get parsed as a single (very long) line.

    ACK 9b5710700eb4b201d1934744d9358a6f8cac39af 🎠

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 9b5710700eb4b201d1934744d9358a6f8cac39af 🎠
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhpqAv+IGmbmMqZTm8QyZm+vxcayWuYrMdiJLTfXFI9zCTNK76ypGKG+jNZruSX
7xIQglyc3knygWVmaS2IO+5ZjhbHdHegI9KnO9yuSeUGiQ8Zy7Plz3BxTJC4JGD2
BzkcTPonHbnJCiqn+E2Ayu17i+SV/WhAVEKVV2HdHQ77YLyZZ0FP1D5D2FB26O4a
J9mWJLRsziCnP6ZigaH5VEQsZuVEYnRbbf6boPesfgnZVkzkaO0knn0myh7ng0FH
iFFdO7IVKJSD4EVlL70LuRM6Z1nq0SP1guO3uqJI3j7ZenZvhAyx+FqG9VKJcMGc
o92hLwIBajbru66glqJAC3zdnn/uGl9NzjYsDm9weQcmTbaibgxmfN6fDU/TB8p2
AcV5i77h7tyWSwLuxzwsb38sEVmc+xEZ/YspXcJCuh+mGe0EVyUDkVgnusA9piSW
L57gsTEV7xI5PN/8Ziq896p78FwWNLPY/D0HdGZ8W9BdGO4/Otk/++fvSz5ClKn9
K4Ifnan+
=os7e
-----END PGP SIGNATURE-----

    Timestamp of file with hash c4478981e5125a91a57c587c5dadab45769a9bcb45d4e1e34725040b83c50fb4 -

    </details>

  30. [test] add 8 error messages to feature_segwit and change version to type
    P2WPKH witness program without signature -> throws "hash mismatch" error
P2WSH witness program without signature -> throws "empty witness" error
same errors for P2SH_P2WPKH and P2SH_P2WSH respectively when passed redeemScript but no signature
P2SH_P2WPKH and P2SH_P2WSH with no signature fail with "Operation not valid with current stack size" when not signed due to missing input
change VER to TYPE and constants WIT_V0 to P2WPKH=0 and WIT_V1 to P2WSH=1
    3c21db7b78
  31. glozow force-pushed on Apr 21, 2020
  32. MarcoFalke commented at 5:40 PM on April 21, 2020: member

    ACK 3c21db7b78 🍾

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 3c21db7b78 🍾
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUj6jgwAn+6koQ12kSueU0EwEin1DsU9lO3sqRd2ddipOJ6ukF6KZv7J8q80pGXC
IVFZFz2EiPi8YdQgu8flJspxVHmiPtLOjDYG13Hr11xcUSAW2826NGrDgpSfD682
+B4xDqAxuVi4XUCXBw+dfU/fqpOPrq8X7cQU8+yOsMc2dU6SfrNPAilzVDZgIw83
WsfZoaSAQBQFdMCH9FQDeevKb+HRsFk4qLOnyR1aNbc6SuHAdYml/E2qEtUT9Gxz
UBgUUPqDsbY7mp1Quw5SAtymzdlcdgf6huR8iziZqTohB3NOLU9fmO3Nroh2tJDF
X7ifqvRPFMFnu/qsfQHj/tYe9sKnpRZfw8rnfRrIASwdMas/TDnJ5YZ4w6TetygO
SZAnExKsVl9wYMtDyMZiIyM235QdStFvHkAo74Er/WZrx18MH8bOjF/bx+c8wRTo
Z47ZqdE2ZdMZapYCL1afCR4eaR3xykf777nNUJkrv9CNGRScHhmVhoumhgsNjwbm
XEO1uuyz
=uVmn
-----END PGP SIGNATURE-----

    Timestamp of file with hash 8a7bc4cb6ed0942427a32e552c127a6f2587b468b9577256a3b3e97a51ba8190 -

    </details>

  33. MarcoFalke merged this on Apr 21, 2020
  34. MarcoFalke closed this on Apr 21, 2020
  35. glozow deleted the branch on May 25, 2020
  36. MarcoFalke locked this on Feb 15, 2022
Contributors
glozowtheStackamitiuttarwarMarcoFalke
Labels
Tests
Linked (view graph)
#15169 Parallelize CheckInputs() in AcceptToMemoryPool()
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-14 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me