test: test OP_CSV empty stack fail in feature_csv_activation.py #17921

pull theStack wants to merge 3 commits into bitcoin:master from theStack:20200113-test-check-for-empty-stack-op_csv changing 1 files +33 −12
  1. theStack commented at 11:18 PM on January 13, 2020: member

    Adds an empty stack failure check for OP_CSV (BIP112) to the functional test feature_csv_activation.py by prepending a valid scriptSig with OP_CHECKSEQUENCEVERIFY. If BIP112 is inactive, the operator just behaves as a NOP (for both tx versions 1 and 2) and the transaction remains valid -- if it is active, the tx is invalid due to an empty stack (for both tx versions 1 and 2, as well).

  2. fanquake added the label Tests on Jan 13, 2020
  3. fanquake requested review from instagibbs on Jan 14, 2020
  4. in test/functional/feature_csv_activation.py:191 in e8f1227311 outdated 
     182 | @@ -175,15 +183,15 @@ def run_test(self):
 183 |          self.log.info("Generate blocks in the past for coinbase outputs.")
 184 |          long_past_time = int(time.time()) - 600 * 1000  # enough to build up to 1000 blocks 10 minutes apart without worrying about getting into the future
 185 |          self.nodes[0].setmocktime(long_past_time - 100)  # enough so that the generated blocks will still all be before long_past_time
 186 | -        self.coinbase_blocks = self.nodes[0].generate(1 + 16 + 2 * 32 + 1)  # 82 blocks generated for inputs
 187 | +        self.coinbase_blocks = self.nodes[0].generate(1 + 16 + 2 * 32 + 2)  # 83 blocks generated for inputs
    instagibbs commented at 1:54 PM on January 14, 2020:

    since we're touching this line: wat

    jimmysong commented at 4:24 PM on January 14, 2020:

    might be worth putting 83 in a variable

  5. in test/functional/feature_csv_activation.py:242 in e8f1227311 outdated 
     233 | @@ -224,7 +234,7 @@ def run_test(self):
 234 |          self.tip = int(inputblockhash, 16)
 235 |          self.tipheight += 1
 236 |          self.last_block_time += 600
 237 | -        assert_equal(len(self.nodes[0].getblock(inputblockhash, True)["tx"]), 82 + 1)
 238 | +        assert_equal(len(self.nodes[0].getblock(inputblockhash, True)["tx"]), 83 + 1)
    instagibbs commented at 1:55 PM on January 14, 2020:

    magic numbers :(

  6. in test/functional/feature_csv_activation.py:306 in e8f1227311 outdated 
     302 | @@ -289,11 +303,12 @@ def run_test(self):
 303 |          self.log.info("Test version 2 txs")
 304 |  
 305 |          success_txs = []
 306 | -        # add BIP113 tx and -1 CSV tx
 307 | +        # add BIP113 tx, -1 CSV tx and empty stack CSV tx
    instagibbs commented at 1:57 PM on January 14, 2020:

    s/stack CSV tx/stack CSV tx should succeed/

    to mirror the comments below doing the opposite

    theStack commented at 4:28 PM on January 14, 2020:

    Done.

  7. in test/functional/feature_csv_activation.py:286 in e8f1227311 outdated 
     282 |          self.log.info("Pre-Soft Fork Tests. All txs should pass.")
 283 |          self.log.info("Test version 1 txs")
 284 |  
 285 |          success_txs = []
 286 | -        # add BIP113 tx and -1 CSV tx
 287 | +        # add BIP113 tx, -1 CSV tx and empty stack CSV tx
    instagibbs commented at 1:57 PM on January 14, 2020:

    s/stack CSV tx/stack CSV tx should succeed/

    to mirror the comments below doing the opposite

    theStack commented at 4:28 PM on January 14, 2020:

    Done.

  8. in test/functional/feature_csv_activation.py:285 in e8f1227311 outdated 
     272 | @@ -263,18 +273,22 @@ def run_test(self):
 273 |          # -1 OP_CSV OP_DROP input
 274 |          bip112tx_special_v1 = create_bip112special(self.nodes[0], bip112specialinput, 1, self.nodeaddress)
 275 |          bip112tx_special_v2 = create_bip112special(self.nodes[0], bip112specialinput, 2, self.nodeaddress)
 276 | +        # (empty stack) OP_CSV input
 277 | +        bip112tx_emptystack_v1 = create_bip112emptystack(self.nodes[0], bip112emptystackinput, 1, self.nodeaddress)
    instagibbs commented at 1:59 PM on January 14, 2020:

    not for this PR but I think a more self-describing variable would be something like bip112tx_emptystack_inert and bip112tx_emptystack_active

    theStack commented at 4:10 PM on January 14, 2020:

    Note that the suffixes 'v1' and 'v2' don't say anything about whether BIP112 is already activated or not, it merely describes what the version field of the tx is set to.

    instagibbs commented at 4:13 PM on January 14, 2020:

    that's still an indirect way of naming it for the reader, not your fault of course

  9. in test/functional/feature_csv_activation.py:401 in e8f1227311 outdated 
     395 | @@ -381,8 +396,9 @@ def run_test(self):
 396 |          self.log.info("BIP 112 tests")
 397 |          self.log.info("Test version 1 txs")
 398 |  
 399 | -        # -1 OP_CSV tx should fail
 400 | +        # -1 OP_CSV tx and (empty stack) OP_CSV tx should fail
 401 |          self.send_blocks([self.create_test_block([bip112tx_special_v1])], success=False)
 402 | +        self.send_blocks([self.create_test_block([bip112tx_emptystack_v1])], success=False)
    instagibbs commented at 2:01 PM on January 14, 2020:

    Let's check the reject reason

  10. in test/functional/feature_csv_activation.py:421 in e8f1227311 outdated 
     415 | @@ -400,8 +416,9 @@ def run_test(self):
 416 |  
 417 |          self.log.info("Test version 2 txs")
 418 |  
 419 | -        # -1 OP_CSV tx should fail
 420 | +        # -1 OP_CSV tx and (empty stack) OP_CSV tx should fail
 421 |          self.send_blocks([self.create_test_block([bip112tx_special_v2])], success=False)
 422 | +        self.send_blocks([self.create_test_block([bip112tx_emptystack_v2])], success=False)
    instagibbs commented at 2:01 PM on January 14, 2020:

    Let's check the reject reason

  11. instagibbs commented at 2:02 PM on January 14, 2020: member

    Your changes look ok I guess, but the magic numbers in mining are throwing me off.

    I think in general this test needs some loving, but I think if the magic mining numbers are explained and we check for specific reject reasons this is an improvement

  12. theStack commented at 3:56 PM on January 14, 2020: member

    @instagibbs: Thanks for reviewing! Before I get into action: should I really put in all those suggested changes (magic numbers elimination/explanation, reject reason checking, adding empty stack test case) in one single PR? I feel it may make more sense to start off with a new PR that only includes reject reason checking for all places (not just the ones concerning the new test add here) first, and eliminating/explaining magic numbers, and only then continue here with adding the test, so that the "improve the CSV test in general" and "add a new concrete CSV subtest" are two separate parts that are independent and make reviewing easier. Thoughts on this? Is just dividing it up here in two commits enough?

  13. instagibbs commented at 4:02 PM on January 14, 2020: member

    Minimally I want to know that it's being rejected for the right reasons, not "oops the script is wrong in a different way".

    Secondary is the magic numbers floating around, but that can be held off for now(though I'd really appreciate it, magic numbers in tests make them very brittle and hard to understand).

  14. in test/functional/feature_csv_activation.py:199 in e8f1227311 outdated 
     192 |          self.tip = int(self.nodes[0].getbestblockhash(), 16)
 193 |          self.nodeaddress = self.nodes[0].getnewaddress()
 194 |  
 195 |          # Activation height is hardcoded
 196 | -        test_blocks = self.generate_blocks(345)
 197 | +        test_blocks = self.generate_blocks(344)
    jimmysong commented at 4:26 PM on January 14, 2020:

    what's the justification for this change? Is the CSV activation at block 344 instead of 345?

    theStack commented at 6:28 PM on January 14, 2020:

    The CSV activation height didn't change (is hardcoded as 432 in regtest mode), but since we are generating one coinbase block more, one block less has to be generated at this point to reach the desired height -- which turned out to be the CSV activation height minus five. Tried to get rid of most magic numbers in commit https://github.com/bitcoin/bitcoin/pull/17921/commits/93f5ee06e5cc620fdea5e990b46f3d23165cd9b0.

  15. theStack force-pushed on Jan 14, 2020
  16. jimmysong commented at 4:29 PM on January 14, 2020: contributor

    Would like some documentation on some of the magic numbers. Otherwise, looks good.

  17. theStack commented at 6:30 PM on January 14, 2020: member

    @jimmysong: Thanks for reviewing! Feels funny to have ones code reviewed by the author of a book that I am just reading right now :joy:

  18. jimmysong commented at 6:35 PM on January 14, 2020: contributor

    glad you're reading it!

  19. DrahtBot commented at 9:46 PM on January 18, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17959 (test: check specific reject reasons in feature_csv_activation.py 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.

  20. DrahtBot commented at 5:55 AM on February 28, 2020: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  21. DrahtBot added the label Needs rebase on Feb 28, 2020
  22. test: test OP_CSV empty stack fail in feature_csv_activation.py
    With BIP112 activated, the operation OP_CHECKSEQUENCEVERIFY (former OP_NOP3)
leads to script interpreter termination with an error if one of the following
conditions is true:
    -> stack is empty
    -> top item on stack is negative (< 0)
    -> top item on stack has disable flag unset and at least one of
       four other conditions is true (contains the core CSV logic)

This commits adds the missing empty stack failure test to the functional test
by prepending a valid scriptSig with just OP_CHECKSEQUENCEVERIFY. If BIP112 is
inactive, the operator just behaves as a NOP (for both tx versions 1 and 2) and
the transaction remains valid -- if it is active, the tx is invalid due to an
empty stack (for both tx versions 1 and 2, as well).
    cbd345a75c
  23. test: check for OP_CSV empty stack fail reject reason in feature_csv_activation.py 09f706ab8e
  24. test: eliminiated magic numbers in feature_csv_activation.py 5ffaf883b9
  25. theStack force-pushed on Feb 28, 2020
  26. theStack commented at 8:08 AM on February 28, 2020: member

    Rebased.

  27. fanquake removed the label Needs rebase on Feb 28, 2020
  28. MarcoFalke merged this on Feb 28, 2020
  29. MarcoFalke closed this on Feb 28, 2020
  30. MarcoFalke referenced this in commit 9aa8145bc0 on Feb 28, 2020
  31. theStack deleted the branch on Dec 1, 2020
  32. PiRK referenced this in commit e2a206329f on Jan 5, 2021
  33. PiRK referenced this in commit 9053b88b1c on Apr 18, 2021
  34. MarcoFalke referenced this in commit 83c715415a on Apr 19, 2021
  35. sidhujag referenced this in commit 91366bf683 on Apr 19, 2021
  36. windsok referenced this in commit 4ccb122979 on Apr 23, 2021
  37. DrahtBot locked this on Feb 15, 2022
Contributors
theStackinstagibbsjimmysongDrahtBot
Labels
Tests
Linked (view graph)
#17959 test: check specific reject reasons in feature_csv_activation.py#20857 test: update documentation in feature_csv_activation.py
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 21:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me