[qa] Overhaul p2p_compactblocks.py #15660

pull sdaftuar wants to merge 1 commits into bitcoin:master from sdaftuar:2019-03-refactor-p2p-compactblocks-2 changing 1 files +68 −139
  1. sdaftuar commented at 10:20 PM on March 24, 2019: member

    Remove tests of:

    • compactblock behavior in a simulated pre-segwit version of bitcoind This should have been removed a long time ago, as it is not generally necessary for us to test the behavior of old nodes (except perhaps if we want to test that upgrading from an old node to a new one behaves properly)

    • compactblock behavior during segwit upgrade (ie verifying that network behavior before and after activation was as expected) This is unnecessary to test now that segwit activation has already happened.

  2. sdaftuar commented at 10:21 PM on March 24, 2019: member

    This makes changes like that proposed in #15633 much easier to make (I have a relatively small diff to the test that would support that change in particular, should we want to adopt a different behavior).

  3. fanquake added the label Tests on Mar 24, 2019
  4. fanquake requested review from gmaxwell on Mar 24, 2019
  5. in test/functional/p2p_compactblocks.py:140 in 5bc755fdd1 outdated 
     136 |          assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.sha256)
 137 |          self.utxos.extend([[tx.sha256, i, out_value] for i in range(10)])
 138 |          return
 139 |  
 140 | +    # Send balance to a segwit output
 141 | +    def make_segwit_output(self, node):
    practicalswift commented at 8:52 AM on March 25, 2019:

    Nit: Since self is not used -- this could be a function instead of a method?

    jnewbery commented at 2:14 PM on March 25, 2019:

    I suggest you combine this with make_utxos(), since both are called only once.

    sdaftuar commented at 4:48 PM on March 29, 2019:

    Done.

    sdaftuar commented at 4:49 PM on March 29, 2019:

    Eliminated this in the latest version.

  6. in test/functional/p2p_compactblocks.py:97 in 5bc755fdd1 outdated 
      92 | @@ -94,11 +93,9 @@ def send_await_disconnect(self, message, timeout=30):
  93 |  class CompactBlocksTest(BitcoinTestFramework):
  94 |      def set_test_params(self):
  95 |          self.setup_clean_chain = True
  96 | -        # Node0 = pre-segwit, node1 = segwit-aware
  97 | -        self.num_nodes = 2
  98 | +        self.num_nodes = 1
  99 |          # This test was written assuming SegWit is activated using BIP9 at height 432 (3x confirmation window).
    jnewbery commented at 1:40 PM on March 25, 2019:

    This comment is no longer accurate and can be removed.

    sdaftuar commented at 4:48 PM on March 29, 2019:

    Done.

  7. in test/functional/p2p_compactblocks.py:143 in 5bc755fdd1 outdated 
     139 |  
 140 | +    # Send balance to a segwit output
 141 | +    def make_segwit_output(self, node):
 142 | +        if node.getbalance() < 2:
 143 | +            node.generate(101)
 144 | +        address = node.getnewaddress("bech32")
    jnewbery commented at 2:14 PM on March 25, 2019:

    The first argument to getnewaddress() is label (the second argument is address_type) so this doesn't actually create a bech32 address. Consider using named arguments to avoid this class of bug.

    Given that the test passes anyway, is it actually testing what you want?

    sdaftuar commented at 4:48 PM on March 29, 2019:

    Fixed.

  8. in test/functional/p2p_compactblocks.py:799 in 5bc755fdd1 outdated 
     802 |          self.log.info("Testing SENDCMPCT p2p message... ")
 803 | -        self.test_sendcmpct(self.nodes[0], self.test_node, 1)
 804 | -        sync_blocks(self.nodes)
 805 | -        self.test_sendcmpct(self.nodes[1], self.segwit_node, 2, old_node=self.old_node)
 806 | -        sync_blocks(self.nodes)
 807 | +        self.test_sendcmpct(self.nodes[0], self.segwit_node, 2, old_node=self.old_node)
    jnewbery commented at 2:17 PM on March 25, 2019:

    Since there's only one node under test now, all of these methods can drop the node argument.

    jnewbery commented at 2:40 PM on March 25, 2019:

    test_compct() always takes the same version so that argument can be dropped.

    Same is true for most of these subtest functions.

    sdaftuar commented at 4:48 PM on March 29, 2019:

    Done.

    sdaftuar commented at 4:48 PM on March 29, 2019:

    Simplified.

  9. in test/functional/p2p_compactblocks.py:788 in 5bc755fdd1 outdated 
     784 | @@ -793,126 +785,56 @@ def announce_cmpct_block(node, peer):
 785 |  
 786 |      def run_test(self):
 787 |          # Setup the p2p connections
 788 | -        self.test_node = self.nodes[0].add_p2p_connection(TestP2PConn())
 789 | -        self.segwit_node = self.nodes[1].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
 790 | -        self.old_node = self.nodes[1].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK)
 791 | +        self.segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
    jnewbery commented at 5:24 PM on March 25, 2019:

    Default network services are NODE_NETWORK | NODE_WITNESS so this argument doesn't need to be passed.

    sdaftuar commented at 4:49 PM on March 29, 2019:

    Removed.

  10. jnewbery commented at 8:28 PM on March 25, 2019: member

    utACK 5bc755fdd166e84d8606a613c379705b362f0bf2

    I've left some comments inline. One is a bug while the rest are style nits.

    Whilst reviewing this, a lot of tidy-ups suggested themselves to me, so I went ahead and made a branch here: https://github.com/jnewbery/bitcoin/tree/tidy_up_p2p_compactblocks. I think the result is quite a bit clearer and easier to follow than the existing p2p_compactblocks.py. I've split out the commits in a vaguely logical way so it should be pretty easy to review. Feel free to take any or all of those changes, or we can follow this up with another PR.

  11. sdaftuar force-pushed on Mar 29, 2019
  12. sdaftuar commented at 4:53 PM on March 29, 2019: member

    I've taken a bunch of the feedback and cherry-picked a few commits from @jnewbery's branch, and updated this PR. Old version is here: https://github.com/sdaftuar/bitcoin/commits/15660.1

  13. jnewbery commented at 8:51 PM on March 29, 2019: member

    tested ACK f97b82149a5cda1443f8f1d085b95c7987b9ba1f. Please squash commits.

  14. [qa] Overhaul p2p_compactblocks.py
    Remove tests of:
 - compactblock behavior in a simulated pre-segwit version of bitcoind
   This should have been removed a long time ago, as it is not generally
   necessary for us to test the behavior of old nodes (except perhaps if we
   want to test that upgrading from an old node to a new one behaves properly)

 - compactblock behavior during segwit upgrade (ie verifying that network
   behavior before and after activation was as expected)
   This is unnecessary to test now that segwit activation has already happened.

Includes changes by John Newbery.
    7813eb1db1
  15. sdaftuar force-pushed on Apr 1, 2019
  16. sdaftuar commented at 9:10 PM on April 1, 2019: member

    Squashed.

  17. jnewbery commented at 3:22 PM on April 2, 2019: member

    utACK 7813eb1db132c023902ad945995cc32a325893ca

    Travis failure is unrelated. I've restarted it.

  18. MarcoFalke added this to the milestone 0.19.0 on Apr 2, 2019
  19. MarcoFalke referenced this in commit efbc86733a on Apr 6, 2019
  20. MarcoFalke merged this on Apr 6, 2019
  21. MarcoFalke closed this on Apr 6, 2019
  22. jonatack referenced this in commit 7f2009f567 on Apr 7, 2019
  23. DrahtBot locked this on Dec 16, 2021
Contributors
sdaftuarpracticalswiftjnewbery


gmaxwell

Labels
Tests

Milestone
0.19.0

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