[tests] Change invalidtxrequest to use BitcoinTestFramework #11771

jnewbery · 2017-11-27T16:35:17Z

Next step in #10603

first commit changes log level for an internal log from INFO to DEBUG. (Not really related, but I started finding the INFO level logging annoying when debuging test failures)
second commit introduces a P2PStub class - a subclass of NodeConnCB which has its own block and tx store and responds appropriately to getdata requests. Not all the functionality is used in invalidtxrequest.py, but will be used in invalidblockrequest.py and p2p-fullblocktest when those are changed to use BitcoinTestFramework
third commit tidies up invalidtxrequest.py
fourth commit removes usage of ComparisonTestFramework

fanquake · 2017-11-30T01:26:06Z

Needs a rebase.

jnewbery · 2017-11-30T03:33:40Z

rebased

jnewbery · 2017-12-02T16:23:33Z

Rebased and made slight improvements to the P2PStub class.

jamesob

utACK 8c3615f

jamesob · 2017-12-02T20:50:45Z

test/functional/test_framework/mininode.py

+        if len(self.block_store) == 0:
+            return
+        current_block_header = self.block_store[next(reversed(self.block_store))]
+        # current_block_header = super(current_tip)


Unnecessary comment?

duh. Vestigial garbage. I'll remove.

jnewbery · 2017-12-02T22:20:14Z

Thanks for the review @jamesob . I've removed the errant comment, and improved comments for the P2PStub methods.

jamesob · 2017-12-03T05:30:09Z

utACK e72c94c

jamesob · 2017-12-03T05:33:13Z

...modulo the legitimate-looking test failure:

  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/invalidtxrequest.py", line 50, in run_test
    node.p2p.send_txs_and_test([tx1], node, False, 16, b'mandatory-script-verify-flag-failed (Invalid OP_IF construction)')
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/mininode.py", line 540, in send_txs_and_test
    assert_equal(reject_code, self.reject_code_received)
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/util.py", line 38, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(16 == None)

jnewbery · 2017-12-04T14:55:58Z

Great. Travis caught a race condition. I wasn't able to reproduce it locally.

Should be fixed in the final commit. @jamesob - let me know when you've taken a look and I'll squash into the Add P2PStub class commit.

jnewbery · 2017-12-11T14:32:07Z

squashed fix and rebased

maflcko

Concept ACK 0d5cfa3074

Some questions inline

maflcko · 2017-12-14T16:08:30Z

test/functional/test_framework/mininode.py

+                    self.send_message(msg_tx(self.tx_store[inv.hash]))
+            elif inv.type == 2 or inv.type == 2 | MSG_WITNESS_FLAG:
+                if inv.hash in self.block_store.keys():
+                    self.send_message(msg_block(self.block_store[inv.hash]))


Mind to simplify the nested ifs?

elif inv.type & MSG_TX and inv.hash in self.block_store.keys(): self.send...

inv types are not bit flags in general, so inv.type & MSG_TX evaluates to true for MSG_FILTERED_BLOCK (and inv.type & MSG_BLOCK also evalutes to true for MSG_FILTERED_BLOCK).

Flattening the nested ifs would give:

if (inv.type == 1 or inv.type == 1 | MSG_WITNESS_FLAG) and inv.hash in self.tx_store.keys(): self.send_message(msg_tx(self.tx_store[inv.hash])) elif (inv.type == 2 or inv.type == 2 | MSG_WITNESS_FLAG) and inv.hash in self.block_store.keys(): self.send_message(msg_block(self.block_store[inv.hash]))

I'm not sure if that's any clearer due to the length of the if statements, but I'm happy to change it if you think it's an improvement.

Nope, fine to leave as is.

#11771 (comment)

In commit "Add P2PStub class"

I also think this is fine as is, but I would suggest taking constants from the c++ code and writing:

if (inv.type & MSG_TYPE_MASK) == MSG_TX and inv.hash in self.tx_store.keys():

taken @ryanofsky's suggested change

maflcko · 2017-12-14T16:14:41Z

test/functional/test_framework/mininode.py

+        if len(self.block_store) == 0:
+            return
+        current_block_header = self.block_store[next(reversed(self.block_store))]
+        if current_block_header is None:


nit: Add a comment why this would ever be true and not dead code?

This isn't actually required. Removing.

maflcko · 2017-12-14T16:27:51Z

test/functional/test_framework/mininode.py

+        self.send_message(msg_headers([blocks[-1]]))
+
+        if request_block:
+            wait_until(lambda: blocks[-1].sha256 in self.getdata_requests)


Since getdata_requests is written to with the mininode lock, why is a lock=mininode_lock not required when reading?

You're right. This is required. Updating.

maflcko · 2017-12-14T16:39:49Z

test/functional/test_framework/mininode.py

+        self.sync_with_ping()
+
+        if success:
+            assert tx.hash in rpc.getrawmempool()


The documentation states "whether they are accepted", but you only check one.

jnewbery · 2017-12-14T18:49:27Z

Fixed @MarcoFalke's comments, along with a few variable renamings. I've pushed as a fixup commit. Will squash once Marco confirms he's happy with the fixup.

