[tests] Change invalidtxrequest to use BitcoinTestFramework

jnewbery commented at 4:35 pm on November 27, 2017: member

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 added the label Tests on Nov 27, 2017

fanquake commented at 1:26 am on November 30, 2017: member

Needs a rebase.

jnewbery force-pushed on Nov 30, 2017

jnewbery commented at 3:33 am on November 30, 2017: member

rebased

jnewbery force-pushed on Dec 2, 2017

jnewbery commented at 4:23 pm on December 2, 2017: member

Rebased and made slight improvements to the P2PStub class.

in test/functional/test_framework/mininode.py:448 in f4c080ff63 outdated

443+
444+        # Assume that the most recent block added is the tip
445+        if len(self.block_store) == 0:
446+            return
447+        current_block_header = self.block_store[next(reversed(self.block_store))]
448+        # current_block_header = super(current_tip)

jamesob commented at 8:50 pm on December 2, 2017:

Unnecessary comment?

jnewbery commented at 10:19 pm on December 2, 2017:

duh. Vestigial garbage. I’ll remove.

jamesob approved

jamesob commented at 8:58 pm on December 2, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/11771/commits/8c3615f7fb932aba99b5b456119812c95fd8a605

jnewbery force-pushed on Dec 2, 2017

jnewbery commented at 10:20 pm on December 2, 2017: member

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

jamesob commented at 5:30 am on December 3, 2017: member

utACK https://github.com/bitcoin/bitcoin/pull/11771/commits/e72c94cda663102c7e188b80e9cdb76026c0b6a7

jamesob commented at 5:33 am on December 3, 2017: member

…modulo the legitimate-looking test failure:

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

jnewbery commented at 2:55 pm on December 4, 2017: member

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 force-pushed on Dec 11, 2017

jnewbery commented at 2:32 pm on December 11, 2017: member

squashed fix and rebased

jnewbery force-pushed on Dec 13, 2017

in test/functional/test_framework/mininode.py:480 in 3d2209f335 outdated

475+            if inv.type == 1 or inv.type == 1 | MSG_WITNESS_FLAG:
476+                if inv.hash in self.tx_store.keys():
477+                    self.send_message(msg_tx(self.tx_store[inv.hash]))
478+            elif inv.type == 2 or inv.type == 2 | MSG_WITNESS_FLAG:
479+                if inv.hash in self.block_store.keys():
480+                    self.send_message(msg_block(self.block_store[inv.hash]))

MarcoFalke commented at 4:08 pm on December 14, 2017:

Mind to simplify the nested ifs?

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

jnewbery commented at 5:01 pm on December 14, 2017:

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:

0            if (inv.type == 1 or inv.type == 1 | MSG_WITNESS_FLAG) and inv.hash in self.tx_store.keys():
1                self.send_message(msg_tx(self.tx_store[inv.hash]))
2            elif (inv.type == 2 or inv.type == 2 | MSG_WITNESS_FLAG) and inv.hash in self.block_store.keys():
3                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.

MarcoFalke commented at 5:47 pm on December 14, 2017:

Nope, fine to leave as is.

ryanofsky commented at 7:00 pm on December 19, 2017:

#11771 (review)

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:

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

jnewbery commented at 8:32 pm on January 18, 2018:

taken @ryanofsky’s suggested change

in test/functional/test_framework/mininode.py:491 in 3d2209f335 outdated

486+
487+        # Assume that the most recent block added is the tip
488+        if len(self.block_store) == 0:
489+            return
490+        current_block_header = self.block_store[next(reversed(self.block_store))]
491+        if current_block_header is None:

MarcoFalke commented at 4:14 pm on December 14, 2017:

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

jnewbery commented at 6:05 pm on December 14, 2017:

This isn’t actually required. Removing.

in test/functional/test_framework/mininode.py:547 in 3d2209f335 outdated

