[Tests] Make p2p_segwit easier to debug

jnewbery commented at 1:34 am on June 14, 2018: member

p2p_segwit.py is a very long test, composed of multiple subtests. When it fails it’s difficult to debug for a couple of reasons:

Control flow jumps between different methods in the test class, so it’s a little difficult to follow the code.
state may be carried forward unintentionally from one subtest to the next.

Improve that by wrapping the subtests with a @subtest decorator which:

logs progress
asserts state after each subtest

As usual, I’ve also included a few commits which generally tidy up the test and improve style.

fanquake added the label Tests on Jun 14, 2018

in test/functional/p2p_segwit.py:555 in a7c0672e7d outdated

550@@ -492,42 +551,42 @@ def test_witness_block_size(self):
551         # The witness program will be a bunch of OP_2DROP's, followed by OP_TRUE.
552         # This should give us plenty of room to tweak the spending tx's
553         # virtual size.
554-        NUM_DROPS = 200 # 201 max ops per script!
555-        NUM_OUTPUTS = 50
556+        num_drops = 200  # 201 max ops per script!
557+        num_outputs = 50

MarcoFalke commented at 5:08 pm on June 14, 2018:

Imo we indicate “compile time” constants with upper case, so I’d prefer if they were left that way, since it makes it easier to read.

MarcoFalke commented at 6:22 pm on June 21, 2018:

C.f. https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style

Constant names are all uppercase, and use _ to separate words.

jnewbery commented at 9:35 pm on June 21, 2018:

That’s not fair! Those are C++ style guides.

But fine, reverted to all caps.

in test/functional/p2p_segwit.py:24 in a7c0672e7d outdated

27+    COutPoint,
28+    CTransaction,
29+    CTxIn,
30+    CTxInWitness,
31+    CTxOut,
32+    CTxWitness,

MarcoFalke commented at 5:08 pm on June 14, 2018:

Those are message types and not from mininode. I’d prefer to keep it the way it was over changing it in the wrong way.

jnewbery commented at 9:35 pm on June 21, 2018:

Thanks for pointing this out. These were previously being imported indirectly through the from mininode import * import, but you’re right that I should fix that to import from messages.py in this commit.

I’ve now put them in the right place.

DrahtBot commented at 8:08 pm on June 14, 2018: member

#13054 (tests: Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports. by practicalswift)
#12360 (Bury bip9 deployments by jnewbery)

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.

MarcoFalke commented at 6:23 pm on June 21, 2018: member

Concept ACK, but let’s try not to overshoot the target by pleasing some obscure style rules that contradict our style guide.

jnewbery commented at 7:30 pm on June 21, 2018: member

Concept ACK, but let’s try not to overshoot the target by pleasing some obscure style rules that contradict our style guide.

Sorry - will address your comments. Was travelling last week.

jnewbery force-pushed on Jun 21, 2018

jnewbery commented at 9:51 pm on June 21, 2018: member

Comments addressed. Thanks for the review @MarcoFalke !

[tests] p2p_segwit: Fix flake8 warnings. f7c7f8ecf3

[tests] p2p_segwit: standardise comments/docstrings. 94a0134a40

[tests] p2p_segwit: re-order function definitions.

This re-orders the defintions in p2p_segwit so subtests are
defined in the order that they're called.

2af4e398dc

[tests] p2p_segwit: wrap subtests with subtest wrapper.

The subtest wrapper logs the name of the subtest.

bfe32734de

[tests] p2p_segwit: Make sure each subtest leaves utxos for the next. 6839863d53

[tests] p2p_segwit: log and assert segwit status in subtest wrapper. 25711c2696

[tests] p2p_segwit: remove unnecessary arguments from subtests. 55e8050853

[tests] p2p_segwit: sync_blocks in subtest wrapper. e3aab295e7

jnewbery force-pushed on Jun 29, 2018

jnewbery commented at 7:45 pm on June 29, 2018: member

