Next step in #10603.
Longer term, it would be better to separate net_processing testing from validation testing, but I think this is still a useful PR, since it moves us away from the comparison test framework.
52- '''
53+ # Add p2p connection to node0
54+ node = self.nodes[0] # convenience reference to the node
55+ node.add_p2p_connection(P2PStub())
56+
57+ NetworkThread().start() # Start up network handling in another thread
68- block = create_block(self.tip, create_coinbase(height), self.block_time)
69- self.block_time += 1
70+ block = create_block(tip, create_coinbase(height), block_time)
71+ block_time += 1
72 block.solve()
73 # Save the coinbase for later
108+ best_block = node.getblock(node.getbestblockhash())
109+ tip = int("0x" + node.getbestblockhash(), 0)
110+ height = best_block["height"] + 1
111+ block_time = best_block["time"] + 1
112+
113+ # Use merkle-root malleability to generate an invalid block with
39- test.run()
40+ # Add p2p connection to node0
41+ node = self.nodes[0] # convenience reference to the node
42+ node.add_p2p_connection(P2PStub())
43+
44+ NetworkThread().start() # Start up network handling in another thread
Thanks for your input @JohnVonNeumann. Three of your nits are on comments that are already in the code, and the last is to revert a pep8 fix (pep8 states that ‘inline comments should be separated by at least two spaces from the statement. They should start with a # and a single space.’)
the invalidtxrequest and invalidblockrequest refactors are covered by #11771 and #11772. If you have functional review of those tests, can you leave comments in those PRs?
Is it typically best off to leave preexisting nits for other PRs? Is that something I could fix myself and submit as a PR? Would that be the typical way to address it? I assume you look to keep the PRs clean otherwise people would just keep bringing up little things and it would be difficult to tell what was actually being added in the PR itself?
Apologies on the last PEP8 spacing comment too! I missed that part when reading the docs!
Will have a look at the other PRs and comment in those. Thanks for the feedback as well.
EDIT: Also sorry for not seeing that you had asked for #11771 and #11772 to be reviewed first, I didn’t read the PR notes thoroughly, will keep an eye on them better from now on.
Is it typically best off to leave preexisting nits for other PRs? Is that something I could fix myself and submit as a PR? Would that be the typical way to address it? I assume you look to keep the PRs clean otherwise people would just keep bringing up little things and it would be difficult to tell what was actually being added in the PR itself?
I usually clean up style nits as I go, and precede functional-change commits with tidy-up commits as I have in this PR.
The nits you’ve raised are more your personal preference than following any particular style convention, so I don’t think there’s any benefit for you to open a PR for them.
Ok this makes sense, although I disagree that all my comments were related directly to my personal preference, for me it was just about keeping the standards throughout the code, for example with the redundant inline comment, I don’t see any of them throughout the newly written code, so it would make sense to not have them in the old code. However, you are probably right, and I’ll happily defer to your experience.
I’ll try and make my reviews more valuable.
Cheers
95@@ -94,228 +96,123 @@ def set_test_params(self):
96 self.tip = None
97 self.blocks = {}
98
99- def add_options(self, parser):
100- super().add_options(parser)
101- parser.add_option("--runbarelyexpensive", dest="runbarelyexpensive", default=True)
In commit “Tidy up p2pfullblocktest”
Can you note that this option was removed in the commit message (and maybe say why it wasn’t useful?)
Skipped first two commits which are from #11771 and not up to date. Reviewed remaining individual commits:
utACK 061f4d8cfba58d299dd913bd96f88bddebae8276
Thanks for a very nice cleanup! Looking forward to seeing this merged!
- move all helper methods to the end
- remove block, create_tx and create_and_sign_tx shortcuts
- remove --runbarelyexpensive option, since it defaults to True and it's
unlikely that anyone ever runs the test with this option set to false.
86+ self.extra_args = [[]]
87+
88+ def run_test(self):
89+ node = self.nodes[0] # convenience reference to the node
90+
91+ network_thread_start()
nit: Could mention in a comment that this is only needed because connect_p2p assumes the thread is already running.
nit: Could rename connect_p2p to reconnect_p2p, since it only works when the thread is already running?
164+ self.sync_blocks([b6], True)
165
166 # Try to create a fork that double-spends
167 # genesis -> b1 (0) -> b2 (1) -> b5 (2) -> b6 (3)
168- # \-> b7 (2) -> b8 (4)
169+ # \-> b7 (3) -> b8 (4)
Why are you changing this comment?
Block 7 spends the output 2 (c.f. line 165)
207- # b14 is invalid, but the node won't know that until it tries to connect
208- # Tip still can't advance because b12 is missing
209- self.next_block(14, spend=out[5], additional_coinbase_value=1)
210- yield self.rejected()
211+ b14 = self.next_block(14, spend=out[5], additional_coinbase_value=1)
212+ self.sync_blocks([b12, b13, b14], False, 16, b'bad-cb-amount', reconnect=True)
(b12 added last) is no longer true with this change.
1201- for i in range(89, LARGE_REORG_SIZE + 89):
1202- self.next_block("alt" + str(i))
1203- test2.blocks_and_transactions.append([self.tip, False])
1204- yield test2
1205+ blocks2 = []
1206+ for i in range(1, LARGE_REORG_SIZE + 1):
1? We are building blocks on top of block with id 88, so naming them "alt89" … seems appropriate.
576@@ -598,77 +577,78 @@ def get_tests(self):
577 self.tip = b46
578 assert 46 not in self.blocks
579 self.blocks[46] = b46
580- yield self.rejected(RejectResult(16, b'bad-blk-length'))
581+ # s = ser_uint256(b46.hashMerkleRoot)
In commit 06d512f11998d9b935edf8b032eb76e491cfb7cc:
Why are you adding this comment? The ser_uint256 symbol is not imported and it will unsurprisingly be all zeros, since it was set to 0.
utACK 1f7a8662cdf2c7775942e3abc90c3745c2adaecc
GitHub hiding my review is probably a bug on their end.
218@@ -218,8 +219,12 @@ def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=N
219 time.sleep(0.05)
220
221 # Print the cause of the timeout
222- assert_greater_than(attempts, attempt)
223- assert_greater_than(timeout, time.time())
224+ predicate_source = inspect.getsourcelines(predicate)
In the last commit:
I think you meant inspect.getsource, since the line itself won’t help when the file name is unknown. And the Traceback already shows the line number and file name (as well as the predicate, in case it fits into a single line).
inspect, since it doesn’t seem to add information unless I am missing something.
I think this provides useful information when the wait_until() fails.
Without this change:
02018-03-19T18:29:56.324000Z TestFramework (ERROR): Assertion failed
1Traceback (most recent call last):
2 File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main
3 self.run_test()
4 File "./feature_block.py", line 502, in run_test
5 self.sync_blocks([b40], False, 16, b'bad-blk-sigops', reconnect=True)
6 File "./feature_block.py", line 1315, in sync_blocks
7 self.nodes[0].p2p.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_code=reject_code, reject_reason=reject_reason, request_block=request_block, timeout=timeout)
8 File "/home/ubuntu/bitcoin/test/functional/test_framework/mininode.py", line 553, in send_blocks_and_test
9 wait_until(lambda: self.reject_code_received == reject_code, lock=mininode_lock, timeout=1)
10 File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 222, in wait_until
11 assert_greater_than(timeout, time.time())
12 File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 42, in assert_greater_than
13 raise AssertionError("%s <= %s" % (str(thing1), str(thing2)))
14AssertionError: 1521484196.279766 <= 1521484196.3246927
With this change:
02018-03-19T18:27:45.353000Z TestFramework (INFO): Reject a block with too many P2SH sigops
12018-03-19T18:27:48.557000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ([' wait_until(lambda: self.reject_code_received == reject_code, lock=mininode_lock, timeout=1)\n'], 553)
22018-03-19T18:27:48.557000Z TestFramework (ERROR): Assertion failed
3Traceback (most recent call last):
4 File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main
5 self.run_test()
6 File "./feature_block.py", line 502, in run_test
7 self.sync_blocks([b40], False, 16, b'bad-blk-sigops', reconnect=True)
8 File "./feature_block.py", line 1315, in sync_blocks
9 self.nodes[0].p2p.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_code=reject_code, reject_reason=reject_reason, request_block=request_block, timeout=timeout)
10 File "/home/ubuntu/bitcoin/test/functional/test_framework/mininode.py", line 553, in send_blocks_and_test
11 wait_until(lambda: self.reject_code_received == reject_code, lock=mininode_lock, timeout=1)
12 File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 227, in wait_until
13 raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
14AssertionError: Predicate ([' wait_until(lambda: self.reject_code_received == reject_code, lock=mininode_lock, timeout=1)\n'], 553) not true after 1 seconds
Needs more (re) acks
On Tue, Mar 27, 2018, 09:21 jamesob notifications@github.com wrote:
Ready for merge?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11773#issuecomment-376522188, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv32N2D39OikZv_aEbDkLMcUm6iy6ks5tijzTgaJpZM4Qr7m3 .
1401+ self.block_heights[block.sha256] = self.block_heights[old_sha256]
1402+ del self.block_heights[old_sha256]
1403+ self.blocks[block_number] = block
1404+ return block
1405+
1406+ def reconnect_p2p(self):
218@@ -218,8 +219,12 @@ def wait_until(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=N
219 time.sleep(0.05)
220
221 # Print the cause of the timeout
222- assert_greater_than(attempts, attempt)
223- assert_greater_than(timeout, time.time())
224+ predicate_source = inspect.getsourcelines(predicate)
225+ logger.error("wait_until() failed. Predicate: {}".format(predicate_source))
wait_until is used and failure is expected, for instance in the p2p_segwit.py test where we advertise a txid from a not-NODE_WITNESS peer and ensure that a subsequence announcement from a NODE_WITNESS peer does not result in a duplicate getdata (while the first request is outstanding).