542+            self.block_store[block.sha256] = block
543+
544+        self.send_message(msg_headers([blocks[-1]]))
545+
546+        if request_block:
547+            wait_until(lambda: blocks[-1].sha256 in self.getdata_requests)

MarcoFalke commented at 4:27 pm on December 14, 2017:

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

jnewbery commented at 6:06 pm on December 14, 2017:

You’re right. This is required. Updating.

in test/functional/test_framework/mininode.py:574 in 3d2209f335 outdated

569+            self.send_message(msg_tx(tx))
570+
571+        self.sync_with_ping()
572+
573+        if success:
574+            assert tx.hash in rpc.getrawmempool()

MarcoFalke commented at 4:39 pm on December 14, 2017:

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

jnewbery commented at 6:48 pm on December 14, 2017:

fixed

MarcoFalke commented at 4:44 pm on December 14, 2017: member

Concept ACK 0d5cfa3074

Some questions inline

jnewbery commented at 6:49 pm on December 14, 2017: member

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.

MarcoFalke commented at 11:13 pm on December 14, 2017: member

fixups look good, though a travis failure:

0./test/functional/test_framework/mininode.py:26:1: F401 'test_framework.util.assert_equal' imported but unused
1
2^---- failure generated from contrib/devtools/lint-python.sh

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

jnewbery force-pushed on Dec 15, 2017

jnewbery commented at 10:35 pm on December 15, 2017: member

squashed && rebased

in test/functional/test_framework/mininode.py:488 in 86dec60188 outdated

483+        """Search back through our block store for the locator, and reply with a headers message if found."""
484+
485+        locator, hash_stop = message.locator, message.hashstop
486+
487+        # Assume that the most recent block added is the tip
488+        if len(self.block_store) == 0:

ryanofsky commented at 7:05 pm on December 19, 2017:

In commit “Add P2PStub class”

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

jnewbery commented at 8:32 pm on January 18, 2018:

indeed

in test/functional/test_framework/mininode.py:495 in 86dec60188 outdated

490+        current_block_header = self.block_store[next(reversed(self.block_store))]
491+
492+        response = msg_headers()
493+        headers_list = [current_block_header]
494+        maxheaders = 2000
495+        while (headers_list[0].sha256 not in locator.vHave):

ryanofsky commented at 7:06 pm on December 19, 2017:

In commit “Add P2PStub class”

Unnecessary parentheses

jnewbery commented at 8:33 pm on January 18, 2018:

removed

in test/functional/test_framework/mininode.py:501 in 86dec60188 outdated

496+            # Walk back through the block store, adding headers to headers_list
497+            # as we go.
498+            prev_block_hash = headers_list[0].hashPrevBlock
499+            if prev_block_hash in self.block_store:
500+                prev_block_header = self.block_store[prev_block_hash]
501+                headers_list.insert(0, prev_block_header)

ryanofsky commented at 7:12 pm on December 19, 2017:

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]

jnewbery commented at 8:34 pm on January 18, 2018:

changed

in test/functional/test_framework/mininode.py:540 in 86dec60188 outdated

535+        with mininode_lock:
536+            self.reject_code_received = None
537+            self.reject_reason_received = None
538+
539+        for block in blocks:
540+            self.block_store[block.sha256] = block

ryanofsky commented at 7:17 pm on December 19, 2017:

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.

ryanofsky commented at 7:38 pm on December 19, 2017:

In commit “Add P2PStub class”

Need mininode_lock here?

jnewbery commented at 8:58 pm on January 18, 2018:

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.

in test/functional/test_framework/mininode.py:467 in 86dec60188 outdated

462+
463+    def __init__(self):
464+        super().__init__()
465+        self.reject_code_received = None
466+        self.reject_reason_received = None
467+        self.block_store = OrderedDict()

ryanofsky commented at 7:23 pm on December 19, 2017:

In commit “Add P2PStub class”