maflcko · 2017-12-14T23:13:00Z

fixups look good, though a travis failure:

./test/functional/test_framework/mininode.py:26:1: F401 'test_framework.util.assert_equal' imported but unused

^---- failure generated from contrib/devtools/lint-python.sh

Feel free to squash. I am going to review from scratch anyway.

jnewbery · 2017-12-15T22:35:49Z

squashed && rebased

ryanofsky

utACK 1c5aa93034a7ad213727ffdfb1bb71eab20f1f80. I left some suggestions, but they are all minor, and you should feel free to ignore them. Overall, this seems like a very nice addition to the test framework.

ryanofsky · 2017-12-19T19:00:25Z

test/functional/test_framework/mininode.py

+                    self.send_message(msg_tx(self.tx_store[inv.hash]))
+            elif inv.type == 2 or inv.type == 2 | MSG_WITNESS_FLAG:
+                if inv.hash in self.block_store.keys():
+                    self.send_message(msg_block(self.block_store[inv.hash]))


#11771 (comment)

In commit "Add P2PStub class"

I also think this is fine as is, but I would suggest taking constants from the c++ code and writing:

if (inv.type & MSG_TYPE_MASK) == MSG_TX and inv.hash in self.tx_store.keys():

ryanofsky · 2017-12-19T19:05:38Z

test/functional/test_framework/mininode.py

+        locator, hash_stop = message.locator, message.hashstop
+
+        # Assume that the most recent block added is the tip
+        if len(self.block_store) == 0:


In commit "Add P2PStub class"

If following PEP8 this should be if not self.block_store:

ryanofsky · 2017-12-19T19:06:09Z

test/functional/test_framework/mininode.py

+        response = msg_headers()
+        headers_list = [current_block_header]
+        maxheaders = 2000
+        while (headers_list[0].sha256 not in locator.vHave):


In commit "Add P2PStub class"

Unnecessary parentheses

ryanofsky · 2017-12-19T19:12:10Z

test/functional/test_framework/mininode.py

+            prev_block_hash = headers_list[0].hashPrevBlock
+            if prev_block_hash in self.block_store:
+                prev_block_header = self.block_store[prev_block_hash]
+                headers_list.insert(0, prev_block_header)


In commit "Add P2PStub class"

