[tests] remove maxblocksinflight.py (functionality covered by other test) #10023
Conversation
|
Needs a rebase. |
jnewbery
force-pushed
the
improvemaxblocksinflight
branch
from
42ad944 to
84e101d
Compare
|
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: 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: bitcoin/test/functional/sendheaders.py Line 517 in 70f6f56 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.
jnewbery
force-pushed
the
improvemaxblocksinflight
branch
from
84e101d to
5f4bcf2
Compare
|
@sdaftuar - as discussed, we can remove maxblocksinflight entirely. |
jnewbery
changed the title
| for inv in message.inv: | ||
| if inv.hash not in self.blockReqCounts: | ||
| self.blockReqCounts[inv.hash] = 0 | ||
| self.blockReqCounts[inv.hash] += 1 |
There was a problem hiding this comment.
If I'm understanding correctly, this method doesn't get called anymore since getdata isn't the response to inv?
|
|
||
| total_requests = 0 | ||
| with mininode_lock: | ||
| for key in self.blockReqCounts: |
There was a problem hiding this comment.
And this should be an empty dict, correct?
| total_requests += self.blockReqCounts[key] | ||
| if self.blockReqCounts[key] > 1: | ||
| raise AssertionError("Error, test failed: block %064x requested more than once" % key) | ||
| if total_requests > MAX_REQUESTS: |
There was a problem hiding this comment.
And finally, total_requests is 0 every time?
There was a problem hiding this comment.
exactly, and 0 <= 128 last time I checked, so this test always passes.
…red by other test) 5f4bcf2 [tests] Remove maxblocksinflight testcase (John Newbery) Tree-SHA512: 827c8b12f4b52684a973bbfc508c5906e640572e22a96b9420a7aea86ad8d4aa4d6fcc2bb091f747e2fdd18c897e0456baff254bd5e3ceaf721bd3f09a2fd60b
…ty covered by other test) 5f4bcf2 [tests] Remove maxblocksinflight testcase (John Newbery) Tree-SHA512: 827c8b12f4b52684a973bbfc508c5906e640572e22a96b9420a7aea86ad8d4aa4d6fcc2bb091f747e2fdd18c897e0456baff254bd5e3ceaf721bd3f09a2fd60b delete maxblocksinflight.py Signed-off-by: Pasta <[email protected]>
5 participants
EDIT: we can remove the maxblocksinflight test entirely. See #10023 (comment)
maxblocksinflight.py had a very strange structure where thefucntionality of the test was a method of the NodeConnCB object. Thiscommit tidies up the structure of the test and brings it in line withthe other functional tests.