rebased

conscott commented at 9:24 am on July 3, 2018: contributor

Tested ACK e3aab295e7c91d08bd577f9fb05f6bb8e585870a

Nice cleanup.

As a follow up, what do you think about moving these functions back to mininode.py? It seems there are similiar patterns in other p2p tests, like here, which could be refactored.

jnewbery commented at 9:29 am on July 3, 2018: member

Thanks @conscott !

what do you think about moving these functions back to mininode.py?

Seems reasonable if they can be used by multiple tests, although blocktools.py might be a better place for them.

laanwj commented at 3:53 pm on July 5, 2018: member

utACK e3aab295e7c91d08bd577f9fb05f6bb8e585870a

laanwj merged this on Jul 5, 2018

laanwj closed this on Jul 5, 2018

laanwj referenced this in commit 3dc2dcfdfc on Jul 5, 2018

in test/functional/p2p_segwit.py:1211 in f7c7f8ecf3 outdated

1209     def test_segwit_versions(self):
1210         self.log.info("Testing standardness/consensus for segwit versions (0-16)")
1211         assert(len(self.utxo))
1212-        NUM_TESTS = 17 # will test OP_0, OP1, ..., OP_16
1213-        if (len(self.utxo) < NUM_TESTS):
1214+        num_tests = 17  # will test OP_0, OP1, ..., OP_16

MarcoFalke commented at 2:20 pm on July 9, 2018:

in commit f7c7f8ecf3 [tests] p2p_segwit: Fix flake8 warnings

You change a constant to lowercase. Imo this makes the code harder to read, as mentioned in my previous review.

jnewbery commented at 2:34 pm on July 9, 2018:

sorry - recapitalized the ones that you mentioned in your review. Missed this one.

in test/functional/p2p_segwit.py:1385 in f7c7f8ecf3 outdated

1378@@ -1328,19 +1379,19 @@ def test_signature_version_1(self):
1379         # Test combinations of signature hashes.
1380         # Split the utxo into a lot of outputs.
1381         # Randomly choose up to 10 to spend, sign with different hashtypes, and
1382-        # output to a random number of outputs.  Repeat NUM_TESTS times.
1383+        # output to a random number of outputs.  Repeat num_tests times.
1384         # Ensure that we've tested a situation where we use SIGHASH_SINGLE with
1385         # an input index > number of outputs.
1386-        NUM_TESTS = 500
1387+        num_tests = 500

MarcoFalke commented at 2:21 pm on July 9, 2018:

same here

jnewbery commented at 8:00 pm on July 13, 2018:

Will address in follow-up PR

in test/functional/p2p_segwit.py:395 in 94a0134a40 outdated

393-    # TODO: we could verify that lockin only happens at the right threshold of
394-    # signalling blocks, rather than just at the right period boundary.
395     def advance_to_segwit_lockin(self):
396+        """Mine enough blocks to lock in segwit, but don't activate."""
397+        # TODO: we could verify that lockin only happens at the right threshold of
398+        # signalling blocks, rather than just at the right period boundary.

MarcoFalke commented at 2:30 pm on July 9, 2018:

Nit (unrelated): In commit 94a0134a40 [tests] p2p_segwit: standardise comments/docstrings.

Could remove this todo, since it should be covered by the BIP9 tests. (No need to re-cover BIP9 logic in every softfork deployment that uses BIP9)

jnewbery commented at 8:00 pm on July 13, 2018:

Will address in follow-up PR

in test/functional/p2p_segwit.py:409 in 94a0134a40 outdated

403@@ -399,19 +404,23 @@ def advance_to_segwit_lockin(self):
404         self.nodes[0].generate(1)
405         assert_equal(get_bip9_status(self.nodes[0], 'segwit')['status'], 'locked_in')
406 
407-    # Mine enough blocks to activate segwit.
408-    # TODO: we could verify that activation only happens at the right threshold
409-    # of signalling blocks, rather than just at the right period boundary.
410     def advance_to_segwit_active(self):
411+        """Mine enough blocks to activate segwit."""
412+        # TODO: we could verify that activation only happens at the right threshold