Usually better to append to end of list than insert at beginning of list. You can get the last maxheaders in reverse order with `header_list[:-maxheaders-1:-1]

ryanofsky · 2017-12-19T19:17:56Z

test/functional/test_framework/mininode.py

+            self.reject_reason_received = None
+
+        for block in blocks:
+            self.block_store[block.sha256] = block


Maybe assert block.sha256 not in block_store because if this is not true, ordering assumption could be broken and next(reversed(block_store)) might not actually return the last block.

In commit "Add P2PStub class"

Need mininode_lock here?

added mininode_lock. I don't think the assert is necessary - we may wish to send the same block over the same P2P interface more than once.

ryanofsky · 2017-12-19T19:51:23Z

test/functional/invalidtxrequest.py

@@ -32,13 +28,10 @@ def run_test(self):
        test.run()

    def get_tests(self):
-        if self.tip is None:


In commit [tests] Fix flake8 warnings in invalidtxrequest

I see you are removing self.tip variable entirely in next commit, but it seems safer not to drop the None condition in this commit, which is supposed to only be fixing flake8 warnings.

self.tip is set to None in run_test() and then run() is called, which calls get_tests(), so self.tip has to be None at this point.

So yes, this isn't strictly fixing a flake8 warning, but it's a safe tidy-up which is removed in the next commit.

ryanofsky · 2017-12-19T19:55:23Z

test/functional/test_framework/mininode.py

@@ -440,3 +454,136 @@ def network_thread_join(timeout=10):
    for thread in network_threads:
        thread.join(timeout)
        assert not thread.is_alive()
+
+class P2PStub(P2PInterface):


In commit "Add P2PStub class"

Name like P2PDataStore might more suggestive than P2PStub

ok, changed to P2PDataStore

ryanofsky · 2017-12-19T19:57:12Z

test/functional/invalidtxrequest.py

-    def get_tests(self):
-        self.tip = int("0x" + self.nodes[0].getbestblockhash(), 0)
-        self.block_time = int(time.time()) + 1
+        tip = int("0x" + self.nodes[0].getbestblockhash(), 0)


In commit "Change invalidtxrequest to use BitcoinTestFramework"

Could do int(self.nodes[0].getbestblockhash(), 16) and drop "0x" prefix, I think.

agree. That's better

ryanofsky · 2017-12-19T19:59:15Z

test/functional/invalidtxrequest.py

        height += 1
-        yield TestInstance([[block, True]])
+        node.p2p.send_blocks_and_test([block], node, True)


In commit "Change invalidtxrequest to use BitcoinTestFramework"

Maybe drop True since that seems to be the default value, or write success=True. Plain True by itself doesn't convey anything and just seems suspicious.

ryanofsky · 2017-12-19T20:01:31Z

test/functional/invalidtxrequest.py

-        tx1 = create_transaction(self.block1.vtx[0], 0, b'\x64', 50 * COIN - 12000)
-        yield TestInstance([[tx1, RejectResult(16, b'mandatory-script-verify-flag-failed')]])
+        tx1 = create_transaction(block1.vtx[0], 0, b'\x64', 50 * COIN - 12000)
+        node.p2p.send_txs_and_test([tx1], node, False, 16, b'mandatory-script-verify-flag-failed (Invalid OP_IF construction)')


In commit "Change invalidtxrequest to use BitcoinTestFramework"

Maybe add success=, reject_code=, keywords for future proofing and clarity.

@conscott

[tests] update tests from changes to mininode in bitcoin#11771 - added by @conscott [tests] trivial update to hex conversion for readability - added by @conscott

maflcko

utACK 95e2e9a, just some dead code and comment nits.

maflcko · 2018-03-13T20:14:28Z

test/functional/test_framework/mininode.py

+            elif (inv.type & MSG_TYPE_MASK) == MSG_BLOCK and inv.hash in self.block_store.keys():
+                self.send_message(msg_block(self.block_store[inv.hash]))
+            else:
+                logger.debug('getdata message type {} received.'.format(hex(inv.type)))


Could be removed. This is already done by P2PConnection._log_message.

maflcko · 2018-03-13T20:14:50Z

test/functional/test_framework/mininode.py

+        headers_list = headers_list[:-maxheaders - 1:-1]
+        response = msg_headers(headers_list)
+
+        if response is not None:


Could be removed per my previous review.

maflcko · 2018-03-13T20:15:41Z

test/functional/test_framework/mininode.py

+         - add all txs to our tx_store
+         - send tx messages for all txs
+         - if success is True: assert that the tx is accepted to the mempool
+         - if success is False: assert that the tx is not accepted to the mempool


nit: Replace "tx is" with "txs are"

…amework e97b113 [tests] Change invalidblockrequest to use BitcoinTestFramework (John Newbery) 2b7064e [tests] Fix flake8 warnings in invalidblockrequest (John Newbery) 54b8c58 [test] Fix nits leftover from 11771 (Conor Scott) Pull request description: Builds on #11771. Please review that PR first Next step in #10603. - first commit tidies up invalidblockrequest.py - second commit removes usage of ComparisonTestFramework Tree-SHA512: 14b10c09c8c0ebef4a9176eb5b883a275d04c096785ee31b84ef594eed346ec6344d7ed32184c5fb397e744725df3911f45cdfadd0810e5a52eaa256084e3456

jnewbery · 2018-03-18T23:04:57Z

Agree with all of @MarcoFalke's comments, which can be addressed in a future PR.

maflcko · 2018-03-29T20:57:25Z

test/functional/test_framework/mininode.py

+                self.block_store[block.sha256] = block
+                self.last_block_hash = block.sha256
+
+        self.send_message(msg_headers([blocks[-1]]))


I can't find a reason why you are sending the full block in a headers message instead of just the header? If there is a reason, it should be documented, otherwise I'd suggest sending just the header.

Yes, I agree - this should be changed to just be the header.

Fixed in #12861

@conscott

[tests] update tests from changes to mininode in bitcoin#11771 - added by @conscott [tests] trivial update to hex conversion for readability - added by @conscott

…stFramework 95e2e9a [tests] Change invalidtxrequest to use BitcoinTestFramework (John Newbery) 359d067 [tests] Fix flake8 warnings in invalidtxrequest (John Newbery) c32cf9f [tests] Add P2PDataStore class (John Newbery) cc046f6 [tests] Reduce NodeConn connection logging from info to debug (John Newbery) Pull request description: Next step in bitcoin#10603 - first commit changes log level for an internal log from INFO to DEBUG. (Not really related, but I started finding the INFO level logging annoying when debuging test failures) - second commit introduces a `P2PStub` class - a subclass of `NodeConnCB` which has its own block and tx store and responds appropriately to getdata requests. Not all the functionality is used in `invalidtxrequest.py`, but will be used in `invalidblockrequest.py` and `p2p-fullblocktest` when those are changed to use `BitcoinTestFramework` - third commit tidies up `invalidtxrequest.py` - fourth commit removes usage of `ComparisonTestFramework` Tree-SHA512: f3085c73c15d6ce894e401490bce8a7fa7cf52b0c9d135ff7e351f1f6f517c99accab8588fcdc443f39ea8315329aaabd66b2baa32499df5a774737882030373

…nTestFramework e97b113 [tests] Change invalidblockrequest to use BitcoinTestFramework (John Newbery) 2b7064e [tests] Fix flake8 warnings in invalidblockrequest (John Newbery) 54b8c58 [test] Fix nits leftover from 11771 (Conor Scott) Pull request description: Builds on bitcoin#11771. Please review that PR first Next step in bitcoin#10603. - first commit tidies up invalidblockrequest.py - second commit removes usage of ComparisonTestFramework Tree-SHA512: 14b10c09c8c0ebef4a9176eb5b883a275d04c096785ee31b84ef594eed346ec6344d7ed32184c5fb397e744725df3911f45cdfadd0810e5a52eaa256084e3456