Using OrderedDict for this seems fragile, and also seems like it requires storing unnecessary ordering information. The only place ordering seems to be used is in the reversed(next(block_store)) call returning the most recent block hash. But it would seem more straightforward and efficient just to add a last_block_hash member variable to hold this instead of trying to encode it within the dict.

ryanofsky commented at 7:25 pm on December 19, 2017:

In commit “Add P2PStub class”

Could use a comment here like # block hash -> CBlock to indicate what the keys and values are.

jnewbery commented at 8:21 pm on January 18, 2018:

yes - makes more sense.

jnewbery commented at 8:22 pm on January 18, 2018:

added

in test/functional/test_framework/mininode.py:471 in 86dec60188 outdated

463+    def __init__(self):
464+        super().__init__()
465+        self.reject_code_received = None
466+        self.reject_reason_received = None
467+        self.block_store = OrderedDict()
468+        self.tx_store = {}

ryanofsky commented at 7:26 pm on December 19, 2017:

In commit “Add P2PStub class”

Could use a comment here like # tx hash -> CTransaction to indicate what the keys and values are.

jnewbery commented at 8:22 pm on January 18, 2018:

added

in test/functional/test_framework/mininode.py:511 in 86dec60188 outdated

506+        headers_list = headers_list[:maxheaders]
507+        hash_list = [x.sha256 for x in headers_list]
508+
509+        # Stop the list at the hash_stop header if found
510+        index = len(headers_list)
511+        if (hash_stop in hash_list):

ryanofsky commented at 7:33 pm on December 19, 2017:

In commit “Add P2PStub class”

Unnecessary parentheses. Also it, seems like you could just check headers_list[0].sha256 != hash_stop in the while loop above and drop this extra truncation code.

jnewbery commented at 8:51 pm on January 18, 2018:

yes. This code was copied from an blockstore.py. I’ve tidied up as you’ve suggested.

in test/functional/test_framework/mininode.py:571 in 86dec60188 outdated

566+        with mininode_lock:
567+            self.reject_code_received = None
568+            self.reject_reason_received = None
569+
570+        for tx in txs:
571+            self.tx_store[tx.sha256] = tx

ryanofsky commented at 7:40 pm on December 19, 2017:

In commit “Add P2PStub class”

Need mininode_lock?

jnewbery commented at 8:58 pm on January 18, 2018:

added

in test/functional/invalidtxrequest.py:35 in 534e6092b4 outdated

27@@ -32,13 +28,10 @@ def run_test(self):
28         test.run()
29 
30     def get_tests(self):
31-        if self.tip is None:

ryanofsky commented at 7:51 pm on December 19, 2017:

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.

jnewbery commented at 2:05 pm on January 17, 2018:

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.

in test/functional/test_framework/mininode.py:458 in 86dec60188 outdated

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

ryanofsky commented at 7:55 pm on December 19, 2017:

In commit “Add P2PStub class”

Name like P2PDataStore might more suggestive than P2PStub

jnewbery commented at 6:08 pm on January 18, 2018:

ok, changed to P2PDataStore

in test/functional/invalidtxrequest.py:30 in 1c5aa93034 outdated

37+        node.p2p.wait_for_verack()
38 
39-    def get_tests(self):
40-        self.tip = int("0x" + self.nodes[0].getbestblockhash(), 0)
41-        self.block_time = int(time.time()) + 1
42+        tip = int("0x" + self.nodes[0].getbestblockhash(), 0)

ryanofsky commented at 7:57 pm on December 19, 2017:

In commit “Change invalidtxrequest to use BitcoinTestFramework”

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

jnewbery commented at 6:01 pm on January 18, 2018:

agree. That’s better

in test/functional/invalidtxrequest.py:42 in 1c5aa93034 outdated

54-        self.tip = block.sha256
55+        block1 = block
56+        tip = block.sha256
57         height += 1
58-        yield TestInstance([[block, True]])
59+        node.p2p.send_blocks_and_test([block], node, True)