MarcoFalke commented at 2:30 pm on July 9, 2018:

same here

jnewbery commented at 8:00 pm on July 13, 2018:

Will address in follow-up PR

jnewbery deleted the branch on Jul 9, 2018

in test/functional/p2p_segwit.py:44 in 2af4e398dc outdated

40@@ -41,7 +41,6 @@
41 from test_framework.mininode import (
42     P2PInterface,
43     mininode_lock,
44-    network_thread_start,

MarcoFalke commented at 2:45 pm on July 9, 2018:

in commit 2af4e398dc [tests] p2p_segwit: re-order function definitions.

Unrelated change

in test/functional/p2p_segwit.py:223 in 2af4e398dc outdated

217@@ -220,7 +218,77 @@ def update_witness_block_with_transactions(self, block, tx_list, nonce=0):
218         block.vtx.extend(tx_list)
219         add_witness_commitment(block, nonce)
220         block.solve()
221-        return
222+
223+    def run_test(self):
224+        # Setup the p2p connections and start up the network thread.

MarcoFalke commented at 2:46 pm on July 9, 2018:

In commit 2af4e398dc [tests] p2p_segwit: re-order function definitions.

Unrelated addition: Tests are not aware of a “network thread”

jnewbery commented at 8:01 pm on July 13, 2018:

Will address in follow-up PR

in test/functional/p2p_segwit.py:223 in 2af4e398dc outdated

217@@ -220,7 +218,77 @@ def update_witness_block_with_transactions(self, block, tx_list, nonce=0):
218         block.vtx.extend(tx_list)
219         add_witness_commitment(block, nonce)
220         block.solve()
221-        return

MarcoFalke commented at 6:21 pm on July 9, 2018:

In commit 2af4e39 [tests] p2p_segwit: re-order function definitions.

Unrelated non-moveonly removal

in test/functional/p2p_segwit.py:1143 in 2af4e398dc outdated

1501-            self.old_node.announce_tx_and_wait_for_getdata(block4.vtx[0])
1502-            assert(block4.sha256 not in self.old_node.getdataset)
1503-
1504-    def test_standardness_v0(self, segwit_activated):
1505-        """Test V0 txout standardness.
1506-

MarcoFalke commented at 6:23 pm on July 9, 2018:

In commit 2af4e398dc [tests] p2p_segwit: re-order function definitions.

Unrelated removal of trailing whitespace (should have been in one of the previous commits)

in test/functional/p2p_segwit.py:1223 in 2af4e398dc outdated

1581-        self.utxo.append(UTXO(tx3.sha256, 0, tx3.vout[0].nValue))
1582-        assert_equal(len(self.nodes[1].getrawmempool()), 0)
1583-
1584-    def test_segwit_versions(self):
1585-        """Test validity of future segwit version transactions.
1586-

MarcoFalke commented at 6:24 pm on July 9, 2018:

In commit 2af4e398dc [tests] p2p_segwit: re-order function definitions.

Unrelated removal of trailing whitespace (should have been in one of the previous commits)

MarcoFalke commented at 6:42 pm on July 9, 2018: member

utACK e3aab295e7c91d08bd577f9fb05f6bb8e585870

Excellent addition of the @subtest wrapper! In the past I used to splatter around those assertions manually.

Left some comments to prove I actually reviewed the individual commits.

jnewbery commented at 9:44 pm on July 9, 2018: member

Thanks for the review @MarcoFalke - I’ll try to address your comments in a follow-up PR.

MarcoFalke referenced this in commit 8f1106da58 on Jul 13, 2018

MarcoFalke locked this on Sep 8, 2021

[Tests] Make p2p_segwit easier to debug #13467