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
Unsure if you guys adhere directly to PEP8 but this comment looks like it could be removed, seems redundant.
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
Would imagine this would be best moved inline as opposed to on it's own line to keep convention with inline comments like on line 32 and 29.
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
Would assume this block comment should be moved into a docstring block like in test/functional/test_framework/mininode.py P2PStub class. Seems like the standard are great in there, I'm assuming that's down to this section being a refactor.
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
Newly added whitespace can be removed.
Just some basic review of code standards/conventions. Mostly applies to the refactored code, new code looks to have done the job from the get go.
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?)
Yep. Added to commit message.
Skipped first two commits which are from #11771 and not up to date. Reviewed remaining individual commits:
ACK
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.
Rebased
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?
Sounds good. Done both.
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)
Yes - you're right. Fixed
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)
The comment above (b12 added last) is no longer true with this change.
yes - removed
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):
Why are you changing this to 1? We are building blocks on top of block with id 88, so naming them "alt89" ... seems appropriate.
Code looks good. Just some comments need update in commit 06d512f11998d9b935edf8b032eb76e491cfb7cc it seems
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.
removed
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).
Imo we could do without 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:
2018-03-19T18:29:56.324000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main
self.run_test()
File "./feature_block.py", line 502, in run_test
self.sync_blocks([b40], False, 16, b'bad-blk-sigops', reconnect=True)
File "./feature_block.py", line 1315, in sync_blocks
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)
File "/home/ubuntu/bitcoin/test/functional/test_framework/mininode.py", line 553, in send_blocks_and_test
wait_until(lambda: self.reject_code_received == reject_code, lock=mininode_lock, timeout=1)
File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 222, in wait_until
assert_greater_than(timeout, time.time())
File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 42, in assert_greater_than
raise AssertionError("%s <= %s" % (str(thing1), str(thing2)))
AssertionError: 1521484196.279766 <= 1521484196.3246927
With this change:
2018-03-19T18:27:45.353000Z TestFramework (INFO): Reject a block with too many P2SH sigops
2018-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)
2018-03-19T18:27:48.557000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main
self.run_test()
File "./feature_block.py", line 502, in run_test
self.sync_blocks([b40], False, 16, b'bad-blk-sigops', reconnect=True)
File "./feature_block.py", line 1315, in sync_blocks
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)
File "/home/ubuntu/bitcoin/test/functional/test_framework/mininode.py", line 553, in send_blocks_and_test
wait_until(lambda: self.reject_code_received == reject_code, lock=mininode_lock, timeout=1)
File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 227, in wait_until
raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ([' wait_until(lambda: self.reject_code_received == reject_code, lock=mininode_lock, timeout=1)\n'], 553) not true after 1 seconds
Thanks for the review @MarcoFalke - I think I've addressed your comments
Thanks re-utACK 265d7c44b1aae06aee93f745a865807732218a73
Test re-ACK 265d7c44b1aae06aee93f745a865807732218a73
Ready for merge?
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 .
@ryanofsky @practicalswift - care to reACK?
utACK 265d7c44b1aae06aee93f745a865807732218a73. No code changes since last review except rebase and tweaks in feature_block.py commit (updating comments and reverting b89 loop change). There was also a commit message change mentioning runbarelyexpensive.
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):
I'm pretty sure this functionality gets repeated in several other tests. If so, we should move this somewhere more general and consolidate usages later on.
Sure, but let's save that for a future PR.
Yeah, definitely later.
4 ACKs. Ready for merge @MarcoFalke ?
utACK 265d7c44b1aae06aee93f745a865807732218a73
@laanwj Could you please merge this, since I am using my travel laptop for the next few days?
It looks like ebf053ac6135941907ecfebccc778da34b585fac is causing feature_block.py to use about 5GB of RSS memory instead of about 1GB, haven't had time to look into quite why yet. It's causing MemoryError exceptions on my 4GB vm :(
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))
This results in a spurious error message when 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).