ryanofsky commented at 7:59 pm on December 19, 2017:

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.

jnewbery commented at 6:03 pm on January 18, 2018:

yes

in test/functional/invalidtxrequest.py:50 in 1c5aa93034 outdated

73         # b'\x64' is OP_NOTIF
74         # Transaction will be rejected with code 16 (REJECT_INVALID)
75-        tx1 = create_transaction(self.block1.vtx[0], 0, b'\x64', 50 * COIN - 12000)
76-        yield TestInstance([[tx1, RejectResult(16, b'mandatory-script-verify-flag-failed')]])
77+        tx1 = create_transaction(block1.vtx[0], 0, b'\x64', 50 * COIN - 12000)
78+        node.p2p.send_txs_and_test([tx1], node, False, 16, b'mandatory-script-verify-flag-failed (Invalid OP_IF construction)')

ryanofsky commented at 8:01 pm on December 19, 2017:

In commit “Change invalidtxrequest to use BitcoinTestFramework”

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

jnewbery commented at 6:07 pm on January 18, 2018:

yes

ryanofsky commented at 8:08 pm on December 19, 2017: member

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.

in test/functional/invalidtxrequest.py:31 in 1c5aa93034 outdated

38 
39-    def get_tests(self):
40-        self.tip = int("0x" + self.nodes[0].getbestblockhash(), 0)
41-        self.block_time = int(time.time()) + 1
42+        tip = int("0x" + self.nodes[0].getbestblockhash(), 0)
43+        block_time = int(time.time()) + 1

ryanofsky commented at 8:07 pm on December 20, 2017:

In commit “Change invalidtxrequest to use BitcoinTestFramework”

Could maybe switch from time.time() to best_block[“time”] to be consistent with invalidblockrequest change in #11172. It seems better in a test environment, to be using information from the environment instead of calling out to the OS.

jnewbery commented at 9:04 pm on January 18, 2018:

Yes, good idea

jnewbery force-pushed on Jan 18, 2018

jnewbery commented at 9:05 pm on January 18, 2018: member

Thanks for the review @ryanofsky - I think I’ve addressed all your comments

in test/functional/test_framework/mininode.py:503 in b987a946ae outdated

498+            prev_block_hash = headers_list[-1].hashPrevBlock
499+            if prev_block_hash in self.block_store:
500+                prev_block_header = self.block_store[prev_block_hash]
501+                headers_list.append(prev_block_header)
502+                if prev_block_header.sha256 == hash_stop:
503+                    # if this is gthe hashstop header, stop here

ryanofsky commented at 8:39 pm on January 23, 2018:

[tests] Add P2PDataStore class

Typo “gthe”

jnewbery commented at 9:09 pm on January 23, 2018:

fixed

ryanofsky commented at 8:50 pm on January 23, 2018: member

utACK 348d4e37cffc7098ff3c2b8128396889e41ec941, looks great! There were too many changes to mention since previous review (mostly in P2PDataStore), all based on previous review comments.

jnewbery force-pushed on Jan 23, 2018

ryanofsky commented at 9:23 pm on January 23, 2018: member

utACK c9772a83644e3c212d87dd0866384533054923a0. Only change is suggested typo fix.

in test/functional/test_framework/mininode.py:496 in c9772a8364 outdated

490+            return
491+        current_block_header = self.last_block_hash
492+
493+        headers_list = [current_block_header]
494+        maxheaders = 2000
495+        while headers_list[-1].sha256 not in locator.vHave:

MarcoFalke commented at 4:24 pm on January 24, 2018:

the type is already a hash (i.e. int)

0AttributeError: 'int' object has no attribute 'sha256'

jnewbery commented at 4:38 pm on January 24, 2018:

Thanks Marco. This was a bug introduced when changing from an orderedDict to just storing the last_block_hash in the P2PDataStore class. Only hit on travis because of a timing window.

This is now fixed.

jnewbery force-pushed on Jan 24, 2018

