test: maxuploadtarget: check for mempool msg disconnect if limit is reached, improve existing test coverage #28996
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
theStack
force-pushed
the
202312-test-verify_maxupload_target_state
branch
from
d7c5300
to
541eeef
Compare
…t.py This ensures that the disconnect happens for the expected reason and also makes it easier to navigate between implementation and test code, i.e. both the questions "do we have test coverage for this disconnect?" (from an implementation reader's perspective) and "where is the code handling this disconnect?" (from a test reader's perspective) can be answered simply by grep-ping the corresponding debug message.
…eached Note that another reason for disconnect after receiving a MEMPOOL msg of a peer is if bloom filters are disabled on the node. This case is covered in the functional test `p2p_nobloomfilter_messages.py`.
theStack
force-pushed
the
202312-test-verify_maxupload_target_state
branch
from
541eeef
to
b58f009
Compare
|
lgtm ACK b58f009 |
| @@ -139,23 +161,32 @@ def run_test(self): | |||
| p2p_conns[2].sync_with_ping() | |||
There was a problem hiding this comment.
This is sort of unrelated, but it may be worth bringing it in given we are updating this test anyway. The line right above this mentions that the time is advanced by 24 hours and then the checks are performed again. However, the time is actually advance 48 hours: it was previously set to 48 hours in the past, and now it's set to the current time.
I guess it wouldn't hurt to also define SECONDS_PER_DAY = 60*60*24 for readability throughout the test.
|
|
||
| # Reconnect to self.nodes[0] | ||
| peer = self.nodes[0].add_p2p_connection(TestP2PConn()) | ||
|
|
||
| # Sending mempool message shouldn't disconnect peer, as total limit isn't reached yet | ||
| peer.send_and_ping(msg_mempool()) | ||
|
|
||
| #retrieve 20 blocks which should be enough to break the 1MB limit |
There was a problem hiding this comment.
nit: whitespace + caps here
PS: idk why this was pinned here, I meant L181
|
ACK b58f009 |
This PR improves existing and adds new test coverage for the
-maxuploadtargetmechanism (feature_maxuploadtarget.py) in the following ways, one commit each:verify the uploadtarget state via the
getnettotalsRPC (uploadtargetresult field):bitcoin/src/rpc/net.cpp
Lines 581 to 582 in 160d236
Note that reaching the total limit (
target_reached== True) always implies that the historical blocks serving limits is also reached (serve_historical_blocks== False), i.e. it's impossible that both flags are set to True.check for peer's specific disconnect reason (in this case,
"historical block serving limit reached, disconnect peer"):bitcoin/src/net_processing.cpp
Lines 2272 to 2280 in 160d236
add a test for a peer disconnect if the uploadtarget is reached and a
mempoolmessage is received (if bloom filters are enabled):bitcoin/src/net_processing.cpp
Lines 4755 to 4763 in 160d236
Note that another reason for disconnect after receiving a MEMPOOL msg of a peer is if bloom filters are disabled on the node. This case is already covered in the functional test
p2p_nobloomfilter_messages.py.