[tests] Change feature_csv_activation.py to use BitcoinTestFramework #11817

pull jnewbery wants to merge 5 commits into bitcoin:master from jnewbery:refactor_bip68-112-113-p2p changing 1 files +263 −303
  1. jnewbery commented at 4:26 pm on December 2, 2017: member

    Next step in #10603.

    • first four commits tidy up bip68-112-113-p2p.py
    • fifth commit removes usage of ComparisonTestFramework
  2. fanquake added the label Tests on Dec 2, 2017
  3. jnewbery force-pushed on Dec 14, 2017
  4. in test/functional/bip68-112-113-p2p.py:86 in e78b600be4 outdated 
    119+    if srlb:
120+        locktime |= SEQ_RANDOM_LOW_BIT
121+    return locktime
122+
123+def all_rlt_txs(txs):
124+    return [txs[tx]['tx'] for tx in txs]
    ryanofsky commented at 8:56 pm on December 20, 2017:

    In commit “Remove nested loops from bip68-112-113-p2p.py”

    Should be able to avoid a lookup with [tx[’tx’] for tx in txs.values()]

    jnewbery commented at 3:32 am on February 14, 2018:
    Yes, that’s better. Fixed
  5. in test/functional/bip68-112-113-p2p.py:140 in e78b600be4 outdated 
    136@@ -152,24 +137,18 @@ def create_test_block(self, txs, version=536870912):
137         return block
138 
139     def create_bip68txs(self, bip68inputs, txversion, locktime_delta=0):
140-        txs = []
141+        """Returns a dictionary of bip68 transactions with different bits set."""
    ryanofsky commented at 9:03 pm on December 20, 2017:

    In commit “Remove nested loops from bip68-112-113-p2p.py”

    It seems better to continue returning a list than a dictionary. A dictionary is just slower and introduces nondeterminism in the test for no reason.

    jnewbery commented at 9:42 pm on February 14, 2018:
    yes - returning a list is much better. Changed. Thanks
  6. ryanofsky commented at 9:06 pm on December 20, 2017: member

    Reviewed following non-#11771 commits:

    • utACK 117f261bfcff1249be96d66f48350338e16580e3 [tests] Change bip68-112-113-p2p.py to use BitcoinTestFramework
    • utACK 7424bb122997da17276526d074e6380f2e112e3f [tests] Move utility functions in bip68-112-113-p2p.py out of class.
    • utACK e78b600be47bf34c4271b217b616a77f8957e531 [tests] Remove nested loops from bip68-112-113-p2p.py
    • utACK 44db6e78cf6acefe07ff9f5edb5f9995c7b45d7a [tests] improve logging in bip68-112-113-p2p.py
    • utACK 9c513676921f93ff3f3886e8c6072fd4c3314249 [tests] fix flake8 nits in bip68-112-113-p2p.py
  7. jnewbery force-pushed on Feb 14, 2018
  8. jnewbery commented at 3:31 am on February 14, 2018: member

    Rebased now that #11771 has been merged.

    edit: Just realised that I have’t addressed Russ’s comment here: #11817 (review). Will do that later

  9. jnewbery force-pushed on Feb 14, 2018
  10. jnewbery commented at 9:41 pm on February 14, 2018: member
    ok, properly rebased and Russ’s comment addressed.
  11. in test/functional/feature_csv_activation.py:196 in 96b02a0f62 outdated 
    254+        self.nodes[0].setmocktime(long_past_time - 100)  # enough so that the generated blocks will still all be before long_past_time
255+        self.coinbase_blocks = self.nodes[0].generate(1 + 16 + 2 * 32 + 1)  # 82 blocks generated for inputs
256+        self.nodes[0].setmocktime(0)  # set time back to present so yielded blocks aren't in the future as we advance last_block_time
257+        self.tipheight = 82  # height of the next block to build
258         self.last_block_time = long_past_time
259         self.tip = int("0x" + self.nodes[0].getbestblockhash(), 0)
    conscott commented at 4:52 pm on February 16, 2018:
    To go along with @ryanofsky earlier comments in related PRs. This can be simplified to int(hash,16)
  12. in test/functional/feature_csv_activation.py:269 in 96b02a0f62 outdated 
    352 
353         self.nodes[0].setmocktime(self.last_block_time + 600)
354-        inputblockhash = self.nodes[0].generate(1)[0] # 1 block generated for inputs to be in chain at height 572
355+        inputblockhash = self.nodes[0].generate(1)[0]  # 1 block generated for inputs to be in chain at height 572
356         self.nodes[0].setmocktime(0)
357         self.tip = int("0x" + inputblockhash, 0)
    conscott commented at 4:52 pm on February 16, 2018:
    Same here. Can be int(hash,16)
  13. conscott commented at 4:53 pm on February 16, 2018: contributor
    Added small nits, but ACK.
  14. laanwj commented at 4:55 pm on February 16, 2018: member
    utACK 96b02a0f62a64819c0048357b9e8b9da91af195f
  15. laanwj requested review from MarcoFalke on Feb 16, 2018
  16. jnewbery renamed this:
    [tests] Change bip68-112-113-p2p to use BitcoinTestFramework
    [tests] Change feature_csv_activation.py to use BitcoinTestFramework
    on Feb 16, 2018
  17. jnewbery commented at 5:55 pm on February 16, 2018: member
    @conscott thanks for the review. I agree that int(hash, 16) is better, but I’ll leave that change out of this PR. The main aim is to change this test to use the BitcoinTestFramework. We can address further nits in a future PR.
  18. laanwj assigned MarcoFalke on Mar 6, 2018
  19. MarcoFalke commented at 0:29 am on March 14, 2018: member
    Needs rebase in the commit that moves the methods. Sorry for letting this sit for so long.
  20. jnewbery commented at 11:22 pm on March 15, 2018: member

    Sorry for letting this sit for so long.

    Not a problem. This is low priority, will rebase next week.

  21. [tests] fix flake8 nits in feature_csv_activation.py 6f7f5bc002
  22. [tests] improve logging in feature_csv_activation.py 2e511d5424
  23. jnewbery commented at 10:48 pm on March 18, 2018: member
    Rebased. Also addressed @conscott’s comments: #11817 (review) in the first commit.
  24. jnewbery force-pushed on Mar 18, 2018
  25. in test/functional/feature_csv_activation.py:150 in c5427d1f4e outdated 
    161+            tx = self.create_transaction(self.nodes[0], bip68inputs[i], self.nodeaddress, Decimal("49.98"))