in test/functional/test_framework/mininode.py:511 in 9490748923 outdated

503+                    break
504+            else:
505+                break
506+
507+        # Truncate the list if there are too many headers
508+        headers_list = headers_list[:-maxheaders - 1:-1]

MarcoFalke commented at 10:52 pm on January 24, 2018:

This will send the headers in the opposite order of what the protocol requires?

jnewbery commented at 0:12 am on January 25, 2018:

This is the same as the existing blockstore.py code. In that code, the headers were inserted at the front of the list and then a msg_headers was constructed. In this version we append the headers at the end of the list and then reverse the order before constructing the msg_headers.

[tests] Reduce NodeConn connection logging from info to debug cc046f66a7

in test/functional/test_framework/mininode.py:514 in 9490748923 outdated

506+
507+        # Truncate the list if there are too many headers
508+        headers_list = headers_list[:-maxheaders - 1:-1]
509+        response = msg_headers(headers_list)
510+
511+        if response is not None:

MarcoFalke commented at 10:53 pm on January 24, 2018:

I don’t see how this could be None

MarcoFalke commented at 10:55 pm on January 24, 2018:

Couldn’t you use the existing blockstore code? I know that for the purpose of this test saving blocks to disk twice is overkill, but by reducing the code and diff, wouldn’t that be preferable?

jnewbery commented at 0:08 am on January 25, 2018:

This is just ported directly from on_getheaders() in comptool.py (which calls into headers_for() in blockstore.py).

You’re right that this could never be None. That was the case before in the original code. I could fix this here (which would make the logical diff from the existing code bigger), or fix it in a follow-up PR.

jnewbery force-pushed on Jan 29, 2018

jnewbery commented at 2:53 pm on January 29, 2018: member

rebased to conform with new naming convention.

in test/functional/test_framework/mininode.py:481 in e4a1b3bf7e outdated

476+        for inv in message.inv:
477+            self.getdata_requests.append(inv.hash)
478+            if (inv.type & MSG_TYPE_MASK) == MSG_TX and inv.hash in self.tx_store.keys():
479+                self.send_message(msg_tx(self.tx_store[inv.hash]))
480+            elif (inv.type & MSG_TYPE_MASK) == MSG_BLOCK and inv.hash in self.block_store.keys():
481+                self.send_message(msg_block(self.block_store[inv.hash]))

jamesob commented at 10:34 pm on January 29, 2018:

Minor thought for debuggability: is it conceivable there’s a case for which an else: clause that logs an unexpected message type might come in handy?

jnewbery commented at 11:34 pm on January 29, 2018:

added logging

in test/functional/test_framework/mininode.py:506 in e4a1b3bf7e outdated

499+                prev_block_header = self.block_store[prev_block_hash]
500+                headers_list.append(prev_block_header)
501+                if prev_block_header.sha256 == hash_stop:
502+                    # if this is the hashstop header, stop here
503+                    break
504+            else:

jamesob commented at 10:34 pm on January 29, 2018:

Worth logging some indication of a miss?

jnewbery commented at 11:34 pm on January 29, 2018:

added logging

in test/functional/test_framework/mininode.py:582 in e4a1b3bf7e outdated

574+
575+        raw_mempool = rpc.getrawmempool()
576+        if success:
577+            # Check that all txs are now in the mempool
578+            for tx in txs:
579+                assert tx.hash in raw_mempool, "{} not found in mempool".format(tx.hash)

jamesob commented at 10:48 pm on January 29, 2018:

I’ve found f strings convenient (f'{tx.hash} not found in mempool') but that’s personal preference.

jnewbery commented at 10:53 pm on January 29, 2018:

I also like f strings, but they’re new in v3.6, and we support older Python versions than that (back to v3.4 at least).

jamesob commented at 10:56 pm on January 29, 2018:

Ah, makes sense.

jamesob approved

jamesob commented at 10:49 pm on January 29, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/11771/commits/5921f2577287e5e0b05f900cf27f3aa537116563

