[Tests] Make p2p_segwit easier to debug #13467

jnewbery · 2018-06-14T01:34:25Z

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.

maflcko · 2018-06-14T17:08:06Z

test/functional/p2p_segwit.py

-        NUM_DROPS = 200 # 201 max ops per script!
-        NUM_OUTPUTS = 50
+        num_drops = 200  # 201 max ops per script!
+        num_outputs = 50


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.

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.

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

But fine, reverted to all caps.

maflcko · 2018-06-14T17:08:51Z

test/functional/p2p_segwit.py

+    CTxIn,
+    CTxInWitness,
+    CTxOut,
+    CTxWitness,


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.

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 · 2018-06-14T20:08:20Z

Note to reviewers: This pull request conflicts with the following ones:

tests: Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports. #13054 (tests: Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports. by practicalswift)
Bury bip9 deployments #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.

maflcko · 2018-06-21T18:23:24Z

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

jnewbery · 2018-06-21T19:30:01Z

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 · 2018-06-21T21:51:51Z

Comments addressed. Thanks for the review @MarcoFalke !

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

The subtest wrapper logs the name of the subtest.

jnewbery · 2018-06-29T19:45:52Z

rebased

conscott

Tested ACK e3aab29

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 · 2018-07-03T09:29:58Z

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 · 2018-07-05T15:53:46Z

utACK e3aab29