162+            tx.nVersion = txversion
163+            tx.vin[0].nSequence = locktime + locktime_delta
164+            tx = self.sign_transaction(self.nodes[0], tx)
165+            tx.rehash()
166+            txs.append({'tx': tx, 'sdf': sdf, 'srhb': srhb, 'stf': stf, 'srlb': srlb})
    MarcoFalke commented at 9:31 pm on March 21, 2018:

    In commit c5427d1f4e3652a1b55968c752fb362b31996186

    The “flags” to indicate if the random high- or low-bit was set are never read. Might as well not include it in the object.

    jnewbery commented at 1:39 pm on March 22, 2018:
    Yep, good spot. I’ve removed the ‘srhb’ and ‘srlb’ keys.
  26. in test/functional/feature_csv_activation.py:179 in c5427d1f4e outdated 
    198+            if (varyOP_CSV):
199+                signtx.vin[0].scriptSig = CScript([locktime, OP_CHECKSEQUENCEVERIFY, OP_DROP] + list(CScript(signtx.vin[0].scriptSig)))
200+            else:
201+                signtx.vin[0].scriptSig = CScript([BASE_RELATIVE_LOCKTIME, OP_CHECKSEQUENCEVERIFY, OP_DROP] + list(CScript(signtx.vin[0].scriptSig)))
202+            tx.rehash()
203+            txs.append({'tx': signtx, 'sdf': sdf, 'srhb': srhb, 'stf': stf, 'srlb': srlb})
    MarcoFalke commented at 9:32 pm on March 21, 2018:
    Same here
    jnewbery commented at 1:40 pm on March 22, 2018:
    fixed as above
  27. in test/functional/feature_csv_activation.py:453 in c5427d1f4e outdated 
    461-        for b25 in range(2):
462-            for b22 in range(2):
463-                for b18 in range(2):
464-                    fail_txs.append(bip112txs_vary_OP_CSV_9_v2[0][b25][b22][b18])  # 16/16 of vary_OP_CSV_9
465-
466+        fail_txs = all_rlt_txs(bip112txs_vary_nSequence_9_v2)  # 16/16 of vary_nSequence_9
    MarcoFalke commented at 9:32 pm on March 21, 2018:

    In commit c5427d1f4e3652a1b55968c752fb362b31996186:

    Any reason you remove the trailing comments in all lines but not in this one?

    jnewbery commented at 1:39 pm on March 22, 2018:
    I missed this one. Now removed.
  28. in test/functional/feature_csv_activation.py:413 in cb5b047a6f outdated 
    418-        yield TestInstance([[self.create_test_block(bip68success_txs), True]])
419+        self.sync_blocks([self.create_test_block(bip68success_txs)])
420         self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
421         for tx in bip68heighttxs:
422-            yield TestInstance([[self.create_test_block([tx]), False]])
423+            self.sync_blocks([self.create_test_block([tx])], False)
    MarcoFalke commented at 9:52 pm on March 21, 2018:

    In commit cb5b047a6f6fb58afee:

    Nit: Sometimes you use named args for the bool, and sometimes you don’t. Might as well use the success=False everywhere.

    jnewbery commented at 1:39 pm on March 22, 2018:
    sounds good. Updated.
  29. MarcoFalke commented at 9:55 pm on March 21, 2018: member
    utACK cb5b047a6f6fb58afee, just some bike-shedding about formatting and comments. Feel free to ignore.
  30. sipa commented at 9:59 pm on March 21, 2018: member
    Concept ACK, and very weak utACK (read through the changes, not a Python expert).
  31. [tests] Remove nested loops from feature_csv_activation.py 
    Makes the test a lot clearer.
    0842edf9ee
  32. [tests] Move utility functions in feature_csv_activation.py out of class. db7ffb9d1b
  33. [tests] Change feature_csv_activation.py to use BitcoinTestFramework 12982682a6
  34. jnewbery force-pushed on Mar 22, 2018
  35. MarcoFalke commented at 4:28 pm on March 22, 2018: member
    re-utACK 12982682a602ee656ce74b5733747a05d70907f8 only changes were style changes that shouldn’t change behaviour.
  36. conscott commented at 12:33 pm on March 28, 2018: contributor
    Re-ACK 12982682a602ee656ce74b5733747a05d70907f8
  37. MarcoFalke merged this on Apr 1, 2018
  38. MarcoFalke closed this on Apr 1, 2018
  39. MarcoFalke referenced this in commit 2a09a78c08 on Apr 1, 2018
  40. jnewbery deleted the branch on Apr 2, 2018
  41. MarcoFalke referenced this in commit 9a2db3b3d5 on Apr 5, 2018
  42. UdjinM6 referenced this in commit e890851e7d on Jan 11, 2020
  43. ckti referenced this in commit 9342190ee6 on Mar 28, 2021
  44. MarcoFalke locked this on Sep 8, 2021


jnewbery ryanofsky conscott laanwj MarcoFalke sipa

Labels
Tests

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: 2024-09-29 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me