Nice improvement. Feel free to ignore the few comments.

jnewbery commented at 11:34 pm on January 29, 2018: member

Thanks for the review @jamesob . I’ve added a fixup commit that adds logging as suggested. If you’re happy with that I’ll squash it into the P2PDataStore commit.

jamesob approved

jamesob commented at 1:53 pm on January 30, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/11771/commits/2ca114cbf50d1161a213c870691d50148b480b9e

[tests] Add P2PDataStore class

P2PDataStore subclasses NodeConnCB. This class keeps a store
of txs and blocks and responds to getdata requests from
the node-under-test.

c32cf9f622

[tests] Fix flake8 warnings in invalidtxrequest 359d067572

[tests] Change invalidtxrequest to use BitcoinTestFramework 95e2e9af12

jnewbery force-pushed on Jan 30, 2018

jnewbery commented at 2:31 pm on January 30, 2018: member

thanks. Fixup commit squashed

jamesob commented at 9:20 pm on January 30, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/11771/commits/359d06757249b34b36688eaef31e7626a46bd52a

laanwj deleted a comment on Jan 30, 2018

MarcoFalke deleted a comment on Jan 30, 2018

in test/functional/p2p_invalid_tx.py:36 in 95e2e9af12

58+        self.log.info("Create a new block with an anyone-can-spend coinbase.")
59         height = 1
60-        block = create_block(self.tip, create_coinbase(height), self.block_time)
61-        self.block_time += 1
62+        block = create_block(tip, create_coinbase(height), block_time)
63+        block_time += 1

conscott commented at 10:44 pm on February 12, 2018:

This line can be removed. block_time is never used again.

in test/functional/p2p_invalid_tx.py:41 in 95e2e9af12

65         # Save the coinbase for later
66-        self.block1 = block
67-        self.tip = block.sha256
68+        block1 = block
69+        tip = block.sha256
70         height += 1

conscott commented at 10:45 pm on February 12, 2018:

This line can also be removed. height is never used again.

in test/functional/p2p_invalid_tx.py:50 in 95e2e9af12

89         # b'\x64' is OP_NOTIF
90         # Transaction will be rejected with code 16 (REJECT_INVALID)
91-        tx1 = create_transaction(self.block1.vtx[0], 0, b'\x64', 50 * COIN - 12000)
92-        yield TestInstance([[tx1, RejectResult(16, b'mandatory-script-verify-flag-failed')]])
93+        tx1 = create_transaction(block1.vtx[0], 0, b'\x64', 50 * COIN - 12000)
94+        node.p2p.send_txs_and_test([tx1], node, success=False, reject_code=16, reject_reason=b'mandatory-script-verify-flag-failed (Invalid OP_IF construction)')

conscott commented at 11:24 pm on February 12, 2018:

send_txs_and_test is created in this PR, but I do not see it being called with sucess=True in any PRs mentioned in #10603

Does it make sense to also send a valid transaction in this code block as well, to verify that codepath?

laanwj assigned laanwj on Feb 13, 2018

laanwj commented at 9:30 am on February 13, 2018: member

Seems to be ready for merge. If you don’t mind @conscott I’ll leave your suggestions for a future PR, this is only the first in a series.

laanwj merged this on Feb 13, 2018

laanwj closed this on Feb 13, 2018

laanwj referenced this in commit 2dbc4a4740 on Feb 13, 2018

jnewbery commented at 1:55 pm on February 13, 2018: member

Thanks for the review @conscott . I’ll address your points in #11772.

jnewbery deleted the branch on Feb 13, 2018

conscott referenced this in commit 11c5482955 on Feb 13, 2018

conscott referenced this in commit 84122956d0 on Feb 13, 2018

conscott referenced this in commit 88725ed71b on Feb 13, 2018

conscott referenced this in commit 8001668b04 on Feb 13, 2018

