EDIT: we can remove the maxblocksinflight test entirely. See #10023 (comment)
~maxblocksinflight.py had a very strange structure where the~ ~fucntionality of the test was a method of the NodeConnCB object. This~ ~commit tidies up the structure of the test and brings it in line with~ ~the other functional tests.~
Needs a rebase.
rebased
Restructuring this sounds good, but now that we don't send getdata's in response to inv's anymore, perhaps this test should be rewritten to use headers for announcements instead?
You're correct. On closer inspection, this test case is actually testing nothing. The only thing it asserts on is that the node hasn't sent us too many getdata requests, but since nodes no longer sends getdata messages in response to block invs, it's just testing that 0 < 256:
./maxblocksinflight.py
2017-04-12 22:07:31.285000 TestFramework (INFO): Initializing test directory /tmp/user/1000/testk493pd32/12847
2017-04-12 22:07:31.563000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:13956
2017-04-12 22:07:33.574000 TestFramework (INFO): Round 0: success (total requests: 0)
2017-04-12 22:07:35.577000 TestFramework (INFO): Round 1: success (total requests: 0)
2017-04-12 22:07:37.582000 TestFramework (INFO): Round 2: success (total requests: 0)
2017-04-12 22:07:39.611000 TestFramework (INFO): Round 3: success (total requests: 0)
2017-04-12 22:07:39.611000 TestFramework (INFO): Stopping nodes
the headers-first sync'ing is covered pretty well by sendheaders.py. Should we just retire this test case?
Well, I'd say that if it's not too much work, we should just fix this test case to test the maxblocksinflight behavior correctly. Ie, announce 17 or more block headers, and verify we only get 16 getdata requests.
This line: https://github.com/bitcoin/bitcoin/blob/70f6f56e9dde0d0eb1a51e70c051b8fa3ba9535f/test/functional/sendheaders.py#L517 and the block below test that when there are more than 16 headers messages sent, the peer will only send get_data for 16 of them.
I think that covers the maxblocksinflight test, but let me know if you think it needs expanding.
maxblocksinflight tested that a node would not send get_data messages
for more than 16 new blocks at the same time. bitcoin core no longer
responds to block invs with get_data, since it does headers-first
sync'ing. This test was therefore testing nothing and can be removed.
the sendheaders test script tests that bitcoin will not send get_headers
for more than 16 blocks simultaneously.
21 | - self.log.debug("got getdata %s" % repr(message)) 22 | - # Log the requests 23 | - for inv in message.inv: 24 | - if inv.hash not in self.blockReqCounts: 25 | - self.blockReqCounts[inv.hash] = 0 26 | - self.blockReqCounts[inv.hash] += 1
If I'm understanding correctly, this method doesn't get called anymore since getdata isn't the response to inv?
correct
51 | - # Wait and see how many blocks were requested 52 | - time.sleep(2) 53 | - 54 | - total_requests = 0 55 | - with mininode_lock: 56 | - for key in self.blockReqCounts:
And this should be an empty dict, correct?
yes
55 | - with mininode_lock: 56 | - for key in self.blockReqCounts: 57 | - total_requests += self.blockReqCounts[key] 58 | - if self.blockReqCounts[key] > 1: 59 | - raise AssertionError("Error, test failed: block %064x requested more than once" % key) 60 | - if total_requests > MAX_REQUESTS:
And finally, total_requests is 0 every time?
exactly, and 0 <= 128 last time I checked, so this test always passes.
ACK 5f4bcf28ef8a1b775b12c9ff06367fd6656e91c3
This removes a test that doesn't do anything.