e3aab29 [tests] p2p_segwit: sync_blocks in subtest wrapper. (John Newbery) 55e8050 [tests] p2p_segwit: remove unnecessary arguments from subtests. (John Newbery) 25711c2 [tests] p2p_segwit: log and assert segwit status in subtest wrapper. (John Newbery) 6839863 [tests] p2p_segwit: Make sure each subtest leaves utxos for the next. (John Newbery) bfe3273 [tests] p2p_segwit: wrap subtests with subtest wrapper. (John Newbery) 2af4e39 [tests] p2p_segwit: re-order function definitions. (John Newbery) 94a0134 [tests] p2p_segwit: standardise comments/docstrings. (John Newbery) f7c7f8e [tests] p2p_segwit: Fix flake8 warnings. (John Newbery) Pull request description: `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. Tree-SHA512: 3650602b3ce9823dc968cc5f2e716757feadc3dbedb3605eb79bb3df91a6db8ae53431f253b440da690e3a8e9d76de84fad4368a2663aeb40e6b9427cf948870

maflcko · 2018-07-09T14:20:23Z

test/functional/p2p_segwit.py

    # Verify that future segwit upgraded transactions are non-standard,
    # but valid in blocks. Can run this before and after segwit activation.
    def test_segwit_versions(self):
        self.log.info("Testing standardness/consensus for segwit versions (0-16)")
        assert(len(self.utxo))
-        NUM_TESTS = 17 # will test OP_0, OP1, ..., OP_16
-        if (len(self.utxo) < NUM_TESTS):
+        num_tests = 17  # will test OP_0, OP1, ..., OP_16


in commit f7c7f8e [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.

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

maflcko

utACK e3aab29

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.

maflcko · 2018-07-09T14:21:03Z

test/functional/p2p_segwit.py

        # Ensure that we've tested a situation where we use SIGHASH_SINGLE with
        # an input index > number of outputs.
-        NUM_TESTS = 500
+        num_tests = 500


Will address in follow-up PR

maflcko · 2018-07-09T14:30:49Z

test/functional/p2p_segwit.py

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


Nit (unrelated):
In commit 94a0134 [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)

Will address in follow-up PR

maflcko · 2018-07-09T14:30:57Z

test/functional/p2p_segwit.py

    def advance_to_segwit_active(self):
+        """Mine enough blocks to activate segwit."""
+        # TODO: we could verify that activation only happens at the right threshold


Will address in follow-up PR

maflcko · 2018-07-09T14:45:40Z

test/functional/p2p_segwit.py

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


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

Unrelated change

maflcko · 2018-07-09T14:46:30Z

test/functional/p2p_segwit.py

-        return
+
+    def run_test(self):
+        # Setup the p2p connections and start up the network thread.


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

Unrelated addition: Tests are not aware of a "network thread"

Will address in follow-up PR

maflcko · 2018-07-09T18:21:27Z

test/functional/p2p_segwit.py

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


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

Unrelated non-moveonly removal

maflcko · 2018-07-09T18:23:14Z

test/functional/p2p_segwit.py

-
-    def test_standardness_v0(self, segwit_activated):
-        """Test V0 txout standardness.
-


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

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

maflcko · 2018-07-09T18:24:40Z

test/functional/p2p_segwit.py

-
-    def test_segwit_versions(self):
-        """Test validity of future segwit version transactions.
-


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

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

jnewbery · 2018-07-09T21:44:57Z

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

eeeef80 qa: Fix some TODOs in p2p_segwit (MarcoFalke) Pull request description: * I believe we don't need to redundantly test versionbits logic in every functional tests that tests a softfork deployment that is being done with versionbits. Thus, remove two `TODO`s that ask for that. * Replace another `TODO` with `wait_until`. * Some style fixups after #13467 Tree-SHA512: c7120404d50579d6f3b9092f1e259959190eeafe520231e3479c8c256a50bf7260ccc93f8301ac0e100c54037053f6849433ebb1c55607e01d94b9812e525083

fanquake added the Tests label Jun 14, 2018

Empact mentioned this pull request Jun 14, 2018

Intermittent Mac CI failure #13469

Closed

maflcko reviewed Jun 14, 2018

View reviewed changes

jnewbery force-pushed the tidy_up_p2p_segwit branch from 1b6730c to daad0c4 Compare June 21, 2018 21:51

DrahtBot mentioned this pull request Jun 26, 2018

qa: Remove need to handle the network thread in tests #13517

Merged

This was referenced Jun 27, 2018

qa: p2p_segwit intermittent failure #13523

Closed

qa: failure in p2p_segwit #11696

Closed

jnewbery added 8 commits June 29, 2018 15:32

[tests] p2p_segwit: Fix flake8 warnings.

f7c7f8e

[tests] p2p_segwit: standardise comments/docstrings.

94a0134

[tests] p2p_segwit: re-order function definitions.

2af4e39

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

[tests] p2p_segwit: wrap subtests with subtest wrapper.

bfe3273

The subtest wrapper logs the name of the subtest.

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

6839863

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

25711c2

[tests] p2p_segwit: remove unnecessary arguments from subtests.

55e8050

[tests] p2p_segwit: sync_blocks in subtest wrapper.

e3aab29

jnewbery force-pushed the tidy_up_p2p_segwit branch from daad0c4 to e3aab29 Compare June 29, 2018 19:45

conscott reviewed Jul 3, 2018

View reviewed changes

laanwj merged commit e3aab29 into bitcoin:master Jul 5, 2018

Bushstar mentioned this pull request Jul 9, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#353

Merged

maflcko reviewed Jul 9, 2018

View reviewed changes

jnewbery deleted the tidy_up_p2p_segwit branch July 9, 2018 14:34

maflcko reviewed Jul 9, 2018

View reviewed changes

maflcko mentioned this pull request Jul 10, 2018

qa: Fix some TODOs in p2p_segwit #13626

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021


		def test_standardness_v0(self, segwit_activated):
		"""Test V0 txout standardness.


		def test_segwit_versions(self):
		"""Test validity of future segwit version transactions.

[Tests] Make p2p_segwit easier to debug #13467

[Tests] Make p2p_segwit easier to debug #13467

Uh oh!

Conversation

jnewbery commented Jun 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 21, 2018

Uh oh!

jnewbery commented Jun 21, 2018

Uh oh!

jnewbery commented Jun 21, 2018

Uh oh!

jnewbery commented Jun 29, 2018

Uh oh!

conscott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 3, 2018

Uh oh!

laanwj commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 9, 2018

Uh oh!

Uh oh!

DrahtBot commented Jun 14, 2018 •

edited

Loading

conscott left a comment •

edited

Loading

jnewbery Jul 13, 2018 •

edited

Loading