conscott referenced this in commit 87161b97f9 on Feb 14, 2018

jnewbery referenced this in commit e97b113b04 on Feb 14, 2018

in test/functional/test_framework/mininode.py:483 in 95e2e9af12

478+            if (inv.type & MSG_TYPE_MASK) == MSG_TX and inv.hash in self.tx_store.keys():
479+                self.send_message(msg_tx(self.tx_store[inv.hash]))
480+            elif (inv.type & MSG_TYPE_MASK) == MSG_BLOCK and inv.hash in self.block_store.keys():
481+                self.send_message(msg_block(self.block_store[inv.hash]))
482+            else:
483+                logger.debug('getdata message type {} received.'.format(hex(inv.type)))

MarcoFalke commented at 8:14 pm on March 13, 2018:

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

in test/functional/test_framework/mininode.py:514 in 95e2e9af12

509+
510+        # Truncate the list if there are too many headers
511+        headers_list = headers_list[:-maxheaders - 1:-1]
512+        response = msg_headers(headers_list)
513+
514+        if response is not None:

MarcoFalke commented at 8:14 pm on March 13, 2018:

Could be removed per my previous review.

in test/functional/test_framework/mininode.py:563 in 95e2e9af12

558+        """Send txs to test node and test whether they're accepted to the mempool.
559+
560+         - add all txs to our tx_store
561+         - send tx messages for all txs
562+         - if success is True: assert that the tx is accepted to the mempool
563+         - if success is False: assert that the tx is not accepted to the mempool

MarcoFalke commented at 8:15 pm on March 13, 2018:

nit: Replace “tx is” with “txs are”

MarcoFalke commented at 8:25 pm on March 13, 2018: member

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

MarcoFalke referenced this in commit 0630974647 on Mar 13, 2018

jnewbery commented at 11:04 pm on March 18, 2018: member

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

in test/functional/test_framework/mininode.py:542 in 95e2e9af12

537+
538+            for block in blocks:
539+                self.block_store[block.sha256] = block
540+                self.last_block_hash = block.sha256
541+
542+        self.send_message(msg_headers([blocks[-1]]))

MarcoFalke commented at 8:57 pm on March 29, 2018:

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.

jnewbery commented at 8:21 pm on March 30, 2018:

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

jnewbery commented at 7:45 pm on April 2, 2018:

Fixed in #12861

HashUnlimited referenced this in commit 424b20b512 on Jun 29, 2018

luke-jr referenced this in commit 9abf7e3b27 on Jul 10, 2018

luke-jr referenced this in commit 6a076dd41f on Jul 11, 2018

lionello referenced this in commit 96cd3f1b4b on Nov 7, 2018

codablock referenced this in commit e3a7aeab7b on Sep 30, 2019

codablock referenced this in commit 21040f773f on Sep 30, 2019

codablock referenced this in commit 8b88ad62b5 on Sep 30, 2019

codablock referenced this in commit 33c77c4039 on Sep 30, 2019

codablock referenced this in commit 9895e2b20e on Oct 2, 2019

codablock referenced this in commit e720d73770 on Oct 2, 2019

codablock referenced this in commit dc335cdee3 on Oct 2, 2019

codablock referenced this in commit d40f2e5825 on Oct 2, 2019

UdjinM6 referenced this in commit 6902ac1f2b on Oct 3, 2019

UdjinM6 referenced this in commit beed5cf1f2 on Oct 3, 2019

codablock referenced this in commit 01c138e86b on Oct 3, 2019

codablock referenced this in commit 3e72a09c16 on Oct 3, 2019

barrystyle referenced this in commit 3598c04df5 on Jan 22, 2020

barrystyle referenced this in commit 2b7f14e055 on Jan 22, 2020

random-zebra referenced this in commit 2ab948e1ad on May 4, 2021

MarcoFalke locked this on Sep 8, 2021

[tests] Change invalidtxrequest to use BitcoinTestFramework #11771